s3api: cache parsed IAM policy engines for fallback auth
Previously, evaluateIAMPolicies created a new PolicyEngine and re-parsed the JSON policy document for every policy on every request. This adds a shared iamPolicyEngine field that caches compiled policies, kept in sync by PutPolicy, DeletePolicy, and bulk config reload paths. - PutPolicy deletes the old cache entry before setting the new one, so a parse failure on update does not leave a stale allow. - Log warnings when policy compilation fails instead of silently discarding errors. - Add test for valid-to-invalid policy update regression.
This commit is contained in:
@@ -68,6 +68,10 @@ type IdentityAccessManagement struct {
|
|||||||
// Bucket policy engine for evaluating bucket policies
|
// Bucket policy engine for evaluating bucket policies
|
||||||
policyEngine *BucketPolicyEngine
|
policyEngine *BucketPolicyEngine
|
||||||
|
|
||||||
|
// Cached policy engine for IAM policy fallback evaluation.
|
||||||
|
// Keyed by policy name, kept in sync by PutPolicy/DeletePolicy.
|
||||||
|
iamPolicyEngine *policy_engine.PolicyEngine
|
||||||
|
|
||||||
// background polling
|
// background polling
|
||||||
stopChan chan struct{}
|
stopChan chan struct{}
|
||||||
shutdownOnce sync.Once
|
shutdownOnce sync.Once
|
||||||
@@ -659,6 +663,7 @@ func (iam *IdentityAccessManagement) ReplaceS3ApiConfiguration(config *iam_pb.S3
|
|||||||
iam.nameToIdentity = nameToIdentity
|
iam.nameToIdentity = nameToIdentity
|
||||||
iam.accessKeyIdent = accessKeyIdent
|
iam.accessKeyIdent = accessKeyIdent
|
||||||
iam.policies = policies
|
iam.policies = policies
|
||||||
|
iam.rebuildIAMPolicyEngineLocked()
|
||||||
|
|
||||||
// Re-add environment-based identities that were preserved
|
// Re-add environment-based identities that were preserved
|
||||||
for _, envIdent := range envIdentities {
|
for _, envIdent := range envIdentities {
|
||||||
@@ -915,6 +920,7 @@ func (iam *IdentityAccessManagement) MergeS3ApiConfiguration(config *iam_pb.S3Ap
|
|||||||
iam.nameToIdentity = nameToIdentity
|
iam.nameToIdentity = nameToIdentity
|
||||||
iam.accessKeyIdent = accessKeyIdent
|
iam.accessKeyIdent = accessKeyIdent
|
||||||
iam.policies = policies
|
iam.policies = policies
|
||||||
|
iam.rebuildIAMPolicyEngineLocked()
|
||||||
// Update authentication state based on whether identities exist
|
// Update authentication state based on whether identities exist
|
||||||
// Once enabled, keep it enabled (one-way toggle)
|
// Once enabled, keep it enabled (one-way toggle)
|
||||||
authJustEnabled := iam.updateAuthenticationState(len(identities))
|
authJustEnabled := iam.updateAuthenticationState(len(identities))
|
||||||
@@ -1661,11 +1667,20 @@ func determineIAMAuthPath(sessionToken, principal, principalArn string) iamAuthP
|
|||||||
|
|
||||||
// evaluateIAMPolicies evaluates attached IAM policies for a user identity.
|
// evaluateIAMPolicies evaluates attached IAM policies for a user identity.
|
||||||
// Returns true if any matching statement explicitly allows the action.
|
// Returns true if any matching statement explicitly allows the action.
|
||||||
|
// Uses the cached iamPolicyEngine to avoid re-parsing policy JSON on every request.
|
||||||
func (iam *IdentityAccessManagement) evaluateIAMPolicies(r *http.Request, identity *Identity, action Action, bucket, object string) bool {
|
func (iam *IdentityAccessManagement) evaluateIAMPolicies(r *http.Request, identity *Identity, action Action, bucket, object string) bool {
|
||||||
if identity == nil || len(identity.PolicyNames) == 0 {
|
if identity == nil || len(identity.PolicyNames) == 0 {
|
||||||
return false
|
return false
|
||||||
}
|
}
|
||||||
|
|
||||||
|
iam.m.RLock()
|
||||||
|
engine := iam.iamPolicyEngine
|
||||||
|
iam.m.RUnlock()
|
||||||
|
|
||||||
|
if engine == nil {
|
||||||
|
return false
|
||||||
|
}
|
||||||
|
|
||||||
resource := buildResourceARN(bucket, object)
|
resource := buildResourceARN(bucket, object)
|
||||||
principal := buildPrincipalARN(identity, r)
|
principal := buildPrincipalARN(identity, r)
|
||||||
s3Action := ResolveS3Action(r, string(action), bucket, object)
|
s3Action := ResolveS3Action(r, string(action), bucket, object)
|
||||||
@@ -1676,16 +1691,6 @@ func (iam *IdentityAccessManagement) evaluateIAMPolicies(r *http.Request, identi
|
|||||||
}
|
}
|
||||||
|
|
||||||
for _, policyName := range identity.PolicyNames {
|
for _, policyName := range identity.PolicyNames {
|
||||||
policy, err := iam.GetPolicy(policyName)
|
|
||||||
if err != nil {
|
|
||||||
continue
|
|
||||||
}
|
|
||||||
|
|
||||||
engine := policy_engine.NewPolicyEngine()
|
|
||||||
if err := engine.SetBucketPolicy(policyName, policy.Content); err != nil {
|
|
||||||
continue
|
|
||||||
}
|
|
||||||
|
|
||||||
result := engine.EvaluatePolicy(policyName, &policy_engine.PolicyEvaluationArgs{
|
result := engine.EvaluatePolicy(policyName, &policy_engine.PolicyEvaluationArgs{
|
||||||
Action: s3Action,
|
Action: s3Action,
|
||||||
Resource: resource,
|
Resource: resource,
|
||||||
@@ -1808,6 +1813,12 @@ func (iam *IdentityAccessManagement) PutPolicy(name string, content string) erro
|
|||||||
iam.policies = make(map[string]*iam_pb.Policy)
|
iam.policies = make(map[string]*iam_pb.Policy)
|
||||||
}
|
}
|
||||||
iam.policies[name] = &iam_pb.Policy{Name: name, Content: content}
|
iam.policies[name] = &iam_pb.Policy{Name: name, Content: content}
|
||||||
|
iam.ensureIAMPolicyEngine()
|
||||||
|
// Remove old entry first so that a parse failure doesn't leave a stale allow.
|
||||||
|
_ = iam.iamPolicyEngine.DeleteBucketPolicy(name)
|
||||||
|
if err := iam.iamPolicyEngine.SetBucketPolicy(name, content); err != nil {
|
||||||
|
glog.Warningf("IAM policy %q is stored but could not be compiled for cache: %v", name, err)
|
||||||
|
}
|
||||||
return nil
|
return nil
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -1826,9 +1837,36 @@ func (iam *IdentityAccessManagement) DeletePolicy(name string) error {
|
|||||||
iam.m.Lock()
|
iam.m.Lock()
|
||||||
defer iam.m.Unlock()
|
defer iam.m.Unlock()
|
||||||
delete(iam.policies, name)
|
delete(iam.policies, name)
|
||||||
|
if iam.iamPolicyEngine != nil {
|
||||||
|
_ = iam.iamPolicyEngine.DeleteBucketPolicy(name)
|
||||||
|
}
|
||||||
return nil
|
return nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// ensureIAMPolicyEngine lazily initializes the shared IAM policy engine.
|
||||||
|
// Must be called with iam.m held.
|
||||||
|
func (iam *IdentityAccessManagement) ensureIAMPolicyEngine() {
|
||||||
|
if iam.iamPolicyEngine == nil {
|
||||||
|
iam.iamPolicyEngine = policy_engine.NewPolicyEngine()
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// rebuildIAMPolicyEngineLocked rebuilds the entire IAM policy engine cache
|
||||||
|
// from the current policies map. Must be called with iam.m held.
|
||||||
|
func (iam *IdentityAccessManagement) rebuildIAMPolicyEngineLocked() {
|
||||||
|
if len(iam.policies) == 0 {
|
||||||
|
iam.iamPolicyEngine = nil
|
||||||
|
return
|
||||||
|
}
|
||||||
|
engine := policy_engine.NewPolicyEngine()
|
||||||
|
for name, p := range iam.policies {
|
||||||
|
if err := engine.SetBucketPolicy(name, p.Content); err != nil {
|
||||||
|
glog.Warningf("IAM policy cache rebuild: skipping invalid policy %q: %v", name, err)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
iam.iamPolicyEngine = engine
|
||||||
|
}
|
||||||
|
|
||||||
// ListPolicies lists all policies
|
// ListPolicies lists all policies
|
||||||
func (iam *IdentityAccessManagement) ListPolicies() []*iam_pb.Policy {
|
func (iam *IdentityAccessManagement) ListPolicies() []*iam_pb.Policy {
|
||||||
iam.m.RLock()
|
iam.m.RLock()
|
||||||
|
|||||||
@@ -374,6 +374,28 @@ func TestVerifyActionPermissionPolicyFallback(t *testing.T) {
|
|||||||
assert.Equal(t, s3err.ErrNone, errCode)
|
assert.Equal(t, s3err.ErrNone, errCode)
|
||||||
})
|
})
|
||||||
|
|
||||||
|
t.Run("valid policy updated to invalid denies access", func(t *testing.T) {
|
||||||
|
iam := &IdentityAccessManagement{}
|
||||||
|
err := iam.PutPolicy("myPolicy", `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":"s3:GetObject","Resource":"arn:aws:s3:::test-bucket/*"}]}`)
|
||||||
|
assert.NoError(t, err)
|
||||||
|
|
||||||
|
identity := &Identity{
|
||||||
|
Name: "policy-user",
|
||||||
|
Account: &AccountAdmin,
|
||||||
|
PolicyNames: []string{"myPolicy"},
|
||||||
|
}
|
||||||
|
|
||||||
|
errCode := iam.VerifyActionPermission(buildRequest(t, http.MethodGet), identity, Action(ACTION_READ), "test-bucket", "test-object")
|
||||||
|
assert.Equal(t, s3err.ErrNone, errCode)
|
||||||
|
|
||||||
|
// Update to invalid JSON — should revoke access.
|
||||||
|
err = iam.PutPolicy("myPolicy", "{broken")
|
||||||
|
assert.NoError(t, err)
|
||||||
|
|
||||||
|
errCode = iam.VerifyActionPermission(buildRequest(t, http.MethodGet), identity, Action(ACTION_READ), "test-bucket", "test-object")
|
||||||
|
assert.Equal(t, s3err.ErrAccessDenied, errCode)
|
||||||
|
})
|
||||||
|
|
||||||
t.Run("actions based path still works", func(t *testing.T) {
|
t.Run("actions based path still works", func(t *testing.T) {
|
||||||
iam := &IdentityAccessManagement{}
|
iam := &IdentityAccessManagement{}
|
||||||
identity := &Identity{
|
identity := &Identity{
|
||||||
|
|||||||
Reference in New Issue
Block a user