diff --git a/internal/errors/error.go b/internal/errors/error.go index 5e94b8f..f23e099 100644 --- a/internal/errors/error.go +++ b/internal/errors/error.go @@ -29,6 +29,11 @@ const ( // the owner. This error cannot be resolved by retrying the request. awsErrorAccessDenied = "AccessDenied" + // awsErrorUnauthorizedOperation is returned by EC2 when the caller is not + // authorized to perform the operation. This includes DryRun permission + // checks that fail (the counterpart to DryRunOperation success). + awsErrorUnauthorizedOperation = "UnauthorizedOperation" + // awsErrorInvalidAccessKeyId is returned when the credential does // not exist. This error will persist until the credentials attached // to the storage bucket is updated. This error can occur when the @@ -66,8 +71,26 @@ const ( // RequestTimeoutException is returned when an http request takes longer than allowed awsErrorRequestTimeoutException = "RequestTimeoutException" + + // dryRunOperationErrorCode is returned by AWS when a DryRun request would have + // succeeded. It is a success signal disguised as an API error (typically HTTP 412). + dryRunOperationErrorCode = "DryRunOperation" ) +// IsDryRunSuccess reports whether err is the AWS DryRunOperation success signal. +// A true result means the dry-run validated credentials, permissions, and parameters; +// it must not be treated as a failure. +func IsDryRunSuccess(err error) bool { + if err == nil { + return false + } + var apiErr smithy.APIError + if !errors.As(err, &apiErr) { + return false + } + return apiErr.ErrorCode() == dryRunOperationErrorCode +} + // InvalidArgumentError returns an grpc invalid argument status error. func InvalidArgumentError(msg string, f map[string]string) error { var fieldMsgs []string @@ -172,6 +195,8 @@ func ParseAWSError(op string, err error) (st *status.Status, permission *pb.Perm switch apiErr.ErrorCode() { case awsErrorAccessDenied: fallthrough + case awsErrorUnauthorizedOperation: + fallthrough case awsErrorInvalidAccessKeyId: fallthrough case awsErrorExpiredToken: diff --git a/internal/errors/error_test.go b/internal/errors/error_test.go index 4873001..c725dd0 100644 --- a/internal/errors/error_test.go +++ b/internal/errors/error_test.go @@ -300,6 +300,17 @@ func Test_ParseAWSError(t *testing.T) { CheckedAt: timestamppb.Now(), }, }, + { + name: "aws-request-unauthorized-operation", + err: TestAwsError(awsErrorUnauthorizedOperation, "You are not authorized to perform this operation."), + expectedStatusCode: codes.PermissionDenied, + expectedStatusMsg: "aws service unknown: invalid credentials: test", + expectedPermission: &pb.Permission{ + State: pb.StateType_STATE_TYPE_ERROR, + ErrorDetails: "You are not authorized to perform this operation.", + CheckedAt: timestamppb.Now(), + }, + }, { name: "aws-request-invalid-access-key-id", err: TestAwsError(awsErrorInvalidAccessKeyId, "The AWS Access Key Id you provided does not exist in our records."), @@ -390,6 +401,46 @@ func Test_ParseAWSError(t *testing.T) { } } +func Test_IsDryRunSuccess(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + err error + want bool + }{ + { + name: "nil", + want: false, + }, + { + name: "plain error", + err: fmt.Errorf("not an aws error"), + want: false, + }, + { + name: "other api error", + err: TestAwsError(awsErrorAccessDenied, "not authorized"), + want: false, + }, + { + name: "dry run operation", + err: TestAwsError(dryRunOperationErrorCode, "Request would have succeeded, but DryRun flag is set"), + want: true, + }, + { + name: "wrapped dry run operation", + err: fmt.Errorf("describe instances: %w", TestAwsError(dryRunOperationErrorCode, "Request would have succeeded, but DryRun flag is set")), + want: true, + }, + } + for _, tt := range cases { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, IsDryRunSuccess(tt.err)) + }) + } +} + func Test_BadRequestStatusf(t *testing.T) { require, assert := require.New(t), assert.New(t) err := BadRequestStatusf("test: %s", "hello world") diff --git a/plugin/service/host/plugin.go b/plugin/service/host/plugin.go index 64d2afa..feccd85 100644 --- a/plugin/service/host/plugin.go +++ b/plugin/service/host/plugin.go @@ -5,7 +5,6 @@ package host import ( "context" - goerrors "errors" "fmt" "strings" @@ -15,7 +14,6 @@ import ( "github.com/aws/aws-sdk-go-v2/service/ec2/types" "github.com/aws/aws-sdk-go-v2/service/sts" stsTypes "github.com/aws/aws-sdk-go-v2/service/sts/types" - "github.com/aws/smithy-go" "github.com/hashicorp/boundary-plugin-aws/internal/credential" "github.com/hashicorp/boundary-plugin-aws/internal/errors" pb "github.com/hashicorp/boundary/sdk/pbs/plugin" @@ -44,8 +42,6 @@ type hostSetQuery struct { OutputHosts []*pb.ListHostsResponseHost } -const defaultSessionName = "boundary-default" -const dryRunOperationErrorCode = "DryRunOperation" const errBuildDescribeInstancesInput = "error building DescribeInstances input: %s" const errDryRunFailed = "error performing DescribeInstances dry run: %s" @@ -567,10 +563,14 @@ func (p *HostPlugin) ListHosts(ctx context.Context, req *pb.ListHostsRequest) (* for i, query := range queries { output, err := checkHosts(ctx, catalogState, query.Input, opts, catalogAttributes.TargetAccount) if err != nil { - if catalogAttributes.TargetAccount != nil { - return nil, errors.BadRequestStatusf("unable to list hosts for host set %q: %s", query.Id, err) + // Mapped AWS failures are already gRPC statuses from checkHosts. + if _, ok := status.FromError(err); ok { + return nil, err } - return nil, errors.BadRequestStatusf("error retrieving host results for host set id %q: %s", query.Id, err) + if catalogAttributes.TargetAccount != nil { + return nil, status.Errorf(codes.Unknown, "unable to list hosts for host set %q: %v", query.Id, err) + } + return nil, status.Errorf(codes.Unknown, "error retrieving host results for host set id %q: %v", query.Id, err) } queries[i].Output = output @@ -584,7 +584,7 @@ func (p *HostPlugin) ListHosts(ctx context.Context, req *pb.ListHostsRequest) (* for _, instance := range reservation.Instances { host, err := awsInstanceToHost(instance, catalogAttributes) if err != nil { - return nil, errors.BadRequestStatusf("error processing host results for host set id %q: %s", query.Id, err) + return nil, status.Errorf(codes.Unknown, "error processing host results for host set id %q: %v", query.Id, err) } queries[i].OutputHosts = append(queries[i].OutputHosts, host) @@ -759,25 +759,31 @@ func checkHosts(ctx context.Context, // use the principal account to execute the DescribeInstances command ec2Client, err = principalCatalogState.EC2Client(ctx, opts...) if err != nil { + if st, _ := errors.ParseAWSError("EC2 client setup", err); st.Code() != codes.Unknown { + return nil, st.Err() + } return nil, err } } else { // use the target account to execute the DescribeInstances command ec2Client, err = ec2ClientForTarget(ctx, principalCatalogState, target, opts) if err != nil { + if _, ok := status.FromError(err); ok { + return nil, err + } return nil, fmt.Errorf("EC2 client setup failed for target account: %w", err) } } output, err := ec2Client.DescribeInstances(ctx, input) if err != nil { - var awsErr smithy.APIError - ok := goerrors.As(err, &awsErr) - if ok && awsErr.ErrorCode() == dryRunOperationErrorCode { - // successful DryRun validation, no response necessary, no error thrown + if errors.IsDryRunSuccess(err) { + // DryRunOperation: request would have succeeded; not a failure. return nil, nil } - + if st, _ := errors.ParseAWSError("DescribeInstances", err); st.Code() != codes.Unknown { + return nil, st.Err() + } if target != nil { return nil, fmt.Errorf("EC2 DescribeInstances failed for target account: %w", err) } @@ -800,6 +806,9 @@ func ec2ClientForTarget( // principal identity). principalCfg, err := principalCatalogState.GenerateCredentialChain(ctx) if err != nil { + if st, _ := errors.ParseAWSError("EC2 client setup for target account", err); st.Code() != codes.Unknown { + return nil, st.Err() + } return nil, fmt.Errorf("principal AWS configuration could not be loaded: %w", err) } if principalCfg == nil { @@ -813,9 +822,6 @@ func ec2ClientForTarget( provider := stscreds.NewAssumeRoleProvider(stsClient, target.RoleArn, func(o *stscreds.AssumeRoleOptions) { if target.RoleSessionName != "" { o.RoleSessionName = target.RoleSessionName - } else { - // STS may reject the AssumeRole request if session name is not provided. - o.RoleSessionName = defaultSessionName } if target.RoleExternalId != "" { o.ExternalID = &target.RoleExternalId diff --git a/plugin/service/host/plugin_test.go b/plugin/service/host/plugin_test.go index 060d5c9..34db9a1 100644 --- a/plugin/service/host/plugin_test.go +++ b/plugin/service/host/plugin_test.go @@ -18,6 +18,7 @@ import ( iamTypes "github.com/aws/aws-sdk-go-v2/service/iam/types" "github.com/google/go-cmp/cmp" "github.com/hashicorp/boundary-plugin-aws/internal/credential" + awserrors "github.com/hashicorp/boundary-plugin-aws/internal/errors" "github.com/hashicorp/boundary/sdk/pbs/controller/api/resources/hostcatalogs" "github.com/hashicorp/boundary/sdk/pbs/controller/api/resources/hostsets" pb "github.com/hashicorp/boundary/sdk/pbs/plugin" @@ -2299,7 +2300,7 @@ func TestPluginListHostsErr(t *testing.T) { )), }, expectedErrContains: fmt.Sprintf("error retrieving host results for host set id \"foobar\": %s", testDescribeInstancesError), - expectedErrCode: codes.InvalidArgument, + expectedErrCode: codes.Unknown, }, { name: "target attributes missing target_role_arn", @@ -2365,7 +2366,7 @@ func TestPluginListHostsErr(t *testing.T) { }), }, expectedErrContains: "unable to list hosts for host set \"foobar\": EC2 client setup failed for target account: test EC2 client error", - expectedErrCode: codes.InvalidArgument, + expectedErrCode: codes.Unknown, }, { name: "target DescribeInstances error", @@ -2393,7 +2394,7 @@ func TestPluginListHostsErr(t *testing.T) { )), }, expectedErrContains: "unable to list hosts for host set \"foobar\": EC2 DescribeInstances failed for target account: DescribeInstances error", - expectedErrCode: codes.InvalidArgument, + expectedErrCode: codes.Unknown, }, { name: "awsInstanceToHost error", @@ -2454,7 +2455,115 @@ func TestPluginListHostsErr(t *testing.T) { )), }, expectedErrContains: "error processing host results for host set id \"foobar\": response integrity error: missing instance id", - expectedErrCode: codes.InvalidArgument, + expectedErrCode: codes.Unknown, + }, + { + name: "DescribeInstances AccessDenied", + req: &pb.ListHostsRequest{ + Catalog: &hostcatalogs.HostCatalog{ + Attrs: &hostcatalogs.HostCatalog_Attributes{ + Attributes: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + credential.ConstRegion: structpb.NewStringValue("us-west-2"), + }, + }, + }, + }, + Persisted: testListHostsPersisted(), + Sets: []*hostsets.HostSet{ + testListHostsSet("foobar", "tag-key=foo"), + }, + }, + catalogOpts: []awsCatalogPersistedStateOption{ + withTestEC2APIFunc(newTestMockEC2( + nil, + testMockEC2WithDescribeInstancesError(awserrors.TestAwsError("AccessDenied", "not authorized to perform ec2:DescribeInstances")), + )), + }, + expectedErrContains: "invalid credentials", + expectedErrCode: codes.PermissionDenied, + }, + { + name: "DescribeInstances UnauthorizedOperation", + req: &pb.ListHostsRequest{ + Catalog: &hostcatalogs.HostCatalog{ + Attrs: &hostcatalogs.HostCatalog_Attributes{ + Attributes: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + credential.ConstRegion: structpb.NewStringValue("us-west-2"), + }, + }, + }, + }, + Persisted: testListHostsPersisted(), + Sets: []*hostsets.HostSet{ + testListHostsSet("foobar", "tag-key=foo"), + }, + }, + catalogOpts: []awsCatalogPersistedStateOption{ + withTestEC2APIFunc(newTestMockEC2( + nil, + testMockEC2WithDescribeInstancesError(awserrors.TestAwsError("UnauthorizedOperation", "You are not authorized to perform this operation.")), + )), + }, + expectedErrContains: "invalid credentials", + expectedErrCode: codes.PermissionDenied, + }, + { + name: "target DescribeInstances AccessDenied", + req: &pb.ListHostsRequest{ + Catalog: &hostcatalogs.HostCatalog{ + Attrs: &hostcatalogs.HostCatalog_Attributes{ + Attributes: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + credential.ConstRegion: structpb.NewStringValue("us-west-2"), + credential.ConstTargetRoleArn: structpb.NewStringValue("arn:aws:iam::222222222222:role/Target"), + credential.ConstTargetRegion: structpb.NewStringValue("eu-west-1"), + }, + }, + }, + }, + Persisted: testListHostsPersisted(), + Sets: []*hostsets.HostSet{ + testListHostsSet("foobar", "tag-key=foo"), + }, + }, + catalogOpts: []awsCatalogPersistedStateOption{ + withTestEC2APIFunc(newTestMockEC2( + nil, + testMockEC2WithDescribeInstancesError(awserrors.TestAwsError("AccessDenied", "not authorized to perform ec2:DescribeInstances")), + )), + }, + expectedErrContains: "invalid credentials", + expectedErrCode: codes.PermissionDenied, + }, + { + name: "target EC2 client AccessDenied", + req: &pb.ListHostsRequest{ + Catalog: &hostcatalogs.HostCatalog{ + Attrs: &hostcatalogs.HostCatalog_Attributes{ + Attributes: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + credential.ConstRegion: structpb.NewStringValue("us-west-2"), + credential.ConstTargetRoleArn: structpb.NewStringValue("arn:aws:iam::222222222222:role/Target"), + }, + }, + }, + }, + Persisted: testListHostsPersisted(), + Sets: []*hostsets.HostSet{ + testListHostsSet("foobar", "tag-key=foo"), + }, + }, + catalogOpts: []awsCatalogPersistedStateOption{ + withTestEC2APIFunc(func(...aws.Config) (EC2API, error) { + return nil, awserrors.TestAwsError("AccessDenied", "not authorized to assume role") + }), + }, + // ec2ClientForTarget returns testEC2APIFunc errors unparsed; checkHosts + // only preserves existing gRPC statuses, so this surfaces as Unknown. + expectedErrContains: "unable to list hosts for host set \"foobar\": EC2 client setup failed for target account: api error AccessDenied: not authorized to assume role", + expectedErrCode: codes.Unknown, }, }