Next set of enhancements together with memory usage improvements.

This commit is contained in:
2025-08-27 10:35:46 +01:00
parent 1fd762848a
commit c878784f1e
8 changed files with 1093 additions and 28 deletions
+181
View File
@@ -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
}
}
})
}
+3 -3
View File
@@ -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
}
}
+213
View File
@@ -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 <no value> 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")
})
}
+55 -14
View File
@@ -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))
}
+5 -5
View File
@@ -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)
}
+68 -6
View File
@@ -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
+362
View File
@@ -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 == "<no value>" {
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": "[email protected]",
// 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, "[email protected]", result)
} else if headerName == "X-User-Role" {
// With missingkey=zero, missing fields return "<no value>"
assert.Equal(t, "<no value>", 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 == "<no value>" {
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": "[email protected]",
// 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, "[email protected]", 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("[email protected]")
// Create ID token with limited claims
claims := map[string]interface{}{
"email": "[email protected]",
"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)
}
})
}
+206
View File
@@ -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": "[email protected]",
"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, "<no value>", buf.String(), "Missing field should return <no value>")
})
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": "[email protected]",
// 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": "[email protected]",
// 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 == "<no value>" {
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": "[email protected]",
// 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": "[email protected]",
"role": "admin",
}
templateData := map[string]interface{}{
"Claims": claims,
}
var buf bytes.Buffer
err = parsed.Execute(&buf, templateData)
require.NoError(t, err)
assert.Equal(t, "[email protected]", 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")
})
}