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{