Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Obfuscates ACL tokens appearing in /v1/acl/<verb>/<token> APIs. #3276

Merged
merged 3 commits into from
Jul 15, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 25 additions & 9 deletions agent/http.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"net/http"
"net/http/pprof"
"net/url"
"regexp"
"strconv"
"strings"
"time"
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the RE is too clever. I've read this a couple of times now and it isn't clear to me what it is trying to achieve. You could try to use multiple REs or at least provide some examples of what this is supposed to match. Since you're trying to obfuscate sensitive data I'd say clarity goes before efficiency. Maybe don't use an RE at all and work with url.Parse or strings.Split.

// 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) {
Expand All @@ -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"
Expand All @@ -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() {
Expand Down
46 changes: 40 additions & 6 deletions agent/http_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
})
}
}

Expand Down