From 5272fd8497e66b003b27cf6ee340b178ad0965d0 Mon Sep 17 00:00:00 2001 From: Michal Niewrzal Date: Mon, 17 Jul 2023 10:42:03 +0200 Subject: [PATCH] satellite/metainfo: do full bucket validation only on create We are doing full bucket name validation for many requests but we should do this only while creating bucket. Other requests will be covered only by basic name length validation. Less strict validation for other requests will make bucket usable in case of invalid bucket names in DB (we have such cases from the past). https://github.com/storj/storj/issues/6044 Change-Id: I3a41050e3637787f788705ef15b5dc4df4d01fc6 --- satellite/metainfo/endpoint_bucket.go | 4 ++-- satellite/metainfo/endpoint_bucket_test.go | 20 +++++++++++------ satellite/metainfo/endpoint_object.go | 25 +++++++++++----------- satellite/metainfo/validation.go | 14 ++++++++---- 4 files changed, 39 insertions(+), 24 deletions(-) diff --git a/satellite/metainfo/endpoint_bucket.go b/satellite/metainfo/endpoint_bucket.go index 43ec1c759..92a705111 100644 --- a/satellite/metainfo/endpoint_bucket.go +++ b/satellite/metainfo/endpoint_bucket.go @@ -71,7 +71,7 @@ func (endpoint *Endpoint) CreateBucket(ctx context.Context, req *pb.BucketCreate } endpoint.usageTracking(keyInfo, req.Header, fmt.Sprintf("%T", req)) - err = endpoint.validateBucket(ctx, req.Name) + err = endpoint.validateBucketName(req.Name) if err != nil { return nil, rpcstatus.Error(rpcstatus.InvalidArgument, err.Error()) } @@ -180,7 +180,7 @@ func (endpoint *Endpoint) DeleteBucket(ctx context.Context, req *pb.BucketDelete } endpoint.usageTracking(keyInfo, req.Header, fmt.Sprintf("%T", req)) - err = endpoint.validateBucket(ctx, req.Name) + err = endpoint.validateBucketNameLength(req.Name) if err != nil { return nil, rpcstatus.Error(rpcstatus.InvalidArgument, err.Error()) } diff --git a/satellite/metainfo/endpoint_bucket_test.go b/satellite/metainfo/endpoint_bucket_test.go index 22aa1bb03..ba40025ff 100644 --- a/satellite/metainfo/endpoint_bucket_test.go +++ b/satellite/metainfo/endpoint_bucket_test.go @@ -118,12 +118,6 @@ func TestBucketNameValidation(t *testing.T) { "test\\", "test%", } for _, name := range invalidNames { - _, err = metainfoClient.BeginObject(ctx, metaclient.BeginObjectParams{ - Bucket: []byte(name), - EncryptedObjectKey: []byte("123"), - }) - require.Error(t, err, "bucket name: %v", name) - require.True(t, errs2.IsRPC(err, rpcstatus.InvalidArgument)) _, err = metainfoClient.CreateBucket(ctx, metaclient.CreateBucketParams{ Name: []byte(name), @@ -131,6 +125,20 @@ func TestBucketNameValidation(t *testing.T) { require.Error(t, err, "bucket name: %v", name) require.True(t, errs2.IsRPC(err, rpcstatus.InvalidArgument)) } + + invalidNames = []string{ + "", "t", "te", + "testbucket-64-0123456789012345678901234567890123456789012345abcd", + } + for _, name := range invalidNames { + // BeginObject validates only bucket name length + _, err = metainfoClient.BeginObject(ctx, metaclient.BeginObjectParams{ + Bucket: []byte(name), + EncryptedObjectKey: []byte("123"), + }) + require.Error(t, err, "bucket name: %v", name) + require.True(t, errs2.IsRPC(err, rpcstatus.InvalidArgument)) + } }) } diff --git a/satellite/metainfo/endpoint_object.go b/satellite/metainfo/endpoint_object.go index b30b20461..e0d3b0725 100644 --- a/satellite/metainfo/endpoint_object.go +++ b/satellite/metainfo/endpoint_object.go @@ -66,7 +66,8 @@ func (endpoint *Endpoint) BeginObject(ctx context.Context, req *pb.ObjectBeginRe return nil, rpcstatus.Error(rpcstatus.InvalidArgument, "Invalid expiration time") } - err = endpoint.validateBucket(ctx, req.Bucket) + // we can do just basic name validation because later we are checking bucket in DB + err = endpoint.validateBucketNameLength(req.Bucket) if err != nil { return nil, rpcstatus.Error(rpcstatus.InvalidArgument, err.Error()) } @@ -310,7 +311,7 @@ func (endpoint *Endpoint) GetObject(ctx context.Context, req *pb.ObjectGetReques } endpoint.usageTracking(keyInfo, req.Header, fmt.Sprintf("%T", req)) - err = endpoint.validateBucket(ctx, req.Bucket) + err = endpoint.validateBucketNameLength(req.Bucket) if err != nil { return nil, rpcstatus.Error(rpcstatus.InvalidArgument, err.Error()) } @@ -409,7 +410,7 @@ func (endpoint *Endpoint) DownloadObject(ctx context.Context, req *pb.ObjectDown return nil, err } - err = endpoint.validateBucket(ctx, req.Bucket) + err = endpoint.validateBucketNameLength(req.Bucket) if err != nil { return nil, rpcstatus.Error(rpcstatus.InvalidArgument, err.Error()) } @@ -804,7 +805,7 @@ func (endpoint *Endpoint) ListObjects(ctx context.Context, req *pb.ObjectListReq } endpoint.usageTracking(keyInfo, req.Header, fmt.Sprintf("%T", req)) - err = endpoint.validateBucket(ctx, req.Bucket) + err = endpoint.validateBucketNameLength(req.Bucket) if err != nil { return nil, rpcstatus.Error(rpcstatus.InvalidArgument, err.Error()) } @@ -944,7 +945,7 @@ func (endpoint *Endpoint) ListPendingObjectStreams(ctx context.Context, req *pb. } endpoint.usageTracking(keyInfo, req.Header, fmt.Sprintf("%T", req)) - err = endpoint.validateBucket(ctx, req.Bucket) + err = endpoint.validateBucketNameLength(req.Bucket) if err != nil { return nil, rpcstatus.Error(rpcstatus.InvalidArgument, err.Error()) } @@ -1057,7 +1058,7 @@ func (endpoint *Endpoint) BeginDeleteObject(ctx context.Context, req *pb.ObjectB } endpoint.usageTracking(keyInfo, req.Header, fmt.Sprintf("%T", req)) - err = endpoint.validateBucket(ctx, req.Bucket) + err = endpoint.validateBucketNameLength(req.Bucket) if err != nil { return nil, rpcstatus.Error(rpcstatus.InvalidArgument, err.Error()) } @@ -1146,7 +1147,7 @@ func (endpoint *Endpoint) GetObjectIPs(ctx context.Context, req *pb.ObjectGetIPs } endpoint.usageTracking(keyInfo, req.Header, fmt.Sprintf("%T", req)) - err = endpoint.validateBucket(ctx, req.Bucket) + err = endpoint.validateBucketNameLength(req.Bucket) if err != nil { return nil, rpcstatus.Error(rpcstatus.InvalidArgument, err.Error()) } @@ -1237,7 +1238,7 @@ func (endpoint *Endpoint) UpdateObjectMetadata(ctx context.Context, req *pb.Obje } endpoint.usageTracking(keyInfo, req.Header, fmt.Sprintf("%T", req)) - err = endpoint.validateBucket(ctx, req.Bucket) + err = endpoint.validateBucketNameLength(req.Bucket) if err != nil { return nil, rpcstatus.Error(rpcstatus.InvalidArgument, err.Error()) } @@ -1615,7 +1616,7 @@ func (endpoint *Endpoint) BeginMoveObject(ctx context.Context, req *pb.ObjectBeg endpoint.usageTracking(keyInfo, req.Header, fmt.Sprintf("%T", req)) for _, bucket := range [][]byte{req.Bucket, req.NewBucket} { - err = endpoint.validateBucket(ctx, bucket) + err = endpoint.validateBucketNameLength(bucket) if err != nil { return nil, rpcstatus.Error(rpcstatus.InvalidArgument, err.Error()) } @@ -1761,7 +1762,7 @@ func (endpoint *Endpoint) FinishMoveObject(ctx context.Context, req *pb.ObjectFi } endpoint.usageTracking(keyInfo, req.Header, fmt.Sprintf("%T", req)) - err = endpoint.validateBucket(ctx, req.NewBucket) + err = endpoint.validateBucketNameLength(req.NewBucket) if err != nil { return nil, rpcstatus.Error(rpcstatus.InvalidArgument, err.Error()) } @@ -1840,7 +1841,7 @@ func (endpoint *Endpoint) BeginCopyObject(ctx context.Context, req *pb.ObjectBeg endpoint.usageTracking(keyInfo, req.Header, fmt.Sprintf("%T", req)) for _, bucket := range [][]byte{req.Bucket, req.NewBucket} { - err = endpoint.validateBucket(ctx, bucket) + err = endpoint.validateBucketNameLength(bucket) if err != nil { return nil, rpcstatus.Error(rpcstatus.InvalidArgument, err.Error()) } @@ -1952,7 +1953,7 @@ func (endpoint *Endpoint) FinishCopyObject(ctx context.Context, req *pb.ObjectFi } endpoint.usageTracking(keyInfo, req.Header, fmt.Sprintf("%T", req)) - err = endpoint.validateBucket(ctx, req.NewBucket) + err = endpoint.validateBucketNameLength(req.NewBucket) if err != nil { return nil, rpcstatus.Error(rpcstatus.InvalidArgument, err.Error()) } diff --git a/satellite/metainfo/validation.go b/satellite/metainfo/validation.go index f6034dc08..caf9f8bb6 100644 --- a/satellite/metainfo/validation.go +++ b/satellite/metainfo/validation.go @@ -247,9 +247,7 @@ func (endpoint *Endpoint) checkRate(ctx context.Context, projectID uuid.UUID) (e return nil } -func (endpoint *Endpoint) validateBucket(ctx context.Context, bucket []byte) (err error) { - defer mon.Task()(&ctx)(&err) - +func (endpoint *Endpoint) validateBucketNameLength(bucket []byte) (err error) { if len(bucket) == 0 { return Error.Wrap(buckets.ErrNoBucket.New("")) } @@ -258,11 +256,19 @@ func (endpoint *Endpoint) validateBucket(ctx context.Context, bucket []byte) (er return Error.New("bucket name must be at least 3 and no more than 63 characters long") } + return nil +} + +func (endpoint *Endpoint) validateBucketName(bucket []byte) error { + if err := endpoint.validateBucketNameLength(bucket); err != nil { + return err + } + // Regexp not used because benchmark shows it will be slower for valid bucket names // https://gist.github.com/mniewrzal/49de3af95f36e63e88fac24f565e444c labels := bytes.Split(bucket, []byte(".")) for _, label := range labels { - err = validateBucketLabel(label) + err := validateBucketLabel(label) if err != nil { return err }