From 9cbd73aba05fe9c67ad009893442227f2fd3ecd1 Mon Sep 17 00:00:00 2001 From: chrislu Date: Sun, 24 Aug 2025 21:21:55 -0700 Subject: [PATCH] fix: implement proper policy condition evaluation and trust policy validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixed the critical issues identified in GitHub PR review that were causing JWT authentication failures in S3 IAM integration tests. ### Problem Identified: - evaluateStringCondition function was a stub that always returned shouldMatch - Trust policy validation was doing basic checks instead of proper evaluation - String conditions (StringEquals, StringNotEquals, StringLike) were ignored - JWT authentication failing with errCode=1 (AccessDenied) ### Solution Implemented: **1. Fixed evaluateStringCondition in policy engine:** - Implemented proper string condition evaluation with context matching - Added support for exact matching (StringEquals/StringNotEquals) - Added wildcard support for StringLike conditions using filepath.Match - Proper type conversion for condition values and context values **2. Implemented comprehensive trust policy validation:** - Added parseJWTTokenForTrustPolicy to extract claims from web identity tokens - Created evaluateTrustPolicy method with proper Principal matching - Added support for Federated principals (OIDC/SAML) - Implemented trust policy condition evaluation - Added proper context mapping (seaweed:FederatedProvider, etc.) **3. Enhanced IAM manager with trust policy evaluation:** - validateTrustPolicyForWebIdentity now uses proper policy evaluation - Extracts JWT claims and maps them to evaluation context - Supports StringEquals, StringNotEquals, StringLike conditions - Proper Principal matching for Federated identity providers ### Technical Details: - Added filepath import for wildcard matching - Added base64, json imports for JWT parsing - Trust policies now check Principal.Federated against token idp claim - Context values properly mapped: idp → seaweed:FederatedProvider - Condition evaluation follows AWS IAM policy semantics ### Addresses GitHub PR Review: This directly fixes the issue mentioned in the PR review about evaluateStringCondition being a stub that doesn't implement actual logic for StringEquals, StringNotEquals, and StringLike conditions. The trust policy validation now properly enforces policy conditions, which should resolve the JWT authentication failures. --- test/s3/iam/test_jwt.go | 41 ----- weed/iam/integration/iam_manager.go | 244 +++++++++++++++++++++++++--- weed/iam/policy/policy_engine.go | 88 +++++++++- 3 files changed, 309 insertions(+), 64 deletions(-) delete mode 100644 test/s3/iam/test_jwt.go diff --git a/test/s3/iam/test_jwt.go b/test/s3/iam/test_jwt.go deleted file mode 100644 index 7e796eb27..000000000 --- a/test/s3/iam/test_jwt.go +++ /dev/null @@ -1,41 +0,0 @@ -package main - -import ( - "fmt" - "time" - "encoding/base64" - "github.com/golang-jwt/jwt/v5" -) - -func main() { - now := time.Now() - signingKeyB64 := "dGVzdC1zaWduaW5nLWtleS0zMi1jaGFyYWN0ZXJzLWxvbmc=" - signingKey, _ := base64.StdEncoding.DecodeString(signingKeyB64) - - sessionId := fmt.Sprintf("test-session-admin-user-TestAdminRole-%d", now.Unix()) - roleArn := "arn:seaweed:iam::role/TestAdminRole" - sessionName := "test-session-admin-user" - principalArn := fmt.Sprintf("arn:seaweed:sts::assumed-role/TestAdminRole/%s", sessionName) - - sessionClaims := jwt.MapClaims{ - "iss": "seaweedfs-sts", - "sub": sessionId, - "iat": now.Unix(), - "exp": now.Add(time.Hour).Unix(), - "nbf": now.Unix(), - "typ": "session", - "role": roleArn, - "snam": sessionName, - "principal": principalArn, - "assumed": principalArn, - "assumed_at": now.Format(time.RFC3339Nano), - "ext_uid": "admin-user", - "idp": "test-oidc", - "max_dur": int64(time.Hour.Seconds()), - "sid": sessionId, - } - - token := jwt.NewWithClaims(jwt.SigningMethodHS256, sessionClaims) - tokenString, _ := token.SignedString(signingKey) - fmt.Println(tokenString) -} diff --git a/weed/iam/integration/iam_manager.go b/weed/iam/integration/iam_manager.go index 095505dcf..1cade2cb9 100644 --- a/weed/iam/integration/iam_manager.go +++ b/weed/iam/integration/iam_manager.go @@ -2,7 +2,11 @@ package integration import ( "context" + "encoding/base64" + "encoding/json" "fmt" + "path/filepath" + "strings" "github.com/seaweedfs/seaweedfs/weed/iam/policy" "github.com/seaweedfs/seaweedfs/weed/iam/providers" @@ -294,29 +298,44 @@ func (m *IAMManager) validateTrustPolicyForWebIdentity(ctx context.Context, role return fmt.Errorf("role has no trust policy") } - // For simplified implementation, we'll do basic validation - // In a full implementation, this would: - // 1. Parse the web identity token - // 2. Check issuer against trust policy - // 3. Validate conditions in trust policy + // Parse the web identity token to extract claims for context + tokenClaims, err := parseJWTTokenForTrustPolicy(webIdentityToken) + if err != nil { + return fmt.Errorf("failed to parse web identity token: %w", err) + } - // Check if trust policy allows web identity assumption - for _, statement := range roleDef.TrustPolicy.Statement { - if statement.Effect == "Allow" { - for _, action := range statement.Action { - if action == "sts:AssumeRoleWithWebIdentity" { - // For testing, just verify there's a Federated principal - if principal, ok := statement.Principal.(map[string]interface{}); ok { - if _, ok := principal["Federated"]; ok { - return nil // Allow - } - } - } - } - } + // Create evaluation context for trust policy validation + requestContext := make(map[string]interface{}) + + // Add standard context values that trust policies might check + if idp, ok := tokenClaims["idp"].(string); ok { + requestContext["seaweed:TokenIssuer"] = idp + requestContext["seaweed:FederatedProvider"] = idp + } + if iss, ok := tokenClaims["iss"].(string); ok { + requestContext["seaweed:Issuer"] = iss + } + if sub, ok := tokenClaims["sub"].(string); ok { + requestContext["seaweed:Subject"] = sub + } + if extUid, ok := tokenClaims["ext_uid"].(string); ok { + requestContext["seaweed:ExternalUserId"] = extUid + } + + // Create evaluation context for trust policy + evalCtx := &policy.EvaluationContext{ + Principal: "web-identity-user", // Placeholder principal for trust policy evaluation + Action: "sts:AssumeRoleWithWebIdentity", + Resource: roleDef.RoleArn, + RequestContext: requestContext, } - return fmt.Errorf("trust policy does not allow web identity assumption") + // Evaluate the trust policy directly + if !m.evaluateTrustPolicy(roleDef.TrustPolicy, evalCtx) { + return fmt.Errorf("trust policy denies web identity assumption") + } + + return nil } // validateTrustPolicyForCredentials validates trust policy for credential assumption @@ -388,3 +407,188 @@ func (m *IAMManager) ExpireSessionForTesting(ctx context.Context, sessionToken s return m.stsService.ExpireSessionForTesting(ctx, sessionToken) } + +// parseJWTTokenForTrustPolicy parses a JWT token to extract claims for trust policy evaluation +func parseJWTTokenForTrustPolicy(tokenString string) (map[string]interface{}, error) { + // Simple JWT parsing without verification (for trust policy context only) + // In production, this should use proper JWT parsing with signature verification + parts := strings.Split(tokenString, ".") + if len(parts) != 3 { + return nil, fmt.Errorf("invalid JWT format") + } + + // Decode the payload (second part) + payload := parts[1] + // Add padding if needed + for len(payload)%4 != 0 { + payload += "=" + } + + decoded, err := base64.URLEncoding.DecodeString(payload) + if err != nil { + return nil, fmt.Errorf("failed to decode JWT payload: %w", err) + } + + var claims map[string]interface{} + if err := json.Unmarshal(decoded, &claims); err != nil { + return nil, fmt.Errorf("failed to unmarshal JWT claims: %w", err) + } + + return claims, nil +} + +// evaluateTrustPolicy evaluates a trust policy against the evaluation context +func (m *IAMManager) evaluateTrustPolicy(trustPolicy *policy.PolicyDocument, evalCtx *policy.EvaluationContext) bool { + if trustPolicy == nil { + return false + } + + // Trust policies work differently from regular policies: + // - They check the Principal field to see who can assume the role + // - They check Action to see what actions are allowed + // - They may have Conditions that must be satisfied + + for _, statement := range trustPolicy.Statement { + if statement.Effect == "Allow" { + // Check if the action matches + actionMatches := false + for _, action := range statement.Action { + if action == evalCtx.Action || action == "*" { + actionMatches = true + break + } + } + if !actionMatches { + continue + } + + // Check if the principal matches + principalMatches := false + if principal, ok := statement.Principal.(map[string]interface{}); ok { + // Check for Federated principal (OIDC/SAML) + if federated, ok := principal["Federated"].(string); ok { + // For web identity, check if the token issuer matches the federated provider + if tokenIssuer, exists := evalCtx.RequestContext["seaweed:FederatedProvider"]; exists { + if issuerStr, ok := tokenIssuer.(string); ok && issuerStr == federated { + principalMatches = true + } + } + } + // Could add other principal types here (AWS, Service, etc.) + } else if principalStr, ok := statement.Principal.(string); ok { + // Handle string principal + if principalStr == "*" { + principalMatches = true + } + } + + if !principalMatches { + continue + } + + // Check conditions if present + if len(statement.Condition) > 0 { + conditionsMatch := m.evaluateTrustPolicyConditions(statement.Condition, evalCtx) + if !conditionsMatch { + continue + } + } + + // All checks passed for this Allow statement + return true + } + } + + return false +} + +// evaluateTrustPolicyConditions evaluates conditions in a trust policy statement +func (m *IAMManager) evaluateTrustPolicyConditions(conditions map[string]map[string]interface{}, evalCtx *policy.EvaluationContext) bool { + for conditionType, conditionBlock := range conditions { + switch conditionType { + case "StringEquals": + if !m.evaluateStringConditionForTrust(conditionBlock, evalCtx, true, false) { + return false + } + case "StringNotEquals": + if !m.evaluateStringConditionForTrust(conditionBlock, evalCtx, false, false) { + return false + } + case "StringLike": + if !m.evaluateStringConditionForTrust(conditionBlock, evalCtx, true, true) { + return false + } + // Add other condition types as needed + default: + // Unknown condition type - fail safe + return false + } + } + return true +} + +// evaluateStringConditionForTrust evaluates string conditions for trust policies +func (m *IAMManager) evaluateStringConditionForTrust(block map[string]interface{}, evalCtx *policy.EvaluationContext, shouldMatch bool, useWildcard bool) bool { + for conditionKey, conditionValue := range block { + contextValues, exists := evalCtx.RequestContext[conditionKey] + if !exists { + if shouldMatch { + return false + } + continue + } + + // Convert context value to string slice + var contextStrings []string + switch v := contextValues.(type) { + case string: + contextStrings = []string{v} + case []string: + contextStrings = v + default: + contextStrings = []string{fmt.Sprintf("%v", v)} + } + + // Convert condition value to string slice + var expectedStrings []string + switch v := conditionValue.(type) { + case string: + expectedStrings = []string{v} + case []string: + expectedStrings = v + default: + expectedStrings = []string{fmt.Sprintf("%v", v)} + } + + // Evaluate the condition + conditionMet := false + for _, expected := range expectedStrings { + for _, contextValue := range contextStrings { + if useWildcard { + matched, err := filepath.Match(expected, contextValue) + if err == nil && matched { + conditionMet = true + break + } + } else { + if expected == contextValue { + conditionMet = true + break + } + } + } + if conditionMet { + break + } + } + + if shouldMatch && !conditionMet { + return false + } + if !shouldMatch && conditionMet { + return false + } + } + + return true +} diff --git a/weed/iam/policy/policy_engine.go b/weed/iam/policy/policy_engine.go index 19dd7cd32..a2ea1212f 100644 --- a/weed/iam/policy/policy_engine.go +++ b/weed/iam/policy/policy_engine.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "net" + "path/filepath" "strings" ) @@ -419,9 +420,90 @@ func (e *PolicyEngine) evaluateIPCondition(block map[string]interface{}, evalCtx // evaluateStringCondition evaluates string-based conditions func (e *PolicyEngine) evaluateStringCondition(block map[string]interface{}, evalCtx *EvaluationContext, shouldMatch bool, useWildcard bool) bool { - // For this simplified implementation, we'll just return true - // In a full implementation, this would evaluate string conditions against request context - return shouldMatch + // Iterate through all condition keys in the block + for conditionKey, conditionValue := range block { + // Get the context values for this condition key + contextValues, exists := evalCtx.RequestContext[conditionKey] + if !exists { + // If the context key doesn't exist, condition fails for positive match + if shouldMatch { + return false + } + continue + } + + // Convert context value to string slice + var contextStrings []string + switch v := contextValues.(type) { + case string: + contextStrings = []string{v} + case []string: + contextStrings = v + case []interface{}: + for _, item := range v { + if str, ok := item.(string); ok { + contextStrings = append(contextStrings, str) + } + } + default: + // Convert to string as fallback + contextStrings = []string{fmt.Sprintf("%v", v)} + } + + // Convert condition value to string slice + var expectedStrings []string + switch v := conditionValue.(type) { + case string: + expectedStrings = []string{v} + case []string: + expectedStrings = v + case []interface{}: + for _, item := range v { + if str, ok := item.(string); ok { + expectedStrings = append(expectedStrings, str) + } else { + expectedStrings = append(expectedStrings, fmt.Sprintf("%v", item)) + } + } + default: + expectedStrings = []string{fmt.Sprintf("%v", v)} + } + + // Evaluate the condition + conditionMet := false + for _, expected := range expectedStrings { + for _, contextValue := range contextStrings { + if useWildcard { + // Use wildcard matching for StringLike conditions + matched, err := filepath.Match(expected, contextValue) + if err == nil && matched { + conditionMet = true + break + } + } else { + // Exact string matching for StringEquals/StringNotEquals + if expected == contextValue { + conditionMet = true + break + } + } + } + if conditionMet { + break + } + } + + // For shouldMatch=true (StringEquals, StringLike): condition must be met + // For shouldMatch=false (StringNotEquals): condition must NOT be met + if shouldMatch && !conditionMet { + return false + } + if !shouldMatch && conditionMet { + return false + } + } + + return true } // ValidatePolicyDocument validates a policy document structure