Skip Menu |

This queue is for tickets about the Mail-DKIM CPAN distribution.

Report information
The Basics
Id: 80425
Status: resolved
Priority: 0/
Queue: Mail-DKIM
People
Owner: jason [...] long.name
Requestors: Mark.Martinec [...] ijs.si
Cc:
AdminCc:
Bug Information
Severity: Wishlist
Broken in: 0.39
Fixed in: (no value)

History Show all quoted text
  Fri Oct 26 15:51:38 2012 Mark.Martinec [...] ijs.si - Ticket created
Subject: Provide a way to specify options to Net::DNS::Resolver - like enabling EDNS0
Now that google.com went overboard with switching to DKIM signatures signed with a 2048-bit key, a DNS query for a public key comes close to an old DNS/UDP limit of 512 bytes. Depending on a recursive DNS server in use, the google's public key comes back either just below the 512 byte limit (when there are no additional answer sections, like with an 'unbound' resolver), or somewhat above this limit (with 'bind'), in which case the Net::DNS::Resolver on seeing a truncated response retries the query using TCP. The issue could be avoided if Net::DNS::Resolver were allowed to form an extended query (EDNS0, RFC 2671), which can tell that a client is willing to accept larger UDP packets than the ancient 512 limit. Here is an example: perl -le 'use Net::DNS; $r=Net::DNS::Resolver->new; $r->udppacketsize(4096); print $r->send("20120113._domainkey.google.com","TXT")->print' Now, not to restrict ourselves to this particular case, it would be nice to be able either to provide additional options to Net::DNS::Resolver (like timeouts, ...), or perhaps just allow an application to pass a configured Net::DNS::Resolver object to be used by Mail::DKIM.
  Tue Nov 06 09:16:41 2012 Mark.Martinec [...] ijs.si - Correspondence added
