diff --git a/agent/http.go b/agent/http.go index 7eb93ba9c7..046a62d235 100644 --- a/agent/http.go +++ b/agent/http.go @@ -6,6 +6,7 @@ import ( "net/http" "net/http/pprof" "net/url" + "regexp" "strconv" "strings" "time" @@ -166,6 +167,29 @@ func (s *HTTPServer) handler(enableDebug bool) http.Handler { return mux } +// aclEndpointRE is used to find old ACL endpoints that take tokens in the URL +// so that we can redact them. The ACL endpoints that take the token in the URL +// are all of the form /v1/acl//, and can optionally include query +// parameters which are indicated by a question mark. We capture the part before +// the token, the token, and any query parameters after, and then reassemble as +// $1$3 (the token in $2 isn't used), which will give: +// +// /v1/acl/clone/foo -> /v1/acl/clone/ +// /v1/acl/clone/foo?token=bar -> /v1/acl/clone/?token= +// +// The query parameter in the example above is obfuscated like any other, after +// this regular expression is applied, so the regular expression substitution +// results in: +// +// /v1/acl/clone/foo?token=bar -> /v1/acl/clone/?token=bar +// ^---- $1 ----^^- $2 -^^-- $3 --^ +// +// And then the loop that looks for parameters called "token" does the last +// step to get to the final redacted form. +var ( + aclEndpointRE = regexp.MustCompile("^(/v1/acl/[^/]+/)([^?]+)([?]?.*)$") +) + // wrap is used to wrap functions to make them more convenient func (s *HTTPServer) wrap(handler func(resp http.ResponseWriter, req *http.Request) (interface{}, error)) http.HandlerFunc { return func(resp http.ResponseWriter, req *http.Request) { @@ -189,6 +213,7 @@ func (s *HTTPServer) wrap(handler func(resp http.ResponseWriter, req *http.Reque logURL = strings.Replace(logURL, token, "", -1) } } + logURL = aclEndpointRE.ReplaceAllString(logURL, "$1$3") if s.blacklist.Block(req.URL.Path) { errMsg := "Endpoint is blocked by agent configuration" @@ -209,15 +234,6 @@ func (s *HTTPServer) wrap(handler func(resp http.ResponseWriter, req *http.Reque fmt.Fprint(resp, errMsg) } - // TODO (slackpad) We may want to consider redacting prepared - // query names/IDs here since they are proxies for tokens. But, - // knowing one only gives you read access to service listings - // which is pretty trivial, so it's probably not worth the code - // complexity and overhead of filtering them out. You can't - // recover the token it's a proxy for with just the query info; - // you'd need the actual token (or a management token) to read - // that back. - // Invoke the handler start := time.Now() defer func() { diff --git a/agent/http_test.go b/agent/http_test.go index 60d7cc1cc1..29a606e89e 100644 --- a/agent/http_test.go +++ b/agent/http_test.go @@ -331,16 +331,50 @@ func TestHTTP_wrap_obfuscateLog(t *testing.T) { a.Start() defer a.Shutdown() - resp := httptest.NewRecorder() - req, _ := http.NewRequest("GET", "/some/url?token=secret1&token=secret2", nil) handler := func(resp http.ResponseWriter, req *http.Request) (interface{}, error) { return nil, nil } - a.srv.wrap(handler)(resp, req) - // Make sure no tokens from the URL show up in the log - if strings.Contains(buf.String(), "secret") { - t.Fatalf("bad: %s", buf.String()) + for _, pair := range [][]string{ + { + "/some/url?token=secret1&token=secret2", + "/some/url?token=&token=", + }, + { + "/v1/acl/clone/secret1", + "/v1/acl/clone/", + }, + { + "/v1/acl/clone/secret1?token=secret2", + "/v1/acl/clone/?token=", + }, + { + "/v1/acl/destroy/secret1", + "/v1/acl/destroy/", + }, + { + "/v1/acl/destroy/secret1?token=secret2", + "/v1/acl/destroy/?token=", + }, + { + "/v1/acl/info/secret1", + "/v1/acl/info/", + }, + { + "/v1/acl/info/secret1?token=secret2", + "/v1/acl/info/?token=", + }, + } { + url, want := pair[0], pair[1] + t.Run(url, func(t *testing.T) { + resp := httptest.NewRecorder() + req, _ := http.NewRequest("GET", url, nil) + a.srv.wrap(handler)(resp, req) + + if got := buf.String(); !strings.Contains(got, want) { + t.Fatalf("got %s want %s", got, want) + } + }) } }