From 8c3d8fcea07e05cda1595deb84510206f44aead7 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Wed, 21 Oct 2026 13:07:47 -0700 Subject: [PATCH] s3tables: keep absent-target answers identical to denied ones (#11905) * s3tables: authorize before revealing a list or tag target exists ListTables, ListViews and the tag operations read the target's catalog metadata before any permission check, so a denied caller could tell an existing name (403 or the resource's own error) from an absent one (an empty list, or 404). Evaluate the operation's permission with whatever bucket context could be read and answer a denied caller the same way for absent, damaged and denied targets. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * s3tables: answer bucket reads uniformly when the entry is not a table bucket A plain S3 bucket directory answers GetTableBucket and ListNamespaces with 500 attribute-not-found while a missing name gets 404, and a denied CreateNamespace is 403 while a missing bucket is 404. Run the permission check before the absence answer and read not-a-table-bucket as NoSuchTableBucket. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * s3tables: cover absent-target answers matching denied ones Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --------- Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- weed/s3api/s3tables/handler.go | 53 ++++++++ .../handler_bucket_get_list_delete.go | 2 +- .../s3tables/handler_existence_hiding_test.go | 120 ++++++++++++++++++ weed/s3api/s3tables/handler_namespace.go | 6 +- weed/s3api/s3tables/handler_policy.go | 25 ++++ weed/s3api/s3tables/handler_table.go | 34 ++++- weed/s3api/s3tables/handler_view.go | 8 +- 7 files changed, 238 insertions(+), 10 deletions(-) diff --git a/weed/s3api/s3tables/handler.go b/weed/s3api/s3tables/handler.go index 608771a037..f5f4cc51cb 100644 --- a/weed/s3api/s3tables/handler.go +++ b/weed/s3api/s3tables/handler.go @@ -459,6 +459,59 @@ func isAuthError(err error) bool { return errors.As(err, &authErr) || errors.Is(err, ErrAccessDenied) } +// authorizeAbsentTarget reports whether the caller may learn the target +// resolved to nothing, so a caller without a grant gets the same response a +// denied existing target would give. The bucket named by the target still +// applies whatever metadata and policy load; with nothing readable to +// evaluate, only an identity-level grant on the operation, the admin, or a +// default-allow deployment passes. +func (h *S3TablesHandler) authorizeAbsentTarget(r *http.Request, filerClient FilerClient, operation, bucketName, namespace, name, resourceARN string) bool { + principal := h.getAccountID(r) + if principal == "" { + return false + } + + var bucketOwner, bucketPolicy string + var bucketTags map[string]string + if bucketName != "" { + bucketPath := GetTableBucketPath(bucketName) + _ = filerClient.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error { + if data, err := h.getExtendedAttribute(r.Context(), client, bucketPath, ExtendedKeyMetadata); err == nil { + var meta tableBucketMetadata + if json.Unmarshal(data, &meta) == nil { + bucketOwner = meta.OwnerAccountID + } + } + if policy, err := h.readPolicy(r, client, bucketPath); err == nil { + bucketPolicy = policy + } + bucketTags, _ = h.readTags(r.Context(), client, bucketPath) + return nil + }) + } + + bucketARN := h.generateTableBucketARN(bucketOwner, bucketName) + ctx := &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), + IsLocalAccountID: h.localAccountIDs, + TableBucketName: bucketName, + Namespace: namespace, + TableName: name, + TableBucketTags: bucketTags, + IdentityActions: getIdentityActions(r), + DefaultAllow: h.defaultAllowFor(r), + } + if resourceARN != "" && + matchesDenyStatement(operation, principal, bucketPolicy, resourceARN, ctx) { + return false + } + if CheckTableOrBucketPermission(operation, principal, "", "", resourceARN, + bucketOwner, bucketPolicy, bucketARN, ctx) { + return true + } + return bucketOwner == "" && checkPermission(operation, principal, "", "", resourceARN, ctx) +} + func (h *S3TablesHandler) readTags(ctx context.Context, client filer_pb.SeaweedFilerClient, path string) (map[string]string, error) { data, err := h.getExtendedAttribute(ctx, client, path, ExtendedKeyTags) if err != nil { diff --git a/weed/s3api/s3tables/handler_bucket_get_list_delete.go b/weed/s3api/s3tables/handler_bucket_get_list_delete.go index ce1e8147b1..1645194ca5 100644 --- a/weed/s3api/s3tables/handler_bucket_get_list_delete.go +++ b/weed/s3api/s3tables/handler_bucket_get_list_delete.go @@ -58,7 +58,7 @@ func (h *S3TablesHandler) handleGetTableBucket(w http.ResponseWriter, r *http.Re }) if err != nil { - if errors.Is(err, filer_pb.ErrNotFound) { + if errors.Is(err, filer_pb.ErrNotFound) || errors.Is(err, ErrAttributeNotFound) { 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 get table bucket: %v", err)) diff --git a/weed/s3api/s3tables/handler_existence_hiding_test.go b/weed/s3api/s3tables/handler_existence_hiding_test.go index dc04e09423..cc1e051777 100644 --- a/weed/s3api/s3tables/handler_existence_hiding_test.go +++ b/weed/s3api/s3tables/handler_existence_hiding_test.go @@ -211,3 +211,123 @@ func TestDeniedTableWritesDoNotRevealViews(t *testing.T) { }) } } + +// A caller without a grant must get the same answer whether a list target +// exists or not, and whether the bucket behind it exists at all. +func TestAbsentListTargetsMatchDeniedAnswers(t *testing.T) { + existing, manager := startRenameManager(t) + missing := s3tablestest.Start(t) + manager.SetTrusted(false) + manager.SetDefaultAllow(false) + + for _, tc := range []struct { + name string + fs *s3tablestest.MemFiler + operation string + denied interface{} + request interface{} + }{ + {"ListTables missing namespace", existing, "ListTables", + &ListTablesRequest{TableBucketARN: mustBucketARN(t), Namespace: []string{"ns"}}, + &ListTablesRequest{TableBucketARN: mustBucketARN(t), Namespace: []string{"ghost"}}}, + {"ListTables missing bucket", missing, "ListTables", + &ListTablesRequest{TableBucketARN: mustBucketARN(t), Namespace: []string{"ns"}}, + &ListTablesRequest{TableBucketARN: mustBucketARN(t), Namespace: []string{"ns"}}}, + {"ListTables missing bucket, all namespaces", missing, "ListTables", + &ListTablesRequest{TableBucketARN: mustBucketARN(t), Namespace: []string{"ns"}}, + &ListTablesRequest{TableBucketARN: mustBucketARN(t)}}, + {"ListViews missing namespace", existing, "ListViews", + &ListViewsRequest{TableBucketARN: mustBucketARN(t), Namespace: []string{"ns"}}, + &ListViewsRequest{TableBucketARN: mustBucketARN(t), Namespace: []string{"ghost"}}}, + {"CreateNamespace missing bucket", missing, "CreateNamespace", + &CreateNamespaceRequest{TableBucketARN: mustBucketARN(t), Namespace: []string{"newns"}}, + &CreateNamespaceRequest{TableBucketARN: mustBucketARN(t), Namespace: []string{"newns"}}}, + } { + t.Run(tc.name, func(t *testing.T) { + denied := runUnauthorizedRequest(t, manager, existing, tc.operation, tc.denied) + assert.Equal(t, 403, denied.status) + got := runUnauthorizedRequest(t, manager, tc.fs, tc.operation, tc.request) + assert.Equal(t, denied, got) + }) + } +} + +// A denied tag operation and the same operation on a missing table must answer +// alike; a granted caller still gets the truthful not-found. +func TestAbsentTagTargetsMatchDeniedAnswers(t *testing.T) { + existing, manager := startRenameManager(t) + manager.SetTrusted(false) + manager.SetDefaultAllow(false) + + existingARN, err := BuildTableARN(DefaultRegion, DefaultAccountID, renameTestBucket, "ns", "t") + require.NoError(t, err) + missingARN, err := BuildTableARN(DefaultRegion, DefaultAccountID, renameTestBucket, "ns", "ghost") + require.NoError(t, err) + + requestFor := func(operation, arn string) interface{} { + switch operation { + case "TagResource": + return &TagResourceRequest{ResourceARN: arn, Tags: map[string]string{"k": "v"}} + case "UntagResource": + return &UntagResourceRequest{ResourceARN: arn, TagKeys: []string{"k"}} + } + return &ListTagsForResourceRequest{ResourceARN: arn} + } + + for _, operation := range []string{"TagResource", "ListTagsForResource", "UntagResource"} { + t.Run(operation, func(t *testing.T) { + denied := runUnauthorizedRequest(t, manager, existing, operation, requestFor(operation, existingARN)) + assert.Equal(t, 403, denied.status) + got := runUnauthorizedRequest(t, manager, existing, operation, requestFor(operation, missingARN)) + assert.Equal(t, denied, got) + }) + } +} + +// A directory that holds a plain S3 bucket name is not a table bucket: every +// caller gets the same 404 an absent name gets, not a 500 that reveals the +// name resolves in the filer. +func TestPlainBucketReadsAsAbsent(t *testing.T) { + existing, manager := startRenameManager(t) + manager.SetTrusted(false) + manager.SetDefaultAllow(false) + existing.Put(TablesPath, "plainbkt", nil) + + plainARN, err := BuildBucketARN(DefaultRegion, DefaultAccountID, "plainbkt") + require.NoError(t, err) + + for _, tc := range []struct { + operation string + request interface{} + }{ + {"GetTableBucket", &GetTableBucketRequest{TableBucketARN: plainARN}}, + {"ListNamespaces", &ListNamespacesRequest{TableBucketARN: plainARN}}, + } { + t.Run(tc.operation, func(t *testing.T) { + got := runUnauthorizedRequest(t, manager, existing, tc.operation, tc.request) + assert.Equal(t, 404, got.status) + assert.Equal(t, ErrCodeNoSuchBucket, got.body.Type) + }) + } +} + +// A caller with an identity-level grant still learns that a name is absent. +func TestGrantedCallerLearnsAbsence(t *testing.T) { + missing := s3tablestest.Start(t) + manager := NewManager() + manager.SetTrusted(false) + manager.SetDefaultAllow(false) + + got := runRequestAs(t, manager, missing, "ListTables", &ListTablesRequest{ + TableBucketARN: mustBucketARN(t), Namespace: []string{"ns"}}, + []string{"s3tables:ListTables"}) + assert.Equal(t, 200, got.status) + + missingTableARN, err := BuildTableARN(DefaultRegion, DefaultAccountID, renameTestBucket, "ns", "ghost") + require.NoError(t, err) + got = runRequestAs(t, manager, missing, "ListTagsForResource", + &ListTagsForResourceRequest{ResourceARN: missingTableARN}, + []string{"s3tables:ListTagsForResource"}) + assert.Equal(t, 404, got.status) + assert.Equal(t, ErrCodeNoSuchTable, got.body.Type) +} diff --git a/weed/s3api/s3tables/handler_namespace.go b/weed/s3api/s3tables/handler_namespace.go index 8e6a7ee6d5..613a85045d 100644 --- a/weed/s3api/s3tables/handler_namespace.go +++ b/weed/s3api/s3tables/handler_namespace.go @@ -140,6 +140,10 @@ func (h *S3TablesHandler) handleCreateNamespace(w http.ResponseWriter, r *http.R return ErrAccessDenied } if errors.Is(err, filer_pb.ErrNotFound) { + if !h.authorizeAbsentTarget(r, filerClient, "CreateNamespace", bucketName, namespaceName, "", "") { + h.writeError(w, http.StatusForbidden, ErrCodeAccessDenied, "not authorized to create namespace in this bucket") + return ErrAccessDenied + } 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 check table bucket: %v", err)) @@ -481,7 +485,7 @@ func (h *S3TablesHandler) handleListNamespaces(w http.ResponseWriter, r *http.Re }) if err != nil { - if errors.Is(err, filer_pb.ErrNotFound) { + if errors.Is(err, filer_pb.ErrNotFound) || errors.Is(err, ErrAttributeNotFound) { 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 list namespaces: %v", err)) diff --git a/weed/s3api/s3tables/handler_policy.go b/weed/s3api/s3tables/handler_policy.go index bea4f3fc19..c152b209a9 100644 --- a/weed/s3api/s3tables/handler_policy.go +++ b/weed/s3api/s3tables/handler_policy.go @@ -659,6 +659,9 @@ func (h *S3TablesHandler) handleTagResource(w http.ResponseWriter, r *http.Reque // Read metadata for ownership check data, err := h.getExtendedAttribute(r.Context(), client, resourcePath, ExtendedKeyMetadata) if err != nil { + if errors.Is(err, filer_pb.ErrNotFound) || errors.Is(err, ErrAttributeNotFound) { + return h.absentTagTargetError(r, client, "TagResource", "not authorized to tag resource", req.ResourceARN, rType) + } return err } @@ -793,6 +796,9 @@ func (h *S3TablesHandler) handleListTagsForResource(w http.ResponseWriter, r *ht // Read metadata for ownership check data, err := h.getExtendedAttribute(r.Context(), client, resourcePath, ExtendedKeyMetadata) if err != nil { + if errors.Is(err, filer_pb.ErrNotFound) || errors.Is(err, ErrAttributeNotFound) { + return h.absentTagTargetError(r, client, "ListTagsForResource", "not authorized to list tags for resource", req.ResourceARN, rType) + } return err } @@ -913,6 +919,9 @@ func (h *S3TablesHandler) handleUntagResource(w http.ResponseWriter, r *http.Req // Read metadata for ownership check data, err := h.getExtendedAttribute(r.Context(), client, resourcePath, ExtendedKeyMetadata) if err != nil { + if errors.Is(err, filer_pb.ErrNotFound) || errors.Is(err, ErrAttributeNotFound) { + return h.absentTagTargetError(r, client, "UntagResource", "not authorized to untag resource", req.ResourceARN, rType) + } return err } @@ -1017,6 +1026,22 @@ func (h *S3TablesHandler) handleUntagResource(w http.ResponseWriter, r *http.Req return nil } +// absentTagTargetError resolves a tag operation when the resource's catalog +// metadata did not load: a caller the operation would refuse gets its denial, +// everyone else the same not-found an absent name gets. +func (h *S3TablesHandler) absentTagTargetError(r *http.Request, client filer_pb.SeaweedFilerClient, operation, deniedMessage, resourceARN string, rType ResourceType) error { + var bucketName, namespace, name string + if rType == ResourceTypeTable { + bucketName, namespace, name, _ = parseTableFromARN(resourceARN) + } else { + bucketName, _ = parseBucketNameFromARN(resourceARN) + } + if !h.authorizeAbsentTarget(r, &singleClient{client}, operation, bucketName, namespace, name, resourceARN) { + return NewAuthError(operation, h.getAccountID(r), deniedMessage) + } + return filer_pb.ErrNotFound +} + // resolveResourcePath determines the resource path and extended attribute key from a resource ARN func (h *S3TablesHandler) resolveResourcePath(resourceARN string) (path string, key string, rType ResourceType, err error) { // Try parsing as table ARN first diff --git a/weed/s3api/s3tables/handler_table.go b/weed/s3api/s3tables/handler_table.go index b9ce7d8834..5ee6f01391 100644 --- a/weed/s3api/s3tables/handler_table.go +++ b/weed/s3api/s3tables/handler_table.go @@ -515,11 +515,14 @@ func (h *S3TablesHandler) handleListTables(w http.ResponseWriter, r *http.Reques bucketTags := map[string]string{} // Fetch namespace metadata and policy + targetAbsent := false data, err := h.getExtendedAttribute(r.Context(), client, namespacePath, ExtendedKeyMetadata) if err != nil { - return err // Not Found handled by caller - } - if err := json.Unmarshal(data, &nsMeta); err != nil { + if !errors.Is(err, filer_pb.ErrNotFound) && !errors.Is(err, ErrAttributeNotFound) { + return err + } + targetAbsent = true + } else if err := json.Unmarshal(data, &nsMeta); err != nil { return err } @@ -527,7 +530,7 @@ func (h *S3TablesHandler) handleListTables(w http.ResponseWriter, r *http.Reques policyData, err := h.getExtendedAttribute(r.Context(), client, namespacePath, ExtendedKeyPolicy) if err == nil { namespacePolicy = string(policyData) - } else if !errors.Is(err, ErrAttributeNotFound) { + } else if !errors.Is(err, ErrAttributeNotFound) && !errors.Is(err, filer_pb.ErrNotFound) { return fmt.Errorf("failed to fetch namespace policy: %w", err) } @@ -537,6 +540,8 @@ func (h *S3TablesHandler) handleListTables(w http.ResponseWriter, r *http.Reques if err := json.Unmarshal(data, &bucketMeta); err != nil { return fmt.Errorf("failed to unmarshal bucket metadata: %w", err) } + } else if errors.Is(err, filer_pb.ErrNotFound) { + targetAbsent = true } else if !errors.Is(err, ErrAttributeNotFound) { return fmt.Errorf("failed to fetch bucket metadata: %w", err) } @@ -544,15 +549,24 @@ func (h *S3TablesHandler) handleListTables(w http.ResponseWriter, r *http.Reques policyData, err = h.getExtendedAttribute(r.Context(), client, bucketPath, ExtendedKeyPolicy) if err == nil { bucketPolicy = string(policyData) - } else if !errors.Is(err, ErrAttributeNotFound) { + } else if !errors.Is(err, ErrAttributeNotFound) && !errors.Is(err, filer_pb.ErrNotFound) { return fmt.Errorf("failed to fetch bucket policy: %w", err) } if tags, err := h.readTags(r.Context(), client, bucketPath); err != nil { - return fmt.Errorf("failed to read bucket tags: %w", err) + if !errors.Is(err, filer_pb.ErrNotFound) { + return fmt.Errorf("failed to read bucket tags: %w", err) + } } else if tags != nil { bucketTags = tags } + if targetAbsent { + if !h.authorizeAbsentTarget(r, &singleClient{client}, "ListTables", bucketName, namespaceName, "", "") { + return ErrAccessDenied + } + return filer_pb.ErrNotFound + } + bucketARN := h.generateTableBucketARN(bucketMeta.OwnerAccountID, bucketName) identityActions := getIdentityActions(r) if !CheckTableOrBucketPermission("ListTables", accountID, @@ -583,7 +597,13 @@ func (h *S3TablesHandler) handleListTables(w http.ResponseWriter, r *http.Reques // Fetch bucket metadata and policy data, err := h.getExtendedAttribute(r.Context(), client, bucketPath, ExtendedKeyMetadata) if err != nil { - return fmt.Errorf("failed to fetch bucket metadata: %w", err) + if !errors.Is(err, filer_pb.ErrNotFound) && !errors.Is(err, ErrAttributeNotFound) { + return fmt.Errorf("failed to fetch bucket metadata: %w", err) + } + if !h.authorizeAbsentTarget(r, &singleClient{client}, "ListTables", bucketName, "", "", "") { + return ErrAccessDenied + } + return filer_pb.ErrNotFound } if err := json.Unmarshal(data, &bucketMeta); err != nil { return fmt.Errorf("failed to unmarshal bucket metadata: %w", err) diff --git a/weed/s3api/s3tables/handler_view.go b/weed/s3api/s3tables/handler_view.go index d3327783f8..57bad00c5c 100644 --- a/weed/s3api/s3tables/handler_view.go +++ b/weed/s3api/s3tables/handler_view.go @@ -279,7 +279,13 @@ func (h *S3TablesHandler) handleListViews(w http.ResponseWriter, r *http.Request var nsMeta namespaceMetadata data, err := h.getExtendedAttribute(r.Context(), client, namespacePath, ExtendedKeyMetadata) if err != nil { - return err + if !errors.Is(err, filer_pb.ErrNotFound) && !errors.Is(err, ErrAttributeNotFound) { + return err + } + if !h.authorizeAbsentTarget(r, &singleClient{client}, "ListViews", bucketName, namespaceName, "", "") { + return ErrAccessDenied + } + return filer_pb.ErrNotFound } if err := json.Unmarshal(data, &nsMeta); err != nil { return err