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 #254] Unclear parsing of RRSIG and SIG records #123

New issue

Closed

opened 2026-03-07 22:22:11 +03:00 by kerem · 7 comments

kerem commented

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

Owner

Originally created by @briansmith on GitHub (Oct 24, 2017).
Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/254

The parsing code contains this:

            RecordType::RRSIG => {
                debug!("reading RRSIG");
                RData::SIG(try!(rdata::sig::read(decoder, rdata_length)))
            }
            RecordType::SIG => {
                debug!("reading SIG");
                RData::SIG(try!(rdata::sig::read(decoder, rdata_length)))
            }

It seems suspicious to me that the code for RecordType::SIG and RecordType::RRSIG would be identical. In particular, it seems strange to me that the distinction between SIG and RRSIG is lost here. I know nothing about DNS, so maybe this makes sense, but I think that at least there should be a comment here explaining why the code is OK.

Originally created by @briansmith on GitHub (Oct 24, 2017). Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/254 The parsing code contains this: ```rust RecordType::RRSIG => { debug!("reading RRSIG"); RData::SIG(try!(rdata::sig::read(decoder, rdata_length))) } RecordType::SIG => { debug!("reading SIG"); RData::SIG(try!(rdata::sig::read(decoder, rdata_length))) } ``` It seems suspicious to me that the code for `RecordType::SIG` and `RecordType::RRSIG` would be identical. In particular, it seems strange to me that the distinction between SIG and RRSIG is lost here. I know nothing about DNS, so maybe this makes sense, but I think that at least there should be a comment here explaining why the code is OK.

kerem closed this issue

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

kerem commented

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

Author

Owner

@bluejekyll commented on GitHub (Oct 24, 2017):

RRSIG records are identical to SIG records in their binary format. I like that the DNS RFC authors decided to do this. So I'm reusing the deserializer for both.

There may have been some options in SIG records that were deprecated in RRSIG, I can't think of any right now, but if we wanted to, we could pass some context into the read method, like the RecordType being read to assert any differences as necessary.

@bluejekyll commented on GitHub (Oct 24, 2017): `RRSIG` records are identical to `SIG` records in their binary format. I like that the DNS RFC authors decided to do this. So I'm reusing the deserializer for both. There may have been some options in `SIG` records that were deprecated in `RRSIG`, I can't think of any right now, but if we wanted to, we could pass some context into the read method, like the `RecordType` being read to assert any differences as necessary.

kerem commented

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

Author

Owner

@briansmith commented on GitHub (Oct 24, 2017):

OK, that makes sense. But:

Why does the parser return a RData::SIG for RRSIGs instead of returning a hypothetical RData::RRSIG? Doesn't the caller of the parser ever need to differentiate the two kinds?
I suspect that maybe it is the case that, in a given context, either RRSIGs are valid, or SIGs are valid, but it's never the case that both SIGs and RRSIGs are allowed in the same record set. Is this the case? If so, shouldn't the parser be given a flag to indicate whether SIG or RRSIG is allowed?

@briansmith commented on GitHub (Oct 24, 2017): OK, that makes sense. But: 1. Why does the parser return a `RData::SIG` for RRSIGs instead of returning a hypothetical `RData::RRSIG`? Doesn't the caller of the parser ever need to differentiate the two kinds? 2. I suspect that maybe it is the case that, in a given context, either RRSIGs are valid, or SIGs are valid, but it's never the case that both SIGs and RRSIGs are allowed in the same record set. Is this the case? If so, shouldn't the parser be given a flag to indicate whether SIG or RRSIG is allowed?

kerem commented

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

Author

Owner

@bluejekyll commented on GitHub (Oct 24, 2017):

