sso-proxy: add preserve host configuration option #55

sporkmonger · 2018-09-18T00:22:09Z

When pointing the proxy directly at an AWS ALB, e.g., the host name will be xxx.yyy.zzz.elb.amazonaws.com, however the service itself will typically be expecting a hostname akin to servicename.intranet.company.com. This change adds a config option that will send the original Host header sent by the client through without change. It also changes the behavior of the X-Forwarded-Host header handling code so that the header will only be set if it doesn't already exist. Currently the existing code will create a second X-Forwarded-Host with potentially a different value if there's a proxy chain, which seems likely to be a bug.

Since the new config option is named PreserveHost and the default zero value is false, the default behavior should continue to be the current behavior. However for either config value this will fix the broken X-Forwarded-Host behavior.

Fixes #52. Will conflict with #49. I'll clean up whichever gets merged second.

sporkmonger · 2018-09-18T21:33:42Z

Ready for review.

sporkmonger · 2018-09-25T21:03:06Z

I think this could be a table-driven test as well, but would need to edit existing tests rather than creating new ones I think.

shrayolacrayon · 2018-10-17T19:42:34Z

internal/proxy/oauthproxy_test.go

@@ -263,6 +263,129 @@ func TestNewReverseProxyTLSSkipVerify(t *testing.T) {
 	}
 }

