diff --git a/auth_flow.go b/auth_flow.go index db489d6..f2205f4 100644 --- a/auth_flow.go +++ b/auth_flow.go @@ -233,7 +233,7 @@ func (t *TraefikOidc) handleCallback(rw http.ResponseWriter, req *http.Request, return } - if nonceClaim != sessionNonce { + if !constantTimeStringCompare(nonceClaim, sessionNonce) { t.logger.Error("Nonce claim does not match session nonce during callback") t.sendErrorResponse(rw, req, "Authentication failed: Nonce mismatch", http.StatusInternalServerError) return diff --git a/middleware.go b/middleware.go index 84295b2..ac872c6 100644 --- a/middleware.go +++ b/middleware.go @@ -788,6 +788,16 @@ func (t *TraefikOidc) forwardAuthorized(rw http.ResponseWriter, req *http.Reques } } + // Bearer source: strip the raw inbound Authorization header to keep the + // token out of downstream service logs (off-by-config for operators who + // chain services that each re-verify the bearer). Stripped here, before + // header-template application, so an operator-supplied Authorization + // template (e.g. "Authorization: Bearer {{.AccessToken}}") can re-supply + // the downstream value rather than being immediately deleted. + if p.Source == sourceBearer && t.stripAuthorizationHeader { + req.Header.Del("Authorization") + } + if len(t.headerTemplates) > 0 { // p.Claims may be nil (e.g. session without an ID token). Templates // referencing .Claims.* will simply produce empty values — matches @@ -853,13 +863,6 @@ func (t *TraefikOidc) forwardAuthorized(rw http.ResponseWriter, req *http.Reques } } - // Bearer source: strip the Authorization header to keep the raw token - // out of downstream service logs. Off-by-config for operators who chain - // services that each re-verify the bearer. - if p.Source == sourceBearer && t.stripAuthorizationHeader { - req.Header.Del("Authorization") - } - t.logger.Debugf("Request authorized for user %s (source=%d), forwarding to next handler", p.Identifier, p.Source) t.next.ServeHTTP(rw, req) diff --git a/review_r45_regression_test.go b/review_r45_regression_test.go new file mode 100644 index 0000000..dc78def --- /dev/null +++ b/review_r45_regression_test.go @@ -0,0 +1,76 @@ +package traefikoidc + +import ( + "net/http" + "net/http/httptest" + "testing" + "text/template" +) + +// TestForwardAuthorized_BearerStripPreservesTemplatedAuthorization is a +// regression test for the order in forwardAuthorized between the bearer +// Authorization strip and header-template application. Previously the strip +// (when source==sourceBearer && StripAuthorizationHeader, the default) ran +// AFTER template application, deleting an operator-supplied Authorization +// template (e.g. "Authorization: Bearer {{.AccessToken}}") and defeating +// its purpose. The strip must run first so an explicit template survives. +func TestForwardAuthorized_BearerStripPreservesTemplatedAuthorization(t *testing.T) { + var forwarded *http.Request + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + forwarded = r.Clone(r.Context()) + w.WriteHeader(http.StatusOK) + }) + + authTpl := template.Must(template.New("auth").Parse("Bearer {{.AccessToken}}")) + oidc := &TraefikOidc{ + logger: NewLogger("debug"), + next: next, + stripAuthorizationHeader: true, + headerTemplates: map[string]*template.Template{ + "Authorization": authTpl, + }, + } + + req := httptest.NewRequest(http.MethodGet, "http://example.com/", nil) + req.Header.Set("Authorization", "Bearer raw-inbound-token") + + rec := httptest.NewRecorder() + oidc.forwardAuthorized(rec, req, &principal{ + Source: sourceBearer, + AccessToken: "access-token-x", + }) + + d := forwarded.Header.Get("Authorization") + // The operator's templated value must survive, replacing the raw inbound + // token that would otherwise leak into downstream logs. + if d != "Bearer access-token-x" { + t.Fatalf("Authorization = %q, want %q", d, "Bearer access-token-x") + } +} + +// TestForwardAuthorized_BearerStripDefault is the regression-guard for the +// default config (strip on, no Authorization template): the raw inbound +// bearer header must still be removed before forwarding. +func TestForwardAuthorized_BearerStripDefault(t *testing.T) { + var forwarded *http.Request + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + forwarded = r.Clone(r.Context()) + w.WriteHeader(http.StatusOK) + }) + + oidc := &TraefikOidc{ + logger: NewLogger("debug"), + next: next, + stripAuthorizationHeader: true, + } + + req := httptest.NewRequest(http.MethodGet, "http://example.com/", nil) + req.Header.Set("Authorization", "Bearer raw-inbound-token") + + rec := httptest.NewRecorder() + oidc.forwardAuthorized(rec, req, &principal{Source: sourceBearer}) + + if v := forwarded.Header.Get("Authorization"); v != "" { + t.Fatalf("Authorization = %q, want empty (stripped)", v) + } +}