NSEC generation support

[jschlyter]

Add function for NSEC generation: dns.dnssec.add_nsec_to_zone(). Finding all secure names are implemented in a separate function as it might be useful for NSEC3 if implemented later.

[jschlyter]

#906 should be merged first if desired (or just close it and merge this one)

[bwelling]

Hi @jschlyter,

I think there are a couple of problems with this.

First, the logic in _get_secure_names isn't quite right. It's using the presence of an NS record to determine that a name is a delegation point, but that's not correct for names which are themselves under a delegation point. For example,

example IN SOA
example IN NS
sub.example IN NS
sub.sub.example IN NS

example will not be marked as a delegation, because it's the origin. sub.example is correctly marked as a delegation. sub.sub.example is incorrectly marked as a delegation, which both means that an NSEC is created for it, and further names under sub.example also get NSECs.

Second, the reason for _get_secure_names being a separate method is so that it can be shared with NSEC3, but NSEC3 has different semantics with respect to empty nonterminals, and would require more than combining the output of _get_secure_names and zone iteration.

I'm not sure that I see the benefit of doing multiple iterations, one to exclude glue and one to actually generate the NSECs; why not iterate once, and at each node, skip it if it's glue, set the current delegation if appropriate, and add the NSEC if it's appropriate?

[rthalley]

The current code is not adding NSEC to the bitmap, but it should per RFC 4035 section 2.3.

[rthalley]

Also, I've got a PR open that will allow iteration of the zone inside of the transaction, as the way of doing it with the released API is clunky. (It's best to do everything transactionally when doing transactions.)

[jschlyter]

The current code is not adding NSEC to the bitmap, but it should per RFC 4035 section 2.3.

Fixed in e067703

[jschlyter]

Also, I've got a PR open that will allow iteration of the zone inside of the transaction, as the way of doing it with the released API is clunky. (It's best to do everything transactionally when doing transactions.)

I'll test that.

[rthalley]

Don't worry about the codeql error on this one, that was my fault when I was removing curio as I missed a reference to it

[jschlyter]

@bwelling single iteration fixed now

[bwelling]

Definitely looks better; thanks.

Before this is merged, though, it might be worth a bit more thought about whether this is the right API.

The main reason to add NSECs to a zone is when signing the zone. With this API, signing the zone would involve calling add_nsec_to_zone, then iterating the zone and determining glue status for every name again when generating signatures. This seems suboptimal. Adding NSECs (only) is really just a special case of signing the zone with no keys.

I think what I'm mostly trying to say is that if the goal is to have a way to sign a zone, I don't think there should be a separate API for adding NSEC records; it should just be part of the signing. If the goal is to incrementally add signing, I think this is a good step, but I would rather see this folded into a more comprehensive signing API between now and the next release, rather than be a permanent API on its own. That doesn't mean that this change couldn't be committed now; just that I don't think it should be released in its current state.

The one change I would like to see before this is committed at all is to allow a (writable) transaction to be passed in, so that the process of adding NSEC records is not required to be the only change in the zone version. If the caller passed None, the code would create a writable transaction. This could use contextlib.nullcontext to either create a writer or use the one passed in.

[jschlyter]

@bwelling I agree, I'll take a look at the context issue. For signing, we could pass an optional function to apply to all secure names. Such a function, which could be a partial function, would be passed the context and the node and can choose what signatures to create. Would that be a way forward?

[jschlyter]

Some ideas in #904 (comment)

[bwelling]

The changes to add signing are a valid technical solution, but they don't answer the real questions I had.

• Why would anyone use an API to add NSEC records, other than as part of signing a zone?
• Wouldn't an API to sign a zone be better?

The rrset_signer callback allows most of the one-shot signing work to be done, but splits the logic between the NSEC generation method and the caller, and doesn't address the rest of signing (adding keys, doing anything with KSKs, probably something else). It also means that if we (possibly you!) ever wanted to add NSEC3 support, then signing a zone using NSEC3 would use a completely different interface, rather than a parameter to a sign_zone method.

I'm bringing this up because once an API is added (and released), we're stuck with it. If the more common operation is "sign a zone", then it would be better overall to have an API for that, and not be stuck with an API for "add NSEC records with a callback for signing".

[jschlyter]

I'll move this discussion to #909, a PR might not be the best place to sort this out.