[GH-ISSUE #27] Client fails to verify records with uppercased names

kerem commented

2026-03-07 22:18:00 +03:00

Owner

Originally created by @SAPikachu on GitHub (Aug 8, 2016).
Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/27

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.

Originally created by @SAPikachu on GitHub (Aug 8, 2016). Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/27 Test case: ``` rust #[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.

kerem

2026-03-07 22:18:00 +03:00

closed this issue
added the
bug
label

kerem commented

2026-03-07 22:18:01 +03:00

Author

Owner

@bluejekyll commented on GitHub (Aug 8, 2016):

Thanks for the testcase and the description!

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

@bluejekyll commented on GitHub (Aug 8, 2016): Thanks for the testcase and the description! I will take a look later tonight and see about fixing it.

kerem commented

2026-03-07 22:18:01 +03:00

Author

Owner

@bluejekyll commented on GitHub (Aug 9, 2016):

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.

@bluejekyll commented on GitHub (Aug 9, 2016): 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.

kerem commented

2026-03-07 22:18:01 +03:00

Author

Owner

@SAPikachu commented on GitHub (Aug 9, 2016):

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

@SAPikachu commented on GitHub (Aug 9, 2016): Thanks and looking forward to the fix! I found this when writing my recursive resolver project. :)

kerem commented

2026-03-07 22:18:01 +03:00

Author

Owner

@bluejekyll commented on GitHub (Aug 9, 2016):

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 commented on GitHub (Aug 9, 2016): 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.

kerem commented

2026-03-07 22:18:01 +03:00

Author

Owner

@bluejekyll commented on GitHub (Aug 9, 2016):

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

@bluejekyll commented on GitHub (Aug 9, 2016): btw, the integration tests are all marked with `#[ignored]`, you can enable these with `cargo test -- --ignored`

kerem commented

2026-03-07 22:18:01 +03:00

Author

Owner

@SAPikachu commented on GitHub (Aug 9, 2016):

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

@SAPikachu commented on GitHub (Aug 9, 2016): Thanks for the fix! Will try it a bit later. :)

kerem commented

2026-03-07 22:18:02 +03:00

Author

Owner

@SAPikachu commented on GitHub (Aug 11, 2016):

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

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

@SAPikachu commented on GitHub (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](https://github.com/bluejekyll/trust-dns/blob/master/src/rr/dnssec/signer.rs#L468) 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. ``` rust 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 `Name`s.

kerem commented

2026-03-07 22:18:02 +03:00

Author

Owner

@bluejekyll commented on GitHub (Aug 11, 2016):

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.

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
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.
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 commented on GitHub (Aug 11, 2016): 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.

kerem commented

2026-03-07 22:18:02 +03:00

Author

Owner

@SAPikachu commented on GitHub (Aug 12, 2016):

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.

@SAPikachu commented on GitHub (Aug 12, 2016): 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](https://tools.ietf.org/html/rfc4343#section-3) 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.

kerem commented

2026-03-07 22:18:02 +03:00

Author

Owner

@bluejekyll commented on GitHub (Aug 12, 2016):

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

@bluejekyll commented on GitHub (Aug 12, 2016): I'll work on fixing the name issue, if you want to send the other fixes, that would be a great help.

kerem commented

2026-03-07 22:18:02 +03:00

Author

Owner

@SAPikachu commented on GitHub (Aug 12, 2016):

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 commented on GitHub (Aug 12, 2016): 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.

kerem commented

2026-03-07 22:18:02 +03:00

Author

Owner

@SAPikachu commented on GitHub (Aug 12, 2016):

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 commented on GitHub (Aug 12, 2016): 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. :)

kerem commented

2026-03-07 22:18:02 +03:00

Author

Owner

@SAPikachu commented on GitHub (Aug 12, 2016):

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

@SAPikachu commented on GitHub (Aug 12, 2016): Here is a test case to help checking the verification issue: ``` rust #[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(); } ```

kerem commented

2026-03-07 22:18:02 +03:00

Author

Owner

@bluejekyll commented on GitHub (Aug 12, 2016):

@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

@bluejekyll commented on GitHub (Aug 12, 2016): @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

kerem commented

2026-03-07 22:18:02 +03:00

Author

Owner

@SAPikachu commented on GitHub (Aug 12, 2016):

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

@SAPikachu commented on GitHub (Aug 12, 2016): OK, I tested and found that SOA record of `Us.` fails to validate. And then I found [RFC 6840 5.1](https://tools.ietf.org/html/rfc6840#section-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](https://github.com/cloudflare/dns/blob/master/dnssec.go#L594) also references this. Hope it is correct this time..

kerem commented

2026-03-07 22:18:02 +03:00

Author

Owner

@bluejekyll commented on GitHub (Aug 12, 2016):

And this is why case-insensitivity is dumb :)

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

@bluejekyll commented on GitHub (Aug 12, 2016): And this is why case-insensitivity is dumb :) I'll patch this up tonight. Thanks for doing all the research!

kerem commented

2026-03-07 22:18:02 +03:00

Author

Owner

@bluejekyll commented on GitHub (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.

@bluejekyll commented on GitHub (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.

kerem commented

2026-03-07 22:18:02 +03:00

Author

Owner

@SAPikachu commented on GitHub (Aug 13, 2016):

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

@SAPikachu commented on GitHub (Aug 13, 2016): Thanks! I just submitted a patch to fix `Name::zone_of` which should be case-insensitive too. With that it works well here. :)

kerem commented

2026-03-07 22:18:03 +03:00

Author

Owner

@bluejekyll commented on GitHub (Aug 13, 2016):

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.

@bluejekyll commented on GitHub (Aug 13, 2016): 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.

kerem commented

2026-03-07 22:18:03 +03:00

Author

Owner

@SAPikachu commented on GitHub (Aug 14, 2016):

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

@SAPikachu commented on GitHub (Aug 14, 2016): It doesn't matter for me since I can use master, so you decide. :)

kerem referenced this issue

2026-03-16 01:53:11 +03:00

[PR #19] [MERGED] migrated to error-chain for all error types #1192

Rows
Columns

[GH-ISSUE #27] Client fails to verify records with uppercased names #19