diff --git a/spinifex/gateway/tagging.go b/spinifex/gateway/tagging.go index 55e8753ac..95c5f0f52 100644 --- a/spinifex/gateway/tagging.go +++ b/spinifex/gateway/tagging.go @@ -3,7 +3,6 @@ package gateway import ( "context" "errors" - "io" "log/slog" "net/http" "strings" @@ -45,12 +44,12 @@ func (gw *GatewayConfig) Tagging_Request(w http.ResponseWriter, r *http.Request) return errors.New(awserrors.ErrorInvalidAction) } - if err := gw.checkPolicy(r, "tagging", action); err != nil { + resources, err := gateway_tagging.ResourceARNs(action) + if err != nil { return err } - - if gw.NATSConn == nil { - return errors.New(awserrors.ErrorServerInternal) + if err := gw.checkPolicyResources(r, "tagging", action, resources); err != nil { + return err } accountID, _ := r.Context().Value(ctxAccountID).(string) @@ -59,10 +58,16 @@ func (gw *GatewayConfig) Tagging_Request(w http.ResponseWriter, r *http.Request) return errors.New(awserrors.ErrorServerInternal) } - body, err := io.ReadAll(r.Body) + // Read ahead of the NATS guard so an oversized body is rejected on its own + // terms rather than behind a connection that happens to be down. + body, err := readBoundedBody(r) if err != nil { slog.Error("Tagging_Request: failed to read body", "err", err) - return errors.New(awserrors.ErrorInvalidParameterValue) + return err + } + + if gw.NATSConn == nil { + return errors.New(awserrors.ErrorServerInternal) } output, err := handler(r.Context(), gw, accountID, body) diff --git a/spinifex/gateway/tagging/authz.go b/spinifex/gateway/tagging/authz.go new file mode 100644 index 000000000..997808de9 --- /dev/null +++ b/spinifex/gateway/tagging/authz.go @@ -0,0 +1,67 @@ +package gateway_tagging + +import ( + "errors" + "log/slog" + "maps" + "slices" + + "github.com/mulgadc/spinifex/spinifex/awserrors" +) + +// The resource a policy check evaluates against when AWS documents no resource +// type for the action, or when the identifier cannot be resolved at gate time. +const anyResource = "*" + +// Where an action's resource is named. Only one member today; the type exists +// so a scoped action reads the same way here as it does in the other services. +type resourceSource uint8 + +const ( + sourceAny resourceSource = iota +) + +// Every action Tagging_Request serves, with the resource AWS evaluates it +// against. Exhaustive by contract: a completeness test compares this table with +// the dispatch table in both directions, so an action cannot be added with a +// silent account-wide grant. +var taggingScopes = map[string]resourceSource{ + // Account-level. The Resource Groups Tagging API defines no resource types, + // so AWS evaluates every tag: action against "*". GetResources aggregates + // the account's EC2 and ELBv2 resources but is not scoped to them. + "GetResources": sourceAny, +} + +// HasScope reports whether action has an explicit tagging scope-table entry. +// taggingActions lives in package gateway, so the completeness test calls this +// rather than reaching into the table. +func HasScope(action string) bool { + _, ok := taggingScopes[action] + return ok +} + +// ScopedActions returns every action represented in the tagging scope table. +func ScopedActions() []string { + return slices.Sorted(maps.Keys(taggingScopes)) +} + +// ResourceARNs resolves the resources a tagging request authorizes against. +// It takes no region, account or body because every action AWS defines for this +// service is account-level; adding a scoped one means widening this signature, +// which is a compile error at the dispatcher rather than a silent "*". +func ResourceARNs(action string) ([]string, error) { + source, ok := taggingScopes[action] + if !ok { + slog.Error("tagging authz: action is served but absent from the scope table", "action", action) + return nil, errors.New(awserrors.ErrorInvalidAction) + } + + switch source { + case sourceAny: + return []string{anyResource}, nil + + default: + slog.Error("tagging authz: unhandled resource source, failing closed", "action", action, "source", source) + return nil, errors.New(awserrors.ErrorInternalError) + } +} diff --git a/spinifex/gateway/tagging/authz_test.go b/spinifex/gateway/tagging/authz_test.go new file mode 100644 index 000000000..0e6b15339 --- /dev/null +++ b/spinifex/gateway/tagging/authz_test.go @@ -0,0 +1,35 @@ +package gateway_tagging_test + +import ( + "testing" + + "github.com/mulgadc/spinifex/spinifex/awserrors" + gateway_tagging "github.com/mulgadc/spinifex/spinifex/gateway/tagging" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The Resource Groups Tagging API defines no resource types, so every action it +// serves is evaluated against "*". Scoping GetResources to the resources it +// happens to return would deny calls AWS permits. +func TestResourceARNsAreAccountLevel(t *testing.T) { + for _, action := range gateway_tagging.ScopedActions() { + resources, err := gateway_tagging.ResourceARNs(action) + require.NoError(t, err, "action %q", action) + assert.Equal(t, []string{"*"}, resources, "action %q", action) + } +} + +// An action absent from the dispatch table cannot reach the resolver, but if one +// ever did it fails closed rather than authorizing account-wide. +func TestResourceARNsUnknownAction(t *testing.T) { + _, err := gateway_tagging.ResourceARNs("BogusAction") + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidAction, err.Error()) +} + +func TestHasScope(t *testing.T) { + assert.True(t, gateway_tagging.HasScope("GetResources")) + assert.False(t, gateway_tagging.HasScope("TagResources")) + assert.Equal(t, []string{"GetResources"}, gateway_tagging.ScopedActions()) +} diff --git a/spinifex/gateway/tagging_authz_completeness_test.go b/spinifex/gateway/tagging_authz_completeness_test.go new file mode 100644 index 000000000..65ad56ea4 --- /dev/null +++ b/spinifex/gateway/tagging_authz_completeness_test.go @@ -0,0 +1,27 @@ +//test:in-package — taggingActions is unexported here, and the completeness test +// exists to compare it against the scope table. + +package gateway + +import ( + "testing" + + gateway_tagging "github.com/mulgadc/spinifex/spinifex/gateway/tagging" + "github.com/stretchr/testify/assert" +) + +// TestTaggingScopeTableIsExhaustive is what stops the next tagging action being +// added with a silent account-wide grant. It asserts both directions, so a scope +// left behind by a deleted or renamed action fails too. +func TestTaggingScopeTableIsExhaustive(t *testing.T) { + for action := range taggingActions { + assert.True(t, gateway_tagging.HasScope(action), + "tagging action %q has no resource scope entry: add one to taggingScopes in gateway/tagging/authz.go", action) + } + + for _, action := range gateway_tagging.ScopedActions() { + _, ok := taggingActions[action] + assert.True(t, ok, + "taggingScopes has an entry for %q, which the dispatch table does not serve: remove it from gateway/tagging/authz.go", action) + } +} diff --git a/spinifex/gateway/tagging_authz_test.go b/spinifex/gateway/tagging_authz_test.go new file mode 100644 index 000000000..7ac56fe49 --- /dev/null +++ b/spinifex/gateway/tagging_authz_test.go @@ -0,0 +1,83 @@ +//test:in-package — drives Tagging_Request through the gateway's unexported test +// helpers (setupTaggingRequest, policyMockIAMService). + +package gateway + +import ( + "net/http/httptest" + "strings" + "testing" + + "github.com/mulgadc/bluebottle/pkg/sigv4" + "github.com/mulgadc/spinifex/spinifex/awserrors" + gateway_tagging "github.com/mulgadc/spinifex/spinifex/gateway/tagging" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// dispatchTagging drives the gateway with no NATS connection. A permitted +// request therefore fails at the NATS-availability guard rather than on +// authorization, which is what proves the policy check ran ahead of the handler. +func dispatchTagging(t *testing.T, gw *GatewayConfig, action, body string) error { + t.Helper() + return gw.Tagging_Request(httptest.NewRecorder(), setupTaggingRequest("ResourceGroupsTaggingAPI_20170126."+action, body)) +} + +// Every tagging action is account-level, so a grant scoped to a resource ARN +// does not reach one. This is the whole enforcement surface the service admits: +// AWS defines no resource types for the Resource Groups Tagging API, so there is +// no ARN a scoped Deny could name. +func TestTaggingRequest_ActionsAreAccountLevel(t *testing.T) { + scoped := scopedPolicyGateway( + statement("Allow", "tagging:GetResources", "arn:aws:ec2:"+authzRegion+":"+authzAccountID+":instance/i-dev"), + ) + assertDenied(t, dispatchTagging(t, scoped, "GetResources", `{}`)) + + wide := scopedPolicyGateway(statement("Allow", "tagging:GetResources", "*")) + assertPermitted(t, dispatchTagging(t, wide, "GetResources", `{}`)) +} + +// A Deny scoped to a resource ARN must not fire: AWS evaluates every tag: +// action against "*", so denying here would reject a call AWS permits. This is +// the D2 guard — it fails the moment the dispatcher or the resolver passes any +// resource beyond "*". +func TestTaggingRequest_ScopedDenyDoesNotFire(t *testing.T) { + gw := scopedPolicyGateway( + statement("Allow", "tagging:*", "*"), + statement("Deny", "tagging:GetResources", "arn:aws:ec2:"+authzRegion+":"+authzAccountID+":instance/i-prod"), + ) + + assertPermitted(t, dispatchTagging(t, gw, "GetResources", `{}`)) +} + +// The property the per-service rollout rests on: every policy that works today +// carries Resource "*", and resolving the scope table cannot turn one into a +// denial. +func TestTaggingRequest_WildcardPolicyStillPermitsEveryAction(t *testing.T) { + gw := scopedPolicyGateway(statement("Allow", "tagging:*", "*")) + + for _, action := range gateway_tagging.ScopedActions() { + t.Run(action, func(t *testing.T) { + assertPermitted(t, dispatchTagging(t, gw, action, `{}`)) + }) + } +} + +// The action half of the evaluator is unchanged: a grant on one action does not +// carry to another the dispatcher serves. +func TestTaggingRequest_UnrelatedActionGrantDenies(t *testing.T) { + gw := scopedPolicyGateway(statement("Allow", "ec2:DescribeTags", "*")) + + assertDenied(t, dispatchTagging(t, gw, "GetResources", `{}`)) +} + +// A body past the signed path's cap cannot be used to exhaust memory: it is +// rejected before the handler reads it. +func TestTaggingRequest_OversizedBodyIsRejected(t *testing.T) { + gw := scopedPolicyGateway(statement("Allow", "tagging:*", "*")) + body := `{"PaginationToken":"` + strings.Repeat("a", sigv4.MaxPayloadLen) + `"}` + + err := dispatchTagging(t, gw, "GetResources", body) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorRequestEntityTooLarge, err.Error()) +}