From 90898dff98338de19c23fab985257e5bd75b32e7 Mon Sep 17 00:00:00 2001 From: James Phillips Date: Fri, 26 Feb 2016 15:59:00 -0800 Subject: [PATCH] Adds missing token redact in the GET path. --- consul/acl.go | 59 +++++++++++++-------- consul/acl_test.go | 38 ++++++++++++++ consul/prepared_query_endpoint.go | 5 +- consul/prepared_query_endpoint_test.go | 71 ++++++++++++++++++++++++-- 4 files changed, 145 insertions(+), 28 deletions(-) diff --git a/consul/acl.go b/consul/acl.go index de305d04f5..24cedf8fc0 100644 --- a/consul/acl.go +++ b/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 -// 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. +// if the user doesn't have a management token. 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 - // to redact it, since we know they don't have a management - // token. - if query.Token == "" { - ret = append(ret, query) - } 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) - } + // Redact any tokens if necessary. We make a copy of just the + // pointer so we don't mess with the caller's slice. + final := query + f.redactPreparedQueryTokens(&final) + ret = append(ret, final) } *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)) } diff --git a/consul/acl_test.go b/consul/acl_test.go index 1e0d489d75..c738d7e3fe 100644 --- a/consul/acl_test.go +++ b/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{ diff --git a/consul/prepared_query_endpoint.go b/consul/prepared_query_endpoint.go index c3f027136e..02a1a4d4bb 100644 --- a/consul/prepared_query_endpoint.go +++ b/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. diff --git a/consul/prepared_query_endpoint_test.go b/consul/prepared_query_endpoint_test.go index a6c37ce880..f584552a8f 100644 --- a/consul/prepared_query_endpoint_test.go +++ b/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{ @@ -814,7 +872,8 @@ func TestPreparedQuery_List(t *testing.T) { Datacenter: "dc1", Op: structs.PreparedQueryCreate, Query: &structs.PreparedQuery{ - Name: "redis-master", + 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",