Merge remote-tracking branch 'origin/hugo-cred-rotation' into test-delete-cred-rotation

plugin/service/storage/README.md

@@ -13,7 +13,6 @@ boundary storage-buckets create \
   -worker-filter '"minio-access" in "/tags/type"' \
   -attr endpoint_url="https://my-minio-instance.dev/s3-api" \
   -attr region=REGION \
-  -attr disable_credential_rotation=true \
   -secret access_key_id='KEY' \
   -secret secret_access_key='SECRET'
 ```
@@ -25,11 +24,6 @@ The following `attributes` are valid on a MinIO storage bucket resource:
 - `endpoint_url` (string, required): Fully-qualified endpoint pointing to a
   MinIO S3 API.
 - `region` (string, optional): The region to configure the storage bucket for.
-- `disable_credential_rotation` (bool, optional): Controls whether the plugin
-  will rotate the incoming credentials and manage a new MinIO service account.
-  If this attribute is set to `false`, or not provided, the plugin will rotate
-  the incoming credentials, using them to create a new MinIO service account,
-  then delete the incoming credentials.
 
 ### Secrets
 

plugin/service/storage/attributes.go

@@ -65,11 +65,11 @@ func (sa *StorageSecrets) AsMap() map[string]any {
 
 // Clone returns a new StorageSecrets object with the same state as the incoming
 // one.
-func (sec *StorageSecrets) Clone() *StorageSecrets {
+func (ss *StorageSecrets) Clone() *StorageSecrets {
 	return &StorageSecrets{
-		AccessKeyId:     sec.AccessKeyId,
-		SecretAccessKey: sec.SecretAccessKey,
-		LastRotatedTime: sec.LastRotatedTime,
+		AccessKeyId:     ss.AccessKeyId,
+		SecretAccessKey: ss.SecretAccessKey,
+		LastRotatedTime: ss.LastRotatedTime,
 	}
 }
 

plugin/service/storage/attributes_test.go

@@ -333,9 +333,9 @@ func TestTestStorageSecretsClone(t *testing.T) {
 		LastRotatedTime: now,
 	}
 
-	secCl := in.Clone()
-	require.Equal(t, in.AccessKeyId, secCl.AccessKeyId)
-	require.Equal(t, in.AccessKeyId, secCl.AccessKeyId)
-	require.Equal(t, in.LastRotatedTime, secCl.LastRotatedTime)
-	require.NotSame(t, in, secCl)
+	ss := in.Clone()
+	require.Equal(t, in.AccessKeyId, ss.AccessKeyId)
+	require.Equal(t, in.AccessKeyId, ss.AccessKeyId)
+	require.Equal(t, in.LastRotatedTime, ss.LastRotatedTime)
+	require.NotSame(t, in, ss)
 }

plugin/service/storage/plugin.go

@@ -189,11 +189,11 @@ func (sp *StoragePlugin) OnUpdateStorageBucket(ctx context.Context, req *pb.OnUp
 			// account, then delete the incoming credentials. The existing
 			// persisted credentials have already been scheduled for deletion
 			// above (if applicable), so we don't need to do it here.
-			rotatedSec, fn, err := rotateCredentials(ctx, newBucket.GetBucketName(), nsa, newSec)
+			ss, fn, err := rotateCredentials(ctx, newBucket.GetBucketName(), nsa, newSec)
 			if err != nil {
 				return nil, status.Errorf(codes.InvalidArgument, "failed to rotate minio credentials: %v", err)
 			}
-			sec = rotatedSec
+			sec = ss
 			deleteRotatedCredsFn = fn
 		} else {
 			// We have new incoming credentials but we're not rotating. Set them
@@ -205,8 +205,16 @@ func (sp *StoragePlugin) OnUpdateStorageBucket(ctx context.Context, req *pb.OnUp
 	} else {
 		if osa.DisableCredentialRotation && !nsa.DisableCredentialRotation {
 			// Handle the case where the user enables credential rotation but
-			// provides no new credentials. This is an error.
-			return nil, status.Errorf(codes.InvalidArgument, "credential rotation enabled with no new credentials provided")
+			// provides no new credentials. In this case, we use the existing
+			// persisted credentials to generate a new service account. The
+			// persisted credentials are, by definition, not being managed by us
+			// (credential rotation was disabled beforehand), so don't schedule
+			// them for deletion.
+			ss, _, err := rotateCredentials(ctx, newBucket.GetBucketName(), nsa, sec)
+			if err != nil {
+				return nil, status.Errorf(codes.InvalidArgument, "failed to rotate minio credentials: %v", err)
+			}
+			sec = ss
 		}
 	}
 
@@ -573,6 +581,7 @@ func (sp *StoragePlugin) DeleteObjects(ctx context.Context, req *pb.DeleteObject
 		Recursive: true,
 	}) {
 		if obj.Err != nil {
+			// check if context cancelled ?
 			return nil, status.Errorf(codes.Unknown, "error iterating minio bucket contents: %v", err)
 		}
 		objects = append(objects, obj)
@@ -590,6 +599,7 @@ func (sp *StoragePlugin) DeleteObjects(ctx context.Context, req *pb.DeleteObject
 	removed := 0
 	for res := range cl.RemoveObjectsWithResult(ctx, bucket.GetBucketName(), emitter, minio.RemoveObjectsOptions{}) {
 		if res.Err != nil {
+			// check if context cancelled ?
 			return nil, status.Errorf(codes.Unknown, "error deleting minio object(s): %v", err)
 		}
 		removed++

plugin/service/storage/plugin_test.go

@@ -907,7 +907,7 @@ func TestOnUpdateStorageBucket(t *testing.T) {
 			errCode: codes.InvalidArgument,
 		},
 		{
-			name: "credRotationNoNewCreds",
+			name: "credRotationFailNoNewCreds",
 			req: &plugin.OnUpdateStorageBucketRequest{
 				CurrentBucket: &storagebuckets.StorageBucket{
 					BucketName: bucketName,
@@ -919,12 +919,32 @@ func TestOnUpdateStorageBucket(t *testing.T) {
 					},
 				},
 				Persisted: &storagebuckets.StorageBucketPersisted{
-					Data: &structpb.Struct{
-						Fields: map[string]*structpb.Value{
-							ConstAccessKeyId:     structpb.NewStringValue(server.ServiceAccountAccessKeyId),
-							ConstSecretAccessKey: structpb.NewStringValue(server.ServiceAccountSecretAccessKey),
-						},
-					},
+					Data: func() *structpb.Struct {
+						creds, err := server.AdminClient.AddServiceAccount(ctx, madmin.AddServiceAccountReq{
+							Policy: json.RawMessage(`
+							{
+								"Statement": [
+									{
+										"Action": [
+											"admin:CreateServiceAccount",
+											"admin:RemoveServiceAccount"
+										],
+										"Effect": "Deny"
+									}
+								],
+								"Version": "2012-10-17"
+							}
+							`),
+						})
+						require.NoError(t, err)
+
+						return &structpb.Struct{
+							Fields: map[string]*structpb.Value{
+								ConstAccessKeyId:     structpb.NewStringValue(creds.AccessKey),
+								ConstSecretAccessKey: structpb.NewStringValue(creds.SecretKey),
+							},
+						}
+					}(),
 				},
 				NewBucket: &storagebuckets.StorageBucket{
 					BucketName: bucketName,
@@ -936,7 +956,7 @@ func TestOnUpdateStorageBucket(t *testing.T) {
 					},
 				},
 			},
-			err:     "credential rotation enabled with no new credentials provided",
+			err:     "failed to rotate minio credentials: failed to create new minio service account: Access Denied.",
 			errCode: codes.InvalidArgument,
 		},
 		{
@@ -1091,6 +1111,77 @@ func TestOnUpdateStorageBucket_SuccessDisableCredRotationAndNewCreds(t *testing.
 	require.Len(t, lsa.Accounts, 2) // Creds in `server` + newCreds.
 }
 
+func TestOnUpdateStorageBucket_SuccessEnableCredRotationNoNewCreds(t *testing.T) {
+	ctx := context.Background()
+	server := internaltest.NewMinioServer(t)
+
+	bucketName := "test"
+	err := server.Client.MakeBucket(ctx, bucketName, minio.MakeBucketOptions{})
+	require.NoError(t, err)
+
+	persistedCreds, err := server.AdminClient.AddServiceAccount(ctx, madmin.AddServiceAccountReq{})
+	require.NoError(t, err)
+
+	req := &plugin.OnUpdateStorageBucketRequest{
+		CurrentBucket: &storagebuckets.StorageBucket{
+			BucketName: bucketName,
+			Attributes: &structpb.Struct{
+				Fields: map[string]*structpb.Value{
+					ConstEndpointUrl:               structpb.NewStringValue("http://" + server.ApiAddr),
+					ConstDisableCredentialRotation: structpb.NewBoolValue(true),
+				},
+			},
+		},
+		Persisted: &storagebuckets.StorageBucketPersisted{
+			Data: &structpb.Struct{
+				Fields: map[string]*structpb.Value{
+					ConstAccessKeyId:     structpb.NewStringValue(persistedCreds.AccessKey),
+					ConstSecretAccessKey: structpb.NewStringValue(persistedCreds.SecretKey),
+				},
+			},
+		},
+		NewBucket: &storagebuckets.StorageBucket{
+			BucketName: bucketName,
+			Attributes: &structpb.Struct{
+				Fields: map[string]*structpb.Value{
+					ConstEndpointUrl:               structpb.NewStringValue("http://" + server.ApiAddr),
+					ConstDisableCredentialRotation: structpb.NewBoolValue(false),
+				},
+			},
+		},
+	}
+
+	// `disable_cred_rotation` disabled -> enable and provide no new creds.
+	// This should use the existing persisted credentials to generate new ones,
+	// then use those. The old persisted credentials should not be deleted.
+	sp := new(StoragePlugin)
+	rsp, err := sp.OnUpdateStorageBucket(ctx, req)
+	require.NoError(t, err)
+	require.NotNil(t, req)
+
+	outAccessKeyId, err := values.GetStringValue(rsp.GetPersisted().GetData(), ConstAccessKeyId, true)
+	require.NoError(t, err)
+	require.NotEqual(t, persistedCreds.AccessKey, outAccessKeyId)
+
+	outSecretAccessKey, err := values.GetStringValue(rsp.GetPersisted().GetData(), ConstSecretAccessKey, true)
+	require.NoError(t, err)
+	require.NotEqual(t, persistedCreds.SecretKey, outSecretAccessKey)
+
+	lastRotated, err := values.GetTimeValue(rsp.GetPersisted().GetData(), ConstLastRotatedTime)
+	require.NoError(t, err)
+	require.False(t, lastRotated.IsZero())
+
+	_, err = server.AdminClient.InfoServiceAccount(ctx, persistedCreds.AccessKey)
+	require.NoError(t, err)
+
+	_, err = server.AdminClient.InfoServiceAccount(ctx, outAccessKeyId)
+	require.NoError(t, err)
+
+	lsa, err := server.AdminClient.ListServiceAccounts(ctx, server.RootUsername)
+	require.NoError(t, err)
+	require.Len(t, lsa.Accounts, 3) // Creds in `server` + persistedCreds + newly rotated creds.
+}
+
 func TestOnUpdateStorageBucket_SuccessEnableCredRotationWithNewCreds(t *testing.T) {
 	ctx := context.Background()
 	server := internaltest.NewMinioServer(t)

plugin/service/storage/rotation.go

@@ -1,6 +1,3 @@
-// Copyright (c) HashiCorp, Inc.
-// SPDX-License-Identifier: MPL-2.0
-
 package storage
 
 import (
@@ -76,9 +73,9 @@ func rotateCredentials(ctx context.Context, bucketName string, sa *StorageAttrib
 		LastRotatedTime: time.Now(),
 	}
 
-	inCl := inSec.Clone()
+	inCl, newCl := inSec.Clone(), newSec.Clone()
 	return newSec, sync.OnceValue(func() error {
-		ac, err := newMadminClient(sa, inCl)
+		ac, err := newMadminClient(sa, newCl)
 		if err != nil {
 			return fmt.Errorf("failed to create minio admin client: %w", err)
 		}

plugin/service/storage/rotation_test.go

@@ -1,6 +1,3 @@
-// Copyright (c) HashiCorp, Inc.
-// SPDX-License-Identifier: MPL-2.0
-
 package storage
 
 import (
@@ -24,13 +21,13 @@ func TestRotateCredentials(t *testing.T) {
 
 	tests := []struct {
 		name       string
-		inSec      *StorageSecrets
+		ss         *StorageSecrets
 		expErrMsg  string
 		credDelErr bool
 	}{
 		{
 			name: "notUsingServiceAccount",
-			inSec: &StorageSecrets{
+			ss: &StorageSecrets{
 				AccessKeyId:     server.RootUsername,
 				SecretAccessKey: server.RootPassword,
 			},
@@ -38,7 +35,7 @@ func TestRotateCredentials(t *testing.T) {
 		},
 		{
 			name: "addServiceAccountFail",
-			inSec: func() *StorageSecrets {
+			ss: func() *StorageSecrets {
 				creds, err := server.AdminClient.AddServiceAccount(ctx, madmin.AddServiceAccountReq{
 					Policy: json.RawMessage(`
 					{
@@ -66,7 +63,7 @@ func TestRotateCredentials(t *testing.T) {
 		},
 		{
 			name: "credentialDeletionFail",
-			inSec: func() *StorageSecrets {
+			ss: func() *StorageSecrets {
 				creds, err := server.AdminClient.AddServiceAccount(ctx, madmin.AddServiceAccountReq{})
 				require.NoError(t, err)
 
@@ -79,7 +76,7 @@ func TestRotateCredentials(t *testing.T) {
 		},
 		{
 			name: "success",
-			inSec: func() *StorageSecrets {
+			ss: func() *StorageSecrets {
 				creds, err := server.AdminClient.AddServiceAccount(ctx, madmin.AddServiceAccountReq{})
 				require.NoError(t, err)
 
@@ -98,26 +95,26 @@ func TestRotateCredentials(t *testing.T) {
 				UseSSL:      false,
 			}
 
-			outSec, delFn, err := rotateCredentials(context.Background(), bucketName, sa, tt.inSec)
+			ss, delFn, err := rotateCredentials(context.Background(), bucketName, sa, tt.ss)
 			if tt.expErrMsg != "" {
 				require.ErrorContains(t, err, tt.expErrMsg)
-				require.Nil(t, outSec)
+				require.Nil(t, ss)
 				require.Nil(t, delFn)
 				return
 			}
 
 			require.NoError(t, err)
-			require.NotNil(t, outSec)
+			require.NotNil(t, ss)
 			require.NotNil(t, delFn)
-			require.NotEqual(t, outSec.AccessKeyId, tt.inSec.AccessKeyId)
-			require.NotEqual(t, outSec.SecretAccessKey, tt.inSec.SecretAccessKey)
-			require.NotEmpty(t, outSec.LastRotatedTime)
+			require.NotEqual(t, ss.AccessKeyId, tt.ss.AccessKeyId)
+			require.NotEqual(t, ss.SecretAccessKey, tt.ss.SecretAccessKey)
+			require.NotEmpty(t, ss.LastRotatedTime)
 
 			// Ensure old creds still exist since the callback hasn't been
 			// called. We then make the delete call and finally assert that the
 			// credential was removed if there was no error when attempting to
 			// delete.
-			_, err = server.AdminClient.InfoServiceAccount(ctx, tt.inSec.AccessKeyId)
+			_, err = server.AdminClient.InfoServiceAccount(ctx, tt.ss.AccessKeyId)
 			require.NoError(t, err)
 
 			if tt.credDelErr {
@@ -125,7 +122,7 @@ func TestRotateCredentials(t *testing.T) {
 				require.ErrorContains(t, delFn(), "failed to delete minio service account")
 				require.ErrorContains(t, delFn(), "failed to delete minio service account") // 2nd call should return the same state (due to sync.Once).
 
-				_, err := server.AdminClient.InfoServiceAccount(ctx, tt.inSec.AccessKeyId)
+				_, err := server.AdminClient.InfoServiceAccount(ctx, tt.ss.AccessKeyId)
 				require.NoError(t, err)
 				return
 			} else {
@@ -133,7 +130,7 @@ func TestRotateCredentials(t *testing.T) {
 				require.NoError(t, delFn()) // 2nd call should return the same state (due to sync.Once).
 			}
 
-			_, err = server.AdminClient.InfoServiceAccount(ctx, tt.inSec.AccessKeyId)
+			_, err = server.AdminClient.InfoServiceAccount(ctx, tt.ss.AccessKeyId)
 			require.ErrorContains(t, err, "service account is not found")
 		})
 	}