[v16] fix: tolerate mismatched key PEM headers (#46727)

Backport #46725 to branch/v16

Issue #43381 introduced a regression where we now fail to parse PKCS8
encoded RSA private keys within an "RSA PRIVATE KEY" PEM block in
some cases.
This format is somewhat non-standard, usually PKCS8 data should be in a
"PRIVATE KEY" PEM block. However, certain versions of OpenSSL and
possibly even Teleport in specific cases have generated private keys in
this format.

This commit updates ParsePrivateKey and ParsePublicKey to be more
tolerant of PKCS8, PKCS1, or PKIX key data no matter which PEM header is
used.

changelog: fixed regression in private key parser to handle mismatched PEM headers

api/utils/keys/privatekey.go

@@ -38,6 +38,7 @@ const (
 	PKCS1PrivateKeyType      = "RSA PRIVATE KEY"
 	PKCS8PrivateKeyType      = "PRIVATE KEY"
 	ECPrivateKeyType         = "EC PRIVATE KEY"
+	OpenSSHPrivateKeyType    = "OPENSSH PRIVATE KEY"
 	pivYubiKeyPrivateKeyType = "PIV YUBIKEY PRIVATE KEY"
 )
 
@@ -161,34 +162,54 @@ func ParsePrivateKey(keyPEM []byte) (*PrivateKey, error) {
 	}
 
 	switch block.Type {
-	case PKCS1PrivateKeyType:
-		cryptoSigner, err := x509.ParsePKCS1PrivateKey(block.Bytes)
-		if err != nil {
-			return nil, trace.Wrap(err)
-		}
-		return NewPrivateKey(cryptoSigner, keyPEM)
-	case ECPrivateKeyType:
-		cryptoSigner, err := x509.ParseECPrivateKey(block.Bytes)
-		if err != nil {
-			return nil, trace.Wrap(err)
-		}
-		return NewPrivateKey(cryptoSigner, keyPEM)
-	case PKCS8PrivateKeyType:
-		priv, err := x509.ParsePKCS8PrivateKey(block.Bytes)
+	case pivYubiKeyPrivateKeyType:
+		priv, err := parseYubiKeyPrivateKeyData(block.Bytes)
+		return priv, trace.Wrap(err, "parsing YubiKey private key")
+	case OpenSSHPrivateKeyType:
+		priv, err := ssh.ParseRawPrivateKey(keyPEM)
 		if err != nil {
 			return nil, trace.Wrap(err)
 		}
 		cryptoSigner, ok := priv.(crypto.Signer)
 		if !ok {
-			return nil, trace.BadParameter("x509.ParsePKCS8PrivateKey returned an invalid private key of type %T", priv)
+			return nil, trace.BadParameter("ssh.ParseRawPrivateKey returned an invalid private key of type %T", priv)
+		}
+		// For some reason ssh.ParseRawPrivateKey returns a *ed25519.PrivateKey
+		// instead of the plain ed25519.PrivateKey which is used everywhere
+		// else. This breaks comparisons and type switches, so explicitly convert it.
+		if pEdwards, ok := cryptoSigner.(*ed25519.PrivateKey); ok {
+			cryptoSigner = *pEdwards
 		}
 		return NewPrivateKey(cryptoSigner, keyPEM)
-	case pivYubiKeyPrivateKeyType:
-		priv, err := parseYubiKeyPrivateKeyData(block.Bytes)
-		if err != nil {
-			return nil, trace.Wrap(err, "failed to parse YubiKey private key")
+	case PKCS1PrivateKeyType, PKCS8PrivateKeyType, ECPrivateKeyType:
+		// The DER format doesn't always exactly match the PEM header, various
+		// versions of Teleport and OpenSSL have been guilty of writing PKCS#8
+		// data into an "RSA PRIVATE KEY" block or vice-versa, so we just try
+		// parsing every DER format. This matches the behavior of [tls.X509KeyPair].
+		var preferredErr error
+		if priv, err := x509.ParsePKCS8PrivateKey(block.Bytes); err == nil {
+			signer, ok := priv.(crypto.Signer)
+			if !ok {
+				return nil, trace.BadParameter("x509.ParsePKCS8PrivateKey returned an invalid private key of type %T", priv)
+			}
+			return NewPrivateKey(signer, keyPEM)
+		} else if block.Type == PKCS8PrivateKeyType {
+			preferredErr = err
+		}
+		if signer, err := x509.ParsePKCS1PrivateKey(block.Bytes); err == nil {
+			return NewPrivateKey(signer, keyPEM)
+		} else if block.Type == PKCS1PrivateKeyType {
+			preferredErr = err
+		}
+		if signer, err := x509.ParseECPrivateKey(block.Bytes); err == nil {
+			return NewPrivateKey(signer, keyPEM)
+		} else if block.Type == ECPrivateKeyType {
+			preferredErr = err
 		}
-		return priv, nil
+		// If all three parse functions returned an error, preferedErr is
+		// guaranteed to be set to the error from the parse function that
+		// usually matches the PEM block type.
+		return nil, trace.Wrap(preferredErr, "parsing private key PEM")
 	default:
 		return nil, trace.BadParameter("unexpected private key PEM type %q", block.Type)
 	}

api/utils/keys/privatekey_test.go

@@ -33,7 +33,7 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func TestMarshalAndParsePrivateKey(t *testing.T) {
+func TestMarshalAndParseKey(t *testing.T) {
 	rsaKey, err := rsa.GenerateKey(rand.Reader, 1024)
 	require.NoError(t, err)
 	ecKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
@@ -52,6 +52,125 @@ func TestMarshalAndParsePrivateKey(t *testing.T) {
 			gotKey, err := ParsePrivateKey(keyPEM)
 			require.NoError(t, err)
 			require.Equal(t, key, gotKey.Signer)
+
+			pubKeyPEM, err := MarshalPublicKey(key.Public())
+			require.NoError(t, err)
+			gotPubKey, err := ParsePublicKey(pubKeyPEM)
+			require.NoError(t, err)
+			require.Equal(t, key.Public(), gotPubKey)
+		})
+	}
+}
+
+func TestParseMismatchedPEMHeader(t *testing.T) {
+	rsaKey, err := ParsePrivateKey(rsaKeyPEM)
+	require.NoError(t, err)
+	rsaPKCS1DER := x509.MarshalPKCS1PrivateKey(rsaKey.Signer.(*rsa.PrivateKey))
+	rsaPKCS8DER, err := x509.MarshalPKCS8PrivateKey(rsaKey.Signer)
+	require.NoError(t, err)
+	rsaPublicPKCS1DER := x509.MarshalPKCS1PublicKey(rsaKey.Public().(*rsa.PublicKey))
+	rsaPublicPKIXDER, err := x509.MarshalPKIXPublicKey(rsaKey.Public())
+	require.NoError(t, err)
+
+	ecdsaKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	require.NoError(t, err)
+	ecdsaPKCS8DER, err := x509.MarshalPKCS8PrivateKey(ecdsaKey)
+	require.NoError(t, err)
+	ecdsaECDER, err := x509.MarshalECPrivateKey(ecdsaKey)
+	require.NoError(t, err)
+
+	for desc, tc := range map[string]struct {
+		pem       []byte
+		expectKey crypto.Signer
+	}{
+		"PKCS1 DER in PRIVATE KEY PEM": {
+			pem: pem.EncodeToMemory(&pem.Block{
+				Type:  "PRIVATE KEY",
+				Bytes: rsaPKCS1DER,
+			}),
+			expectKey: rsaKey.Signer,
+		},
+		"RSA PKCS8 DER in RSA PRIVATE KEY PEM": {
+			pem: pem.EncodeToMemory(&pem.Block{
+				Type:  "RSA PRIVATE KEY",
+				Bytes: rsaPKCS8DER,
+			}),
+			expectKey: rsaKey.Signer,
+		},
+		"ECDSA PKCS8 DER in EC PRIVATE KEY PEM": {
+			pem: pem.EncodeToMemory(&pem.Block{
+				Type:  "EC PRIVATE KEY",
+				Bytes: ecdsaPKCS8DER,
+			}),
+			expectKey: ecdsaKey,
+		},
+		"EC DER in PRIVATE KEY PEM": {
+			pem: pem.EncodeToMemory(&pem.Block{
+				Type:  "PRIVATE KEY",
+				Bytes: ecdsaECDER,
+			}),
+			expectKey: ecdsaKey,
+		},
+	} {
+		t.Run(desc, func(t *testing.T) {
+			key, err := ParsePrivateKey(tc.pem)
+			require.NoError(t, err)
+			require.Equal(t, tc.expectKey, key.Signer)
+		})
+	}
+
+	for desc, tc := range map[string]struct {
+		pem       []byte
+		expectKey crypto.PublicKey
+	}{
+		"PKCS1 DER in PUBLIC KEY PEM": {
+			pem: pem.EncodeToMemory(&pem.Block{
+				Type:  "PUBLIC KEY",
+				Bytes: rsaPublicPKCS1DER,
+			}),
+			expectKey: rsaKey.Public(),
+		},
+		"PKIX DER in RSA PUBLIC KEY PEM": {
+			pem: pem.EncodeToMemory(&pem.Block{
+				Type:  "RSA PUBLIC KEY",
+				Bytes: rsaPublicPKIXDER,
+			}),
+			expectKey: rsaKey.Public(),
+		},
+	} {
+		t.Run(desc, func(t *testing.T) {
+			pubKey, err := ParsePublicKey(tc.pem)
+			require.NoError(t, err)
+			require.Equal(t, tc.expectKey, pubKey)
+		})
+	}
+}
+
+// TestParseCorruptedKey tests that we actually return an error and don't panic
+// when parsing some trivially corrupted key PEMs. This is mostly to validate
+// that the preferredErr logic in Parse(Private|Public)Key returns an error for
+// each PEM type.
+func TestParseCorruptedKey(t *testing.T) {
+	for _, tc := range []string{
+		"RSA PRIVATE KEY",
+		"PRIVATE KEY",
+		"EC PRIVATE KEY",
+	} {
+		t.Run(tc, func(t *testing.T) {
+			b := pem.EncodeToMemory(&pem.Block{Type: tc, Bytes: []byte("foo")})
+			_, err := ParsePrivateKey(b)
+			require.Error(t, err)
+		})
+	}
+
+	for _, tc := range []string{
+		"RSA PUBLIC KEY",
+		"PUBLIC KEY",
+	} {
+		t.Run(tc, func(t *testing.T) {
+			b := pem.EncodeToMemory(&pem.Block{Type: tc, Bytes: []byte("foo")})
+			_, err := ParsePublicKey(b)
+			require.Error(t, err)
 		})
 	}
 }

api/utils/keys/publickey.go

@@ -66,24 +66,26 @@ func ParsePublicKey(keyPEM []byte) (crypto.PublicKey, error) {
 	}
 
 	switch block.Type {
-	case PKCS1PublicKeyType:
-		pub, pkcs1Err := x509.ParsePKCS1PublicKey(block.Bytes)
-		if pkcs1Err != nil {
-			// Failed to parse as PKCS#1. We have been known to stuff PKIX DER encoded RSA public keys into
-			// "RSA PUBLIC KEY" PEM blocks, so try to parse as PKIX.
-			pub, pkixErr := x509.ParsePKIXPublicKey(block.Bytes)
-			if pkixErr != nil {
-				// Parsing as both formats failed. We really should expect PKCS#1 in this PEM block, so return
-				// that error.
-				return nil, trace.Wrap(pkcs1Err)
-			}
-			return pub, nil
-		}
-		return pub, nil
-	case PKIXPublicKeyType:
-		pub, err := x509.ParsePKIXPublicKey(block.Bytes)
-		return pub, trace.Wrap(err)
+	case PKCS1PublicKeyType, PKIXPublicKeyType:
 	default:
 		return nil, trace.BadParameter("unsupported public key type %q", block.Type)
 	}
+
+	// We have been known to stuff PKIX DER encoded RSA public keys into
+	// "RSA PUBLIC KEY" PEM blocks, so just try to parse either.
+	var preferredErr error
+	if pub, err := x509.ParsePKIXPublicKey(block.Bytes); err == nil {
+		return pub, nil
+	} else if block.Type == PKIXPublicKeyType {
+		preferredErr = err
+	}
+	if pub, err := x509.ParsePKCS1PublicKey(block.Bytes); err == nil {
+		return pub, nil
+	} else if block.Type == PKCS1PublicKeyType {
+		preferredErr = err
+	}
+	// If both parse functions returned an error, preferedErr is guaranteed to
+	// be set to the error from the parse function that usually matches the PEM
+	// block type.
+	return nil, trace.Wrap(preferredErr, "parsing public key PEM")
 }