From f6ef90a6052d6c7c7c834149bc204730c8ccf63a Mon Sep 17 00:00:00 2001 From: Julian Sommer <115516918+juliansommer@users.noreply.github.com> Date: Mon, 10 Aug 2026 12:28:03 +1000 Subject: [PATCH] fix: fail closed in the resource authorization check (#755) --- spinifex/gateway/acm_test.go | 8 +- spinifex/gateway/auth_test.go | 4 +- spinifex/gateway/bedrock_request_test.go | 27 ++-- .../ecr_createdeleterepository_test.go | 1 + .../gateway/ecr_describerepositories_test.go | 1 + .../gateway/ecr_getauthorizationtoken_test.go | 5 +- spinifex/gateway/ecr_image_test.go | 4 +- spinifex/gateway/ecr_test.go | 10 +- spinifex/gateway/ecs_test.go | 16 +- spinifex/gateway/eks_test.go | 10 +- spinifex/gateway/elbv2_test.go | 4 +- spinifex/gateway/gateway.go | 26 ++-- spinifex/gateway/gateway_test.go | 147 +++++++++++------- spinifex/gateway/iam_request_test.go | 6 +- spinifex/gateway/oidc_discovery_test.go | 2 +- spinifex/gateway/policy_testhelpers_test.go | 38 +++++ spinifex/gateway/rds_test.go | 32 ++-- spinifex/gateway/spinifex_modelaccess_test.go | 4 +- spinifex/gateway/spinifex_test.go | 20 ++- spinifex/gateway/tagging_test.go | 8 +- 20 files changed, 237 insertions(+), 136 deletions(-) create mode 100644 spinifex/gateway/policy_testhelpers_test.go diff --git a/spinifex/gateway/acm_test.go b/spinifex/gateway/acm_test.go index 2c7e984d5..0f7b3343f 100644 --- a/spinifex/gateway/acm_test.go +++ b/spinifex/gateway/acm_test.go @@ -19,7 +19,7 @@ func setupACMRequest(target, body string) *http.Request { } ctx := context.WithValue(req.Context(), ctxService, "acm") ctx = context.WithValue(ctx, ctxAccountID, "123456789012") - return req.WithContext(ctx) + return withTestIdentity(req.WithContext(ctx)) } func TestACMActionFromTarget(t *testing.T) { @@ -48,7 +48,7 @@ func TestACMActionsMap_AllActionsRegistered(t *testing.T) { } func TestACMRequest_MissingTarget(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} w := httptest.NewRecorder() err := gw.ACM_Request(w, setupACMRequest("", "")) require.Error(t, err) @@ -56,7 +56,7 @@ func TestACMRequest_MissingTarget(t *testing.T) { } func TestACMRequest_UnknownAction(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} w := httptest.NewRecorder() err := gw.ACM_Request(w, setupACMRequest("CertificateManager.BogusAction", "{}")) require.Error(t, err) @@ -66,7 +66,7 @@ func TestACMRequest_UnknownAction(t *testing.T) { // A known action with no NATS connection passes routing + policy and fails at // the NATS-availability guard. func TestACMRequest_KnownActionNoNATS(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} w := httptest.NewRecorder() err := gw.ACM_Request(w, setupACMRequest("CertificateManager.ListCertificates", "{}")) require.Error(t, err) diff --git a/spinifex/gateway/auth_test.go b/spinifex/gateway/auth_test.go index 7ea2168d2..78d2ddd09 100644 --- a/spinifex/gateway/auth_test.go +++ b/spinifex/gateway/auth_test.go @@ -580,7 +580,7 @@ func TestSigV4Auth_MalformedTimestamp(t *testing.T) { // --- writeSigV4Error Response Format Tests --- func TestWriteSigV4Error_ResponseFormat(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true, Region: testRegion} + gw := &GatewayConfig{DisableLogging: true, Region: testRegion, IAMService: allowAllIAMService()} testCases := []struct { errorCode string @@ -630,7 +630,7 @@ func TestWriteSigV4Error_ResponseFormat(t *testing.T) { } func TestWriteSigV4Error_IgnoresClientRequestID(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true, Region: testRegion} + gw := &GatewayConfig{DisableLogging: true, Region: testRegion, IAMService: allowAllIAMService()} w := httptest.NewRecorder() req := httptest.NewRequest(http.MethodGet, "/", nil) diff --git a/spinifex/gateway/bedrock_request_test.go b/spinifex/gateway/bedrock_request_test.go index 6352f6272..43cddde81 100644 --- a/spinifex/gateway/bedrock_request_test.go +++ b/spinifex/gateway/bedrock_request_test.go @@ -74,10 +74,10 @@ func (grantAllModels) Granted(_ context.Context, _, _ string) (bool, error) { re // newBedrockRequestGateway builds a GatewayConfig with a real NATS connection // (satisfying Bedrock_Request/BedrockRuntime_Request's nil-check only — no -// NATS subject handling is required for these routes), no IAMService (so -// checkPolicy is a no-op), BedrockEndpoints pinned at the given vLLM stub, and -// every model granted — access is deny-by-default, so without a resolver these -// routes would all return before reaching the code under test. +// NATS subject handling is required for these routes), an allow-everything +// IAMService so the policy gate passes, BedrockEndpoints pinned at the given +// vLLM stub, and every model granted — access is deny-by-default, so without a +// resolver these routes would all return before reaching the code under test. // ListFoundationModels/GetFoundationModel read the weights resolver through a // package-level setter rather than a GatewayConfig field, so it is installed // here and reset on cleanup. @@ -89,6 +89,7 @@ func newBedrockRequestGateway(t *testing.T, vllmURL string) *GatewayConfig { return &GatewayConfig{ NATSConn: nc, DisableLogging: true, + IAMService: allowAllIAMService(), BedrockEndpoints: map[string]string{ bedrockTestLlamaModelID: vllmURL, }, @@ -99,6 +100,8 @@ func newBedrockRequestGateway(t *testing.T, vllmURL string) *GatewayConfig { func bedrockRequestWithAccount(method, path, body string) *http.Request { req := httptest.NewRequest(method, path, strings.NewReader(body)) ctx := context.WithValue(req.Context(), ctxAccountID, bedrockTestAccount) + ctx = context.WithValue(ctx, ctxIdentity, "alice") + ctx = context.WithValue(ctx, ctxPrincipalType, principalTypeUser) return req.WithContext(ctx) } @@ -206,15 +209,17 @@ func TestBedrockRequest_UnknownRouteReturnsInvalidAction(t *testing.T) { assert.Equal(t, awserrors.ErrorInvalidAction, err.Error()) } -func TestBedrockRequest_MissingAccountIDReturnsServerInternal(t *testing.T) { +func TestBedrockRequest_MissingAccountIDReturnsInternalError(t *testing.T) { ts := newVLLMStub(t) gw := newBedrockRequestGateway(t, ts.URL) - req := httptest.NewRequest(http.MethodGet, "/foundation-models", nil) + req := withTestIdentity(httptest.NewRequest(http.MethodGet, "/foundation-models", nil)) w := httptest.NewRecorder() + // Account-less requests are rejected by the policy gate, which runs ahead of + // the handler's own account guard. err := gw.Bedrock_Request(w, req) require.Error(t, err) - assert.Equal(t, awserrors.ErrorServerInternal, err.Error()) + assert.Equal(t, awserrors.ErrorInternalError, err.Error()) } func TestBedrockRuntimeRequest_Converse(t *testing.T) { @@ -266,15 +271,17 @@ func TestBedrockRuntimeRequest_InvokeModel(t *testing.T) { }`, w.Body.String()) } -func TestBedrockRuntimeRequest_MissingAccountIDReturnsServerInternal(t *testing.T) { +func TestBedrockRuntimeRequest_MissingAccountIDReturnsInternalError(t *testing.T) { ts := newVLLMStub(t) gw := newBedrockRequestGateway(t, ts.URL) - req := httptest.NewRequest(http.MethodPost, "/model/"+bedrockTestLlamaModelID+"/converse", strings.NewReader("{}")) + req := withTestIdentity(httptest.NewRequest(http.MethodPost, "/model/"+bedrockTestLlamaModelID+"/converse", strings.NewReader("{}"))) w := httptest.NewRecorder() + // Account-less requests are rejected by the policy gate, which runs ahead of + // the handler's own account guard. err := gw.BedrockRuntime_Request(w, req) require.Error(t, err) - assert.Equal(t, awserrors.ErrorServerInternal, err.Error()) + assert.Equal(t, awserrors.ErrorInternalError, err.Error()) } // newVLLMStreamStub stands up an httptest server answering the OpenAI diff --git a/spinifex/gateway/ecr_createdeleterepository_test.go b/spinifex/gateway/ecr_createdeleterepository_test.go index fe68eabc9..b7e2ebfce 100644 --- a/spinifex/gateway/ecr_createdeleterepository_test.go +++ b/spinifex/gateway/ecr_createdeleterepository_test.go @@ -30,6 +30,7 @@ func newRepoLifecycleGateway(t *testing.T) (*GatewayConfig, *nats.Conn) { serveECRMeta(t, nc, handlers_ecr.SubjectManifestList, svc.ManifestList) gw := &GatewayConfig{ NATSConn: nc, Region: ecrTestRegion, InternalSuffix: ecrTestSuffix, DisableLogging: true, + IAMService: allowAllIAMService(), } return gw, nc } diff --git a/spinifex/gateway/ecr_describerepositories_test.go b/spinifex/gateway/ecr_describerepositories_test.go index 55b60ba0e..8f53766b0 100644 --- a/spinifex/gateway/ecr_describerepositories_test.go +++ b/spinifex/gateway/ecr_describerepositories_test.go @@ -55,6 +55,7 @@ func newDescribeReposGateway(t *testing.T, repos ...string) *GatewayConfig { } return &GatewayConfig{ NATSConn: nc, Region: ecrTestRegion, InternalSuffix: ecrTestSuffix, DisableLogging: true, + IAMService: allowAllIAMService(), } } diff --git a/spinifex/gateway/ecr_getauthorizationtoken_test.go b/spinifex/gateway/ecr_getauthorizationtoken_test.go index fd6a8dfc9..2079d3fab 100644 --- a/spinifex/gateway/ecr_getauthorizationtoken_test.go +++ b/spinifex/gateway/ecr_getauthorizationtoken_test.go @@ -18,6 +18,7 @@ func TestHandleGetAuthorizationToken_MintsUsableToken(t *testing.T) { gw := &GatewayConfig{ Region: ecrTestRegion, InternalSuffix: ecrTestSuffix, ECRTokenIssuer: iss, ECRTokenVerifier: verify, DisableLogging: true, + IAMService: allowAllIAMService(), } req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader("{}")) @@ -61,6 +62,7 @@ func TestHandleGetAuthorizationToken_ProxyEndpointCarriesPort(t *testing.T) { gw := &GatewayConfig{ Region: ecrTestRegion, InternalSuffix: ecrTestSuffix, RegistryPort: "9999", ECRTokenIssuer: iss, ECRTokenVerifier: verify, DisableLogging: true, + IAMService: allowAllIAMService(), } req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader("{}")) @@ -82,7 +84,7 @@ func TestHandleGetAuthorizationToken_ProxyEndpointCarriesPort(t *testing.T) { } func TestHandleGetAuthorizationToken_NoIssuerNotImplemented(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader("{}")) ctx := context.WithValue(req.Context(), ctxAccountID, ecrTestAccount) err := gw.handleGetAuthorizationToken(httptest.NewRecorder(), req.WithContext(ctx)) @@ -94,6 +96,7 @@ func TestECRRequest_GetAuthorizationTokenDispatched(t *testing.T) { gw := &GatewayConfig{ Region: ecrTestRegion, InternalSuffix: ecrTestSuffix, ECRTokenIssuer: iss, ECRTokenVerifier: verify, DisableLogging: true, + IAMService: allowAllIAMService(), } w := httptest.NewRecorder() req := setupECRRequest("AmazonEC2ContainerRegistry_V20150921.GetAuthorizationToken", "{}") diff --git a/spinifex/gateway/ecr_image_test.go b/spinifex/gateway/ecr_image_test.go index 6d8731574..1bc823f57 100644 --- a/spinifex/gateway/ecr_image_test.go +++ b/spinifex/gateway/ecr_image_test.go @@ -23,7 +23,7 @@ import ( func newImageGateway(t *testing.T) *GatewayConfig { t.Helper() reg := gateway_ecr.NewRegistry(objectstore.NewMemoryObjectStore(), handlers_ecr.NewMemoryMetaStore(), ecrTestAccount) - return &GatewayConfig{ECRRegistry: reg, Region: ecrTestRegion, InternalSuffix: ecrTestSuffix, DisableLogging: true} + return &GatewayConfig{ECRRegistry: reg, Region: ecrTestRegion, InternalSuffix: ecrTestSuffix, DisableLogging: true, IAMService: allowAllIAMService()} } // seedGatewayRepo creates the repository metadata so push/PutImage handlers — @@ -233,7 +233,7 @@ func TestImageHandlers_GuardRails(t *testing.T) { assert.Equal(t, "ServerInternal", err.Error()) // Nil registry -> ServerInternal. - bare := &GatewayConfig{DisableLogging: true} + bare := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} err = bare.handleListImages(httptest.NewRecorder(), imageReq(t, `{"repositoryName":"team/app"}`)) require.Error(t, err) assert.Equal(t, "ServerInternal", err.Error()) diff --git a/spinifex/gateway/ecr_test.go b/spinifex/gateway/ecr_test.go index 06b7d472b..e9bc54406 100644 --- a/spinifex/gateway/ecr_test.go +++ b/spinifex/gateway/ecr_test.go @@ -53,14 +53,14 @@ func TestECRActionsMap_CoreActionsRegistered(t *testing.T) { } func TestECRRequest_MissingTarget(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} err := gw.ECR_Request(httptest.NewRecorder(), setupECRRequest("", "")) require.Error(t, err) assert.Equal(t, awserrors.ErrorMissingAction, err.Error()) } func TestECRRequest_UnknownAction(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} err := gw.ECR_Request(httptest.NewRecorder(), setupECRRequest("AmazonEC2ContainerRegistry_V20150921.MadeUpAction", "{}")) require.Error(t, err) @@ -71,7 +71,7 @@ func TestECRRequest_UnknownAction(t *testing.T) { // handler lands. The repo/image actions are now served inline, so // ListRepositories stands in as a still-stubbed action. func TestECRRequest_KnownActionNotImplemented(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} err := gw.ECR_Request(httptest.NewRecorder(), setupECRRequest("AmazonEC2ContainerRegistry_V20150921.ListRepositories", "{}")) require.Error(t, err) @@ -79,7 +79,7 @@ func TestECRRequest_KnownActionNotImplemented(t *testing.T) { } func TestOCIRegistry_VersionCheck(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} r := chi.NewRouter() gw.mountOCIRegistry(r) @@ -91,7 +91,7 @@ func TestOCIRegistry_VersionCheck(t *testing.T) { } func TestOCIRegistry_StubReturns501(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} r := chi.NewRouter() gw.mountOCIRegistry(r) diff --git a/spinifex/gateway/ecs_test.go b/spinifex/gateway/ecs_test.go index 796d5d0a2..d1c92ced6 100644 --- a/spinifex/gateway/ecs_test.go +++ b/spinifex/gateway/ecs_test.go @@ -20,7 +20,7 @@ func setupECSRequest(target, body string) *http.Request { } ctx := context.WithValue(req.Context(), ctxService, "ecs") ctx = context.WithValue(ctx, ctxAccountID, "123456789012") - return req.WithContext(ctx) + return withTestIdentity(req.WithContext(ctx)) } func TestECSActionFromTarget(t *testing.T) { @@ -62,14 +62,14 @@ func TestECSActionsMap_NamespaceRegistered(t *testing.T) { } func TestECSRequest_MissingTarget(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} err := gw.ECS_Request(httptest.NewRecorder(), setupECSRequest("", "")) require.Error(t, err) assert.Equal(t, awserrors.ErrorMissingAction, err.Error()) } func TestECSRequest_UnknownAction(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} err := gw.ECS_Request(httptest.NewRecorder(), setupECSRequest("AmazonEC2ContainerServiceV20141113.MadeUpAction", "{}")) require.Error(t, err) @@ -78,7 +78,7 @@ func TestECSRequest_UnknownAction(t *testing.T) { // An action whose real handler has not yet landed still resolves to the 501 stub. func TestECSRequest_KnownActionNotImplemented(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} err := gw.ECS_Request(httptest.NewRecorder(), setupECSRequest("AmazonEC2ContainerServiceV20141113.UpdateCluster", "{}")) require.Error(t, err) @@ -90,10 +90,12 @@ func TestECSRequest_KnownActionNotImplemented(t *testing.T) { func TestECSRequest_MissingAccountID(t *testing.T) { req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader("{}")) req.Header.Set("X-Amz-Target", "AmazonEC2ContainerServiceV20141113.ListClusters") - req = req.WithContext(context.WithValue(req.Context(), ctxService, "ecs")) + req = withTestIdentity(req.WithContext(context.WithValue(req.Context(), ctxService, "ecs"))) - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} + // Account-less requests are rejected by the policy gate, which runs ahead of + // the handler's own account guard. err := gw.ECS_Request(httptest.NewRecorder(), req) require.Error(t, err) - assert.Equal(t, awserrors.ErrorServerInternal, err.Error()) + assert.Equal(t, awserrors.ErrorInternalError, err.Error()) } diff --git a/spinifex/gateway/eks_test.go b/spinifex/gateway/eks_test.go index 5ceae844f..0960e0e6f 100644 --- a/spinifex/gateway/eks_test.go +++ b/spinifex/gateway/eks_test.go @@ -174,11 +174,11 @@ func TestLookupEKSAction_UnknownReturnsFalse(t *testing.T) { } func TestEKSRequest_UnknownActionReturnsInvalidAction(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} req := httptest.NewRequest(http.MethodGet, "/totally-unknown", nil) ctx := context.WithValue(req.Context(), ctxService, "eks") ctx = context.WithValue(ctx, ctxAccountID, "111122223333") - req = req.WithContext(ctx) + req = withTestIdentity(req.WithContext(ctx)) w := httptest.NewRecorder() err := gw.EKS_Request(w, req) @@ -187,11 +187,11 @@ func TestEKSRequest_UnknownActionReturnsInvalidAction(t *testing.T) { } func TestEKSRequest_MissingNATSReturnsServerInternal(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} req := httptest.NewRequest(http.MethodGet, "/clusters", nil) ctx := context.WithValue(req.Context(), ctxService, "eks") ctx = context.WithValue(ctx, ctxAccountID, "111122223333") - req = req.WithContext(ctx) + req = withTestIdentity(req.WithContext(ctx)) w := httptest.NewRecorder() err := gw.EKS_Request(w, req) @@ -200,7 +200,7 @@ func TestEKSRequest_MissingNATSReturnsServerInternal(t *testing.T) { } func TestErrorHandler_EKSEmitsJSONNotXML(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} req := httptest.NewRequest(http.MethodGet, "/clusters", nil) ctx := context.WithValue(req.Context(), ctxService, "eks") req = req.WithContext(ctx) diff --git a/spinifex/gateway/elbv2_test.go b/spinifex/gateway/elbv2_test.go index 6af333fda..e75c65c8d 100644 --- a/spinifex/gateway/elbv2_test.go +++ b/spinifex/gateway/elbv2_test.go @@ -20,7 +20,7 @@ func setupELBv2Request(body string) *http.Request { } func TestELBv2Request_MissingAction(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} w := httptest.NewRecorder() err := gw.ELBv2_Request(w, setupELBv2Request("")) require.Error(t, err) @@ -28,7 +28,7 @@ func TestELBv2Request_MissingAction(t *testing.T) { } func TestELBv2Request_UnknownAction(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} w := httptest.NewRecorder() err := gw.ELBv2_Request(w, setupELBv2Request("Action=FakeAction")) require.Error(t, err) diff --git a/spinifex/gateway/gateway.go b/spinifex/gateway/gateway.go index c6581400a..7820f0a41 100644 --- a/spinifex/gateway/gateway.go +++ b/spinifex/gateway/gateway.go @@ -447,9 +447,10 @@ func (gw *GatewayConfig) checkPolicy(r *http.Request, service, action string) er } // checkPolicyResource evaluates IAM policies against a specific resource ARN. -// Root users bypass evaluation. Nil IAMService, or a request with no SigV4 -// auth context, allows (pre-IAM compatibility). Used by EC2 paths that -// enforce iam:PassRole before attaching an instance profile. +// Root users bypass evaluation. A nil IAMService is a server fault and an +// unauthenticated request is denied — neither can reach a gated handler through +// the route tree. Used by EC2 paths that enforce iam:PassRole before attaching +// an instance profile. func (gw *GatewayConfig) checkPolicyResource(r *http.Request, service, action, resource string) error { // Every dispatcher — query-protocol and REST-JSON alike — reaches this // point with its resolved action, so telemetry enrichment lives here @@ -458,15 +459,14 @@ func (gw *GatewayConfig) checkPolicyResource(r *http.Request, service, action, r recordResolvedAction(r.Context(), service, action) if gw.IAMService == nil { - slog.Warn("checkPolicy: IAM service not available, skipping policy check", - "service", service, "action", action) - return nil + slog.Error("checkPolicy: IAM service not available", "service", service, "action", action) + return errors.New(awserrors.ErrorInternalError) } identityVal := r.Context().Value(ctxIdentity) if identityVal == nil { - // No auth context — pre-IAM compatibility - return nil + slog.Warn("checkPolicy: request carries no auth context", "service", service, "action", action) + return errors.New(awserrors.ErrorAccessDenied) } identity, ok := identityVal.(string) if !ok { @@ -474,9 +474,9 @@ func (gw *GatewayConfig) checkPolicyResource(r *http.Request, service, action, r return errors.New(awserrors.ErrorInternalError) } if identity == "" { - // Pre-IAM compatibility: an authenticated request with no identity name - // predates per-principal policy enforcement. - return nil + slog.Warn("checkPolicy: authenticated request carries no identity name", + "service", service, "action", action) + return errors.New(awserrors.ErrorAccessDenied) } accountID, _ := r.Context().Value(ctxAccountID).(string) if accountID == "" { @@ -506,9 +506,7 @@ func mustCtxString(r *http.Request, key contextKey) string { // enforcement: given an already-resolved principal (from SigV4 or, for /v2/* // ECR requests, a freshly rehydrated token identity), it resolves the // principal's current policies and evaluates iamAction against resource. -// Unlike checkPolicyResource, a nil IAMService fails closed here — callers -// that want pre-IAM-compatibility bypass behavior must check for that before -// calling in, exactly as checkPolicyResource does. +// A nil IAMService fails closed here, matching checkPolicyResource. func (gw *GatewayConfig) evaluatePrincipalPolicy(principal principalContext, iamAction, resource string) error { if gw.IAMService == nil { slog.Error("evaluatePrincipalPolicy: IAM service not available", "action", iamAction) diff --git a/spinifex/gateway/gateway_test.go b/spinifex/gateway/gateway_test.go index f02218c34..14b41191d 100644 --- a/spinifex/gateway/gateway_test.go +++ b/spinifex/gateway/gateway_test.go @@ -157,7 +157,7 @@ func TestGenerateIAMErrorResponse_ValidXML(t *testing.T) { } func TestErrorHandler_IAMService(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ctx := context.WithValue(r.Context(), ctxService, "iam") @@ -179,7 +179,7 @@ func TestErrorHandler_IAMService(t *testing.T) { } func TestErrorHandler_UnknownError(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ctx := context.WithValue(r.Context(), ctxService, "ec2") @@ -200,7 +200,7 @@ func TestErrorHandler_UnknownError(t *testing.T) { } func TestErrorHandler_WrappedErrorCode(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ctx := context.WithValue(r.Context(), ctxService, "ec2") @@ -224,7 +224,7 @@ func TestErrorHandler_WrappedErrorCode(t *testing.T) { // fidelity gap: a call site that names the blocking resource via awserrors.Errorf // must have that wording reach the client instead of the generic ErrorLookup text. func TestErrorHandler_PrefersCallSiteMessage(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ctx := context.WithValue(r.Context(), ctxService, "ec2") @@ -249,7 +249,7 @@ func TestErrorHandler_PrefersCallSiteMessage(t *testing.T) { // ResourceInUseException collision check: ACM's DeleteCertificate must not // surface EKS's "cluster already exists" wording for the shared wire code. func TestErrorHandler_ACMResourceInUse_UsesACMWording(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} req := httptest.NewRequest(http.MethodPost, "/", nil) ctx := context.WithValue(req.Context(), ctxService, "acm") req = req.WithContext(ctx) @@ -270,7 +270,7 @@ func TestErrorHandler_ACMResourceInUse_UsesACMWording(t *testing.T) { // own ResourceInUseException must keep its existing wording, unaffected by the // ACM-specific override. func TestErrorHandler_EKSResourceInUse_UsesEKSWording(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} req := httptest.NewRequest(http.MethodPost, "/", nil) ctx := context.WithValue(req.Context(), ctxService, "eks") req = req.WithContext(ctx) @@ -290,7 +290,7 @@ func TestErrorHandler_EKSResourceInUse_UsesEKSWording(t *testing.T) { // case: an error path that supplies no message (the vast majority of call // sites) must keep rendering exactly today's ErrorLookup text, unchanged. func TestErrorHandler_NoMessageSupplied_MatchesErrorLookup(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ctx := context.WithValue(r.Context(), ctxService, "ec2") @@ -309,7 +309,7 @@ func TestErrorHandler_NoMessageSupplied_MatchesErrorLookup(t *testing.T) { } func TestErrorHandler_ELBv2Service(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ctx := context.WithValue(r.Context(), ctxService, "elasticloadbalancing") @@ -331,7 +331,7 @@ func TestErrorHandler_ELBv2Service(t *testing.T) { } func TestErrorHandler_EC2Service(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ctx := context.WithValue(r.Context(), ctxService, "ec2") @@ -351,7 +351,7 @@ func TestErrorHandler_EC2Service(t *testing.T) { } func TestErrorHandler_IgnoresClientRequestID(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ctx := context.WithValue(r.Context(), ctxService, "ec2") @@ -369,7 +369,7 @@ func TestErrorHandler_IgnoresClientRequestID(t *testing.T) { } func TestErrorHandler_ContentTypeXML(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ctx := context.WithValue(r.Context(), ctxService, "ec2") @@ -694,7 +694,7 @@ func TestParseAWSQueryArgs_CapturedClientBodies(t *testing.T) { } func TestGetService(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} tests := []struct { name string @@ -789,7 +789,7 @@ func TestGetService(t *testing.T) { } func TestRequest_NoServiceContext(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} req := httptest.NewRequest(http.MethodPost, "/", nil) w := httptest.NewRecorder() @@ -802,7 +802,7 @@ func TestRequest_NoServiceContext(t *testing.T) { } func TestRequest_UnsupportedService(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} req := httptest.NewRequest(http.MethodPost, "/", nil) ctx := context.WithValue(req.Context(), ctxService, "s3") req = req.WithContext(ctx) @@ -830,7 +830,7 @@ func TestRequest_MalformedQueryString_EndToEnd(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { // A live NATS connection bypasses the cluster-unavailable gate. - gw := &GatewayConfig{DisableLogging: true, NATSConn: connectedNATS(t)} + gw := &GatewayConfig{DisableLogging: true, NATSConn: connectedNATS(t), IAMService: allowAllIAMService()} req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(tc.body)) ctx := context.WithValue(req.Context(), ctxService, tc.service) ctx = context.WithValue(ctx, ctxAccountID, "123456789012") @@ -848,7 +848,7 @@ func TestRequest_MalformedQueryString_EndToEnd(t *testing.T) { } func TestRequest_EC2MissingAction(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true, NATSConn: connectedNATS(t)} + gw := &GatewayConfig{DisableLogging: true, NATSConn: connectedNATS(t), IAMService: allowAllIAMService()} req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader("")) ctx := context.WithValue(req.Context(), ctxService, "ec2") ctx = context.WithValue(ctx, ctxAccountID, "123456789012") @@ -899,12 +899,11 @@ func setupEC2Request(body string, accountID string) *http.Request { if accountID != "" { ctx = context.WithValue(ctx, ctxAccountID, accountID) } - req = req.WithContext(ctx) - return req + return withTestIdentity(req.WithContext(ctx)) } func TestEC2Request_MissingAction(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} req := setupEC2Request("", "123456789012") w := httptest.NewRecorder() @@ -914,7 +913,7 @@ func TestEC2Request_MissingAction(t *testing.T) { } func TestEC2Request_UnknownAction(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} req := setupEC2Request("Action=FakeAction", "123456789012") w := httptest.NewRecorder() @@ -924,7 +923,7 @@ func TestEC2Request_UnknownAction(t *testing.T) { } func TestEC2Request_NilNATSNonLocalAction(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true, NATSConn: nil} + gw := &GatewayConfig{DisableLogging: true, NATSConn: nil, IAMService: allowAllIAMService()} req := setupEC2Request("Action=DescribeInstances", "123456789012") w := httptest.NewRecorder() @@ -939,6 +938,7 @@ func TestEC2Request_NilNATSLocalAction(t *testing.T) { NATSConn: nil, Region: "us-east-1", AZ: "us-east-1a", + IAMService: allowAllIAMService(), } req := setupEC2Request("Action=DescribeRegions", "123456789012") w := httptest.NewRecorder() @@ -955,18 +955,19 @@ func TestEC2Request_NilNATSLocalAction(t *testing.T) { } func TestEC2Request_MissingAccountID(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true, NATSConn: nil} + gw := &GatewayConfig{DisableLogging: true, NATSConn: nil, IAMService: allowAllIAMService()} // Use a local action so we don't fail on nil NATS first req := setupEC2Request("Action=DescribeRegions", "") w := httptest.NewRecorder() + // The policy gate rejects an account-less request before the handler's own guard. err := gw.EC2_Request(w, req) require.Error(t, err) - assert.Equal(t, awserrors.ErrorServerInternal, err.Error()) + assert.Equal(t, awserrors.ErrorInternalError, err.Error()) } func TestEC2Request_DescribeAccountAttributes(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true, NATSConn: nil} + gw := &GatewayConfig{DisableLogging: true, NATSConn: nil, IAMService: allowAllIAMService()} req := setupEC2Request("Action=DescribeAccountAttributes", "123456789012") w := httptest.NewRecorder() @@ -987,6 +988,7 @@ func TestEC2Request_DescribeAvailabilityZones(t *testing.T) { NATSConn: nil, Region: "us-east-1", AZ: "us-east-1a", + IAMService: allowAllIAMService(), } req := setupEC2Request("Action=DescribeAvailabilityZones", "123456789012") w := httptest.NewRecorder() @@ -1002,20 +1004,64 @@ func TestEC2Request_DescribeAvailabilityZones(t *testing.T) { assert.Contains(t, string(body), "DescribeAvailabilityZonesResponse") } -func TestCheckPolicy_NilIAMService(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true, IAMService: nil} - req := httptest.NewRequest(http.MethodPost, "/", nil) +// TestCheckPolicy_FailsClosed covers the three states that once returned allow +// before any policy was consulted. A missing service is a server fault; a +// request with no resolved principal is denied. +func TestCheckPolicy_FailsClosed(t *testing.T) { + authed := func(identity string) *http.Request { + req := httptest.NewRequest(http.MethodPost, "/", nil) + ctx := context.WithValue(req.Context(), ctxIdentity, identity) + ctx = context.WithValue(ctx, ctxAccountID, "123456789012") + ctx = context.WithValue(ctx, ctxPrincipalType, principalTypeUser) + return req.WithContext(ctx) + } - err := gw.checkPolicy(req, "ec2", "DescribeInstances") - assert.NoError(t, err) + cases := []struct { + name string + gw *GatewayConfig + req *http.Request + wantErr string + }{ + { + name: "nil IAM service", + gw: &GatewayConfig{DisableLogging: true, IAMService: nil}, + req: authed("alice"), + wantErr: awserrors.ErrorInternalError, + }, + { + name: "no auth context", + gw: &GatewayConfig{DisableLogging: true, IAMService: &mockIAMService{}}, + req: httptest.NewRequest(http.MethodPost, "/", nil), + wantErr: awserrors.ErrorAccessDenied, + }, + { + name: "empty identity", + gw: &GatewayConfig{DisableLogging: true, IAMService: &mockIAMService{}}, + req: authed(""), + wantErr: awserrors.ErrorAccessDenied, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := tc.gw.checkPolicy(tc.req, "ec2", "DescribeInstances") + require.Error(t, err) + assert.Equal(t, tc.wantErr, err.Error()) + }) + } } -func TestCheckPolicy_NoIdentityInContext(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true, IAMService: &mockIAMService{}} +// TestCheckPolicy_PassRoleFailsClosed proves the change reaches a real gated +// call site, not just the helper: an unauthenticated RunInstances carrying an +// instance profile is denied rather than passing the iam:PassRole check. +func TestCheckPolicy_PassRoleFailsClosed(t *testing.T) { + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} req := httptest.NewRequest(http.MethodPost, "/", nil) - err := gw.checkPolicy(req, "ec2", "DescribeInstances") - assert.NoError(t, err) + err := gw.checkPolicyResource(req, "iam", "PassRole", + "arn:aws:iam::123456789012:role/app") + require.Error(t, err) + assert.Equal(t, awserrors.ErrorAccessDenied, err.Error()) } func TestCheckPolicy_RootUserGlobalAccount(t *testing.T) { @@ -1132,18 +1178,6 @@ func TestCheckPolicy_GetUserPoliciesError(t *testing.T) { assert.Equal(t, awserrors.ErrorInternalError, err.Error()) } -func TestCheckPolicy_EmptyIdentity(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true, IAMService: &mockIAMService{}} - req := httptest.NewRequest(http.MethodPost, "/", nil) - ctx := context.WithValue(req.Context(), ctxIdentity, "") - ctx = context.WithValue(ctx, ctxAccountID, "123456789012") - ctx = context.WithValue(ctx, ctxPrincipalType, principalTypeUser) - req = req.WithContext(ctx) - - err := gw.checkPolicy(req, "ec2", "DescribeInstances") - assert.NoError(t, err) -} - func TestCheckPolicy_MissingAccountID(t *testing.T) { gw := &GatewayConfig{DisableLogging: true, IAMService: &mockIAMService{}} req := httptest.NewRequest(http.MethodPost, "/", nil) @@ -1245,7 +1279,7 @@ func TestImportKeyPair_Base64PaddingWorkaround(t *testing.T) { "PublicKeyMaterial": "c3NoLXJzYSBBQUFBQjNOemFDMXljMkVBQUFBREFRQUJBQUFCZ1FD%3D%3D", } - gw := &GatewayConfig{DisableLogging: true, NATSConn: nil} + gw := &GatewayConfig{DisableLogging: true, NATSConn: nil, IAMService: allowAllIAMService()} // NATS is nil so the handler errors, but PublicKeyMaterial is modified before that. _, _ = handler("ImportKeyPair", q, gw, "123456789012", nil) @@ -1258,7 +1292,7 @@ func TestImportKeyPair_Base64PaddingWorkaround(t *testing.T) { // --- Throttle middleware integration tests --- func TestThrottleKeyFuncs_ExtractsAccountAndAction(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} keyFuncs := gw.throttleKeyFuncs() require.Len(t, keyFuncs, 2) @@ -1277,7 +1311,7 @@ func TestThrottleKeyFuncs_ExtractsAccountAndAction(t *testing.T) { } func TestThrottleKeyFuncs_UnknownAction(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} keyFuncs := gw.throttleKeyFuncs() // No ctxAction in context — should return "unknown". @@ -1291,7 +1325,7 @@ func TestThrottleKeyFuncs_UnknownAction(t *testing.T) { } func TestThrottleKeyFuncs_MissingAccountID(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} keyFuncs := gw.throttleKeyFuncs() // No ctxAccountID in context — should return an error. @@ -1303,7 +1337,7 @@ func TestThrottleKeyFuncs_MissingAccountID(t *testing.T) { } func TestWriteThrottleError_EC2(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} req := httptest.NewRequest(http.MethodPost, "/", nil) ctx := context.WithValue(req.Context(), ctxService, "ec2") @@ -1321,7 +1355,7 @@ func TestWriteThrottleError_EC2(t *testing.T) { } func TestWriteThrottleError_IAM(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} req := httptest.NewRequest(http.MethodPost, "/", nil) ctx := context.WithValue(req.Context(), ctxService, "iam") @@ -1350,6 +1384,7 @@ func TestThrottleMiddleware_Integration(t *testing.T) { gw := &GatewayConfig{ DisableLogging: true, Throttler: throttler, + IAMService: allowAllIAMService(), } inner := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { @@ -1387,7 +1422,7 @@ func TestThrottleMiddleware_Integration(t *testing.T) { func TestThrottleMiddleware_DisabledConfig(t *testing.T) { // When Throttler is nil, SetupRoutes skips middleware — no panic. - gw := &GatewayConfig{DisableLogging: true, Throttler: nil} + gw := &GatewayConfig{DisableLogging: true, Throttler: nil, IAMService: allowAllIAMService()} handler := gw.SetupRoutes() req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader("Action=DescribeInstances")) @@ -1411,7 +1446,7 @@ func TestThrottleMiddleware_PerActionIsolation(t *testing.T) { throttler := ratelimit.New(cfg) defer throttler.Stop() - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} inner := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) @@ -1442,7 +1477,7 @@ func TestThrottleMiddleware_PerActionIsolation(t *testing.T) { } func TestRequest_ClusterUnavailableNilConn_EC2(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} req := httptest.NewRequest(http.MethodPost, "/", nil) ctx := context.WithValue(req.Context(), ctxService, "ec2") @@ -1462,7 +1497,7 @@ func TestRequest_ClusterUnavailableNilConn_EC2(t *testing.T) { } func TestRequest_ClusterUnavailableNilConn_IAM(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} req := httptest.NewRequest(http.MethodPost, "/", nil) ctx := context.WithValue(req.Context(), ctxService, "iam") @@ -1485,7 +1520,7 @@ func TestRequest_ClusterUnavailableClosedConn(t *testing.T) { require.NoError(t, err) nc.Close() - gw := &GatewayConfig{DisableLogging: true, NATSConn: nc} + gw := &GatewayConfig{DisableLogging: true, NATSConn: nc, IAMService: allowAllIAMService()} req := httptest.NewRequest(http.MethodPost, "/", nil) ctx := context.WithValue(req.Context(), ctxService, "ec2") diff --git a/spinifex/gateway/iam_request_test.go b/spinifex/gateway/iam_request_test.go index 5b9af5de7..cf7462198 100644 --- a/spinifex/gateway/iam_request_test.go +++ b/spinifex/gateway/iam_request_test.go @@ -162,6 +162,8 @@ func setupIAMRequestHandler(svc handlers_iam.IAMService) http.Handler { ctx := r.Context() ctx = context.WithValue(ctx, ctxService, "iam") ctx = context.WithValue(ctx, ctxAccountID, "000000000000") + ctx = context.WithValue(ctx, ctxIdentity, "root") + ctx = context.WithValue(ctx, ctxPrincipalType, principalTypeUser) r = r.WithContext(ctx) if err := gw.IAM_Request(w, r); err != nil { gw.ErrorHandler(w, r, err) @@ -287,6 +289,8 @@ func TestIAMRequest_NilService(t *testing.T) { ctx := r.Context() ctx = context.WithValue(ctx, ctxService, "iam") ctx = context.WithValue(ctx, ctxAccountID, "000000000000") + ctx = context.WithValue(ctx, ctxIdentity, "root") + ctx = context.WithValue(ctx, ctxPrincipalType, principalTypeUser) r = r.WithContext(ctx) if err := gw.IAM_Request(w, r); err != nil { gw.ErrorHandler(w, r, err) @@ -330,7 +334,7 @@ func TestIAMRequest_MissingAccountID(t *testing.T) { } handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ctx := context.WithValue(r.Context(), ctxService, "iam") - r = r.WithContext(ctx) + r = withTestIdentity(r.WithContext(ctx)) if err := gw.IAM_Request(w, r); err != nil { gw.ErrorHandler(w, r, err) } diff --git a/spinifex/gateway/oidc_discovery_test.go b/spinifex/gateway/oidc_discovery_test.go index 757820271..b3672baf7 100644 --- a/spinifex/gateway/oidc_discovery_test.go +++ b/spinifex/gateway/oidc_discovery_test.go @@ -38,7 +38,7 @@ func seedDiscoveryCluster(t *testing.T) http.Handler { t.Fatalf("put jwks: %v", err) } - gw := &GatewayConfig{DisableLogging: true, NATSConn: nc} + gw := &GatewayConfig{DisableLogging: true, NATSConn: nc, IAMService: allowAllIAMService()} return gw.SetupRoutes() } diff --git a/spinifex/gateway/policy_testhelpers_test.go b/spinifex/gateway/policy_testhelpers_test.go new file mode 100644 index 000000000..dcd54f166 --- /dev/null +++ b/spinifex/gateway/policy_testhelpers_test.go @@ -0,0 +1,38 @@ +package gateway + +import ( + "context" + "net/http" + + handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam" +) + +// allowAllIAMService resolves an allow-everything policy for every principal. +// Tests that exercise handler behaviour rather than authorization use it so the +// policy gate passes on a real evaluation instead of a bypass. +func allowAllIAMService() *policyMockIAMService { + allow := []handlers_iam.PolicyDocument{{ + Version: "2012-10-17", + Statement: []handlers_iam.Statement{ + {Effect: "Allow", Action: handlers_iam.StringOrArr{"*"}, Resource: handlers_iam.StringOrArr{"*"}}, + }, + }} + return &policyMockIAMService{ + getUserPoliciesFn: func(_, _ string) ([]handlers_iam.PolicyDocument, error) { return allow, nil }, + getRolePoliciesFn: func(_, _ string) ([]handlers_iam.PolicyDocument, error) { return allow, nil }, + } +} + +// withTestIdentity attaches the identity half of the auth context the SigV4 +// middleware sets, so a hand-built request reaches the handler instead of being +// denied. Existing values are preserved for tests that set their own principal. +func withTestIdentity(req *http.Request) *http.Request { + ctx := req.Context() + if ctx.Value(ctxIdentity) == nil { + ctx = context.WithValue(ctx, ctxIdentity, "alice") + } + if ctx.Value(ctxPrincipalType) == nil { + ctx = context.WithValue(ctx, ctxPrincipalType, principalTypeUser) + } + return req.WithContext(ctx) +} diff --git a/spinifex/gateway/rds_test.go b/spinifex/gateway/rds_test.go index bc7ad3383..0bbeda38b 100644 --- a/spinifex/gateway/rds_test.go +++ b/spinifex/gateway/rds_test.go @@ -23,7 +23,7 @@ func setupRDSRequest(body string) *http.Request { req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(body)) ctx := context.WithValue(req.Context(), ctxService, "rds") ctx = context.WithValue(ctx, ctxAccountID, rdsTestAccountID) - return req.WithContext(ctx) + return withTestIdentity(req.WithContext(ctx)) } // A signed customer request: an IAM user with a resolvable identity, which is @@ -83,14 +83,14 @@ func newRDSGatewayWithResolver( } func TestRDSRequest_MissingAction(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} err := gw.RDS_Request(httptest.NewRecorder(), setupRDSRequest("")) require.Error(t, err) assert.Equal(t, awserrors.ErrorMissingAction, err.Error()) } func TestRDSRequest_UnknownAction(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} err := gw.RDS_Request(httptest.NewRecorder(), setupRDSRequest("Action=FakeAction")) require.Error(t, err) assert.Equal(t, awserrors.ErrorInvalidAction, err.Error()) @@ -98,7 +98,7 @@ func TestRDSRequest_UnknownAction(t *testing.T) { // A malformed percent-encoding is a client error, not an internal one. func TestRDSRequest_MalformedQueryString(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} err := gw.RDS_Request(httptest.NewRecorder(), setupRDSRequest("Action=%zz")) require.Error(t, err) assert.Equal(t, awserrors.ErrorMalformedQueryString, err.Error()) @@ -106,7 +106,7 @@ func TestRDSRequest_MalformedQueryString(t *testing.T) { func TestRDSRequest_MalformedScalarReturnsHTTP400(t *testing.T) { _, nc, _ := testutil.StartTestJetStream(t) - gw := &GatewayConfig{DisableLogging: true, NATSConn: nc} + gw := &GatewayConfig{DisableLogging: true, NATSConn: nc, IAMService: allowAllIAMService()} for _, body := range []string{ "Action=CreateDBInstance&AllocatedStorage=abc", "Action=CreateDBInstance&MultiAZ=yes", @@ -128,7 +128,7 @@ func TestRDSRequest_MalformedScalarReturnsHTTP400(t *testing.T) { // A known action still needs a NATS connection to reach the control plane, so // the disconnected case must fail rather than answer from the gateway alone. func TestRDSRequest_NoNATSConn(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} err := gw.RDS_Request(httptest.NewRecorder(), setupRDSRequest("Action=DescribeDBInstances")) require.Error(t, err) assert.Equal(t, awserrors.ErrorServerInternal, err.Error()) @@ -217,20 +217,24 @@ func TestRDSRequest_CrossAccountARNIsRejectedNotEvaluated(t *testing.T) { assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err)) } -// An authenticated request with no resolvable identity predates per-principal -// enforcement and keeps its documented allow, rather than newly failing closed. -func TestRDSRequest_NoIdentityKeepsPreIAMAllow(t *testing.T) { - gw, probe := newRDSPolicyGateway(nil) // no policies: an evaluated request would be denied - err := gw.RDS_Request(httptest.NewRecorder(), setupRDSRequest("Action=DescribeDBInstances")) +// A request with no resolvable identity is denied outright: there is no +// principal to evaluate, so the gate refuses before consulting any policy. +func TestRDSRequest_NoIdentityDenied(t *testing.T) { + gw, probe := newRDSPolicyGateway(nil) + req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader("Action=DescribeDBInstances")) + ctx := context.WithValue(req.Context(), ctxService, "rds") + ctx = context.WithValue(ctx, ctxAccountID, rdsTestAccountID) + + err := gw.RDS_Request(httptest.NewRecorder(), req.WithContext(ctx)) require.Error(t, err) - assert.Equal(t, awserrors.ErrorServerInternal, err.Error()) + assert.Equal(t, awserrors.ErrorAccessDenied, err.Error()) assert.Zero(t, probe.consulted, "a request with no identity must not be evaluated at all") } // RDS errors must use the IAM-style envelope: the aws-sdk-go // query unmarshaler rejects the EC2 shape for this service. func TestRDSErrorHandler_UsesIAMEnvelope(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} w := httptest.NewRecorder() gw.ErrorHandler(w, setupRDSRequest("Action=CreateDBInstance"), errNotImplementedForTest) @@ -240,7 +244,7 @@ func TestRDSErrorHandler_UsesIAMEnvelope(t *testing.T) { } func TestRDSErrorHandler_UsesRDSUnsupportedActionWording(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} w := httptest.NewRecorder() gw.ErrorHandler(w, setupRDSRequest("Action=CreateDBCluster"), errors.New(awserrors.ErrorOperationNotSupported)) diff --git a/spinifex/gateway/spinifex_modelaccess_test.go b/spinifex/gateway/spinifex_modelaccess_test.go index 053a22219..a8081401f 100644 --- a/spinifex/gateway/spinifex_modelaccess_test.go +++ b/spinifex/gateway/spinifex_modelaccess_test.go @@ -29,6 +29,7 @@ func spinifexAccessRequest(t *testing.T, gw *GatewayConfig, action, accountID st ctx = context.WithValue(ctx, ctxAccountID, accountID) ctx = context.WithValue(ctx, ctxIdentity, "admin") ctx = context.WithValue(ctx, ctxService, "spinifex") + ctx = context.WithValue(ctx, ctxPrincipalType, principalTypeUser) req = req.WithContext(ctx) w := httptest.NewRecorder() @@ -48,6 +49,7 @@ func accessGateway(t *testing.T) *GatewayConfig { return &GatewayConfig{ DisableLogging: true, BedrockAccessAdmin: gateway_bedrock.NewModelAccessStore(js, 1), + IAMService: allowAllIAMService(), } } @@ -116,7 +118,7 @@ func TestSpinifex_ModelAccess_MissingParameters(t *testing.T) { // TestSpinifex_ModelAccess_NoStoreIsServerError covers a gateway built without // a grant store: the admin actions refuse rather than panicking on nil. func TestSpinifex_ModelAccess_NoStoreIsServerError(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} adminAccount := admin.DefaultAccountID() for _, action := range []string{"GrantModelAccess", "RevokeModelAccess", "ListModelAccess"} { diff --git a/spinifex/gateway/spinifex_test.go b/spinifex/gateway/spinifex_test.go index f95f8472c..91a93f77f 100644 --- a/spinifex/gateway/spinifex_test.go +++ b/spinifex/gateway/spinifex_test.go @@ -21,6 +21,7 @@ func spinifexRequest(t *testing.T, gw *GatewayConfig, action, accountID, identit ctx = context.WithValue(ctx, ctxAccountID, accountID) ctx = context.WithValue(ctx, ctxIdentity, identity) ctx = context.WithValue(ctx, ctxService, "spinifex") + ctx = context.WithValue(ctx, ctxPrincipalType, principalTypeUser) req = req.WithContext(ctx) w := httptest.NewRecorder() @@ -38,6 +39,7 @@ func TestSpinifex_GetVersion_Admin(t *testing.T) { DisableLogging: true, Version: "v0.5.0-43-gae1deb5", Commit: "ae1deb5", + IAMService: allowAllIAMService(), } w := spinifexRequest(t, gw, "GetVersion", admin.DefaultAccountID(), "admin") assert.Equal(t, http.StatusOK, w.Code) @@ -50,6 +52,7 @@ func TestSpinifex_GetVersion_NonAdmin_Denied(t *testing.T) { DisableLogging: true, Version: "v0.5.0", Commit: "abc123", + IAMService: allowAllIAMService(), } w := spinifexRequest(t, gw, "GetVersion", "000000000002", "alice") require.Equal(t, http.StatusForbidden, w.Code) @@ -57,28 +60,28 @@ func TestSpinifex_GetVersion_NonAdmin_Denied(t *testing.T) { } func TestSpinifex_GetNodes_NonAdmin_Denied(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} w := spinifexRequest(t, gw, "GetNodes", "000000000002", "alice") require.Equal(t, http.StatusForbidden, w.Code) assert.Contains(t, w.Body.String(), awserrors.ErrorAccessDenied) } func TestSpinifex_GetVMs_NonAdmin_Denied(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} w := spinifexRequest(t, gw, "GetVMs", "000000000002", "alice") require.Equal(t, http.StatusForbidden, w.Code) assert.Contains(t, w.Body.String(), awserrors.ErrorAccessDenied) } func TestSpinifex_InvalidAction(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} w := spinifexRequest(t, gw, "DoesNotExist", admin.DefaultAccountID(), "admin") require.Equal(t, http.StatusForbidden, w.Code) assert.Contains(t, w.Body.String(), awserrors.ErrorInvalidAction) } func TestSpinifex_MissingAction(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader("")) ctx := context.WithValue(req.Context(), ctxService, "spinifex") ctx = context.WithValue(ctx, ctxAccountID, admin.DefaultAccountID()) @@ -92,22 +95,25 @@ func TestSpinifex_MissingAction(t *testing.T) { } func TestSpinifex_NoAccountID(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} body := "Action=GetVersion" req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(body)) ctx := context.WithValue(req.Context(), ctxService, "spinifex") - req = req.WithContext(ctx) + req = withTestIdentity(req.WithContext(ctx)) w := httptest.NewRecorder() + // Account-less requests are rejected by the policy gate, which runs ahead of + // the handler's own account guard. err := gw.Spinifex_Request(w, req) require.Error(t, err) - assert.Equal(t, awserrors.ErrorServerInternal, err.Error()) + assert.Equal(t, awserrors.ErrorInternalError, err.Error()) } func TestSpinifex_GetNodes_NoNATS(t *testing.T) { gw := &GatewayConfig{ DisableLogging: true, NATSConn: nil, + IAMService: allowAllIAMService(), } w := spinifexRequest(t, gw, "GetNodes", admin.DefaultAccountID(), "admin") require.Equal(t, http.StatusForbidden, w.Code) diff --git a/spinifex/gateway/tagging_test.go b/spinifex/gateway/tagging_test.go index 0c3ebb661..b0c35da58 100644 --- a/spinifex/gateway/tagging_test.go +++ b/spinifex/gateway/tagging_test.go @@ -19,7 +19,7 @@ func setupTaggingRequest(target, body string) *http.Request { } ctx := context.WithValue(req.Context(), ctxService, "tagging") ctx = context.WithValue(ctx, ctxAccountID, "123456789012") - return req.WithContext(ctx) + return withTestIdentity(req.WithContext(ctx)) } func TestTaggingActionFromTarget(t *testing.T) { @@ -40,7 +40,7 @@ func TestTaggingActionsMap_AllActionsRegistered(t *testing.T) { } func TestTaggingRequest_MissingTarget(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} w := httptest.NewRecorder() err := gw.Tagging_Request(w, setupTaggingRequest("", "")) require.Error(t, err) @@ -48,7 +48,7 @@ func TestTaggingRequest_MissingTarget(t *testing.T) { } func TestTaggingRequest_UnknownAction(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} w := httptest.NewRecorder() err := gw.Tagging_Request(w, setupTaggingRequest("ResourceGroupsTaggingAPI_20170126.TagResources", "{}")) require.Error(t, err) @@ -58,7 +58,7 @@ func TestTaggingRequest_UnknownAction(t *testing.T) { // A known action with no NATS connection passes routing + policy and fails at // the NATS-availability guard. func TestTaggingRequest_KnownActionNoNATS(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true} + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} w := httptest.NewRecorder() err := gw.Tagging_Request(w, setupTaggingRequest("ResourceGroupsTaggingAPI_20170126.GetResources", "{}")) require.Error(t, err)