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

proposal: crypto/ecdsa: add UnmarshalPublicKey #63963

Open
mrwonko opened this issue Nov 6, 2023 · 18 comments
Open

proposal: crypto/ecdsa: add UnmarshalPublicKey #63963

mrwonko opened this issue Nov 6, 2023 · 18 comments
Labels
Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@mrwonko
Copy link

mrwonko commented Nov 6, 2023

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.

@gopherbot gopherbot added this to the Proposal milestone Nov 6, 2023
@mateusz834
Copy link
Member

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
Copy link
Author

mrwonko commented Nov 6, 2023

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
Copy link
Member

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
Copy link
Author

mrwonko commented Nov 6, 2023

Gave that a test, seems to work, thanks!

@mateusz834
Copy link
Member

mateusz834 commented Nov 6, 2023

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 seankhliao added the Proposal-Crypto Proposal related to crypto packages or other security issues label Nov 6, 2023
@seankhliao
Copy link
Member

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

@ThadThompson
Copy link

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
Copy link
Author

mrwonko commented Jan 22, 2024

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
Copy link
Member

@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
Copy link
Contributor

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
Copy link

@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
Copy link
Author

mrwonko commented Jan 23, 2024

@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
Copy link
Contributor

@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 #63963 (comment) you were looking for crypto/ecdh -> crypto/ecdsa. Is what you are doing ECDH or ECDSA?

@FiloSottile
Copy link
Contributor

FiloSottile commented Jan 24, 2024

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

@FiloSottile FiloSottile changed the title proposal: crypto/ecdsa: conversion from ecdh Public/PrivateKey proposal: crypto/ecdsa: add UnmarshalPublicKey Jan 24, 2024
@ThadThompson
Copy link

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
Copy link
Author

mrwonko commented Jan 25, 2024

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 added a commit to slackhq/nebula that referenced this issue Apr 3, 2024
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
wadey added a commit to slackhq/nebula that referenced this issue Apr 3, 2024
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
@wadey
Copy link
Contributor

wadey commented Apr 4, 2024

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

@FiloSottile
Copy link
Contributor

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.

wadey added a commit to slackhq/nebula that referenced this issue Apr 30, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
Status: Incoming
Development

No branches or pull requests

7 participants