diff --git a/README.md b/README.md index 3cdf8ab..ff47644 100644 --- a/README.md +++ b/README.md @@ -379,8 +379,32 @@ headers: value: "{{{{range $i, $e := .Claims.roles}}}}{{{{if $i}}}},{{{{end}}}}{{{{$e}}}}{{{{end}}}}" ``` -Available bindings: `.Claims.`, `.AccessToken`, `.IdToken`, -`.RefreshToken`. Names are case-sensitive (`.Claims`, not `.claims`). +Available bindings: `.Claims.`, `.AccessToken`, `.IdToken` +(or `.IDToken`), `.RefreshToken`. Names are case-sensitive (`.Claims`, not +`.claims`). + +Header templates are validated at startup (a failing template stops the +middleware from loading). Only a fixed set of claim fields may be emitted — +standard OIDC claims plus common provider claims (`email`, `name`, +`given_name`, `family_name`, `preferred_username`, `sub`, `groups`, `roles`, +`realm_access`, `resource_access`, `oid`, `tid`, `upn`, `hd`, `picture`, +`locale`, `email_verified`, and a few more; see `safeClaimsFields` in +`template_validation.go`). To emit a claim not on that list, add it to +`allowedClaims`: + +```yaml +allowedClaims: + - employee_id +headers: + - name: X-Employee-Id + value: "{{{{.Claims.employee_id}}}}" +``` + +Rendering the whole context (`{{.}}`, `{{$}}`), the whole claims map +(`{{.Claims}}`), or any non-listed claim is rejected — this prevents a template +from accidentally forwarding raw tokens or unlisted claims. `range`/`with` must +target a specific listed claim (e.g. `{{range .Claims.groups}}`); `get`/`default` +are the only functions allowed. > **Escape with quadruple braces.** If you see > `can't evaluate field AccessToken in type bool`, Traefik's YAML parser ate diff --git a/coverage_boost_final_test.go b/coverage_boost_final_test.go index 004351f..1b8894d 100644 --- a/coverage_boost_final_test.go +++ b/coverage_boost_final_test.go @@ -796,7 +796,7 @@ func TestValidateTemplateSecure_CoverageBoost(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := validateTemplateSecure(tt.template) + err := validateTemplateSecure(tt.template, nil) if tt.shouldError && err == nil { t.Errorf("Expected error for template: %s", tt.template) } diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index bac456d..533d1f5 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -441,13 +441,46 @@ headers: ``` **Template Variables:** -- `{{.Claims.field}}` - ID token claims +- `{{.Claims.field}}` - ID token claims (only whitelisted fields; see below) - `{{.AccessToken}}` - Raw access token -- `{{.IdToken}}` - Raw ID token +- `{{.IdToken}}` / `{{.IDToken}}` - Raw ID token (both spellings work) - `{{.RefreshToken}}` - Raw refresh token **Important:** Use double curly braces (`{{{{` and `}}}}`) to escape templates in YAML. +**Validation and the claims whitelist.** Header templates are parsed and +validated when the middleware loads; an invalid or disallowed template stops the +middleware from starting (the route then returns Traefik's +`invalid handler type: `). To prevent a template from accidentally +forwarding raw tokens or sensitive claims, only a fixed set of claim fields may +be emitted — the standard OIDC claims plus common provider claims (`email`, +`name`, `given_name`, `family_name`, `preferred_username`, `sub`, `groups`, +`roles`, `internal_role`, `role`, `department`, `organization`, `realm_access`, +`resource_access`, `oid`, `tid`, `upn`, `hd`, `picture`, `locale`, `zoneinfo`, +`phone_number`, `email_verified`, `updated_at`; the authoritative list is +`safeClaimsFields` in `template_validation.go`). Rejected: rendering the whole +context (`{{.}}`, `{{$}}`) or the whole claims map (`{{.Claims}}`), any claim not +on the list, functions other than `get`/`default`, and `range`/`with` that do not +target a specific listed claim (e.g. `{{range .Claims}}` is rejected; `{{range +.Claims.groups}}` is allowed). + +**`allowedClaims`** extends the whitelist for claims your provider issues that +are not built in: + +```yaml +allowedClaims: + - employee_id + - cost_center +headers: + - name: "X-Employee-Id" + value: "{{{{.Claims.employee_id}}}}" +``` + +Listed names apply to both direct access (`{{.Claims.employee_id}}`) and +`get` (`{{get .Claims "employee_id"}}`). Subfields of a whitelisted map-valued +claim (e.g. `realm_access.roles`) are already accessible without listing them +individually. + --- ## Security Headers diff --git a/issue149_regression_test.go b/issue149_regression_test.go new file mode 100644 index 0000000..537eb32 --- /dev/null +++ b/issue149_regression_test.go @@ -0,0 +1,303 @@ +package traefikoidc + +import ( + "bytes" + "strings" + "testing" + "text/template" +) + +// Issue #149: after NewWithContext started calling config.Validate() (commit +// 546ceb9), `headers` configs that had worked in 0.8.25 began failing, so New() +// returned a nil handler and Traefik logged the opaque router error +// "invalid handler type: ". Two independent header-validation checks were +// too strict: +// +// 1. validateTemplateSecure rejected the documented multi-valued-claim join +// pattern "{{range .. := .Claims.groups}}..." as a dangerous {{range}}. +// 2. Config.Validate required every header value to contain {{ }}, rejecting +// static/literal values (e.g. a shared proxy secret). +// +// These tests pin the fix while proving the security boundary still holds. + +// issue149BaseConfig returns a complete, valid config with no headers. Header +// cases are layered on per-test. No network is performed by Config.Validate. +func issue149BaseConfig() *Config { + return &Config{ + ProviderURL: "https://provider.example.com", + ClientID: "frigate-client", + ClientSecret: "frigate-client-secret", + CallbackURL: "/oauth2/callback", + SessionEncryptionKey: "0123456789abcdef0123456789abcdef0123456789", + RateLimit: 100, + Scopes: []string{"openid", "profile", "email", "groups"}, + } +} + +// TestIssue149_HeadersConfigValidates is the exact reproduction: the issue's +// three headers (after Traefik file-provider un-escaping) must now validate. +func TestIssue149_HeadersConfigValidates(t *testing.T) { + cfg := issue149BaseConfig() + cfg.Headers = []TemplatedHeader{ + {Name: "X-Forwarded-Preferred-Username", Value: `{{.Claims.preferred_username}}`}, + {Name: "X-Forwarded-Groups", Value: `{{range $i, $e := .Claims.groups}}{{if $i}},{{end}}{{$e}}{{end}}`}, + {Name: "X-Proxy-Secret", Value: `super-secret-shared-value`}, + } + if err := cfg.Validate(); err != nil { + t.Fatalf("issue #149 headers config must validate after the fix, got: %v", err) + } +} + +// TestIssue149_TemplateSecurityBoundary asserts the relaxed validator accepts +// the documented range/if join over claims while still blocking genuinely +// dangerous templates and access to non-whitelisted claims. +func TestIssue149_TemplateSecurityBoundary(t *testing.T) { + cases := []struct { + name string + template string + shouldFail bool + }{ + // --- allowed: documented, safe patterns --- + {"direct claim", `{{.Claims.preferred_username}}`, false}, + {"documented groups join", `{{range $i, $e := .Claims.groups}}{{if $i}},{{end}}{{$e}}{{end}}`, false}, + {"documented roles join", `{{range $i, $e := .Claims.roles}}{{if $i}},{{end}}{{$e}}{{end}}`, false}, + {"simple range over claims", `{{range .Claims.groups}}{{.}} {{end}}`, false}, + {"default over claim", `{{default "none" .Claims.email}}`, false}, + {"access token", `{{.AccessToken}}`, false}, + {"with over claim field", `{{with .Claims.email}}{{.}}{{end}}`, false}, + {"id token IdToken spelling", `{{.IdToken}}`, false}, // documented spelling (C6) + {"id token IDToken spelling", `{{.IDToken}}`, false}, // actual data key (C6) + + // --- still blocked: with over the bare Claims map leaks arbitrary fields --- + {"with over bare claims map", `{{with .Claims}}{{.password}}{{end}}`, true}, + + // --- still blocked: dangerous control/functions --- + {"range over non-claims data", `{{range .Items}}{{.}}{{end}}`, true}, + {"call function", `{{call .Func}}`, true}, + {"define template", `{{define "x"}}{{.}}{{end}}`, true}, + {"template inclusion", `{{template "other"}}`, true}, + {"printf", `{{printf "%s" .}}`, true}, + + // --- still blocked: non-whitelisted claim access, incl. via range --- + {"non-whitelisted claim direct", `{{.Claims.password}}`, true}, + {"non-whitelisted claim via range", `{{range $i, $e := .Claims.ssn}}{{$e}}{{end}}`, true}, + {"no recognized variable", `{{.Unknown}}`, true}, + + // --- still blocked: whole-Claims-map dump via range (issue #149 review, C1). + // A decoy {{.Claims.sub}} must NOT launder a bare-map range past the gate. + {"whole claims map dump via range", `{{range .Claims}}{{.}} {{end}}{{.Claims.sub}}`, true}, + {"claims keys dump via range assign", `{{range $k, $v := .Claims}}{{$k}} {{end}}{{.Claims.sub}}`, true}, + // --- still blocked: newline-obfuscated actions — the AST parser resolves + // whitespace, so these are just {{range}}/{{index}} and are rejected. + {"newline-obfuscated range over map", "{{\nrange .Claims}}{{.}}{{end}}{{.Claims.sub}}", true}, + {"newline-obfuscated index exfil", "{{\nindex .Claims \"password\"}}{{.Claims.sub}}", true}, + + // --- still blocked: whole-root / whole-Claims-map render (AST review). A + // whitelisted decoy {{.Claims.email}} must NOT launder a bare-map/root dump. + {"whole claims map render", `{{.Claims}}`, true}, + {"whole root render dot", `{{.}}`, true}, + {"whole root render dollar", `{{$}}`, true}, + {"root claims via dollar", `{{$.Claims}}`, true}, + {"decoy plus root dump", `{{.Claims.email}}{{.}}`, true}, + {"decoy plus map dump", `{{.Claims.email}}{{.Claims}}`, true}, + {"var-laundered map dump", `{{$x := .Claims}}{{$x}}{{.Claims.sub}}`, true}, + {"index builtin blocked", `{{index .Claims "password"}}`, true}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := validateTemplateSecure(tc.template, nil) + if tc.shouldFail && err == nil { + t.Fatalf("expected %q to be rejected, but it passed", tc.template) + } + if !tc.shouldFail && err != nil { + t.Fatalf("expected %q to be allowed, but it was rejected: %v", tc.template, err) + } + }) + } +} + +// TestIssue149_GroupsTemplateRendersForwardableValue proves the end goal: the +// documented groups join template parses, executes to a comma-separated string, +// and passes the runtime header sanitizer — so the header is actually emitted, +// not merely accepted by validation. Parse options mirror main.go:414. +func TestIssue149_GroupsTemplateRendersForwardableValue(t *testing.T) { + const tmplStr = `{{range $i, $e := .Claims.groups}}{{if $i}},{{end}}{{$e}}{{end}}` + + tmpl, err := template.New("X-Forwarded-Groups").Option("missingkey=zero").Parse(tmplStr) + if err != nil { + t.Fatalf("documented groups template must parse: %v", err) + } + + data := map[string]any{ + "Claims": map[string]any{ + "groups": []any{"admin", "users", "viewers"}, + }, + } + var buf bytes.Buffer + if err := tmpl.Execute(&buf, data); err != nil { + t.Fatalf("groups template must execute: %v", err) + } + if got := buf.String(); got != "admin,users,viewers" { + t.Fatalf("groups render mismatch: got %q want %q", got, "admin,users,viewers") + } + + // The comma-joined value must survive the runtime injection guard; commas + // are not control/bidi runes, so it forwards intact. + if reason := headerValueReason(buf.String(), headerTemplateMaxLen); reason != "" { + t.Fatalf("rendered groups header was dropped by sanitizer: %s", reason) + } +} + +// TestIssue149_GetEnforcesClaimsWhitelist proves the runtime `get` helper cannot +// exfiltrate a non-whitelisted claim, a raw token, or the whole data map — even +// though validateTemplateSecure accepts {{get ...}} syntactically. This is the +// defense-in-depth layer for C2/C3/C4 from the issue #149 security review. +func TestIssue149_GetEnforcesClaimsWhitelist(t *testing.T) { + data := map[string]any{ + "AccessToken": "AT-SECRET", + "IDToken": "IDT", + "IdToken": "IDT", + "RefreshToken": "RT-SECRET", + "Claims": map[string]any{ + "email": "e@x.com", + "password": "PW-SECRET", + "ssn": "123-45-6789", + }, + } + fns := headerTemplateFuncMap(nil) + render := func(tmplStr string) string { + tmpl := template.Must(template.New("h").Funcs(fns).Option("missingkey=zero").Parse(tmplStr)) + var b bytes.Buffer + if err := tmpl.Execute(&b, data); err != nil { + t.Fatalf("execute %q: %v", tmplStr, err) + } + return b.String() + } + + // Every exfil attempt must render empty (or the default fallback), never a secret. + leaky := map[string]string{ + "get non-whitelisted claim": `{{get .Claims "password"}}`, + "get ssn": `{{get .Claims "ssn"}}`, + "get raw token off root": `{{get . "AccessToken"}}`, + "get whole claims map": `{{get . "Claims"}}`, + "default wrapping get(ssn)": `{{default "none" (get .Claims "ssn")}}`, + } + for name, tmplStr := range leaky { + t.Run(name, func(t *testing.T) { + got := render(tmplStr) + for _, secret := range []string{"PW-SECRET", "AT-SECRET", "RT-SECRET", "123-45-6789"} { + if strings.Contains(got, secret) { + t.Fatalf("%q leaked %q (rendered %q)", tmplStr, secret, got) + } + } + }) + } + + // A whitelisted claim via get must still resolve. + if got := render(`{{get .Claims "email"}}`); got != "e@x.com" { + t.Fatalf(`get of whitelisted "email" must work, got %q`, got) + } +} + +// TestIssue149_IdTokenBothSpellingsRender pins C6: both {{.IdToken}} (documented) +// and {{.IDToken}} (runtime data key) resolve to the ID token, and both validate. +func TestIssue149_IdTokenBothSpellingsRender(t *testing.T) { + data := map[string]any{"IDToken": "IDT-REAL", "IdToken": "IDT-REAL"} + for _, tmplStr := range []string{`{{.IdToken}}`, `{{.IDToken}}`} { + if err := validateTemplateSecure(tmplStr, nil); err != nil { + t.Fatalf("%q must validate: %v", tmplStr, err) + } + tmpl := template.Must(template.New("h").Option("missingkey=zero").Parse(tmplStr)) + var b bytes.Buffer + if err := tmpl.Execute(&b, data); err != nil { + t.Fatalf("execute %q: %v", tmplStr, err) + } + if b.String() != "IDT-REAL" { + t.Fatalf("%q rendered %q, want IDT-REAL", tmplStr, b.String()) + } + } +} + +// TestIssue149_AllowedClaimsExtendsWhitelist pins the allowedClaims config +// option: a custom claim is rejected by default but validates and renders once +// listed, via both direct access and get. +func TestIssue149_AllowedClaimsExtendsWhitelist(t *testing.T) { + const custom = "employee_id" + + // Default (no allowedClaims): the custom claim is not forwardable. + base := issue149BaseConfig() + base.Headers = []TemplatedHeader{{Name: "X-Emp", Value: `{{.Claims.employee_id}}`}} + if err := base.Validate(); err == nil { + t.Fatalf("custom claim %q must be rejected without allowedClaims", custom) + } + + // With allowedClaims: direct access and get both validate. + cfg := issue149BaseConfig() + cfg.AllowedClaims = []string{custom} + cfg.Headers = []TemplatedHeader{ + {Name: "X-Emp", Value: `{{.Claims.employee_id}}`}, + {Name: "X-Emp-Get", Value: `{{get .Claims "employee_id"}}`}, + } + if err := cfg.Validate(); err != nil { + t.Fatalf("allowedClaims must permit %q, got: %v", custom, err) + } + + // The whitelist (validation) and the runtime get share the same effective set. + whitelist := claimsWhitelist(cfg.AllowedClaims) + if err := validateTemplateSecure(`{{get .Claims "employee_id"}}`, whitelist); err != nil { + t.Fatalf("get on allowed custom claim must validate: %v", err) + } + fns := headerTemplateFuncMap(whitelist) + tmpl := template.Must(template.New("h").Funcs(fns).Option("missingkey=zero").Parse(`{{get .Claims "employee_id"}}`)) + var b bytes.Buffer + if err := tmpl.Execute(&b, map[string]any{"Claims": map[string]any{"employee_id": "E-42"}}); err != nil { + t.Fatalf("render: %v", err) + } + if b.String() != "E-42" { + t.Fatalf("runtime get of allowed claim rendered %q, want E-42", b.String()) + } + + // A still-unlisted claim stays rejected even when another is allowed. + if err := validateTemplateSecure(`{{.Claims.ssn}}`, whitelist); err == nil { + t.Fatal("non-allowed claim ssn must still be rejected") + } +} + +// TestIssue149_StaticAndDelimiterHandling covers the Config.Validate header loop +// for literal values and malformed delimiters (validateTemplateSecure is not +// invoked for pure literals). +func TestIssue149_StaticAndDelimiterHandling(t *testing.T) { + cases := []struct { + name string + value string + shouldFail bool + errContains string + }{ + {"static literal secret", `super-secret-shared-value`, false, ""}, + {"static with equals/base64", `AAAA1234==`, false, ""}, + {"static with CRLF is rejected", "line1\r\nline2", true, "invalid characters"}, + {"unbalanced open only", `{{.Claims.email`, true, "unbalanced template delimiters"}, + {"unbalanced close only", `.Claims.email}}`, true, "unbalanced template delimiters"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + cfg := issue149BaseConfig() + cfg.Headers = []TemplatedHeader{{Name: "X-Test", Value: tc.value}} + err := cfg.Validate() + if tc.shouldFail { + if err == nil { + t.Fatalf("expected value %q to be rejected, but it passed", tc.value) + } + if tc.errContains != "" && !strings.Contains(err.Error(), tc.errContains) { + t.Fatalf("expected error to contain %q, got: %v", tc.errContains, err) + } + return + } + if err != nil { + t.Fatalf("expected value %q to be allowed, but it was rejected: %v", tc.value, err) + } + }) + } +} diff --git a/main.go b/main.go index 7e98f69..10bb662 100644 --- a/main.go +++ b/main.go @@ -83,6 +83,39 @@ var defaultExcludedURLs = map[string]struct{}{ "/favicon": {}, } +// headerTemplateFuncMap returns the function map available to custom header +// value templates, gated by the effective claims whitelist (built-in + +// config.AllowedClaims). It exposes exactly two helpers: +// - default: substitute a fallback when a value is nil/empty. +// - get: safe map access RESTRICTED to whitelisted claim keys, so it cannot be +// used to read a non-whitelisted claim, a raw token, or the whole data map +// (issue #149 review). This is the sole runtime enforcement of the claims +// whitelist for `get`; static validation cannot reliably parse its arguments. +func headerTemplateFuncMap(allowedClaims map[string]bool) template.FuncMap { + if allowedClaims == nil { + allowedClaims = safeClaimsFields + } + return 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 !allowedClaims[key] { + return "" + } + if mapVal, ok := m.(map[string]interface{}); ok { + if val, exists := mapVal[key]; exists { + return val + } + } + return "" + }, + } +} + // New creates a new TraefikOidc middleware instance. // It initializes all components including caches, HTTP clients, session management, // templates, and starts background processes for metadata discovery. @@ -124,6 +157,11 @@ func NewWithContext(ctx context.Context, config *Config, next http.Handler, name // any session. Traefik's yaegi plugin analyzer supplies a valid key via // .traefik.yml testData, so it passes; only misconfigured deployments fail. if err := config.Validate(); err != nil { + // Surface the concrete reason. When New() returns a nil handler, Traefik + // only logs the opaque router-level "invalid handler type: " and + // swallows this error, leaving operators no way to see WHY the plugin + // failed to build (issue #149). Log it explicitly at construction. + logger.Errorf("traefikoidc: invalid configuration, middleware not built: %v", err) return nil, fmt.Errorf("invalid configuration: %w", err) } // Setup HTTP client @@ -388,22 +426,7 @@ func NewWithContext(ctx context.Context, config *Config, next http.Handler, name t.headerTemplates = make(map[string]*template.Template) - 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 "" - }, - } + funcMap := headerTemplateFuncMap(claimsWhitelist(config.AllowedClaims)) for _, header := range config.Headers { tmpl := template.New(header.Name).Funcs(funcMap).Option("missingkey=zero") diff --git a/middleware.go b/middleware.go index 0dea547..84295b2 100644 --- a/middleware.go +++ b/middleware.go @@ -796,6 +796,7 @@ func (t *TraefikOidc) forwardAuthorized(rw http.ResponseWriter, req *http.Reques templateData := map[string]interface{}{ "AccessToken": p.AccessToken, "IDToken": p.IDToken, + "IdToken": p.IDToken, // documented spelling (README/CONFIGURATION); alias so {{.IdToken}} renders (issue #149 review) "RefreshToken": p.RefreshToken, "Claims": p.Claims, } diff --git a/regression/regression_test.go b/regression/regression_test.go index b66570b..25542dd 100644 --- a/regression/regression_test.go +++ b/regression/regression_test.go @@ -369,7 +369,10 @@ func testIssue60SafeTemplateFunctions(t *testing.T) { config.Headers = []traefikoidc.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") + // The template validator (template_validation.go) rejects these; Config.Validate + // wraps the reason with this stable prefix. Assert the behavior (rejected by + // the security validator) rather than a specific message word. + assert.Contains(t, err.Error(), "failed security validation", "should be rejected by the template security validator") } // Test all safe patterns from the documentation diff --git a/settings.go b/settings.go index db89c72..48ce5d5 100644 --- a/settings.go +++ b/settings.go @@ -9,32 +9,10 @@ import ( "net/http" "net/url" "os" - "regexp" "strconv" "strings" ) -// templateDelimWhitespace matches whitespace immediately inside Go template -// delimiters: after "{{" or before "}}". Go's text/template treats "{{ get", -// "{{ get" and "{{get" identically, so the security validator must collapse -// this whitespace before pattern-matching — otherwise a leading space both -// rejects valid templates (issue #148) and lets dangerous patterns such as -// "{{ range" slip past detection. -var templateDelimWhitespace = regexp.MustCompile(`\{\{[ \t]+|[ \t]+\}\}`) - -// normalizeTemplateDelimiters collapses inner-delimiter whitespace so that -// "{{ get .Claims \"x\" }}" becomes "{{get .Claims \"x\"}}" for matching. -// Trim markers ("{{-", "-}}") are left intact: the dash is not whitespace, so -// they remain detectable as dangerous patterns. -func normalizeTemplateDelimiters(templateStr string) string { - return templateDelimWhitespace.ReplaceAllStringFunc(templateStr, func(m string) string { - if strings.HasPrefix(m, "{{") { - return "{{" - } - return "}}" - }) -} - // TemplatedHeader represents a custom HTTP header with a templated value. // The value can contain template expressions that will be evaluated for each // authenticated request, such as {{.claims.email}} or {{.accessToken}}. @@ -77,8 +55,14 @@ type Config struct { AllowedUserDomains []string `json:"allowedUserDomains"` AllowedUsers []string `json:"allowedUsers"` Headers []TemplatedHeader `json:"headers"` - ExtraAuthParams map[string]string `json:"extraAuthParams,omitempty"` - RefreshGracePeriodSeconds int `json:"refreshGracePeriodSeconds"` + // AllowedClaims extends the built-in claims whitelist that header value + // templates may emit. Add the exact claim name (e.g. "employee_id") to allow + // {{.Claims.employee_id}} / {{get .Claims "employee_id"}}. Applies to both + // template validation and the runtime get helper. Built-in claims (email, + // groups, roles, realm_access, ...) need not be listed. + AllowedClaims []string `json:"allowedClaims,omitempty"` + ExtraAuthParams map[string]string `json:"extraAuthParams,omitempty"` + RefreshGracePeriodSeconds int `json:"refreshGracePeriodSeconds"` // MaxRefreshTokenAgeSeconds is a heuristic upper bound on the lifetime of // a stored refresh token. Once the token has been in the session longer // than this, requests treat it as expired up-front - returning 401 to @@ -552,7 +536,9 @@ func (c *Config) Validate() error { } } - // Validate headers configuration for template security + // Validate headers configuration for template security. Header templates may + // emit the built-in claims plus any operator-configured AllowedClaims. + allowedClaims := claimsWhitelist(c.AllowedClaims) for _, header := range c.Headers { if header.Name == "" { return fmt.Errorf("header name cannot be empty") @@ -560,8 +546,20 @@ func (c *Config) Validate() error { if header.Value == "" { return fmt.Errorf("header value template cannot be empty") } - if !strings.Contains(header.Value, "{{") || !strings.Contains(header.Value, "}}") { - return fmt.Errorf("header value '%s' does not appear to be a valid template (missing {{ }})", header.Value) + hasOpen := strings.Contains(header.Value, "{{") + hasClose := strings.Contains(header.Value, "}}") + if hasOpen != hasClose { + return fmt.Errorf("header value '%s' has unbalanced template delimiters (need both {{ and }})", header.Value) + } + if !hasOpen { + // Static/literal header value with no template actions (e.g. a shared + // proxy secret or a constant marker). Allowed as-is — it worked before + // the constructor started calling Validate() (issue #149). Reject only + // header-injection characters; the rendered value is emitted verbatim. + if strings.ContainsAny(header.Value, "\r\n\x00") { + return fmt.Errorf("header value for '%s' contains invalid characters (CR/LF/NUL)", header.Name) + } + continue } // Provide more helpful guidance for common template errors BEFORE security validation @@ -579,7 +577,7 @@ func (c *Config) Validate() error { } // Validate template syntax and security - if err := validateTemplateSecure(header.Value); err != nil { + if err := validateTemplateSecure(header.Value, allowedClaims); err != nil { return fmt.Errorf("header template '%s' failed security validation: %w", header.Value, err) } } @@ -587,196 +585,50 @@ func (c *Config) Validate() error { return nil } +// safeClaimsFields is the single source of truth for which claim fields a header +// template may emit. It gates both static validation (validateTemplateSecure) +// and the runtime `get` function (main.go funcMap), so a claim can never be +// forwarded via `get`/`index` that a direct {{.Claims.x}} reference would reject. +// Includes standard OIDC claims and common provider-specific claims. +var safeClaimsFields = map[string]bool{ + // Standard OIDC claims + "email": true, + "name": true, + "given_name": true, + "family_name": true, + "preferred_username": true, + "sub": true, + "iss": true, + "aud": true, + "exp": true, + "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 +} + // validateTemplateSecure validates template expressions for security vulnerabilities. // It checks for dangerous template patterns that could lead to code execution or data leaks // while allowing safe custom functions for field access and default values. -func validateTemplateSecure(templateStr string) error { - // Normalize inner-delimiter whitespace up front so that "{{ get .Claims }}" - // is validated identically to "{{get .Claims}}" (issue #148). All matching - // below operates on the normalized form, which also prevents a stray space - // from smuggling a dangerous pattern (e.g. "{{ range") past detection. - templateStr = normalizeTemplateDelimiters(templateStr) - - // 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 (except our safe ones) - "{{range", // Range over arbitrary data - "{{define", // Template definitions - "{{template", // Template inclusions - "{{block", // Block definitions - "{{/*", // Comments that could hide malicious code - "{{-", // Trim whitespace (could be used to obfuscate) - "-}}", // Trim whitespace (could be used to obfuscate) - "{{printf", // Printf functions - "{{print", // Print functions (but not our safe ones) - "{{println", // Println functions - "{{html", // HTML functions - "{{js", // JavaScript functions - "{{urlquery", // URL query functions - "{{index", // Index access to arbitrary data - "{{slice", // Slice operations - "{{len", // Length operations on arbitrary data - "{{eq", // Comparison operations - "{{ne", // Comparison operations - "{{lt", // Comparison operations - "{{le", // Comparison operations - "{{gt", // Comparison operations - "{{ge", // Comparison operations - "{{and", // Logical operations - "{{or", // Logical operations - "{{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 { - // 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) - } - } - - // Validate template variables against whitelist - allowedPatterns := []string{ - "{{.AccessToken}}", - "{{.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 - hasAllowedPattern := false - for _, pattern := range allowedPatterns { - if strings.Contains(templateStr, pattern) { - hasAllowedPattern = true - break - } - } - - if !hasAllowedPattern { - 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, - "family_name": true, - "preferred_username": true, - "sub": true, - "iss": true, - "aud": true, - "exp": true, - "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 - start := strings.Index(templateStr, "{{.Claims.") - for start != -1 { - end := strings.Index(templateStr[start:], "}}") - if end == -1 { - return fmt.Errorf("malformed Claims template syntax") - } - - // Extract the content between "{{.Claims." and "}}" - // start+10 skips "{{.Claims." and start+end is the position of "}}" - claimsContent := templateStr[start+10 : start+end] - - // Get the field name (first part before any dots) - fieldName := strings.Split(claimsContent, ".")[0] - - if !safeClaimsFields[fieldName] { - return fmt.Errorf("access to Claims.%s is not allowed for security reasons", fieldName) - } - - // Search for next occurrence - nextStart := strings.Index(templateStr[start+end+2:], "{{.Claims.") - if nextStart != -1 { - start = start + end + 2 + nextStart - } else { - start = -1 - } - } - } - - // Prevent code injection through template syntax - if strings.Contains(templateStr, "{{") && strings.Contains(templateStr, "}}") { - // Count opening and closing braces - openCount := strings.Count(templateStr, "{{") - closeCount := strings.Count(templateStr, "}}") - if openCount != closeCount { - return fmt.Errorf("unbalanced template braces") - } - } - - return nil -} +// validateTemplateSecure lives in template_validation.go (AST-based validation). // isValidSecureURL checks if a given string represents a valid, absolute HTTPS URL. // It uses url.Parse and checks for a nil error, an "https" scheme, and a non-empty host. diff --git a/template_validation.go b/template_validation.go new file mode 100644 index 0000000..0ae8a78 --- /dev/null +++ b/template_validation.go @@ -0,0 +1,362 @@ +package traefikoidc + +import ( + "fmt" + "strings" + "text/template/parse" +) + +// allowedRootTokenFields are the non-Claims values a header template may render +// directly from the root data context. Both ID-token spellings are accepted +// (templateData carries IDToken and IdToken). +var allowedRootTokenFields = map[string]bool{ + "AccessToken": true, + "IdToken": true, + "IDToken": true, + "RefreshToken": true, +} + +// validateTemplateParseFuncs is the function set the parser is told about. Only +// get/default exist; every other identifier used as a command (printf, call, +// index, js, ...) then fails to parse and is rejected — no blocklist needed. +var validateTemplateParseFuncs = map[string]any{ + "get": struct{}{}, + "default": struct{}{}, +} + +// claimsWhitelist returns the effective set of claim field names a header +// template may emit: the built-in safeClaimsFields plus any operator-configured +// extras (config.AllowedClaims). Blank/whitespace extras are ignored. The result +// gates BOTH static validation and the runtime get helper so the two never drift. +func claimsWhitelist(extra []string) map[string]bool { + m := make(map[string]bool, len(safeClaimsFields)+len(extra)) + for k := range safeClaimsFields { + m[k] = true + } + for _, c := range extra { + if c = strings.TrimSpace(c); c != "" { + m[c] = true + } + } + return m +} + +// templateValidator walks a parsed header template against a specific claims +// whitelist (built-in + operator-configured). Holding the set on the walker +// keeps it out of every method signature. +type templateValidator struct { + allowed map[string]bool +} + +// validateTemplateSecure parses a header value template and walks its AST, +// permitting ONLY: literal text; the whitelisted root token fields; whitelisted +// .Claims. access; range/with over a whitelisted .Claims.; the +// safe get/default helpers; and {{if}} control flow. Everything else — bare +// {{.}}/{{$}} (whole-root dump), bare {{.Claims}} (whole-map dump), non- +// whitelisted claims, unknown functions, define/template/block — is rejected. +// +// allowed is the effective claim whitelist (see claimsWhitelist); a nil set +// falls back to the built-in safeClaimsFields. +// +// Parsing (not substring matching) is what makes this sound: the parser resolves +// whitespace, comments and the meaning of "." per scope, so obfuscation and the +// "whitelisted decoy + bare-map render" bypass class cannot slip through. +func validateTemplateSecure(templateStr string, allowed map[string]bool) error { + if allowed == nil { + allowed = safeClaimsFields + } + trees, err := parse.Parse("headerTemplate", templateStr, "{{", "}}", validateTemplateParseFuncs) + if err != nil { + return fmt.Errorf("invalid template: %w", err) + } + // {{define}}/{{block}} produce additional named trees; a header value template + // must be a single anonymous template, so reject anything that defines more. + if len(trees) != 1 { + return fmt.Errorf("template definitions ({{define}}/{{block}}) are not allowed") + } + tree, ok := trees["headerTemplate"] + if !ok || tree.Root == nil { + return fmt.Errorf("invalid template: empty parse tree") + } + v := &templateValidator{allowed: allowed} + // dotIsRoot=true: "." is the whole data context at the top level. Inside a + // range/with over a claims field it rebinds to that (already-whitelisted) + // element, so relative access there is safe. + return v.walkNode(tree.Root, true) +} + +// walkNode validates a node and its children. dotIsRoot reports whether "." +// currently refers to the root data context (true) or to a whitelisted claim +// element rebound by an enclosing range/with (false). +func (v *templateValidator) walkNode(n parse.Node, dotIsRoot bool) error { + switch node := n.(type) { + case nil: + return nil + case *parse.ListNode: + if node == nil { + return nil + } + for _, child := range node.Nodes { + if err := v.walkNode(child, dotIsRoot); err != nil { + return err + } + } + return nil + case *parse.TextNode: + return nil + case *parse.ActionNode: + return v.validatePipe(node.Pipe, dotIsRoot) + case *parse.IfNode: + return v.walkBranch(node.Pipe, node.List, node.ElseList, dotIsRoot, dotIsRoot) + case *parse.WithNode: + // with rebinds "." to the target inside its body; the target must be a + // whitelisted claim, so the body runs with dotIsRoot=false. + if err := v.validateClaimSelector(node.Pipe, dotIsRoot, "with"); err != nil { + return err + } + return v.walkBranch(nil, node.List, node.ElseList, false, dotIsRoot) + case *parse.RangeNode: + // range iterates the target and rebinds "." to each element; the target + // must be a whitelisted claim collection, so the body is dotIsRoot=false. + if err := v.validateClaimSelector(node.Pipe, dotIsRoot, "range"); err != nil { + return err + } + return v.walkBranch(nil, node.List, node.ElseList, false, dotIsRoot) + case *parse.TemplateNode: + return fmt.Errorf("template inclusion is not allowed") + default: + return fmt.Errorf("unsupported template construct: %s", n.String()) + } +} + +// walkBranch validates an if/with/range: an optional condition pipe (evaluated +// in the outer scope), the body (bodyDotIsRoot), and the else branch (outer +// scope). For with/range the condition pipe is validated by the caller and +// condPipe is nil here. +func (v *templateValidator) walkBranch(condPipe *parse.PipeNode, body, elseList *parse.ListNode, bodyDotIsRoot, outerDotIsRoot bool) error { + if condPipe != nil { + if err := v.validatePipe(condPipe, outerDotIsRoot); err != nil { + return err + } + } + if err := v.walkNode(body, bodyDotIsRoot); err != nil { + return err + } + if elseList != nil { + if err := v.walkNode(elseList, outerDotIsRoot); err != nil { + return err + } + } + return nil +} + +// validatePipe validates every command in a pipeline, including the right-hand +// side of any variable declaration ($x := ...). Declared vars therefore always +// hold already-validated values, so later references to them are safe. +func (v *templateValidator) validatePipe(pipe *parse.PipeNode, dotIsRoot bool) error { + if pipe == nil { + return nil + } + for _, cmd := range pipe.Cmds { + if err := v.validateCommand(cmd, dotIsRoot); err != nil { + return err + } + } + return nil +} + +// validateCommand validates a single command (one stage of a pipeline). The +// first argument may be the get/default function; the rest, and every argument +// of a non-function command, must be a safe value expression. +func (v *templateValidator) validateCommand(cmd *parse.CommandNode, dotIsRoot bool) error { + if cmd == nil || len(cmd.Args) == 0 { + return nil + } + if id, ok := cmd.Args[0].(*parse.IdentifierNode); ok { + return v.validateFunctionCall(id.Ident, cmd.Args[1:], dotIsRoot) + } + for _, arg := range cmd.Args { + if err := v.validateValue(arg, dotIsRoot); err != nil { + return err + } + } + return nil +} + +// validateFunctionCall allows only get/default. get's map argument must be a +// claims reference (never the root) and default's arguments are validated as +// values; the runtime get additionally enforces the key whitelist. +func (v *templateValidator) validateFunctionCall(name string, args []parse.Node, dotIsRoot bool) error { + switch name { + case "default": + for _, arg := range args { + if err := v.validateValue(arg, dotIsRoot); err != nil { + return err + } + } + return nil + case "get": + if len(args) < 1 { + return fmt.Errorf("get requires a map argument") + } + // The map argument must reference .Claims, never the root map (which + // would expose tokens / the whole Claims map). Remaining args (the key, + // possibly a pipeline) are validated as values. + if err := v.validateClaimsReference(args[0], dotIsRoot); err != nil { + return fmt.Errorf("get is only allowed on .Claims: %w", err) + } + // A literal key must be whitelisted (matches direct .Claims. + // strictness); a dynamic key is enforced by the runtime get. + if len(args) >= 2 { + if key, ok := args[1].(*parse.StringNode); ok && !v.allowed[key.Text] { + return fmt.Errorf("access to Claims.%s is not allowed for security reasons", key.Text) + } + } + for _, arg := range args[1:] { + if err := v.validateValue(arg, dotIsRoot); err != nil { + return err + } + } + return nil + default: + return fmt.Errorf("function %q is not allowed in header templates", name) + } +} + +// validateValue validates an expression that may be rendered or passed as an +// argument. It rejects anything resolving to the whole root or the whole Claims +// map, and any non-whitelisted claim. +func (v *templateValidator) validateValue(node parse.Node, dotIsRoot bool) error { + switch val := node.(type) { + case *parse.StringNode, *parse.NumberNode, *parse.BoolNode, *parse.NilNode: + return nil + case *parse.DotNode: + // "." is the whole root at the top level (dumps tokens + all claims); it + // is a single already-whitelisted claim element inside a range/with body. + if dotIsRoot { + return fmt.Errorf("rendering the whole data context ({{.}}) is not allowed") + } + return nil + case *parse.FieldNode: + return v.validateFieldAccess(val.Ident, dotIsRoot) + case *parse.VariableNode: + return v.validateVariableAccess(val.Ident) + case *parse.PipeNode: + return v.validatePipe(val, dotIsRoot) + case *parse.ChainNode: + // (base).Field.Field — the field chain only narrows access, so validating + // the base value is sufficient. + return v.validateValue(val.Node, dotIsRoot) + default: + return fmt.Errorf("unsupported template expression: %s", node.String()) + } +} + +// validateFieldAccess validates a dot-relative field access ".A.B.C" given +// whether "." is the root. In an element scope (dotIsRoot=false) the element is +// already a whitelisted claim, so any sub-field access is safe. +func (v *templateValidator) validateFieldAccess(idents []string, dotIsRoot bool) error { + if !dotIsRoot { + return nil + } + if len(idents) == 0 { + return fmt.Errorf("rendering the whole data context is not allowed") + } + root := idents[0] + if allowedRootTokenFields[root] { + return nil + } + if root == "Claims" { + if len(idents) < 2 { + return fmt.Errorf("rendering the whole Claims map ({{.Claims}}) is not allowed") + } + if !v.allowed[idents[1]] { + return fmt.Errorf("access to Claims.%s is not allowed for security reasons", idents[1]) + } + // By design, only the top-level claim is whitelisted; deeper subfields + // (idents[2:]) of a map-valued whitelisted claim such as realm_access / + // resource_access are forwarded as-is. Whitelisting a map claim means + // opting into its nested contents, and the documented Keycloak pattern + // {{with .Claims.realm_access}}{{.roles}}{{end}} relies on it; nested + // provider keys can't be enumerated to whitelist individually. + return nil + } + return fmt.Errorf("access to .%s is not allowed in header templates", root) +} + +// validateVariableAccess validates a $-variable reference. The bare root ("$" +// and "$.x") is treated as a root field access; a named range/with/declaration +// variable holds an already-validated value, so it and its sub-fields are safe. +func (v *templateValidator) validateVariableAccess(idents []string) error { + if len(idents) == 0 { + return nil + } + if idents[0] == "$" { + // "$" is the root: "$" alone dumps everything; "$.Claims.email" is a + // root-absolute field access validated like ".Claims.email". + if len(idents) == 1 { + return fmt.Errorf("rendering the whole data context ({{$}}) is not allowed") + } + return v.validateFieldAccess(idents[1:], true) + } + // Named variable ($e, $i, $x): holds a validated element/index/value. + return nil +} + +// validateClaimsReference requires a node to reference .Claims (or $.Claims), +// never the root map — used for get's map argument, whose key is separately +// enforced against the whitelist at render time. +func (v *templateValidator) validateClaimsReference(node parse.Node, dotIsRoot bool) error { + switch ref := node.(type) { + case *parse.FieldNode: + if dotIsRoot && len(ref.Ident) >= 1 && ref.Ident[0] == "Claims" { + if len(ref.Ident) >= 2 && !v.allowed[ref.Ident[1]] { + return fmt.Errorf("access to Claims.%s is not allowed for security reasons", ref.Ident[1]) + } + return nil + } + case *parse.VariableNode: + if len(ref.Ident) >= 2 && ref.Ident[0] == "$" && ref.Ident[1] == "Claims" { + if len(ref.Ident) >= 3 && !v.allowed[ref.Ident[2]] { + return fmt.Errorf("access to Claims.%s is not allowed for security reasons", ref.Ident[2]) + } + return nil + } + } + return fmt.Errorf("expected a .Claims reference") +} + +// validateClaimSelector validates the target of a range/with: it must iterate a +// specific whitelisted claim field (.Claims. or $.Claims.), never +// the bare Claims map or any non-claims value. +func (v *templateValidator) validateClaimSelector(pipe *parse.PipeNode, dotIsRoot bool, kind string) error { + if pipe == nil || len(pipe.Cmds) == 0 { + return fmt.Errorf("%s requires a .Claims. target", kind) + } + cmd := pipe.Cmds[len(pipe.Cmds)-1] + if len(cmd.Args) == 0 { + return fmt.Errorf("%s requires a .Claims. target", kind) + } + target := cmd.Args[0] + if err := v.validateClaimField(target, dotIsRoot); err != nil { + return fmt.Errorf("%s is only allowed over a whitelisted claims field, e.g. {{range $i, $e := .Claims.groups}} or {{with .Claims.email}}: %w", kind, err) + } + return nil +} + +// validateClaimField requires a node to be a specific whitelisted claim field +// (.Claims. or $.Claims.) — stricter than +// validateClaimsReference, which also permits the bare .Claims map for get. +func (v *templateValidator) validateClaimField(node parse.Node, dotIsRoot bool) error { + switch field := node.(type) { + case *parse.FieldNode: + if dotIsRoot && len(field.Ident) >= 2 && field.Ident[0] == "Claims" && v.allowed[field.Ident[1]] { + return nil + } + case *parse.VariableNode: + if len(field.Ident) >= 3 && field.Ident[0] == "$" && field.Ident[1] == "Claims" && v.allowed[field.Ident[2]] { + return nil + } + } + return fmt.Errorf("not a whitelisted .Claims.") +}