search

miekg / dns

DNS library in Go
https://miek.nl/2014/august/16/go-dns-package
BSD 3-Clause "New" or "Revised" License
7.86k stars 1.12k forks source link

DNSSEC grab public key from private for DNSKEY #1558

Open baest opened 2 months ago

baest commented 2 months ago

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 commented 2 months ago

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

baest commented 2 months ago

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 commented 2 months ago

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 https://github.com/miekg/dns/issues/1558#issuecomment-2074395902, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACWIW7BX3RA3XTTY2G4AP3Y65VATAVCNFSM6AAAAABGUI6QW6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANZUGM4TKOJQGI . You are receiving this because you modified the open/close state.Message ID: @.***>

baest commented 2 months ago

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.

https://github.com/miekg/dns/pull/1560

miekg commented 2 months ago

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 https://github.com/miekg/dns/pull/1560

— Reply to this email directly, view it on GitHub https://github.com/miekg/dns/issues/1558#issuecomment-2080019785, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACWIW6MMXCQFXZUKSUPKQLY7KU4PAVCNFSM6AAAAABGUI6QW6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBQGAYTSNZYGU . You are receiving this because you modified the open/close state.Message ID: @.***>

baest commented 2 months ago

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

baest commented 2 months ago

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