Make TLS verification of upstream servers configurable #49

sporkmonger · 2018-09-15T00:21:28Z

When pointing the proxy directly at an AWS ALB, e.g., the cert presented by the ALB is guaranteed to be invalid, as you can't get a cert for *.elb.amazonaws.com. Depending on your deployment setup you may or may not be able to easily automate creation of corresponding DNS records to match your certs. And of course your certs might be self-signed. Therefore it's desirable to be able to disable TLS verification for upstream servers in some cases. Particularly because the real security barrier is the necessary firewall rule that ensures nothing else can talk to the upstream server.

Fixes #47.

shrayolacrayon

Thank you for opening @sporkmonger, some initial requests!

shrayolacrayon · 2018-09-18T21:09:59Z

internal/proxy/proxy_config.go

@@ -48,6 +48,7 @@ type UpstreamConfig struct {

 	SkipAuthCompiledRegex []*regexp.Regexp
 	AllowedGroups         []string
+	TLSVerify             bool


It looks like in this case, the default for TLSVerify would be false, which would be a less secure default. Can you change this field to be InsecureSkipVerify, which when set to true, will enable InsecureSkipVerify? This will allow us to default to a more secure configuration.

Good point, and agree that default should be to verify. I meant to address that and forgot. However InsecureSkipVerify is a good name only inside a config struct named TLSConfig. It's not obvious that it's even TLS related outside that context. Given that, I'd be inclined to explicitly set a default rather than relying on zero values.

What if we make the name TLSSkipVerify?

Main reason I'd choose TLSVerify over TLSSkipVerify is that TLSVerify / TLS_VERIFY / --tls-verify and similar are in wide usage in other tools, whereas TLSSkipVerify would be relatively uncommon. I also generally don't like to let language details (like what Go's zero values are) to leak out into user-facing interface design, and I think of config as part of the interface design.

"--tls-verify": 23,400 search results
"TLSVerify": 12,800 search results
"--tls-skip-verify": 2,210 search results
"TLSSkipVerify": 1,600 search results

That said, there's certainly other tools using TLSSkipVerify, like Vault and KSonnet, so it's not without precedent. Funny how it seems to be mostly prevalent in Go apps. 😛

shrayolacrayon · 2018-09-18T21:10:30Z

internal/proxy/proxy_config.go

@@ -75,6 +76,7 @@ type OptionsConfig struct {
 	HeaderOverrides map[string]string `yaml:"header_overrides"`
 	SkipAuthRegex   []string          `yaml:"skip_auth_regex"`
 	AllowedGroups   []string          `yaml:"allowed_groups"`
+	TLSVerify       bool              `yaml:"tls_verify"`


(nit) Can we add a unit test for this config value to ensure that it is being set?

Absolutely. I was having trouble understanding the test suite when I first wrote the PR, but a couple PRs in, I think it should be no problem.

sporkmonger · 2018-09-18T22:28:01Z

Added tests for TLS verification, wanted to make sure everyone's OK w/ keeping the TLSVerify configuration key name and just explicitly setting a default value before proceeding.

sporkmonger · 2018-09-19T00:44:09Z

Ended up writing the code for it because it turned out to be easier than I thought it would be, but it's super easy to switch if you do prefer TLSSkipVerify.

sporkmonger · 2018-09-19T00:47:07Z

internal/proxy/proxy_config.go

-	proxy.TLSVerify = proxy.RouteConfig.Options.TLSVerify
+	if proxy.RouteConfig.Options.TLSVerify != nil {
+		proxy.TLSVerify = *proxy.RouteConfig.Options.TLSVerify
+	}


One item of note, we don't want to set the explicit default value here because of the if proxy.RouteConfig.Options == nil short-circuit at the top of the function. Doing so causes half the test suite to fail. I put it in loadServiceConfigs instead.

shrayolacrayon · 2018-09-19T19:12:56Z

internal/proxy/proxy_config.go

@@ -75,6 +76,7 @@ type OptionsConfig struct {
 	HeaderOverrides map[string]string `yaml:"header_overrides"`
 	SkipAuthRegex   []string          `yaml:"skip_auth_regex"`
 	AllowedGroups   []string          `yaml:"allowed_groups"`
+	TLSVerify       *bool             `yaml:"tls_verify"`


I think it still makes sense to use TLSSkipVerify as a bool, which removes the need for any default setting on load of the service configs.

OK, I'll change it.

sporkmonger · 2018-09-19T19:36:02Z

OK, it's been switched to TLSSkipVerify.

shrayolacrayon · 2018-09-25T20:25:27Z

internal/proxy/oauthproxy_test.go

@@ -201,6 +201,110 @@ func TestNewReverseProxyHostname(t *testing.T) {

 }

+func TestNewReverseProxyTLSSkipVerifyFalse(t *testing.T) {


While some of our tests in this file are not, we are trying to standardize on writing table driven tests in our codebase, could you consolidate these two tests into one function?

I prefer table driven tests, was just mirroring what was already there. Happy to convert over.

sporkmonger · 2018-09-25T21:00:38Z

Does it make it easier to review if I squash earlier in the PR or at the end?

shrayolacrayon · 2018-09-25T21:11:40Z

At the end would be easier, thanks for asking, this also looks good to me if you'd like to rebase + squash, we can get this merged!

sporkmonger · 2018-09-25T21:31:41Z

All rebased and squashed!

shrayolacrayon · 2018-09-25T22:14:31Z

internal/proxy/oauthproxy.go


 // RoundTrip round trips the request and deletes security headers before returning the response.
 func (t *upstreamTransport) RoundTrip(req *http.Request) (*http.Response, error) {
-	resp, err := http.DefaultTransport.RoundTrip(req)
+	transport := &http.Transport{


Rather than creating a new transport every time, can we make this initialize one once and reuse it instead? This will hopefully reduce the overhead of TCP connections made.

shrayolacrayon · 2018-09-25T22:20:11Z

Added one more request!

sporkmonger · 2018-09-25T22:38:29Z

No worries! Downside is it's a little more repetitious this way, but because of the shape of the structure and pointers, it's a little more challenging to factor that out in a way that wouldn't be buggy. Open to ideas.

shrayolacrayon · 2018-09-25T23:04:45Z

internal/proxy/oauthproxy.go

@@ -167,7 +154,21 @@ func (t *upstreamTransport) RoundTrip(req *http.Request) (*http.Response, error)
 // It adds an X-Forwarded-Host header that is the request's host.
 func NewReverseProxy(to *url.URL, config *UpstreamConfig) *httputil.ReverseProxy {
 	proxy := httputil.NewSingleHostReverseProxy(to)
-	proxy.Transport = &upstreamTransport{InsecureSkipVerify: config.TLSSkipVerify}
+	proxy.Transport = &upstreamTransport{


nit: we can probably create a function that initializes the upstreamTransport{}, something like NewUpstreamTransport(insecureSkipVerify bool) so that we're not repeating the config code twice?

Yeah, I like that solution better.

sporkmonger · 2018-09-25T23:16:35Z

OK, I think this should be ready to go. Squash again?

sporkmonger · 2018-09-25T23:26:07Z

Thanks for the help getting the PR cleaned up!

while1eq1 · 2018-10-03T03:30:20Z

Beautiful. Ive already been using this prior to the merge. Thanks @sporkmonger

sporkmonger force-pushed the tls-verify-config branch from f6edec7 to a6a87fa Compare September 17, 2018 22:32

sporkmonger mentioned this pull request Sep 18, 2018

sso-proxy: add preserve host configuration option #55

Merged

sporkmonger force-pushed the tls-verify-config branch from a6a87fa to e4679d6 Compare September 18, 2018 18:03

shrayolacrayon reviewed Sep 18, 2018

View reviewed changes

sporkmonger force-pushed the tls-verify-config branch 2 times, most recently from a0b0d5b to 5c24ded Compare September 18, 2018 22:26

sporkmonger force-pushed the tls-verify-config branch from 5c24ded to 9a1a35f Compare September 18, 2018 22:30

sporkmonger commented Sep 19, 2018

View reviewed changes

shrayolacrayon reviewed Sep 19, 2018

View reviewed changes

sporkmonger force-pushed the tls-verify-config branch from f311a8f to 3eca28c Compare September 19, 2018 19:34

sporkmonger force-pushed the tls-verify-config branch from 3eca28c to 735ed1f Compare September 24, 2018 20:12

shrayolacrayon reviewed Sep 25, 2018

View reviewed changes

sporkmonger force-pushed the tls-verify-config branch from 735ed1f to 9440668 Compare September 25, 2018 20:59

sporkmonger force-pushed the tls-verify-config branch 2 times, most recently from e2eed96 to 780b504 Compare September 25, 2018 21:31

shrayolacrayon reviewed Sep 25, 2018

View reviewed changes

Configurable TLS verification of upstream servers

3a839e6

sporkmonger force-pushed the tls-verify-config branch from d3801cf to 3a839e6 Compare September 25, 2018 23:25

Merge branch 'master' into tls-verify-config

2a908a4

shrayolacrayon approved these changes Oct 1, 2018

View reviewed changes

shrayolacrayon merged commit 9776b52 into buzzfeed:master Oct 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make TLS verification of upstream servers configurable #49

Make TLS verification of upstream servers configurable #49

sporkmonger commented Sep 15, 2018

shrayolacrayon left a comment

shrayolacrayon Sep 18, 2018

sporkmonger Sep 18, 2018

shrayolacrayon Sep 18, 2018

sporkmonger Sep 18, 2018 •

edited

Loading

sporkmonger Sep 18, 2018 •

edited

Loading

shrayolacrayon Sep 18, 2018

sporkmonger Sep 18, 2018 •

edited

Loading

sporkmonger commented Sep 18, 2018

sporkmonger commented Sep 19, 2018

sporkmonger Sep 19, 2018 •

edited

Loading

shrayolacrayon Sep 19, 2018

sporkmonger Sep 19, 2018

sporkmonger commented Sep 19, 2018

shrayolacrayon Sep 25, 2018

sporkmonger Sep 25, 2018

sporkmonger commented Sep 25, 2018

shrayolacrayon commented Sep 25, 2018

sporkmonger commented Sep 25, 2018

shrayolacrayon Sep 25, 2018

shrayolacrayon commented Sep 25, 2018

sporkmonger commented Sep 25, 2018

shrayolacrayon Sep 25, 2018

sporkmonger Sep 25, 2018

sporkmonger commented Sep 25, 2018

sporkmonger commented Sep 25, 2018

while1eq1 commented Oct 3, 2018

		@@ -201,6 +201,110 @@ func TestNewReverseProxyHostname(t *testing.T) {

		}

		func TestNewReverseProxyTLSSkipVerifyFalse(t *testing.T) {

Make TLS verification of upstream servers configurable #49

Make TLS verification of upstream servers configurable #49

Conversation

sporkmonger commented Sep 15, 2018

shrayolacrayon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sporkmonger Sep 18, 2018 • edited Loading

Choose a reason for hiding this comment

sporkmonger Sep 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sporkmonger Sep 18, 2018 • edited Loading

Choose a reason for hiding this comment

sporkmonger commented Sep 18, 2018

sporkmonger commented Sep 19, 2018

sporkmonger Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sporkmonger commented Sep 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sporkmonger commented Sep 25, 2018

shrayolacrayon commented Sep 25, 2018

sporkmonger commented Sep 25, 2018

Choose a reason for hiding this comment

shrayolacrayon commented Sep 25, 2018

sporkmonger commented Sep 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sporkmonger commented Sep 25, 2018

sporkmonger commented Sep 25, 2018

while1eq1 commented Oct 3, 2018

sporkmonger Sep 18, 2018 •

edited

Loading

sporkmonger Sep 18, 2018 •

edited

Loading

sporkmonger Sep 18, 2018 •

edited

Loading

sporkmonger Sep 19, 2018 •

edited

Loading