From 8b87e8bb6a60a087d0ec81fda7598d08060ec2c1 Mon Sep 17 00:00:00 2001 From: Hugo <10965479+hugoghx@users.noreply.github.com> Date: Fri, 22 Mar 2024 15:20:11 +0000 Subject: [PATCH] feat(plugin): Implement Credential Rotation --- plugin/service/storage/attributes.go | 51 +- plugin/service/storage/attributes_test.go | 70 +- plugin/service/storage/plugin.go | 131 ++- plugin/service/storage/plugin_test.go | 995 +++++++++++++++++++++- 4 files changed, 1206 insertions(+), 41 deletions(-) diff --git a/plugin/service/storage/attributes.go b/plugin/service/storage/attributes.go index 92accbd..c6b149f 100644 --- a/plugin/service/storage/attributes.go +++ b/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" - ConstAccessKeyId = "access_key_id" - ConstSecretAccessKey = "secret_access_key" - ConstRegion = "region" + ConstEndpointUrl = "endpoint_url" + ConstAccessKeyId = "access_key_id" + ConstSecretAccessKey = "secret_access_key" + 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 (ss *StorageSecrets) Clone() *StorageSecrets { + return &StorageSecrets{ + AccessKeyId: ss.AccessKeyId, + SecretAccessKey: ss.SecretAccessKey, + LastRotatedTime: ss.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, - Region: region, - UseSSL: useSSL, + EndpointUrl: endpointUrl, + Region: region, + 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 } diff --git a/plugin/service/storage/attributes_test.go b/plugin/service/storage/attributes_test.go index c48716d..d7ddb50 100644 --- a/plugin/service/storage/attributes_test.go +++ b/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"), - ConstRegion: structpb.NewStringValue("us-east-1"), + ConstEndpointUrl: structpb.NewStringValue("http://foo.bar"), + ConstRegion: structpb.NewStringValue("us-east-1"), + ConstDisableCredentialRotation: structpb.NewBoolValue(true), }, }, expStorageAttributes: &StorageAttributes{ - EndpointUrl: "foo.bar", - Region: "us-east-1", - UseSSL: false, + EndpointUrl: "foo.bar", + Region: "us-east-1", + 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", - "secret_access_key": "secret_access_key_value", + "access_key_id": "access_key_id_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, + } + + 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) +} diff --git a/plugin/service/storage/plugin.go b/plugin/service/storage/plugin.go index f36a238..6e79f05 100644 --- a/plugin/service/storage/plugin.go +++ b/plugin/service/storage/plugin.go @@ -13,6 +13,7 @@ import ( "io" "os" "path" + "sync" "github.com/google/uuid" "github.com/hashicorp/boundary/sdk/pbs/controller/api/resources/storagebuckets" @@ -63,6 +64,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, @@ -81,6 +92,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 @@ -95,24 +113,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 } @@ -128,8 +144,77 @@ 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 { - return nil, status.Errorf(codes.InvalidArgument, "failed to ensure service account: %s", err) + if nsa.DisableCredentialRotation && newBucket.GetSecrets() == nil { + 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. + 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 = ss + 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. 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 + } } cl, err := minio.New(nsa.EndpointUrl, &minio.Options{ @@ -150,14 +235,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 } diff --git a/plugin/service/storage/plugin_test.go b/plugin/service/storage/plugin_test.go index 973198f..77e4778 100644 --- a/plugin/service/storage/plugin_test.go +++ b/plugin/service/storage/plugin_test.go @@ -8,15 +8,18 @@ import ( "bytes" "context" "crypto/sha256" + "encoding/json" "fmt" "io" "os" "path" "testing" + "time" "github.com/google/go-cmp/cmp" "github.com/google/uuid" internaltest "github.com/hashicorp/boundary-plugin-minio/internal/testing" + "github.com/hashicorp/boundary-plugin-minio/internal/values" "github.com/hashicorp/boundary/sdk/pbs/controller/api/resources/storagebuckets" "github.com/hashicorp/boundary/sdk/pbs/plugin" pb "github.com/hashicorp/boundary/sdk/pbs/plugin" @@ -188,11 +191,11 @@ func TestOnCreateStorageBucket(t *testing.T) { require.NoError(t, err) tests := []struct { - name string - minioClFn func(t *testing.T) *minio.Client - req *plugin.OnCreateStorageBucketRequest - expRsp *plugin.OnCreateStorageBucketResponse - expErrMsg string + name string + req *plugin.OnCreateStorageBucketRequest + expRsp *plugin.OnCreateStorageBucketResponse + expCredRotation bool + expErrMsg string }{ { name: "nilBucket", @@ -258,7 +261,8 @@ func TestOnCreateStorageBucket(t *testing.T) { BucketName: bucketName, Attributes: &structpb.Struct{ Fields: map[string]*structpb.Value{ - ConstEndpointUrl: structpb.NewStringValue("http://" + server.ApiAddr), + ConstEndpointUrl: structpb.NewStringValue("http://" + server.ApiAddr), + ConstDisableCredentialRotation: structpb.NewBoolValue(true), }, }, Secrets: &structpb.Struct{ @@ -279,13 +283,55 @@ func TestOnCreateStorageBucket(t *testing.T) { expErrMsg: "failed to verify provided minio environment: failed to put object", }, { - name: "success", + name: "credRotationFail", + req: func() *plugin.OnCreateStorageBucketRequest { + 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 &plugin.OnCreateStorageBucketRequest{ + Bucket: &storagebuckets.StorageBucket{ + BucketName: bucketName, + Attributes: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + ConstEndpointUrl: structpb.NewStringValue("http://" + server.ApiAddr), + ConstDisableCredentialRotation: structpb.NewBoolValue(false), + }, + }, + Secrets: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + ConstAccessKeyId: structpb.NewStringValue(creds.AccessKey), + ConstSecretAccessKey: structpb.NewStringValue(creds.SecretKey), + }, + }, + }, + } + }(), + expErrMsg: "failed to rotate minio credentials: failed to create new minio service account: Access Denied", + }, + { + name: "successNoRotation", req: &plugin.OnCreateStorageBucketRequest{ Bucket: &storagebuckets.StorageBucket{ BucketName: bucketName, Attributes: &structpb.Struct{ Fields: map[string]*structpb.Value{ - ConstEndpointUrl: structpb.NewStringValue("http://" + server.ApiAddr), + ConstEndpointUrl: structpb.NewStringValue("http://" + server.ApiAddr), + ConstDisableCredentialRotation: structpb.NewBoolValue(true), }, }, Secrets: &structpb.Struct{ @@ -307,6 +353,41 @@ func TestOnCreateStorageBucket(t *testing.T) { }, }, }, + { + name: "successWithRotation", + req: func() *plugin.OnCreateStorageBucketRequest { + creds, err := server.AdminClient.AddServiceAccount(ctx, madmin.AddServiceAccountReq{}) + require.NoError(t, err) + + return &plugin.OnCreateStorageBucketRequest{ + Bucket: &storagebuckets.StorageBucket{ + BucketName: bucketName, + Attributes: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + ConstEndpointUrl: structpb.NewStringValue("http://" + server.ApiAddr), + ConstDisableCredentialRotation: structpb.NewBoolValue(false), + }, + }, + Secrets: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + ConstAccessKeyId: structpb.NewStringValue(creds.AccessKey), + ConstSecretAccessKey: structpb.NewStringValue(creds.SecretKey), + }, + }, + }, + } + }(), + expRsp: &plugin.OnCreateStorageBucketResponse{ + Persisted: &storagebuckets.StorageBucketPersisted{ + Data: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + // Determined during test runtime. + }, + }, + }, + }, + expCredRotation: true, + }, } for _, tt := range tests { @@ -321,7 +402,39 @@ func TestOnCreateStorageBucket(t *testing.T) { require.NoError(t, err) require.NotNil(t, rsp) - require.Empty(t, cmp.Diff(tt.expRsp, rsp, protocmp.Transform())) + + if tt.expCredRotation { + inAccessKeyId, err := values.GetStringValue(tt.req.Bucket.GetSecrets(), ConstAccessKeyId, true) + require.NoError(t, err) + + inSecretAccessKey, err := values.GetStringValue(tt.req.Bucket.GetSecrets(), ConstSecretAccessKey, true) + require.NoError(t, err) + + rspAccessKeyId, err := values.GetStringValue(rsp.GetPersisted().GetData(), ConstAccessKeyId, true) + require.NoError(t, err) + require.NotEqual(t, inAccessKeyId, rspAccessKeyId) + + rspSecretAccessKey, err := values.GetStringValue(rsp.GetPersisted().GetData(), ConstSecretAccessKey, true) + require.NoError(t, err) + require.NotEqual(t, inSecretAccessKey, rspSecretAccessKey) + + rspLastRotatedTime, err := values.GetTimeValue(rsp.GetPersisted().GetData(), ConstLastRotatedTime) + require.NoError(t, err) + require.False(t, rspLastRotatedTime.IsZero()) + + _, err = server.AdminClient.InfoServiceAccount(ctx, inAccessKeyId) + require.ErrorContains(t, err, "specified service account is not found") + + _, err = server.AdminClient.InfoServiceAccount(ctx, rspAccessKeyId) + require.NoError(t, err) + } else { + require.Empty(t, cmp.Diff(tt.expRsp, rsp, protocmp.Transform(), + cmp.FilterPath(func(p cmp.Path) bool { + step, ok := p.Last().(cmp.MapIndex) + return ok && step.Key().String() == ConstLastRotatedTime + }, cmp.Ignore()), + )) + } }) } } @@ -394,7 +507,7 @@ func TestOnUpdateStorageBucket(t *testing.T) { errCode: codes.InvalidArgument, }, { - name: "nilNewBucketSecrets", + name: "errorReadingNewBucketSecrets", req: &plugin.OnUpdateStorageBucketRequest{ NewBucket: &storagebuckets.StorageBucket{ BucketName: "foo", @@ -403,20 +516,30 @@ func TestOnUpdateStorageBucket(t *testing.T) { ConstEndpointUrl: structpb.NewStringValue("http://" + server.ApiAddr), }, }, + Secrets: &structpb.Struct{}, }, CurrentBucket: &storagebuckets.StorageBucket{ + BucketName: "foo", Attributes: &structpb.Struct{ Fields: map[string]*structpb.Value{ ConstEndpointUrl: structpb.NewStringValue("http://" + server.ApiAddr), }, }, }, + Persisted: &storagebuckets.StorageBucketPersisted{ + Data: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + ConstAccessKeyId: structpb.NewStringValue("foo"), + ConstSecretAccessKey: structpb.NewStringValue("bar"), + }, + }, + }, }, err: "empty secrets input", errCode: codes.InvalidArgument, }, { - name: "errorReadingAttributes", + name: "errorReadingNewBucketAttributes", req: &plugin.OnUpdateStorageBucketRequest{ NewBucket: &storagebuckets.StorageBucket{ BucketName: "foo", @@ -435,7 +558,7 @@ func TestOnUpdateStorageBucket(t *testing.T) { errCode: codes.InvalidArgument, }, { - name: "errorReadingSecrets", + name: "errorReadingPersistedSecrets", req: &plugin.OnUpdateStorageBucketRequest{ NewBucket: &storagebuckets.StorageBucket{ BucketName: "foo", @@ -482,6 +605,14 @@ 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), + }, + }, + }, }, err: "cannot update attribute BucketName", errCode: codes.InvalidArgument, @@ -512,6 +643,14 @@ 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), + }, + }, + }, }, err: "cannot update attribute BucketPrefix", errCode: codes.InvalidArgument, @@ -542,6 +681,14 @@ 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), + }, + }, + }, }, err: "cannot update attribute Region", errCode: codes.InvalidArgument, @@ -571,10 +718,52 @@ 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), + }, + }, + }, }, err: "cannot update attribute EndpointUrl", errCode: codes.InvalidArgument, }, + { + name: "disableCredRotationOnRotatedCreds", + req: &plugin.OnUpdateStorageBucketRequest{ + NewBucket: &storagebuckets.StorageBucket{ + BucketName: bucketName, + Attributes: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + ConstEndpointUrl: structpb.NewStringValue("http://" + server.ApiAddr), + ConstDisableCredentialRotation: structpb.NewBoolValue(true), + }, + }, + }, + CurrentBucket: &storagebuckets.StorageBucket{ + BucketName: bucketName, + Attributes: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + ConstEndpointUrl: structpb.NewStringValue("http://" + server.ApiAddr), + ConstDisableCredentialRotation: structpb.NewBoolValue(false), + }, + }, + }, + Persisted: &storagebuckets.StorageBucketPersisted{ + Data: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + ConstAccessKeyId: structpb.NewStringValue(server.RootUsername), + ConstSecretAccessKey: structpb.NewStringValue(server.RootPassword), + ConstLastRotatedTime: structpb.NewStringValue(time.Now().Format(time.RFC3339Nano)), + }, + }, + }, + }, + err: "cannot disable rotation for already-rotated credentials", + errCode: codes.InvalidArgument, + }, { name: "usingNonServiceAccount", req: &plugin.OnUpdateStorageBucketRequest{ @@ -619,7 +808,8 @@ func TestOnUpdateStorageBucket(t *testing.T) { BucketName: bucketName, Attributes: &structpb.Struct{ Fields: map[string]*structpb.Value{ - ConstEndpointUrl: structpb.NewStringValue("http://" + server.ApiAddr), + ConstEndpointUrl: structpb.NewStringValue("http://" + server.ApiAddr), + ConstDisableCredentialRotation: structpb.NewBoolValue(true), }, }, Secrets: &structpb.Struct{ @@ -657,13 +847,127 @@ func TestOnUpdateStorageBucket(t *testing.T) { errCode: codes.InvalidArgument, }, { - name: "success", + name: "credRotationFailWithNewCreds", + 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(false), + }, + }, + }, + Persisted: &storagebuckets.StorageBucketPersisted{ + Data: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + ConstAccessKeyId: structpb.NewStringValue(server.ServiceAccountAccessKeyId), + ConstSecretAccessKey: structpb.NewStringValue(server.ServiceAccountSecretAccessKey), + ConstLastRotatedTime: structpb.NewStringValue(time.Now().Format(time.RFC3339Nano)), + }, + }, + }, + NewBucket: &storagebuckets.StorageBucket{ + BucketName: bucketName, + Attributes: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + ConstEndpointUrl: structpb.NewStringValue("http://" + server.ApiAddr), + ConstDisableCredentialRotation: structpb.NewBoolValue(false), + }, + }, + Secrets: 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), + }, + } + }(), + }, + }, + err: "failed to rotate minio credentials: failed to create new minio service account: Access Denied", + errCode: codes.InvalidArgument, + }, + { + name: "credRotationFailNoNewCreds", + 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: 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, + Attributes: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + ConstEndpointUrl: structpb.NewStringValue("http://" + server.ApiAddr), + ConstDisableCredentialRotation: structpb.NewBoolValue(false), + }, + }, + }, + }, + err: "failed to rotate minio credentials: failed to create new minio service account: Access Denied.", + errCode: codes.InvalidArgument, + }, + { + name: "successNoRotation", req: &plugin.OnUpdateStorageBucketRequest{ NewBucket: &storagebuckets.StorageBucket{ BucketName: bucketName, Attributes: &structpb.Struct{ Fields: map[string]*structpb.Value{ - ConstEndpointUrl: structpb.NewStringValue("http://" + server.ApiAddr), + ConstEndpointUrl: structpb.NewStringValue("http://" + server.ApiAddr), + ConstDisableCredentialRotation: structpb.NewBoolValue(true), }, }, Secrets: &structpb.Struct{ @@ -717,16 +1021,667 @@ func TestOnUpdateStorageBucket(t *testing.T) { require.NoError(err) require.NotNil(res) - require.EqualValues(tt.expected, res) + require.Empty(cmp.Diff(tt.expected, res, protocmp.Transform(), + cmp.FilterPath(func(p cmp.Path) bool { + step, ok := p.Last().(cmp.MapIndex) + return ok && step.Key().String() == ConstLastRotatedTime + }, cmp.Ignore()), + )) }) } } -func TestOnDeleteStorageBucket(t *testing.T) { - sp := new(StoragePlugin) - rsp, err := sp.OnDeleteStorageBucket(context.Background(), &plugin.OnDeleteStorageBucketRequest{}) +func TestOnUpdateStorageBucket_SuccessDisableCredRotationAndNewCreds(t *testing.T) { + ctx := context.Background() + server := internaltest.NewMinioServer(t) + + bucketName := "test" + err := server.Client.MakeBucket(ctx, bucketName, minio.MakeBucketOptions{}) require.NoError(t, err) - require.NotNil(t, rsp) + + persistedCreds, err := server.AdminClient.AddServiceAccount(ctx, madmin.AddServiceAccountReq{}) + require.NoError(t, err) + newCreds, 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(false), + }, + }, + }, + Persisted: &storagebuckets.StorageBucketPersisted{ + Data: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + ConstAccessKeyId: structpb.NewStringValue(persistedCreds.AccessKey), + ConstSecretAccessKey: structpb.NewStringValue(persistedCreds.SecretKey), + ConstLastRotatedTime: structpb.NewStringValue(time.Now().Format(time.RFC3339Nano)), + }, + }, + }, + NewBucket: &storagebuckets.StorageBucket{ + BucketName: bucketName, + Attributes: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + ConstEndpointUrl: structpb.NewStringValue("http://" + server.ApiAddr), + ConstDisableCredentialRotation: structpb.NewBoolValue(true), + }, + }, + Secrets: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + ConstAccessKeyId: structpb.NewStringValue(newCreds.AccessKey), + ConstSecretAccessKey: structpb.NewStringValue(newCreds.SecretKey), + }, + }, + }, + } + + // `disable_cred_rotation` enabled -> disable and provide new creds. + // This should delete the existing creds and set the new ones to be used + // directly. + 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.Equal(t, newCreds.AccessKey, outAccessKeyId) + + outSecretAccessKey, err := values.GetStringValue(rsp.GetPersisted().GetData(), ConstSecretAccessKey, true) + require.NoError(t, err) + require.Equal(t, newCreds.SecretKey, outSecretAccessKey) + + lastRotated, err := values.GetTimeValue(rsp.GetPersisted().GetData(), ConstLastRotatedTime) + require.NoError(t, err) + require.True(t, lastRotated.IsZero()) + + _, err = server.AdminClient.InfoServiceAccount(ctx, persistedCreds.AccessKey) + require.ErrorContains(t, err, "specified service account is not found") + + _, err = server.AdminClient.InfoServiceAccount(ctx, newCreds.AccessKey) + require.NoError(t, err) + + lsa, err := server.AdminClient.ListServiceAccounts(ctx, server.RootUsername) + require.NoError(t, err) + 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) + + 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) + + newCreds, 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), + }, + }, + Secrets: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + ConstAccessKeyId: structpb.NewStringValue(newCreds.AccessKey), + ConstSecretAccessKey: structpb.NewStringValue(newCreds.SecretKey), + }, + }, + }, + } + + // `disable_cred_rotation` disabled -> enable and provide new creds. + // This should use the incoming credentials to generate new ones, then + // delete the incoming ones and use the generated ones. 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) + require.NotEqual(t, newCreds.AccessKey, outAccessKeyId) + + outSecretAccessKey, err := values.GetStringValue(rsp.GetPersisted().GetData(), ConstSecretAccessKey, true) + require.NoError(t, err) + require.NotEqual(t, persistedCreds.SecretKey, outSecretAccessKey) + require.NotEqual(t, newCreds.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, newCreds.AccessKey) + require.ErrorContains(t, err, "specified service account is not found") + + _, 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_SuccessNoCredRotationChangeDisabledNewCreds(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) + + newCreds, err := server.AdminClient.AddServiceAccount(ctx, madmin.AddServiceAccountReq{}) + require.NoError(t, err) + + bucketAttrs := &structpb.Struct{ + Fields: map[string]*structpb.Value{ + ConstEndpointUrl: structpb.NewStringValue("http://" + server.ApiAddr), + ConstDisableCredentialRotation: structpb.NewBoolValue(true), + }, + } + req := &plugin.OnUpdateStorageBucketRequest{ + CurrentBucket: &storagebuckets.StorageBucket{ + BucketName: bucketName, + Attributes: bucketAttrs, + }, + 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: bucketAttrs, + Secrets: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + ConstAccessKeyId: structpb.NewStringValue(newCreds.AccessKey), + ConstSecretAccessKey: structpb.NewStringValue(newCreds.SecretKey), + }, + }, + }, + } + + // Don't touch disable cred rotation (disabled) and provide new credentials. + // Should set the new credentials to be used. 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.Equal(t, newCreds.AccessKey, outAccessKeyId) + + outSecretAccessKey, err := values.GetStringValue(rsp.GetPersisted().GetData(), ConstSecretAccessKey, true) + require.NoError(t, err) + require.Equal(t, newCreds.SecretKey, outSecretAccessKey) + + _, err = server.AdminClient.InfoServiceAccount(ctx, persistedCreds.AccessKey) + require.NoError(t, err) + + _, err = server.AdminClient.InfoServiceAccount(ctx, newCreds.AccessKey) + 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 + newCreds +} + +func TestOnUpdateStorageBucket_SuccessNoCredRotationChangeEnabledNewCreds(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) + + newCreds, err := server.AdminClient.AddServiceAccount(ctx, madmin.AddServiceAccountReq{}) + require.NoError(t, err) + + bucketAttrs := &structpb.Struct{ + Fields: map[string]*structpb.Value{ + ConstEndpointUrl: structpb.NewStringValue("http://" + server.ApiAddr), + ConstDisableCredentialRotation: structpb.NewBoolValue(false), + }, + } + req := &plugin.OnUpdateStorageBucketRequest{ + CurrentBucket: &storagebuckets.StorageBucket{ + BucketName: bucketName, + Attributes: bucketAttrs, + }, + Persisted: &storagebuckets.StorageBucketPersisted{ + Data: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + ConstAccessKeyId: structpb.NewStringValue(persistedCreds.AccessKey), + ConstSecretAccessKey: structpb.NewStringValue(persistedCreds.SecretKey), + ConstLastRotatedTime: structpb.NewStringValue(time.Now().Format(time.RFC3339Nano)), + }, + }, + }, + NewBucket: &storagebuckets.StorageBucket{ + BucketName: bucketName, + Attributes: bucketAttrs, + Secrets: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + ConstAccessKeyId: structpb.NewStringValue(newCreds.AccessKey), + ConstSecretAccessKey: structpb.NewStringValue(newCreds.SecretKey), + }, + }, + }, + } + + // Don't touch disable cred rotation (enabled) and provide new credentials. + // Should use the incoming credentials to generate new ones, then delete the + // incoming ones and use the generated ones. The old persisted credentials + // should also 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) + require.NotEqual(t, newCreds.AccessKey, outAccessKeyId) + + outSecretAccessKey, err := values.GetStringValue(rsp.GetPersisted().GetData(), ConstSecretAccessKey, true) + require.NoError(t, err) + require.NotEqual(t, persistedCreds.SecretKey, outSecretAccessKey) + require.NotEqual(t, newCreds.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.ErrorContains(t, err, "specified service account is not found") + + _, err = server.AdminClient.InfoServiceAccount(ctx, newCreds.AccessKey) + require.ErrorContains(t, err, "specified service account is not found") + + _, 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, 2) // Creds in `server` + newly rotated creds. +} + +func TestOnCreateStorageBucket_NoChangesCredRotationDisabled(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) + + bucketAttrs := &structpb.Struct{ + Fields: map[string]*structpb.Value{ + ConstEndpointUrl: structpb.NewStringValue("http://" + server.ApiAddr), + ConstDisableCredentialRotation: structpb.NewBoolValue(true), + }, + } + req := &plugin.OnUpdateStorageBucketRequest{ + CurrentBucket: &storagebuckets.StorageBucket{ + BucketName: bucketName, + Attributes: bucketAttrs, + }, + Persisted: &storagebuckets.StorageBucketPersisted{ + Data: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + ConstAccessKeyId: structpb.NewStringValue(persistedCreds.AccessKey), + ConstSecretAccessKey: structpb.NewStringValue(persistedCreds.SecretKey), + ConstLastRotatedTime: structpb.NewStringValue(time.Time{}.Format(time.RFC3339Nano)), + }, + }, + }, + NewBucket: &storagebuckets.StorageBucket{ + BucketName: bucketName, + Attributes: bucketAttrs, + }, + } + + // No changes with credential rotation disabled. This should change nothing. + sp := new(StoragePlugin) + rsp, err := sp.OnUpdateStorageBucket(ctx, req) + require.NoError(t, err) + require.NotNil(t, req) + + require.Empty(t, cmp.Diff(req.GetPersisted(), rsp.GetPersisted(), protocmp.Transform())) + + _, err = server.AdminClient.InfoServiceAccount(ctx, persistedCreds.AccessKey) + require.NoError(t, err) + + lsa, err := server.AdminClient.ListServiceAccounts(ctx, server.RootUsername) + require.NoError(t, err) + require.Len(t, lsa.Accounts, 2) // Creds in `server` + persistedCreds +} + +func TestOnCreateStorageBucket_NoChangesCredRotationEnabled(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) + + bucketAttrs := &structpb.Struct{ + Fields: map[string]*structpb.Value{ + ConstEndpointUrl: structpb.NewStringValue("http://" + server.ApiAddr), + ConstDisableCredentialRotation: structpb.NewBoolValue(false), + }, + } + req := &plugin.OnUpdateStorageBucketRequest{ + CurrentBucket: &storagebuckets.StorageBucket{ + BucketName: bucketName, + Attributes: bucketAttrs, + }, + Persisted: &storagebuckets.StorageBucketPersisted{ + Data: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + ConstAccessKeyId: structpb.NewStringValue(persistedCreds.AccessKey), + ConstSecretAccessKey: structpb.NewStringValue(persistedCreds.SecretKey), + ConstLastRotatedTime: structpb.NewStringValue(time.Now().Format(time.RFC3339Nano)), + }, + }, + }, + NewBucket: &storagebuckets.StorageBucket{ + BucketName: bucketName, + Attributes: bucketAttrs, + }, + } + + // No changes with credential rotation enabled. This should change nothing. + sp := new(StoragePlugin) + rsp, err := sp.OnUpdateStorageBucket(ctx, req) + require.NoError(t, err) + require.NotNil(t, req) + + require.Empty(t, cmp.Diff(req.GetPersisted(), rsp.GetPersisted(), protocmp.Transform())) + + _, err = server.AdminClient.InfoServiceAccount(ctx, persistedCreds.AccessKey) + require.NoError(t, err) + + lsa, err := server.AdminClient.ListServiceAccounts(ctx, server.RootUsername) + require.NoError(t, err) + require.Len(t, lsa.Accounts, 2) // Creds in `server` + persistedCreds +} + +func TestOnDeleteStorageBucket(t *testing.T) { + ctx := context.Background() + server := internaltest.NewMinioServer(t) + + tests := []struct { + name string + in *pb.OnDeleteStorageBucketRequest + expCredsDeleted bool + expErrMsg string + }{ + { + name: "nilBucket", + in: &pb.OnDeleteStorageBucketRequest{ + Bucket: nil, + }, + expErrMsg: "no storage bucket information found", + }, + { + name: "nilPersistedSecrets", + in: &pb.OnDeleteStorageBucketRequest{ + Bucket: &storagebuckets.StorageBucket{ + BucketName: "test", + }, + Persisted: nil, + }, + expErrMsg: "empty secrets input", + }, + { + name: "rotationNilAttributes", + in: &pb.OnDeleteStorageBucketRequest{ + Bucket: &storagebuckets.StorageBucket{ + BucketName: "test", + Attributes: nil, + }, + Persisted: &storagebuckets.StorageBucketPersisted{ + Data: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + ConstAccessKeyId: structpb.NewStringValue("foo"), + ConstSecretAccessKey: structpb.NewStringValue("bar"), + ConstLastRotatedTime: structpb.NewStringValue(time.Now().Format(time.RFC3339Nano)), + }, + }, + }, + }, + expErrMsg: "empty attributes input", + }, + { + name: "rotationFail", + in: &pb.OnDeleteStorageBucketRequest{ + Bucket: &storagebuckets.StorageBucket{ + BucketName: "test", + 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("foo"), + ConstSecretAccessKey: structpb.NewStringValue("bar"), + ConstLastRotatedTime: structpb.NewStringValue(time.Now().Format(time.RFC3339Nano)), + }, + }, + }, + }, + expErrMsg: "failed to delete minio service account", + }, + { + name: "successNoRotation", + in: func() *pb.OnDeleteStorageBucketRequest { + creds, err := server.AdminClient.AddServiceAccount(ctx, madmin.AddServiceAccountReq{}) + require.NoError(t, err) + + return &pb.OnDeleteStorageBucketRequest{ + Bucket: &storagebuckets.StorageBucket{ + BucketName: "test", + 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(creds.AccessKey), + ConstSecretAccessKey: structpb.NewStringValue(creds.SecretKey), + ConstLastRotatedTime: structpb.NewStringValue(time.Time{}.Format(time.RFC3339Nano)), + }, + }, + }, + } + }(), + expCredsDeleted: false, + }, + { + name: "successWithRotation", + in: func() *pb.OnDeleteStorageBucketRequest { + creds, err := server.AdminClient.AddServiceAccount(ctx, madmin.AddServiceAccountReq{}) + require.NoError(t, err) + + return &pb.OnDeleteStorageBucketRequest{ + Bucket: &storagebuckets.StorageBucket{ + BucketName: "test", + Attributes: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + ConstEndpointUrl: structpb.NewStringValue("http://" + server.ApiAddr), + ConstDisableCredentialRotation: structpb.NewBoolValue(false), + }, + }, + }, + Persisted: &storagebuckets.StorageBucketPersisted{ + Data: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + ConstAccessKeyId: structpb.NewStringValue(creds.AccessKey), + ConstSecretAccessKey: structpb.NewStringValue(creds.SecretKey), + ConstLastRotatedTime: structpb.NewStringValue(time.Now().Format(time.RFC3339Nano)), + }, + }, + }, + } + }(), + expCredsDeleted: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + sp := new(StoragePlugin) + rsp, err := sp.OnDeleteStorageBucket(ctx, tt.in) + if tt.expErrMsg != "" { + require.ErrorContains(t, err, tt.expErrMsg) + return + } + + require.NoError(t, err) + require.NotNil(t, rsp) + + accessKeyIn, err := values.GetStringValue(tt.in.GetPersisted().GetData(), ConstAccessKeyId, true) + require.NoError(t, err) + + _, err = server.AdminClient.InfoServiceAccount(ctx, accessKeyIn) + if tt.expCredsDeleted { + require.ErrorContains(t, err, "specified service account is not found") + } else { + require.NoError(t, err) + } + }) + } } func TestValidatePermissions(t *testing.T) {