diff --git a/weed/s3api/s3_action_resolver.go b/weed/s3api/s3_action_resolver.go index 3a1e7db4c..8f86eb8f1 100644 --- a/weed/s3api/s3_action_resolver.go +++ b/weed/s3api/s3_action_resolver.go @@ -33,6 +33,22 @@ func ResolveS3Action(r *http.Request, baseAction string, bucket string, object s return baseAction } + // Dedicated object-lock actions already name the operation — a query + // parameter must not re-map them. In particular the governance-bypass + // check authorizes against a synthetic DELETE ?versionId request, whose + // shape would otherwise resolve to s3:DeleteObjectVersion and satisfy + // the bypass check with the delete-version permission alone. + 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 077b23dbd..f11f37216 100644 --- a/weed/s3api/s3_action_resolver_test.go +++ b/weed/s3api/s3_action_resolver_test.go @@ -212,6 +212,47 @@ func TestResolveS3Action_Quota(t *testing.T) { } } +// Dedicated object-lock actions already name the operation being authorized; +// competing query parameters must not re-map them. Notably the synthetic +// DELETE ?versionId request behind the governance-bypass check must stay +// s3:BypassGovernanceRetention rather than resolving to s3:DeleteObjectVersion. +func TestResolveS3ActionDedicatedObjectLockActions(t *testing.T) { + tests := []struct { + name string + method string + object string + query string + baseAction string + want string + }{ + {"bypass on versioned delete shape", http.MethodDelete, "key", "versionId=abc123", + s3_constants.ACTION_BYPASS_GOVERNANCE_RETENTION, s3_constants.S3_ACTION_BYPASS_GOVERNANCE}, + {"bypass on batch delete shape", http.MethodPost, "", "delete", + s3_constants.ACTION_BYPASS_GOVERNANCE_RETENTION, s3_constants.S3_ACTION_BYPASS_GOVERNANCE}, + {"get retention with versionId", http.MethodGet, "key", "retention&versionId=abc123", + s3_constants.ACTION_GET_OBJECT_RETENTION, s3_constants.S3_ACTION_GET_OBJECT_RETENTION}, + {"put retention with versionId", http.MethodPut, "key", "retention&versionId=abc123", + s3_constants.ACTION_PUT_OBJECT_RETENTION, s3_constants.S3_ACTION_PUT_OBJECT_RETENTION}, + {"get legal hold with versionId", http.MethodGet, "key", "legal-hold&versionId=abc123", + s3_constants.ACTION_GET_OBJECT_LEGAL_HOLD, s3_constants.S3_ACTION_GET_OBJECT_LEGAL_HOLD}, + {"put legal hold with versionId", http.MethodPut, "key", "legal-hold&versionId=abc123", + s3_constants.ACTION_PUT_OBJECT_LEGAL_HOLD, s3_constants.S3_ACTION_PUT_OBJECT_LEGAL_HOLD}, + {"get object-lock config", http.MethodGet, "", "object-lock&versioning", + s3_constants.ACTION_GET_BUCKET_OBJECT_LOCK_CONFIG, s3_constants.S3_ACTION_GET_BUCKET_OBJECT_LOCK}, + {"put object-lock config", http.MethodPut, "", "object-lock", + s3_constants.ACTION_PUT_BUCKET_OBJECT_LOCK_CONFIG, s3_constants.S3_ACTION_PUT_BUCKET_OBJECT_LOCK}, + } + + 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) + } + }) + } +} + // A base action naming another service carries no S3 request shape, so a query // parameter on the request must not redirect it to an S3 action. func TestResolveS3ActionKeepsNonS3Service(t *testing.T) { diff --git a/weed/s3api/s3api_object_lock_actions_authz_test.go b/weed/s3api/s3api_object_lock_actions_authz_test.go index a7ac525e4..b785e8324 100644 --- a/weed/s3api/s3api_object_lock_actions_authz_test.go +++ b/weed/s3api/s3api_object_lock_actions_authz_test.go @@ -55,6 +55,39 @@ func TestGovernanceBypassUsesDedicatedAuthorization(t *testing.T) { } } +// The bypass check authorizes against a synthetic request shaped like the +// real one; DELETE ?versionId used to re-resolve to s3:DeleteObjectVersion, +// so the delete-version grant silently satisfied the bypass check. +func TestGovernanceBypassDoesNotInheritDeleteObjectVersion(t *testing.T) { + iam := &IdentityAccessManagement{} + require.NoError(t, iam.PutPolicy("DeleteVersions", + `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["s3:DeleteObject","s3:DeleteObjectVersion"],"Resource":"arn:aws:s3:::worm/*"}]}`)) + require.NoError(t, iam.PutPolicy("BypassGovernance", + `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["s3:DeleteObject","s3:DeleteObjectVersion","s3:BypassGovernanceRetention"],"Resource":"arn:aws:s3:::worm/*"}]}`)) + s3a := &S3ApiServer{iam: iam} + + req := httptest.NewRequest(http.MethodDelete, "http://localhost:8333/worm/doc.txt?versionId=abc123", nil) + req = mux.SetURLVars(req, map[string]string{"bucket": "worm", "object": "doc.txt"}) + + req = req.WithContext(s3_constants.SetIdentityInContext(req.Context(), &Identity{ + Name: "deleter", + PolicyNames: []string{"DeleteVersions"}, + Account: &Account{Id: "test-account"}, + })) + if s3a.checkGovernanceBypassPermission(req, "worm", "/doc.txt") { + t.Fatal("s3:DeleteObjectVersion alone satisfied the governance bypass check") + } + + req = req.WithContext(s3_constants.SetIdentityInContext(req.Context(), &Identity{ + Name: "breaker", + PolicyNames: []string{"BypassGovernance"}, + Account: &Account{Id: "test-account"}, + })) + if !s3a.checkGovernanceBypassPermission(req, "worm", "/doc.txt") { + t.Fatal("s3:BypassGovernanceRetention did not satisfy the governance bypass check") + } +} + func TestGovernanceBypassUsesBodyObjectKey(t *testing.T) { iam := newTestIAM() iam.identities[0].Actions = []Action{