Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto/ecdh: provide and/or implement an interface #56052

Closed
rautammi opened this issue Oct 5, 2022 · 20 comments
Closed

crypto/ecdh: provide and/or implement an interface #56052

rautammi opened this issue Oct 5, 2022 · 20 comments
Labels
Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@rautammi
Copy link

rautammi commented Oct 5, 2022

I am very sorry for being very late with this one. Would it be possible to expose PrivateKey as an interface, allowing one to create implementations of Curve and PrivateKey which utilizes a static private key from an HSM for ECDH operation? Somewhat following the idiom from crypto.Signer.

@gopherbot gopherbot added this to the Proposal milestone Oct 5, 2022
@rautammi rautammi changed the title proposal: affected/package: crypto/ecdh Support for PKCS#11 proposal: crypto/ecdh: Support for PKCS#11 Oct 5, 2022
@ianlancetaylor ianlancetaylor added the Proposal-Crypto Proposal related to crypto packages or other security issues label Oct 5, 2022
@ianlancetaylor
Copy link
Contributor

CC @golang/security

@ericlagergren
Copy link
Contributor

@rautammi alternatively, APIs that need an ECDH private key could accept an interface instead of requiring *ecdh.PrivateKey. There is already crypto.PrivateKey and crypto.PublicKey.

@ericchiang
Copy link
Contributor

For reference, go-piv has supported ECDH backed by a YubiKey for a while now:

https://pkg.go.dev/github.com/go-piv/piv-go/piv?utm_source=godoc#ECDSAPrivateKey.SharedKey

I don't think crypto/ecdh is the place to support this? E.g. crypto/rsa and crypto/ecdsa don't support out-of-process private keys. If there are packages that want to support both crypto/ecdh and external keys, that interface probably belongs with those packages or in the crypto package (like crypto.Signer/crypto.Decrypter)

@rautammi
Copy link
Author

rautammi commented Oct 7, 2022

I think @ericchiang is right.
It has been awesome, that well-behaving packages are using crypto.Signer, allowing one to, for example, very easily add PKCS#11 support for TLS or SSH authentication, without any changes to those packages at all.
So, what we need, in addition to this crypto/ecdh, is some kind of crypto.Exchanger interface, which crypto/ecdh then implements, and which usage is recommended for any package agreeing shared secrets using a static private key.

@rautammi
Copy link
Author

rautammi commented Oct 7, 2022

I would say the person who invented crypto.Signer was a genius! Let she or he design an equally generic and future proof interface for key exchange too.

@rsc
Copy link
Contributor

rsc commented Oct 12, 2022

/cc @golang/security

@rsc
Copy link
Contributor

rsc commented Oct 12, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@FiloSottile
Copy link
Contributor

I agree that it would be nice to have an interface type for key exchanges, and while it might be too late to come up with one for Go 1.20, we should make sure the crypto/ecdh package design is not hostile to one.

Looking forward, it would probably be appropriate for it to fit the shape of KEMs, to work with PQC KEMs.

A KEM has three operations: KeyGen, Encap, and Decap.

KeyGen() -> public key, secret key
Encapsulate(public key) -> ciphertext, shared key
Decapsulate(secret key, ciphertext) -> shared key

With crypto/ecdh,

  • KeyGen is ecdh.P256().GenerateKey()
  • Encap is ecdh.P256().GenerateKey() followed by ecdh.P256().ECDH(privateKey, peerPublicKey)
    • the ciphertext is just the generated public key
  • Decap is ecdh.P256().NewPublicKey() followed by ecdh.P256().ECDH(privateKey, peerPublicKey)

because ECDH is very symmetrical.

While thinking about this I also started having doubts on whether ECDH should be a method on Curve or on PrivateKey. My initial thinking was that a method on PrivateKey like KeyExchange(publicKey []byte) would make it possible for it to implement a Signer-like interface, but that only works for DH-like symmetrical KEMs that don't have a separate Encap and Decap. If we had such a method on PrivateKey, it would also make sense to move ECDH to PrivateKey.

I'd be interested to hear proposals for KEM-friendly interfaces, and how crypto/ecdh might implement them. I went through a few options but found none that I liked so far.

@FiloSottile FiloSottile changed the title proposal: crypto/ecdh: Support for PKCS#11 proposal: crypto/ecdh: provide and/or implement an interface Oct 19, 2022
@rautammi
Copy link
Author

