DNSSEC grab public key from private for DNSKEY

[baest]

Hi

First off, thanks for a great library! I noticed that if you take a DNSKEY and call NewPrivateKey or ReadPrivateKey on it given a private key, the public key that is set in the crypto.PrivateKey is taken from the DNSKEY and not extracted from the key itself.

pub := k.publicKeyRSA() 
...
priv.PublicKey = *pub

(k is the DNSKEY struct) and the same for ECDSA keys. ED25519 grabs the public key from the private key as I expect it would.

I know you want to keep the library small, but I want to hear if you would be willing to consider a pull requests with this behaviour changed and maybe also allow the methods to be called without a DNSKEY (or with an empty DNSKEY) as I don't see this is needed anymore and would at least make my use case simpler? I can of course create the PR.

My relevant use case btw is to load a private key generated elsewhere and stored in a database and create a corresponding DS record from it.

Thanks
Martin

[miekg]

but why can't you just set priv.PublicKey to whatever you want afterwards?

[baest]

For my usecase where I just want to load the private key to get the public key, I would have to invent a valid DNSKEY, use that to load the private key. Then I get a private key containing, possibly, another public key, which I then have to replace (if the public isn't live yet and therefore can't be queried using DNS). You also have a comment in the code that you should validate that the public key matches the private key which would, if implemented, break the above.
It would also offer the possibility of doing this without a DNSSEC struct and be consistent with how it is done for ED22519.

[miekg]

TBF I think only the RSA keys had this weird requirement that you need the pubkey to get the privkey and newer also don't need that. Anyhow, a fake DNSKEY doesn't break that bank and can prolly be reused as well. Happy to remove that TODO and update the docs that that check isn't performed.

…

On Wed, 24 Apr 2024, 10:34 Martin Frausing, ***@***.***> wrote: For my usecase where I just want to load the private key to get the public key, I would have to invent a valid DNSKEY, use that to load the private key. Then I get a private key containing, possibly, another public key, which I then have to replace (if the public isn't live yet and therefore can't be queried using DNS). You also have a comment in the code that you should validate that the public key matches the private key which would, if implemented break, the above. It would also offer the possibility of doing this without a DNSSEC struct and be consistent with how it is done for ED22519. — Reply to this email directly, view it on GitHub <#1558 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACWIW7BX3RA3XTTY2G4AP3Y65VATAVCNFSM6AAAAABGUI6QW6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANZUGM4TKOJQGI> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

[baest]

I don't think I was able to explain what I wanted to do and to be honest after reading the code further, I also changed my mind about using the DNSKEY to load the PrivateKey. This new code should work as before, IF you are loading a PrivateKey into a DNSKEY with the corresponding PublicKey. I would say in any case the behaviour when mixing different keys should be an error as your comment suggested. Now the PublicKey would be overwritten which make sense to me.

#1560

[miekg]

I'm trying to understand the difff, but this doesn't update any documentation, nor this this add a test to embed this deeper. So nack on this

…

On Fri, 26 Apr 2024, 20:43 Martin Frausing, ***@***.***> wrote: I don't think I was able to explain what I wanted to do and to be honest after reading the code further, I also changed about using the DNSKEY to load the PrivateKey. This new code should work as before, IF you are loading a PrivateKey into a DNSKEY with the corresponding PublicKey. I would say in any case the behaviour when mixing different keys should be an error as your comment suggested. Now the PublicKey would be overwritten which make sense to me. #1560 <#1560> — Reply to this email directly, view it on GitHub <#1558 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACWIW6MMXCQFXZUKSUPKQLY7KU4PAVCNFSM6AAAAABGUI6QW6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBQGAYTSNZYGU> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

[baest]

Thanks for the quick reply. I will revisit the PR and add tests and/or documentation changes

[baest]

Ok I have now improved the PR with the following:

1. Check that algorithm in the DNSKEY matches what the algorithm being loaded with NewPrivateKey
2. Check that the PublicKey in the DNSKEY matches what the PublicKey contained in the PrivateKey being loaded with NewPrivateKey
3. Test that both cases works
4. Test that an empty(ish) DNSKEY can be used to load a PrivateKey
5. Clarify in the docs that the PublicKey is set in the DNSKEY when it is used to load a PrivateKey

[miekg]

I still fail to see how

I would have to invent a valid DNSKEY,

is an insurmountable problem? That 3... 5 lines of some foo in a non-performance critical path?

[baest]

I of course respect that you don't want to merge this PR. But let me clarify a few things, this isn't only "That 3... 5 lines of some foo in a non-performance critical path". For me, this is not a matter of inconvenience where I would have to write a few lines to create a new DNSSEC struct. Rather, I would need to implement or copy various parts of your library into my own codebase to handle my use case (loading a private key from a database, retrieving the public key, and calculating the DS from it)

Without this PR:

1. When calling NewPrivateKey on a DNSSEC struct you need to have a DNSSEC struct with the correct algorithm set or it will fail with ErrKeyAlgMismatch. If you just get an unknown private key and wish to load it, you would first need to parse the algorithm from the key yourself, basically copying the parsing function or scanning the private key for the algorithm.

2. When calling NewPrivateKey on a DNSSEC struct, the struct is not used for anything but to override the public key contained within the private key with the public key from the DNSSEC struct for the returned private key.

	case RSASHA1, RSASHA1NSEC3SHA1, RSASHA256, RSASHA512:
		priv, err := readPrivateKeyRSA(m)
		if err != nil {
			return nil, err
		}
		pub := k.publicKeyRSA()
		if pub == nil {
			return nil, ErrKey
		}
		priv.PublicKey = *pub
		return priv, nil

When you have a public key inside the key you're loading, why should it be overridden with data from the DNSSEC struct and in such a way that the new public key (within the private key) is unavailable for you without parsing the key yourself.

I think an argument can be made that my solution; writing to the public key inside the DNSSEC struct is not correct either and I'm more than willing to change this to not use the DNSSEC struct at all if you think that is more correct.

3. If what I just described in 2. is the approach you want to take, then why isn't that the case for ED25519:

	case ED25519:
		return readPrivateKeyED25519(m)

This actually does more or less what I'm advocating, if you disregard the bit about the algorithm for 1.

If I'm not able to convince you, would you then consider making readPrivateKeyRSA, readPrivateKeyECDSA, readPrivateKeyED25519 and parseKey public functions somehow so my usecase can be handled by implementing that?