From: Mark.Martinec [...] ijs.si
Attached is a proposed patch. Besides adding a DnsResolver option, it contains two small extras: - avoids an undef warning: t/verifier.t ................. 1/104 Use of uninitialized value $query_type in lc at .../blib/lib/Mail/DKIM/PublicKey.pm line 86. Use of uninitialized value $query_type in concatenation (.) or string at .../blib/lib/Mail/DKIM/PublicKey.pm line 88. - uses Net::DNS::Resolver::send instead of Net::DNS::Resolver::query, it makes no sense to apply a default domain in case of a non-FQDN; also paves a path to using bgsend() for potential async queries.
Subject: Mail-DKIM-0.39-edns0.patch
diff -r -U2 Mail-DKIM-0.39-orig/lib/Mail/DKIM/AuthorDomainPolicy.pm Mail-DKIM-0.39-new/lib/Mail/DKIM/AuthorDomainPolicy.pm --- Mail-DKIM-0.39-orig/lib/Mail/DKIM/AuthorDomainPolicy.pm 2010-01-23 18:44:52.000000000 +0100 +++ Mail-DKIM-0.39-new/lib/Mail/DKIM/AuthorDomainPolicy.pm 2012-11-06 14:17:33.220434302 +0100 @@ -42,4 +42,5 @@ Protocol => "dns", Author => 'jsmith@example.org', + DnsResolver => Net::DNS::Resolver->new, # optional ); @@ -82,5 +83,7 @@ } - my @resp = Mail::DKIM::DNS::query($prms{Domain}, "MX"); + my @resp = Mail::DKIM::DNS::query($prms{Domain}, "MX", + DnsResolver => $prms{DnsResolver} ); + if (!@resp && $@ eq "NXDOMAIN") { diff -r -U2 Mail-DKIM-0.39-orig/lib/Mail/DKIM/DNS.pm Mail-DKIM-0.39-new/lib/Mail/DKIM/DNS.pm --- Mail-DKIM-0.39-orig/lib/Mail/DKIM/DNS.pm 2010-01-23 18:44:52.000000000 +0100 +++ Mail-DKIM-0.39-new/lib/Mail/DKIM/DNS.pm 2012-11-06 14:34:06.527433369 +0100 @@ -26,8 +26,8 @@ sub query { - my ($domain, $type) = @_; + my ($domain, $type, %prms) = @_; - my $rslv = Net::DNS::Resolver->new() - or die "can't create DNS resolver"; + my $rslv = $prms{DnsResolver} || Net::DNS::Resolver->new; + $rslv or die "can't create DNS resolver"; # @@ -49,5 +49,5 @@ eval { - $resp = $rslv->query($domain, $type); + $resp = $rslv->send($domain, $type); }; my $E = $@; @@ -100,5 +100,5 @@ my $warning; eval { - @resp = query($domain, $type); + @resp = query($domain, $type, %prms); $warning = $@; undef $@; diff -r -U2 Mail-DKIM-0.39-orig/lib/Mail/DKIM/Policy.pm Mail-DKIM-0.39-new/lib/Mail/DKIM/Policy.pm --- Mail-DKIM-0.39-orig/lib/Mail/DKIM/Policy.pm 2010-01-23 18:44:52.000000000 +0100 +++ Mail-DKIM-0.39-new/lib/Mail/DKIM/Policy.pm 2012-11-06 02:05:19.407434501 +0100 @@ -98,4 +98,5 @@ $host, "TXT", Callbacks => \%callbacks, + DnsResolver => $prms{DnsResolver}, ); return $waiter; diff -r -U2 Mail-DKIM-0.39-orig/lib/Mail/DKIM/PublicKey.pm Mail-DKIM-0.39-new/lib/Mail/DKIM/PublicKey.pm --- Mail-DKIM-0.39-orig/lib/Mail/DKIM/PublicKey.pm 2010-01-23 18:44:52.000000000 +0100 +++ Mail-DKIM-0.39-new/lib/Mail/DKIM/PublicKey.pm 2012-11-06 14:11:43.800434696 +0100 @@ -41,4 +41,5 @@ Selector => "brisbane", Domain => "example.com", + DnsResolver => Net::DNS::Resolver->new, # optional ); @@ -84,6 +85,7 @@ my ($query_type, $query_options) = split(/\//, $prms{Protocol}, 2); - if (lc($query_type) ne "dns") - { + if (!defined $query_type) { + die "empty or unrecognized query type\n"; + } elsif (lc($query_type) ne "dns") { die "unknown query type '$query_type'\n"; } @@ -123,4 +125,5 @@ $host, "TXT", Callbacks => \%callbacks, + DnsResolver => $prms{DnsResolver}, ); return $waiter; diff -r -U2 Mail-DKIM-0.39-orig/lib/Mail/DKIM/Signature.pm Mail-DKIM-0.39-new/lib/Mail/DKIM/Signature.pm --- Mail-DKIM-0.39-orig/lib/Mail/DKIM/Signature.pm 2010-11-14 22:05:44.000000000 +0100 +++ Mail-DKIM-0.39-new/lib/Mail/DKIM/Signature.pm 2012-11-06 15:08:22.032432356 +0100 @@ -49,4 +49,5 @@ bless $self, $class; + $self->{DnsResolver} = $prms{DnsResolver}; $self->version("1"); $self->algorithm($prms{'Algorithm'} || "rsa-sha1"); @@ -506,4 +507,5 @@ my $self = shift; return if exists $self->{public_key_query}; + my %prms = @_; my $on_success = sub { @@ -517,4 +519,5 @@ $self->{public_key_query} = Mail::DKIM::PublicKey->fetch_async( + DnsResolver => $prms{DnsResolver}, Protocol => $self->protocol, Selector => $self->selector, @@ -540,5 +543,5 @@ delete $self->{public}; delete $self->{public_error}; - $self->fetch_public_key; + $self->fetch_public_key(@_); } @@ -564,5 +567,5 @@ if (not exists $self->{public_key_query}) { - $self->fetch_public_key; + $self->fetch_public_key(@_); } diff -r -U2 Mail-DKIM-0.39-orig/lib/Mail/DKIM/Verifier.pm Mail-DKIM-0.39-new/lib/Mail/DKIM/Verifier.pm --- Mail-DKIM-0.39-orig/lib/Mail/DKIM/Verifier.pm 2010-11-14 22:05:44.000000000 +0100 +++ Mail-DKIM-0.39-new/lib/Mail/DKIM/Verifier.pm 2012-11-06 15:05:23.737432715 +0100 @@ -96,5 +96,5 @@ my $dkim = Mail::DKIM::Verifier->new(%options); -The only option supported at this time is: +Options supported at this time are: =over @@ -105,4 +105,18 @@ is written to the referenced string or file handle. +=item DnsResolver + +a DNS resolver object. When the option is missing or undefined, method +Net::DNS::Resolver->new() will be called to provide a resolver object. +This options allows for more flexibility in passing options to a +DNS resolver, e.g.: + + my $r; + $r = Net::DNS::Resolver->new(udp_timeout => 3, tcp_timeout => 3, retry => 2); + $r->udppacketsize(1280); # enables EDNS0, sets acceptable UDP packet size + $r->persistent_udp(1); +# $r->debug(1); + my $dkim = Mail::DKIM::Verifier->new($r); + =back @@ -159,5 +173,6 @@ my $signature = Mail::DKIM::Signature->parse($line); $self->add_signature($signature); - $signature->fetch_public_key; + $signature->fetch_public_key( + DnsResolver => $self->{DnsResolver} ); }; if ($@) @@ -179,5 +194,6 @@ my $signature = Mail::DKIM::DkSignature->parse($line); $self->add_signature($signature); - $signature->fetch_public_key; + $signature->fetch_public_key( + DnsResolver => $self->{DnsResolver} ); }; if ($@) @@ -435,5 +451,6 @@ eval { - $pkey = $signature->get_public_key; + $pkey = $signature->get_public_key( + DnsResolver => $self->{DnsResolver} ); }; if ($@) @@ -571,6 +588,7 @@ return map { Mail::DKIM::AuthorDomainPolicy->fetch( - Protocol => "dns", Author => $_, + Protocol => "dns", + DnsResolver => $self->{DnsResolver}, ) } @authors; @@ -604,6 +622,7 @@ # fetch the policy return Mail::DKIM::DkimPolicy->fetch( - Protocol => "dns", Author => $author, + Protocol => "dns", + DnsResolver => $self->{DnsResolver}, ); } @@ -647,4 +666,5 @@ Author => $author, Sender => $sender, + DnsResolver => $self->{DnsResolver}, ); }
  Tue Nov 06 13:33:27 2012 Mark.Martinec [...] ijs.si - Correspondence added