+func TestNewReverseProxyPreserveHost(t *testing.T) {


can we make these table drive tests as well?

Almost done w/ these and so far confirmed that replacing a bunch of test functions w/ two bigger table-driven test functions hasn't altered code coverage.

shrayolacrayon · 2018-10-17T19:43:13Z

internal/proxy/proxy_config.go

@@ -81,6 +82,7 @@ type OptionsConfig struct {
 	SkipAuthRegex   []string          `yaml:"skip_auth_regex"`
 	AllowedGroups   []string          `yaml:"allowed_groups"`
 	TLSSkipVerify   bool              `yaml:"tls_skip_verify"`
+	PreserveHost    bool              `yaml:"preserve_host"`


might be good to add a config unit test as well

jphines · 2018-11-16T21:22:19Z

👋 hi @sporkmonger!

I'll be helping you get this PR merged! Looks like there are some lingering comments from @shrayolacrayon -- is there anything from us you need to get those resolved?

sporkmonger · 2018-11-17T01:48:01Z

Hey, sorry about the delay, was heads-down on the Azure AD PR. I’ve got this on my plate next.

sporkmonger · 2018-11-28T03:10:48Z

This should be ready to review again.

digitalresistor · 2018-11-28T23:02:53Z

X-Forwarded-Host should be specified multiple times, preferably each time a new proxy forwards it and had received the request using an alternate "Host:" line from an upstream.

Having two is not a bug.

HTTP header folding will change:

X-Forwarded-Host: example.com
X-Forwarded-Host: whatever.internal.example.com

into:

X-Forwarded-Host: example.com, whatever.internal.example.com

It is up to client libraries to parse the X-Forwarded-Host and use the first value: example.com (the proxy closest to the original client), unless it only trusts 1 proxy, in which case it should use whatever.internal.example.com instead.

Yes, why do you have to count the proxies you trust, well because the following:

X-Forwarded-Host: attacker.com, example.com, whatever.internal.example.com, lb.whatever.example.com

And the application is configured to only support 3 proxies, then attacker.com should be dropped as it is not trusted, and is likely from an attacker sending an X-Forwarded-Host directly to your first proxy, which is dutifully passing it on as it doesn't strip HTTP headers (Amazon ELB for instance doesn't strip those headers, it also doesn't add an X-Forwarded-Host, but instead passes the Host: header through untouched...).

Please don't remove the multiple X-Forwarded-Host headers, unless there is some knob to turn that on/off.

sporkmonger · 2018-11-28T23:44:46Z

I'd advocate for using https://tools.ietf.org/html/rfc7239, which is actually specified, unlike X-Forwarded-Host, but sadly it's not well supported yet. But yeah, this probably shouldn't have been in this PR in the first place. Dropped it.

digitalresistor · 2018-11-29T05:35:51Z

internal/proxy/oauthproxy.go

@@ -212,9 +214,13 @@ func NewRewriteReverseProxy(route *RewriteRoute, config *UpstreamConfig) *httput
 		}
 		director := httputil.NewSingleHostReverseProxy(target).Director

-		req.Header.Add("X-Forwarded-Host", req.Host)
+		if req.Header.Get("X-Forwarded-Host") == "" {


Here is one more instance where a new X-Forwarded-Host is not added if there is already one in the request.

sporkmonger · 2018-11-29T21:31:34Z

internal/proxy/oauthproxy_test.go

@@ -427,7 +427,7 @@ func TestRoundTrip(t *testing.T) {
 	}{
 		{
 			name: "no error",
-			url:  "https://www.example.com/",
+			url:  "http://www.example.com/",


Looks like it's already been fixed actually.

jphines · 2018-11-30T17:14:23Z

@bertjwregeer Thanks for the review! I was going to leave a similar comment regarding the X-Forwarded-Host and that I wasn't super comfortable being with it potentially being overridden by the client.

@sporkmonger Going to run some integration tests and see if I can get this merged this afternoon.

digitalresistor · 2018-12-01T02:48:36Z

@jphines I'm a maintainer for a pure python web server, I've been knee deep in X-Forwarded-* stuff for supporting proxies and munging the WSGI environment to match, so I did a drive by review :-)

sporkmonger · 2018-12-01T02:51:01Z

Much appreciated. I’m pretty familiar with `X-Forwarded-For`, but hadn’t ever needed to handle `X-Forwarded-Host` prior.

sporkmonger force-pushed the preserve-host-config branch 3 times, most recently from 7a4bd0f to f362a2e Compare September 18, 2018 21:17

sporkmonger force-pushed the preserve-host-config branch 3 times, most recently from 40c5338 to 2f0eb24 Compare September 24, 2018 20:12

sporkmonger force-pushed the preserve-host-config branch from 2f0eb24 to b262bfe Compare October 3, 2018 01:39

shrayolacrayon reviewed Oct 17, 2018

View reviewed changes

shrayolacrayon assigned jphines Nov 2, 2018

mreiferson mentioned this pull request Nov 26, 2018

sso-proxy: configuration option to preserve client Host header #52

Closed

mreiferson changed the title ~~Add preserve host configuration option~~ sso-proxy: add preserve host configuration option Nov 26, 2018

mreiferson added the enhancement New feature or request label Nov 26, 2018

sporkmonger and others added 3 commits November 27, 2018 19:09

Add preserve host configuration option

2bc72ab

Add config test for preserve_host option

efe1190

Roll everything up into a pair of table driven tests

98c64ce

sporkmonger force-pushed the preserve-host-config branch from e61ec54 to 98c64ce Compare November 28, 2018 03:09

Bob Aman added 2 commits November 28, 2018 15:40

Avoid TLS failure due to expired cert on example.com

be27614

Don't limit to a single X-Forwarded-Host after all

21a072e

digitalresistor reviewed Nov 29, 2018

View reviewed changes

sporkmonger commented Nov 29, 2018

View reviewed changes

Don't limit to a single X-Forwarded-Host after all

5b76cd6

jphines approved these changes Dec 1, 2018

View reviewed changes

Merge branch 'master' into preserve-host-config

9f81a78

jphines merged commit 6a6f920 into buzzfeed:master Dec 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sso-proxy: add preserve host configuration option #55

sso-proxy: add preserve host configuration option #55

sporkmonger commented Sep 18, 2018

sporkmonger commented Sep 18, 2018

sporkmonger commented Sep 25, 2018

shrayolacrayon Oct 17, 2018

sporkmonger Nov 28, 2018

sporkmonger Nov 28, 2018

shrayolacrayon Oct 17, 2018

sporkmonger Nov 28, 2018

jphines commented Nov 16, 2018

sporkmonger commented Nov 17, 2018 •

edited

Loading

sporkmonger commented Nov 28, 2018

digitalresistor commented Nov 28, 2018

sporkmonger commented Nov 28, 2018

digitalresistor Nov 29, 2018

sporkmonger Nov 29, 2018

jphines commented Nov 30, 2018

digitalresistor commented Dec 1, 2018

sporkmonger commented Dec 1, 2018 via email •

edited

Loading

sso-proxy: add preserve host configuration option #55

sso-proxy: add preserve host configuration option #55

Conversation

sporkmonger commented Sep 18, 2018

sporkmonger commented Sep 18, 2018

sporkmonger commented Sep 25, 2018

shrayolacrayon Oct 17, 2018

Choose a reason for hiding this comment

sporkmonger Nov 28, 2018

Choose a reason for hiding this comment

sporkmonger Nov 28, 2018

Choose a reason for hiding this comment

shrayolacrayon Oct 17, 2018

Choose a reason for hiding this comment

sporkmonger Nov 28, 2018

Choose a reason for hiding this comment

jphines commented Nov 16, 2018

sporkmonger commented Nov 17, 2018 • edited Loading

sporkmonger commented Nov 28, 2018

digitalresistor commented Nov 28, 2018

sporkmonger commented Nov 28, 2018

digitalresistor Nov 29, 2018

Choose a reason for hiding this comment

sporkmonger Nov 29, 2018

Choose a reason for hiding this comment

jphines commented Nov 30, 2018

digitalresistor commented Dec 1, 2018

sporkmonger commented Dec 1, 2018 via email • edited Loading

sporkmonger commented Nov 17, 2018 •

edited

Loading

sporkmonger commented Dec 1, 2018 via email •

edited

Loading