From 0d8855d1aad7b4dfa8b1675941f43d4ec24f57cd Mon Sep 17 00:00:00 2001 From: Julian Sommer <115516918+juliansommer@users.noreply.github.com> Date: Mon, 7 Sep 2026 10:54:05 +1000 Subject: [PATCH] fix: authorize EC2 PassRole against the stored role ARN (#1008) --- .../instance/IamInstanceProfileAssociation.go | 5 +- .../IamInstanceProfileAssociation_test.go | 75 +++++++++++++++++-- .../run_instances_passrole_test.go | 54 ++++++++++--- 3 files changed, 116 insertions(+), 18 deletions(-) diff --git a/spinifex/gateway/ec2/instance/IamInstanceProfileAssociation.go b/spinifex/gateway/ec2/instance/IamInstanceProfileAssociation.go index da71d0000..a5b0c12ff 100644 --- a/spinifex/gateway/ec2/instance/IamInstanceProfileAssociation.go +++ b/spinifex/gateway/ec2/instance/IamInstanceProfileAssociation.go @@ -41,7 +41,10 @@ func resolveAndAuthorizeProfile(spec *ec2.IamInstanceProfileSpecification, iamSv return nil, err } if profile.RoleName != "" && passRoleCheck != nil { - roleARN := arn.FormatIAMPath(arn.IAMRole, profile.AccountID, "/", profile.RoleName) + roleARN, err := iamSvc.CanonicalResourceARN(profile.AccountID, arn.IAMRole, profile.RoleName) + if err != nil { + return nil, err + } if err := passRoleCheck(roleARN); err != nil { return nil, err } diff --git a/spinifex/gateway/ec2/instance/IamInstanceProfileAssociation_test.go b/spinifex/gateway/ec2/instance/IamInstanceProfileAssociation_test.go index ab0cae354..d7a16cd67 100644 --- a/spinifex/gateway/ec2/instance/IamInstanceProfileAssociation_test.go +++ b/spinifex/gateway/ec2/instance/IamInstanceProfileAssociation_test.go @@ -10,6 +10,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/spinifex/arn" "github.com/mulgadc/spinifex/spinifex/awserrors" handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam" spxtypes "github.com/mulgadc/spinifex/spinifex/types" @@ -31,14 +32,13 @@ const ( testProfileIDOther = "AIPAEXAMPLE0000000002" ) -// fakeIAMService embeds the IAMService interface so it satisfies the contract -// without hand-writing every method. The EC2 IAM-profile gateway code only -// touches ResolveInstanceProfile; any other method nil-panics, surfacing an -// unexpected call rather than masking it with a zero value. +// fakeIAMService implements profile and canonical role lookups. +// Other methods nil-panic to surface unexpected calls. type fakeIAMService struct { handlers_iam.IAMService - resolveFn func(accountID, nameOrARN string) (*handlers_iam.InstanceProfile, error) + resolveFn func(accountID, nameOrARN string) (*handlers_iam.InstanceProfile, error) + canonicalFn func(accountID string, kind arn.IAMResourceType, name string) (string, error) } func (f *fakeIAMService) ResolveInstanceProfile(accountID, nameOrARN string) (*handlers_iam.InstanceProfile, error) { @@ -48,6 +48,16 @@ func (f *fakeIAMService) ResolveInstanceProfile(accountID, nameOrARN string) (*h return nil, errors.New(awserrors.ErrorIAMNoSuchEntity) } +func (f *fakeIAMService) CanonicalResourceARN(accountID string, kind arn.IAMResourceType, name string) (string, error) { + if f.canonicalFn != nil { + return f.canonicalFn(accountID, kind, name) + } + if accountID == testGwAccountID && kind == arn.IAMRole && name == "app-role" { + return testRoleARNApp, nil + } + return "", errors.New(awserrors.ErrorIAMNoSuchEntity) +} + var _ handlers_iam.IAMService = (*fakeIAMService)(nil) // profileWithRole builds a resolved profile fixture pointing at the test role. @@ -682,3 +692,58 @@ func TestAssociationIDFormat(t *testing.T) { suffix := strings.TrimPrefix(id, "iip-assoc-") assert.NotEmpty(t, suffix, "association ID must carry a non-empty random suffix") } + +func TestResolveAndAuthorizeProfile_CanonicalRoleARN(t *testing.T) { + t.Parallel() + for _, path := range []string{"/", "/team/", "/team/services/"} { + t.Run(path, func(t *testing.T) { + t.Parallel() + storedARN := "arn:aws:iam::111122223333:role" + path + "app-role" + svc := &fakeIAMService{ + resolveFn: func(string, string) (*handlers_iam.InstanceProfile, error) { + return profileWithRole(), nil + }, + canonicalFn: func(accountID string, kind arn.IAMResourceType, name string) (string, error) { + require.Equal(t, testGwAccountID, accountID) + require.Equal(t, arn.IAMRole, kind) + require.Equal(t, "app-role", name) + return storedARN, nil + }, + } + var checkedARN string + _, err := resolveAndAuthorizeProfile( + &ec2.IamInstanceProfileSpecification{Name: aws.String(testProfileNameApp)}, + svc, testGwAccountID, func(roleARN string) error { + checkedARN = roleARN + return nil + }) + require.NoError(t, err) + require.Equal(t, storedARN, checkedARN) + }) + } +} + +func TestResolveAndAuthorizeProfile_CanonicalLookupError(t *testing.T) { + t.Parallel() + for _, lookupErr := range []error{errors.New(awserrors.ErrorIAMNoSuchEntity), nats.ErrTimeout} { + t.Run(lookupErr.Error(), func(t *testing.T) { + t.Parallel() + svc := &fakeIAMService{ + resolveFn: func(string, string) (*handlers_iam.InstanceProfile, error) { + return profileWithRole(), nil + }, + canonicalFn: func(string, arn.IAMResourceType, string) (string, error) { + return "", lookupErr + }, + } + profile, err := resolveAndAuthorizeProfile( + &ec2.IamInstanceProfileSpecification{Name: aws.String(testProfileNameApp)}, + svc, testGwAccountID, func(string) error { + t.Fatal("PassRole must not run when the role lookup fails") + return nil + }) + require.ErrorIs(t, err, lookupErr) + require.Nil(t, profile) + }) + } +} diff --git a/tests/integration/run_instances_passrole_test.go b/tests/integration/run_instances_passrole_test.go index cb5b8b8f3..71edc544d 100644 --- a/tests/integration/run_instances_passrole_test.go +++ b/tests/integration/run_instances_passrole_test.go @@ -33,27 +33,30 @@ func prPolicyWithPassRole(targetRoleARN string) string { ]}`, targetRoleARN) } -// TestRunInstances_DeniedWithoutPassRole proves RunInstances enforces -// iam:PassRole (resolveAndAuthorizeInstanceProfile -> -// resolveAndAuthorizeProfile in gateway/ec2/instance/RunInstances.go / -// IamInstanceProfileAssociation.go) against the role inside a supplied -// instance profile: a caller granted ec2:RunInstances but not iam:PassRole on -// the target role is denied, and the identical session is allowed once the -// role grants that specific PassRole. Isolating PassRole as the sole variable -// (ec2:RunInstances is granted throughout) proves the denial is genuinely -// gated on the missing PassRole grant rather than some other authz gap. +// TestRunInstances_DeniedWithoutPassRole exercises stored role paths with real IAM +// policy evaluation: missing grants and explicit denies reject the request, +// while an allow naming the stored role ARN permits dispatch. func TestRunInstances_DeniedWithoutPassRole(t *testing.T) { + for _, path := range []string{"/", "/team/", "/team/services/"} { + t.Run(path, func(t *testing.T) { + testInstanceProfilePassRole(t, path) + }) + } +} + +func testInstanceProfilePassRole(t *testing.T, path string) { + t.Helper() gw := StartGateway(t) iamCli := gw.IAMClient(t) - targetRoleARN := iamRoleARN(gw.AccountID, prTargetRole) - // The role backing the instance profile the caller wants to attach. - _, err := iamCli.CreateRole(&iam.CreateRoleInput{ + targetRole, err := iamCli.CreateRole(&iam.CreateRoleInput{ RoleName: aws.String(prTargetRole), + Path: aws.String(path), AssumeRolePolicyDocument: aws.String(iamTrustPolicyEC2Standard), }) require.NoError(t, err, "create-role (target)") + targetRoleARN := aws.StringValue(targetRole.Role.Arn) _, err = iamCli.CreateInstanceProfile(&iam.CreateInstanceProfileInput{ InstanceProfileName: aws.String(prTargetProfile), @@ -113,6 +116,33 @@ func TestRunInstances_DeniedWithoutPassRole(t *testing.T) { _, err = sessionCli.EC2.RunInstances(runInput) requireAWSErrorCode(t, err, "AccessDenied") + // An explicit path-scoped deny must override an otherwise unrestricted grant. + denyPolicy, err := iamCli.CreatePolicy(&iam.CreatePolicyInput{ + PolicyName: aws.String("pr-deny-path"), + PolicyDocument: aws.String(fmt.Sprintf(`{"Version":"2012-10-17","Statement":[ + {"Effect":"Allow","Action":"*","Resource":"*"}, + {"Effect":"Deny","Action":"iam:PassRole","Resource":"arn:aws:iam::%s:role%s*"} + ]}`, gw.AccountID, path)), + }) + require.NoError(t, err) + _, err = iamCli.AttachRolePolicy(&iam.AttachRolePolicyInput{ + RoleName: callerRoleOut.Role.RoleName, + PolicyArn: denyPolicy.Policy.Arn, + }) + require.NoError(t, err) + _, err = sessionCli.EC2.RunInstances(runInput) + requireAWSErrorCode(t, err, "AccessDenied") + _, err = sessionCli.EC2.AssociateIamInstanceProfile(&ec2.AssociateIamInstanceProfileInput{ + InstanceId: aws.String("i-0123456789abcdef0"), + IamInstanceProfile: runInput.IamInstanceProfile, + }) + requireAWSErrorCode(t, err, "AccessDenied") + _, err = iamCli.DetachRolePolicy(&iam.DetachRolePolicyInput{ + RoleName: callerRoleOut.Role.RoleName, + PolicyArn: denyPolicy.Policy.Arn, + }) + require.NoError(t, err) + // Grant iam:PassRole scoped to the exact target role ARN; the identical // session must now be allowed through to a real launch. withPassRolePolicy, err := iamCli.CreatePolicy(&iam.CreatePolicyInput{