Don't do any DNSSEC record processing when DNSSEC is disabled at build-time. #267
Conversation
This will make it easier to make the "dnssec" feature to completely disable the DNSSEC processing.
| @@ -526,7 +531,8 @@ impl Message { | |||
| records.push(record) | |||
| } else { | |||
| match record.rr_type() { | |||
| RecordType::SIG => { | |||
| #[cfg(feature = "dnssec")] | |||
| RecordType::DNSSEC(DNSSECRecordType::SIG) => { | |||
There was a problem hiding this comment.
Technically speaking this is SIG0 signing, not DNSSec. i.e. used for dynamic update. Should we create a separate feature for this?
(after looking at the rest of the changes that might not be necessary, we'll just need docs which clarify that DNSSec needs to be enabled for SIG0 based dynamic update.)
| @@ -606,7 +612,8 @@ impl Message { | |||
| for fin in finals { | |||
| match fin.rr_type() { | |||
| // SIG0's are special, and come at the very end of the message | |||
| RecordType::SIG => self.add_sig0(fin), | |||
| #[cfg(feature = "dnssec")] | |||
| RecordType::DNSSEC(DNSSECRecordType::SIG) => self.add_sig0(fin), | |||
There was a problem hiding this comment.
see comment about about SIG0.
| RData::NULL(..) => RecordType::NULL, | ||
| RData::OPT(..) => RecordType::OPT, | ||
| RData::PTR(..) => RecordType::PTR, | ||
| RData::SIG(..) => RecordType::SIG, |
There was a problem hiding this comment.
Again, see comment above about SIG0
| // RP, // 17 RFC 1183 Responsible person | ||
| /// RFC 2535 (2931) Signature, to support 2137 Update | ||
| SIG, |
proto/src/rr/rr_set.rs
Outdated
| @@ -679,9 +692,11 @@ mod test { | |||
| } | |||
|
|
|||
| #[test] | |||
| #[cfg(feature = "dnssec")] // XXX: Should there be a non-DNSSEC version of this? | |||
There was a problem hiding this comment.
this only filters based on rfc 6975. so it's valid to disable for dnssec...
IIUC, dnssec-ring or dnssec-openssl is already required to get sig0 working, because those flags control which (if any) crypto implementation to use. So, I don't think this PR changes anything w.r.t. that. In the future, we may want to enable sig0 signing and/or sig0 verification separate from DNSSEC. I think that could be done pretty straightforwardly on top of this PR. |
I simply took any crypto-looking thing and moved it to the dnssec submodule and the new DNSSEC types. I can see that this PR is technically wrong. My biased opinion is that it isn't so wrong that we need to change it to add new sig0-specific features before it is merged. Perhaps what we should do is document in the |
Client needs additional work before it can avoid all the DNSSEC stuff when DNSSEC is disabled. Just keep it buiding for now.
Yes, by the end of my review I had come to this conclusion.
Agreed. |
In the discussoin of #265, @bluejekyll wrote:
Implement that.
First, centralize the majority of DNSSEC-specific code into rr::dnssec, so that most of DNSSEC code is in rr::dnssec or into SecureDnsHandle. This makes it easier for somebody reading the code to focus on either the non-DNSSEC logic or the DNSSEC logic, which is helpful for code auditing.
Next, stop building almost all of the DNSSEC-specific logic when the "dnssec" default feature isn't enabled.