For better or worse (you'll notice this in other RData impls), there isn't a 1-1 relationship between RData inner type variants and RecordType. At the moment, in all contexts where you use RData, you generally also have the RecordType for the Record to know what inner RData you have.

For example RData::CNAME(Name), RData::NS(Name), and RData::PTR(Name) all wrap Name, so they use the same deserializer for reading the binary for as each other: https://github.com/bluejekyll/trust-dns/blob/master/proto/src/rr/record_data.rs#L118

SIG and RRSIG are using that same pattern. So the Actual RData type is the enum variant, which is strongly typed in the Record: https://github.com/bluejekyll/trust-dns/blob/master/proto/src/rr/resource.rs#L70 . There is a 1-1 relationship between RData variants and RecordTypes.

Now if your question is, why doesn't the read method return RData instead of the inner type, the answer to that lies in other types, which reuse the readers for some of these types such-as SOA: https://github.com/bluejekyll/trust-dns/blob/master/proto/src/rr/rdata/soa.rs#L215

If you want to create an intermediary wrapper type like RData::RRSIG(RRSIG(SIG)) for clarity, I'd be open to that, but I guess I don't see a ton of value in that, unless that in some way decreases the potential of making some security mistake in terms of downgrade attacks and similar.

@bluejekyll commented on GitHub (Oct 24, 2017): For better or worse (you'll notice this in other `RData` impls), there isn't a 1-1 relationship between `RData` inner type variants and `RecordType`. At the moment, in all contexts where you use `RData`, you generally also have the `RecordType` for the `Record` to know what inner `RData` you have. For example `RData::CNAME(Name)`, `RData::NS(Name)`, and `RData::PTR(Name)` all wrap `Name`, so they use the same deserializer for reading the binary for as each other: https://github.com/bluejekyll/trust-dns/blob/master/proto/src/rr/record_data.rs#L118 `SIG` and `RRSIG` are using that same pattern. So the Actual `RData` type is the enum variant, which is strongly typed in the `Record`: https://github.com/bluejekyll/trust-dns/blob/master/proto/src/rr/resource.rs#L70 . There is a 1-1 relationship between `RData` variants and `RecordType`s. Now if your question is, why doesn't the `read` method return `RData` instead of the inner type, the answer to that lies in other types, which reuse the readers for some of these types such-as `SOA`: https://github.com/bluejekyll/trust-dns/blob/master/proto/src/rr/rdata/soa.rs#L215 If you want to create an intermediary wrapper type like `RData::RRSIG(RRSIG(SIG))` for clarity, I'd be open to that, but I guess I don't see a ton of value in that, unless that in some way decreases the potential of making some security mistake in terms of downgrade attacks and similar.

kerem commented

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

Author

Owner

@briansmith commented on GitHub (Oct 24, 2017):

Is it valid to have a recordset with both one or more RRSIGs and one or more SIGs? Or are they mutually exclusive within a given record set?

@briansmith commented on GitHub (Oct 24, 2017): Is it valid to have a recordset with both one or more RRSIGs and one or more SIGs? Or are they mutually exclusive within a given record set?

kerem commented

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

Author

Owner

@bluejekyll commented on GitHub (Oct 24, 2017):

RecordSets should only be signed with RRSIG at this point. SIG (I'm pretty sure) is deprecated and only valid for use in SIG0 signed Messages (dynamic update) operations.

@bluejekyll commented on GitHub (Oct 24, 2017): `RecordSet`s should only be signed with `RRSIG` at this point. `SIG` (I'm pretty sure) is deprecated and only valid for use in `SIG0` signed Messages (dynamic update) operations.

kerem commented

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

Author

Owner

@bluejekyll commented on GitHub (Oct 27, 2017):

I'm going to close this unless there other issues. Let reopen if you believe there's unanswered questions here.

@bluejekyll commented on GitHub (Oct 27, 2017): I'm going to close this unless there other issues. Let reopen if you believe there's unanswered questions here.

kerem commented

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

Author

Owner

@briansmith commented on GitHub (Oct 27, 2017):

Not now. I think in the future we may get rid of the tests of record_type() in favor of pattern matching on the RData::Sig constructor, in which case we may need separate RData::Sig and RData::RRSig constructors? But not now. Thanks for the explanation!

@briansmith commented on GitHub (Oct 27, 2017): Not now. I think in the future we may get rid of the tests of `record_type()` in favor of pattern matching on the `RData::Sig` constructor, in which case we may need separate `RData::Sig` and `RData::RRSig` constructors? But not now. Thanks for the explanation!

kerem referenced this issue

2026-03-16 01:56:26 +03:00

[PR #123] [MERGED] Deprecate Protocol flags in KEY records #1251