diff --git a/docs/identity/iam-roles-and-instance-profiles/README.md b/docs/identity/iam-roles-and-instance-profiles/README.md index d7449030f..3cacb26f3 100644 --- a/docs/identity/iam-roles-and-instance-profiles/README.md +++ b/docs/identity/iam-roles-and-instance-profiles/README.md @@ -281,7 +281,7 @@ aws ec2 disassociate-iam-instance-profile \ ## Assuming a Role -Users and services can assume a role directly with STS, provided the trust policy allows their principal: +Users and services can assume a role directly with STS, provided **both** gates pass: the role's trust policy admits their principal, and their own identity policy grants `sts:AssumeRole` on the role's ARN. See [STS](/docs/sts) for the identity-side grant. ```bash aws sts assume-role \ diff --git a/docs/identity/sts/README.md b/docs/identity/sts/README.md index b7af6c778..00c6f8661 100644 --- a/docs/identity/sts/README.md +++ b/docs/identity/sts/README.md @@ -42,7 +42,7 @@ STS (Security Token Service) issues **temporary credentials**: an `ASIA`-prefixe Spinifex supports the three main ways to obtain them: -- **`assume-role`** — exchange your IAM user credentials for a [role's](/docs/iam-roles-and-instance-profiles) permissions, gated by the role's trust policy. +- **`assume-role`** — exchange your IAM user credentials for a [role's](/docs/iam-roles-and-instance-profiles) permissions, gated by both the role's trust policy and your own `sts:AssumeRole` grant. - **`get-session-token`** — get a time-boxed copy of your own [IAM user's](/docs/iam-users-and-policies) permissions. - **`assume-role-with-web-identity`** — exchange an OIDC ID token (a Kubernetes ServiceAccount token) for role credentials, with no IAM credentials at all. @@ -79,7 +79,14 @@ Run with temporary role credentials, the ARN switches to the assumed-role form ## Assuming a Role -`assume-role` works when the target role's trust policy allows your principal. Create a role that trusts an IAM user (see [IAM Roles and Instance Profiles](/docs/iam-roles-and-instance-profiles) for the full role lifecycle): +`assume-role` needs two independent grants, and a missing one denies the call: + +1. The target role's **trust policy** must admit your principal. +2. Your own **identity policy** must grant `sts:AssumeRole` on the role's ARN. + +The second is why `"Principal": {"AWS": "arn:aws:iam::000000000001:root"}` in a trust policy is safe: it delegates the decision to identity policies in that account rather than opening the role to everyone in it. + +Create a role that trusts an IAM user (see [IAM Roles and Instance Profiles](/docs/iam-roles-and-instance-profiles) for the full role lifecycle): ```bash cat > /tmp/deploy-trust.json << 'EOF' @@ -102,6 +109,15 @@ aws iam put-role-policy --role-name deploy --policy-name ec2-read \ --policy-document '{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["ec2:Describe*"],"Resource":"*"}]}' ``` +That policy is what the *session* may do. The caller still needs its own grant to make the call at all: + +```bash +aws iam put-user-policy --user-name admin --policy-name assume-deploy \ + --policy-document '{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":"sts:AssumeRole","Resource":"arn:aws:iam::000000000001:role/deploy"}]}' +``` + +Scope `Resource` to the role ARN exactly as `create-role` returned it, path included — a wildcard such as `arn:aws:iam::000000000001:role/*` grants assumption of every role the account's trust policies admit you to. + Then assume it: ```bash @@ -128,7 +144,7 @@ aws sts assume-role \ The session name tags every session so audit logs can tell *who* used the role; it appears in the assumed-role ARN and the `UserId`. -If the trust policy does not allow your principal, `assume-role` fails with `AccessDenied` — no matter what IAM permissions you hold. +If the trust policy does not allow your principal, `assume-role` fails with `AccessDenied` — no matter what IAM permissions you hold. It fails the same way when your own identity policy carries no `sts:AssumeRole` grant on the role, no matter what the trust policy says. ### Session Duration @@ -303,7 +319,13 @@ Everything the validator accepts is evaluated at assume time; explicit `Deny` st ### AccessDenied When Assuming a Role -The role's trust policy does not allow your principal, or the role does not exist — a missing role is deliberately indistinguishable from a denied one, matching AWS. Check who you are, that the role exists, and what it trusts: +Either gate can produce this. A denial from your own identity policy names the principal, action and resource: + +``` +User: arn:aws:iam::000000000001:user/admin is not authorized to perform: sts:AssumeRole on resource: arn:aws:iam::000000000001:role/deploy +``` + +A bare `AccessDenied` means the role's trust policy does not allow your principal, or the role does not exist — a missing role is deliberately indistinguishable from a denied one, matching AWS. Check who you are, that the role exists, what it trusts, and what you are granted: ```bash aws sts get-caller-identity diff --git a/spinifex/gateway/admin.go b/spinifex/gateway/admin.go index cbdc6538f..c2f6f1f11 100644 --- a/spinifex/gateway/admin.go +++ b/spinifex/gateway/admin.go @@ -101,7 +101,7 @@ func (gw *GatewayConfig) Admin_Request(w http.ResponseWriter, r *http.Request) { } if err := gw.authorizeAdmin(r, method); err != nil { - gw.writeAdminError(w, requestID, err.Error(), "") + gw.writeAdminErrorFor(w, requestID, err) return } @@ -141,7 +141,7 @@ func (gw *GatewayConfig) Admin_Request(w http.ResponseWriter, r *http.Request) { err = errors.New(awserrors.ErrorInvalidAction) } if err != nil { - gw.writeAdminError(w, requestID, err.Error(), "") + gw.writeAdminErrorFor(w, requestID, err) return } @@ -199,9 +199,23 @@ func (gw *GatewayConfig) authorizeAdmin(r *http.Request, method string) error { return gw.checkPolicy(r, "spinifex", method) } +// writeAdminErrorFor resolves an error to its code and any message its call +// site attached, so a wrapped error renders as its own code instead of falling +// through to InternalError. +func (gw *GatewayConfig) writeAdminErrorFor(w http.ResponseWriter, requestID string, err error) { + code, message, ok := awserrors.ResolveErrorDetail(err) + if !ok { + // The cause reaches neither the wire nor the envelope's own unmapped-code + // log, so this is the only place it is recorded. + slog.Error("Admin: unresolvable error", "requestId", requestID, "err", err) + code = awserrors.ErrorInternalError + } + gw.writeAdminError(w, requestID, code, message) +} + // writeAdminError writes the JSON error envelope. message overrides the -// registry text when a specific cause is safe to disclose; it must never carry -// a parameter value, only a parameter name. +// registry text when a specific cause is safe to disclose; a policy denial +// names principal, action and resource, all of them caller-derivable. func (gw *GatewayConfig) writeAdminError(w http.ResponseWriter, requestID, code, message string) { detail, ok := awserrors.ErrorLookup[code] if !ok { diff --git a/spinifex/gateway/admin_deleteaccount_job_test.go b/spinifex/gateway/admin_deleteaccount_job_test.go index a5b70edd7..995827990 100644 --- a/spinifex/gateway/admin_deleteaccount_job_test.go +++ b/spinifex/gateway/admin_deleteaccount_job_test.go @@ -361,7 +361,7 @@ func TestAdminDeleteAccountRefusesProtectedAccounts(t *testing.T) { deleteAccountBody(t, accountID, "super-admin@example.com", deleteClientToken)) require.Error(t, err) - assert.Equal(t, awserrors.ErrorAccessDenied, err.Error()) + assertDenied(t, err) }) } } diff --git a/spinifex/gateway/admin_test.go b/spinifex/gateway/admin_test.go index 9acb25241..b4511000d 100644 --- a/spinifex/gateway/admin_test.go +++ b/spinifex/gateway/admin_test.go @@ -145,7 +145,8 @@ func TestAuthorizeAdminGates(t *testing.T) { return } require.Error(t, err) - assert.Equal(t, tc.wantErr, err.Error()) + code, _ := awserrors.ResolveErrorCode(err) + assert.Equal(t, tc.wantErr, code) }) } } @@ -158,7 +159,7 @@ func TestAuthorizeAdminDeniesUnauthenticated(t *testing.T) { err := gw.authorizeAdmin(req, "CreateAccount") require.Error(t, err) - assert.Equal(t, awserrors.ErrorAccessDenied, err.Error()) + assertDenied(t, err) } // adminRequest drives Admin_Request with the chi route param the router sets. diff --git a/spinifex/gateway/conditionkeys_test.go b/spinifex/gateway/conditionkeys_test.go index f51a3f87c..81367773f 100644 --- a/spinifex/gateway/conditionkeys_test.go +++ b/spinifex/gateway/conditionkeys_test.go @@ -153,7 +153,8 @@ func TestCheckPolicyResources_ConditionalAllowHonoursSourceIP(t *testing.T) { return } require.Error(t, err) - assert.Equal(t, tt.wantErr, err.Error()) + code, _ := awserrors.ResolveErrorCode(err) + assert.Equal(t, tt.wantErr, code) }) } } @@ -189,7 +190,8 @@ func TestCheckPolicyResources_ConditionalDenyHonoursSourceIP(t *testing.T) { return } require.Error(t, err) - assert.Equal(t, tt.wantErr, err.Error()) + code, _ := awserrors.ResolveErrorCode(err) + assert.Equal(t, tt.wantErr, code) }) } } diff --git a/spinifex/gateway/ec2_authz_test.go b/spinifex/gateway/ec2_authz_test.go index 96828d0c6..1e2dd6299 100644 --- a/spinifex/gateway/ec2_authz_test.go +++ b/spinifex/gateway/ec2_authz_test.go @@ -50,10 +50,24 @@ func dispatchEC2(t *testing.T, gw *GatewayConfig, body string) error { return gw.EC2_Request(httptest.NewRecorder(), setupEC2Request(body, authzAccountID)) } +// assertDenied matches on the resolved code, not the message: a policy denial +// names the principal, action and resource ahead of the code. func assertDenied(t *testing.T, err error) { t.Helper() require.Error(t, err) - assert.Equal(t, awserrors.ErrorAccessDenied, err.Error()) + code, ok := awserrors.ResolveErrorCode(err) + require.True(t, ok, "unresolvable error code: %v", err) + assert.Equal(t, awserrors.ErrorAccessDenied, code) +} + +// assertNotDenied asserts the policy gate passed where the handler failure that +// follows is not fixed. It resolves the code for the same reason assertDenied +// does: the denial message is no longer the bare code. +func assertNotDenied(t *testing.T, err error) { + t.Helper() + require.Error(t, err) + code, _ := awserrors.ResolveErrorCode(err) + assert.NotEqual(t, awserrors.ErrorAccessDenied, code) } // assertPermitted asserts the policy gate passed. The request then fails on the diff --git a/spinifex/gateway/ecr_authz_test.go b/spinifex/gateway/ecr_authz_test.go index 25a4fcca8..6c2af4d47 100644 --- a/spinifex/gateway/ecr_authz_test.go +++ b/spinifex/gateway/ecr_authz_test.go @@ -82,15 +82,7 @@ func TestECRRequest_OmittedRepositoryListEnumeratesTheRegistry(t *testing.T) { wide := scopedPolicyGateway( statement("Allow", "ecr:DescribeRepositories", "arn:aws:ecr:*:*:repository/*"), ) - assert.NotEqual(t, awserrors.ErrorAccessDenied, - errString(dispatchECR(t, wide, "DescribeRepositories", `{}`))) -} - -func errString(err error) string { - if err == nil { - return "" - } - return err.Error() + assertNotDenied(t, dispatchECR(t, wide, "DescribeRepositories", `{}`)) } // A caller-supplied registryId would let a request slide out from under a Deny diff --git a/spinifex/gateway/ecs_authz_test.go b/spinifex/gateway/ecs_authz_test.go index 09e0cf491..e7824ecc7 100644 --- a/spinifex/gateway/ecs_authz_test.go +++ b/spinifex/gateway/ecs_authz_test.go @@ -32,7 +32,7 @@ func dispatchECS(t *testing.T, gw *GatewayConfig, action, body string) error { func assertReachedHandler(t *testing.T, err error) { t.Helper() require.Error(t, err) - assert.NotEqual(t, awserrors.ErrorAccessDenied, err.Error()) + assertNotDenied(t, err) } func ecsResourceARN(resource string) string { diff --git a/spinifex/gateway/ecs_passrole_test.go b/spinifex/gateway/ecs_passrole_test.go index 55cb3cd53..2a4d7870a 100644 --- a/spinifex/gateway/ecs_passrole_test.go +++ b/spinifex/gateway/ecs_passrole_test.go @@ -9,7 +9,6 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ecs" - "github.com/mulgadc/spinifex/spinifex/awserrors" handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -69,7 +68,7 @@ func TestECSRegisterTaskDefinition_DeniedWithoutPassRole(t *testing.T) { err := gw.ECS_Request(httptest.NewRecorder(), req) require.Error(t, err) - assert.Equal(t, awserrors.ErrorAccessDenied, err.Error()) + assertDenied(t, err) } func TestECSRegisterTaskDefinition_AllowedWithPassRole(t *testing.T) { @@ -87,7 +86,7 @@ func TestECSRegisterTaskDefinition_AllowedWithPassRole(t *testing.T) { // disconnected) NATS dispatch; whatever fails downstream, it must not be // the PassRole denial. require.Error(t, err) - assert.NotEqual(t, awserrors.ErrorAccessDenied, err.Error()) + assertNotDenied(t, err) } func TestECSRegisterTaskDefinition_NoRoleSkipsPassRole(t *testing.T) { @@ -104,12 +103,11 @@ func TestECSRegisterTaskDefinition_NoRoleSkipsPassRole(t *testing.T) { err := gw.ECS_Request(httptest.NewRecorder(), req) require.Error(t, err) // still fails downstream (nil NATS conn), just not on PassRole - assert.NotEqual(t, awserrors.ErrorAccessDenied, err.Error()) + assertNotDenied(t, err) } -// TestECSRegisterTaskDefinition_DenialRendersAccessDenied403 proves the denial -// reaches the wire as AWS's real PassRole-denial shape: HTTP 403, AWS JSON 1.1 -// envelope, the same AccessDenied code/message the EC2 iam:PassRole path uses. +// TestECSRegisterTaskDefinition_DenialRendersAccessDenied403 proves shared +// policy denials stay opaque while retaining the service's 403 envelope. func TestECSRegisterTaskDefinition_DenialRendersAccessDenied403(t *testing.T) { mock := &policyMockIAMService{ getUserPoliciesFn: func(_, _ string) ([]handlers_iam.PolicyDocument, error) { diff --git a/spinifex/gateway/gateway.go b/spinifex/gateway/gateway.go index e950b27b8..fa56ffb6b 100644 --- a/spinifex/gateway/gateway.go +++ b/spinifex/gateway/gateway.go @@ -692,12 +692,39 @@ func (gw *GatewayConfig) evaluatePrincipalPolicyResources( if iampolicy.EvaluateWithKeys(iamAction, resource, policies, keys) == iampolicy.Deny { slog.Info("evaluatePrincipalPolicy: access denied", "identity", logIdentity, "action", iamAction, "resource", resource) - return errors.New(awserrors.ErrorAccessDenied) + return &identityPolicyDenialError{ + principal: principal, logIdentity: logIdentity, + action: iamAction, resource: resource, + } } } return nil } +// identityPolicyDenialError keeps shared authorization failures opaque. A +// caller that proves its resource is non-sensitive may opt into details. +type identityPolicyDenialError struct { + principal principalContext + logIdentity string + action string + resource string +} + +func (d *identityPolicyDenialError) Error() string { + return awserrors.ErrorAccessDenied +} + +func (d *identityPolicyDenialError) detailedError() error { + callerARN, err := buildCallerARN(d.principal.accountID, d.principal.identity, + d.principal.principalType, d.principal.assumedRoleARN) + if err != nil { + callerARN = d.logIdentity + } + return awserrors.Errorf(awserrors.ErrorAccessDenied, + "User: %s is not authorized to perform: %s on resource: %s", + callerARN, d.action, d.resource) +} + func (gw *GatewayConfig) ErrorHandler(w http.ResponseWriter, r *http.Request, err error) { svc, _ := gw.GetService(r) slog.Debug("ErrorHandler", "service", svc, "error", err.Error()) diff --git a/spinifex/gateway/gateway_test.go b/spinifex/gateway/gateway_test.go index 3f50cdcbf..28baea544 100644 --- a/spinifex/gateway/gateway_test.go +++ b/spinifex/gateway/gateway_test.go @@ -1115,9 +1115,36 @@ func TestCheckPolicy_FailsClosed(t *testing.T) { } } +// Shared authorization errors must not expose a canonical resource that was +// resolved from storage. STS opts into detailed wording at its dispatcher. +func TestCheckPolicyResources_DenialIsOpaque(t *testing.T) { + mock := &policyMockIAMService{ + getUserPoliciesFn: func(_, _ string) ([]handlers_iam.PolicyDocument, error) { + return nil, nil + }, + } + gw := &GatewayConfig{DisableLogging: true, IAMService: mock} + req := httptest.NewRequest(http.MethodPost, "/", nil) + ctx := context.WithValue(req.Context(), ctxIdentity, "alice") + ctx = context.WithValue(ctx, ctxAccountID, "123456789012") + ctx = context.WithValue(ctx, ctxPrincipalType, principalTypeUser) + req = req.WithContext(ctx) + + resource := "arn:aws:iam::123456789012:role/private/target" + err := gw.checkPolicyResources(req, "iam", "GetRole", []string{resource}) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorAccessDenied, err.Error()) + + code, message, ok := awserrors.ResolveErrorDetail(err) + require.True(t, ok) + assert.Equal(t, awserrors.ErrorAccessDenied, code) + assert.Empty(t, message) + assert.NotContains(t, err.Error(), resource) +} + // TestCheckPolicy_PassRoleFailsClosed proves the change reaches a real gated -// call site, not just the helper: an unauthenticated RunInstances carrying an -// instance profile is denied rather than passing the iam:PassRole check. +// call site: an unauthenticated RunInstances carrying an instance profile is +// denied rather than passing the iam:PassRole check. func TestCheckPolicy_PassRoleFailsClosed(t *testing.T) { gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} req := httptest.NewRequest(http.MethodPost, "/", nil) @@ -1125,7 +1152,7 @@ func TestCheckPolicy_PassRoleFailsClosed(t *testing.T) { err := gw.checkPolicyResources(req, "iam", "PassRole", []string{"arn:aws:iam::123456789012:role/app"}) require.Error(t, err) - assert.Equal(t, awserrors.ErrorAccessDenied, err.Error()) + assertDenied(t, err) } func TestCheckPolicy_RootUserGlobalAccount(t *testing.T) { @@ -1154,7 +1181,7 @@ func TestCheckPolicy_AssumedRoleSessionNamedRoot(t *testing.T) { err := gw.checkPolicy(req, "ec2", "DescribeInstances") require.Error(t, err) - assert.Equal(t, awserrors.ErrorAccessDenied, err.Error()) + assertDenied(t, err) } func TestCheckPolicy_NonRootAllowPolicy(t *testing.T) { @@ -1203,7 +1230,7 @@ func TestCheckPolicy_NonRootDenyPolicy(t *testing.T) { err := gw.checkPolicy(req, "ec2", "DescribeInstances") require.Error(t, err) - assert.Equal(t, awserrors.ErrorAccessDenied, err.Error()) + assertDenied(t, err) } func TestCheckPolicy_NonRootNoPolicies(t *testing.T) { @@ -1221,7 +1248,7 @@ func TestCheckPolicy_NonRootNoPolicies(t *testing.T) { err := gw.checkPolicy(req, "ec2", "DescribeInstances") require.Error(t, err) - assert.Equal(t, awserrors.ErrorAccessDenied, err.Error()) + assertDenied(t, err) } func TestCheckPolicy_GetUserPoliciesError(t *testing.T) { diff --git a/spinifex/gateway/iam_authz_test.go b/spinifex/gateway/iam_authz_test.go index cdc03c541..4e7654cab 100644 --- a/spinifex/gateway/iam_authz_test.go +++ b/spinifex/gateway/iam_authz_test.go @@ -78,7 +78,7 @@ func TestIAMRequest_ScopedDenyFires(t *testing.T) { ) err := dispatchIAM(t, gw, "Action=DeleteRole&RoleName=prod") - require.EqualError(t, err, awserrors.ErrorAccessDenied) + assertDenied(t, err) require.NoError(t, dispatchIAM(t, gw, "Action=DeleteRole&RoleName=dev")) assert.Equal(t, []string{"dev"}, svc.deleted) } @@ -91,7 +91,7 @@ func TestIAMRequest_ScopedAllowGrants(t *testing.T) { require.NoError(t, dispatchIAM(t, gw, "Action=DeleteRole&RoleName=dev")) err := dispatchIAM(t, gw, "Action=DeleteRole&RoleName=prod") - require.EqualError(t, err, awserrors.ErrorAccessDenied) + assertDenied(t, err) assert.Equal(t, []string{"dev"}, svc.deleted) } @@ -107,7 +107,7 @@ func TestIAMRequest_MissingTargetAndLookupFailure(t *testing.T) { gw, _ := iamScopedGateway(nil, statement("Allow", "iam:DeleteRole", "arn:aws:iam::*:role/missing")) err := dispatchIAM(t, gw, "Action=DeleteRole&RoleName=missing") - require.EqualError(t, err, awserrors.ErrorAccessDenied) + assertDenied(t, err) }) t.Run("storage failure fails closed", func(t *testing.T) { diff --git a/spinifex/gateway/iam_request_test.go b/spinifex/gateway/iam_request_test.go index 171d363b3..5537dfa69 100644 --- a/spinifex/gateway/iam_request_test.go +++ b/spinifex/gateway/iam_request_test.go @@ -36,6 +36,17 @@ type flexMockIAMService struct { lookupAccessKeyFn func(string) (*handlers_iam.AccessKey, error) getAccountSummaryFn func(string, *iam.GetAccountSummaryInput) (*iam.GetAccountSummaryOutput, error) + getRoleFn func(string, *iam.GetRoleInput) (*iam.GetRoleOutput, error) +} + +// GetRole answers NoSuchEntity by default: the STS identity gate resolves the +// target role, and a mock that invented one would let a test pass against a +// role the account does not hold. +func (m *flexMockIAMService) GetRole(accountID string, input *iam.GetRoleInput) (*iam.GetRoleOutput, error) { + if m.getRoleFn != nil { + return m.getRoleFn(accountID, input) + } + return nil, errors.New(awserrors.ErrorIAMNoSuchEntity) } func (m *flexMockIAMService) CreateUser(accountID string, input *iam.CreateUserInput) (*iam.CreateUserOutput, error) { diff --git a/spinifex/gateway/policy/evaluator_test.go b/spinifex/gateway/policy/evaluator_test.go index aa54efb28..4c3148545 100644 --- a/spinifex/gateway/policy/evaluator_test.go +++ b/spinifex/gateway/policy/evaluator_test.go @@ -69,7 +69,7 @@ func TestEvaluate_IAMInstanceProfileActionStrings(t *testing.T) { } // TestEvaluate_STSActionStrings pins the action strings emitted by the STS -// gateway dispatcher (gateway/sts.go stsActions + checkPolicy(r, "sts", action) +// gateway dispatcher (gateway/sts.go stsActions + the stsPolicyGatedActions // call site). Locks in that every STS verb the dispatcher accepts is matchable // by the evaluator under both wildcard and service-scoped policies, so a future // rename of any stsActions key surfaces here. diff --git a/spinifex/gateway/rds_test.go b/spinifex/gateway/rds_test.go index fc96c225c..9c5364682 100644 --- a/spinifex/gateway/rds_test.go +++ b/spinifex/gateway/rds_test.go @@ -144,7 +144,7 @@ func TestRDSRequest_InternalActionsDeniedToCustomerAdmin(t *testing.T) { err := gw.RDS_Request(httptest.NewRecorder(), setupRDSUserRequest("Action="+action, rdsTestAccountID)) require.Error(t, err) - assert.Equal(t, awserrors.ErrorAccessDenied, err.Error()) + assertDenied(t, err) }) } } @@ -169,7 +169,7 @@ func TestRDSRequest_AgentRoleCannotCallCustomerActions(t *testing.T) { err := gw.RDS_Request(httptest.NewRecorder(), setupRDSAgentRequest("Action=DescribeDBInstances", "i-0abc123")) require.Error(t, err) - assert.Equal(t, awserrors.ErrorAccessDenied, err.Error()) + assertDenied(t, err) } // A policy scoped to db:prod-* has to actually restrict: checked against "*" it @@ -189,7 +189,7 @@ func TestRDSRequest_ResourceScopedPolicyRestrictsByIdentifier(t *testing.T) { err = gw.RDS_Request(httptest.NewRecorder(), setupRDSUserRequest("Action=DeleteDBInstance&DBInstanceIdentifier=dev-orders", rdsTestAccountID)) require.Error(t, err) - assert.Equal(t, awserrors.ErrorAccessDenied, err.Error()) + assertDenied(t, err) } func TestRDSRequest_SnapshotActionsAuthorizeSourceAndTarget(t *testing.T) { @@ -254,7 +254,7 @@ func TestRDSRequest_SnapshotActionsUseOnePolicySnapshot(t *testing.T) { rdsTestAccountID, )) require.Error(t, err) - assert.Equal(t, awserrors.ErrorAccessDenied, err.Error()) + assertDenied(t, err) assert.Equal(t, 1, probe.consulted) } @@ -291,7 +291,7 @@ func TestRDSRequest_SnapshotActionsHonorTargetDeny(t *testing.T) { err := gw.RDS_Request(httptest.NewRecorder(), setupRDSUserRequest(tt.query, rdsTestAccountID)) require.Error(t, err) assert.Equal(t, 1, probe.consulted) - assert.Equal(t, awserrors.ErrorAccessDenied, err.Error()) + assertDenied(t, err) }) } } @@ -303,7 +303,7 @@ func TestRDSRequest_UnscopedActionsEvaluateAgainstAnyResource(t *testing.T) { err := gw.RDS_Request(httptest.NewRecorder(), setupRDSUserRequest("Action=CreateDBInstance&DBInstanceIdentifier=prod-orders", rdsTestAccountID)) require.Error(t, err) - assert.Equal(t, awserrors.ErrorAccessDenied, err.Error()) + assertDenied(t, err) } // Cross-account references are refused at the ARN, not merely unauthorized: @@ -331,7 +331,7 @@ func TestRDSRequest_NoIdentityDenied(t *testing.T) { err := gw.RDS_Request(httptest.NewRecorder(), req.WithContext(ctx)) require.Error(t, err) - assert.Equal(t, awserrors.ErrorAccessDenied, err.Error()) + assertDenied(t, err) assert.Zero(t, probe.consulted, "a request with no identity must not be evaluated at all") } diff --git a/spinifex/gateway/sts.go b/spinifex/gateway/sts.go index fccaf0db4..3efaacc36 100644 --- a/spinifex/gateway/sts.go +++ b/spinifex/gateway/sts.go @@ -5,11 +5,13 @@ import ( "log/slog" "net/http" + "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/sts" spxarn "github.com/mulgadc/spinifex/spinifex/arn" "github.com/mulgadc/spinifex/spinifex/awsec2query" "github.com/mulgadc/spinifex/spinifex/awserrors" gateway_sts "github.com/mulgadc/spinifex/spinifex/gateway/sts" + handlers_sts "github.com/mulgadc/spinifex/spinifex/handlers/sts" "github.com/mulgadc/spinifex/spinifex/utils" ) @@ -76,6 +78,47 @@ var anonymousSTSActions = map[string]bool{ "AssumeRoleWithWebIdentity": true, } +// stsPolicyGatedActions lists the actions requiring a pass on the caller's +// identity policy. GetCallerIdentity and GetSessionToken are authentication +// operations AWS requires no permission for, and AssumeRoleWithWebIdentity +// arrives with no identity to evaluate. +var stsPolicyGatedActions = map[string]bool{ + "AssumeRole": true, +} + +// stsPolicyResource returns the ARN a gated action is evaluated against. +// AssumeRole resolves the target role so the resource is the ARN IAM stored, +// which a policy naming a pathed role matches and an invented path does not. +func (gw *GatewayConfig) stsPolicyResource(queryArgs map[string]string) (string, error) { + // An absent RoleArn is a malformed request, not an unauthorized one: saying + // so beats sending the caller to widen a policy that was never the cause. + roleARN := queryArgs["RoleArn"] + if roleARN == "" { + return "", errors.New(awserrors.ErrorMissingParameter) + } + if gw.IAMService == nil { + slog.Error("STS: IAM service not available", "action", "AssumeRole") + return "", errors.New(awserrors.ErrorInternalError) + } + + _, role, err := handlers_sts.ResolveRoleByARN(gw.IAMService, roleARN) + switch { + case err == nil: + return aws.StringValue(role.Arn), nil + case errors.Is(err, handlers_sts.ErrRoleUnresolved): + // Echoing the supplied ARN keeps the denial identical whether or not the + // role exists. The handler refuses an ARN that is not the stored one, so + // a grant matched here on an invented path still ends in a denial there. + return roleARN, nil + } + if code, ok := awserrors.ResolveErrorCode(err); ok && code == awserrors.ErrorValidationError { + slog.Debug("STS: unparseable RoleArn", "roleArn", roleARN) + return "", err + } + slog.Error("STS: role lookup failed ahead of the identity gate", "roleArn", roleARN, "err", err) + return "", errors.New(awserrors.ErrorInternalError) +} + func (gw *GatewayConfig) STS_Request(w http.ResponseWriter, r *http.Request) error { queryArgs, err := readQueryArgs(r) if err != nil { @@ -98,10 +141,6 @@ func (gw *GatewayConfig) STS_Request(w http.ResponseWriter, r *http.Request) err return errors.New(awserrors.ErrorInternalError) } - // STS actions are not gated by caller IAM policy: each action enforces its - // own rule in the handler (role trust policy, web-identity JWT, or the - // always-allowed GetCallerIdentity), so no checkPolicy pass runs here. - // Anonymous actions carry no SigV4 envelope; handler ignores the zero caller. var caller stsCaller if !anonymousSTSActions[action] { @@ -111,6 +150,22 @@ func (gw *GatewayConfig) STS_Request(w http.ResponseWriter, r *http.Request) err } } + // The identity policy is the first of two gates; the handler still applies + // the role's trust policy. Scoping to the target role lets a policy grant + // assumption of one role without granting all of them. + if stsPolicyGatedActions[action] { + resource, rerr := gw.stsPolicyResource(queryArgs) + if rerr != nil { + return rerr + } + if err := gw.checkPolicyResources(r, "sts", action, []string{resource}); err != nil { + if denial, ok := errors.AsType[*identityPolicyDenialError](err); ok { + return denial.detailedError() + } + return err + } + } + xmlOutput, err := handler(action, queryArgs, gw, caller) if err != nil { return err diff --git a/spinifex/gateway/sts/AssumeRole.go b/spinifex/gateway/sts/AssumeRole.go index d474a74f5..a7d925ab8 100644 --- a/spinifex/gateway/sts/AssumeRole.go +++ b/spinifex/gateway/sts/AssumeRole.go @@ -10,8 +10,9 @@ import ( ) // AssumeRole validates the request and delegates to STSService. Authorization -// is gated solely by the role's trust policy (no checkPolicy call), matching -// AWS behaviour. callerARN is resolved by the dispatcher from the SigV4 principal. +// takes two gates: the dispatcher checks the caller's identity policy against +// the target role ARN, and STSService checks the role's trust policy. +// callerARN is resolved by the dispatcher from the SigV4 principal. func AssumeRole(callerAccountID, callerARN, callerIdentity string, input *sts.AssumeRoleInput, svc handlers_sts.STSService) (*sts.AssumeRoleOutput, error) { if input == nil { return nil, errors.New(awserrors.ErrorMissingParameter) diff --git a/spinifex/gateway/sts/GetSessionToken.go b/spinifex/gateway/sts/GetSessionToken.go index 6770aa9e9..51b979e49 100644 --- a/spinifex/gateway/sts/GetSessionToken.go +++ b/spinifex/gateway/sts/GetSessionToken.go @@ -5,9 +5,9 @@ import ( handlers_sts "github.com/mulgadc/spinifex/spinifex/handlers/sts" ) -// GetSessionToken delegates to the STSService; not gated by caller IAM policy -// (STS_Request runs no checkPolicy pass). callerUserName and callerAccessKeyID -// come from the SigV4 context. +// GetSessionToken delegates to the STSService; not gated by caller IAM policy, +// as AWS requires no permission for authentication operations. callerUserName +// and callerAccessKeyID come from the SigV4 context. func GetSessionToken(callerAccountID, callerUserName, callerPrincipalType, callerAccessKeyID string, input *sts.GetSessionTokenInput, svc handlers_sts.STSService) (*sts.GetSessionTokenOutput, error) { return svc.GetSessionToken(callerAccountID, callerUserName, callerPrincipalType, callerAccessKeyID, input) } diff --git a/spinifex/gateway/sts_policy_test.go b/spinifex/gateway/sts_policy_test.go new file mode 100644 index 000000000..84c804500 --- /dev/null +++ b/spinifex/gateway/sts_policy_test.go @@ -0,0 +1,323 @@ +// (stsRequestParams, principalTypeUser, the context keys) and shares the +// policy mocks with the other in-package authz suites. +// +//test:in-package — the gate is exercised through unexported request wiring +package gateway + +import ( + "errors" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/iam" + "github.com/aws/aws-sdk-go/service/sts" + "github.com/mulgadc/spinifex/spinifex/awserrors" + handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam" + "github.com/mulgadc/spinifex/spinifex/utils" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + stsTestRoleName = "app" + stsTestRoleARN = "arn:aws:iam::000000000000:role/app" +) + +// stsPolicyIAMService serves the identity policies the STS gate evaluates, the +// GetUser lookup GetCallerIdentity performs, and the GetRole the gate resolves +// the target role with. +type stsPolicyIAMService struct { + policyMockIAMService + + roles map[string]string +} + +func (m *stsPolicyIAMService) GetUser(_ string, _ *iam.GetUserInput) (*iam.GetUserOutput, error) { + return &iam.GetUserOutput{User: &iam.User{UserId: aws.String("AIDAALICE000")}}, nil +} + +func (m *stsPolicyIAMService) GetRole(_ string, input *iam.GetRoleInput) (*iam.GetRoleOutput, error) { + arn, ok := m.roles[aws.StringValue(input.RoleName)] + if !ok { + return nil, errors.New(awserrors.ErrorIAMNoSuchEntity) + } + return &iam.GetRoleOutput{Role: &iam.Role{ + RoleName: input.RoleName, + Arn: aws.String(arn), + }}, nil +} + +// stsIdentityPolicy builds an IAM service returning the given statements as the +// caller's only identity policy, for a user and for an assumed role alike. The +// one stored role is the pathless `app`; withRoles replaces the set. +func stsIdentityPolicy(statements ...handlers_iam.Statement) *stsPolicyIAMService { + docs := []handlers_iam.PolicyDocument{{Version: "2012-10-17", Statement: statements}} + svc := &stsPolicyIAMService{roles: map[string]string{stsTestRoleName: stsTestRoleARN}} + svc.getUserPoliciesFn = func(_, _ string) ([]handlers_iam.PolicyDocument, error) { + return docs, nil + } + svc.getRolePoliciesFn = func(_, _ string) ([]handlers_iam.PolicyDocument, error) { + return docs, nil + } + return svc +} + +// withRoles replaces the roles GetRole answers for, keyed by name and mapped to +// the ARN IAM stored — which is what a resource-scoped grant is matched against. +func (m *stsPolicyIAMService) withRoles(roles map[string]string) *stsPolicyIAMService { + m.roles = roles + return m +} + +// stsStatement is shorthand for a single-action, single-resource statement. +func stsStatement(effect, action, resource string) handlers_iam.Statement { + return handlers_iam.Statement{ + Effect: effect, + Action: handlers_iam.StringOrArr{action}, + Resource: handlers_iam.StringOrArr{resource}, + } +} + +// deniedSTSService fails the test if any gated handler is reached, so a passing +// denial can only have come from the gate. +func deniedSTSService(t *testing.T) *flexMockSTSService { + t.Helper() + return &flexMockSTSService{ + assumeRoleFn: func(string, string, string, *sts.AssumeRoleInput) (*sts.AssumeRoleOutput, error) { + t.Fatal("handler reached: the identity-policy gate should have denied this request") + return nil, nil + }, + } +} + +// stsPolicyRequest dispatches an STS action as alice with the given identity +// policies. +func stsPolicyRequest(t *testing.T, iamSvc handlers_iam.IAMService, stsSvc *flexMockSTSService, body string) *http.Response { + t.Helper() + if stsSvc == nil { + stsSvc = deniedSTSService(t) + } + return stsPolicyRequestAs(t, stsRequestParams{ + accountID: utils.GlobalAccountID, + identity: "alice", + principalType: principalTypeUser, + accessKey: "AKIAEXAMPLE", + stsSvc: stsSvc, + iamSvc: iamSvc, + }, body) +} + +func stsPolicyRequestAs(t *testing.T, p stsRequestParams, body string) *http.Response { + t.Helper() + req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(body)) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + return doRequest(setupSTSRequestHandler(p), req) +} + +func assertSTSAccessDenied(t *testing.T, resp *http.Response) string { + t.Helper() + require.Equal(t, http.StatusForbidden, resp.StatusCode) + b, _ := io.ReadAll(resp.Body) + assert.Contains(t, string(b), "AccessDenied") + return string(b) +} + +// TestSTSRequest_AssumeRole_IdentityPolicyGate covers the identity-side gate. +// The STS mock would happily mint credentials, so a denial can only come from +// the policy check that now runs ahead of the role's trust policy. +func TestSTSRequest_AssumeRole_IdentityPolicyGate(t *testing.T) { + body := "Action=AssumeRole&RoleArn=" + stsTestRoleARN + "&RoleSessionName=s1" + + t.Run("empty policy denied", func(t *testing.T) { + assertSTSAccessDenied(t, stsPolicyRequest(t, stsIdentityPolicy(), nil, body)) + }) + + t.Run("explicit deny beats allow", func(t *testing.T) { + svc := stsIdentityPolicy( + stsStatement("Allow", "sts:*", "*"), + stsStatement("Deny", "sts:AssumeRole", "*"), + ) + assertSTSAccessDenied(t, stsPolicyRequest(t, svc, nil, body)) + }) + + t.Run("allowed principal reaches handler", func(t *testing.T) { + called := false + stsSvc := &flexMockSTSService{ + assumeRoleFn: func(string, string, string, *sts.AssumeRoleInput) (*sts.AssumeRoleOutput, error) { + called = true + return &sts.AssumeRoleOutput{ + Credentials: &sts.Credentials{AccessKeyId: aws.String("ASIAEXAMPLE123")}, + }, nil + }, + } + svc := stsIdentityPolicy(stsStatement("Allow", "sts:AssumeRole", "*")) + resp := stsPolicyRequest(t, svc, stsSvc, body) + require.Equal(t, http.StatusOK, resp.StatusCode) + assert.True(t, called) + }) +} + +// TestSTSRequest_AssumeRole_AssumedRoleCallerIsGated is the case that would +// have caught the ECS agent regression: a role session whose underlying role +// grants a non-sts action and nothing else must not be able to assume anything. +func TestSTSRequest_AssumeRole_AssumedRoleCallerIsGated(t *testing.T) { + body := "Action=AssumeRole&RoleArn=" + stsTestRoleARN + "&RoleSessionName=s1" + params := func(iamSvc handlers_iam.IAMService, stsSvc *flexMockSTSService) stsRequestParams { + return stsRequestParams{ + accountID: utils.GlobalAccountID, + identity: "s1", + principalType: principalTypeAssumedRole, + accessKey: "ASIAEXAMPLE", + assumedRoleARN: "arn:aws:sts::000000000000:assumed-role/ecsInstanceRole/s1", + assumedRoleID: "AROAECS:s1", + underlyingRoleARN: "arn:aws:iam::000000000000:role/ecsInstanceRole", + stsSvc: stsSvc, + iamSvc: iamSvc, + } + } + + t.Run("ecs-only role denied", func(t *testing.T) { + svc := stsIdentityPolicy(stsStatement("Allow", "ecs:*", "*")) + resp := stsPolicyRequestAs(t, params(svc, deniedSTSService(t)), body) + assertSTSAccessDenied(t, resp) + }) + + t.Run("role granted sts:AssumeRole reaches handler", func(t *testing.T) { + svc := stsIdentityPolicy( + stsStatement("Allow", "ecs:*", "*"), + stsStatement("Allow", "sts:AssumeRole", "arn:aws:iam::000000000000:role/*"), + ) + resp := stsPolicyRequestAs(t, params(svc, &flexMockSTSService{}), body) + assert.Equal(t, http.StatusOK, resp.StatusCode) + }) +} + +// TestSTSRequest_AssumeRole_PolicyScopedToRole proves the gate evaluates the +// target role ARN, not "*", so a grant on one role does not open another. +func TestSTSRequest_AssumeRole_PolicyScopedToRole(t *testing.T) { + roles := map[string]string{ + stsTestRoleName: stsTestRoleARN, + "other": "arn:aws:iam::000000000000:role/other", + } + + t.Run("granted role allowed", func(t *testing.T) { + svc := stsIdentityPolicy(stsStatement("Allow", "sts:AssumeRole", stsTestRoleARN)).withRoles(roles) + resp := stsPolicyRequest(t, svc, &flexMockSTSService{}, + "Action=AssumeRole&RoleArn="+stsTestRoleARN+"&RoleSessionName=s1") + assert.Equal(t, http.StatusOK, resp.StatusCode) + }) + + t.Run("other role denied", func(t *testing.T) { + svc := stsIdentityPolicy(stsStatement("Allow", "sts:AssumeRole", stsTestRoleARN)).withRoles(roles) + resp := stsPolicyRequest(t, svc, nil, + "Action=AssumeRole&RoleArn=arn:aws:iam::000000000000:role/other&RoleSessionName=s1") + assertSTSAccessDenied(t, resp) + }) +} + +// TestSTSRequest_AssumeRole_PathIsResolvedNotTrusted pins the gate to the ARN +// IAM stored, so a role genuinely stored under a path is matched by its full +// ARN. An ARN that is not the stored one is refused by role resolution itself +// — see TestResolveRoleByARN in handlers/sts. +func TestSTSRequest_AssumeRole_PathIsResolvedNotTrusted(t *testing.T) { + t.Run("stored ARN matching exactly is admitted", func(t *testing.T) { + svc := stsIdentityPolicy( + stsStatement("Allow", "sts:AssumeRole", "arn:aws:iam::000000000000:role/app-*"), + ).withRoles(map[string]string{"app-worker": "arn:aws:iam::000000000000:role/app-worker"}) + resp := stsPolicyRequest(t, svc, &flexMockSTSService{}, + "Action=AssumeRole&RoleArn=arn:aws:iam::000000000000:role/app-worker&RoleSessionName=s1") + assert.Equal(t, http.StatusOK, resp.StatusCode) + }) + + t.Run("a pathed role matches a policy naming its full ARN", func(t *testing.T) { + const pathed = "arn:aws:iam::000000000000:role/team/app-worker" + svc := stsIdentityPolicy(stsStatement("Allow", "sts:AssumeRole", pathed)). + withRoles(map[string]string{"app-worker": pathed}) + resp := stsPolicyRequest(t, svc, &flexMockSTSService{}, + "Action=AssumeRole&RoleArn="+pathed+"&RoleSessionName=s1") + assert.Equal(t, http.StatusOK, resp.StatusCode) + }) + + t.Run("malformed ARN rejected before evaluation", func(t *testing.T) { + resp := stsPolicyRequest(t, stsIdentityPolicy(), nil, + "Action=AssumeRole&RoleArn=not-an-arn&RoleSessionName=s1") + require.Equal(t, http.StatusBadRequest, resp.StatusCode) + b, _ := io.ReadAll(resp.Body) + assert.Contains(t, string(b), "ValidationError") + }) +} + +// TestSTSRequest_AssumeRole_DenialDoesNotDiscloseRoleExistence keeps the +// detailed denial message safe to return: it must read identically whether or +// not the role exists, which a lookup added ahead of the gate would break. +func TestSTSRequest_AssumeRole_DenialDoesNotDiscloseRoleExistence(t *testing.T) { + body := "Action=AssumeRole&RoleArn=" + stsTestRoleARN + "&RoleSessionName=s1" + + existing := assertSTSAccessDenied(t, stsPolicyRequest(t, stsIdentityPolicy(), nil, body)) + missing := assertSTSAccessDenied(t, + stsPolicyRequest(t, stsIdentityPolicy().withRoles(nil), nil, body)) + + assert.Equal(t, stripRequestID(existing), stripRequestID(missing)) + assert.Contains(t, existing, "is not authorized to perform: sts:AssumeRole on resource: "+stsTestRoleARN) + + // A pathed role is where the two can diverge: a resolved role names its + // stored ARN, so an unresolved one must name what the caller supplied + // rather than any form derived from it. + const pathed = "arn:aws:iam::000000000000:role/team/svc" + pathedBody := "Action=AssumeRole&RoleArn=" + pathed + "&RoleSessionName=s1" + + pathedExisting := assertSTSAccessDenied(t, stsPolicyRequest(t, + stsIdentityPolicy().withRoles(map[string]string{"svc": pathed}), nil, pathedBody)) + pathedMissing := assertSTSAccessDenied(t, + stsPolicyRequest(t, stsIdentityPolicy().withRoles(nil), nil, pathedBody)) + + assert.Equal(t, stripRequestID(pathedExisting), stripRequestID(pathedMissing)) + assert.Contains(t, pathedExisting, "on resource: "+pathed) +} + +// stripRequestID removes the per-request UUID so two error bodies can be +// compared for everything else. +func stripRequestID(body string) string { + start := strings.Index(body, "") + end := strings.Index(body, "") + if start < 0 || end < start { + return body + } + return body[:start] + body[end:] +} + +// TestSTSRequest_GetSessionToken_NotGated pins the AWS contract: it is an +// authentication operation requiring no permission, so an explicit Deny on the +// identity policy does not stop it reaching the handler. +func TestSTSRequest_GetSessionToken_NotGated(t *testing.T) { + svc := stsIdentityPolicy(stsStatement("Deny", "sts:*", "*")) + + called := false + stsSvc := &flexMockSTSService{ + getSessionTokenFn: func(string, string, string, string, *sts.GetSessionTokenInput) (*sts.GetSessionTokenOutput, error) { + called = true + return &sts.GetSessionTokenOutput{ + Credentials: &sts.Credentials{AccessKeyId: aws.String("ASIAEXAMPLE123")}, + }, nil + }, + } + + resp := stsPolicyRequest(t, svc, stsSvc, "Action=GetSessionToken&DurationSeconds=3600") + require.Equal(t, http.StatusOK, resp.StatusCode) + assert.True(t, called) +} + +// TestSTSRequest_GetCallerIdentity_NotGated pins the AWS contract: the action +// requires no permission and cannot be denied by policy. +func TestSTSRequest_GetCallerIdentity_NotGated(t *testing.T) { + svc := stsIdentityPolicy(stsStatement("Deny", "sts:*", "*")) + resp := stsPolicyRequest(t, svc, &flexMockSTSService{}, "Action=GetCallerIdentity") + require.Equal(t, http.StatusOK, resp.StatusCode) + + b, _ := io.ReadAll(resp.Body) + assert.Contains(t, string(b), "GetCallerIdentityResult") +} diff --git a/spinifex/gateway/sts_request_test.go b/spinifex/gateway/sts_request_test.go index 01bf88993..d2c440509 100644 --- a/spinifex/gateway/sts_request_test.go +++ b/spinifex/gateway/sts_request_test.go @@ -83,8 +83,11 @@ type stsRequestParams struct { accessKey string assumedRoleARN string assumedRoleID string - stsSvc handlers_sts.STSService - iamSvc handlers_iam.IAMService + // underlyingRoleARN is what the policy gate resolves an assumed-role + // caller's policies by; SessionName never participates. + underlyingRoleARN string + stsSvc handlers_sts.STSService + iamSvc handlers_iam.IAMService } func setupSTSRequestHandler(p stsRequestParams) http.Handler { @@ -109,6 +112,9 @@ func setupSTSRequestHandler(p stsRequestParams) http.Handler { if p.assumedRoleID != "" { ctx = context.WithValue(ctx, ctxAssumedRoleID, p.assumedRoleID) } + if p.underlyingRoleARN != "" { + ctx = context.WithValue(ctx, ctxUnderlyingRoleARN, p.underlyingRoleARN) + } r = r.WithContext(ctx) if err := gw.STS_Request(w, r); err != nil { gw.ErrorHandler(w, r, err) @@ -140,6 +146,7 @@ func TestSTSRequest_AssumeRole_Success(t *testing.T) { principalType: principalTypeUser, accessKey: "AKIAEXAMPLE", stsSvc: svc, + iamSvc: stsIdentityPolicy(stsStatement("Allow", "sts:AssumeRole", "*")), }) body := "Action=AssumeRole&RoleArn=arn:aws:iam::000000000000:role/app&RoleSessionName=s1" @@ -310,6 +317,9 @@ func TestSTSRequest_AssumeRole_MissingRoleArn(t *testing.T) { principalType: principalTypeUser, accessKey: "AKIAEXAMPLE", stsSvc: svc, + // An empty policy: a request missing RoleArn is malformed whatever the + // caller holds, so the gate must not answer it with a denial. + iamSvc: stsIdentityPolicy(), }) req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader("Action=AssumeRole&RoleSessionName=s1")) @@ -345,6 +355,39 @@ func TestSTSRequest_NilService_InternalError(t *testing.T) { assert.Contains(t, string(b), "InternalError") } +// TestSTSRequest_AssumeRole_NilIAMService_InternalError pins the gate to the +// same fail-closed answer checkPolicyResources gives: the target-role lookup +// runs ahead of it, so an absent IAM service must not reach a nil interface. +func TestSTSRequest_AssumeRole_NilIAMService_InternalError(t *testing.T) { + svc := &flexMockSTSService{ + assumeRoleFn: func(string, string, string, *sts.AssumeRoleInput) (*sts.AssumeRoleOutput, error) { + t.Fatal("handler reached: an unavailable IAM service must fail closed") + return nil, nil + }, + } + gw := &GatewayConfig{DisableLogging: true, STSService: svc, IAMService: nil} + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + ctx := context.WithValue(r.Context(), ctxService, "sts") + ctx = context.WithValue(ctx, ctxAccountID, utils.GlobalAccountID) + ctx = context.WithValue(ctx, ctxIdentity, "alice") + ctx = context.WithValue(ctx, ctxPrincipalType, principalTypeUser) + ctx = context.WithValue(ctx, ctxAccessKey, "AKIAEXAMPLE") + r = r.WithContext(ctx) + if err := gw.STS_Request(w, r); err != nil { + gw.ErrorHandler(w, r, err) + } + }) + + req := httptest.NewRequest(http.MethodPost, "/", + strings.NewReader("Action=AssumeRole&RoleArn="+stsTestRoleARN+"&RoleSessionName=s1")) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + + resp := doRequest(handler, req) + assert.Equal(t, 500, resp.StatusCode) + b, _ := io.ReadAll(resp.Body) + assert.Contains(t, string(b), "InternalError") +} + func TestSTSRequest_AssumeRole_ServiceError_PropagatesAccessDenied(t *testing.T) { // AccessDenied from the handler (trust-policy denial) must reach the wire as IAM ErrorResponse + 403. svc := &flexMockSTSService{ @@ -358,6 +401,7 @@ func TestSTSRequest_AssumeRole_ServiceError_PropagatesAccessDenied(t *testing.T) principalType: principalTypeUser, accessKey: "AKIAEXAMPLE", stsSvc: svc, + iamSvc: stsIdentityPolicy(stsStatement("Allow", "sts:AssumeRole", "*")), }) body := "Action=AssumeRole&RoleArn=arn:aws:iam::000000000000:role/app&RoleSessionName=s1" diff --git a/spinifex/handlers/ecs/capacity_test.go b/spinifex/handlers/ecs/capacity_test.go index ac300225e..52dbe60a9 100644 --- a/spinifex/handlers/ecs/capacity_test.go +++ b/spinifex/handlers/ecs/capacity_test.go @@ -7,7 +7,6 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" - "github.com/aws/aws-sdk-go/service/iam" "github.com/mulgadc/spinifex/spinifex/awserrors" iammock "github.com/mulgadc/spinifex/spinifex/handlers/iam/mock" "github.com/mulgadc/spinifex/spinifex/tags" @@ -66,9 +65,8 @@ func imageMatchesFilters(img *ec2.Image, filters []*ec2.Filter) bool { return true } -// stubIAM embeds the shared SystemInstanceRoleEnsurer mock for the Get*/Create* -// find-or-create behaviour, and adds CreatePolicy/AttachRolePolicy locally -// since ecsIAM needs both beyond what the ensurer interface covers. +// stubIAM is the shared SystemInstanceRoleEnsurer mock, which covers the whole +// ecsIAM surface. type stubIAM struct { *iammock.SystemInstanceRoleEnsurer } @@ -83,15 +81,6 @@ var ( _ ecsImageResolver = (*filteringStubImages)(nil) ) -func (stubIAM) CreatePolicy(accountID string, _ *iam.CreatePolicyInput) (*iam.CreatePolicyOutput, error) { - arn := "arn:aws:iam::" + accountID + ":policy/" + ecsInstanceRolePolicyName - return &iam.CreatePolicyOutput{Policy: &iam.Policy{Arn: aws.String(arn)}}, nil -} - -func (stubIAM) AttachRolePolicy(_ string, _ *iam.AttachRolePolicyInput) (*iam.AttachRolePolicyOutput, error) { - return &iam.AttachRolePolicyOutput{}, nil -} - func ecsNodeImage() []*ec2.Image { return []*ec2.Image{{ ImageId: aws.String("ami-ecs"), diff --git a/spinifex/handlers/ecs/instancerole.go b/spinifex/handlers/ecs/instancerole.go index 6bc07cd9e..734fa1cfa 100644 --- a/spinifex/handlers/ecs/instancerole.go +++ b/spinifex/handlers/ecs/instancerole.go @@ -2,12 +2,8 @@ package handlers_ecs import ( "fmt" - "time" - "github.com/aws/aws-sdk-go/aws" - "github.com/aws/aws-sdk-go/service/iam" - "github.com/mulgadc/spinifex/spinifex/arn" - "github.com/mulgadc/spinifex/spinifex/awserrors" + handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam" ) const ( @@ -15,153 +11,38 @@ const ( // container instances. The agent draws its creds from this role over IMDS. ecsInstanceRoleName = "ecsInstanceRole" - // ecsInstanceRolePolicyName is the customer-managed policy granting ecs:* so - // the gateway's assumed-role ECS authz (checkPolicy) admits the agent. + // ecsInstanceRolePolicyName is the inline policy granting the agent the + // actions the gateway enforces for its assumed-role principal. ecsInstanceRolePolicyName = "ecsInstanceRolePolicy" - - // ecsAssumeRolePolicy is the EC2 trust the STS AssumeRoleForInstance path - // whitelists; it must match exactly for IMDS role-cred vending to admit it. - ecsAssumeRolePolicy = `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Principal":{"Service":"ec2.amazonaws.com"},"Action":"sts:AssumeRole"}]}` - - // ecsInstanceRolePolicyDoc grants the agent the ECS control-plane actions the - // gateway enforces for assumed-role principals, plus ECR read so it can pull - // task images via the instance role when a task carries no execution role. - ecsInstanceRolePolicyDoc = `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":"ecs:*","Resource":"*"},{"Effect":"Allow","Action":["ecr:GetAuthorizationToken","ecr:BatchCheckLayerAvailability","ecr:GetDownloadUrlForLayer","ecr:BatchGetImage"],"Resource":"*"}]}` - - // ecsAttachRetries bounds the wait for a just-created policy to become - // visible to the attach, and ecsAttachRetryBase scales the linear backoff. - ecsAttachRetries = 5 - ecsAttachRetryBase = 100 * time.Millisecond ) -// ecsAttachRetrySleep is the test seam for the attach backoff. -var ecsAttachRetrySleep = time.Sleep +// ecsInstanceRolePolicyDoc grants the agent the ECS control-plane actions the +// gateway enforces for assumed-role principals, ECR read so it can pull task +// images when a task carries no execution role, and sts:AssumeRole so it can +// mint task and execution role credentials. Assumption stays bounded by each +// target role's trust policy. +func ecsInstanceRolePolicyDoc(accountID string) string { + return fmt.Sprintf(`{"Version":"2012-10-17","Statement":[`+ + `{"Effect":"Allow","Action":"ecs:*","Resource":"*"},`+ + `{"Effect":"Allow","Action":["ecr:GetAuthorizationToken","ecr:BatchCheckLayerAvailability",`+ + `"ecr:GetDownloadUrlForLayer","ecr:BatchGetImage"],"Resource":"*"},`+ + `{"Effect":"Allow","Action":"sts:AssumeRole","Resource":"arn:aws:iam::%s:role/*"}]}`, accountID) +} -// ensureECSInstanceProfile find-or-creates the ecsInstanceRole, its ecs:* policy -// attachment, and the matching instance profile, returning the profile ARN. -// Every step is idempotent so concurrent ProvisionCapacity calls converge. +// ensureECSInstanceProfile find-or-creates the ecsInstanceRole, re-asserts its +// inline policy, and binds the matching instance profile, returning the profile +// ARN. Every step is idempotent so concurrent callers converge, and the policy +// is written rather than created so an existing role picks up a changed +// document instead of keeping the one it was provisioned with. func (s *Service) ensureECSInstanceProfile(accountID string) (string, error) { - if err := s.ensureECSRole(accountID); err != nil { - return "", err - } - if err := s.ensureECSRolePolicy(accountID); err != nil { - return "", err - } - return s.ensureECSInstanceProfileBinding(accountID) + return handlers_iam.EnsureSystemInstanceProfile(s.deps.IAM, accountID, + ecsInstanceRoleName, ecsInstanceRolePolicyName, ecsInstanceRolePolicyDoc(accountID)) } -// ensureECSRole find-or-creates ecsInstanceRole with the EC2 trust policy, -// converging on a racing creator via re-GetRole. -func (s *Service) ensureECSRole(accountID string) error { - _, err := s.deps.IAM.GetRole(accountID, &iam.GetRoleInput{RoleName: aws.String(ecsInstanceRoleName)}) - if err == nil { - return nil - } - if err.Error() != awserrors.ErrorIAMNoSuchEntity { - return fmt.Errorf("get role %q: %w", ecsInstanceRoleName, err) - } - - _, err = s.deps.IAM.CreateRole(accountID, &iam.CreateRoleInput{ - RoleName: aws.String(ecsInstanceRoleName), - AssumeRolePolicyDocument: aws.String(ecsAssumeRolePolicy), - }) - if err == nil { - return nil - } - if err.Error() != awserrors.ErrorIAMEntityAlreadyExists { - return fmt.Errorf("create role %q: %w", ecsInstanceRoleName, err) - } - // A racing call created it first; re-read to converge. - if _, gerr := s.deps.IAM.GetRole(accountID, &iam.GetRoleInput{RoleName: aws.String(ecsInstanceRoleName)}); gerr != nil { - return fmt.Errorf("re-get role %q: %w", ecsInstanceRoleName, gerr) - } - return nil -} - -// ensureECSRolePolicy creates the ecs:* policy (resolving an existing one on a -// race) and attaches it to the role; an already-attached policy is success. -func (s *Service) ensureECSRolePolicy(accountID string) error { - policyARN := arn.FormatIAMPath(arn.IAMPolicy, accountID, "/", ecsInstanceRolePolicyName) - - out, err := s.deps.IAM.CreatePolicy(accountID, &iam.CreatePolicyInput{ - PolicyName: aws.String(ecsInstanceRolePolicyName), - PolicyDocument: aws.String(ecsInstanceRolePolicyDoc), - }) - switch { - case err == nil: - if out != nil && out.Policy != nil { - policyARN = aws.StringValue(out.Policy.Arn) - } - case err.Error() == awserrors.ErrorIAMEntityAlreadyExists: - // Policy already exists; the account ARN form is deterministic. - default: - return fmt.Errorf("create policy %q: %w", ecsInstanceRolePolicyName, err) - } - - // The attach checks the policy exists, and the policy buckets are - // replicated, so a read that lands on a replica which has not caught up - // answers NoSuchEntity for the write a microsecond above. Retry that one - // code rather than fail a provision over a write known to have landed. - for attempt := 0; ; attempt++ { - _, err = s.deps.IAM.AttachRolePolicy(accountID, &iam.AttachRolePolicyInput{ - RoleName: aws.String(ecsInstanceRoleName), - PolicyArn: aws.String(policyARN), - }) - if err == nil || err.Error() == awserrors.ErrorIAMEntityAlreadyExists { - return nil - } - if err.Error() != awserrors.ErrorIAMNoSuchEntity || attempt == ecsAttachRetries { - return fmt.Errorf("attach policy %q to role %q: %w", policyARN, ecsInstanceRoleName, err) - } - ecsAttachRetrySleep(time.Duration(attempt+1) * ecsAttachRetryBase) - } -} - -// ensureECSInstanceProfileBinding guarantees the ecsInstanceRole instance -// profile exists and carries the role, returning its ARN. Mirrors the EKS -// node-profile ensure path; idempotent under concurrent launches. -func (s *Service) ensureECSInstanceProfileBinding(accountID string) (string, error) { - out, err := s.deps.IAM.GetInstanceProfile(accountID, &iam.GetInstanceProfileInput{ - InstanceProfileName: aws.String(ecsInstanceRoleName), - }) - if err == nil { - return s.attachECSRoleToProfile(accountID, aws.StringValue(out.InstanceProfile.Arn), len(out.InstanceProfile.Roles) > 0) - } - if err.Error() != awserrors.ErrorIAMNoSuchEntity { - return "", fmt.Errorf("get instance profile %q: %w", ecsInstanceRoleName, err) - } - - created, err := s.deps.IAM.CreateInstanceProfile(accountID, &iam.CreateInstanceProfileInput{ - InstanceProfileName: aws.String(ecsInstanceRoleName), - }) - if err != nil { - if err.Error() != awserrors.ErrorIAMEntityAlreadyExists { - return "", fmt.Errorf("create instance profile %q: %w", ecsInstanceRoleName, err) - } - got, gerr := s.deps.IAM.GetInstanceProfile(accountID, &iam.GetInstanceProfileInput{ - InstanceProfileName: aws.String(ecsInstanceRoleName), - }) - if gerr != nil { - return "", fmt.Errorf("re-get instance profile %q: %w", ecsInstanceRoleName, gerr) - } - return s.attachECSRoleToProfile(accountID, aws.StringValue(got.InstanceProfile.Arn), len(got.InstanceProfile.Roles) > 0) - } - return s.attachECSRoleToProfile(accountID, aws.StringValue(created.InstanceProfile.Arn), false) -} - -// attachECSRoleToProfile adds the role to the profile unless already attached, -// returning the profile ARN. A LimitExceeded error means a racing launch -// attached it first and is treated as success. -func (s *Service) attachECSRoleToProfile(accountID, profileARN string, alreadyAttached bool) (string, error) { - if alreadyAttached { - return profileARN, nil - } - _, err := s.deps.IAM.AddRoleToInstanceProfile(accountID, &iam.AddRoleToInstanceProfileInput{ - InstanceProfileName: aws.String(ecsInstanceRoleName), - RoleName: aws.String(ecsInstanceRoleName), - }) - if err != nil && err.Error() != awserrors.ErrorIAMLimitExceeded { - return "", fmt.Errorf("add role to instance profile %q: %w", ecsInstanceRoleName, err) - } - return profileARN, nil +// convergeECSInstanceRole re-asserts the inline policy on an account that +// already has the role, so a running cluster picks up a changed document +// without creating IAM entities in accounts that never launched capacity. +func (s *Service) convergeECSInstanceRole(accountID string) error { + return handlers_iam.ConvergeSystemRolePolicy(s.deps.IAM, accountID, + ecsInstanceRoleName, ecsInstanceRolePolicyName, ecsInstanceRolePolicyDoc(accountID)) } diff --git a/spinifex/handlers/ecs/instancerole_retry_test.go b/spinifex/handlers/ecs/instancerole_retry_test.go deleted file mode 100644 index 6cc9fc3bb..000000000 --- a/spinifex/handlers/ecs/instancerole_retry_test.go +++ /dev/null @@ -1,90 +0,0 @@ -// Drives ensureECSRolePolicy directly and swaps the unexported retry sleep, so -// it needs the package's own scope. -// -//test:in-package -package handlers_ecs - -import ( - "errors" - "testing" - "time" - - "github.com/aws/aws-sdk-go/service/iam" - "github.com/mulgadc/spinifex/spinifex/awserrors" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -// flakyAttachIAM answers NoSuchEntity from AttachRolePolicy a set number of -// times before succeeding, standing in for a replica that has not yet caught up -// with the policy written immediately before. -type flakyAttachIAM struct { - stubIAM - - notFoundTimes int - attaches int -} - -func (f *flakyAttachIAM) AttachRolePolicy(_ string, _ *iam.AttachRolePolicyInput) (*iam.AttachRolePolicyOutput, error) { - f.attaches++ - if f.attaches <= f.notFoundTimes { - return nil, errors.New(awserrors.ErrorIAMNoSuchEntity) - } - return &iam.AttachRolePolicyOutput{}, nil -} - -// alwaysDeniedIAM answers a code the retry must not swallow. -type alwaysDeniedIAM struct { - stubIAM - - attaches int -} - -func (a *alwaysDeniedIAM) AttachRolePolicy(_ string, _ *iam.AttachRolePolicyInput) (*iam.AttachRolePolicyOutput, error) { - a.attaches++ - return nil, errors.New(awserrors.ErrorAccessDenied) -} - -func noAttachSleep(t *testing.T) { - t.Helper() - original := ecsAttachRetrySleep - ecsAttachRetrySleep = func(time.Duration) {} - t.Cleanup(func() { ecsAttachRetrySleep = original }) -} - -// The policy is created and then attached. A NoSuchEntity from that attach is a -// stale read of a write that landed, so provisioning must converge rather than -// fail the whole call. -func TestEnsureECSRolePolicyRetriesAStaleNoSuchEntity(t *testing.T) { - noAttachSleep(t) - fake := &flakyAttachIAM{stubIAM: newStubIAM(), notFoundTimes: 3} - svc := &Service{deps: Deps{IAM: fake}} - - require.NoError(t, svc.ensureECSRolePolicy("000000000106")) - assert.Equal(t, 4, fake.attaches, "three stale reads then the one that sees the policy") -} - -// The retry is bounded: a policy that genuinely never appears still fails. -func TestEnsureECSRolePolicyGivesUpOnAPersistentNoSuchEntity(t *testing.T) { - noAttachSleep(t) - fake := &flakyAttachIAM{stubIAM: newStubIAM(), notFoundTimes: 1000} - svc := &Service{deps: Deps{IAM: fake}} - - err := svc.ensureECSRolePolicy("000000000106") - require.Error(t, err) - assert.ErrorContains(t, err, awserrors.ErrorIAMNoSuchEntity) - assert.Equal(t, ecsAttachRetries+1, fake.attaches, "bounded by ecsAttachRetries") -} - -// Only the stale-read code is retried. Anything else is a real answer and must -// come back on the first attempt. -func TestEnsureECSRolePolicyDoesNotRetryOtherErrors(t *testing.T) { - noAttachSleep(t) - fake := &alwaysDeniedIAM{stubIAM: newStubIAM()} - svc := &Service{deps: Deps{IAM: fake}} - - err := svc.ensureECSRolePolicy("000000000106") - require.Error(t, err) - assert.ErrorContains(t, err, awserrors.ErrorAccessDenied) - assert.Equal(t, 1, fake.attaches, "a denial is not a visibility race") -} diff --git a/spinifex/handlers/ecs/scheduler.go b/spinifex/handlers/ecs/scheduler.go index 46315a930..7e33cfbe9 100644 --- a/spinifex/handlers/ecs/scheduler.go +++ b/spinifex/handlers/ecs/scheduler.go @@ -3,6 +3,8 @@ package handlers_ecs import ( "context" "encoding/json" + "errors" + "fmt" "log/slog" "strings" "sync" @@ -30,6 +32,11 @@ const ( // UI exit reason) before it is dropped, matching AWS's ~1h STOPPED window. sweepInterval = 60 * time.Second stoppedTaskRetention = 1 * time.Hour + + // convergeInterval is how often the leader re-asserts the instance-role + // policy. The document changes only across releases, so this trades a slower + // pickup after an upgrade for a fleet-wide write every reconcile tick. + convergeInterval = 5 * time.Minute ) // Scheduler is the per-daemon ECS control loop. A single leader (elected via the @@ -59,9 +66,10 @@ func NewScheduler(nc *nats.Conn, svc *Service, holder string) *Scheduler { TTL: KVBucketECSLeaderTTL, Renew: leaseRefresh, Retry: leaseRefresh, - // The leader owns the Layer-2 bus subscriptions. A node that cannot wire them stands - // down rather than hold the lease and process nothing. - OnGained: sc.subscribeBus, + // The leader owns the Layer-2 bus subscriptions and the instance-role + // converge pass. A node that can do neither stands down rather than hold + // the lease and process nothing. + OnGained: sc.onGained, OnLost: sc.unsubscribeBus, }) return sc @@ -85,9 +93,11 @@ func (sc *Scheduler) Run(ctx context.Context) { reaperTicker := time.NewTicker(reaperInterval) reconcileTicker := time.NewTicker(reconcileInterval) sweepTicker := time.NewTicker(sweepInterval) + convergeTicker := time.NewTicker(convergeInterval) defer reaperTicker.Stop() defer reconcileTicker.Stop() defer sweepTicker.Stop() + defer convergeTicker.Stop() leaseDone := make(chan struct{}) go func() { @@ -107,6 +117,8 @@ func (sc *Scheduler) Run(ctx context.Context) { sc.runIfLeader("service reconcile", func() error { return sc.svc.reconcileAllServices(ctx) }) case <-sweepTicker.C: sc.runIfLeader("stopped-task sweep", func() error { return sc.sweepStoppedTasks(ctx) }) + case <-convergeTicker.C: + sc.runIfLeader("instance role converge", func() error { return sc.convergeInstanceRoles(ctx) }) } } } @@ -123,6 +135,16 @@ func (sc *Scheduler) runIfLeader(pass string, run func() error) { } } +// onGained refuses the lease when this node cannot converge instance roles. +// A leader without IAM holds the lease and never writes the grant, so every +// agent's credentials lapse an hour later with nothing but a per-tick log. +func (sc *Scheduler) onGained(ctx context.Context) error { + if sc.svc.deps.IAM == nil { + return errors.New("IAM dependency not wired: cannot converge instance roles") + } + return sc.subscribeBus(ctx) +} + // subscribeBus wires the wildcard Layer-2 subscriptions onto the service KV // writers. All clusters/accounts fan into the leader. func (sc *Scheduler) subscribeBus(_ context.Context) error { @@ -268,6 +290,34 @@ func (sc *Scheduler) stopInstanceTasks(ctx context.Context, kv jetstream.KeyValu } } +// convergeInstanceRoles re-asserts the ecsInstanceRole policy on every account +// that already holds the role. Provisioning runs only when capacity changes, so +// a cluster that is merely running would otherwise never pick up a changed +// document and its agent would fail its next credential refresh. +func (sc *Scheduler) convergeInstanceRoles(ctx context.Context) error { + // A pass that cannot converge anything is a misconfiguration that silently + // breaks agent credentials an hour later, so it is not a benign no-op. + if sc.svc.deps.IAM == nil { + return errors.New("instance-role converge disabled: IAM dependency not wired") + } + buckets, err := accountBuckets(ctx, sc.nc) + if err != nil { + return err + } + failed := 0 + for _, bucket := range buckets { + if err := sc.svc.convergeECSInstanceRole(bucket.accountID); err != nil { + failed++ + slog.Error("ECS scheduler: converge instance role failed", + "accountId", bucket.accountID, "err", err) + } + } + if failed > 0 { + return fmt.Errorf("converge instance roles: %d of %d accounts failed", failed, len(buckets)) + } + return nil +} + // accountIDFromBucket extracts the account ID from an ECS per-account bucket name. func accountIDFromBucket(bucket string) (string, bool) { if !strings.HasPrefix(bucket, KVBucketECSAccountPrefix) { diff --git a/spinifex/handlers/ecs/scheduler_test.go b/spinifex/handlers/ecs/scheduler_test.go new file mode 100644 index 000000000..5d1b779a9 --- /dev/null +++ b/spinifex/handlers/ecs/scheduler_test.go @@ -0,0 +1,96 @@ +// Leader election, the converge pass and the instance-role document are all +// unexported, and the pass is only reachable through the scheduler's own state. +// +//test:in-package — the converge pass has no exported entry point +package handlers_ecs + +import ( + "context" + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ecs" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/mulgadc/spinifex/spinifex/testutil" +) + +// TestScheduler_StandsDownWithoutIAM is the fleet outage this guards: a leader +// that cannot converge instance roles holds the lease, writes no sts:AssumeRole +// grant, and every agent's credentials lapse an hour later. +func TestScheduler_StandsDownWithoutIAM(t *testing.T) { + _, nc, js := testutil.StartTestJetStream(t) + svc := NewService(nc, testRegion, "") + sc := NewScheduler(nc, svc, "holder-a") + require.NoError(t, sc.leaseErr) + + assert.False(t, sc.lease.TryAcquire(t.Context()), "a node without IAM must refuse the lease") + assert.False(t, sc.lease.Held()) + assert.Zero(t, sc.subCount()) + + // The key must be free, or a healthy node waits out the full TTL. + kv, err := InitLeaderBucket(t.Context(), js) + require.NoError(t, err) + _, err = kv.Get(t.Context(), schedulerLeaderKey) + require.Error(t, err) +} + +// TestConvergeInstanceRoles_RewritesProvisionedAccounts pins the pass that +// carries the grant to already-running clusters, which provisioning alone +// never reaches once capacity has stopped changing. +func TestConvergeInstanceRoles_RewritesProvisionedAccounts(t *testing.T) { + _, nc, _ := testutil.StartTestJetStream(t) + iam := newStubIAM() + svc := NewService(nc, testRegion, "").WithDeps(Deps{IAM: iam}) + _, err := svc.CreateCluster(context.Background(), &ecs.CreateClusterInput{ClusterName: aws.String("web")}, testAccountID) + require.NoError(t, err) + _, err = svc.ensureECSInstanceProfile(testAccountID) + require.NoError(t, err) + + iam.RolePolicies[ecsInstanceRoleName] = `{"Version":"2012-10-17","Statement":[]}` + sc := NewScheduler(nc, svc, "holder-a") + require.NoError(t, sc.convergeInstanceRoles(t.Context())) + + assert.Contains(t, iam.RolePolicies[ecsInstanceRoleName], "sts:AssumeRole") +} + +// TestConvergeInstanceRoles_SkipsAccountsWithoutTheRole keeps the pass from +// creating customer-visible IAM entities in an account that only ever created +// a cluster: the bucket exists there, but nothing draws credentials. +func TestConvergeInstanceRoles_SkipsAccountsWithoutTheRole(t *testing.T) { + _, nc, _ := testutil.StartTestJetStream(t) + iam := newStubIAM() + svc := NewService(nc, testRegion, "").WithDeps(Deps{IAM: iam}) + _, err := svc.CreateCluster(context.Background(), &ecs.CreateClusterInput{ClusterName: aws.String("web")}, testAccountID) + require.NoError(t, err) + + sc := NewScheduler(nc, svc, "holder-a") + require.NoError(t, sc.convergeInstanceRoles(t.Context())) + + assert.Zero(t, iam.CreateRoleCalls) + assert.Zero(t, iam.CreateInstanceProfileCalls) + assert.Empty(t, iam.PolicyCalls) +} + +// TestConvergeInstanceRoles_NilIAMIsAnError keeps the guard fail-closed for a +// caller that reaches the pass without going through leader election. +func TestConvergeInstanceRoles_NilIAMIsAnError(t *testing.T) { + _, nc, _ := testutil.StartTestJetStream(t) + sc := NewScheduler(nc, NewService(nc, testRegion, ""), "holder-a") + + err := sc.convergeInstanceRoles(t.Context()) + require.Error(t, err) + assert.Contains(t, err.Error(), "IAM", "the log must name the missing dependency") +} + +// TestECSInstanceRolePolicyDoc_GrantsAssumeRole is the identity-side half of the +// two-gate model for the agent: without it every task-role and execution-role +// credential mint is denied about an hour after an upgrade. +func TestECSInstanceRolePolicyDoc_GrantsAssumeRole(t *testing.T) { + doc := ecsInstanceRolePolicyDoc(testAccountID) + + assert.Contains(t, doc, `{"Effect":"Allow","Action":"sts:AssumeRole","Resource":"arn:aws:iam::`+testAccountID+`:role/*"}`) + assert.Contains(t, doc, `"ecs:*"`) + assert.Contains(t, doc, `"ecr:GetAuthorizationToken"`) +} diff --git a/spinifex/handlers/ecs/service.go b/spinifex/handlers/ecs/service.go index b7ade649d..c22b49e38 100644 --- a/spinifex/handlers/ecs/service.go +++ b/spinifex/handlers/ecs/service.go @@ -15,8 +15,8 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" "github.com/aws/aws-sdk-go/service/ecs" - "github.com/aws/aws-sdk-go/service/iam" "github.com/mulgadc/spinifex/spinifex/awserrors" + handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam" "github.com/nats-io/nats.go" "github.com/nats-io/nats.go/jetstream" ) @@ -87,17 +87,9 @@ type ecsImageResolver interface { } // ecsIAM is the narrow IAM surface ProvisionCapacity needs to find-or-create the -// ecsInstanceRole, its ecs:* policy, and the instance profile workers expose -// over IMDS. Method signatures match the daemon IAM service implementation. -type ecsIAM interface { - GetRole(accountID string, input *iam.GetRoleInput) (*iam.GetRoleOutput, error) - CreateRole(accountID string, input *iam.CreateRoleInput) (*iam.CreateRoleOutput, error) - CreatePolicy(accountID string, input *iam.CreatePolicyInput) (*iam.CreatePolicyOutput, error) - AttachRolePolicy(accountID string, input *iam.AttachRolePolicyInput) (*iam.AttachRolePolicyOutput, error) - GetInstanceProfile(accountID string, input *iam.GetInstanceProfileInput) (*iam.GetInstanceProfileOutput, error) - CreateInstanceProfile(accountID string, input *iam.CreateInstanceProfileInput) (*iam.CreateInstanceProfileOutput, error) - AddRoleToInstanceProfile(accountID string, input *iam.AddRoleToInstanceProfileInput) (*iam.AddRoleToInstanceProfileOutput, error) -} +// ecsInstanceRole, its inline policy, and the instance profile workers expose +// over IMDS. It is the same shape the ELBv2 and EKS system roles use. +type ecsIAM = handlers_iam.SystemInstanceRoleEnsurer // Deps are the collaborators ProvisionCapacity needs: the gateway endpoint/CA to // seed the agent's config, the IAM service to back the instance profile, the diff --git a/spinifex/handlers/ecs/service_test.go b/spinifex/handlers/ecs/service_test.go index cae94692e..16271b5a0 100644 --- a/spinifex/handlers/ecs/service_test.go +++ b/spinifex/handlers/ecs/service_test.go @@ -449,7 +449,7 @@ func TestScheduler_ReapBucket_StopsStaleInstanceTasks(t *testing.T) { // Leader election: first holder wins Create; a second holder is rejected. func TestScheduler_AcquireLease_SingleLeader(t *testing.T) { _, nc, _ := testutil.StartTestJetStream(t) - svc := NewService(nc, testRegion, "") + svc := NewService(nc, testRegion, "").WithDeps(Deps{IAM: newStubIAM()}) a := NewScheduler(nc, svc, "holder-a") b := NewScheduler(nc, svc, "holder-b") require.True(t, a.lease.TryAcquire(t.Context())) @@ -462,7 +462,7 @@ func TestScheduler_AcquireLease_SingleLeader(t *testing.T) { // conversion could introduce and the one thing kvlease cannot check itself. func TestScheduler_LeadershipWiresBusSubscriptions(t *testing.T) { _, nc, _ := testutil.StartTestJetStream(t) - svc := NewService(nc, testRegion, "") + svc := NewService(nc, testRegion, "").WithDeps(Deps{IAM: newStubIAM()}) sc := NewScheduler(nc, svc, "holder-a") require.NoError(t, sc.leaseErr) @@ -479,7 +479,7 @@ func TestScheduler_LeadershipWiresBusSubscriptions(t *testing.T) { // and loser writing the same KV records is the split-brain this lease prevents. func TestScheduler_LoserHoldsNoSubscriptions(t *testing.T) { _, nc, _ := testutil.StartTestJetStream(t) - svc := NewService(nc, testRegion, "") + svc := NewService(nc, testRegion, "").WithDeps(Deps{IAM: newStubIAM()}) a := NewScheduler(nc, svc, "holder-a") b := NewScheduler(nc, svc, "holder-b") @@ -491,7 +491,7 @@ func TestScheduler_LoserHoldsNoSubscriptions(t *testing.T) { func TestScheduler_RunReleasesLeaseOnShutdown(t *testing.T) { _, nc, js := testutil.StartTestJetStream(t) - svc := NewService(nc, testRegion, "") + svc := NewService(nc, testRegion, "").WithDeps(Deps{IAM: newStubIAM()}) sc := NewScheduler(nc, svc, "holder-a") require.NoError(t, sc.leaseErr) diff --git a/spinifex/handlers/iam/roles.go b/spinifex/handlers/iam/roles.go index a74618ad4..83ff455f7 100644 --- a/spinifex/handlers/iam/roles.go +++ b/spinifex/handlers/iam/roles.go @@ -362,10 +362,17 @@ func (s *IAMServiceImpl) PutRolePolicy(accountID string, input *iam.PutRolePolic return nil, errors.New(awserrors.ErrorIAMMalformedPolicyDocument) } + // An unchanged document writes nothing: converge loops re-assert the same + // policy on every pass, and each write would burn a KV revision. + changed := false err := s.updateRoleCAS(ctx, accountID, roleName, func(role *Role) (bool, error) { if role.InlinePolicies == nil { role.InlinePolicies = map[string]string{} } + changed = role.InlinePolicies[policyName] != policyDoc + if !changed { + return false, nil + } role.InlinePolicies[policyName] = policyDoc return true, nil }) @@ -373,7 +380,9 @@ func (s *IAMServiceImpl) PutRolePolicy(accountID string, input *iam.PutRolePolic return nil, err } - slog.Info("IAM inline policy put on role", "accountID", accountID, "roleName", roleName, "policyName", policyName) + if changed { + slog.Info("IAM inline policy put on role", "accountID", accountID, "roleName", roleName, "policyName", policyName) + } return &iam.PutRolePolicyOutput{}, nil } diff --git a/spinifex/handlers/iam/system_role.go b/spinifex/handlers/iam/system_role.go index cc7b0425b..d7208f7cf 100644 --- a/spinifex/handlers/iam/system_role.go +++ b/spinifex/handlers/iam/system_role.go @@ -43,6 +43,26 @@ func EnsureSystemInstanceProfile(e SystemInstanceRoleEnsurer, accountID, roleNam return ensureSystemInstanceProfile(e, accountID, roleName) } +// ConvergeSystemRolePolicy re-asserts a system role's inline policy on an +// account that already carries the role. An absent role is a no-op: nothing +// there draws credentials from it, so provisioning one would be unasked-for. +func ConvergeSystemRolePolicy(e SystemInstanceRoleEnsurer, accountID, roleName, policyName, policyDoc string) error { + if _, err := e.GetRole(accountID, &iam.GetRoleInput{RoleName: aws.String(roleName)}); err != nil { + if err.Error() == awserrors.ErrorIAMNoSuchEntity { + return nil + } + return fmt.Errorf("get role %q: %w", roleName, err) + } + if _, err := e.PutRolePolicy(accountID, &iam.PutRolePolicyInput{ + RoleName: aws.String(roleName), + PolicyName: aws.String(policyName), + PolicyDocument: aws.String(policyDoc), + }); err != nil { + return fmt.Errorf("put role policy %q on %q: %w", policyName, roleName, err) + } + return nil +} + // ensureSystemRole creates the role with the EC2 trust policy when absent. A // racing create that lost (EntityAlreadyExists) is tolerated as success. func ensureSystemRole(e SystemInstanceRoleEnsurer, accountID, roleName string) error { diff --git a/spinifex/handlers/iam/system_role_test.go b/spinifex/handlers/iam/system_role_test.go index 8485b8f94..5998efaff 100644 --- a/spinifex/handlers/iam/system_role_test.go +++ b/spinifex/handlers/iam/system_role_test.go @@ -49,3 +49,41 @@ func TestEnsureSystemInstanceProfile_Idempotent(t *testing.T) { assert.Equal(t, 1, f.CreateInstanceProfileCalls, "profile created once") assert.Equal(t, 1, f.AddRoleToInstanceProfileCalls, "role attached once") } + +// TestConvergeSystemRolePolicy_RewritesExistingRole asserts an account that +// already carries the role picks up a changed document. +func TestConvergeSystemRolePolicy_RewritesExistingRole(t *testing.T) { + f := iammock.New() + _, err := handlers_iam.EnsureSystemInstanceProfile(f, testRoleAcct, testRoleName, testPolicyName, testPolicyDoc) + require.NoError(t, err) + + const updated = `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["svc:DoThing","sts:AssumeRole"],"Resource":"*"}]}` + require.NoError(t, handlers_iam.ConvergeSystemRolePolicy(f, testRoleAcct, testRoleName, testPolicyName, updated)) + assert.Equal(t, updated, f.RolePolicies[testRoleName]) +} + +// TestConvergeSystemRolePolicy_AbsentRoleIsNotProvisioned is the property that +// keeps a fleet-wide converge pass from writing IAM entities into accounts that +// never launched capacity: no role means nothing to converge. +func TestConvergeSystemRolePolicy_AbsentRoleIsNotProvisioned(t *testing.T) { + f := iammock.New() + require.NoError(t, handlers_iam.ConvergeSystemRolePolicy(f, testRoleAcct, testRoleName, testPolicyName, testPolicyDoc)) + + assert.Zero(t, f.CreateRoleCalls) + assert.Zero(t, f.CreateInstanceProfileCalls) + assert.Empty(t, f.PolicyCalls) + assert.NotContains(t, f.RolePolicies, testRoleName) +} + +// TestConvergeSystemRolePolicy_PropagatesPutFailure keeps a failed write a +// failure: swallowing it reports a converged fleet that is not converged. +func TestConvergeSystemRolePolicy_PropagatesPutFailure(t *testing.T) { + f := iammock.New() + _, err := handlers_iam.EnsureSystemInstanceProfile(f, testRoleAcct, testRoleName, testPolicyName, testPolicyDoc) + require.NoError(t, err) + + f.PutRolePolicyErr = assert.AnError + err = handlers_iam.ConvergeSystemRolePolicy(f, testRoleAcct, testRoleName, testPolicyName, testPolicyDoc) + require.Error(t, err) + assert.ErrorIs(t, err, assert.AnError) +} diff --git a/spinifex/handlers/sts/assume_role.go b/spinifex/handlers/sts/assume_role.go index 68fab6876..d880cb102 100644 --- a/spinifex/handlers/sts/assume_role.go +++ b/spinifex/handlers/sts/assume_role.go @@ -21,7 +21,6 @@ import ( "github.com/aws/aws-sdk-go/service/sts" "github.com/nats-io/nats.go/jetstream" - "github.com/mulgadc/bluebottle/pkg/auth" "github.com/mulgadc/spinifex/spinifex/awserrors" handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam" "github.com/mulgadc/spinifex/spinifex/utils" @@ -152,20 +151,15 @@ func (s *STSServiceImpl) AssumeRoleForInstance(accountID, roleARN, instanceID st // resolves the role, clamps the duration, evaluates the trust policy, and mints credentials. // principalSource is "" for HTTPS (derived from callerARN), ec2.amazonaws.com for IMDS. func (s *STSServiceImpl) assumeRoleForCaller(ctx context.Context, callerARN, principalSource, roleARN, sessionName, sourceIdentity string, requestedDuration int64) (*sts.AssumeRoleOutput, error) { - roleAccountID, roleName, err := auth.ParseRoleARN(roleARN) + roleAccountID, role, err := ResolveRoleByARN(s.iamSvc, roleARN) if err != nil { - return nil, errors.New(awserrors.ErrorValidationError) - } - - roleOut, err := s.iamSvc.GetRole(roleAccountID, &iam.GetRoleInput{RoleName: aws.String(roleName)}) - if err != nil { - // All misses are masked to AccessDenied, matching AWS and preventing role enumeration. - if err.Error() == awserrors.ErrorIAMNoSuchEntity { + // A miss and a non-canonical ARN are both masked to AccessDenied, + // matching AWS and preventing role enumeration. + if errors.Is(err, ErrRoleUnresolved) { return nil, errors.New(awserrors.ErrorAccessDenied) } return nil, err } - role := roleOut.Role duration := requestedDuration if duration == 0 { diff --git a/spinifex/handlers/sts/role_arn.go b/spinifex/handlers/sts/role_arn.go new file mode 100644 index 000000000..7d54d3f9d --- /dev/null +++ b/spinifex/handlers/sts/role_arn.go @@ -0,0 +1,43 @@ +package handlers_sts + +import ( + "errors" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/iam" + + "github.com/mulgadc/bluebottle/pkg/auth" + "github.com/mulgadc/spinifex/spinifex/awserrors" +) + +// ErrRoleUnresolved reports a caller-supplied role ARN that names no role the +// caller may learn about: either no such role, or an ARN that is not the form +// IAM stored. Both are masked to AccessDenied on the wire. +var ErrRoleUnresolved = errors.New("role ARN does not resolve to a stored role") + +// RoleGetter is the IAM surface role resolution needs. +type RoleGetter interface { + GetRole(accountID string, input *iam.GetRoleInput) (*iam.GetRoleOutput, error) +} + +// ResolveRoleByARN resolves the role a caller-supplied ARN names and verifies +// the ARN is the one IAM stored, returning the role's account. The lookup +// discards any path in the supplied ARN, so comparing the stored ARN back is +// what stops an invented path reaching a role the ARN does not name. +func ResolveRoleByARN(svc RoleGetter, roleARN string) (string, *iam.Role, error) { + accountID, roleName, err := auth.ParseRoleARN(roleARN) + if err != nil { + return "", nil, errors.New(awserrors.ErrorValidationError) + } + out, err := svc.GetRole(accountID, &iam.GetRoleInput{RoleName: aws.String(roleName)}) + if err != nil { + if err.Error() == awserrors.ErrorIAMNoSuchEntity { + return "", nil, ErrRoleUnresolved + } + return "", nil, err + } + if out == nil || out.Role == nil || aws.StringValue(out.Role.Arn) != roleARN { + return "", nil, ErrRoleUnresolved + } + return accountID, out.Role, nil +} diff --git a/spinifex/handlers/sts/role_arn_test.go b/spinifex/handlers/sts/role_arn_test.go new file mode 100644 index 000000000..e6a351c38 --- /dev/null +++ b/spinifex/handlers/sts/role_arn_test.go @@ -0,0 +1,75 @@ +package handlers_sts_test + +import ( + "errors" + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/iam" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/mulgadc/spinifex/spinifex/awserrors" + handlers_sts "github.com/mulgadc/spinifex/spinifex/handlers/sts" +) + +// roleGetterStub answers GetRole from a name-to-stored-ARN map, the way IAM +// keys roles, and reports NoSuchEntity for anything else. +type roleGetterStub map[string]string + +func (s roleGetterStub) GetRole(_ string, input *iam.GetRoleInput) (*iam.GetRoleOutput, error) { + arn, ok := s[aws.StringValue(input.RoleName)] + if !ok { + return nil, errors.New(awserrors.ErrorIAMNoSuchEntity) + } + return &iam.GetRoleOutput{Role: &iam.Role{RoleName: input.RoleName, Arn: aws.String(arn)}}, nil +} + +// TestResolveRoleByARN pins the comparison every caller-supplied role ARN rests +// on. The lookup discards any path the caller wrote, so an ARN that is not the +// one IAM stored must not reach the role its trailing name happens to match. +func TestResolveRoleByARN(t *testing.T) { + const account = "000000000000" + roles := roleGetterStub{ + "admin": "arn:aws:iam::000000000000:role/admin", + "app-worker": "arn:aws:iam::000000000000:role/team/app-worker", + } + + t.Run("stored ARN resolves", func(t *testing.T) { + gotAccount, role, err := handlers_sts.ResolveRoleByARN(roles, "arn:aws:iam::000000000000:role/admin") + require.NoError(t, err) + assert.Equal(t, account, gotAccount) + assert.Equal(t, "arn:aws:iam::000000000000:role/admin", aws.StringValue(role.Arn)) + }) + + t.Run("a pathed role resolves by its full stored ARN", func(t *testing.T) { + _, role, err := handlers_sts.ResolveRoleByARN(roles, "arn:aws:iam::000000000000:role/team/app-worker") + require.NoError(t, err) + assert.Equal(t, "arn:aws:iam::000000000000:role/team/app-worker", aws.StringValue(role.Arn)) + }) + + t.Run("an invented path does not reach the role it names", func(t *testing.T) { + // `app-x/admin` looks up `admin`, whose stored ARN carries no such path. + _, _, err := handlers_sts.ResolveRoleByARN(roles, "arn:aws:iam::000000000000:role/app-x/admin") + require.ErrorIs(t, err, handlers_sts.ErrRoleUnresolved) + }) + + t.Run("a pathed role is not reachable by its pathless ARN", func(t *testing.T) { + _, _, err := handlers_sts.ResolveRoleByARN(roles, "arn:aws:iam::000000000000:role/app-worker") + require.ErrorIs(t, err, handlers_sts.ErrRoleUnresolved) + }) + + t.Run("a role that does not exist is unresolved", func(t *testing.T) { + _, _, err := handlers_sts.ResolveRoleByARN(roles, "arn:aws:iam::000000000000:role/nobody") + require.ErrorIs(t, err, handlers_sts.ErrRoleUnresolved) + }) + + t.Run("a malformed ARN is a validation error", func(t *testing.T) { + _, _, err := handlers_sts.ResolveRoleByARN(roles, "not-an-arn") + require.Error(t, err) + assert.NotErrorIs(t, err, handlers_sts.ErrRoleUnresolved) + code, ok := awserrors.ResolveErrorCode(err) + require.True(t, ok) + assert.Equal(t, awserrors.ErrorValidationError, code) + }) +} diff --git a/tests/e2e/ecs/ecs_test.go b/tests/e2e/ecs/ecs_test.go index 6da8fd153..13629492b 100644 --- a/tests/e2e/ecs/ecs_test.go +++ b/tests/e2e/ecs/ecs_test.go @@ -380,7 +380,16 @@ func ensureInstanceProfile(t *testing.T, c *harness.AWSClient) { }) tolerateExists(t, err, "create-role ecsInstanceRole") - const policy = `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":"ecs:*","Resource":"*"}]}` + // Mirrors what the control plane provisions: ecs:*, ECR read, and the + // sts:AssumeRole the agent needs to mint task-role credentials. Seeding less + // than production would leave the harness failing alongside it rather than + // ahead of it. + policy := `{"Version":"2012-10-17","Statement":[` + + `{"Effect":"Allow","Action":"ecs:*","Resource":"*"},` + + `{"Effect":"Allow","Action":["ecr:GetAuthorizationToken","ecr:BatchCheckLayerAvailability",` + + `"ecr:GetDownloadUrlForLayer","ecr:BatchGetImage"],"Resource":"*"},` + + `{"Effect":"Allow","Action":"sts:AssumeRole","Resource":"arn:aws:iam::` + + harness.IAMAccountID(t, c) + `:role/*"}]}` _, err = c.IAM.PutRolePolicy(&iam.PutRolePolicyInput{ RoleName: aws.String(name), PolicyName: aws.String("ecsInstanceRolePolicy"), diff --git a/tests/integration/sts_test.go b/tests/integration/sts_test.go index 87594de82..48d9199e4 100644 --- a/tests/integration/sts_test.go +++ b/tests/integration/sts_test.go @@ -282,6 +282,18 @@ func TestSTSAssumeRoleAndGetCallerIdentity(t *testing.T) { require.NoError(t, err, "create-role (chain)") chainedRoleARN := aws.StringValue(chainCreateOut.Role.Arn) + // The chained assume needs both gates: the trust policy above, and an + // identity-side grant on the session's own role. An inline policy keeps + // the teardown below free of managed-policy detaches. + _, err = iamCli.PutRolePolicy(&iam.PutRolePolicyInput{ + RoleName: aws.String(stsRoleName), + PolicyName: aws.String("sts-role-chain-assume"), + PolicyDocument: aws.String(fmt.Sprintf( + `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":"sts:AssumeRole","Resource":%q}]}`, + chainedRoleARN)), + }) + require.NoError(t, err, "put-role-policy (chain)") + chainOut, err := sessionCli.STS.AssumeRole(&sts.AssumeRoleInput{ RoleArn: aws.String(chainedRoleARN), RoleSessionName: aws.String(stsSessionNameChain), @@ -365,6 +377,12 @@ func TestSTSAssumeRoleAndGetCallerIdentity(t *testing.T) { // is unaffected by trust-policy references in either direction. _, err = iamCli.DeleteRole(&iam.DeleteRoleInput{RoleName: aws.String(stsRoleNameChain)}) require.NoError(t, err, "delete-role (chain)") + // An inline policy is a subordinate entity: DeleteRole rejects until it goes. + _, err = iamCli.DeleteRolePolicy(&iam.DeleteRolePolicyInput{ + RoleName: aws.String(stsRoleName), + PolicyName: aws.String("sts-role-chain-assume"), + }) + require.NoError(t, err, "delete-role-policy (chain)") _, err = iamCli.DeleteRole(&iam.DeleteRoleInput{RoleName: aws.String(stsRoleName)}) require.NoError(t, err, "delete-role") }