diff --git a/internal/values/proto.go b/internal/values/proto.go index 7d553b7..d6513d9 100644 --- a/internal/values/proto.go +++ b/internal/values/proto.go @@ -5,6 +5,7 @@ package values import ( "fmt" + "time" "google.golang.org/protobuf/types/known/structpb" ) @@ -37,6 +38,52 @@ func GetStringValue(in *structpb.Struct, k string, required bool) (string, error return s, nil } +// GetBoolValue returns a boolean value and no error if the given key is found +// in the provided proto struct input. An error is returned if the key is not +// found and required is true, or the value type is not a boolean. +func GetBoolValue(in *structpb.Struct, k string, required bool) (bool, error) { + mv := in.GetFields() + v, ok := mv[k] + if !ok { + if required { + return false, fmt.Errorf("missing required value %q", k) + } + + return false, nil + } + + b, ok := v.AsInterface().(bool) + if !ok { + return false, fmt.Errorf("unexpected type for value %q: want bool, got %T", k, v.AsInterface()) + } + + return b, nil +} + +// GetTimeValue returns a time.Time object and no error if the given key is +// found in the provided proto struct input. An error is returned if the value +// in the struct isn't a string or the value is not parsable by time.Parse. The +// expected time layout is RFC3339Nano. +func GetTimeValue(in *structpb.Struct, k string) (time.Time, error) { + mv := in.GetFields() + v, ok := mv[k] + if !ok { + return time.Time{}, nil + } + + tRaw, ok := v.AsInterface().(string) + if !ok { + return time.Time{}, fmt.Errorf("unexpected type for value %q: want string, got %T", k, v.AsInterface()) + } + + t, err := time.Parse(time.RFC3339Nano, tRaw) + if err != nil { + return time.Time{}, fmt.Errorf("could not parse time in value %q: %w", k, err) + } + + return t, nil +} + // StructFields returns a map[string]struct{} of the proto struct input. func StructFields(s *structpb.Struct) map[string]struct{} { m := make(map[string]struct{}, len(s.GetFields())) diff --git a/internal/values/proto_test.go b/internal/values/proto_test.go index 0aa4b53..b38fdd9 100644 --- a/internal/values/proto_test.go +++ b/internal/values/proto_test.go @@ -5,6 +5,7 @@ package values import ( "testing" + "time" "github.com/stretchr/testify/require" "google.golang.org/protobuf/types/known/structpb" @@ -78,3 +79,132 @@ func TestGetStringValue(t *testing.T) { }) } } + +func TestGetBoolValue(t *testing.T) { + cases := []struct { + name string + in map[string]any + key string + required bool + expected bool + expectedErr string + }{ + { + name: "required missing", + in: map[string]any{}, + key: "foo", + required: true, + expectedErr: "missing required value \"foo\"", + }, + { + name: "optional missing", + in: map[string]any{}, + key: "foo", + expected: false, + }, + { + name: "non-bool value", + in: map[string]any{ + "foo": "bar", + }, + key: "foo", + expectedErr: "unexpected type for value \"foo\": want bool, got string", + }, + { + name: "good", + in: map[string]any{ + "foo": true, + }, + key: "foo", + expected: true, + }, + } + + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + require := require.New(t) + + input, err := structpb.NewStruct(tc.in) + require.NoError(err) + + actual, err := GetBoolValue(input, tc.key, tc.required) + if tc.expectedErr != "" { + require.EqualError(err, tc.expectedErr) + return + } + + require.NoError(err) + require.Equal(tc.expected, actual) + }) + } +} + +func TestGetTimeValue(t *testing.T) { + staticTime := time.Now() + + cases := []struct { + name string + in map[string]any + key string + expected time.Time + expectedErr string + }{ + { + name: "missing", + in: map[string]any{}, + key: "foo", + expected: time.Time{}, + }, + { + name: "non-time value", + in: map[string]any{ + "foo": 1, + }, + key: "foo", + expectedErr: "unexpected type for value \"foo\": want string, got float64", + }, + { + name: "bad parse", + in: map[string]any{ + "foo": "bar", + }, + key: "foo", + expectedErr: "could not parse time in value \"foo\": parsing time \"bar\" as \"2006-01-02T15:04:05.999999999Z07:00\": cannot parse \"bar\" as \"2006\"", + }, + { + name: "good", + in: map[string]any{ + "foo": staticTime.Format(time.RFC3339Nano), + }, + key: "foo", + expected: func() time.Time { + u, err := time.Parse(time.RFC3339Nano, staticTime.Format(time.RFC3339Nano)) + if err != nil { + panic(err) + } + + return u + }(), + }, + } + + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + require := require.New(t) + + input, err := structpb.NewStruct(tc.in) + require.NoError(err) + + actual, err := GetTimeValue(input, tc.key) + if tc.expectedErr != "" { + require.EqualError(err, tc.expectedErr) + return + } + + require.NoError(err) + require.Equal(tc.expected, actual) + }) + } +} 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 caa31ac..a56bc14 100644 --- a/plugin/service/storage/plugin.go +++ b/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,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{ @@ -151,14 +236,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 078e48c..538a85b 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) { diff --git a/plugin/service/storage/rotation.go b/plugin/service/storage/rotation.go new file mode 100644 index 0000000..2db5484 --- /dev/null +++ b/plugin/service/storage/rotation.go @@ -0,0 +1,88 @@ +package storage + +import ( + "context" + "encoding/json" + "fmt" + "sync" + "time" + + "github.com/minio/madmin-go/v3" +) + +const ( + defaultPolicyTemplate = `{ + "Statement": [ + { + "Action": [ + "s3:GetObject", + "s3:GetObjectAttributes", + "s3:PutObject", + "s3:DeleteObject" + ], + "Effect": "Allow", + "Resource": "arn:aws:s3:::%[1]s/*" + }, + { + "Action": "s3:ListBucket", + "Effect": "Allow", + "Resource": "arn:aws:s3:::%[1]s" + }, + { + "Action": [ + "admin:CreateServiceAccount", + "admin:RemoveServiceAccount" + ], + "Effect": "Allow" + } + ], + "Version": "2012-10-17" + }` +) + +type deleteInputCredsFn func() error + +// rotateCredentials uses the incoming service account credentials to generate a +// new service account. It returns a new StorageSecrets object with the new +// service account credentials and a callback function to delete the service +// account provided in the input. +func rotateCredentials(ctx context.Context, bucketName string, sa *StorageAttributes, inSec *StorageSecrets) (*StorageSecrets, deleteInputCredsFn, error) { + err := ensureServiceAccount(ctx, sa, inSec) + if err != nil { + return nil, nil, fmt.Errorf("failed to ensure minio service account credentials: %w", err) + } + + ac, err := newMadminClient(sa, inSec) + if err != nil { + return nil, nil, fmt.Errorf("failed to create minio admin client: %w", err) + } + + policy := json.RawMessage(fmt.Sprintf(defaultPolicyTemplate, bucketName)) + newCreds, err := ac.AddServiceAccount(ctx, madmin.AddServiceAccountReq{ + Name: "Boundary MinIO Service Account", + Description: fmt.Sprintf("Boundary-managed MinIO service account for storage bucket '%s'", bucketName), + Policy: policy, + }) + if err != nil { + return nil, nil, fmt.Errorf("failed to create new minio service account: %w", err) + } + + newSec := &StorageSecrets{ + AccessKeyId: newCreds.AccessKey, + SecretAccessKey: newCreds.SecretKey, + LastRotatedTime: time.Now(), + } + + inCl, newCl := inSec.Clone(), newSec.Clone() + return newSec, sync.OnceValue(func() error { + ac, err := newMadminClient(sa, newCl) + if err != nil { + return fmt.Errorf("failed to create minio admin client: %w", err) + } + err = ac.DeleteServiceAccount(ctx, inCl.AccessKeyId) + if err != nil { + return fmt.Errorf("failed to delete minio service account: %w", err) + } + return nil + }), nil +} diff --git a/plugin/service/storage/rotation_test.go b/plugin/service/storage/rotation_test.go new file mode 100644 index 0000000..c1b724a --- /dev/null +++ b/plugin/service/storage/rotation_test.go @@ -0,0 +1,137 @@ +package storage + +import ( + "context" + "encoding/json" + "testing" + + internaltest "github.com/hashicorp/boundary-plugin-minio/internal/testing" + "github.com/minio/madmin-go/v3" + "github.com/minio/minio-go/v7" + "github.com/stretchr/testify/require" +) + +func TestRotateCredentials(t *testing.T) { + ctx := context.Background() + server := internaltest.NewMinioServer(t) + + bucketName := "test-bucket" + err := server.Client.MakeBucket(ctx, bucketName, minio.MakeBucketOptions{}) + require.NoError(t, err) + + tests := []struct { + name string + ss *StorageSecrets + expErrMsg string + credDelErr bool + }{ + { + name: "notUsingServiceAccount", + ss: &StorageSecrets{ + AccessKeyId: server.RootUsername, + SecretAccessKey: server.RootPassword, + }, + expErrMsg: "failed to ensure minio service account credentials: failed to obtain service account info: The specified service account is not found", + }, + { + name: "addServiceAccountFail", + ss: func() *StorageSecrets { + 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 &StorageSecrets{ + AccessKeyId: creds.AccessKey, + SecretAccessKey: creds.SecretKey, + } + }(), + expErrMsg: "failed to create new minio service account: Access Denied", + }, + { + name: "credentialDeletionFail", + ss: func() *StorageSecrets { + creds, err := server.AdminClient.AddServiceAccount(ctx, madmin.AddServiceAccountReq{}) + require.NoError(t, err) + + return &StorageSecrets{ + AccessKeyId: creds.AccessKey, + SecretAccessKey: creds.SecretKey, + } + }(), + credDelErr: true, + }, + { + name: "success", + ss: func() *StorageSecrets { + creds, err := server.AdminClient.AddServiceAccount(ctx, madmin.AddServiceAccountReq{}) + require.NoError(t, err) + + return &StorageSecrets{ + AccessKeyId: creds.AccessKey, + SecretAccessKey: creds.SecretKey, + } + }(), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + sa := &StorageAttributes{ + EndpointUrl: server.ApiAddr, + UseSSL: false, + } + + ss, delFn, err := rotateCredentials(context.Background(), bucketName, sa, tt.ss) + if tt.expErrMsg != "" { + require.ErrorContains(t, err, tt.expErrMsg) + require.Nil(t, ss) + require.Nil(t, delFn) + return + } + + require.NoError(t, err) + require.NotNil(t, ss) + require.NotNil(t, delFn) + 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.ss.AccessKeyId) + require.NoError(t, err) + + if tt.credDelErr { + sa.EndpointUrl = "localhost:1" + 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) + require.NoError(t, err) + return + } else { + require.NoError(t, delFn()) + require.NoError(t, delFn()) // 2nd call should return the same state (due to sync.Once). + } + + _, err = server.AdminClient.InfoServiceAccount(ctx, tt.ss.AccessKeyId) + require.ErrorContains(t, err, "service account is not found") + }) + } +}