NSEC3 generation support. #416

ximon18 · 2024-10-15T14:18:02Z

Currently lacks collision detection and tests, though has been manually tested using ldns-verify-zone, dnssec-verify and named-checkzone both 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.

A private key converted into a 'KeyPair' can be exported in the conventional DNS format. This is an important step in implementing 'ldns-keygen' using 'domain'. It is up to the implementation modules to provide conversion to and from 'KeyPair'; some impls (e.g. for HSMs) won't support it at all.

'Sign' is a more generic version of 'sign::key::SigningKey' that does not provide public key information. It does not try to abstract over all the functionality of a keypair, since that can depend on the underlying cryptographic implementation.

There are probably lots of bugs in this implementation, I'll add some tests soon.

Also fixes 'cargo clippy' issues, particularly with the MSRV.

I'm going to add a corresponding 'PublicKey' type, at which point it becomes important to differentiate from the generic representations and actual cryptographic implementations.

Key generation, for now, will only be provided by the OpenSSL backend (coming soon). However, generic keys (for RSA/SHA-256 or Ed25519) can be imported into the Ring backend and used freely.

The OpenSSL backend supports import from and export to generic secret keys, making the formatting and parsing machinery for them usable. The next step is to implement generation of keys.

There were bugs in the Base64 encoding/decoding that are not worth trying to debug; there's a perfectly usable Base64 implementation in the crate already.

I had to swap out the RSA key since 'ring' found it to be too small.

- RSA signatures were being made with an unspecified padding scheme. - ECDSA signatures were being output in ASN.1 DER format, instead of the fixed-size format required by DNSSEC (and output by 'ring'). - Tests for signature failures are now added for both backends.

bal-e · 2024-10-16T07:04:56Z

src/sign/ring.rs

+pub fn nsec3_hash<N, SaltOcts, HashOcts>(
+    owner: N,
+    algorithm: Nsec3HashAlg,
+    iterations: u16,
+    salt: &Nsec3Salt<SaltOcts>,
+) -> Result<OwnerHash<HashOcts>, Nsec3HashError>


Why not just take the owner name and an Nsec3param?

nsec3_default_hash can then be replaced by calling this function with default() for the second argument.

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.

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).

bal-e · 2024-10-16T07:08:30Z

src/sign/ring.rs

+    let owner_hash = OwnerHash::from_octets(hash)
+        .map_err(|_| Nsec3HashError::OwnerHashError)?;


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).

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/ring.rs

bal-e · 2024-10-16T07:29:29Z

src/sign/records.rs

+        nsec3_recs: Vec<Record<N, Nsec3<Octets>>>,
+        nsec3param_rec: Record<N, Nsec3param<Octets>>,


Just name these records and params?

I was expecting to revisit the design of these APIs. If it remains as such then yes I agree.

src/sign/ring.rs

ximon18 · 2024-10-30T12:31:36Z

@bal-e: I realize that I moved the nsec3_hash() function from validator/nsec.rs to sign/ring.rs I think because that more clearly showed that it was Ring powered/dependent. However, we were discussing yesterday that lots of functionality is shared between signing and validating and that right now you are putting everything under validation, while I proposed a common module shared by both signing and validating. Either way, if you are currently putting everything under validation, this move to sign is inconsistent and perhaps you think I should move it back?

bal-e · 2024-10-30T16:49:47Z

@ximon18: yeah, I think this should be moved under validate for now. We'll gather up the shared functionality in that module for now, then decide how to distribute it.

…feedback.

ximon18 · 2024-10-30T23:13:27Z

@ximon18: yeah, I think this should be moved under validate for now. We'll gather up the shared functionality in that module for now, then decide how to distribute it.

Done.

src/sign/records.rs

bal-e · 2024-10-31T10:45:02Z

src/sign/records.rs

+            //    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 {


I don't think the if statement is necessary, the for loop will run for zero iterations if this condition is not true.

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.

bal-e · 2024-10-31T10:47:24Z

src/sign/records.rs

+                // 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() {


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?

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.

src/validate.rs

…EC3." This reverts commit a04c917.

…ed).

…ity fixes from the downstream multiple-signing-key branch.

… partial backport of changes from the downstream multiple-signing-key branch.

arya dradjica added 30 commits October 2, 2024 13:42

[sign] Define trait 'Sign'

7b51569

'Sign' is a more generic version of 'sign::key::SigningKey' that does not provide public key information. It does not try to abstract over all the functionality of a keypair, since that can depend on the underlying cryptographic implementation.

[sign] Implement parsing from the DNS format

cb97321

There are probably lots of bugs in this implementation, I'll add some tests soon.

[sign] Provide some error information

db51ae6

Also fixes 'cargo clippy' issues, particularly with the MSRV.

[sign] Move 'KeyPair' to 'generic::SecretKey'

a505415

I'm going to add a corresponding 'PublicKey' type, at which point it becomes important to differentiate from the generic representations and actual cryptographic implementations.

[sign/generic] Add 'PublicKey'

ea80694

[sign] Rewrite the 'ring' module to use the 'Sign' trait

7c94006

Key generation, for now, will only be provided by the OpenSSL backend (coming soon). However, generic keys (for RSA/SHA-256 or Ed25519) can be imported into the Ring backend and used freely.

Implement DNSSEC signing with OpenSSL

f9564c1

