Handle resolving proxy tokens when parsing HTTP requests #4453

mkeeler · 2018-07-26T15:23:05Z

The underlying problem with managed proxies and ACLs was that many HTTP endpoints were not ProxyToken aware. They passed along the proxy token to the RPC endpoints on the servers which knew nothing about proxy tokens.

This PR puts proxy token -> service token translation logic into our HTTP token handling. Additionally I added the parseTokenProxyResolve to be able to disable this auto-translation in the case of a few connect specific endpoints that want to run verifyProxyToken themselves (which has a few more checks - even though they may provide only marginal benefit in the case where each service has their own ACL token).

How I tested:

Brought up a 3 server and 1 client cluster
- Client is running consul with an additional python -m SimpleHTTPServer 8080 running in the background to provide the "web" service on port 8080
Bootstrapped ACLs (this doesn't get less painful the more I do it)
Enabled the connect proxy for my web service on my client
Ran consul connect proxy -service test -upstream web:9191 on one of my consul servers (the cluster was in docker behind a nat bridge so I needed to get into the network and doing it on the server was the easiest way (although I could have done it elsewhere)
Tried curling localhost:9191 and got a tls: bad certificate error
Realized my policy was setup to deny by default
Created intention to allow test -> web
Curl localhost:9191 again and get the directory listing for where my python server was running.

Currently getting this all setup for a unit test is impractical at best and impossible at worst or I would have implemented that too.

mkeeler · 2018-07-26T16:00:32Z

The travis failures seem like just flaky tests. I ran them all locally and they passed.

mkeeler-mbp:connect-acls mkeeler$ gpm_consul . go test -run TestIntentionApply_aclUpdateChange ./agent/consul
ok      github.com/hashicorp/consul/agent/consul        0.464s
mkeeler-mbp:connect-acls mkeeler$ gpm_consul . go test -run TestRPC_ReadyForConsistentReads ./agent/consul
ok      github.com/hashicorp/consul/agent/consul        0.529s
mkeeler-mbp:connect-acls mkeeler$ gpm_consul . go test -run TestACL_Authority_Found ./agent/consul
ok      github.com/hashicorp/consul/agent/consul        0.539s
mkeeler-mbp:connect-acls mkeeler$ gpm_consul . go test -run TestHealth_ServiceChecks_FilterACL ./agent/consul
ok      github.com/hashicorp/consul/agent/consul        0.588s
mkeeler-mbp:connect-acls mkeeler$ gpm_consul . go test -run TestTxn_Apply_ACLDeny ./agent/consul
ok      github.com/hashicorp/consul/agent/consul        0.630s
mkeeler-mbp:connect-acls mkeeler$ gpm_consul . go test -run TestCatalog_ListServices ./agent/consul
ok      github.com/hashicorp/consul/agent/consul        0.609s

banks

We can test this without starting actual proxies too so we should probably do that.

banks · 2018-07-26T15:48:35Z

agent/http.go

+	s.parseTokenProxyResolve(req, token, true)
+}
+
+func (s *HTTPServer) parseTokenProxyResolve(req *http.Request, token *string, resolve bool) {


nit a comment about what resolve does and why it's needed (maybe link to this PR if you like) might be useful

banks · 2018-07-26T16:02:34Z

agent/agent_endpoint.go

 	// Store DC in the ConnectCALeafRequest but query opts separately
-	if done := s.parse(resp, req, &args.Datacenter, &qOpts); done {
+	s.parseDC(req, &args.Datacenter)
+	s.parseTokenProxyResolve(req, &qOpts.Token, false)


It's a bit janky that when we want to use the extended verification the boiler plate increases.

I realise this probably had the advantage of being a tiny change compared with refactoring verify method a bit but I feel like that might be worth it.

I'm in two minds about whether to untangle this now or just land it as a fix and come back later.

I think it is important enough to fix nicely though? What do you think?

Oh how about:

make a parseWithoutResolvingProxyToken (yay names) that does all of the same as parse but with false here.

Or alternatively, leave this just as parse as it is here, but change verifyProxyToken to accept the req directly and do it's own unpacking of the raw value rather than relying on qOpts?

This makes the case where we want to call verifyProxyToken later less clunky. Also add some comments about why the verifyProxyToken calls are needed for some connect endpoints.

mkeeler · 2018-07-26T19:24:54Z

I retested the latest changes in my manual test scenario. Additionally the new unit tests I added checks a bunch of endpoints to make sure that passing proxy tokens to them works in place of acl tokens (tests out resolving proxy tokens to acl tokens).

banks

Great. Good catches on the security quirks we are relying on with verify too!

banks · 2018-07-27T10:58:26Z

agent/agent_endpoint.go

-	// Parse the token
+	// Parse the token - don't resolve a proxy token to a real token
+	// that will be done with a call to verifyProxyToken later along with
+	// other security relevant checks.


Like this. Is it worth adding to CALeadCert too?

I added similar wording there too now.

banks · 2018-07-27T11:03:31Z

agent/http.go

 // to use both parseWait and parseDC.
-func (s *HTTPServer) parse(resp http.ResponseWriter, req *http.Request, dc *string, b *structs.QueryOptions) bool {
+func (s *HTTPServer) parseResolveProxy(resp http.ResponseWriter, req *http.Request, dc *string, b *structs.QueryOptions, resolveProxyToken bool) bool {


Names are hard :(

I can't think of anything better but it's a bit unintuitive that parseResolveProxy optionally doesn't resolve the proxy! I'd be tempted to name it something like parseInternal or parseWithOptions to indicate that it's mostly a way to de-duplicate logic from higher level callers?

Not a blocker at all though.

banks · 2018-07-27T11:04:36Z

agent/http_test.go

+		handler  func(s *HTTPServer, resp http.ResponseWriter, req *http.Request) (interface{}, error)
+	}
+
+	// This is no exhaustive list of all of our endpoints and is only testing GET endpoints


"not an exhaustive list"?

banks · 2018-07-27T11:07:18Z

agent/http_test.go

+
+	type endpointCheck struct {
+		endpoint string
+		handler  func(s *HTTPServer, resp http.ResponseWriter, req *http.Request) (interface{}, error)


Huh TIL type of a method can be expressed this way with receiver as first arg. I knew that receivers are just syntax sugar on that generally but not that this was possible...

Yeah, I learned that by reading how we register HTTP endpoints because we do the same thing there.

mkeeler · 2018-07-27T19:48:18Z

Another note is that I ran tests locally and everything passed.

banks requested changes Jul 26, 2018

View reviewed changes

Handle resolving proxy tokens when parsing HTTP requests

f2544aa

mkeeler force-pushed the bugfix/connect-acls branch 2 times, most recently from 0e45b62 to 79a6abc Compare July 26, 2018 18:22

Add parseResolveProxy function

91d95bb

This makes the case where we want to call verifyProxyToken later less clunky. Also add some comments about why the verifyProxyToken calls are needed for some connect endpoints.

mkeeler force-pushed the bugfix/connect-acls branch from 79a6abc to 91d95bb Compare July 26, 2018 18:22

Add a proxy token resolution test

195ad50

banks approved these changes Jul 27, 2018

View reviewed changes

PR Updates

7b9751d

mkeeler merged commit 870a6ad into master Jul 30, 2018

mkeeler deleted the bugfix/connect-acls branch September 6, 2018 19:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle resolving proxy tokens when parsing HTTP requests #4453

Handle resolving proxy tokens when parsing HTTP requests #4453

mkeeler commented Jul 26, 2018

mkeeler commented Jul 26, 2018

banks left a comment

banks Jul 26, 2018

banks Jul 26, 2018

mkeeler commented Jul 26, 2018

banks left a comment

banks Jul 27, 2018

mkeeler Jul 27, 2018

banks Jul 27, 2018

banks Jul 27, 2018

banks Jul 27, 2018

mkeeler Jul 27, 2018

mkeeler commented Jul 27, 2018

Handle resolving proxy tokens when parsing HTTP requests #4453

Handle resolving proxy tokens when parsing HTTP requests #4453

Conversation

mkeeler commented Jul 26, 2018

mkeeler commented Jul 26, 2018

banks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkeeler commented Jul 26, 2018

banks 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkeeler commented Jul 27, 2018