fix: reject an unresolvable policy variable on the IAM write path (#986)

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

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=

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()
 

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)

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

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

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) {

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)