From c878784f1e1e5433d60160513a29ac60d136065d Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Wed, 27 Aug 2025 10:35:46 +0100 Subject: [PATCH] Next set of enhancements together with memory usage improvements. --- azure_oauth_callback_test.go | 181 +++++++++++++ error_recovery.go | 6 +- issue60_integration_test.go | 213 +++++++++++++++ main.go | 69 ++++- robustness_test.go | 10 +- settings.go | 74 ++++- template_processing_test.go | 362 +++++++++++++++++++++++++ templated_header_missing_field_test.go | 206 ++++++++++++++ 8 files changed, 1093 insertions(+), 28 deletions(-) create mode 100644 azure_oauth_callback_test.go create mode 100644 issue60_integration_test.go create mode 100644 template_processing_test.go create mode 100644 templated_header_missing_field_test.go diff --git a/azure_oauth_callback_test.go b/azure_oauth_callback_test.go new file mode 100644 index 0000000..ab1a8fb --- /dev/null +++ b/azure_oauth_callback_test.go @@ -0,0 +1,181 @@ +package traefikoidc + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestAzureOAuthCallbackScenario tests the exact scenario from issue #53 +// This test ensures that cookies set during OAuth initiation are available +// during the callback from Azure AD +func TestAzureOAuthCallbackScenario(t *testing.T) { + t.Run("Azure_OAuth_Complete_Flow", func(t *testing.T) { + sessionManager, err := NewSessionManager("test-encryption-key-32-characters", false, NewLogger("debug")) + require.NoError(t, err) + + // Step 1: User visits https://app.example.com/protected + // Traefik receives this as http://internal/protected with headers + initReq := httptest.NewRequest("GET", "http://internal/protected", nil) + initReq.Header.Set("X-Forwarded-Proto", "https") + initReq.Header.Set("X-Forwarded-Host", "app.example.com") + initReq.Header.Set("User-Agent", "Mozilla/5.0 (Windows NT 10.0; Win64; x64)") + initReq.Host = "internal" // The actual host Traefik sees + + // Get session and prepare for OAuth + session, err := sessionManager.GetSession(initReq) + require.NoError(t, err) + + // Set OAuth flow data + csrfToken := "azure-csrf-state-token" + nonce := "azure-nonce-value" + codeVerifier := "pkce-code-verifier" + + session.SetCSRF(csrfToken) + session.SetNonce(nonce) + session.SetCodeVerifier(codeVerifier) + session.SetIncomingPath("/protected") + session.MarkDirty() + + // Save session + rec := httptest.NewRecorder() + err = session.Save(initReq, rec) + require.NoError(t, err) + + // Examine the cookies that would be sent to the browser + cookies := rec.Result().Cookies() + require.NotEmpty(t, cookies, "Cookies must be set for OAuth flow") + + var mainCookie *http.Cookie + for _, cookie := range cookies { + if cookie.Name == "_oidc_raczylo_m" { + mainCookie = cookie + break + } + } + require.NotNil(t, mainCookie, "Main session cookie must be set") + + // Verify cookie attributes for Azure OAuth + assert.True(t, mainCookie.Secure, "Cookie MUST be Secure for HTTPS") + assert.Equal(t, http.SameSiteLaxMode, mainCookie.SameSite, + "MUST be Lax to allow Azure callback from different domain") + assert.Equal(t, "app.example.com", mainCookie.Domain, + "Domain must match X-Forwarded-Host for browser to send it back") + assert.Equal(t, "/", mainCookie.Path, "Path must be root") + assert.True(t, mainCookie.HttpOnly, "HttpOnly for security") + + // Step 2: User is redirected to Azure AD login + // Azure AD redirects back to https://app.example.com/oidc/callback?code=xxx&state=xxx + // Traefik receives this as http://internal/oidc/callback with headers + + callbackReq := httptest.NewRequest("GET", + "http://internal/oidc/callback?code=AzureAuthCode&state="+csrfToken, nil) + callbackReq.Header.Set("X-Forwarded-Proto", "https") + callbackReq.Header.Set("X-Forwarded-Host", "app.example.com") + callbackReq.Header.Set("User-Agent", "Mozilla/5.0 (Windows NT 10.0; Win64; x64)") + callbackReq.Host = "internal" + + // Browser sends cookies because: + // 1. Request is to https://app.example.com (matches cookie domain) + // 2. Cookie has Secure flag and request is HTTPS + // 3. Cookie has SameSite=Lax which allows top-level navigation from Azure + for _, cookie := range cookies { + callbackReq.AddCookie(cookie) + } + + // Get session in callback + callbackSession, err := sessionManager.GetSession(callbackReq) + require.NoError(t, err) + + // Verify session data is available - THIS WAS FAILING IN ISSUE #53 + retrievedCSRF := callbackSession.GetCSRF() + assert.Equal(t, csrfToken, retrievedCSRF, + "CSRF token MUST be available in callback (was missing in issue #53)") + + retrievedNonce := callbackSession.GetNonce() + assert.Equal(t, nonce, retrievedNonce, + "Nonce MUST be available for security validation") + + retrievedCodeVerifier := callbackSession.GetCodeVerifier() + assert.Equal(t, codeVerifier, retrievedCodeVerifier, + "PKCE verifier MUST be available for token exchange") + + retrievedPath := callbackSession.GetIncomingPath() + assert.Equal(t, "/protected", retrievedPath, + "Original path MUST be available for post-auth redirect") + }) + + t.Run("Cookie_Not_Sent_With_Wrong_Domain", func(t *testing.T) { + // This test verifies that cookies with wrong domain won't be sent + sessionManager, err := NewSessionManager("test-encryption-key-32-characters", false, NewLogger("debug")) + require.NoError(t, err) + + // Initial request sets cookie for app.example.com + initReq := httptest.NewRequest("GET", "http://internal/protected", nil) + initReq.Header.Set("X-Forwarded-Proto", "https") + initReq.Header.Set("X-Forwarded-Host", "app.example.com") + initReq.Header.Set("User-Agent", "Mozilla/5.0") + + session, err := sessionManager.GetSession(initReq) + require.NoError(t, err) + session.SetCSRF("test-csrf") + + rec := httptest.NewRecorder() + err = session.Save(initReq, rec) + require.NoError(t, err) + + // Callback comes to different domain + callbackReq := httptest.NewRequest("GET", "http://internal/oidc/callback", nil) + callbackReq.Header.Set("X-Forwarded-Proto", "https") + callbackReq.Header.Set("X-Forwarded-Host", "different.example.com") // Different domain! + callbackReq.Header.Set("User-Agent", "Mozilla/5.0") + + // Browser wouldn't send cookies because domain doesn't match + // So we simulate that by not adding cookies + + callbackSession, err := sessionManager.GetSession(callbackReq) + require.NoError(t, err) + + // Session should be empty + assert.Empty(t, callbackSession.GetCSRF(), + "CSRF should be empty when cookies aren't sent due to domain mismatch") + }) + + t.Run("SameSite_Strict_Would_Break_OAuth", func(t *testing.T) { + // This test demonstrates why we can't use SameSite=Strict for OAuth + // With Strict, cookies wouldn't be sent when redirecting from Azure to our app + + sessionManager, err := NewSessionManager("test-encryption-key-32-characters", false, NewLogger("debug")) + require.NoError(t, err) + + // If we had SameSite=Strict (which we don't anymore), the browser would: + // 1. Set cookie when user visits app.example.com + // 2. NOT send cookie when Azure redirects back to app.example.com/callback + // This is because the request originates from login.microsoftonline.com + + // Our fix ensures we use SameSite=Lax which allows top-level navigation + req := httptest.NewRequest("GET", "http://internal/test", nil) + req.Header.Set("X-Forwarded-Proto", "https") + req.Header.Set("X-Forwarded-Host", "app.example.com") + + session, err := sessionManager.GetSession(req) + require.NoError(t, err) + session.SetCSRF("test") + + rec := httptest.NewRecorder() + err = session.Save(req, rec) + require.NoError(t, err) + + cookies := rec.Result().Cookies() + for _, cookie := range cookies { + if cookie.Name == "_oidc_raczylo_m" { + assert.Equal(t, http.SameSiteLaxMode, cookie.SameSite, + "Must use Lax, not Strict, for OAuth to work") + break + } + } + }) +} diff --git a/error_recovery.go b/error_recovery.go index bc671c2..2c110c0 100644 --- a/error_recovery.go +++ b/error_recovery.go @@ -174,9 +174,9 @@ type CircuitBreakerConfig struct { // DefaultCircuitBreakerConfig returns default circuit breaker configuration func DefaultCircuitBreakerConfig() CircuitBreakerConfig { return CircuitBreakerConfig{ - MaxFailures: 5, - Timeout: 30 * time.Second, - ResetTimeout: 10 * time.Second, + MaxFailures: 2, // Reduced from 5 to open circuit faster + Timeout: 60 * time.Second, // Increased from 30s to reduce retry frequency + ResetTimeout: 30 * time.Second, // Increased from 10s to wait longer before retrying } } diff --git a/issue60_integration_test.go b/issue60_integration_test.go new file mode 100644 index 0000000..b5c26bf --- /dev/null +++ b/issue60_integration_test.go @@ -0,0 +1,213 @@ +package traefikoidc + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestIssue60Integration tests the complete fix for issue #60 +// This test verifies that the plugin can handle missing claim fields without panicking +func TestIssue60Integration(t *testing.T) { + t.Run("Config_With_Safe_Functions_Validates", func(t *testing.T) { + config := CreateConfig() + config.ProviderURL = "https://example.com" + config.ClientID = "test-client" + config.ClientSecret = "test-secret" + config.CallbackURL = "/callback" + config.SessionEncryptionKey = "test-encryption-key-32-characters" + + // Templates using safe functions for missing fields + config.Headers = []TemplatedHeader{ + {Name: "X-User-Email", Value: "{{.Claims.email}}"}, + {Name: "X-User-Role", Value: "{{get .Claims \"internal_role\"}}"}, + {Name: "X-User-Dept", Value: "{{default \"unknown\" .Claims.department}}"}, + {Name: "X-User-Groups", Value: "{{with .Claims.groups}}{{.}}{{end}}"}, + } + + // Configuration should validate successfully + err := config.Validate() + assert.NoError(t, err, "Config with safe template functions should validate") + }) + + t.Run("Direct_Template_Access_Works", func(t *testing.T) { + config := CreateConfig() + config.ProviderURL = "https://example.com" + config.ClientID = "test-client" + config.ClientSecret = "test-secret" + config.CallbackURL = "/callback" + config.SessionEncryptionKey = "test-encryption-key-32-characters" + + // Direct claim access (will return if missing with missingkey=zero) + config.Headers = []TemplatedHeader{ + {Name: "X-User-Email", Value: "{{.Claims.email}}"}, + {Name: "X-Internal-Role", Value: "{{.Claims.internal_role}}"}, + } + + err := config.Validate() + assert.NoError(t, err, "Direct claim access should validate") + }) + + t.Run("Config_Rejects_Dangerous_Templates", func(t *testing.T) { + config := CreateConfig() + config.ProviderURL = "https://example.com" + config.ClientID = "test-client" + config.ClientSecret = "test-secret" + config.CallbackURL = "/callback" + config.SessionEncryptionKey = "test-encryption-key-32-characters" + + // Dangerous template patterns should be rejected + dangerousTemplates := []TemplatedHeader{ + {Name: "X-Bad-1", Value: "{{call .SomeFunc}}"}, + {Name: "X-Bad-2", Value: "{{range .Items}}{{.}}{{end}}"}, + {Name: "X-Bad-3", Value: "{{index .Array 0}}"}, + {Name: "X-Bad-4", Value: "{{printf \"%s\" .Data}}"}, + } + + for _, header := range dangerousTemplates { + config.Headers = []TemplatedHeader{header} + err := config.Validate() + require.Error(t, err, "Dangerous template should be rejected: %s", header.Value) + assert.Contains(t, err.Error(), "dangerous", "Error should mention dangerous pattern") + } + }) + + t.Run("Verify_Template_Execution_Context", func(t *testing.T) { + // This test verifies that our template context matches what's actually used + // The context should have these fields (all capitalized): + // - AccessToken + // - IDToken (or IdToken) + // - RefreshToken + // - Claims (map[string]interface{}) + + config := CreateConfig() + config.ProviderURL = "https://example.com" + config.ClientID = "test-client" + config.ClientSecret = "test-secret" + config.CallbackURL = "/callback" + config.SessionEncryptionKey = "test-encryption-key-32-characters" + + // These should all be valid based on the actual template context + validContextTemplates := []TemplatedHeader{ + {Name: "X-Access-Token", Value: "{{.AccessToken}}"}, + {Name: "X-ID-Token", Value: "{{.IdToken}}"}, + {Name: "X-Refresh-Token", Value: "{{.RefreshToken}}"}, + {Name: "X-User-Email", Value: "{{.Claims.email}}"}, + {Name: "X-User-Sub", Value: "{{.Claims.sub}}"}, + } + + config.Headers = validContextTemplates + err := config.Validate() + assert.NoError(t, err, "All valid context fields should pass validation") + }) + + t.Run("Common_Azure_AD_Claims", func(t *testing.T) { + // Test Azure AD specific claims mentioned in issue #60 + config := CreateConfig() + config.ProviderURL = "https://example.com" + config.ClientID = "test-client" + config.ClientSecret = "test-secret" + config.CallbackURL = "/callback" + config.SessionEncryptionKey = "test-encryption-key-32-characters" + + // Azure AD commonly uses these claim fields + config.Headers = []TemplatedHeader{ + {Name: "X-User-Email", Value: "{{.Claims.email}}"}, + {Name: "X-User-OID", Value: "{{.Claims.oid}}"}, + {Name: "X-User-TID", Value: "{{.Claims.tid}}"}, + {Name: "X-User-UPN", Value: "{{.Claims.upn}}"}, + {Name: "X-Internal-Role", Value: "{{.Claims.internal_role}}"}, // Custom claim from issue #60 + } + + err := config.Validate() + assert.NoError(t, err, "Azure AD claims should validate") + }) +} + +// TestIssue60RealWorldScenarios tests real-world scenarios from issue #60 +func TestIssue60RealWorldScenarios(t *testing.T) { + t.Run("Missing_Internal_Role_Field", func(t *testing.T) { + // This is the exact scenario from issue #60 + // User passes {{.Claims.internal_role}} but the field doesn't exist + config := CreateConfig() + config.ProviderURL = "https://example.com" + config.ClientID = "test-client" + config.ClientSecret = "test-secret" + config.CallbackURL = "/callback" + config.SessionEncryptionKey = "test-encryption-key-32-characters" + + // The problematic template from issue #60 + config.Headers = []TemplatedHeader{ + {Name: "X-Internal-Role", Value: "{{.Claims.internal_role}}"}, + } + + // Should validate (internal_role is in the safe fields list) + err := config.Validate() + assert.NoError(t, err, "Template with internal_role should validate") + }) + + t.Run("Safe_Access_Patterns_From_Guide", func(t *testing.T) { + // Test all the safe patterns documented in TEMPLATE_HEADERS_GUIDE.md + config := CreateConfig() + config.ProviderURL = "https://example.com" + config.ClientID = "test-client" + config.ClientSecret = "test-secret" + config.CallbackURL = "/callback" + config.SessionEncryptionKey = "test-encryption-key-32-characters" + + // All safe patterns from the guide + config.Headers = []TemplatedHeader{ + // Basic field access + {Name: "X-User-Role", Value: "{{.Claims.internal_role}}"}, + + // Using the get function + {Name: "X-User-Role-Get", Value: "{{get .Claims \"internal_role\"}}"}, + + // Using the default function + {Name: "X-User-Role-Default", Value: "{{default \"guest\" .Claims.role}}"}, + + // Nested fields with 'with' + {Name: "X-User-Admin", Value: "{{with .Claims.groups}}{{.admin}}{{end}}"}, + } + + err := config.Validate() + assert.NoError(t, err, "All safe patterns from guide should validate") + }) +} + +// TestIssue60DoubleProcessingConcern tests the user's specific concern about double processing +func TestIssue60DoubleProcessingConcern(t *testing.T) { + t.Run("Template_Not_Evaluated_During_Config_Parse", func(t *testing.T) { + // The user was concerned that templates might be processed twice: + // 1. Once when Traefik parses the config + // 2. Once when the plugin executes the template + + // This test verifies that templates are stored as strings during config parsing + config := &Config{ + Headers: []TemplatedHeader{ + {Name: "X-Test", Value: "{{.Claims.test}}"}, + }, + } + + // The template should still be a raw string after config creation + assert.Equal(t, "{{.Claims.test}}", config.Headers[0].Value, + "Template should remain as raw string in config") + + // The template is only parsed/executed when the plugin initializes and processes requests + // Not during config unmarshaling + }) + + t.Run("Functions_Preserved_Through_Config_Marshaling", func(t *testing.T) { + // Test that our custom function syntax survives config marshaling/unmarshaling + originalValue := `{{get .Claims "internal_role"}}` + header := TemplatedHeader{ + Name: "X-Role", + Value: originalValue, + } + + // Even after any marshaling/unmarshaling, the template string should be preserved + assert.Equal(t, originalValue, header.Value, + "Template with functions should be preserved exactly") + }) +} diff --git a/main.go b/main.go index 0cd652d..a030ee7 100644 --- a/main.go +++ b/main.go @@ -44,11 +44,11 @@ func createDefaultHTTPClient() *http.Client { ForceAttemptHTTP2: true, TLSHandshakeTimeout: 3 * time.Second, // TLS handshake timeout ExpectContinueTimeout: 1 * time.Second, // Timeout for 100-continue responses - MaxIdleConns: 20, // Maximum idle connections across all hosts - MaxIdleConnsPerHost: 5, // Maximum idle connections per host - IdleConnTimeout: 60 * time.Second, // Maximum idle time before connection close + MaxIdleConns: 10, // Reduced from 20 to prevent memory buildup + MaxIdleConnsPerHost: 2, // Reduced from 5 to limit per-host connections + IdleConnTimeout: 30 * time.Second, // Reduced from 60 to close idle connections faster DisableKeepAlives: false, // Enable connection reuse - MaxConnsPerHost: 20, // Maximum connections per host + MaxConnsPerHost: 10, // Reduced from 20 to limit concurrent connections ResponseHeaderTimeout: 5 * time.Second, // Timeout for reading response headers DisableCompression: false, // Enable compression for bandwidth efficiency WriteBufferSize: 4096, // Write buffer size for connections @@ -748,11 +748,33 @@ func New(ctx context.Context, next http.Handler, config *Config, name string) (h t.startTokenCleanup() t.tokenExchanger = t // Initialize the interface field to self - // Initialize and parse header templates + // Initialize and parse header templates with safe field access t.headerTemplates = make(map[string]*template.Template) + + // Create custom template functions for safe field access + funcMap := template.FuncMap{ + // "default" returns a default value if the input is nil or empty + "default": func(defaultVal interface{}, val interface{}) interface{} { + if val == nil || val == "" { + return defaultVal + } + return val + }, + // "get" safely gets a value from a map, returning empty string if not found + "get": func(m interface{}, key string) interface{} { + if mapVal, ok := m.(map[string]interface{}); ok { + if val, exists := mapVal[key]; exists { + return val + } + } + return "" + }, + } + for _, header := range config.Headers { - // Use a default empty template to set a proper name for error reporting - tmpl := template.New(header.Name) + // Create template with custom functions and missingkey=zero option + // This prevents panics when accessing non-existent fields + tmpl := template.New(header.Name).Funcs(funcMap).Option("missingkey=zero") // Parse the template with proper error handling parsedTmpl, err := tmpl.Parse(header.Value) @@ -838,16 +860,28 @@ func (t *TraefikOidc) updateMetadataEndpoints(metadata *ProviderMetadata) { // Parameters: // - providerURL: The base URL of bogged OIDC provider, used for subsequent refresh attempts. func (t *TraefikOidc) startMetadataRefresh(providerURL string) { - ticker := time.NewTicker(1 * time.Hour) - t.goroutineWG.Add(1) // Track this goroutine + // Use longer interval to reduce memory pressure from refresh attempts + ticker := time.NewTicker(2 * time.Hour) // Increased from 1 hour + t.goroutineWG.Add(1) // Track this goroutine go func() { defer t.goroutineWG.Done() // Signal completion when goroutine exits defer ticker.Stop() // Ensure ticker is always stopped + consecutiveFailures := 0 for { select { case <-ticker.C: + // Skip refresh if we've had too many consecutive failures + if consecutiveFailures >= 3 { + t.logger.Debug("Skipping metadata refresh due to consecutive failures") + consecutiveFailures++ // Still increment to track skips + if consecutiveFailures > 10 { + consecutiveFailures = 3 // Reset to prevent overflow but keep skipping + } + continue + } + t.logger.Debug("Refreshing OIDC metadata") var metadata *ProviderMetadata var err error @@ -858,14 +892,17 @@ func (t *TraefikOidc) startMetadataRefresh(providerURL string) { metadata, err = t.metadataCache.GetMetadata(providerURL, t.httpClient, t.logger) } if err != nil { - t.logger.Errorf("Failed to refresh metadata: %v", err) + consecutiveFailures++ + t.logger.Errorf("Failed to refresh metadata (attempt %d): %v", consecutiveFailures, err) continue } if metadata != nil { t.updateMetadataEndpoints(metadata) t.logger.Debug("Successfully refreshed metadata") + consecutiveFailures = 0 // Reset on success } else { + consecutiveFailures++ t.logger.Error("Received nil metadata during refresh") } case <-t.metadataRefreshStopChan: @@ -896,11 +933,11 @@ func (t *TraefikOidc) startMetadataRefresh(providerURL string) { func discoverProviderMetadata(providerURL string, httpClient *http.Client, l *Logger) (*ProviderMetadata, error) { wellKnownURL := strings.TrimSuffix(providerURL, "/") + "/.well-known/openid-configuration" - // Create retry executor with configuration optimized for test and production environments + // Create retry executor with reduced attempts to prevent memory buildup retryConfig := RetryConfig{ - MaxAttempts: 4, + MaxAttempts: 2, // Reduced from 4 to fail faster when provider is down InitialDelay: 10 * time.Millisecond, - MaxDelay: 100 * time.Millisecond, + MaxDelay: 50 * time.Millisecond, // Reduced from 100ms BackoffFactor: 2.0, EnableJitter: true, RetryableErrors: []string{ @@ -958,12 +995,16 @@ func fetchMetadata(wellKnownURL string, httpClient *http.Client) (*ProviderMetad defer func() { if resp != nil && resp.Body != nil { + // Drain the body before closing to ensure connection can be reused + io.Copy(io.Discard, resp.Body) resp.Body.Close() } }() if resp.StatusCode != http.StatusOK { - bodyBytes, _ := io.ReadAll(resp.Body) + // Limit body read to prevent memory issues with large error responses + limitReader := io.LimitReader(resp.Body, 1024*10) // 10KB limit + bodyBytes, _ := io.ReadAll(limitReader) return nil, fmt.Errorf("failed to fetch provider metadata from %s: status code %d, body: %s", wellKnownURL, resp.StatusCode, string(bodyBytes)) } diff --git a/robustness_test.go b/robustness_test.go index 463d402..c3cfe6f 100644 --- a/robustness_test.go +++ b/robustness_test.go @@ -327,12 +327,12 @@ func TestProviderFailureRecovery(t *testing.T) { var requestCount int64 server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { count := atomic.AddInt64(&requestCount, 1) - if count <= 3 { - // Fail first 3 requests + if count <= 1 { + // Fail first request only (we now retry max 2 times) w.WriteHeader(http.StatusInternalServerError) return } - // Succeed after 3 failures + // Succeed on 2nd attempt metadata := ProviderMetadata{ Issuer: "https://test-issuer.com", AuthURL: "https://test-issuer.com/auth", @@ -361,8 +361,8 @@ func TestProviderFailureRecovery(t *testing.T) { t.Errorf("Expected metadata to be returned after recovery") } - // Should have taken some time due to retries (at least the sum of delays: 10ms + 20ms + 40ms = 70ms) - expectedMinDuration := 70 * time.Millisecond + // Should have taken some time due to retries (with 2 max attempts: ~10ms delay) + expectedMinDuration := 10 * time.Millisecond if duration < expectedMinDuration { t.Errorf("Expected discovery to take at least %v due to retries, but took %v", expectedMinDuration, duration) } diff --git a/settings.go b/settings.go index bded998..c1f520b 100644 --- a/settings.go +++ b/settings.go @@ -26,7 +26,7 @@ type TemplatedHeader struct { // It provides all necessary settings to configure OpenID Connect authentication // with various providers like Auth0, Logto, or any standard OIDC provider. type Config struct { - HTTPClient *http.Client + HTTPClient *http.Client `json:"-"` // Exclude from JSON marshaling ProviderURL string `json:"providerURL"` RevocationURL string `json:"revocationURL"` CallbackURL string `json:"callbackURL"` @@ -218,11 +218,27 @@ func (c *Config) Validate() error { // validateTemplateSecure validates template expressions for security vulnerabilities func validateTemplateSecure(templateStr string) error { + // Allow our specific safe custom functions + // These are added specifically to handle missing fields safely (issue #60) + safeCustomFunctions := []string{ + "{{get ", // Safe map access function + "{{default ", // Safe default value function + } + + // Check if template uses safe custom functions + usesSafeFunctions := false + for _, safeFn := range safeCustomFunctions { + if strings.Contains(templateStr, safeFn) { + usesSafeFunctions = true + // These functions are explicitly allowed for safe field access + } + } + // Check for dangerous template functions and patterns + // Skip certain checks if using our safe functions dangerousPatterns := []string{ - "{{call", // Function calls + "{{call", // Function calls (except our safe ones) "{{range", // Range over arbitrary data - "{{with", // With statements that could access unexpected data "{{define", // Template definitions "{{template", // Template inclusions "{{block", // Block definitions @@ -230,7 +246,7 @@ func validateTemplateSecure(templateStr string) error { "{{-", // Trim whitespace (could be used to obfuscate) "-}}", // Trim whitespace (could be used to obfuscate) "{{printf", // Printf functions - "{{print", // Print functions + "{{print", // Print functions (but not our safe ones) "{{println", // Println functions "{{html", // HTML functions "{{js", // JavaScript functions @@ -249,9 +265,31 @@ func validateTemplateSecure(templateStr string) error { "{{not", // Logical operations } + // Allow 'with' for safe conditional access + if !strings.Contains(templateStr, "{{with .Claims") { + dangerousPatterns = append(dangerousPatterns, "{{with") + } + templateLower := strings.ToLower(templateStr) for _, pattern := range dangerousPatterns { - if strings.Contains(templateLower, pattern) { + // Skip check if it's one of our safe functions + if usesSafeFunctions && (pattern == "{{call" || pattern == "{{print") { + // Allow these if we're using safe functions + continue + } + + // Special handling for comparison operators to avoid false positives with "get" and "default" + if pattern == "{{ge" && (strings.Contains(templateStr, "{{get ") || strings.Contains(templateStr, "{{default ")) { + // Skip {{ge check if we're using the safe {{get or {{default functions + continue + } + + // Skip {{de checks if using {{default + if pattern == "{{define" && strings.Contains(templateStr, "{{default ") { + continue + } + + if strings.Contains(templateLower, strings.ToLower(pattern)) { return fmt.Errorf("dangerous template pattern detected: %s", pattern) } } @@ -262,6 +300,9 @@ func validateTemplateSecure(templateStr string) error { "{{.IdToken}}", "{{.RefreshToken}}", "{{.Claims.", + "{{get ", // Safe custom function + "{{default ", // Safe custom function + "{{with ", // Safe conditional (when used with Claims) } // Check if template contains only allowed patterns @@ -274,13 +315,15 @@ func validateTemplateSecure(templateStr string) error { } if !hasAllowedPattern { - return fmt.Errorf("template must use only allowed variables: AccessToken, IdToken, RefreshToken, or Claims.*") + return fmt.Errorf("template must use only allowed variables: AccessToken, IdToken, RefreshToken, Claims.*, or safe functions (get, default, with)") } // Validate claims access patterns if strings.Contains(templateStr, "{{.Claims.") { // Simple validation - ensure claims access is to known safe fields + // This list includes standard OIDC claims and common provider-specific claims safeClaimsFields := map[string]bool{ + // Standard OIDC claims "email": true, "name": true, "given_name": true, @@ -293,6 +336,25 @@ func validateTemplateSecure(templateStr string) error { "iat": true, "groups": true, "roles": true, + // Common custom claims + "internal_role": true, // Custom roles field (issue #60) + "role": true, // Alternative role field + "department": true, // Organization info + "organization": true, // Organization info + // Provider-specific claims + "realm_access": true, // Keycloak specific + "resource_access": true, // Keycloak specific + "oid": true, // Azure AD object ID + "tid": true, // Azure AD tenant ID + "upn": true, // Azure AD User Principal Name + "hd": true, // Google hosted domain + "picture": true, // Profile picture + // Additional standard claims + "locale": true, // User locale + "zoneinfo": true, // Timezone + "phone_number": true, // Contact info + "email_verified": true, // Email verification status + "updated_at": true, // Last update time } // Extract field names from Claims access diff --git a/template_processing_test.go b/template_processing_test.go new file mode 100644 index 0000000..24e83a4 --- /dev/null +++ b/template_processing_test.go @@ -0,0 +1,362 @@ +package traefikoidc + +import ( + "bytes" + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + "text/template" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestTemplateDoubleProcessing tests if template strings are being double-processed +// This addresses the user's concern about potential double processing by the parser +func TestTemplateDoubleProcessing(t *testing.T) { + t.Run("Template_Strings_Not_Double_Processed", func(t *testing.T) { + // Simulate how Traefik passes config to the plugin + // Traefik uses YAML/TOML config which gets unmarshaled into the Config struct + // yamlConfig example: + // headers: + // - name: "X-User-Email" + // value: "{{.Claims.email}}" + // - name: "X-User-Role" + // value: "{{.Claims.internal_role}}" + + // This simulates what Traefik does internally - it parses YAML/TOML and creates a Config struct + // The template strings are NOT processed at this stage, they're just strings + config := &Config{ + Headers: []TemplatedHeader{ + {Name: "X-User-Email", Value: "{{.Claims.email}}"}, + {Name: "X-User-Role", Value: "{{.Claims.internal_role}}"}, + }, + } + + // Verify that template strings are still raw (not processed) + assert.Equal(t, "{{.Claims.email}}", config.Headers[0].Value) + assert.Equal(t, "{{.Claims.internal_role}}", config.Headers[1].Value) + + // Now simulate what happens when the plugin initializes + // The template strings should only be parsed once during initialization + headerTemplates := make(map[string]*template.Template) + + funcMap := template.FuncMap{ + "default": func(defaultVal interface{}, val interface{}) interface{} { + if val == nil || val == "" || val == "" { + return defaultVal + } + return val + }, + "get": func(m interface{}, key string) interface{} { + if mapVal, ok := m.(map[string]interface{}); ok { + if val, exists := mapVal[key]; exists { + return val + } + } + return "" + }, + } + + for _, header := range config.Headers { + tmpl := template.New(header.Name).Funcs(funcMap).Option("missingkey=zero") + parsedTmpl, err := tmpl.Parse(header.Value) + require.NoError(t, err) + headerTemplates[header.Name] = parsedTmpl + } + + // Test execution with actual claims + claims := map[string]interface{}{ + "email": "user@example.com", + // Note: internal_role is missing + } + + templateData := map[string]interface{}{ + "Claims": claims, + } + + // Execute templates + for headerName, tmpl := range headerTemplates { + var buf bytes.Buffer + err := tmpl.Execute(&buf, templateData) + require.NoError(t, err) + + result := buf.String() + if headerName == "X-User-Email" { + assert.Equal(t, "user@example.com", result) + } else if headerName == "X-User-Role" { + // With missingkey=zero, missing fields return "" + assert.Equal(t, "", result) + } + } + }) + + t.Run("Config_Marshaling_Preserves_Template_Syntax", func(t *testing.T) { + // Test that marshaling/unmarshaling config doesn't affect template strings + originalConfig := &Config{ + ProviderURL: "https://example.com", + ClientID: "test-client", + ClientSecret: "test-secret", + CallbackURL: "/callback", + SessionEncryptionKey: "test-encryption-key-32-characters", + Headers: []TemplatedHeader{ + {Name: "X-User-Email", Value: "{{.Claims.email}}"}, + {Name: "X-User-Role", Value: "{{get .Claims \"internal_role\"}}"}, + {Name: "X-User-Dept", Value: "{{default \"unknown\" .Claims.department}}"}, + }, + } + + // Marshal to JSON (simulating Traefik's config processing) + jsonData, err := json.Marshal(originalConfig) + require.NoError(t, err) + + // Unmarshal back + var unmarshaledConfig Config + err = json.Unmarshal(jsonData, &unmarshaledConfig) + require.NoError(t, err) + + // Verify template strings are preserved exactly + assert.Equal(t, "{{.Claims.email}}", unmarshaledConfig.Headers[0].Value) + assert.Equal(t, `{{get .Claims "internal_role"}}`, unmarshaledConfig.Headers[1].Value) + assert.Equal(t, `{{default "unknown" .Claims.department}}`, unmarshaledConfig.Headers[2].Value) + }) + + t.Run("Template_Functions_Work_After_Config_Processing", func(t *testing.T) { + // Simulate the full flow from config to execution + jsonConfig := `{ + "providerURL": "https://example.com", + "clientID": "test-client", + "clientSecret": "test-secret", + "callbackURL": "/callback", + "sessionEncryptionKey": "test-encryption-key-32-characters", + "headers": [ + {"name": "X-User-Email", "value": "{{.Claims.email}}"}, + {"name": "X-User-Role", "value": "{{get .Claims \"internal_role\"}}"}, + {"name": "X-User-Dept", "value": "{{default \"engineering\" .Claims.department}}"} + ] + }` + + var config Config + err := json.Unmarshal([]byte(jsonConfig), &config) + require.NoError(t, err) + + // Initialize templates with functions + funcMap := template.FuncMap{ + "default": func(defaultVal interface{}, val interface{}) interface{} { + if val == nil || val == "" || val == "" { + return defaultVal + } + return val + }, + "get": func(m interface{}, key string) interface{} { + if mapVal, ok := m.(map[string]interface{}); ok { + if val, exists := mapVal[key]; exists { + return val + } + } + return "" + }, + } + + headerTemplates := make(map[string]*template.Template) + for _, header := range config.Headers { + tmpl := template.New(header.Name).Funcs(funcMap).Option("missingkey=zero") + parsedTmpl, err := tmpl.Parse(header.Value) + require.NoError(t, err) + headerTemplates[header.Name] = parsedTmpl + } + + // Test with claims + claims := map[string]interface{}{ + "email": "user@example.com", + // internal_role and department are missing + } + + templateData := map[string]interface{}{ + "Claims": claims, + } + + results := make(map[string]string) + for headerName, tmpl := range headerTemplates { + var buf bytes.Buffer + err := tmpl.Execute(&buf, templateData) + require.NoError(t, err) + results[headerName] = buf.String() + } + + // Verify results + assert.Equal(t, "user@example.com", results["X-User-Email"]) + assert.Equal(t, "", results["X-User-Role"]) // get function returns empty string + assert.Equal(t, "engineering", results["X-User-Dept"]) // default function provides fallback + }) +} + +// TestTemplateIntegrationWithPlugin tests template processing in the actual plugin +func TestTemplateIntegrationWithPlugin(t *testing.T) { + t.Run("Plugin_Handles_Missing_Claims_Safely", func(t *testing.T) { + // Set up test OIDC server + var testServerURL string + testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/.well-known/openid-configuration": + json.NewEncoder(w).Encode(map[string]interface{}{ + "issuer": testServerURL, + "authorization_endpoint": testServerURL + "/auth", + "token_endpoint": testServerURL + "/token", + "jwks_uri": testServerURL + "/jwks", + "userinfo_endpoint": testServerURL + "/userinfo", + }) + case "/jwks": + json.NewEncoder(w).Encode(map[string]interface{}{ + "keys": []interface{}{}, + }) + default: + http.NotFound(w, r) + } + })) + defer testServer.Close() + testServerURL = testServer.URL + + // Create config with templates that reference potentially missing fields + config := &Config{ + ProviderURL: testServer.URL, + ClientID: "test-client", + ClientSecret: "test-secret", + CallbackURL: "/callback", + SessionEncryptionKey: "test-encryption-key-32-characters", + Headers: []TemplatedHeader{ + {Name: "X-User-Email", Value: "{{.Claims.email}}"}, + {Name: "X-User-Role", Value: "{{.Claims.internal_role}}"}, + }, + } + + // Initialize plugin + ctx := context.Background() + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Check headers set by the plugin + email := r.Header.Get("X-User-Email") + role := r.Header.Get("X-User-Role") + + // Write headers to response for testing + w.Header().Set("X-Test-Email", email) + w.Header().Set("X-Test-Role", role) + w.WriteHeader(http.StatusOK) + }) + + handler, err := New(ctx, next, config, "test-plugin") + require.NoError(t, err) + + traefikOidc, ok := handler.(*TraefikOidc) + require.True(t, ok) + + // Create a mock session with claims + req := httptest.NewRequest("GET", "/protected", nil) + + // Create session and set authentication + session, err := traefikOidc.sessionManager.GetSession(req) + require.NoError(t, err) + + // Set authentication with minimal claims (missing internal_role) + session.SetAuthenticated(true) + session.SetEmail("user@example.com") + + // Create ID token with limited claims + claims := map[string]interface{}{ + "email": "user@example.com", + "sub": "user123", + // internal_role is missing + } + + // Create a simple test JWT (signature verification is mocked in tests) + idToken, _ := createTestJWT(nil, "test-issuer", "test-client", claims) + session.SetIDToken(idToken) + + // Save session + rec := httptest.NewRecorder() + err = session.Save(req, rec) + require.NoError(t, err) + + // Create new request with session cookie + cookies := rec.Result().Cookies() + req2 := httptest.NewRequest("GET", "/protected", nil) + for _, cookie := range cookies { + req2.AddCookie(cookie) + } + + // Process request through plugin + rec2 := httptest.NewRecorder() + handler.ServeHTTP(rec2, req2) + + // Plugin should handle missing claims gracefully + // The request should proceed without errors + assert.NotEqual(t, http.StatusInternalServerError, rec2.Code) + }) +} + +// Removed createTestJWT as it already exists in main_test.go + +// TestTemplateSyntaxValidation tests that template syntax is properly validated +func TestTemplateSyntaxValidation(t *testing.T) { + t.Run("Valid_Template_Syntax", func(t *testing.T) { + validTemplates := []string{ + "{{.Claims.email}}", + "{{.Claims.internal_role}}", + "{{.AccessToken}}", + "{{.IdToken}}", + "{{.RefreshToken}}", + } + + for _, tmplStr := range validTemplates { + err := validateTemplateSecure(tmplStr) + assert.NoError(t, err, "Template should be valid: %s", tmplStr) + } + }) + + t.Run("Invalid_Template_Syntax_Blocked", func(t *testing.T) { + invalidTemplates := []struct { + template string + reason string + }{ + {"{{call .SomeFunc}}", "function calls not allowed"}, + {"{{range .Items}}{{.}}{{end}}", "range not allowed"}, + {"{{with .Data}}{{.Field}}{{end}}", "with statements blocked"}, + {"{{index .Array 0}}", "index access blocked"}, + {"{{printf \"%s\" .Data}}", "printf blocked"}, + } + + for _, tc := range invalidTemplates { + err := validateTemplateSecure(tc.template) + assert.Error(t, err, "Template should be invalid: %s (%s)", tc.template, tc.reason) + assert.Contains(t, strings.ToLower(err.Error()), "dangerous") + } + }) + + t.Run("Template_With_Custom_Functions", func(t *testing.T) { + // These templates use our safe custom functions which are now allowed + templates := []string{ + "{{get .Claims \"internal_role\"}}", + "{{default \"guest\" .Claims.role}}", + } + + // These safe custom functions should now be allowed + for _, tmplStr := range templates { + err := validateTemplateSecure(tmplStr) + assert.NoError(t, err, "Safe custom functions should be allowed: %s", tmplStr) + } + + // But other function calls should still be blocked + dangerousFunctions := []string{ + "{{call .SomeFunc}}", + "{{index .Array 0}}", + "{{slice .Data 0 10}}", + } + + for _, tmplStr := range dangerousFunctions { + err := validateTemplateSecure(tmplStr) + assert.Error(t, err, "Dangerous function calls should still be blocked: %s", tmplStr) + } + }) +} diff --git a/templated_header_missing_field_test.go b/templated_header_missing_field_test.go new file mode 100644 index 0000000..5a63a5e --- /dev/null +++ b/templated_header_missing_field_test.go @@ -0,0 +1,206 @@ +package traefikoidc + +import ( + "bytes" + "testing" + "text/template" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestTemplatedHeaderMissingField tests that accessing non-existent claim fields doesn't cause panics (issue #60) +func TestTemplatedHeaderMissingField(t *testing.T) { + t.Run("Missing_Claim_Field_Returns_Empty", func(t *testing.T) { + // Create a template with the missingkey=zero option + funcMap := template.FuncMap{ + "default": func(defaultVal interface{}, val interface{}) interface{} { + if val == nil || val == "" { + return defaultVal + } + return val + }, + "get": func(m interface{}, key string) interface{} { + if mapVal, ok := m.(map[string]interface{}); ok { + if val, exists := mapVal[key]; exists { + return val + } + } + return "" + }, + } + + tmpl := template.New("test").Funcs(funcMap).Option("missingkey=zero") + parsed, err := tmpl.Parse("{{.Claims.internal_role}}") + require.NoError(t, err) + + // Create template data with claims that don't have internal_role + claims := map[string]interface{}{ + "email": "user@example.com", + "sub": "1234567890", + "name": "John Doe", + // Note: internal_role is NOT present + } + + templateData := map[string]interface{}{ + "Claims": claims, + } + + // Execute template - should not panic + var buf bytes.Buffer + err = parsed.Execute(&buf, templateData) + require.NoError(t, err, "Template execution should not fail for missing field") + + // Should return empty string for missing field with missingkey=zero + assert.Equal(t, "", buf.String(), "Missing field should return ") + }) + + t.Run("Safe_Access_Pattern_For_Nested_Fields", func(t *testing.T) { + funcMap := template.FuncMap{ + "get": func(m interface{}, key string) interface{} { + if mapVal, ok := m.(map[string]interface{}); ok { + if val, exists := mapVal[key]; exists { + return val + } + } + return "" + }, + } + + tmpl := template.New("test").Funcs(funcMap) + // Use 'with' to safely check if field exists before accessing nested properties + parsed, err := tmpl.Parse(`{{with .Claims.groups}}{{.admin}}{{end}}`) + require.NoError(t, err) + + claims := map[string]interface{}{ + "email": "user@example.com", + // groups field doesn't exist + } + + templateData := map[string]interface{}{ + "Claims": claims, + } + + var buf bytes.Buffer + err = parsed.Execute(&buf, templateData) + require.NoError(t, err, "Should handle nested missing fields with 'with' construct") + assert.Equal(t, "", buf.String(), "Should return empty string when field doesn't exist") + }) + + t.Run("Using_Get_Function_For_Safe_Access", func(t *testing.T) { + funcMap := template.FuncMap{ + "get": func(m interface{}, key string) interface{} { + if mapVal, ok := m.(map[string]interface{}); ok { + if val, exists := mapVal[key]; exists { + return val + } + } + return "" + }, + } + + tmpl := template.New("test").Funcs(funcMap) + // Use the get function to safely access the field + parsed, err := tmpl.Parse(`{{get .Claims "internal_role"}}`) + require.NoError(t, err) + + claims := map[string]interface{}{ + "email": "user@example.com", + // internal_role not present + } + + templateData := map[string]interface{}{ + "Claims": claims, + } + + var buf bytes.Buffer + err = parsed.Execute(&buf, templateData) + require.NoError(t, err) + assert.Equal(t, "", buf.String(), "get function should return empty string for missing field") + }) + + t.Run("Using_Default_Function_For_Fallback", func(t *testing.T) { + funcMap := template.FuncMap{ + "default": func(defaultVal interface{}, val interface{}) interface{} { + if val == nil || val == "" || val == "" { + return defaultVal + } + return val + }, + } + + tmpl := template.New("test").Funcs(funcMap).Option("missingkey=zero") + // Use default to provide a fallback value + parsed, err := tmpl.Parse(`{{default "guest" .Claims.role}}`) + require.NoError(t, err) + + claims := map[string]interface{}{ + "email": "user@example.com", + // role not present + } + + templateData := map[string]interface{}{ + "Claims": claims, + } + + var buf bytes.Buffer + err = parsed.Execute(&buf, templateData) + require.NoError(t, err) + assert.Equal(t, "guest", buf.String(), "default function should provide fallback value") + }) + + t.Run("Existing_Field_Still_Works", func(t *testing.T) { + funcMap := template.FuncMap{ + "get": func(m interface{}, key string) interface{} { + if mapVal, ok := m.(map[string]interface{}); ok { + if val, exists := mapVal[key]; exists { + return val + } + } + return "" + }, + } + + tmpl := template.New("test").Funcs(funcMap).Option("missingkey=zero") + parsed, err := tmpl.Parse("{{.Claims.email}}") + require.NoError(t, err) + + claims := map[string]interface{}{ + "email": "user@example.com", + "role": "admin", + } + + templateData := map[string]interface{}{ + "Claims": claims, + } + + var buf bytes.Buffer + err = parsed.Execute(&buf, templateData) + require.NoError(t, err) + assert.Equal(t, "user@example.com", buf.String(), "Existing fields should work normally") + }) +} + +// TestHeaderTemplateIntegration tests the full integration of templated headers +func TestHeaderTemplateIntegration(t *testing.T) { + t.Run("Headers_With_Missing_Claims_Dont_Crash", func(t *testing.T) { + config := CreateConfig() + config.ProviderURL = "https://example.com" + config.ClientID = "test-client" + config.ClientSecret = "test-secret" + config.CallbackURL = "/callback" + config.SessionEncryptionKey = "test-encryption-key-32-characters" + + // Add headers that reference potentially missing fields + config.Headers = []TemplatedHeader{ + {Name: "X-User-Email", Value: "{{.Claims.email}}"}, + {Name: "X-User-Role", Value: "{{.Claims.internal_role}}"}, // This field might not exist + {Name: "X-User-Groups", Value: "{{.Claims.groups}}"}, // This field might not exist + } + + // We can't fully initialize the plugin without network access, + // but we can test that the configuration validates + err := config.Validate() + assert.NoError(t, err, "Configuration should be valid even with potentially missing fields") + }) +}