Private Algorithm Support

[jschlyter]

I've committed PR #934 to support signing with private algorithms. The idea was to create abstract base classes for Private/Public keys and implement generic functions to sign/verify using these. RRSIG validation is not yet supported.

My first use for this is testing key rollover to unknown algorithms, but this will also be useful for testing signing PQ algorithms and other new algorithms.

Comments welcome!

[rthalley]

Thanks, I'll take a look later this weekend! I will definitely want dnspython to support PQ DNSSEC when we know what that is, and enabling testing of various alternatives is good.

[jschlyter]

I'm a bit unsure where to implement signature validation for private algorithms. One would need a dictionary from (algorithm: Algorithm, key_prefix: bytes), where key prefix is either a wire encoded domain name (for 253) or a BER encoded OID (for 254). One option would possible to pass this dictionary down via dns.dnssec.validate() all the way down to _validate_signature.

N.B. hashing must be done in private algorithm class, not in the main logic as for other algorithms. Because of this,chosen_hash is always None.

[rthalley]

Sorry about the delay getting back to you. I like the class encapsulation you've done in the draft PR. Re validation, passing the dictionary is certainly one way to go, but I wanted to bring up another as it is so similar an issue. Dnspython lets you add your own rdata class implementations, and these are not treated any differently by the code from the built-ins. To add a type, you use the dns.rdata.register_type() method. So I wonder if we should have a registry that the code uses instead of passing a dictionary around. This might also be nicer for other code using dnspython and DNSSEC (I even have such code!) where it wouldn't need to be modified to support private algorithms other than calling the registration function. So one easy change instead of making sure you do the right thing in all verify cases.

For that matter, I wonder if all of the algorithms should be abstracted into classes so the main DNSSEC code isn't full of big "if" switches. This is, of course, not central to what you are doing, but might make things cleaner.

So imagine that there is a registry of algorithms keyed by (id, prefix), and the prefix is b'' for non-private types. The mainline code would be aware of the need to extract a prefix if the algorithm was private. Then you'd get your algorithm and prefix (if needed), look up the right implementation, and then call it in all cases.

What do people think?

[rthalley]

You have come up with the kind of thing I was asking for, but having read it I now have a concern about the large amount of boilerplate that will be created for all the hash variants. Maybe we could have classes PublicRSA and PrivateRSA that handle all RSA, and populate algorithm and chosen_hash in the constructor? This doesn't seem too painful as the info would be in the dnskey for from_dnskey, and it wouldn't be to ugly to pass them to generate(). Thoughts?

[jschlyter]

I think the amount of additional code is minimal, I'll create a PR this week to show a possible way forward.

[jschlyter]

Step 1 - move algorithm sign/verify/dnskey to separate classes, master...jschlyter:dnspython:dnssec_alg_cls

Comments appreciated before continuing.

[jschlyter]

Step 2 - add function to register new algorithms with optional key prefix for private algorithms

[jschlyter]

Should we support validation only algorithms? If so, we need to support registration for signing, validation or both.

[jschlyter]

You have come up with the kind of thing I was asking for, but having read it I now have a concern about the large amount of boilerplate that will be created for all the hash variants. Maybe we could have classes PublicRSA and PrivateRSA that handle all RSA, and populate algorithm and chosen_hash in the constructor? This doesn't seem too painful as the info would be in the dnskey for from_dnskey, and it wouldn't be to ugly to pass them to generate(). Thoughts?

I ended up using dataclasses to address this, if you don't want to use that it's easy to downgrade to pure classes.

[bwelling]

Having looked at the implementation, I have a couple of questions.

• The algorithm specification includes the private algorithm code and a prefix, and looking up an algorithm iterates the dict with prefix matching. Both PRIVATEOID and PRIVATEDNS specify how the algorithm is encoded (either an OID with a length byte or an uncompressed domain name). Why can't get_algorithm_cls extract the OID or domain name and do a direct dict lookup?

• Both RSA and EDDSA have a common base class, and the individual algorithms use the base class. ECDSA defines a P256 class, and the P384 subclasses that. Should it be done the same way as the others?

• It's not completely clear how this will be backwards compatible. The existing code has methods that expect PublicKey or PrivateKey objects, where callers would be passing low-level cryptography keys. To avoid breaking that, there would presumably need to be some way to "promote" those into dns.dnssecalgs keys.

[jschlyter]

• The algorithm specification includes the private algorithm code and a prefix, and looking up an algorithm iterates the dict with prefix matching. Both PRIVATEOID and PRIVATEDNS specify how the algorithm is encoded (either an OID with a length byte or an uncompressed domain name). Why can't get_algorithm_cls extract the OID or domain name and do a direct dict lookup?

Yes, I should do that - much cleaner.

• Both RSA and EDDSA have a common base class, and the individual algorithms use the base class. ECDSA defines a P256 class, and the P384 subclasses that. Should it be done the same way as the others?

This is the case for DSA and ECDSA. For EDDSA the base class is different as the underlying key classes are different. RSA could be based of one of (any really) of the RSA algorithms, but I did a base class since I could not make up my mind. None of these reasons are really valid, ED448 could be based on ED25519 and all the RSA algorithms of RSASHA256. Should I clean it up?

• It's not completely clear how this will be backwards compatible. The existing code has methods that expect PublicKey or PrivateKey objects, where callers would be passing low-level cryptography keys. To avoid breaking that, there would presumably need to be some way to "promote" those into dns.dnssecalgs keys.

Once we agree on the framework I will move on implementing this in the dns.dnssec. We should still accept PublicKey and PrivateKey objects and I have create from_key constructors for this purpose.

[rthalley]

I'm ok with this direction, but echoing what Brian said. And I definitely want to maintain backwards compatibility with the existing APIs.

[jschlyter]

My first take to ensure backwards compatibility will be to modify dns.dnssec without touching any existing test cases.

[bwelling]

• Both RSA and EDDSA have a common base class, and the individual algorithms use the base class. ECDSA defines a P256 class, and the P384 subclasses that. Should it be done the same way as the others?

This is the case for DSA and ECDSA. For EDDSA the base class is different as the underlying key classes are different. RSA could be based of one of (any really) of the RSA algorithms, but I did a base class since I could not make up my mind. None of these reasons are really valid, ED448 could be based on ED25519 and all the RSA algorithms of RSASHA256. Should I clean it up?

It wasn't as noticable for DSA, as while there are technically two algorithm identifiers, they're basically identical. For ECDSA, it seems strange to have the P384 implementation using the P256 implementation, rather than both of them using the same base class.

• It's not completely clear how this will be backwards compatible. The existing code has methods that expect PublicKey or PrivateKey objects, where callers would be passing low-level cryptography keys. To avoid breaking that, there would presumably need to be some way to "promote" those into dns.dnssecalgs keys.

Once we agree on the framework I will move on implementing this in the dns.dnssec. We should still accept PublicKey and PrivateKey objects and I have create from_key constructors for this purpose.

Sounds good. Thanks!

[jschlyter]

Step 3 - integrating into dns.dnssec (with full backwards compatibility) complete.

[jschlyter]

PR filed as #944