From f2ee5433a405af2fb62bb24f8ad94de0f01e7df3 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 17 Jul 2026 20:08:36 +0300 Subject: [PATCH 1/2] fix(api): gate empty-account history rows on ownership + enforce API-key constraints on money paths F1 (adversarial-review, medium/cross-scope PII+financial leak): filterPurchaseHistoryByAllowedAccounts previously passed ALL empty-AccountID rows through to scoped users unconditionally (issue #1032 / #621 fix). Any user with view:purchases could see other users' multi-account in-flight rows, including CreatedByUserEmail (PII) and dollar amounts. Fix: gate the exemption on ownership -- only pass through when p.CreatedByUserID == session.UserID. Preserves the #621/#1032 behavior for each user's own ambient in-flight rows; drops other users' rows silently. F2 (adversarial-review, low/med -- money path): requirePermissionConstraints always called HasPermissionForConstraintsAPI with session.UserID, which re-derives constraints from the owning user's GROUP permissions and ignores the user API key's own Constraints (MaxPurchaseAmount, AccountIDs, Providers, etc.). A CI key capped at $100 could spend up to the user's full group limit. Fix: thread the key's DB ID via Session.UserAPIKeyID (set in requirePermission when authenticating via HasAPIKeyPermissionAPI). requirePermissionConstraints now calls HasAPIKeyPermissionForConstraintsAPI when session.UserAPIKeyID != "", evaluating constraints against the key's effective permissions (intersection of key + user group permissions). Falls closed on any lookup error. New methods: - auth.Service.HasAPIKeyPermissionForConstraintsAPI - AuthServiceInterface.HasAPIKeyPermissionForConstraintsAPI - Session.UserAPIKeyID field (json:"-") HasAPIKeyPermissionAPI now returns (userID, keyID, allowed, err) instead of (userID, allowed, err) to avoid a redundant DB lookup. Regression tests: - TestHandler_getHistory_ScopedUserSeesEmptyAccountRows: updated to set CreatedByUserID so the scoped user's own row still passes - TestHandler_getHistory_ScopedUserCannotSeeOtherUsersEmptyAccountRows: F1 guard (fails pre-fix) - TestHandler_executePurchase_UserAPIKeyConstraintsDenied: F2 guard (fails pre-fix, asserts HasAPIKeyPermissionForConstraintsAPI is called) --- internal/api/handler.go | 26 ++++++- internal/api/handler_apikeys_test.go | 13 ++-- internal/api/handler_history.go | 26 ++++--- internal/api/handler_history_test.go | 88 +++++++++++++++++++++--- internal/api/handler_purchases_test.go | 59 ++++++++++++++++ internal/api/handler_ri_exchange_test.go | 7 +- internal/api/mocks_test.go | 19 +++-- internal/api/types.go | 24 +++++-- internal/auth/service_apikeys_api.go | 50 ++++++++++++-- internal/auth/service_apikeys_test.go | 16 +++-- internal/server/app.go | 8 ++- 11 files changed, 282 insertions(+), 54 deletions(-) diff --git a/internal/api/handler.go b/internal/api/handler.go index e415214e3..76726348f 100644 --- a/internal/api/handler.go +++ b/internal/api/handler.go @@ -237,12 +237,15 @@ func (h *Handler) requirePermission(ctx context.Context, req *events.LambdaFunct // resolveAuthenticatedUserID, so a stale x-api-key header cannot lock // out a caller that also presents a valid session. if apiKey != "" { - userID, has, err := h.auth.HasAPIKeyPermissionAPI(ctx, apiKey, action, resource) + userID, keyID, has, err := h.auth.HasAPIKeyPermissionAPI(ctx, apiKey, action, resource) if err == nil { if !has { return nil, NewClientError(403, fmt.Sprintf("permission denied: requires %s on %s", action, resource)) } - return &Session{UserID: userID}, nil + // Thread the key's database ID so requirePermissionConstraints can + // evaluate constraints against the key's effective permissions (not + // just the owning user's group permissions). + return &Session{UserID: userID, UserAPIKeyID: keyID}, nil } logging.Debugf("User API key permission check failed: %v", err) } @@ -291,6 +294,13 @@ const unattributedAccountConstraint = "unattributed" // infrastructure credential with no user row, so it bypasses the check just // like it bypasses requirePermission's per-user lookup. Fails closed on a // missing auth service or a lookup error. +// +// For user-API-key sessions (session.UserAPIKeyID != ""), constraints are +// evaluated against the KEY's effective permissions (the intersection of the +// key's own constraints and the owning user's group permissions). This +// prevents a CI key with MaxPurchaseAmount=$100 from spending up to the +// owning user's full group limit by inheriting the broader group permissions +// (adversarial-review F2). func (h *Handler) requirePermissionConstraints(ctx context.Context, session *Session, action, resource string, constraintSets []auth.PermissionConstraints) error { if session == nil { return fmt.Errorf("internal error: nil session passed to requirePermissionConstraints") @@ -301,6 +311,18 @@ func (h *Handler) requirePermissionConstraints(ctx context.Context, session *Ses if h.auth == nil { return fmt.Errorf("authentication service not configured") } + // User API key: evaluate constraints against the key's effective permissions, + // not the owning user's full group permissions. + if session.UserAPIKeyID != "" { + has, err := h.auth.HasAPIKeyPermissionForConstraintsAPI(ctx, session.UserAPIKeyID, session.UserID, action, resource, constraintSets) + if err != nil { + return fmt.Errorf("permission constraint check failed: %w", err) + } + if !has { + return NewClientError(403, fmt.Sprintf("permission denied: this request exceeds the constraints configured on your %s permission for %s", action, resource)) + } + return nil + } has, err := h.auth.HasPermissionForConstraintsAPI(ctx, session.UserID, action, resource, constraintSets) if err != nil { return fmt.Errorf("permission constraint check failed: %w", err) diff --git a/internal/api/handler_apikeys_test.go b/internal/api/handler_apikeys_test.go index 2bcb893ca..05ee4d5f2 100644 --- a/internal/api/handler_apikeys_test.go +++ b/internal/api/handler_apikeys_test.go @@ -672,20 +672,23 @@ func TestRequirePermission_UserAPIKey(t *testing.T) { mockAuth := new(MockAuthService) t.Cleanup(func() { mockAuth.AssertExpectations(t) }) mockAuth.On("HasAPIKeyPermissionAPI", ctx, "cudly-user-key", "view", "recommendations"). - Return("user-123", true, nil) + Return("user-123", "key-abc", true, nil) handler := &Handler{auth: mockAuth} session, err := handler.requirePermission(ctx, newReq(), "view", "recommendations") require.NoError(t, err) require.NotNil(t, session) assert.Equal(t, "user-123", session.UserID) + // Key ID must be threaded so requirePermissionConstraints can evaluate + // key-scoped caps (adversarial-review F2). + assert.Equal(t, "key-abc", session.UserAPIKeyID) }) t.Run("regression #1142: scoped key is denied an out-of-scope permission with 403", func(t *testing.T) { mockAuth := new(MockAuthService) t.Cleanup(func() { mockAuth.AssertExpectations(t) }) mockAuth.On("HasAPIKeyPermissionAPI", ctx, "cudly-user-key", "execute", "purchases"). - Return("user-123", false, nil) + Return("user-123", "", false, nil) handler := &Handler{auth: mockAuth} session, err := handler.requirePermission(ctx, newReq(), "execute", "purchases") @@ -701,7 +704,7 @@ func TestRequirePermission_UserAPIKey(t *testing.T) { mockAuth := new(MockAuthService) t.Cleanup(func() { mockAuth.AssertExpectations(t) }) mockAuth.On("HasAPIKeyPermissionAPI", ctx, "cudly-user-key", "view", "recommendations"). - Return("", false, errors.New("invalid API key")) + Return("", "", false, errors.New("invalid API key")) handler := &Handler{auth: mockAuth} session, err := handler.requirePermission(ctx, newReq(), "view", "recommendations") @@ -716,7 +719,7 @@ func TestRequirePermission_UserAPIKey(t *testing.T) { mockAuth := new(MockAuthService) t.Cleanup(func() { mockAuth.AssertExpectations(t) }) mockAuth.On("HasAPIKeyPermissionAPI", ctx, "stale-key", "view", "recommendations"). - Return("", false, errors.New("invalid API key")) + Return("", "", false, errors.New("invalid API key")) mockAuth.On("ValidateSession", ctx, "session-token"). Return(&Session{UserID: "user-456"}, nil) mockAuth.On("HasPermissionAPI", ctx, "user-456", "view", "recommendations"). @@ -761,7 +764,7 @@ func TestGetRecommendations_UserAPIKey(t *testing.T) { mockAuth := new(MockAuthService) t.Cleanup(func() { mockAuth.AssertExpectations(t) }) mockAuth.On("HasAPIKeyPermissionAPI", ctx, "cudly-user-key", "view", "recommendations"). - Return("user-123", true, nil) + Return("user-123", "key-abc", true, nil) // Account scoping resolves against the key's owning user. mockAuth.On("GetAllowedAccountsAPI", ctx, "user-123"). Return([]string(nil), nil) diff --git a/internal/api/handler_history.go b/internal/api/handler_history.go index c0dc320b4..6226d0733 100644 --- a/internal/api/handler_history.go +++ b/internal/api/handler_history.go @@ -955,14 +955,14 @@ func appendMissing(dst []string, vals ...string) []string { // through unchanged. // // Rows with an empty AccountID are exempt from the drop for scoped users -// (issue #1032, regression of #621). An empty AccountID means the execution -// was ambient (exec.CloudAccountID == nil) AND its recommendations carry no -// common account. These are in-flight financial actions the user owns that -// cannot be attributed to a specific cloud account — dropping them silently -// re-introduces the #621 disappearance bug for non-admin users. Passing them -// through is safe: the session's allowed_accounts gate already ensures the -// user has view:purchases permission, and unattributed rows carry no -// account-specific data that would violate cross-tenant isolation. +// only when the requesting user created the row (issue #1032, regression of +// #621). An empty AccountID means the execution was ambient +// (exec.CloudAccountID == nil) AND its recommendations carry no common +// account. These are in-flight financial actions the user owns that cannot be +// attributed to a specific cloud account; dropping them silently re-introduces +// the #621 disappearance bug. The exemption is now gated on ownership so that +// another user's multi-account in-flight row (including CreatedByUserEmail PII +// and dollar amounts) is not visible to unrelated scoped users. func (h *Handler) filterPurchaseHistoryByAllowedAccounts(ctx context.Context, session *Session, purchases []config.PurchaseHistoryRecord) ([]config.PurchaseHistoryRecord, error) { allowed, err := h.getAllowedAccounts(ctx, session) if err != nil { @@ -976,10 +976,14 @@ func (h *Handler) filterPurchaseHistoryByAllowedAccounts(ctx context.Context, se for _rvc := range purchases { p := purchases[_rvc] // Empty AccountID: unattributed ambient/multi-account synthesized row. - // Pass through so scoped users see in-flight financial actions that - // cannot be pinned to a single account (issue #1032 / #621 regression). + // Pass through only when the requesting user created the row (issue + // #1032 / #621 regression + adversarial-review F1). Dropping rows + // owned by other users prevents PII (CreatedByUserEmail) and dollar + // amounts from leaking across user boundaries. if p.AccountID == "" { - filtered = append(filtered, p) + if p.CreatedByUserID == session.UserID { + filtered = append(filtered, p) + } continue } if auth.MatchesAccount(allowed, p.AccountID, nameByID[p.AccountID]) { diff --git a/internal/api/handler_history_test.go b/internal/api/handler_history_test.go index a9aca5c91..8caafa080 100644 --- a/internal/api/handler_history_test.go +++ b/internal/api/handler_history_test.go @@ -483,12 +483,17 @@ func TestHandler_getHistory_GetIsReadOnly(t *testing.T) { func TestHandler_getHistory_ScopedUserSeesEmptyAccountRows(t *testing.T) { ctx := context.Background() - // Ambient execution: CloudAccountID == nil, recommendations also have no - // account — this is the multi-account/ambient-credentials path. + const scopedUserID = "scoped-user-id" + + // Ambient execution created by the scoped user: CloudAccountID == nil and + // recommendations carry no common account. CreatedByUserID must match the + // requesting session so the ownership gate (adversarial-review F1) passes. + creatorID := scopedUserID ambientExec := config.PurchaseExecution{ - ExecutionID: "ambient-exec-1", - Status: "pending", - ScheduledDate: time.Now(), + ExecutionID: "ambient-exec-1", + Status: "pending", + ScheduledDate: time.Now(), + CreatedByUserID: &creatorID, Recommendations: []config.RecommendationRecord{ // No CloudAccountID on either rec → collapseRecommendationAccount // returns "" → executionToHistoryRow sets AccountID = "". @@ -510,14 +515,16 @@ func TestHandler_getHistory_ScopedUserSeesEmptyAccountRows(t *testing.T) { // Scoped user: only allowed to access one specific account UUID. scopedUser := &Session{ - UserID: "scoped-user-id", + UserID: scopedUserID, Email: "scoped@example.com", } mockAuth := new(MockAuthService) mockAuth.On("ValidateSession", ctx, "scoped-token").Return(scopedUser, nil) - mockAuth.On("HasPermissionAPI", ctx, "scoped-user-id", "view", "purchases").Return(true, nil) + mockAuth.On("HasPermissionAPI", ctx, scopedUserID, "view", "purchases").Return(true, nil) // allowed_accounts is non-empty → user is scoped (not unrestricted). - mockAuth.On("GetAllowedAccountsAPI", ctx, "scoped-user-id").Return([]string{"aaaaaaaa-aaaa-4aaa-aaaa-aaaaaaaaaaaa"}, nil) + mockAuth.On("GetAllowedAccountsAPI", ctx, scopedUserID).Return([]string{"aaaaaaaa-aaaa-4aaa-aaaa-aaaaaaaaaaaa"}, nil) + // resolveUserEmails resolves creator emails; stub for the scoped user's own ID. + mockAuth.On("GetUser", ctx, scopedUserID).Return(&User{Email: "scoped@example.com"}, nil).Maybe() handler := &Handler{ auth: mockAuth, @@ -533,14 +540,75 @@ func TestHandler_getHistory_ScopedUserSeesEmptyAccountRows(t *testing.T) { resp := result.(HistoryResponse) require.Len(t, resp.Purchases, 1, - "issue #1032 / #621 regression: a scoped user must see in-flight ambient-account executions; dropping them re-creates the financial-action-vanishes bug") + "issue #1032 / #621 regression: a scoped user must see their own in-flight ambient-account executions; dropping them re-creates the financial-action-vanishes bug") assert.Equal(t, "ambient-exec-1", resp.Purchases[0].PurchaseID) assert.Equal(t, "", resp.Purchases[0].AccountID, - "the AccountID is empty (ambient execution) and must remain visible to scoped users") + "the AccountID is empty (ambient execution) and must remain visible to scoped users who created the row") assert.Equal(t, "pending", resp.Purchases[0].Status) assert.Equal(t, 1, resp.Summary.TotalPending) } +// TestHandler_getHistory_ScopedUserCannotSeeOtherUsersEmptyAccountRows is the +// adversarial-review F1 regression test: a scoped user must NOT see in-flight +// ambient-account rows (AccountID == "") created by OTHER users. Before the +// fix, filterPurchaseHistoryByAllowedAccounts passed ALL empty-AccountID rows +// through unconditionally, leaking other users' CreatedByUserEmail (PII) and +// dollar amounts to any scoped user who had view:purchases. +// +// Fails pre-fix: user B's ambient row passes through the empty-AccountID +// exemption and resp.Purchases has length 1 instead of 0. +func TestHandler_getHistory_ScopedUserCannotSeeOtherUsersEmptyAccountRows(t *testing.T) { + ctx := context.Background() + + const userAID = "user-a-id" + const userBID = "user-b-id" + + // Ambient execution created by user B with PII-carrying fields. + creatorID := userBID + otherUsersExec := config.PurchaseExecution{ + ExecutionID: "other-user-ambient-exec", + Status: "pending", + ScheduledDate: time.Now(), + CreatedByUserID: &creatorID, + Recommendations: []config.RecommendationRecord{ + // No cloud account → AccountID will be "". + {Provider: "aws", Service: "ec2", Region: "us-east-1"}, + }, + } + + mockStore := new(MockConfigStore) + approverEmail := "ops@example.com" + mockStore.On("GetAllPurchaseHistory", ctx, 100).Return([]config.PurchaseHistoryRecord{}, nil) + mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything).Return([]config.PurchaseExecution{otherUsersExec}, nil) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{NotificationEmail: &approverEmail}, nil) + mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + return nil, nil + } + + // User A: scoped to one account, different from user B. + userASession := &Session{UserID: userAID, Email: "a@example.com"} + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "user-a-token").Return(userASession, nil) + mockAuth.On("HasPermissionAPI", ctx, userAID, "view", "purchases").Return(true, nil) + mockAuth.On("GetAllowedAccountsAPI", ctx, userAID).Return([]string{"aaaaaaaa-aaaa-4aaa-aaaa-aaaaaaaaaaaa"}, nil) + // resolveUserEmails may try to look up user B's email; stub it to return a + // value so the test exercises the actual row filtering, not an email lookup error. + mockAuth.On("GetUser", ctx, userBID).Return(&User{Email: "b@example.com"}, nil).Maybe() + + handler := &Handler{auth: mockAuth, config: mockStore} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer user-a-token"}, + } + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + result, err := handler.getHistory(ctx, req, map[string]string{}) + require.NoError(t, err) + + resp := result.(HistoryResponse) + assert.Empty(t, resp.Purchases, + "adversarial-review F1: scoped user A must NOT see another user's empty-account in-flight row (PII/financial leak)") +} + // TestHandler_expireStaleExecutionsAsync_SystemActorIsNil asserts that the // async stale-expire sweep passes nil as the actor param to // TransitionExecutionStatus. Expiry is a system-initiated path (no human diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index bfb1919c2..8700a6f5f 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -3618,6 +3618,65 @@ func TestHandler_executePurchase_PermissionConstraintsDenied(t *testing.T) { assert.Contains(t, ce.Error(), "constraints") } +// TestHandler_executePurchase_UserAPIKeyConstraintsDenied is the +// adversarial-review F2 regression test: a user API key with a +// MaxPurchaseAmount cap must be denied when the request exceeds that cap, even +// if the OWNING USER's group permissions allow a higher limit. Pre-fix, +// requirePermissionConstraints always called HasPermissionForConstraintsAPI +// with session.UserID, which re-derives from the user's GROUP permissions and +// silently ignores the key's own Constraints — so a $100-capped CI key could +// spend up to the user's full limit. +// +// Fails pre-fix: HasAPIKeyPermissionForConstraintsAPI is never called; +// HasPermissionForConstraintsAPI (the user path) is called instead and returns +// true (user has no cap), so the request is not rejected and falls through to +// execution. +func TestHandler_executePurchase_UserAPIKeyConstraintsDenied(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + // No store expectations: the request must be rejected before saving. + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + const ownerUserID = "eeeeeeee-eeee-eeee-eeee-eeeeeeeeeeee" + const keyID = "key-id-ci-100-cap" + + // requirePermission routes to HasAPIKeyPermissionAPI for x-api-key headers. + // Returns the owning user's ID and the key's DB ID so the session carries + // UserAPIKeyID for the downstream constraint check. + mockAuth.On("HasAPIKeyPermissionAPI", ctx, "ci-key-value", "execute", "purchases"). + Return(ownerUserID, keyID, true, nil) + // validatePurchaseRecommendationScope uses GetAllowedAccountsAPI to scope + // the request; return empty so no account filter is applied. + mockAuth.On("GetAllowedAccountsAPI", ctx, ownerUserID).Return([]string{}, nil) + // requirePermissionConstraints must call HasAPIKeyPermissionForConstraintsAPI + // (not HasPermissionForConstraintsAPI) when session.UserAPIKeyID != "". + // The key's cap blocks the $500 purchase. + mockAuth.On("HasAPIKeyPermissionForConstraintsAPI", ctx, keyID, ownerUserID, "execute", "purchases", + mock.MatchedBy(func(sets []auth.PermissionConstraints) bool { + if len(sets) != 1 { + return false + } + return sets[0].MaxPurchaseAmount == 500.0 + })).Return(false, nil) + + handler := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"x-api-key": "ci-key-value"}, + Body: `{"recommendations": [ + {"id": "rec-1", "provider": "aws", "service": "ec2", "region": "us-east-1", "count": 1, "term": 1, "payment": "all-upfront", "upfront_cost": 500.0, "savings": 50.0} + ]}`, + } + _, err := handler.executePurchase(ctx, req) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a 403 clientError, got: %v", err) + assert.Equal(t, 403, ce.code) + assert.Contains(t, ce.Error(), "constraints", + "adversarial-review F2: user API key's MaxPurchaseAmount cap must be enforced at execution time") +} + // TestPurchaseConstraintSets_AccountDimensionAlwaysPopulated pins the SEC-01 // fail-closed shape of the AccountIDs dimension: every constraint set must // carry a non-empty AccountIDs list. The auth matcher treats an empty diff --git a/internal/api/handler_ri_exchange_test.go b/internal/api/handler_ri_exchange_test.go index 957c33ce3..a18b4d53d 100644 --- a/internal/api/handler_ri_exchange_test.go +++ b/internal/api/handler_ri_exchange_test.go @@ -1035,8 +1035,11 @@ func (m *mockAuthForExchange) RevokeAPIKeyAPI(_ context.Context, _, _ string) er func (m *mockAuthForExchange) ValidateUserAPIKeyAPI(_ context.Context, _ string) (any, any, error) { return nil, nil, nil } -func (m *mockAuthForExchange) HasAPIKeyPermissionAPI(_ context.Context, _, _, _ string) (string, bool, error) { - return "admin", true, nil +func (m *mockAuthForExchange) HasAPIKeyPermissionAPI(_ context.Context, _, _, _ string) (string, string, bool, error) { + return "admin", "", true, nil +} +func (m *mockAuthForExchange) HasAPIKeyPermissionForConstraintsAPI(_ context.Context, _, _, _, _ string, _ []auth.PermissionConstraints) (bool, error) { + return true, nil } func (m *mockAuthForExchange) GetAllowedAccountsAPI(_ context.Context, _ string) ([]string, error) { return nil, nil diff --git a/internal/api/mocks_test.go b/internal/api/mocks_test.go index 981708dd8..4bfc68568 100644 --- a/internal/api/mocks_test.go +++ b/internal/api/mocks_test.go @@ -263,13 +263,15 @@ func (m *MockAuthService) GetUserPermissionsAPI(ctx context.Context, userID stri } // allowConstraintChecks stubs the SEC-01 execution-time permission -// constraint check (HasPermissionForConstraintsAPI) to succeed for any -// request, modeling a granting permission with no Constraints configured. -// Tests that target constraint behavior register an explicit expectation -// instead. +// constraint check (HasPermissionForConstraintsAPI and the user-API-key +// variant HasAPIKeyPermissionForConstraintsAPI) to succeed for any request, +// modeling a granting permission with no Constraints configured. Tests that +// target constraint behavior register explicit expectations instead. func (m *MockAuthService) allowConstraintChecks() { m.On("HasPermissionForConstraintsAPI", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything). Return(true, nil).Maybe() + m.On("HasAPIKeyPermissionForConstraintsAPI", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return(true, nil).Maybe() } // grantAdmin makes every HasPermissionAPI check succeed, modeling an @@ -328,7 +330,12 @@ func (m *MockAuthService) ValidateUserAPIKeyAPI(ctx context.Context, apiKey stri return args.Get(0), args.Get(1), args.Error(2) } -func (m *MockAuthService) HasAPIKeyPermissionAPI(ctx context.Context, apiKey, action, resource string) (string, bool, error) { +func (m *MockAuthService) HasAPIKeyPermissionAPI(ctx context.Context, apiKey, action, resource string) (string, string, bool, error) { args := m.Called(ctx, apiKey, action, resource) - return args.String(0), args.Bool(1), args.Error(2) + return args.String(0), args.String(1), args.Bool(2), args.Error(3) +} + +func (m *MockAuthService) HasAPIKeyPermissionForConstraintsAPI(ctx context.Context, keyID, userID, action, resource string, constraintSets []auth.PermissionConstraints) (bool, error) { + args := m.Called(ctx, keyID, userID, action, resource, constraintSets) + return args.Bool(0), args.Error(1) } diff --git a/internal/api/types.go b/internal/api/types.go index 3ebaac1b8..e51e2e093 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -214,12 +214,20 @@ type AuthServiceInterface interface { // HasAPIKeyPermissionAPI validates a user API key and checks the // requested action/resource against the key's effective permissions // (the intersection of the key's scoped permissions with the owning - // user's group-derived permissions). Returns the owning user's ID and - // whether the permission is held; a non-nil error means the key did - // not validate or the lookup failed, and callers must deny (fail - // closed). Wired into requirePermission so per-key scoping is + // user's group-derived permissions). Returns the owning user's ID, the + // key's database ID, and whether the permission is held; a non-nil error + // means the key did not validate or the lookup failed, and callers must + // deny (fail closed). Wired into requirePermission so per-key scoping is // enforced at request time (issue #1142). - HasAPIKeyPermissionAPI(ctx context.Context, apiKey, action, resource string) (string, bool, error) + HasAPIKeyPermissionAPI(ctx context.Context, apiKey, action, resource string) (userID, keyID string, allowed bool, err error) + // HasAPIKeyPermissionForConstraintsAPI checks request-derived permission + // constraint sets against a user API key's effective permissions (the + // intersection of the key's own constraints and the owning user's + // group-derived permissions). Must be called after HasAPIKeyPermissionAPI + // confirms action/resource access. Enforces per-key caps (MaxPurchaseAmount, + // Providers, Services, Regions, AccountIDs) that the key's owner cannot + // override via their broader group permissions (adversarial-review F2). + HasAPIKeyPermissionForConstraintsAPI(ctx context.Context, keyID, userID, action, resource string, constraintSets []auth.PermissionConstraints) (bool, error) } // Auth request/response types (to avoid import cycle with auth package). @@ -260,6 +268,12 @@ type PasswordResetConfirm struct { type Session struct { UserID string `json:"user_id"` Email string `json:"email"` + // UserAPIKeyID is the database key ID when the request was authenticated via + // a user API key (not the admin infra key). Empty for bearer-token and + // admin-API-key sessions. Used by requirePermissionConstraints to evaluate + // constraints against the key's effective permissions rather than the owning + // user's full group permissions (adversarial-review F2). + UserAPIKeyID string `json:"-"` } type User struct { diff --git a/internal/auth/service_apikeys_api.go b/internal/auth/service_apikeys_api.go index 26d994f3f..9361ec70a 100644 --- a/internal/auth/service_apikeys_api.go +++ b/internal/auth/service_apikeys_api.go @@ -139,25 +139,65 @@ func (s *Service) ValidateUserAPIKeyAPI(ctx context.Context, apiKey string) (*Us // of the key's scoped permissions with the owning user's group-derived // permissions (ComputeEffectivePermissions). A key created without explicit // permissions inherits the owner's full permission set. Returns the owning -// user's ID and whether the permission is held. +// user's ID, the key's database ID, and whether the permission is held. +// The key ID is threaded to callers so they can pass it to +// HasAPIKeyPermissionForConstraintsAPI without a redundant DB lookup. // // Fail closed: any validation failure (unknown, revoked, or expired key, // inactive owner) or permission-lookup error returns a non-nil error and // callers must deny access. -func (s *Service) HasAPIKeyPermissionAPI(ctx context.Context, apiKey, action, resource string) (userID string, allowed bool, err error) { +func (s *Service) HasAPIKeyPermissionAPI(ctx context.Context, apiKey, action, resource string) (userID, keyID string, allowed bool, err error) { key, user, err := s.ValidateUserAPIKey(ctx, apiKey) if err != nil { - return "", false, err + return "", "", false, err } perms, err := s.ComputeEffectivePermissions(ctx, key, user) if err != nil { - return "", false, fmt.Errorf("computing effective API key permissions: %w", err) + return "", "", false, fmt.Errorf("computing effective API key permissions: %w", err) } // AuthContext.HasPermission applies the same matching semantics as // session-based checks, including the admin:* wildcard with its // money-spending carve-outs (issue #923). effective := &AuthContext{User: user, Permissions: perms} - return user.ID, effective.HasPermission(action, resource), nil + return user.ID, key.ID, effective.HasPermission(action, resource), nil +} + +// HasAPIKeyPermissionForConstraintsAPI checks request-derived permission +// constraint sets against a user API key's effective permissions (the +// intersection of the key's own permissions with the owning user's +// group-derived permissions). Callers must have already confirmed the key +// grants action/resource via HasAPIKeyPermissionAPI; this enforces the +// Constraints dimension (MaxPurchaseAmount, Providers, Services, Regions, +// AccountIDs) at execution time for user-API-key sessions (adversarial-review +// F2, issue #1141 extension). +// +// Fail closed: an empty constraintSets slice is a caller bug; any DB lookup +// failure returns an error and callers must deny. +func (s *Service) HasAPIKeyPermissionForConstraintsAPI(ctx context.Context, keyID, userID, action, resource string, constraintSets []PermissionConstraints) (bool, error) { + if len(constraintSets) == 0 { + return false, fmt.Errorf("no permission constraint sets provided for %s on %s", action, resource) + } + key, err := s.store.GetAPIKeyByID(ctx, keyID) + if err != nil { + return false, fmt.Errorf("failed to load API key for constraint check: %w", err) + } + if key == nil { + return false, fmt.Errorf("API key not found") + } + user, err := s.lookupAPIKeyUser(ctx, userID) + if err != nil { + return false, err + } + perms, err := s.ComputeEffectivePermissions(ctx, key, user) + if err != nil { + return false, fmt.Errorf("computing effective API key permissions: %w", err) + } + for i := range constraintSets { + if !s.permissionsAllow(perms, action, resource, &constraintSets[i]) { + return false, nil + } + } + return true, nil } diff --git a/internal/auth/service_apikeys_test.go b/internal/auth/service_apikeys_test.go index d8fc2f5f7..b2da5fd93 100644 --- a/internal/auth/service_apikeys_test.go +++ b/internal/auth/service_apikeys_test.go @@ -871,12 +871,15 @@ func TestService_HasAPIKeyPermissionAPI(t *testing.T) { return service } - t.Run("scoped key grants its in-scope permission", func(t *testing.T) { + t.Run("scoped key grants its in-scope permission and returns key ID", func(t *testing.T) { service := setup(t, []Permission{{Action: ActionView, Resource: ResourceRecommendations}}) - userID, has, err := service.HasAPIKeyPermissionAPI(ctx, rawKey, ActionView, ResourceRecommendations) + userID, keyID, has, err := service.HasAPIKeyPermissionAPI(ctx, rawKey, ActionView, ResourceRecommendations) require.NoError(t, err) assert.Equal(t, "user-123", userID) + // Key ID is returned so callers can pass it to + // HasAPIKeyPermissionForConstraintsAPI without a redundant DB lookup. + assert.NotEmpty(t, keyID, "key ID must be non-empty so constraint checks can use it") assert.True(t, has) }) @@ -885,7 +888,7 @@ func TestService_HasAPIKeyPermissionAPI(t *testing.T) { // group also grants create:plans. The key must NOT inherit it. service := setup(t, []Permission{{Action: ActionView, Resource: ResourceRecommendations}}) - userID, has, err := service.HasAPIKeyPermissionAPI(ctx, rawKey, ActionCreate, ResourcePlans) + userID, _, has, err := service.HasAPIKeyPermissionAPI(ctx, rawKey, ActionCreate, ResourcePlans) require.NoError(t, err) assert.Equal(t, "user-123", userID) assert.False(t, has, "scoped key must not grant permissions outside its scope") @@ -895,7 +898,7 @@ func TestService_HasAPIKeyPermissionAPI(t *testing.T) { // The key claims admin:users but the user's group never granted it. service := setup(t, []Permission{{Action: ActionAdmin, Resource: ResourceUsers}}) - userID, has, err := service.HasAPIKeyPermissionAPI(ctx, rawKey, ActionAdmin, ResourceUsers) + userID, _, has, err := service.HasAPIKeyPermissionAPI(ctx, rawKey, ActionAdmin, ResourceUsers) require.NoError(t, err) assert.Equal(t, "user-123", userID) assert.False(t, has, "key must not grant permissions the owning user does not hold") @@ -904,7 +907,7 @@ func TestService_HasAPIKeyPermissionAPI(t *testing.T) { t.Run("unscoped key inherits the owner's group permissions", func(t *testing.T) { service := setup(t, nil) - userID, has, err := service.HasAPIKeyPermissionAPI(ctx, rawKey, ActionCreate, ResourcePlans) + userID, _, has, err := service.HasAPIKeyPermissionAPI(ctx, rawKey, ActionCreate, ResourcePlans) require.NoError(t, err) assert.Equal(t, "user-123", userID) assert.True(t, has) @@ -917,9 +920,10 @@ func TestService_HasAPIKeyPermissionAPI(t *testing.T) { mockStore.On("GetAPIKeyByHash", ctx, keyHash).Return(nil, nil) - userID, has, err := service.HasAPIKeyPermissionAPI(ctx, rawKey, ActionView, ResourceRecommendations) + userID, keyID, has, err := service.HasAPIKeyPermissionAPI(ctx, rawKey, ActionView, ResourceRecommendations) assert.Error(t, err) assert.Empty(t, userID) + assert.Empty(t, keyID) assert.False(t, has) }) } diff --git a/internal/server/app.go b/internal/server/app.go index 45f56c92d..fbd8b687c 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -1168,7 +1168,11 @@ func (a *authServiceAdapter) ValidateUserAPIKeyAPI(ctx context.Context, apiKey s return a.service.ValidateUserAPIKeyAPI(ctx, apiKey) } -func (a *authServiceAdapter) HasAPIKeyPermissionAPI(ctx context.Context, apiKey, action, resource string) (userID string, allowed bool, err error) { - userID, allowed, err = a.service.HasAPIKeyPermissionAPI(ctx, apiKey, action, resource) +func (a *authServiceAdapter) HasAPIKeyPermissionAPI(ctx context.Context, apiKey, action, resource string) (userID, keyID string, allowed bool, err error) { + userID, keyID, allowed, err = a.service.HasAPIKeyPermissionAPI(ctx, apiKey, action, resource) return } + +func (a *authServiceAdapter) HasAPIKeyPermissionForConstraintsAPI(ctx context.Context, keyID, userID, action, resource string, constraintSets []auth.PermissionConstraints) (bool, error) { + return a.service.HasAPIKeyPermissionForConstraintsAPI(ctx, keyID, userID, action, resource, constraintSets) +} From f3ea51f9bc04be962f9bea576d456e23c225944d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 17 Jul 2026 20:48:09 +0300 Subject: [PATCH 2/2] fix(api): address CodeRabbit security findings on #1454 C1 (handler.go): extract authorizeAPIKey helper and fail closed when HasAPIKeyPermissionAPI returns an empty userID or keyID despite has=true. Without the guard, an empty keyID produces a bearer-session-shaped Session (UserAPIKeyID="") that bypasses the API-key constraint path in requirePermissionConstraints. Extraction keeps requirePermission below the gocyclo-10 pre-commit threshold. C2 (service_apikeys_api.go + service_apikeys.go): enforce the owner's group constraint limits independently in HasAPIKeyPermissionForConstraintsAPI. ComputeEffectivePermissions retains the key's constraint values (e.g. MaxPurchaseAmount) even when they exceed the owner's group limits; a key capped at $1000 owned by a user capped at $100 could previously authorize a $500 request. The fix fetches ownerAuthCtx once (replacing the implicit GetAuthContext inside ComputeEffectivePermissions) and checks each constraint set against both the key's effective permissions and the owner's group permissions. computeEffectivePermissionsFromAuthCtx is extracted as a pure helper so the single authCtx fetch is shared between both checks. Regression test: TestService_HasAPIKeyPermissionForConstraintsAPI_OwnerCapEnforced confirms the $500 request is denied when owner cap is $100, and a $50 request within both caps is allowed. --- internal/api/handler.go | 49 ++++++++++++++++------ internal/auth/service_apikeys.go | 38 ++++++++++-------- internal/auth/service_apikeys_api.go | 17 +++++++- internal/auth/service_apikeys_test.go | 58 +++++++++++++++++++++++++++ 4 files changed, 131 insertions(+), 31 deletions(-) diff --git a/internal/api/handler.go b/internal/api/handler.go index 76726348f..914ecc6d7 100644 --- a/internal/api/handler.go +++ b/internal/api/handler.go @@ -236,18 +236,12 @@ func (h *Handler) requirePermission(ctx context.Context, req *events.LambdaFunct // that fails validation falls through to bearer-token auth, matching // resolveAuthenticatedUserID, so a stale x-api-key header cannot lock // out a caller that also presents a valid session. - if apiKey != "" { - userID, keyID, has, err := h.auth.HasAPIKeyPermissionAPI(ctx, apiKey, action, resource) - if err == nil { - if !has { - return nil, NewClientError(403, fmt.Sprintf("permission denied: requires %s on %s", action, resource)) - } - // Thread the key's database ID so requirePermissionConstraints can - // evaluate constraints against the key's effective permissions (not - // just the owning user's group permissions). - return &Session{UserID: userID, UserAPIKeyID: keyID}, nil - } - logging.Debugf("User API key permission check failed: %v", err) + session, apiKeyErr := h.authorizeAPIKey(ctx, apiKey, action, resource) + if apiKeyErr != nil { + return nil, apiKeyErr + } + if session != nil { + return session, nil } token := h.extractBearerToken(req) @@ -271,6 +265,37 @@ func (h *Handler) requirePermission(ctx context.Context, req *events.LambdaFunct return session, nil } +// authorizeAPIKey checks whether the given API key value has the required +// permission. It returns: +// - (nil, nil) if apiKey is absent or fails validation (caller falls +// through to bearer-token auth) +// - (session, nil) if the key grants the permission and identity is valid +// - (nil, 403 error) if the key is present and actively denies the request +// - (nil, other error) if an invariant is violated (e.g. incomplete identity) +// +// Fail closed: empty userID or keyID after a successful grant is an internal +// error, not a fall-through, so the caller cannot silently skip key constraints. +func (h *Handler) authorizeAPIKey(ctx context.Context, apiKey, action, resource string) (*Session, error) { + if apiKey == "" { + return nil, nil + } + userID, keyID, has, err := h.auth.HasAPIKeyPermissionAPI(ctx, apiKey, action, resource) + if err != nil { + logging.Debugf("User API key permission check failed: %v", err) + return nil, nil // validation error: fall through to bearer-token auth + } + if !has { + return nil, NewClientError(403, fmt.Sprintf("permission denied: requires %s on %s", action, resource)) + } + if userID == "" || keyID == "" { + return nil, fmt.Errorf("API key permission check returned incomplete identity") + } + // Thread the key's database ID so requirePermissionConstraints can evaluate + // constraints against the key's effective permissions (not just the owning + // user's group permissions). + return &Session{UserID: userID, UserAPIKeyID: keyID}, nil +} + // unattributedAccountConstraint is the request-side AccountIDs value passed // to requirePermissionConstraints when a request cannot be attributed to a // registered cloud account (a recommendation without a cloud_account_id, or diff --git a/internal/auth/service_apikeys.go b/internal/auth/service_apikeys.go index 3f36f5f39..b8e2a5b94 100644 --- a/internal/auth/service_apikeys.go +++ b/internal/auth/service_apikeys.go @@ -310,7 +310,26 @@ func (s *Service) UpdateLastUsed(ctx context.Context, keyID string) error { return s.store.UpdateAPIKeyLastUsed(ctx, keyID) } -// ComputeEffectivePermissions computes the intersection of API key permissions and user permissions +// computeEffectivePermissionsFromAuthCtx returns the subset of key permissions +// that the owner's authCtx also grants at the action/resource level, keeping +// the key's own constraint limits. If the key has no specific permissions the +// owner's full permission set is returned (key inherits owner). The result +// carries the key's constraints, not the owner's; callers that need both +// sources must check ownerAuthCtx.Permissions independently. +func computeEffectivePermissionsFromAuthCtx(key *UserAPIKey, authCtx *AuthContext) []Permission { + if len(key.Permissions) == 0 { + return authCtx.Permissions + } + effectivePerms := make([]Permission, 0, len(key.Permissions)) + for _, keyPerm := range key.Permissions { + if authCtx.HasPermission(keyPerm.Action, keyPerm.Resource) { + effectivePerms = append(effectivePerms, keyPerm) + } + } + return effectivePerms +} + +// ComputeEffectivePermissions computes the intersection of API key permissions and user permissions. // This ensures an API key cannot grant more permissions than the user has. // // Administrators-group members carry {admin, *}: with no key-specific @@ -319,24 +338,9 @@ func (s *Service) UpdateLastUsed(ctx context.Context, keyID string) error { // group-derived path preserves the previous role == admin behavior without a // special case. func (s *Service) ComputeEffectivePermissions(ctx context.Context, apiKey *UserAPIKey, user *User) ([]Permission, error) { - // Get user's auth context authCtx, err := s.GetAuthContext(ctx, user.ID) if err != nil { return nil, fmt.Errorf("failed to get user auth context: %w", err) } - - // If API key has no specific permissions, use user's permissions - if len(apiKey.Permissions) == 0 { - return authCtx.Permissions, nil - } - - // Compute intersection: only permissions the user has AND the key has - effectivePerms := []Permission{} - for _, keyPerm := range apiKey.Permissions { - if authCtx.HasPermission(keyPerm.Action, keyPerm.Resource) { - effectivePerms = append(effectivePerms, keyPerm) - } - } - - return effectivePerms, nil + return computeEffectivePermissionsFromAuthCtx(apiKey, authCtx), nil } diff --git a/internal/auth/service_apikeys_api.go b/internal/auth/service_apikeys_api.go index 9361ec70a..b80876672 100644 --- a/internal/auth/service_apikeys_api.go +++ b/internal/auth/service_apikeys_api.go @@ -186,18 +186,31 @@ func (s *Service) HasAPIKeyPermissionForConstraintsAPI(ctx context.Context, keyI if key == nil { return false, fmt.Errorf("API key not found") } + // lookupAPIKeyUser also verifies that the user is still active. user, err := s.lookupAPIKeyUser(ctx, userID) if err != nil { return false, err } - perms, err := s.ComputeEffectivePermissions(ctx, key, user) + // Fetch the owner's auth context once; it is used for two purposes: + // 1. Computing the key's effective permissions (key ∩ owner at action/resource level) + // 2. Independently enforcing the owner's group constraint limits. + // This prevents a key whose MaxPurchaseAmount (or other constraint) exceeds the owner's + // group limit from authorizing more than the owner's group allows (CR finding). + ownerAuthCtx, err := s.GetAuthContext(ctx, user.ID) if err != nil { - return false, fmt.Errorf("computing effective API key permissions: %w", err) + return false, fmt.Errorf("failed to get owner auth context for constraint check: %w", err) } + perms := computeEffectivePermissionsFromAuthCtx(key, ownerAuthCtx) + // Each constraint set must independently pass both gates: + // - The key's effective permissions (key's constraint limits, e.g. MaxPurchaseAmount). + // - The owner's group permissions (owner's constraint limits). for i := range constraintSets { if !s.permissionsAllow(perms, action, resource, &constraintSets[i]) { return false, nil } + if !s.permissionsAllow(ownerAuthCtx.Permissions, action, resource, &constraintSets[i]) { + return false, nil + } } return true, nil } diff --git a/internal/auth/service_apikeys_test.go b/internal/auth/service_apikeys_test.go index b2da5fd93..364ba838e 100644 --- a/internal/auth/service_apikeys_test.go +++ b/internal/auth/service_apikeys_test.go @@ -928,6 +928,64 @@ func TestService_HasAPIKeyPermissionAPI(t *testing.T) { }) } +// TestService_HasAPIKeyPermissionForConstraintsAPI_OwnerCapEnforced is a +// regression guard for adversarial-review finding C2 (CR finding on #1454): +// a key whose MaxPurchaseAmount exceeds the owner's group limit must NOT be +// allowed to spend more than the owner's group allows. +func TestService_HasAPIKeyPermissionForConstraintsAPI_OwnerCapEnforced(t *testing.T) { + ctx := context.Background() + grpID := "buyer-group" + keyID := "key-99" + userID := "user-88" + + // Owner's group is capped at $100. + ownerGroup := &Group{ + ID: grpID, + Permissions: []Permission{ + {Action: ActionExecute, Resource: ResourcePurchases, Constraints: &PermissionConstraints{MaxPurchaseAmount: 100}}, + }, + } + owner := &User{ID: userID, Email: "buyer@example.com", Active: true, GroupIDs: []string{grpID}} + // Key is scoped to execute:purchases but with a much higher cap of $1000. + key := &UserAPIKey{ + ID: keyID, + UserID: userID, + Permissions: []Permission{ + {Action: ActionExecute, Resource: ResourcePurchases, Constraints: &PermissionConstraints{MaxPurchaseAmount: 1000}}, + }, + IsActive: true, + } + + buildStore := func(t *testing.T) *MockStore { + t.Helper() + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + mockStore.On("GetAPIKeyByID", ctx, keyID).Return(key, nil) + // lookupAPIKeyUser + GetAuthContext both call GetUserByID. + mockStore.On("GetUserByID", ctx, userID).Return(owner, nil) + mockStore.On("GetGroup", ctx, grpID).Return(ownerGroup, nil) + return mockStore + } + + t.Run("key cap $1000 cannot exceed owner cap $100 (regression C2)", func(t *testing.T) { + service := &Service{store: buildStore(t)} + // Request amount $500: within key cap but over owner cap. + constraintSets := []PermissionConstraints{{MaxPurchaseAmount: 500}} + ok, err := service.HasAPIKeyPermissionForConstraintsAPI(ctx, keyID, userID, ActionExecute, ResourcePurchases, constraintSets) + require.NoError(t, err) + assert.False(t, ok, "request exceeding owner group cap must be denied even when within key cap") + }) + + t.Run("request within both key and owner cap is allowed", func(t *testing.T) { + service := &Service{store: buildStore(t)} + // Request amount $50: within both key cap ($1000) and owner cap ($100). + constraintSets := []PermissionConstraints{{MaxPurchaseAmount: 50}} + ok, err := service.HasAPIKeyPermissionForConstraintsAPI(ctx, keyID, userID, ActionExecute, ResourcePurchases, constraintSets) + require.NoError(t, err) + assert.True(t, ok, "request within both caps must be allowed") + }) +} + func TestService_validateAPIKeyPermissions(t *testing.T) { ctx := context.Background()