Skip to content

Commit f963d2a

Browse files
committed
fix(oidc): rebase on main, fix test/lint regressions after ES256 migration
- Replace TestAzureSigner_ExponentRange (RSA exponent tests) with TestAzureSigner_ECKeyCompleteness: the azure_factory_test.go added on main (#1044) tested RSA exponent validation removed by this PR; new test covers EC key nil-field rejection to keep coverage parity. - Apply fieldalignment ordering (govet) to aws_signer.go, azure_signer.go, gcp_signer.go, and the updated azure_factory_test.go struct literals. - Suppress staticcheck SA1019 on elliptic.Marshal in ComputeKeyID with an inline nolint; elliptic.Marshal is the only stdlib path from *ecdsa.PublicKey to the uncompressed point without converting through crypto/ecdh. - Fix staticcheck QF1008 (remove unnecessary .PublicKey embed selector) in aws_signer_test.go and backend_jws_test.go. - Fix shadow declarations in signer_test.go (json.Unmarshal err vars). - Replace deprecated ecdsa.Sign with ecdsa.SignASN1 + derToRawECDSASignature in the fakeAzureKVClient test stub (backend_jws_test.go). - Fix gofmt import ordering in backend_jws_test.go (cloud.google.com before github.com).
1 parent 2e0bfb9 commit f963d2a

8 files changed

Lines changed: 81 additions & 65 deletions

File tree

‎internal/oidc/aws_signer.go‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,12 +24,11 @@ type AWSKMSClient interface {
2424
// The private key never leaves KMS.
2525
type AWSKMSSigner struct {
2626
client AWSKMSClient
27-
keyID string
28-
29-
once sync.Once
27+
err error
3028
pubKey *ecdsa.PublicKey
29+
keyID string
3130
kid string
32-
err error
31+
once sync.Once
3332
}
3433

3534
// NewAWSKMSSigner constructs a signer bound to the given KMS key. The

‎internal/oidc/aws_signer_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ func TestAWSKMSSignerRoundTrip(t *testing.T) {
5151
if !ok {
5252
t.Fatalf("public key is not *ecdsa.PublicKey, got %T", rawPub)
5353
}
54-
if ecPub.X.Cmp(key.PublicKey.X) != 0 || ecPub.Y.Cmp(key.PublicKey.Y) != 0 {
54+
if ecPub.X.Cmp(key.X) != 0 || ecPub.Y.Cmp(key.Y) != 0 {
5555
t.Fatal("public key point mismatch")
5656
}
5757

‎internal/oidc/azure_factory_test.go‎

Lines changed: 54 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,9 @@ package oidc
22

33
import (
44
"context"
5+
"crypto/ecdsa"
6+
"crypto/elliptic"
57
"crypto/rand"
6-
"crypto/rsa"
7-
"math/big"
88
"testing"
99

1010
"github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys"
@@ -13,73 +13,93 @@ import (
1313
)
1414

1515
// fakeAzureKeyVaultClient is a minimal AzureKeyVaultClient backed by an
16-
// in-process RSA key. Used to exercise resolveOnce without a real Key Vault.
16+
// in-process EC key. Used to exercise resolveOnce without a real Key Vault.
1717
type fakeAzureKeyVaultClient struct {
18-
key *rsa.PublicKey
19-
eBytes []byte // raw bytes for the public exponent (override for M6 tests)
2018
signErr error
19+
key *ecdsa.PublicKey
20+
// xBytes and yBytes allow callers to inject nil to simulate incomplete
21+
// responses from the Key Vault API.
22+
xBytes []byte
23+
yBytes []byte
24+
nilKey bool // if true, return a KeyBundle with Key==nil
2125
}
2226

2327
func (f *fakeAzureKeyVaultClient) Sign(_ context.Context, _, _ string, _ azkeys.SignParameters, _ *azkeys.SignOptions) (azkeys.SignResponse, error) {
2428
return azkeys.SignResponse{}, f.signErr
2529
}
2630

2731
func (f *fakeAzureKeyVaultClient) GetKey(_ context.Context, _, _ string, _ *azkeys.GetKeyOptions) (azkeys.GetKeyResponse, error) {
28-
eBytes := f.eBytes
29-
if eBytes == nil {
30-
// Normal path: real exponent from the RSA key.
31-
e := big.NewInt(int64(f.key.E))
32-
eBytes = e.Bytes()
32+
if f.nilKey {
33+
return azkeys.GetKeyResponse{
34+
KeyBundle: azkeys.KeyBundle{Key: nil},
35+
}, nil
36+
}
37+
xBytes := f.xBytes
38+
if xBytes == nil {
39+
xBytes = f.key.X.Bytes()
40+
}
41+
yBytes := f.yBytes
42+
if yBytes == nil {
43+
yBytes = f.key.Y.Bytes()
44+
}
45+
// Use sentinel value to represent "caller explicitly passed nil" vs
46+
// "caller didn't override" -- an empty slice signals nil field.
47+
var jwkX, jwkY []byte
48+
if f.xBytes != nil || xBytes != nil {
49+
jwkX = xBytes
50+
}
51+
if f.yBytes != nil || yBytes != nil {
52+
jwkY = yBytes
3353
}
3454
keyBundle := azkeys.JSONWebKey{
35-
N: f.key.N.Bytes(),
36-
E: eBytes,
55+
X: jwkX,
56+
Y: jwkY,
3757
}
3858
return azkeys.GetKeyResponse{
3959
KeyBundle: azkeys.KeyBundle{Key: &keyBundle},
4060
}, nil
4161
}
4262

4363
// ---------------------------------------------------------------------------
44-
// M6 — Azure public-exponent overflow guard
64+
// M6 -- Azure EC key completeness guard (replaces RSA exponent-range check)
4565
// ---------------------------------------------------------------------------
4666

47-
// TestAzureSigner_ExponentRange verifies that resolveOnce rejects oversized or
48-
// non-positive exponents before constructing the rsa.PublicKey (03-M6).
49-
func TestAzureSigner_ExponentRange(t *testing.T) {
50-
key, err := rsa.GenerateKey(rand.Reader, 2048)
67+
// TestAzureSigner_ECKeyCompleteness verifies that resolveOnce rejects Key
68+
// Vault responses that are missing the EC public-key coordinates, which
69+
// would otherwise produce a zero-valued *ecdsa.PublicKey and sign invalid
70+
// tokens. A valid response with both X and Y must be accepted.
71+
func TestAzureSigner_ECKeyCompleteness(t *testing.T) {
72+
key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
5173
require.NoError(t, err)
5274

5375
cases := []struct {
76+
client *fakeAzureKeyVaultClient
5477
name string
55-
eBytes []byte // raw bytes sent as the exponent
56-
wantErr bool
5778
errSubstr string
79+
wantErr bool
5880
}{
5981
{
60-
name: "normal exponent 65537 accepted",
61-
eBytes: big.NewInt(65537).Bytes(),
82+
name: "valid EC key accepted",
83+
client: &fakeAzureKeyVaultClient{
84+
key: &key.PublicKey,
85+
},
6286
wantErr: false,
6387
},
6488
{
65-
name: "exponent 0 rejected",
66-
eBytes: big.NewInt(0).Bytes(),
89+
name: "key bundle with Key=nil rejected",
90+
client: &fakeAzureKeyVaultClient{
91+
key: &key.PublicKey,
92+
nilKey: true,
93+
},
6794
wantErr: true,
68-
errSubstr: "exponent",
69-
},
70-
{
71-
name: "exponent too large (> MaxInt32) rejected",
72-
eBytes: new(big.Int).Add(big.NewInt(0x7fffffff), big.NewInt(1)).Bytes(),
73-
wantErr: true,
74-
errSubstr: "exponent",
95+
errSubstr: "missing X or Y",
7596
},
7697
}
7798

7899
for _, tc := range cases {
79100
tc := tc
80101
t.Run(tc.name, func(t *testing.T) {
81-
client := &fakeAzureKeyVaultClient{key: &key.PublicKey, eBytes: tc.eBytes}
82-
signer := NewAzureKeyVaultSignerFromClient(client, "test-key", "")
102+
signer := NewAzureKeyVaultSignerFromClient(tc.client, "test-key", "")
83103
ctx := context.Background()
84104

85105
_, err := signer.PublicKey(ctx)
@@ -94,7 +114,7 @@ func TestAzureSigner_ExponentRange(t *testing.T) {
94114
}
95115

96116
// ---------------------------------------------------------------------------
97-
// M7 — factory precise error for half-configured Azure
117+
// M7 -- factory precise error for half-configured Azure
98118
// ---------------------------------------------------------------------------
99119

100120
// TestNewSignerFromEnv_AzureHalfConfigured verifies that exactly one of the
@@ -107,9 +127,9 @@ func TestNewSignerFromEnv_AzureHalfConfigured(t *testing.T) {
107127
name string
108128
vaultURL string
109129
keyName string
130+
errSubstr string
110131
wantErr bool
111132
wantNil bool
112-
errSubstr string
113133
}{
114134
{
115135
name: "both empty = disabled (nil, nil)",

‎internal/oidc/azure_signer.go‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -24,13 +24,12 @@ type AzureKeyVaultClient interface {
2424
// private half never leaves the vault.
2525
type AzureKeyVaultSigner struct {
2626
client AzureKeyVaultClient
27+
err error
28+
pubKey *ecdsa.PublicKey
2729
keyName string
28-
keyVersion string // may be empty = latest
29-
30-
once sync.Once
31-
pubKey *ecdsa.PublicKey
32-
kid string
33-
err error
30+
keyVersion string
31+
kid string
32+
once sync.Once
3433
}
3534

3635
// NewAzureKeyVaultSigner constructs a signer against a Key Vault using

‎internal/oidc/backend_jws_test.go‎

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,8 @@ import (
99
"encoding/pem"
1010
"testing"
1111

12-
"github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys"
1312
kmspb "cloud.google.com/go/kms/apiv1/kmspb"
13+
"github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys"
1414
)
1515

1616
// These tests assert that every signer backend produces an RFC 7518
@@ -81,15 +81,14 @@ func (f *fakeAzureKVClient) Sign(_ context.Context, _, _ string, params azkeys.S
8181
// Real Azure Key Vault ES256 returns the raw R||S (IEEE P1363) form,
8282
// NOT DER. Mirror that here so the test fails if the signer wrongly
8383
// DER-converts it. Sign over the digest the caller passed in Value.
84-
r, s, err := ecdsa.Sign(rand.Reader, f.key, params.Value)
84+
der, err := ecdsa.SignASN1(rand.Reader, f.key, params.Value)
85+
if err != nil {
86+
return azkeys.SignResponse{}, err
87+
}
88+
raw, err := derToRawECDSASignature(der)
8589
if err != nil {
8690
return azkeys.SignResponse{}, err
8791
}
88-
raw := make([]byte, 64)
89-
rb := r.Bytes()
90-
sb := s.Bytes()
91-
copy(raw[32-len(rb):32], rb)
92-
copy(raw[64-len(sb):], sb)
9392
return azkeys.SignResponse{KeyOperationResult: azkeys.KeyOperationResult{Result: raw}}, nil
9493
}
9594

@@ -99,8 +98,8 @@ func (f *fakeAzureKVClient) GetKey(_ context.Context, _, _ string, _ *azkeys.Get
9998
jwk := &azkeys.JSONWebKey{
10099
Kty: &kty,
101100
Crv: &crv,
102-
X: f.key.PublicKey.X.Bytes(),
103-
Y: f.key.PublicKey.Y.Bytes(),
101+
X: f.key.X.Bytes(),
102+
Y: f.key.Y.Bytes(),
104103
}
105104
return azkeys.GetKeyResponse{KeyBundle: azkeys.KeyBundle{Key: jwk}}, nil
106105
}

‎internal/oidc/gcp_signer.go‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -40,12 +40,11 @@ func (w gcpKMSWrapper) GetPublicKey(ctx context.Context, req *kmspb.GetPublicKey
4040
// private half never leaves the KMS.
4141
type GCPKMSSigner struct {
4242
client GCPKMSClient
43-
keyResource string // full resource name, incl. /cryptoKeyVersions/N
44-
45-
once sync.Once
46-
pubKey *ecdsa.PublicKey
47-
kid string
48-
err error
43+
err error
44+
pubKey *ecdsa.PublicKey
45+
keyResource string
46+
kid string
47+
once sync.Once
4948
}
5049

5150
// NewGCPKMSSigner constructs a signer bound to a specific KMS key

‎internal/oidc/signer.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -204,7 +204,7 @@ func ComputeKeyID(pub crypto.PublicKey) (string, error) {
204204
if k == nil {
205205
return "", fmt.Errorf("oidc: nil ecdsa public key")
206206
}
207-
uncompressed := elliptic.Marshal(k.Curve, k.X, k.Y)
207+
uncompressed := elliptic.Marshal(k.Curve, k.X, k.Y) //nolint:staticcheck // elliptic.Marshal is the only Go stdlib way to get the uncompressed point from *ecdsa.PublicKey without converting to crypto/ecdh first.
208208
sum := sha256.Sum256(uncompressed)
209209
return base64.RawURLEncoding.EncodeToString(sum[:]), nil
210210
default:

‎internal/oidc/signer_test.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -84,8 +84,8 @@ func TestLocalSignerMintAndVerify(t *testing.T) {
8484
t.Fatalf("decode header: %v", err)
8585
}
8686
var header map[string]any
87-
if err := json.Unmarshal(headerBytes, &header); err != nil {
88-
t.Fatalf("unmarshal header: %v", err)
87+
if umErr := json.Unmarshal(headerBytes, &header); umErr != nil {
88+
t.Fatalf("unmarshal header: %v", umErr)
8989
}
9090
// Positively assert ES256 -- this is the invariant the fix-422 change guards.
9191
if header["alg"] != "ES256" {
@@ -104,8 +104,8 @@ func TestLocalSignerMintAndVerify(t *testing.T) {
104104
t.Fatalf("decode claims: %v", err)
105105
}
106106
var decoded map[string]any
107-
if err := json.Unmarshal(claimsBytes, &decoded); err != nil {
108-
t.Fatalf("unmarshal claims: %v", err)
107+
if umErr := json.Unmarshal(claimsBytes, &decoded); umErr != nil {
108+
t.Fatalf("unmarshal claims: %v", umErr)
109109
}
110110
if decoded["iss"] != claims["iss"] {
111111
t.Errorf("iss mismatch: %v vs %v", decoded["iss"], claims["iss"])

0 commit comments

Comments
 (0)