feat(plugin): Implement Credential Rotation

plugin/service/storage/attributes.go

@@ -6,6 +6,7 @@ package storage
 import (
 	"fmt"
 	"strings"
+	"time"
 
 	"github.com/hashicorp/boundary-plugin-minio/internal/errors"
 	"github.com/hashicorp/boundary-plugin-minio/internal/values"
@@ -15,10 +16,12 @@ import (
 )
 
 const (
-	ConstEndpointUrl     = "endpoint_url"
+	ConstEndpointUrl               = "endpoint_url"
-	ConstAccessKeyId     = "access_key_id"
+	ConstAccessKeyId               = "access_key_id"
-	ConstSecretAccessKey = "secret_access_key"
+	ConstSecretAccessKey           = "secret_access_key"
-	ConstRegion          = "region"
+	ConstRegion                    = "region"
+	ConstDisableCredentialRotation = "disable_credential_rotation"
+	ConstLastRotatedTime           = "creds_last_rotated_time"
 )
 
 type StorageAttributes struct {
@@ -31,6 +34,11 @@ type StorageAttributes struct {
 	// UseSSL determines if the MinIO SDK will use SSL when communicating with
 	// the server. This field is determined based on the endpoint URL prefix.
 	UseSSL bool
+	// DisableCredentialRotation is used to configure whether this plugin should
+	// manage the MinIO credentials or not. By default, this field is set to
+	// false. See the plugin's OnCreateStorageBucket, OnUpdateStorageBucket and
+	// OnDeleteStorageBucket for how we use this.
+	DisableCredentialRotation bool
 }
 
 type StorageSecrets struct {
@@ -40,6 +48,10 @@ type StorageSecrets struct {
 	// SecretAccessKey is the MinIO Secret Access Key. This field is required
 	// and comes from user input.
 	SecretAccessKey string
+	// LastRotatedTime is the time the secrets contained in this object were
+	// last rotated. Will be set to the zero-value (time.Time{}) if the
+	// credentials haven't been rotated.
+	LastRotatedTime time.Time
 }
 
 // AsMap returns a map StorageAttributes's secret fields as a map.
@@ -47,6 +59,17 @@ func (sa *StorageSecrets) AsMap() map[string]any {
 	return map[string]any{
 		ConstAccessKeyId:     sa.AccessKeyId,
 		ConstSecretAccessKey: sa.SecretAccessKey,
+		ConstLastRotatedTime: sa.LastRotatedTime.Format(time.RFC3339Nano),
+	}
+}
+
+// Clone returns a new StorageSecrets object with the same state as the incoming
+// one.
+func (sec *StorageSecrets) Clone() *StorageSecrets {
+	return &StorageSecrets{
+		AccessKeyId:     sec.AccessKeyId,
+		SecretAccessKey: sec.SecretAccessKey,
+		LastRotatedTime: sec.LastRotatedTime,
 	}
 }
 
@@ -83,6 +106,12 @@ func getStorageAttributes(inAttributes *structpb.Struct) (*StorageAttributes, er
 	}
 	delete(unknownAttrFields, ConstRegion)
 
+	disableCredentialRotation, err := values.GetBoolValue(inAttributes, ConstDisableCredentialRotation, false)
+	if err != nil {
+		badFields[fmt.Sprintf("attributes.%s", ConstDisableCredentialRotation)] = err.Error()
+	}
+	delete(unknownAttrFields, ConstDisableCredentialRotation)
+
 	for s := range unknownAttrFields {
 		badFields[fmt.Sprintf("attributes.%s", s)] = "unrecognized field"
 	}
@@ -91,9 +120,10 @@ func getStorageAttributes(inAttributes *structpb.Struct) (*StorageAttributes, er
 		return nil, errors.InvalidArgumentError("invalid or unrecognized attributes found", badFields)
 	}
 	return &StorageAttributes{
-		EndpointUrl: endpointUrl,
+		EndpointUrl:               endpointUrl,
-		Region:      region,
+		Region:                    region,
-		UseSSL:      useSSL,
+		UseSSL:                    useSSL,
+		DisableCredentialRotation: disableCredentialRotation,
 	}, nil
 }
 
@@ -118,6 +148,12 @@ func getStorageSecrets(inSecrets *structpb.Struct) (*StorageSecrets, error) {
 	}
 	delete(unknownSecretFields, ConstSecretAccessKey)
 
+	lastRotated, err := values.GetTimeValue(inSecrets, ConstLastRotatedTime)
+	if err != nil {
+		badFields[fmt.Sprintf("secrets.%s", ConstLastRotatedTime)] = err.Error()
+	}
+	delete(unknownSecretFields, ConstLastRotatedTime)
+
 	for s := range unknownSecretFields {
 		badFields[fmt.Sprintf("secrets.%s", s)] = "unrecognized field"
 	}
@@ -128,5 +164,6 @@ func getStorageSecrets(inSecrets *structpb.Struct) (*StorageSecrets, error) {
 	return &StorageSecrets{
 		AccessKeyId:     accessKeyId,
 		SecretAccessKey: secretAccessKey,
+		LastRotatedTime: lastRotated,
 	}, nil
 }

plugin/service/storage/attributes_test.go

@@ -5,6 +5,7 @@ package storage
 
 import (
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/require"
 	"google.golang.org/protobuf/types/known/structpb"
@@ -89,18 +90,30 @@ func TestGetStorageAttributes(t *testing.T) {
 			},
 			expErrMsg: "attributes.endpoint_url: missing required value \"endpoint_url\"",
 		},
+		{
+			name: "disableCredRotationBadType",
+			inAttributes: &structpb.Struct{
+				Fields: map[string]*structpb.Value{
+					ConstEndpointUrl:               structpb.NewStringValue("http://bar"),
+					ConstDisableCredentialRotation: structpb.NewStringValue("notbool"),
+				},
+			},
+			expErrMsg: "attributes.disable_credential_rotation: unexpected type for value \"disable_credential_rotation\": want bool, got string",
+		},
 		{
 			name: "successWithNoSSL",
 			inAttributes: &structpb.Struct{
 				Fields: map[string]*structpb.Value{
-					ConstEndpointUrl: structpb.NewStringValue("http://foo.bar"),
+					ConstEndpointUrl:               structpb.NewStringValue("http://foo.bar"),
-					ConstRegion:      structpb.NewStringValue("us-east-1"),
+					ConstRegion:                    structpb.NewStringValue("us-east-1"),
+					ConstDisableCredentialRotation: structpb.NewBoolValue(true),
 				},
 			},
 			expStorageAttributes: &StorageAttributes{
-				EndpointUrl: "foo.bar",
+				EndpointUrl:               "foo.bar",
-				Region:      "us-east-1",
+				Region:                    "us-east-1",
-				UseSSL:      false,
+				UseSSL:                    false,
+				DisableCredentialRotation: true,
 			},
 		},
 		{
@@ -215,6 +228,17 @@ func TestGetStorageSecrets(t *testing.T) {
 			},
 			expErrMsg: "secrets.secret_access_key: unexpected type for value \"secret_access_key\": want string, got bool",
 		},
+		{
+			name: "lastRotatedBadType",
+			inSecrets: &structpb.Struct{
+				Fields: map[string]*structpb.Value{
+					ConstAccessKeyId:     structpb.NewStringValue("foo"),
+					ConstSecretAccessKey: structpb.NewStringValue("bar"),
+					ConstLastRotatedTime: structpb.NewStringValue("nottime"),
+				},
+			},
+			expErrMsg: "secrets.creds_last_rotated_time: could not parse time in value \"creds_last_rotated_time\"",
+		},
 		{
 			name: "unknownSecret",
 			inSecrets: &structpb.Struct{
@@ -239,6 +263,21 @@ func TestGetStorageSecrets(t *testing.T) {
 				SecretAccessKey: "bar",
 			},
 		},
+		{
+			name: "successWithLastRotatedTime",
+			inSecrets: &structpb.Struct{
+				Fields: map[string]*structpb.Value{
+					ConstAccessKeyId:     structpb.NewStringValue("foo"),
+					ConstSecretAccessKey: structpb.NewStringValue("bar"),
+					ConstLastRotatedTime: structpb.NewStringValue(time.Date(2024, 01, 02, 03, 04, 05, 06, time.UTC).Format(time.RFC3339Nano)),
+				},
+			},
+			expStorageSecrets: &StorageSecrets{
+				AccessKeyId:     "foo",
+				SecretAccessKey: "bar",
+				LastRotatedTime: time.Date(2024, 01, 02, 03, 04, 05, 06, time.UTC),
+			},
+		},
 	}
 
 	for _, tt := range tests {
@@ -268,10 +307,12 @@ func TestStorageSecretsAsMap(t *testing.T) {
 			in: &StorageSecrets{
 				AccessKeyId:     "access_key_id_value",
 				SecretAccessKey: "secret_access_key_value",
+				LastRotatedTime: time.Date(2024, 01, 02, 03, 04, 05, 06, time.UTC),
 			},
 			expMap: map[string]any{
-				"access_key_id":     "access_key_id_value",
+				"access_key_id":           "access_key_id_value",
-				"secret_access_key": "secret_access_key_value",
+				"secret_access_key":       "secret_access_key_value",
+				"creds_last_rotated_time": "2024-01-02T03:04:05.000000006Z",
 			},
 		},
 	}
@@ -283,3 +324,18 @@ func TestStorageSecretsAsMap(t *testing.T) {
 		})
 	}
 }
