Skip to content
This repository was archived by the owner on Dec 7, 2020. It is now read-only.

Added --preserve-host option to retain host header in upstream request. #328

Merged
merged 3 commits into from
Jul 5, 2018
Merged

Added --preserve-host option to retain host header in upstream request. #328

merged 3 commits into from
Jul 5, 2018

Conversation

jgroffen
Copy link
Contributor

@jgroffen jgroffen commented Mar 8, 2018

Useful for upstream webapps that rely on the host header, for instance to generate weblinks that match the host originally requested.

Similar to the Apache mod_proxy ProxyPreserveHost option: https://httpd.apache.org/docs/trunk/mod/mod_proxy.html#proxypreservehost

If a host header is set in the headers option, --preserve-host is ignored.

@jgroffen
Copy link
Contributor Author

jgroffen commented Mar 8, 2018

Rebased from master to resolve merge conflicts (and keep this PR to a single commit).

@@ -50,7 +50,7 @@ func (r *oauthProxy) proxyMiddleware(next http.Handler) http.Handler {
if v := req.Header.Get("Host"); v != "" {
req.Host = v
req.Header.Del("Host")
} else {
} else if !r.config.PreserveHost {
Copy link
Contributor

@gambol99 gambol99 Mar 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @jgroffen ... so flag is used to control the Host header from the incoming request be preserved for upstream, which the if statement just above is doing. .. perhaps i'm missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @gambol99, I found that without this patch all code paths would change the host header of the upstream request - either to the configured upstream host name or an overridden header in the headers config. Maybe there was something screwy with my config though, I'll mark it as WIP and check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @gambol99 - I'm finding that this if statement on line 50 is never equating to true - I don't know why because the incoming request does have a host header.

I verified the following configuration:

listen : kcproxy.internal.com
upstream-url : webserver.internal.com

... when keycloak-proxy is behind a public-facing proxy with a request chain that looks like:

  • public.com --proxy to--> kcproxy.internal.com --proxy to--> webserver.internal.com

... produces the following results:

  1. request to public.com host header: public.com
  2. request to kcpublic.internal.com host header: public.com
  3. request to webserver.internal.com host header: webserver.internal.com

Note that PreserveHost is enabled for the public.com proxy - an Apache HTTPD proxy in this case.

I verified this result using the 2.2.1 released binary. I also checked a request with a valid token vs a request that has no auth yet and performs OIDC redicrect to keycloak (where SSO happens).

If the preserve-host parameter is set in my changed codebase, the following happens instead:

  1. request to public.com host header: public.com
  2. request to kcpublic.internal.com host header: public.com
  3. request to webserver.internal.com host header: public.com

We made a change where we don't need to preserve the host header anymore (why I took so long to get back to you, sorry :( ). Preserving host headers is not a preferred approach, but some applications rely on it.

@jgroffen jgroffen changed the title Added --preserve-host option to retain host header in upstream request. [WIP] Added --preserve-host option to retain host header in upstream request. Mar 12, 2018
@jgroffen jgroffen changed the title [WIP] Added --preserve-host option to retain host header in upstream request. Added --preserve-host option to retain host header in upstream request. Jun 27, 2018
@gambol99
Copy link
Contributor

gambol99 commented Jul 5, 2018

LGTM .. thank you kindly @jgroffen

@gambol99 gambol99 merged commit 7eb2392 into louketo:master Jul 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants