From 4ec564469aa2b46b6f999fd5a2812a8fd278d525 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Thu, 17 Sep 2026 11:43:52 -0700 Subject: [PATCH] s3tables: hide inaccessible catalog resources (#11365) * s3tables: hide inaccessible table buckets * s3tables: hide inaccessible namespaces * s3tables: hide inaccessible tables * s3tables: hide inaccessible resources in rename and namespace delete RenameTable/RenameView denied on the source now report the same not-found as a missing source, and the destination name conflict is checked only after destination authorization so a denied caller cannot distinguish an existing destination namespace or name from a missing one. DeleteNamespace denials use the same formatted message as a missing namespace. --- .../handler_bucket_get_list_delete.go | 4 +- .../s3tables/handler_existence_hiding_test.go | 128 ++++++++++++++++++ weed/s3api/s3tables/handler_namespace.go | 6 +- weed/s3api/s3tables/handler_rename_test.go | 6 +- weed/s3api/s3tables/handler_table.go | 37 ++--- 5 files changed, 158 insertions(+), 23 deletions(-) create mode 100644 weed/s3api/s3tables/handler_existence_hiding_test.go diff --git a/weed/s3api/s3tables/handler_bucket_get_list_delete.go b/weed/s3api/s3tables/handler_bucket_get_list_delete.go index c457fd72c..a85e151dc 100644 --- a/weed/s3api/s3tables/handler_bucket_get_list_delete.go +++ b/weed/s3api/s3tables/handler_bucket_get_list_delete.go @@ -74,7 +74,7 @@ func (h *S3TablesHandler) handleGetTableBucket(w http.ResponseWriter, r *http.Re IdentityActions: identityActions, DefaultAllow: h.defaultAllowFor(r), }) { - h.writeError(w, http.StatusForbidden, ErrCodeAccessDenied, "not authorized to get table bucket details") + h.writeError(w, http.StatusNotFound, ErrCodeNoSuchBucket, fmt.Sprintf("table bucket %s not found", bucketName)) return ErrAccessDenied } @@ -333,7 +333,7 @@ func (h *S3TablesHandler) handleDeleteTableBucket(w http.ResponseWriter, r *http if errors.Is(err, filer_pb.ErrNotFound) { h.writeError(w, http.StatusNotFound, ErrCodeNoSuchBucket, fmt.Sprintf("table bucket %s not found", bucketName)) } else if isAuthError(err) { - h.writeError(w, http.StatusForbidden, ErrCodeAccessDenied, err.Error()) + h.writeError(w, http.StatusNotFound, ErrCodeNoSuchBucket, fmt.Sprintf("table bucket %s not found", bucketName)) } else { h.writeError(w, http.StatusInternalServerError, ErrCodeInternalError, fmt.Sprintf("failed to delete table bucket: %v", err)) } diff --git a/weed/s3api/s3tables/handler_existence_hiding_test.go b/weed/s3api/s3tables/handler_existence_hiding_test.go new file mode 100644 index 000000000..b677daca7 --- /dev/null +++ b/weed/s3api/s3tables/handler_existence_hiding_test.go @@ -0,0 +1,128 @@ +package s3tables + +import ( + "context" + "encoding/json" + "net/http/httptest" + "testing" + + "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3tables/s3tablestest" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type s3TablesHTTPError struct { + status int + body S3TablesError +} + +func runUnauthorizedRequest(t *testing.T, m *Manager, fs *s3tablestest.MemFiler, operation string, input interface{}) s3TablesHTTPError { + t.Helper() + + body, err := json.Marshal(input) + require.NoError(t, err) + identity := &testIdentity{Name: "attacker", Account: &testIdentityAccount{Id: "attacker"}, Actions: []string{"Read"}} + ctx := s3_constants.SetIdentityInContext(context.Background(), identity) + req, err := newManagerRequest(ctx, operation, body, "attacker") + require.NoError(t, err) + + recorder := httptest.NewRecorder() + m.handler.HandleRequest(recorder, req, NewManagerClient(fs.Client)) + result := recorder.Result() + defer result.Body.Close() + + var response S3TablesError + require.NoError(t, json.NewDecoder(result.Body).Decode(&response)) + return s3TablesHTTPError{status: result.StatusCode, body: response} +} + +func TestTableBucketAuthorizationDenialMatchesMissing(t *testing.T) { + existing, manager := startRenameManager(t) + missing := s3tablestest.Start(t) + manager.SetTrusted(false) + manager.SetDefaultAllow(false) + + for _, operation := range []string{"GetTableBucket", "DeleteTableBucket"} { + t.Run(operation, func(t *testing.T) { + var request interface{} = &GetTableBucketRequest{TableBucketARN: mustBucketARN(t)} + if operation == "DeleteTableBucket" { + request = &DeleteTableBucketRequest{TableBucketARN: mustBucketARN(t)} + } + + want := runUnauthorizedRequest(t, manager, missing, operation, request) + got := runUnauthorizedRequest(t, manager, existing, operation, request) + assert.Equal(t, want, got) + assert.Equal(t, 404, got.status) + assert.Equal(t, ErrCodeNoSuchBucket, got.body.Type) + }) + } +} + +func TestNamespaceAuthorizationDenialMatchesMissing(t *testing.T) { + existing, manager := startRenameManager(t) + missing := s3tablestest.Start(t) + manager.SetTrusted(false) + manager.SetDefaultAllow(false) + + for _, operation := range []string{"GetNamespace", "UpdateNamespace", "DeleteNamespace"} { + t.Run(operation, func(t *testing.T) { + var request interface{} = &GetNamespaceRequest{TableBucketARN: mustBucketARN(t), Namespace: []string{"ns"}} + switch operation { + case "UpdateNamespace": + request = &UpdateNamespaceRequest{TableBucketARN: mustBucketARN(t), Namespace: []string{"ns"}} + case "DeleteNamespace": + request = &DeleteNamespaceRequest{TableBucketARN: mustBucketARN(t), Namespace: []string{"ns"}} + } + + want := runUnauthorizedRequest(t, manager, missing, operation, request) + got := runUnauthorizedRequest(t, manager, existing, operation, request) + assert.Equal(t, want, got) + assert.Equal(t, 404, got.status) + assert.Equal(t, ErrCodeNoSuchNamespace, got.body.Type) + }) + } +} + +func TestTableAuthorizationDenialMatchesMissing(t *testing.T) { + existing, manager := startRenameManager(t) + missing := s3tablestest.Start(t) + manager.SetTrusted(false) + manager.SetDefaultAllow(false) + + for _, operation := range []string{"GetTable", "UpdateTable", "DeleteTable", "RenameTable"} { + t.Run(operation, func(t *testing.T) { + var request interface{} = &GetTableRequest{TableBucketARN: mustBucketARN(t), Namespace: []string{"ns"}, Name: "t"} + switch operation { + case "UpdateTable": + request = &UpdateTableRequest{TableBucketARN: mustBucketARN(t), Namespace: []string{"ns"}, Name: "t", VersionToken: "wrong"} + case "DeleteTable": + request = &DeleteTableRequest{TableBucketARN: mustBucketARN(t), Namespace: []string{"ns"}, Name: "t", VersionToken: "wrong"} + case "RenameTable": + request = &RenameTableRequest{TableBucketARN: mustBucketARN(t), SourceNamespace: []string{"ns"}, SourceName: "t", DestNamespace: []string{"ns"}, DestName: "t2"} + } + + want := runUnauthorizedRequest(t, manager, missing, operation, request) + got := runUnauthorizedRequest(t, manager, existing, operation, request) + assert.Equal(t, want, got) + assert.Equal(t, 404, got.status) + assert.Equal(t, ErrCodeNoSuchTable, got.body.Type) + }) + } +} + +func TestDeleteTableAuthorizedVersionMismatchStillConflicts(t *testing.T) { + fs, manager := startRenameManager(t) + err := manager.Execute(context.Background(), NewManagerClient(fs.Client), "DeleteTable", &DeleteTableRequest{ + TableBucketARN: mustBucketARN(t), + Namespace: []string{"ns"}, + Name: "t", + VersionToken: "wrong", + }, nil, "") + + require.Error(t, err) + var s3Err *S3TablesError + require.ErrorAs(t, err, &s3Err) + assert.Equal(t, ErrCodeConflict, s3Err.Type) + assert.NotNil(t, fs.Get(GetNamespacePath(renameTestBucket, "ns"), "t")) +} diff --git a/weed/s3api/s3tables/handler_namespace.go b/weed/s3api/s3tables/handler_namespace.go index 8a825c510..33213ab2d 100644 --- a/weed/s3api/s3tables/handler_namespace.go +++ b/weed/s3api/s3tables/handler_namespace.go @@ -261,7 +261,7 @@ func (h *S3TablesHandler) handleGetNamespace(w http.ResponseWriter, r *http.Requ IdentityActions: identityActions, DefaultAllow: h.defaultAllowFor(r), }) { - h.writeError(w, http.StatusNotFound, ErrCodeNoSuchNamespace, "namespace not found") + h.writeError(w, http.StatusNotFound, ErrCodeNoSuchNamespace, fmt.Sprintf("namespace %s not found", flattenNamespace(req.Namespace))) return ErrAccessDenied } @@ -351,7 +351,7 @@ func (h *S3TablesHandler) handleUpdateNamespace(w http.ResponseWriter, r *http.R IdentityActions: identityActions, DefaultAllow: h.defaultAllowFor(r), }) { - h.writeError(w, http.StatusForbidden, ErrCodeAccessDenied, "not authorized to update namespace") + h.writeError(w, http.StatusNotFound, ErrCodeNoSuchNamespace, fmt.Sprintf("namespace %s not found", flattenNamespace(req.Namespace))) return ErrAccessDenied } @@ -637,7 +637,7 @@ func (h *S3TablesHandler) handleDeleteNamespace(w http.ResponseWriter, r *http.R IdentityActions: identityActions, DefaultAllow: h.defaultAllowFor(r), }) { - h.writeError(w, http.StatusNotFound, ErrCodeNoSuchNamespace, "namespace not found") + h.writeError(w, http.StatusNotFound, ErrCodeNoSuchNamespace, fmt.Sprintf("namespace %s not found", flattenNamespace(req.Namespace))) return ErrAccessDenied } diff --git a/weed/s3api/s3tables/handler_rename_test.go b/weed/s3api/s3tables/handler_rename_test.go index 88fa99a63..326a3f3bb 100644 --- a/weed/s3api/s3tables/handler_rename_test.go +++ b/weed/s3api/s3tables/handler_rename_test.go @@ -169,7 +169,9 @@ func TestRenameTableDestNamespaceMissing(t *testing.T) { } // A principal allowed to rename the source must still be denied when it cannot -// create a table in the destination namespace. +// create a table in the destination namespace. The denial reports the same +// not-found as a missing destination namespace so the caller cannot probe for +// namespaces it may not touch. func TestRenameTableDestNamespaceUnauthorized(t *testing.T) { fs, m := startRenameManager(t) m.SetTrusted(false) @@ -204,7 +206,7 @@ func TestRenameTableDestNamespaceUnauthorized(t *testing.T) { require.Error(t, err) var s3Err *S3TablesError require.ErrorAs(t, err, &s3Err) - assert.Equal(t, ErrCodeAccessDenied, s3Err.Type) + assert.Equal(t, ErrCodeNoSuchNamespace, s3Err.Type) assert.NotNil(t, fs.Get(GetNamespacePath(renameTestBucket, "ns"), "t"), "source must be untouched") assert.Nil(t, fs.Get(GetNamespacePath(renameTestBucket, "dest"), "t2"), "destination must not be written") diff --git a/weed/s3api/s3tables/handler_table.go b/weed/s3api/s3tables/handler_table.go index 4eb7d88f6..af5cc7b77 100644 --- a/weed/s3api/s3tables/handler_table.go +++ b/weed/s3api/s3tables/handler_table.go @@ -898,12 +898,6 @@ func (h *S3TablesHandler) handleDeleteTable(w http.ResponseWriter, r *http.Reque return fmt.Errorf("failed to unmarshal table metadata: %w", err) } - if req.VersionToken != "" { - if metadata.VersionToken != req.VersionToken { - return ErrVersionTokenMismatch - } - } - // Fetch table policy if it exists policyData, err := h.getExtendedAttribute(r.Context(), client, tablePath, ExtendedKeyPolicy) if err != nil { @@ -980,9 +974,13 @@ func (h *S3TablesHandler) handleDeleteTable(w http.ResponseWriter, r *http.Reque DefaultAllow: h.defaultAllowFor(r), }) if !tableAllowed && !bucketAllowed { - h.writeError(w, http.StatusForbidden, ErrCodeAccessDenied, "not authorized to delete table") + h.writeError(w, http.StatusNotFound, ErrCodeNoSuchTable, fmt.Sprintf("table %s not found", tableName)) return NewAuthError("DeleteTable", principal, "not authorized to delete table") } + if req.VersionToken != "" && metadata.VersionToken != req.VersionToken { + h.writeError(w, http.StatusConflict, ErrCodeConflict, "version token mismatch") + return ErrVersionTokenMismatch + } // Delete the table err = filerClient.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error { @@ -1268,14 +1266,17 @@ func (h *S3TablesHandler) renameCatalogEntry(w http.ResponseWriter, r *http.Requ DefaultAllow: h.defaultAllowFor(r), }) if !tableAllowed && !bucketAllowed { - h.writeError(w, http.StatusForbidden, ErrCodeAccessDenied, "not authorized to rename "+kind.noun) + h.writeError(w, http.StatusNotFound, kind.notFoundCode, fmt.Sprintf("%s %s not found", kind.noun, srcName)) return NewAuthError(kind.renameOp, principal, "not authorized to rename "+kind.noun) } - // Require the destination namespace to exist and the destination table to be free. + // Require the destination namespace to exist. Whether the destination name + // is taken is only recorded here; reporting the conflict before the + // destination authorization check would disclose it to denied callers. destNamespacePath := GetNamespacePath(bucketName, destNamespace) var destNamespaceMetadata namespaceMetadata var destNamespacePolicy string + var destExists bool err = filerClient.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error { data, err := h.getExtendedAttribute(r.Context(), client, destNamespacePath, ExtendedKeyMetadata) if err != nil { @@ -1291,7 +1292,7 @@ func (h *S3TablesHandler) renameCatalogEntry(w http.ResponseWriter, r *http.Requ return fmt.Errorf("failed to fetch destination namespace policy: %w", err) } if _, err := h.getExtendedAttribute(r.Context(), client, destPath, ExtendedKeyMetadata); err == nil { - return ErrTableAlreadyExists + destExists = true } else if !errors.Is(err, filer_pb.ErrNotFound) && !errors.Is(err, ErrAttributeNotFound) { return err } @@ -1299,9 +1300,7 @@ func (h *S3TablesHandler) renameCatalogEntry(w http.ResponseWriter, r *http.Requ }) if err != nil { - if errors.Is(err, ErrTableAlreadyExists) { - h.writeError(w, http.StatusConflict, kind.existsCode, fmt.Sprintf("%s %s already exists", kind.noun, destName)) - } else if errors.Is(err, filer_pb.ErrNotFound) { + if errors.Is(err, filer_pb.ErrNotFound) { h.writeError(w, http.StatusNotFound, ErrCodeNoSuchNamespace, fmt.Sprintf("namespace %s not found", destNamespace)) } else { h.writeError(w, http.StatusInternalServerError, ErrCodeInternalError, fmt.Sprintf("failed to check destination: %v", err)) @@ -1311,7 +1310,8 @@ func (h *S3TablesHandler) renameCatalogEntry(w http.ResponseWriter, r *http.Requ // Renaming places the table into the destination namespace, so the principal // must also be allowed to create a table there (the source check alone lets a - // caller move tables into namespaces they don't control). + // caller move tables into namespaces they don't control). Denials report the + // same not-found as a missing destination namespace. destNamespaceAllowed := CheckPermissionWithContext(kind.createOp, principal, destNamespaceMetadata.OwnerAccountID, destNamespacePolicy, bucketARN, &PolicyContext{ TableBucketName: bucketName, Namespace: destNamespace, @@ -1329,10 +1329,15 @@ func (h *S3TablesHandler) renameCatalogEntry(w http.ResponseWriter, r *http.Requ DefaultAllow: h.defaultAllowFor(r), }) if !destNamespaceAllowed && !destBucketAllowed { - h.writeError(w, http.StatusForbidden, ErrCodeAccessDenied, "not authorized to create "+kind.noun+" in the destination namespace") + h.writeError(w, http.StatusNotFound, ErrCodeNoSuchNamespace, fmt.Sprintf("namespace %s not found", destNamespace)) return NewAuthError(kind.renameOp, principal, "not authorized to create "+kind.noun+" in the destination namespace") } + if destExists { + h.writeError(w, http.StatusConflict, kind.existsCode, fmt.Sprintf("%s %s already exists", kind.noun, destName)) + return ErrTableAlreadyExists + } + metadata.Name = destName metadata.Namespace = destNamespace metadata.ModifiedAt = time.Now() @@ -1538,7 +1543,7 @@ func (h *S3TablesHandler) handleUpdateTable(w http.ResponseWriter, r *http.Reque }) if !tableAllowed && !bucketAllowed { - h.writeError(w, http.StatusForbidden, ErrCodeAccessDenied, "not authorized to update table") + h.writeError(w, http.StatusNotFound, ErrCodeNoSuchTable, "table not found") return NewAuthError("UpdateTable", principal, "not authorized to update table") }