From 79f5604e479f607bc93c3e81b4fe76bf9e6e7753 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Tue, 20 Oct 2026 01:25:10 +0100 Subject: [PATCH] sec(oidc): check Azure Key Vault key type and curve before using it for ES256 resolveOnce read only X and Y from the Key Vault JWK and parsed them as a P-256 point, never looking at kty or crv. A P-384 or P-521 key failed with an opaque coordinate-length error, and a P-256K (secp256k1) key whose coordinates happened to parse on P-256 was accepted and would be published in the JWKS labeled as P-256. Require kty EC or EC-HSM and crv P-256 before building the public key, and name the key and the reported values in the error so the operator sees the misconfigured key instead of a downstream parse or sign failure. Closes #46 Co-Authored-By: Claude Opus 5.5 --- internal/oidc/azure_factory_test.go | 128 +++++++++++++++++++++++++++- internal/oidc/azure_signer.go | 29 +++++++ 2 files changed, 154 insertions(+), 3 deletions(-) diff --git a/internal/oidc/azure_factory_test.go b/internal/oidc/azure_factory_test.go index 6fdd50e8e..e46f5b5cc 100644 --- a/internal/oidc/azure_factory_test.go +++ b/internal/oidc/azure_factory_test.go @@ -21,7 +21,15 @@ type fakeAzureKeyVaultClient struct { // responses from the Key Vault API. xBytes []byte yBytes []byte - nilKey bool // if true, return a KeyBundle with Key==nil + // kty and crv override the key type and curve the fake reports. Empty + // means "derive from key": EC plus the key's curve name, which is what + // Key Vault returns for a software-protected EC key. omitKty and omitCrv + // report the field as absent (nil). + kty azkeys.KeyType + crv azkeys.CurveName + omitKty bool + omitCrv bool + nilKey bool // if true, return a KeyBundle with Key==nil } func (f *fakeAzureKeyVaultClient) Sign(_ context.Context, _, _ string, _ azkeys.SignParameters, _ *azkeys.SignOptions) (azkeys.SignResponse, error) { @@ -63,14 +71,38 @@ func (f *fakeAzureKeyVaultClient) GetKey(_ context.Context, _, _ string, _ *azke jwkY = yBytes } keyBundle := azkeys.JSONWebKey{ - X: jwkX, - Y: jwkY, + Kty: f.jwkKty(), + Crv: f.jwkCrv(), + X: jwkX, + Y: jwkY, } return azkeys.GetKeyResponse{ KeyBundle: azkeys.KeyBundle{Key: &keyBundle}, }, nil } +func (f *fakeAzureKeyVaultClient) jwkKty() *azkeys.KeyType { + if f.omitKty { + return nil + } + kty := f.kty + if kty == "" { + kty = azkeys.KeyTypeEC + } + return &kty +} + +func (f *fakeAzureKeyVaultClient) jwkCrv() *azkeys.CurveName { + if f.omitCrv { + return nil + } + crv := f.crv + if crv == "" { + crv = azkeys.CurveName(f.key.Curve.Params().Name) + } + return &crv +} + // --------------------------------------------------------------------------- // M6 -- Azure EC key completeness guard (replaces RSA exponent-range check) // --------------------------------------------------------------------------- @@ -124,6 +156,96 @@ func TestAzureSigner_ECKeyCompleteness(t *testing.T) { } } +// --------------------------------------------------------------------------- +// #46 -- Azure EC key type and curve guard +// --------------------------------------------------------------------------- + +// TestAzureSigner_RejectsNonP256Key verifies that the signer checks the key +// type and curve Key Vault reports before building the public key. Sign +// always requests ES256, so only an EC (or EC-HSM) P-256 key is usable. +// Without the check a P-256K (secp256k1) key whose 32-byte coordinates also +// parse on P-256 was accepted and later published in the JWKS labeled as +// P-256, and a P-384 key failed with an opaque coordinate-length error that +// did not name the misconfigured curve. +func TestAzureSigner_RejectsNonP256Key(t *testing.T) { + p256, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + p384, err := ecdsa.GenerateKey(elliptic.P384(), rand.Reader) + require.NoError(t, err) + + cases := []struct { + client *fakeAzureKeyVaultClient + name string + errSubstr []string + }{ + { + name: "EC P-256 accepted", + client: &fakeAzureKeyVaultClient{key: &p256.PublicKey}, + }, + { + name: "EC-HSM P-256 accepted", + client: &fakeAzureKeyVaultClient{key: &p256.PublicKey, kty: azkeys.KeyTypeECHSM}, + }, + { + name: "P-384 key rejected naming the curve", + client: &fakeAzureKeyVaultClient{key: &p384.PublicKey}, + errSubstr: []string{`crv="P-384"`, `kty="EC"`}, + }, + { + name: "P-521 curve rejected", + client: &fakeAzureKeyVaultClient{key: &p256.PublicKey, crv: azkeys.CurveNameP521}, + errSubstr: []string{`crv="P-521"`}, + }, + { + // The coordinates are a valid P-256 point, so only the + // reported curve shows the key is not usable for ES256. + name: "P-256K curve with P-256-valid coordinates rejected", + client: &fakeAzureKeyVaultClient{key: &p256.PublicKey, crv: azkeys.CurveNameP256K}, + errSubstr: []string{`crv="P-256K"`}, + }, + { + name: "missing curve rejected", + client: &fakeAzureKeyVaultClient{key: &p256.PublicKey, omitCrv: true}, + errSubstr: []string{"crv="}, + }, + { + name: "RSA key type rejected", + client: &fakeAzureKeyVaultClient{key: &p256.PublicKey, kty: azkeys.KeyTypeRSA}, + errSubstr: []string{`kty="RSA"`, `crv="P-256"`}, + }, + { + name: "missing key type rejected", + client: &fakeAzureKeyVaultClient{key: &p256.PublicKey, omitKty: true}, + errSubstr: []string{"kty="}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + signer := NewAzureKeyVaultSignerFromClient(tc.client, "test-key", "") + ctx := context.Background() + + pub, pubErr := signer.PublicKey(ctx) + kid, kidErr := signer.KeyID(ctx) + if len(tc.errSubstr) == 0 { + require.NoError(t, pubErr) + require.NoError(t, kidErr) + assert.NotNil(t, pub) + assert.NotEmpty(t, kid) + return + } + require.Error(t, pubErr) + assert.Nil(t, pub, "a rejected key must not be returned") + require.Error(t, kidErr, "KeyID must report the same rejection") + assert.Empty(t, kid) + for _, want := range tc.errSubstr { + assert.Contains(t, pubErr.Error(), want) + } + assert.Contains(t, pubErr.Error(), "test-key", "error must name the misconfigured key") + }) + } +} + // --------------------------------------------------------------------------- // M7 -- factory precise error for half-configured Azure // --------------------------------------------------------------------------- diff --git a/internal/oidc/azure_signer.go b/internal/oidc/azure_signer.go index 8bb87e8b1..b515849db 100644 --- a/internal/oidc/azure_signer.go +++ b/internal/oidc/azure_signer.go @@ -111,6 +111,10 @@ func (s *AzureKeyVaultSigner) resolveOnce(ctx context.Context) { s.err = fmt.Errorf("oidc: azure keyvault returned incomplete EC key (missing X or Y)") return } + if typeErr := validateES256KeyType(resp.Key); typeErr != nil { + s.err = fmt.Errorf("oidc: azure keyvault key %q: %w", s.keyName, typeErr) + return + } // JWK coordinates may omit leading zeros; right-align each into 32 bytes. if len(resp.Key.X) > 32 || len(resp.Key.Y) > 32 { s.err = fmt.Errorf("oidc: azure keyvault EC key coordinate exceeds 32 bytes") @@ -134,3 +138,28 @@ func (s *AzureKeyVaultSigner) resolveOnce(ctx context.Context) { s.kid = kid }) } + +// validateES256KeyType checks the key type and curve Key Vault reports for +// the key. Sign always requests ES256, so only an EC (software) or EC-HSM key +// on P-256 is usable. Checking the reported curve matters even when the +// coordinates parse as a P-256 point: a P-256K (secp256k1) key also has +// 32-byte coordinates, and without this check it would be published in the +// JWKS labeled as P-256. +func validateES256KeyType(key *azkeys.JSONWebKey) error { + ktyOK := key.Kty != nil && (*key.Kty == azkeys.KeyTypeEC || *key.Kty == azkeys.KeyTypeECHSM) + crvOK := key.Crv != nil && *key.Crv == azkeys.CurveNameP256 + if ktyOK && crvOK { + return nil + } + return fmt.Errorf("must be an EC or EC-HSM key on curve P-256 for ES256, got kty=%s crv=%s", + quotedOrMissing(key.Kty), quotedOrMissing(key.Crv)) +} + +// quotedOrMissing formats an optional Key Vault enum field for an error +// message, distinguishing an absent field from an empty one. +func quotedOrMissing[T ~string](v *T) string { + if v == nil { + return "" + } + return fmt.Sprintf("%q", string(*v)) +}