From 2aa8f1de2724ce6e1b4eb593ab2e5d216ae9a083 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 8 Oct 2026 20:07:01 +0200 Subject: [PATCH] fix(api): refuse direct RI exchange execution while 4-eyes mode is on executeExchange (AWS) and executeAzureExchange quote and run an irreversible exchange in one request, so a single user holding execute:ri-exchange bypassed the dual-control policy that the approve path enforces. With RequireDifferentApprover on, both now return 403 after authorization and before any quote or exchange call; a global-config lookup failure fails closed. Closes #517 Co-Authored-By: Claude Sonnet 5.5 --- internal/api/handler_ri_exchange.go | 82 ++++++++++++++----- .../api/handler_ri_exchange_azure_test.go | 29 +++++++ internal/api/handler_ri_exchange_test.go | 66 +++++++++++++++ 3 files changed, 155 insertions(+), 22 deletions(-) diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 30208fb71..80110685f 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -1202,6 +1202,11 @@ func (h *Handler) executeAzureExchange(ctx context.Context, req *events.LambdaFu return nil, err } + err = h.requireNoDirectExchangeUnderFourEyes(ctx) + if err != nil { + return nil, err + } + targets, err := toAzureExchangeTargets(body.Targets, body.SubscriptionID) if err != nil { return nil, err @@ -1898,28 +1903,7 @@ func (h *Handler) executeExchange(ctx context.Context, req *events.LambdaFunctio region := body.Region - // Enforce the per-permission Constraints configured on the granting - // execute:ri-exchange permission (SEC-01, issue #1141). RI exchanges - // are AWS EC2 only and region-scoped, and operate on the RIs of the - // deployment's own AWS account, so AccountIDs carries the registered - // cloud account the running deployment resolves to (fail closed on a - // resolution error; unattributedAccountConstraint when the deployment - // maps to no registered account, so an AccountIDs-constrained - // permission denies). The amount cap is checked against the caller's - // max_payment_due_usd guardrail, which ExecuteExchange independently - // enforces against the actual quoted payment due. - maxPayment, _ := maxRat.Float64() - cloudAccountID, err := h.resolveReshapeCloudAccountIDOrUnattributed(ctx) - if err != nil { - return nil, fmt.Errorf("failed to resolve cloud account scope: %w", err) - } - err = h.requirePermissionConstraints(ctx, session, "execute", "ri-exchange", []auth.PermissionConstraints{{ - AccountIDs: []string{cloudAccountID}, - Providers: []string{string(common.ProviderAWS)}, - Services: []string{string(common.ServiceEC2)}, - Regions: []string{region}, - MaxPurchaseAmount: maxPayment, - }}) + cloudAccountID, err := h.authorizeAWSExchangeExecution(ctx, session, region, maxRat) if err != nil { return nil, err } @@ -2486,6 +2470,60 @@ func (h *Handler) authorizeSessionApproveRIExchange(ctx context.Context, session return nil } +// authorizeAWSExchangeExecution applies the gates on the direct AWS execute +// endpoint that follow session permission and scope: the permission +// Constraints, then the 4-eyes policy. It returns the deployment's cloud +// account ID for the idempotency key. +func (h *Handler) authorizeAWSExchangeExecution(ctx context.Context, session *Session, region string, maxRat *big.Rat) (string, error) { + // Enforce the per-permission Constraints configured on the granting + // execute:ri-exchange permission (SEC-01, issue #1141). RI exchanges + // are AWS EC2 only and region-scoped, and operate on the RIs of the + // deployment's own AWS account, so AccountIDs carries the registered + // cloud account the running deployment resolves to (fail closed on a + // resolution error; unattributedAccountConstraint when the deployment + // maps to no registered account, so an AccountIDs-constrained + // permission denies). The amount cap is checked against the caller's + // max_payment_due_usd guardrail, which ExecuteExchange independently + // enforces against the actual quoted payment due. + maxPayment, _ := maxRat.Float64() + cloudAccountID, err := h.resolveReshapeCloudAccountIDOrUnattributed(ctx) + if err != nil { + return "", fmt.Errorf("failed to resolve cloud account scope: %w", err) + } + err = h.requirePermissionConstraints(ctx, session, "execute", "ri-exchange", []auth.PermissionConstraints{{ + AccountIDs: []string{cloudAccountID}, + Providers: []string{string(common.ProviderAWS)}, + Services: []string{string(common.ServiceEC2)}, + Regions: []string{region}, + MaxPurchaseAmount: maxPayment, + }}) + if err != nil { + return "", err + } + + err = h.requireNoDirectExchangeUnderFourEyes(ctx) + if err != nil { + return "", err + } + return cloudAccountID, nil +} + +// requireNoDirectExchangeUnderFourEyes refuses the direct execute endpoints +// while 4-eyes mode is on. A direct execute quotes and runs an irreversible +// exchange in one request, so there is no pending record and no second person +// to approve it; the approval flow (pending record + different approver) is the +// only path that satisfies the policy. +func (h *Handler) requireNoDirectExchangeUnderFourEyes(ctx context.Context) error { + cfg, err := h.config.GetGlobalConfig(ctx) + if err != nil { + return fmt.Errorf("4-eyes policy check: failed to load global config: %w", err) + } + if cfg != nil && cfg.RequireDifferentApprover { + return NewClientError(403, "direct RI exchange execution is disabled while 4-eyes approval mode is on; use the approval flow") + } + return nil +} + // requireDifferentRIExchangeApprover applies the purchase 4-eyes check when a creator // is recorded; automated exchanges have none and stay approvable (see issue #233). func (h *Handler) requireDifferentRIExchangeApprover(ctx context.Context, session *Session, record *config.RIExchangeRecord) error { diff --git a/internal/api/handler_ri_exchange_azure_test.go b/internal/api/handler_ri_exchange_azure_test.go index 8d569388c..9a221005b 100644 --- a/internal/api/handler_ri_exchange_azure_test.go +++ b/internal/api/handler_ri_exchange_azure_test.go @@ -2223,3 +2223,32 @@ func TestExchangeRegions(t *testing.T) { }) } } + +// TestExecuteAzureExchange_FourEyesRefusesDirectExecute pins issue #517 for +// Azure: with RequireDifferentApprover on, the direct execute endpoint refuses +// after authorization and before any quote or exchange call. The ops client +// registers only the ownership listing, so a CalculateExchange or +// ExecuteExchange call would fail the strict mock. +func TestExecuteAzureExchange_FourEyesRefusesDirectExecute(t *testing.T) { + ctx := context.Background() + opsClient := new(mockAzureExchangeOpsClient) + ownsAzureSource(opsClient) + t.Cleanup(func() { opsClient.AssertExpectations(t) }) + + h := newAzureExecuteSourceGateHandler(t, opsClient) + store, ok := h.config.(*MockConfigStore) + require.True(t, ok) + store.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{RequireDifferentApprover: true}, nil) + + _, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: validAzureExecuteBody, + }) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a ClientError, got: %v", err) + assert.Equal(t, 403, ce.code) + assert.Equal(t, "direct RI exchange execution is disabled while 4-eyes approval mode is on; use the approval flow", ce.Error()) + opsClient.AssertNotCalled(t, "ExecuteExchange", mock.Anything, mock.Anything) + opsClient.AssertNotCalled(t, "CalculateExchange", mock.Anything, mock.Anything, mock.Anything) +} diff --git a/internal/api/handler_ri_exchange_test.go b/internal/api/handler_ri_exchange_test.go index ad780569c..9afaea356 100644 --- a/internal/api/handler_ri_exchange_test.go +++ b/internal/api/handler_ri_exchange_test.go @@ -2552,3 +2552,69 @@ func TestExecuteApprovedExchange_LedgerWriteFailure_ReturnsError(t *testing.T) { assert.Contains(t, err.Error(), "exch-h4-test", "error must include the exchange ID for operator correlation with AWS") } + +// fourEyesExchangeBody is a valid AWS execute body used by the 4-eyes gate tests. +const fourEyesExchangeBody = `{"ri_ids":["ri-123"],"target_offering_id":"off-1","target_count":1,"max_payment_due_usd":"250.50","region":"eu-central-1"}` + +// newFourEyesExecuteExchangeHandler builds an AWS handler whose session clears +// permission, account scope and constraints, so the only remaining gate before +// any exchange side effect is the 4-eyes policy under test. +func newFourEyesExecuteExchangeHandler(t *testing.T, store *MockConfigStore) *Handler { + t.Helper() + ctx := context.Background() + const deploymentAccountID = "11111111-2222-3333-4444-555555555555" + + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + mockAuth.On("ValidateSession", ctx, "exchange-token").Return(&Session{UserID: "eeeeeeee-eeee-eeee-eeee-eeeeeeeeeeee"}, nil) + mockAuth.On("HasPermissionAPI", ctx, "eeeeeeee-eeee-eeee-eeee-eeeeeeeeeeee", "execute", "ri-exchange").Return(true, nil) + allowAnyAccountScope(mockAuth) + mockAuth.On("HasPermissionForConstraintsAPI", ctx, "eeeeeeee-eeee-eeee-eeee-eeeeeeeeeeee", "execute", "ri-exchange", mock.Anything).Return(true, nil) + + return &Handler{ + auth: mockAuth, + config: store, + reshapeAccountResolver: func(_ context.Context) (string, error) { + return deploymentAccountID, nil + }, + } +} + +// TestExecuteExchange_FourEyesRefusesDirectExecute pins issue #517: with +// RequireDifferentApprover on, the direct AWS execute endpoint must refuse +// before any exchange is attempted. The handler has no exchange client or +// idempotency store wired, so any code path past the gate would panic. +func TestExecuteExchange_FourEyesRefusesDirectExecute(t *testing.T) { + ctx := context.Background() + store := &MockConfigStore{} + store.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{RequireDifferentApprover: true}, nil) + h := newFourEyesExecuteExchangeHandler(t, store) + + _, err := h.executeExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer exchange-token"}, + Body: fourEyesExchangeBody, + }) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a ClientError, got: %v", err) + assert.Equal(t, 403, ce.code) + assert.Equal(t, "direct RI exchange execution is disabled while 4-eyes approval mode is on; use the approval flow", ce.Error()) +} + +// TestExecuteExchange_FourEyesPolicyLookupErrorFailsClosed: an unreadable +// global config must not be treated as "4-eyes off". +func TestExecuteExchange_FourEyesPolicyLookupErrorFailsClosed(t *testing.T) { + ctx := context.Background() + store := &MockConfigStore{} + store.On("GetGlobalConfig", mock.Anything).Return(nil, errors.New("db down")) + h := newFourEyesExecuteExchangeHandler(t, store) + + _, err := h.executeExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer exchange-token"}, + Body: fourEyesExchangeBody, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "4-eyes policy check") + _, isClient := IsClientError(err) + assert.False(t, isClient, "a lookup failure is a server error, not a client refusal") +}