From: Mark.Martinec [...] ijs.si
Show quoted text
> - uses Net::DNS::Resolver::send instead of Net::DNS::Resolver::query, > it makes no sense to apply a default domain in case of a non-FQDN; > also paves a path to using bgsend() for potential async queries.
Btw, the use of send() instead of query() is also compatible with Mail::SPF::Server, along with an equivalent passing of a resolver object from a caller: $ man Mail::SPF::Server [...] Mail::SPF::Server is a server class for processing SPF requests. Each server instance can be configured with specific processing parameters. Also, the default Net::DNS::Resolver DNS resolver used for making DNS look-ups can be overridden with a custom resolver object. [...] dns_resolver: [...] An optional DNS resolver object. If none is specified, a new Net::DNS::Resolver object is used. The resolver object may be of a different class, but it must provide an interface similar to Net::DNS::Resolver -- at least the "send" and "errorstring" methods must be supported, and the "send" method must return either an object of class Net::DNS::Packet, or, in the case of an error, undef.
  Wed Nov 28 10:12:34 2012 jason [...] long.name - Taken
  Wed Nov 28 12:58:40 2012 jason [...] long.name - Correspondence added
Regarding this, On Tue Nov 06 09:16:41 2012, Mark.Martinec@ijs.si wrote: Show quoted text
> > - uses Net::DNS::Resolver::send instead of Net::DNS::Resolver::query, > it makes no sense to apply a default domain in case of a non-FQDN; > also paves a path to using bgsend() for potential async queries.
The documentation for Net::DNS::Resolver::query() says the search domain is NOT applied. From what I can tell, the difference between query() and send() is not whether the search list is applied but rather how failures are handled. Do you concur? I think I'm ok with replacing query() with send(), as long as I know the ramifications. Jason
  Wed Nov 28 12:58:41 2012 The RT System itself - Status changed from 'new' to 'open'
  Wed Nov 28 14:25:10 2012 Mark.Martinec [...] ijs.si - Correspondence added
