s3: require a bucket-policy action to write a bucket policy (#10444)

* s3: require a bucket-policy action to write a bucket policy

PutBucketPolicy and DeleteBucketPolicy were gated on ACTION_WRITE, the same
action that grants object writes. An explicit Allow in a bucket policy
short-circuits IAM entirely -- authRequestWithAuthType sets policyAllows and
skips VerifyActionPermission -- so anyone who could write an object could
author a policy granting itself, or anonymous, anything on the bucket.

That is what separates a bucket policy from the sibling bucket controls also
gated on ACTION_WRITE: rewriting cors or lifecycle can destroy data, but only
a policy hands out access.

Give the two verbs their own actions, mapped to the AWS names that were
already defined but unrouted. ACTION_ADMIN would also have closed it, but it
resolves to s3:* for IAM identities, forcing a blanket grant on a user holding
a precise s3:PutBucketPolicy. Admins are unaffected, since isAdmin
short-circuits CanDo, and an operator can delegate with PutBucketPolicy:bucket.

The route binding is asserted from the router source: checking the action
constants alone still passes when the route says ACTION_WRITE.

* s3: also read the action from a direct iam.Auth call in the route test

Routes read iam.Auth(cb.Limit(handler, ACTION)), a multi-value pass-through:
Limit returns (http.HandlerFunc, Action) and those become Auth's parameters, so
the action Auth authorizes on is Limit's second argument and the two cannot
disagree -- Auth(Limit(h, X), Y) does not compile.

A route that skipped Limit and called Auth with its own action would compile,
though, and the test reported that as a missing route rather than as the wrong
action. Recognise the two-argument Auth form so it names the action instead.

* s3: make the bucket-policy actions grantable through an IAM policy

The new actions close the escalation only if an operator can grant them, and
they were not reachable: MapToStatementAction had no entry for PutBucketPolicy,
so an IAM policy naming s3:PutBucketPolicy was rejected outright with "not a
valid action". GetBucketPolicy was unmapped the same way.

DeleteBucketPolicy was mapped, but to ACTION_ADMIN -- granting an identity
permission to delete a bucket policy handed it full administrative access.

Map all three to the actions the router now uses, and add the reverse
direction so an identity holding them renders back as a policy statement
instead of a bare "s3:".

* admin: offer the bucket-policy permissions in the user editor

The two new actions are otherwise only grantable by hand-editing identity JSON
or by calling the IAM API, so an operator using the UI cannot delegate bucket
policy management without granting Admin.

Regenerating this file also picks up codegen the repo has not taken yet: the
checked-in _templ.go files were produced by templ v0.3.1001 while go.mod pins
v0.3.1020, so the generator rewrites the attribute-value calls. That churn is
confined to this one file; running `make generate` in weed/admin reproduces it
across all 36.

weed/admin/view/app/object_store_users.templ

@@ -239,6 +239,10 @@ templ ObjectStoreUsers(data dash.ObjectStoreUsersData) {
                                     <option value="GetBucketObjectLockConfiguration">Get Bucket Object Lock Configuration</option>
                                     <option value="PutBucketObjectLockConfiguration">Put Bucket Object Lock Configuration</option>
                                 </optgroup>
+                                <optgroup label="Bucket Policy Permissions">
+                                    <option value="PutBucketPolicy">Put Bucket Policy</option>
+                                    <option value="DeleteBucketPolicy">Delete Bucket Policy</option>
+                                </optgroup>
                                 <optgroup label="S3 Tables Permissions">
                                     <option value="S3TablesAdmin">S3 Tables Admin (Full Access)</option>
                                     <option value="CreateTableBucket">Create Table Bucket</option>
@@ -347,6 +351,10 @@ templ ObjectStoreUsers(data dash.ObjectStoreUsersData) {
                                     <option value="GetBucketObjectLockConfiguration">Get Bucket Object Lock Configuration</option>
                                     <option value="PutBucketObjectLockConfiguration">Put Bucket Object Lock Configuration</option>
                                 </optgroup>
+                                <optgroup label="Bucket Policy Permissions">
+                                    <option value="PutBucketPolicy">Put Bucket Policy</option>
+                                    <option value="DeleteBucketPolicy">Delete Bucket Policy</option>
+                                </optgroup>
                                 <optgroup label="S3 Tables Permissions">
                                     <option value="S3TablesAdmin">S3 Tables Admin (Full Access)</option>
                                     <option value="CreateTableBucket">Create Table Bucket</option>

weed/iam/helpers.go

@@ -118,8 +118,13 @@ var baseS3ActionMap = map[string]string{
 	"PutBucketAcl":        s3_constants.ACTION_WRITE_ACP,
 	"GetBucketAcl":        s3_constants.ACTION_READ_ACP,
 	// Bucket operations
-	"DeleteBucket":                     s3_constants.ACTION_DELETE_BUCKET,
-	"DeleteBucketPolicy":               s3_constants.ACTION_ADMIN,
+	"DeleteBucket": s3_constants.ACTION_DELETE_BUCKET,
+	// Bucket policy is permissions management: an explicit Allow in one skips
+	// the IAM check, so it gets its own actions rather than folding into Write
+	// (which any object writer holds) or Admin (which grants everything).
+	"GetBucketPolicy":                  s3_constants.ACTION_READ,
+	"PutBucketPolicy":                  s3_constants.ACTION_PUT_BUCKET_POLICY,
+	"DeleteBucketPolicy":               s3_constants.ACTION_DELETE_BUCKET_POLICY,
 	"ListBucket":                       s3_constants.ACTION_LIST,
 	"ListBucketVersions":               s3_constants.ACTION_LIST,
 	"ListAllMyBuckets":                 s3_constants.ACTION_LIST,
@@ -186,6 +191,10 @@ func MapToIdentitiesAction(action string) string {
 		return StatementActionTagging
 	case s3_constants.ACTION_DELETE_BUCKET:
 		return StatementActionDelete
+	case s3_constants.ACTION_PUT_BUCKET_POLICY:
+		return "PutBucketPolicy"
+	case s3_constants.ACTION_DELETE_BUCKET_POLICY:
+		return "DeleteBucketPolicy"
 	default:
 		return ""
 	}

weed/s3api/s3_action_resolver.go

@@ -334,6 +334,10 @@ func mapBaseActionToS3Format(baseAction string) string {
 		return s3_constants.S3_ACTION_GET_BUCKET_OBJECT_LOCK
 	case s3_constants.ACTION_PUT_BUCKET_OBJECT_LOCK_CONFIG:
 		return s3_constants.S3_ACTION_PUT_BUCKET_OBJECT_LOCK
+	case s3_constants.ACTION_PUT_BUCKET_POLICY:
+		return s3_constants.S3_ACTION_PUT_BUCKET_POLICY
+	case s3_constants.ACTION_DELETE_BUCKET_POLICY:
+		return s3_constants.S3_ACTION_DELETE_BUCKET_POLICY
 	default:
 		// For unknown actions, prefix with s3: to maintain format consistency
 		return "s3:" + baseAction

weed/s3api/s3_constants/s3_actions.go

@@ -16,6 +16,8 @@ const (
 	ACTION_PUT_OBJECT_LEGAL_HOLD         = "PutObjectLegalHold"
 	ACTION_GET_BUCKET_OBJECT_LOCK_CONFIG = "GetBucketObjectLockConfiguration"
 	ACTION_PUT_BUCKET_OBJECT_LOCK_CONFIG = "PutBucketObjectLockConfiguration"
+	ACTION_PUT_BUCKET_POLICY             = "PutBucketPolicy"
+	ACTION_DELETE_BUCKET_POLICY          = "DeleteBucketPolicy"
 
 	SeaweedStorageDestinationHeader = "x-seaweedfs-destination"
 	MultipartUploadsFolder          = ".uploads"

weed/s3api/s3api_bucket_policy_authz_test.go

@@ -0,0 +1,188 @@
+package s3api
+
+import (
+	"go/ast"
+	"go/parser"
+	"go/token"
+	"strings"
+	"testing"
+
+	iamlib "github.com/seaweedfs/seaweedfs/weed/iam"
+	"github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants"
+)
+
+// The constants above only close the escalation if the routes are actually
+// bound to them, so pin the binding itself. Checking the action in isolation
+// passes just as happily when the route still says ACTION_WRITE.
+func TestBucketPolicyRoutesUseTheirOwnActions(t *testing.T) {
+	bindings := handlerActionBindings(t)
+
+	for _, tc := range []struct{ handler, want string }{
+		{"PutBucketPolicyHandler", "ACTION_PUT_BUCKET_POLICY"},
+		{"DeleteBucketPolicyHandler", "ACTION_DELETE_BUCKET_POLICY"},
+	} {
+		got, ok := bindings[tc.handler]
+		if !ok {
+			t.Errorf("no route found for %s", tc.handler)
+			continue
+		}
+		if got != tc.want {
+			t.Errorf("%s is gated on %s, want %s -- %s is implied by object write and lets a writer rewrite the bucket's authorization",
+				tc.handler, got, tc.want, got)
+		}
+	}
+}
+
+// handlerActionBindings maps each handler name to the action constant its route
+// authorizes on, read out of the router source.
+//
+// Routes read `iam.Auth(cb.Limit(handler, ACTION))`, which is a multi-value
+// pass-through: Limit returns (http.HandlerFunc, Action) and those become Auth's
+// two parameters, so the action Auth authorizes on is Limit's second argument
+// and the two cannot disagree -- `Auth(Limit(h, X), Y)` does not compile. Auth
+// called directly with its own action is still recognised, so a route that
+// bypasses Limit is reported against the action it really uses rather than as a
+// missing route.
+func handlerActionBindings(t *testing.T) map[string]string {
+	t.Helper()
+	fset := token.NewFileSet()
+	file, err := parser.ParseFile(fset, "s3api_server.go", nil, 0)
+	if err != nil {
+		t.Fatalf("parse s3api_server.go: %v", err)
+	}
+
+	bindings := make(map[string]string)
+	ast.Inspect(file, func(n ast.Node) bool {
+		call, ok := n.(*ast.CallExpr)
+		if !ok || len(call.Args) != 2 {
+			return true
+		}
+		sel, ok := call.Fun.(*ast.SelectorExpr)
+		if !ok || (sel.Sel.Name != "Limit" && sel.Sel.Name != "Auth") {
+			return true
+		}
+		handler, ok := call.Args[0].(*ast.SelectorExpr)
+		if !ok || !strings.HasSuffix(handler.Sel.Name, "Handler") {
+			return true
+		}
+		if action, ok := call.Args[1].(*ast.Ident); ok {
+			bindings[handler.Sel.Name] = action.Name
+		}
+		return true
+	})
+
+	if len(bindings) < 20 {
+		t.Fatalf("parsed only %d handler bindings, the router shape must have changed", len(bindings))
+	}
+	return bindings
+}
+
+// Writing a bucket policy is permissions management, not object writing: an
+// explicit Allow in a bucket policy short-circuits the IAM check entirely
+// (authRequestWithAuthType sets policyAllows and skips VerifyActionPermission).
+// So an identity that can only write objects must not be able to author one --
+// otherwise object-write escalates to arbitrary authorization on the bucket,
+// including granting anonymous access.
+func TestBucketPolicyWriteIsNotImpliedByObjectWrite(t *testing.T) {
+	objectWriter := &Identity{
+		Name:    "object-writer",
+		Actions: []Action{Action(s3_constants.ACTION_WRITE + ":test-bucket")},
+	}
+
+	if !objectWriter.CanDo(s3_constants.ACTION_WRITE, "test-bucket", "some/key") {
+		t.Fatal("precondition failed: the identity should be able to write objects")
+	}
+
+	for _, action := range []string{
+		s3_constants.ACTION_PUT_BUCKET_POLICY,
+		s3_constants.ACTION_DELETE_BUCKET_POLICY,
+	} {
+		if objectWriter.CanDo(Action(action), "test-bucket", "") {
+			t.Errorf("an object writer was allowed to %s, which escalates to arbitrary bucket authorization", action)
+		}
+	}
+}
+
+// An admin identity keeps managing bucket policies; the tightening must not
+// lock the operator out of the surface it protects.
+func TestBucketPolicyWriteAllowedForAdmin(t *testing.T) {
+	for _, tc := range []struct {
+		name     string
+		identity *Identity
+	}{
+		{"global admin", &Identity{Name: "admin", Actions: []Action{Action(s3_constants.ACTION_ADMIN)}}},
+		{"bucket admin", &Identity{Name: "bucket-admin", Actions: []Action{Action(s3_constants.ACTION_ADMIN + ":test-bucket")}}},
+		{"explicit delegation", &Identity{Name: "policy-manager", Actions: []Action{
+			Action(s3_constants.ACTION_PUT_BUCKET_POLICY + ":test-bucket"),
+			Action(s3_constants.ACTION_DELETE_BUCKET_POLICY + ":test-bucket"),
+		}}},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			for _, action := range []string{
+				s3_constants.ACTION_PUT_BUCKET_POLICY,
+				s3_constants.ACTION_DELETE_BUCKET_POLICY,
+			} {
+				if !tc.identity.CanDo(Action(action), "test-bucket", "") {
+					t.Errorf("%s was denied %s", tc.name, action)
+				}
+			}
+		})
+	}
+}
+
+// The new actions are only reachable if an operator can actually grant them.
+// An IAM policy naming s3:PutBucketPolicy is rejected outright when the action
+// has no mapping ("not a valid action"), so the grant this change requires has
+// to round-trip through the IAM helpers.
+func TestBucketPolicyActionsAreGrantableViaIamPolicy(t *testing.T) {
+	for _, tc := range []struct{ policyAction, wantIdentityAction string }{
+		{"s3:PutBucketPolicy", s3_constants.ACTION_PUT_BUCKET_POLICY},
+		{"PutBucketPolicy", s3_constants.ACTION_PUT_BUCKET_POLICY},
+		{"s3:DeleteBucketPolicy", s3_constants.ACTION_DELETE_BUCKET_POLICY},
+		{"DeleteBucketPolicy", s3_constants.ACTION_DELETE_BUCKET_POLICY},
+		{"s3:GetBucketPolicy", s3_constants.ACTION_READ},
+	} {
+		if got := iamlib.MapToStatementAction(tc.policyAction); got != tc.wantIdentityAction {
+			t.Errorf("MapToStatementAction(%q) = %q, want %q -- an unmapped action is rejected as invalid, so the permission cannot be granted",
+				tc.policyAction, got, tc.wantIdentityAction)
+		}
+	}
+
+	// Granting delete-policy must not hand back blanket admin, which is what
+	// the previous mapping did.
+	if got := iamlib.MapToStatementAction("s3:DeleteBucketPolicy"); got == s3_constants.ACTION_ADMIN {
+		t.Error("s3:DeleteBucketPolicy still maps to Admin, so granting it grants everything")
+	}
+
+	// And the reverse direction, used to render an identity's actions back as
+	// policy statements.
+	for _, tc := range []struct{ identityAction, wantPolicyAction string }{
+		{s3_constants.ACTION_PUT_BUCKET_POLICY, "PutBucketPolicy"},
+		{s3_constants.ACTION_DELETE_BUCKET_POLICY, "DeleteBucketPolicy"},
+	} {
+		if got := iamlib.MapToIdentitiesAction(tc.identityAction); got != tc.wantPolicyAction {
+			t.Errorf("MapToIdentitiesAction(%q) = %q, want %q -- an empty result renders as a bare \"s3:\"",
+				tc.identityAction, got, tc.wantPolicyAction)
+		}
+	}
+}
+
+// For IAM-policy identities the router action becomes the S3 action name that
+// gets matched, so it has to be the AWS one. Mapping to s3:* (what ACTION_ADMIN
+// resolves to) would force a blanket grant instead of s3:PutBucketPolicy.
+func TestBucketPolicyActionsMapToAwsActionNames(t *testing.T) {
+	for _, tc := range []struct{ action, want string }{
+		{s3_constants.ACTION_PUT_BUCKET_POLICY, s3_constants.S3_ACTION_PUT_BUCKET_POLICY},
+		{s3_constants.ACTION_DELETE_BUCKET_POLICY, s3_constants.S3_ACTION_DELETE_BUCKET_POLICY},
+	} {
+		if got := mapBaseActionToS3Format(tc.action); got != tc.want {
+			t.Errorf("mapBaseActionToS3Format(%q) = %q, want %q", tc.action, got, tc.want)
+		}
+	}
+
+	// The escalation in AWS terms: s3:PutObject must not resolve to the same
+	// action name the policy write is checked against.
+	if mapBaseActionToS3Format(s3_constants.ACTION_WRITE) == s3_constants.S3_ACTION_PUT_BUCKET_POLICY {
+		t.Fatal("object write and bucket-policy write resolve to the same S3 action")
+	}
+}

weed/s3api/s3api_server.go

@@ -870,9 +870,9 @@ func (s3a *S3ApiServer) registerRouter(router *mux.Router) {
 		// GetBucketPolicy
 		bucket.Methods(http.MethodGet).HandlerFunc(track(s3a.iam.Auth(s3a.cb.Limit(s3a.GetBucketPolicyHandler, ACTION_READ)), "GET")).Queries("policy", "")
 		// PutBucketPolicy
-		bucket.Methods(http.MethodPut).HandlerFunc(track(s3a.iam.Auth(s3a.cb.Limit(s3a.PutBucketPolicyHandler, ACTION_WRITE)), "PUT")).Queries("policy", "")
+		bucket.Methods(http.MethodPut).HandlerFunc(track(s3a.iam.Auth(s3a.cb.Limit(s3a.PutBucketPolicyHandler, ACTION_PUT_BUCKET_POLICY)), "PUT")).Queries("policy", "")
 		// DeleteBucketPolicy
-		bucket.Methods(http.MethodDelete).HandlerFunc(track(s3a.iam.Auth(s3a.cb.Limit(s3a.DeleteBucketPolicyHandler, ACTION_WRITE)), "DELETE")).Queries("policy", "")
+		bucket.Methods(http.MethodDelete).HandlerFunc(track(s3a.iam.Auth(s3a.cb.Limit(s3a.DeleteBucketPolicyHandler, ACTION_DELETE_BUCKET_POLICY)), "DELETE")).Queries("policy", "")
 
 		// GetBucketCors
 		bucket.Methods(http.MethodGet).HandlerFunc(track(s3a.iam.Auth(s3a.cb.Limit(s3a.GetBucketCorsHandler, ACTION_READ)), "GET")).Queries("cors", "")