From dc4c62e742de323ffe7ba53e75a7151bdba116de Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Wed, 28 Jan 2026 11:49:57 -0800 Subject: [PATCH] s3tables: harden auth and error handling - Add authorization checks to all S3 Tables handlers (policy, table ops) to enforce security - Improve error handling to distinguish between NotFound (404) and InternalError (500) - Fix directory FileMode usage in filer_ops - Improve test randomness for version tokens - Update permissions comments to acknowledge IAM gaps --- test/s3tables/s3tables_integration_test.go | 2 +- weed/s3api/s3tables/handler_policy.go | 86 +++++++++++++++++++++- weed/s3api/s3tables/handler_table.go | 36 ++++++++- weed/s3api/s3tables/permissions.go | 3 + 4 files changed, 122 insertions(+), 5 deletions(-) diff --git a/test/s3tables/s3tables_integration_test.go b/test/s3tables/s3tables_integration_test.go index ac7fcb0d5..12aef1077 100644 --- a/test/s3tables/s3tables_integration_test.go +++ b/test/s3tables/s3tables_integration_test.go @@ -495,7 +495,7 @@ func waitForS3Ready(endpoint string, timeout time.Duration) error { // randomString generates a random string for unique naming func randomString(length int) string { const charset = "abcdefghijklmnopqrstuvwxyz0123456789" - rng := rand.New(rand.NewSource(time.Now().UnixNano())) + rng := rand.New(rand.NewSource(time.Now().UnixNano() + rand.Int63())) b := make([]byte, length) for i := range b { b[i] = charset[rng.Intn(len(charset))] diff --git a/weed/s3api/s3tables/handler_policy.go b/weed/s3api/s3tables/handler_policy.go index d16ab879a..516c979e6 100644 --- a/weed/s3api/s3tables/handler_policy.go +++ b/weed/s3api/s3tables/handler_policy.go @@ -11,6 +11,13 @@ import ( // handlePutTableBucketPolicy puts a policy on a table bucket func (h *S3TablesHandler) handlePutTableBucketPolicy(w http.ResponseWriter, r *http.Request, filerClient FilerClient) error { + // Check permission + principal := h.getPrincipalFromRequest(r) + if !CheckPermission("PutTableBucketPolicy", principal, h.accountID) { + h.writeError(w, http.StatusForbidden, ErrCodeAccessDenied, "not authorized to put table bucket policy") + return NewAuthError("PutTableBucketPolicy", principal, "not authorized to put table bucket policy") + } + var req PutTableBucketPolicyRequest if err := h.readRequestBody(r, &req); err != nil { h.writeError(w, http.StatusBadRequest, ErrCodeInvalidRequest, err.Error()) @@ -65,6 +72,13 @@ func (h *S3TablesHandler) handlePutTableBucketPolicy(w http.ResponseWriter, r *h // handleGetTableBucketPolicy gets the policy of a table bucket func (h *S3TablesHandler) handleGetTableBucketPolicy(w http.ResponseWriter, r *http.Request, filerClient FilerClient) error { + // Check permission + principal := h.getPrincipalFromRequest(r) + if !CheckPermission("GetTableBucketPolicy", principal, h.accountID) { + h.writeError(w, http.StatusForbidden, ErrCodeAccessDenied, "not authorized to get table bucket policy") + return NewAuthError("GetTableBucketPolicy", principal, "not authorized to get table bucket policy") + } + var req GetTableBucketPolicyRequest if err := h.readRequestBody(r, &req); err != nil { h.writeError(w, http.StatusBadRequest, ErrCodeInvalidRequest, err.Error()) @@ -91,7 +105,11 @@ func (h *S3TablesHandler) handleGetTableBucketPolicy(w http.ResponseWriter, r *h }) if err != nil { - h.writeError(w, http.StatusNotFound, ErrCodeNoSuchPolicy, "table bucket policy not found") + if errors.Is(err, filer_pb.ErrNotFound) || errors.Is(err, ErrAttributeNotFound) { + h.writeError(w, http.StatusNotFound, ErrCodeNoSuchPolicy, "table bucket policy not found") + } else { + h.writeError(w, http.StatusInternalServerError, ErrCodeInternalError, fmt.Sprintf("failed to get table bucket policy: %v", err)) + } return err } @@ -105,6 +123,13 @@ func (h *S3TablesHandler) handleGetTableBucketPolicy(w http.ResponseWriter, r *h // handleDeleteTableBucketPolicy deletes the policy of a table bucket func (h *S3TablesHandler) handleDeleteTableBucketPolicy(w http.ResponseWriter, r *http.Request, filerClient FilerClient) error { + // Check permission + principal := h.getPrincipalFromRequest(r) + if !CheckPermission("DeleteTableBucketPolicy", principal, h.accountID) { + h.writeError(w, http.StatusForbidden, ErrCodeAccessDenied, "not authorized to delete table bucket policy") + return NewAuthError("DeleteTableBucketPolicy", principal, "not authorized to delete table bucket policy") + } + var req DeleteTableBucketPolicyRequest if err := h.readRequestBody(r, &req); err != nil { h.writeError(w, http.StatusBadRequest, ErrCodeInvalidRequest, err.Error()) @@ -138,6 +163,13 @@ func (h *S3TablesHandler) handleDeleteTableBucketPolicy(w http.ResponseWriter, r // handlePutTablePolicy puts a policy on a table func (h *S3TablesHandler) handlePutTablePolicy(w http.ResponseWriter, r *http.Request, filerClient FilerClient) error { + // Check permission + principal := h.getPrincipalFromRequest(r) + if !CheckPermission("PutTablePolicy", principal, h.accountID) { + h.writeError(w, http.StatusForbidden, ErrCodeAccessDenied, "not authorized to put table policy") + return NewAuthError("PutTablePolicy", principal, "not authorized to put table policy") + } + var req PutTablePolicyRequest if err := h.readRequestBody(r, &req); err != nil { h.writeError(w, http.StatusBadRequest, ErrCodeInvalidRequest, err.Error()) @@ -198,6 +230,13 @@ func (h *S3TablesHandler) handlePutTablePolicy(w http.ResponseWriter, r *http.Re // handleGetTablePolicy gets the policy of a table func (h *S3TablesHandler) handleGetTablePolicy(w http.ResponseWriter, r *http.Request, filerClient FilerClient) error { + // Check permission + principal := h.getPrincipalFromRequest(r) + if !CheckPermission("GetTablePolicy", principal, h.accountID) { + h.writeError(w, http.StatusForbidden, ErrCodeAccessDenied, "not authorized to get table policy") + return NewAuthError("GetTablePolicy", principal, "not authorized to get table policy") + } + var req GetTablePolicyRequest if err := h.readRequestBody(r, &req); err != nil { h.writeError(w, http.StatusBadRequest, ErrCodeInvalidRequest, err.Error()) @@ -230,7 +269,11 @@ func (h *S3TablesHandler) handleGetTablePolicy(w http.ResponseWriter, r *http.Re }) if err != nil { - h.writeError(w, http.StatusNotFound, ErrCodeNoSuchPolicy, "table policy not found") + if errors.Is(err, filer_pb.ErrNotFound) || errors.Is(err, ErrAttributeNotFound) { + h.writeError(w, http.StatusNotFound, ErrCodeNoSuchPolicy, "table policy not found") + } else { + h.writeError(w, http.StatusInternalServerError, ErrCodeInternalError, fmt.Sprintf("failed to get table policy: %v", err)) + } return err } @@ -244,6 +287,13 @@ func (h *S3TablesHandler) handleGetTablePolicy(w http.ResponseWriter, r *http.Re // handleDeleteTablePolicy deletes the policy of a table func (h *S3TablesHandler) handleDeleteTablePolicy(w http.ResponseWriter, r *http.Request, filerClient FilerClient) error { + // Check permission + principal := h.getPrincipalFromRequest(r) + if !CheckPermission("DeleteTablePolicy", principal, h.accountID) { + h.writeError(w, http.StatusForbidden, ErrCodeAccessDenied, "not authorized to delete table policy") + return NewAuthError("DeleteTablePolicy", principal, "not authorized to delete table policy") + } + var req DeleteTablePolicyRequest if err := h.readRequestBody(r, &req); err != nil { h.writeError(w, http.StatusBadRequest, ErrCodeInvalidRequest, err.Error()) @@ -299,6 +349,13 @@ func (h *S3TablesHandler) handleTagResource(w http.ResponseWriter, r *http.Reque return fmt.Errorf("tags are required") } + // Check permission + principal := h.getPrincipalFromRequest(r) + if !CanManageTags(principal, h.accountID) { + h.writeError(w, http.StatusForbidden, ErrCodeAccessDenied, "not authorized to tag resource") + return NewAuthError("TagResource", principal, "not authorized to tag resource") + } + // Parse resource ARN to determine if it's a bucket or table resourcePath, extendedKey, err := h.resolveResourcePath(req.ResourceARN) if err != nil { @@ -311,10 +368,14 @@ func (h *S3TablesHandler) handleTagResource(w http.ResponseWriter, r *http.Reque err = filerClient.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error { data, err := h.getExtendedAttribute(r.Context(), client, resourcePath, extendedKey) if err == nil { - json.Unmarshal(data, &existingTags) + return json.Unmarshal(data, &existingTags) } return nil }) + if err != nil { + h.writeError(w, http.StatusInternalServerError, ErrCodeInternalError, fmt.Sprintf("failed to read existing tags: %v", err)) + return err + } // Merge new tags for k, v := range req.Tags { @@ -342,6 +403,13 @@ func (h *S3TablesHandler) handleTagResource(w http.ResponseWriter, r *http.Reque // handleListTagsForResource lists tags for a resource func (h *S3TablesHandler) handleListTagsForResource(w http.ResponseWriter, r *http.Request, filerClient FilerClient) error { + // Check permission + principal := h.getPrincipalFromRequest(r) + if !CheckPermission("ListTagsForResource", principal, h.accountID) { + h.writeError(w, http.StatusForbidden, ErrCodeAccessDenied, "not authorized to list tags for resource") + return NewAuthError("ListTagsForResource", principal, "not authorized to list tags for resource") + } + var req ListTagsForResourceRequest if err := h.readRequestBody(r, &req); err != nil { h.writeError(w, http.StatusBadRequest, ErrCodeInvalidRequest, err.Error()) @@ -368,6 +436,11 @@ func (h *S3TablesHandler) handleListTagsForResource(w http.ResponseWriter, r *ht return json.Unmarshal(data, &tags) }) + if err != nil { + h.writeError(w, http.StatusInternalServerError, ErrCodeInternalError, fmt.Sprintf("failed to list tags: %v", err)) + return err + } + resp := &ListTagsForResourceResponse{ Tags: tags, } @@ -394,6 +467,13 @@ func (h *S3TablesHandler) handleUntagResource(w http.ResponseWriter, r *http.Req return fmt.Errorf("tagKeys are required") } + // Check permission + principal := h.getPrincipalFromRequest(r) + if !CheckPermission("UntagResource", principal, h.accountID) { + h.writeError(w, http.StatusForbidden, ErrCodeAccessDenied, "not authorized to untag resource") + return NewAuthError("UntagResource", principal, "not authorized to untag resource") + } + resourcePath, extendedKey, err := h.resolveResourcePath(req.ResourceARN) if err != nil { h.writeError(w, http.StatusBadRequest, ErrCodeInvalidRequest, err.Error()) diff --git a/weed/s3api/s3tables/handler_table.go b/weed/s3api/s3tables/handler_table.go index 6b5858580..01e407622 100644 --- a/weed/s3api/s3tables/handler_table.go +++ b/weed/s3api/s3tables/handler_table.go @@ -9,11 +9,19 @@ import ( "strings" "time" + "github.com/seaweedfs/seaweedfs/weed/glog" "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" ) // handleCreateTable creates a new table in a namespace func (h *S3TablesHandler) handleCreateTable(w http.ResponseWriter, r *http.Request, filerClient FilerClient) error { + // Check permission + principal := h.getPrincipalFromRequest(r) + if !CanCreateTable(principal, h.accountID) { + h.writeError(w, http.StatusForbidden, ErrCodeAccessDenied, "not authorized to create table") + return NewAuthError("CreateTable", principal, "not authorized to create table") + } + var req CreateTableRequest if err := h.readRequestBody(r, &req); err != nil { h.writeError(w, http.StatusBadRequest, ErrCodeInvalidRequest, err.Error()) @@ -172,6 +180,13 @@ func (h *S3TablesHandler) handleCreateTable(w http.ResponseWriter, r *http.Reque // handleGetTable gets details of a table func (h *S3TablesHandler) handleGetTable(w http.ResponseWriter, r *http.Request, filerClient FilerClient) error { + // Check permission + principal := h.getPrincipalFromRequest(r) + if !CanGetTable(principal, h.accountID) { + h.writeError(w, http.StatusForbidden, ErrCodeAccessDenied, "not authorized to get table") + return NewAuthError("GetTable", principal, "not authorized to get table") + } + var req GetTableRequest if err := h.readRequestBody(r, &req); err != nil { h.writeError(w, http.StatusBadRequest, ErrCodeInvalidRequest, err.Error()) @@ -217,7 +232,11 @@ func (h *S3TablesHandler) handleGetTable(w http.ResponseWriter, r *http.Request, }) if err != nil { - h.writeError(w, http.StatusNotFound, ErrCodeNoSuchTable, fmt.Sprintf("table %s not found", tableName)) + if errors.Is(err, filer_pb.ErrNotFound) { + h.writeError(w, http.StatusNotFound, ErrCodeNoSuchTable, fmt.Sprintf("table %s not found", tableName)) + } else { + h.writeError(w, http.StatusInternalServerError, ErrCodeInternalError, fmt.Sprintf("failed to get table: %v", err)) + } return err } @@ -241,6 +260,13 @@ func (h *S3TablesHandler) handleGetTable(w http.ResponseWriter, r *http.Request, // handleListTables lists all tables in a namespace or bucket func (h *S3TablesHandler) handleListTables(w http.ResponseWriter, r *http.Request, filerClient FilerClient) error { + // Check permission + principal := h.getPrincipalFromRequest(r) + if !CanListTables(principal, h.accountID) { + h.writeError(w, http.StatusForbidden, ErrCodeAccessDenied, "not authorized to list tables") + return NewAuthError("ListTables", principal, "not authorized to list tables") + } + var req ListTablesRequest if err := h.readRequestBody(r, &req); err != nil { h.writeError(w, http.StatusBadRequest, ErrCodeInvalidRequest, err.Error()) @@ -410,6 +436,7 @@ func (h *S3TablesHandler) listTablesInAllNamespaces(ctx context.Context, filerCl // List tables in this namespace if err := h.listTablesInNamespaceWithClient(ctx, client, bucketName, namespace, prefix, maxTables-len(*tables), tables); err != nil { + glog.Warningf("S3Tables: failed to list tables in namespace %s/%s: %v", bucketName, namespace, err) continue } @@ -429,6 +456,13 @@ func (h *S3TablesHandler) listTablesInAllNamespaces(ctx context.Context, filerCl // handleDeleteTable deletes a table from a namespace func (h *S3TablesHandler) handleDeleteTable(w http.ResponseWriter, r *http.Request, filerClient FilerClient) error { + // Check permission + principal := h.getPrincipalFromRequest(r) + if !CanDeleteTable(principal, h.accountID) { + h.writeError(w, http.StatusForbidden, ErrCodeAccessDenied, "not authorized to delete table") + return NewAuthError("DeleteTable", principal, "not authorized to delete table") + } + var req DeleteTableRequest if err := h.readRequestBody(r, &req); err != nil { h.writeError(w, http.StatusBadRequest, ErrCodeInvalidRequest, err.Error()) diff --git a/weed/s3api/s3tables/permissions.go b/weed/s3api/s3tables/permissions.go index 5fc04f803..2725f8e31 100644 --- a/weed/s3api/s3tables/permissions.go +++ b/weed/s3api/s3tables/permissions.go @@ -94,6 +94,7 @@ func CheckPermission(operation, principal, owner string) bool { // For now, only the owner can perform operations // This can be extended to support more granular permissions via policies + // TODO: Integrate with full IAM policy evaluation return false } @@ -181,8 +182,10 @@ func ExtractPrincipalFromContext(contextID string) string { } } + // Extract from context, e.g., "user123" or "account-id" // Extract from context, e.g., "user123" or "account-id" // This is a simplified version - in production, this would parse AWS auth headers + // TODO: Parse AWS Signature V4 identity or mTLS identity if strings.Contains(contextID, ":") { parts := strings.Split(contextID, ":") return parts[0]