http_client_pool.go SharedTransportPool.GetOrCreateTransport built the shared
http.Transport with hardcoded MaxConnsPerHost=5, MaxIdleConnsPerHost=2,
MaxIdleConns=10, IdleConnTimeout=30s while HTTPClientConfig's versions of
those fields drove configKey (cache identity) and were validated and tuned
per preset (Default: 20/10/50/30s; OIDCProvider: 50/25/100/90s). The
config was thus valid, keyed, and validated but silently ignored in the
transport used by main.go (CreatePooledHTTPClient) - dead config that left
OIDC-provider connection tuning without effect. The sibling factory path
(http_client_factory.go) already applied these values.
Now the pooled transport derives MaxConnsPerHost, MaxIdleConnsPerHost,
MaxIdleConns, and IdleConnTimeout from the config, consistent with the
factory path and with configKey.
Test: added TestSharedTransportPoolAppliesConfigLimits, which fails on the
old hardcoded code (fixed values) and passes now.
Verified: full source-order suite, full -race -count=1 -shuffle=on ./...
(all packages), golangci-lint run ./... (0 issues).
error_recovery.go: ResetTimeout was copied into the CircuitBreaker but never
consulted by the state machine (used only for a metric), so the documented
"how long to wait before fully closing the circuit" (reset_timeout, default
30s) had no effect: Open->HalfOpen used Timeout (60s), and a single
half-open success closed the circuit immediately, losing the intended
open -> half-open -> (reset cooldown) -> closed hysteresis and letting a
stray success immediately reopen (bounce).
Now the circuit remains half-open after entering it until resetTimeout has
elapsed; only then can a successful request fully close it. Half-open
requests are still allowed as probes. Default behavior is more
conservative (up to Timeout+ResetTimeout before full close).
Tests: updated TestCircuitBreakerHalfOpenTransition to the corrected
semantics; added TestCircuitBreakerResetTimeoutEnforced (fails on the old
dead-field code, where resetTimeout was ignored).
Verified: full source-order suite, full -race -count=1 -shuffle=on ./...
(all packages), golangci-lint run ./... (0 issues).
Four verified fixes from the round-36 agent team (each confirmed against source
before acting):
- settings.go: Config.Validate now skips the client-credential block when
Dynamic Client Registration is enabled and no clientID is pre-provisioned
(RFC 7591 provides credentials at registration). Previously the
unconditional 'clientID is required' made DCR unreachable via New.
- dynamic_client_registration.go: default registered token_endpoint_auth_method
to client_secret_post to match the plugin's runtime default; an enforcing
IdP would otherwise reject the plugin's token exchange.
- utilities.go: gate the shared per-provider singleton-metadata-refresh task
stop on lastInstance (matching singleton-token-cleanup) so one instance's
tear down no longer kills 2h metadata refresh for a surviving sibling.
- token_manager.go: stop self-recording the JTI into the per-instance
tokenBlacklist on every verify. On the cookie path the same valid token is
re-presented per request; an LRU eviction of the raw-token cache (MaxSize
1000 / 5 MiB) then turned a legitimate cache miss into a false 'token
replay detected'. Replay tracking stays in the shared shardedReplayCache.
Regression tests: new review_r36_regression_test.go (4 tests); updated
dynamic_client_registration_test.go and security_edge_cases_test.go which had
encoded the old defaults.
Verified: full source-order suite, full -race -count=1 -shuffle=on ./...
(all packages), and golangci-lint run ./... (0 issues).
Concurrency/lifecycle (P1, highest-value):
- singleton_resources.go: RegisterBackgroundTask now REPLACES a stopped
task instead of reusing it. BackgroundTask.Start is sync.Once-guarded,
so a task stopped on instance close and re-registered (Traefik
close+recreate for the same provider) was permanently dead: IsTaskRunning
returned false (started && !stopped) so callers retried a start that
was a no-op. Metadata refresh, token cleanup, and health checks now
resume across a recreate cycle.
- error_recovery.go: the shared graceful-degradation health task was
bound to the FIRST instance's performHealthChecks (later instances'
checks never ran) and any instance's Close stopped it for all. Added a
global live-instance registry; the shared task runs every instance's
checks and is stopped only when the last instance closes (mirrors the
singleton-token-cleanup gate).
Storage/cache:
- hybrid.go recordL2Error: Store(now) then immediately compared
time.Since(t)<1s against its own write => ALWAYS fired, so a single
transient L2 error flipped the whole cache into fallback (full L2
bypass -> upstream stampede). Now enters fallback only on a PRIOR
error within the window (true "multiple errors"). Updated two tests
that had encoded the old single-error-fallback bug.
- redis_pool.go: ConnectionPool.Put checked closed but sent on
p.connections without holding p.mu; Close closed it under p.mu.
Window => 'send on closed channel' panic at shutdown. Put now holds
p.mu (the lock only Close used).
HTTP/transport:
- jwk.go fetchJWKS: added an explicit >1 MiB guard (was silent partial
read, could yield a truncated keyset and spurious missing-kid).
- http_client_pool.go configKey: now includes every transport-behavior
field it actually consumes (compression, HTTP2, keep-alives, timeouts,
buffer sizes, dial) so behaviorally different configs no longer share
one transport built with the first config's settings.
Each fix has a regression test. Verified: full source-order suite green,
golangci-lint 0 issues, 3x full -race -count=1 -shuffle=on ./... all exit 0.
GetResourceManager returned the process-global via an unprotected read
(singleton_resources.go:50), while resetResourceManagerForTesting and
ResetResourceManager write it under resourceManagerMutex. A live background
task (graceful-degradation health check started by one test) reading the
singleton while a shuffled test resets it was a data race (found via
-race -shuffle: Write at singleton_resources_test.go:941/942 vs Read at
autocleanup.go:554 / error_recovery.go:910).
Fix: hold resourceManagerMutex around the read, matching the sibling
GetGlobalTaskRegistry (autocleanup.go:414). Added a deterministic
concurrent regression test (GetResourceManager vs reset under -race).
Verified: 4x full -race -count=1 -shuffle=on ./... all exit 0, 0 races,
0 fails; source-order suite green; lint clean.
- TransportPool configKey: write conn-count ints verbatim (strconv) instead of
byte() truncation — a config value >=256 previously truncated and made two
distinct configs collide on one key, silently sharing a transport with the
wrong settings (real fix, gosec G115).
- Document design-intentional gosec findings with //nolint:gosec + rationale:
G117 (clients_secret intentionally marshaled into stored creds JSON),
G704 (tokenURL is the trusted discovery endpoint), G124 (delete/test cookie,
Secure set conditionally), G118 (long-lived background refresh) and pool
utils short-ID rune, testutil mock server redirects.
Result: full-tree golangci-lint now reports 0 issues (was ~12). Build, vet,
and the full test suite remain green.
Issue #152 asks for a manual introspection endpoint because some IdPs
omit introspection_endpoint from discovery. Analysis found the two
documented overrides revocationURL and oidcEndSessionURL were dead
code: the 1b49e13 rebuild dropped their constructor wiring.
Add the IntrospectionURL config field and wire all three overrides.
The overrides apply inside updateMetadataEndpoints, so they survive
every metadata refresh. A manual value always wins over discovery.
The constructor also seeds the live fields, so a failed discovery
does not lose operator configuration.
Manual values skip the poisoned-discovery defences on purpose. They
sit at the same trust tier as providerURL and isValidSecureURL gates
them at config time.
Add issue152_regression_test.go: 8 tests pin precedence, refresh
survival, cold-start seeding, end-to-end introspection over HTTP,
and the validation contract.
Issue #154: a providerURL that points at a localhost IdP caused an
infinite redirect loop. validateHost rejected loopback hosts
unconditionally, so buildAuthURL returned an empty string.
Derive allowLoopbackHosts from a loopback providerURL at construction
time. Discovery validation already used this derivation, so the two
validation layers now agree. This adds no new config surface.
Cloud metadata hosts (169.254.169.254, metadata.google.internal),
link-local, and unspecified addresses stay blocked unconditionally.
Non-loopback providers keep identical behaviour.
Add issue154_regression_test.go: 8 tests pin the new allowance, the
default block, and the security envelope.
The install snippet in the README still pinned v0.7.10 and the example
configs v0.8.0, so anyone following the docs installed a release far
behind the current v1.0.29.
Co-authored-by: Markus Renschler <[email protected]>
The plugin catalog analyzer failed v1.0.28 with "invalid handler
type: <nil>" (#151). Two defects combined.
The analyzer feeds .traefik.yml testData to New() verbatim. The header
values used quadruple braces on a false premise: no configuration path
collapses them. Template validation rejected them, so New() failed. The
testData now carries plain, valid templates.
New() also returned failures through a multi-value tail call
(return NewWithContext(...)). Yaegi v0.16.1 zeroes both results of that
statement on the error path. The analyzer therefore saw (nil, nil) and
reported the opaque handler-type message instead of the real validation
error. New() now splits the return explicitly. Construction failures
surface as real error messages under yaegi and in Traefik logs.
The regression tests pin both fixes. README and CONFIGURATION.md now
give correct escaping guidance: plain double braces everywhere; file
provider users escape with a raw string. Quadruple braces do not work
anywhere.
Fixes#151.
* fix(settings): allow documented range + static header templates (#149)
Upgrading past the release where NewWithContext began calling
config.Validate() (546ceb9) broke `headers` configs that worked in 0.8.25:
New() returned a nil handler and Traefik logged the opaque "invalid handler
type: <nil>". Two header-validation checks were stricter than the documented,
legitimate usage:
- validateTemplateSecure rejected the documented multi-valued-claim join
"{{range $i,$e := .Claims.groups}}...{{end}}" as a dangerous {{range}},
even though README/CONFIGURATION show exactly that for groups/roles.
- Config.Validate required every header value to contain {{ }}, rejecting
static/literal values such as a shared proxy secret.
Fixes:
- Allow static/literal header values (reject only CR/LF/NUL header-injection
chars); flag unbalanced {{ }} explicitly.
- Allow {{range}} only when its clause targets .Claims.<field>; a range over
the bare .Claims map stays blocked and a decoy {{.Claims.x}} elsewhere no
longer launders it. Generalize the claims whitelist scan to .Claims. so a
claim reached via range/with/default is whitelisted identically to a direct
{{.Claims.x}} reference.
- normalizeTemplateDelimiters now collapses ALL whitespace inside {{ }}
(newline/CR/FF too), closing a newline-obfuscation bypass ({{\nrange,
{{\nindex) found in review.
- Log the concrete validation reason at construction so operators see WHY the
middleware failed to build instead of only "invalid handler type: <nil>".
Adds issue149_regression_test.go: pins the exact issue config, the documented
groups template rendering "admin,users" past the runtime sanitizer, static and
delimiter edge cases, and the security boundary (whole-Claims-map dump via
range, non-whitelisted claims, call/define/template/printf, and
newline-obfuscated actions all rejected).
* fix(settings): AST-based header template validation, close whitelist bypasses
Follow-up hardening to the #149 work. An adversarial review showed the
substring-based validateTemplateSecure could not actually bound what a header
template reads: get/index with arbitrary keys, {{with .Claims}} field access,
and a whitelisted-decoy + bare {{.}}/{{$}}/{{.Claims}} render all leaked the
whole root (every raw token) or the whole Claims map past validation. The core
problem is structural — the meaning of "." depends on scope, which substrings
cannot see.
Replace the substring validator with an AST walk (text/template/parse):
- Parse with only get/default known; every other function (printf, call, index,
js, urlquery, ...) then fails to parse and is rejected — no blocklist.
- Reject {{define}}/{{block}} (they produce extra trees).
- Walk the tree tracking whether "." is the root. Permit ONLY: literal text;
the four root token fields (AccessToken, IdToken, IDToken, RefreshToken);
whitelisted .Claims.<field>; range/with over a whitelisted .Claims.<field>
("." inside the body is the already-whitelisted element); get on .Claims with
a whitelisted literal key; default over safe values; {{if}} control flow and
$-scoped range/with/decl variables. Everything else — bare {{.}} / {{$}}
(whole-root dump), bare {{.Claims}} (whole-map dump), non-whitelisted claims,
$.Claims root reach — is rejected.
Also closes the get/default exfil and the with-over-bare-map vectors, and fixes
ID-token forwarding: templateData now carries both IDToken and IdToken and the
validator accepts {{.IdToken}} (documented) and {{.IDToken}}; previously the
documented spelling rendered empty and the working one was rejected.
Runtime `get` additionally enforces the claims whitelist on its key, and
safeClaimsFields is promoted to a package-level single source of truth shared by
the validator and the get helper. The header template funcMap is extracted to
headerTemplateFuncMap() so tests exercise the exact production map. Removes the
now-dead normalizeTemplateDelimiters/regex (the parser handles whitespace).
Extends issue149_regression_test.go: renders through the real funcMap to prove
get cannot leak; asserts the whole-root/whole-map/decoy/var-launder/index
vectors are rejected while every documented pattern (groups/roles join,
with .Claims.email, get .Claims "internal_role", default, both IdToken
spellings) still validates and renders.
* feat(settings): allowedClaims to extend the header-template claim whitelist
The AST validator and the runtime `get` only permit a fixed set of claim fields
in header value templates. Providers issue custom claims (e.g. employee_id) that
aren't built in, and there was no supported way to forward them — the previous
`get` escape hatch was closed by the security hardening.
Add `allowedClaims []string` to Config. Listed names extend the effective
whitelist for BOTH static validation and the runtime `get` (a single
claimsWhitelist() = built-in safeClaimsFields + AllowedClaims, so the two never
drift). Built-in claims need not be listed; unlisted claims stay rejected.
allowedClaims:
- employee_id
headers:
- name: X-Employee-Id
value: "{{`{{.Claims.employee_id}}`}}"
Implementation: the whitelist is threaded through the validator (now a small
templateValidator struct carrying the allowed set) and headerTemplateFuncMap
instead of the package global, so it is per-config with no shared mutable state.
Docs: README and docs/CONFIGURATION document the whitelist, the validation
behaviour, and allowedClaims (grounded in safeClaimsFields).
Tests: TestIssue149_AllowedClaimsExtendsWhitelist pins that a custom claim is
rejected by default and validates + renders once listed, via both direct access
and get, while an unlisted claim stays rejected. A regression assertion that
coupled to the old validator's "dangerous" wording now checks the stable
"failed security validation" wrapper (the templates are still rejected).
validateTemplateSecure matched safe functions with literal strings.Contains
("{{get "), so a space after {{ ("{{ get") failed the whitelist and
Config.Validate() rejected the config, surfacing as Traefik's opaque
"invalid handler type: <nil>". Go's text/template treats {{ get and {{get
identically, so the validator was stricter than the engine.
Normalize inner-delimiter whitespace before matching. This also closes a
security gap where a leading space let dangerous patterns ("{{ range",
"{{ call") evade detection.
Adds unit + regression coverage.
* fix(security): encrypt session cookies + fail closed on invalid config
Batch 1 of security audit remediation (ranks 1, 2, 6).
- session.go: derive independent HMAC + AES-256 keys via stdlib HKDF-SHA256
and build the gorilla cookie store with both, so session cookies are now
encrypted, not merely signed. The single-key store previously left OIDC
access/refresh/ID tokens recoverable from raw cookie bytes. Cookie format
changes, so existing sessions are invalidated on deploy (one-time re-login).
- main.go: call config.Validate() at construction and error out on failure,
instead of silently substituting a public hardcoded encryption key for
empty/short keys (which allowed session forgery). The yaegi analyzer
passes via .traefik.yml testData.
- settings.go: isValidSecureURL permits plaintext HTTP for loopback hosts
only (RFC 8252); remote providers must still use HTTPS.
- tests: complete configs that did not satisfy Validate(); add regression
tests in security_audit_fixes_test.go.
Configs below documented minimums (rateLimit < 10, key < 32 chars) are now
rejected at startup (fail closed).
* fix(security): validate discovered OIDC endpoints + pin introspection host
Batch 2 of security audit remediation (ranks 3, 4).
- url_helpers.go: add validateDiscoveredEndpoint, an SSRF screen for endpoints
taken from the provider discovery document (jwks_uri, token, authorization,
revocation, end_session, introspection, registration). Blocks link-local
(cloud metadata 169.254.169.254), multicast, unspecified and private
addresses (unless allowPrivateIPAddresses); blocks loopback unless the
configured providerURL is itself loopback (dev/test). Cross-domain JWKS
hosts (e.g. Google) stay allowed. Add sameHost helper.
- main.go: updateMetadataEndpoints screens every discovered endpoint and
blanks any that fail (fail closed downstream). The introspection endpoint
carries the client secret via HTTP Basic, so it is additionally pinned to
the providerURL host to stop a poisoned discovery document exfiltrating the
secret to an attacker-controlled host.
- tests: regression tests for the SSRF guard and the host pin.
* fix(security): close open redirects + anchor excluded-URL matching
Batch 3 of security audit remediation (ranks 5, 14, 15).
- auth_flow.go: run the stored incoming path through normalizeLogoutPath
before using it as the post-login redirect, so //evil.com and /\evil.com
payloads become host-relative (open-redirect, rank 5).
- url_helpers.go: excluded-URL matching is anchored at a natural boundary
(exact, sub-path "/", or file extension "."), so excluding "/public" no
longer also bypasses auth on "/publicsecret"; "/favicon" still matches
"/favicon.ico" (rank 14).
- internal/utils: X-Forwarded-Host is sanitized (first value only; reject
CRLF/whitespace/multi-value) before building redirect URLs (rank 15).
- helpers.go: the logout redirect used when there is no provider end-session
endpoint is host-relative, never an absolute URL derived from the
client-controllable request host (logout open-redirect, rank 15).
- tests: update two logout cases that asserted the old absolute redirect;
add regression tests.
* fix(security): reject unverified Azure tokens; fix transport TLS reuse
Batch 4 of security audit remediation (ranks 7, 11).
- token_validation_rs.go: an Azure nonce-bearing access token that cannot be
cryptographically verified no longer returns "authenticated" when there is
no ID token to corroborate it; it refreshes (if possible) or forces
re-authentication instead of failing open (rank 7).
- http_client_pool.go: the at-limit transport-reuse path now takes the write
lock before mutating refCount (fixes a data race) and only reuses a
transport whose TLS settings (CA pool + InsecureSkipVerify) match the
caller's, never one with a different trust store; if none matches it returns
nil so the caller falls back to a verifying default transport (rank 11).
- tests: add a transport-pool TLS-isolation regression test.
* fix(security): stop logging templated header values (token leak)
Batch 5 of security audit remediation (rank 16).
middleware.go: templated downstream headers commonly carry the access token
(e.g. "Authorization: Bearer {{.AccessToken}}"). The debug log line printed
the full header value, leaking credentials into logs. Log the header name and
byte length instead.
* fix(security): cache-key collision, cache-config divergence, fleet cleanup
Batch 6 of security audit remediation (ranks 9, 10, 12).
- token_manager.go: detectTokenType keys its cache on a SHA-256 hash of the
full token instead of the first 32 chars (which are only the base64url JWT
header). Distinct tokens sharing alg+kid no longer collide and get
mis-classified (rank 10).
- cache_manager.go: the process-global cache manager is initialized once and
shared across plugin instances; it now logs a loud warning when a later
instance requests a different explicit Redis backend that is silently
ignored, surfacing the cross-instance state-isolation hazard (rank 9).
- singleton_resources.go / main.go / utilities.go: track a process-global live
instance count; the shared singleton-token-cleanup task is stopped only when
the LAST instance shuts down, so one instance's Close() (e.g. a config reload)
no longer kills cleanup for surviving instances (rank 12).
- tests: update TestDetectTokenTypeCaching for the new key; add regression tests.
* fix(security): bound introspection cache + cookie lifetime to config
Batch 7 of security audit remediation (ranks 8, 13).
- token_introspection.go: when requireTokenIntrospection is enabled, cap the
positive introspection-result cache at 30s (instead of 5m) so a token
revoked at the provider stops passing within ~30s, matching the operator's
near-real-time revocation expectation (rank 8).
- session.go: bind the cookie store's MaxAge to the configured sessionMaxAge,
so the cookie codec's cryptographic timestamp validity is no longer fixed at
gorilla's 30-day default; a stolen cookie is valid only for the configured
session lifetime (rank 13).
- tests: add a cookie-lifetime regression test.
* fix(security): low-severity hardening (cache, DoS caps, PKCE, throttle)
Batch 8 of security audit remediation — low severity
(ranks 24, 25, 27, 29, 31, 36, 37, 41, 45, 46, 49).
- universal_cache.go: updateLocalCache updates an existing key in place instead
of orphaning its LRU element and double-counting currentSize/currentMemory
(rank 36 — the only production-reachable bug in this batch).
- jwk.go / metadata_cache.go / token_introspection.go: bound response bodies
with io.LimitReader (1 MiB) to prevent memory exhaustion from a hostile or
buggy provider (ranks 24, 25).
- jwk.go: skip JWKs not usable for signature verification (use != sig, or
key_ops without "verify") when building the key set (rank 49).
- auth_flow.go: fail closed at the callback when PKCE is enabled but the code
verifier is missing, instead of silently dropping it (rank 27).
- utilities.go / main.go: match allowedUserDomains case-insensitively (rank 31).
- bearer_auth.go: a single success no longer wipes an active per-IP penalty;
the counter resets only when no penalty is in effect (rank 29).
- main.go: handle (not discard) the NewSessionManager error (rank 37).
- error_recovery.go: take a write lock in isServiceDegraded (it deletes from a
map); compare retryable-error substrings case-insensitively (ranks 45, 46).
- singleton_resources.go: bind the generic-cache cleanup goroutine to the
resource-manager shutdown channel so it cannot outlive its owner (rank 41).
- tests: update the bearer throttle test to the corrected penalty semantics.
* fix(security): header sanitization, issuer pinning, fail-closed paths
Batch 9 of security audit remediation (ranks 18, 19, 20, 21, 22, 30, 33, 34).
- middleware.go / bearer_auth.go: sanitize claim-derived values on the cookie
auth path before injecting them into downstream headers. Drop group/role and
identifier values containing control chars, bidi-override runes, or the
, ; = delimiters (a comma would inject phantom entries into X-User-Groups);
reject control/bidi/over-length in rendered templated header output (but
permit , ; = in free-form values such as a bearer token). The bearer path
already sanitized; the cookie path did not (ranks 33, 34).
- main.go / metadata_cache.go: pin the discovered issuer to the configured
provider host (sameHost) and refuse/never-cache a mismatch, so a poisoned
discovery document cannot redefine the JWT trust anchor (ranks 21, 22).
- token_introspection.go: when a distinct API audience is configured, fail
closed on a missing or mismatched introspection audience; aud parsed as
string-or-array per RFC 7662 (rank 19).
- logout.go: front-channel logout requires a matching issuer; an empty iss is
rejected (blocks unauthenticated forced-logout via a known sid) (rank 30).
- token_validation_rs.go: an opaque access token with no ID token and no
successful introspection fails closed (re-auth) instead of authenticating
(ranks 18, 20).
- tests: realistic same-host provider mocks; regression tests for the header
sanitization distinction and the fail-closed paths.
* chore(security): remove unwired dead code with latent footguns
Batch 10 of security audit remediation — delete confirmed-dead, unwired
subsystems (ranks 26, 35, 50). None had a production caller (grep-verified);
removal eliminates the latent footguns and ~2.1k lines of dead code.
- token_validator.go (deleted): an unused *TokenValidator whose validateJWT set
Valid=true with NO signature verification — a severe footgun if ever wired
(rank 50). The wired RS-aware validators are unaffected.
- security_monitoring.go (deleted): an unused *SecurityMonitor / ExtractClientIP
that trusted spoofable X-Forwarded-For / X-Real-IP. The live bearer throttle
uses clientIPForBearer (RemoteAddr-only), unchanged (rank 35).
- dynamic_client_registration.go: removed the RFC 7592 management methods
(Update/Read/DeleteClientRegistration) that dereferenced an attacker-
influenced RegistrationClientURI with the registration token attached and no
HTTPS/SSRF gate, and had no callers. The wired RFC 7591 RegisterClient and
credential-store helpers are kept (rank 26).
- tests: removed the tests covering the deleted code.
* chore: add Makefile with yaegi load validation
No Makefile existed. The new `yaegi-validate` target interprets the plugin
under the yaegi interpreter the same way Traefik loads it, catching yaegi-only
incompatibilities (unsupported stdlib symbols, reflection edge cases) that the
native `go build` / `go test` toolchain does not. Importing the plugin forces
yaegi to interpret every file plus its vendored deps; CreateConfig + New
exercise the instantiation path.
- cmd/yaegicheck/main.go: the load driver, marked //go:build ignore so it is
excluded from `go build ./...` (avoids VCS-stamping a main binary, which
fails in git-worktree layouts) yet is run explicitly by yaegi.
- Makefile: build / fmt / vet / lint / test / vendor / yaegi-validate / check
targets; `make check` runs vet + tests + yaegi-validate.
Verified: `make yaegi-validate` passes on this branch — the HKDF cookie
encryption, net-based endpoint validation, and claim sanitizers all interpret
and instantiate cleanly under yaegi.
* ci: bump workflow Go toolchain to 1.25; pin yaegi-validate to v0.16.1
Traefik v3.7.1 (the deployed version) is built with `go 1.25.0`, so the PR and
release workflows now use Go 1.25.x to match the toolchain Traefik uses.
Important distinction: the CI Go version is the build TOOLCHAIN. The plugin's
actual interpreter-compatibility ceiling is the yaegi version Traefik bundles
(v0.16.1, which declares go 1.21 and ships a ~Go 1.22 stdlib symbol surface),
NOT the CI Go version. That ceiling is enforced by `make yaegi-validate` plus
the go.mod language directive — e.g. it is why HKDF is hand-rolled with
hmac+sha256 rather than Go 1.24's crypto/hkdf, which yaegi v0.16.1 lacks.
Also pin Makefile YAEGI_VERSION to v0.16.1 (what Traefik v3.7.1 vendors) so
yaegi-validate exercises the real deployed interpreter instead of @latest,
which could pass on a newer yaegi that supports symbols the deployed one does
not.
* docs: align README/CONFIGURATION with branch behavior changes
- excludedURLs: documented as segment/extension-boundary matching (was
"prefix-matched") — "/public" no longer also matches "/publicsecret" (rank 14).
- Front-channel logout now requires a matching `iss`; requests without one are
rejected with 400 (rank 30).
- Add an "Upgrading from an earlier release" note: session cookies are now
AES-256 encrypted with lifetime tracking sessionMaxAge (one-time re-login on
upgrade), and invalid configuration (rateLimit < 10, key < 32 bytes, missing
callbackURL, non-HTTPS remote providerURL) now fails closed at startup.
* fix: remove staticcheck-flagged unused functions; wire staticcheck into make check
CI Static Analysis (standalone staticcheck) failed with U1000 "unused":
- dynamic_client_registration.go: deleteCredentialsFromStore — its only caller
was the RFC 7592 DeleteClientRegistration removed in the dead-code batch.
- token_test.go: createTestJWTSimple — its only callers were the TokenValidator
tests removed in the same batch.
Both confirmed to have zero remaining callers and removed. build / vet /
go test ./... / staticcheck ./... all green.
The pre-commit hook runs golangci-lint, but CI runs standalone staticcheck
(which flags U1000). Add a `staticcheck` Makefile target and include it in
`make check` so this class of finding is caught locally before push.
* fix(test): stabilize flaky TestWorkerPool_TaskPanic
tasksFailed is incremented in the worker's deferred recover(), which runs after the panicking task's own defer wg.Done(). wg.Wait() could therefore return before the failure was recorded, so reading the counter immediately raced and flaked on slow CI runners. Poll until the failure lands (2s budget) instead. Verified 200x plain + 50x under -race/GOMAXPROCS=1.
universal_cache: stop the write-lock convoy / 100%-CPU spin (observed via pprof: one ServeHTTP goroutine holding c.mu.Lock for hours while 119 requests queued). The per-request populate path (updateLocalCache) PushFronted a duplicate LRU node + overwrote items[key] without removing the prior node; once eviction deleted the key, orphan nodes at Back() were never removable and the eviction loop spun forever under the write lock. Replace the entry in place (mirroring setLocal) and harden evictOldest with a forward-progress guard. Adds universal_cache_orphan_test.go.
telemetry: delete the hand-rolled client; call oss-telemetry v0.2.3 (vendored, Yaegi-safe) directly from New(), once per process via sync.Once.
version: add version.go + workflow-prepare.sh so the release semver is stamped into source at build time (the value cannot be resolved at runtime under Yaegi). dev/source builds keep the 0.0.0-dev sentinel and emit no telemetry.
Adds optional extraAuthParams map[string]string config.
Extra params are appended to the authorization request but can never
override plugin-managed params (client_id, state, nonce, etc.).
UniversalCache.getLocal(): when a cached token expires, the RLock fast
path (line 385-398) previously fell through to c.mu.Lock() (write lock).
Under Yaegi, the write-lock holder takes 10-100ms for LRU manipulation,
and Go's RWMutex writer-priority blocks ALL new RLock callers. A single
expired-token event turned every concurrent request from read-parallel
into write-serialized — the convoy that produced the 737-goroutine
pileup at 0x400275a608 (pprof captured at /tmp/traefik-spike-1779663149).
Fix: return (nil, false) immediately on expiry for Token/JWK/Session
cache types. The periodic cleanup goroutine handles eviction. Write lock
is never taken on the read path for these cache types.
refreshAttemptTracker.mutateState(): the CAS loop used
t.state.CompareAndSwap(t.state.Load(), next) — a second Load that can
see a different value from a concurrent writer, silently overwriting
their update. Fixed to CompareAndSwap(cur, next) using the snapshot we
computed the mutation from.
After v1.0.20 the non-RS validation chain had no production callers —
middleware.ServeHTTP dispatched exclusively through isUserAuthenticatedRS.
The orphaned functions stayed reachable only from a handful of test
files and risked silent logic drift against their RS counterparts.
Deleted from production code (~440 LOC):
- auth_flow.go: isUserAuthenticated
- token_manager.go: validateAzureTokens
- token_manager.go: validateGoogleTokens
- token_manager.go: validateStandardTokens
- token_manager.go: validateTokenExpiry
- removed now-unused encoding/base64 and encoding/json imports
from token_manager.go (only the deleted validateStandardTokens
needed them; the RS variant in token_validation_rs.go keeps its
own imports).
Added (3 LOC):
- token_validation_rs.go: validateGoogleTokensRS (trivial delegator,
parity with the deleted non-RS variant so isUserAuthenticatedRS
can dispatch cleanly).
Tests ported (10 call sites across 3 files):
- audience_test.go: ts.tOidc.validateStandardTokens
- azure_oidc_test.go: tOidc.validateAzureTokens,
ts.tOidc.validateGoogleTokens,
ts.tOidc.validateAzureTokens,
ts.tOidc.isUserAuthenticated
- issue134_followup_graph_test.go: oidc.validateAzureTokens (4x)
Each ported site now constructs a *requestState from its existing
*SessionData via (&requestState{}).captureSession(session) and calls
the *RS variant. Same data, different read source.
Net diff: -440 LOC production, ~+25 LOC tests, +3 LOC stub.
Production now has a single source of truth for token validation;
no parallel implementations to keep in sync.
All tests pass with -race; golangci-lint clean.
Adds token_validation_rs.go with requestState-aware variants of the
token validation path:
isUserAuthenticatedRS(rs) -> dispatches by provider
validateStandardTokensRS(rs) -> standard path (eliminates 17 RLocks)
validateAzureTokensRS(rs) -> Azure path (eliminates 10 RLocks)
validateGoogleTokensRS(rs) -> delegates to standard
validateTokenExpiryRS(rs, tok) -> shared expiry check (eliminates 4 RLocks)
middleware.ServeHTTP now calls isUserAuthenticatedRS(rs) on the hot
path. The pre-v1.0.20 non-RS variants are kept untouched for tests
and any future caller that doesn't have a captured snapshot.
Why
---
The standard validation path read SessionData via session.GetX() 17
times, with GetRefreshToken alone called 11 times (every "return
'needs refresh'" branch re-reads it). Each call acquires
sd.sessionMutex.RLock(). Under Yaegi each RLock costs ~1-5ms of
interpreter dispatch. The captured snapshot already lives on rs, so
the RS variants substitute direct struct field reads.
Per-request cost on the hot authenticated path
----------------------------------------------
ServeHTTP enters:
+ 1 RLock to populate rs (was 0)
Validation path:
Standard: was 17 RLocks, now 0
Azure: was 10 RLocks, now 0
processAuthorizedRequestRS:
was 4-6 GetX calls, now 0 (already in v1.0.19)
Net: ~22-27 fewer Yaegi-dispatched RLock acquisitions per authenticated
request on the hot path.
Caveats
-------
* Refresh / expired / callback paths still use the non-RS validators
because they can mutate session state between validation and use.
* The RS variants are by-design line-for-line equivalents of the
originals. If logic in the originals changes, the RS variants need
matching updates. This is acceptable for now; a future refactor
could collapse them once the non-RS callers are gone.
All tests pass with -race; golangci-lint clean.
Adds requeststate.go and threads a *requestState through the
ServeHTTP -> processAuthorizedRequestRS -> forwardAuthorized path.
rs is allocated once at the top of ServeHTTP, populates SessionData
field snapshots under a SINGLE sd.sessionMutex.RLock, and caches the
MetadataSnapshot. Downstream handlers read the cached fields instead
of calling session.GetX() / t.metadataSnap() repeatedly.
Why
---
Under Yaegi each method dispatch (including RWMutex.RLock) costs
~1-5ms of interpreter overhead. SessionData getters each take an
RLock on sd.sessionMutex; the previous hot path called 5-7 of them
per request (GetAuthenticated, GetAccessToken, GetIDToken,
GetRefreshToken, GetUserIdentifier, plus the same set again inside
processAuthorizedRequest). With one batched RLock + cached fields,
that drops to a single RLock for the whole handler chain.
This is scoped — not a wholesale architectural refactor:
* requestState is per-request (alloc at ServeHTTP entry, dropped on
return). It is NOT a shared cache and never escapes the request.
* The original processAuthorizedRequest is kept unchanged for any
callers we don't migrate this round (bearer path, callback
handlers, expired-token handlers). New code path is the RS-aware
processAuthorizedRequestRS, which middleware.ServeHTTP now uses for
the happy authenticated-and-not-needing-refresh case.
* Cross-request caches (tokenCache, JWKCache, sessionEntries,
sessionInvalidationCache) are unchanged. rs is additive, not a
replacement.
What this does NOT change
-------------------------
* The refresh path still calls session.GetX() in middleware.go
(handleExpiredToken, refreshToken, defaultInitiateAuthentication)
because those flows can mutate session state and a stale rs would
be wrong.
* validateStandardTokens still has its own session.GetX() calls.
Deep plumbing into the token-verification path is a follow-up.
* No semantic changes to authentication, refresh, or session
lifecycle — only the read path is optimised.
All tests pass with -race; golangci-lint clean.
Two related lock-free snapshot refactors addressing the remaining
post-v1.0.16 code-review findings.
1. refreshAttemptTracker: per-field atomic.Load/Store -> atomic.Value
snapshot of *attemptState (refresh_coordinator.go).
Previously each tracker held five independently-atomic fields. The
cooldown-exit reset wrote cooldownEndNano = 0 first, then separately
stored attempts = 1 and windowStartNano = now. A concurrent
isInCooldown call could observe cooldownEndNano = 0 (reset just
completed) with attempts still at MaxRefreshAttempts, immediately
triggering a fresh cooldown — a benign double-trigger race that
nonetheless meant the state machine had observable intermediate
states.
New design: state is a *attemptState (immutable) published via
atomic.Value. All transitions (record/success/failure/window-reset/
cooldown-enter/cooldown-exit) go through mutateState, which runs a
CAS loop: load current snapshot -> construct fresh snapshot ->
CompareAndSwap. Either the entire new state publishes or none of
it does — no intermediate visibility, no cross-field race.
Under Yaegi this collapses 3-5 per-field atomic dispatches into one
atomic.Value.Load on the read path. Write paths pay an extra
allocation for the new snapshot but avoid the cross-field hazard.
2. MetadataSnapshot: hot-path readers use atomic.Value instead of
metadataMu.RLock (middleware.go, types.go, main.go, utilities.go).
middleware.ServeHTTP previously took metadataMu.RLock on every
non-bypass request to read the single field issuerURL. Under Yaegi
each RLock acquisition costs 1-5ms of interpreter dispatch.
updateMetadataEndpoints now also publishes an immutable
*MetadataSnapshot via atomic.Value; the hot-path reader loads it
in one op via t.metadataSnap(). Falls back to the legacy
metadataMu.RLock pattern when the snapshot is unpublished (some
test setups initialize the struct fields directly without going
through updateMetadataEndpoints).
Less-frequent callers (helpers, logout, token_introspection) still
take metadataMu.RLock and are unchanged. The snapshot strictly
subsets the metadataMu-protected fields, so those readers see
identical data.
Note on atomic.Pointer[T]: this would have been the cleaner type but
yaegi v0.16.1's stdlib (used by traefik:v3.7.1) exposes only the
legacy unsafe.Pointer-based atomic primitives — no generic Pointer[T].
atomic.Value provides the same semantics via interface{} + type assert.
All tests pass with -race; golangci-lint clean.
Three related changes addressing post-v1.0.15 code-review findings and
the user's observation that we have been "throwing maps around" — under
Yaegi every sync.Map / atomic / mutex dispatch costs ~1-5ms of
interpreter overhead, so the number of dispatches per request matters
as much as whether they are lock-free.
1. Remove cleanupTimers map + cleanupTimerMu sync.Mutex.
scheduleDelayedCleanup previously tracked every pending timer in a
map guarded by a mutex so a duplicate scheduling could cancel the
prior timer. That "shouldn't happen" path was the only consumer of
the map, but the mutex fired on every successful refresh
completion — another per-request Yaegi-dispatched lock.
performCleanup is already idempotent (LoadAndDelete on the sync.Map),
so a duplicate firing is at worst a no-op second call. Dropped the
map entirely; time.AfterFunc callback now calls performCleanup
directly.
Net: -1 sync.Mutex, -1 map field, -2 Lock/Unlock pairs per refresh
completion. Shutdown simplified — no need to enumerate-and-stop
timers since the callbacks no longer need teardown.
2. Reorder applyLeaderGates: cooldown check BEFORE recordRefreshAttempt.
Previously incremented the attempt counter and then checked cooldown.
Under burst load (many concurrent leaders with different token hashes
but the same session) every goroutine could increment past
MaxRefreshAttempts before any one of them observed the threshold,
so the gate fired too late — same thundering-herd shape that drove
v1.0.14 into the ground. Reordering makes the gate authoritative:
only attempts that pass the gate are recorded.
Semantic change: with MaxRefreshAttempts=N, exactly N attempts now
run to completion before the (N+1)th is denied. Previously the Nth
was denied as it tried to record (off-by-one stricter). Test
assertion updated to N (was N-1).
3. Fix getOrCreateOperation MaxConcurrentRefreshes overshoot.
The previous CAS-loop allowed a transient overshoot of up to N-1
leaders when several goroutines all observed `current < max` in the
same scheduling slice before any one of them succeeded their CAS —
visible to readers as currentInFlightRefreshes > MaxConcurrentRefreshes
for a brief window.
Replaced with the ticket-and-return pattern: increment optimistically,
decrement if we overshot. Strictly bounded: only the goroutine that
produces max+1 sees max+1 as committed; the rest decrement back
immediately. No CAS retry loop needed.
What was NOT done in this commit, and why:
* metadataMu.RLock cached via atomic snapshot — code-reviewer flagged
this at severity 7 (3 RLocks per request: middleware.go:213,
token_manager.go:349, token_manager.go:408). The clean fix is an
atomic.Pointer[*MetadataSnapshot], but generic atomic.Pointer[T] is
NOT exposed by yaegi v0.16.1's stdlib (only legacy unsafe.Pointer
primitives). atomic.Value would work but requires a snapshot-struct
refactor across ~15 call sites (helpers/logout/token_introspection/
token_manager/main/middleware). Deferred to a focused future PR.
* isInCooldown multi-field reset race — the cooldown-reset CAS wins
on cooldownEndNano, then separately stores attempts/consecutiveFailures/
windowStartNano. A concurrent isInCooldown can briefly see the
pre-reset attempts value and trigger a fresh cooldown. Semantic glitch
(double-cooldown), not a correctness disaster. Fix is a single atomic
pointer swap of an immutable snapshot — same atomic.Pointer constraint
as above. Deferred.
All tests pass with -race; golangci-lint clean.
JWKCache.GetJWKS previously held a sync.RWMutex.Lock() across the entire
HTTP round-trip to the IdP's JWKS endpoint (jwk.go:93). On a cold cache
(cold pod, JWK rotation, transient network blip) every concurrent
request piled up on this single global write-lock. Under Yaegi each
Lock() acquisition costs 10-50ms of interpreter dispatch — same
architectural shape as the bugs v1.0.14 and v1.0.15 already fixed,
just one that hadn't surfaced as the dominant bottleneck yet.
Code-review post-spike #2 flagged this at confidence 9/10 as the next
likely death-spiral on pod cold-start.
Change replaces the lock with a sync.Map-based singleflight: the first
caller for a given JWKS URL performs the fetch; concurrent callers
attach to the same *jwksFetch and wait on its done channel for the
result. Cold-cache cost is now O(1) HTTP fetch regardless of how many
goroutines are waiting, and no Yaegi-dispatched lock is held during the
fetch itself.
Correctness:
- LoadOrStore winner does the work; losers wait on a done channel.
- Done channel close is in a defer, so panics in fetchJWKS still
unblock waiters.
- Map entry is removed in the same defer, so a fresh failed fetch can
be retried by the next request without waiting for any stale entry.
- ctx.Done() unblocks waiters independently of the leader's progress.
- Re-checks the cache after winning LoadOrStore, since another fetch
may have populated the cache between the initial miss and the win.
Cleanup: also removes a stray yaegi-extract output file
(github_com-lukaszraczylo-traefikoidc.go) that was accidentally
committed during local yaegi compatibility testing.
All tests pass with -race; golangci-lint clean.
The v1.0.14 fix replaced one contended sync.RWMutex (RefreshCoordinator.
refreshMutex) with sync.Map. Production showed the same death-spiral
signature recurring ~2 hours later — same shape, different mutex:
65 goroutines stuck on a sync.(*RWMutex).Lock at one address, pod
pinned at 1000m CPU, identical Yaegi runCfg/reflect.Value.Call stack
pattern. The mutex was RefreshCoordinator.attemptsMutex.
Generalising: under Yaegi (interpreted Go for traefik plugins), any
per-request global mutex acquisition is a latent serialization point.
reflect.Value.Call dispatch on a held lock turns a microsecond
critical section into a multi-millisecond one, and on a GOMAXPROCS=1
pod the queue is unbounded.
This commit removes every per-request global mutex on the hot path:
1. RefreshCoordinator.attemptsMutex (sync.RWMutex)
sessionRefreshAttempts: map -> sync.Map.
refreshAttemptTracker: all fields atomic (int32, int64 UnixNano,
cooldownEndNano == 0 as the not-in-cooldown sentinel, replacing
the inCooldown bool).
isInCooldown / recordRefreshAttempt / recordRefreshSuccess /
recordRefreshFailure all become lock-free. Cooldown entry uses
CompareAndSwapInt64 so only one goroutine logs the transition.
2. RefreshCircuitBreaker.mutex (sync.RWMutex)
lastFailureTime / lastSuccessTime -> atomic.Int64 UnixNano.
state and failures already atomic.
AllowRequest / RecordSuccess / RecordFailure now pure atomic ops.
3. TraefikOidc.firstRequestMutex (sync.Mutex)
firstRequestReceived bool -> firstRequestStarted int32.
metadataRefreshStarted bool -> metadataRefreshStartedAtomic int32.
ServeHTTP bootstrap path uses CompareAndSwapInt32 — fires once,
zero steady-state cost. Previously the mutex was acquired on
every non-health request forever.
4. TraefikOidc.metadataRetryMutex (sync.Mutex)
lastMetadataRetryTime time.Time -> lastMetadataRetryNano int64.
The 30-second retry throttle is now a CAS on lastMetadataRetryNano.
cleanupStaleEntries iterates via sync.Map.Range; eviction is a
CompareAndDelete by pointer identity so a tracker freshly re-used by
a concurrent caller is not lost.
Empirical evidence (3 specialist-agent analysis of the v1.0.14 spike,
profiles in /tmp/traefik-spike-1779511683/):
* mutex profile: 97% delay in sync.(*Mutex).Unlock via
HTTPHandlerSwitcher -> accesslog -> metrics -> backoff.RetryNotify
* 65 stuck goroutines at one RWMutex address (0x40022eb648),
identical Yaegi CFG pointer, all on rc.attemptsMutex via
recordRefreshAttempt + isInCooldown
* traffic driver: long-lived in-cluster Go-http-client doing
~5.4 req/s POST embeddings via OIDC cookie session → same
sessionID → contention all funnels to one tracker entry
Yaegi support for sync/atomic confirmed at
github.com/traefik/[email protected].1/stdlib/go1_22_sync_atomic.go:
AddInt32/Int64, LoadInt32/Int64, StoreInt32/Int64,
CompareAndSwapInt32/Int64 all exposed via reflect.ValueOf. Yaegi
dispatches each call through reflect.Value.Call to the COMPILED
atomic.* function, which executes a single hardware CAS/LOCK-XADD
instruction. Each atomic op still pays Yaegi dispatch cost but
cannot block — no queueing, no death spiral.
Trade-off acknowledged: v1.0.15 issues ~6-8 atomic/sync.Map ops per
leader-path request vs the 4 mutex ops of v1.0.14. Under low
contention this is a modest CPU bump. Under high contention it's
an unbounded → bounded transformation. Net win.
All tests pass with -race; golangci-lint clean.
lukaszraczylo
