From d7314e932c3cfbc8f39c38071e8cc9e716864476 Mon Sep 17 00:00:00 2001 From: chrislu Date: Wed, 27 Aug 2025 00:05:21 -0700 Subject: [PATCH] address comments --- test/s3/iam/Makefile | 7 ++-- test/s3/iam/docker-compose.yml | 2 -- test/s3/iam/s3_iam_distributed_test.go | 6 ++-- test/s3/iam/s3_keycloak_integration_test.go | 37 ++++++++++----------- weed/iam/providers/provider.go | 29 ++++++++++++---- weed/s3api/s3_iam_middleware.go | 14 ++++---- 6 files changed, 55 insertions(+), 40 deletions(-) diff --git a/test/s3/iam/Makefile b/test/s3/iam/Makefile index 9d02473be..0acf7184a 100644 --- a/test/s3/iam/Makefile +++ b/test/s3/iam/Makefile @@ -221,8 +221,11 @@ docker-up: ## Start all services with Docker Compose (including Keycloak) @echo "๐Ÿณ Starting services with Docker Compose including Keycloak..." @docker compose up -d @echo "โณ Waiting for services to be healthy..." - @sleep 30 - @echo "โœ… Services should be ready" + @timeout 120 bash -c 'until curl -s http://localhost:8080/health/ready > /dev/null 2>&1; do sleep 2; done' || (echo "โŒ Keycloak failed to become ready" && exit 1) + @timeout 60 bash -c 'until curl -s http://localhost:8333 > /dev/null 2>&1; do sleep 2; done' || (echo "โŒ S3 API failed to become ready" && exit 1) + @timeout 60 bash -c 'until curl -s http://localhost:8888 > /dev/null 2>&1; do sleep 2; done' || (echo "โŒ Filer failed to become ready" && exit 1) + @timeout 60 bash -c 'until curl -s http://localhost:9333 > /dev/null 2>&1; do sleep 2; done' || (echo "โŒ Master failed to become ready" && exit 1) + @echo "โœ… All services are healthy and ready" docker-down: ## Stop all Docker Compose services @echo "๐Ÿณ Stopping Docker Compose services..." diff --git a/test/s3/iam/docker-compose.yml b/test/s3/iam/docker-compose.yml index e106a26fb..af9ad3be0 100644 --- a/test/s3/iam/docker-compose.yml +++ b/test/s3/iam/docker-compose.yml @@ -107,8 +107,6 @@ services: GLOG_v: "3" command: > sh -c " - echo 'Waiting for dependencies...' && - sleep 10 && echo 'Starting S3 API with IAM...' && weed -v=3 s3 -ip=weed-s3 -port=8333 -filer=weed-filer:8888 diff --git a/test/s3/iam/s3_iam_distributed_test.go b/test/s3/iam/s3_iam_distributed_test.go index b8114ae32..0e45dd09e 100644 --- a/test/s3/iam/s3_iam_distributed_test.go +++ b/test/s3/iam/s3_iam_distributed_test.go @@ -234,13 +234,13 @@ func TestS3IAMDistributedTests(t *testing.T) { maxTotalErrorsRelaxed := 4 // Allow max 4 total errors (50% rate) - acceptable for resource-constrained CI if len(errorList) > maxTotalErrorsRelaxed { - t.Errorf("โŒ Too many total errors: %d (%.1f%%) - exceeds relaxed threshold of %d (%.1f%%). System is unstable under concurrent load.", + 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%%) - within relaxed CI limits. Normal CI environment variability.", + t.Logf("โš ๏ธ Concurrent operations completed with %d errors (%.1f%%) - within relaxed CI limits. Normal CI environment variability.", len(errorList), errorRate*100) } else if len(errorList) > 0 { - t.Logf("โœ… Concurrent operations completed with %d errors (%.1f%%) - good performance for CI environment!", + t.Logf("โœ… Concurrent operations completed with %d errors (%.1f%%) - good performance for CI environment!", len(errorList), errorRate*100) } else { t.Logf("๐ŸŽ‰ All %d concurrent operations completed successfully - excellent CI performance!", totalOperations) diff --git a/test/s3/iam/s3_keycloak_integration_test.go b/test/s3/iam/s3_keycloak_integration_test.go index 9b8623307..0bb87161d 100644 --- a/test/s3/iam/s3_keycloak_integration_test.go +++ b/test/s3/iam/s3_keycloak_integration_test.go @@ -78,11 +78,8 @@ func TestKeycloakAuthentication(t *testing.T) { parts := strings.Split(token, ".") if len(parts) >= 2 { payload := parts[1] - // Add padding if needed - for len(payload)%4 != 0 { - payload += "=" - } - decoded, err := base64.StdEncoding.DecodeString(payload) + // JWTs use URL-safe base64 encoding without padding (RFC 4648 ยง5) + decoded, err := base64.RawURLEncoding.DecodeString(payload) if err == nil { var claims map[string]interface{} if json.Unmarshal(decoded, &claims) == nil { @@ -92,24 +89,24 @@ func TestKeycloakAuthentication(t *testing.T) { } } - // First test with direct HTTP request to verify OIDC authentication works - t.Logf("Testing with direct HTTP request...") - err = framework.TestKeycloakTokenDirectly(token) - require.NoError(t, err, "Direct HTTP test should succeed") + // First test with direct HTTP request to verify OIDC authentication works + t.Logf("Testing with direct HTTP request...") + err = framework.TestKeycloakTokenDirectly(token) + require.NoError(t, err, "Direct HTTP test should succeed") - // Create S3 client with Keycloak token - s3Client, err := framework.CreateS3ClientWithKeycloakToken(token) - require.NoError(t, err) + // Create S3 client with Keycloak token + s3Client, err := framework.CreateS3ClientWithKeycloakToken(token) + require.NoError(t, err) - // Test that read-only user can list buckets - t.Logf("Testing ListBuckets with AWS SDK...") - _, err = s3Client.ListBuckets(&s3.ListBucketsInput{}) - assert.NoError(t, err, "Read-only user should be able to list buckets") + // Test that read-only user can list buckets + t.Logf("Testing ListBuckets with AWS SDK...") + _, err = s3Client.ListBuckets(&s3.ListBucketsInput{}) + assert.NoError(t, err, "Read-only user should be able to list buckets") - // Test that read-only user cannot create buckets - t.Logf("Testing CreateBucket with AWS SDK...") - err = framework.CreateBucket(s3Client, testKeycloakBucket+"-readonly") - assert.Error(t, err, "Read-only user should not be able to create buckets") + // Test that read-only user cannot create buckets + t.Logf("Testing CreateBucket with AWS SDK...") + err = framework.CreateBucket(s3Client, testKeycloakBucket+"-readonly") + assert.Error(t, err, "Read-only user should not be able to create buckets") }) t.Run("invalid_user_authentication", func(t *testing.T) { diff --git a/weed/iam/providers/provider.go b/weed/iam/providers/provider.go index f014dc8db..6f1e3ca14 100644 --- a/weed/iam/providers/provider.go +++ b/weed/iam/providers/provider.go @@ -4,7 +4,8 @@ import ( "context" "fmt" "net/mail" - "path/filepath" + "regexp" + "strings" "time" "github.com/seaweedfs/seaweedfs/weed/glog" @@ -219,12 +220,28 @@ func (r *MappingRule) Matches(claims *TokenClaims) bool { } // matchValue checks if a value matches the rule value (with wildcard support) +// Uses AWS IAM-compliant case-insensitive wildcard matching for consistency with policy engine func (r *MappingRule) matchValue(value string) bool { - matched, err := filepath.Match(r.Value, value) + matched := awsWildcardMatch(r.Value, value) + glog.V(3).Infof("AWS IAM pattern match result: '%s' matches '%s' = %t", value, r.Value, matched) + return matched +} + +// awsWildcardMatch performs case-insensitive wildcard matching like AWS IAM +// This function ensures consistent matching behavior across the IAM system +func awsWildcardMatch(pattern, value string) bool { + // Convert pattern to regex with case-insensitive matching + // AWS uses * for any sequence and ? for any single character + regexPattern := strings.ReplaceAll(pattern, "*", ".*") + regexPattern = strings.ReplaceAll(regexPattern, "?", ".") + regexPattern = "^" + regexPattern + "$" + + // Compile with case-insensitive flag + regex, err := regexp.Compile("(?i)" + regexPattern) if err != nil { - glog.V(3).Infof("Error matching '%s' against pattern '%s': %v", value, r.Value, err) - return false + // Fallback to simple case-insensitive comparison if regex fails + return strings.EqualFold(pattern, value) } - glog.V(3).Infof("Pattern match result: '%s' matches '%s' = %t", value, r.Value, matched) - return matched + + return regex.MatchString(value) } diff --git a/weed/s3api/s3_iam_middleware.go b/weed/s3api/s3_iam_middleware.go index aa9debaa2..4cb45c520 100644 --- a/weed/s3api/s3_iam_middleware.go +++ b/weed/s3api/s3_iam_middleware.go @@ -285,7 +285,7 @@ func determineGranularS3Action(r *http.Request, fallbackAction Action, bucket st hasGranularIndicators := hasSpecificQueryParameters(query) // Only check for method-action mismatch when there are NO granular indicators - // This handles test scenarios where method and action don't align but no specific query params exist + // This provides fallback behavior for cases where HTTP method doesn't align with intended action if !hasGranularIndicators && isMethodActionMismatch(method, fallbackAction) { return mapLegacyActionToIAM(fallbackAction) } @@ -482,28 +482,28 @@ func hasSpecificQueryParameters(query url.Values) bool { return false } -// isMethodActionMismatch detects when HTTP method doesn't align with intended action -// This handles testing scenarios and edge cases where method and action don't naturally correspond +// isMethodActionMismatch detects when HTTP method doesn't align with the intended S3 action +// This provides a mechanism to use fallback action mapping when there's a semantic mismatch func isMethodActionMismatch(method string, fallbackAction Action) bool { switch fallbackAction { case s3_constants.ACTION_WRITE: // WRITE actions should typically use PUT, POST, or DELETE methods - // If method is GET/HEAD, it's likely a test or edge case - use fallback + // GET/HEAD methods indicate read-oriented operations return method == "GET" || method == "HEAD" case s3_constants.ACTION_READ: // READ actions should typically use GET or HEAD methods - // If method is PUT, POST, DELETE, it's likely a test or edge case - use fallback + // PUT, POST, DELETE methods indicate write-oriented operations return method == "PUT" || method == "POST" || method == "DELETE" case s3_constants.ACTION_LIST: // LIST actions should typically use GET method - // If method is PUT, POST, DELETE, it's likely a test or edge case - use fallback + // PUT, POST, DELETE methods indicate write-oriented operations return method == "PUT" || method == "POST" || method == "DELETE" case s3_constants.ACTION_DELETE_BUCKET: // DELETE_BUCKET should use DELETE method - // If method is GET, HEAD, PUT, POST, it's likely a test or edge case - use fallback + // Other methods indicate different operation types return method != "DELETE" default: