From 5fdb061c993920d860f3e67f7997cd7d2e4075ba Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Sat, 26 Sep 2026 05:00:23 -0700 Subject: [PATCH] s3: pin object-lock actions so request shape cannot re-derive them ResolveS3Action derives the authorized action from the request's method+query, overriding the caller's base action. A governance-bypass check on DELETE ?versionId=... therefore evaluated s3:DeleteObjectVersion instead of s3:BypassGovernanceRetention, letting an identity that holds version-delete but not bypass delete a GOVERNANCE-locked version (GHSA-j4hf-8r83-7fjj). The same substitution let s3:GetObjectVersion satisfy GetObjectRetention/GetObjectLegalHold on ?versionId requests, and silently dropped bucket-policy grants of the bypass action on versioned deletes. Object Lock actions name a concrete permission checked on top of another operation's shape; resolve them to themselves, like iam:/sts: actions. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- weed/s3api/s3_action_resolver.go | 14 +++++ weed/s3api/s3_action_resolver_test.go | 61 +++++++++++++++++++ .../s3api_object_lock_actions_authz_test.go | 33 ++++++++++ 3 files changed, 108 insertions(+) diff --git a/weed/s3api/s3_action_resolver.go b/weed/s3api/s3_action_resolver.go index 3a1e7db4cd..8b85e2e9b1 100644 --- a/weed/s3api/s3_action_resolver.go +++ b/weed/s3api/s3_action_resolver.go @@ -33,6 +33,20 @@ func ResolveS3Action(r *http.Request, baseAction string, bucket string, object s return baseAction } + // Object Lock actions are checked on top of another operation's request + // shape (e.g. BypassGovernanceRetention on a versioned DELETE), so the + // shape must not re-derive them. + switch baseAction { + case s3_constants.ACTION_BYPASS_GOVERNANCE_RETENTION, + s3_constants.ACTION_GET_OBJECT_RETENTION, + s3_constants.ACTION_PUT_OBJECT_RETENTION, + s3_constants.ACTION_GET_OBJECT_LEGAL_HOLD, + s3_constants.ACTION_PUT_OBJECT_LEGAL_HOLD, + s3_constants.ACTION_GET_BUCKET_OBJECT_LOCK_CONFIG, + s3_constants.ACTION_PUT_BUCKET_OBJECT_LOCK_CONFIG: + return mapBaseActionToS3Format(baseAction) + } + if r == nil || r.URL == nil { // No HTTP context available: fall back to coarse-grained mapping // This ensures consistent behavior and avoids returning empty strings diff --git a/weed/s3api/s3_action_resolver_test.go b/weed/s3api/s3_action_resolver_test.go index 077b23dbd8..ddbd03e3fb 100644 --- a/weed/s3api/s3_action_resolver_test.go +++ b/weed/s3api/s3_action_resolver_test.go @@ -114,6 +114,67 @@ func TestResolveS3Action_AttributesBeforeVersionId(t *testing.T) { } } +func TestResolveS3Action_ObjectLockActionsIgnoreRequestShape(t *testing.T) { + tests := []struct { + name string + query string + method string + baseAction string + object string + want string + }{ + { + name: "bypass on versioned delete", + query: "versionId=abc123", + method: http.MethodDelete, + baseAction: s3_constants.ACTION_BYPASS_GOVERNANCE_RETENTION, + object: "key", + want: s3_constants.S3_ACTION_BYPASS_GOVERNANCE, + }, + { + name: "bypass on batch delete", + query: "delete", + method: http.MethodPost, + baseAction: s3_constants.ACTION_BYPASS_GOVERNANCE_RETENTION, + object: "key", + want: s3_constants.S3_ACTION_BYPASS_GOVERNANCE, + }, + { + name: "get retention with versionId", + query: "retention&versionId=abc123", + method: http.MethodGet, + baseAction: s3_constants.ACTION_GET_OBJECT_RETENTION, + object: "key", + want: s3_constants.S3_ACTION_GET_OBJECT_RETENTION, + }, + { + name: "get legal hold with versionId", + query: "legal-hold&versionId=abc123", + method: http.MethodGet, + baseAction: s3_constants.ACTION_GET_OBJECT_LEGAL_HOLD, + object: "key", + want: s3_constants.S3_ACTION_GET_OBJECT_LEGAL_HOLD, + }, + { + name: "put retention with versionId", + query: "retention&versionId=abc123", + method: http.MethodPut, + baseAction: s3_constants.ACTION_PUT_OBJECT_RETENTION, + object: "key", + want: s3_constants.S3_ACTION_PUT_OBJECT_RETENTION, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r, _ := http.NewRequest(tt.method, "http://localhost/bucket/"+tt.object+"?"+tt.query, nil) + if got := ResolveS3Action(r, tt.baseAction, "bucket", tt.object); got != tt.want { + t.Errorf("ResolveS3Action() = %q, want %q", got, tt.want) + } + }) + } +} + // Bucket subresources registered with ACTION_ADMIN must resolve to their own // S3 actions so a policy granting one of them does not need s3:*, and so no // broader grant sweeps them in. diff --git a/weed/s3api/s3api_object_lock_actions_authz_test.go b/weed/s3api/s3api_object_lock_actions_authz_test.go index a7ac525e4d..7f5c98861a 100644 --- a/weed/s3api/s3api_object_lock_actions_authz_test.go +++ b/weed/s3api/s3api_object_lock_actions_authz_test.go @@ -2,6 +2,7 @@ package s3api import ( "context" + "fmt" "net/http" "net/http/httptest" "testing" @@ -55,6 +56,38 @@ func TestGovernanceBypassUsesDedicatedAuthorization(t *testing.T) { } } +func TestGovernanceBypassChecksBypassActionOnVersionedDelete(t *testing.T) { + arn := "arn:aws:s3:::test-bucket/test-object" + for _, tc := range []struct { + name string + policyAction string + allowed bool + }{ + {"bypass permission", "s3:BypassGovernanceRetention", true}, + {"delete version only", "s3:DeleteObjectVersion", false}, + } { + t.Run(tc.name, func(t *testing.T) { + iam := newTestIAM() + iam.identities[0].Actions = nil + iam.identities[0].Account = &Account{Id: "test-account"} + iam.policyEngine = NewBucketPolicyEngine() + policy := fmt.Sprintf(`{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Principal":"*","Action":"%s","Resource":"%s"}]}`, + tc.policyAction, arn) + require.NoError(t, iam.policyEngine.engine.SetBucketPolicy("test-bucket", policy)) + s3a := &S3ApiServer{iam: iam} + + req := httptest.NewRequest(http.MethodDelete, "http://localhost:8333/test-bucket/test-object?versionId=v1", nil) + req = mux.SetURLVars(req, map[string]string{"bucket": "test-bucket", "object": "test-object"}) + require.NoError(t, signRawHTTPRequest(context.Background(), req, + "AKIAIOSFODNN7EXAMPLE", "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY", "us-east-1")) + + if got := s3a.checkGovernanceBypassPermission(req, "test-bucket", "/test-object"); got != tc.allowed { + t.Errorf("checkGovernanceBypassPermission() = %v, want %v", got, tc.allowed) + } + }) + } +} + func TestGovernanceBypassUsesBodyObjectKey(t *testing.T) { iam := newTestIAM() iam.identities[0].Actions = []Action{