From 1c87d178a2faf6b82e01c05df59032bdf14d2efc Mon Sep 17 00:00:00 2001 From: Delaine Rogers Date: Thu, 24 Sep 2026 11:12:33 -0700 Subject: [PATCH] fix(host): fix role_arn fallback bug (#82) * Fix role arn fallthrough bug * add unit test * bump awsutil version * fix tests --- go.mod | 2 +- go.sum | 4 +- internal/credential/state.go | 25 +++++++ plugin/service/host/plugin.go | 18 +++++ plugin/service/host/plugin_test.go | 114 +++++++++++++++++++++++++++-- 5 files changed, 153 insertions(+), 10 deletions(-) diff --git a/go.mod b/go.mod index c178e50..75aed3e 100644 --- a/go.mod +++ b/go.mod @@ -14,7 +14,7 @@ require ( github.com/google/uuid v1.6.0 github.com/hashicorp/boundary/sdk v0.0.59 github.com/hashicorp/go-multierror v1.1.1 - github.com/hashicorp/go-secure-stdlib/awsutil/v2 v2.1.2 + github.com/hashicorp/go-secure-stdlib/awsutil/v2 v2.1.3 github.com/hashicorp/go-secure-stdlib/parseutil v0.2.0 github.com/hashicorp/terraform-json v0.23.0 github.com/mitchellh/mapstructure v1.5.0 diff --git a/go.sum b/go.sum index b4969f4..d3c174b 100644 --- a/go.sum +++ b/go.sum @@ -94,8 +94,8 @@ github.com/hashicorp/go-multierror v1.1.1 h1:H5DkEtf6CXdFp0N0Em5UCwQpXMWke8IA0+l github.com/hashicorp/go-multierror v1.1.1/go.mod h1:iw975J/qwKPdAO1clOe2L8331t/9/fmwbPZ6JB6eMoM= github.com/hashicorp/go-plugin v1.7.0 h1:YghfQH/0QmPNc/AZMTFE3ac8fipZyZECHdDPshfk+mA= github.com/hashicorp/go-plugin v1.7.0/go.mod h1:BExt6KEaIYx804z8k4gRzRLEvxKVb+kn0NMcihqOqb8= -github.com/hashicorp/go-secure-stdlib/awsutil/v2 v2.1.2 h1:KlDrZWx/wYpnUcFPB0s9uhPjyNZoeIfhrV2MN/Ly76s= -github.com/hashicorp/go-secure-stdlib/awsutil/v2 v2.1.2/go.mod h1:6+rVulOPNCQbL3Xv2iLCqM0JmU2WO2wRzP1C6hBKeB8= +github.com/hashicorp/go-secure-stdlib/awsutil/v2 v2.1.3 h1:mLe7IMysLSedhuu00W7AXsbfb90AA7UKfZmw3OkJ8n4= +github.com/hashicorp/go-secure-stdlib/awsutil/v2 v2.1.3/go.mod h1:6+rVulOPNCQbL3Xv2iLCqM0JmU2WO2wRzP1C6hBKeB8= github.com/hashicorp/go-secure-stdlib/base62 v0.1.2 h1:ET4pqyjiGmY09R5y+rSd70J2w45CtbWDNvGqWp/R3Ng= github.com/hashicorp/go-secure-stdlib/base62 v0.1.2/go.mod h1:EdWO6czbmthiwZ3/PUsDV+UD1D5IRU4ActiaWGwt0Yw= github.com/hashicorp/go-secure-stdlib/configutil/v2 v2.0.13 h1:TayxZ5drfMP0G6T++WvnLESGLOWeHtdWDWWTAi2e3Qk= diff --git a/internal/credential/state.go b/internal/credential/state.go index 774c131..8dcdce2 100644 --- a/internal/credential/state.go +++ b/internal/credential/state.go @@ -112,10 +112,35 @@ func NewAwsCredentialPersistedState(opts ...AwsCredentialPersistedStateOption) ( // and runs sts.GetCallerIdentity for the current credentials, which is done // to check that the credentials are valid. This method returns a status error // with PluginError details. +// +// When RoleARN is set and no static key is present (ambient role path), +// credentials are eagerly retrieved before calling GetCallerIdentity. This +// forces sts:AssumeRole to be called immediately so that any role errors +// (e.g. no such role, access denied) surface here rather than being deferred +// to the first real AWS API call. func (s *AwsCredentialPersistedState) ValidateCreds(ctx context.Context) error { if s.CredentialsConfig == nil { return errors.BadRequestStatus("missing credentials config") } + + // For the ambient role path (RoleARN set, no static key), eagerly resolve + // the credential chain so that sts:AssumeRole is called now. Without this, + // the AssumeRoleProvider inside the chain is lazy and GetCallerIdentity + // would succeed even with an invalid role ARN (it uses the mock/ambient + // identity directly rather than the assumed one). + cc := s.CredentialsConfig + if cc.RoleARN != "" && cc.AccessKey == "" { + cfg, err := s.GenerateCredentialChain(ctx) + if err != nil { + st, _ := errors.ParseAWSError("validating credentials", err) + return st.Err() + } + if _, err := cfg.Credentials.Retrieve(ctx); err != nil { + st, _ := errors.ParseAWSError("validating credentials", err) + return st.Err() + } + } + if _, err := s.CredentialsConfig.GetCallerIdentity(ctx, s.testOpts...); err != nil { st, _ := errors.ParseAWSError("validating credentials", err) return st.Err() diff --git a/plugin/service/host/plugin.go b/plugin/service/host/plugin.go index feccd85..4d19768 100644 --- a/plugin/service/host/plugin.go +++ b/plugin/service/host/plugin.go @@ -86,6 +86,15 @@ func (p *HostPlugin) OnCreateCatalog(ctx context.Context, req *pb.OnCreateCatalo } } + // Validate assumed-role credentials at catalog-creation time so that an + // invalid role_arn is rejected immediately rather than silently falling + // back to ambient credentials. + if credential.GetCredentialType(credState.CredentialsConfig) == credential.DynamicAWS { + if err := credState.ValidateCreds(ctx); err != nil { + return nil, err + } + } + catalogState, err := newAwsCatalogPersistedState( append([]awsCatalogPersistedStateOption{ withCredentials(credState), @@ -181,6 +190,15 @@ func (p *HostPlugin) OnUpdateCatalog(ctx context.Context, req *pb.OnUpdateCatalo if err != nil { return nil, errors.BadRequestStatusf("error setting up new credential persisted state: %s", err) } + + // Validate assumed-role credentials when the role_arn changes so that + // an invalid ARN is caught before replacing the existing credential state. + if credential.GetCredentialType(newCredState.CredentialsConfig) == credential.DynamicAWS { + if err := newCredState.ValidateCreds(ctx); err != nil { + return nil, err + } + } + newCatalogState, err := newAwsCatalogPersistedState( append([]awsCatalogPersistedStateOption{ withCredentials(newCredState), diff --git a/plugin/service/host/plugin_test.go b/plugin/service/host/plugin_test.go index 34db9a1..f8edf3f 100644 --- a/plugin/service/host/plugin_test.go +++ b/plugin/service/host/plugin_test.go @@ -16,6 +16,8 @@ import ( "github.com/aws/aws-sdk-go-v2/service/ec2/types" "github.com/aws/aws-sdk-go-v2/service/iam" iamTypes "github.com/aws/aws-sdk-go-v2/service/iam/types" + "github.com/aws/aws-sdk-go-v2/service/sts" + stsTypes "github.com/aws/aws-sdk-go-v2/service/sts/types" "github.com/google/go-cmp/cmp" "github.com/hashicorp/boundary-plugin-aws/internal/credential" awserrors "github.com/hashicorp/boundary-plugin-aws/internal/errors" @@ -29,6 +31,38 @@ import ( "google.golang.org/protobuf/testing/protocmp" "google.golang.org/protobuf/types/known/structpb" ) +// testRoleARN is a syntactically valid ARN used for role_arn test cases. +// All STS calls against it are handled by injected mocks — no real AWS +// account is contacted. +const testRoleARN = "arn:aws:iam::123456789012:role/BoundaryTestRole" + +// validAssumeRoleOutput returns a mock AssumeRoleOutput with synthetic +// temporary credentials for use with WithSTSAPIFunc. +func validAssumeRoleOutput() *sts.AssumeRoleOutput { + return &sts.AssumeRoleOutput{ + Credentials: &stsTypes.Credentials{ + AccessKeyId: aws.String("ASIAmockassumedkey"), + SecretAccessKey: aws.String("mockassumedsecret"), + SessionToken: aws.String("mockassumedsessiontoken"), + Expiration: aws.Time(time.Now().Add(time.Hour)), + }, + } +} + +// catalogAttrsStruct builds a catalog attributes structpb for the given +// region. roleArn is written only when non-empty. disableRotation must be +// set explicitly to avoid implicit rotation behaviour in tests. +func catalogAttrsStruct(region, roleArn string, disableRotation bool) *structpb.Struct { + fields := map[string]*structpb.Value{ + credential.ConstRegion: structpb.NewStringValue(region), + credential.ConstDisableCredentialRotation: structpb.NewBoolValue(disableRotation), + } + if roleArn != "" { + fields[credential.ConstRoleArn] = structpb.NewStringValue(roleArn) + } + return &structpb.Struct{Fields: fields} +} + func TestPluginOnCreateCatalogSuccess(t *testing.T) { tests := []struct { @@ -139,17 +173,13 @@ func TestPluginOnCreateCatalogSuccess(t *testing.T) { }, }, { + // role_arn set, no static keys. AssumeRole mock succeeds. + // Persisted secrets are empty — dynamic credentials are not stored. name: "usingAssumeRole", req: &pb.OnCreateCatalogRequest{ Catalog: &hostcatalogs.HostCatalog{ Attrs: &hostcatalogs.HostCatalog_Attributes{ - Attributes: &structpb.Struct{ - Fields: map[string]*structpb.Value{ - credential.ConstRegion: structpb.NewStringValue("us-east-1"), - credential.ConstRoleArn: structpb.NewStringValue("arn:0123:test:rolearn"), - credential.ConstDisableCredentialRotation: structpb.NewBoolValue(true), - }, - }, + Attributes: catalogAttrsStruct("us-east-1", testRoleARN, true), }, }, }, @@ -159,6 +189,13 @@ func TestPluginOnCreateCatalogSuccess(t *testing.T) { testMockEC2WithDescribeInstancesOutput(&ec2.DescribeInstancesOutput{}), )), }, + credOpts: []credential.AwsCredentialPersistedStateOption{ + credential.WithStateTestOpts([]awsutil.Option{ + awsutil.WithSTSAPIFunc(awsutil.NewMockSTS( + awsutil.WithAssumeRoleOutput(validAssumeRoleOutput()), + )), + }), + }, expRsp: &pb.OnCreateCatalogResponse{Persisted: &pb.HostCatalogPersisted{Secrets: &structpb.Struct{}}}, }, } @@ -316,6 +353,20 @@ func TestPluginOnUpdateCatalogSuccess(t *testing.T) { testMockEC2WithDescribeInstancesOutput(&ec2.DescribeInstancesOutput{}), )), }, + credOpts: []credential.AwsCredentialPersistedStateOption{ + credential.WithStateTestOpts([]awsutil.Option{ + awsutil.WithSTSAPIFunc(awsutil.NewMockSTS( + awsutil.WithAssumeRoleOutput(&sts.AssumeRoleOutput{ + Credentials: &stsTypes.Credentials{ + AccessKeyId: aws.String("ASIAfoobar"), + SecretAccessKey: aws.String("secretkeyfoobar"), + SessionToken: aws.String("sessiontokenfoobar"), + Expiration: aws.Time(time.Now().Add(time.Hour)), + }, + }), + )), + }), + }, expRsp: &pb.OnUpdateCatalogResponse{ Persisted: &pb.HostCatalogPersisted{Secrets: &structpb.Struct{}}, }, @@ -422,6 +473,16 @@ func TestPluginOnUpdateCatalogSuccess(t *testing.T) { credOpts: []credential.AwsCredentialPersistedStateOption{ credential.WithStateTestOpts([]awsutil.Option{ awsutil.WithIAMAPIFunc(awsutil.NewMockIAM()), + awsutil.WithSTSAPIFunc(awsutil.NewMockSTS( + awsutil.WithAssumeRoleOutput(&sts.AssumeRoleOutput{ + Credentials: &stsTypes.Credentials{ + AccessKeyId: aws.String("ASIAfoobar"), + SecretAccessKey: aws.String("secretkeyfoobar"), + SessionToken: aws.String("sessiontokenfoobar"), + Expiration: aws.Time(time.Now().Add(time.Hour)), + }, + }), + )), }), }, expRsp: &pb.OnUpdateCatalogResponse{ @@ -749,6 +810,35 @@ func TestPluginOnCreateCatalogErr(t *testing.T) { expectedErrContains: testDescribeInstancesError, expectedErrCode: codes.Unknown, }, + { + // BUG FIX: role_arn set, no static keys. AssumeRole mock returns + // an error. ValidateCreds now eagerly calls Retrieve() which forces + // sts:AssumeRole, so the error surfaces here at creation time. + // Previously this succeeded silently via the ambient credential chain. + name: "role_arn AssumeRole error — correctly rejected", + req: &pb.OnCreateCatalogRequest{ + Catalog: &hostcatalogs.HostCatalog{ + Attrs: &hostcatalogs.HostCatalog_Attributes{ + Attributes: catalogAttrsStruct("us-east-1", testRoleARN, true), + }, + }, + }, + credOpts: []credential.AwsCredentialPersistedStateOption{ + credential.WithStateTestOpts([]awsutil.Option{ + awsutil.WithSTSAPIFunc(awsutil.NewMockSTS( + awsutil.WithAssumeRoleError(errors.New("simulated AssumeRole failure")), + )), + }), + }, + catalogOpts: []awsCatalogPersistedStateOption{ + withTestEC2APIFunc(newTestMockEC2( + nil, + testMockEC2WithDescribeInstancesOutput(&ec2.DescribeInstancesOutput{}), + )), + }, + expectedErrContains: "simulated AssumeRole failure", + expectedErrCode: codes.Unknown, + }, } for _, tc := range cases { @@ -1037,6 +1127,16 @@ func TestPluginOnUpdateCatalogErr(t *testing.T) { }, }, }, + // ValidateCreds is called for DynamicAWS credentials before the dry-run + // DescribeInstances. Provide a mock STS that returns a successful AssumeRole + // so the validate step passes and execution reaches the EC2 mock. + credOpts: []credential.AwsCredentialPersistedStateOption{ + credential.WithStateTestOpts([]awsutil.Option{ + awsutil.WithSTSAPIFunc(awsutil.NewMockSTS( + awsutil.WithAssumeRoleOutput(validAssumeRoleOutput()), + )), + }), + }, catalogOpts: []awsCatalogPersistedStateOption{ withTestEC2APIFunc(newTestMockEC2( nil,