Merge pull request #1766 from hashicorp/b-redact-in-get

Adds missing token redact in the GET path.

consul/acl.go

@@ -370,14 +370,37 @@ func (f *aclFilter) filterNodeDump(dump *structs.NodeDump) {
 	*dump = nd
 }
 
+// redactPreparedQueryTokens will redact any tokens unless the client has a
+// management token. This eases the transition to delegated authority over
+// prepared queries, since it was easy to capture management tokens in Consul
+// 0.6.3 and earlier, and we don't want to willy-nilly show those. This does
+// have the limitation of preventing delegated non-management users from seeing
+// captured tokens, but they can at least see whether or not a token is set.
+func (f *aclFilter) redactPreparedQueryTokens(query **structs.PreparedQuery) {
+	// Management tokens can see everything with no filtering.
+	if f.acl.ACLList() {
+		return
+	}
+
+	// Let the user see if there's a blank token, otherwise we need
+	// to redact it, since we know they don't have a management
+	// token.
+	if (*query).Token != "" {
+		// Redact the token, using a copy of the query structure
+		// since we could be pointed at a live instance from the
+		// state store so it's not safe to modify it. Note that
+		// this clone will still point to things like underlying
+		// arrays in the original, but for modifying just the
+		// token it will be safe to use.
+		clone := *(*query)
+		clone.Token = redactedToken
+		*query = &clone
+	}
+}
+
 // filterPreparedQueries is used to filter prepared queries based on ACL rules.
 // We prune entries the user doesn't have access to, and we redact any tokens
-// unless the client has a management token. This eases the transition to
+// if the user doesn't have a management token.
-// delegated authority over prepared queries, since it was easy to capture
-// management tokens in Consul 0.6.3 and earlier, and we don't want to
-// willy-nilly show those. This does have the limitation of preventing delegated
-// non-management users from seeing captured tokens, but they can at least see
-// that they are set.
 func (f *aclFilter) filterPreparedQueries(queries *structs.PreparedQueries) {
 	// Management tokens can see everything with no filtering.
 	if f.acl.ACLList() {
@@ -396,22 +419,11 @@ func (f *aclFilter) filterPreparedQueries(queries *structs.PreparedQueries) {
 			continue
 		}
 
-		// Let the user see if there's a blank token, otherwise we need
+		// Redact any tokens if necessary. We make a copy of just the
-		// to redact it, since we know they don't have a management
+		// pointer so we don't mess with the caller's slice.
-		// token.
+		final := query
-		if query.Token == "" {
+		f.redactPreparedQueryTokens(&final)
-			ret = append(ret, query)
+		ret = append(ret, final)
-		} else {
-			// Redact the token, using a copy of the query structure
-			// since we could be pointed at a live instance from the
-			// state store so it's not safe to modify it. Note that
-			// this clone will still point to things like underlying
-			// arrays in the original, but for modifying just the
-			// token it will be safe to use.
-			clone := *query
-			clone.Token = redactedToken
-			ret = append(ret, &clone)
-		}
 	}
 	*queries = ret
 }
@@ -461,6 +473,9 @@ func (s *Server) filterACL(token string, subj interface{}) error {
 	case *structs.IndexedPreparedQueries:
 		filt.filterPreparedQueries(&v.Queries)
 
+	case **structs.PreparedQuery:
+		filt.redactPreparedQueryTokens(v)
+
 	default:
 		panic(fmt.Errorf("Unhandled type passed to ACL filter: %#v", subj))
 	}

consul/acl_test.go

@@ -862,6 +862,44 @@ func TestACL_filterNodeDump(t *testing.T) {
 	}
 }
 
