fix: implement proper policy condition evaluation and trust policy validation

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.
2026-08-19 13:46:58 +00:00 · 2025-08-24 21:21:55 -07:00
parent 0cbb29161d
commit 9cbd73aba0
3 changed files with 310 additions and 65 deletions
@@ -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)
-}
@@ -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
-
-	// 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
-						}
-					}
-				}
-			}
-		}
+	// 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)
 	}

-	return fmt.Errorf("trust policy does not allow web identity assumption")
+	// 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,
+	}
+
+	// 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
+}
@@ -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