From 52ef32ece771f784e5c7019d985084578f0168b0 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Tue, 19 May 2026 15:17:04 +0100 Subject: [PATCH] =?UTF-8?q?fix(oidcgate):=20security=20hardening=20?= =?UTF-8?q?=E2=80=94=20sanitize=20XFU,=20guardrails,=20validations?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- cmd/oidcgate/config.go | 22 +++++ cmd/oidcgate/config_test.go | 166 ++++++++++++++++++++++++++++++++++++ cmd/oidcgate/main.go | 3 +- forwarded_uri.go | 30 +++++-- forwarded_uri_test.go | 36 ++++++++ 5 files changed, 251 insertions(+), 6 deletions(-) diff --git a/cmd/oidcgate/config.go b/cmd/oidcgate/config.go index 83935aa..75880ec 100644 --- a/cmd/oidcgate/config.go +++ b/cmd/oidcgate/config.go @@ -82,6 +82,28 @@ func Load(path string) (*Config, error) { return nil, fmt.Errorf("config: missing required 'listen' (or OIDCGATE_LISTEN env var)") } + if !strings.HasPrefix(cfg.OIDC.CallbackURL, "/") { + return nil, fmt.Errorf("config: callbackURL must be a path starting with '/', got %q", cfg.OIDC.CallbackURL) + } + if !strings.HasPrefix(cfg.OIDC.LogoutURL, "/") { + return nil, fmt.Errorf("config: logoutURL must be a path starting with '/', got %q", cfg.OIDC.LogoutURL) + } + + reserved := []string{ + sentinelPath, + cfg.AuthPath, + cfg.StartPath, + cfg.OIDC.CallbackURL, + cfg.OIDC.LogoutURL, + } + for _, ex := range cfg.OIDC.ExcludedURLs { + for _, r := range reserved { + if r != "" && strings.HasPrefix(r, ex) { + return nil, fmt.Errorf("config: excludedURL %q would bypass reserved oidcgate path %q", ex, r) + } + } + } + // Force standalone semantics: trust X-Forwarded-Uri. cfg.OIDC.TrustForwardedURI = true return cfg, nil diff --git a/cmd/oidcgate/config_test.go b/cmd/oidcgate/config_test.go index aa976a5..171910c 100644 --- a/cmd/oidcgate/config_test.go +++ b/cmd/oidcgate/config_test.go @@ -6,6 +6,27 @@ import ( "testing" ) +// minimalYAML is a base config accepted by Load with no surprises. +const minimalYAML = ` +listen: ":8080" +providerURL: "https://idp.example" +clientID: "abc" +clientSecret: "secret" +sessionEncryptionKey: "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" +callbackURL: "/oauth2/callback" +logoutURL: "/oauth2/logout" +` + +func writeConfig(t *testing.T, content string) string { + t.Helper() + dir := t.TempDir() + path := filepath.Join(dir, "config.yaml") + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + return path +} + func TestLoad_YAMLRoundTrip(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "config.yaml") @@ -135,3 +156,148 @@ redis: t.Errorf("redis address: want redis:6379, got %q", cfg.OIDC.Redis.Address) } } + +// Fix 5: callbackURL / logoutURL must start with "/" +func TestLoad_RejectsAbsoluteCallbackURL(t *testing.T) { + path := writeConfig(t, ` +listen: ":8080" +providerURL: "https://idp.example" +clientID: "abc" +clientSecret: "secret" +sessionEncryptionKey: "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" +callbackURL: "https://app.example.com/oauth2/callback" +logoutURL: "/oauth2/logout" +`) + if _, err := Load(path); err == nil { + t.Fatal("callbackURL with absolute URL must be rejected") + } +} + +func TestLoad_RejectsAbsoluteLogoutURL(t *testing.T) { + path := writeConfig(t, ` +listen: ":8080" +providerURL: "https://idp.example" +clientID: "abc" +clientSecret: "secret" +sessionEncryptionKey: "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" +callbackURL: "/oauth2/callback" +logoutURL: "https://app.example.com/oauth2/logout" +`) + if _, err := Load(path); err == nil { + t.Fatal("logoutURL with absolute URL must be rejected") + } +} + +// Fix 2: excludedURLs must not prefix reserved paths +func TestLoad_RejectsExcludedURLPrefixingReservedPath(t *testing.T) { + path := writeConfig(t, ` +listen: ":8080" +providerURL: "https://idp.example" +clientID: "abc" +clientSecret: "secret" +sessionEncryptionKey: "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" +callbackURL: "/oauth2/callback" +logoutURL: "/oauth2/logout" +excludedURLs: ["/"] +`) + if _, err := Load(path); err == nil { + t.Fatal("excludedURLs: ['/'] must be rejected (bypasses all reserved paths)") + } +} + +func TestLoad_AllowsNonOverlappingExcludedURL(t *testing.T) { + path := writeConfig(t, minimalYAML+`excludedURLs: ["/public"] +`) + if _, err := Load(path); err != nil { + t.Fatalf("non-overlapping excludedURL must be accepted: %v", err) + } +} + +// Fix 3: env override coverage — every envScalarFields entry must have a +// matching case in setScalarField. isAllZeroForField detects drift. +func TestEnvOverrideCoverage(t *testing.T) { + for _, field := range envScalarFields { + field := field + t.Run(field, func(t *testing.T) { + probe := "/safe/probe-" + field + if field == "SessionEncryptionKey" { + probe = "fedcba9876543210fedcba9876543210fedcba9876543210fedcba9876543210" + } + if field == "LogLevel" { + probe = "debug" + } + if field == "ClientAuthMethod" { + probe = "client_secret_post" + } + if field == "ClientAssertionAlg" { + probe = "RS256" + } + + var fresh Config + setScalarField(&fresh, field, probe) + if isAllZeroForField(&fresh, field, probe) { + t.Fatalf("envScalarFields includes %q but setScalarField has no matching case (drift)", field) + } + }) + } +} + +// isAllZeroForField returns true when setScalarField did NOT set the expected +// field — i.e., the switch is missing a case for `field`. +func isAllZeroForField(cfg *Config, field, probe string) bool { + switch field { + case "Listen": + return cfg.Listen != probe + case "AuthPath": + return cfg.AuthPath != probe + case "StartPath": + return cfg.StartPath != probe + case "ProviderURL": + return cfg.OIDC.ProviderURL != probe + case "ClientID": + return cfg.OIDC.ClientID != probe + case "ClientSecret": + return cfg.OIDC.ClientSecret != probe + case "Audience": + return cfg.OIDC.Audience != probe + case "CallbackURL": + return cfg.OIDC.CallbackURL != probe + case "LogoutURL": + return cfg.OIDC.LogoutURL != probe + case "PostLogoutRedirectURI": + return cfg.OIDC.PostLogoutRedirectURI != probe + case "SessionEncryptionKey": + return cfg.OIDC.SessionEncryptionKey != probe + case "CookiePrefix": + return cfg.OIDC.CookiePrefix != probe + case "CookieDomain": + return cfg.OIDC.CookieDomain != probe + case "LogLevel": + return cfg.OIDC.LogLevel != probe + case "RevocationURL": + return cfg.OIDC.RevocationURL != probe + case "OIDCEndSessionURL": + return cfg.OIDC.OIDCEndSessionURL != probe + case "UserIdentifierClaim": + return cfg.OIDC.UserIdentifierClaim != probe + case "GroupClaimName": + return cfg.OIDC.GroupClaimName != probe + case "RoleClaimName": + return cfg.OIDC.RoleClaimName != probe + case "ClientAuthMethod": + return cfg.OIDC.ClientAuthMethod != probe + case "ClientAssertionPrivateKey": + return cfg.OIDC.ClientAssertionPrivateKey != probe + case "ClientAssertionKeyPath": + return cfg.OIDC.ClientAssertionKeyPath != probe + case "ClientAssertionKeyID": + return cfg.OIDC.ClientAssertionKeyID != probe + case "ClientAssertionAlg": + return cfg.OIDC.ClientAssertionAlg != probe + case "CACertPath": + return cfg.OIDC.CACertPath != probe + case "CACertPEM": + return cfg.OIDC.CACertPEM != probe + } + return true // unknown field → drift +} diff --git a/cmd/oidcgate/main.go b/cmd/oidcgate/main.go index cd8a449..14d6a72 100644 --- a/cmd/oidcgate/main.go +++ b/cmd/oidcgate/main.go @@ -23,6 +23,7 @@ func main() { } ctx, cancel := context.WithCancel(context.Background()) + defer cancel() success := newSuccessHandler() middleware, err := traefikoidc.NewWithContext(ctx, &cfg.OIDC, success, "oidcgate") @@ -42,11 +43,11 @@ func main() { if err := shutdown(srv); err != nil { log.Printf("oidcgate: shutdown error: %v", err) } - cancel() }() log.Printf("oidcgate: listening on %s", cfg.Listen) if err := srv.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) { + cancel() log.Fatalf("oidcgate: serve: %v", err) } log.Println("oidcgate: stopped") diff --git a/forwarded_uri.go b/forwarded_uri.go index 32b6cfa..7f4e7a5 100644 --- a/forwarded_uri.go +++ b/forwarded_uri.go @@ -1,17 +1,37 @@ package traefikoidc -import "net/http" +import ( + "net/http" + "net/url" + "strings" +) // originalRequestURI returns the request URI that should be used as the // post-login redirect target. When TrustForwardedURI is enabled and the -// X-Forwarded-Uri header is present, that header wins — this lets the -// oidcgate forward-auth daemon recover the real URL when nginx/Caddy/etc -// proxied a stripped subrequest to /oauth2/auth or /oauth2/start. +// X-Forwarded-Uri header carries a safe same-origin path, that header +// wins. Otherwise (or if the header is missing/unsafe), falls back to +// req.URL.RequestURI() — the path the request reached the proxy with. +// +// "Safe" means: starts with "/", does NOT start with "//" (protocol-relative +// URLs can change host), and has no scheme or host after parsing. This +// prevents an attacker-controllable header from triggering an open redirect +// via http.Redirect later in the auth flow. func (t *TraefikOidc) originalRequestURI(req *http.Request) string { if t.trustForwardedURI { - if v := req.Header.Get("X-Forwarded-Uri"); v != "" { + if v := req.Header.Get("X-Forwarded-Uri"); v != "" && isSafeRedirectTarget(v) { return v } } return req.URL.RequestURI() } + +func isSafeRedirectTarget(v string) bool { + if !strings.HasPrefix(v, "/") || strings.HasPrefix(v, "//") { + return false + } + u, err := url.Parse(v) + if err != nil { + return false + } + return u.Host == "" && u.Scheme == "" +} diff --git a/forwarded_uri_test.go b/forwarded_uri_test.go index b92c285..260fb3c 100644 --- a/forwarded_uri_test.go +++ b/forwarded_uri_test.go @@ -31,3 +31,39 @@ func TestOriginalRequestURI_TrustEnabledNoHeader(t *testing.T) { t.Fatalf("trust-on no header: want /protected, got %q", got) } } +func TestOriginalRequestURI_RejectsAbsoluteURL(t *testing.T) { + tr := &TraefikOidc{trustForwardedURI: true} + req := httptest.NewRequest(http.MethodGet, "/protected", nil) + req.Header.Set("X-Forwarded-Uri", "https://evil.example/phish") + if got := tr.originalRequestURI(req); got != "/protected" { + t.Fatalf("absolute URL must be rejected, want /protected fallback, got %q", got) + } +} + +func TestOriginalRequestURI_RejectsProtocolRelative(t *testing.T) { + tr := &TraefikOidc{trustForwardedURI: true} + req := httptest.NewRequest(http.MethodGet, "/protected", nil) + req.Header.Set("X-Forwarded-Uri", "//evil.example/phish") + if got := tr.originalRequestURI(req); got != "/protected" { + t.Fatalf("protocol-relative URL must be rejected, want /protected fallback, got %q", got) + } +} + +func TestOriginalRequestURI_AcceptsSafePathWithQuery(t *testing.T) { + tr := &TraefikOidc{trustForwardedURI: true} + req := httptest.NewRequest(http.MethodGet, "/protected", nil) + req.Header.Set("X-Forwarded-Uri", "/safe?x=1&y=2") + if got := tr.originalRequestURI(req); got != "/safe?x=1&y=2" { + t.Fatalf("safe path with query must be accepted, got %q", got) + } +} + +func TestOriginalRequestURI_RejectsBareHostnameNoSlash(t *testing.T) { + tr := &TraefikOidc{trustForwardedURI: true} + req := httptest.NewRequest(http.MethodGet, "/protected", nil) + req.Header.Set("X-Forwarded-Uri", "evil.example/phish") + if got := tr.originalRequestURI(req); got != "/protected" { + t.Fatalf("non-/ prefix must be rejected, got %q", got) + } +} +