From bea3f04bf87b93f5354f392fe2216a88e6031350 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 1 Oct 2026 11:01:25 +0200 Subject: [PATCH] release: v7.15.5 (#3553) * update to release version v7.15.5 * Merge commit from fork Signed-off-by: Jan Larwig Co-authored-by: blakeroberts-wk * Merge commit from fork Signed-off-by: Jan Larwig * Merge commit from fork * fix: validate trusted IP proxy headers Respect trusted proxy boundaries before using real-client-IP headers for authentication bypass decisions and safely traverse X-Forwarded-For chains. Signed-off-by: Jan Larwig * fix: trusted-ip header bypass Signed-off-by: Jan Larwig * docs: add changelog entry Signed-off-by: Jan Larwig --------- Signed-off-by: Jan Larwig Signed-off-by: Jan Larwig * docs: changelog for v7.15.5 Signed-off-by: Jan Larwig * docs: update order of owners for prow Signed-off-by: Jan Larwig * ci: make the linter happy again Signed-off-by: Jan Larwig --------- Signed-off-by: Jan Larwig Signed-off-by: Jan Larwig Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Jan Larwig Co-authored-by: blakeroberts-wk Co-authored-by: Jan Larwig --- CHANGELOG.md | 70 +- OWNERS | 4 +- .../docker-compose-alpha-config.yaml | 2 +- .../docker-compose-gitea.yaml | 2 +- .../docker-compose-keycloak.yaml | 2 +- .../docker-compose-nginx.yaml | 2 +- .../docker-compose-traefik.yaml | 2 +- contrib/local-environment/docker-compose.yaml | 2 +- docs/docs/behaviour.md | 1 + docs/docs/configuration/integrations/index.md | 5 + docs/docs/configuration/integrations/nginx.md | 11 + docs/docs/configuration/overview.md | 75 +- docs/docs/installation.md | 2 +- .../configuration/integrations/index.md | 5 + .../configuration/integrations/nginx.md | 52 +- .../version-7.15.x/configuration/overview.md | 75 +- .../version-7.15.x/installation.md | 2 +- oauthproxy.go | 48 +- oauthproxy_test.go | 703 +++++++++++++++++- pkg/apis/options/options.go | 6 +- pkg/ip/realclientip.go | 58 +- pkg/ip/realclientip_test.go | 103 ++- pkg/requests/util/util.go | 51 +- pkg/requests/util/util_test.go | 105 ++- pkg/validation/allowlist.go | 2 +- pkg/validation/options.go | 11 +- pkg/validation/options_test.go | 67 ++ 27 files changed, 1360 insertions(+), 108 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 985a408f..7e3d73ff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,16 +2,84 @@ ## Release Highlights +## Important Notes + +## Breaking Changes + +## Changes since v7.15.5 + +# V7.15.5 + +## Release Highlights + +- 🔵 Golang version upgrade to v1.26.8 + - Upgrade of all dependencies to their latest versions +- 🕵️‍♀️ Vulnerabilities have been addressed + - [CVE-2026-56855](https://nvd.nist.gov/vuln/detail/CVE-2026-56855) + - [CVE-2026-78662](https://nvd.nist.gov/vuln/detail/CVE-2026-78662) + - [CVE-2026-84304](https://nvd.nist.gov/vuln/detail/CVE-2026-84304) + - [CVE-2026-84445](https://nvd.nist.gov/vuln/detail/CVE-2026-84445) + ## Important Notes The Bitbucket provider `--bitbucket-team` flag got deprecated and we added `--bitbucket-workspace` flag to restrict logins to members of a specific workspace instead of a team. The `--bitbucket-team` flag is still supported and will act like workspace but will be removed in a future release. Please update your configuration to use the new `--bitbucket-workspace` flag. For more information, refer to [Bitbucket teams API deprecation](https://developer.atlassian.com/cloud/bitbucket/bitbucket-api-teams-deprecation/). Additionally refer to OAuth client configuration for Bitbucket provider in the [documentation](https://oauth2-proxy.github.io/oauth2-proxy/configuration/providers/bitbucket/). for changes in the scopes (Account>Read) is now required if you restrict by workspace. +Security Advisories: + +- (Critical) [GHSA-63jm-59jj-478j](https://github.com/oauth2-proxy/oauth2-proxy/security/advisories/GHSA-63jm-59jj-478j) Authentication bypass via inconsistent skip-auth path interpretation +- (Critical) [GHSA-wr5q-7wxw-x568](https://github.com/oauth2-proxy/oauth2-proxy/security/advisories/GHSA-wr5q-7wxw-x568) Authentication bypass via spoofed client-IP headers in OAuth2 Proxy +- (Moderate) [GHSA-hhqp-vx7f-5c6m](https://github.com/oauth2-proxy/oauth2-proxy/security/advisories/GHSA-hhqp-vx7f-5c6m) Credential disclosure through OAuth callback error logging + +Read more below + +## Critical Fixes + +For a small subset of deployments these fixes might be breaking change for the sake of fixing trust/security boundaries. + +### [GHSA-63jm-59jj-478j](https://github.com/oauth2-proxy/oauth2-proxy/security/advisories/GHSA-63jm-59jj-478j) Authentication bypass via inconsistent skip-auth path interpretation +**Skip-auth path matching is now stricter**: `--skip-auth-route` and +`--skip-auth-regex` no longer grant exemptions for invalid or ambiguous paths, +even when a positive or negated rule would otherwise match. This includes dot +segments, repeated slashes (including leading `//`), semicolons, backslashes, +fragment-like content, decoded question marks, and control characters, including +encoded forms. For example, previously public `/public/file;version=1` and +`/public//file` now follow normal authentication and authorization. `#` and `%23` +are no longer silently stripped before matching. + +Trusted `X-Forwarded-Uri` metadata must use origin form (`/path?query`), not a full +URL. Before upgrading, review public-route exemptions and external-auth header +configuration. If unusual paths must remain public, expose them separately from +the protected routing boundary rather than broadening skip-auth rules. + +Flag names and configuration syntax are unchanged. Ordinary query strings, +trailing-slash distinctions, and unambiguous escaped characters retain their +matching behavior. The fix does not rewrite upstream request targets or +unconditionally reject authenticated requests; existing router behavior and +separately configured exemptions remain unchanged. + +### [GHSA-wr5q-7wxw-x568](https://github.com/oauth2-proxy/oauth2-proxy/security/advisories/GHSA-wr5q-7wxw-x568) Authentication bypass via spoofed client-IP headers in OAuth2 Proxy +**Trusted client-IP resolution now enforces trusted proxy boundaries**: When +`--reverse-proxy` is enabled, client-IP headers configured via +`--real-client-ip-header` are accepted only from direct peers that match +`--trusted-proxy-ip`. If the direct peer is not trusted, the transport peer +address is evaluated instead. + +For `X-Forwarded-For`, the chain is traversed from right to left, skipping +intermediate proxies in `--trusted-proxy-ip` and treating the first untrusted +address as the client. Leftmost entries cannot be spoofed past the trusted proxy +boundary. If `--trusted-proxy-ip` is left unset, OAuth2 Proxy preserves backwards +compatibility by trusting all source addresses and retaining the leftmost +behavior, which does not protect against spoofed XFF headers. To secure +`--trusted-ip` exemptions behind a reverse proxy, configure explicit, narrowly +scoped `--trusted-proxy-ip` CIDR ranges and ensure the proxy sanitizes client-IP +headers. Missing or malformed client-IP headers from a trusted proxy will no +longer fall back to granting exemptions based on the proxy's own IP. + ## Breaking Changes ## Changes since v7.15.4 -<<<<<<< HEAD - [#3546](https://github.com/oauth2-proxy/oauth2-proxy/pull/3546) fix: strip the port from the request host when matching cookie domains @kirilju - [#3547](https://github.com/oauth2-proxy/oauth2-proxy/pull/3547) fix: refresh additional claims for OIDC and MS Entra ID providers and properly populate additional claims during login (@Apollo3zehn) - [#3477](https://github.com/oauth2-proxy/oauth2-proxy/pull/3477) fix(bitbucket): auth failure due to Bitbucket OAuth 2.0 [changes on May 4th 2026](https://developer.atlassian.com/cloud/bitbucket/changelog/#CHANGE-3052) @mfouad diff --git a/OWNERS b/OWNERS index db0ad93e..be2d335e 100644 --- a/OWNERS +++ b/OWNERS @@ -1,6 +1,6 @@ reviewers: - - JoelSpeed - tuunit + - JoelSpeed approvers: - - JoelSpeed - tuunit + - JoelSpeed diff --git a/contrib/local-environment/docker-compose-alpha-config.yaml b/contrib/local-environment/docker-compose-alpha-config.yaml index a5dab6b1..d07a07ae 100644 --- a/contrib/local-environment/docker-compose-alpha-config.yaml +++ b/contrib/local-environment/docker-compose-alpha-config.yaml @@ -14,7 +14,7 @@ version: "3.0" services: oauth2-proxy: container_name: oauth2-proxy - image: quay.io/oauth2-proxy/oauth2-proxy:v7.15.4 + image: quay.io/oauth2-proxy/oauth2-proxy:v7.15.5 command: --config /oauth2-proxy.cfg --alpha-config /oauth2-proxy-alpha-config.yaml hostname: oauth2-proxy volumes: diff --git a/contrib/local-environment/docker-compose-gitea.yaml b/contrib/local-environment/docker-compose-gitea.yaml index ce6fc7d6..66cc9efd 100644 --- a/contrib/local-environment/docker-compose-gitea.yaml +++ b/contrib/local-environment/docker-compose-gitea.yaml @@ -14,7 +14,7 @@ version: '3.0' services: oauth2-proxy: container_name: oauth2-proxy - image: quay.io/oauth2-proxy/oauth2-proxy:v7.15.4 + image: quay.io/oauth2-proxy/oauth2-proxy:v7.15.5 command: --config /oauth2-proxy.cfg hostname: oauth2-proxy volumes: diff --git a/contrib/local-environment/docker-compose-keycloak.yaml b/contrib/local-environment/docker-compose-keycloak.yaml index 6bb9ac26..e43c09f9 100644 --- a/contrib/local-environment/docker-compose-keycloak.yaml +++ b/contrib/local-environment/docker-compose-keycloak.yaml @@ -14,7 +14,7 @@ version: "3.0" services: oauth2-proxy: container_name: oauth2-proxy - image: quay.io/oauth2-proxy/oauth2-proxy:v7.15.4 + image: quay.io/oauth2-proxy/oauth2-proxy:v7.15.5 command: --config /oauth2-proxy.cfg hostname: oauth2-proxy volumes: diff --git a/contrib/local-environment/docker-compose-nginx.yaml b/contrib/local-environment/docker-compose-nginx.yaml index 98e797a5..9d943220 100644 --- a/contrib/local-environment/docker-compose-nginx.yaml +++ b/contrib/local-environment/docker-compose-nginx.yaml @@ -12,7 +12,7 @@ version: "3.0" services: oauth2-proxy: - image: quay.io/oauth2-proxy/oauth2-proxy:v7.15.4 + image: quay.io/oauth2-proxy/oauth2-proxy:v7.15.5 ports: [] hostname: oauth2-proxy container_name: oauth2-proxy diff --git a/contrib/local-environment/docker-compose-traefik.yaml b/contrib/local-environment/docker-compose-traefik.yaml index 9015b42d..be210f18 100644 --- a/contrib/local-environment/docker-compose-traefik.yaml +++ b/contrib/local-environment/docker-compose-traefik.yaml @@ -18,7 +18,7 @@ services: oauth2-proxy: container_name: oauth2-proxy - image: quay.io/oauth2-proxy/oauth2-proxy:v7.15.4 + image: quay.io/oauth2-proxy/oauth2-proxy:v7.15.5 command: --config /oauth2-proxy.cfg ports: [] hostname: oauth2-proxy diff --git a/contrib/local-environment/docker-compose.yaml b/contrib/local-environment/docker-compose.yaml index c5b7b897..b88ee511 100644 --- a/contrib/local-environment/docker-compose.yaml +++ b/contrib/local-environment/docker-compose.yaml @@ -13,7 +13,7 @@ version: "3.0" services: oauth2-proxy: container_name: oauth2-proxy - image: quay.io/oauth2-proxy/oauth2-proxy:v7.15.4 + image: quay.io/oauth2-proxy/oauth2-proxy:v7.15.5 command: --config /oauth2-proxy.cfg hostname: oauth2-proxy volumes: diff --git a/docs/docs/behaviour.md b/docs/docs/behaviour.md index d0be452e..605ec013 100644 --- a/docs/docs/behaviour.md +++ b/docs/docs/behaviour.md @@ -5,6 +5,7 @@ title: Behaviour 1. Authentication Requirement: All requests passing through the proxy to upstream applications require authentication, excluding default proxy endpoints. - Exception: If the request matches a skipped route (configured via `--skip-auth-route`): + - Invalid or ambiguous paths cannot grant this exemption, even with a negated rule. See [Skip-auth path matching](configuration/overview.md#skip-auth-path-matching). - Authentication is not enforced, but the proxy will opportunistically attempt to validate a session cookie (`--cookie-name`) or JWT (`--skip-jwt-bearer-tokens`) if present in the request. - Configured user info and authentication headers (e.g., `--pass-access-token`) are injected to upstream routes when validation succeeds. diff --git a/docs/docs/configuration/integrations/index.md b/docs/docs/configuration/integrations/index.md index b773910b..8826bfe4 100644 --- a/docs/docs/configuration/integrations/index.md +++ b/docs/docs/configuration/integrations/index.md @@ -32,6 +32,11 @@ When integrating with Kubernetes web UIs, make sure to: Most integrations require the following OAuth2 Proxy configuration: - `--reverse-proxy=true`: Required to correctly handle `X-Forwarded-*` headers +- `--trusted-proxy-ip=`: Restricts forwarded headers, including + client identity used by `--trusted-ip`, to the reverse proxies' addresses or + networks. Include the direct peer and every trusted hop that can appear in + XFF. Without this option, all source addresses are trusted for backwards + compatibility. - **Session storage**: For production deployments with large tokens due to a lot of claims like AD groups, use `--session-store-type=redis` For provider-specific configuration, see the [OAuth Provider Configuration](../providers/index.md) documentation. diff --git a/docs/docs/configuration/integrations/nginx.md b/docs/docs/configuration/integrations/nginx.md index 06d9654c..d31cde0c 100644 --- a/docs/docs/configuration/integrations/nginx.md +++ b/docs/docs/configuration/integrations/nginx.md @@ -15,6 +15,17 @@ Configure OAuth2 Proxy with Nginx using the `auth_request` directive. Includes e **This option requires `--reverse-proxy` option to be set.** +When Nginx connects from localhost as shown below, also configure OAuth2 Proxy +with `--trusted-proxy-ip=127.0.0.1/32`. The example overwrites `X-Real-IP` with +Nginx's transport peer address; do not pass through a client-supplied value, +especially when `--trusted-ip` is enabled. + +If using `--real-client-ip-header=X-Forwarded-For` with an appending proxy +configuration such as `$proxy_add_x_forwarded_for`, list every trusted proxy hop +in `--trusted-proxy-ip`. OAuth2 Proxy selects the first untrusted address from +the right, not a client-supplied leftmost entry. The trust-all compatibility +default cannot provide this protection. + The [Nginx `auth_request` directive](http://nginx.org/en/docs/http/ngx_http_auth_request_module.html) allows Nginx to authenticate requests via the oauth2-proxy's `/auth` endpoint, which only returns a 202 Accepted response or a 401 Unauthorized response without proxying the request through. For example: ```nginx diff --git a/docs/docs/configuration/overview.md b/docs/docs/configuration/overview.md index 965953fa..1548b0f3 100644 --- a/docs/docs/configuration/overview.md +++ b/docs/docs/configuration/overview.md @@ -194,9 +194,40 @@ Provider specific options can be found on their respective subpages. ### Proxy Options :::warning -When `--reverse-proxy` is enabled, configure `--trusted-proxy-ip` to the IPs or CIDR ranges of the reverse proxies that are allowed to send `X-Forwarded-*` headers. If you leave it unset, OAuth2 Proxy currently trusts all source IPs for backwards compatibility, which means a client that can reach OAuth2 Proxy directly may be able to spoof forwarded headers. +When `--reverse-proxy` is enabled, configure `--trusted-proxy-ip` to the IPs or CIDR ranges of the reverse proxies that are allowed to send `X-Forwarded-*` headers. Include the direct peer and every trusted proxy hop that can appear in `X-Forwarded-For`. If you leave it unset, OAuth2 Proxy currently trusts all source IPs for backwards compatibility, which means a client that can reach OAuth2 Proxy directly may be able to spoof forwarded headers. ::: +### Trusted client IPs behind a reverse proxy + +`--trusted-ip` is an authentication bypass, not a list of trusted reverse +proxies. When combining it with `--reverse-proxy`, always configure +`--trusted-proxy-ip` with the direct proxy and every trusted intermediate proxy +that can appear in XFF, and prevent clients from reaching OAuth2 Proxy directly. + +OAuth2 Proxy accepts `--real-client-ip-header` only from those trusted peers. +For `X-Forwarded-For`, it combines repeated header fields in their received order, +then walks the chain from right to left and skips addresses covered by +`--trusted-proxy-ip`. The first untrusted address is treated as the client; +entries to its left are not used. This also applies to client-IP logging and +callback diagnostics. For example, if a trusted proxy appends the actual client +to `10.0.0.5, 198.51.100.23`, the client is `198.51.100.23`, not the potentially +forged `10.0.0.5`. For single-value headers such as `X-Real-IP`, configure the +edge proxy to overwrite any value supplied by the client. + +If every address in the chain is in a trusted proxy range, the leftmost address +is used for compatibility. Leaving `--trusted-proxy-ip` unset trusts all IPv4 and +IPv6 addresses, so it retains this leftmost behavior and does **not** protect +against forged XFF entries. Explicit, narrowly scoped proxy ranges are required. + +For a trusted proxy, a missing client-IP header or malformed address encountered +before reaching the client does not grant a trusted-IP exemption, even if the +proxy's transport IP is in `--trusted-ip`. Logging falls back to the transport IP in these cases; +callback diagnostics retain the `transport IP (client IP)` format when both are +available. When reverse-proxy mode is disabled or the peer is not trusted, only +the transport IP is evaluated. Unix socket connections have no transport IP to +exempt; in reverse-proxy mode they retain the existing behavior of accepting +client-IP headers, with the same XFF chain resolution. + | Flag / Config Field | Type | Description | Default | | ----------------------------------------------------------------------------- | -------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------- | | flag: `--allow-query-semicolons`
toml: `allow_query_semicolons` | bool | allow the use of semicolons in query args ([required for some legacy applications](https://github.com/golang/go/issues/25192)) | `false` | @@ -211,19 +242,19 @@ When `--reverse-proxy` is enabled, configure `--trusted-proxy-ip` to the IPs or | flag: `--htpasswd-file`
toml: `htpasswd_file` | string | additionally authenticate against a htpasswd file. Entries must be created with `htpasswd -B` for bcrypt encryption | | | flag: `--htpasswd-user-group`
toml: `htpasswd_user_groups` | string \| list | the groups to be set on sessions for htpasswd users | | | flag: `--proxy-prefix`
toml: `proxy_prefix` | string | the url root path that this proxy should be nested under (e.g. /`/sign_in`) | `"/oauth2"` | -| flag: `--real-client-ip-header`
toml: `real_client_ip_header` | string | Header used to determine the real IP of the client, requires `--reverse-proxy` to be set (one of: X-Forwarded-For, X-Real-IP, X-ProxyUser-IP, X-Envoy-External-Address, or CF-Connecting-IP) | X-Real-IP | +| flag: `--real-client-ip-header`
toml: `real_client_ip_header` | string | Header used to determine the real IP of the client when the direct caller matches `--trusted-proxy-ip`; requires `--reverse-proxy` (one of: X-Forwarded-For, X-Real-IP, X-ProxyUser-IP, X-Envoy-External-Address, or CF-Connecting-IP) | X-Real-IP | | flag: `--redirect-url`
toml: `redirect_url` | string | the OAuth Redirect URL, e.g. `"https://internalapp.yourcompany.com/oauth2/callback"` | | | flag: `--relative-redirect-url`
toml: `relative_redirect_url` | bool | allow relative OAuth Redirect URL.` | false | | flag: `--reverse-proxy`
toml: `reverse_proxy` | bool | are we running behind a reverse proxy, controls whether headers like X-Real-IP are accepted and allows X-Forwarded-\{Proto,Host,Uri\} headers to be used on redirect selection | false | -| flag: `--trusted-proxy-ip`
toml: `trusted_proxy_ips` | string \| list | list of IPs or CIDR ranges allowed to supply `X-Forwarded-*` headers when `--reverse-proxy` is enabled. If not set, OAuth2 Proxy preserves backwards compatibility by trusting all source IPs (`0.0.0.0/0`, `::/0`) and logs a warning at startup. Configure this to your reverse proxy addresses to prevent forwarded header spoofing. | `"0.0.0.0/0", "::/0"` | +| flag: `--trusted-proxy-ip`
toml: `trusted_proxy_ips` | string \| list | list of direct proxy IPs or CIDR ranges allowed to supply `X-Forwarded-*` headers when `--reverse-proxy` is enabled. Include every trusted proxy hop that can appear in XFF. If not set, OAuth2 Proxy preserves backwards compatibility by trusting all source IPs (`0.0.0.0/0`, `::/0`) and logs a warning at startup. | `"0.0.0.0/0", "::/0"` | | flag: `--signature-key`
toml: `signature_key` | string | GAP-Signature request signature key (algorithm:secretkey) | | | flag: `--skip-auth-preflight`
toml: `skip_auth_preflight` | bool | will skip authentication for OPTIONS requests | false | -| flag: `--skip-auth-regex`
toml: `skip_auth_regex` | string \| list | (DEPRECATED for `--skip-auth-route`) bypass authentication for requests paths that match (may be given multiple times). Path matching is performed against the normalized path only; fragment identifiers (`#`) and their URL-encoded form (`%23`) are stripped before evaluation. | | -| flag: `--skip-auth-route`
toml: `skip_auth_routes` | string \| list | bypass authentication for requests that match the method & path. Format: method=path_regex OR method!=path_regex. For all methods: path_regex OR !=path_regex. Path matching is performed against the normalized path only; fragment identifiers (`#`) and their URL-encoded form (`%23`) are stripped before evaluation. | | +| flag: `--skip-auth-regex`
toml: `skip_auth_regex` | string \| list | (DEPRECATED for `--skip-auth-route`) bypass authentication for requests whose paths match (may be given multiple times). Matching uses the decoded path without query parameters; invalid or ambiguous paths cannot grant a path exemption. See [Skip-auth path matching](#skip-auth-path-matching). | | +| flag: `--skip-auth-route`
toml: `skip_auth_routes` | string \| list | bypass authentication for requests that match the method & path. Format: method=path_regex OR method!=path_regex. For all methods: path_regex OR !=path_regex. Matching uses the decoded path without query parameters; invalid or ambiguous paths cannot grant a path exemption, including with negated rules. See [Skip-auth path matching](#skip-auth-path-matching). | | | flag: `--skip-jwt-bearer-tokens`
toml: `skip_jwt_bearer_tokens` | bool | will skip requests that have verified JWT bearer tokens (the token must have [`aud`](https://en.wikipedia.org/wiki/JSON_Web_Token#Standard_fields) that matches this client id or one of the extras from `extra-jwt-issuers`) | false | | flag: `--skip-provider-button`
toml: `skip_provider_button` | bool | will skip sign-in-page to directly reach the next step: oauth/start | false | | flag: `--ssl-insecure-skip-verify`
toml: `ssl_insecure_skip_verify` | bool | skip validation of certificates presented when using HTTPS providers | false | -| flag: `--trusted-ip`
toml: `trusted_ips` | string \| list | list of IPs or CIDR ranges to allow to bypass authentication (may be given multiple times). When combined with `--reverse-proxy` and optionally `--real-client-ip-header` this will evaluate the trust of the IP stored in an HTTP header by a reverse proxy rather than the layer-3/4 remote address. WARNING: trusting IPs has inherent security flaws, especially when obtaining the IP address from an HTTP header (reverse-proxy mode). Use this option only if you understand the risks and how to manage them. | | +| flag: `--trusted-ip`
toml: `trusted_ips` | string \| list | list of client IPs or CIDR ranges allowed to bypass authentication (may be given multiple times). With `--reverse-proxy`, the selected real-client-IP header is accepted only from direct callers matching `--trusted-proxy-ip`; otherwise the transport peer address is used. WARNING: this option bypasses authentication. Configure explicit trusted proxy ranges and ensure the proxy sanitizes client-IP headers. | | | flag: `--whitelist-domain`
toml: `whitelist_domains` | string \| list | allowed domains for redirection after authentication. Prefix domain with a `.` or a `*.` to allow subdomains (e.g. `.example.com`, `*.example.com`) [^2] | | [^2]: When using the `whitelist-domain` option, any domain prefixed with a `.` or a `*.` will allow any subdomain of the specified domain as a valid redirect URL. By default, only empty ports are allowed. This translates to allowing the default port of the URL's protocol (80 for HTTP, 443 for HTTPS, etc.) since browsers omit them. To allow only a specific port, add it to the whitelisted domain: `example.com:8080`. To allow any port, use `*`: `example.com:*`. @@ -273,6 +304,34 @@ When `--reverse-proxy` is enabled, configure `--trusted-proxy-ip` to the IPs or | flag: `--upstream-timeout`
toml: `upstream_timeout` | duration | maximum amount of time the server will wait for a response from the upstream | 30s | | flag: `--upstream`
toml: `upstreams` | string \| list | the http url(s) of the upstream endpoint, file:// paths for static files or `static://` for static response. Routing is based on the path | | +### Skip-auth path matching + +Both `--skip-auth-route` and the deprecated `--skip-auth-regex` match the +percent-decoded path, excluding query parameters. In reverse-proxy mode, +`X-Forwarded-Uri` is used only when the existing forwarded-header trust checks +permit it. Original-URI metadata must use origin form (`/path?query`), not an +absolute URL, authority, or relative path. + +Invalid or ambiguous targets do not grant a path-based authentication exemption. +This check happens before any regular expression or its negation is evaluated. +In particular, exemptions are declined for malformed path escapes, `.` or `..` +path segments, repeated slashes (including leading `//`), semicolons, backslashes, +fragment-like `#` content, decoded `?` characters, and control characters. +Encoded forms are checked after a single decoding pass; paths are not recursively +decoded. + +This intentionally also requires normal authentication for valid semicolon paths +such as `/public/file;version=1` and repeated-slash paths such as `/public//file`. +Fragments are no longer silently stripped. Ordinary query strings do not affect +matching. Unambiguous escaped characters remain supported, and trailing slashes +remain significant: `/public` and `/public/` are distinct matching paths. + +These checks do not normalize or rewrite the upstream request target. A declined +path exemption follows normal authentication and authorization; it is not an +unconditional rejection of authenticated requests using unusual paths. Existing +router redirects and separately configured exemptions, such as trusted client +IPs or skipped preflight requests, are unchanged. + ## Configuration Validation The `--config-test` flag validates your configuration file without starting the proxy server. This is useful for: @@ -361,7 +420,7 @@ Logging of requests to the `/ping` endpoint (or using `--ping-user-agent`) and t ## Auth Log Format -Authentication logs are logs which are guaranteed to contain a username or email address of a user attempting to authenticate. These logs are output by default in the below format: +Authentication logs describe authentication attempts. They include the user's username or email address when known, or `-` otherwise. OAuth callback diagnostics for missing or invalid CSRF cookies are authentication logs, controlled by `--auth-logging`, and include cookie names but not cookie values. These logs are output by default in the below format: ``` - - [2015/03/19 17:20:19] [] @@ -392,7 +451,7 @@ Available variables for auth logging: | RequestMethod | GET | The request method. | | Timestamp | 2015/03/19 17:20:19 | The date and time of the logging event. | | UserAgent | - | The full user agent as reported by the requesting client. | -| Username | username@email.com | The email or username of the auth request. | +| Username | username@email.com | The email or username of the auth request, or `-` if not yet known. | | Status | AuthSuccess | The status of the auth request. See above for details. | ## Request Log Format diff --git a/docs/docs/installation.md b/docs/docs/installation.md index 11bce1a5..a75c8429 100644 --- a/docs/docs/installation.md +++ b/docs/docs/installation.md @@ -5,7 +5,7 @@ title: Installation 1. Choose how to deploy: - a. Using a [Prebuilt Binary](https://github.com/oauth2-proxy/oauth2-proxy/releases) (current release is `v7.15.4`) + a. Using a [Prebuilt Binary](https://github.com/oauth2-proxy/oauth2-proxy/releases) (current release is `v7.15.5`) b. Using Go to install the latest release ```bash diff --git a/docs/versioned_docs/version-7.15.x/configuration/integrations/index.md b/docs/versioned_docs/version-7.15.x/configuration/integrations/index.md index b773910b..8826bfe4 100644 --- a/docs/versioned_docs/version-7.15.x/configuration/integrations/index.md +++ b/docs/versioned_docs/version-7.15.x/configuration/integrations/index.md @@ -32,6 +32,11 @@ When integrating with Kubernetes web UIs, make sure to: Most integrations require the following OAuth2 Proxy configuration: - `--reverse-proxy=true`: Required to correctly handle `X-Forwarded-*` headers +- `--trusted-proxy-ip=`: Restricts forwarded headers, including + client identity used by `--trusted-ip`, to the reverse proxies' addresses or + networks. Include the direct peer and every trusted hop that can appear in + XFF. Without this option, all source addresses are trusted for backwards + compatibility. - **Session storage**: For production deployments with large tokens due to a lot of claims like AD groups, use `--session-store-type=redis` For provider-specific configuration, see the [OAuth Provider Configuration](../providers/index.md) documentation. diff --git a/docs/versioned_docs/version-7.15.x/configuration/integrations/nginx.md b/docs/versioned_docs/version-7.15.x/configuration/integrations/nginx.md index ca6402a9..d31cde0c 100644 --- a/docs/versioned_docs/version-7.15.x/configuration/integrations/nginx.md +++ b/docs/versioned_docs/version-7.15.x/configuration/integrations/nginx.md @@ -15,6 +15,17 @@ Configure OAuth2 Proxy with Nginx using the `auth_request` directive. Includes e **This option requires `--reverse-proxy` option to be set.** +When Nginx connects from localhost as shown below, also configure OAuth2 Proxy +with `--trusted-proxy-ip=127.0.0.1/32`. The example overwrites `X-Real-IP` with +Nginx's transport peer address; do not pass through a client-supplied value, +especially when `--trusted-ip` is enabled. + +If using `--real-client-ip-header=X-Forwarded-For` with an appending proxy +configuration such as `$proxy_add_x_forwarded_for`, list every trusted proxy hop +in `--trusted-proxy-ip`. OAuth2 Proxy selects the first untrusted address from +the right, not a client-supplied leftmost entry. The trust-all compatibility +default cannot provide this protection. + The [Nginx `auth_request` directive](http://nginx.org/en/docs/http/ngx_http_auth_request_module.html) allows Nginx to authenticate requests via the oauth2-proxy's `/auth` endpoint, which only returns a 202 Accepted response or a 401 Unauthorized response without proxying the request through. For example: ```nginx @@ -60,23 +71,23 @@ server { auth_request_set $auth_cookie $upstream_http_set_cookie; add_header Set-Cookie $auth_cookie; - # When using the --set-authorization-header flag, some provider's cookies can exceed the 4kb - # limit and so the OAuth2 Proxy splits these into multiple parts. + # Large OIDC tokens can make the session cookie exceed the 4KB limit, + # causing OAuth2 Proxy to split the cookie into multiple parts. # Nginx normally only copies the first `Set-Cookie` header from the auth_request to the response, - # so if your cookies are larger than 4kb, you will need to extract additional cookies manually. - auth_request_set $auth_cookie_name_upstream_1 $upstream_cookie_auth_cookie_name_1; + # so extract the second part when using the default cookie name, `_oauth2_proxy`. + auth_request_set $auth_cookie_upstream_1 $upstream_cookie__oauth2_proxy_1; # Extract the Cookie attributes from the first Set-Cookie header and append them # to the second part ($upstream_cookie_* variables only contain the raw cookie content) if ($auth_cookie ~* "(; .*)") { - set $auth_cookie_name_0 $auth_cookie; - set $auth_cookie_name_1 "auth_cookie_name_1=$auth_cookie_name_upstream_1$1"; + set $auth_cookie_0 $auth_cookie; + set $auth_cookie_1 "_oauth2_proxy_1=$auth_cookie_upstream_1$1"; } # Send both Set-Cookie headers now if there was a second part - if ($auth_cookie_name_upstream_1) { - add_header Set-Cookie $auth_cookie_name_0; - add_header Set-Cookie $auth_cookie_name_1; + if ($auth_cookie_upstream_1) { + add_header Set-Cookie $auth_cookie_0; + add_header Set-Cookie $auth_cookie_1; } proxy_pass http://backend/; @@ -91,6 +102,24 @@ server { } ``` +### Multi-part session cookies + +The example above is ready to use with the default `--cookie-name=_oauth2_proxy` setting and handles a session split into two cookies. If you configure a custom cookie name, update both references to the second cookie part: + +| Cookie configuration | Nginx upstream cookie variable | `Set-Cookie` name | +| --- | --- | --- | +| Default (`_oauth2_proxy`) | `$upstream_cookie__oauth2_proxy_1` | `_oauth2_proxy_1=` | +| `--cookie-name=my_cookie` | `$upstream_cookie_my_cookie_1` | `my_cookie_1=` | + +For example, with `--cookie-name=my_cookie`, change the relevant lines to: + +```nginx +auth_request_set $auth_cookie_upstream_1 $upstream_cookie_my_cookie_1; +set $auth_cookie_1 "my_cookie_1=$auth_cookie_upstream_1$1"; +``` + +If a session is split into more than two cookies, repeat the extraction for each additional numeric suffix. For consistently large sessions, prefer `--session-store-type=redis` so only a small session ticket is stored in the browser cookie. + ### Understanding the `error_page` redirect pattern The `auth_request` directive expects the authentication endpoint (`/oauth2/auth`) to return: @@ -137,12 +166,11 @@ location @oauth2_signin { #### API / Machine routes (no redirect) -For API endpoints where clients expect a 401/403 status code (not a redirect): +For API endpoints where clients expect a 401/403 status code (not a redirect), reuse the same `auth_request` header forwarding configuration from the main example as needed for your backend: ```nginx location /api/ { auth_request /oauth2/auth; - error_page 401 =401; # Pass through the 401 status proxy_pass http://backend/; } ``` @@ -167,8 +195,6 @@ It is recommended to use `--session-store-type=redis` when expecting large sessi For a complete example of integrating oauth2-proxy with Kubernetes Dashboard on AKS using Azure Entra ID, including RBAC configuration and troubleshooting, see the [Kubernetes Dashboard on AKS](../providers/ms_entra_id.md#kubernetes-dashboard-on-aks) section in the Microsoft Entra ID provider documentation. ::: -You have to substitute *name* with the actual cookie name you configured via --cookie-name parameter. If you don't set a custom cookie name the variable should be "$upstream_cookie__oauth2_proxy_1" instead of "$upstream_cookie_name_1" and the new cookie-name should be "_oauth2_proxy_1=" instead of "name_1=". - :::note If you set up your OAuth2 provider to rotate your client secret, you can use the `client-secret-file` option to reload the secret when it is updated. ::: diff --git a/docs/versioned_docs/version-7.15.x/configuration/overview.md b/docs/versioned_docs/version-7.15.x/configuration/overview.md index b145065a..1548b0f3 100644 --- a/docs/versioned_docs/version-7.15.x/configuration/overview.md +++ b/docs/versioned_docs/version-7.15.x/configuration/overview.md @@ -194,9 +194,40 @@ Provider specific options can be found on their respective subpages. ### Proxy Options :::warning -When `--reverse-proxy` is enabled, configure `--trusted-proxy-ip` to the IPs or CIDR ranges of the reverse proxies that are allowed to send `X-Forwarded-*` headers. If you leave it unset, OAuth2 Proxy currently trusts all source IPs for backwards compatibility, which means a client that can reach OAuth2 Proxy directly may be able to spoof forwarded headers. +When `--reverse-proxy` is enabled, configure `--trusted-proxy-ip` to the IPs or CIDR ranges of the reverse proxies that are allowed to send `X-Forwarded-*` headers. Include the direct peer and every trusted proxy hop that can appear in `X-Forwarded-For`. If you leave it unset, OAuth2 Proxy currently trusts all source IPs for backwards compatibility, which means a client that can reach OAuth2 Proxy directly may be able to spoof forwarded headers. ::: +### Trusted client IPs behind a reverse proxy + +`--trusted-ip` is an authentication bypass, not a list of trusted reverse +proxies. When combining it with `--reverse-proxy`, always configure +`--trusted-proxy-ip` with the direct proxy and every trusted intermediate proxy +that can appear in XFF, and prevent clients from reaching OAuth2 Proxy directly. + +OAuth2 Proxy accepts `--real-client-ip-header` only from those trusted peers. +For `X-Forwarded-For`, it combines repeated header fields in their received order, +then walks the chain from right to left and skips addresses covered by +`--trusted-proxy-ip`. The first untrusted address is treated as the client; +entries to its left are not used. This also applies to client-IP logging and +callback diagnostics. For example, if a trusted proxy appends the actual client +to `10.0.0.5, 198.51.100.23`, the client is `198.51.100.23`, not the potentially +forged `10.0.0.5`. For single-value headers such as `X-Real-IP`, configure the +edge proxy to overwrite any value supplied by the client. + +If every address in the chain is in a trusted proxy range, the leftmost address +is used for compatibility. Leaving `--trusted-proxy-ip` unset trusts all IPv4 and +IPv6 addresses, so it retains this leftmost behavior and does **not** protect +against forged XFF entries. Explicit, narrowly scoped proxy ranges are required. + +For a trusted proxy, a missing client-IP header or malformed address encountered +before reaching the client does not grant a trusted-IP exemption, even if the +proxy's transport IP is in `--trusted-ip`. Logging falls back to the transport IP in these cases; +callback diagnostics retain the `transport IP (client IP)` format when both are +available. When reverse-proxy mode is disabled or the peer is not trusted, only +the transport IP is evaluated. Unix socket connections have no transport IP to +exempt; in reverse-proxy mode they retain the existing behavior of accepting +client-IP headers, with the same XFF chain resolution. + | Flag / Config Field | Type | Description | Default | | ----------------------------------------------------------------------------- | -------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------- | | flag: `--allow-query-semicolons`
toml: `allow_query_semicolons` | bool | allow the use of semicolons in query args ([required for some legacy applications](https://github.com/golang/go/issues/25192)) | `false` | @@ -211,19 +242,19 @@ When `--reverse-proxy` is enabled, configure `--trusted-proxy-ip` to the IPs or | flag: `--htpasswd-file`
toml: `htpasswd_file` | string | additionally authenticate against a htpasswd file. Entries must be created with `htpasswd -B` for bcrypt encryption | | | flag: `--htpasswd-user-group`
toml: `htpasswd_user_groups` | string \| list | the groups to be set on sessions for htpasswd users | | | flag: `--proxy-prefix`
toml: `proxy_prefix` | string | the url root path that this proxy should be nested under (e.g. /`/sign_in`) | `"/oauth2"` | -| flag: `--real-client-ip-header`
toml: `real_client_ip_header` | string | Header used to determine the real IP of the client, requires `--reverse-proxy` to be set (one of: X-Forwarded-For, X-Real-IP, X-ProxyUser-IP, X-Envoy-External-Address, or CF-Connecting-IP) | X-Real-IP | +| flag: `--real-client-ip-header`
toml: `real_client_ip_header` | string | Header used to determine the real IP of the client when the direct caller matches `--trusted-proxy-ip`; requires `--reverse-proxy` (one of: X-Forwarded-For, X-Real-IP, X-ProxyUser-IP, X-Envoy-External-Address, or CF-Connecting-IP) | X-Real-IP | | flag: `--redirect-url`
toml: `redirect_url` | string | the OAuth Redirect URL, e.g. `"https://internalapp.yourcompany.com/oauth2/callback"` | | | flag: `--relative-redirect-url`
toml: `relative_redirect_url` | bool | allow relative OAuth Redirect URL.` | false | | flag: `--reverse-proxy`
toml: `reverse_proxy` | bool | are we running behind a reverse proxy, controls whether headers like X-Real-IP are accepted and allows X-Forwarded-\{Proto,Host,Uri\} headers to be used on redirect selection | false | -| flag: `--trusted-proxy-ip`
toml: `trusted_proxy_ips` | string \| list | list of IPs or CIDR ranges allowed to supply `X-Forwarded-*` headers when `--reverse-proxy` is enabled. If not set, OAuth2 Proxy preserves backwards compatibility by trusting all source IPs (`0.0.0.0/0`, `::/0`) and logs a warning at startup. Configure this to your reverse proxy addresses to prevent forwarded header spoofing. | `"0.0.0.0/0", "::/0"` | +| flag: `--trusted-proxy-ip`
toml: `trusted_proxy_ips` | string \| list | list of direct proxy IPs or CIDR ranges allowed to supply `X-Forwarded-*` headers when `--reverse-proxy` is enabled. Include every trusted proxy hop that can appear in XFF. If not set, OAuth2 Proxy preserves backwards compatibility by trusting all source IPs (`0.0.0.0/0`, `::/0`) and logs a warning at startup. | `"0.0.0.0/0", "::/0"` | | flag: `--signature-key`
toml: `signature_key` | string | GAP-Signature request signature key (algorithm:secretkey) | | | flag: `--skip-auth-preflight`
toml: `skip_auth_preflight` | bool | will skip authentication for OPTIONS requests | false | -| flag: `--skip-auth-regex`
toml: `skip_auth_regex` | string \| list | (DEPRECATED for `--skip-auth-route`) bypass authentication for requests paths that match (may be given multiple times) | | -| flag: `--skip-auth-route`
toml: `skip_auth_routes` | string \| list | bypass authentication for requests that match the method & path. Format: method=path_regex OR method!=path_regex. For all methods: path_regex OR !=path_regex | | +| flag: `--skip-auth-regex`
toml: `skip_auth_regex` | string \| list | (DEPRECATED for `--skip-auth-route`) bypass authentication for requests whose paths match (may be given multiple times). Matching uses the decoded path without query parameters; invalid or ambiguous paths cannot grant a path exemption. See [Skip-auth path matching](#skip-auth-path-matching). | | +| flag: `--skip-auth-route`
toml: `skip_auth_routes` | string \| list | bypass authentication for requests that match the method & path. Format: method=path_regex OR method!=path_regex. For all methods: path_regex OR !=path_regex. Matching uses the decoded path without query parameters; invalid or ambiguous paths cannot grant a path exemption, including with negated rules. See [Skip-auth path matching](#skip-auth-path-matching). | | | flag: `--skip-jwt-bearer-tokens`
toml: `skip_jwt_bearer_tokens` | bool | will skip requests that have verified JWT bearer tokens (the token must have [`aud`](https://en.wikipedia.org/wiki/JSON_Web_Token#Standard_fields) that matches this client id or one of the extras from `extra-jwt-issuers`) | false | | flag: `--skip-provider-button`
toml: `skip_provider_button` | bool | will skip sign-in-page to directly reach the next step: oauth/start | false | | flag: `--ssl-insecure-skip-verify`
toml: `ssl_insecure_skip_verify` | bool | skip validation of certificates presented when using HTTPS providers | false | -| flag: `--trusted-ip`
toml: `trusted_ips` | string \| list | list of IPs or CIDR ranges to allow to bypass authentication (may be given multiple times). When combined with `--reverse-proxy` and optionally `--real-client-ip-header` this will evaluate the trust of the IP stored in an HTTP header by a reverse proxy rather than the layer-3/4 remote address. WARNING: trusting IPs has inherent security flaws, especially when obtaining the IP address from an HTTP header (reverse-proxy mode). Use this option only if you understand the risks and how to manage them. | | +| flag: `--trusted-ip`
toml: `trusted_ips` | string \| list | list of client IPs or CIDR ranges allowed to bypass authentication (may be given multiple times). With `--reverse-proxy`, the selected real-client-IP header is accepted only from direct callers matching `--trusted-proxy-ip`; otherwise the transport peer address is used. WARNING: this option bypasses authentication. Configure explicit trusted proxy ranges and ensure the proxy sanitizes client-IP headers. | | | flag: `--whitelist-domain`
toml: `whitelist_domains` | string \| list | allowed domains for redirection after authentication. Prefix domain with a `.` or a `*.` to allow subdomains (e.g. `.example.com`, `*.example.com`) [^2] | | [^2]: When using the `whitelist-domain` option, any domain prefixed with a `.` or a `*.` will allow any subdomain of the specified domain as a valid redirect URL. By default, only empty ports are allowed. This translates to allowing the default port of the URL's protocol (80 for HTTP, 443 for HTTPS, etc.) since browsers omit them. To allow only a specific port, add it to the whitelisted domain: `example.com:8080`. To allow any port, use `*`: `example.com:*`. @@ -273,6 +304,34 @@ When `--reverse-proxy` is enabled, configure `--trusted-proxy-ip` to the IPs or | flag: `--upstream-timeout`
toml: `upstream_timeout` | duration | maximum amount of time the server will wait for a response from the upstream | 30s | | flag: `--upstream`
toml: `upstreams` | string \| list | the http url(s) of the upstream endpoint, file:// paths for static files or `static://` for static response. Routing is based on the path | | +### Skip-auth path matching + +Both `--skip-auth-route` and the deprecated `--skip-auth-regex` match the +percent-decoded path, excluding query parameters. In reverse-proxy mode, +`X-Forwarded-Uri` is used only when the existing forwarded-header trust checks +permit it. Original-URI metadata must use origin form (`/path?query`), not an +absolute URL, authority, or relative path. + +Invalid or ambiguous targets do not grant a path-based authentication exemption. +This check happens before any regular expression or its negation is evaluated. +In particular, exemptions are declined for malformed path escapes, `.` or `..` +path segments, repeated slashes (including leading `//`), semicolons, backslashes, +fragment-like `#` content, decoded `?` characters, and control characters. +Encoded forms are checked after a single decoding pass; paths are not recursively +decoded. + +This intentionally also requires normal authentication for valid semicolon paths +such as `/public/file;version=1` and repeated-slash paths such as `/public//file`. +Fragments are no longer silently stripped. Ordinary query strings do not affect +matching. Unambiguous escaped characters remain supported, and trailing slashes +remain significant: `/public` and `/public/` are distinct matching paths. + +These checks do not normalize or rewrite the upstream request target. A declined +path exemption follows normal authentication and authorization; it is not an +unconditional rejection of authenticated requests using unusual paths. Existing +router redirects and separately configured exemptions, such as trusted client +IPs or skipped preflight requests, are unchanged. + ## Configuration Validation The `--config-test` flag validates your configuration file without starting the proxy server. This is useful for: @@ -361,7 +420,7 @@ Logging of requests to the `/ping` endpoint (or using `--ping-user-agent`) and t ## Auth Log Format -Authentication logs are logs which are guaranteed to contain a username or email address of a user attempting to authenticate. These logs are output by default in the below format: +Authentication logs describe authentication attempts. They include the user's username or email address when known, or `-` otherwise. OAuth callback diagnostics for missing or invalid CSRF cookies are authentication logs, controlled by `--auth-logging`, and include cookie names but not cookie values. These logs are output by default in the below format: ``` - - [2015/03/19 17:20:19] [] @@ -392,7 +451,7 @@ Available variables for auth logging: | RequestMethod | GET | The request method. | | Timestamp | 2015/03/19 17:20:19 | The date and time of the logging event. | | UserAgent | - | The full user agent as reported by the requesting client. | -| Username | username@email.com | The email or username of the auth request. | +| Username | username@email.com | The email or username of the auth request, or `-` if not yet known. | | Status | AuthSuccess | The status of the auth request. See above for details. | ## Request Log Format diff --git a/docs/versioned_docs/version-7.15.x/installation.md b/docs/versioned_docs/version-7.15.x/installation.md index 11bce1a5..a75c8429 100644 --- a/docs/versioned_docs/version-7.15.x/installation.md +++ b/docs/versioned_docs/version-7.15.x/installation.md @@ -5,7 +5,7 @@ title: Installation 1. Choose how to deploy: - a. Using a [Prebuilt Binary](https://github.com/oauth2-proxy/oauth2-proxy/releases) (current release is `v7.15.4`) + a. Using a [Prebuilt Binary](https://github.com/oauth2-proxy/oauth2-proxy/releases) (current release is `v7.15.5`) b. Using Go to install the latest release ```bash diff --git a/oauthproxy.go b/oauthproxy.go index f8dc5471..ea5e4733 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -595,8 +595,8 @@ func isAllowedMethod(req *http.Request, route allowedRoute) bool { return route.method == "" || req.Method == route.method } -func isAllowedPath(req *http.Request, route allowedRoute) bool { - matches := route.pathRegex.MatchString(requestutil.GetRequestPath(req)) +func isAllowedPath(requestPath string, route allowedRoute) bool { + matches := route.pathRegex.MatchString(requestPath) if route.negate { return !matches @@ -607,8 +607,16 @@ func isAllowedPath(req *http.Request, route allowedRoute) bool { // IsAllowedRoute is used to check if the request method & path is allowed without auth func (p *OAuthProxy) isAllowedRoute(req *http.Request) bool { + if len(p.allowedRoutes) == 0 { + return false + } + requestPath, err := requestutil.GetRequestPath(req) + if err != nil { + logger.Errorf("Skipping path-based authentication exemption: %v", err) + return false + } for _, route := range p.allowedRoutes { - if isAllowedMethod(req, route) && isAllowedPath(req, route) { + if isAllowedMethod(req, route) && isAllowedPath(requestPath, route) { return true } } @@ -630,10 +638,16 @@ func (p *OAuthProxy) isTrustedIP(req *http.Request) bool { return false } - remoteAddr, err := ip.GetClientIP(p.realClientIPParser, req) + var remoteAddr net.IP + var err error + scope := middlewareapi.GetRequestScope(req) + if scope != nil && scope.CanTrustForwardedHeaders(req) { + remoteAddr, err = ip.GetClientIPFromTrustedProxy(p.realClientIPParser, req, scope.TrustedProxies) + } else { + remoteAddr, err = ip.GetClientIP(nil, req) + } if err != nil { logger.Errorf("Error obtaining real IP for trusted IP list: %v", err) - // Possibly spoofed X-Real-IP header return false } @@ -883,7 +897,15 @@ func (p *OAuthProxy) doOAuthStart(rw http.ResponseWriter, req *http.Request, ove // OAuthCallback is the OAuth2 authentication flow callback that finishes the // OAuth2 authentication flow func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) { - remoteAddr := ip.GetClientString(p.realClientIPParser, req, true) + realClientIPParser := p.realClientIPParser + var trustedProxies *ip.NetSet + scope := middlewareapi.GetRequestScope(req) + if scope != nil && scope.CanTrustForwardedHeaders(req) { + trustedProxies = scope.TrustedProxies + } else { + realClientIPParser = nil + } + remoteAddr := ip.GetClientString(realClientIPParser, req, trustedProxies, true) // finish the oauth cycle // #nosec G120 -- The default max size in Go is already capped at 10MB so this would be the absolute max and is @@ -918,7 +940,7 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) { // There are a lot of issues opened complaining about missing CSRF cookies. // Try to log the INs and OUTs of OAuthProxy, to be easier to analyse these issues. LoggingCSRFCookiesInOAuthCallback(req, cookieName) - logger.Println(req, logger.AuthFailure, "Invalid authentication via OAuth2: unable to obtain CSRF cookie: %s (state=%s)", err, nonce) + logger.PrintAuthf("", req, logger.AuthFailure, "Invalid authentication via OAuth2: unable to obtain CSRF cookie: %s", err) p.ErrorPage(rw, req, http.StatusForbidden, err.Error(), "Login Failed: Unable to find a valid CSRF token. Please try again.") return } @@ -1344,26 +1366,26 @@ func (p *OAuthProxy) errorJSON(rw http.ResponseWriter, code int) { rw.Write([]byte("{}")) } -// LoggingCSRFCookiesInOAuthCallback Log all CSRF cookies found in HTTP request OAuth callback, -// which were successfully parsed +// LoggingCSRFCookiesInOAuthCallback logs CSRF cookie names, never values, to +// diagnose a missing or invalid CSRF cookie in an OAuth callback. func LoggingCSRFCookiesInOAuthCallback(req *http.Request, cookieName string) { cookies := req.Cookies() if len(cookies) == 0 { - logger.Println(req, logger.AuthFailure, "No cookies were found in OAuth callback.") + logger.PrintAuthf("", req, logger.AuthFailure, "No cookies were found in OAuth callback.") return } for _, c := range cookies { if cookieName == c.Name { - logger.Println(req, logger.AuthFailure, "CSRF cookie %s was found in OAuth callback.", c.Name) + logger.PrintAuthf("", req, logger.AuthFailure, "CSRF cookie %s was found in OAuth callback.", c.Name) return } if strings.HasSuffix(c.Name, "_csrf") { - logger.Println(req, logger.AuthFailure, "CSRF cookie %s was found in OAuth callback, but it is not the expected one (%s).", c.Name, cookieName) + logger.PrintAuthf("", req, logger.AuthFailure, "CSRF cookie %s was found in OAuth callback, but it is not the expected one (%s).", c.Name, cookieName) return } } - logger.Println(req, logger.AuthFailure, "Cookies were found in OAuth callback, but none was a CSRF cookie.") + logger.PrintAuthf("", req, logger.AuthFailure, "Cookies were found in OAuth callback, but none was a CSRF cookie.") } diff --git a/oauthproxy_test.go b/oauthproxy_test.go index b3271e5b..81af11aa 100644 --- a/oauthproxy_test.go +++ b/oauthproxy_test.go @@ -1,16 +1,21 @@ package main import ( + "bytes" "context" "crypto" "encoding/base64" + "errors" "fmt" "io" "net/http" "net/http/httptest" + "net/http/httputil" "net/url" + "path" "regexp" "strings" + "sync/atomic" "testing" "time" @@ -20,6 +25,8 @@ import ( "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/authentication/hmacauth" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/cookies" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/encryption" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/ip" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" internaloidc "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/providers/oidc" sessionscookie "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/sessions/cookie" @@ -27,6 +34,7 @@ import ( "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/util/ptr" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/validation" "github.com/oauth2-proxy/oauth2-proxy/v7/providers" + "github.com/onsi/ginkgo/v2" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -439,6 +447,191 @@ func (patTest *PassAccessTokenTest) getCallbackEndpoint() (httpCode int, cookie return rw.Code, cookie } +func TestOAuthCallbackCSRFCookieLogging(t *testing.T) { + opts := baseTestOptions() + require.NoError(t, validation.Validate(opts)) + proxy, err := NewOAuthProxy(opts, func(string) bool { return true }) + require.NoError(t, err) + + csrf, err := cookies.NewCSRF(proxy.CookieOptions, "synthetic-pkce-verifier") + require.NoError(t, err) + csrfCookie, err := csrf.SetCookie(httptest.NewRecorder(), httptest.NewRequest(http.MethodGet, "/", nil)) + require.NoError(t, err) + secret, err := proxy.CookieOptions.GetSecret() + require.NoError(t, err) + value, _, valid := encryption.Validate(csrfCookie, secret, proxy.CookieOptions.CSRFExpire) + require.True(t, valid) + expiredValue, err := encryption.SignedValue(secret, csrfCookie.Name, value, time.Now().Add(-proxy.CookieOptions.CSRFExpire-time.Hour)) + require.NoError(t, err) + + sessionCookie := &http.Cookie{Name: proxy.CookieOptions.Name, Value: "synthetic-session-credential"} + invalidCookie := &http.Cookie{Name: csrfCookie.Name, Value: "synthetic-invalid-csrf-credential"} + expiredCookie := &http.Cookie{Name: csrfCookie.Name, Value: expiredValue} + otherCookie := &http.Cookie{Name: "_other_csrf", Value: "synthetic-other-csrf-credential"} + testCases := []struct { + name string + cookies []*http.Cookie + diagnostic string + }{ + { + name: "no cookies", + diagnostic: "No cookies were found in OAuth callback.", + }, + { + name: "missing CSRF cookie", + cookies: []*http.Cookie{sessionCookie}, + diagnostic: "Cookies were found in OAuth callback, but none was a CSRF cookie.", + }, + { + name: "invalid expected CSRF cookie", + cookies: []*http.Cookie{sessionCookie, invalidCookie}, + diagnostic: fmt.Sprintf("CSRF cookie %s was found in OAuth callback.", csrfCookie.Name), + }, + { + name: "expired expected CSRF cookie", + cookies: []*http.Cookie{sessionCookie, expiredCookie}, + diagnostic: fmt.Sprintf("CSRF cookie %s was found in OAuth callback.", csrfCookie.Name), + }, + { + name: "other CSRF cookie", + cookies: []*http.Cookie{sessionCookie, otherCookie}, + diagnostic: fmt.Sprintf("CSRF cookie %s was found in OAuth callback, but it is not the expected one (%s).", otherCookie.Name, csrfCookie.Name), + }, + } + loggingCases := []struct { + name string + standard bool + auth bool + request bool + }{ + {name: "all enabled", standard: true, auth: true, request: true}, + {name: "auth disabled", standard: true, request: true}, + {name: "only auth enabled", auth: true}, + {name: "all disabled"}, + } + for _, tc := range testCases { + for _, logging := range loggingCases { + for _, authScheme := range []string{"Basic", "Bearer"} { + t.Run(tc.name+"/"+logging.name+"/"+authScheme, func(t *testing.T) { + var output, errors bytes.Buffer + logger.SetOutput(&output) + logger.SetErrOutput(&errors) + logger.SetStandardEnabled(logging.standard) + logger.SetAuthEnabled(logging.auth) + logger.SetReqEnabled(logging.request) + logger.SetAuthTemplate("AUTH {{.RequestID}} {{.Username}} {{.Status}} {{.Message}}") + logger.SetReqTemplate("REQUEST " + logger.DefaultRequestLoggingFormat) + t.Cleanup(func() { + logger.SetOutput(ginkgo.GinkgoWriter) + logger.SetErrOutput(ginkgo.GinkgoWriter) + logger.SetStandardEnabled(true) + logger.SetAuthEnabled(true) + logger.SetReqEnabled(true) + logger.SetAuthTemplate(logger.DefaultAuthLoggingFormat) + logger.SetReqTemplate(logger.DefaultRequestLoggingFormat) + }) + + query := url.Values{ + "code": {"synthetic-callback-code"}, + "state": {encodeState(csrf.HashOAuthState(), "/", false)}, + } + req := httptest.NewRequest(http.MethodGet, "/oauth2/callback?"+query.Encode(), nil) + req.Header.Set("X-Request-Id", "callback-log-test") + credential := "synthetic-bearer-credential" + if authScheme == "Basic" { + credential = base64.StdEncoding.EncodeToString([]byte("synthetic-user:synthetic-password")) + } + req.Header.Set("Authorization", authScheme+" "+credential) + req.Header.Set("X-Synthetic-Secret", "synthetic-header-credential") + for _, cookie := range tc.cookies { + req.AddCookie(cookie) + } + + rw := httptest.NewRecorder() + proxy.ServeHTTP(rw, req) + + assert.Equal(t, http.StatusForbidden, rw.Code) + assert.Contains(t, rw.Body.String(), "Unable to find a valid CSRF token") + assert.Empty(t, rw.Result().Cookies()) + assert.Empty(t, errors.String()) + logs := output.String() + errors.String() + assert.NotContains(t, logs, credential) + assert.NotContains(t, logs, "synthetic-header-credential") + for _, cookie := range tc.cookies { + assert.NotContains(t, logs, cookie.Value) + } + expectedAuth := "AUTH callback-log-test - AuthFailure " + tc.diagnostic + "\n" + + fmt.Sprintf("AUTH callback-log-test - AuthFailure Invalid authentication via OAuth2: unable to obtain CSRF cookie: CSRF cookie with name '%s' was not found\n", csrfCookie.Name) + if !logging.auth { + expectedAuth = "" + } + if logging.request { + assert.True(t, strings.HasPrefix(output.String(), expectedAuth+"REQUEST "), "unexpected logs: %s", output.String()) + assert.Equal(t, strings.Count(expectedAuth, "\n")+1, strings.Count(output.String(), "\n")) + } else { + assert.Equal(t, expectedAuth, output.String()) + } + }) + } + } + } +} + +type failingSaveSessionStore struct { + sessions.SessionStore +} + +func (failingSaveSessionStore) Save(http.ResponseWriter, *http.Request, *sessions.SessionState) error { + return errors.New("session save failed") +} + +func TestOAuthCallbackClientIPLogging(t *testing.T) { + test, err := NewPassAccessTokenTest(PassAccessTokenTestOptions{ValidToken: true}) + require.NoError(t, err) + t.Cleanup(test.Close) + t.Cleanup(func() { logger.SetErrOutput(ginkgo.GinkgoWriter) }) + test.proxy.sessionStore = failingSaveSessionStore{test.proxy.sessionStore} + test.proxy.realClientIPParser, err = ip.GetRealClientIPParser("X-Forwarded-For") + require.NoError(t, err) + trustedProxies, err := ip.ParseNetSet([]string{"192.0.2.0/24"}) + require.NoError(t, err) + + for _, tc := range []struct { + name string + remoteAddr string + headerValues []string + expected string + }{ + {"Untrusted peer", "198.51.100.23:1234", []string{"10.0.0.5"}, "198.51.100.23"}, + {"Appending trusted proxy", "192.0.2.10:1234", []string{"10.0.0.5", "198.51.100.23, 192.0.2.20"}, "192.0.2.10 (198.51.100.23)"}, + {"Malformed relevant hop", "192.0.2.10:1234", []string{"10.0.0.5, invalid, 192.0.2.20"}, "192.0.2.10"}, + } { + t.Run(tc.name, func(t *testing.T) { + csrf, err := cookies.NewCSRF(test.proxy.CookieOptions, "") + require.NoError(t, err) + req := httptest.NewRequest(http.MethodGet, "/oauth2/callback?code=callback_code&state="+encodeState(csrf.HashOAuthState(), "%2F", false), nil) + req.RemoteAddr = tc.remoteAddr + for _, value := range tc.headerValues { + req.Header.Add("X-Forwarded-For", value) + } + req = middlewareapi.AddRequestScope(req, &middlewareapi.RequestScope{ + ReverseProxy: true, + TrustedProxies: trustedProxies, + }) + csrfCookie, err := csrf.SetCookie(httptest.NewRecorder(), req) + require.NoError(t, err) + req.AddCookie(csrfCookie) + var output bytes.Buffer + logger.SetErrOutput(&output) + rw := httptest.NewRecorder() + test.proxy.OAuthCallback(rw, req) + assert.Equal(t, http.StatusInternalServerError, rw.Code) + assert.Contains(t, output.String(), "Error saving session state for "+tc.expected+": session save failed") + assert.NotContains(t, output.String(), "10.0.0.5") + }) + } +} + // getEndpointWithCookie makes a requests againt the oauthproxy with passed requestPath // and cookie and returns body and status code. func (patTest *PassAccessTokenTest) getEndpointWithCookie(cookie string, endpoint string) (httpCode int, accessToken string) { @@ -2174,9 +2367,18 @@ func baseTestOptions() *options.Options { } func TestTrustedIPs(t *testing.T) { + xffRequest := func(remoteAddr string, values ...string) *http.Request { + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.RemoteAddr = remoteAddr + for _, value := range values { + req.Header.Add("X-Forwarded-For", value) + } + return req + } tests := []struct { name string trustedIPs []string + trustedProxyIPs []string reverseProxy bool realClientIPHeader string req *http.Request @@ -2250,11 +2452,13 @@ func TestTrustedIPs(t *testing.T) { { name: "TrustsLocalhostInReverseProxyMode", trustedIPs: []string{"127.0.0.0/8", "::1"}, + trustedProxyIPs: []string{"192.0.2.10"}, reverseProxy: true, realClientIPHeader: "X-Forwarded-For", req: func() *http.Request { req, _ := http.NewRequest(http.MethodGet, "/", nil) req.Header.Add("X-Forwarded-For", "127.0.0.1") + req.RemoteAddr = "192.0.2.10:1234" return req }(), expectTrusted: true, @@ -2263,11 +2467,13 @@ func TestTrustedIPs(t *testing.T) { { name: "TrustsIP6LocalostInReverseProxyMode", trustedIPs: []string{"127.0.0.0/8", "::1"}, + trustedProxyIPs: []string{"192.0.2.10"}, reverseProxy: true, realClientIPHeader: "X-Forwarded-For", req: func() *http.Request { req, _ := http.NewRequest(http.MethodGet, "/", nil) req.Header.Add("X-Forwarded-For", "::1") + req.RemoteAddr = "192.0.2.10:1234" return req }(), expectTrusted: true, @@ -2276,11 +2482,13 @@ func TestTrustedIPs(t *testing.T) { { name: "DoesNotTrustRandomIP4Address", trustedIPs: []string{"127.0.0.0/8", "::1"}, + trustedProxyIPs: []string{"192.0.2.10"}, reverseProxy: true, realClientIPHeader: "X-Forwarded-For", req: func() *http.Request { req, _ := http.NewRequest(http.MethodGet, "/", nil) req.Header.Add("X-Forwarded-For", "12.34.56.78") + req.RemoteAddr = "192.0.2.10:1234" return req }(), expectTrusted: false, @@ -2289,11 +2497,13 @@ func TestTrustedIPs(t *testing.T) { { name: "DoesNotTrustRandomIP6Address", trustedIPs: []string{"127.0.0.0/8", "::1"}, + trustedProxyIPs: []string{"192.0.2.10"}, reverseProxy: true, realClientIPHeader: "X-Forwarded-For", req: func() *http.Request { req, _ := http.NewRequest(http.MethodGet, "/", nil) req.Header.Add("X-Forwarded-For", "::2") + req.RemoteAddr = "192.0.2.10:1234" return req }(), expectTrusted: false, @@ -2302,11 +2512,13 @@ func TestTrustedIPs(t *testing.T) { { name: "RespectsCorrectHeaderInReverseProxyMode", trustedIPs: []string{"127.0.0.0/8", "::1"}, + trustedProxyIPs: []string{"192.0.2.10"}, reverseProxy: true, realClientIPHeader: "X-Forwarded-For", req: func() *http.Request { req, _ := http.NewRequest(http.MethodGet, "/", nil) req.Header.Add("X-Real-IP", "::1") + req.RemoteAddr = "192.0.2.10:1234" return req }(), expectTrusted: false, @@ -2315,15 +2527,164 @@ func TestTrustedIPs(t *testing.T) { { name: "DoesNotTrustGarbageInReverseProxyMode", trustedIPs: []string{"127.0.0.0/8", "::1"}, + trustedProxyIPs: []string{"192.0.2.10"}, reverseProxy: true, realClientIPHeader: "X-Forwarded-For", req: func() *http.Request { req, _ := http.NewRequest(http.MethodGet, "/", nil) req.Header.Add("X-Forwarded-For", "adsfljk29242as!!") + req.RemoteAddr = "192.0.2.10:1234" return req }(), expectTrusted: false, }, + { + name: "IgnoresRealClientHeaderFromUntrustedPeer", + trustedIPs: []string{"10.0.0.0/8"}, + trustedProxyIPs: []string{"192.0.2.10"}, + reverseProxy: true, + realClientIPHeader: "X-Real-IP", + req: func() *http.Request { + req, _ := http.NewRequest(http.MethodGet, "/", nil) + req.Header.Set("X-Real-IP", "10.0.0.5") + req.RemoteAddr = "198.51.100.23:1234" + return req + }(), + expectTrusted: false, + }, + { + name: "UsesTransportAddressForUntrustedPeer", + trustedIPs: []string{"198.51.100.23"}, + trustedProxyIPs: []string{"192.0.2.10"}, + reverseProxy: true, + realClientIPHeader: "X-Real-IP", + req: func() *http.Request { + req, _ := http.NewRequest(http.MethodGet, "/", nil) + req.Header.Set("X-Real-IP", "10.0.0.5") + req.RemoteAddr = "198.51.100.23:1234" + return req + }(), + expectTrusted: true, + }, + { + name: "UsesRealClientHeaderFromTrustedPeer", + trustedIPs: []string{"10.0.0.0/8"}, + trustedProxyIPs: []string{"192.0.2.10"}, + reverseProxy: true, + realClientIPHeader: "X-Real-IP", + req: func() *http.Request { + req, _ := http.NewRequest(http.MethodGet, "/", nil) + req.Header.Set("X-Real-IP", "10.0.0.5") + req.RemoteAddr = "192.0.2.10:1234" + return req + }(), + expectTrusted: true, + }, + { + name: "RejectsSpoofedLeftmostXFFEntry", + trustedIPs: []string{"10.0.0.0/8"}, + trustedProxyIPs: []string{"192.0.2.10"}, + reverseProxy: true, + realClientIPHeader: "X-Forwarded-For", + req: func() *http.Request { + req, _ := http.NewRequest(http.MethodGet, "/", nil) + req.Header.Set("X-Forwarded-For", "10.0.0.5, 198.51.100.23") + req.RemoteAddr = "192.0.2.10:1234" + return req + }(), + expectTrusted: false, + }, + { + name: "IgnoresXFFFromUntrustedPeer", + trustedIPs: []string{"10.0.0.0/8"}, + trustedProxyIPs: []string{"192.0.2.0/24"}, + reverseProxy: true, + realClientIPHeader: "X-Forwarded-For", + req: xffRequest("198.51.100.23:1234", "10.0.0.5"), + }, + { + name: "RejectsSpoofedRepeatedXFF", + trustedIPs: []string{"10.0.0.0/8"}, + trustedProxyIPs: []string{"192.0.2.0/24"}, + reverseProxy: true, + realClientIPHeader: "X-Forwarded-For", + req: xffRequest("192.0.2.10:1234", "10.0.0.5", "198.51.100.23, 192.0.2.20"), + }, + { + name: "TrustsClientThroughMultipleProxies", + trustedIPs: []string{"10.0.0.0/8"}, + trustedProxyIPs: []string{"192.0.2.0/24"}, + reverseProxy: true, + realClientIPHeader: "X-Forwarded-For", + req: xffRequest("192.0.2.10:1234", " 10.0.0.5:1234 , 192.0.2.20", "192.0.2.21:443"), + expectTrusted: true, + }, + { + name: "TrustsIPv6ClientThroughIPv6Proxy", + trustedIPs: []string{"2001:db8:2::/48"}, + trustedProxyIPs: []string{"2001:db8:1::/48"}, + reverseProxy: true, + realClientIPHeader: "X-Forwarded-For", + req: xffRequest("[2001:db8:1::10]:1234", "[2001:db8:2::23]:1234, 2001:db8:1::20"), + expectTrusted: true, + }, + { + name: "MalformedRelevantHopIsNotExempt", + trustedIPs: []string{"10.0.0.0/8", "192.0.2.0/24"}, + trustedProxyIPs: []string{"192.0.2.0/24"}, + reverseProxy: true, + realClientIPHeader: "X-Forwarded-For", + req: xffRequest("192.0.2.10:1234", "10.0.0.5, invalid, 192.0.2.20"), + }, + { + name: "MissingXFFDoesNotFallBackToExemptPeer", + trustedIPs: []string{"192.0.2.0/24"}, + trustedProxyIPs: []string{"192.0.2.0/24"}, + reverseProxy: true, + realClientIPHeader: "X-Forwarded-For", + req: xffRequest("192.0.2.10:1234"), + }, + { + name: "EntireChainTrustedUsesLeftmost", + trustedIPs: []string{"192.0.2.30"}, + trustedProxyIPs: []string{"192.0.2.0/24"}, + reverseProxy: true, + realClientIPHeader: "X-Forwarded-For", + req: xffRequest("192.0.2.10:1234", "192.0.2.30, 192.0.2.20"), + expectTrusted: true, + }, + { + name: "TrustAllDefaultRetainsLeftmostCompatibility", + trustedIPs: []string{"10.0.0.0/8"}, + reverseProxy: true, + realClientIPHeader: "X-Forwarded-For", + req: xffRequest("192.0.2.10:1234", "10.0.0.5, 198.51.100.23"), + expectTrusted: true, + }, + { + name: "ReverseProxyOffIgnoresXFF", + trustedIPs: []string{"10.0.0.0/8"}, + trustedProxyIPs: []string{"192.0.2.0/24"}, + realClientIPHeader: "X-Forwarded-For", + req: xffRequest("192.0.2.10:1234", "10.0.0.5"), + }, + { + name: "UnixSocketUsesTrustedProxyChain", + trustedIPs: []string{"10.0.0.0/8"}, + trustedProxyIPs: []string{"192.0.2.0/24"}, + reverseProxy: true, + realClientIPHeader: "X-Forwarded-For", + req: xffRequest("@", "10.0.0.5, 192.0.2.20"), + expectTrusted: true, + }, + { + name: "UnixSocketRejectsSpoofedChain", + trustedIPs: []string{"10.0.0.0/8"}, + trustedProxyIPs: []string{"192.0.2.0/24"}, + reverseProxy: true, + realClientIPHeader: "X-Forwarded-For", + req: xffRequest("@", "10.0.0.5, 198.51.100.23"), + }, // Check doesn't trust if garbage is provided (no reverse-proxy). { name: "DoesNotTrustGarbage", @@ -2352,25 +2713,78 @@ func TestTrustedIPs(t *testing.T) { }, } opts.TrustedIPs = tt.trustedIPs + opts.TrustedProxyIPs = tt.trustedProxyIPs opts.ReverseProxy = tt.reverseProxy opts.RealClientIPHeader = tt.realClientIPHeader err := validation.Validate(opts) - assert.NoError(t, err) + require.NoError(t, err) proxy, err := NewOAuthProxy(opts, func(string) bool { return true }) - assert.NoError(t, err) - rw := httptest.NewRecorder() + require.NoError(t, err) - proxy.ServeHTTP(rw, tt.req) - if tt.expectTrusted { - assert.Equal(t, 200, rw.Code) - } else { - assert.Equal(t, 403, rw.Code) + for _, endpoint := range []struct { + name string + path string + trustedStatus int + deniedStatus int + }{ + {"Proxy", "/", http.StatusOK, http.StatusForbidden}, + {"AuthOnly", "/oauth2/auth", http.StatusAccepted, http.StatusUnauthorized}, + } { + t.Run(endpoint.name, func(t *testing.T) { + req := tt.req.Clone(tt.req.Context()) + req.URL.Path = endpoint.path + rw := httptest.NewRecorder() + proxy.ServeHTTP(rw, req) + if tt.expectTrusted { + assert.Equal(t, endpoint.trustedStatus, rw.Code) + } else { + assert.Equal(t, endpoint.deniedStatus, rw.Code) + } + }) } }) } } +func TestTrustedIPsMissingRequestConfiguration(t *testing.T) { + trustedIPs, err := ip.ParseNetSet([]string{"10.0.0.0/8", "192.0.2.0/24"}) + require.NoError(t, err) + trustedProxies, err := ip.ParseNetSet([]string{"192.0.2.0/24"}) + require.NoError(t, err) + parser, err := ip.GetRealClientIPParser("X-Forwarded-For") + require.NoError(t, err) + + tests := []struct { + name string + scope *middlewareapi.RequestScope + remoteAddr string + nilParser bool + expectTrusted bool + }{ + {"Missing scope ignores header", nil, "198.51.100.23:1234", false, false}, + {"Missing scope permits direct trusted client", nil, "10.0.0.5:1234", false, true}, + {"Missing trusted proxies ignores header", &middlewareapi.RequestScope{ReverseProxy: true}, "198.51.100.23:1234", false, false}, + {"Missing parser does not fall back to exempt peer", &middlewareapi.RequestScope{ReverseProxy: true, TrustedProxies: trustedProxies}, "192.0.2.10:1234", true, false}, + {"Missing parser permits direct trusted client", &middlewareapi.RequestScope{ReverseProxy: true, TrustedProxies: trustedProxies}, "10.0.0.5:1234", true, true}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + proxy := &OAuthProxy{trustedIPs: trustedIPs, realClientIPParser: parser} + if test.nilParser { + proxy.realClientIPParser = nil + } + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.RemoteAddr = test.remoteAddr + req.Header.Set("X-Forwarded-For", "10.0.0.5") + if test.scope != nil { + req = middlewareapi.AddRequestScope(req, test.scope) + } + assert.Equal(t, test.expectTrusted, proxy.isTrustedIP(req)) + }) + } +} + func Test_buildRoutesAllowlist(t *testing.T) { type expectedAllowedRoute struct { method string @@ -2657,6 +3071,279 @@ func TestApiRoutes(t *testing.T) { } } +func TestSkipAuthPathRouting(t *testing.T) { + for _, mode := range []string{"default proxy", "raw proxy", "external auth"} { + t.Run(mode, func(t *testing.T) { + var hits, mutations atomic.Int32 + backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + hits.Add(1) + // Model the reported servlet ordering: strip literal matrix parameters, + // then decode and resolve dot segments. + segments := strings.Split(r.URL.EscapedPath(), "/") + for i, segment := range segments { + segments[i], _, _ = strings.Cut(segment, ";") + } + decodedPath, err := url.PathUnescape(strings.Join(segments, "/")) + if err != nil { + http.Error(w, "invalid fixture path", http.StatusBadRequest) + return + } + routedPath := path.Clean(decodedPath) + if strings.HasPrefix(routedPath, "/protected/") && r.Method == http.MethodPost { + mutations.Add(1) + } + _, _ = io.WriteString(w, r.RequestURI) + })) + t.Cleanup(backend.Close) + + opts := baseTestOptions() + opts.ForceJSONErrors = true + if mode == "external auth" { + opts.ReverseProxy = true + opts.TrustedProxyIPs = []string{"127.0.0.1/32"} + } + opts.SkipAuthRoutes = []string{"POST=^/public/"} + opts.UpstreamServers = options.UpstreamConfig{ + ProxyRawPath: ptr.To(mode == "raw proxy"), + Upstreams: []options.Upstream{{ + ID: "backend", Path: "/", URI: backend.URL, + }}, + } + require.NoError(t, validation.Validate(opts)) + proxy, err := NewOAuthProxy(opts, func(string) bool { return true }) + require.NoError(t, err) + + newFrontend := func(t *testing.T, proxy *OAuthProxy) *httptest.Server { + t.Helper() + var handler http.Handler = proxy + if mode == "external auth" { + backendURL, err := url.Parse(backend.URL) + require.NoError(t, err) + forward := httputil.NewSingleHostReverseProxy(backendURL) + handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + authReq := httptest.NewRequest(r.Method, "/oauth2/auth", nil) + authReq.RemoteAddr = "127.0.0.1:4180" + authReq.Header = r.Header.Clone() + authReq.Header.Set("X-Forwarded-Uri", r.RequestURI) + authResponse := httptest.NewRecorder() + proxy.ServeHTTP(authResponse, authReq) + if authResponse.Code != http.StatusAccepted { + w.WriteHeader(authResponse.Code) + return + } + forward.ServeHTTP(w, r) + }) + } + return httptest.NewServer(handler) + } + client := &http.Client{ + CheckRedirect: func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse }, + } + + send := func(t *testing.T, server, target string, cookies []*http.Cookie) (int, string) { + t.Helper() + req, err := http.NewRequest(http.MethodPost, server+target, nil) + require.NoError(t, err) + for _, cookie := range cookies { + req.AddCookie(cookie) + } + resp, err := client.Do(req) + require.NoError(t, err) + defer resp.Body.Close() + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + return resp.StatusCode, string(body) + } + + created, expires := time.Now(), time.Now().Add(time.Hour) + cookieResponse := httptest.NewRecorder() + require.NoError(t, proxy.SaveSession(cookieResponse, httptest.NewRequest(http.MethodGet, "/", nil), &sessions.SessionState{ + User: "user", Email: "user@example.com", CreatedAt: &created, ExpiresOn: &expires, + })) + for _, tc := range []struct { + name string + target string + pattern string + public bool + protected bool + routerRedirect bool + decodedRedirect bool + }{ + {name: "encoded parent", target: "/public/%2e%2e/protected/change", protected: true, decodedRedirect: true}, + {name: "mixed encoded parent", target: "/public/.%2E/protected/change", protected: true, decodedRedirect: true}, + {name: "encoded separators", target: "/public%2f..%2fprotected/change", protected: true, decodedRedirect: true}, + {name: "literal parent", target: "/public/../protected/change", protected: true, routerRedirect: true}, + {name: "current segment", target: "/public/./file", routerRedirect: true}, + {name: "matrix parent", target: "/public/..;/protected/change", protected: true}, + {name: "encoded matrix parent", target: "/public/%2e%2e;/protected/change", protected: true}, + {name: "encoded matrix semicolon control", target: "/public/%2e%2e%3b/protected/change"}, + {name: "matrix value", target: "/public/..;version=1/protected/change", protected: true}, + {name: "matrix current and parent", target: "/public/.;/..;/protected/change", protected: true}, + {name: "nested matrix parents", target: "/public/a/..;/..;/protected/change", protected: true}, + {name: "leading double slash", target: "//protected/public", pattern: "^/public$", protected: true, routerRedirect: true}, + {name: "encoded leading slash", target: "/%2fprotected/public", pattern: "^/public$", protected: true, decodedRedirect: true}, + {name: "repeated slash", target: "/public//file", routerRedirect: true}, + {name: "encoded repeated slash", target: "/public/%2ffile", decodedRedirect: true}, + {name: "valid semicolon", target: "/public/file;version=1"}, + {name: "encoded semicolon", target: "/public/file%3bversion=1"}, + {name: "fragment suffix", target: "/public/file%23suffix"}, + {name: "ordinary public", target: "/public/file", public: true}, + {name: "public query", target: "/public/file?next=/../protected;v=1&tag=%23", public: true}, + {name: "public trailing slash", target: "/public/", pattern: "^/public/$", public: true}, + {name: "trailing slash is significant", target: "/public/file/", pattern: "^/public/file$"}, + {name: "escaped space", target: "/public/a%20b", pattern: "^/public/a b$", public: true}, + {name: "escaped letter", target: "/public/%66ile", pattern: "^/public/file$", public: true}, + {name: "ordinary protected", target: "/protected/change", protected: true}, + } { + t.Run(tc.name, func(t *testing.T) { + // Prove each original target reaches the intended downstream fixture. + status, body := send(t, backend.URL, tc.target, nil) + require.Equal(t, http.StatusOK, status) + require.Equal(t, tc.target, body) + require.EqualValues(t, 1, hits.Swap(0)) + var wantMutations int32 + if tc.protected { + wantMutations = 1 + } + require.Equal(t, wantMutations, mutations.Swap(0)) + + pattern := tc.pattern + if pattern == "" { + pattern = "^/public/" + } + configs := []string{"regex", "route"} + if tc.pattern == "" || tc.protected { + configs = append(configs, "negated route") + } + for _, config := range configs { + t.Run(config, func(t *testing.T) { + opts.SkipAuthRegex, opts.SkipAuthRoutes = nil, nil + switch config { + case "regex": + opts.SkipAuthRegex = []string{pattern} + case "negated route": + opts.SkipAuthRoutes = []string{"POST!=^/protected"} + default: + opts.SkipAuthRoutes = []string{"POST=" + pattern} + } + testProxy, err := NewOAuthProxy(opts, func(string) bool { return true }) + require.NoError(t, err) + frontend := newFrontend(t, testProxy) + t.Cleanup(frontend.Close) + + wantStatus := http.StatusUnauthorized + if tc.public { + wantStatus = http.StatusOK + } else if tc.routerRedirect && mode != "external auth" { + wantStatus = http.StatusMovedPermanently + } + status, _ := send(t, frontend.URL, tc.target, nil) + assert.Equal(t, wantStatus, status) + var wantHits int32 + if tc.public { + wantHits = 1 + } + assert.Equal(t, wantHits, hits.Swap(0)) + assert.Zero(t, mutations.Swap(0)) + + wantStatus = http.StatusOK + wantHits = 1 + redirects := (tc.routerRedirect && mode != "external auth") || + (tc.decodedRedirect && mode == "default proxy") + if redirects { + wantStatus = http.StatusMovedPermanently + wantHits = 0 + } + status, body := send(t, frontend.URL, tc.target, cookieResponse.Result().Cookies()) + assert.Equal(t, wantStatus, status) + if !redirects { + assert.Equal(t, tc.target, body) + } + assert.Equal(t, wantHits, hits.Swap(0)) + assert.Equal(t, wantMutations*wantHits, mutations.Swap(0)) + }) + } + }) + } + }) + } +} + +func TestSkipAuthForwardedTargets(t *testing.T) { + for _, config := range []string{"regex", "route", "negated route", "empty path"} { + t.Run(config, func(t *testing.T) { + opts := baseTestOptions() + opts.ReverseProxy = true + opts.TrustedProxyIPs = []string{"127.0.0.1/32"} + switch config { + case "regex": + opts.SkipAuthRegex = []string{".*"} + case "route": + opts.SkipAuthRoutes = []string{"GET=.*"} + case "negated route": + opts.SkipAuthRoutes = []string{"GET!=^/protected"} + case "empty path": + opts.SkipAuthRoutes = []string{"GET=^$"} + } + require.NoError(t, validation.Validate(opts)) + proxy, err := NewOAuthProxy(opts, func(string) bool { return true }) + require.NoError(t, err) + created, expires := time.Now(), time.Now().Add(time.Hour) + cookieResponse := httptest.NewRecorder() + require.NoError(t, proxy.SaveSession(cookieResponse, httptest.NewRequest(http.MethodGet, "/", nil), &sessions.SessionState{ + User: "user", Email: "user@example.com", CreatedAt: &created, ExpiresOn: &expires, + })) + + for _, target := range []string{ + "/public/../protected", "/public/%2e%2e/protected", + "//protected/public", "/public//file", + "/public/..;/protected", "/public/%2e%2e%3b/protected", + "/public/file;version=1", "/public/%zz", "/public/%", + "public/file", "https://example.com/public", "example.com:443", "*", + "/public#fragment", "/public%23fragment", "/public%3fquery", + "/public\\file", "/public/%00", + } { + t.Run(target, func(t *testing.T) { + for _, authenticated := range []bool{false, true} { + req := httptest.NewRequest(http.MethodGet, "/oauth2/auth", nil) + req.RemoteAddr = "127.0.0.1:4180" + req.Header.Set("X-Forwarded-Uri", target) + if authenticated { + for _, cookie := range cookieResponse.Result().Cookies() { + req.AddCookie(cookie) + } + } + rw := httptest.NewRecorder() + proxy.ServeHTTP(rw, req) + wantStatus := http.StatusUnauthorized + if authenticated { + wantStatus = http.StatusAccepted + } + assert.Equal(t, wantStatus, rw.Code) + assert.Equal(t, target, req.Header.Get("X-Forwarded-Uri")) + assert.Equal(t, "/oauth2/auth", req.RequestURI) + } + }) + } + + for _, method := range []string{http.MethodGet, http.MethodPost} { + t.Run("ordinary public "+method, func(t *testing.T) { + req := httptest.NewRequest(method, "/oauth2/auth", nil) + req.RemoteAddr = "127.0.0.1:4180" + req.Header.Set("X-Forwarded-Uri", "/public/file?query=value") + rw := httptest.NewRecorder() + proxy.ServeHTTP(rw, req) + wantStatus := http.StatusUnauthorized + if config != "empty path" && (config == "regex" || method == http.MethodGet) { + wantStatus = http.StatusAccepted + } + assert.Equal(t, wantStatus, rw.Code) + }) + } + }) + } +} + func TestAllowedRequest(t *testing.T) { upstreamServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(200) diff --git a/pkg/apis/options/options.go b/pkg/apis/options/options.go index ac2b13c8..11226fce 100644 --- a/pkg/apis/options/options.go +++ b/pkg/apis/options/options.go @@ -119,9 +119,9 @@ func NewFlagSet() *pflag.FlagSet { flagSet := pflag.NewFlagSet("oauth2-proxy", pflag.ExitOnError) flagSet.Bool("reverse-proxy", false, "are we running behind a reverse proxy, controls whether headers like X-Real-Ip are accepted") - flagSet.String("real-client-ip-header", "X-Real-IP", "Header used to determine the real IP of the client (one of: X-Forwarded-For, X-Real-IP, X-ProxyUser-IP, X-Envoy-External-Address, or CF-Connecting-IP)") - flagSet.StringSlice("trusted-proxy-ip", []string{}, "list of IPs or CIDR ranges that are allowed to set X-Forwarded-* headers when --reverse-proxy is enabled. Defaults to trusting all IPs for backwards compatibility; configure this to your reverse proxy addresses to prevent header spoofing.") - flagSet.StringSlice("trusted-ip", []string{}, "list of IPs or CIDR ranges to allow to bypass authentication. WARNING: trusting by IP has inherent security flaws, read the configuration documentation for more information.") + flagSet.String("real-client-ip-header", "X-Real-IP", "Header used to determine the real IP of the client when the direct caller is a trusted proxy (one of: X-Forwarded-For, X-Real-IP, X-ProxyUser-IP, X-Envoy-External-Address, or CF-Connecting-IP)") + flagSet.StringSlice("trusted-proxy-ip", []string{}, "list of direct proxy IPs or CIDR ranges allowed to set X-Forwarded-* headers when --reverse-proxy is enabled, including every trusted proxy hop in X-Forwarded-For. Defaults to trusting all IPs for backwards compatibility.") + flagSet.StringSlice("trusted-ip", []string{}, "list of IPs or CIDR ranges to allow to bypass authentication. WARNING: trusting by IP has inherent security flaws; with --reverse-proxy, configure --trusted-proxy-ip and read the configuration documentation.") flagSet.Bool("force-https", false, "force HTTPS redirect for HTTP requests") flagSet.String("redirect-url", "", "the OAuth Redirect URL. ie: \"https://internalapp.yourcompany.com/oauth2/callback\"") flagSet.Bool("relative-redirect-url", false, "allow relative OAuth Redirect URL.") diff --git a/pkg/ip/realclientip.go b/pkg/ip/realclientip.go index db8f2595..a8754ae2 100644 --- a/pkg/ip/realclientip.go +++ b/pkg/ip/realclientip.go @@ -49,18 +49,58 @@ func (p xForwardedForClientIPParser) GetRealClientIP(h http.Header) (net.IP, err if commaIndex := strings.IndexRune(ipStr, ','); commaIndex != -1 { ipStr = ipStr[:commaIndex] } - ipStr = strings.TrimSpace(ipStr) + return parseClientIP(ipStr, p.header) +} + +// GetClientIPFromTrustedProxy obtains the client IP from a header supplied by +// a caller that has already been verified as a trusted proxy. +func GetClientIPFromTrustedProxy(p ipapi.RealClientIPParser, req *http.Request, trustedProxies *NetSet) (net.IP, error) { + if p == nil { + return nil, fmt.Errorf("real client IP parser is required") + } + + xffParser, ok := p.(*xForwardedForClientIPParser) + if !ok || xffParser.header != http.CanonicalHeaderKey("X-Forwarded-For") { + return p.GetRealClientIP(req.Header) + } + + if trustedProxies == nil { + return nil, fmt.Errorf("trusted proxy list is required to parse X-Forwarded-For") + } + + xff := strings.Join(req.Header.Values(xffParser.header), ",") + if xff == "" { + return nil, nil + } + + chain := strings.Split(xff, ",") + for i := len(chain) - 1; i >= 0; i-- { + clientIP, err := parseClientIP(chain[i], xffParser.header) + if err != nil { + return nil, err + } + + if i == 0 || !trustedProxies.Has(clientIP) { + return clientIP, nil + } + } + + return nil, nil +} + +func parseClientIP(ipStr string, header string) (net.IP, error) { + ipStr = strings.TrimSpace(ipStr) if ipHost, _, err := net.SplitHostPort(ipStr); err == nil { ipStr = ipHost } - ip := net.ParseIP(ipStr) - if ip == nil { - return nil, fmt.Errorf("unable to parse ip (%s) from %s header", ipStr, http.CanonicalHeaderKey(p.header)) + clientIP := net.ParseIP(ipStr) + if clientIP == nil { + return nil, fmt.Errorf("unable to parse ip (%s) from %s header", ipStr, http.CanonicalHeaderKey(header)) } - return ip, nil + return clientIP, nil } // GetClientIP obtains the perceived end-user IP address from headers if p != nil else from req.RemoteAddr. @@ -90,11 +130,13 @@ func getRemoteIP(req *http.Request) (net.IP, error) { //revive:enable:indent-error-flow } -// GetClientString obtains the human readable string of the remote IP and optionally the real client IP if available -func GetClientString(p ipapi.RealClientIPParser, req *http.Request, full bool) (s string) { +// GetClientString obtains the human readable string of the remote IP and optionally +// the real client IP. Callers must pass a nil parser unless the peer is trusted. +// Missing or invalid client headers leave only the transport IP in the output. +func GetClientString(p ipapi.RealClientIPParser, req *http.Request, trustedProxies *NetSet, full bool) (s string) { var realClientIPStr string if p != nil { - if realClientIP, err := p.GetRealClientIP(req.Header); err == nil && realClientIP != nil { + if realClientIP, err := GetClientIPFromTrustedProxy(p, req, trustedProxies); err == nil && realClientIP != nil { realClientIPStr = realClientIP.String() } } diff --git a/pkg/ip/realclientip_test.go b/pkg/ip/realclientip_test.go index 3cbca114..3aa490b4 100644 --- a/pkg/ip/realclientip_test.go +++ b/pkg/ip/realclientip_test.go @@ -8,6 +8,7 @@ import ( ipapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/ip" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestGetRealClientIPParser(t *testing.T) { @@ -106,6 +107,93 @@ func TestXForwardedForClientIPParserIgnoresOthers(t *testing.T) { assert.Equal(t, ip, net.ParseIP(expectedIPString)) } +func TestGetClientIPFromTrustedProxy(t *testing.T) { + parser, err := GetRealClientIPParser("X-Forwarded-For") + require.NoError(t, err) + trustedProxies, err := ParseNetSet([]string{"192.0.2.0/24", "2001:db8:1::/48"}) + require.NoError(t, err) + + tests := []struct { + name string + headerValues []string + expectedIP string + errString string + }{ + {"Ignores spoofed leftmost entry", []string{"10.0.0.5, 198.51.100.23"}, "198.51.100.23", ""}, + {"Walks past trusted intermediate proxies", []string{"198.51.100.23, 192.0.2.20, 192.0.2.21"}, "198.51.100.23", ""}, + {"Uses leftmost entry when entire chain is trusted", []string{"192.0.2.30, 192.0.2.20"}, "192.0.2.30", ""}, + {"Repeated fields preserve chain order", []string{"10.0.0.5", "198.51.100.23, 192.0.2.20", "192.0.2.21"}, "198.51.100.23", ""}, + {"Whitespace and IPv4 port", []string{" 198.51.100.23:1234 ,\t192.0.2.20:443 "}, "198.51.100.23", ""}, + {"IPv6 client and proxies", []string{"2001:db8:2::23, 2001:db8:1::20"}, "2001:db8:2::23", ""}, + {"IPv6 port and mixed chain", []string{" [2001:db8:2::23]:1234 , [2001:db8:1::20]:443, 192.0.2.20"}, "2001:db8:2::23", ""}, + {"Stops at first untrusted hop", []string{"10.0.0.5, 198.51.100.22, 198.51.100.23, 192.0.2.20"}, "198.51.100.23", ""}, + {"Ignores malformed entry beyond boundary", []string{"invalid, 198.51.100.23, 192.0.2.20"}, "198.51.100.23", ""}, + {"Missing header", nil, "", ""}, + {"Empty header", []string{""}, "", ""}, + {"Whitespace only", []string{" \t"}, "", "unable to parse ip () from X-Forwarded-For header"}, + {"Malformed rightmost hop", []string{"10.0.0.5, invalid"}, "", "unable to parse ip (invalid) from X-Forwarded-For header"}, + {"Malformed relevant intermediate hop", []string{"10.0.0.5, invalid, 192.0.2.20"}, "", "unable to parse ip (invalid) from X-Forwarded-For header"}, + {"Malformed leftmost in otherwise trusted chain", []string{"invalid, 192.0.2.20"}, "", "unable to parse ip (invalid) from X-Forwarded-For header"}, + {"Empty intermediate hop", []string{"10.0.0.5,,192.0.2.20"}, "", "unable to parse ip () from X-Forwarded-For header"}, + {"Empty last field", []string{"10.0.0.5", ""}, "", "unable to parse ip () from X-Forwarded-For header"}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + req := &http.Request{Header: http.Header{}} + for _, value := range test.headerValues { + req.Header.Add("X-Forwarded-For", value) + } + + clientIP, err := GetClientIPFromTrustedProxy(parser, req, trustedProxies) + if test.errString != "" { + assert.EqualError(t, err, test.errString) + } else { + assert.NoError(t, err) + } + assert.Equal(t, net.ParseIP(test.expectedIP), clientIP) + }) + } +} + +func TestGetClientIPFromTrustedProxyConfiguration(t *testing.T) { + xff, err := GetRealClientIPParser("X-Forwarded-For") + require.NoError(t, err) + realIP, err := GetRealClientIPParser("X-Real-IP") + require.NoError(t, err) + trustAll, err := ParseNetSet([]string{"0.0.0.0/0", "::/0"}) + require.NoError(t, err) + + tests := []struct { + name string + parser ipapi.RealClientIPParser + trustedProxies *NetSet + expectedIP string + errString string + }{ + {"Nil parser", nil, trustAll, "", "real client IP parser is required"}, + {"Nil trusted proxies", xff, nil, "", "trusted proxy list is required to parse X-Forwarded-For"}, + {"Empty trusted proxies", xff, NewNetSet(), "2001:db8::23", ""}, + {"Trust all retains leftmost compatibility", xff, trustAll, "10.0.0.5", ""}, + {"Single value header", realIP, trustAll, "198.51.100.23", ""}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + req := &http.Request{Header: http.Header{ + "X-Forwarded-For": {"10.0.0.5, 2001:db8::23"}, + "X-Real-Ip": {"198.51.100.23"}, + }} + clientIP, err := GetClientIPFromTrustedProxy(test.parser, req, test.trustedProxies) + if test.errString != "" { + assert.EqualError(t, err, test.errString) + } else { + assert.NoError(t, err) + } + assert.Equal(t, net.ParseIP(test.expectedIP), clientIP) + }) + } +} + func TestGetRemoteIP(t *testing.T) { tests := []struct { remoteAddr string @@ -147,6 +235,8 @@ func TestGetRemoteIP(t *testing.T) { func TestGetClientString(t *testing.T) { p := &xForwardedForClientIPParser{header: http.CanonicalHeaderKey("X-Forwarded-For")} + trustedProxies, err := ParseNetSet([]string{"192.0.2.0/24", "2001:db8:1::/48"}) + require.NoError(t, err) tests := []struct { parser ipapi.RealClientIPParser @@ -155,7 +245,7 @@ func TestGetClientString(t *testing.T) { expectedClient string expectedClientFull string }{ - // Should fail quietly, only printing warnings to the log + // Preserve transport-only output when no client address is available. {nil, "", "", "", ""}, // Unix domain socket — no IP available {nil, "@", "", "", ""}, @@ -164,6 +254,13 @@ func TestGetClientString(t *testing.T) { {nil, "10.254.244.165:62750", "", "10.254.244.165", "10.254.244.165"}, // Parser is nil, the contents of X-Forwarded-For should be ignored in all cases. {nil, "[2001:470:26:307:a5a1:1177:2ae3:e9c3]:48290", "127.0.0.1", "2001:470:26:307:a5a1:1177:2ae3:e9c3", "2001:470:26:307:a5a1:1177:2ae3:e9c3"}, + {p, "192.0.2.10:443", "10.0.0.5, 198.51.100.23, 192.0.2.20", "198.51.100.23", "192.0.2.10 (198.51.100.23)"}, + {p, "[2001:db8:1::10]:443", "[2001:db8:2::23]:1234, 2001:db8:1::20", "2001:db8:2::23", "2001:db8:1::10 (2001:db8:2::23)"}, + {p, "192.0.2.10:443", "10.0.0.5, invalid, 192.0.2.20", "192.0.2.10", "192.0.2.10"}, + {p, "192.0.2.10:443", "invalid, 198.51.100.23", "198.51.100.23", "192.0.2.10 (198.51.100.23)"}, + {p, "192.0.2.10:443", "192.0.2.30, 192.0.2.20", "192.0.2.30", "192.0.2.10 (192.0.2.30)"}, + {p, "@", "198.51.100.23, 192.0.2.20", "198.51.100.23", " (198.51.100.23)"}, + {p, "@", "invalid", "", ""}, } for _, test := range tests { @@ -174,10 +271,10 @@ func TestGetClientString(t *testing.T) { RemoteAddr: test.remoteAddr, } - client := GetClientString(test.parser, req, false) + client := GetClientString(test.parser, req, trustedProxies, false) assert.Equal(t, test.expectedClient, client) - clientFull := GetClientString(test.parser, req, true) + clientFull := GetClientString(test.parser, req, trustedProxies, true) assert.Equal(t, test.expectedClientFull, clientFull) } } diff --git a/pkg/requests/util/util.go b/pkg/requests/util/util.go index 568ebcc6..ebfed274 100644 --- a/pkg/requests/util/util.go +++ b/pkg/requests/util/util.go @@ -1,6 +1,7 @@ package util import ( + "errors" "net/http" "net/url" "strings" @@ -45,35 +46,35 @@ func GetRequestURI(req *http.Request) string { return uri } -// GetRequestPath returns the request URI or X-Forwarded-Uri if present and the -// request came from a trusted reverse proxy but always strips the query -// parameters and fragment suffixes and only returns the pure path. -func GetRequestPath(req *http.Request) string { - uri := stripRequestFragment(GetRequestURI(req)) - - // Parse URI and return only the path component - if parsedURL, err := url.Parse(uri); err == nil { - return stripRequestFragment(parsedURL.Path) +// GetRequestPath returns a decoded path suitable for skip-auth matching, using +// X-Forwarded-Uri only for a trusted reverse proxy. An error means the path must +// not grant an authentication exemption. It does not modify the request URL. +func GetRequestPath(req *http.Request) (string, error) { + uri := GetRequestURI(req) + if !strings.HasPrefix(uri, "/") || strings.ContainsAny(uri, "# \t\r\n") { + return "", errors.New("request target is not an unambiguous origin-form URI") } - // Fallback: strip query parameters manually - return stripRequestQuery(uri) -} - -func stripRequestFragment(uri string) string { - if idx := strings.Index(uri, "#"); idx != -1 { - return uri[:idx] + // Unlike url.Parse, ParseRequestURI keeps a leading // in the path. + parsedURL, err := url.ParseRequestURI(uri) + if err != nil { + return "", errors.New("invalid request target") } - - return uri -} - -func stripRequestQuery(uri string) string { - if idx := strings.Index(uri, "?"); idx != -1 { - return uri[:idx] + requestPath := parsedURL.Path + if strings.ContainsAny(requestPath, ";\\#?") || strings.Contains(requestPath, "//") { + return "", errors.New("request path contains ambiguous separators") } - - return uri + for _, char := range requestPath { + if char < 0x20 || char == 0x7f { + return "", errors.New("request path contains a control character") + } + } + for _, segment := range strings.Split(requestPath, "/") { + if segment == "." || segment == ".." { + return "", errors.New("request path contains a dot segment") + } + } + return requestPath, nil } // CanTrustForwardedHeaders determines if forwarded headers should be processed diff --git a/pkg/requests/util/util_test.go b/pkg/requests/util/util_test.go index c4185b35..84e9101a 100644 --- a/pkg/requests/util/util_test.go +++ b/pkg/requests/util/util_test.go @@ -143,6 +143,93 @@ var _ = Describe("Util Suite", func() { }) Context("GetRequestPath", func() { + DescribeTable("preserves unambiguous paths without changing the request", + func(target, expected string) { + for _, forwarded := range []bool{false, true} { + req = httptest.NewRequest(http.MethodGet, target, nil) + if forwarded { + req = httptest.NewRequest(http.MethodGet, "/oauth2/auth", nil) + req.RemoteAddr = "127.0.0.1:4180" + req = middleware.AddRequestScope(req, &middleware.RequestScope{ + ReverseProxy: true, TrustedProxies: trustedProxies, + }) + req.Header.Set("X-Forwarded-Uri", target) + } + originalURL, originalTarget := *req.URL, req.RequestURI + requestPath, err := util.GetRequestPath(req) + Expect(err).NotTo(HaveOccurred()) + Expect(requestPath).To(Equal(expected)) + Expect(*req.URL).To(Equal(originalURL)) + Expect(req.RequestURI).To(Equal(originalTarget)) + } + }, + Entry("root", "/", "/"), + Entry("public path", "/public/file", "/public/file"), + Entry("protected path", "/protected/file", "/protected/file"), + Entry("trailing slash", "/public/", "/public/"), + Entry("query ignored", "/public/file?next=/../protected;v=1&tag=%23", "/public/file"), + Entry("invalid query escape does not affect the path", "/public/file?q=%zz", "/public/file"), + Entry("escaped space", "/public/a%20b", "/public/a b"), + Entry("escaped letter", "/public/%66ile", "/public/file"), + Entry("escaped Unicode", "/public/caf%C3%A9", "/public/caf\u00e9"), + Entry("escaped slash", "/public%2ffile", "/public/file"), + Entry("literal plus", "/public/a+b", "/public/a+b"), + Entry("escaped plus", "/public/a%2bb", "/public/a+b"), + Entry("escaped percent", "/public/100%25", "/public/100%"), + Entry("dot within a segment", "/public/file.txt", "/public/file.txt"), + Entry("multiple dots are not a parent segment", "/public/...", "/public/..."), + ) + + DescribeTable("declines ambiguous or invalid original request targets", + func(target string) { + req.RemoteAddr = "127.0.0.1:4180" + req = middleware.AddRequestScope(req, &middleware.RequestScope{ + ReverseProxy: true, TrustedProxies: trustedProxies, + }) + req.Header.Set("X-Forwarded-Uri", target) + requestPath, err := util.GetRequestPath(req) + Expect(err).To(HaveOccurred()) + Expect(requestPath).To(BeEmpty()) + Expect(err.Error()).NotTo(ContainSubstring("sensitive-value")) + }, + Entry("literal parent segment", "/public/../protected?token=sensitive-value"), + Entry("encoded parent segment", "/public/%2e%2E/protected"), + Entry("encoded slash exposes parent segment", "/public%2f..%2fprotected"), + Entry("current segment", "/public/./file"), + Entry("terminal parent segment", "/public/.."), + Entry("terminal current segment", "/public/."), + Entry("leading double slash", "//protected/public"), + Entry("repeated slash", "/public//file"), + Entry("encoded repeated slash", "/public/%2ffile"), + Entry("matrix parent segment", "/public/..;/protected"), + Entry("encoded matrix parent segment", "/public/%2e%2e%3b/protected"), + Entry("valid matrix parameter", "/public/file;version=1"), + Entry("matrix parameter containing a slash", "/protected;/public"), + Entry("invalid escape", "/public/%zz?token=sensitive-value"), + Entry("incomplete escape", "/public/%"), + Entry("relative target", "public/file"), + Entry("absolute target", "https://example.com/public/file"), + Entry("authority target", "example.com:443"), + Entry("asterisk target", "*"), + Entry("literal fragment", "/public#sensitive-value"), + Entry("encoded fragment", "/public%23sensitive-value"), + Entry("encoded question mark", "/public%3fsensitive-value"), + Entry("backslash", "/public\\..\\protected"), + Entry("encoded backslash", "/public%5c..%5cprotected"), + Entry("encoded control character", "/public/%00"), + Entry("encoded delete character", "/public/%7f"), + Entry("unescaped space", "/public/a b"), + ) + + It("ignores a forged forwarded URI from an untrusted peer", func() { + req.RemoteAddr = "192.0.2.10:4180" + req = middleware.AddRequestScope(req, &middleware.RequestScope{ + ReverseProxy: true, TrustedProxies: trustedProxies, + }) + req.Header.Set("X-Forwarded-Uri", "/public") + Expect(util.GetRequestPath(req)).To(Equal(uriNoQueryParams)) + }) + Context("trusted forwarded headers are disabled", func() { BeforeEach(func() { req = middleware.AddRequestScope(req, &middleware.RequestScope{}) @@ -152,21 +239,25 @@ var _ = Describe("Util Suite", func() { Expect(util.GetRequestPath(req)).To(Equal(uriNoQueryParams)) }) - It("drops fragment content from a parsed request path", func() { + It("declines fragment content in a parsed request path", func() { // Simulate net/http ParseRequestURI preserving '#' in URL.Path. req.URL.Path = "/foo/secret#/bar" req.URL.RawPath = "/foo/secret%23/bar" - Expect(util.GetRequestPath(req)).To(Equal("/foo/secret")) + requestPath, err := util.GetRequestPath(req) + Expect(err).To(HaveOccurred()) + Expect(requestPath).To(BeEmpty()) }) - It("drops fragment-like suffixes from encoded number signs", func() { + It("declines encoded number signs", func() { req = httptest.NewRequest( http.MethodGet, fmt.Sprintf("%s://%s/foo/secret%%23/bar?query=param", proto, host), nil, ) req = middleware.AddRequestScope(req, &middleware.RequestScope{}) - Expect(util.GetRequestPath(req)).To(Equal("/foo/secret")) + requestPath, err := util.GetRequestPath(req) + Expect(err).To(HaveOccurred()) + Expect(requestPath).To(BeEmpty()) }) It("ignores X-Forwarded-Uri and returns the URI (without query params)", func() { @@ -193,9 +284,11 @@ var _ = Describe("Util Suite", func() { Expect(util.GetRequestPath(req)).To(Equal("/some/other/path")) }) - It("drops fragment-like suffixes from the X-Forwarded-Uri", func() { + It("declines fragment-like suffixes from the X-Forwarded-Uri", func() { req.Header.Add("X-Forwarded-Uri", "/foo/secret%23/bar?query=param") - Expect(util.GetRequestPath(req)).To(Equal("/foo/secret")) + requestPath, err := util.GetRequestPath(req) + Expect(err).To(HaveOccurred()) + Expect(requestPath).To(BeEmpty()) }) }) }) diff --git a/pkg/validation/allowlist.go b/pkg/validation/allowlist.go index dcc0d361..3083d818 100644 --- a/pkg/validation/allowlist.go +++ b/pkg/validation/allowlist.go @@ -20,7 +20,7 @@ func validateAllowlists(o *options.Options) []string { msgs = append(msgs, validateTrustedIPs(o)...) if len(o.TrustedIPs) > 0 && o.ReverseProxy { - _, err := fmt.Fprintln(os.Stderr, "WARNING: mixing --trusted-ip with --reverse-proxy is a potential security vulnerability. An attacker can inject a trusted IP into an X-Real-IP or X-Forwarded-For header if they aren't properly protected outside of oauth2-proxy") + _, err := fmt.Fprintln(os.Stderr, "WARNING: --trusted-ip bypasses authentication. When using it with --reverse-proxy, configure --trusted-proxy-ip and ensure the proxy overwrites the selected real-client-IP header (or safely appends X-Forwarded-For).") if err != nil { panic(err) } diff --git a/pkg/validation/options.go b/pkg/validation/options.go index 13ce2e0b..cab48709 100644 --- a/pkg/validation/options.go +++ b/pkg/validation/options.go @@ -8,6 +8,7 @@ import ( "net/url" "strings" + middlewareapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/middleware" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/authentication/hmacauth" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/ip" @@ -89,7 +90,15 @@ func Validate(o *options.Options) error { // Allow the logger to get client IPs logger.SetGetClientFunc(func(r *http.Request) string { - return ip.GetClientString(o.GetRealClientIPParser(), r, false) + parser := o.GetRealClientIPParser() + var trustedProxies *ip.NetSet + scope := middlewareapi.GetRequestScope(r) + if scope != nil && scope.CanTrustForwardedHeaders(r) { + trustedProxies = scope.TrustedProxies + } else { + parser = nil + } + return ip.GetClientString(parser, r, trustedProxies, false) }) } diff --git a/pkg/validation/options_test.go b/pkg/validation/options_test.go index 5ea748c1..710fad78 100644 --- a/pkg/validation/options_test.go +++ b/pkg/validation/options_test.go @@ -1,16 +1,24 @@ package validation import ( + "bytes" "crypto" + "net/http" + "net/http/httptest" "net/url" "os" "strings" "testing" "time" + middlewareapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/middleware" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/ip" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/util/ptr" + "github.com/onsi/ginkgo/v2" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) const ( @@ -224,6 +232,65 @@ func TestRealClientIPHeader(t *testing.T) { assert.Nil(t, o.GetRealClientIPParser()) } +func TestRealClientIPLogging(t *testing.T) { + t.Cleanup(func() { + logger.SetOutput(ginkgo.GinkgoWriter) + logger.SetAuthTemplate(logger.DefaultAuthLoggingFormat) + logger.SetReqTemplate(logger.DefaultRequestLoggingFormat) + logger.SetGetClientFunc(func(r *http.Request) string { return r.RemoteAddr }) + }) + tests := []struct { + name string + remoteAddr string + headerValues []string + reverseProxy bool + nilParser bool + expected string + }{ + {"Untrusted peer", "198.51.100.23:1234", []string{"10.0.0.5"}, true, false, "198.51.100.23"}, + {"Appending trusted proxy", "192.0.2.10:1234", []string{"10.0.0.5, 198.51.100.23"}, true, false, "198.51.100.23"}, + {"Repeated fields and multiple proxies", "192.0.2.10:1234", []string{"10.0.0.5", "198.51.100.23, 192.0.2.20", "192.0.2.21"}, true, false, "198.51.100.23"}, + {"Legitimate client", "192.0.2.10:1234", []string{"10.0.0.5, 192.0.2.20"}, true, false, "10.0.0.5"}, + {"Missing header", "192.0.2.10:1234", nil, true, false, "192.0.2.10"}, + {"Malformed relevant hop", "192.0.2.10:1234", []string{"10.0.0.5, invalid, 192.0.2.20"}, true, false, "192.0.2.10"}, + {"Nil parser", "192.0.2.10:1234", []string{"10.0.0.5"}, true, true, "192.0.2.10"}, + {"Reverse proxy disabled in scope", "192.0.2.10:1234", []string{"10.0.0.5"}, false, false, "192.0.2.10"}, + {"IPv6", "[2001:db8:1::10]:1234", []string{"10.0.0.5", " [2001:db8:2::23]:443 , 2001:db8:1::20"}, true, false, "2001:db8:2::23"}, + {"Unix socket", "@", []string{"10.0.0.5, 198.51.100.23"}, true, false, "198.51.100.23"}, + {"Entire chain trusted", "192.0.2.10:1234", []string{"192.0.2.30, 192.0.2.20"}, true, false, "192.0.2.30"}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + o := testOptions() + o.ReverseProxy = true + o.RealClientIPHeader = "X-Forwarded-For" + o.TrustedProxyIPs = []string{"192.0.2.0/24", "2001:db8:1::/48"} + o.Logging.AuthFormat = "{{.Client}}" + o.Logging.RequestFormat = "{{.Client}}" + require.NoError(t, Validate(o)) + if test.nilParser { + o.SetRealClientIPParser(nil) + } + trustedProxies, err := ip.ParseNetSet(o.TrustedProxyIPs) + require.NoError(t, err) + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.RemoteAddr = test.remoteAddr + for _, value := range test.headerValues { + req.Header.Add("X-Forwarded-For", value) + } + req = middlewareapi.AddRequestScope(req, &middlewareapi.RequestScope{ + ReverseProxy: test.reverseProxy, + TrustedProxies: trustedProxies, + }) + var output bytes.Buffer + logger.SetOutput(&output) + logger.PrintAuthf("", req, logger.AuthFailure, "denied") + logger.PrintReq("", "", req, *req.URL, time.Now(), http.StatusForbidden, 0) + assert.Equal(t, test.expected+"\n"+test.expected+"\n", output.String()) + }) + } +} + func TestProviderCAFilesError(t *testing.T) { file, err := os.CreateTemp("", "absent.*.crt") assert.NoError(t, err)