diff --git a/auth/access-control.go b/auth/access-control.go index 2cecc5d1..75b6efa3 100644 --- a/auth/access-control.go +++ b/auth/access-control.go @@ -133,6 +133,11 @@ func VerifyPublicAccess(ctx context.Context, be backend.Backend, action Action, } if err == nil { err = VerifyPublicBucketPolicy(policy, bucket, object, action) + if errors.Is(err, errExplicitDeny) { + // Explicit public-policy Deny has higher precedence than any + // public ACL grant, so do not continue to ACL fallback. + return s3err.GetAPIError(s3err.ErrAccessDenied) + } if err == nil { // if ACLs are disabled, and the bucket grants public access, // policy actions should return 'MethodNotAllowed' diff --git a/auth/access-control_test.go b/auth/access-control_test.go index 67b1837b..fd5dd961 100644 --- a/auth/access-control_test.go +++ b/auth/access-control_test.go @@ -17,6 +17,7 @@ package auth import ( "context" "encoding/json" + "errors" "testing" "github.com/aws/aws-sdk-go-v2/service/s3" @@ -41,6 +42,74 @@ func (b noBucketPolicyBackend) GetBucketAcl(_ context.Context, _ *s3.GetBucketAc return json.Marshal(b.srcAcl) } +type publicBucketPolicyBackend struct { + backend.BackendUnsupported + policy []byte + acl ACL + aclCalls int +} + +func (b *publicBucketPolicyBackend) GetBucketPolicy(_ context.Context, _ string) ([]byte, error) { + return b.policy, nil +} + +func (b *publicBucketPolicyBackend) GetBucketAcl(_ context.Context, _ *s3.GetBucketAclInput) ([]byte, error) { + b.aclCalls++ + return json.Marshal(b.acl) +} + +func publicReadACL() ACL { + return ACL{ + Owner: "owner", + Grantees: []Grantee{ + { + Permission: PermissionRead, + Access: "all-users", + Type: types.TypeGroup, + }, + }, + } +} + +func TestVerifyPublicAccess_PublicPolicyDenyStopsACLFallback(t *testing.T) { + be := &publicBucketPolicyBackend{ + policy: []byte(`{ + "Statement": [{ + "Effect": "Deny", + "Principal": "*", + "Action": "s3:GetObject", + "Resource": "arn:aws:s3:::bucket/private/*" + }] + }`), + acl: publicReadACL(), + } + + err := VerifyPublicAccess(context.Background(), be, GetObjectAction, PermissionRead, "bucket", "private/secret.txt") + + assert.Error(t, err) + assert.True(t, errors.Is(err, s3err.GetAPIError(s3err.ErrAccessDenied))) + assert.Equal(t, 0, be.aclCalls) +} + +func TestVerifyPublicAccess_PublicPolicyNoMatchFallsBackToACL(t *testing.T) { + be := &publicBucketPolicyBackend{ + policy: []byte(`{ + "Statement": [{ + "Effect": "Deny", + "Principal": "*", + "Action": "s3:GetObject", + "Resource": "arn:aws:s3:::bucket/private/*" + }] + }`), + acl: publicReadACL(), + } + + err := VerifyPublicAccess(context.Background(), be, GetObjectAction, PermissionRead, "bucket", "public/object.txt") + + assert.NoError(t, err) + assert.Equal(t, 1, be.aclCalls) +} + func TestVerifyObjectCopyAccess_URLEncodedSlashSeparator(t *testing.T) { const testUser = "testuser" diff --git a/auth/acl.go b/auth/acl.go index 45123223..807efbc7 100644 --- a/auth/acl.go +++ b/auth/acl.go @@ -472,7 +472,7 @@ func VerifyPublicBucketACL(ctx context.Context, be backend.Backend, bucket strin } if !acl.IsPublic(permission) { - return ErrAccessDenied + return errAccessDenied } return nil diff --git a/auth/bucket_policy.go b/auth/bucket_policy.go index f30188b0..5c47764f 100644 --- a/auth/bucket_policy.go +++ b/auth/bucket_policy.go @@ -23,7 +23,19 @@ import ( "github.com/versity/versitygw/s3err" ) -var ErrAccessDenied = errors.New("access denied") +var errAccessDenied = errors.New("access denied") +var errExplicitDeny = errors.New("explicit deny") + +// policyDecision preserves the difference between "not allowed" and "denied". +// Public bucket authorization needs that distinction so no-match can fall back +// to ACLs while explicit Deny cannot. +type policyDecision int + +const ( + policyDecisionNoMatch policyDecision = iota + policyDecisionAllow + policyDecisionDeny +) type policyErr string @@ -105,9 +117,7 @@ func (bp *BucketPolicy) isAllowed(principal string, action Action, resource stri return isAllowed } -// IsPublicFor checks if the bucket policy statements contain -// an entity granting public access to the given resource and action -func (bp *BucketPolicy) isPublicFor(resource string, action Action) bool { +func (bp *BucketPolicy) publicDecisionFor(resource string, action Action) policyDecision { var isAllowed bool for _, statement := range bp.Statement { if statement.isPublicFor(resource, action) { @@ -115,12 +125,16 @@ func (bp *BucketPolicy) isPublicFor(resource string, action Action) bool { case BucketPolicyAccessTypeAllow: isAllowed = true case BucketPolicyAccessTypeDeny: - return false + return policyDecisionDeny } } } - return isAllowed + // A matching Allow grants access only when no matching Deny was found. + if isAllowed { + return policyDecisionAllow + } + return policyDecisionNoMatch } // IsPublic checks if one of bucket policy statments grant @@ -270,11 +284,14 @@ func VerifyPublicBucketPolicy(policy []byte, bucket, object string, action Actio resource += "/" + object } - if !bucketPolicy.isPublicFor(resource, action) { - return ErrAccessDenied + switch bucketPolicy.publicDecisionFor(resource, action) { + case policyDecisionAllow: + return nil + case policyDecisionDeny: + return errExplicitDeny + default: + return errAccessDenied } - - return nil } // matchPattern checks if the input string matches the given pattern with wildcard(`*`) and any character(`?`). diff --git a/tests/certs/azurite.pem b/tests/certs/azurite.pem index 4f4cf8c9..a31052c0 100644 --- a/tests/certs/azurite.pem +++ b/tests/certs/azurite.pem @@ -1,25 +1,20 @@ -----BEGIN CERTIFICATE----- -MIIEMzCCApugAwIBAgIQbYDa23tko31nfiGrCJ4XfDANBgkqhkiG9w0BAQsFADBj -MR4wHAYDVQQKExVta2NlcnQgZGV2ZWxvcG1lbnQgQ0ExHDAaBgNVBAsME2JlbkBC -eXRlTmV4dXMubG9jYWwxIzAhBgNVBAMMGm1rY2VydCBiZW5AQnl0ZU5leHVzLmxv -Y2FsMB4XDTI0MDIyMTA2MDY1OFoXDTI2MDUyMTA1MDY1OFowRzEnMCUGA1UEChMe -bWtjZXJ0IGRldmVsb3BtZW50IGNlcnRpZmljYXRlMRwwGgYDVQQLDBNiZW5AQnl0 -ZU5leHVzLmxvY2FsMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA1GhK -E4c3+m1jVEe/ldLGBjQ9IgKxPHcd94wqlgc1AnngJ3/FY7Ck1SlivAnq546Le4z/ -xpShwlylQg6WsNkrnpFZ/OIF69FAw9HJaz2GHMrZvE1q3zZnD55iwER+m3rPuRpb -dtYLoOX3O4L3fxBSP3jUwJlOmT81YylT+aZb5EmWF3j4JoCs2PIbPI/SI4zmQfk8 -2pUZUKDGuIT2E8zab3Ut2kAjP/D03nwvLMSNB/Yp8dbrWsdtjDzQmhpfgbw/cqrx -cEJe59Hfbt2SyfAa/VCkT1+OzQ3q6bGLJiQALY3r3ovOL+JYgircSMwum/yriqYB -A4Ys2vky/1aCd+MSvQIDAQABo38wfTAOBgNVHQ8BAf8EBAMCBaAwEwYDVR0lBAww -CgYIKwYBBQUHAwEwHwYDVR0jBBgwFoAUHWzgJTYXxrKTuaJyaD2ddH4g43QwNQYD -VR0RBC4wLIIJbG9jYWxob3N0ggdhenVyaXRlhwR/AAABhxAAAAAAAAAAAAAAAAAA -AAABMA0GCSqGSIb3DQEBCwUAA4IBgQBANjSWFEE2XrQfCtqKjHRtYn+WnjfeUmJW -4qfNl971spR+ui+WMNob53EKGV9O55Ai8zWUKhL14GSDo4bqygcqCSP7esUaqW5x -LgqGkIDPiPN18OfWxZonlvcSJYaWJ9dL4Xix7OmPXQV88oNUYh5zASJRZ8mN3vZK -v0BElS5w7X4pM8jWkIEVUjmgSQJAxapDnbDPCIntH13rjYPHZW3zF1wjWW/mfW59 -+thLVpG0U9w8knDU9TJQWPX9rQB8lLBHYfBy14ebuMWmDl25Rr+wcUjFp76HP8f9 -D8wX7ZTJlMlaH60vbTi8ylsKMlu6eHxJ8TBNdRI5+Xa94Pi6A81a9EiagpU2uZs1 -P/v/3ug9AtCFoEBB2groj3waC6QTOrJArHhvioLKBz7yMsfGRQCSA+VK+KGuqE6f -E/Knv1Phf8HIe6OpDNXqlJo9+5m0sz/y6F2OaeNqM643Y78OQRFijtDKu+TmB+t0 -4lo/rMVz6A0vIOBBQkiJz82xezvCiHQ= +MIIDPjCCAiagAwIBAgIUbnQOs4tA0EPoTm6oxbLmCyrNPZEwDQYJKoZIhvcNAQEL +BQAwEjEQMA4GA1UEAwwHYXp1cml0ZTAeFw0yNjA1MjExNjA4MThaFw0zNjA1MTgx +NjA4MThaMBIxEDAOBgNVBAMMB2F6dXJpdGUwggEiMA0GCSqGSIb3DQEBAQUAA4IB +DwAwggEKAoIBAQDUaEoThzf6bWNUR7+V0sYGND0iArE8dx33jCqWBzUCeeAnf8Vj +sKTVKWK8Cernjot7jP/GlKHCXKVCDpaw2SuekVn84gXr0UDD0clrPYYcytm8TWrf +NmcPnmLARH6bes+5Glt21gug5fc7gvd/EFI/eNTAmU6ZPzVjKVP5plvkSZYXePgm +gKzY8hs8j9IjjOZB+TzalRlQoMa4hPYTzNpvdS3aQCM/8PTefC8sxI0H9inx1uta +x22MPNCaGl+BvD9yqvFwQl7n0d9u3ZLJ8Br9UKRPX47NDerpsYsmJAAtjevei84v +4liCKtxIzC6b/KuKpgEDhiza+TL/VoJ34xK9AgMBAAGjgYswgYgwHQYDVR0OBBYE +FNKVJRyPXIu+vUuKVKBNTiEriJfHMB8GA1UdIwQYMBaAFNKVJRyPXIu+vUuKVKBN +TiEriJfHMA8GA1UdEwEB/wQFMAMBAf8wNQYDVR0RBC4wLIIJbG9jYWxob3N0ggdh +enVyaXRlhwR/AAABhxAAAAAAAAAAAAAAAAAAAAABMA0GCSqGSIb3DQEBCwUAA4IB +AQCRUO0ZOT1hgFw9G/H8iABSARQrkUj6d/ueGYzCtgS6Lu1NBOBb53QkuDjG0wEz +MObnYe2dffxwPrxe3wsNv0QmjhL6eQ1IgrKWdb2wr3sa9HBCykRSLUJwfkWaVuXx +/PEKKVMGED/PGcf+Yjd+x8mqrxyD8nvG2F+UmBNlHCCSEs64mUCtPFjE2b2bxj7N +21EGNm73dAUJo+8YGVG6pRs1KDfvP7XyLUg6rhtjCoV5gFjwWzRcf7U3gbD6tbVs +YAKJDdGQQ1Zil8B5Gb0vZ7w5kJGItxwL5Zxc2KX8A83R60pe0W31Tpy4k6xp1cxy +JhFixfy9eg96EtR+Fq7i9ahK -----END CERTIFICATE----- diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index fe7f5df1..24c29d44 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -1049,6 +1049,7 @@ func TestPublicBuckets(ts *TestState) { ts.Run(PublicBucket_public_object_policy) } ts.Run(PublicBucket_public_acl) + ts.Run(PublicBucket_policy_deny_overrides_public_acl) ts.Run(PublicBucket_signed_streaming_payload) ts.Run(PublicBucket_incorrect_sha256_hash) } @@ -1892,6 +1893,7 @@ func GetIntTests() IntTests { "PublicBucket_public_bucket_policy": PublicBucket_public_bucket_policy, "PublicBucket_public_object_policy": PublicBucket_public_object_policy, "PublicBucket_public_acl": PublicBucket_public_acl, + "PublicBucket_policy_deny_overrides_public_acl": PublicBucket_policy_deny_overrides_public_acl, "PublicBucket_signed_streaming_payload": PublicBucket_signed_streaming_payload, "PublicBucket_incorrect_sha256_hash": PublicBucket_incorrect_sha256_hash, "PutBucketVersioning_non_existing_bucket": PutBucketVersioning_non_existing_bucket, diff --git a/tests/integration/public_bucket.go b/tests/integration/public_bucket.go index 7482f9ff..a709bbba 100644 --- a/tests/integration/public_bucket.go +++ b/tests/integration/public_bucket.go @@ -2435,6 +2435,61 @@ func PublicBucket_public_acl(s *S3Conf) error { }, withAnonymousClient(), withOwnership(types.ObjectOwnershipBucketOwnerPreferred)) } +func PublicBucket_policy_deny_overrides_public_acl(s *S3Conf) error { + testName := "PublicBucket_policy_deny_overrides_public_acl" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + rootClient := s.GetClient() + publicKey := "public/object" + privateKey := "private/secret" + + for _, key := range []string{publicKey, privateKey} { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := rootClient.PutObject(ctx, &s3.PutObjectInput{ + Bucket: &bucket, + Key: &key, + Body: bytes.NewReader([]byte(key)), + }) + cancel() + if err != nil { + return err + } + } + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := rootClient.PutBucketAcl(ctx, &s3.PutBucketAclInput{ + Bucket: &bucket, + ACL: types.BucketCannedACLPublicRead, + }) + cancel() + if err != nil { + return err + } + + policy := genPolicyDoc("Deny", `"*"`, `"s3:GetObject"`, fmt.Sprintf(`"arn:aws:s3:::%s/private/*"`, bucket)) + if err := putBucketPolicy(rootClient, bucket, policy); err != nil { + return err + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.GetObject(ctx, &s3.GetObjectInput{ + Bucket: &bucket, + Key: &publicKey, + }) + cancel() + if err != nil { + return fmt.Errorf("expected public-read ACL to allow non-denied object: %w", err) + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.GetObject(ctx, &s3.GetObjectInput{ + Bucket: &bucket, + Key: &privateKey, + }) + cancel() + return checkApiErr(err, s3err.GetAPIError(s3err.ErrAccessDenied)) + }, withAnonymousClient(), withOwnership(types.ObjectOwnershipBucketOwnerPreferred)) +} + func PublicBucket_signed_streaming_payload(s *S3Conf) error { testName := "PublicBucket_signed_streaming_payload" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {