From 79a48256f533ff7d83c67733d9900bccf72b7c96 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Tue, 7 Apr 2026 13:21:30 -0700 Subject: [PATCH] fix(s3): populate s3:prefix from query param for ListObjects policy conditions (#8971) * fix(s3): populate s3:prefix from query param for ListObjects policy conditions (#8969) ListObjectsV2/V1 requests with prefix-restricted STS session policies were denied because: 1. s3:prefix was derived from objectKey, which the auth middleware set to the prefix value, but the resource ARN then included the prefix (e.g. arn:aws:s3:::bucket/prefix) instead of staying at bucket level (arn:aws:s3:::bucket) as AWS requires for ListBucket. 2. When objectKey was empty (no middleware propagation), s3:prefix was never populated from the query parameter at all. Now AuthorizeAction extracts the prefix query parameter directly, sets it as s3:prefix in the request context, and uses a bucket-level resource ARN when the objectKey matches the propagated prefix. * fix(s3): use AWS-style wildcard matching for StringLike policy conditions filepath.Match treats * as not matching /, which breaks IAM StringLike conditions on paths (e.g. arn:aws:s3:::bucket/* won't match nested keys). Replace with a case-sensitive variant of AwsWildcardMatch that correctly treats * as matching any character including /. * refactor(s3): replace regex wildcard matching with string-based matcher Use the existing wildcard.MatchesWildcard utility instead of compiling and caching regexes for IAM wildcard matching. Removes the regexCache, its mutex, and the sync import. * refactor(s3): inline and remove AwsWildcardMatch wrapper functions Replace all call sites with direct wildcard.MatchesWildcard calls. * fix(s3): scope s3:prefix condition key to list operations only The s3:prefix logic was running for all actions, so a GetObject on "foo/bar" would wrongly populate s3:prefix. Restrict it to action "List" and always reset resourceObjectKey to "" so the resource ARN stays at bucket level. Also set s3:prefix to "" when no prefix is provided, so policies with StringEquals {"s3:prefix": ""} evaluate correctly. --- weed/iam/policy/aws_iam_compliance_test.go | 4 +- weed/iam/policy/policy_engine.go | 62 ++------- weed/iam/providers/provider.go | 5 +- weed/s3api/s3_end_to_end_test.go | 147 +++++++++++++++++++++ weed/s3api/s3_iam_middleware.go | 26 ++-- 5 files changed, 179 insertions(+), 65 deletions(-) diff --git a/weed/iam/policy/aws_iam_compliance_test.go b/weed/iam/policy/aws_iam_compliance_test.go index cff61c9a4..eb28a7e03 100644 --- a/weed/iam/policy/aws_iam_compliance_test.go +++ b/weed/iam/policy/aws_iam_compliance_test.go @@ -1,8 +1,10 @@ package policy import ( + "strings" "testing" + "github.com/seaweedfs/seaweedfs/weed/util/wildcard" "github.com/stretchr/testify/assert" ) @@ -286,7 +288,7 @@ func TestAWSWildcardMatch(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := AwsWildcardMatch(tt.pattern, tt.value) + result := wildcard.MatchesWildcard(strings.ToLower(tt.pattern), strings.ToLower(tt.value)) assert.Equal(t, tt.expected, result, "AWS wildcard match should match expected") }) } diff --git a/weed/iam/policy/policy_engine.go b/weed/iam/policy/policy_engine.go index 2daa0a3e1..e51d34ed0 100644 --- a/weed/iam/policy/policy_engine.go +++ b/weed/iam/policy/policy_engine.go @@ -5,12 +5,12 @@ import ( "encoding/json" "fmt" "net" - "path/filepath" "regexp" "strconv" "strings" - "sync" "time" + + "github.com/seaweedfs/seaweedfs/weed/util/wildcard" ) // Effect represents the policy evaluation result @@ -21,10 +21,7 @@ const ( EffectDeny Effect = "Deny" ) -// Package-level regex cache for performance optimization var ( - regexCache = make(map[string]*regexp.Regexp) - regexCacheMu sync.RWMutex policyVariablePattern = regexp.MustCompile(`\$\{([^}]+)\}`) safePolicyVariables = map[string]bool{ // AWS standard identity variables @@ -1064,8 +1061,7 @@ func (e *PolicyEngine) EvaluateStringCondition(block map[string]interface{}, eva for _, expected := range expectedStrings { expandedExpected := expandPolicyVariables(expected, evalCtx) if useWildcard { - // Use filepath.Match for case-sensitive wildcard matching, as required by StringLike - if matched, _ := filepath.Match(expandedExpected, contextValue); matched { + if wildcard.MatchesWildcard(expandedExpected, contextValue) { contextValueMatchedSet = true break } @@ -1106,13 +1102,11 @@ func (e *PolicyEngine) EvaluateStringCondition(block map[string]interface{}, eva for _, expected := range expectedStrings { expandedExpected := expandPolicyVariables(expected, evalCtx) if useWildcard { - // Use filepath.Match for case-sensitive wildcard matching, as required by StringLike - if matched, _ := filepath.Match(expandedExpected, contextValue); matched { + if wildcard.MatchesWildcard(expandedExpected, contextValue) { contextValueMatchedSet = true break } } else { - // For StringEquals/StringNotEquals, also support policy variables but be case-sensitive if expandedExpected == contextValue { contextValueMatchedSet = true break @@ -1229,7 +1223,7 @@ func awsIAMMatch(pattern, value string, evalCtx *EvaluationContext) bool { // Step 4: Handle AWS-style wildcards (case-insensitive) if strings.Contains(expandedPattern, "*") || strings.Contains(expandedPattern, "?") { - return AwsWildcardMatch(expandedPattern, value) + return wildcard.MatchesWildcard(strings.ToLower(expandedPattern), strings.ToLower(value)) } return false @@ -1265,44 +1259,6 @@ func expandPolicyVariables(pattern string, evalCtx *EvaluationContext) string { return result } -// AwsWildcardMatch performs case-insensitive wildcard matching like AWS IAM -func AwsWildcardMatch(pattern, value string) bool { - // Create regex pattern key for caching - // First escape all regex metacharacters, then replace wildcards - regexPattern := regexp.QuoteMeta(pattern) - regexPattern = strings.ReplaceAll(regexPattern, "\\*", ".*") - regexPattern = strings.ReplaceAll(regexPattern, "\\?", ".") - regexPattern = "^" + regexPattern + "$" - regexKey := "(?i)" + regexPattern - - // Try to get compiled regex from cache - regexCacheMu.RLock() - regex, found := regexCache[regexKey] - regexCacheMu.RUnlock() - - if !found { - // Compile and cache the regex - compiledRegex, err := regexp.Compile(regexKey) - if err != nil { - // Fallback to simple case-insensitive comparison if regex fails - return strings.EqualFold(pattern, value) - } - - // Store in cache with write lock - regexCacheMu.Lock() - // Double-check in case another goroutine added it - if existingRegex, exists := regexCache[regexKey]; exists { - regex = existingRegex - } else { - regexCache[regexKey] = compiledRegex - regex = compiledRegex - } - regexCacheMu.Unlock() - } - - return regex.MatchString(value) -} - // evaluateStringConditionIgnoreCase evaluates string conditions with case insensitivity func (e *PolicyEngine) evaluateStringConditionIgnoreCase(block map[string]interface{}, evalCtx *EvaluationContext, shouldMatch bool, useWildcard bool, forAllValues bool) bool { for key, expectedValues := range block { @@ -1347,7 +1303,7 @@ func (e *PolicyEngine) evaluateStringConditionIgnoreCase(block map[string]interf case string: expandedPattern := expandPolicyVariables(v, evalCtx) if useWildcard { - if AwsWildcardMatch(expandedPattern, ctxStr) { + if wildcard.MatchesWildcard(strings.ToLower(expandedPattern), strings.ToLower(ctxStr)) { itemMatchedSet = true } } else { @@ -1369,7 +1325,7 @@ func (e *PolicyEngine) evaluateStringConditionIgnoreCase(block map[string]interf for _, valStr := range slice { expandedPattern := expandPolicyVariables(valStr, evalCtx) if useWildcard { - if AwsWildcardMatch(expandedPattern, ctxStr) { + if wildcard.MatchesWildcard(strings.ToLower(expandedPattern), strings.ToLower(ctxStr)) { itemMatchedSet = true break } @@ -1409,7 +1365,7 @@ func (e *PolicyEngine) evaluateStringConditionIgnoreCase(block map[string]interf case string: expandedPattern := expandPolicyVariables(v, evalCtx) if useWildcard { - if AwsWildcardMatch(expandedPattern, ctxStr) { + if wildcard.MatchesWildcard(strings.ToLower(expandedPattern), strings.ToLower(ctxStr)) { itemMatchedSet = true } } else { @@ -1431,7 +1387,7 @@ func (e *PolicyEngine) evaluateStringConditionIgnoreCase(block map[string]interf for _, valStr := range slice { expandedPattern := expandPolicyVariables(valStr, evalCtx) if useWildcard { - if AwsWildcardMatch(expandedPattern, ctxStr) { + if wildcard.MatchesWildcard(strings.ToLower(expandedPattern), strings.ToLower(ctxStr)) { itemMatchedSet = true break } diff --git a/weed/iam/providers/provider.go b/weed/iam/providers/provider.go index 3b7affc8e..98b8343fd 100644 --- a/weed/iam/providers/provider.go +++ b/weed/iam/providers/provider.go @@ -4,10 +4,11 @@ import ( "context" "fmt" "net/mail" + "strings" "time" "github.com/seaweedfs/seaweedfs/weed/glog" - "github.com/seaweedfs/seaweedfs/weed/iam/policy" + "github.com/seaweedfs/seaweedfs/weed/util/wildcard" ) // IdentityProvider defines the interface for external identity providers @@ -225,7 +226,7 @@ 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 := policy.AwsWildcardMatch(r.Value, value) + matched := wildcard.MatchesWildcard(strings.ToLower(r.Value), strings.ToLower(value)) glog.V(3).Infof("AWS IAM pattern match result: '%s' matches '%s' = %t", value, r.Value, matched) return matched } diff --git a/weed/s3api/s3_end_to_end_test.go b/weed/s3api/s3_end_to_end_test.go index 3fa20194d..15f1c33d5 100644 --- a/weed/s3api/s3_end_to_end_test.go +++ b/weed/s3api/s3_end_to_end_test.go @@ -214,6 +214,153 @@ func TestS3MultipartUploadWithJWT(t *testing.T) { } } +// TestS3ListObjectsV2PrefixCondition tests that ListObjectsV2 requests with a prefix +// query parameter correctly populate s3:prefix in the policy evaluation context and +// use bucket-level resource ARNs, so that policies with s3:prefix conditions work. +// This reproduces the bug reported in https://github.com/seaweedfs/seaweedfs/issues/8969 +func TestS3ListObjectsV2PrefixCondition(t *testing.T) { + // Set up IAM system + iamManager := integration.NewIAMManager() + config := &integration.IAMConfig{ + STS: &sts.STSConfig{ + TokenDuration: sts.FlexibleDuration{Duration: time.Hour}, + MaxSessionLength: sts.FlexibleDuration{Duration: time.Hour * 12}, + Issuer: "test-sts", + SigningKey: []byte("test-signing-key-32-characters-long"), + }, + Policy: &policy.PolicyEngineConfig{ + DefaultEffect: "Deny", + StoreType: "memory", + }, + Roles: &integration.RoleStoreConfig{ + StoreType: "memory", + }, + } + + err := iamManager.Initialize(config, func() string { return "localhost:8888" }) + require.NoError(t, err) + + setupTestProviders(t, iamManager) + + s3IAMIntegration := NewS3IAMIntegration(iamManager, "localhost:8888") + require.NotNil(t, s3IAMIntegration) + + ctx := context.Background() + + // Create a role with a policy that allows ListBucket only with a specific s3:prefix condition. + // This is the pattern used by Lakekeeper-vended STS credentials (issue #8969). + prefixPolicy := &policy.PolicyDocument{ + Version: "2012-10-17", + Statement: []policy.Statement{ + { + Sid: "AllowListUnderWarehouse", + Effect: "Allow", + Action: []string{"s3:ListBucket"}, + Resource: []string{ + "arn:aws:s3:::examples", + }, + Condition: map[string]map[string]interface{}{ + "StringLike": { + "s3:prefix": []string{"warehouse/*"}, + }, + }, + }, + { + Sid: "AllowSTSSessionValidation", + Effect: "Allow", + Action: []string{"sts:ValidateSession"}, + Resource: []string{"*"}, + }, + }, + } + + iamManager.CreatePolicy(ctx, "", "PrefixRestrictedPolicy", prefixPolicy) + iamManager.CreateRole(ctx, "", "PrefixRestrictedRole", &integration.RoleDefinition{ + RoleName: "PrefixRestrictedRole", + TrustPolicy: &policy.PolicyDocument{ + Version: "2012-10-17", + Statement: []policy.Statement{ + { + Effect: "Allow", + Principal: map[string]interface{}{"Federated": "test-oidc"}, + Action: []string{"sts:AssumeRoleWithWebIdentity"}, + }, + }, + }, + AttachedPolicies: []string{"PrefixRestrictedPolicy"}, + }) + + // Assume role to get a session token + validJWTToken := createTestJWTEndToEnd(t, "https://test-issuer.com", "test-user-123", "test-signing-key") + response, err := iamManager.AssumeRoleWithWebIdentity(ctx, &sts.AssumeRoleWithWebIdentityRequest{ + RoleArn: "arn:aws:iam::role/PrefixRestrictedRole", + WebIdentityToken: validJWTToken, + RoleSessionName: "prefix-test-session", + }) + require.NoError(t, err) + + sessionToken := response.Credentials.SessionToken + require.NotEmpty(t, sessionToken) + + // Authenticate to get IAM identity + authReq := httptest.NewRequest("GET", "/examples", http.NoBody) + authReq.Header.Set("Authorization", "Bearer "+sessionToken) + identity, errCode := s3IAMIntegration.AuthenticateJWT(ctx, authReq) + require.Equal(t, s3err.ErrNone, errCode, "Authentication should succeed") + + tests := []struct { + name string + url string + bucket string + objKey string + expected s3err.ErrorCode + }{ + { + name: "ListObjectsV2 with matching prefix query param and empty objectKey", + url: "/examples?list-type=2&prefix=warehouse/data", + bucket: "examples", + objKey: "", + expected: s3err.ErrNone, + }, + { + name: "ListObjectsV2 with matching prefix propagated as objectKey", + url: "/examples?list-type=2&prefix=warehouse/data", + bucket: "examples", + objKey: "warehouse/data", + expected: s3err.ErrNone, + }, + { + name: "ListObjectsV1 with matching prefix query param", + url: "/examples?prefix=warehouse/files", + bucket: "examples", + objKey: "", + expected: s3err.ErrNone, + }, + { + name: "ListObjectsV2 with non-matching prefix should be denied", + url: "/examples?list-type=2&prefix=other/path", + bucket: "examples", + objKey: "", + expected: s3err.ErrAccessDenied, + }, + { + name: "ListObjectsV2 with no prefix should be denied", + url: "/examples?list-type=2", + bucket: "examples", + objKey: "", + expected: s3err.ErrAccessDenied, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req := httptest.NewRequest("GET", tt.url, http.NoBody) + result := s3IAMIntegration.AuthorizeAction(ctx, identity, Action("List"), tt.bucket, tt.objKey, req) + assert.Equal(t, tt.expected, result, "unexpected authorization result for %s", tt.name) + }) + } +} + // TestS3CORSWithJWT tests CORS preflight requests with IAM func TestS3CORSWithJWT(t *testing.T) { s3Server, iamManager := setupCompleteS3IAMSystem(t) diff --git a/weed/s3api/s3_iam_middleware.go b/weed/s3api/s3_iam_middleware.go index af454dee8..90caa3b4c 100644 --- a/weed/s3api/s3_iam_middleware.go +++ b/weed/s3api/s3_iam_middleware.go @@ -250,20 +250,28 @@ func (s3iam *S3IAMIntegration) AuthorizeAction(ctx context.Context, identity *IA return s3err.ErrAccessDenied } - // Build resource ARN for the S3 operation - resourceArn := buildS3ResourceArn(bucket, objectKey) - // Extract request context for policy conditions requestContext := extractRequestContext(r) - // Add s3:prefix to request context based on object key - // This ensures that policy conditions referencing s3:prefix (like StringLike) - // work correctly for both ListObjects (where objectKey is the prefix) and - // object operations (where we treat the object key as the prefix for matching) - if objectKey != "" && objectKey != "/" { - requestContext["s3:prefix"] = objectKey + // For list operations, populate the s3:prefix condition key and ensure the + // resource ARN stays at bucket level (matching AWS ListBucket semantics). + // See https://github.com/seaweedfs/seaweedfs/issues/8969 + resourceObjectKey := objectKey + if action == "List" { + listPrefix := r.URL.Query().Get("prefix") + if listPrefix != "" { + requestContext["s3:prefix"] = listPrefix + } else if objectKey != "" && objectKey != "/" { + requestContext["s3:prefix"] = objectKey + } else { + requestContext["s3:prefix"] = "" + } + resourceObjectKey = "" } + // Build resource ARN for the S3 operation + resourceArn := buildS3ResourceArn(bucket, resourceObjectKey) + // Add identity claims to request context for policy variables // Only add claim keys if they don't already exist (to avoid overwriting request-derived context) if identity.Claims != nil {