fix: honor explicit public bucket policy deny

Distinguish public bucket policy no-match from explicit deny during anonymous access checks. This preserves ACL fallback only for requests that are not allowed by policy, while ensuring a matching Deny statement short-circuits authorization and returns AccessDenied even when a public ACL would otherwise grant access.

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'

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"
 

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

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(`?`).

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-----

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,

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 {