From: Mark.Martinec [...] ijs.si
Show quoted text
> The documentation for Net::DNS::Resolver::query() says the search > domain is NOT applied. From what I can tell, the difference between > query() and send() is not whether the search list is applied but > rather how failures are handled. Do you concur? I think I'm ok with > replacing query() with send(), as long as I know the ramifications.
$ man Net::DNS::Resolver : query Performs a DNS query for the given name; the search list is not applied. If the name doesn't contain any dots and defnames is true then the default domain will be appended. send Performs a DNS query for the given name. Neither the searchlist nor the default domain will be appended. So as long a defnames is not set to true, query() and send() are pretty much the same. Indeed the query() additionally checks for presence of the 'answer' section in a response, although I believe the Mail::DKIM would also notice absence of the 'answer' section. This is the code in Net/DNS/Resolver/Base.pm : sub query { my $self = shift; my $name = shift || '.'; # resolve name containing no dots or colons by appending domain my @suffix = ($self->{domain} || ()) if $name !~ m/[:.]/ and $self->{defnames}; my $fqname = join '.', $name, @suffix; print ';; query(', join(', ', $fqname, @_), ")\n" if $self->{debug}; my $packet = $self->send($fqname, @_) || return undef; return $packet if $packet->header->ancount; # answer found return undef; } I'm suggesting the use of send() instead of query() mostly for compatibility with Mail::SPF::Server.
  Wed Nov 28 15:04:01 2012 jason [...] long.name - Correspondence added
Yup, you're right about the default domain. I'm replacing query() with send() as you suggested. For specifying options to the resolver, I looked at your patch but I am considering a simpler change than that... adding a global configuration variable to control the resolver. So basically, the usage pattern would be the following. At the top of the script that uses Mail::DKIM, you would have my $res = Net::DNS::Resolver->new(); $res->udppacketsize(2048); $Mail::DKIM::DNS::RESOLVER = $res; and then all the Mail::DKIM functions will use the specified resolver object. It's not as pure a solution, perhaps, but in my mind this is more of a configuration thing than a per-instance setting. Any objections? Jason
  Wed Nov 28 15:20:34 2012 Mark.Martinec [...] ijs.si - Correspondence added
Subject: Re: [rt.cpan.org #80425] Provide a way to specify options to Net::DNS::Resolver - like enabling EDNS0
Date: Wed, 28 Nov 2012 21:20:20 +0100
To: "Jason Long via RT" <bug-Mail-DKIM [...] rt.cpan.org>
From: Mark Martinec <Mark.Martinec [...] ijs.si>
Jason, Thanks for looking into this. Show quoted text
> For specifying options to the resolver, I looked at your patch but I am > considering a simpler change than that... adding a global configuration > variable to control the resolver. > > So basically, the usage pattern would be the following. At the top of > the script that uses Mail::DKIM, you would have > > my $res = Net::DNS::Resolver->new(); > $res->udppacketsize(2048); > > $Mail::DKIM::DNS::RESOLVER = $res; > > and then all the Mail::DKIM functions will use the specified resolver > object. It's not as pure a solution, perhaps, but in my mind this is > more of a configuration thing than a per-instance setting. > > Any objections?
It gives me an ugly warning: Name "Mail::DKIM::DNS::RESOLVER" used only once: possible typo at ... and one cannot have two instances of a Mail::DKIM::Verifier, each with its own resolver - but I could live with that. Mark
  Mon Feb 04 14:27:11 2013 jason [...] long.name - Correspondence added
Ok, I finally got the new version ready for testing; it's now available as version 0.39_6 (developer release) on CPAN. The new version contains a supported way of overriding the DNS resolver. Yeah, if you're using an older version of the Mail::DKIM module, you'll get a warning about a variable name used only once. I suppose you'll need a 'no warnings' directive to avoid that. Version 0.39_6 also changes the DEFAULT resolver to use a large udp packet size (2048 bytes), so maybe you won't even need to override the resolver at all. I intend to release this as version 0.40 later this week, if I do not discover any problems with the new version. Hopefully I won't once again get distracted by some other project for several weeks :). Jason
  Thu Feb 07 15:38:06 2013 jason [...] long.name - Correspondence added
As noted in another ticket, the change to use EDNS0 by default has been reverted. In version 0.40, which has just been sent on its way to CPAN, there is now a documented subroutine available for enabling EDNS0 if desired. Do something like if (Mail::DKIM::Verifier->VERSION >= 0.40) { Mail::DKIM::DNS::enable_EDNS0(); } I'm going to mark this ticket resolved.
  Thu Feb 07 15:38:07 2013 jason [...] long.name - Status changed from 'open' to 'resolved'