+
+func TestTestStorageSecretsClone(t *testing.T) {
+	now := time.Now()
+	in := &StorageSecrets{
+		AccessKeyId:     "foo",
+		SecretAccessKey: "bar",
+		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)
+}

plugin/service/storage/plugin.go

@@ -14,6 +14,7 @@ import (
 	"os"
 	"path"
 	"strings"
+	"sync"
 
 	"github.com/google/uuid"
 	"github.com/hashicorp/boundary/sdk/pbs/controller/api/resources/storagebuckets"
@@ -64,6 +65,16 @@ func (sp *StoragePlugin) OnCreateStorageBucket(ctx context.Context, req *pb.OnCr
 		return nil, status.Errorf(codes.InvalidArgument, "failed to ensure service account: %v", err)
 	}
 
+	deleteOldCredsFn := func() error { return nil }
+	if !sa.DisableCredentialRotation {
+		newSec, fn, err := rotateCredentials(ctx, bucket.GetBucketName(), sa, sec)
+		if err != nil {
+			return nil, status.Errorf(codes.InvalidArgument, "failed to rotate minio credentials: %v", err)
+		}
+		sec = newSec
+		deleteOldCredsFn = fn
+	}
+
 	cl, err := minio.New(sa.EndpointUrl, &minio.Options{
 		Creds:  credentials.NewStaticV4(sec.AccessKeyId, sec.SecretAccessKey, ""),
 		Secure: sa.UseSSL,
@@ -82,6 +93,13 @@ func (sp *StoragePlugin) OnCreateStorageBucket(ctx context.Context, req *pb.OnCr
 	if err != nil {
 		return nil, status.Errorf(codes.Internal, "failed to convert storage attributes to protobuf struct: %v", err)
 	}
+
+	// Given that at this point everything is OK with the bucket, we shouldn't
+	// let a failure to delete the old credentials fail the entire process. If
+	// we ever establish a way to log non-critical errors in the plugin, log
+	// this.
+	_ = deleteOldCredsFn()
+
 	return &pb.OnCreateStorageBucketResponse{
 		Persisted: &storagebuckets.StorageBucketPersisted{Data: persistedData},
 	}, nil
@@ -96,24 +114,22 @@ func (sp *StoragePlugin) OnUpdateStorageBucket(ctx context.Context, req *pb.OnUp
 	if newBucket.BucketName == "" {
 		return nil, status.Error(codes.InvalidArgument, "new bucketName is required")
 	}
+
 	oldBucket := req.GetCurrentBucket()
 	if oldBucket == nil {
 		return nil, status.Error(codes.InvalidArgument, "current bucket is required")
 	}
-
-	// current attributes
 	osa, err := getStorageAttributes(oldBucket.GetAttributes())
 	if err != nil {
 		return nil, err
 	}
 
-	// new attributes and secrets
 	nsa, err := getStorageAttributes(newBucket.GetAttributes())
 	if err != nil {
 		return nil, err
 	}
 
-	sec, err := getStorageSecrets(newBucket.GetSecrets())
+	sec, err := getStorageSecrets(req.GetPersisted().GetData())
 	if err != nil {
 		return nil, err
 	}
@@ -129,8 +145,69 @@ func (sp *StoragePlugin) OnUpdateStorageBucket(ctx context.Context, req *pb.OnUp
 		return nil, status.Errorf(codes.InvalidArgument, "cannot update attribute EndpointUrl")
 	}
 
-	if err = ensureServiceAccount(ctx, nsa, sec); err != nil {
+	if nsa.DisableCredentialRotation && newBucket.GetSecrets() == nil {
-		return nil, status.Errorf(codes.InvalidArgument, "failed to ensure service account: %s", err)
+		if !sec.LastRotatedTime.IsZero() {
+			return nil, status.Error(codes.InvalidArgument, "cannot disable rotation for already-rotated credentials")
+		}
+	}
+
+	deleteRotatedCredsFn := func() error { return nil }
+	deleteOldPersistedCredsFn := func() error { return nil }
+	if newBucket.GetSecrets() != nil {
+		// Preemptively check if we're managing the current persisted
+		// credentials and schedule them for later deletion if we are, since in
+		// both rotation and non-rotation cases below, we'll be replacing these.
+		if !sec.LastRotatedTime.IsZero() {
+			sc := sec.Clone()
+			deleteOldPersistedCredsFn = sync.OnceValue(func() error {
+				cl, err := newMadminClient(nsa, sc)
+				if err != nil {
+					return fmt.Errorf("failed to create new minio admin client: %w", err)
+				}
+				err = cl.DeleteServiceAccount(ctx, sc.AccessKeyId)
+				if err != nil {
+					return fmt.Errorf("failed to delete minio service account: %w", err)
+				}
+
+				return nil
+			})
+		}
+
+		newSec, err := getStorageSecrets(newBucket.GetSecrets())
+		if err != nil {
+			return nil, err
+		}
+
+		err = ensureServiceAccount(ctx, nsa, newSec)
+		if err != nil {
+			return nil, status.Errorf(codes.InvalidArgument, "failed to ensure service account: %s", err)
+		}
+
+		if !nsa.DisableCredentialRotation {
+			// We have new incoming credentials and we're also rotating,
+			// therefore we use incoming credentials to create a new service
+			// 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)
+			if err != nil {
+				return nil, status.Errorf(codes.InvalidArgument, "failed to rotate minio credentials: %v", err)
+			}
+			sec = rotatedSec
+			deleteRotatedCredsFn = fn
+		} else {
+			// We have new incoming credentials but we're not rotating. Set them
+			// directly for validation below. The existing persisted credentials
+			// have already been scheduled for deletion above (if applicable),
+			// so we don't need to do it here.
+			sec = newSec
+		}
+	} 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")
+		}
 	}
 
 	cl, err := minio.New(nsa.EndpointUrl, &minio.Options{
@@ -151,14 +228,46 @@ func (sp *StoragePlugin) OnUpdateStorageBucket(ctx context.Context, req *pb.OnUp
 		return nil, status.Errorf(codes.InvalidArgument, "failed to convert storage attributes to protobuf struct: %s", err)
 	}
 
+	// Given that at this point everything is OK with the bucket, we shouldn't
+	// let a failure to delete credentials fail the entire process. If we ever
+	// establish a way to log non-critical errors in the plugin, log this.
+	_ = deleteRotatedCredsFn()
+	_ = deleteOldPersistedCredsFn()
+
 	return &pb.OnUpdateStorageBucketResponse{
 		Persisted: &storagebuckets.StorageBucketPersisted{Data: persisted},
 	}, nil
 }
 
 // OnDeleteStorageBucket is a hook that runs when a storage bucket is deleted.
-// Since this plugin manages no state at the moment, this function is a no-op.
 func (sp *StoragePlugin) OnDeleteStorageBucket(ctx context.Context, req *pb.OnDeleteStorageBucketRequest) (*pb.OnDeleteStorageBucketResponse, error) {
+	bucket := req.GetBucket()
+	if bucket == nil {
+		return nil, status.Error(codes.InvalidArgument, "no storage bucket information found")
+	}
+
+	sec, err := getStorageSecrets(req.GetPersisted().GetData())
+	if err != nil {
+		return nil, err
+	}
+
+	if !sec.LastRotatedTime.IsZero() {
+		sa, err := getStorageAttributes(bucket.GetAttributes())
+		if err != nil {
+			return nil, err
+		}
+
+		ac, err := newMadminClient(sa, sec)
+		if err != nil {
+			return nil, status.Errorf(codes.InvalidArgument, "failed to create minio admin client: %v", err)
+		}
+
+		err = ac.DeleteServiceAccount(ctx, sec.AccessKeyId)
+		if err != nil {
+			return nil, status.Errorf(codes.Unknown, "failed to delete minio service account: %v", err)
+		}
+	}
+
 	return &pb.OnDeleteStorageBucketResponse{}, nil
 }
 

plugin/service/storage/rotation.go

@@ -73,9 +73,9 @@ func rotateCredentials(ctx context.Context, bucketName string, sa *StorageAttrib
 		LastRotatedTime: time.Now(),
 	}
 
-	inCl, newCl := inSec.Clone(), newSec.Clone()
+	inCl := inSec.Clone()
 	return newSec, sync.OnceValue(func() error {
-		ac, err := newMadminClient(sa, newCl)
+		ac, err := newMadminClient(sa, inCl)
 		if err != nil {
 			return fmt.Errorf("failed to create minio admin client: %w", err)
 		}

plugin/service/storage/rotation_test.go

@@ -21,13 +21,13 @@ func TestRotateCredentials(t *testing.T) {
 
 	tests := []struct {
 		name       string
-		ss         *StorageSecrets
+		inSec      *StorageSecrets
 		expErrMsg  string
 		credDelErr bool
 	}{
 		{
 			name: "notUsingServiceAccount",
-			ss: &StorageSecrets{
+			inSec: &StorageSecrets{
 				AccessKeyId:     server.RootUsername,
 				SecretAccessKey: server.RootPassword,
 			},
@@ -35,7 +35,7 @@ func TestRotateCredentials(t *testing.T) {
 		},
 		{
 			name: "addServiceAccountFail",
-			ss: func() *StorageSecrets {
+			inSec: func() *StorageSecrets {
 				creds, err := server.AdminClient.AddServiceAccount(ctx, madmin.AddServiceAccountReq{
 					Policy: json.RawMessage(`
 					{
@@ -63,7 +63,7 @@ func TestRotateCredentials(t *testing.T) {
 		},
 		{
 			name: "credentialDeletionFail",
-			ss: func() *StorageSecrets {
+			inSec: func() *StorageSecrets {
 				creds, err := server.AdminClient.AddServiceAccount(ctx, madmin.AddServiceAccountReq{})
 				require.NoError(t, err)
 
@@ -76,7 +76,7 @@ func TestRotateCredentials(t *testing.T) {
 		},
 		{
 			name: "success",
-			ss: func() *StorageSecrets {
+			inSec: func() *StorageSecrets {
 				creds, err := server.AdminClient.AddServiceAccount(ctx, madmin.AddServiceAccountReq{})
 				require.NoError(t, err)
 
@@ -95,26 +95,26 @@ func TestRotateCredentials(t *testing.T) {
 				UseSSL:      false,
 			}
 
-			ss, delFn, err := rotateCredentials(context.Background(), bucketName, sa, tt.ss)
+			outSec, delFn, err := rotateCredentials(context.Background(), bucketName, sa, tt.inSec)
 			if tt.expErrMsg != "" {
 				require.ErrorContains(t, err, tt.expErrMsg)
-				require.Nil(t, ss)
+				require.Nil(t, outSec)
 				require.Nil(t, delFn)
 				return
 			}
 
 			require.NoError(t, err)
-			require.NotNil(t, ss)
+			require.NotNil(t, outSec)
 			require.NotNil(t, delFn)
-			require.NotEqual(t, ss.AccessKeyId, tt.ss.AccessKeyId)
+			require.NotEqual(t, outSec.AccessKeyId, tt.inSec.AccessKeyId)
-			require.NotEqual(t, ss.SecretAccessKey, tt.ss.SecretAccessKey)
+			require.NotEqual(t, outSec.SecretAccessKey, tt.inSec.SecretAccessKey)
-			require.NotEmpty(t, ss.LastRotatedTime)
+			require.NotEmpty(t, outSec.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.ss.AccessKeyId)
+			_, err = server.AdminClient.InfoServiceAccount(ctx, tt.inSec.AccessKeyId)
 			require.NoError(t, err)
 
 			if tt.credDelErr {
@@ -122,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.ss.AccessKeyId)
+				_, err := server.AdminClient.InfoServiceAccount(ctx, tt.inSec.AccessKeyId)
 				require.NoError(t, err)
 				return
 			} else {
@@ -130,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.ss.AccessKeyId)
+			_, err = server.AdminClient.InfoServiceAccount(ctx, tt.inSec.AccessKeyId)
 			require.ErrorContains(t, err, "service account is not found")
 		})
 	}