diff --git a/spinifex/handlers/iam/roles.go b/spinifex/handlers/iam/roles.go index 8bbf98789..2df41380b 100644 --- a/spinifex/handlers/iam/roles.go +++ b/spinifex/handlers/iam/roles.go @@ -673,12 +673,22 @@ func roleToSDK(r *Role) *iam.Role { return out } -// STSActionAssumeRoleWithWebIdentity is the only action that may carry a Condition block. +// STSActionAssumeRoleWithWebIdentity is one of the two actions that may carry a Condition block. const STSActionAssumeRoleWithWebIdentity = "sts:AssumeRoleWithWebIdentity" +// stsActionAssumeRole mirrors handlers_sts's unexported constant of the same value. +// Duplicated rather than imported: handlers_sts imports this package, not the reverse. +const stsActionAssumeRole = "sts:AssumeRole" + +// conditionKeySourceAccount is the only condition key accepted on the sts:AssumeRole +// trust-policy path. handlers_sts.assumeRoleConditionsHold is the evaluator that must +// accept exactly this key for this action — the two lists must never drift apart, or +// a stored condition becomes a security control nothing enforces. +const conditionKeySourceAccount = "aws:SourceAccount" + // ValidateTrustPolicyDocument parses and validates an AssumeRolePolicyDocument JSON string. -// Rejects NotPrincipal and NotAction at write time. Condition blocks are accepted only for -// sts:AssumeRoleWithWebIdentity with StringEquals — any wider shape would be silently ignored at runtime. +// Rejects NotPrincipal and NotAction at write time. Condition blocks are accepted only on the +// two actions STS evaluates conditions for — any wider shape would be silently ignored at runtime. func ValidateTrustPolicyDocument(docJSON string) (*TrustPolicyDocument, error) { if len(docJSON) > maxTrustPolicyDocumentSize { return nil, fmt.Errorf("trust policy exceeds maximum size of %d bytes", maxTrustPolicyDocumentSize) @@ -728,24 +738,57 @@ func ValidateTrustPolicyDocument(docJSON string) (*TrustPolicyDocument, error) { return &doc, nil } -// validateWebIdentityCondition gates Condition blocks to the IRSA case: -// Action must be exactly sts:AssumeRoleWithWebIdentity and operator must be StringEquals only. +// validateWebIdentityCondition gates Condition blocks to the two actions STS +// evaluates conditions for: sts:AssumeRoleWithWebIdentity (the IRSA {iss}:sub / +// {iss}:aud keys, StringEquals only) and sts:AssumeRole (aws:SourceAccount, +// StringEquals only). Everything else is rejected at write time — accepting a +// condition nothing evaluates would store a security control that never fires. func validateWebIdentityCondition(i int, stmt TrustStatement) error { - if len(stmt.Action) != 1 || stmt.Action[0] != STSActionAssumeRoleWithWebIdentity { - return fmt.Errorf("statement %d: trust policy Condition blocks are only supported on statements whose Action is exactly [%q]; v1 does not evaluate conditions for other actions", i, STSActionAssumeRoleWithWebIdentity) + var action string + if len(stmt.Action) == 1 { + action = stmt.Action[0] } - var ops map[string]json.RawMessage + if action != STSActionAssumeRoleWithWebIdentity && action != stsActionAssumeRole { + return fmt.Errorf("statement %d: trust policy Condition blocks are only supported on statements whose Action is exactly [%q] or [%q]; v1 does not evaluate conditions for other actions", i, STSActionAssumeRoleWithWebIdentity, stsActionAssumeRole) + } + + var ops map[string]map[string]json.RawMessage if err := json.Unmarshal(stmt.Condition, &ops); err != nil { - return fmt.Errorf("statement %d: Condition must be a JSON object: %w", i, err) + return fmt.Errorf("statement %d: Condition must be a JSON object of operator to key/value: %w", i, err) } - for op := range ops { + + if action == STSActionAssumeRoleWithWebIdentity { + for op := range ops { + if op != "StringEquals" { + return fmt.Errorf("statement %d: Condition operator %q on action %q is not supported in this release; only StringEquals is supported", i, op, action) + } + } + return nil + } + + // action == stsActionAssumeRole: the evaluator (handlers_sts.assumeRoleConditionsHold) + // enforces only aws:SourceAccount under StringEquals; name the offending + // operator, key and action rather than a bare "malformed" refusal. + for op, keys := range ops { if op != "StringEquals" { - return fmt.Errorf("statement %d: Condition operator %q is not supported in this release; only StringEquals is supported", i, op) + return fmt.Errorf("statement %d: Condition operator %q on key %q for action %q is not supported in this release; only StringEquals is supported", i, op, conditionKeysJoined(keys), action) + } + for key := range keys { + if key != conditionKeySourceAccount { + return fmt.Errorf("statement %d: Condition key %q with operator %q for action %q is not supported in this release; only %q is supported — see docs/specs/platform/rfc0-7_aws-divergence.md", i, key, op, action, conditionKeySourceAccount) + } } } return nil } +// conditionKeysJoined names the keys under a rejected operator so the error +// message identifies what was written, not just that the operator is wrong. +func conditionKeysJoined(keys map[string]json.RawMessage) string { + names := slices.Sorted(maps.Keys(keys)) + return strings.Join(names, ", ") +} + // isRawJSONNonEmpty reports whether a json.RawMessage carries a meaningful value. // Whitespace, null, and empty object/array all count as empty. func isRawJSONNonEmpty(raw json.RawMessage) bool { diff --git a/spinifex/handlers/iam/roles_test.go b/spinifex/handlers/iam/roles_test.go index c9a743048..1ce0d6caf 100644 --- a/spinifex/handlers/iam/roles_test.go +++ b/spinifex/handlers/iam/roles_test.go @@ -169,10 +169,20 @@ func TestCreateRole_MalformedTrustPolicy_NamesTheReason(t *testing.T) { want []string }{ { - name: "condition-on-a-non-web-identity-action", - doc: `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Principal":{"Service":"ecs-tasks.amazonaws.com"},"Action":"sts:AssumeRole","Condition":{"StringEquals":{"aws:SourceAccount":"000000000001"}}}]}`, + // sts:AssumeRole is itself one of the two actions Condition blocks are + // supported on, so this exercises an action outside both, which must + // still be refused at write time. + name: "condition-on-an-unsupported-action", + doc: `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Principal":{"Service":"ecs-tasks.amazonaws.com"},"Action":"sts:GetSessionToken","Condition":{"StringEquals":{"aws:SourceAccount":"000000000001"}}}]}`, want: []string{"statement 0", STSActionAssumeRoleWithWebIdentity}, }, + { + // sts:AssumeRole is supported, but only for aws:SourceAccount — every + // other key must still be refused, naming the key and the action. + name: "unsupported-key-on-assume-role", + doc: `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Principal":{"Service":"ecs-tasks.amazonaws.com"},"Action":"sts:AssumeRole","Condition":{"StringEquals":{"aws:SourceArn":"arn:aws:ecs:ap-southeast-2:000000000001:*"}}}]}`, + want: []string{"statement 0", "aws:SourceArn", "sts:AssumeRole"}, + }, { name: "bad-effect", doc: `{"Version":"2012-10-17","Statement":[{"Effect":"Maybe","Principal":{"Service":"ec2.amazonaws.com"},"Action":"sts:AssumeRole"}]}`, @@ -208,7 +218,7 @@ func TestUpdateAssumeRolePolicy_MalformedTrustPolicy_NamesTheReason(t *testing.T _, err := svc.UpdateAssumeRolePolicy(testAccountID, &iam.UpdateAssumeRolePolicyInput{ RoleName: aws.String("update-reason"), - PolicyDocument: aws.String(`{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Principal":{"Service":"ecs-tasks.amazonaws.com"},"Action":"sts:AssumeRole","Condition":{"StringEquals":{"aws:SourceAccount":"000000000001"}}}]}`), + PolicyDocument: aws.String(`{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Principal":{"Service":"ecs-tasks.amazonaws.com"},"Action":"sts:GetSessionToken","Condition":{"StringEquals":{"aws:SourceAccount":"000000000001"}}}]}`), }) require.Error(t, err) code, message, ok := awserrors.ResolveErrorDetail(err) @@ -1373,6 +1383,63 @@ func TestCreateRole_MalformedTrustPolicy_ConditionRejected_IpAddress(t *testing. assert.Contains(t, err.Error(), awserrors.ErrorIAMMalformedPolicyDocument) } +// A StringEquals condition on aws:SourceAccount for sts:AssumeRole is accepted +// at write time, where before every Condition on that action was refused +// outright regardless of key. +func TestCreateRole_TrustPolicy_SourceAccountAccepted(t *testing.T) { + t.Parallel() + svc := setupTestIAMService(t) + + doc := `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Principal":{"Service":"ecs-tasks.amazonaws.com"},"Action":"sts:AssumeRole","Condition":{"StringEquals":{"aws:SourceAccount":"` + testAccountID + `"}}}]}` + _, err := svc.CreateRole(testAccountID, &iam.CreateRoleInput{ + RoleName: aws.String("with-sourceaccount"), + AssumeRolePolicyDocument: aws.String(doc), + }) + require.NoError(t, err) +} + +// The ECS module's own aws:SourceArn condition is refused with a message +// naming the key, not a bare MalformedPolicyDocument. +func TestCreateRole_MalformedTrustPolicy_SourceArnRejected_NamesTheKey(t *testing.T) { + t.Parallel() + svc := setupTestIAMService(t) + + doc := `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Principal":{"Service":"ecs-tasks.amazonaws.com"},"Action":"sts:AssumeRole","Condition":{"ArnLike":{"aws:SourceArn":"arn:aws:ecs:ap-southeast-2:` + testAccountID + `:*"}}}]}` + _, err := svc.CreateRole(testAccountID, &iam.CreateRoleInput{ + RoleName: aws.String("with-sourcearn"), + AssumeRolePolicyDocument: aws.String(doc), + }) + require.Error(t, err) + + code, message, ok := awserrors.ResolveErrorDetail(err) + require.True(t, ok) + assert.Equal(t, awserrors.ErrorIAMMalformedPolicyDocument, code) + assert.Contains(t, message, "aws:SourceArn") + assert.Contains(t, message, "sts:AssumeRole") +} + +// TestCreateRole_MalformedTrustPolicy_SourceArnUnderStringEquals_NamesTheKey +// exercises the key-rejection branch directly: StringEquals is the supported +// operator, so the refusal must come from the key itself, not the operator. +func TestCreateRole_MalformedTrustPolicy_SourceArnUnderStringEquals_NamesTheKey(t *testing.T) { + t.Parallel() + svc := setupTestIAMService(t) + + doc := `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Principal":{"Service":"ecs-tasks.amazonaws.com"},"Action":"sts:AssumeRole","Condition":{"StringEquals":{"aws:SourceArn":"arn:aws:ecs:ap-southeast-2:` + testAccountID + `:*"}}}]}` + _, err := svc.CreateRole(testAccountID, &iam.CreateRoleInput{ + RoleName: aws.String("with-sourcearn-stringequals"), + AssumeRolePolicyDocument: aws.String(doc), + }) + require.Error(t, err) + + code, message, ok := awserrors.ResolveErrorDetail(err) + require.True(t, ok) + assert.Equal(t, awserrors.ErrorIAMMalformedPolicyDocument, code) + assert.Contains(t, message, "aws:SourceArn") + assert.Contains(t, message, "StringEquals") + assert.Contains(t, message, "sts:AssumeRole") +} + func TestCreateRole_TrustPolicy_EmptyConditionAccepted(t *testing.T) { t.Parallel() svc := setupTestIAMService(t) diff --git a/spinifex/handlers/iam/service_impl.go b/spinifex/handlers/iam/service_impl.go index 4741d4c2f..a551bb2b7 100644 --- a/spinifex/handlers/iam/service_impl.go +++ b/spinifex/handlers/iam/service_impl.go @@ -2217,7 +2217,7 @@ func validateStatementRestrictions(i int, stmt Statement) error { for op, keys := range stmt.Condition { for key, values := range keys { if !iampolicy.SupportedCondition(op, key) { - return fmt.Errorf("statement %d: Condition operator %q on key %q is not supported in this release", i, op, key) + return fmt.Errorf("statement %d: Condition operator %q on key %q is not supported in this release%s", i, op, key, unsupportedConditionKeyReason(key)) } if err := validateConditionValues(i, op, key, values); err != nil { return err @@ -2227,6 +2227,20 @@ func validateStatementRestrictions(i int, stmt Statement) error { return nil } +// unsupportedConditionKeyReason names why a specific rejected key cannot be +// enforced, for the keys that have a recorded divergence. Empty for any other +// unsupported key, leaving the base message unchanged. +func unsupportedConditionKeyReason(key string) string { + switch { + case key == "aws:SourceArn": + return "; no server-side binding exists from a caller's session to the ARN of the resource that invoked it — see docs/specs/platform/rfc0-7_aws-divergence.md" + case strings.HasPrefix(key, "aws:RequestTag/"): + return "; request-tag values are not resolved at any policy door yet — see docs/specs/platform/rfc0-7_aws-divergence.md" + default: + return "" + } +} + // validateConditionValues rejects leaf values the matcher can only ever compare // false. An allowlisted operator over an unparseable value is still an inert // restriction, which is the failure this validation exists to prevent. diff --git a/spinifex/handlers/sts/assume_role.go b/spinifex/handlers/sts/assume_role.go index 0f9add53d..e69843f65 100644 --- a/spinifex/handlers/sts/assume_role.go +++ b/spinifex/handlers/sts/assume_role.go @@ -206,6 +206,11 @@ func (s *STSServiceImpl) assumeRoleForCaller(ctx context.Context, callerARN, pri }, nil } +// assumeRoleConditionKeySourceAccount is the only condition key evaluated on the +// sts:AssumeRole trust-policy path. validateWebIdentityCondition (handlers_iam) +// is the write-time gate that must accept exactly this key for this action. +const assumeRoleConditionKeySourceAccount = "aws:SourceAccount" + // evalTrustPolicy implements AWS's explicit-deny-wins semantics: first pass scans all // Deny statements, second pass scans all Allows. A single-pass loop would skip a later // Deny and silently grant access. @@ -216,6 +221,15 @@ func evalTrustPolicy(docJSON, callerARN string, principalSources []string) error return fmt.Errorf("stored trust policy invalid: %w", err) } + // sourceAccount is the caller ARN's own account — available whenever the + // caller is an authenticated HTTPS SigV4 identity, and not caller-asserted. + // It is empty on the IMDS path, where principalSource is synthesised and + // there is no caller ARN to parse. + sourceAccount := "" + if caller, ok := parsePrincipalARN(callerARN); ok { + sourceAccount = caller.account + } + for _, stmt := range doc.Statement { if stmt.Effect != handlers_iam.PolicyEffectDeny { continue @@ -227,7 +241,7 @@ func evalTrustPolicy(docJSON, callerARN string, principalSources []string) error if err != nil { return err } - if match { + if match && assumeRoleConditionsHold(stmt.Condition, sourceAccount) { return errors.New(awserrors.ErrorAccessDenied) } } @@ -243,7 +257,7 @@ func evalTrustPolicy(docJSON, callerARN string, principalSources []string) error if err != nil { return err } - if match { + if match && assumeRoleConditionsHold(stmt.Condition, sourceAccount) { return nil } } @@ -251,6 +265,39 @@ func evalTrustPolicy(docJSON, callerARN string, principalSources []string) error return errors.New(awserrors.ErrorAccessDenied) } +// assumeRoleConditionsHold evaluates a trust-policy Condition block on the +// sts:AssumeRole path. Only StringEquals on aws:SourceAccount is supported — +// the write-time gate accepts nothing wider for this action — and an unknown +// key or operator fails closed rather than being silently ignored, the same +// shape conditionsHold uses for AssumeRoleWithWebIdentity. No Condition holds +// trivially, matching the pre-existing no-condition-arm behaviour. +func assumeRoleConditionsHold(raw json.RawMessage, sourceAccount string) bool { + if !isCondRawNonEmpty(raw) { + return true + } + var ops map[string]map[string]json.RawMessage + if err := json.Unmarshal(raw, &ops); err != nil { + return false + } + for op, kv := range ops { + if op != "StringEquals" { + // Validator rejects other operators at write time; reaching here means tampering. + return false + } + for key, expectedRaw := range kv { + if key != assumeRoleConditionKeySourceAccount { + // Unknown condition key — fail closed rather than silently ignore a stricter check. + return false + } + expected, ok := unmarshalStringOrArray(expectedRaw) + if !ok || sourceAccount == "" || !anyEquals(expected, []string{sourceAccount}) { + return false + } + } + } + return true +} + func matchTrustAction(actions []string) bool { for _, a := range actions { if a == stsActionAssumeRole || a == stsActionWildcard || a == globalWildcard { diff --git a/spinifex/handlers/sts/assume_role_test.go b/spinifex/handlers/sts/assume_role_test.go index 871a138e1..3c857c1f8 100644 --- a/spinifex/handlers/sts/assume_role_test.go +++ b/spinifex/handlers/sts/assume_role_test.go @@ -4,6 +4,7 @@ import ( "crypto/hmac" "crypto/sha256" "encoding/base64" + "encoding/json" "fmt" "strings" "testing" @@ -657,6 +658,94 @@ func TestAssumeRole_ECSTasks_ExplicitDenyWins(t *testing.T) { assert.Equal(t, awserrors.ErrorAccessDenied, err.Error()) } +// ----- Trust-policy Condition evaluation on sts:AssumeRole (mulga-yxd9p, B) -- + +// trustPolicyAllowingECSTasksWithSourceAccount is the shape terraform-aws-modules/ecs +// writes: a Service principal plus an aws:SourceAccount confused-deputy condition. +func trustPolicyAllowingECSTasksWithSourceAccount(accountID string) string { + return fmt.Sprintf(`{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Principal":{"Service":"ecs-tasks.amazonaws.com"},"Action":"sts:AssumeRole","Condition":{"StringEquals":{"aws:SourceAccount":%q}}}]}`, accountID) +} + +// TestAssumeRole_SourceAccountCondition_MatchingAccountGrants pins that the new +// condition arm on evalTrustPolicy does not merely tolerate a stored +// aws:SourceAccount condition — it is evaluated, and a caller attributed to the +// role's own account (the only shape serviceSourcesForCaller ever attributes) grants. +func TestAssumeRole_SourceAccountCondition_MatchingAccountGrants(t *testing.T) { + svc, _ := newTestSetup(t) + role := createRoleInAccount(t, svc, testCallerAccountID, "task-role-sa-match", + trustPolicyAllowingECSTasksWithSourceAccount(testCallerAccountID)) + + out, err := svc.AssumeRole(testCallerAccountID, instanceRoleCallerARN(testCallerAccountID, testInstanceID), + ecsInstanceRoleName, basicAssumeRoleInput(*role.Arn, "ecs-task-sa-1")) + require.NoError(t, err) + require.NotNil(t, out.Credentials) +} + +// TestAssumeRole_SourceAccountCondition_NonMatchingAccountDenies is the +// discriminating case: the condition names an account other than the caller's +// own, so it must not be satisfied even though the Service principal matches. +func TestAssumeRole_SourceAccountCondition_NonMatchingAccountDenies(t *testing.T) { + svc, _ := newTestSetup(t) + role := createRoleInAccount(t, svc, testCallerAccountID, "task-role-sa-mismatch", + trustPolicyAllowingECSTasksWithSourceAccount(testCrossAccountID)) + + _, err := svc.AssumeRole(testCallerAccountID, instanceRoleCallerARN(testCallerAccountID, testInstanceID), + ecsInstanceRoleName, basicAssumeRoleInput(*role.Arn, "ecs-task-sa-2")) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorAccessDenied, err.Error()) +} + +// TestAssumeRole_SourceAccountCondition_DenyEvaluatedBeforeAllow pins that the +// Deny pass still runs first when a condition is present: an unconditional +// Allow is listed first, but a conditioned Deny matching the caller's account +// must still win. +func TestAssumeRole_SourceAccountCondition_DenyEvaluatedBeforeAllow(t *testing.T) { + svc, _ := newTestSetup(t) + policy := fmt.Sprintf(`{"Version":"2012-10-17","Statement":[ + {"Effect":"Allow","Principal":{"Service":"ecs-tasks.amazonaws.com"},"Action":"sts:AssumeRole"}, + {"Effect":"Deny","Principal":{"Service":"ecs-tasks.amazonaws.com"},"Action":"sts:AssumeRole","Condition":{"StringEquals":{"aws:SourceAccount":%q}}} + ]}`, testCallerAccountID) + role := createRoleInAccount(t, svc, testCallerAccountID, "task-role-sa-deny-first", policy) + + _, err := svc.AssumeRole(testCallerAccountID, instanceRoleCallerARN(testCallerAccountID, testInstanceID), + ecsInstanceRoleName, basicAssumeRoleInput(*role.Arn, "ecs-task-sa-3")) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorAccessDenied, err.Error()) +} + +// TestAssumeRoleConditionsHold_UnknownKeyFailsClosed unit-tests the evaluator +// directly: the write-time gate never lets an unrecognised key reach this +// function through CreateRole, so the only way to exercise the fail-closed +// branch is a stored document the current gate would no longer accept — +// exactly the corruption case evalTrustPolicy's own doc comment names. +func TestAssumeRoleConditionsHold_UnknownKeyFailsClosed(t *testing.T) { + raw := json.RawMessage(`{"StringEquals":{"aws:PrincipalTag/team":"platform"}}`) + assert.False(t, assumeRoleConditionsHold(raw, testCallerAccountID), + "an unrecognised condition key must fail closed, not be silently ignored") +} + +// TestAssumeRoleConditionsHold_NoConditionAlwaysHolds pins the pre-existing +// no-condition-arm behaviour: an absent Condition must keep matching, exactly +// as it did before this evaluator existed. +func TestAssumeRoleConditionsHold_NoConditionAlwaysHolds(t *testing.T) { + assert.True(t, assumeRoleConditionsHold(nil, "")) + assert.True(t, assumeRoleConditionsHold(json.RawMessage(`{}`), "")) +} + +// TestAssumeRole_ECSTasks_Regression_NoConditionStillGrants is the regression +// the plan calls out by name: the ECS task-role attribution path this whole +// change sits on top of must keep working for a trust policy that carries no +// Condition at all. +func TestAssumeRole_ECSTasks_Regression_NoConditionStillGrants(t *testing.T) { + svc, _ := newTestSetup(t) + role := createRoleInAccount(t, svc, testCallerAccountID, "task-role-regression", trustPolicyAllowingECSTasks()) + + out, err := svc.AssumeRole(testCallerAccountID, instanceRoleCallerARN(testCallerAccountID, testInstanceID), + ecsInstanceRoleName, basicAssumeRoleInput(*role.Arn, "ecs-task-regression")) + require.NoError(t, err) + require.NotNil(t, out.Credentials) +} + func TestServiceSourcesForCaller(t *testing.T) { const acct = testCallerAccountID cases := []struct {