proposal: crypto/ecdsa: add UnmarshalPublicKey

[mrwonko]

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 using ecdh.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:

func ecdhPublicKeyToECDSA(key *ecdh.PublicKey) (*ecdsa.PublicKey, error) {
    b, err := x509.MarshalPKIXPublicKey(key)
    if err != nil {
        return nil, fmt.Errorf("marshaling ecdh public key: %w", err)
    }
    parsedKey, err := x509.ParsePKIXPublicKey(b)
    if err != nil {
        return nil, fmt.Errorf("parsing marshaled ecdh public key: %w", err)
    }
    res, ok := parsedKey.(*ecdsa.PublicKey)
    if !ok {
        return nil, fmt.Errorf("x509.ParsePKIXPublicKey returned %T instead of *ecdsa.PublicKey", parsedKey)
    }
    return res, nil
}

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.

[mateusz834]

You can use the Bytes method on *ecdh.PublicKey to convert it to *ecdsa.PublicKey, like so:

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.

[mrwonko]

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?

[mateusz834]

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()),
}

[mrwonko]

Gave that a test, seems to work, thanks!

[mateusz834]

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 ecdh.Curve.NewPrivateKey, ecdh.Curve.NewPublicKey.

package ecdsa // crypto/ecdsa
func NewPrivateKey(curve elliptic.Curve, raw []byte) (*ecdsa.PrivateKey, error)
func NewPublicKey(curve elliptic.Curve, raw []byte) (*ecdsa.PublicKey, error)

[seankhliao]

cc @golang/security see also #56088 which added the conversion in the other direction

[ThadThompson]

Hey @FiloSottile could you offer guidance on this? I also have an uncompressed point public key for ECDSA, and would reach for elliptic.Unmarshal, but it's been deprecated to push people toward the new method in ECDH.

[mrwonko]

I think this proposal looks like this now:

// It panics when the [*ecdh.PrivateKey.Curve] is not one of [ecdh.P256], 
// [ecdh.P384], [ecdh.P521].

Looking at this again, an error might be more idiomatic than a panic.

@ThadThompson sounds like you also need ecdh.P256().NewPublicKey() followed by the ecdhToECDSAPublicKey() function posted above.

[mateusz834]

@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)

[FiloSottile]

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 elliptic.Unmarshal or ecdh.P256().NewPublicKey) to crypto/ecdsa keys, is that correct?

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?

[ThadThompson]

@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.

[mrwonko]

@FiloSottile I generate the key using the Terraform tls_private_key resource (algorithm="ECDSA", ecdsa_curve="P256"), so I end up with an "EC PRIVATE KEY" PEM Block which I first pem.Decode() and then x509.ParseECPrivateKey. The result is a *ecdsa.PrivateKey, which I then convert to an *ecdh.PrivateKey as outlined above.

If there was a function to directly parse it into an *ecdh.PrivateKey, that would indeed also solve my issue, without encouraging dangerous key re-use.

[FiloSottile]

@ThadThompson yeah I think there's a good argument for uncompressed point to *ecdsa.PublicKey parsing, to replace the deprecated elliptic.Unmarshal. I'll repurpose this proposal.

If there was a function to directly parse it into an *ecdh.PrivateKey, that would indeed also solve my issue, without encouraging dangerous key re-use.

@mrwonko, I am confused, in https://github.com/golang/go/issues/63963#issue-1978832126 you were looking for crypto/ecdh -> crypto/ecdsa. Is what you are doing ECDH or ECDSA?

[FiloSottile]

The proposed parsing API is

package crypto/ecdsa 

// UnmarshalPublicKey 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.
//
// Note that public keys are more commonly encoded in DER or PEM format, which
// can be parsed with [crypto/x509.ParsePKIXPublicKey].
func UnmarshalPublicKey(curve elliptic.Curve, data []byte) (*PublicKey, error)

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

[ThadThompson]

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 UnmarshalPublicKey and MarshalPublicKey functions, I might assume they were the normal way to encode/decode keys, since they look less 'special' than x509.MarshalPKIXPublicKey (especially if they're in the ecdsa package). Whereas something like UnmarshalRawPublicKey/MarshalRawPublicKey would give me pause long enough to read the comment explaining the situation.

[mrwonko]

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 *ecdsa.PrivateKey for cryptography: https://github.com/SherClockHolmes/webpush-go/pull/60/files#diff-dbe7c041c9083465e1a0ae342ab62d150c039297ce888b47c5935968573606caR34-R44

[wadey]

The proposed ecdsa.UnmarshalPublicKey would be perfect to replace our use of the deprecated elliptic.Unmarshal:

• https://github.com/slackhq/nebula/blob/v1.8.2/cert/cert.go#L568-L569

[FiloSottile]

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.

[ThadThompson]

Also RFC 9420 - The Messaging Layer Security (MLS) Protocol https://www.rfc-editor.org/rfc/rfc9420.html#section-5.1.1-6