Skip to content

Commit 83165d3

Browse files
committed
fix(oidc): reject non-P-256 ECDSA keys instead of publishing a corrupt JWK
PublicJWK's doc comment claimed only P-256 keys are accepted, but the code accepted any *ecdsa.PublicKey and unconditionally hardcoded Crv: "P-256" / Alg: "ES256" in the output. A misconfigured wrong-curve key (e.g. P-384) would be published through /.well-known/jwks.json as a mislabeled P-256 JWK with actually-longer coordinates, and the real failure would only surface later at the KMS signing call, far from the misconfiguration's cause. Check ecPub.Curve against elliptic.P256() and return an explicit error immediately in PublicJWK, and in the AWS KMS / GCP KMS signers' resolveOnce so a wrong-curve key fails fast with a clear message at key resolution instead of producing a corrupt public JWKS. (Azure Key Vault's resolveOnce already forces P-256 when parsing the raw point, so it needed no change.) Adds regression tests generating a P-384 key and asserting each path returns an error; verified failing pre-fix and passing post-fix.
1 parent 5dd849a commit 83165d3

6 files changed

Lines changed: 74 additions & 0 deletions

File tree

‎internal/oidc/aws_signer.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"crypto"
66
"crypto/ecdsa"
7+
"crypto/elliptic"
78
"crypto/x509"
89
"fmt"
910
"sync"
@@ -99,6 +100,10 @@ func (s *AWSKMSSigner) resolveOnce(ctx context.Context) {
99100
s.err = fmt.Errorf("oidc: kms key is not ECDSA (got %T); key must be ECC_NIST_P256", pub)
100101
return
101102
}
103+
if ecPub.Curve != elliptic.P256() {
104+
s.err = fmt.Errorf("oidc: kms key curve is %s; key must be ECC_NIST_P256 (ES256)", ecPub.Curve.Params().Name)
105+
return
106+
}
102107
kid, err := ComputeKeyID(ecPub)
103108
if err != nil {
104109
s.err = err

‎internal/oidc/aws_signer_test.go‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,3 +75,21 @@ func TestAWSKMSSignerRoundTrip(t *testing.T) {
7575
// convert it to the RFC 7518 raw R||S form before Mint encodes it.
7676
assertRawES256JWS(t, jws, ecPub)
7777
}
78+
79+
// TestAWSKMSSignerRejectsWrongCurve guards against a misconfigured KMS
80+
// key (e.g. ECC_NIST_P384 instead of the required ECC_NIST_P256) being
81+
// accepted silently. resolveOnce must fail fast with a clear error
82+
// instead of caching a wrong-curve public key that only breaks later
83+
// at JWKS-publish or signing time.
84+
func TestAWSKMSSignerRejectsWrongCurve(t *testing.T) {
85+
ctx := context.Background()
86+
key, err := ecdsa.GenerateKey(elliptic.P384(), rand.Reader)
87+
if err != nil {
88+
t.Fatalf("gen p384 key: %v", err)
89+
}
90+
signer := NewAWSKMSSignerFromClient(&fakeKMSClient{key: key}, "alias/wrong-curve")
91+
92+
if _, err := signer.PublicKey(ctx); err == nil {
93+
t.Fatal("PublicKey accepted a P-384 KMS key; want an error rejecting the wrong curve")
94+
}
95+
}

‎internal/oidc/backend_jws_test.go‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,24 @@ func TestGCPKMSSignerEmitsRawES256(t *testing.T) {
7272
assertRawES256JWS(t, jws, &key.PublicKey)
7373
}
7474

75+
// TestGCPKMSSignerRejectsWrongCurve guards against a misconfigured KMS
76+
// key (e.g. EC_SIGN_P384_SHA384 instead of the required
77+
// EC_SIGN_P256_SHA256) being accepted silently. resolveOnce must fail
78+
// fast instead of caching a wrong-curve public key.
79+
func TestGCPKMSSignerRejectsWrongCurve(t *testing.T) {
80+
ctx := context.Background()
81+
key, err := ecdsa.GenerateKey(elliptic.P384(), rand.Reader)
82+
if err != nil {
83+
t.Fatalf("gen p384 key: %v", err)
84+
}
85+
signer := NewGCPKMSSignerFromClient(&fakeGCPKMSClient{key: key},
86+
"projects/p/locations/global/keyRings/r/cryptoKeys/k/cryptoKeyVersions/1")
87+
88+
if _, err := signer.PublicKey(ctx); err == nil {
89+
t.Fatal("PublicKey accepted a P-384 KMS key; want an error rejecting the wrong curve")
90+
}
91+
}
92+
7593
// --- Azure: raw R||S path ---
7694

7795
type fakeAzureKVClient struct {

‎internal/oidc/gcp_signer.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"crypto"
66
"crypto/ecdsa"
7+
"crypto/elliptic"
78
"crypto/x509"
89
"encoding/pem"
910
"fmt"
@@ -125,6 +126,10 @@ func (s *GCPKMSSigner) resolveOnce(ctx context.Context) {
125126
s.err = fmt.Errorf("oidc: gcp kms key is not ECDSA (got %T); key must be EC_SIGN_P256_SHA256", pub)
126127
return
127128
}
129+
if ecPub.Curve != elliptic.P256() {
130+
s.err = fmt.Errorf("oidc: gcp kms key curve is %s; key must be EC_SIGN_P256_SHA256 (ES256)", ecPub.Curve.Params().Name)
131+
return
132+
}
128133
kid, err := ComputeKeyID(ecPub)
129134
if err != nil {
130135
s.err = err

‎internal/oidc/jwks.go‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"crypto"
66
"crypto/ecdsa"
7+
"crypto/elliptic"
78
"encoding/base64"
89
"fmt"
910
)
@@ -38,6 +39,14 @@ func PublicJWK(pub crypto.PublicKey, kid string) (JWK, error) {
3839
if !ok || ecPub == nil {
3940
return JWK{}, fmt.Errorf("oidc: PublicJWK requires *ecdsa.PublicKey, got %T", pub)
4041
}
42+
// Fail loud on a wrong-curve key instead of publishing a corrupt JWK:
43+
// the Crv/Alg fields below are hardcoded to "P-256"/ES256, so a
44+
// misconfigured P-384 (or other) key would otherwise be mislabeled
45+
// as P-256 with coordinates of the wrong length, and only surface as
46+
// a KMS signing failure far from the actual misconfiguration.
47+
if ecPub.Curve != elliptic.P256() {
48+
return JWK{}, fmt.Errorf("oidc: PublicJWK requires a P-256 key (ES256), got curve %s", ecPub.Curve.Params().Name)
49+
}
4150
byteLen := (ecPub.Curve.Params().BitSize + 7) / 8
4251
// Derive the fixed-width, already left-padded coordinates from the
4352
// uncompressed SEC 1 point (0x04 || X || Y) via crypto/ecdh, matching

‎internal/oidc/signer_test.go‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@ package oidc
33
import (
44
"context"
55
"crypto/ecdsa"
6+
"crypto/elliptic"
7+
"crypto/rand"
68
"crypto/sha256"
79
"encoding/base64"
810
"encoding/json"
@@ -157,6 +159,23 @@ func TestBuildJWKS(t *testing.T) {
157159
// RSA fields are structurally absent from the EC JWK type.
158160
}
159161

162+
// TestPublicJWKRejectsWrongCurve guards against a misconfigured
163+
// non-P-256 KMS/Key Vault key (e.g. P-384) silently producing a
164+
// corrupt JWK: pre-fix, PublicJWK hardcoded Crv/Alg to "P-256"/ES256
165+
// regardless of ecPub.Curve, so a P-384 key would be published as a
166+
// mislabeled P-256 JWK with 48-byte (not 32-byte) coordinates, and the
167+
// misconfiguration would only surface later as an opaque KMS signing
168+
// failure. Post-fix, PublicJWK must reject it immediately.
169+
func TestPublicJWKRejectsWrongCurve(t *testing.T) {
170+
key, err := ecdsa.GenerateKey(elliptic.P384(), rand.Reader)
171+
if err != nil {
172+
t.Fatalf("gen p384 key: %v", err)
173+
}
174+
if _, err := PublicJWK(&key.PublicKey, "some-kid"); err == nil {
175+
t.Fatal("PublicJWK accepted a P-384 key; want an error rejecting the wrong curve")
176+
}
177+
}
178+
160179
func TestBuildDiscovery(t *testing.T) {
161180
d := BuildDiscovery("https://cudly.example.com")
162181
if d.Issuer != "https://cudly.example.com" {

0 commit comments

Comments
 (0)