Fix PEM armor for EC private keys when encoding (#876)

* Incorporate #875

* Test PEM roundtrip for other key types

* Use more constants

jwk/jwk.go

@@ -234,8 +234,11 @@ func PublicRawKeyOf(v interface{}) (interface{}, error) {
 }
 
 const (
-	pmPrivateKey = `PRIVATE KEY`
-	pmPublicKey  = `PUBLIC KEY`
+	pmPrivateKey    = `PRIVATE KEY`
+	pmPublicKey     = `PUBLIC KEY`
+	pmECPrivateKey  = `EC PRIVATE KEY`
+	pmRSAPublicKey  = `RSA PUBLIC KEY`
+	pmRSAPrivateKey = `RSA PRIVATE KEY`
 )
 
 // EncodeX509 encodes the key into a byte sequence in ASN.1 DER format
@@ -263,13 +266,13 @@ func EncodeX509(v interface{}) (string, []byte, error) {
 	// Try to convert it into a certificate
 	switch v := v.(type) {
 	case *rsa.PrivateKey:
-		return "RSA PRIVATE KEY", x509.MarshalPKCS1PrivateKey(v), nil
+		return pmRSAPrivateKey, x509.MarshalPKCS1PrivateKey(v), nil
 	case *ecdsa.PrivateKey:
 		marshaled, err := x509.MarshalECPrivateKey(v)
 		if err != nil {
 			return "", nil, err
 		}
-		return "ECDSA PRIVATE KEY", marshaled, nil
+		return pmECPrivateKey, marshaled, nil
 	case ed25519.PrivateKey:
 		marshaled, err := x509.MarshalPKCS8PrivateKey(v)
 		if err != nil {
@@ -316,19 +319,19 @@ func DecodePEM(src []byte) (interface{}, []byte, error) {
 
 	switch block.Type {
 	// Handle the semi-obvious cases
-	case "RSA PRIVATE KEY":
+	case pmRSAPrivateKey:
 		key, err := x509.ParsePKCS1PrivateKey(block.Bytes)
 		if err != nil {
 			return nil, nil, fmt.Errorf(`failed to parse PKCS1 private key: %w`, err)
 		}
 		return key, rest, nil
-	case "RSA PUBLIC KEY":
+	case pmRSAPublicKey:
 		key, err := x509.ParsePKCS1PublicKey(block.Bytes)
 		if err != nil {
 			return nil, nil, fmt.Errorf(`failed to parse PKCS1 public key: %w`, err)
 		}
 		return key, rest, nil
-	case "EC PRIVATE KEY":
+	case pmECPrivateKey:
 		key, err := x509.ParseECPrivateKey(block.Bytes)
 		if err != nil {
 			return nil, nil, fmt.Errorf(`failed to parse EC private key: %w`, err)

jwk/jwk_test.go

@@ -214,28 +214,65 @@ func VerifyKey(t *testing.T, def map[string]keyDef) {
 		}
 	})
 	t.Run("Roundtrip", func(t *testing.T) {
-		buf, err := json.Marshal(key)
-		if !assert.NoError(t, err, `json.Marshal should succeed`) {
-			return
+		var supportsPEM bool
+		switch key.KeyType() {
+		case jwa.OKP, jwa.OctetSeq:
+		default:
+			supportsPEM = true
 		}
 
-		newkey, err := jwk.ParseKey(buf)
-		if !assert.NoError(t, err, `jwk.ParseKey should succeed`) {
-			return
-		}
+		for _, usePEM := range []bool{true, false} {
+			if usePEM && !supportsPEM {
+				continue
+			}
+			t.Run(fmt.Sprintf("WithPEM(%t)", usePEM), func(t *testing.T) {
+				var buf []byte
+				if usePEM {
+					pem, err := jwk.EncodePEM(key)
+					if !assert.NoError(t, err, `jwk.EncodePEM should succeed`) {
+						return
+					}
+					buf = pem
+				} else {
+					jsonbuf, err := json.Marshal(key)
+					if !assert.NoError(t, err, `json.Marshal should succeed`) {
+						return
+					}
+					buf = jsonbuf
+				}
 
-		m1, err := key.AsMap(context.TODO())
-		if !assert.NoError(t, err, `key.AsMap should succeed`) {
-			return
-		}
+				newkey, err := jwk.ParseKey(buf, jwk.WithPEM(usePEM))
+				if !assert.NoError(t, err, `jwk.ParseKey should succeed`) {
+					return
+				}
 
-		m2, err := newkey.AsMap(context.TODO())
-		if !assert.NoError(t, err, `key.AsMap should succeed`) {
-			return
-		}
+				m1, err := key.AsMap(context.TODO())
+				if !assert.NoError(t, err, `key.AsMap should succeed`) {
+					return
+				}
 
-		if !assert.Equal(t, m1, m2, `keys should match`) {
-			return
+				m2, err := newkey.AsMap(context.TODO())
+				if !assert.NoError(t, err, `key.AsMap should succeed`) {
+					return
+				}
+
+				// PEM does not preserve these keys
+				if usePEM {
+					delete(m1, `private`)
+					delete(m1, jwk.AlgorithmKey)
+					delete(m1, jwk.KeyIDKey)
+					delete(m1, jwk.KeyOpsKey)
+					delete(m1, jwk.KeyUsageKey)
+					delete(m1, jwk.X509CertChainKey)
+					delete(m1, jwk.X509CertThumbprintKey)
+					delete(m1, jwk.X509CertThumbprintS256Key)
+					delete(m1, jwk.X509URLKey)
+				}
+
+				if !assert.Equal(t, m1, m2, `keys should match`) {
+					return
+				}
+			})
 		}
 	})
 	t.Run("Raw", func(t *testing.T) {
@@ -2070,3 +2107,26 @@ func TestGH730(t *testing.T) {
 	require.NoError(t, set.AddKey(key), `first AddKey should succeed`)
 	require.Error(t, set.AddKey(key), `second AddKey should fail`)
 }
+
+// This test was lifted from #875. See tests under Roundtrip/WithPEM(true) for other key types
+func TestECDSAPEM(t *testing.T) {
+	// go make an EC key at https://mkjwk.org/
+	key, err := jwk.ParseKey([]byte(`{
+		"kty": "EC",
+		"d": "zqYPTs5gMEwtidOqjlFJSk6L4BQSfhCJX6FTgbuuiE0",
+		"crv": "P-256",
+		"x": "AYwhwiE1hXWdfwu-HlBSsY5Chxycu-LyE6WsZ_w2DO4",
+		"y": "zumemGclMFkimMsKMXlLdKYWtLle58e4N9hDPcN7lig"
+	}`))
+	if err != nil {
+		t.Fatal(err)
+	}
+	pem, err := jwk.EncodePEM(key)
+	if err != nil {
+		t.Fatal(err)
+	}
+	_, err = jwk.ParseKey(pem, jwk.WithPEM(true))
+	if err != nil {
+		t.Fatal(err)
+	}
+}