diff --git a/weed/s3api/s3tables/handler_table.go b/weed/s3api/s3tables/handler_table.go index f76ed8879..8111480f3 100644 --- a/weed/s3api/s3tables/handler_table.go +++ b/weed/s3api/s3tables/handler_table.go @@ -419,28 +419,20 @@ func (h *S3TablesHandler) handleGetTable(w http.ResponseWriter, r *http.Request, tableARN := h.generateTableARN(metadata.OwnerAccountID, bucketName, namespace+"/"+tableName) bucketARN := h.generateTableBucketARN(bucketMetadata.OwnerAccountID, bucketName) identityActions := getIdentityActions(r) - tableAllowed := CheckPermissionWithContext("GetTable", accountID, metadata.OwnerAccountID, tablePolicy, tableARN, &PolicyContext{ - TrustedIssuers: h.trustedIssuers(r), - TableBucketName: bucketName, - Namespace: namespace, - TableName: tableName, - TableBucketTags: bucketTags, - ResourceTags: tableTags, - IdentityActions: identityActions, - DefaultAllow: h.defaultAllowFor(r), - }) - bucketAllowed := CheckPermissionWithContext("GetTable", accountID, bucketMetadata.OwnerAccountID, bucketPolicy, bucketARN, &PolicyContext{ - TrustedIssuers: h.trustedIssuers(r), - TableBucketName: bucketName, - Namespace: namespace, - TableName: tableName, - TableBucketTags: bucketTags, - ResourceTags: tableTags, - IdentityActions: identityActions, - DefaultAllow: h.defaultAllowFor(r), - }) + allowed := CheckTableOrBucketPermission("GetTable", accountID, + metadata.OwnerAccountID, tablePolicy, tableARN, + bucketMetadata.OwnerAccountID, bucketPolicy, bucketARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), + TableBucketName: bucketName, + Namespace: namespace, + TableName: tableName, + TableBucketTags: bucketTags, + ResourceTags: tableTags, + IdentityActions: identityActions, + DefaultAllow: h.defaultAllowFor(r), + }) - if !tableAllowed && !bucketAllowed { + if !allowed { h.writeError(w, http.StatusNotFound, ErrCodeNoSuchTable, fmt.Sprintf("table %s not found", tableName)) return ErrAccessDenied } @@ -562,23 +554,16 @@ func (h *S3TablesHandler) handleListTables(w http.ResponseWriter, r *http.Reques bucketARN := h.generateTableBucketARN(bucketMeta.OwnerAccountID, bucketName) identityActions := getIdentityActions(r) - nsAllowed := CheckPermissionWithContext("ListTables", accountID, nsMeta.OwnerAccountID, namespacePolicy, bucketARN, &PolicyContext{ - TrustedIssuers: h.trustedIssuers(r), - TableBucketName: bucketName, - Namespace: namespaceName, - TableBucketTags: bucketTags, - IdentityActions: identityActions, - DefaultAllow: h.defaultAllowFor(r), - }) - bucketAllowed := CheckPermissionWithContext("ListTables", accountID, bucketMeta.OwnerAccountID, bucketPolicy, bucketARN, &PolicyContext{ - TrustedIssuers: h.trustedIssuers(r), - TableBucketName: bucketName, - Namespace: namespaceName, - TableBucketTags: bucketTags, - IdentityActions: identityActions, - DefaultAllow: h.defaultAllowFor(r), - }) - if !nsAllowed && !bucketAllowed { + if !CheckTableOrBucketPermission("ListTables", accountID, + nsMeta.OwnerAccountID, namespacePolicy, bucketARN, + bucketMeta.OwnerAccountID, bucketPolicy, bucketARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), + TableBucketName: bucketName, + Namespace: namespaceName, + TableBucketTags: bucketTags, + IdentityActions: identityActions, + DefaultAllow: h.defaultAllowFor(r), + }) { return ErrAccessDenied } @@ -955,27 +940,19 @@ func (h *S3TablesHandler) handleDeleteTable(w http.ResponseWriter, r *http.Reque bucketARN := h.generateTableBucketARN(bucketMetadata.OwnerAccountID, bucketName) principal := h.getAccountID(r) identityActions := getIdentityActions(r) - tableAllowed := CheckPermissionWithContext("DeleteTable", principal, metadata.OwnerAccountID, tablePolicy, tableARN, &PolicyContext{ - TrustedIssuers: h.trustedIssuers(r), - TableBucketName: bucketName, - Namespace: namespaceName, - TableName: tableName, - TableBucketTags: bucketTags, - ResourceTags: tableTags, - IdentityActions: identityActions, - DefaultAllow: h.defaultAllowFor(r), - }) - bucketAllowed := CheckPermissionWithContext("DeleteTable", principal, bucketMetadata.OwnerAccountID, bucketPolicy, bucketARN, &PolicyContext{ - TrustedIssuers: h.trustedIssuers(r), - TableBucketName: bucketName, - Namespace: namespaceName, - TableName: tableName, - TableBucketTags: bucketTags, - ResourceTags: tableTags, - IdentityActions: identityActions, - DefaultAllow: h.defaultAllowFor(r), - }) - if !tableAllowed && !bucketAllowed { + allowed := CheckTableOrBucketPermission("DeleteTable", principal, + metadata.OwnerAccountID, tablePolicy, tableARN, + bucketMetadata.OwnerAccountID, bucketPolicy, bucketARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), + TableBucketName: bucketName, + Namespace: namespaceName, + TableName: tableName, + TableBucketTags: bucketTags, + ResourceTags: tableTags, + IdentityActions: identityActions, + DefaultAllow: h.defaultAllowFor(r), + }) + if !allowed { if h.canReadEntry("GetTable", principal, metadata.OwnerAccountID, bucketMetadata.OwnerAccountID, tablePolicy, bucketPolicy, tableARN, bucketARN, bucketName, namespaceName, tableName, bucketTags, tableTags, identityActions, r) { h.writeError(w, http.StatusForbidden, ErrCodeAccessDenied, "not authorized to delete table") } else { @@ -1147,8 +1124,9 @@ func (h *S3TablesHandler) canReadEntry(readOp, principal, tableOwner, bucketOwne IdentityActions: identityActions, DefaultAllow: h.defaultAllowFor(r), } - return CheckPermissionWithContext(readOp, principal, tableOwner, tablePolicy, tableARN, ctx) || - CheckPermissionWithContext(readOp, principal, bucketOwner, bucketPolicy, bucketARN, ctx) + return CheckTableOrBucketPermission(readOp, principal, + tableOwner, tablePolicy, tableARN, + bucketOwner, bucketPolicy, bucketARN, ctx) } // handleRenameTable moves a table's catalog entry to a new namespace/name within @@ -1319,27 +1297,19 @@ func (h *S3TablesHandler) renameCatalogEntry(w http.ResponseWriter, r *http.Requ bucketARN := h.generateTableBucketARN(bucketMetadata.OwnerAccountID, bucketName) principal := h.getAccountID(r) identityActions := getIdentityActions(r) - tableAllowed := CheckPermissionWithContext(kind.renameOp, principal, metadata.OwnerAccountID, tablePolicy, tableARN, &PolicyContext{ - TrustedIssuers: h.trustedIssuers(r), - TableBucketName: bucketName, - Namespace: srcNamespace, - TableName: srcName, - TableBucketTags: bucketTags, - ResourceTags: tableTags, - IdentityActions: identityActions, - DefaultAllow: h.defaultAllowFor(r), - }) - bucketAllowed := CheckPermissionWithContext(kind.renameOp, principal, bucketMetadata.OwnerAccountID, bucketPolicy, bucketARN, &PolicyContext{ - TrustedIssuers: h.trustedIssuers(r), - TableBucketName: bucketName, - Namespace: srcNamespace, - TableName: srcName, - TableBucketTags: bucketTags, - ResourceTags: tableTags, - IdentityActions: identityActions, - DefaultAllow: h.defaultAllowFor(r), - }) - if !tableAllowed && !bucketAllowed { + allowed := CheckTableOrBucketPermission(kind.renameOp, principal, + metadata.OwnerAccountID, tablePolicy, tableARN, + bucketMetadata.OwnerAccountID, bucketPolicy, bucketARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), + TableBucketName: bucketName, + Namespace: srcNamespace, + TableName: srcName, + TableBucketTags: bucketTags, + ResourceTags: tableTags, + IdentityActions: identityActions, + DefaultAllow: h.defaultAllowFor(r), + }) + if !allowed { readTags := tableTags if !kind.tagsForRead { readTags = nil @@ -1394,25 +1364,17 @@ func (h *S3TablesHandler) renameCatalogEntry(w http.ResponseWriter, r *http.Requ // must also be allowed to create a table there (the source check alone lets a // 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{ - TrustedIssuers: h.trustedIssuers(r), - TableBucketName: bucketName, - Namespace: destNamespace, - TableName: destName, - TableBucketTags: bucketTags, - IdentityActions: identityActions, - DefaultAllow: h.defaultAllowFor(r), - }) - destBucketAllowed := CheckPermissionWithContext(kind.createOp, principal, bucketMetadata.OwnerAccountID, bucketPolicy, bucketARN, &PolicyContext{ - TrustedIssuers: h.trustedIssuers(r), - TableBucketName: bucketName, - Namespace: destNamespace, - TableName: destName, - TableBucketTags: bucketTags, - IdentityActions: identityActions, - DefaultAllow: h.defaultAllowFor(r), - }) - if !destNamespaceAllowed && !destBucketAllowed { + if !CheckTableOrBucketPermission(kind.createOp, principal, + destNamespaceMetadata.OwnerAccountID, destNamespacePolicy, bucketARN, + bucketMetadata.OwnerAccountID, bucketPolicy, bucketARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), + TableBucketName: bucketName, + Namespace: destNamespace, + TableName: destName, + TableBucketTags: bucketTags, + IdentityActions: identityActions, + DefaultAllow: h.defaultAllowFor(r), + }) { 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") } @@ -1607,28 +1569,20 @@ func (h *S3TablesHandler) handleUpdateTable(w http.ResponseWriter, r *http.Reque principal := h.getAccountID(r) identityActions := getIdentityActions(r) - tableAllowed := CheckPermissionWithContext("UpdateTable", principal, metadata.OwnerAccountID, tablePolicy, tableARN, &PolicyContext{ - TrustedIssuers: h.trustedIssuers(r), - TableBucketName: bucketName, - Namespace: namespaceName, - TableName: tableName, - TableBucketTags: bucketTags, - ResourceTags: tableTags, - IdentityActions: identityActions, - DefaultAllow: h.defaultAllowFor(r), - }) - bucketAllowed := CheckPermissionWithContext("UpdateTable", principal, bucketMetadata.OwnerAccountID, bucketPolicy, bucketARN, &PolicyContext{ - TrustedIssuers: h.trustedIssuers(r), - TableBucketName: bucketName, - Namespace: namespaceName, - TableName: tableName, - TableBucketTags: bucketTags, - ResourceTags: tableTags, - IdentityActions: identityActions, - DefaultAllow: h.defaultAllowFor(r), - }) + allowed := CheckTableOrBucketPermission("UpdateTable", principal, + metadata.OwnerAccountID, tablePolicy, tableARN, + bucketMetadata.OwnerAccountID, bucketPolicy, bucketARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), + TableBucketName: bucketName, + Namespace: namespaceName, + TableName: tableName, + TableBucketTags: bucketTags, + ResourceTags: tableTags, + IdentityActions: identityActions, + DefaultAllow: h.defaultAllowFor(r), + }) - if !tableAllowed && !bucketAllowed { + if !allowed { if h.canReadEntry("GetTable", principal, metadata.OwnerAccountID, bucketMetadata.OwnerAccountID, tablePolicy, bucketPolicy, tableARN, bucketARN, bucketName, namespaceName, tableName, bucketTags, tableTags, identityActions, r) { h.writeError(w, http.StatusForbidden, ErrCodeAccessDenied, "not authorized to update table") } else { @@ -1833,9 +1787,9 @@ func (h *S3TablesHandler) authorizeCreateTable(w http.ResponseWriter, r *http.Re IdentityActions: identityActions, DefaultAllow: h.defaultAllowFor(r), } - nsAllowed := CheckPermissionWithContext("CreateTable", accountID, namespaceMetadata.OwnerAccountID, namespacePolicy, bucketARN, policyContext) - bucketAllowed := CheckPermissionWithContext("CreateTable", accountID, bucketMetadata.OwnerAccountID, bucketPolicy, bucketARN, policyContext) - if !nsAllowed && !bucketAllowed { + if !CheckTableOrBucketPermission("CreateTable", accountID, + namespaceMetadata.OwnerAccountID, namespacePolicy, bucketARN, + bucketMetadata.OwnerAccountID, bucketPolicy, bucketARN, policyContext) { h.writeError(w, http.StatusForbidden, ErrCodeAccessDenied, "not authorized to create table in this namespace") return nil, ErrAccessDenied } diff --git a/weed/s3api/s3tables/handler_view.go b/weed/s3api/s3tables/handler_view.go index 70cb37d37..7cea342da 100644 --- a/weed/s3api/s3tables/handler_view.go +++ b/weed/s3api/s3tables/handler_view.go @@ -643,10 +643,10 @@ func (h *S3TablesHandler) authorizeViewOp(r *http.Request, operation, principal, IdentityActions: identityActions, DefaultAllow: h.defaultAllowFor(r), } - resourceAllowed := CheckPermissionWithContext(operation, principal, resourceOwner, resourcePolicy, resourceARN, ctx) bucketARN := h.generateTableBucketARN(bucketOwner, bucketName) - bucketAllowed := CheckPermissionWithContext(operation, principal, bucketOwner, bucketPolicy, bucketARN, ctx) - return resourceAllowed || bucketAllowed + return CheckTableOrBucketPermission(operation, principal, + resourceOwner, resourcePolicy, resourceARN, + bucketOwner, bucketPolicy, bucketARN, ctx) } // singleClient adapts a bound filer client to the FilerClient interface so the diff --git a/weed/s3api/s3tables/permissions.go b/weed/s3api/s3tables/permissions.go index f00e5ce2c..ee81400db 100644 --- a/weed/s3api/s3tables/permissions.go +++ b/weed/s3api/s3tables/permissions.go @@ -113,8 +113,65 @@ func CheckPermissionWithContext(operation, principal, owner, resourcePolicy, res return checkPermission(operation, principal, owner, resourcePolicy, resourceARN, ctx) } +// CheckTableOrBucketPermission combines a table or namespace policy with its +// table bucket's policy into one decision: a matching Deny on either policy +// vetoes the request even when the other policy would allow it. +func CheckTableOrBucketPermission(operation, principal, resourceOwner, resourcePolicy, resourceARN, bucketOwner, bucketPolicy, bucketARN string, ctx *PolicyContext) bool { + if principal == "" { + return false + } + if principal == s3_constants.AccountAdminId { + return true + } + if matchesDenyStatement(operation, principal, resourcePolicy, resourceARN, ctx) || + matchesDenyStatement(operation, principal, bucketPolicy, bucketARN, ctx) { + return false + } + // Each side fails closed when its owner is unknown, matching + // CheckPermissionWithContext; an ownerless side contributes nothing. + resourceOK := resourceOwner != "" && + checkPermission(operation, principal, resourceOwner, resourcePolicy, resourceARN, ctx) + bucketOK := bucketOwner != "" && + checkPermission(operation, principal, bucketOwner, bucketPolicy, bucketARN, ctx) + return resourceOK || bucketOK +} + +// matchesDenyStatement reports whether the JSON policy carries a statement +// matching this request with Effect Deny. Malformed policies return false; +// the caller's normal evaluation still denies them. +func matchesDenyStatement(operation, principal, resourcePolicy, resourceARN string, ctx *PolicyContext) bool { + if resourcePolicy == "" { + return false + } + fullAction := operation + if !strings.Contains(operation, ":") { + fullAction = "s3tables:" + operation + } + var policy PolicyDocument + if err := json.Unmarshal([]byte(resourcePolicy), &policy); err != nil { + return false + } + for _, stmt := range policy.Statement { + if stmt.Effect != "Deny" || + !matchesCaller(stmt.Principal, principal, ctx) || + !matchesAction(stmt.Action, fullAction) || + (resourceARN != "" && !matchesResource(stmt.Resource, resourceARN)) || + !matchesConditions(stmt.Condition, ctx) { + continue + } + return true + } + return false +} + func checkPermission(operation, principal, owner, resourcePolicy, resourceARN string, ctx *PolicyContext) bool { + // An explicit Deny in the resource policy vetoes owner and identity + // grants as well as allows within the same document. + if matchesDenyStatement(operation, principal, resourcePolicy, resourceARN, ctx) { + return false + } + // Owner always has permission. if principal == owner || matchesLegacySubject(owner, principal, ctx) { return true diff --git a/weed/s3api/s3tables/permissions_test.go b/weed/s3api/s3tables/permissions_test.go index d19712164..25dfaf765 100644 --- a/weed/s3api/s3tables/permissions_test.go +++ b/weed/s3api/s3tables/permissions_test.go @@ -254,6 +254,79 @@ func TestCheckPermissionWithDefaultAllow(t *testing.T) { } } +func TestCheckTableOrBucketPermissionDenyVeto(t *testing.T) { + tablePolicy := `{ + "Version": "2012-10-17", + "Statement": [{ + "Effect": "Deny", + "Principal": "*", + "Action": "s3tables:DeleteTable", + "Resource": "*" + }] + }` + bucketAllow := `{ + "Version": "2012-10-17", + "Statement": [{ + "Effect": "Allow", + "Principal": "*", + "Action": "s3tables:*", + "Resource": "*" + }] + }` + tableARN := "arn:aws:s3tables:us-east-1:acct:bucket/b/table/ns/t" + bucketARN := "arn:aws:s3tables:us-east-1:acct:bucket/b" + ctx := &PolicyContext{TableBucketName: "b", Namespace: "ns", TableName: "t"} + + // A Deny on the table policy vetoes the bucket Allow. + if CheckTableOrBucketPermission("DeleteTable", "user123", "owner", tablePolicy, tableARN, "owner", bucketAllow, bucketARN, ctx) { + t.Error("table Deny should veto bucket Allow") + } + + // The same Deny in the bucket policy vetoes a table Allow. + if CheckTableOrBucketPermission("DeleteTable", "user123", "owner", bucketAllow, tableARN, "owner", tablePolicy, bucketARN, ctx) { + t.Error("bucket Deny should veto table Allow") + } + + // A table Deny also vetoes ownership of the table. + if CheckTableOrBucketPermission("DeleteTable", "owner", "owner", tablePolicy, tableARN, "owner", "", bucketARN, ctx) { + t.Error("table Deny should veto the owner grant") + } + + // No Deny anywhere: either policy may allow. + if !CheckTableOrBucketPermission("DeleteTable", "user123", "other-owner", "", tableARN, "owner", bucketAllow, bucketARN, ctx) { + t.Error("bucket Allow should grant when no policy denies") + } +} + +func TestCheckTableOrBucketPermissionOwnerlessFailsClosed(t *testing.T) { + ctx := &PolicyContext{ + IdentityActions: []string{"s3tables:*"}, + DefaultAllow: true, + } + allowPolicy := `{"Statement":[{"Effect":"Allow","Principal":"*","Action":"s3tables:*","Resource":"*"}]}` + + if CheckTableOrBucketPermission("GetTable", "user1", + "", "", "arn:aws:s3tables:t", + "", "", "arn:aws:s3tables:b", ctx) { + t.Error("identity actions and default allow must not reach an ownerless resource") + } + + // With no identity or default grants in play, an ownerless resource + // side contributes nothing even when its own policy allows. + plainCtx := &PolicyContext{} + if CheckTableOrBucketPermission("GetTable", "user1", + "", allowPolicy, "arn:aws:s3tables:t", + "owner", "", "arn:aws:s3tables:b", plainCtx) { + t.Error("an ownerless resource side must not grant even via an explicit allow") + } + + if !CheckTableOrBucketPermission("GetTable", "user1", + "", "", "arn:aws:s3tables:t", + "owner", allowPolicy, "arn:aws:s3tables:b", plainCtx) { + t.Error("the bucket side can still grant when its owner is set") + } +} + // Owners written before issuer scoping hold a bare subject. Re-anchoring it // is only unambiguous when exactly one OIDC issuer is configured; under two // or more issuers a bare subject could belong to either, so it must not