Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Client fails to verify records with uppercased names #27

Closed
SAPikachu opened this issue Aug 8, 2016 · 20 comments
Closed

Client fails to verify records with uppercased names #27

SAPikachu opened this issue Aug 8, 2016 · 20 comments
Labels
Milestone

Comments

@SAPikachu
Copy link
Contributor

SAPikachu commented Aug 8, 2016

Test case:

#[test]
fn test_dnssec_rollernet_td() {
    use trust_dns::udp::UdpClientConnection;
    use trust_dns::client::Client;
    let c = Client::new(UdpClientConnection::new("8.8.8.8:53".parse().unwrap()).unwrap());
    c.secure_query(
        &Name::parse("rollernet.us.", None).unwrap(),
        DNSClass::IN,
        RecordType::DS,
    ).unwrap();
}

This fails with No DNSKEY proof available error while validating the returned NSEC record, but according to http://dnsviz.net/d/rollernet.us/dnssec/ , the server returns correct RRSIG. From what I can tell, this server returns uppercased names in the record, but all names are converted to lowercase on deserialization, which breaks verification.

@bluejekyll
Copy link
Member

Thanks for the testcase and the description!

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

@bluejekyll bluejekyll added bug and removed bug labels Aug 8, 2016
@bluejekyll
Copy link
Member

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.

@SAPikachu
Copy link
Contributor Author

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

@bluejekyll
Copy link
Member

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.

@bluejekyll
Copy link
Member

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

@SAPikachu
Copy link
Contributor Author

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

@bluejekyll bluejekyll added this to the 0.7.0 milestone Aug 9, 2016
bluejekyll added a commit that referenced this issue Aug 9, 2016
fixes #27: remove implicit case conversion of labels
@SAPikachu
Copy link
Contributor Author

SAPikachu commented Aug 11, 2016

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.

@bluejekyll
Copy link
Member

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.

@bluejekyll bluejekyll reopened this Aug 11, 2016
@SAPikachu
Copy link
Contributor Author

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.

@bluejekyll
Copy link
Member

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

@SAPikachu
Copy link
Contributor Author

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.

@SAPikachu
Copy link
Contributor Author

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. :)

@SAPikachu
Copy link
Contributor Author

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();
  }

@bluejekyll
Copy link
Member

@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

@SAPikachu
Copy link
Contributor Author

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..

@bluejekyll
Copy link
Member

And this is why case-insensitivity is dumb :)

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

@bluejekyll
Copy link
Member

bluejekyll commented Aug 13, 2016

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.

@SAPikachu
Copy link
Contributor Author

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

@bluejekyll
Copy link
Member

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 notifications@github.com 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.

@SAPikachu
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants