From ac7c48c3ea3dbb556fcf3f2cd9a2a17b9702fc92 Mon Sep 17 00:00:00 2001
From: James Phillips <slackpad@gmail.com>
Date: Sat, 15 Jul 2017 00:07:08 -0700
Subject: [PATCH] Obfuscates ACL tokens appearing in /v1/acl/<verb>/<token>
 APIs. (#3276)

* Obfuscates ACL tokens appearing in /v1/acl APIs.

* Makes test positively identify the desired strings.

* Adds an example and explanation of the regular expression.
---
 agent/http.go      | 34 +++++++++++++++++++++++++---------
 agent/http_test.go | 46 ++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 65 insertions(+), 15 deletions(-)

diff --git a/agent/http.go b/agent/http.go
index 7eb93ba9c..046a62d23 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/<verb>/<token>, 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<hidden>$3 (the token in $2 isn't used), which will give:
+//
+// /v1/acl/clone/foo           -> /v1/acl/clone/<hidden>
+// /v1/acl/clone/foo?token=bar -> /v1/acl/clone/<hidden>?token=<hidden>
+//
+// 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/<hidden>?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, "<hidden>", -1)
 			}
 		}
+		logURL = aclEndpointRE.ReplaceAllString(logURL, "$1<hidden>$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 60d7cc1cc..29a606e89 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=<hidden>&token=<hidden>",
+		},
+		{
+			"/v1/acl/clone/secret1",
+			"/v1/acl/clone/<hidden>",
+		},
+		{
+			"/v1/acl/clone/secret1?token=secret2",
+			"/v1/acl/clone/<hidden>?token=<hidden>",
+		},
+		{
+			"/v1/acl/destroy/secret1",
+			"/v1/acl/destroy/<hidden>",
+		},
+		{
+			"/v1/acl/destroy/secret1?token=secret2",
+			"/v1/acl/destroy/<hidden>?token=<hidden>",
+		},
+		{
+			"/v1/acl/info/secret1",
+			"/v1/acl/info/<hidden>",
+		},
+		{
+			"/v1/acl/info/secret1?token=secret2",
+			"/v1/acl/info/<hidden>?token=<hidden>",
+		},
+	} {
+		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)
+			}
+		})
 	}
 }