From a7f4b88a61cd93bb833374b8b03b8357b2db4754 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Sun, 26 Jul 2026 00:52:22 -0700 Subject: [PATCH] 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 | 8 + .../view/app/object_store_users_templ.go | 20 +- weed/iam/helpers.go | 13 +- weed/s3api/s3_action_resolver.go | 4 + weed/s3api/s3_constants/s3_actions.go | 2 + weed/s3api/s3api_bucket_policy_authz_test.go | 188 ++++++++++++++++++ weed/s3api/s3api_server.go | 4 +- 7 files changed, 225 insertions(+), 14 deletions(-) create mode 100644 weed/s3api/s3api_bucket_policy_authz_test.go diff --git a/weed/admin/view/app/object_store_users.templ b/weed/admin/view/app/object_store_users.templ index 8842cb96a..ded49d85e 100644 --- a/weed/admin/view/app/object_store_users.templ +++ b/weed/admin/view/app/object_store_users.templ @@ -239,6 +239,10 @@ templ ObjectStoreUsers(data dash.ObjectStoreUsersData) { + + + + @@ -347,6 +351,10 @@ templ ObjectStoreUsers(data dash.ObjectStoreUsersData) { + + + + diff --git a/weed/admin/view/app/object_store_users_templ.go b/weed/admin/view/app/object_store_users_templ.go index 20ba912f5..6bab48447 100644 --- a/weed/admin/view/app/object_store_users_templ.go +++ b/weed/admin/view/app/object_store_users_templ.go @@ -1,6 +1,6 @@ // Code generated by templ - DO NOT EDIT. -// templ: version: v0.3.1001 +// templ: version: v0.3.1020 package app //lint:file-ignore SA4006 This context is only used if a nested component is present. @@ -132,11 +132,11 @@ func ObjectStoreUsers(data dash.ObjectStoreUsersData) templ.Component { return templ_7745c5c3_Err } var templ_7745c5c3_Var8 string - templ_7745c5c3_Var8, templ_7745c5c3_Err = templ.JoinStringErrs(user.Username) + templ_7745c5c3_Var8, templ_7745c5c3_Err = templ.ResolveAttributeValue(user.Username) if templ_7745c5c3_Err != nil { return templ.Error{Err: templ_7745c5c3_Err, FileName: `view/app/object_store_users.templ`, Line: 140, Col: 121} } - _, templ_7745c5c3_Err = templ_7745c5c3_Buffer.WriteString(templ.EscapeString(templ_7745c5c3_Var8)) + _, templ_7745c5c3_Err = templ_7745c5c3_Buffer.WriteString(templ_7745c5c3_Var8) if templ_7745c5c3_Err != nil { return templ_7745c5c3_Err } @@ -150,11 +150,11 @@ func ObjectStoreUsers(data dash.ObjectStoreUsersData) templ.Component { return templ_7745c5c3_Err } var templ_7745c5c3_Var9 string - templ_7745c5c3_Var9, templ_7745c5c3_Err = templ.JoinStringErrs(user.Username) + templ_7745c5c3_Var9, templ_7745c5c3_Err = templ.ResolveAttributeValue(user.Username) if templ_7745c5c3_Err != nil { return templ.Error{Err: templ_7745c5c3_Err, FileName: `view/app/object_store_users.templ`, Line: 145, Col: 117} } - _, templ_7745c5c3_Err = templ_7745c5c3_Buffer.WriteString(templ.EscapeString(templ_7745c5c3_Var9)) + _, templ_7745c5c3_Err = templ_7745c5c3_Buffer.WriteString(templ_7745c5c3_Var9) if templ_7745c5c3_Err != nil { return templ_7745c5c3_Err } @@ -169,11 +169,11 @@ func ObjectStoreUsers(data dash.ObjectStoreUsersData) templ.Component { return templ_7745c5c3_Err } var templ_7745c5c3_Var10 string - templ_7745c5c3_Var10, templ_7745c5c3_Err = templ.JoinStringErrs(user.Username) + templ_7745c5c3_Var10, templ_7745c5c3_Err = templ.ResolveAttributeValue(user.Username) if templ_7745c5c3_Err != nil { return templ.Error{Err: templ_7745c5c3_Err, FileName: `view/app/object_store_users.templ`, Line: 151, Col: 126} } - _, templ_7745c5c3_Err = templ_7745c5c3_Buffer.WriteString(templ.EscapeString(templ_7745c5c3_Var10)) + _, templ_7745c5c3_Err = templ_7745c5c3_Buffer.WriteString(templ_7745c5c3_Var10) if templ_7745c5c3_Err != nil { return templ_7745c5c3_Err } @@ -188,11 +188,11 @@ func ObjectStoreUsers(data dash.ObjectStoreUsersData) templ.Component { return templ_7745c5c3_Err } var templ_7745c5c3_Var11 string - templ_7745c5c3_Var11, templ_7745c5c3_Err = templ.JoinStringErrs(user.Username) + templ_7745c5c3_Var11, templ_7745c5c3_Err = templ.ResolveAttributeValue(user.Username) if templ_7745c5c3_Err != nil { return templ.Error{Err: templ_7745c5c3_Err, FileName: `view/app/object_store_users.templ`, Line: 157, Col: 119} } - _, templ_7745c5c3_Err = templ_7745c5c3_Buffer.WriteString(templ.EscapeString(templ_7745c5c3_Var11)) + _, templ_7745c5c3_Err = templ_7745c5c3_Buffer.WriteString(templ_7745c5c3_Var11) if templ_7745c5c3_Err != nil { return templ_7745c5c3_Err } @@ -240,7 +240,7 @@ func ObjectStoreUsers(data dash.ObjectStoreUsersData) templ.Component { return templ_7745c5c3_Err } } - templ_7745c5c3_Err = templruntime.WriteString(templ_7745c5c3_Buffer, 24, "
Hold Ctrl/Cmd to select multiple permissions
Apply selected permissions to specific buckets or all buckets
Hold Ctrl/Cmd to select multiple buckets
Hold Ctrl/Cmd to select multiple policies
Edit User
Apply selected permissions to specific buckets or all buckets
Hold Ctrl/Cmd to select multiple buckets
User Details
Manage Access Keys
Access Keys for

Leave blank to auto-generate.

") + templ_7745c5c3_Err = templruntime.WriteString(templ_7745c5c3_Buffer, 24, "
Hold Ctrl/Cmd to select multiple permissions
Apply selected permissions to specific buckets or all buckets
Hold Ctrl/Cmd to select multiple buckets
Hold Ctrl/Cmd to select multiple policies
Edit User
Apply selected permissions to specific buckets or all buckets
Hold Ctrl/Cmd to select multiple buckets
User Details
Manage Access Keys
Access Keys for

Leave blank to auto-generate.

") if templ_7745c5c3_Err != nil { return templ_7745c5c3_Err } diff --git a/weed/iam/helpers.go b/weed/iam/helpers.go index ae8c8fd6a..c335f8759 100644 --- a/weed/iam/helpers.go +++ b/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 "" } diff --git a/weed/s3api/s3_action_resolver.go b/weed/s3api/s3_action_resolver.go index fa2e0a134..b67af7d25 100644 --- a/weed/s3api/s3_action_resolver.go +++ b/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 diff --git a/weed/s3api/s3_constants/s3_actions.go b/weed/s3api/s3_constants/s3_actions.go index 67d6a961f..f5122bb6d 100644 --- a/weed/s3api/s3_constants/s3_actions.go +++ b/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" diff --git a/weed/s3api/s3api_bucket_policy_authz_test.go b/weed/s3api/s3api_bucket_policy_authz_test.go new file mode 100644 index 000000000..32838b9f9 --- /dev/null +++ b/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") + } +} diff --git a/weed/s3api/s3api_server.go b/weed/s3api/s3api_server.go index 413984cd0..0645ee88c 100644 --- a/weed/s3api/s3api_server.go +++ b/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", "")