diff --git a/spinifex/gateway/conditionkeys_completeness_test.go b/spinifex/gateway/conditionkeys_completeness_test.go new file mode 100644 index 000000000..21dd12b84 --- /dev/null +++ b/spinifex/gateway/conditionkeys_completeness_test.go @@ -0,0 +1,160 @@ +//test:in-package — requestConditionKeys, principalContext and the principal type +// constants are unexported, and the gate exists to compare what they emit with +// the evaluator's registries. + +package gateway + +import ( + "crypto/tls" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/mulgadc/bluebottle/pkg/iampolicy" + handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The keys this door resolves. predastore's mirror of this list adds s3:prefix +// and nothing else; a key added at one door and not the other fails there. +var gatewayDoorKeys = []string{ + iampolicy.KeySecureTransport, + iampolicy.KeyUsername, + iampolicy.KeyPrincipalAccount, + iampolicy.KeySourceIP, +} + +// Every condition key the evaluator names, so the write-path gate below covers +// the whole namespace rather than the subset this door happens to supply. +var allConditionKeys = []string{ + iampolicy.KeySourceIP, + iampolicy.KeyS3Prefix, + iampolicy.KeySecureTransport, + iampolicy.KeyUsername, + iampolicy.KeyPrincipalAccount, + iampolicy.KeyUserID, + // Deliberately outside the allowlist: there is no MFA in the stack, so the + // key could never be true. It is here to prove the validator says no. + "aws:MultiFactorAuthPresent", +} + +// One value per operator that the leaf validator accepts, so a rejection can +// only come from the operator/key allowlist and not from the value. +var operatorValues = map[string]string{ + iampolicy.OpStringEquals: "alice", + iampolicy.OpStringLike: "ali*", + iampolicy.OpIPAddress: "10.0.0.0/8", + iampolicy.OpBool: "true", + // Operators the evaluator does not implement. Accepting one would store a + // restriction that compares false forever. + "StringNotEquals": "alice", + "ArnLike": "arn:aws:iam::000000000001:user/alice", + "NumericLessThan": "3", + "DateGreaterThan": "2026-01-01T00:00:00Z", +} + +// emittedKeys drives requestConditionKeys with everything a request can carry, +// for both principal types, and returns the union. +func emittedKeys(t *testing.T) map[string]string { + t.Helper() + + principals := map[string]principalContext{ + "user": { + identity: "alice", + accountID: "000000000001", + principalType: principalTypeUser, + }, + "assumed-role": { + identity: "session", + accountID: "000000000001", + principalType: principalTypeAssumedRole, + assumedRoleARN: "arn:aws:sts::000000000001:assumed-role/SharedOps/session", + }, + } + + union := make(map[string]string) + for name, principal := range principals { + r := httptest.NewRequest(http.MethodPost, "/?prefix=home/", nil) + r.TLS = &tls.ConnectionState{} + r.RemoteAddr = "10.4.1.9:52344" + for key := range requestConditionKeys(r, principal) { + union[key] = name + } + } + return union +} + +// A key this door supplies that no policy can name is carried to the evaluator +// and dropped there — the silent half of the write-path/evaluator disagreement. +func TestRequestConditionKeys_EveryEmittedKeyIsUsableInAPolicy(t *testing.T) { + for key, principal := range emittedKeys(t) { + supported := false + for op := range operatorValues { + if iampolicy.SupportedCondition(op, key) { + supported = true + break + } + } + _, unresolvable := iampolicy.UnsupportedVariable("${" + key + "}") + assert.True(t, supported || !unresolvable, + "requestConditionKeys emits %q for a %s principal, but the evaluator neither enforces a "+ + "condition on it nor substitutes it: a policy naming it can never fire", key, principal) + } +} + +// The door's key set as a whole, so a key gained or lost here is a deliberate +// change made at both doors rather than a drift between them. +func TestRequestConditionKeys_MatchesTheDoorKeySet(t *testing.T) { + emitted := make([]string, 0, len(gatewayDoorKeys)) + for key := range emittedKeys(t) { + emitted = append(emitted, key) + } + assert.ElementsMatch(t, gatewayDoorKeys, emitted, + "the AWS gateway door key set changed: update predastore's mirror in "+ + "internal/gate/conditionkeys_completeness_test.go and the door table in bluebottle's door_test.go") + + // s3:prefix is the one key the S3 gate adds over this set, and it is absent + // here even when the request carries a prefix parameter. + assert.NotContains(t, emitted, iampolicy.KeyS3Prefix) +} + +// The write path and the evaluator are the same allowlist or they are a bug: +// a pair the validator accepts but the evaluator cannot enforce is stored as an +// inert restriction, and one it rejects that the evaluator does enforce is a +// policy an operator cannot write. +func TestValidatePolicyDocument_AcceptsExactlyTheSupportedConditions(t *testing.T) { + for _, key := range allConditionKeys { + for op, value := range operatorValues { + doc := conditionDocument(t, op, key, value) + _, err := handlers_iam.ValidatePolicyDocument(doc) + + if iampolicy.SupportedCondition(op, key) { + assert.NoError(t, err, + "the evaluator enforces %s on %q but the write path rejects it, so the policy "+ + "cannot be written", op, key) + continue + } + assert.Error(t, err, + "the write path accepts %s on %q, which the evaluator does not enforce: the condition "+ + "would be stored and then compare false forever", op, key) + } + } +} + +func conditionDocument(t *testing.T, op, key, value string) string { + t.Helper() + + doc, err := json.Marshal(map[string]any{ + "Version": "2012-10-17", + "Statement": []map[string]any{{ + "Effect": "Allow", + "Action": []string{"s3:GetObject"}, + "Resource": []string{"arn:aws:s3:::reports/*"}, + "Condition": map[string]any{op: map[string]any{key: []string{value}}}, + }}, + }) + require.NoError(t, err, "marshalling the %s/%s document", op, key) + return string(doc) +} diff --git a/spinifex/gateway/ecrapi_authz_completeness_test.go b/spinifex/gateway/ecrapi_authz_completeness_test.go new file mode 100644 index 000000000..198aa3040 --- /dev/null +++ b/spinifex/gateway/ecrapi_authz_completeness_test.go @@ -0,0 +1,38 @@ +//test:in-package — ecrInlineActions is unexported here, and ECR dispatch is the +// union of it and the ecrapi namespace. + +package gateway + +import ( + "testing" + + gateway_ecrapi "github.com/mulgadc/spinifex/spinifex/gateway/ecrapi" + "github.com/stretchr/testify/assert" +) + +// TestECRScopeTableIsExhaustive is what stops the next ECR 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 TestECRScopeTableIsExhaustive(t *testing.T) { + for action := range gateway_ecrapi.Actions { + assert.True(t, gateway_ecrapi.HasScope(action), + "ecr action %q has no resource scope entry: add one to ecrScopes in gateway/ecrapi/authz.go", action) + } + + for _, action := range gateway_ecrapi.ScopedActions() { + _, ok := gateway_ecrapi.Actions[action] + assert.True(t, ok, + "ecrScopes has an entry for %q, which the dispatch table does not serve: remove it from gateway/ecrapi/authz.go", action) + } +} + +// ECR_Request resolves the action against the ecrapi namespace before it reaches +// the inline table, so an inline handler outside that namespace is unreachable — +// and, more to the point, would never have had its resources scoped. +func TestECRInlineActionsAreInTheNamespace(t *testing.T) { + for action := range ecrInlineActions { + _, ok := gateway_ecrapi.Actions[action] + assert.True(t, ok, + "inline ECR handler %q is not in gateway_ecrapi.Actions, so ECR_Request rejects it as InvalidAction", action) + } +} diff --git a/spinifex/gateway/ecs_authz_completeness_test.go b/spinifex/gateway/ecs_authz_completeness_test.go new file mode 100644 index 000000000..54da58744 --- /dev/null +++ b/spinifex/gateway/ecs_authz_completeness_test.go @@ -0,0 +1,24 @@ +package gateway_test + +import ( + "testing" + + gateway_ecs "github.com/mulgadc/spinifex/spinifex/gateway/ecs" + "github.com/stretchr/testify/assert" +) + +// TestECSScopeTableIsExhaustive is what stops the next ECS 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 TestECSScopeTableIsExhaustive(t *testing.T) { + for action := range gateway_ecs.Actions { + assert.True(t, gateway_ecs.HasScope(action), + "ecs action %q has no resource scope entry: add one to ecsScopes in gateway/ecs/authz.go", action) + } + + for _, action := range gateway_ecs.ScopedActions() { + _, ok := gateway_ecs.Actions[action] + assert.True(t, ok, + "ecsScopes has an entry for %q, which the dispatch table does not serve: remove it from gateway/ecs/authz.go", action) + } +} diff --git a/spinifex/gateway/rds/authz.go b/spinifex/gateway/rds/authz.go index cce1348ed..8345b4034 100644 --- a/spinifex/gateway/rds/authz.go +++ b/spinifex/gateway/rds/authz.go @@ -61,6 +61,19 @@ func requireAgentPrincipal(ctx context.Context, caller Caller) error { return nil } +// HasScope reports whether the action's policy check evaluates a resource ARN +// rather than the account-wide "*". +func HasScope(action string) bool { + def, ok := actions[action] + return ok && len(def.scopes) > 0 +} + +// ScopedActions returns every action whose policy check evaluates a resource +// ARN, in stable order. +func ScopedActions() []string { + return actionNames(func(def actionDef) bool { return len(def.scopes) > 0 }) +} + // ResourceARN builds every resource the action's policy checks evaluate. A // policy written for arn:aws:rds:*:*:db:prod-* but checked against "*" would // permit every instance in the account, which is worse than no policy at all. diff --git a/spinifex/gateway/rds_authz_completeness_test.go b/spinifex/gateway/rds_authz_completeness_test.go new file mode 100644 index 000000000..c0d0d08d9 --- /dev/null +++ b/spinifex/gateway/rds_authz_completeness_test.go @@ -0,0 +1,67 @@ +package gateway_test + +import ( + "slices" + "testing" + + gateway_rds "github.com/mulgadc/spinifex/spinifex/gateway/rds" + "github.com/stretchr/testify/assert" +) + +// RDS carries its scopes on the dispatch table itself, so the two cannot +// disagree the way the other services' can. What can still go wrong is an action +// landing with no scopes and nobody noticing it now evaluates against "*", so +// the gate is the account-level half: every entry here is a written decision +// that the action addresses no single resource. +var rdsAccountLevelActions = map[string]string{ + // Creates: the resource does not exist yet, which is how AWS evaluates them. + "CreateDBInstance": "the instance does not exist until this call makes it", + "CreateDBSubnetGroup": "the subnet group does not exist until this call makes it", + "CreateDBParameterGroup": "the parameter group does not exist until this call makes it", + + // Describes that filter rather than address one resource. + "DescribeDBInstances": "filters the account's instances", + "DescribeDBSnapshots": "filters the account's snapshots", + "DescribeDBInstanceAutomatedBackups": "filters the account's automated backups", + "DescribeDBSubnetGroups": "filters the account's subnet groups", + "DescribeDBParameterGroups": "filters the account's parameter groups", + "DescribeEvents": "the event ring is per-account and a filter names no one resource", + + // Catalogs read no per-account state at all. + "DescribeDBEngineVersions": "a static engine catalog, not tenant state", + "DescribeOrderableDBInstanceOptions": "cluster capability, not tenant state", + + // Internal agent actions. AuthorizeCaller refuses these to every customer + // principal by class before any policy is evaluated, so a resource scope + // would narrow nothing. + "RegisterDBInstance": "agent-only, gated by principal class", + "SubmitDBStateChange": "agent-only, gated by principal class", + "PollDBCommands": "agent-only, gated by principal class", + "GetDBBootstrapConfig": "agent-only, gated by principal class", + "AcknowledgeDBBootstrap": "agent-only, gated by principal class", +} + +// TestRDSScopeTableIsExhaustive is what stops the next RDS action being added +// with a silent account-wide grant. Every served action is either scoped or +// carries a written reason for being account-level, and an action in both sets +// or in neither fails. +func TestRDSScopeTableIsExhaustive(t *testing.T) { + unsupported := gateway_rds.UnsupportedActionNames() + + for _, action := range gateway_rds.ActionNames() { + if slices.Contains(unsupported, action) { + continue + } + _, accountLevel := rdsAccountLevelActions[action] + assert.NotEqual(t, gateway_rds.HasScope(action), accountLevel, + "rds action %q must either carry a scope in gateway/rds/handler.go or a written reason "+ + "in rdsAccountLevelActions, not both and not neither", action) + } + + for action := range rdsAccountLevelActions { + assert.True(t, gateway_rds.HasAction(action), + "rdsAccountLevelActions names %q, which RDS does not serve: remove it", action) + assert.NotContains(t, unsupported, action, + "rdsAccountLevelActions names %q, which is recognised but not offered: remove it", action) + } +}