s3tables: let an explicit policy Deny veto fallback and owner allows (#11772)

* s3tables: let an explicit policy Deny veto fallback and owner allows

Table operations evaluated the table or namespace policy and the bucket
policy as an OR, so an explicit Deny in one policy was overridden by an
Allow in the other. Owner and identity grants also short-circuited
before the resource policy was read. Deny statements are now evaluated
first at both levels, matching AWS explicit-deny semantics.

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>

* s3tables: fail closed when a resource side has no owner

---------

Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>

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
 	}

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

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

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