fixup! feat(plugin): Implement Credential Rotation

hashicorp · Apr 3, 2024 · 352f3a3 · 352f3a3
1 parent 90d6438
commit 352f3a3
Show file tree

Hide file tree

Showing 2 changed files with 10 additions and 109 deletions.
diff --git a/plugin/service/storage/plugin.go b/plugin/service/storage/plugin.go
@@ -204,16 +204,8 @@ 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. 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.
-			rotatedSec, _, err := rotateCredentials(ctx, newBucket.GetBucketName(), nsa, sec)
-			if err != nil {
-				return nil, status.Errorf(codes.InvalidArgument, "failed to rotate minio credentials: %v", err)
-			}
-			sec = rotatedSec
+			// provides no new credentials. This is an error.
+			return nil, status.Errorf(codes.InvalidArgument, "credential rotation enabled with no new credentials provided")
 		}
 	}
 

diff --git a/plugin/service/storage/plugin_test.go b/plugin/service/storage/plugin_test.go
@@ -907,7 +907,7 @@ func TestOnUpdateStorageBucket(t *testing.T) {
 			errCode: codes.InvalidArgument,
 		},
 		{
-			name: "credRotationFailNoNewCreds",
+			name: "credRotationNoNewCreds",
 			req: &plugin.OnUpdateStorageBucketRequest{
 				CurrentBucket: &storagebuckets.StorageBucket{
 					BucketName: bucketName,
@@ -919,32 +919,12 @@ func TestOnUpdateStorageBucket(t *testing.T) {
 					},
 				},
 				Persisted: &storagebuckets.StorageBucketPersisted{
-					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),
-							},
-						}
-					}(),
+					Data: &structpb.Struct{
+						Fields: map[string]*structpb.Value{
+							ConstAccessKeyId:     structpb.NewStringValue(server.ServiceAccountAccessKeyId),
+							ConstSecretAccessKey: structpb.NewStringValue(server.ServiceAccountSecretAccessKey),
+						},
+					},
 				},
 				NewBucket: &storagebuckets.StorageBucket{
 					BucketName: bucketName,
@@ -956,7 +936,7 @@ func TestOnUpdateStorageBucket(t *testing.T) {
 					},
 				},
 			},
-			err:     "failed to rotate minio credentials: failed to create new minio service account: Access Denied.",
+			err:     "credential rotation enabled with no new credentials provided",
 			errCode: codes.InvalidArgument,
 		},
 		{
@@ -1111,77 +1091,6 @@ 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)