From e6c50f9ba9dded56dfb8d4e356f08eae31593080 Mon Sep 17 00:00:00 2001 From: chrislu Date: Tue, 26 Aug 2025 22:32:42 -0700 Subject: [PATCH] address comments --- test/s3/iam/s3_iam_distributed_test.go | 36 ++-- weed/iam/policy/policy_engine.go | 12 +- .../policy/policy_variable_matching_test.go | 191 ++++++++++++++++++ 3 files changed, 218 insertions(+), 21 deletions(-) create mode 100644 weed/iam/policy/policy_variable_matching_test.go diff --git a/test/s3/iam/s3_iam_distributed_test.go b/test/s3/iam/s3_iam_distributed_test.go index 9634640f3..0720f8fb3 100644 --- a/test/s3/iam/s3_iam_distributed_test.go +++ b/test/s3/iam/s3_iam_distributed_test.go @@ -104,9 +104,9 @@ func TestS3IAMDistributedTests(t *testing.T) { t.Run("distributed_concurrent_operations", func(t *testing.T) { // Test concurrent operations across distributed instances - // Reduced concurrency (3x2=6 total ops) for CI stability - const numGoroutines = 3 - const numOperationsPerGoroutine = 2 + // Increased concurrency to effectively detect race conditions and system stability + const numGoroutines = 10 // More goroutines to stress test concurrency + const numOperationsPerGoroutine = 5 // More operations per goroutine for better coverage var wg sync.WaitGroup errors := make(chan error, numGoroutines*numOperationsPerGoroutine) @@ -215,25 +215,31 @@ func TestS3IAMDistributedTests(t *testing.T) { } } - // STRICT CONCURRENCY TESTING: Use appropriate thresholds for the operation count - // For totalOperations=6, error thresholds need to be adjusted to avoid unreachable conditions - + // RIGOROUS CONCURRENCY TESTING: Use strict thresholds for effective race condition detection + // For totalOperations=50, we can set much more realistic and strict error thresholds + // Serious errors (race conditions, deadlocks) should be zero for reliable testing - maxSeriousErrors := 0 // Allow 0 serious errors max - any serious error indicates system issues + maxSeriousErrors := 0 // Zero tolerance for serious errors - any indicates system issues if len(seriousErrors) > maxSeriousErrors { t.Errorf("❌ %d serious error(s) detected, indicating potential concurrency bugs. First error: %v", len(seriousErrors), seriousErrors[0]) } - - // For total errors, allow more flexibility with small operation counts - maxTotalErrors := 2 // Allow 2 total errors max (33.3%) for small test size - if len(errorList) > maxTotalErrors { - t.Errorf("❌ Too many total errors: %d (%.1f%%) - max allowed: %d. This suggests system instability under concurrent load.", - len(errorList), errorRate*100, maxTotalErrors) + + // For total errors, use strict thresholds that can actually detect system instability + maxTotalErrorsStrict := 2 // Allow max 2 total errors (4% rate) - very strict + maxTotalErrorsRelaxed := 5 // Allow max 5 total errors (10% rate) - fallback for CI environments + + if len(errorList) > maxTotalErrorsRelaxed { + t.Errorf("❌ Too many total errors: %d (%.1f%%) - exceeds relaxed threshold of %d (%.1f%%). System is unstable under concurrent load.", + len(errorList), errorRate*100, maxTotalErrorsRelaxed, float64(maxTotalErrorsRelaxed)/float64(totalOperations)*100) + } else if len(errorList) > maxTotalErrorsStrict { + t.Logf("⚠️ Concurrent operations completed with %d errors (%.1f%%) - exceeds strict threshold but within relaxed limits. Consider investigating.", + len(errorList), errorRate*100) } else if len(errorList) > 0 { - t.Logf("⚠️ Concurrent operations completed with %d errors (%.1f%%) - within acceptable range for testing", len(errorList), errorRate*100) + t.Logf("✅ Concurrent operations completed with %d errors (%.1f%%) - within strict thresholds, excellent stability!", + len(errorList), errorRate*100) } else { - t.Logf("✅ All concurrent operations completed successfully - excellent concurrency handling!") + t.Logf("🎉 All %d concurrent operations completed successfully - perfect concurrency handling!", totalOperations) } }) } diff --git a/weed/iam/policy/policy_engine.go b/weed/iam/policy/policy_engine.go index 703eafe7e..d063c5f2c 100644 --- a/weed/iam/policy/policy_engine.go +++ b/weed/iam/policy/policy_engine.go @@ -301,12 +301,12 @@ func (e *PolicyEngine) Evaluate(ctx context.Context, filerAddress string, evalCt // statementMatches checks if a statement matches the evaluation context func (e *PolicyEngine) statementMatches(statement *Statement, evalCtx *EvaluationContext) bool { // Check action match - if !e.matchesActions(statement.Action, evalCtx.Action) { + if !e.matchesActions(statement.Action, evalCtx.Action, evalCtx) { return false } // Check resource match - if !e.matchesResources(statement.Resource, evalCtx.Resource) { + if !e.matchesResources(statement.Resource, evalCtx.Resource, evalCtx) { return false } @@ -319,9 +319,9 @@ func (e *PolicyEngine) statementMatches(statement *Statement, evalCtx *Evaluatio } // matchesActions checks if any action in the list matches the requested action -func (e *PolicyEngine) matchesActions(actions []string, requestedAction string) bool { +func (e *PolicyEngine) matchesActions(actions []string, requestedAction string, evalCtx *EvaluationContext) bool { for _, action := range actions { - if matchAction(action, requestedAction) { + if awsIAMMatch(action, requestedAction, evalCtx) { return true } } @@ -329,9 +329,9 @@ func (e *PolicyEngine) matchesActions(actions []string, requestedAction string) } // matchesResources checks if any resource in the list matches the requested resource -func (e *PolicyEngine) matchesResources(resources []string, requestedResource string) bool { +func (e *PolicyEngine) matchesResources(resources []string, requestedResource string, evalCtx *EvaluationContext) bool { for _, resource := range resources { - if matchResource(resource, requestedResource) { + if awsIAMMatch(resource, requestedResource, evalCtx) { return true } } diff --git a/weed/iam/policy/policy_variable_matching_test.go b/weed/iam/policy/policy_variable_matching_test.go new file mode 100644 index 000000000..ad2305603 --- /dev/null +++ b/weed/iam/policy/policy_variable_matching_test.go @@ -0,0 +1,191 @@ +package policy + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestPolicyVariableMatchingInActionsAndResources tests that Actions and Resources +// now support policy variables like ${aws:username} just like string conditions do +func TestPolicyVariableMatchingInActionsAndResources(t *testing.T) { + engine := NewPolicyEngine() + config := &PolicyEngineConfig{ + DefaultEffect: "Deny", + StoreType: "memory", + } + + err := engine.Initialize(config) + require.NoError(t, err) + + ctx := context.Background() + filerAddress := "" + + // Create a policy that uses policy variables in Action and Resource fields + policyDoc := &PolicyDocument{ + Version: "2012-10-17", + Statement: []Statement{ + { + Sid: "AllowUserSpecificActions", + Effect: "Allow", + Action: []string{ + "s3:Get*", // Regular wildcard + "s3:${aws:principaltype}*", // Policy variable in action + }, + Resource: []string{ + "arn:aws:s3:::user-${aws:username}/*", // Policy variable in resource + "arn:aws:s3:::shared/${saml:username}/*", // Different policy variable + }, + }, + }, + } + + err = engine.AddPolicy(filerAddress, "user-specific-policy", policyDoc) + require.NoError(t, err) + + tests := []struct { + name string + principal string + action string + resource string + requestContext map[string]interface{} + expectedEffect Effect + description string + }{ + { + name: "policy_variable_in_action_matches", + principal: "test-user", + action: "s3:AssumedRole", // Should match s3:${aws:principaltype}* when principaltype=AssumedRole + resource: "arn:aws:s3:::user-testuser/file.txt", + requestContext: map[string]interface{}{ + "aws:username": "testuser", + "aws:principaltype": "AssumedRole", + }, + expectedEffect: EffectAllow, + description: "Action with policy variable should match when variable is expanded", + }, + { + name: "policy_variable_in_resource_matches", + principal: "alice", + action: "s3:GetObject", + resource: "arn:aws:s3:::user-alice/document.pdf", // Should match user-${aws:username}/* + requestContext: map[string]interface{}{ + "aws:username": "alice", + }, + expectedEffect: EffectAllow, + description: "Resource with policy variable should match when variable is expanded", + }, + { + name: "saml_username_variable_in_resource", + principal: "bob", + action: "s3:GetObject", + resource: "arn:aws:s3:::shared/bob/data.json", // Should match shared/${saml:username}/* + requestContext: map[string]interface{}{ + "saml:username": "bob", + }, + expectedEffect: EffectAllow, + description: "SAML username variable should be expanded in resource patterns", + }, + { + name: "policy_variable_no_match_wrong_user", + principal: "charlie", + action: "s3:GetObject", + resource: "arn:aws:s3:::user-alice/file.txt", // charlie trying to access alice's files + requestContext: map[string]interface{}{ + "aws:username": "charlie", + }, + expectedEffect: EffectDeny, + description: "Policy variable should prevent access when username doesn't match", + }, + { + name: "missing_policy_variable_context", + principal: "dave", + action: "s3:GetObject", + resource: "arn:aws:s3:::user-dave/file.txt", + requestContext: map[string]interface{}{ + // Missing aws:username context + }, + expectedEffect: EffectDeny, + description: "Missing policy variable context should result in no match", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + evalCtx := &EvaluationContext{ + Principal: tt.principal, + Action: tt.action, + Resource: tt.resource, + RequestContext: tt.requestContext, + } + + result, err := engine.Evaluate(ctx, filerAddress, evalCtx, []string{"user-specific-policy"}) + require.NoError(t, err, "Policy evaluation should not error") + + assert.Equal(t, tt.expectedEffect, result.Effect, + "Test %s: %s. Expected %s but got %s", + tt.name, tt.description, tt.expectedEffect, result.Effect) + }) + } +} + +// TestActionResourceConsistencyWithStringConditions verifies that Actions, Resources, +// and string conditions all use the same AWS IAM-compliant matching logic +func TestActionResourceConsistencyWithStringConditions(t *testing.T) { + engine := NewPolicyEngine() + config := &PolicyEngineConfig{ + DefaultEffect: "Deny", + StoreType: "memory", + } + + err := engine.Initialize(config) + require.NoError(t, err) + + ctx := context.Background() + filerAddress := "" + + // Policy that uses case-insensitive matching in all three areas + policyDoc := &PolicyDocument{ + Version: "2012-10-17", + Statement: []Statement{ + { + Sid: "CaseInsensitiveMatching", + Effect: "Allow", + Action: []string{"S3:GET*"}, // Uppercase action pattern + Resource: []string{"arn:aws:s3:::TEST-BUCKET/*"}, // Uppercase resource pattern + Condition: map[string]map[string]interface{}{ + "StringLike": { + "s3:RequestedRegion": "US-*", // Uppercase condition pattern + }, + }, + }, + }, + } + + err = engine.AddPolicy(filerAddress, "case-insensitive-policy", policyDoc) + require.NoError(t, err) + + evalCtx := &EvaluationContext{ + Principal: "test-user", + Action: "s3:getobject", // lowercase action + Resource: "arn:aws:s3:::test-bucket/file.txt", // lowercase resource + RequestContext: map[string]interface{}{ + "s3:RequestedRegion": "us-east-1", // lowercase condition value + }, + } + + result, err := engine.Evaluate(ctx, filerAddress, evalCtx, []string{"case-insensitive-policy"}) + require.NoError(t, err) + + // All should match due to case-insensitive AWS IAM-compliant matching + assert.Equal(t, EffectAllow, result.Effect, + "Actions, Resources, and Conditions should all use case-insensitive AWS IAM matching") + + // Verify that matching statements were found + assert.Len(t, result.MatchingStatements, 1, + "Should have exactly one matching statement") + assert.Equal(t, "Allow", string(result.MatchingStatements[0].Effect), + "Matching statement should have Allow effect") +}