diff --git a/go.mod b/go.mod index 9e87d7576..7cb4f304a 100644 --- a/go.mod +++ b/go.mod @@ -21,7 +21,7 @@ require ( github.com/jackc/pgx/v5 v5.10.0 github.com/klauspost/cpuid/v2 v2.4.0 github.com/miekg/dns v1.1.73 - github.com/mulgadc/bluebottle v1.18.1-0.20260901051501-b00bc7828992 + github.com/mulgadc/bluebottle v1.18.1-0.20260902235515-6023971275c0 github.com/mulgadc/northstar v1.18.0 github.com/mulgadc/predastore v1.18.1-0.20260831064140-ff6a1dc42441 github.com/mulgadc/viperblock v1.18.0 diff --git a/go.sum b/go.sum index 12f1f07d5..22cefe695 100644 --- a/go.sum +++ b/go.sum @@ -1329,6 +1329,8 @@ github.com/muesli/termenv v0.16.0 h1:S5AlUN9dENB57rsbnkPyfdGuWIlkmzJjbFf0Tf5FWUc github.com/muesli/termenv v0.16.0/go.mod h1:ZRfOIKPFDYQoDFF4Olj7/QJbW60Ol/kL1pU3VfY/Cnk= github.com/mulgadc/bluebottle v1.18.1-0.20260901051501-b00bc7828992 h1:ss5/vEr5STIZXBWAZZthlAS6q9i44aOMbaFjZL9ediY= github.com/mulgadc/bluebottle v1.18.1-0.20260901051501-b00bc7828992/go.mod h1:XBIMI+G2s3wuJNiEdYtlGDvCacw82OXEw50RIQhowYg= +github.com/mulgadc/bluebottle v1.18.1-0.20260902235515-6023971275c0 h1:img64+hlZeHkjPbF/l9FPhiPNW+p0/9sqDpv8PopbpE= +github.com/mulgadc/bluebottle v1.18.1-0.20260902235515-6023971275c0/go.mod h1:XBIMI+G2s3wuJNiEdYtlGDvCacw82OXEw50RIQhowYg= github.com/mulgadc/northstar v1.18.0 h1:ksRjIKE6qtrw/ZoYaJh8JRBh+xdnuDw8FsmFLFCiXbg= github.com/mulgadc/northstar v1.18.0/go.mod h1:z6iol77R1vsIQyIBc1JQboUPhSJ6eG4bipzfbZZbLEM= github.com/mulgadc/predastore v1.18.1-0.20260831064140-ff6a1dc42441 h1:5t+iuKpnRWXkKOmh5lLBoziBaWD+H71edWO/O4ceMp8= diff --git a/spinifex/gateway/conditionkeys_completeness_test.go b/spinifex/gateway/conditionkeys_completeness_test.go index 21dd12b84..fa3316502 100644 --- a/spinifex/gateway/conditionkeys_completeness_test.go +++ b/spinifex/gateway/conditionkeys_completeness_test.go @@ -143,6 +143,55 @@ func TestValidatePolicyDocument_AcceptsExactlyTheSupportedConditions(t *testing. } } +// The variable half of the same question. A reference the write path accepts and +// the evaluator cannot resolve is stored as a pattern that selects nothing on an +// Allow and everything it names on a Deny; one it rejects that the evaluator does +// resolve is a policy an operator cannot write. +func TestValidatePolicyDocument_AcceptsExactlyTheSubstitutableVariables(t *testing.T) { + for _, key := range append(allConditionKeys, "aws:bogus") { + reference := iampolicy.VariablePrefix + key + "}" + _, unresolvable := iampolicy.UnsupportedVariable(reference) + + for _, doc := range []string{ + resourceDocument(t, "arn:aws:s3:::reports/"+reference+"/*"), + conditionDocument(t, iampolicy.OpStringLike, iampolicy.KeyS3Prefix, reference+"/*"), + } { + _, err := handlers_iam.ValidatePolicyDocument(doc) + if unresolvable { + assert.Error(t, err, + "the write path accepts %s, which the evaluator cannot resolve: the policy would "+ + "be stored selecting nothing on an Allow and everything on a Deny", reference) + continue + } + assert.NoError(t, err, + "the evaluator resolves %s but the write path rejects it, so the policy cannot be "+ + "written", reference) + } + } +} + +// A malformed reference resolves nowhere and is rejected on every path. +func TestValidatePolicyDocument_RejectsUnterminatedVariable(t *testing.T) { + _, err := handlers_iam.ValidatePolicyDocument( + resourceDocument(t, "arn:aws:s3:::reports/"+iampolicy.VariablePrefix+iampolicy.KeyUsername)) + assert.ErrorContains(t, err, "unterminated policy variable reference") +} + +func resourceDocument(t *testing.T, resource 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{resource}, + }}, + }) + require.NoError(t, err, "marshalling the %q document", resource) + return string(doc) +} + func conditionDocument(t *testing.T, op, key, value string) string { t.Helper() diff --git a/spinifex/handlers/iam/groups.go b/spinifex/handlers/iam/groups.go index 93222732d..29e26fec7 100644 --- a/spinifex/handlers/iam/groups.go +++ b/spinifex/handlers/iam/groups.go @@ -422,7 +422,8 @@ func (s *IAMServiceImpl) PutGroupPolicy(accountID string, input *iam.PutGroupPol return nil, errors.New(awserrors.ErrorIAMInvalidInput) } if _, err := ValidatePolicyDocument(policyDoc); err != nil { - return nil, errors.New(awserrors.ErrorIAMMalformedPolicyDocument) + return nil, awserrors.Errorf(awserrors.ErrorIAMMalformedPolicyDocument, + "policy %q on group %q: %w", policyName, groupName, err) } group, err := s.getGroup(ctx, accountID, groupName) diff --git a/spinifex/handlers/iam/roles.go b/spinifex/handlers/iam/roles.go index 72f8a9f00..e6f9a3bae 100644 --- a/spinifex/handlers/iam/roles.go +++ b/spinifex/handlers/iam/roles.go @@ -365,7 +365,8 @@ func (s *IAMServiceImpl) PutRolePolicy(accountID string, input *iam.PutRolePolic return nil, errors.New(awserrors.ErrorIAMInvalidInput) } if _, err := ValidatePolicyDocument(policyDoc); err != nil { - return nil, errors.New(awserrors.ErrorIAMMalformedPolicyDocument) + return nil, awserrors.Errorf(awserrors.ErrorIAMMalformedPolicyDocument, + "policy %q on role %q: %w", policyName, roleName, err) } // An unchanged document writes nothing: converge loops re-assert the same diff --git a/spinifex/handlers/iam/service_impl.go b/spinifex/handlers/iam/service_impl.go index 5f7c915b9..d687f0369 100644 --- a/spinifex/handlers/iam/service_impl.go +++ b/spinifex/handlers/iam/service_impl.go @@ -1183,8 +1183,8 @@ func (s *IAMServiceImpl) CreatePolicy(accountID string, input *iam.CreatePolicyI kvKey := accountID + "." + policyName if _, err := ValidatePolicyDocument(*input.PolicyDocument); err != nil { - slog.Debug("CreatePolicy: invalid policy document", "policyName", policyName, "err", err) - return nil, errors.New(awserrors.ErrorIAMMalformedPolicyDocument) + return nil, awserrors.Errorf(awserrors.ErrorIAMMalformedPolicyDocument, + "policy %q: %w", policyName, err) } path := aws.StringValue(input.Path) @@ -1524,7 +1524,8 @@ func (s *IAMServiceImpl) PutUserPolicy(accountID string, input *iam.PutUserPolic return nil, errors.New(awserrors.ErrorIAMInvalidInput) } if _, err := ValidatePolicyDocument(policyDoc); err != nil { - return nil, errors.New(awserrors.ErrorIAMMalformedPolicyDocument) + return nil, awserrors.Errorf(awserrors.ErrorIAMMalformedPolicyDocument, + "policy %q on user %q: %w", policyName, userName, err) } user, err := s.getUser(ctx, accountID, userName) @@ -2059,6 +2060,11 @@ func validateStatementRestrictions(i int, stmt Statement) error { if len(stmt.NotResource) > 0 { return fmt.Errorf("statement %d: NotResource blocks are not supported in this release; use Resource with an explicit list instead", i) } + for _, resource := range stmt.Resource { + if err := validatePolicyVariables(i, "Resource", resource); err != nil { + return err + } + } for op, keys := range stmt.Condition { for key, values := range keys { if !iampolicy.SupportedCondition(op, key) { @@ -2080,6 +2086,13 @@ func validateConditionValues(i int, op, key string, values ConditionValue) error return fmt.Errorf("statement %d: Condition operator %q on key %q has no value", i, op, key) } for _, v := range values { + // Only the string operators expand variables; a reference in a Bool or + // IpAddress value is already rejected as unparseable below. + if op == iampolicy.OpStringEquals || op == iampolicy.OpStringLike { + if err := validatePolicyVariables(i, fmt.Sprintf("Condition %s on key %q", op, key), v); err != nil { + return err + } + } switch op { case iampolicy.OpIPAddress: if _, prefixErr := netip.ParsePrefix(v); prefixErr != nil { @@ -2097,6 +2110,36 @@ func validateConditionValues(i int, op, key string, values ConditionValue) error return nil } +// supportedVariableList renders the substitutable keys for an error message, +// read from the evaluator's registry so the advice cannot go stale. The keys are +// copied into the rendered form rather than rewritten in place, since the +// registry owns nothing the caller may modify. +func supportedVariableList() string { + keys := iampolicy.SubstitutableKeys() + refs := make([]string, 0, len(keys)) + for _, key := range keys { + refs = append(refs, iampolicy.VariablePrefix+key+"}") + } + return strings.Join(refs, ", ") +} + +// validatePolicyVariables rejects a ${...} reference the evaluator can never +// resolve, naming the field it sits in. Such a pattern selects nothing on an +// Allow and everything it named on a Deny, so the policy would be stored meaning +// something other than what it reads as. +func validatePolicyVariables(i int, field, value string) error { + switch key, fault := iampolicy.UnresolvableVariable(value); fault { + case iampolicy.VariableUnterminated: + return fmt.Errorf("statement %d: %s: %q has an unterminated policy variable reference: "+ + "%q is missing its closing brace", i, field, value, iampolicy.VariablePrefix+key) + case iampolicy.VariableUnknownKey: + return fmt.Errorf("statement %d: %s: %q references policy variable %q, which no request supplies; "+ + "the supported variables are %s", i, field, value, key, supportedVariableList()) + default: + return nil + } +} + // summaryQuotaDefaults holds the static SummaryMap entries returned by // GetAccountSummary. Spinifex's quota system (handlers/quota) only enforces // infrastructure dimensions (vCPUs, VPCs, subnets, EIPs, EBS) and models no IAM diff --git a/spinifex/handlers/iam/service_impl_test.go b/spinifex/handlers/iam/service_impl_test.go index 9daf3bbc1..2af27a789 100644 --- a/spinifex/handlers/iam/service_impl_test.go +++ b/spinifex/handlers/iam/service_impl_test.go @@ -1861,6 +1861,70 @@ func TestValidatePolicyDocument_RejectsMalformedConditionValues(t *testing.T) { } } +// A reference the evaluator cannot resolve makes an Allow select nothing and a +// Deny select everything it named, so the policy does not do what it reads as. +// Rejecting it on write is the only place an operator can be told. +func TestValidatePolicyDocument_RejectsUnresolvableVariables(t *testing.T) { + t.Parallel() + tests := []struct { + name string + stmt string + wantErr string + }{ + {"userid in a resource", `"Action":"s3:*","Resource":"arn:aws:s3:::home/${aws:userid}/*"`, + `references policy variable "aws:userid"`}, + {"userid in a StringEquals value", + `"Action":"s3:*","Resource":"*","Condition":{"StringEquals":{"aws:username":"${aws:userid}"}}`, + `references policy variable "aws:userid"`}, + {"userid in a StringLike value", + `"Action":"s3:*","Resource":"*","Condition":{"StringLike":{"s3:prefix":"${aws:userid}/*"}}`, + `references policy variable "aws:userid"`}, + {"unknown key", `"Action":"s3:*","Resource":"arn:aws:s3:::home/${aws:bogus}/*"`, + `references policy variable "aws:bogus"`}, + {"condition key that is not substitutable", + `"Action":"s3:*","Resource":"arn:aws:s3:::home/${aws:SourceIp}/*"`, + `references policy variable "aws:SourceIp"`}, + {"unterminated", `"Action":"s3:*","Resource":"arn:aws:s3:::home/${aws:username"`, + "unterminated policy variable reference"}, + {"empty reference", `"Action":"s3:*","Resource":"arn:aws:s3:::home/${}/*"`, + "references policy variable"}, + {"second resource carries it", + `"Action":"s3:*","Resource":["arn:aws:s3:::a/*","arn:aws:s3:::${aws:userid}/*"]`, + `references policy variable "aws:userid"`}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + _, err := ValidatePolicyDocument(`{"Version":"2012-10-17","Statement":[{"Effect":"Allow",` + + tt.stmt + `}]}`) + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + }) + } +} + +// ${aws:username} resolves for an IAM user and is deliberately absent for a role +// session, so it is inert on a role-only path but correct on a user path. +// Rejecting it here would refuse a policy that works. +func TestValidatePolicyDocument_AcceptsSupportedVariables(t *testing.T) { + t.Parallel() + for _, stmt := range []string{ + `"Action":"s3:*","Resource":"arn:aws:s3:::home/${aws:username}/*"`, + `"Action":"s3:*","Resource":"arn:aws:s3:::${aws:PrincipalAccount}/*"`, + `"Action":"s3:*","Resource":"*","Condition":{"StringEquals":{"aws:username":"${aws:username}"}}`, + `"Action":"s3:*","Resource":"*","Condition":{"StringLike":{"s3:prefix":"home/${aws:username}/*"}}`, + // AWS's literal escapes: the only way to write these characters + // literally in a pattern, so they must survive the gate. + `"Action":"s3:*","Resource":"arn:aws:s3:::home/${*}/${?}/${$}"`, + // A "$" that opens nothing is ordinary text. + `"Action":"s3:*","Resource":"arn:aws:s3:::home/$aws:username/*"`, + } { + _, err := ValidatePolicyDocument(`{"Version":"2012-10-17","Statement":[{"Effect":"Allow",` + + stmt + `}]}`) + assert.NoError(t, err, "the evaluator resolves this policy's variables, so the write path must accept it: %s", stmt) + } +} + // D4's lenient leaf parsing must survive at the API surface: a native JSON bool // is what Terraform emits, and rejecting it would fail ordinary valid policies. func TestValidatePolicyDocument_AcceptsNativeBoolAndCIDRForms(t *testing.T) { diff --git a/spinifex/handlers/iam/user_inline_test.go b/spinifex/handlers/iam/user_inline_test.go index f30ef8bb0..afc241a06 100644 --- a/spinifex/handlers/iam/user_inline_test.go +++ b/spinifex/handlers/iam/user_inline_test.go @@ -99,6 +99,37 @@ func TestPutUserPolicy_MalformedDocument(t *testing.T) { assert.Contains(t, err.Error(), awserrors.ErrorIAMMalformedPolicyDocument) } +// The rejection has to name the variable and the alternatives, or the operator +// gets the same "policy document is malformed" they would get for a stray brace +// and no way to tell a supported reference from an unsupported one. The document +// here is syntactically perfect, so the code alone says nothing. +func TestPutUserPolicy_UnresolvableVariableNamesTheCause(t *testing.T) { + t.Parallel() + svc := setupTestIAMService(t) + createTestUser(t, svc, "variable-user") + + _, err := svc.PutUserPolicy(testAccountID, &iam.PutUserPolicyInput{ + UserName: aws.String("variable-user"), + PolicyName: aws.String("Inert"), + PolicyDocument: aws.String(`{"Version":"2012-10-17","Statement":[{"Effect":"Allow", + "Action":"s3:GetObject","Resource":"arn:aws:s3:::home/${aws:userid}/*"}]}`), + }) + require.Error(t, err) + + code, message, ok := awserrors.ResolveErrorDetail(err) + require.True(t, ok, "the error carries no registered AWS code, so it reaches the client as a 500") + assert.Equal(t, awserrors.ErrorIAMMalformedPolicyDocument, code) + assert.Contains(t, message, "aws:userid", "the message does not name the offending variable") + assert.Contains(t, message, "${aws:username}", "the message does not name the variables that do resolve") + + // The rejection is also a rejection: nothing was stored. + _, err = svc.GetUserPolicy(testAccountID, &iam.GetUserPolicyInput{ + UserName: aws.String("variable-user"), + PolicyName: aws.String("Inert"), + }) + assert.Error(t, err) +} + func TestPutUserPolicy_OversizedDocument(t *testing.T) { t.Parallel() svc := setupTestIAMService(t)