Client fails to verify records with uppercased names about trust-dns HOT 20 CLOSED

Thanks for the testcase and the description!

I will take a look later tonight and see about fixing it.

from trust-dns.

And this is why case-insensitivity is dumb :)

I'll patch this up tonight. Thanks for doing all the research!

from trust-dns.

Nice work on the diagnosis. I knew that I had a problem here for other reasons, but you definitely found it. I'm adding your test case to the code for now, as an integration test.

I knew I had a query response issue to fix on improperly converting the case of this. I will try to push a fix tonight. Just adding some other test cases to validate cases.

from trust-dns.

Thanks and looking forward to the fix! I found this when writing my recursive resolver project. :)

from trust-dns.

Ok, I just pushed a patch to the 0.7.1_patch branch. Though, I have been having trouble with the UDP tests passing consistently, the TCP ones do regularly.

I'll have to look into why the UDP ones fail inconsistently. I will try and push a release to creates.io tomorrow for this. It also fixes an inconsistency issue with queries where the case of the responses was being changed.

from trust-dns.

btw, the integration tests are all marked with #[ignored], you can enable these with cargo test -- --ignored

from trust-dns.

Thanks for the fix! Will try it a bit later. :)

from trust-dns.

OK, have been fiddling with this over the last 2 days and I have the following observations:

1. The reason why UDP doesn't work was actually not entirely because of this issue. DNSKEY of us. exceeds 512 bytes so the server returns truncated result, and we have to revert to TCP to get validation to work.
2. TCP doesn't work on my system initially, and I found that we have to reregister the socket in event loop when getting WouldBlock (https://github.com/bluejekyll/trust-dns/blob/master/src/tcp/tcp_client_connection.rs#L138). After copying the reregister code above to that branch it works fine. Not quite sure whether this is correct though, any idea?
3. Although we have to keep case of names in RDATA, we have to convert the owner name of RRset to lowercase before calculating signature. Google's DNS server returns lowercased name as owner name so it doesn't break when testing with it, but my recursive resolver get uppercased name from the authority name server (you can see it with dig ds rollernet.us. @156.154.124.70 +dnssec) and fails to validate. I tried to replace the line of code to following snippet and it is fixed.

        let mut lower_name = Vec::<String>::new();
        for i in 0..(name.num_labels() as usize) {
            // lower_name.push(name[i].clone());
            lower_name.push(name[i].to_lowercase());
        }
        assert!(Name::with_labels(lower_name).emit_as_canonical(&mut encoder, true).is_ok());

Sidenote: Just a suggestion, maybe we should re-implement Eq, PartialEq and Hash for Name? Since in most cases we should do case-insensitive comparison between Names.

from trust-dns.

Thanks for all the hard work on this. I really appreciate it. Are you working off a release or HEAD? I'd like to start applying these changes to HEAD.

1. I'm using this default for edns sizes (standard MTU): edns.set_max_payload(1500); Do you know if it's larger than that? You can play with that setting here: https://github.com/bluejekyll/trust-dns/blob/master/src/client/client.rs#L508
2. What operating system are you using? It could be OS dependent. I know that MIO has gotten a lot of updates recently, I should probably upgrade that. Beyond that, we might need to reregister on WouldBlock, but again it might be OS dependent, but should be harmful to do it regardless. I just haven't noticed that being necessary.
3. Gah, I should have reread the spec when I fixed that. I was too aggressive on that change. Excellent.

Do you want to submit separate patches for these against HEAD? Otherwise I can look at fixing them.

Sidenote: I definitely considered this, but I got caught in an internal debate. Do we want to also compare regardless of case, or do we want people to be explicit? The reason that I went the direction I did is that I decided we at least needed both. We can invert that decision though, and make Eq + Hash case insensitive, and then offer a case_sensitive compare function.

from trust-dns.

Sure, I will send my fixes to the 3 issues as pull request a bit later. :)

Regarding sidenote: I am actually biased as my project needs case insensitive comparison and hashing everywhere. :) Anyways I just did a bit of research and found RFC 4343 which explicitly states name comparison should be case-insensitive. I agree that maybe in some cases we need to do case-sensitive comparison, but it is much rarer and it will be more convenient to implement Eq-related traits as case insensitive by default IMO.

from trust-dns.

I'll work on fixing the name issue, if you want to send the other fixes, that would be a great help.

from trust-dns.

Oh I forgot to post about the UDP issue. If I remember correctly, the offending response is more than 1700 bytes so 1500 in our setting is not enough. Although the real issue is that it seems Google's server hardcoded 512 bytes in their EDNS header, so it will never send us the correct response and we have to revert to TCP anyways.

from trust-dns.

Just submitted patch for the TCP issue. Not quite sure where is the correct place for the Name fix so I guess I'd better leave it to you. :)

from trust-dns.

Here is a test case to help checking the verification issue:

  #[test]
  fn test_dnssec_rollernet_td_tcp_mixed_case() {
    use ::tcp::TcpClientConnection;
    use ::client::Client;
    use ::rr::Name;
    use ::logger::TrustDnsLogger;
    use log::LogLevel;

    TrustDnsLogger::enable_logging(LogLevel::Debug);

    let c = Client::new(TcpClientConnection::new("8.8.8.8:53".parse().unwrap()).unwrap());
    c.secure_query(
      &Name::parse("RollErnet.Us.", None).unwrap(),
      DNSClass::IN,
      RecordType::DS,
    ).unwrap();
  }

from trust-dns.

@SAPikachu I added that test, and went through and adjusted all cases. I added the case-insensitive cmp, eq, and hash.

I got the test_dnssec_rollernet_td_tcp, this adjusts the case in the record labels in the RRSIG validation, but it doesn't adjust the signer_name. The spec seems to say that both should be lowercased, but when I do both test_dnssec_rollernet_td_tcp fails to validate.

neither change gets test_dnssec_rollernet_td_tcp_mixed_case passing.

To be clear, what is in the 0.7.3_name_patch has test_dnssec_rollernet_td_tcp passing, but not test_dnssec_rollernet_td_tcp_mixed_case

from trust-dns.

OK, I tested and found that SOA record of Us. fails to validate. And then I found RFC 6840 5.1 mentions that only names in NSEC records need to keep their case, and other common records need to be converted. CloudFlare's DNSSEC validator also references this. Hope it is correct this time..

from trust-dns.

Ok, I think this recent patch fixes it up... I pulled your TCP patch in to make the connections more reliable. Good catch on that fix! I'm going to push a new version and then merge this back to master.

The serialization code could definitely be cleaned up at this point, it's a little ugly.

from trust-dns.

Thanks! I just submitted a patch to fix Name::zone_of which should be case-insensitive too. With that it works well here. :)

from trust-dns.

Is this something you want in the 0.7.x release? Otherwise I'll just leave merged on master and then include it in the next release.

On Aug 13, 2016, at 6:23 AM, Joe Hu [email protected] wrote:

Thanks! I just submitted a patch to fix Name::zone_of which should be case-insensitive too. With that it works well here. :)

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub, or mute the thread.

from trust-dns.

It doesn't matter for me since I can use master, so you decide. :)

from trust-dns.