diff --git a/internal/credential/attributes.go b/internal/credential/attributes.go index 855df5e..d11ae77 100644 --- a/internal/credential/attributes.go +++ b/internal/credential/attributes.go @@ -79,7 +79,7 @@ func GetCredentialsConfig(secrets *structpb.Struct, attrs *CredentialAttributes, case accessKey != "" && secretKey != "" && attrs.RoleArn != "": badFields[fmt.Sprintf("secrets.%s", ConstAccessKeyId)] = "conflicts with role_arn value" badFields[fmt.Sprintf("secrets.%s", ConstSecretAccessKey)] = "conflicts with role_arn value" - badFields[fmt.Sprintf("secrets.%s", ConstRoleArn)] = "conflicts with access_key_id and secret_access_key values" + badFields[fmt.Sprintf("attributes.%s", ConstRoleArn)] = "conflicts with access_key_id and secret_access_key values" // static credential is missing it's secret_access_key case accessKey != "" && secretKey == "": badFields[fmt.Sprintf("secrets.%s", ConstSecretAccessKey)] = "missing required value" diff --git a/internal/credential/attributes_test.go b/internal/credential/attributes_test.go index 4718c1e..c778fd4 100644 --- a/internal/credential/attributes_test.go +++ b/internal/credential/attributes_test.go @@ -244,7 +244,7 @@ func TestGetCredentialsConfig(t *testing.T) { "foo": "bar", }, }, - expectedErrContains: "secrets.access_key_id: conflicts with role_arn value, secrets.role_arn: conflicts with access_key_id and secret_access_key values, secrets.secret_access_key: conflicts with role_arn value", + expectedErrContains: "attributes.role_arn: conflicts with access_key_id and secret_access_key values, secrets.access_key_id: conflicts with role_arn value, secrets.secret_access_key: conflicts with role_arn value", }, { name: "with dynamic credentials and no disable credential rotation", diff --git a/plugin/service/storage/plugin.go b/plugin/service/storage/plugin.go index 83cfca4..8e6a8c8 100644 --- a/plugin/service/storage/plugin.go +++ b/plugin/service/storage/plugin.go @@ -161,6 +161,28 @@ func (p *StoragePlugin) OnUpdateStorageBucket(ctx context.Context, req *pb.OnUpd // 1. the new bucket has secrets for static credentials, in which case we need to delete the old static credentials // 2. the new bucket has a different roleARN value for dynamic credentials if newBucket.GetSecrets() != nil || newStorageAttributes.RoleArn != oldStorageAttributes.RoleArn { + // Ensure the incoming credentials are valid for interaction with the S3 + // Bucket before replacing them. + newCredState, err := cred.NewAwsCredentialPersistedState( + append([]cred.AwsCredentialPersistedStateOption{ + cred.WithCredentialsConfig(updatedCredentials), + }, p.testCredStateOpts...)..., + ) + if err != nil { + return nil, status.Errorf(codes.InvalidArgument, "error setting up new credential persisted state: %s", err) + } + newStorageState, err := newAwsStoragePersistedState( + append([]awsStoragePersistedStateOption{ + withCredentials(newCredState), + }, p.testStorageStateOpts...)..., + ) + if err != nil { + return nil, status.Errorf(codes.InvalidArgument, "error loading persisted state: %s", err) + } + if err := dryRunValidation(ctx, newStorageState, newStorageAttributes, newBucket); err != nil { + return nil, status.Errorf(codes.InvalidArgument, "failed to validate incoming credentials: %s", err) + } + // Replace the existing credential state. // This checks the timestamp on the last rotation time as well // and deletes the credentials if we are managing them diff --git a/plugin/service/storage/plugin_test.go b/plugin/service/storage/plugin_test.go index 09eefbe..9f9a807 100644 --- a/plugin/service/storage/plugin_test.go +++ b/plugin/service/storage/plugin_test.go @@ -663,6 +663,16 @@ func TestStoragePlugin_OnUpdateStorageBucket(t *testing.T) { }, }, }, + storageOpts: []awsStoragePersistedStateOption{ + withTestS3APIFunc( + newTestMockS3( + nil, + testMockS3WithPutObjectOutput(&s3.PutObjectOutput{}), + testMockS3WithGetObjectOutput(&s3.GetObjectOutput{}), + testMockS3WithHeadObjectOutput(&s3.HeadObjectOutput{}), + ), + ), + }, credOpts: []credential.AwsCredentialPersistedStateOption{ credential.WithStateTestOpts([]awsutilv2.Option{ awsutilv2.WithIAMAPIFunc( @@ -933,7 +943,7 @@ func TestStoragePlugin_OnUpdateStorageBucket(t *testing.T) { ), }, expectedErrContains: "head object failed oops", - expectedErrCode: codes.Internal, + expectedErrCode: codes.InvalidArgument, }, { name: "dynamic to dynamic credentials success", @@ -1014,7 +1024,7 @@ func TestStoragePlugin_OnUpdateStorageBucket(t *testing.T) { ), }, expectedErrContains: "put object failed oops", - expectedErrCode: codes.Internal, + expectedErrCode: codes.InvalidArgument, }, { name: "success static non rotated to dynamic credentials", @@ -1090,6 +1100,16 @@ func TestStoragePlugin_OnUpdateStorageBucket(t *testing.T) { }, }, }, + storageOpts: []awsStoragePersistedStateOption{ + withTestS3APIFunc( + newTestMockS3( + nil, + testMockS3WithPutObjectOutput(&s3.PutObjectOutput{}), + testMockS3WithGetObjectOutput(&s3.GetObjectOutput{}), + testMockS3WithHeadObjectOutput(&s3.HeadObjectOutput{}), + ), + ), + }, credOpts: []credential.AwsCredentialPersistedStateOption{ credential.WithStateTestOpts([]awsutilv2.Option{ awsutilv2.WithIAMAPIFunc( @@ -1195,7 +1215,7 @@ func TestStoragePlugin_OnUpdateStorageBucket(t *testing.T) { ), }, expectedErrContains: "put object fail oops", - expectedErrCode: codes.Internal, + expectedErrCode: codes.InvalidArgument, }, { name: "success dynamic to static non-rotated credentials", @@ -1275,6 +1295,16 @@ func TestStoragePlugin_OnUpdateStorageBucket(t *testing.T) { Data: &structpb.Struct{Fields: map[string]*structpb.Value{}}, }, }, + storageOpts: []awsStoragePersistedStateOption{ + withTestS3APIFunc( + newTestMockS3( + nil, + testMockS3WithPutObjectOutput(&s3.PutObjectOutput{}), + testMockS3WithGetObjectOutput(&s3.GetObjectOutput{}), + testMockS3WithHeadObjectOutput(&s3.HeadObjectOutput{}), + ), + ), + }, credOpts: []credential.AwsCredentialPersistedStateOption{ credential.WithStateTestOpts([]awsutilv2.Option{ awsutilv2.WithSTSAPIFunc(