For existing PQC-KEMs, I think an interface would be very similar to crypto.Decrypter, And I would go as far as stating that it would not be too unintuitive to use crypto.Decrypter for that purpose.
But like @FiloSottile stated, ECDH is has a bit different properties, as is symmetric. And it would be applied in a different way. One cannot implement Noise framework's quad-ECDH using, for example, Kyber as a drop-in replacement.
I would vote for PrivateKey.KeyExchange(pubicKey []byte) as an interface for DH-like key exchanges.

@rsc
Copy link
Contributor

rsc commented Oct 26, 2022

What would the API be specifically?

@rautammi
Copy link
Author

rautammi commented Oct 27, 2022

type Exchanger interface {
    Public() crypto.PublicKey
    Exchange(peerPublic []byte, opts ExchangerOpts) (sharedSecret []byte, err error)
}
type ExchangerOpts any

Exchanger would be an opaque private key.
Maybe ExchangerOpts should implement HashFunc() crypto.Hash for passing the hash function to be used in whitening of the shared secret.

@rsc
Copy link
Contributor

rsc commented Nov 2, 2022

Talked to @FiloSottile about this, and he believes we should move ECDH to the PrivateKey type, and then an interface can be defined (not necessarily in this package) that PrivateKey implements.

@rsc
Copy link
Contributor

rsc commented Nov 9, 2022

To elaborate on my comment from last week, the idea is to take ecdh.PrivateKey, which already has Bytes, Curve, Equal, Public, and PublicKey methods, and move the ECDH method over there too. Then any hardware key can implement these methods, and code that wants to take either kind can define an interface listing the methods it needs. In this case we would not need to define an interface in the standard library.

Does anyone object to that approach?

@gopherbot
Copy link

Change https://go.dev/cl/450335 mentions this issue: crypto/ecdh: move ECDH method to PrivateKey

@gopherbot
Copy link

Change https://go.dev/cl/451095 mentions this issue: internal/wycheproof: update Go 1.20 crypto/ecdh API

gopherbot pushed a commit to golang/crypto that referenced this issue Nov 16, 2022
For golang/go#56052

Change-Id: If34d01132e221ff525319e43d127ef14579f9054
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/451095
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Joedian Reid <joedian@golang.org>
Auto-Submit: Filippo Valsorda <filippo@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Run-TryBot: Joedian Reid <joedian@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
@rsc
Copy link
Contributor

rsc commented Nov 16, 2022

Reopening.

@rsc rsc reopened this Nov 16, 2022
@rsc
Copy link
Contributor

rsc commented Nov 16, 2022

This was marked as closed by the CL that moved the ECDH method. Now it is possible to define an interface outside the standard library that ecdh.PrivateKey implements, and hardware keys can implement it too. So it seems like we don't need to add an interface to the standard library right now.

Do I have that right?

@FiloSottile
Copy link
Contributor

Yeah, that sounds right to me. Eventually we might define a ECDH or KEM interface in the standard library if we need to consume it, but in the meantime hardware implementations can implement the PrivateKey methods and use their own interface.

Sorry for jumping the gun on the proposal process, I hadn't realized this wasn't accepted yet and I wanted to get the changes in before the freeze since crypto/ecdh is new in Go 1.20.

@rsc
Copy link
Contributor

rsc commented Nov 30, 2022

Sounds like no one objects, and since the work is already done, I'll just move it to accepted and close it.

@rsc
Copy link
Contributor

rsc commented Nov 30, 2022

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: crypto/ecdh: provide and/or implement an interface crypto/ecdh: provide and/or implement an interface Nov 30, 2022
@rsc rsc modified the milestones: Proposal, Backlog Nov 30, 2022
@rsc rsc closed this as completed Nov 30, 2022
LewiGoddard added a commit to LewiGoddard/crypto that referenced this issue Feb 16, 2023
For golang/go#56052

Change-Id: If34d01132e221ff525319e43d127ef14579f9054
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/451095
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Joedian Reid <joedian@golang.org>
Auto-Submit: Filippo Valsorda <filippo@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Run-TryBot: Joedian Reid <joedian@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
@dmitshur dmitshur modified the milestones: Backlog, Go1.20 Jun 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
Status: Accepted
Development

No branches or pull requests

8 participants