The great rename plus cleanup. #488
Conversation
TransitioningNsecToNsec3, and TransitioningNsec3ToNsec.
Move parse_from_bind, format_as_bind, and display_as_bind to dnssec::common. Move key_size to dnssec::validator::base.
|
|
||
| impl std::error::Error for FromBytesError {} | ||
|
|
||
| //----------- GenerateError -------------------------------------------------- |
There was a problem hiding this comment.
It seems like GenerateError (excluding the conversion impl below) is the same for openssl and ring - would it be an idea to move it into common?
There was a problem hiding this comment.
In the future, if more crypto implementations are added, they could potentially bring new kinds of generation errors here (e.g. HSM I/O error? IDK). It makes sense to me to avoid unifying them and requiring a single generation error type across all implementations, unless we have a clearer perspective on how future crypto implementations might operate.
| Ok(n.len() * 8 - n[0].leading_zeros() as usize) | ||
| } | ||
| SecAlg::ECDSAP256SHA256 | SecAlg::ECDSAP384SHA384 => { | ||
| // ECDSA public keys have two points. |
There was a problem hiding this comment.
The original code said that ECDSA public keys have a marker byte as well and subtracted one from the len to account for this. Is that no longer the case?
There was a problem hiding this comment.
I think it is ring and openssl that have the marker byte and DNSKEY doesn't.
Co-authored-by: Ximon Eighteen <[email protected]>
Co-authored-by: Ximon Eighteen <[email protected]>
src/crypto/common.rs
Outdated
| /// A message digest. | ||
| pub enum Digest { | ||
| #[cfg(feature = "ring")] | ||
| /// A message digest computed using ring. | ||
| Ring(ring::Digest), | ||
| #[cfg(feature = "openssl")] | ||
| /// A message digest computed using openssl. | ||
| Openssl(openssl::Digest), | ||
| } | ||
|
|
||
| impl AsRef<[u8]> for Digest { | ||
| fn as_ref(&self) -> &[u8] { | ||
| match self { | ||
| #[cfg(feature = "ring")] | ||
| Digest::Ring(digest) => digest.as_ref(), | ||
| #[cfg(feature = "openssl")] | ||
| Digest::Openssl(digest) => digest.as_ref(), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not sure that Digest needs to be an enum over all the cryptographic backends. At the end of the day, it's just a (DigestType, Vec<u8>), right? Perhaps we should define it that way for simplicity.
There was a problem hiding this comment.
It seems that that would require an extra Vec. No it just gets AsRef from the underlying ring or openssl type.
src/crypto/openssl.rs
Outdated
| key.extend(&exp_len.to_be_bytes()); | ||
| } else { | ||
| unreachable!( | ||
| "RSA exponents are (much) shorter than 64KiB" |
There was a problem hiding this comment.
Yeah, I think I had originally written this in a call to .expect(). It should be rephrased now (and can in fact include the detected length in the error message).
|
|
||
| impl std::error::Error for FromBytesError {} | ||
|
|
||
| //----------- GenerateError -------------------------------------------------- |
There was a problem hiding this comment.
In the future, if more crypto implementations are added, they could potentially bring new kinds of generation errors here (e.g. HSM I/O error? IDK). It makes sense to me to avoid unifying them and requiring a single generation error type across all implementations, unless we have a clearer perspective on how future crypto implementations might operate.
Following PR comments (#488).
Move DNSSEC modules around. This PR removes the validate module. The validate module becomes dnssec::validator::base.
Remove apex_owner from sign_rrset_in.
Move DNSKEY generation and signing to the application.