-
Notifications
You must be signed in to change notification settings - Fork 18k
crypto/ecdsa: add NewPublicKey and PublicKey.Bytes #63963
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
Comments
You can use the func ecdhToECDSAPublicKey(key *ecdh.PublicKey) *ecdsa.PublicKey {
rawKey := key.Bytes()
switch key.Curve() {
case ecdh.P256():
return &ecdsa.PublicKey{
Curve: elliptic.P256(),
X: big.NewInt(0).SetBytes(rawKey[1:33]),
Y: big.NewInt(0).SetBytes(rawKey[33:]),
}
case ecdh.P384():
return &ecdsa.PublicKey{
Curve: elliptic.P384(),
X: big.NewInt(0).SetBytes(rawKey[1:49]),
Y: big.NewInt(0).SetBytes(rawKey[49:]),
}
case ecdh.P521():
return &ecdsa.PublicKey{
Curve: elliptic.P521(),
X: big.NewInt(0).SetBytes(rawKey[1:67]),
Y: big.NewInt(0).SetBytes(rawKey[67:]),
}
default:
panic("cannot convert non-NIST *ecdh.PublicKey to *ecdsa.PublicKey")
}
} But I agree that it is not convenient to write this. |
Ah, thanks for the info, that at least looks more efficient, if a little obscure. I'll use that instead of x509 for the time being 👍 What does it look like for PrivateKey? |
Not tested, but i think something like this should work: key := &ecdsa.PrivateKey{
PublicKey: *ecdhToECDSAPublicKey(priv.PublicKey()),
D: big.NewInt(0).SetBytes(priv.Bytes()),
} |
Gave that a test, seems to work, thanks! |
I think this proposal looks like this now: package ecdsa // crypto/ecdsa
// NewPrivateKeyFromECDH converts [*ecdh.PrivateKey] into [*PrivateKey].
// It panics when the [*ecdh.PrivateKey.Curve] is not one of [ecdh.P256],
// [ecdh.P384], [ecdh.P521].
func NewPrivateKeyFromECDH(priv *ecdh.PrivateKey) *ecdsa.PrivateKey
// NewPublicKeyFromECDH converts [*ecdh.PublicKey] into [*PublicKey].
// It panics when the [*ecdh.PublicKey.Curve] is not one of [ecdh.P256],
// [ecdh.P384], [ecdh.P521].
func NewPublicKeyFromECDH(priv *ecdh.PublicKey) *ecdsa.PublicKey As an alternative we might provide functions that parse PublicKey/PrivateKey from raw bytes, similar to package ecdsa // crypto/ecdsa
func NewPrivateKey(curve elliptic.Curve, raw []byte) (*ecdsa.PrivateKey, error)
func NewPublicKey(curve elliptic.Curve, raw []byte) (*ecdsa.PublicKey, error) |
cc @golang/security |
Hey @FiloSottile could you offer guidance on this? I also have an uncompressed point public key for ECDSA, and would reach for |
Looking at this again, an @ThadThompson sounds like you also need |
@mrwonko Yeah, right, but because there is only one error condition it might also be a bool instead. Either way is fine. func NewPrivateKeyFromECDH(priv *ecdh.PrivateKey) (priv *ecdsa.PrivateKey, ok bool) |
Keys should generally not be reused for both ECDH and ECDSA. It can be safe, but it's not something we want to encourage blindly. crypto/ecdsa to crypto/ecdh exists because crypto/x509 parsing functions will unavoidably return a crypto/ecdsa key, where in fact you might want a crypto/ecdh one. (The encoding of the two are the same.) However, here as far as I can tell what folks need is not a conversion, but a way to go from uncompressed point encoding (the input to If that's right, I think we should add new parsing functions to crypto/ecdsa, rather than involve crypto/ecdh. @mrwonko, I see the details on the public key format, but what format are you parsing the private key from? |
@FiloSottile yes, exactly. I'm writing a solution involving both signatures and key exchange. While using different keys for ECDH and ECDSA, the key exchange is being used in HPKE which specifies uncompressed point serialization for the P curves (https://datatracker.ietf.org/doc/html/rfc9180#section-7.1.1). It seemed reasonable to just use the same format for the signature public key as well. It would be intuitive if there were corresponding functions to load and validate the keys for their respective use cases - even if the exact same logic is running under the hood. |
@FiloSottile I generate the key using the Terraform If there was a function to directly parse it into an |
@ThadThompson yeah I think there's a good argument for uncompressed point to
@mrwonko, I am confused, in #63963 (comment) you were looking for crypto/ecdh -> crypto/ecdsa. Is what you are doing ECDH or ECDSA? |
The proposed parsing API is
Should we add MarshalPublicKey too, a PublicKey.Marshal method, or let clients use PublicKey.ECDH.Bytes? I'm not a fan of PublicKey.Marshal because it would encourage using that over x509.MarshalPKIXPublicKey, and this is not a common encoding. MarshalPublicKey is a little better because it's less immediately discoverable. /cc @golang/proposal-review |
As I've seen it described in various places as one of 'SECG', 'X963' or 'raw' format, having that comment is significantly helpful to anyone coming from a different system. I don't have a strong feeling on it, but if I came along and saw |
Sorry for the confusion, I was looking at the wrong use case. This is for webpush-go, which needs to turn incoming bytes into an |
elliptic.Marshal was deprecated, we can replace it with the ECDH methods even though we aren't using ECDH here. See: - golang/go@f03fb14 We still using elliptic.Unmarshal because this issue needs to be resolved: - golang/go#63963
elliptic.Marshal was deprecated, we can replace it with the ECDH methods even though we aren't using ECDH here. See: - golang/go@f03fb14 We still using elliptic.Unmarshal because this issue needs to be resolved: - golang/go#63963
The proposed |
I looked into Web Push a bit further, to figure out if the private key raw encoding is necessary, and I don't think it is. RFC 8292, Section 3.2 does indeed use the raw encoding for ECDSA public keys, but there is no specified format for the private key, so I think implementations should just use the standard PKCS#8. https://datatracker.ietf.org/doc/html/rfc8292#section-3.2 (Also note that all the other operations in Web Push aside from the VAPID signature can be done with crypto/ecdh. The spec is clear that "An application server MUST select a different private key for the key exchange [RFC8291] and signing the authentication token." so there is no need for conversions.) Looks like Nebula exposed both raw private and public key encoding for ECDSA, though. Overall, one RFC and one proprietary protocol are not indicative of widespread adoption of the raw key format for ECDSA. Personally, I might actually prefer it over PKIX / PKCS#8, but since the latter is predominant, I think deprecated functions might actually be sufficient as support: they give a clear signal that this is not the common path. I'm leaning towards not implementing this, but happy to keep it open to collect more feedback. |
elliptic.Marshal was deprecated, we can replace it with the ECDH methods even though we aren't using ECDH here. See: - golang/go@f03fb14 We still using elliptic.Unmarshal because this issue needs to be resolved: - golang/go#63963
Also RFC 9420 - The Messaging Layer Security (MLS) Protocol |
I'm working with ECDSA keys and facing now the elliptic.Marshal() deprecation. I've tried the ecdsa.PublicKey.ECDH().Bytes() workaround, but is not enough: the elliptic package has P224 curve and ecdh package has not, so it's not working for ECDSA_P224 keys. I think a more complete solution like the proposed here is needed. |
Ok, I think there is critical mass for the public key encoding: MLS, TLS, ACVP all use it, and we want to move folks away from I had a bit of time to think about it, and I like New proposed API
The private keys are a different tradeoff. Raw private keys are less common, and not as well defined (e.g. are leading zeroes required?), but also more problematic to implement outside the library: the user needs to do a ScalarBaseMult with crypto/elliptic to generate the public portion. Should we add similar |
@FiloSottile Sorry to drag this one up again but I have been caught up in this one as I was recently looking through the codebase for So I thought I would be nice and submit a PR to fix it but I've spent hours trying to find a combination of I'm left thinking there's a simple way I've missed but I've re-read the stdlib API a bazillion times and I think if there is something its not exposed or has not been written ? |
Unfortunately, JWK just made poor decisions. Keys should be treated as bytes strings, which are decoded and encoded according to a specification. Crypto libraries are then charged with encoding and decoding keys from these specified formats, performing validation in the process. We support the common PKIX format, and this issue is about supporting the metadata-less SEC 1 format, which would have been a perfectly fine encoding for JWK to use. JWK choose to represent them as... I am not even sure? I think x and y coordinates as big-endian integers encoded with base64url (with or without leading zeroes?). It's not in RFC 7517, but that's what the examples look like. No modern API should accept x and y coordinates as integers. That's how you get invalid curve attacks. So it's to be expected that using JWK would require reencoding the keys in a standard format before using them. I think the code you link to should be able to pass |
Interesting, and that's certainly one construct I had not tried. However whilst on my earlier quest to self-resolve this, I stumbled accross this SO post which suggests that for P-521, neither X or Y are fixed length and therefore I don't know what I'm padding to ? I'm also going to test against an alternative jwk library incase its an issue with that particular one. |
Pad P-521 to 66 bytes. It's only variable length (like all other curves, just more likely) when using generic functions like math/big.Int.Bytes, which is indeed why we want to steer users away from interacting with the unfortunately exposed PublicKey.X and PublicKey.Y values. That's ECDSA, though. For ECDH the crypto/ecdh API is fixed length. |
Thanks @FiloSottile. I've learnt a lot today ! I won't drag this issue any more off-topic, not least because having done some backround reading on what was being implemented on that Go repo I was looking to submit a PR to, I don't think there's escaping the X,Y and |
I reflected a bit more on this, and realized that we can deprecate the X and Y fields after we introduce these APIs, which should reach all the unsafe uses of these @udf2457, no worries, the conversation was useful to realize the above! That does look low level enough that you'll need the nistec package (filippo.io/nistec) which is the backend of crypto/ecdsa and crypto/ecdh. It uses the same SEC 1 format, so my recommendation to reencode JWK as SEC 1 stands. If any JOSE library maintainer is reading, I think that's what you should do both internally and externally, rather than touch X and Y or having your users do it: reencode as SEC 1 and expose that as |
Given that there are multiple formats, can we make that part of the name? E.g., Otherwise, this sounds good. Since we want for 1.25, and there hasn't been recent discussion, I think we can move this to likely accept, and take the week the decide which name to use. |
No strong opinion, I went for |
Do we need to wait a cycle before deprecating |
Based on the discussion above, this proposal seems like a likely accept. The proposal is to add support for decoding and encoding the uncompressed format of ECDSA keys, and deprecating package crypto/ecdsa
// ParseUncompressedPublicKey parses a public key encoded as an uncompressed point according
// to SEC 1, Version 2.0, Section 2.3.3 (also known as the X9.62 uncompressed
// format). It returns an error if the point is not in uncompressed form, is not
// on the curve, or is the point at infinity.
//
// curve must be one of [elliptic.P224], [elliptic.P256], [elliptic.P384], or
// [elliptic.P521], or ParseUncompressedPublicKey returns an error.
//
// ParseUncompressedPublicKey accepts the same format as [ecdh.Curve.NewPublicKey] does for
// NIST curves, but returns a [PublicKey] instead of an [ecdh.PublicKey].
//
// Note that public keys are more commonly encoded in DER (or PEM) format, which
// can be parsed with [x509.ParsePKIXPublicKey] (and [encoding/pem]).
func ParseUncompressedPublicKey(curve elliptic.Curve, data []byte) (*PublicKey, error)
// Bytes encodes the public key as an uncompressed point according to SEC 1,
// Version 2.0, Section 2.3.3 (also known as the X9.62 uncompressed format).
// It returns an error if the public key is invalid.
//
// PublicKey.Curve must be one of [elliptic.P224], [elliptic.P256],
// [elliptic.P384], or [elliptic.P521], or Bytes returns an error.
//
// Bytes returns the same format as [ecdh.PublicKey.Bytes] does for NIST curves.
//
// Note that public keys are more commonly encoded in DER (or PEM) format, which
// can be generated with [x509.MarshalPKIXPublicKey] (and [encoding/pem]).
func (*PublicKey) Bytes() ([]byte, error)
type PublicKey struct {
elliptic.Curve
// X, Y are the coordinates of the public key point.
//
// Deprecated: modifying the raw coordinates can produce invalid keys, and
// may invalidate internal optimizations; moreover, [big.Int] methods are
// not suitable for operating on cryptographic values. To encode and decode
// PublicKey values, use [PublicKey.Bytes] and [ParseUncompressedPublicKey] (or
// [x509.MarshalPKIXPublicKey] and [x509.ParsePKIXPublicKey]). For ECDH, use
// [crypto/ecdh]. For lower-level elliptic curve operations, use a
// third-party module like filippo.io/nistec.
X, Y *big.Int
} |
No change in consensus, so accepted. 🎉 The proposal is to add support for decoding and encoding the uncompressed format of ECDSA keys, and deprecating package crypto/ecdsa
// ParseUncompressedPublicKey parses a public key encoded as an uncompressed point according
// to SEC 1, Version 2.0, Section 2.3.3 (also known as the X9.62 uncompressed
// format). It returns an error if the point is not in uncompressed form, is not
// on the curve, or is the point at infinity.
//
// curve must be one of [elliptic.P224], [elliptic.P256], [elliptic.P384], or
// [elliptic.P521], or ParseUncompressedPublicKey returns an error.
//
// ParseUncompressedPublicKey accepts the same format as [ecdh.Curve.NewPublicKey] does for
// NIST curves, but returns a [PublicKey] instead of an [ecdh.PublicKey].
//
// Note that public keys are more commonly encoded in DER (or PEM) format, which
// can be parsed with [x509.ParsePKIXPublicKey] (and [encoding/pem]).
func ParseUncompressedPublicKey(curve elliptic.Curve, data []byte) (*PublicKey, error)
// Bytes encodes the public key as an uncompressed point according to SEC 1,
// Version 2.0, Section 2.3.3 (also known as the X9.62 uncompressed format).
// It returns an error if the public key is invalid.
//
// PublicKey.Curve must be one of [elliptic.P224], [elliptic.P256],
// [elliptic.P384], or [elliptic.P521], or Bytes returns an error.
//
// Bytes returns the same format as [ecdh.PublicKey.Bytes] does for NIST curves.
//
// Note that public keys are more commonly encoded in DER (or PEM) format, which
// can be generated with [x509.MarshalPKIXPublicKey] (and [encoding/pem]).
func (*PublicKey) Bytes() ([]byte, error)
type PublicKey struct {
elliptic.Curve
// X, Y are the coordinates of the public key point.
//
// Deprecated: modifying the raw coordinates can produce invalid keys, and
// may invalidate internal optimizations; moreover, [big.Int] methods are
// not suitable for operating on cryptographic values. To encode and decode
// PublicKey values, use [PublicKey.Bytes] and [ParseUncompressedPublicKey] (or
// [x509.MarshalPKIXPublicKey] and [x509.ParsePKIXPublicKey]). For ECDH, use
// [crypto/ecdh]. For lower-level elliptic curve operations, use a
// third-party module like filippo.io/nistec.
X, Y *big.Int
} |
I'm working with the JS Web Push API to send push messages, which uses an ECDH Key in base64-encoded X9.62 uncompressed form, like
BJ932huv68tUDxifpf6qlzuRa_JBF-2E9J47alSQRuxpmt3QFtiCnhqXlPgZuGWKZzcp5jAzQYeHpMUB990JiHM
. I can easily parse that usingecdh.P256().NewPublicKey(base64DecodedBytes)
, but that yields an*ecdh.PublicKey
, while all the cryptography functions need an*ecdsa.PublicKey
.There appears to be no straightforward conversion from
*ecdh.PublicKey
to*ecdsa.PublicKey
, the best thing I've found is an x509 PKIX roundtrip:That feels clunky, inefficient, and I lose type safety / need a type assertion. For the inverse conversion, there's already an
ecdsa.PublicKey.ECDH()
function. I propose adding something similar for ecdh to ecdsa conversion, both for public and private keys. I imagine it will have to be added to the ecdsa package to avoid cyclic imports.The text was updated successfully, but these errors were encountered: