alexdalitz / dnsruby

Dnsruby is a feature-complete DNS(SEC) client for Ruby, as used by many of the world's largest DNS registries and the OpenDNSSEC project
Other
197 stars 77 forks source link

prereq on absent(name, 'CNAME') doesn't work with update #102

Closed seandilda closed 8 years ago

seandilda commented 8 years ago

When doing an update, if I set a .absent(name, 'CNAME'), then I get an error back. The initial error is a tsignotsigned error. If I switch to using #query_raw, then I see the error is a formerror. If I try an A record or TXT record, there's no error.

This is against bind 9.9.2

seandilda commented 8 years ago

I did a wireshark comparison of the packet from Dnsruby and the packet from nsupdate. It appears that Dnsruby is giving the prereq an rdata of '.' which gives a length of 1. RFC 2136 that FORMERR is the proper response to this request.

alexdalitz commented 8 years ago

Thanks very much for the detailed problem report! I will look at this and try to get a fix in place by the end of the weekend.

Thanks,

Alex.

Sent from my iPad

On 3 May 2016, at 20:10, Sean Dilda notifications@github.com wrote:

I did a wireshark comparison of the packet from Dnsruby and the packet from nsupdate. It appears that Dnsruby is giving the prereq an rdata of '.' which gives a length of 1. RFC 2136 that FORMERR is the proper response to this request.

— You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub

alexdalitz commented 8 years ago

Hi -

I've tried to reproduce your error with the following code -

def test_absent_cname update = Update.new() rr = update.absent("target_name", "CNAME") assert(rr, 'nxdomain() returned RR'); assert_equal(rr.name.to_s, "target_name", 'nxdomain - right name'); assert_equal(rr.ttl, 0, 'nxdomain - right ttl');
assert_equal(rr.klass.string, 'NONE', 'nxdomain - right class'); assert_equal(rr.type.string, 'CNAME', 'nxdomain - right type'); assert(is_empty(rr.rdata), 'nxdomain - data empty');
end

this code succeeds. Am I right in understanding that your issue relates to the rdata holding '.' with an rdlength of 1? The above code seems to show that this is not the case.

Could you please provide a test case to demonstrate the failure?

Thanks!

Alex.

seandilda commented 8 years ago

From a quick (and possibly faulty) trace, it looks like the encoding of rdata for DomainName ends up calling rr.domainname.canonical, which is not empty if you add that check to the above code.

I'm pretty busy this weekend, but if I find time, I'll try to do a more thorough trace.

alexdalitz commented 8 years ago

Hi -

Thanks for that.

I've made a change in the put_name method so that it doesn't insist on encoding zero length names as "." - could you please test this branch and let me know if it helps your issue?

https://github.com/alexdalitz/dnsruby/compare/update-absent?expand=1

Thanks,

Alex.

seandilda commented 8 years ago

I just tested it and it still has the problem. However, it gave me a great starting point and I think I have a fix that works.

It looks like the if should go around '@data << "\0"' instead of the loop. When I made that change, updates started working as expected. Would you rather fix yourself or have me submit a pull request?

alexdalitz commented 8 years ago

Thanks for the response. I'll try to have a look this evening and will add your fix if everything checks out.

Thanks for your help!

alexdalitz commented 8 years ago

Sadly that suggestion breaks other code, so I've pushed another potential fix to this branch, which is a little more specific.

Could you please test again?

Thanks!

seandilda commented 8 years ago

I was afraid that would be the case :(

I just checked the new commit and it works great for me. I'm now getting the expected Dnsruby::YXRRSet error instead of the FormErr. And the same code adds records just fine when there is no CNAME present.

seandilda commented 8 years ago

Thanks for fixing this problem!

alexdalitz commented 8 years ago

Hey thanks for getting back so quickly. Glad to hear the problem is resolved, and thanks very much for your help fixing it. If you need this in a new release soon, then do let me know. There are a few other issues I need to close down quite soon...

alexdalitz commented 8 years ago

Oh - that’s now merged to master…

On 9 May 2016, at 19:21, Sean Dilda notifications@github.com wrote:

Thanks for fixing this problem!

— You are receiving this because you commented. Reply to this email directly or view it on GitHub https://github.com/alexdalitz/dnsruby/issues/102#issuecomment-217946205