NSEC3 and multiple key signing support. #416
Conversation
bal-e
force-pushed
the
dnssec-key
branch
2 times, most recently
from
a79aac6 to
0f54a8d
Compare
src/sign/ring.rs
Outdated
| pub fn nsec3_hash<N, SaltOcts, HashOcts>( | ||
| owner: N, | ||
| algorithm: Nsec3HashAlg, | ||
| iterations: u16, | ||
| salt: &Nsec3Salt<SaltOcts>, | ||
| ) -> Result<OwnerHash<HashOcts>, Nsec3HashError> |
There was a problem hiding this comment.
Why not just take the owner name and an Nsec3param?
There was a problem hiding this comment.
nsec3_default_hash can then be replaced by calling this function with default() for the second argument.
There was a problem hiding this comment.
Why not just take the owner name and an
Nsec3param?
I had that but took it out. There was a reason. I'll see if I can remember why.
There was a problem hiding this comment.
Ah I know why. The nsec3s() function takes an Nsec3Param struct which it uses to create the NSEC3PARAM RR at the apex of the zone. For the NSEC3PARAM RR RFC 5155 says that the opt-out flag in the flags field MUST be zero, so for RFC compliance the Nsec3Param struct passed to nsec3s() should have the opt-out flag set to zero.
Honestly I think this is a bit of a foot-gun and perhaps best not to pass an Nsec3Param to nsec3s() but instead only the other fields (algorithm, iterations, salt), however MAYBE in future it will be legal to set some of the other flag bits in the flags field and a user would want to have those set in the created NSEC3PARAM RR... so for that reason nsec3s() currently takes an Nsec3Param as input.
When generating NSEC3 RRs, and when opt-out is enabled, the flags value in the given Nsec3Param cannot be used as-is because the opt-out flag must be set to 1 (but NOT in the NSEC3PARAM RR), and rather than copy the given Nsec3Param or modify it and then pass it to nsec3_hash() I felt it was better to just pass only the values actually needed for hashing in, as NSEC3 hashing doesn't need the flags field at all, and also that way users don't have to think about what value to set the unused Nsec3Param::flags field to when invoking nsec3_hash() directly (as dnst nsec3-hash does).
src/sign/ring.rs
Outdated
| let owner_hash = OwnerHash::from_octets(hash) | ||
| .map_err(|_| Nsec3HashError::OwnerHashError)?; |
There was a problem hiding this comment.
I don't think this should return an OwnerHashError. There are two failure cases for from_octets: if the hash is more than 255 bytes (impossible since NSEC3 doesn't support any such digest algorithms) or if memory could not be allocated (in which case we should return AppendError).
There was a problem hiding this comment.
It's not clear to me from the docs on OwnerHashError that it only fails relating to length, it just says "The hashing process produced an invalid owner hash" and I have no way of knowing when that error might occur or why.
src/sign/records.rs
Outdated
| nsec3_recs: Vec<Record<N, Nsec3<Octets>>>, | ||
| nsec3param_rec: Record<N, Nsec3param<Octets>>, |
There was a problem hiding this comment.
Just name these records and params?
There was a problem hiding this comment.
I was expecting to revisit the design of these APIs. If it remains as such then yes I agree.
|
@bal-e: I realize that I moved the |
|
@ximon18: yeah, I think this should be moved under |
Done. |
src/sign/records.rs
Outdated
| // the apex and the original owner name." | ||
| let distance_to_root = name.owner().iter_labels().count(); | ||
| let distance_to_apex = distance_to_root - apex_label_count; | ||
| if distance_to_apex > 1 { |
There was a problem hiding this comment.
I don't think the if statement is necessary, the for loop will run for zero iterations if this condition is not true.
There was a problem hiding this comment.
True, but the if statement matches nicely with the RFC text and makes it clear that if we enter this block it is because of the condition identified by the RFC text.
src/sign/records.rs
Outdated
| // It will NOT construct the last name as that will be dealt | ||
| // with in the next outer loop iteration. | ||
| // - a.b.c.mail.example.com | ||
| for n in (1..distance_to_apex - 1).rev() { |
There was a problem hiding this comment.
If distance_to_apex is 2, then this loop will never execute. Could the outer condition have been distance_to_apex > 2, then? Is there an off-by-one error somewhere?
There was a problem hiding this comment.
Thanks! I was planning to add proper test cases but for now have extended my test input zone to cover this missing case and realize now why I had some additional logic in there before which I removed because I couldn't see what value it was adding... 😛 Looking at the comments again I see I even left in a description of the additional logic I removed, i.e. tracking the last non-empty non-terminal label.
…ity fixes from the downstream multiple-signing-key branch.
… partial backport of changes from the downstream multiple-signing-key branch.
- Remove unnecessary apex argument and trait fn as it can be derived from the input. - Remove unnecesarsy skip_before, the input should be the zone and nothing else. - Replace one unwrap() with an error return instead. - The Sort generic argument should be called Sort everywhere, not S. - SignableZone and SignableZoneInPlace don't no longer need specific impls for SortedRecords and Vec but can instead be blanket impls. - Handle the case that the zone has too many apex SOAs. - Factor out apex SOA lookup code. - Added some RustDoc.
…glect to sign the NSEC(3)s.
…n module by moving keys/keypair.rs to crypto/common.rs.
…ose submodule is useful for code structure but not for documentation.
…s NSEC doesn't do any hashing, only NSEC3 does.
…of non-existence, and the full term is authenticated denial of existence.
…as they relate to signing in general, and rename the signing sub-module as it is specific to signature generation.
Currently lacks collision detection and tests, though has been manually tested using
ldns-verify-zone,dnssec-verifyandnamed-checkzoneboth with and without opt-out and also including both signed and unsigned delegations.I'm posting this here as a draft to allow for alignment and early feedback from the team working on various pieces of DNSSEC support for
domain.Note: This PR includes the content of #444.