diff --git a/internal/auth/service_password.go b/internal/auth/service_password.go index 30f7075bd..59e9ba085 100644 --- a/internal/auth/service_password.go +++ b/internal/auth/service_password.go @@ -307,11 +307,24 @@ func (s *Service) ChangePassword(ctx context.Context, userID string, req ChangeP // token stays valid: this prevents a griefing attack where an adversary who // knows the victim's email repeatedly requests resets to perpetually // invalidate the victim's legitimate link. +// +// The stored token is either a reset token (PasswordResetExpiry) or an invite +// token (PasswordSetupExpiry), and only its expiry is stored. A reset token +// never has more than PasswordResetExpiry left, so a longer remaining window +// identifies an invite; deriving an invite's age from the reset lifetime made +// it negative for the whole week, silently dropping every "re-send my setup +// link" request (audit finding A03-009). A remaining window beyond even the +// invite lifetime still yields a negative age, so it stays rate-limited. func resetRecentlyIssued(user *User, email string) bool { if user.PasswordResetExpiry == nil { return false } - tokenAge := PasswordResetExpiry - time.Until(*user.PasswordResetExpiry) + remaining := time.Until(*user.PasswordResetExpiry) + lifetime := PasswordResetExpiry + if remaining > PasswordResetExpiry { + lifetime = PasswordSetupExpiry + } + tokenAge := lifetime - remaining if tokenAge < PasswordResetRateLimit { logging.Debugf("Password reset rate-limited for %s (token age %s < %s)", redactEmail(email), tokenAge.Round(time.Second), PasswordResetRateLimit) diff --git a/internal/auth/service_password_invite_test.go b/internal/auth/service_password_invite_test.go new file mode 100644 index 000000000..e55ddca8e --- /dev/null +++ b/internal/auth/service_password_invite_test.go @@ -0,0 +1,109 @@ +package auth + +import ( + "context" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// Audit finding A03-009 (issue #263): an invite token is issued with +// PasswordSetupExpiry (7d), but the forgot-password rate limit derived the +// token's age as if every token had the 1h PasswordResetExpiry. For an invite +// that age came out negative for almost the whole week, so the documented +// "re-mail the setup link via Forgot Password" recovery returned success +// without issuing a token or sending mail. +// +// The refusal case stubs NO write or email expectation: testify panics if +// either lands, so the assertion cannot be vacuous. + +// invitedUser is an account created through the invite flow and not yet +// activated: inactive, never deactivated, holding the invite token. +func invitedUser(issuedAgo time.Duration) *User { + expiry := time.Now().Add(PasswordSetupExpiry - issuedAgo) + return &User{ + ID: "user-invited", + Email: "invited@example.com", + PasswordResetToken: "invite-token-hash", + PasswordResetExpiry: &expiry, + Active: false, + } +} + +func TestService_RequestPasswordReset_InvitedUser(t *testing.T) { + ctx := context.Background() + + t.Run("invite older than the rate limit gets a fresh link", func(t *testing.T) { + mockStore := new(MockStore) + mockEmail := new(MockEmailSender) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + t.Cleanup(func() { mockEmail.AssertExpectations(t) }) + svc := createTestService(mockStore, mockEmail) + + user := invitedUser(time.Hour) + inviteExpiry := *user.PasswordResetExpiry + + mockStore.On("GetUserByEmail", ctx, user.Email).Return(user, nil).Once() + mockStore.On("SetPasswordResetToken", ctx, mock.MatchedBy(func(u *User) bool { + // A normal 1h reset token replaces the invite token. + return u.ID == user.ID && u.PasswordResetToken != "invite-token-hash" && + u.PasswordResetExpiry != nil && + time.Until(*u.PasswordResetExpiry) <= PasswordResetExpiry + }), mock.MatchedBy(func(readExpiry *time.Time) bool { + // The write is conditioned on the invite row it read. + return readExpiry != nil && readExpiry.Equal(inviteExpiry) + })).Return(nil).Once() + mockEmail.On("SendPasswordResetEmail", ctx, user.Email, mock.AnythingOfType("string")).Return(nil).Once() + + require.NoError(t, svc.RequestPasswordReset(ctx, user.Email)) + }) + + t.Run("invite inside the rate limit window is not reissued", func(t *testing.T) { + mockStore := new(MockStore) + mockEmail := new(MockEmailSender) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + t.Cleanup(func() { mockEmail.AssertExpectations(t) }) + svc := createTestService(mockStore, mockEmail) + + user := invitedUser(PasswordResetRateLimit / 2) + mockStore.On("GetUserByEmail", ctx, user.Email).Return(user, nil).Once() + // SetPasswordResetToken and SendPasswordResetEmail must NOT be called. + + require.NoError(t, svc.RequestPasswordReset(ctx, user.Email)) + }) +} + +// TestResetRecentlyIssued pins the age derivation for both token kinds at the +// rate-limit boundary, so neither a reset token nor an invite token can drift +// to "always limited" or "never limited". +func TestResetRecentlyIssued(t *testing.T) { + cases := []struct { + name string + lifetime time.Duration + issuedAgo time.Duration + want bool + }{ + {"reset token just issued", PasswordResetExpiry, 5 * time.Second, true}, + {"reset token past the limit", PasswordResetExpiry, PasswordResetRateLimit + 5*time.Second, false}, + {"invite token just issued", PasswordSetupExpiry, 5 * time.Second, true}, + {"invite token past the limit", PasswordSetupExpiry, PasswordResetRateLimit + 5*time.Second, false}, + {"invite token a day old", PasswordSetupExpiry, 24 * time.Hour, false}, + // Just over PasswordResetExpiry left is still an invite (aged ~7d), not a + // reset token issued in the future, so it is not limited. + {"invite token about to expire", PasswordSetupExpiry, PasswordSetupExpiry - PasswordResetExpiry - 30*time.Second, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + expiry := time.Now().Add(tc.lifetime - tc.issuedAgo) + user := &User{PasswordResetExpiry: &expiry} + assert.Equal(t, tc.want, resetRecentlyIssued(user, "u@example.com")) + }) + } + + t.Run("no token is never limited", func(t *testing.T) { + assert.False(t, resetRecentlyIssued(&User{}, "u@example.com")) + }) +} diff --git a/internal/auth/service_reset_invalidation_db_test.go b/internal/auth/service_reset_invalidation_db_test.go index e678a8cf0..afd65d12b 100644 --- a/internal/auth/service_reset_invalidation_db_test.go +++ b/internal/auth/service_reset_invalidation_db_test.go @@ -3,7 +3,11 @@ package auth import ( + "context" + "errors" + "net/url" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -75,3 +79,78 @@ func TestIntegration_CompletePasswordResetEnforcesExpiryInSQL(t *testing.T) { require.ErrorIs(t, err, ErrUserChanged) }) } + +// inviteMailSink fails every invite send, the case CreateUserResult answers +// by pointing the admin at Forgot Password, and records each reset link. +type inviteMailSink struct { + resetURLs []string +} + +func (m *inviteMailSink) SendPasswordResetEmail(_ context.Context, _, resetURL string) error { + m.resetURLs = append(m.resetURLs, resetURL) + return nil +} +func (m *inviteMailSink) SendWelcomeEmail(context.Context, string, string, string) error { + return nil +} +func (m *inviteMailSink) SendUserInviteEmail(context.Context, string, string) error { + return errors.New("smtp unavailable") +} + +// TestIntegration_InvitedUserForgotPasswordReissuesLink is audit finding +// A03-009 on real PostgreSQL: an invite whose email failed is recovered via +// Forgot Password, even while the 7-day invite token is still live, and two +// concurrent requests still store and mail exactly one token. +func TestIntegration_InvitedUserForgotPasswordReissuesLink(t *testing.T) { + db := setupAuthTestDB(t) + store := NewPostgresStore(db) + ctx := t.Context() + newSvc := func(st StoreInterface, mail EmailSenderInterface) *Service { + svc := NewService(ServiceConfig{Store: st, EmailSender: mail, DashboardURL: "https://dashboard.example.com"}) + svc.bcryptCostOverride = 4 + return svc + } + + barrier := &credentialReadBarrier{StoreInterface: store} + mail := &inviteMailSink{} + svc := newSvc(barrier, mail) + created, err := svc.CreateUser(ctx, "", CreateUserRequest{Email: "invite-forgot@example.com", GroupIDs: []string{DefaultPurchaserGroupID}}) + require.NoError(t, err) + require.NotNil(t, created.InviteEmailSent) + require.False(t, *created.InviteEmailSent, "fixture: the invite email must have failed") + invited := created.User + + // The invite went out an hour ago: well past the rate limit, with almost + // all of its 7-day window left. + _, err = db.Exec(ctx, "UPDATE users SET password_reset_expiry = password_reset_expiry - interval '1 hour' WHERE id = $1", invited.ID) + require.NoError(t, err) + inviteHash := invited.PasswordResetToken + + // A second request lands between this one's read and its write. + otherMail := &inviteMailSink{} + barrier.afterRead = func(ctx context.Context, _ *User) { + require.NoError(t, newSvc(store, otherMail).RequestPasswordReset(ctx, invited.Email)) + } + require.NoError(t, svc.RequestPasswordReset(ctx, invited.Email)) + + require.Len(t, otherMail.resetURLs, 1, "the first committed request must issue and mail a link") + assert.Empty(t, mail.resetURLs, "the losing request must not mail a token it did not store") + + stored, err := store.GetUserByID(ctx, invited.ID) + require.NoError(t, err) + assert.False(t, stored.Active) + assert.NotEqual(t, inviteHash, stored.PasswordResetToken, "the invite token must be replaced") + require.NotNil(t, stored.PasswordResetExpiry) + assert.LessOrEqual(t, time.Until(*stored.PasswordResetExpiry), PasswordResetExpiry) + + link, err := url.Parse(otherMail.resetURLs[0]) + require.NoError(t, err) + token := link.Query().Get("token") + require.Equal(t, hashSessionToken(token), stored.PasswordResetToken, "the mailed link must be the stored token") + + require.NoError(t, svc.ConfirmPasswordReset(ctx, PasswordResetConfirm{Token: token, NewPassword: credentialRaceNew})) + activated, err := store.GetUserByID(ctx, invited.ID) + require.NoError(t, err) + assert.True(t, activated.Active, "completing the re-sent link must activate the invited account") + assert.True(t, svc.verifyPassword(credentialRaceNew, activated.PasswordHash)) +} diff --git a/internal/oidc/aws_signer.go b/internal/oidc/aws_signer.go index ebc98dabe..6e4c4d024 100644 --- a/internal/oidc/aws_signer.go +++ b/internal/oidc/aws_signer.go @@ -7,7 +7,6 @@ import ( "crypto/elliptic" "crypto/x509" "fmt" - "sync" awsconfig "github.com/aws/aws-sdk-go-v2/config" "github.com/aws/aws-sdk-go-v2/service/kms" @@ -25,11 +24,8 @@ type AWSKMSClient interface { // The private key never leaves KMS. type AWSKMSSigner struct { client AWSKMSClient - err error - pubKey *ecdsa.PublicKey keyID string - kid string - once sync.Once + keys publicKeyCache } // NewAWSKMSSigner constructs a signer bound to the given KMS key. The @@ -70,46 +66,32 @@ func (s *AWSKMSSigner) Sign(ctx context.Context, digest []byte) ([]byte, error) return derToRawECDSASignature(out.Signature) } -// PublicKey fetches the public half of the KMS key once and caches it. -// Subsequent calls return the cached value. +// PublicKey fetches the public half of the KMS key and caches it once a +// fetch succeeds. A failed fetch is returned and retried on the next call. func (s *AWSKMSSigner) PublicKey(ctx context.Context) (crypto.PublicKey, error) { - s.resolveOnce(ctx) - return s.pubKey, s.err + return s.keys.publicKey(ctx, s.fetchPublicKey) } // KeyID returns the JWK kid derived from the cached public key. func (s *AWSKMSSigner) KeyID(ctx context.Context) (string, error) { - s.resolveOnce(ctx) - return s.kid, s.err + return s.keys.keyID(ctx, s.fetchPublicKey) } -func (s *AWSKMSSigner) resolveOnce(ctx context.Context) { - s.once.Do(func() { - out, err := s.client.GetPublicKey(ctx, &kms.GetPublicKeyInput{KeyId: &s.keyID}) - if err != nil { - s.err = fmt.Errorf("oidc: kms:GetPublicKey: %w", err) - return - } - pub, err := x509.ParsePKIXPublicKey(out.PublicKey) - if err != nil { - s.err = fmt.Errorf("oidc: parse kms public key: %w", err) - return - } - ecPub, ok := pub.(*ecdsa.PublicKey) - if !ok { - s.err = fmt.Errorf("oidc: kms key is not ECDSA (got %T); key must be ECC_NIST_P256", pub) - return - } - if ecPub.Curve != elliptic.P256() { - s.err = fmt.Errorf("oidc: kms key curve is %s; key must be ECC_NIST_P256 (ES256)", ecPub.Curve.Params().Name) - return - } - kid, err := ComputeKeyID(ecPub) - if err != nil { - s.err = err - return - } - s.pubKey = ecPub - s.kid = kid - }) +func (s *AWSKMSSigner) fetchPublicKey(ctx context.Context) (*ecdsa.PublicKey, error) { + out, err := s.client.GetPublicKey(ctx, &kms.GetPublicKeyInput{KeyId: &s.keyID}) + if err != nil { + return nil, fmt.Errorf("oidc: kms:GetPublicKey: %w", err) + } + pub, err := x509.ParsePKIXPublicKey(out.PublicKey) + if err != nil { + return nil, fmt.Errorf("oidc: parse kms public key: %w", err) + } + ecPub, ok := pub.(*ecdsa.PublicKey) + if !ok { + return nil, fmt.Errorf("oidc: kms key is not ECDSA (got %T); key must be ECC_NIST_P256", pub) + } + if ecPub.Curve != elliptic.P256() { + return nil, fmt.Errorf("oidc: kms key curve is %s; key must be ECC_NIST_P256 (ES256)", ecPub.Curve.Params().Name) + } + return ecPub, nil } diff --git a/internal/oidc/aws_signer_test.go b/internal/oidc/aws_signer_test.go index 4399b0651..d9194b48b 100644 --- a/internal/oidc/aws_signer_test.go +++ b/internal/oidc/aws_signer_test.go @@ -78,7 +78,7 @@ func TestAWSKMSSignerRoundTrip(t *testing.T) { // TestAWSKMSSignerRejectsWrongCurve guards against a misconfigured KMS // key (e.g. ECC_NIST_P384 instead of the required ECC_NIST_P256) being -// accepted silently. resolveOnce must fail fast with a clear error +// accepted silently. fetchPublicKey must fail fast with a clear error // instead of caching a wrong-curve public key that only breaks later // at JWKS-publish or signing time. func TestAWSKMSSignerRejectsWrongCurve(t *testing.T) { diff --git a/internal/oidc/azure_factory_test.go b/internal/oidc/azure_factory_test.go index e46f5b5cc..3be1adb81 100644 --- a/internal/oidc/azure_factory_test.go +++ b/internal/oidc/azure_factory_test.go @@ -13,7 +13,7 @@ import ( ) // fakeAzureKeyVaultClient is a minimal AzureKeyVaultClient backed by an -// in-process EC key. Used to exercise resolveOnce without a real Key Vault. +// in-process EC key. Used to exercise fetchPublicKey without a real Key Vault. type fakeAzureKeyVaultClient struct { signErr error key *ecdsa.PublicKey @@ -107,7 +107,7 @@ func (f *fakeAzureKeyVaultClient) jwkCrv() *azkeys.CurveName { // M6 -- Azure EC key completeness guard (replaces RSA exponent-range check) // --------------------------------------------------------------------------- -// TestAzureSigner_ECKeyCompleteness verifies that resolveOnce rejects Key +// TestAzureSigner_ECKeyCompleteness verifies that fetchPublicKey rejects Key // Vault responses that are missing the EC public-key coordinates, which // would otherwise produce a zero-valued *ecdsa.PublicKey and sign invalid // tokens. A valid response with both X and Y must be accepted. diff --git a/internal/oidc/azure_signer.go b/internal/oidc/azure_signer.go index b515849db..8cfc47229 100644 --- a/internal/oidc/azure_signer.go +++ b/internal/oidc/azure_signer.go @@ -6,7 +6,6 @@ import ( "crypto/ecdsa" "crypto/elliptic" "fmt" - "sync" "github.com/Azure/azure-sdk-for-go/sdk/azidentity" "github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys" @@ -23,12 +22,9 @@ type AzureKeyVaultClient interface { // private half never leaves the vault. type AzureKeyVaultSigner struct { client AzureKeyVaultClient - err error - pubKey *ecdsa.PublicKey keyName string keyVersion string - kid string - once sync.Once + keys publicKeyCache } // newAzureKeyVaultClient builds the real AzureKeyVaultClient using the @@ -87,56 +83,42 @@ func (s *AzureKeyVaultSigner) Sign(ctx context.Context, digest []byte) ([]byte, return resp.Result, nil } -// PublicKey fetches the public half of the Key Vault key once and -// caches it. +// PublicKey fetches the public half of the Key Vault key and caches it +// once a fetch succeeds. A failed fetch is returned and retried on the next +// call. func (s *AzureKeyVaultSigner) PublicKey(ctx context.Context) (crypto.PublicKey, error) { - s.resolveOnce(ctx) - return s.pubKey, s.err + return s.keys.publicKey(ctx, s.fetchPublicKey) } // KeyID returns a stable kid derived from the public key point. func (s *AzureKeyVaultSigner) KeyID(ctx context.Context) (string, error) { - s.resolveOnce(ctx) - return s.kid, s.err + return s.keys.keyID(ctx, s.fetchPublicKey) } -func (s *AzureKeyVaultSigner) resolveOnce(ctx context.Context) { - s.once.Do(func() { - resp, err := s.client.GetKey(ctx, s.keyName, s.keyVersion, nil) - if err != nil { - s.err = fmt.Errorf("oidc: azure keyvault GetKey: %w", err) - return - } - if resp.Key == nil || resp.Key.X == nil || resp.Key.Y == nil { - 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") - return - } - var uncompressed [65]byte - uncompressed[0] = 0x04 - copy(uncompressed[1+32-len(resp.Key.X):33], resp.Key.X) - copy(uncompressed[33+32-len(resp.Key.Y):65], resp.Key.Y) - ecPub, err := ecdsa.ParseUncompressedPublicKey(elliptic.P256(), uncompressed[:]) - if err != nil { - s.err = fmt.Errorf("oidc: azure keyvault parse EC public key: %w", err) - return - } - kid, err := ComputeKeyID(ecPub) - if err != nil { - s.err = err - return - } - s.pubKey = ecPub - s.kid = kid - }) +func (s *AzureKeyVaultSigner) fetchPublicKey(ctx context.Context) (*ecdsa.PublicKey, error) { + resp, err := s.client.GetKey(ctx, s.keyName, s.keyVersion, nil) + if err != nil { + return nil, fmt.Errorf("oidc: azure keyvault GetKey: %w", err) + } + if resp.Key == nil || resp.Key.X == nil || resp.Key.Y == nil { + return nil, fmt.Errorf("oidc: azure keyvault returned incomplete EC key (missing X or Y)") + } + if typeErr := validateES256KeyType(resp.Key); typeErr != nil { + return nil, fmt.Errorf("oidc: azure keyvault key %q: %w", s.keyName, typeErr) + } + // JWK coordinates may omit leading zeros; right-align each into 32 bytes. + if len(resp.Key.X) > 32 || len(resp.Key.Y) > 32 { + return nil, fmt.Errorf("oidc: azure keyvault EC key coordinate exceeds 32 bytes") + } + var uncompressed [65]byte + uncompressed[0] = 0x04 + copy(uncompressed[1+32-len(resp.Key.X):33], resp.Key.X) + copy(uncompressed[33+32-len(resp.Key.Y):65], resp.Key.Y) + ecPub, err := ecdsa.ParseUncompressedPublicKey(elliptic.P256(), uncompressed[:]) + if err != nil { + return nil, fmt.Errorf("oidc: azure keyvault parse EC public key: %w", err) + } + return ecPub, nil } // validateES256KeyType checks the key type and curve Key Vault reports for diff --git a/internal/oidc/backend_jws_test.go b/internal/oidc/backend_jws_test.go index afa29063b..2125ca6aa 100644 --- a/internal/oidc/backend_jws_test.go +++ b/internal/oidc/backend_jws_test.go @@ -74,7 +74,7 @@ func TestGCPKMSSignerEmitsRawES256(t *testing.T) { // TestGCPKMSSignerRejectsWrongCurve guards against a misconfigured KMS // key (e.g. EC_SIGN_P384_SHA384 instead of the required -// EC_SIGN_P256_SHA256) being accepted silently. resolveOnce must fail +// EC_SIGN_P256_SHA256) being accepted silently. fetchPublicKey must fail // fast instead of caching a wrong-curve public key. func TestGCPKMSSignerRejectsWrongCurve(t *testing.T) { ctx := context.Background() diff --git a/internal/oidc/gcp_signer.go b/internal/oidc/gcp_signer.go index a132f8a87..f414db09a 100644 --- a/internal/oidc/gcp_signer.go +++ b/internal/oidc/gcp_signer.go @@ -9,7 +9,6 @@ import ( "encoding/pem" "fmt" "hash/crc32" - "sync" kms "cloud.google.com/go/kms/apiv1" kmspb "cloud.google.com/go/kms/apiv1/kmspb" @@ -41,11 +40,8 @@ func (w gcpKMSWrapper) GetPublicKey(ctx context.Context, req *kmspb.GetPublicKey // private half never leaves the KMS. type GCPKMSSigner struct { client GCPKMSClient - err error - pubKey *ecdsa.PublicKey keyResource string - kid string - once sync.Once + keys publicKeyCache } // NewGCPKMSSigner constructs a signer bound to a specific KMS key @@ -92,50 +88,36 @@ func (s *GCPKMSSigner) Sign(ctx context.Context, digest []byte) ([]byte, error) return derToRawECDSASignature(resp.Signature) } -// PublicKey fetches the public half of the KMS key once and caches it. +// PublicKey fetches the public half of the KMS key and caches it once a +// fetch succeeds. A failed fetch is returned and retried on the next call. func (s *GCPKMSSigner) PublicKey(ctx context.Context) (crypto.PublicKey, error) { - s.resolveOnce(ctx) - return s.pubKey, s.err + return s.keys.publicKey(ctx, s.fetchPublicKey) } // KeyID returns a stable kid derived from the cached public key. func (s *GCPKMSSigner) KeyID(ctx context.Context) (string, error) { - s.resolveOnce(ctx) - return s.kid, s.err + return s.keys.keyID(ctx, s.fetchPublicKey) } -func (s *GCPKMSSigner) resolveOnce(ctx context.Context) { - s.once.Do(func() { - resp, err := s.client.GetPublicKey(ctx, &kmspb.GetPublicKeyRequest{Name: s.keyResource}) - if err != nil { - s.err = fmt.Errorf("oidc: gcp kms GetPublicKey: %w", err) - return - } - block, _ := pem.Decode([]byte(resp.Pem)) - if block == nil { - s.err = fmt.Errorf("oidc: gcp kms public key is not PEM-encoded") - return - } - pub, err := x509.ParsePKIXPublicKey(block.Bytes) - if err != nil { - s.err = fmt.Errorf("oidc: parse gcp kms public key: %w", err) - return - } - ecPub, ok := pub.(*ecdsa.PublicKey) - if !ok { - s.err = fmt.Errorf("oidc: gcp kms key is not ECDSA (got %T); key must be EC_SIGN_P256_SHA256", pub) - return - } - if ecPub.Curve != elliptic.P256() { - s.err = fmt.Errorf("oidc: gcp kms key curve is %s; key must be EC_SIGN_P256_SHA256 (ES256)", ecPub.Curve.Params().Name) - return - } - kid, err := ComputeKeyID(ecPub) - if err != nil { - s.err = err - return - } - s.pubKey = ecPub - s.kid = kid - }) +func (s *GCPKMSSigner) fetchPublicKey(ctx context.Context) (*ecdsa.PublicKey, error) { + resp, err := s.client.GetPublicKey(ctx, &kmspb.GetPublicKeyRequest{Name: s.keyResource}) + if err != nil { + return nil, fmt.Errorf("oidc: gcp kms GetPublicKey: %w", err) + } + block, _ := pem.Decode([]byte(resp.Pem)) + if block == nil { + return nil, fmt.Errorf("oidc: gcp kms public key is not PEM-encoded") + } + pub, err := x509.ParsePKIXPublicKey(block.Bytes) + if err != nil { + return nil, fmt.Errorf("oidc: parse gcp kms public key: %w", err) + } + ecPub, ok := pub.(*ecdsa.PublicKey) + if !ok { + return nil, fmt.Errorf("oidc: gcp kms key is not ECDSA (got %T); key must be EC_SIGN_P256_SHA256", pub) + } + if ecPub.Curve != elliptic.P256() { + return nil, fmt.Errorf("oidc: gcp kms key curve is %s; key must be EC_SIGN_P256_SHA256 (ES256)", ecPub.Curve.Params().Name) + } + return ecPub, nil } diff --git a/internal/oidc/signer.go b/internal/oidc/signer.go index 29e37aaf3..49a4f3dc2 100644 --- a/internal/oidc/signer.go +++ b/internal/oidc/signer.go @@ -30,6 +30,7 @@ import ( "encoding/json" "fmt" "math/big" + "sync" ) // p256SigComponentLen is the fixed byte length of each of the R and S @@ -55,7 +56,8 @@ type Signer interface { Sign(ctx context.Context, digest []byte) ([]byte, error) // PublicKey returns the public key corresponding to the signer. - // Cached after the first call per implementation. Returns + // Cached after the first successful call per implementation; a failed + // lookup is returned and retried on the next call. Returns // *ecdsa.PublicKey for ES256 signers. PublicKey(ctx context.Context) (crypto.PublicKey, error) @@ -194,6 +196,56 @@ func (s *LocalSigner) KeyID(_ context.Context) (string, error) { return s.kid, nil } +// publicKeyCache memoizes a KMS-backed signer's public key and kid. +// +// Only a successful resolution is cached. A failed fetch (a throttle, a +// network blip, or a caller whose ctx was already canceled) is returned to +// that caller and the next call fetches again; caching the error as well, as +// the earlier sync.Once did, poisoned the process-wide signer until the +// instance recycled (audit finding A03-014). The mutex serializes first +// callers, so concurrent callers never issue parallel fetches and a success +// is fetched exactly once. +type publicKeyCache struct { + pubKey *ecdsa.PublicKey + kid string + mu sync.Mutex +} + +// get returns the cached key and kid, calling fetch to resolve them when +// nothing has been cached yet. +func (c *publicKeyCache) get(ctx context.Context, fetch func(context.Context) (*ecdsa.PublicKey, error)) (*ecdsa.PublicKey, string, error) { + c.mu.Lock() + defer c.mu.Unlock() + if c.pubKey != nil { + return c.pubKey, c.kid, nil + } + pub, err := fetch(ctx) + if err != nil { + return nil, "", err + } + kid, err := ComputeKeyID(pub) + if err != nil { + return nil, "", err + } + c.pubKey, c.kid = pub, kid + return pub, kid, nil +} + +// publicKey adapts get to Signer.PublicKey, returning an untyped nil on error. +func (c *publicKeyCache) publicKey(ctx context.Context, fetch func(context.Context) (*ecdsa.PublicKey, error)) (crypto.PublicKey, error) { + pub, _, err := c.get(ctx, fetch) + if err != nil { + return nil, err + } + return pub, nil +} + +// keyID adapts get to Signer.KeyID. +func (c *publicKeyCache) keyID(ctx context.Context, fetch func(context.Context) (*ecdsa.PublicKey, error)) (string, error) { + _, kid, err := c.get(ctx, fetch) + return kid, err +} + // ComputeKeyID returns a stable kid for a public key. // For *ecdsa.PublicKey: SHA-256 of the uncompressed point (0x04 || X || Y), // base64url-encoded without padding. diff --git a/internal/oidc/signer_test.go b/internal/oidc/signer_test.go index 2bc51d0e9..8a4afc979 100644 --- a/internal/oidc/signer_test.go +++ b/internal/oidc/signer_test.go @@ -8,10 +8,16 @@ import ( "crypto/sha256" "encoding/base64" "encoding/json" + "errors" "math/big" "strings" + "sync" + "sync/atomic" "testing" + kmspb "cloud.google.com/go/kms/apiv1/kmspb" + "github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys" + "github.com/aws/aws-sdk-go-v2/service/kms" "github.com/golang-jwt/jwt/v5" ) @@ -223,3 +229,216 @@ func TestComputeKeyIDChangesOnKeyRotation(t *testing.T) { t.Errorf("different keys produced the same kid: %s", k1) } } + +// Audit finding A03-014 (issue #263): each KMS-backed signer resolved its +// public key inside sync.Once and stored the error with it, so one failed +// first lookup (a throttle, a network blip, or a caller whose ctx was already +// canceled) poisoned the process-wide signer until the instance recycled. +// Only a successful resolution may be cached. + +var errTransientKeyLookup = errors.New("transient key lookup failure") + +// keyLookupGate makes a fake backend's public-key fetch fail while failures +// is positive (and whenever the caller's ctx is done), and counts every +// fetch so a test can tell a retried lookup from a memoized one. +type keyLookupGate struct { + failures atomic.Int32 + calls atomic.Int32 +} + +func (g *keyLookupGate) enter(ctx context.Context) error { + g.calls.Add(1) + if err := ctx.Err(); err != nil { + return err + } + if g.failures.Add(-1) >= 0 { + return errTransientKeyLookup + } + return nil +} + +type gatedAWSKMS struct { + *fakeKMSClient + gate *keyLookupGate +} + +func (c gatedAWSKMS) GetPublicKey(ctx context.Context, in *kms.GetPublicKeyInput, opts ...func(*kms.Options)) (*kms.GetPublicKeyOutput, error) { + if err := c.gate.enter(ctx); err != nil { + return nil, err + } + return c.fakeKMSClient.GetPublicKey(ctx, in, opts...) +} + +type gatedAzureKeyVault struct { + *fakeAzureKeyVaultClient + gate *keyLookupGate +} + +func (c gatedAzureKeyVault) GetKey(ctx context.Context, name, version string, opts *azkeys.GetKeyOptions) (azkeys.GetKeyResponse, error) { + if err := c.gate.enter(ctx); err != nil { + return azkeys.GetKeyResponse{}, err + } + return c.fakeAzureKeyVaultClient.GetKey(ctx, name, version, opts) +} + +type gatedGCPKMS struct { + *fakeGCPKMSClient + gate *keyLookupGate +} + +func (c gatedGCPKMS) GetPublicKey(ctx context.Context, req *kmspb.GetPublicKeyRequest, opts ...interface{}) (*kmspb.PublicKey, error) { + if err := c.gate.enter(ctx); err != nil { + return nil, err + } + return c.fakeGCPKMSClient.GetPublicKey(ctx, req, opts...) +} + +type gatedSignerBackend struct { + build func(key *ecdsa.PrivateKey, gate *keyLookupGate) Signer + name string +} + +func gatedSignerBackends() []gatedSignerBackend { + return []gatedSignerBackend{ + {name: "aws", build: func(key *ecdsa.PrivateKey, gate *keyLookupGate) Signer { + return NewAWSKMSSignerFromClient(gatedAWSKMS{&fakeKMSClient{key: key}, gate}, "alias/test-key") + }}, + {name: "azure", build: func(key *ecdsa.PrivateKey, gate *keyLookupGate) Signer { + return NewAzureKeyVaultSignerFromClient(gatedAzureKeyVault{&fakeAzureKeyVaultClient{key: &key.PublicKey}, gate}, "test-key", "") + }}, + {name: "gcp", build: func(key *ecdsa.PrivateKey, gate *keyLookupGate) Signer { + return NewGCPKMSSignerFromClient(gatedGCPKMS{&fakeGCPKMSClient{key: key}, gate}, + "projects/p/locations/global/keyRings/r/cryptoKeys/k/cryptoKeyVersions/1") + }}, + } +} + +func newP256TestKey(t *testing.T) (*ecdsa.PrivateKey, string) { + t.Helper() + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Fatalf("gen key: %v", err) + } + kid, err := ComputeKeyID(&key.PublicKey) + if err != nil { + t.Fatalf("kid: %v", err) + } + return key, kid +} + +func TestKMSSignerRetriesFailedKeyLookup(t *testing.T) { + canceled, cancel := context.WithCancel(context.Background()) + cancel() + firstCalls := []struct { + ctx context.Context + wantErr error + name string + failures int32 + }{ + {name: "transient error", ctx: context.Background(), failures: 1, wantErr: errTransientKeyLookup}, + {name: "canceled first caller", ctx: canceled, failures: 0, wantErr: context.Canceled}, + } + for _, backend := range gatedSignerBackends() { + for _, first := range firstCalls { + t.Run(backend.name+"/"+first.name, func(t *testing.T) { + ctx := context.Background() + key, wantKid := newP256TestKey(t) + gate := &keyLookupGate{} + gate.failures.Store(first.failures) + signer := backend.build(key, gate) + + if _, err := signer.KeyID(first.ctx); !errors.Is(err, first.wantErr) { + t.Fatalf("first KeyID err = %v, want %v", err, first.wantErr) + } + + pub, err := signer.PublicKey(ctx) + if err != nil { + t.Fatalf("PublicKey after a failed first lookup: %v (the failure was cached)", err) + } + if ecPub, ok := pub.(*ecdsa.PublicKey); !ok || !ecPub.Equal(&key.PublicKey) { + t.Fatalf("PublicKey = %v, want the backend key", pub) + } + kid, err := signer.KeyID(ctx) + if err != nil || kid != wantKid { + t.Fatalf("KeyID = %q, %v; want %q", kid, err, wantKid) + } + // One failed fetch, one successful fetch, then the cache. + if got := gate.calls.Load(); got != 2 { + t.Errorf("key lookups = %d, want 2 (retry once, then memoize success)", got) + } + }) + } + } +} + +// TestKMSSignerConcurrentFirstResolution races many first callers against a +// backend whose first fetch fails: only the caller that drew the failure may +// see it, the rest must get the key, and the success is fetched exactly once. +func TestKMSSignerConcurrentFirstResolution(t *testing.T) { + const callers = 16 + for _, backend := range gatedSignerBackends() { + t.Run(backend.name, func(t *testing.T) { + ctx := context.Background() + key, wantKid := newP256TestKey(t) + gate := &keyLookupGate{} + gate.failures.Store(1) + signer := backend.build(key, gate) + + var ( + start sync.WaitGroup + done sync.WaitGroup + failed atomic.Int32 + wrong atomic.Int32 + ) + start.Add(1) + for range callers { + done.Add(1) + go func() { + defer done.Done() + start.Wait() + kid, err := signer.KeyID(ctx) + switch { + case errors.Is(err, errTransientKeyLookup): + failed.Add(1) + case err != nil || kid != wantKid: + wrong.Add(1) + } + }() + } + start.Done() + done.Wait() + + if got := failed.Load(); got != 1 { + t.Errorf("callers that saw the transient failure = %d, want 1", got) + } + if got := wrong.Load(); got != 0 { + t.Errorf("callers with an unexpected result = %d, want 0", got) + } + if got := gate.calls.Load(); got != 2 { + t.Errorf("key lookups = %d, want 2 (one failure, one memoized success)", got) + } + }) + } +} + +// TestKMSSignerPublicKeyErrorIsUntypedNil pins that a failed lookup returns a +// nil crypto.PublicKey interface, not one wrapping a typed-nil +// *ecdsa.PublicKey, so a caller's `pub == nil` check stays meaningful. +func TestKMSSignerPublicKeyErrorIsUntypedNil(t *testing.T) { + for _, backend := range gatedSignerBackends() { + t.Run(backend.name, func(t *testing.T) { + key, _ := newP256TestKey(t) + gate := &keyLookupGate{} + gate.failures.Store(1) + signer := backend.build(key, gate) + + pub, err := signer.PublicKey(context.Background()) + if !errors.Is(err, errTransientKeyLookup) { + t.Fatalf("PublicKey err = %v, want %v", err, errTransientKeyLookup) + } + if pub != nil { + t.Errorf("PublicKey on error = %#v, want an untyped nil", pub) + } + }) + } +}