+func TestACL_redactPreparedQueryTokens(t *testing.T) {
+	query := &structs.PreparedQuery{
+		ID:    "f004177f-2c28-83b7-4229-eacc25fe55d1",
+		Token: "root",
+	}
+
+	expected := &structs.PreparedQuery{
+		ID:    "f004177f-2c28-83b7-4229-eacc25fe55d1",
+		Token: "root",
+	}
+
+	// Try permissive filtering with a management token. This will allow the
+	// embedded token to be seen.
+	filt := newAclFilter(acl.ManageAll(), nil)
+	filt.redactPreparedQueryTokens(&query)
+	if !reflect.DeepEqual(query, expected) {
+		t.Fatalf("bad: %#v", &query)
+	}
+
+	// Hang on to the entry with a token, which needs to survive the next
+	// operation.
+	original := query
+
+	// Now try permissive filtering with a client token, which should cause
+	// the embedded token to get redacted.
+	filt = newAclFilter(acl.AllowAll(), nil)
+	filt.redactPreparedQueryTokens(&query)
+	expected.Token = redactedToken
+	if !reflect.DeepEqual(query, expected) {
+		t.Fatalf("bad: %#v", *query)
+	}
+
+	// Make sure that the original object didn't lose its token.
+	if original.Token != "root" {
+		t.Fatalf("bad token: %s", original.Token)
+	}
+}
+
 func TestACL_filterPreparedQueries(t *testing.T) {
 	queries := structs.PreparedQueries{
 		&structs.PreparedQuery{

consul/prepared_query_endpoint.go

@@ -219,11 +219,12 @@ func (p *PreparedQuery) Get(args *structs.PreparedQuerySpecificRequest,
 			}
 
 			// If no prefix ACL applies to this query, then they are
-			// always allowed to see it if they have the ID.
+			// always allowed to see it if they have the ID. We still
+			// have to filter the remaining object for tokens.
 			reply.Index = index
 			reply.Queries = structs.PreparedQueries{query}
 			if _, ok := query.GetACLPrefix(); !ok {
-				return nil
+				return p.srv.filterACL(args.Token, &reply.Queries[0])
 			}
 
 			// Otherwise, attempt to filter it the usual way.

consul/prepared_query_endpoint_test.go

@@ -711,7 +711,6 @@ func TestPreparedQuery_Get(t *testing.T) {
 
 	// Try again with no token, this should work since this query is only
 	// managed by an ID (no name) so no ACLs apply to it.
-	query.Query.ID = reply
 	{
 		req := &structs.PreparedQuerySpecificRequest{
 			Datacenter:   "dc1",
@@ -736,6 +735,65 @@ func TestPreparedQuery_Get(t *testing.T) {
 		}
 	}
 
+	// Capture a token.
+	query.Op = structs.PreparedQueryUpdate
+	query.Query.Token = "le-token"
+	if err := msgpackrpc.CallWithCodec(codec, "PreparedQuery.Apply", &query, &reply); err != nil {
+		t.Fatalf("err: %v", err)
+	}
+
+	// This should get redacted when we read it back without a token.
+	query.Query.Token = redactedToken
+	{
+		req := &structs.PreparedQuerySpecificRequest{
+			Datacenter:   "dc1",
+			QueryID:      query.Query.ID,
+			QueryOptions: structs.QueryOptions{Token: ""},
+		}
+		var resp structs.IndexedPreparedQueries
+		if err := msgpackrpc.CallWithCodec(codec, "PreparedQuery.Get", req, &resp); err != nil {
+			t.Fatalf("err: %v", err)
+		}
+
+		if len(resp.Queries) != 1 {
+			t.Fatalf("bad: %v", resp)
+		}
+		actual := resp.Queries[0]
+		if resp.Index != actual.ModifyIndex {
+			t.Fatalf("bad index: %d", resp.Index)
+		}
+		actual.CreateIndex, actual.ModifyIndex = 0, 0
+		if !reflect.DeepEqual(actual, query.Query) {
+			t.Fatalf("bad: %v", actual)
+		}
+	}
+
+	// But a management token should be able to see it.
+	query.Query.Token = "le-token"
+	{
+		req := &structs.PreparedQuerySpecificRequest{
+			Datacenter:   "dc1",
+			QueryID:      query.Query.ID,
+			QueryOptions: structs.QueryOptions{Token: "root"},
+		}
+		var resp structs.IndexedPreparedQueries
+		if err := msgpackrpc.CallWithCodec(codec, "PreparedQuery.Get", req, &resp); err != nil {
+			t.Fatalf("err: %v", err)
+		}
+
+		if len(resp.Queries) != 1 {
+			t.Fatalf("bad: %v", resp)
+		}
+		actual := resp.Queries[0]
+		if resp.Index != actual.ModifyIndex {
+			t.Fatalf("bad index: %d", resp.Index)
+		}
+		actual.CreateIndex, actual.ModifyIndex = 0, 0
+		if !reflect.DeepEqual(actual, query.Query) {
+			t.Fatalf("bad: %v", actual)
+		}
+	}
+
 	// Try to get an unknown ID.
 	{
 		req := &structs.PreparedQuerySpecificRequest{
@@ -815,6 +873,7 @@ func TestPreparedQuery_List(t *testing.T) {
 		Op:         structs.PreparedQueryCreate,
 		Query: &structs.PreparedQuery{
 			Name:  "redis-master",
+			Token: "le-token",
 			Service: structs.ServiceQuery{
 				Service: "the-redis",
 			},
@@ -826,8 +885,10 @@ func TestPreparedQuery_List(t *testing.T) {
 		t.Fatalf("err: %v", err)
 	}
 
-	// Capture the ID and read back the query to verify.
+	// Capture the ID and read back the query to verify. We also make sure
+	// the captured token gets redacted.
 	query.Query.ID = reply
+	query.Query.Token = redactedToken
 	{
 		req := &structs.DCSpecificRequest{
 			Datacenter:   "dc1",
@@ -868,7 +929,9 @@ func TestPreparedQuery_List(t *testing.T) {
 		}
 	}
 
-	// But a management token should work.
+	// But a management token should work, and be able to see the captured
+	// token.
+	query.Query.Token = "le-token"
 	{
 		req := &structs.DCSpecificRequest{
 			Datacenter:   "dc1",