From 899dcfe05318adda693241db1d65d850354c16b0 Mon Sep 17 00:00:00 2001 From: James Phillips Date: Wed, 24 Feb 2016 01:26:16 -0800 Subject: [PATCH] Completes switch of prepared_query ACLs to govern query names. --- acl/acl_test.go | 39 ++++ consul/acl.go | 22 ++- consul/acl_test.go | 36 ++-- consul/prepared_query_endpoint.go | 44 +++-- consul/prepared_query_endpoint_test.go | 239 ++++++++++--------------- consul/structs/prepared_query.go | 11 +- consul/structs/prepared_query_test.go | 15 +- 7 files changed, 210 insertions(+), 196 deletions(-) diff --git a/acl/acl_test.go b/acl/acl_test.go index a22752999f..69c4f0a1bf 100644 --- a/acl/acl_test.go +++ b/acl/acl_test.go @@ -352,6 +352,16 @@ func TestPolicyACL_Parent(t *testing.T) { Policy: PolicyRead, }, }, + PreparedQueries: []*PreparedQueryPolicy{ + &PreparedQueryPolicy{ + Prefix: "other", + Policy: PolicyWrite, + }, + &PreparedQueryPolicy{ + Prefix: "foo", + Policy: PolicyRead, + }, + }, } root, err := New(deny, policyRoot) if err != nil { @@ -379,6 +389,12 @@ func TestPolicyACL_Parent(t *testing.T) { Policy: PolicyDeny, }, }, + PreparedQueries: []*PreparedQueryPolicy{ + &PreparedQueryPolicy{ + Prefix: "bar", + Policy: PolicyDeny, + }, + }, } acl, err := New(root, policy) if err != nil { @@ -431,6 +447,29 @@ func TestPolicyACL_Parent(t *testing.T) { } } + // Test prepared queries + type querycase struct { + inp string + read bool + write bool + } + querycases := []querycase{ + {"foo", true, false}, + {"foobar", true, false}, + {"bar", false, false}, + {"barbaz", false, false}, + {"baz", false, false}, + {"nope", false, false}, + } + for _, c := range querycases { + if c.read != acl.PreparedQueryRead(c.inp) { + t.Fatalf("Prepared query fail: %#v", c) + } + if c.write != acl.PreparedQueryWrite(c.inp) { + t.Fatalf("Prepared query fail: %#v", c) + } + } + // Check some management functions that chain up if acl.ACLList() { t.Fatalf("should not allow") diff --git a/consul/acl.go b/consul/acl.go index f795997bad..4fe4bb725f 100644 --- a/consul/acl.go +++ b/consul/acl.go @@ -377,19 +377,29 @@ func (f *aclFilter) filterNodeDump(dump *structs.NodeDump) { // 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 and we want to discourage using them going forward. +// that they are set. func (f *aclFilter) filterPreparedQueries(queries *structs.PreparedQueries) { - isManagementToken := f.acl.ACLList() + // Management tokens can see everything with no filtering. + if f.acl.ACLList() { + return + } + + // Otherwise, we need to see what the token has access to. ret := make(structs.PreparedQueries, 0, len(*queries)) for _, query := range *queries { - if !f.acl.PreparedQueryRead(query.GetACLPrefix()) { + // If no prefix ACL applies to this query then filter it, since + // we know at this point the user doesn't have a management + // token. + prefix := query.GetACLPrefix() + if prefix == nil || !f.acl.PreparedQueryRead(*prefix) { f.logger.Printf("[DEBUG] consul: dropping prepared query %q from result due to ACLs", query.ID) continue } - // Secure tokens unless there's a management token doing the - // query. - if isManagementToken || query.Token == "" { + // 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 diff --git a/consul/acl_test.go b/consul/acl_test.go index 43070f5473..1e0d489d75 100644 --- a/consul/acl_test.go +++ b/consul/acl_test.go @@ -866,37 +866,35 @@ func TestACL_filterPreparedQueries(t *testing.T) { queries := structs.PreparedQueries{ &structs.PreparedQuery{ ID: "f004177f-2c28-83b7-4229-eacc25fe55d1", - Service: structs.ServiceQuery{ - Service: "foo", - }, }, &structs.PreparedQuery{ - ID: "f004177f-2c28-83b7-4229-eacc25fe55d2", + ID: "f004177f-2c28-83b7-4229-eacc25fe55d2", + Name: "query-with-no-token", + }, + &structs.PreparedQuery{ + ID: "f004177f-2c28-83b7-4229-eacc25fe55d3", + Name: "query-with-a-token", Token: "root", - Service: structs.ServiceQuery{ - Service: "bar", - }, }, } expected := structs.PreparedQueries{ &structs.PreparedQuery{ ID: "f004177f-2c28-83b7-4229-eacc25fe55d1", - Service: structs.ServiceQuery{ - Service: "foo", - }, }, &structs.PreparedQuery{ - ID: "f004177f-2c28-83b7-4229-eacc25fe55d2", + ID: "f004177f-2c28-83b7-4229-eacc25fe55d2", + Name: "query-with-no-token", + }, + &structs.PreparedQuery{ + ID: "f004177f-2c28-83b7-4229-eacc25fe55d3", + Name: "query-with-a-token", Token: "root", - Service: structs.ServiceQuery{ - Service: "bar", - }, }, } // Try permissive filtering with a management token. This will allow the - // embedded tokens to be seen. + // embedded token to be seen. filt := newAclFilter(acl.ManageAll(), nil) filt.filterPreparedQueries(&queries) if !reflect.DeepEqual(queries, expected) { @@ -905,13 +903,15 @@ func TestACL_filterPreparedQueries(t *testing.T) { // Hang on to the entry with a token, which needs to survive the next // operation. - original := queries[1] + original := queries[2] // Now try permissive filtering with a client token, which should cause - // the embedded tokens to get redacted. + // the embedded token to get redacted, and the query with no name to get + // filtered out. filt = newAclFilter(acl.AllowAll(), nil) filt.filterPreparedQueries(&queries) - expected[1].Token = redactedToken + expected[2].Token = redactedToken + expected = append(structs.PreparedQueries{}, expected[1], expected[2]) if !reflect.DeepEqual(queries, expected) { t.Fatalf("bad: %#v", queries) } diff --git a/consul/prepared_query_endpoint.go b/consul/prepared_query_endpoint.go index 29aa0c55a3..e17773e2c4 100644 --- a/consul/prepared_query_endpoint.go +++ b/consul/prepared_query_endpoint.go @@ -56,21 +56,25 @@ func (p *PreparedQuery) Apply(args *structs.PreparedQueryRequest, reply *string) } *reply = args.Query.ID - // Do an ACL check. We need to make sure they are allowed to write - // to whatever prefix is incoming, and any existing prefix if this - // isn't a create operation. + // Get the ACL token for the request for the checks below. acl, err := p.srv.resolveToken(args.Token) if err != nil { return err } - if acl != nil && !acl.PreparedQueryWrite(args.Query.GetACLPrefix()) { - p.srv.logger.Printf("[WARN] consul.prepared_query: Operation on prepared query '%s' denied due to ACLs", args.Query.ID) - return permissionDeniedErr + + // If prefix ACLs apply to the incoming query, then do an ACL check. We + // need to make sure they have write access for whatever they are + // proposing. + if prefix := args.Query.GetACLPrefix(); prefix != nil { + if acl != nil && !acl.PreparedQueryWrite(*prefix) { + p.srv.logger.Printf("[WARN] consul.prepared_query: Operation on prepared query '%s' denied due to ACLs", args.Query.ID) + return permissionDeniedErr + } } // This is the second part of the check above. If they are referencing - // an existing query then make sure it exists and that they have perms - // that let the modify that prefix as well. + // an existing query then make sure it exists and that they have write + // access to whatever they are changing, if prefix ACLs apply to it. if args.Op != structs.PreparedQueryCreate { state := p.srv.fsm.State() _, query, err := state.PreparedQueryGet(args.Query.ID) @@ -80,9 +84,12 @@ func (p *PreparedQuery) Apply(args *structs.PreparedQueryRequest, reply *string) if query == nil { return fmt.Errorf("Cannot modify non-existent prepared query: '%s'", args.Query.ID) } - if acl != nil && !acl.PreparedQueryWrite(query.GetACLPrefix()) { - p.srv.logger.Printf("[WARN] consul.prepared_query: Operation on prepared query '%s' denied due to ACLs", args.Query.ID) - return permissionDeniedErr + + if prefix := query.GetACLPrefix(); prefix != nil { + if acl != nil && !acl.PreparedQueryWrite(*prefix) { + p.srv.logger.Printf("[WARN] consul.prepared_query: Operation on prepared query '%s' denied due to ACLs", args.Query.ID) + return permissionDeniedErr + } } } @@ -205,17 +212,24 @@ func (p *PreparedQuery) Get(args *structs.PreparedQuerySpecificRequest, return ErrQueryNotFound } + // If no prefix ACL applies to this query, then they are + // always allowed to see it if they have the ID. reply.Index = index reply.Queries = structs.PreparedQueries{query} + if prefix := query.GetACLPrefix(); prefix == nil { + return nil + } + + // Otherwise, attempt to filter it the usual way. if err := p.srv.filterACL(args.Token, reply); err != nil { return err } - // If ACLs filtered out query, then let them know that - // access to this is forbidden, since they are requesting - // a specific query. + // Since this is a GET of a specific query, if ACLs have + // prevented us from returning something that exists, + // then alert the user with a permission denied error. if len(reply.Queries) == 0 { - p.srv.logger.Printf("[DEBUG] consul.prepared_query: Request to get prepared query '%s' denied due to ACLs", args.QueryID) + p.srv.logger.Printf("[WARN] consul.prepared_query: Request to get prepared query '%s' denied due to ACLs", args.QueryID) return permissionDeniedErr } diff --git a/consul/prepared_query_endpoint_test.go b/consul/prepared_query_endpoint_test.go index ba406a23d3..620b816ee3 100644 --- a/consul/prepared_query_endpoint_test.go +++ b/consul/prepared_query_endpoint_test.go @@ -27,25 +27,6 @@ func TestPreparedQuery_Apply(t *testing.T) { testutil.WaitForLeader(t, s1.RPC, "dc1") - // Set up a node and service in the catalog. - { - req := structs.RegisterRequest{ - Datacenter: "dc1", - Node: "foo", - Address: "127.0.0.1", - Service: &structs.NodeService{ - Service: "redis", - Tags: []string{"master"}, - Port: 8000, - }, - } - var reply struct{} - err := msgpackrpc.CallWithCodec(codec, "Catalog.Register", &req, &reply) - if err != nil { - t.Fatalf("err: %v", err) - } - } - // Set up a bare bones query. query := structs.PreparedQueryRequest{ Datacenter: "dc1", @@ -233,33 +214,14 @@ func TestPreparedQuery_Apply_ACLDeny(t *testing.T) { } } - // Set up a node and service in the catalog. - { - req := structs.RegisterRequest{ - Datacenter: "dc1", - Node: "foo", - Address: "127.0.0.1", - Service: &structs.NodeService{ - Service: "redis", - Tags: []string{"master"}, - Port: 8000, - }, - WriteRequest: structs.WriteRequest{Token: "root"}, - } - var reply struct{} - err := msgpackrpc.CallWithCodec(codec, "Catalog.Register", &req, &reply) - if err != nil { - t.Fatalf("err: %v", err) - } - } - // Set up a bare bones query. query := structs.PreparedQueryRequest{ Datacenter: "dc1", Op: structs.PreparedQueryCreate, Query: &structs.PreparedQuery{ + Name: "redis-master", Service: structs.ServiceQuery{ - Service: "redis", + Service: "the-redis", }, }, } @@ -423,41 +385,6 @@ func TestPreparedQuery_Apply_ACLDeny(t *testing.T) { } } - // Make another query. - query.Op = structs.PreparedQueryCreate - query.Query.ID = "" - query.WriteRequest.Token = token - if err = msgpackrpc.CallWithCodec(codec, "PreparedQuery.Apply", &query, &reply); err != nil { - t.Fatalf("err: %v", err) - } - - // Check that it's there and that the token did not get captured. - query.Query.ID = reply - query.Query.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) - } - } - // A management token should be able to delete the query no matter what. query.Op = structs.PreparedQueryDelete query.WriteRequest.Token = "root" @@ -484,10 +411,10 @@ func TestPreparedQuery_Apply_ACLDeny(t *testing.T) { } } - // Use the root token to make a query for a different service. + // Use the root token to make a query under a different name. query.Op = structs.PreparedQueryCreate query.Query.ID = "" - query.Query.Service.Service = "cassandra" + query.Query.Name = "cassandra" query.WriteRequest.Token = "root" if err = msgpackrpc.CallWithCodec(codec, "PreparedQuery.Apply", &query, &reply); err != nil { t.Fatalf("err: %v", err) @@ -523,7 +450,7 @@ func TestPreparedQuery_Apply_ACLDeny(t *testing.T) { // Now try to change that to redis with the valid redis token. This will // fail because that token can't change cassandra queries. query.Op = structs.PreparedQueryUpdate - query.Query.Service.Service = "redis" + query.Query.Name = "redis" query.WriteRequest.Token = token err = msgpackrpc.CallWithCodec(codec, "PreparedQuery.Apply", &query, &reply) if err == nil || !strings.Contains(err.Error(), permissionDenied) { @@ -678,34 +605,14 @@ func TestPreparedQuery_Get(t *testing.T) { } } - // Set up a node and service in the catalog. - { - req := structs.RegisterRequest{ - Datacenter: "dc1", - Node: "foo", - Address: "127.0.0.1", - Service: &structs.NodeService{ - Service: "redis", - Tags: []string{"master"}, - Port: 8000, - }, - WriteRequest: structs.WriteRequest{Token: "root"}, - } - var reply struct{} - err := msgpackrpc.CallWithCodec(codec, "Catalog.Register", &req, &reply) - if err != nil { - t.Fatalf("err: %v", err) - } - } - // Set up a bare bones query. query := structs.PreparedQueryRequest{ Datacenter: "dc1", Op: structs.PreparedQueryCreate, Query: &structs.PreparedQuery{ - Name: "my-query", + Name: "redis-master", Service: structs.ServiceQuery{ - Service: "redis", + Service: "the-redis", }, }, WriteRequest: structs.WriteRequest{Token: token}, @@ -784,6 +691,40 @@ func TestPreparedQuery_Get(t *testing.T) { } } + // Now update the query to take away its name. + query.Op = structs.PreparedQueryUpdate + query.Query.Name = "" + if err := msgpackrpc.CallWithCodec(codec, "PreparedQuery.Apply", &query, &reply); err != nil { + t.Fatalf("err: %v", err) + } + + // 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", + 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) + } + } + // Try to get an unknown ID. { req := &structs.PreparedQuerySpecificRequest{ @@ -841,26 +782,6 @@ func TestPreparedQuery_List(t *testing.T) { } } - // Set up a node and service in the catalog. - { - req := structs.RegisterRequest{ - Datacenter: "dc1", - Node: "foo", - Address: "127.0.0.1", - Service: &structs.NodeService{ - Service: "redis", - Tags: []string{"master"}, - Port: 8000, - }, - WriteRequest: structs.WriteRequest{Token: "root"}, - } - var reply struct{} - err := msgpackrpc.CallWithCodec(codec, "Catalog.Register", &req, &reply) - if err != nil { - t.Fatalf("err: %v", err) - } - } - // Query with a legit token but no queries. { req := &structs.DCSpecificRequest{ @@ -882,9 +803,9 @@ func TestPreparedQuery_List(t *testing.T) { Datacenter: "dc1", Op: structs.PreparedQueryCreate, Query: &structs.PreparedQuery{ - Name: "my-query", + Name: "redis-master", Service: structs.ServiceQuery{ - Service: "redis", + Service: "the-redis", }, }, WriteRequest: structs.WriteRequest{Token: token}, @@ -959,6 +880,54 @@ func TestPreparedQuery_List(t *testing.T) { t.Fatalf("bad: %v", actual) } } + + // Now take away the query name. + query.Op = structs.PreparedQueryUpdate + query.Query.Name = "" + if err := msgpackrpc.CallWithCodec(codec, "PreparedQuery.Apply", &query, &reply); err != nil { + t.Fatalf("err: %v", err) + } + + // A query with the redis token shouldn't show anything since it doesn't + // match any un-named queries. + { + req := &structs.DCSpecificRequest{ + Datacenter: "dc1", + QueryOptions: structs.QueryOptions{Token: token}, + } + var resp structs.IndexedPreparedQueries + if err := msgpackrpc.CallWithCodec(codec, "PreparedQuery.List", req, &resp); err != nil { + t.Fatalf("err: %v", err) + } + + if len(resp.Queries) != 0 { + t.Fatalf("bad: %v", resp) + } + } + + // But a management token should work. + { + req := &structs.DCSpecificRequest{ + Datacenter: "dc1", + QueryOptions: structs.QueryOptions{Token: "root"}, + } + var resp structs.IndexedPreparedQueries + if err := msgpackrpc.CallWithCodec(codec, "PreparedQuery.List", 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) + } + } } // This is a beast of a test, but the setup is so extensive it makes sense to @@ -1025,30 +994,6 @@ func TestPreparedQuery_Execute(t *testing.T) { } } - // Create an ACL with write permission to make queries for the service. - var queryCRUDToken string - { - var rules = ` - prepared_query "foo" { - policy = "write" - } - ` - - req := structs.ACLRequest{ - Datacenter: "dc1", - Op: structs.ACLSet, - ACL: structs.ACL{ - Name: "User token", - Type: structs.ACLTypeClient, - Rules: rules, - }, - WriteRequest: structs.WriteRequest{Token: "root"}, - } - if err := msgpackrpc.CallWithCodec(codec1, "ACL.Apply", &req, &queryCRUDToken); err != nil { - t.Fatalf("err: %v", err) - } - } - // Set up some nodes in each DC that host the service. { for i := 0; i < 10; i++ { @@ -1092,7 +1037,7 @@ func TestPreparedQuery_Execute(t *testing.T) { TTL: "10s", }, }, - WriteRequest: structs.WriteRequest{Token: queryCRUDToken}, + WriteRequest: structs.WriteRequest{Token: "root"}, } if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Apply", &query, &query.Query.ID); err != nil { t.Fatalf("err: %v", err) diff --git a/consul/structs/prepared_query.go b/consul/structs/prepared_query.go index d88c2f073c..7d57e49a14 100644 --- a/consul/structs/prepared_query.go +++ b/consul/structs/prepared_query.go @@ -72,9 +72,14 @@ type PreparedQuery struct { RaftIndex } -// GetACLPrefix returns the prefix of this query for ACL purposes. -func (pq *PreparedQuery) GetACLPrefix() string { - return pq.Service.Service +// GetACLPrefix returns the prefix to look up the prepared_query ACL policy for +// this query, or nil if such a policy doesn't apply. +func (pq *PreparedQuery) GetACLPrefix() *string { + if pq.Name != "" { + return &pq.Name + } + + return nil } type PreparedQueries []*PreparedQuery diff --git a/consul/structs/prepared_query_test.go b/consul/structs/prepared_query_test.go index c8be15cbec..14aaedcc25 100644 --- a/consul/structs/prepared_query_test.go +++ b/consul/structs/prepared_query_test.go @@ -4,13 +4,14 @@ import ( "testing" ) -func TestStructs_PreparedQuery_GetACLPrefix(t *testing.T) { - query := &PreparedQuery{ - Service: ServiceQuery{ - Service: "foo", - }, +func TestStructs_PreparedQuery_GetACLInfo(t *testing.T) { + ephemeral := &PreparedQuery{} + if prefix := ephemeral.GetACLPrefix(); prefix != nil { + t.Fatalf("bad: %#v", prefix) } - if prefix := query.GetACLPrefix(); prefix != "foo" { - t.Fatalf("bad: %s", prefix) + + named := &PreparedQuery{Name: "hello"} + if prefix := named.GetACLPrefix(); prefix == nil || *prefix != "hello" { + t.Fatalf("bad: %#v", prefix) } }