From 56480cb7f183d70a90ba2f63944783a5473f4cbd Mon Sep 17 00:00:00 2001 From: Alan Shaw Date: Fri, 4 Sep 2026 09:40:47 -0400 Subject: [PATCH 1/3] fix: differentiate between bucket exists vs owned when creating bucket --- pkg/rpc/failure.go | 1 + pkg/rpc/failure_test.go | 8 ++++++++ pkg/rpc/service/bucket/errors.go | 20 +++++++++++++------- pkg/rpc/service/bucket/service.go | 7 ++++++- pkg/rpc/service/bucket/service_test.go | 13 +++++++++++-- 5 files changed, 39 insertions(+), 10 deletions(-) diff --git a/pkg/rpc/failure.go b/pkg/rpc/failure.go index d87811dd..72e5a1b1 100644 --- a/pkg/rpc/failure.go +++ b/pkg/rpc/failure.go @@ -55,6 +55,7 @@ func bucketFailure(res failer, err error) error { switch { case errors.Is(err, bucketsvc.ErrOperationMismatch), errors.Is(err, bucketsvc.ErrBucketExists), + errors.Is(err, bucketsvc.ErrBucketAlreadyOwned), errors.Is(err, bucketsvc.ErrBucketNotEmpty), errors.Is(err, bucketsvc.ErrUnknownBucket), errors.Is(err, bucketsvc.ErrUnknownAccessKey), diff --git a/pkg/rpc/failure_test.go b/pkg/rpc/failure_test.go index 5a05de28..68608655 100644 --- a/pkg/rpc/failure_test.go +++ b/pkg/rpc/failure_test.go @@ -41,6 +41,14 @@ func TestBucketFailure(t *testing.T) { requireName(t, f.got, bucketsvc.BucketExistsErrorName) }) + t.Run("wrapped already-owned sentinel is set as failure with its name", func(t *testing.T) { + f := &recordingFailer{} + err := fmt.Errorf("%w: %q", bucketsvc.ErrBucketAlreadyOwned, "foo") + require.NoError(t, bucketFailure(f, err)) + require.True(t, f.called) + requireName(t, f.got, bucketsvc.BucketAlreadyOwnedErrorName) + }) + t.Run("propagated auth sentinel is set as failure with its name", func(t *testing.T) { f := &recordingFailer{} require.NoError(t, bucketFailure(f, auth.ErrOperationNotPermitted)) diff --git a/pkg/rpc/service/bucket/errors.go b/pkg/rpc/service/bucket/errors.go index 98282673..f37a8705 100644 --- a/pkg/rpc/service/bucket/errors.go +++ b/pkg/rpc/service/bucket/errors.go @@ -6,12 +6,13 @@ import "github.com/fil-forge/ucantone/errors" // Ingot, mapping to canonical S3 error responses) can match on the stable Name() // of a serialized failure. const ( - OperationMismatchErrorName = "OperationMismatch" - BucketExistsErrorName = "BucketExists" - BucketNotEmptyErrorName = "BucketNotEmpty" - UnknownBucketErrorName = "UnknownBucket" - UnknownAccessKeyErrorName = "UnknownAccessKey" - InvalidArgumentErrorName = "InvalidArgument" + OperationMismatchErrorName = "OperationMismatch" + BucketExistsErrorName = "BucketExists" + BucketAlreadyOwnedErrorName = "BucketAlreadyOwnedByYou" + BucketNotEmptyErrorName = "BucketNotEmpty" + UnknownBucketErrorName = "UnknownBucket" + UnknownAccessKeyErrorName = "UnknownAccessKey" + InvalidArgumentErrorName = "InvalidArgument" ) // Known errors returned by the bucket [Service]. Handlers pass these to @@ -22,8 +23,13 @@ var ( // ErrOperationMismatch is returned when the signed request's S3 operation does // not match the invoked bucket command (create/delete/list). ErrOperationMismatch = errors.New(OperationMismatchErrorName, "request operation does not match the command") - // ErrBucketExists is returned when creating a bucket whose name already exists. + // ErrBucketExists is returned when creating a bucket whose name already exists + // under a different owner. ErrBucketExists = errors.New(BucketExistsErrorName, "bucket already exists") + // ErrBucketAlreadyOwned is returned when creating a bucket whose name already + // exists and is owned by the requesting tenant (the S3 + // BucketAlreadyOwnedByYou case, distinct from BucketAlreadyExists). + ErrBucketAlreadyOwned = errors.New(BucketAlreadyOwnedErrorName, "bucket already exists and is owned by you") // ErrBucketNotEmpty is returned when deleting a bucket whose space is not empty. ErrBucketNotEmpty = errors.New(BucketNotEmptyErrorName, "bucket is not empty") // ErrUnknownBucket is returned when the named bucket does not exist. diff --git a/pkg/rpc/service/bucket/service.go b/pkg/rpc/service/bucket/service.go index 0e69d44a..d3a62895 100644 --- a/pkg/rpc/service/bucket/service.go +++ b/pkg/rpc/service/bucket/service.go @@ -107,8 +107,13 @@ func (s *Service) Create(ctx context.Context, issuer did.DID, args *s3bkt.Create return nil, nil, fmt.Errorf("%w: %s", ErrOperationMismatch, authz.Operation) } - _, err = s.buckets.GetByName(ctx, authz.BucketName) + rec, err := s.buckets.GetByName(ctx, authz.BucketName) if err == nil { + // An existing name owned by the requesting tenant is + // BucketAlreadyOwnedByYou; owned by anyone else is BucketAlreadyExists. + if rec.Tenant == authz.Tenant.ID { + return nil, nil, fmt.Errorf("%w: %q", ErrBucketAlreadyOwned, authz.BucketName) + } return nil, nil, fmt.Errorf("%w: %q", ErrBucketExists, authz.BucketName) } else if !errors.Is(err, store.ErrRecordNotFound) { return nil, nil, fmt.Errorf("looking up bucket: %w", err) diff --git a/pkg/rpc/service/bucket/service_test.go b/pkg/rpc/service/bucket/service_test.go index e0e35b30..a72df2b8 100644 --- a/pkg/rpc/service/bucket/service_test.go +++ b/pkg/rpc/service/bucket/service_test.go @@ -131,13 +131,22 @@ func TestCreate(t *testing.T) { require.ErrorIs(t, err, auth.ErrOperationNotPermitted) }) - t.Run("rejects a duplicate bucket name", func(t *testing.T) { + t.Run("rejects a duplicate name owned by another tenant", func(t *testing.T) { svc, buckets := setup(t, []string{"s3:CreateBucket"}, &fakeSprue{}, delegationmemory.New()) - require.NoError(t, buckets.Add(ctx, testutil.RandomDID(t), tenantID, bucketName)) + // Owner is a different tenant → BucketAlreadyExists. + require.NoError(t, buckets.Add(ctx, testutil.RandomDID(t), testutil.RandomDID(t), bucketName)) _, _, err := svc.Create(ctx, providerID, args()) require.ErrorIs(t, err, bucketsvc.ErrBucketExists) }) + t.Run("rejects re-creating a bucket you already own", func(t *testing.T) { + svc, buckets := setup(t, []string{"s3:CreateBucket"}, &fakeSprue{}, delegationmemory.New()) + // Owner is the requesting tenant → BucketAlreadyOwnedByYou. + require.NoError(t, buckets.Add(ctx, testutil.RandomDID(t), tenantID, bucketName)) + _, _, err := svc.Create(ctx, providerID, args()) + require.ErrorIs(t, err, bucketsvc.ErrBucketAlreadyOwned) + }) + t.Run("rolls back the bucket when provisioning fails", func(t *testing.T) { svc, buckets := setup(t, []string{"s3:CreateBucket"}, &fakeSprue{provErr: errors.New("sprue unavailable")}, delegationmemory.New()) _, _, err := svc.Create(ctx, providerID, args()) From fd3f21a6c18d5a77891330b0dfeaaa04403aa9a3 Mon Sep 17 00:00:00 2001 From: Alan Shaw Date: Fri, 4 Sep 2026 09:44:42 -0400 Subject: [PATCH 2/3] refactor: error name --- pkg/rpc/service/bucket/errors.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/pkg/rpc/service/bucket/errors.go b/pkg/rpc/service/bucket/errors.go index f37a8705..5d7aaa93 100644 --- a/pkg/rpc/service/bucket/errors.go +++ b/pkg/rpc/service/bucket/errors.go @@ -8,7 +8,7 @@ import "github.com/fil-forge/ucantone/errors" const ( OperationMismatchErrorName = "OperationMismatch" BucketExistsErrorName = "BucketExists" - BucketAlreadyOwnedErrorName = "BucketAlreadyOwnedByYou" + BucketAlreadyOwnedErrorName = "BucketAlreadyOwnedByTenant" BucketNotEmptyErrorName = "BucketNotEmpty" UnknownBucketErrorName = "UnknownBucket" UnknownAccessKeyErrorName = "UnknownAccessKey" @@ -27,9 +27,10 @@ var ( // under a different owner. ErrBucketExists = errors.New(BucketExistsErrorName, "bucket already exists") // ErrBucketAlreadyOwned is returned when creating a bucket whose name already - // exists and is owned by the requesting tenant (the S3 - // BucketAlreadyOwnedByYou case, distinct from BucketAlreadyExists). - ErrBucketAlreadyOwned = errors.New(BucketAlreadyOwnedErrorName, "bucket already exists and is owned by you") + // exists and is owned by the requesting tenant. Ingot maps it to the S3 + // BucketAlreadyOwnedByYou response, distinct from BucketAlreadyExists (a + // name owned by a different tenant). + ErrBucketAlreadyOwned = errors.New(BucketAlreadyOwnedErrorName, "bucket already exists and is owned by the requesting tenant") // ErrBucketNotEmpty is returned when deleting a bucket whose space is not empty. ErrBucketNotEmpty = errors.New(BucketNotEmptyErrorName, "bucket is not empty") // ErrUnknownBucket is returned when the named bucket does not exist. From 3ead5e174cf4aa0675123fb1871cb46ec5e80e15 Mon Sep 17 00:00:00 2001 From: Alan Shaw Date: Fri, 4 Sep 2026 10:06:54 -0400 Subject: [PATCH 3/3] chore: upgrade smelt and libforge --- go.mod | 4 ++-- go.sum | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/go.mod b/go.mod index aa6595ea..48bf46a9 100644 --- a/go.mod +++ b/go.mod @@ -10,8 +10,8 @@ require ( github.com/aws/aws-sdk-go-v2/service/s3 v1.103.3 github.com/aws/smithy-go v1.27.2 github.com/docker/docker v28.5.2+incompatible - github.com/fil-forge/libforge v0.0.0-20260827180828-c9252ac89b0e - github.com/fil-forge/smelt v0.0.0-20260825195303-2d623ae3f04e + github.com/fil-forge/libforge v0.0.0-20260904125112-81372e7200bf + github.com/fil-forge/smelt v0.0.0-20260828105933-8ba0939fb9a7 github.com/fil-forge/swarf v0.0.1-0.20260821142121-d5d1a0a56f00 github.com/fil-forge/ucantone v0.0.0-20260827134420-25cf8340b9a1 github.com/hashicorp/vault-client-go v0.4.3 diff --git a/go.sum b/go.sum index b01f0e52..765aaba6 100644 --- a/go.sum +++ b/go.sum @@ -156,10 +156,10 @@ github.com/fatih/color v1.16.0 h1:zmkK9Ngbjj+K0yRhTVONQh1p/HknKYSlNT+vZCzyokM= github.com/fatih/color v1.16.0/go.mod h1:fL2Sau1YI5c0pdGEVCbKQbLXB6edEj1ZgiY4NijnWvE= github.com/felixge/httpsnoop v1.1.0 h1:3YtUj32ZZkqZtt3sZZsClsymw/QDuVfpNhoA31zeORc= github.com/felixge/httpsnoop v1.1.0/go.mod h1:Zqxgdd+1Rkcz8euOqdr7lqgCRJztwr5hp9vDSi5UZCE= -github.com/fil-forge/libforge v0.0.0-20260827180828-c9252ac89b0e h1:2na0I6mpUhgwW6tg67S9rTFVOkr9gchdZFvG44QcA0I= -github.com/fil-forge/libforge v0.0.0-20260827180828-c9252ac89b0e/go.mod h1:09nIz6/BJD0Qdb8YrLwVeNQwQprTCFWVPULL3c5CNJs= -github.com/fil-forge/smelt v0.0.0-20260825195303-2d623ae3f04e h1:TfSprzfKeAAjglQCXM5uxNSmX1AN+jpEYPBkIj4breE= -github.com/fil-forge/smelt v0.0.0-20260825195303-2d623ae3f04e/go.mod h1:OUO2GxgUihmYfywNIfvWTQocg6jqaNiTd04cG5BbdII= +github.com/fil-forge/libforge v0.0.0-20260904125112-81372e7200bf h1:FstLBgWd8aItdY2nG9fN4bleCMPIj3Q0u6/Sj9+rBwo= +github.com/fil-forge/libforge v0.0.0-20260904125112-81372e7200bf/go.mod h1:09nIz6/BJD0Qdb8YrLwVeNQwQprTCFWVPULL3c5CNJs= +github.com/fil-forge/smelt v0.0.0-20260828105933-8ba0939fb9a7 h1:0aFgIYRVCUGzs5CKrnxrjnEm1wxs+yT3f6ju3CBV5I0= +github.com/fil-forge/smelt v0.0.0-20260828105933-8ba0939fb9a7/go.mod h1:OUO2GxgUihmYfywNIfvWTQocg6jqaNiTd04cG5BbdII= github.com/fil-forge/swarf v0.0.1-0.20260821142121-d5d1a0a56f00 h1:uSJ2YCxOJQA7PDnDiBJAOstoJcw2IPnbpssKYCihsxg= github.com/fil-forge/swarf v0.0.1-0.20260821142121-d5d1a0a56f00/go.mod h1:k1gay5Byjloml4k1SylxhRff1rMH4LcGKpbMEDLZR1o= github.com/fil-forge/ucantone v0.0.0-20260827134420-25cf8340b9a1 h1:Tzy3lZ7+LAyVK4+qyWM97UMOtnXx05Wdn+rcTDLdBDE=