mirror of
https://github.com/seaweedfs/seaweedfs.git
synced 2026-09-20 15:04:37 +00:00
s3: bucket-policy Allow must not override an identity explicit Deny (#11256)
* s3: add isActionExplicitlyDeniedByApplicablePolicies helper Add a helper that reports whether any applicable identity-side policy (attached IAM policies, enabled-group policies, or the IAM-integration session policy) explicitly denies an action. It reuses the existing evaluateAttachedIAMPolicies, resolveS3AuthTarget, buildPrincipalARN, and isActionExplicitlyDeniedByIAM helpers, and fails closed on evaluation errors. A nil identity has no identity-side policy plane, so the helper returns false to keep the bucket policy authoritative for anonymous access. No behavior change yet; the next commits apply it to the two bucket-policy Allow short-circuits. * s3: enforce identity explicit Deny before bucket-policy Allow authRequestWithAuthType short-circuits on a matching bucket-policy Allow and skips VerifyActionPermission, so an explicit Deny in an authenticated identity attached, group, or session policy is bypassed. A non-admin principal with s3:PutBucketPolicy can install a bucket-policy Allow for itself and read an object its identity policy explicitly denies. Before honoring a bucket-policy Allow, check the applicable identity-side policies for a matching explicit Deny via the new isActionExplicitlyDeniedByApplicablePolicies helper, and fail closed. The cross-account behavior is preserved: a bucket Allow still supplies the Allow an identity policy omits (implicit denial), and a nil identity keeps the bucket policy authoritative for anonymous access. Regression tests cover the explicit-Deny override, the implicit-deny Allow preservation, and the unmatched-key fall-through control. * s3: enforce identity explicit Deny in secondary object-key auth authorizeObjectKeyAction authorizes keys the request URL does not name (CopySource, DeleteObjects body keys, POST Object form keys) and shares the same bucket-policy Allow short-circuit as the primary path, so an explicit Deny in the identity, group, or session policy is bypassed the same way when a bucket policy allows the secondary key. Apply isActionExplicitlyDeniedByApplicablePolicies before accepting the bucket-policy Allow, mirroring the primary path. A regression test covers AuthorizeCopySource for both the explicit-Deny override and the implicit-deny Allow preservation.
This commit is contained in:
Chris Lu
@@ -1813,8 +1813,14 @@ func (iam *IdentityAccessManagement) authRequestWithAuthType(r *http.Request, ac
|
||||
} else if evaluated {
|
||||
// A bucket policy exists and was evaluated with a matching statement
|
||||
if allowed {
|
||||
// Policy explicitly allows this action - grant access immediately
|
||||
// This bypasses IAM checks to support cross-account access and policy-only principals
|
||||
// A resource policy may supply the Allow an identity policy omits
|
||||
// (for cross-account access and policy-only principals), but it
|
||||
// must not override an applicable explicit Deny from an identity,
|
||||
// group, or session policy.
|
||||
if iam.isActionExplicitlyDeniedByApplicablePolicies(r, identity, action, bucket, object) {
|
||||
glog.V(3).Infof("identity policy explicitly denies %s to %s on %s/%s despite bucket policy allow", identity.Name, action, bucket, object)
|
||||
return identity, s3err.ErrAccessDenied, reqAuthType
|
||||
}
|
||||
glog.V(3).Infof("Bucket policy allows %s to %s on %s/%s (bypassing IAM)", identity.Name, action, bucket, object)
|
||||
policyAllows = true
|
||||
} else {
|
||||
@@ -2607,6 +2613,24 @@ func (iam *IdentityAccessManagement) isActionExplicitlyDeniedByIAM(r *http.Reque
|
||||
return denied
|
||||
}
|
||||
|
||||
// isActionExplicitlyDeniedByApplicablePolicies reports whether any applicable
|
||||
// identity-side policy (attached IAM policies, enabled-group policies, or the
|
||||
// IAM-integration session policy) explicitly denies the action. A bucket
|
||||
// policy may supply the Allow an identity policy omits, but it must not
|
||||
// override a matching explicit Deny. A nil identity has no identity-side
|
||||
// policy plane, so the bucket policy remains authoritative for public access.
|
||||
func (iam *IdentityAccessManagement) isActionExplicitlyDeniedByApplicablePolicies(r *http.Request, identity *Identity, action Action, bucket, object string) bool {
|
||||
if identity == nil {
|
||||
return false
|
||||
}
|
||||
if iam.evaluateAttachedIAMPolicies(r, identity, action, bucket, object) == attachedIAMPolicyDeny {
|
||||
return true
|
||||
}
|
||||
s3Action, resourceArn := resolveS3AuthTarget(action, bucket, object, r)
|
||||
principal := buildPrincipalARN(identity, r)
|
||||
return iam.isActionExplicitlyDeniedByIAM(r, identity, principal, s3Action, resourceArn)
|
||||
}
|
||||
|
||||
// authorizationRoute is the mechanism that decides a request/identity pair's
|
||||
// permissions: the IAM integration, locally attached IAM policies, the
|
||||
// identity's legacy Actions, or nothing at all.
|
||||
@@ -2844,6 +2868,9 @@ func (iam *IdentityAccessManagement) authorizeObjectKeyAction(r *http.Request, i
|
||||
}
|
||||
if evaluated {
|
||||
if allowed {
|
||||
if iam.isActionExplicitlyDeniedByApplicablePolicies(keyReq, identity, action, bucket, objectKey) {
|
||||
return s3err.ErrAccessDenied
|
||||
}
|
||||
return s3err.ErrNone
|
||||
}
|
||||
return s3err.ErrAccessDenied
|
||||
|
||||
@@ -0,0 +1,182 @@
|
||||
package s3api
|
||||
|
||||
import (
|
||||
"fmt"
|
||||
"net/http"
|
||||
"sync"
|
||||
"testing"
|
||||
|
||||
"github.com/gorilla/mux"
|
||||
"github.com/seaweedfs/seaweedfs/weed/pb/iam_pb"
|
||||
"github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants"
|
||||
"github.com/seaweedfs/seaweedfs/weed/s3api/s3err"
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
|
||||
// Reproducer for GHSA-xxc3-72qf-3g5r: a bucket-policy Allow must not override an
|
||||
// applicable explicit Deny in an authenticated identity's attached policy.
|
||||
//
|
||||
// restricted-reader holds an attached IAM policy that allows GetObject on
|
||||
// allowed/*, explicitly denies GetObject on secret/*, and may install a bucket
|
||||
// policy. The bucket policy separately allows it GetObject on secret/* and
|
||||
// other/*. AWS evaluation lets a resource policy supply the Allow an identity
|
||||
// policy omits (other/*), but an explicit Deny must still win (secret/*).
|
||||
|
||||
const (
|
||||
bpdBucket = "victim-bucket"
|
||||
bpdAccessKey = "LOCALDENYKEY00000001"
|
||||
bpdSecretKey = "local-deny-secret-for-loopback-only"
|
||||
bpdPrincipal = "arn:aws:iam::000000000000:user/restricted-reader"
|
||||
bpdPolicyName = "RestrictedRead"
|
||||
bpdIdentityName = "restricted-reader"
|
||||
bpdAccountID = "000000000000"
|
||||
bpdAccountName = "restricted-reader"
|
||||
)
|
||||
|
||||
const bpdIdentityPolicy = `{
|
||||
"Version":"2012-10-17",
|
||||
"Statement":[
|
||||
{"Effect":"Allow","Action":"s3:PutBucketPolicy","Resource":"arn:aws:s3:::` + bpdBucket + `"},
|
||||
{"Effect":"Allow","Action":"s3:GetObject","Resource":"arn:aws:s3:::` + bpdBucket + `/allowed/*"},
|
||||
{"Effect":"Deny","Action":"s3:GetObject","Resource":"arn:aws:s3:::` + bpdBucket + `/secret/*"}
|
||||
]
|
||||
}`
|
||||
|
||||
// bpdBucketPolicyDoc returns a bucket policy that allows restricted-reader
|
||||
// GetObject on the given prefixes. The secret/* statement is the bypass under
|
||||
// test; the other/* statement supplies an Allow over an implicit identity
|
||||
// denial, which the fix must preserve.
|
||||
func bpdBucketPolicyDoc(t *testing.T, prefixes ...string) string {
|
||||
t.Helper()
|
||||
stmts := ""
|
||||
for i, p := range prefixes {
|
||||
if i > 0 {
|
||||
stmts += ","
|
||||
}
|
||||
stmts += fmt.Sprintf(
|
||||
`{"Effect":"Allow","Principal":{"AWS":"%s"},"Action":"s3:GetObject","Resource":"arn:aws:s3:::%s/%s"}`,
|
||||
bpdPrincipal, bpdBucket, p)
|
||||
}
|
||||
return fmt.Sprintf(`{"Version":"2012-10-17","Statement":[%s]}`, stmts)
|
||||
}
|
||||
|
||||
// newBucketPolicyDenyIAM builds an IAM whose restricted-reader identity carries
|
||||
// the attached RestrictedRead policy, plus a bucket policy engine that allows
|
||||
// the reader on the supplied prefixes.
|
||||
func newBucketPolicyDenyIAM(t *testing.T, bucketPolicy string) *IdentityAccessManagement {
|
||||
t.Helper()
|
||||
iam := &IdentityAccessManagement{
|
||||
hashes: make(map[string]*sync.Pool),
|
||||
hashCounters: make(map[string]*int32),
|
||||
}
|
||||
err := iam.loadS3ApiConfiguration(&iam_pb.S3ApiConfiguration{
|
||||
Accounts: []*iam_pb.Account{
|
||||
{Id: bpdAccountID, DisplayName: bpdAccountName},
|
||||
},
|
||||
Identities: []*iam_pb.Identity{
|
||||
{
|
||||
Name: "admin",
|
||||
Actions: []string{"Admin"},
|
||||
Credentials: []*iam_pb.Credential{
|
||||
{AccessKey: "LOCALADMINKEY0000001", SecretKey: "local-admin-secret-for-loopback-only"},
|
||||
},
|
||||
},
|
||||
{
|
||||
Name: bpdIdentityName,
|
||||
Account: &iam_pb.Account{Id: bpdAccountID, DisplayName: bpdAccountName},
|
||||
PolicyNames: []string{bpdPolicyName},
|
||||
Credentials: []*iam_pb.Credential{
|
||||
{AccessKey: bpdAccessKey, SecretKey: bpdSecretKey},
|
||||
},
|
||||
},
|
||||
},
|
||||
Policies: []*iam_pb.Policy{
|
||||
{Name: bpdPolicyName, Content: bpdIdentityPolicy},
|
||||
},
|
||||
})
|
||||
require.NoError(t, err)
|
||||
|
||||
engine := NewBucketPolicyEngine()
|
||||
require.NoError(t, engine.engine.SetBucketPolicy(bpdBucket, bucketPolicy))
|
||||
iam.policyEngine = engine
|
||||
return iam
|
||||
}
|
||||
|
||||
// bpdSignedRequest builds a SigV4-signed S3 request for the given key, signed
|
||||
// as the restricted-reader, with mux vars set so GetBucketAndObject resolves.
|
||||
func bpdSignedRequest(t *testing.T, method, objectKey string) *http.Request {
|
||||
t.Helper()
|
||||
urlStr := fmt.Sprintf("http://127.0.0.1:9000/%s/%s", bpdBucket, objectKey)
|
||||
req := mustNewRequest(method, urlStr, 0, nil, t)
|
||||
require.NoError(t, signRequestV4(req, bpdAccessKey, bpdSecretKey))
|
||||
return mux.SetURLVars(req, map[string]string{"bucket": bpdBucket, "object": objectKey})
|
||||
}
|
||||
|
||||
// bpdReader looks up the loaded restricted-reader identity.
|
||||
func bpdReader(t *testing.T, iam *IdentityAccessManagement) *Identity {
|
||||
t.Helper()
|
||||
ident := iam.lookupByIdentityName(bpdIdentityName)
|
||||
require.NotNil(t, ident, "restricted-reader must be loaded")
|
||||
return ident
|
||||
}
|
||||
|
||||
// TestBucketPolicyAllowDoesNotOverrideIdentityExplicitDeny is the primary
|
||||
// reproducer: a bucket-policy Allow on secret/* must not let restricted-reader
|
||||
// read an object its attached policy explicitly denies.
|
||||
func TestBucketPolicyAllowDoesNotOverrideIdentityExplicitDeny(t *testing.T) {
|
||||
iam := newBucketPolicyDenyIAM(t, bpdBucketPolicyDoc(t, "secret/*", "other/*"))
|
||||
|
||||
// Precondition: the bucket policy alone would allow the reader on secret/*.
|
||||
req := bpdSignedRequest(t, http.MethodGet, "secret/payroll.txt")
|
||||
allowed, evaluated, err := iam.policyEngine.EvaluatePolicy(
|
||||
bpdBucket, "secret/payroll.txt", s3_constants.ACTION_READ, bpdPrincipal, req, nil, nil)
|
||||
require.NoError(t, err)
|
||||
require.True(t, evaluated && allowed, "bucket policy must allow the reader on secret/*")
|
||||
|
||||
_, errCode := iam.authRequest(req, s3_constants.ACTION_READ)
|
||||
assert.Equal(t, s3err.ErrAccessDenied, errCode,
|
||||
"a bucket-policy Allow must not override an explicit Deny in the identity policy")
|
||||
}
|
||||
|
||||
// TestBucketPolicyAllowSuppliesImplicitIdentityDeny preserves the cross-account
|
||||
// behavior: a bucket-policy Allow on a prefix the identity policy does not
|
||||
// mention (other/*) must still grant access.
|
||||
func TestBucketPolicyAllowSuppliesImplicitIdentityDeny(t *testing.T) {
|
||||
iam := newBucketPolicyDenyIAM(t, bpdBucketPolicyDoc(t, "secret/*", "other/*"))
|
||||
|
||||
_, errCode := iam.authRequest(bpdSignedRequest(t, http.MethodGet, "other/data.txt"), s3_constants.ACTION_READ)
|
||||
assert.Equal(t, s3err.ErrNone, errCode,
|
||||
"a bucket-policy Allow must supply the Allow an identity policy omits (implicit denial)")
|
||||
}
|
||||
|
||||
// TestBucketPolicyAllowControlPathUnaffected checks the path the bucket policy
|
||||
// does not match: allowed/* falls through to the identity policy, which allows.
|
||||
func TestBucketPolicyAllowControlPathUnaffected(t *testing.T) {
|
||||
iam := newBucketPolicyDenyIAM(t, bpdBucketPolicyDoc(t, "secret/*", "other/*"))
|
||||
|
||||
_, errCode := iam.authRequest(bpdSignedRequest(t, http.MethodGet, "allowed/report.txt"), s3_constants.ACTION_READ)
|
||||
assert.Equal(t, s3err.ErrNone, errCode,
|
||||
"a key the bucket policy does not match must fall through to the identity policy")
|
||||
}
|
||||
|
||||
// TestAuthorizeObjectKeyActionBucketAllowDoesNotOverrideIdentityDeny covers the
|
||||
// secondary object-key authorization path (CopySource, DeleteObjects body
|
||||
// keys, POST Object form keys): the same bucket-Allow short-circuit must also
|
||||
// honor an applicable explicit identity Deny.
|
||||
func TestAuthorizeObjectKeyActionBucketAllowDoesNotOverrideIdentityDeny(t *testing.T) {
|
||||
iam := newBucketPolicyDenyIAM(t, bpdBucketPolicyDoc(t, "secret/*", "other/*"))
|
||||
reader := bpdReader(t, iam)
|
||||
req := bpdSignedRequest(t, http.MethodGet, "secret/payroll.txt")
|
||||
|
||||
// CopySource reads the source key; the bucket policy allows it but the
|
||||
// identity policy explicitly denies GetObject on secret/*.
|
||||
assert.Equal(t, s3err.ErrAccessDenied,
|
||||
iam.AuthorizeCopySource(req, reader, bpdBucket, "secret/payroll.txt", ""),
|
||||
"AuthorizeCopySource must honor the identity explicit Deny despite a bucket-policy Allow")
|
||||
|
||||
// other/* has no identity statement, so the bucket Allow supplies access.
|
||||
assert.Equal(t, s3err.ErrNone,
|
||||
iam.AuthorizeCopySource(req, reader, bpdBucket, "other/data.txt", ""),
|
||||
"AuthorizeCopySource must preserve the bucket Allow over an implicit identity denial")
|
||||
}
|
||||
Reference in New Issue
Block a user