starred/hickory-dns

Fork 0

mirror of https://github.com/hickory-dns/hickory-dns.git synced 2026-04-25 11:15:54 +03:00

[GH-ISSUE #118] calculation of key_tag is wrong #58

New issue

Closed

opened 2026-03-07 22:18:26 +03:00 by kerem · 9 comments

kerem commented

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

Owner

Originally created by @jannic on GitHub (Apr 21, 2017).
Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/118

In client/src/rr/dnssec/signer.rs, pub fn calculate_key_tag, the algorithm to calculate the key tag is applied to the public key (self.key.to_public_bytes()).

But according to the RFC (as quoted in the comment above the code), the algorithm should be applied to "the RDATA part of the KEY RR", which also includes the fieds flags, protocol and algorithm. (https://tools.ietf.org/html/rfc2535#section-3.1)

Unfortunately, AFAICT, Signer doesn't even contain enough information to properly fill the 'flags' field.

To verify my assumption that the current behavior is wrong, I patched calculate_key_tag to use an RDATA structure with fixed values:

    pub fn calculate_key_tag(&self) -> DnsSecResult<u16> {
        let mut ac: usize = 0;

        let rdata = RData::KEY(DNSKEY::new(false,
                                           false,
                                           false,
                                           self.algorithm(),
                                           self.key()
                                           .to_public_bytes()
                                           .expect("to_vec failed")));
        let mut bytes: Vec<u8> = Vec::with_capacity(512);
        {
            let mut e = BinEncoder::new(&mut bytes);
            rdata.emit(&mut e).unwrap();
        }

        for (i, k) in bytes.iter().enumerate() {
            ac += if i & 0x0001 == 0x0001 {
                *k as usize
            } else {
                (*k as usize) << 8
            };
        }

        ac += (ac >> 16) & 0xFFFF;
        return Ok((ac & 0xFFFF) as u16); // this is unnecessary, no?
    }

This code calculates the same key tag (AKA key id) as bind's dnssec-keygen.

Originally created by @jannic on GitHub (Apr 21, 2017). Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/118 In `client/src/rr/dnssec/signer.rs`, `pub fn calculate_key_tag`, the algorithm to calculate the key tag is applied to the public key (`self.key.to_public_bytes()`). But according to the RFC (as quoted in the comment above the code), the algorithm should be applied to "the RDATA part of the KEY RR", which also includes the fieds flags, protocol and algorithm. (https://tools.ietf.org/html/rfc2535#section-3.1) Unfortunately, AFAICT, Signer doesn't even contain enough information to properly fill the 'flags' field. To verify my assumption that the current behavior is wrong, I patched calculate_key_tag to use an RDATA structure with fixed values: ``` pub fn calculate_key_tag(&self) -> DnsSecResult<u16> { let mut ac: usize = 0; let rdata = RData::KEY(DNSKEY::new(false, false, false, self.algorithm(), self.key() .to_public_bytes() .expect("to_vec failed"))); let mut bytes: Vec<u8> = Vec::with_capacity(512); { let mut e = BinEncoder::new(&mut bytes); rdata.emit(&mut e).unwrap(); } for (i, k) in bytes.iter().enumerate() { ac += if i & 0x0001 == 0x0001 { *k as usize } else { (*k as usize) << 8 }; } ac += (ac >> 16) & 0xFFFF; return Ok((ac & 0xFFFF) as u16); // this is unnecessary, no? } ``` This code calculates the same key tag (AKA key id) as bind's `dnssec-keygen`.

kerem

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

closed this issue
added the
bug
label

kerem commented

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

Author

Owner

@bluejekyll commented on GitHub (Apr 22, 2017):

Thanks! Do you want to submit a PR?

I wasn't very confident in this code, but wasn't relying on it for much so hadn't needed to go back to it.

@bluejekyll commented on GitHub (Apr 22, 2017): Thanks! Do you want to submit a PR? I wasn't very confident in this code, but wasn't relying on it for much so hadn't needed to go back to it.

kerem commented

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

Author

Owner

@bluejekyll commented on GitHub (Apr 22, 2017):

Btw, the key_pair has a function to_dnskey() on it. We can change this to take in the booleans, right now it's assumed that all KeyPairs are zone signing keys.

@bluejekyll commented on GitHub (Apr 22, 2017): Btw, the `key_pair` has a function `to_dnskey()` on it. We can change this to take in the booleans, right now it's assumed that all KeyPairs are zone signing keys.

kerem commented

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

Author

Owner

@jannic commented on GitHub (Apr 22, 2017):

The code I posted isn't ready for an PR. It doesn't cover the flags possible in a KEY RR. The flags are different from DNSKEY flags, even though those resource records are very similar:

In the example code, I just selected DNSKEY flags which happen to have the same bit pattern as the KEY flags of the key I was using. But a clean solution whould need a way to specify KEY flags, directly.

I am trying to use trust_dns to send dynamic DNS updates to a BIND server, so I'll have a look if I can prepare a proper PR. But my time is quite limited, and this is not a high-priority task, so don't hold your breath :-)

@jannic commented on GitHub (Apr 22, 2017): The code I posted isn't ready for an PR. It doesn't cover the flags possible in a KEY RR. The flags are different from DNSKEY flags, even though those resource records are very similar: - [The KEY RR Flag Field](https://tools.ietf.org/html/rfc2535#section-3.1.2) - [The [DNSKEY] Flags Field](https://tools.ietf.org/html/rfc4034#section-2.1.1) In the example code, I just selected DNSKEY flags which happen to have the same bit pattern as the KEY flags of the key I was using. But a clean solution whould need a way to specify KEY flags, directly. I am trying to use trust_dns to send dynamic DNS updates to a BIND server, so I'll have a look if I can prepare a proper PR. But my time is quite limited, and this is not a high-priority task, so don't hold your breath :-)

kerem commented

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

Author

Owner

@bluejekyll commented on GitHub (Apr 22, 2017):

Ok. No problem. I'll take a look at getting this into the 0.10.1 release I'm working on.

DNSKEY and KEY are basically identical, which is why I implemented them with the same RData. I'll look at some options for changing the impl for the two to cleanup the flags section.

Btw, what are you using KEY for, if you don't mind my asking?

@bluejekyll commented on GitHub (Apr 22, 2017): Ok. No problem. I'll take a look at getting this into the 0.10.1 release I'm working on. DNSKEY and KEY are basically identical, which is why I implemented them with the same RData. I'll look at some options for changing the impl for the two to cleanup the flags section. Btw, what are you using KEY for, if you don't mind my asking?

kerem commented

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

Author

Owner

@jannic commented on GitHub (Apr 22, 2017):

I'm signing dynamic DNS updates. As described here: http://jpmens.net/2010/12/01/securing-dynamic-dns-updates-ddns-with-sig0/ (But with an RSASHA256 key.)

The data to be put into DNS is coming from some software written in rust, so I'd prefer to use a rust library to send out the DNS updates. Even tough just calling an external nsupdate process would work as well, I guess.

This is just a hobby project, so I'm free to use whatever technology I like, even if it's new and still has some bugs.

@jannic commented on GitHub (Apr 22, 2017): I'm signing dynamic DNS updates. As described here: http://jpmens.net/2010/12/01/securing-dynamic-dns-updates-ddns-with-sig0/ (But with an RSASHA256 key.) The data to be put into DNS is coming from some software written in rust, so I'd prefer to use a rust library to send out the DNS updates. Even tough just calling an external nsupdate process would work as well, I guess. This is just a hobby project, so I'm free to use whatever technology I like, even if it's new and still has some bugs.

kerem commented

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

Author

Owner

@bluejekyll commented on GitHub (Apr 22, 2017):

Cool. Thanks for the link, maybe I can make some compatibility tests for validating that my SIG0 impl is cross compatible, I definitely want it to be. My trust-dns server implementation doesn't rely on the key-id, and is obviously currently only testing against itself.

Thanks!

@bluejekyll commented on GitHub (Apr 22, 2017): Cool. Thanks for the link, maybe I can make some compatibility tests for validating that my SIG0 impl is cross compatible, I definitely want it to be. My trust-dns server implementation doesn't rely on the key-id, and is obviously currently only testing against itself. Thanks!

kerem commented

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

Author

Owner

@jannic commented on GitHub (Apr 22, 2017):

If you don't have a bind server at hand or want to save the time configuring it, I can send you a key which you can use to send updates to a test domain I already configured.

@jannic commented on GitHub (Apr 22, 2017): If you don't have a bind server at hand or want to save the time configuring it, I can send you a key which you can use to send updates to a test domain I already configured.

kerem commented

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

Author

Owner

@bluejekyll commented on GitHub (Apr 22, 2017):

If you want to send along the configuration, that would be a big help.

I've wanted to set this up for a while, I already have a reproducible build for bind that I use for a benchmark. I can build off that to create a repeatable build for compatibility and then run that in a travis-ci job.

@bluejekyll commented on GitHub (Apr 22, 2017): If you want to send along the configuration, that would be a big help. I've wanted to set this up for a while, I already have a reproducible build for bind that I use for a benchmark. I can build off that to create a repeatable build for compatibility and then run that in a travis-ci job.

kerem commented

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

Author

Owner

@bluejekyll commented on GitHub (May 7, 2017):

fixed in #119

@bluejekyll commented on GitHub (May 7, 2017): fixed in #119

kerem referenced this issue

2026-03-16 01:54:00 +03:00

[PR #58] [MERGED] add some tests around rc and memoizer #1210