DNSSEC signing?

[jschlyter]

Are there any work done on DNSSEC signing support? If not, I might start some work.

[jschlyter]

Unless @rthalley has a large chunk of uncommitted code for signing, I'll start coding.

[rthalley]

I don't think there is any current work on signing. Are you interested in just basic "make a signature" code or are you interested in signing whole zones? Discussing things here before doing a lot of work might save time later too.

[jschlyter]

My initial need was to create RRSIGs on the fly while regression testing my DS scanner, so my plan is to create functions that:

1. create DNSKEY RRs from a Cryptography RSA/ECDSA/RDDSA public key (initial prototype written)
2. sign RRsets using a Cryptography RSA/ECDSA/RDDSA private key

Extending this with zone signing should be trivial, either procedural or by extending the zone object.

Trying to do test driven development, my next step is to gather the test vectors from the RFCs. This turns out to be a challenge since the format used – "Private-key-format: v1.2" – doesn't seem to be documented anywhere.

[bwelling]

It's the format for BIND .private key files, which I've never seen good documentation for. Fortunately, it's not all that complicated; the algorithm is on the second line, and the rest of the lines are base64 encoded key fields, which usually correspond to the fields that openssl keys would use (and I would guess that Cryptography would be similar).

I've written a few parsers in non-BIND code for these over the years, and can provide more information or help if needed.

A couple of other things worth mentioning:

• the ECDSA signatures in the RFC contain entropy, so you can't write code that imports the keys and then generates matching signatures.
• the EDDSA signatures in the RFC are wrong; the correct ones are in the Errata

[jschlyter]

@rthalley Given that cryptography is imported last in dnssec.py, would it be acceptable to put all signing specific functions (with typing) in a separate source file and import around the same place? It's hard to do things like isinstance(public_key, ed25519.Ed25519PublicKey) without cryptography imported.

[rthalley]

Why not do them in dnssec.py after the verification functions etc. but before the imports? It shouldn't matter that the imports are last; python doesn't care. Does your IDE object? With the IDE I'm using, it still knows they are imported and does proper completions anywhere. We could move the imports higher if needed.

[jschlyter]

If you want to use typing, you need to have the symbols imported before using them. Putting all functions using cryptography in its own file (and only import it from dns.dnssec) would solve that I believe. That would enable constructs like:

PublicKey = Union[
    rsa.RSAPublicKey,
    ec.EllipticCurvePublicKey,
    ed25519.Ed25519PublicKey,
    ed448.Ed448PublicKey,
]

and allow using PublicKey as a type. Mostly syntactic sugar though, but quite useful. I can write my code without if needed.

[jschlyter]

Some initial draft code for DNSKEY generation in https://github.com/jschlyter/dnspython/tree/rrset_signer - comments appreciated

[nils-wisiol]

Thanks for your work on this, @jschlyter!

I've glanced over it and found some nits:

• The doc comments says algorithm is a hash function (SHA1, 256, ...) but the way I read the code it should identify the public key scheme
• The test cases do not actually test the result of the function
• dnspython defines it's own exceptions in many places, maybe that's appropriate here, too, instead of ValueError / TypeError
• There is a reference to RFC 3110 for RSA, but not for the others.
• I checked the treatment of large exponents (_exp_len > 255) and believe it makes sense, but also think that this is a corner case that deserves either it's own test case or a NotImplementedException. To create such keys, you could use cryptography.hazmat.primitives.asymmetric.rsa.generate_private_key(public_exponent=65537, key_size=2048).public_key() with appropriate public_exponent and key_size.
• RFC 3110 does not allow leading zeros, and I have confirmed that pn.n.to_bytes((pn.n.bit_length() + 7) // 8, "big") accounts for that

[jschlyter]

• The doc comments says algorithm is a hash function (SHA1, 256, ...) but the way I read the code it should identify the public key scheme

Will fix.

• The test cases do not actually test the result of the function

That will be updated once I have gathered test vectors using some other tool that can read the keys and emit DNSKEY RRs for comparison.

• dnspython defines it's own exceptions in many places, maybe that's appropriate here, too, instead of ValueError / TypeError

Good point.

• There is a reference to RFC 3110 for RSA, but not for the others.

Will add.

• I checked the treatment of large exponents (_exp_len > 255) and believe it makes sense, but also think that this is a corner case that deserves either it's own test case or a NotImplementedException. To create such keys, you could use cryptography.hazmat.primitives.asymmetric.rsa.generate_private_key(public_exponent=65537, key_size=2048).public_key() with appropriate public_exponent and key_size.

Yes, we should have test cases for large exponents. I have that in my local code already.

[jschlyter]

One could argue that key_to_dnskey should be a method in the DNSKEYBase class, WDTY @rthalley ?

[jschlyter]

@nils-wisiol more tests added now, please check

[bwelling]

I don't think key_to_dnskey makes sense as a method in the DNSKEYBase class. The closest existing method I can see is dns.dnssec.make_ds, and having a routine to convert a native key to a DNSKEY in the same place seems more consistent. I do wonder if the new routine should be named something like make_dnskey, also for consistency.

[jschlyter]

make_dnskey it is, much better.

[bwelling]

One thing that looks strange to me is that one of the parameters to _key_to_dnskey is:

algorithm: Union[int, str] = None,

but as far as I can tell, it requires an algorithm. Should the = None be removed?

[jschlyter]

Yes, I should remove the default default algorithm selection - the = None was a leftover since I remove the default.

[jschlyter]

Draft signer now in https://github.com/jschlyter/dnspython/tree/rrset_signer. Comments on parameters to dns.dnssec.sign appreciated.

• inception/expiration as datetime and/or integer?
• pass signer as DNSKEY + signer name or as RRset?

[bwelling]

Minor comments, based on a quick scan. I haven't actually tried using it.

The code doesn't appear to handle the (Name, Rdataset) form of rrset, even though the pydoc says that it does, and it probably should for consistency with validate().

Should there be some way to specify inception/expiration using YYYYMMDDHHMMSS notation?

Is the verify parameter just for development? It doesn't seem like a necessary part of the API.

The DSA code calls pn = private_key.public_key().public_numbers() for no reason.

I'm not sure about the signer parameters. Using an RRset seems more consistent, but has nonobvious semantics. Could the RRset have size > 1 where the code would look for the matching DNSKEY in it, or would that be an error?

[jschlyter]

The code doesn't appear to handle the (Name, Rdataset) form of rrset, even though the pydoc says that it does, and it probably should for consistency with validate().

I believe we either need the rrset, or have a way of setting the TTL.

Should there be some way to specify inception/expiration using YYYYMMDDHHMMSS notation?

Yes, we could allow inception/expiration as a string.

Is the verify parameter just for development? It doesn't seem like a necessary part of the API.

It's not necessary, should be dropped I guess.

The DSA code calls pn = private_key.public_key().public_numbers() for no reason.

This should be fixed. As a matter of fact, I can't find out how to calculate the DSA t parameter - the RFC is not very clear on this and the references are moot.

I'm not sure about the signer parameters. Using an RRset seems more consistent, but has nonobvious semantics. Could the RRset have size > 1 where the code would look for the matching DNSKEY in it, or would that be an error?

I prefer that one provides exactly one key for signing. The RR is needed to extract keytag and algorithm.

[jschlyter]

Thinking a bit more about the inception/expiration handling, I believe it should be consistent. Is trying to parse different types (int, datetime, str) useful or featurism? Should validate() accept the same types for now?

[bwelling]

The code doesn't appear to handle the (Name, Rdataset) form of rrset, even though the pydoc says that it does, and it probably should for consistency with validate().

I believe we either need the rrset, or have a way of setting the TTL.

Rdataset has a TTL.

Should there be some way to specify inception/expiration using YYYYMMDDHHMMSS notation?

Yes, we could allow inception/expiration as a string.

Thinking a bit more about the inception/expiration handling, I believe it should be consistent. Is trying to parse different types (int, datetime, str) useful or featurism? Should validate() accept the same types for now?

Hard to say. validate would (I assume) typically be called with the default current time, so supporting multiple formats probably isn't too important. sign is more likely to be called with an alternate start time, I would think. RRSIG.sigtime_to_posixtime might be enough to remove the need for a string.

The DSA code calls pn = private_key.public_key().public_numbers() for no reason.

This should be fixed. As a matter of fact, I can't find out how to calculate the DSA t parameter - the RFC is not very clear on this and the references are moot.

RFC 2536 says this, which can be reversed to derive T. It's never been clear to me why T is needed, though; none of the other algorithms encode an explicit size.

P is in the range 2**(511+64T) < P < 2**(512+64T) and so is 64 + 8*T octets long.

I'm not sure about the signer parameters. Using an RRset seems more consistent, but has nonobvious semantics. Could the RRset have size > 1 where the code would look for the matching DNSKEY in it, or would that be an error?

I prefer that one provides exactly one key for signing. The RR is needed to extract keytag and algorithm.

The advantage of a larger RRset is that if you wanted to sign with multiple keys, you could loop over the private keys and pass the same RRset each time. The disadvantage is that it's confusing.

[jschlyter]

Thanks for your comments, I've fixed most of the above now except the keys as RRset.

[jschlyter]

For the record: I've implemented support for DSA for completeness - not because anyone should actually use it.

[jschlyter]

PR #866 submitted

[jschlyter]

#866 merged

[rthalley]

Thanks for all the work!