[Bug] confidential.CertFromPEM fails with Azure Key Vault: "failed to parse private key (use ParsePKCS1PrivateKey instead for this key format)"

[dagood]

Which version of MSAL Go are you using?
github.com/AzureAD/microsoft-authentication-library-for-go v1.2.2 (latest)

Where is the issue?
Confidential client - client certificate

microsoft-authentication-library-for-go/apps/confidential/confidential.go

Lines 98 to 116 in 882b562

	case "PRIVATE KEY":
	if priv != nil {
	return nil, nil, errors.New("found multiple private key blocks")
	}
	var err error
	priv, err = x509.ParsePKCS8PrivateKey(block.Bytes)
	if err != nil {
	return nil, nil, fmt.Errorf("could not decode private key: %v", err)
	}
	case "RSA PRIVATE KEY":
	if priv != nil {
	return nil, nil, errors.New("found multiple private key blocks")
	}
	var err error
	priv, err = x509.ParsePKCS1PrivateKey(block.Bytes)
	if err != nil {
	return nil, nil, fmt.Errorf("could not decode private key: %v", err)
	}

This code assumes that all PRIVATE KEY blocks can be parsed by ParsePKCS8PrivateKey and all RSA PRIVATE KEY blocks can be parsed by ParsePKCS1PrivateKey, but Azure Key Vault gives me a PRIVATE KEY block that requires ParsePKCS1PrivateKey.

As far as I know, the block type in PEM files is technically only a hint, not well-specified. I'm not sure whether this bug is really on Azure Key Vault or this library, but it seems reasonable for this library to support Azure Key Vault in practice.

Is this a new or an existing app?
New experiment.

What version of Go are you using (go version)?
go1.22.3

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
set GOARCH=amd64
set GOHOSTARCH=amd64
set GOHOSTOS=windows
...

Repro
Get a JSON blob from Azure Key Vault using az keyvault secret show [...]. Pass the string as bytes into this function to try to decode and set it up as a confidential.Credential that uses its cert:

func NewCredFromAzureKeyVaultJSON(vaultJSON []byte) (confidential.Credential, error) {
	fail := func(err string) (confidential.Credential, error) {
		return confidential.Credential{}, errors.New(err)
	}
	var data struct {
		Value string `json:"value"`
	}
	if err := json.Unmarshal(vaultJSON, &data); err != nil {
		return fail("unable to decode JSON")
	}
	pfx, err := base64.StdEncoding.DecodeString(data.Value)
	if err != nil {
		return fail("unable to decode base64 value")
	}
	blocks, err := pkcs12.ToPEM(pfx, "")
	if err != nil {
		return fail("unable to convert PFX data to PEM blocks")
	}
	// Multiple blocks are expected. Find the private key and certificates.
	var pemBuf bytes.Buffer
	for _, block := range blocks {
		err := pem.Encode(&pemBuf, block)
		if err != nil {
			return fail("unable to encode PEM block")
		}
	}
	certs, priv, err := confidential.CertFromPEM(pemBuf.Bytes(), "")
	if err != nil {
		return fail("unable to create cert from PEM blocks")
	}
	return confidential.NewCredFromCert(certs, priv)
}

I don't have an example cert or Azure Key Vault response ready at the moment, unfortuantely. I'm not sure how the one I'm using was created. This may only affect a certain kind of certificate.

Expected behavior
Returns a usable confidential.Credential.

Actual behavior
Get an error from confidential.CertFromPEM:

could not decode private key: x509: failed to parse private key (use ParsePKCS1PrivateKey instead for this key format)

This comes from the case "PRIVATE KEY": case, not "RSA PRIVATE KEY".

Possible solution
Replacing confidential.CertFromPEM with a custom copy with this change works, and the resulting confidential.Credential is usable:

 		case "PRIVATE KEY":
[...]
 			priv, err = x509.ParsePKCS8PrivateKey(block.Bytes)
+			// Also try ParsePKCS1PrivateKey, because this might not be compatible with ParsePKCS8PrivateKey.
+			if err != nil {
+ 				priv, err = x509.ParsePKCS1PrivateKey(block.Bytes)
+			}
 			if err != nil {
 				return nil, nil, fmt.Errorf("could not decode private key: %v", err)
 			}

(Keeping track of both errors and returning both might be better, rather than overwriting one with the other.)

[dagood]

Alternative workaround: modify blocks when iterating through it:

	// ...
	for _, block := range blocks {
		if block.Type == "PRIVATE KEY" {
			_, err = x509.ParsePKCS1PrivateKey(block.Bytes)
			if err == nil {
				block.Type = "RSA PRIVATE KEY"
			}
		}
		err := pem.Encode(&pemBuf, block)
		if err != nil {
			return fail("unable to encode PEM block")
		}
	}
	// ...