The OpenSSL backend supports import from and export to generic secret keys, making the formatting and parsing machinery for them usable. The next step is to implement generation of keys.

[sign/openssl] Implement key generation

c705428

[sign/openssl] Test key generation and import/export

68476e7

[sign/openssl] Add support for ECDSA

b68b639

[sign/openssl] satisfy clippy

79b2a08

[sign/openssl] Implement the 'Sign' trait

1aeeede

Install OpenSSL in CI builds

90af63d

Ensure 'openssl' dep supports 3.x.x

6370035

[workflows/ci] Use 'vcpkg' instead of vendoring OpenSSL

d53f85a

Ensure 'openssl' dep exposes necessary interfaces

5148bd3

[workflows/ci] Record location of 'vcpkg'

13bebd7

[workflows/ci] Use a YAML def for 'VCPKG_ROOT'

c86f234

[workflows/ci] Fix a vcpkg triplet to use

8939603

Upgrade openssl to 0.10.57 for bitflags 2.x

9ed1f44

[workflows/ci] Use dynamic linking for vcpkg openssl

24b443c

[workflows/ci] Correctly annotate 'vcpkg'

d3a071d

[sign/openssl] Implement exporting public keys

669da93

[sign/ring] Implement exporting public keys

8a0c59a

[sign/generic] Test (de)serialization for generic secret keys

7c6cde1

There were bugs in the Base64 encoding/decoding that are not worth trying to debug; there's a perfectly usable Base64 implementation in the crate already.

[sign] Thoroughly test import/export in both backends

d6a5313

I had to swap out the RSA key since 'ring' found it to be too small.

[sign] Remove debugging code and satisfy clippy

8321bbf

[sign] Account for CR LF in tests

db6820e

ximon18 mentioned this pull request Oct 17, 2024

Use domain crate NSEC3 support. NLnetLabs/dnst#6

Open

bal-e force-pushed the dnssec-key branch 2 times, most recently from a79aac6 to 0f54a8d Compare October 24, 2024 14:22

ximon18 added 4 commits October 28, 2024 16:11

Merge branch 'dnssec-key' into initial-nsec3-generation

735712c

Merge branch 'dnssec-key' into initial-nsec3-generation

4447cc6

Merge branch 'dnssec-key' into initial-nsec3-generation

d84e2c0

Merge branch 'dnssec-key' into initial-nsec3-generation

02c625b

bal-e reviewed Oct 30, 2024

View reviewed changes

ximon18 added 2 commits October 31, 2024 00:07

Move nsec3_hash() back into the validator module per review feedback.

8bf2c9f

Move nsec3_hash() to the validate (not validator!) module per review …

beb8e52

…feedback.

bal-e reviewed Oct 31, 2024

View reviewed changes

ximon18 and others added 16 commits October 31, 2024 22:27

Extend test file with records useful for manual testing of NSEC3.

a04c917

Revert "Extend test file with records useful for manual testing of NS…

abaab27

…EC3." This reverts commit a04c917.

Review feedback.

3c53e9e

Review feedback.

50433f0

Review feedback inspired change (though not actually what was suggest…

70e998a

…ed).

Display NSEC3 without trailing space if the bitmap is empty.

9268dd3

Backport NSEC3 improvements and upstream dnssec-key branch compatibil…

fb7e9ef

…ity fixes from the downstream multiple-signing-key branch.

[sign,validate] Add 'display_as_bind()' to key bytes types

414ea6c

[sign,validate] remove unused imports

2bde7aa

[sign] Document everything

98db88b

Update to work with changes in the upstream dnssec-key branch using a…

8877c22

… partial backport of changes from the downstream multiple-signing-key branch.

Minor tweaks.

40d65ac

Merge branch 'dnssec-key' into initial-nsec3-generation

c0fc11b

Add some Arbitrary impls to support cargo-fuzz based fuzz testing.

bdeddde

Impl Display for Nsec3HashError.

f2cabc3

Cargo fmt.

109370d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NSEC3 generation support. #416

NSEC3 generation support. #416

ximon18 commented Oct 15, 2024 •

edited

Loading

bal-e Oct 16, 2024

bal-e Oct 16, 2024

ximon18 Oct 30, 2024

ximon18 Oct 30, 2024 •

edited

Loading

bal-e Oct 16, 2024

ximon18 Oct 30, 2024

bal-e Oct 16, 2024

ximon18 Oct 30, 2024

ximon18 commented Oct 30, 2024

bal-e commented Oct 30, 2024

ximon18 commented Oct 30, 2024

bal-e Oct 31, 2024

ximon18 Oct 31, 2024

bal-e Oct 31, 2024

ximon18 Oct 31, 2024 •

edited

Loading

		let owner_hash = OwnerHash::from_octets(hash)
		.map_err(\|_\| Nsec3HashError::OwnerHashError)?;

		nsec3_recs: Vec<Record<N, Nsec3<Octets>>>,
		nsec3param_rec: Record<N, Nsec3param<Octets>>,

NSEC3 generation support. #416

Are you sure you want to change the base?

NSEC3 generation support. #416

Conversation

ximon18 commented Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ximon18 Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ximon18 commented Oct 30, 2024

bal-e commented Oct 30, 2024

ximon18 commented Oct 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ximon18 Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

ximon18 commented Oct 15, 2024 •

edited

Loading

ximon18 Oct 30, 2024 •

edited

Loading

ximon18 Oct 31, 2024 •

edited

Loading