Sorry, I missed the mailing list replies on this. Feel free to close.

Created attachment 5115 [details]
a fix to sa-update, and some other DNS-related tidbits

trunk:

r1418317 | mmartinec | 2012-12-07 15:04:22 +0100 (Fri, 07 Dec 2012)

- Net::DNS 0.69 $rr->rdatastr no longer quotes returned TXT
  ([rt.cpan.org #81760])
- some other DNS-related cosmetics
- more RR types in AskDNS.pm



Copied from a mailing list:

 
Mark Martinec wrote:
> Net::DNS has broken sa-update -
> https://rt.cpan.org/Ticket/Display.html?id=81760

Interesting. The Net::DNS 0.68 docs claim:

  man Net::DNS::RR::TXT

   char_str_list
        print "Individual <character-string> list: \n\t",
              join("\n\t", $rr->char_str_list());
       Returns a list of the individual <character-string> elements, as
       unquoted strings.  Used by TXT->rdatastr and TXT->rr_rdata.

       NB: rdatastr will return quoted strings.

But the inserted quotation marks seem silly indeed.

> This patch to sa-update would work around Net::DNS 0.70. $1 should never be
> used anyways unless you're sure the regex matched. It's just asking for
> trouble.

Thanks, fixed now in trunk, r1418317.
Used the same approach/code for sa-update, as was properly used
in SpamAssassin/Dns.pm.

> Also local $1 makes me nervous. It could potentially behave
> different on different versions of perl depending on if bugs existed with
> local or $1.

It's the other way around: missing local($1) could behave differently
depending on a version of perl. Having global $n variables localized
before use makes them undefined and taint-free, and avoids potentially
polluting the same variable used by a caller when invoking some subroutine
or hitting an exception handler. This applies to any version of perl,
it makes a good practice.

Kevin A. McGrail wrote:
Thanks Mark for reviewing this.  And Todd, good catch.
I agree with  Mark's patch re: local but otherwise a good fix.

Hi,

I'm responsible for the new Net::DNS release.

I believe you can also work around the issue by putting a space in the version TXT resource record and an @ in the mirror TXT resource record, like this:

2.3.3.updates.spamassassin.org.    TXT  "1418219 "
mirrors.updates.spamassassin.org.  TXT  "http://anonymous@spamassassin.apache.org/updates/MIRRORED.BY"

The space and the @ will force a double quoted return value from rdatastr even with the new Net::DNS.
I have tested this with sa-update version svn917659 running on Perl version 5.12.4 on FreeBSD by putting those records in updates.spamassassin.net-dns.org and changing sa-update with:

  my @channels = ( 'updates.spamassassin.net-dns.org' );

I suppose you can judge best for yourself if this would work on other versions of sa-update and if it is a "proper" workaround.

I would like to know if your going to release with the fix (or apply the trick just mentioned) shortly, to asses whether we need to revert the behaviour of TXT RR's or not...

Best regards,

Willem Toorop

PS. I have the intention to e-mail about upcoming releases to developers that use Net::DNS in their packages in the future, hopefully with the effect that such nasty issues come up before release.

PS2. I have tested new Net::DNS with spamassassin before release, but did not notice this issue because sa-update was quiet as normal. Shouldn't sa-update at least give a warning when it is unable to determine whether it needs to update or not?

If Net::DNS will not be putting the quotes back, It seems like this patch would do well to go into a 3.3.3 branch with only this fix to be released to CPAN. Is this in the works? Potentially all 3.3.2 users who have updated Net::DNS are now broken. Also any new installs are potentially broken.

Created attachment 5117 [details]
fix my previous patch

Bug 6872, fix breakage introduced by my previous change - DNS replies
with no answers would wait till their timeout
  Sending lib/Mail/SpamAssassin/DnsResolver.pm
Committed revision 1420890.

Before returning to the original problem report, first a quick explanation
on what my first patch broke. The symptoms were noticed by very long nightly
runs of masschecks caused by unnecessarily long scan times. If there was
at least one unsuccessful DNS reply, it was ignored and the code then
waited until RBL timeout (default 15 seconds, often set to 5 seconds).

The culprit is the following section in the old code in DnsResolver.pm
(before any changes):

    if (defined $packet &&
        defined $packet->header &&
        defined $packet->question &&
        defined $packet->answer)
    {  ... deal with a received DNS packet ... }


The problem here is that $packet->question and $packet->answer
return a list (possibly empty). The defined() of such list always
returns true, so the above test just wasted unnecessary cycles
to test the question and answer sections, but then ignored the
result.

My first patch tried to 'fix' this by doing what seemed to be
the intention, i.e. ignore a packet with an empty answer section.
But that is wrong, such a response still needs to hit its
callback routine and be removed from the list of waited-upon
queries. As it happens, the original (broken) code managed
to be doing the right thing, while the 'fixed' code broke it.

Now, the today's change hopefully does *the*right*thing*,
while also providing a more informative debugging.

Still not quite right, this should be better:

Sending lib/Mail/SpamAssassin/DnsResolver.pm
Committed revision 1420906.

(In reply to comment #3, by Willem Toorop)

Now back to the original problem.

> I believe you can also work around the issue by putting a space in the
> version TXT resource record and an @ in the mirror TXT resource record,
> like this:
> 
> 2.3.3.updates.spamassassin.org.    TXT  "1418219 "
> mirrors.updates.spamassassin.org.  TXT 
> "http://anonymous@spamassassin.apache.org/updates/MIRRORED.BY"
> 
> The space and the @ will force a double quoted return value from rdatastr
> even with the new Net::DNS.
> I have tested this with sa-update version svn917659 running on Perl version
> 5.12.4 on FreeBSD ...

This looks like a good and innocent-enough workaround for
a bug in SpamAssassin caused by the change in Net::DNS.

I tested it with SA 3.3 (uses wget) and 3.4(trunk) (uses curl or
wget or fetch or wget), and it seems to do the job, while allowing
Net::DNS folks to proceed with their wish of retaining this new behaviour.

(there is a lot to be said about the incorrect assumption throughout
most of the SpamAssassin code that the Net::DNS API is 8-bit clean,
conflicting with their (silly, in my view) concept that the API uses
the same encoding as zone ascii text files - but I'll leave that to
another posting)


> I would like to know if your going to release with the fix (or apply the
> trick just mentioned) shortly, to asses whether we need to revert the
> behaviour of TXT RR's or not...

Considering the time it takes/took some distributions and some installations
to pick up a new version of SpamAssassin in the past, I think it is futile
to think of going into trouble of releasing a 3.3.3 just to fix this problem,
knowing that it will only reach a very limited audience.

I think adopting Willem's suggestion is a good (yet hackish) short term
solution until the 3.4 can be released to a general public (noting that
several sites are already using 3.4(trunk) in production).


> PS. I have the intention to e-mail about upcoming releases to developers
> that use Net::DNS in their packages in the future, hopefully with the effect
> that such nasty issues come up before release.

Thanks, good idea.

> PS2. I have tested new Net::DNS with spamassassin before release, but did
> not notice this issue because sa-update was quiet as normal.

Neither did I  :)

> Shouldn't sa-update at least give a warning when it is unable to determine
> whether it needs to update or not?

It does, as far as I can tell:

spamassassin-3.3(branch):

# sa-update     
http: GET 3.3 request failed: 400 URL must be absolute: 400 URL must be absolute 
error: no mirror data available for channel updates.spamassassin.org
channel: MIRRORED.BY contents were missing, channel failed

(exit status 4)


spamassassin(trunk):

#  sa-update     
channel: no 'mirrors.updates.spamassassin.org' record found, channel failed

(exit status 4)

and with "1418219 " but without "anonymous@":

# sa-update     
error: no mirror data available for channel updates.spamassassin.org
channel: MIRRORED.BY file location was not in DNS, channel failed

(exit status 4)

(In reply to comment #8 by Mark Martinec)
> I think adopting Willem's suggestion is a good (yet hackish) short term
> solution until the 3.4 can be released to a general public (noting that
> several sites are already using 3.4(trunk) in production).

Thanks! I hope you will apply it. Let me know if you will so I can release 0.71 with the other issues resolved (DKIM etc).

For smoothest transition I think it would be best to first update mirrors.updates.spamassassin.org, and then wait for one hour (the TTL of the records) before updating <version>.updates.spamassassin.org. That way you are sure that if an sa-update gets a new version number from DNS, it can also get the url for fetching MIRRORED.BY.

If you alter them at the same time you have a small risk that a resolver still has the old value for mirrors.updates.spamassassin.org in its cache whilst the <version>.updates.spamassassin.org is updated causing sa-update to fail with:

  error: no mirror data available for channel updates.spamassassin.org
  channel: MIRRORED.BY file location was not in DNS, channel failed

Best regards, Willem Toorop

Created attachment 5118 [details]
A minimalistic patch to sa-update for SA 3.3

*** Bug 6878 has been marked as a duplicate of this bug. ***

Meanwhile the Net::DNS 0.71 has been released, temporarily reverting the
change that broke sa-update, until we get our 3.4.0 out - thank you Willem
and the rest of the NLnet Lab crew!

This is from Net::DNS 0.71 release notes:
  **** 0.71 Dec 15, 2012
  Temporary workaround rt.cpan.org #81760
    The rdatastr method for TXT RRs will return unconditionally 
    quoted rdata fields to work around an issue with updating 
    SpamAssassin rules.  This workaround will be reverted after
    release of a version of SpamAssassin which resolves the issue.

After some offline discussion with Willem (me complaining about
the difficulties when a caller wants to access data strings from
TXT records with multiple RDATA <character-string> fields, while
also avoiding the Net::DNS habit of using a zone-file format
encodings in the API, only to have the caller go into trouble
of undoing the unnecessary encoding) - Willem suggested the
following:

> > So what can I do? Check a Net::DNS version and use char_str_list()
> > with older Net::DNS, and txtdata() with newer???
>
> I promise to discuss this thoroughly with Dick and Olaf and report back
> with a statement on which you can build and depend.
>
> In the mean time, I encourage you to use txtdata for getting the values
> of <version>.updates.spamassassin.org and
> mirros.updates.spamassassin.org. As those records have only a single
> rdata field, txtdata would return the same value since Net::DNS 0.34.

This sounds like a good advise, especially since Net::DNS 0.34 already
happen to be the lowest supported version of Net::DNS in SpamAssassin.

So I'm folding in a couple of changes to implement it. Will attach
the diff in the next posting. The following new comments in the code
and a few samples of the change should make the reasoning clearer,
I hope:

-    $log = $answer->rdatastr;
-    $log =~ s/^"(.*)"\z/$1/s;
+    # txtdata returns a non- zone-file-format encoded result, unlike rdatastr;
+    # avoid space-separated RDATA <character-string> fields if possible,
+    # txtdata provides a list of strings in a list context since Net::DNS 0.69
+    $log = join('',$answer->txtdata);


+  # NOTE:  $rr->rdatastr returns the result encoded in a DNS zone file
+  # format, i.e. enclosed in double quotes if a result contains whitespace
+  # (or other funny characters), and may use \DDD encoding or \X quoting as
+  # per RFC 1035.  Using $rr->txtdata instead avoids this unnecessary encoding
+  # step and a need for decoding by a caller, returning an unmodified string.
+  # Caveat: in case of multiple RDATA <character-string> fields contained
+  # in a resource record (TXT, SPF, HINFO), starting with Net::DNS 0.69
+  # the $rr->txtdata in a list context returns these strings as a list.
+  # The $rr->txtdata in a scalar context always returns a single string
+  # with <character-string> fields joined by a single space character as
+  # a separator.  The $rr->txtdata in Net::DNS 0.68 and older returned
+  # such joined space-separated string even in a list context.
+  #
+  # From Net::DNS maintainers (Willem Toorop, NLnet Labs):
+  #   I encourage you to use txtdata for getting the values of
+  # <version>.updates.spamassassin.org and mirros.updates.spamassassin.org.
+  # As those records have only a single rdata field, txtdata would return
+  # the same value since Net::DNS 0.34.

-      my $text = $rr->rdatastr;
-      if (defined $text && $text ne '') {
-        local($1);
-        $text =~ s/^"(.*)"\z/$1/s;
-        push(@result,$text);
-      }
+      # scalar context!
+      my $text = $rr->UNIVERSAL::can('txtdata') ? $rr->txtdata : $rr->rdatastr;
+      push(@result,$text)  if defined $text && $text ne '';


Trunk:
  Bug 6872 - replace $rr->rdatastr with $rr->txtdata where appropriate
Sending lib/Mail/SpamAssassin/Dns.pm
Sending lib/Mail/SpamAssassin/DnsResolver.pm
Sending lib/Mail/SpamAssassin/Plugin/AskDNS.pm
Sending lib/Mail/SpamAssassin/Plugin/URIDNSBL.pm
Sending sa-update.raw
Committed revision 1425068.

Created attachment 5124 [details]
the change in r1425068, replaces $rr->rdatastr with $rr->txtdata where appropriate

*** Bug 6882 has been marked as a duplicate of this bug. ***

Good catch, and sorry about the duplicate bug report.

On an informative note, which I will send to the perl ports manager as well;
the current FreeBSD userbase is in for a treat...

========================================================================(215)[root@firefly:~](1)>head /usr/ports/dns/p5-Net-DNS/Makefile
# Created by: James FitzGibbon <jfitz@FreeBSD.org>
# $FreeBSD: ports/dns/p5-Net-DNS/Makefile,v 1.90 2012/12/17 05:50:12 svnexp Exp $

PORTNAME=       Net-DNS
PORTVERSION=    0.70
CATEGORIES=     dns net perl5 ipv6
MASTER_SITES=   CPAN
PKGNAMEPREFIX=  p5-

MAINTAINER=     perl@FreeBSD.org

========================================================================

I ran into this issue while doing a fresh install.

> Good catch, and sorry about the duplicate bug report.
> 
> On an informative note, which I will send to the perl ports manager as well;
> the current FreeBSD userbase is in for a treat...
> 
> [root@firefly:~](1)>head /usr/ports/dns/p5-Net-DNS/Makefile
> # Created by: James FitzGibbon <jfitz@FreeBSD.org>
> # $FreeBSD: ports/dns/p5-Net-DNS/Makefile,v 1.90 2012/12/17 05:50:12 svnexp
> Exp $
> PORTNAME=       Net-DNS
> PORTVERSION=    0.70


The port is at 0.71 since 2012-12-18, you have a stale copy:

# Created by: James FitzGibbon <jfitz@FreeBSD.org>
# $FreeBSD: ports/dns/p5-Net-DNS/Makefile,v 1.91 2012/12/18 09:25:02 svnexp Exp $
PORTNAME=       Net-DNS
PORTVERSION=    0.71

Apologies, my support ticket should have been filed as a PEBKAC issue.

*enables a certain update from crontab which was commented out for safety reasons*


I thank you humbly for your time and effort.
I bid you a good night!

I am closing this ticket because we are not going to backport to 3.3 and trunk can handle the new versions which should give us enough time to handle the change with 3.4.0's release.

I (still) have the same bug, versions:

SpamAssassin version 3.4.0
  running on Perl version 5.10.1
CentOS release 6.6 (Final)

I got this error with /usr/share/spamassassin/sa-update.cron (result took a very long time!):
http: GET 3.3 request failed: 400 URL must be absolute: 400 URL must be absolute 
error: no mirror data available for channel sought.rules.yerp.org
channel: MIRRORED.BY contents were missing, channel failed
http: GET 3.3 request failed: 400 URL must be absolute: 400 URL must be absolute 
error: no mirror data available for channel updates.spamassassin.org
channel: MIRRORED.BY contents were missing, channel failed
18-Mar-2015 05:15:06: SpamAssassin: Update available, but download or extract failed

It occurred with Net::DNS 0.72 (version not 100% sure) and 0.82 (version=sure)
I eventually got it working again downgrading to 0.65 (used from an other server I manage that I had not yet upgraded the Perl-libs of)

(In reply to E Toering from comment #19)
> I (still) have the same bug, versions:
> 
> SpamAssassin version 3.4.0
>   running on Perl version 5.10.1
> CentOS release 6.6 (Final)
> 
> I got this error with /usr/share/spamassassin/sa-update.cron (result took a
> very long time!):
> http: GET 3.3 request failed: 400 URL must be absolute: 400 URL must be
> absolute 
> error: no mirror data available for channel sought.rules.yerp.org
> channel: MIRRORED.BY contents were missing, channel failed
> http: GET 3.3 request failed: 400 URL must be absolute: 400 URL must be
> absolute 
> error: no mirror data available for channel updates.spamassassin.org
> channel: MIRRORED.BY contents were missing, channel failed
> 18-Mar-2015 05:15:06: SpamAssassin: Update available, but download or
> extract failed
> 
> It occurred with Net::DNS 0.72 (version not 100% sure) and 0.82
> (version=sure)
> I eventually got it working again downgrading to 0.65 (used from an other
> server I manage that I had not yet upgraded the Perl-libs of)

sought rules are deprecated - not updated - dead
you can remove that from your script

I'm using Centos 6.6
perl-Net-DNS-0.71-1.el6.rfx.x86_64

cannot reproduce.