From 633c231d67b38d465af6f1c1fbda2c039bdb6ce3 Mon Sep 17 00:00:00 2001 From: James Phillips Date: Tue, 23 Feb 2016 00:12:58 -0800 Subject: [PATCH 1/9] Creates new "prepared-query" ACL type and new token capture behavior. Prior to this change, prepared queries had the following behavior for ACLs, which will need to change to support templates: 1. A management token, or a token with read access to the service being queried needed to be provided in order to create a prepared query. 2. The token used to create the prepared query was stored with the query in the state store and used to execute the query. 3. A management token, or the token used to create the query needed to be supplied to perform and CRUD operations on an existing prepared query. This was pretty subtle and complicated behavior, and won't work for templates since the service name is computed at execution time. To solve this, we introduce a new "prepared-query" ACL type, where the prefix applies to the query name for static prepared query types and to the prefix for template prepared query types. With this change, the new behavior is: 1. A management token, or a token with "prepared-query" write access to the query name or (soon) the given template prefix is required to do any CRUD operations on a prepared query, or to list prepared queries (the list is filtered by this ACL). 2. You will no longer need a management token to list prepared queries, but you will only be able to see prepared queries that you have access to (you get an empty list instead of permission denied). 3. When listing or getting a query, because it was easy to capture management tokens given the past behavior, this will always blank out the "Token" field (replacing the contents as ) for all tokens unless a management token is supplied. Going forward, we should discourage people from binding tokens for execution unless strictly necessary. 4. No token will be captured by default when a prepared query is created. If the user wishes to supply an execution token then can pass it in via the "Token" field in the prepared query definition. Otherwise, this field will default to empty. 5. At execution time, we will use the captured token if it exists with the prepared query definition, otherwise we will use the token that's passed in with the request, just like we do for other RPCs (or you can use the agent's configured token for DNS). 6. Prepared queries with no name (accessible only by ID) will not require ACLs to create or modify (execution time will depend on the service ACL configuration). Our argument here is that these are designed to be ephemeral and the IDs are as good as an ACL. Management tokens will be able to list all of these. These changes enable templates, but also enable delegation of authority to manage the prepared query namespace. --- acl/acl.go | 123 +++-- acl/acl_test.go | 129 ++++-- acl/policy.go | 84 ++-- acl/policy_test.go | 113 ++++- consul/acl.go | 46 ++ consul/acl_test.go | 68 +++ consul/prepared_query_endpoint.go | 110 ++--- consul/prepared_query_endpoint_test.go | 438 ++++++++++++------ consul/state/prepared_query.go | 11 +- consul/state/prepared_query_test.go | 10 +- consul/structs/prepared_query.go | 5 + consul/structs/prepared_query_test.go | 16 + .../docs/agent/http/query.html.markdown | 68 ++- .../source/docs/internals/acl.html.markdown | 54 +++ 14 files changed, 889 insertions(+), 386 deletions(-) create mode 100644 consul/structs/prepared_query_test.go diff --git a/acl/acl.go b/acl/acl.go index 344afc5c3..33f5e2337 100644 --- a/acl/acl.go +++ b/acl/acl.go @@ -58,6 +58,14 @@ type ACL interface { // EventWrite determines if a specific event may be fired. EventWrite(string) bool + // PrepardQueryRead determines if a specific prepared query can be read + // to show its contents (this is not used for execution). + PreparedQueryRead(string) bool + + // PreparedQueryWrite determines if a specific prepared query can be + // created, modified, or deleted. + PreparedQueryWrite(string) bool + // KeyringRead determines if the encryption keyring used in // the gossip layer can be read. KeyringRead() bool @@ -70,12 +78,6 @@ type ACL interface { // ACLModify checks for permission to manipulate ACLs ACLModify() bool - - // QueryList checks for permission to list all the prepared queries. - QueryList() bool - - // QueryModify checks for permission to modify any prepared query. - QueryModify() bool } // StaticACL is used to implement a base ACL policy. It either @@ -114,6 +116,14 @@ func (s *StaticACL) EventWrite(string) bool { return s.defaultAllow } +func (s *StaticACL) PreparedQueryRead(string) bool { + return s.defaultAllow +} + +func (s *StaticACL) PreparedQueryWrite(string) bool { + return s.defaultAllow +} + func (s *StaticACL) KeyringRead() bool { return s.defaultAllow } @@ -130,14 +140,6 @@ func (s *StaticACL) ACLModify() bool { return s.allowManage } -func (s *StaticACL) QueryList() bool { - return s.allowManage -} - -func (s *StaticACL) QueryModify() bool { - return s.allowManage -} - // AllowAll returns an ACL rule that allows all operations func AllowAll() ACL { return allowAll @@ -183,6 +185,9 @@ type PolicyACL struct { // eventRules contains the user event policies eventRules *radix.Tree + // preparedQueryRules contains the prepared query policies + preparedQueryRules *radix.Tree + // keyringRules contains the keyring policies. The keyring has // a very simple yes/no without prefix matching, so here we // don't need to use a radix tree. @@ -193,10 +198,11 @@ type PolicyACL struct { // and a parent policy to resolve missing cases. func New(parent ACL, policy *Policy) (*PolicyACL, error) { p := &PolicyACL{ - parent: parent, - keyRules: radix.New(), - serviceRules: radix.New(), - eventRules: radix.New(), + parent: parent, + keyRules: radix.New(), + serviceRules: radix.New(), + eventRules: radix.New(), + preparedQueryRules: radix.New(), } // Load the key policy @@ -214,6 +220,11 @@ func New(parent ACL, policy *Policy) (*PolicyACL, error) { p.eventRules.Insert(ep.Event, ep.Policy) } + // Load the prepared query policy + for _, pq := range policy.PreparedQueries { + p.preparedQueryRules.Insert(pq.Prefix, pq.Policy) + } + // Load the keyring policy p.keyringRule = policy.Keyring @@ -226,9 +237,7 @@ func (p *PolicyACL) KeyRead(key string) bool { _, rule, ok := p.keyRules.LongestPrefix(key) if ok { switch rule.(string) { - case KeyPolicyRead: - return true - case KeyPolicyWrite: + case PolicyRead, PolicyWrite: return true default: return false @@ -245,7 +254,7 @@ func (p *PolicyACL) KeyWrite(key string) bool { _, rule, ok := p.keyRules.LongestPrefix(key) if ok { switch rule.(string) { - case KeyPolicyWrite: + case PolicyWrite: return true default: return false @@ -260,7 +269,7 @@ func (p *PolicyACL) KeyWrite(key string) bool { func (p *PolicyACL) KeyWritePrefix(prefix string) bool { // Look for a matching rule that denies _, rule, ok := p.keyRules.LongestPrefix(prefix) - if ok && rule.(string) != KeyPolicyWrite { + if ok && rule.(string) != PolicyWrite { return false } @@ -268,7 +277,7 @@ func (p *PolicyACL) KeyWritePrefix(prefix string) bool { deny := false p.keyRules.WalkPrefix(prefix, func(path string, rule interface{}) bool { // We have a rule to prevent a write in a sub-directory! - if rule.(string) != KeyPolicyWrite { + if rule.(string) != PolicyWrite { deny = true return true } @@ -296,9 +305,7 @@ func (p *PolicyACL) ServiceRead(name string) bool { if ok { switch rule { - case ServicePolicyWrite: - return true - case ServicePolicyRead: + case PolicyRead, PolicyWrite: return true default: return false @@ -316,7 +323,7 @@ func (p *PolicyACL) ServiceWrite(name string) bool { if ok { switch rule { - case ServicePolicyWrite: + case PolicyWrite: return true default: return false @@ -333,9 +340,7 @@ func (p *PolicyACL) EventRead(name string) bool { // Longest-prefix match on event names if _, rule, ok := p.eventRules.LongestPrefix(name); ok { switch rule { - case EventPolicyRead: - return true - case EventPolicyWrite: + case PolicyRead, PolicyWrite: return true default: return false @@ -351,20 +356,58 @@ func (p *PolicyACL) EventRead(name string) bool { func (p *PolicyACL) EventWrite(name string) bool { // Longest-prefix match event names if _, rule, ok := p.eventRules.LongestPrefix(name); ok { - return rule == EventPolicyWrite + return rule == PolicyWrite } // No match, use parent return p.parent.EventWrite(name) } +// PreparedQueryRead checks if reading (listing) of a prepared query is +// allowed - this isn't execution, just listing its contents. +func (p *PolicyACL) PreparedQueryRead(prefix string) bool { + // Check for an exact rule or catch-all + _, rule, ok := p.preparedQueryRules.LongestPrefix(prefix) + + if ok { + switch rule { + case PolicyRead, PolicyWrite: + return true + default: + return false + } + } + + // No matching rule, use the parent. + return p.parent.PreparedQueryRead(prefix) +} + +// PreparedQueryWrite checks if writing (creating, updating, or deleting) of a +// prepared query is allowed. +func (p *PolicyACL) PreparedQueryWrite(prefix string) bool { + // Check for an exact rule or catch-all + _, rule, ok := p.preparedQueryRules.LongestPrefix(prefix) + + if ok { + switch rule { + case PolicyWrite: + return true + default: + return false + } + } + + // No matching rule, use the parent. + return p.parent.PreparedQueryWrite(prefix) +} + // KeyringRead is used to determine if the keyring can be // read by the current ACL token. func (p *PolicyACL) KeyringRead() bool { switch p.keyringRule { - case KeyringPolicyRead, KeyringPolicyWrite: + case PolicyRead, PolicyWrite: return true - case KeyringPolicyDeny: + case PolicyDeny: return false default: return p.parent.KeyringRead() @@ -373,7 +416,7 @@ func (p *PolicyACL) KeyringRead() bool { // KeyringWrite determines if the keyring can be manipulated. func (p *PolicyACL) KeyringWrite() bool { - if p.keyringRule == KeyringPolicyWrite { + if p.keyringRule == PolicyWrite { return true } return p.parent.KeyringWrite() @@ -388,13 +431,3 @@ func (p *PolicyACL) ACLList() bool { func (p *PolicyACL) ACLModify() bool { return p.parent.ACLModify() } - -// QueryList checks if listing of all prepared queries is allowed. -func (p *PolicyACL) QueryList() bool { - return p.parent.QueryList() -} - -// QueryModify checks if modifying of any prepared query is allowed. -func (p *PolicyACL) QueryModify() bool { - return p.parent.QueryModify() -} diff --git a/acl/acl_test.go b/acl/acl_test.go index 06cdfb755..a22752999 100644 --- a/acl/acl_test.go +++ b/acl/acl_test.go @@ -53,6 +53,12 @@ func TestStaticACL(t *testing.T) { if !all.EventWrite("foobar") { t.Fatalf("should allow") } + if !all.PreparedQueryRead("foobar") { + t.Fatalf("should allow") + } + if !all.PreparedQueryWrite("foobar") { + t.Fatalf("should allow") + } if !all.KeyringRead() { t.Fatalf("should allow") } @@ -65,12 +71,6 @@ func TestStaticACL(t *testing.T) { if all.ACLModify() { t.Fatalf("should not allow") } - if all.QueryList() { - t.Fatalf("should not allow") - } - if all.QueryModify() { - t.Fatalf("should not allow") - } if none.KeyRead("foobar") { t.Fatalf("should not allow") @@ -96,6 +96,12 @@ func TestStaticACL(t *testing.T) { if none.EventWrite("") { t.Fatalf("should not allow") } + if none.PreparedQueryRead("foobar") { + t.Fatalf("should not allow") + } + if none.PreparedQueryWrite("foobar") { + t.Fatalf("should not allow") + } if none.KeyringRead() { t.Fatalf("should now allow") } @@ -108,12 +114,6 @@ func TestStaticACL(t *testing.T) { if none.ACLModify() { t.Fatalf("should not allow") } - if none.QueryList() { - t.Fatalf("should not allow") - } - if none.QueryModify() { - t.Fatalf("should not allow") - } if !manage.KeyRead("foobar") { t.Fatalf("should allow") @@ -133,6 +133,12 @@ func TestStaticACL(t *testing.T) { if !manage.EventWrite("foobar") { t.Fatalf("should allow") } + if !manage.PreparedQueryRead("foobar") { + t.Fatalf("should allow") + } + if !manage.PreparedQueryWrite("foobar") { + t.Fatalf("should allow") + } if !manage.KeyringRead() { t.Fatalf("should allow") } @@ -145,12 +151,6 @@ func TestStaticACL(t *testing.T) { if !manage.ACLModify() { t.Fatalf("should allow") } - if !manage.QueryList() { - t.Fatalf("should allow") - } - if !manage.QueryModify() { - t.Fatalf("should allow") - } } func TestPolicyACL(t *testing.T) { @@ -159,51 +159,69 @@ func TestPolicyACL(t *testing.T) { Keys: []*KeyPolicy{ &KeyPolicy{ Prefix: "foo/", - Policy: KeyPolicyWrite, + Policy: PolicyWrite, }, &KeyPolicy{ Prefix: "foo/priv/", - Policy: KeyPolicyDeny, + Policy: PolicyDeny, }, &KeyPolicy{ Prefix: "bar/", - Policy: KeyPolicyDeny, + Policy: PolicyDeny, }, &KeyPolicy{ Prefix: "zip/", - Policy: KeyPolicyRead, + Policy: PolicyRead, }, }, Services: []*ServicePolicy{ &ServicePolicy{ Name: "", - Policy: ServicePolicyWrite, + Policy: PolicyWrite, }, &ServicePolicy{ Name: "foo", - Policy: ServicePolicyRead, + Policy: PolicyRead, }, &ServicePolicy{ Name: "bar", - Policy: ServicePolicyDeny, + Policy: PolicyDeny, }, &ServicePolicy{ Name: "barfoo", - Policy: ServicePolicyWrite, + Policy: PolicyWrite, }, }, Events: []*EventPolicy{ &EventPolicy{ Event: "", - Policy: EventPolicyRead, + Policy: PolicyRead, }, &EventPolicy{ Event: "foo", - Policy: EventPolicyWrite, + Policy: PolicyWrite, }, &EventPolicy{ Event: "bar", - Policy: EventPolicyDeny, + Policy: PolicyDeny, + }, + }, + PreparedQueries: []*PreparedQueryPolicy{ + &PreparedQueryPolicy{ + Prefix: "", + Policy: PolicyRead, + }, + &PreparedQueryPolicy{ + Prefix: "foo", + Policy: PolicyWrite, + }, + &PreparedQueryPolicy{ + Prefix: "bar", + Policy: PolicyDeny, + }, + &PreparedQueryPolicy{ + Prefix: "zoo", + Policy: PolicyWrite, }, }, } @@ -284,6 +302,31 @@ func TestPolicyACL(t *testing.T) { t.Fatalf("Event fail: %#v", c) } } + + // Test prepared queries + type querycase struct { + inp string + read bool + write bool + } + querycases := []querycase{ + {"foo", true, true}, + {"foobar", true, true}, + {"bar", false, false}, + {"barbaz", false, false}, + {"baz", true, false}, + {"nope", true, false}, + {"zoo", true, true}, + {"zookeeper", true, true}, + } + 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) + } + } } func TestPolicyACL_Parent(t *testing.T) { @@ -292,21 +335,21 @@ func TestPolicyACL_Parent(t *testing.T) { Keys: []*KeyPolicy{ &KeyPolicy{ Prefix: "foo/", - Policy: KeyPolicyWrite, + Policy: PolicyWrite, }, &KeyPolicy{ Prefix: "bar/", - Policy: KeyPolicyRead, + Policy: PolicyRead, }, }, Services: []*ServicePolicy{ &ServicePolicy{ Name: "other", - Policy: ServicePolicyWrite, + Policy: PolicyWrite, }, &ServicePolicy{ Name: "foo", - Policy: ServicePolicyRead, + Policy: PolicyRead, }, }, } @@ -319,21 +362,21 @@ func TestPolicyACL_Parent(t *testing.T) { Keys: []*KeyPolicy{ &KeyPolicy{ Prefix: "foo/priv/", - Policy: KeyPolicyRead, + Policy: PolicyRead, }, &KeyPolicy{ Prefix: "bar/", - Policy: KeyPolicyDeny, + Policy: PolicyDeny, }, &KeyPolicy{ Prefix: "zip/", - Policy: KeyPolicyRead, + Policy: PolicyRead, }, }, Services: []*ServicePolicy{ &ServicePolicy{ Name: "bar", - Policy: ServicePolicyDeny, + Policy: PolicyDeny, }, }, } @@ -395,12 +438,6 @@ func TestPolicyACL_Parent(t *testing.T) { if acl.ACLModify() { t.Fatalf("should not allow") } - if acl.QueryList() { - t.Fatalf("should not allow") - } - if acl.QueryModify() { - t.Fatalf("should not allow") - } } func TestPolicyACL_Keyring(t *testing.T) { @@ -412,9 +449,9 @@ func TestPolicyACL_Keyring(t *testing.T) { } keyringcases := []keyringcase{ {"", false, false}, - {KeyringPolicyRead, true, false}, - {KeyringPolicyWrite, true, true}, - {KeyringPolicyDeny, false, false}, + {PolicyRead, true, false}, + {PolicyWrite, true, true}, + {PolicyDeny, false, false}, } for _, c := range keyringcases { acl, err := New(DenyAll(), &Policy{Keyring: c.inp}) diff --git a/acl/policy.go b/acl/policy.go index 9009ee76b..fca018a65 100644 --- a/acl/policy.go +++ b/acl/policy.go @@ -7,28 +7,20 @@ import ( ) const ( - KeyPolicyDeny = "deny" - KeyPolicyRead = "read" - KeyPolicyWrite = "write" - ServicePolicyDeny = "deny" - ServicePolicyRead = "read" - ServicePolicyWrite = "write" - EventPolicyRead = "read" - EventPolicyWrite = "write" - EventPolicyDeny = "deny" - KeyringPolicyWrite = "write" - KeyringPolicyRead = "read" - KeyringPolicyDeny = "deny" + PolicyDeny = "deny" + PolicyRead = "read" + PolicyWrite = "write" ) // Policy is used to represent the policy specified by // an ACL configuration. type Policy struct { - ID string `hcl:"-"` - Keys []*KeyPolicy `hcl:"key,expand"` - Services []*ServicePolicy `hcl:"service,expand"` - Events []*EventPolicy `hcl:"event,expand"` - Keyring string `hcl:"keyring"` + ID string `hcl:"-"` + Keys []*KeyPolicy `hcl:"key,expand"` + Services []*ServicePolicy `hcl:"service,expand"` + Events []*EventPolicy `hcl:"event,expand"` + PreparedQueries []*PreparedQueryPolicy `hcl:"prepared_query,expand"` + Keyring string `hcl:"keyring"` } // KeyPolicy represents a policy for a key @@ -61,6 +53,30 @@ func (e *EventPolicy) GoString() string { return fmt.Sprintf("%#v", *e) } +// PreparedQueryPolicy represents a prepared query policy. +type PreparedQueryPolicy struct { + Prefix string `hcl:",key"` + Policy string +} + +func (e *PreparedQueryPolicy) GoString() string { + return fmt.Sprintf("%#v", *e) +} + +// isPolicyValid makes sure the given string matches one of the valid policies. +func isPolicyValid(policy string) bool { + switch policy { + case PolicyDeny: + return true + case PolicyRead: + return true + case PolicyWrite: + return true + default: + return false + } +} + // Parse is used to parse the specified ACL rules into an // intermediary set of policies, before being compiled into // the ACL @@ -78,44 +94,34 @@ func Parse(rules string) (*Policy, error) { // Validate the key policy for _, kp := range p.Keys { - switch kp.Policy { - case KeyPolicyDeny: - case KeyPolicyRead: - case KeyPolicyWrite: - default: + if !isPolicyValid(kp.Policy) { return nil, fmt.Errorf("Invalid key policy: %#v", kp) } } // Validate the service policy for _, sp := range p.Services { - switch sp.Policy { - case ServicePolicyDeny: - case ServicePolicyRead: - case ServicePolicyWrite: - default: + if !isPolicyValid(sp.Policy) { return nil, fmt.Errorf("Invalid service policy: %#v", sp) } } // Validate the user event policies for _, ep := range p.Events { - switch ep.Policy { - case EventPolicyRead: - case EventPolicyWrite: - case EventPolicyDeny: - default: + if !isPolicyValid(ep.Policy) { return nil, fmt.Errorf("Invalid event policy: %#v", ep) } } - // Validate the keyring policy - switch p.Keyring { - case KeyringPolicyRead: - case KeyringPolicyWrite: - case KeyringPolicyDeny: - case "": // Special case to allow omitting the keyring policy - default: + // Validate the prepared query policies + for _, pq := range p.PreparedQueries { + if !isPolicyValid(pq.Policy) { + return nil, fmt.Errorf("Invalid prepared_query policy: %#v", pq) + } + } + + // Validate the keyring policy - this one is allowed to be empty + if p.Keyring != "" && !isPolicyValid(p.Keyring) { return nil, fmt.Errorf("Invalid keyring policy: %#v", p.Keyring) } diff --git a/acl/policy_test.go b/acl/policy_test.go index eb0528ffc..ae4d76a16 100644 --- a/acl/policy_test.go +++ b/acl/policy_test.go @@ -6,7 +6,7 @@ import ( "testing" ) -func TestParse(t *testing.T) { +func TestACLPolicy_Parse_HCL(t *testing.T) { inp := ` key "" { policy = "read" @@ -35,52 +35,75 @@ event "foo" { event "bar" { policy = "deny" } +prepared_query "" { + policy = "read" +} +prepared_query "foo" { + policy = "write" +} +prepared_query "bar" { + policy = "deny" +} keyring = "deny" ` exp := &Policy{ Keys: []*KeyPolicy{ &KeyPolicy{ Prefix: "", - Policy: KeyPolicyRead, + Policy: PolicyRead, }, &KeyPolicy{ Prefix: "foo/", - Policy: KeyPolicyWrite, + Policy: PolicyWrite, }, &KeyPolicy{ Prefix: "foo/bar/", - Policy: KeyPolicyRead, + Policy: PolicyRead, }, &KeyPolicy{ Prefix: "foo/bar/baz", - Policy: KeyPolicyDeny, + Policy: PolicyDeny, }, }, Services: []*ServicePolicy{ &ServicePolicy{ Name: "", - Policy: ServicePolicyWrite, + Policy: PolicyWrite, }, &ServicePolicy{ Name: "foo", - Policy: ServicePolicyRead, + Policy: PolicyRead, }, }, Events: []*EventPolicy{ &EventPolicy{ Event: "", - Policy: EventPolicyRead, + Policy: PolicyRead, }, &EventPolicy{ Event: "foo", - Policy: EventPolicyWrite, + Policy: PolicyWrite, }, &EventPolicy{ Event: "bar", - Policy: EventPolicyDeny, + Policy: PolicyDeny, }, }, - Keyring: KeyringPolicyDeny, + PreparedQueries: []*PreparedQueryPolicy{ + &PreparedQueryPolicy{ + Prefix: "", + Policy: PolicyRead, + }, + &PreparedQueryPolicy{ + Prefix: "foo", + Policy: PolicyWrite, + }, + &PreparedQueryPolicy{ + Prefix: "bar", + Policy: PolicyDeny, + }, + }, + Keyring: PolicyDeny, } out, err := Parse(inp) @@ -93,7 +116,7 @@ keyring = "deny" } } -func TestParse_JSON(t *testing.T) { +func TestACLPolicy_Parse_JSON(t *testing.T) { inp := `{ "key": { "": { @@ -128,52 +151,77 @@ func TestParse_JSON(t *testing.T) { "policy": "deny" } }, + "prepared_query": { + "": { + "policy": "read" + }, + "foo": { + "policy": "write" + }, + "bar": { + "policy": "deny" + } + }, "keyring": "deny" }` exp := &Policy{ Keys: []*KeyPolicy{ &KeyPolicy{ Prefix: "", - Policy: KeyPolicyRead, + Policy: PolicyRead, }, &KeyPolicy{ Prefix: "foo/", - Policy: KeyPolicyWrite, + Policy: PolicyWrite, }, &KeyPolicy{ Prefix: "foo/bar/", - Policy: KeyPolicyRead, + Policy: PolicyRead, }, &KeyPolicy{ Prefix: "foo/bar/baz", - Policy: KeyPolicyDeny, + Policy: PolicyDeny, }, }, Services: []*ServicePolicy{ &ServicePolicy{ Name: "", - Policy: ServicePolicyWrite, + Policy: PolicyWrite, }, &ServicePolicy{ Name: "foo", - Policy: ServicePolicyRead, + Policy: PolicyRead, }, }, Events: []*EventPolicy{ &EventPolicy{ Event: "", - Policy: EventPolicyRead, + Policy: PolicyRead, }, &EventPolicy{ Event: "foo", - Policy: EventPolicyWrite, + Policy: PolicyWrite, }, &EventPolicy{ Event: "bar", - Policy: EventPolicyDeny, + Policy: PolicyDeny, }, }, - Keyring: KeyringPolicyDeny, + PreparedQueries: []*PreparedQueryPolicy{ + &PreparedQueryPolicy{ + Prefix: "", + Policy: PolicyRead, + }, + &PreparedQueryPolicy{ + Prefix: "foo", + Policy: PolicyWrite, + }, + &PreparedQueryPolicy{ + Prefix: "bar", + Policy: PolicyDeny, + }, + }, + Keyring: PolicyDeny, } out, err := Parse(inp) @@ -186,11 +234,30 @@ func TestParse_JSON(t *testing.T) { } } -func TestACLPolicy_badPolicy(t *testing.T) { +func TestACLPolicy_Keyring_Empty(t *testing.T) { + inp := ` +keyring = "" + ` + exp := &Policy{ + Keyring: "", + } + + out, err := Parse(inp) + if err != nil { + t.Fatalf("err: %v", err) + } + + if !reflect.DeepEqual(out, exp) { + t.Fatalf("bad: %#v %#v", out, exp) + } +} + +func TestACLPolicy_Bad_Policy(t *testing.T) { cases := []string{ `key "" { policy = "nope" }`, `service "" { policy = "nope" }`, `event "" { policy = "nope" }`, + `prepared_query "" { policy = "nope" }`, `keyring = "nope"`, } for _, c := range cases { diff --git a/consul/acl.go b/consul/acl.go index 9d2c1d94b..f795997ba 100644 --- a/consul/acl.go +++ b/consul/acl.go @@ -32,6 +32,10 @@ const ( // is no token ID provided anonymousToken = "anonymous" + // redactedToken is shown in structures with embedded tokens when they + // are not allowed to be displayed + redactedToken = "" + // Maximum number of cached ACL entries aclCacheSize = 256 ) @@ -366,6 +370,42 @@ func (f *aclFilter) filterNodeDump(dump *structs.NodeDump) { *dump = nd } +// 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 and we want to discourage using them going forward. +func (f *aclFilter) filterPreparedQueries(queries *structs.PreparedQueries) { + isManagementToken := f.acl.ACLList() + ret := make(structs.PreparedQueries, 0, len(*queries)) + for _, query := range *queries { + if !f.acl.PreparedQueryRead(query.GetACLPrefix()) { + 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 == "" { + 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) + } + } + *queries = ret +} + // filterACL is used to filter results from our service catalog based on the // rules configured for the provided token. The subject is scrubbed and // modified in-place, leaving only resources the token can access. @@ -402,9 +442,15 @@ func (s *Server) filterACL(token string, subj interface{}) error { case *structs.IndexedCheckServiceNodes: filt.filterCheckServiceNodes(&v.Nodes) + case *structs.CheckServiceNodes: + filt.filterCheckServiceNodes(v) + case *structs.IndexedNodeDump: filt.filterNodeDump(&v.Dump) + case *structs.IndexedPreparedQueries: + filt.filterPreparedQueries(&v.Queries) + 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 702be3f95..43070f547 100644 --- a/consul/acl_test.go +++ b/consul/acl_test.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "os" + "reflect" "testing" "github.com/hashicorp/consul/acl" @@ -861,6 +862,73 @@ func TestACL_filterNodeDump(t *testing.T) { } } +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", + 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", + Token: "root", + Service: structs.ServiceQuery{ + Service: "bar", + }, + }, + } + + // Try permissive filtering with a management token. This will allow the + // embedded tokens to be seen. + filt := newAclFilter(acl.ManageAll(), nil) + filt.filterPreparedQueries(&queries) + if !reflect.DeepEqual(queries, expected) { + t.Fatalf("bad: %#v", queries) + } + + // Hang on to the entry with a token, which needs to survive the next + // operation. + original := queries[1] + + // Now try permissive filtering with a client token, which should cause + // the embedded tokens to get redacted. + filt = newAclFilter(acl.AllowAll(), nil) + filt.filterPreparedQueries(&queries) + expected[1].Token = redactedToken + if !reflect.DeepEqual(queries, expected) { + t.Fatalf("bad: %#v", queries) + } + + // Make sure that the original object didn't lose its token. + if original.Token != "root" { + t.Fatalf("bad token: %s", original.Token) + } + + // Now try restrictive filtering. + filt = newAclFilter(acl.DenyAll(), nil) + filt.filterPreparedQueries(&queries) + if len(queries) != 0 { + t.Fatalf("bad: %#v", queries) + } +} + func TestACL_unhandledFilterType(t *testing.T) { defer func(t *testing.T) { if recover() == nil { diff --git a/consul/prepared_query_endpoint.go b/consul/prepared_query_endpoint.go index 1227f7883..29aa0c55a 100644 --- a/consul/prepared_query_endpoint.go +++ b/consul/prepared_query_endpoint.go @@ -56,14 +56,21 @@ func (p *PreparedQuery) Apply(args *structs.PreparedQueryRequest, reply *string) } *reply = args.Query.ID - // Grab the ACL because we need it in several places below. + // 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. 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 + } - // Enforce that any modify operation has the same token used when the - // query was created, or a management token with sufficient rights. + // 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. if args.Op != structs.PreparedQueryCreate { state := p.srv.fsm.State() _, query, err := state.PreparedQueryGet(args.Query.ID) @@ -73,8 +80,8 @@ 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 (query.Token != args.Token) && (acl != nil && !acl.QueryModify()) { - p.srv.logger.Printf("[WARN] consul.prepared_query: Operation on prepared query '%s' denied because ACL didn't match ACL used to create the query, and a management token wasn't supplied", 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 } } @@ -86,11 +93,6 @@ func (p *PreparedQuery) Apply(args *structs.PreparedQueryRequest, reply *string) return fmt.Errorf("Invalid prepared query: %v", err) } - if acl != nil && !acl.ServiceRead(args.Query.Service.Service) { - p.srv.logger.Printf("[WARN] consul.prepared_query: Operation on prepared query for service '%s' denied due to ACLs", args.Query.Service.Service) - return permissionDeniedErr - } - case structs.PreparedQueryDelete: // Nothing else to verify here, just do the delete (we only look // at the ID field for this op). @@ -99,10 +101,7 @@ func (p *PreparedQuery) Apply(args *structs.PreparedQueryRequest, reply *string) return fmt.Errorf("Unknown prepared query operation: %s", args.Op) } - // At this point the token has been vetted, so make sure the token that - // is stored in the state store matches what was supplied. - args.Query.Token = args.Token - + // Commit the query to the state store. resp, err := p.srv.raftApply(structs.PreparedQueryRequestType, args) if err != nil { p.srv.logger.Printf("[ERR] consul.prepared_query: Apply failed %v", err) @@ -128,7 +127,7 @@ func parseQuery(query *structs.PreparedQuery) error { // transaction. Otherwise, people could "steal" queries that they don't // have proper ACL rights to change. // - Session is optional and checked for integrity during the transaction. - // - Token is checked outside this fn. + // - Token is checked when a query is executed. // Parse the service query sub-structure. if err := parseService(&query.Service); err != nil { @@ -148,7 +147,7 @@ func parseQuery(query *structs.PreparedQuery) error { // checked, as noted in the comments below. This also updates all the parsed // fields of the query. func parseService(svc *structs.ServiceQuery) error { - // Service is required. We check integrity during the transaction. + // Service is required. if svc.Service == "" { return fmt.Errorf("Must provide a service name to query") } @@ -191,13 +190,6 @@ func (p *PreparedQuery) Get(args *structs.PreparedQuerySpecificRequest, return err } - // We will use this in the loop to see if the caller is allowed to see - // the query. - acl, err := p.srv.resolveToken(args.Token) - if err != nil { - return err - } - // Get the requested query. state := p.srv.fsm.State() return p.srv.blockingRPC( @@ -213,13 +205,20 @@ func (p *PreparedQuery) Get(args *structs.PreparedQuerySpecificRequest, return ErrQueryNotFound } - if (query.Token != args.Token) && (acl != nil && !acl.QueryList()) { - p.srv.logger.Printf("[WARN] consul.prepared_query: Request to get prepared query '%s' denied because ACL didn't match ACL used to create the query, and a management token wasn't supplied", args.QueryID) + reply.Index = index + reply.Queries = structs.PreparedQueries{query} + 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. + 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) return permissionDeniedErr } - reply.Index = index - reply.Queries = structs.PreparedQueries{query} return nil }) } @@ -230,16 +229,6 @@ func (p *PreparedQuery) List(args *structs.DCSpecificRequest, reply *structs.Ind return err } - // This always requires a management token. - acl, err := p.srv.resolveToken(args.Token) - if err != nil { - return err - } - if acl != nil && !acl.QueryList() { - p.srv.logger.Printf("[WARN] consul.prepared_query: Request to list prepared queries denied due to ACLs") - return permissionDeniedErr - } - // Get the list of queries. state := p.srv.fsm.State() return p.srv.blockingRPC( @@ -253,7 +242,7 @@ func (p *PreparedQuery) List(args *structs.DCSpecificRequest, reply *structs.Ind } reply.Index, reply.Queries = index, queries - return nil + return p.srv.filterACL(args.Token, reply) }) } @@ -290,6 +279,21 @@ func (p *PreparedQuery) Execute(args *structs.PreparedQueryExecuteRequest, return err } + // If they supplied a token with the query, use that, otherwise use the + // token passed in with the request. + token := args.QueryOptions.Token + if query.Token != "" { + token = query.Token + } + if err := p.srv.filterACL(token, &reply.Nodes); err != nil { + return err + } + + // TODO (slackpad) We could add a special case here that will avoid the + // fail over if we filtered everything due to ACLs. This seems like it + // might not be worth the code complexity and behavior differences, + // though, since this is essentially a misconfiguration. + // Shuffle the results in case coordinates are not available if they // requested an RTT sort. reply.Nodes.Shuffle() @@ -340,6 +344,16 @@ func (p *PreparedQuery) ExecuteRemote(args *structs.PreparedQueryExecuteRemoteRe return err } + // If they supplied a token with the query, use that, otherwise use the + // token passed in with the request. + token := args.QueryOptions.Token + if args.Query.Token != "" { + token = args.Query.Token + } + if err := p.srv.filterACL(token, &reply.Nodes); err != nil { + return err + } + // We don't bother trying to do an RTT sort here since we are by // definition in another DC. We just shuffle to make sure that we // balance the load across the results. @@ -354,7 +368,7 @@ func (p *PreparedQuery) ExecuteRemote(args *structs.PreparedQueryExecuteRemoteRe } // execute runs a prepared query in the local DC without any failover. We don't -// apply any sorting options at this level - it should be done up above. +// apply any sorting options or ACL checks at this level - it should be done up above. func (p *PreparedQuery) execute(query *structs.PreparedQuery, reply *structs.PreparedQueryExecuteResponse) error { state := p.srv.fsm.State() @@ -363,20 +377,6 @@ func (p *PreparedQuery) execute(query *structs.PreparedQuery, return err } - // This is kind of a paranoia ACL check, in case something changed with - // the token from the time the query was registered. Note that we use - // the token stored with the query, NOT the passed-in one, which is - // critical to how queries work (the query becomes a proxy for a lookup - // using the ACL it was created with). - acl, err := p.srv.resolveToken(query.Token) - if err != nil { - return err - } - if acl != nil && !acl.ServiceRead(query.Service.Service) { - p.srv.logger.Printf("[WARN] consul.prepared_query: Execute of prepared query for service '%s' denied due to ACLs", query.Service.Service) - return permissionDeniedErr - } - // Filter out any unhealthy nodes. nodes = nodes.Filter(query.Service.OnlyPassing) @@ -555,8 +555,8 @@ func queryFailover(q queryServer, query *structs.PreparedQuery, // Note that we pass along the limit since it can be applied // remotely to save bandwidth. We also pass along the consistency - // mode information we were given, so that applies to the remote - // query as well. + // mode information and token we were given, so that applies to + // the remote query as well. remote := &structs.PreparedQueryExecuteRemoteRequest{ Datacenter: dc, Query: *query, diff --git a/consul/prepared_query_endpoint_test.go b/consul/prepared_query_endpoint_test.go index 59d2297ea..ba406a23d 100644 --- a/consul/prepared_query_endpoint_test.go +++ b/consul/prepared_query_endpoint_test.go @@ -75,7 +75,8 @@ func TestPreparedQuery_Apply(t *testing.T) { // Fix up the ID but invalidate the query itself. This proves we call // parseQuery for a create, but that function is checked in detail as - // part of another test. + // part of another test so we don't have to exercise all the checks + // here. query.Op = structs.PreparedQueryCreate query.Query.ID = "" query.Query.Service.Failover.NearestN = -1 @@ -86,14 +87,14 @@ func TestPreparedQuery_Apply(t *testing.T) { // Fix that and make sure it propagates an error from the Raft apply. query.Query.Service.Failover.NearestN = 0 - query.Query.Service.Service = "nope" + query.Query.Session = "nope" err = msgpackrpc.CallWithCodec(codec, "PreparedQuery.Apply", &query, &reply) - if err == nil || !strings.Contains(err.Error(), "invalid service") { + if err == nil || !strings.Contains(err.Error(), "failed session lookup") { t.Fatalf("bad: %v", err) } // Fix that and make sure the apply goes through. - query.Query.Service.Service = "redis" + query.Query.Session = "" if err = msgpackrpc.CallWithCodec(codec, "PreparedQuery.Apply", &query, &reply); err != nil { t.Fatalf("err: %v", err) } @@ -208,12 +209,12 @@ func TestPreparedQuery_Apply_ACLDeny(t *testing.T) { testutil.WaitForLeader(t, s1.RPC, "dc1") - // Create two ACLs with read permission to the service. - var token1, token2 string + // Create an ACL with write permissions for redis queries. + var token string { var rules = ` - service "redis" { - policy = "read" + prepared_query "redis" { + policy = "write" } ` @@ -227,17 +228,9 @@ func TestPreparedQuery_Apply_ACLDeny(t *testing.T) { }, WriteRequest: structs.WriteRequest{Token: "root"}, } - var reply string - - if err := msgpackrpc.CallWithCodec(codec, "ACL.Apply", &req, &reply); err != nil { + if err := msgpackrpc.CallWithCodec(codec, "ACL.Apply", &req, &token); err != nil { t.Fatalf("err: %v", err) } - token1 = reply - - if err := msgpackrpc.CallWithCodec(codec, "ACL.Apply", &req, &reply); err != nil { - t.Fatalf("err: %v", err) - } - token2 = reply } // Set up a node and service in the catalog. @@ -280,14 +273,16 @@ func TestPreparedQuery_Apply_ACLDeny(t *testing.T) { } // Now add the token and try again. - query.WriteRequest.Token = token1 + query.WriteRequest.Token = token if err = msgpackrpc.CallWithCodec(codec, "PreparedQuery.Apply", &query, &reply); err != nil { t.Fatalf("err: %v", err) } // Capture the ID and set the token, then read back the query to verify. + // Note that unlike previous versions of Consul, we DO NOT capture the + // token. We will set that here just to be explicit about it. query.Query.ID = reply - query.Query.Token = token1 + query.Query.Token = "" { req := &structs.PreparedQuerySpecificRequest{ Datacenter: "dc1", @@ -312,38 +307,22 @@ func TestPreparedQuery_Apply_ACLDeny(t *testing.T) { } } - // Try to do an update with a different token that does have access to - // the service, but isn't the one that was used to create the query. + // Try to do an update without a token; this should get rejected. query.Op = structs.PreparedQueryUpdate - query.WriteRequest.Token = token2 - err = msgpackrpc.CallWithCodec(codec, "PreparedQuery.Apply", &query, &reply) - if err == nil || !strings.Contains(err.Error(), permissionDenied) { - t.Fatalf("bad: %v", err) - } - - // Try again with no token. query.WriteRequest.Token = "" err = msgpackrpc.CallWithCodec(codec, "PreparedQuery.Apply", &query, &reply) if err == nil || !strings.Contains(err.Error(), permissionDenied) { t.Fatalf("bad: %v", err) } - // Try again with the original token. This should go through. - query.WriteRequest.Token = token1 + // Try again with the original token; this should go through. + query.WriteRequest.Token = token if err = msgpackrpc.CallWithCodec(codec, "PreparedQuery.Apply", &query, &reply); err != nil { t.Fatalf("err: %v", err) } - // Try to do a delete with a different token that does have access to - // the service, but isn't the one that was used to create the query. + // Try to do a delete with no token; this should get rejected. query.Op = structs.PreparedQueryDelete - query.WriteRequest.Token = token2 - err = msgpackrpc.CallWithCodec(codec, "PreparedQuery.Apply", &query, &reply) - if err == nil || !strings.Contains(err.Error(), permissionDenied) { - t.Fatalf("bad: %v", err) - } - - // Try again with no token. query.WriteRequest.Token = "" err = msgpackrpc.CallWithCodec(codec, "PreparedQuery.Apply", &query, &reply) if err == nil || !strings.Contains(err.Error(), permissionDenied) { @@ -351,7 +330,7 @@ func TestPreparedQuery_Apply_ACLDeny(t *testing.T) { } // Try again with the original token. This should go through. - query.WriteRequest.Token = token1 + query.WriteRequest.Token = token if err = msgpackrpc.CallWithCodec(codec, "PreparedQuery.Apply", &query, &reply); err != nil { t.Fatalf("err: %v", err) } @@ -378,15 +357,15 @@ func TestPreparedQuery_Apply_ACLDeny(t *testing.T) { // Make the query again. query.Op = structs.PreparedQueryCreate query.Query.ID = "" - query.Query.Token = "" - query.WriteRequest.Token = token1 + 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. + // Check that it's there, and again make sure that the token did not get + // captured. query.Query.ID = reply - query.Query.Token = token1 + query.Query.Token = "" { req := &structs.PreparedQuerySpecificRequest{ Datacenter: "dc1", @@ -418,8 +397,8 @@ func TestPreparedQuery_Apply_ACLDeny(t *testing.T) { t.Fatalf("err: %v", err) } - // That last update should have changed the token to the management one. - query.Query.Token = "root" + // That last update should not have captured a token. + query.Query.Token = "" { req := &structs.PreparedQuerySpecificRequest{ Datacenter: "dc1", @@ -447,15 +426,14 @@ func TestPreparedQuery_Apply_ACLDeny(t *testing.T) { // Make another query. query.Op = structs.PreparedQueryCreate query.Query.ID = "" - query.Query.Token = "" - query.WriteRequest.Token = token1 + 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. + // Check that it's there and that the token did not get captured. query.Query.ID = reply - query.Query.Token = token1 + query.Query.Token = "" { req := &structs.PreparedQuerySpecificRequest{ Datacenter: "dc1", @@ -505,6 +483,52 @@ func TestPreparedQuery_Apply_ACLDeny(t *testing.T) { t.Fatalf("bad: %v", resp) } } + + // Use the root token to make a query for a different service. + query.Op = structs.PreparedQueryCreate + query.Query.ID = "" + query.Query.Service.Service = "cassandra" + query.WriteRequest.Token = "root" + 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) + } + } + + // 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.WriteRequest.Token = token + err = msgpackrpc.CallWithCodec(codec, "PreparedQuery.Apply", &query, &reply) + if err == nil || !strings.Contains(err.Error(), permissionDenied) { + t.Fatalf("bad: %v", err) + } } func TestPreparedQuery_Apply_ForwardLeader(t *testing.T) { @@ -630,12 +654,12 @@ func TestPreparedQuery_Get(t *testing.T) { testutil.WaitForLeader(t, s1.RPC, "dc1") - // Create two ACLs with read permission to the service. - var token1, token2 string + // Create an ACL with write permissions for redis queries. + var token string { var rules = ` - service "redis" { - policy = "read" + prepared_query "redis" { + policy = "write" } ` @@ -649,17 +673,9 @@ func TestPreparedQuery_Get(t *testing.T) { }, WriteRequest: structs.WriteRequest{Token: "root"}, } - var reply string - - if err := msgpackrpc.CallWithCodec(codec, "ACL.Apply", &req, &reply); err != nil { + if err := msgpackrpc.CallWithCodec(codec, "ACL.Apply", &req, &token); err != nil { t.Fatalf("err: %v", err) } - token1 = reply - - if err := msgpackrpc.CallWithCodec(codec, "ACL.Apply", &req, &reply); err != nil { - t.Fatalf("err: %v", err) - } - token2 = reply } // Set up a node and service in the catalog. @@ -692,21 +708,20 @@ func TestPreparedQuery_Get(t *testing.T) { Service: "redis", }, }, - WriteRequest: structs.WriteRequest{Token: token1}, + WriteRequest: structs.WriteRequest{Token: token}, } var reply string if err := msgpackrpc.CallWithCodec(codec, "PreparedQuery.Apply", &query, &reply); err != nil { t.Fatalf("err: %v", err) } - // Capture the ID and set the token, then read back the query to verify. + // Capture the ID, then read back the query to verify. query.Query.ID = reply - query.Query.Token = token1 { req := &structs.PreparedQuerySpecificRequest{ Datacenter: "dc1", QueryID: query.Query.ID, - QueryOptions: structs.QueryOptions{Token: token1}, + QueryOptions: structs.QueryOptions{Token: token}, } var resp structs.IndexedPreparedQueries if err := msgpackrpc.CallWithCodec(codec, "PreparedQuery.Get", req, &resp); err != nil { @@ -726,27 +741,7 @@ func TestPreparedQuery_Get(t *testing.T) { } } - // Now try to read it with a token that has read access to the - // service but isn't the token used to create the query. This should - // be denied. - { - req := &structs.PreparedQuerySpecificRequest{ - Datacenter: "dc1", - QueryID: query.Query.ID, - QueryOptions: structs.QueryOptions{Token: token2}, - } - var resp structs.IndexedPreparedQueries - err := msgpackrpc.CallWithCodec(codec, "PreparedQuery.Get", req, &resp) - if err == nil || !strings.Contains(err.Error(), permissionDenied) { - t.Fatalf("bad: %v", err) - } - - if len(resp.Queries) != 0 { - t.Fatalf("bad: %v", resp) - } - } - - // Try again with no token, which should also be denied. + // Try again with no token, which should return an error. { req := &structs.PreparedQuerySpecificRequest{ Datacenter: "dc1", @@ -794,7 +789,7 @@ func TestPreparedQuery_Get(t *testing.T) { req := &structs.PreparedQuerySpecificRequest{ Datacenter: "dc1", QueryID: generateUUID(), - QueryOptions: structs.QueryOptions{Token: token1}, + QueryOptions: structs.QueryOptions{Token: token}, } var resp structs.IndexedPreparedQueries if err := msgpackrpc.CallWithCodec(codec, "PreparedQuery.Get", req, &resp); err != nil { @@ -822,12 +817,12 @@ func TestPreparedQuery_List(t *testing.T) { testutil.WaitForLeader(t, s1.RPC, "dc1") - // Create an ACL with read permission to the service. + // Create an ACL with write permissions for redis queries. var token string { var rules = ` - service "redis" { - policy = "read" + prepared_query "redis" { + policy = "write" } ` @@ -866,11 +861,11 @@ func TestPreparedQuery_List(t *testing.T) { } } - // Query with a legit management token but no queries. + // Query with a legit token but no queries. { req := &structs.DCSpecificRequest{ Datacenter: "dc1", - QueryOptions: structs.QueryOptions{Token: "root"}, + QueryOptions: structs.QueryOptions{Token: token}, } var resp structs.IndexedPreparedQueries if err := msgpackrpc.CallWithCodec(codec, "PreparedQuery.List", req, &resp); err != nil { @@ -899,36 +894,41 @@ func TestPreparedQuery_List(t *testing.T) { t.Fatalf("err: %v", err) } - // Capture the ID and set the token, then try to list all the queries. - // A management token is required so this should be denied. + // Capture the ID and read back the query to verify. query.Query.ID = reply - query.Query.Token = token { req := &structs.DCSpecificRequest{ Datacenter: "dc1", QueryOptions: structs.QueryOptions{Token: token}, } var resp structs.IndexedPreparedQueries - err := msgpackrpc.CallWithCodec(codec, "PreparedQuery.List", req, &resp) - if err == nil || !strings.Contains(err.Error(), permissionDenied) { - t.Fatalf("bad: %v", err) + if err := msgpackrpc.CallWithCodec(codec, "PreparedQuery.List", req, &resp); err != nil { + t.Fatalf("err: %v", err) } - if len(resp.Queries) != 0 { + 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) + } } - // An empty token should fail in a similar way. + // An empty token should result in an empty list because of ACL + // filtering. { req := &structs.DCSpecificRequest{ Datacenter: "dc1", QueryOptions: structs.QueryOptions{Token: ""}, } var resp structs.IndexedPreparedQueries - err := msgpackrpc.CallWithCodec(codec, "PreparedQuery.List", req, &resp) - if err == nil || !strings.Contains(err.Error(), permissionDenied) { - t.Fatalf("bad: %v", err) + if err := msgpackrpc.CallWithCodec(codec, "PreparedQuery.List", req, &resp); err != nil { + t.Fatalf("err: %v", err) } if len(resp.Queries) != 0 { @@ -936,7 +936,7 @@ func TestPreparedQuery_List(t *testing.T) { } } - // Now try a legit management token. + // But a management token should work. { req := &structs.DCSpecificRequest{ Datacenter: "dc1", @@ -978,8 +978,6 @@ func TestPreparedQuery_Execute(t *testing.T) { dir2, s2 := testServerWithConfig(t, func(c *Config) { c.Datacenter = "dc2" c.ACLDatacenter = "dc1" - c.ACLMasterToken = "root" - c.ACLDefaultPolicy = "deny" }) defer os.RemoveAll(dir2) defer s2.Shutdown() @@ -1004,7 +1002,7 @@ func TestPreparedQuery_Execute(t *testing.T) { }) // Create an ACL with read permission to the service. - var token string + var execToken string { var rules = ` service "foo" { @@ -1022,7 +1020,31 @@ func TestPreparedQuery_Execute(t *testing.T) { }, WriteRequest: structs.WriteRequest{Token: "root"}, } - if err := msgpackrpc.CallWithCodec(codec1, "ACL.Apply", &req, &token); err != nil { + if err := msgpackrpc.CallWithCodec(codec1, "ACL.Apply", &req, &execToken); err != nil { + t.Fatalf("err: %v", err) + } + } + + // 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) } } @@ -1070,7 +1092,7 @@ func TestPreparedQuery_Execute(t *testing.T) { TTL: "10s", }, }, - WriteRequest: structs.WriteRequest{Token: token}, + WriteRequest: structs.WriteRequest{Token: queryCRUDToken}, } if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Apply", &query, &query.Query.ID); err != nil { t.Fatalf("err: %v", err) @@ -1099,6 +1121,7 @@ func TestPreparedQuery_Execute(t *testing.T) { req := structs.PreparedQueryExecuteRequest{ Datacenter: "dc1", QueryIDOrName: query.Query.ID, + QueryOptions: structs.QueryOptions{Token: execToken}, } var reply structs.PreparedQueryExecuteResponse @@ -1121,6 +1144,7 @@ func TestPreparedQuery_Execute(t *testing.T) { Datacenter: "dc1", QueryIDOrName: query.Query.ID, Limit: 3, + QueryOptions: structs.QueryOptions{Token: execToken}, } var reply structs.PreparedQueryExecuteResponse @@ -1163,6 +1187,7 @@ func TestPreparedQuery_Execute(t *testing.T) { Datacenter: "dc1", Node: "node3", }, + QueryOptions: structs.QueryOptions{Token: execToken}, } var reply structs.PreparedQueryExecuteResponse @@ -1188,6 +1213,7 @@ func TestPreparedQuery_Execute(t *testing.T) { req := structs.PreparedQueryExecuteRequest{ Datacenter: "dc1", QueryIDOrName: query.Query.ID, + QueryOptions: structs.QueryOptions{Token: execToken}, } var reply structs.PreparedQueryExecuteResponse @@ -1247,6 +1273,7 @@ func TestPreparedQuery_Execute(t *testing.T) { req := structs.PreparedQueryExecuteRequest{ Datacenter: "dc1", QueryIDOrName: query.Query.ID, + QueryOptions: structs.QueryOptions{Token: execToken}, } var reply structs.PreparedQueryExecuteResponse @@ -1274,6 +1301,7 @@ func TestPreparedQuery_Execute(t *testing.T) { req := structs.PreparedQueryExecuteRequest{ Datacenter: "dc1", QueryIDOrName: query.Query.ID, + QueryOptions: structs.QueryOptions{Token: execToken}, } var reply structs.PreparedQueryExecuteResponse @@ -1302,6 +1330,7 @@ func TestPreparedQuery_Execute(t *testing.T) { req := structs.PreparedQueryExecuteRequest{ Datacenter: "dc1", QueryIDOrName: query.Query.ID, + QueryOptions: structs.QueryOptions{Token: execToken}, } var reply structs.PreparedQueryExecuteResponse @@ -1337,6 +1366,7 @@ func TestPreparedQuery_Execute(t *testing.T) { req := structs.PreparedQueryExecuteRequest{ Datacenter: "dc1", QueryIDOrName: query.Query.ID, + QueryOptions: structs.QueryOptions{Token: execToken}, } var reply structs.PreparedQueryExecuteResponse @@ -1358,6 +1388,113 @@ func TestPreparedQuery_Execute(t *testing.T) { } } + // Make a new exec token that can't read the service. + var denyToken string + { + var rules = ` + service "foo" { + policy = "deny" + } + ` + + 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, &denyToken); err != nil { + t.Fatalf("err: %v", err) + } + } + + // Make sure the query gets denied with this token. + { + req := structs.PreparedQueryExecuteRequest{ + Datacenter: "dc1", + QueryIDOrName: query.Query.ID, + QueryOptions: structs.QueryOptions{Token: denyToken}, + } + + var reply structs.PreparedQueryExecuteResponse + if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Execute", &req, &reply); err != nil { + t.Fatalf("err: %v", err) + } + + if len(reply.Nodes) != 0 || + reply.Datacenter != "dc1" || reply.Failovers != 0 || + reply.Service != query.Query.Service.Service || + !reflect.DeepEqual(reply.DNS, query.Query.DNS) || + !reply.QueryMeta.KnownLeader { + t.Fatalf("bad: %v", reply) + } + } + + // Bake the exec token into the query. + query.Query.Token = execToken + if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Apply", &query, &query.Query.ID); err != nil { + t.Fatalf("err: %v", err) + } + + // Now even querying with the deny token should work. + { + req := structs.PreparedQueryExecuteRequest{ + Datacenter: "dc1", + QueryIDOrName: query.Query.ID, + QueryOptions: structs.QueryOptions{Token: denyToken}, + } + + var reply structs.PreparedQueryExecuteResponse + if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Execute", &req, &reply); err != nil { + t.Fatalf("err: %v", err) + } + + if len(reply.Nodes) != 8 || + reply.Datacenter != "dc1" || reply.Failovers != 0 || + reply.Service != query.Query.Service.Service || + !reflect.DeepEqual(reply.DNS, query.Query.DNS) || + !reply.QueryMeta.KnownLeader { + t.Fatalf("bad: %v", reply) + } + for _, node := range reply.Nodes { + if node.Node.Node == "node1" || node.Node.Node == "node3" { + t.Fatalf("bad: %v", node) + } + } + } + + // Un-bake the token. + query.Query.Token = "" + if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Apply", &query, &query.Query.ID); err != nil { + t.Fatalf("err: %v", err) + } + + // Make sure the query gets denied again with the deny token. + { + req := structs.PreparedQueryExecuteRequest{ + Datacenter: "dc1", + QueryIDOrName: query.Query.ID, + QueryOptions: structs.QueryOptions{Token: denyToken}, + } + + var reply structs.PreparedQueryExecuteResponse + if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Execute", &req, &reply); err != nil { + t.Fatalf("err: %v", err) + } + + if len(reply.Nodes) != 0 || + reply.Datacenter != "dc1" || reply.Failovers != 0 || + reply.Service != query.Query.Service.Service || + !reflect.DeepEqual(reply.DNS, query.Query.DNS) || + !reply.QueryMeta.KnownLeader { + t.Fatalf("bad: %v", reply) + } + } + // Now fail everything in dc1 and we should get an empty list back. for i := 0; i < 10; i++ { setHealth(fmt.Sprintf("node%d", i+1), structs.HealthCritical) @@ -1366,6 +1503,7 @@ func TestPreparedQuery_Execute(t *testing.T) { req := structs.PreparedQueryExecuteRequest{ Datacenter: "dc1", QueryIDOrName: query.Query.ID, + QueryOptions: structs.QueryOptions{Token: execToken}, } var reply structs.PreparedQueryExecuteResponse @@ -1393,6 +1531,7 @@ func TestPreparedQuery_Execute(t *testing.T) { req := structs.PreparedQueryExecuteRequest{ Datacenter: "dc1", QueryIDOrName: query.Query.ID, + QueryOptions: structs.QueryOptions{Token: execToken}, } var reply structs.PreparedQueryExecuteResponse @@ -1420,7 +1559,10 @@ func TestPreparedQuery_Execute(t *testing.T) { Datacenter: "dc1", QueryIDOrName: query.Query.ID, Limit: 3, - QueryOptions: structs.QueryOptions{RequireConsistent: true}, + QueryOptions: structs.QueryOptions{ + Token: execToken, + RequireConsistent: true, + }, } var reply structs.PreparedQueryExecuteResponse @@ -1448,6 +1590,7 @@ func TestPreparedQuery_Execute(t *testing.T) { req := structs.PreparedQueryExecuteRequest{ Datacenter: "dc1", QueryIDOrName: query.Query.ID, + QueryOptions: structs.QueryOptions{Token: execToken}, } var reply structs.PreparedQueryExecuteResponse @@ -1477,47 +1620,60 @@ func TestPreparedQuery_Execute(t *testing.T) { t.Fatalf("unique shuffle ratio too low: %d/100", len(uniques)) } - // Finally, take away the token's ability to read the service. - { - var rules = ` - service "foo" { - policy = "deny" - } - ` - - req := structs.ACLRequest{ - Datacenter: "dc1", - Op: structs.ACLSet, - ACL: structs.ACL{ - ID: token, - Name: "User token", - Type: structs.ACLTypeClient, - Rules: rules, - }, - WriteRequest: structs.WriteRequest{Token: "root"}, - } - if err := msgpackrpc.CallWithCodec(codec1, "ACL.Apply", &req, &token); err != nil { - t.Fatalf("err: %v", err) - } - } - - // Now the query should be denied. + // Make sure the query response from dc2 gets denied with the deny token. { req := structs.PreparedQueryExecuteRequest{ Datacenter: "dc1", QueryIDOrName: query.Query.ID, + QueryOptions: structs.QueryOptions{Token: denyToken}, } var reply structs.PreparedQueryExecuteResponse - err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Execute", &req, &reply) - if err == nil || !strings.Contains(err.Error(), permissionDenied) { - t.Fatalf("bad: %v", err) + if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Execute", &req, &reply); err != nil { + t.Fatalf("err: %v", err) } - if len(reply.Nodes) != 0 { + if len(reply.Nodes) != 0 || + reply.Datacenter != "dc2" || reply.Failovers != 1 || + reply.Service != query.Query.Service.Service || + !reflect.DeepEqual(reply.DNS, query.Query.DNS) || + !reply.QueryMeta.KnownLeader { t.Fatalf("bad: %v", reply) } } + + // Bake the exec token into the query. + query.Query.Token = execToken + if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Apply", &query, &query.Query.ID); err != nil { + t.Fatalf("err: %v", err) + } + + // Now even querying with the deny token should work. + { + req := structs.PreparedQueryExecuteRequest{ + Datacenter: "dc1", + QueryIDOrName: query.Query.ID, + QueryOptions: structs.QueryOptions{Token: denyToken}, + } + + var reply structs.PreparedQueryExecuteResponse + if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Execute", &req, &reply); err != nil { + t.Fatalf("err: %v", err) + } + + if len(reply.Nodes) != 9 || + reply.Datacenter != "dc2" || reply.Failovers != 1 || + reply.Service != query.Query.Service.Service || + !reflect.DeepEqual(reply.DNS, query.Query.DNS) || + !reply.QueryMeta.KnownLeader { + t.Fatalf("bad: %v", reply) + } + for _, node := range reply.Nodes { + if node.Node.Node == "node3" { + t.Fatalf("bad: %v", node) + } + } + } } func TestPreparedQuery_Execute_ForwardLeader(t *testing.T) { diff --git a/consul/state/prepared_query.go b/consul/state/prepared_query.go index f2fc1a8b0..dad07b896 100644 --- a/consul/state/prepared_query.go +++ b/consul/state/prepared_query.go @@ -119,14 +119,9 @@ func (s *StateStore) preparedQuerySetTxn(tx *memdb.Txn, idx uint64, query *struc } } - // Verify that the service exists. - service, err := tx.First("services", "service", query.Service.Service) - if err != nil { - return fmt.Errorf("failed service lookup: %s", err) - } - if service == nil { - return fmt.Errorf("invalid service %#v", query.Service.Service) - } + // We do not verify the service here, nor the token, if any. These are + // checked at execute time and not doing integrity checking on them + // helps avoid bootstrapping chicken and egg problems. // Insert the query. if err := tx.Insert("prepared-queries", query); err != nil { diff --git a/consul/state/prepared_query_test.go b/consul/state/prepared_query_test.go index e8261b8c4..83c3625c3 100644 --- a/consul/state/prepared_query_test.go +++ b/consul/state/prepared_query_test.go @@ -54,15 +54,16 @@ func TestStateStore_PreparedQuerySet_PreparedQueryGet(t *testing.T) { // Build a legit-looking query with the most basic options. query := &structs.PreparedQuery{ - ID: testUUID(), + ID: testUUID(), + Session: "nope", Service: structs.ServiceQuery{ Service: "redis", }, } - // The set will still fail because the service isn't registered yet. + // The set will still fail because the session is bogus. err = s.PreparedQuerySet(1, query) - if err == nil || !strings.Contains(err.Error(), "invalid service") { + if err == nil || !strings.Contains(err.Error(), "failed session lookup") { t.Fatalf("bad: %v", err) } @@ -71,9 +72,10 @@ func TestStateStore_PreparedQuerySet_PreparedQueryGet(t *testing.T) { t.Fatalf("bad index: %d", idx) } - // Now register the service. + // Now register the service and remove the bogus session. testRegisterNode(t, s, 1, "foo") testRegisterService(t, s, 2, "foo", "redis") + query.Session = "" // This should go through. if err := s.PreparedQuerySet(3, query); err != nil { diff --git a/consul/structs/prepared_query.go b/consul/structs/prepared_query.go index af360f7d2..d88c2f073 100644 --- a/consul/structs/prepared_query.go +++ b/consul/structs/prepared_query.go @@ -72,6 +72,11 @@ type PreparedQuery struct { RaftIndex } +// GetACLPrefix returns the prefix of this query for ACL purposes. +func (pq *PreparedQuery) GetACLPrefix() string { + return pq.Service.Service +} + type PreparedQueries []*PreparedQuery type IndexedPreparedQueries struct { diff --git a/consul/structs/prepared_query_test.go b/consul/structs/prepared_query_test.go new file mode 100644 index 000000000..c8be15cbe --- /dev/null +++ b/consul/structs/prepared_query_test.go @@ -0,0 +1,16 @@ +package structs + +import ( + "testing" +) + +func TestStructs_PreparedQuery_GetACLPrefix(t *testing.T) { + query := &PreparedQuery{ + Service: ServiceQuery{ + Service: "foo", + }, + } + if prefix := query.GetACLPrefix(); prefix != "foo" { + t.Fatalf("bad: %s", prefix) + } +} diff --git a/website/source/docs/agent/http/query.html.markdown b/website/source/docs/agent/http/query.html.markdown index a16e08bbd..f38dc1b71 100644 --- a/website/source/docs/agent/http/query.html.markdown +++ b/website/source/docs/agent/http/query.html.markdown @@ -29,8 +29,8 @@ The following endpoints are supported: Not all endpoints support blocking queries and all consistency modes, see details in the sections below. -The query endpoints support the use of ACL tokens. Prepared queries have some -special handling of ACL tokens that are highlighted in the sections below. +The query endpoints support the use of ACL Tokens. Prepared queries have some +special handling of ACL Tokens that are highlighted in the sections below. ### /v1/query @@ -44,6 +44,10 @@ its ID if it is created successfully. By default, the datacenter of the agent is queried; however, the dc can be provided using the "?dc=" query parameter. +If ACLs are enabled, then the client will need to supply a token with `prepared_query` +write priveliges sufficient to match the service name being queried and the `Name` +given to the query, if any. See also the note about the `Token` field below. + The create operation expects a JSON request body that defines the prepared query, like this example: @@ -51,6 +55,7 @@ like this example: { "Name": "my-query", "Session": "adf4238a-882b-9ddc-4a9d-5b6758e4159e", + "Token": "", "Service": { "Service": "redis", "Failover": { @@ -76,6 +81,24 @@ of using its ID. given session is invalidated. This is optional, and if not given the prepared query must be manually removed when no longer needed. + +`Token` is a captured ACL Token to use when the query is executed. This allows +queries to be executed by clients with lesser or even no ACL Token, so this +should be used with care. The token itself can only be seen by clients with a +management token. If the `Token` field is left blank, the client's ACL Token +will be used to determine if they have access to the service being queried. If +the client does not supply an ACL Token, the anonymous token will be used. + +Note that Consul version 0.6.3 and earlier would automatically capture the ACL +Token for use in the future when prepared queries were executed and would +execute with the same privileges as the definer of the prepared query. Older +queries wishing to obtain the new behavior will need to be updated to remove +their captured `Token` field. Capturing ACL Tokens is analogous to +[PostgreSQL’s SECURITY DEFINER](http://www.postgresql.org/docs/current/static/sql-createfunction.html) +attribute which can be set on functions. This change in effect moves Consul +from using `SECURITY DEFINER` by default to `SECURITY INVOKER` by default for +new Prepared Queries. + The set of fields inside the `Service` structure define the query's behavior. `Service` is the name of the service to query. This is required. @@ -130,18 +153,6 @@ a JSON body: } ``` -If ACLs are enabled, then the provided token will be used to check access to -the service being queried, and it will be saved along with the query for use -when the query is executed. This is key to allowing prepared queries to work -via the DNS interface, and it's important to note that prepared query IDs and -names become a read-only proxy for the token used to create the query. - -The query IDs that Consul generates are done in the same manner as ACL tokens, -so provide equal strength, but names may be more guessable and should be used -carefully with ACLs. Also, the token used to create the prepared query (or a -management token) is required to read the query back, so the ability to execute -a prepared query is not enough to get access to the actual token. - #### GET Method When using the GET method, Consul will provide a listing of all prepared queries. @@ -150,8 +161,10 @@ By default, the datacenter of the agent is queried; however, the dc can be provided using the "?dc=" query parameter. This endpoint supports blocking queries and all consistency modes. -Since this listing includes sensitive ACL tokens, this is a privileged endpoint -and always requires a management token to be supplied if ACLs are enabled. +If ACLs are enabled, then the client will only see prepared queries for which their +token has `prepared_query` read privileges. A management token will be able to see all +prepared queries. Tokens will be displayed as `` unless a management token is +used. This returns a JSON list of prepared queries, which looks like: @@ -161,7 +174,7 @@ This returns a JSON list of prepared queries, which looks like: "ID": "8f246b77-f3e1-ff88-5b48-8ec93abf3e05", "Name": "my-query", "Session": "adf4238a-882b-9ddc-4a9d-5b6758e4159e", - "Token": "", + "Token": "", "Service": { "Service": "redis", "Failover": { @@ -194,8 +207,9 @@ The `PUT` method allows an existing prepared query to be updated. By default, the datacenter of the agent is queried; however, the dc can be provided using the "?dc=" query parameter. -If ACLs are enabled, then the same token used to create the query (or a -management token) must be supplied. +If ACLs are enabled, then the client will need to supply a token with `prepared_query` +write priveliges sufficient to match the service name being queried and the `Name` +given to the query, if any. The body is the same as is used to create a prepared query, as described above. @@ -213,8 +227,10 @@ The returned response is the same as the list of prepared queries above, only with a single item present. If the query does not exist then a 404 status code will be returned. -If ACLs are enabled, then the same token used to create the query (or a -management token) must be supplied. +If ACLs are enabled, then the client will only see prepared queries for which their +token has `prepared_query` read privileges. A management token will be able to see all +prepared queries. Tokens will be displayed as `` unless a management token is +used. #### DELETE Method @@ -223,8 +239,9 @@ The `DELETE` method is used to delete a prepared query. By default, the datacenter of the agent is queried; however, the dc can be provided using the "?dc=" query parameter. -If ACLs are enabled, then the same token used to create the query (or a -management token) must be supplied. +If ACLs are enabled, then the client will need to supply a token with `prepared_query` +write priveliges sufficient to match the service name being queried and the `Name` +given to the query, if any. No body is required as part of this request. @@ -249,8 +266,9 @@ order each time the query is executed. An optional "?limit=" parameter can be used to limit the size of the list to the given number of nodes. This is applied after any sorting or shuffling. -The ACL token supplied when the prepared query was created will be used to -execute the request, so no ACL token needs to be supplied (it will be ignored). +If an ACL Token was bound to the query when it was defined then it will be used +when executing the request. Otherwise, the client's supplied ACL Token will be +used. No body is required as part of this request. diff --git a/website/source/docs/internals/acl.html.markdown b/website/source/docs/internals/acl.html.markdown index c73b7fe77..0a7acc58f 100644 --- a/website/source/docs/internals/acl.html.markdown +++ b/website/source/docs/internals/acl.html.markdown @@ -147,6 +147,19 @@ event "" { As always, the more secure way to handle user events is to explicitly grant access to each API token based on the events they should be able to fire. +### Blacklist mode and Prepared Queries + +Versions of Consul after 0.6.3 use a new [`prepared_query` ACL](#prepared_query_acls) +policy to control creating, updating, and deleting prepared queries. If you are +upgrading from a previous version of Consul, you will need to add this policy to +your ACL tokens if you want them to be able to manage prepared queries. + +It is not recommended to open up this policy for "write" by default, since clients +will be able to change any prepared query. Versions 0.6.3 and prior would enforce +that only the token that created a query (or a management token) could update it, +but this behavior has been removed in favor of the new +[`prepared_query` ACL](#prepared_query_acls). + ### Blacklist mode and Keyring Operations Consul 0.6 and later supports securing the encryption keyring operations using @@ -209,6 +222,10 @@ applied to any user event without a matching policy, is provided by an empty string. An event policy is one of "read", "write", or "deny". Currently, only the "write" level is enforced during event firing. Events can always be read. +Prepared query policies control access to create, update, and delete prepared +queries. Service policies are used when executing prepared queries. See +[below](#prepared_query_acls) for more details. + We make use of the [HashiCorp Configuration Language (HCL)](https://github.com/hashicorp/hcl/) to specify policy. This language is human readable and interoperable @@ -251,6 +268,11 @@ event "destroy-" { policy = "deny" } +# Default prepared queries to read-only. +prepared_query "" { + policy = "read" +} + # Read-only mode for the encryption keyring by default (list only) keyring = "read" ``` @@ -286,6 +308,11 @@ This is equivalent to the following JSON input: "policy": "deny" } }, + "prepared_query": { + "": { + "policy": "read" + } + }, "keyring": "read" } ``` @@ -325,3 +352,30 @@ making it appear as though the restricted services do not exist. Consul's DNS interface is also affected by restrictions to service registrations. If the token used by the agent does not have access to a given service, then the DNS interface will return no records when queried for it. + + +## Prepared Query ACLs + +In Consul 0.6, a new [Prepared Query](/docs/agent/http/query.html) feature was +added that allows complex service queries to be defined and then executed later +via an ID or name. + +Consul 0.6.3 and earlier would use the client's service policy to determine if +the client could register a prepared query (the client would need at least "read" +permission to the service). This was easy to use, but it did not allow for very +good control of the prepared query namespace. + +After 0.6.3, we introduced a new `prepared_query` ACL policy type that is used +to control the prepared query namespace. Having "write" access to a given prefix +allows a client to create, update, or delete only prepared queries for services +matching that prefix and with prepared query `Name` fields matching that prefix. + +It is not recommended to open up this policy for "write" by default, since clients +will be able to change any prepared query. Versions 0.6.3 and prior would enforce +that only the token that created a query (or a management token) could update it, +but this behavior has been removed in favor of this new ACL policy. + +Execution of prepared queries is governed by the `Token` captured in the query, +or by the client's ACL Token. See the +[`Token` field documentation](/docs/agent/http/query.html#token) for more +details. From a8ac27fa49e7c29b228ba94495136d0374c831df Mon Sep 17 00:00:00 2001 From: James Phillips Date: Tue, 23 Feb 2016 20:57:16 -0800 Subject: [PATCH 2/9] Refactors docs into a more complete state for prepared query ACLs. --- .../docs/agent/http/query.html.markdown | 14 +- .../source/docs/internals/acl.html.markdown | 133 ++++++++++++++---- 2 files changed, 113 insertions(+), 34 deletions(-) diff --git a/website/source/docs/agent/http/query.html.markdown b/website/source/docs/agent/http/query.html.markdown index f38dc1b71..12553180b 100644 --- a/website/source/docs/agent/http/query.html.markdown +++ b/website/source/docs/agent/http/query.html.markdown @@ -15,7 +15,7 @@ Prepared queries allow you to register a complex service query and then execute it later via its ID or name to get a set of healthy nodes that provide a given service. This is particularly useful in combination with Consul's [DNS Interface](/docs/agent/dns.html) as it allows for much richer queries than -would be possible given the limited interface DNS provides. +would be possible given the limited syntax DNS provides. The following endpoints are supported: @@ -30,7 +30,11 @@ Not all endpoints support blocking queries and all consistency modes, see details in the sections below. The query endpoints support the use of ACL Tokens. Prepared queries have some -special handling of ACL Tokens that are highlighted in the sections below. +special handling of ACL Tokens that are called out where applicable with the +details of each endpoint. + +See the [Prepared Query ACLs](/docs/internals/acl.html#prepared_query_acls) +internals guide for more details about how prepared query policies work. ### /v1/query @@ -45,7 +49,7 @@ By default, the datacenter of the agent is queried; however, the dc can be provided using the "?dc=" query parameter. If ACLs are enabled, then the client will need to supply a token with `prepared_query` -write priveliges sufficient to match the service name being queried and the `Name` +write privileges sufficient to match the service name being queried and the `Name` given to the query, if any. See also the note about the `Token` field below. The create operation expects a JSON request body that defines the prepared query, @@ -208,7 +212,7 @@ By default, the datacenter of the agent is queried; however, the dc can be provided using the "?dc=" query parameter. If ACLs are enabled, then the client will need to supply a token with `prepared_query` -write priveliges sufficient to match the service name being queried and the `Name` +write privileges sufficient to match the service name being queried and the `Name` given to the query, if any. The body is the same as is used to create a prepared query, as described above. @@ -240,7 +244,7 @@ By default, the datacenter of the agent is queried; however, the dc can be provided using the "?dc=" query parameter. If ACLs are enabled, then the client will need to supply a token with `prepared_query` -write priveliges sufficient to match the service name being queried and the `Name` +write privileges sufficient to match the service name being queried and the `Name` given to the query, if any. No body is required as part of this request. diff --git a/website/source/docs/internals/acl.html.markdown b/website/source/docs/internals/acl.html.markdown index 0a7acc58f..d216be304 100644 --- a/website/source/docs/internals/acl.html.markdown +++ b/website/source/docs/internals/acl.html.markdown @@ -149,16 +149,8 @@ access to each API token based on the events they should be able to fire. ### Blacklist mode and Prepared Queries -Versions of Consul after 0.6.3 use a new [`prepared_query` ACL](#prepared_query_acls) -policy to control creating, updating, and deleting prepared queries. If you are -upgrading from a previous version of Consul, you will need to add this policy to -your ACL tokens if you want them to be able to manage prepared queries. - -It is not recommended to open up this policy for "write" by default, since clients -will be able to change any prepared query. Versions 0.6.3 and prior would enforce -that only the token that created a query (or a management token) could update it, -but this behavior has been removed in favor of the new -[`prepared_query` ACL](#prepared_query_acls). +After Consul 0.6.3, significant changes were made to ACLs for prepared queries, +incuding a new `prepared_query` ACL policy. See [Prepared Query ACLs](#prepared_query_acls) below for more details. ### Blacklist mode and Keyring Operations @@ -356,26 +348,109 @@ service, then the DNS interface will return no records when queried for it. ## Prepared Query ACLs -In Consul 0.6, a new [Prepared Query](/docs/agent/http/query.html) feature was -added that allows complex service queries to be defined and then executed later -via an ID or name. +Prepared queries have a more complicated relationship with ACLs than most other +Consul resources because they have a lifecycle related to creating, reading, +updating, and deleting queries, and then a separate lifecycle related to +executing queries. -Consul 0.6.3 and earlier would use the client's service policy to determine if -the client could register a prepared query (the client would need at least "read" -permission to the service). This was easy to use, but it did not allow for very -good control of the prepared query namespace. +As we've gotten feedback from Consul users, we've evolved how prepared queries +use ACLs. In this section we first cover the current implementation, and then we +follow that with details about what's changed between specific versions of Consul. -After 0.6.3, we introduced a new `prepared_query` ACL policy type that is used -to control the prepared query namespace. Having "write" access to a given prefix -allows a client to create, update, or delete only prepared queries for services -matching that prefix and with prepared query `Name` fields matching that prefix. +#### Managing Prepared Queries -It is not recommended to open up this policy for "write" by default, since clients -will be able to change any prepared query. Versions 0.6.3 and prior would enforce -that only the token that created a query (or a management token) could update it, -but this behavior has been removed in favor of this new ACL policy. +Managing prepared queries includes creating, reading, updating, and deleting +queries. There are a few variations, each of which uses ACLs in one of two +ways: open, protected by unguessable IDs or closed, managed by ACL policies. +These variations are covered here, with examples: -Execution of prepared queries is governed by the `Token` captured in the query, -or by the client's ACL Token. See the -[`Token` field documentation](/docs/agent/http/query.html#token) for more -details. +* Static queries with no `Name` defined are not controlled by any ACL policies. + These types of queries are meant to be ephemeral and not shared to untrusted + clients, and they are only reachable if the prepared query ID is known. Since + these IDs are generated using the same random ID scheme as ACL Tokens, it is + infeasible to guess them. When listing all prepared queries, only a management + token will be able to see these types, though clients can read instances for + which they have an ID. An example use for this type is a query built by a + startup script, tied to a session, and written to a configuration file for a + process to use via DNS. + +* Static queries with a `Name` defined are controlled by the + [`prepared_query`](/docs/internals/acl.html#prepared_query_acls) ACL policy. + Clients are required to have an ACL token with a prefix sufficient to cover + the name they are trying to manage, with a longest prefix match providing a + way to define more specific policies. Clients can list or read queries for + which they have "read" access based on their prefix, and similar they can + update any queries for which they have "write" access. An example use for + this type is a query with a well-known name (eg. prod-master-customer-db) + that is used and known by many clients to provide geo-failover behavior for + a database. + +#### Executing Pepared Queries + +When prepared queries are executed via DNS lookups or HTTP requests, the +management ACL isn't used in any way. Instead, ACLs are applied to the service +being queried, similar to how other service lookups work, except that prepared +queries have some special behavior related to where the ACL Token comes +from: + +* If an ACL Token was captured when the prepared query was defined, it will be + used to perform the service lookup. This allows queries to be executed by + clients with lesser or even no ACL Token, so this should be used with care. + +* If no ACL Token was captured, then the client's ACL Token will be used to + perform the service lookup. + +* If no ACL Token was captured, and the client has no ACL Token, then the + anonymous token will be used to perform the service lookup. + +Capturing ACL Tokens is analogous to +[PostgreSQL’s](http://www.postgresql.org/docs/current/static/sql-createfunction.html) +`SECURITY DEFINER` attribute which can be set on functions, and using the client's ACL +Token is similar to the complementary `SECURITY INVOKER` attribute. + + +#### ACL Implementation Changes + +Prepared queries were originally introduced in Consul 0.6.0, and ACL behavior remained +unchanged through version 0.6.3, but was then changed to allow better management of the +prepared query namespace. + +These differences are outlined in the table below: + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
OperationVersion <= 0.6.3 Version > 0.6.3
Create static query without `Name`The ACL Token used to create the prepared query is checked to make sure it can access the service being queried. This token is captured as the `Token` to use when executing the prepared query.No ACL policies are used as long as no `Name` is defined. No `Token` is captured by default unless specifically supplied by the client when creating the query.
Create static query with `Name`The ACL Token used to create the prepared query is checked to make sure it can access the service being queried. This token is captured as the `Token` to use when executing the prepared query.The client token's `prepared_query` ACL policy is used to determine if the client is allowed to register a query for the given `Name`. No `Token` is captured by default unless specifically supplied by the client when creating the query.
Manage static query without `Name`The ACL Token used to create the query, or a management token must be supplied in order to perform these operations.Any client with the ID of the query can perform these operations.
Manage static query with a `Name`The ACL token used to create the query, or a management token must be supplied in order to perform these operations.Similar to create, the client token's `prepared_query` ACL policy is used to determine if these operations are allowed.
List queriesA management token is required to list any queries.The client token's `prepared_query` ACL policy is used to determine which queries they can see. Only management tokens can see prepared queries without `Name`.
Execute querySince a `Token` is always captured when a query is created, that is used to check access to the service being queried. Any token supplied by the client is ignored.The captured token, client's token, or anonymous token is used to filter the results, as described above.
From 54f0b7bbb657c8fe5b25d626c1d1864b50a0573f Mon Sep 17 00:00:00 2001 From: James Phillips Date: Wed, 24 Feb 2016 01:26:16 -0800 Subject: [PATCH 3/9] 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 a22752999..69c4f0a1b 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 f795997ba..4fe4bb725 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 43070f547..1e0d489d7 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 29aa0c55a..e17773e2c 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 ba406a23d..620b816ee 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 d88c2f073..7d57e49a1 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 c8be15cbe..14aaedcc2 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) } } From 0ea990f3d2d8b853fd9be01b76d2bcafbd7ee96a Mon Sep 17 00:00:00 2001 From: James Phillips Date: Wed, 24 Feb 2016 01:33:10 -0800 Subject: [PATCH 4/9] Adds an upgrade note about the new ACL behavior. --- website/source/docs/upgrade-specific.html.markdown | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/website/source/docs/upgrade-specific.html.markdown b/website/source/docs/upgrade-specific.html.markdown index 14b27e190..51088f9c2 100644 --- a/website/source/docs/upgrade-specific.html.markdown +++ b/website/source/docs/upgrade-specific.html.markdown @@ -14,6 +14,20 @@ details provided for their upgrades as a result of new features or changed behavior. This page is used to document those details separately from the standard upgrade flow. +## Consul 0.6.4 + +Consul 0.6.4 made some substantial changes to how ACLs work with prepared +queries. Existing queries will execute with no changes, but there are important +differences to understand about how prepared queries are managed before you +upgrade. In particular, prepared queries with no `Name` defined will no longer +require any ACL to manage them, and prepared queries with a `Name` defined are +now governed by a new `prepared_query` ACL policy that will need to be configured +after the upgrade. + +See the [Prepared Query ACLs](/docs/internals/acl.html#prepared_query_acls) +internals guide for more details about the new behavior and how it compares to +previous versions of Consul. + ## Consul 0.6 Consul version 0.6 is a very large release with many enhancements and From 1c7ee582f99c970d8e6189fece19341d15044522 Mon Sep 17 00:00:00 2001 From: James Phillips Date: Wed, 24 Feb 2016 16:17:20 -0800 Subject: [PATCH 5/9] Renames a unit test. --- consul/structs/prepared_query_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/consul/structs/prepared_query_test.go b/consul/structs/prepared_query_test.go index 14aaedcc2..4de25d2a3 100644 --- a/consul/structs/prepared_query_test.go +++ b/consul/structs/prepared_query_test.go @@ -4,7 +4,7 @@ import ( "testing" ) -func TestStructs_PreparedQuery_GetACLInfo(t *testing.T) { +func TestStructs_PreparedQuery_GetACLPrefix(t *testing.T) { ephemeral := &PreparedQuery{} if prefix := ephemeral.GetACLPrefix(); prefix != nil { t.Fatalf("bad: %#v", prefix) From 3b91618d7d85822595fca15429e300ce32bc1a3b Mon Sep 17 00:00:00 2001 From: James Phillips Date: Wed, 24 Feb 2016 16:26:43 -0800 Subject: [PATCH 6/9] Changes to more idiomatic "ok" pattern for prefix getter. --- consul/acl.go | 6 +++--- consul/prepared_query_endpoint.go | 10 +++++----- consul/structs/prepared_query.go | 9 +++++---- consul/structs/prepared_query_test.go | 6 +++--- 4 files changed, 16 insertions(+), 15 deletions(-) diff --git a/consul/acl.go b/consul/acl.go index 4fe4bb725..de305d04f 100644 --- a/consul/acl.go +++ b/consul/acl.go @@ -389,9 +389,9 @@ func (f *aclFilter) filterPreparedQueries(queries *structs.PreparedQueries) { for _, query := range *queries { // 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) { + // token, otherwise see what the policy says. + prefix, ok := query.GetACLPrefix() + if !ok || !f.acl.PreparedQueryRead(prefix) { f.logger.Printf("[DEBUG] consul: dropping prepared query %q from result due to ACLs", query.ID) continue } diff --git a/consul/prepared_query_endpoint.go b/consul/prepared_query_endpoint.go index e17773e2c..7fc9ac86f 100644 --- a/consul/prepared_query_endpoint.go +++ b/consul/prepared_query_endpoint.go @@ -65,8 +65,8 @@ func (p *PreparedQuery) Apply(args *structs.PreparedQueryRequest, reply *string) // 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) { + if prefix, ok := args.Query.GetACLPrefix(); ok { + 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 } @@ -85,8 +85,8 @@ func (p *PreparedQuery) Apply(args *structs.PreparedQueryRequest, reply *string) return fmt.Errorf("Cannot modify non-existent prepared query: '%s'", args.Query.ID) } - if prefix := query.GetACLPrefix(); prefix != nil { - if acl != nil && !acl.PreparedQueryWrite(*prefix) { + if prefix, ok := query.GetACLPrefix(); ok { + 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 } @@ -216,7 +216,7 @@ func (p *PreparedQuery) Get(args *structs.PreparedQuerySpecificRequest, // always allowed to see it if they have the ID. reply.Index = index reply.Queries = structs.PreparedQueries{query} - if prefix := query.GetACLPrefix(); prefix == nil { + if _, ok := query.GetACLPrefix(); !ok { return nil } diff --git a/consul/structs/prepared_query.go b/consul/structs/prepared_query.go index 7d57e49a1..6058ca50f 100644 --- a/consul/structs/prepared_query.go +++ b/consul/structs/prepared_query.go @@ -73,13 +73,14 @@ type PreparedQuery struct { } // 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 { +// this query, and whether the prefix applies to this query. You always need to +// check the ok value before using the prefix. +func (pq *PreparedQuery) GetACLPrefix() (string, bool) { if pq.Name != "" { - return &pq.Name + return pq.Name, true } - return nil + return "", false } type PreparedQueries []*PreparedQuery diff --git a/consul/structs/prepared_query_test.go b/consul/structs/prepared_query_test.go index 4de25d2a3..b80fff897 100644 --- a/consul/structs/prepared_query_test.go +++ b/consul/structs/prepared_query_test.go @@ -6,12 +6,12 @@ import ( func TestStructs_PreparedQuery_GetACLPrefix(t *testing.T) { ephemeral := &PreparedQuery{} - if prefix := ephemeral.GetACLPrefix(); prefix != nil { - t.Fatalf("bad: %#v", prefix) + if prefix, ok := ephemeral.GetACLPrefix(); ok { + t.Fatalf("bad: %s", prefix) } named := &PreparedQuery{Name: "hello"} - if prefix := named.GetACLPrefix(); prefix == nil || *prefix != "hello" { + if prefix, ok := named.GetACLPrefix(); !ok || prefix != "hello" { t.Fatalf("bad: %#v", prefix) } } From 2f7eac8b86885304f12c7c80fdd7b510ae8ce5b6 Mon Sep 17 00:00:00 2001 From: James Phillips Date: Wed, 24 Feb 2016 16:57:55 -0800 Subject: [PATCH 7/9] Renames "prepared_query" ACL policy to "query". --- acl/policy.go | 4 ++-- acl/policy_test.go | 10 +++++----- consul/prepared_query_endpoint_test.go | 6 +++--- website/source/docs/agent/http/query.html.markdown | 10 +++++----- website/source/docs/internals/acl.html.markdown | 14 +++++++------- 5 files changed, 22 insertions(+), 22 deletions(-) diff --git a/acl/policy.go b/acl/policy.go index fca018a65..a0e56da42 100644 --- a/acl/policy.go +++ b/acl/policy.go @@ -19,7 +19,7 @@ type Policy struct { Keys []*KeyPolicy `hcl:"key,expand"` Services []*ServicePolicy `hcl:"service,expand"` Events []*EventPolicy `hcl:"event,expand"` - PreparedQueries []*PreparedQueryPolicy `hcl:"prepared_query,expand"` + PreparedQueries []*PreparedQueryPolicy `hcl:"query,expand"` Keyring string `hcl:"keyring"` } @@ -116,7 +116,7 @@ func Parse(rules string) (*Policy, error) { // Validate the prepared query policies for _, pq := range p.PreparedQueries { if !isPolicyValid(pq.Policy) { - return nil, fmt.Errorf("Invalid prepared_query policy: %#v", pq) + return nil, fmt.Errorf("Invalid query policy: %#v", pq) } } diff --git a/acl/policy_test.go b/acl/policy_test.go index ae4d76a16..c59a4e014 100644 --- a/acl/policy_test.go +++ b/acl/policy_test.go @@ -35,13 +35,13 @@ event "foo" { event "bar" { policy = "deny" } -prepared_query "" { +query "" { policy = "read" } -prepared_query "foo" { +query "foo" { policy = "write" } -prepared_query "bar" { +query "bar" { policy = "deny" } keyring = "deny" @@ -151,7 +151,7 @@ func TestACLPolicy_Parse_JSON(t *testing.T) { "policy": "deny" } }, - "prepared_query": { + "query": { "": { "policy": "read" }, @@ -257,7 +257,7 @@ func TestACLPolicy_Bad_Policy(t *testing.T) { `key "" { policy = "nope" }`, `service "" { policy = "nope" }`, `event "" { policy = "nope" }`, - `prepared_query "" { policy = "nope" }`, + `query "" { policy = "nope" }`, `keyring = "nope"`, } for _, c := range cases { diff --git a/consul/prepared_query_endpoint_test.go b/consul/prepared_query_endpoint_test.go index 620b816ee..16c37cb54 100644 --- a/consul/prepared_query_endpoint_test.go +++ b/consul/prepared_query_endpoint_test.go @@ -194,7 +194,7 @@ func TestPreparedQuery_Apply_ACLDeny(t *testing.T) { var token string { var rules = ` - prepared_query "redis" { + query "redis" { policy = "write" } ` @@ -585,7 +585,7 @@ func TestPreparedQuery_Get(t *testing.T) { var token string { var rules = ` - prepared_query "redis" { + query "redis" { policy = "write" } ` @@ -762,7 +762,7 @@ func TestPreparedQuery_List(t *testing.T) { var token string { var rules = ` - prepared_query "redis" { + query "redis" { policy = "write" } ` diff --git a/website/source/docs/agent/http/query.html.markdown b/website/source/docs/agent/http/query.html.markdown index 12553180b..3140e8f97 100644 --- a/website/source/docs/agent/http/query.html.markdown +++ b/website/source/docs/agent/http/query.html.markdown @@ -48,7 +48,7 @@ its ID if it is created successfully. By default, the datacenter of the agent is queried; however, the dc can be provided using the "?dc=" query parameter. -If ACLs are enabled, then the client will need to supply a token with `prepared_query` +If ACLs are enabled, then the client will need to supply a token with `query` write privileges sufficient to match the service name being queried and the `Name` given to the query, if any. See also the note about the `Token` field below. @@ -166,7 +166,7 @@ provided using the "?dc=" query parameter. This endpoint supports blocking queries and all consistency modes. If ACLs are enabled, then the client will only see prepared queries for which their -token has `prepared_query` read privileges. A management token will be able to see all +token has `query` read privileges. A management token will be able to see all prepared queries. Tokens will be displayed as `` unless a management token is used. @@ -211,7 +211,7 @@ The `PUT` method allows an existing prepared query to be updated. By default, the datacenter of the agent is queried; however, the dc can be provided using the "?dc=" query parameter. -If ACLs are enabled, then the client will need to supply a token with `prepared_query` +If ACLs are enabled, then the client will need to supply a token with `query` write privileges sufficient to match the service name being queried and the `Name` given to the query, if any. @@ -232,7 +232,7 @@ only with a single item present. If the query does not exist then a 404 status code will be returned. If ACLs are enabled, then the client will only see prepared queries for which their -token has `prepared_query` read privileges. A management token will be able to see all +token has `query` read privileges. A management token will be able to see all prepared queries. Tokens will be displayed as `` unless a management token is used. @@ -243,7 +243,7 @@ The `DELETE` method is used to delete a prepared query. By default, the datacenter of the agent is queried; however, the dc can be provided using the "?dc=" query parameter. -If ACLs are enabled, then the client will need to supply a token with `prepared_query` +If ACLs are enabled, then the client will need to supply a token with `query` write privileges sufficient to match the service name being queried and the `Name` given to the query, if any. diff --git a/website/source/docs/internals/acl.html.markdown b/website/source/docs/internals/acl.html.markdown index d216be304..0bac34910 100644 --- a/website/source/docs/internals/acl.html.markdown +++ b/website/source/docs/internals/acl.html.markdown @@ -150,7 +150,7 @@ access to each API token based on the events they should be able to fire. ### Blacklist mode and Prepared Queries After Consul 0.6.3, significant changes were made to ACLs for prepared queries, -incuding a new `prepared_query` ACL policy. See [Prepared Query ACLs](#prepared_query_acls) below for more details. +incuding a new `query` ACL policy. See [Prepared Query ACLs](#prepared_query_acls) below for more details. ### Blacklist mode and Keyring Operations @@ -261,7 +261,7 @@ event "destroy-" { } # Default prepared queries to read-only. -prepared_query "" { +query "" { policy = "read" } @@ -300,7 +300,7 @@ This is equivalent to the following JSON input: "policy": "deny" } }, - "prepared_query": { + "query": { "": { "policy": "read" } @@ -375,7 +375,7 @@ These variations are covered here, with examples: process to use via DNS. * Static queries with a `Name` defined are controlled by the - [`prepared_query`](/docs/internals/acl.html#prepared_query_acls) ACL policy. + [`query`](/docs/internals/acl.html#prepared_query_acls) ACL policy. Clients are required to have an ACL token with a prefix sufficient to cover the name they are trying to manage, with a longest prefix match providing a way to define more specific policies. Clients can list or read queries for @@ -431,7 +431,7 @@ These differences are outlined in the table below: Create static query with `Name` The ACL Token used to create the prepared query is checked to make sure it can access the service being queried. This token is captured as the `Token` to use when executing the prepared query. - The client token's `prepared_query` ACL policy is used to determine if the client is allowed to register a query for the given `Name`. No `Token` is captured by default unless specifically supplied by the client when creating the query. + The client token's `query` ACL policy is used to determine if the client is allowed to register a query for the given `Name`. No `Token` is captured by default unless specifically supplied by the client when creating the query. Manage static query without `Name` @@ -441,12 +441,12 @@ These differences are outlined in the table below: Manage static query with a `Name` The ACL token used to create the query, or a management token must be supplied in order to perform these operations. - Similar to create, the client token's `prepared_query` ACL policy is used to determine if these operations are allowed. + Similar to create, the client token's `query` ACL policy is used to determine if these operations are allowed. List queries A management token is required to list any queries. - The client token's `prepared_query` ACL policy is used to determine which queries they can see. Only management tokens can see prepared queries without `Name`. + The client token's `query` ACL policy is used to determine which queries they can see. Only management tokens can see prepared queries without `Name`. Execute query From c75256ac8bfabfb7f374a285446d2c13d1358564 Mon Sep 17 00:00:00 2001 From: James Phillips Date: Wed, 24 Feb 2016 17:23:09 -0800 Subject: [PATCH 8/9] Adds a check for users re-submitting the redacted token. --- consul/prepared_query_endpoint.go | 8 +++++++- consul/prepared_query_endpoint_test.go | 11 +++++++++++ website/source/docs/agent/http/query.html.markdown | 8 ++++---- 3 files changed, 22 insertions(+), 5 deletions(-) diff --git a/consul/prepared_query_endpoint.go b/consul/prepared_query_endpoint.go index 7fc9ac86f..c3f027136 100644 --- a/consul/prepared_query_endpoint.go +++ b/consul/prepared_query_endpoint.go @@ -134,7 +134,13 @@ func parseQuery(query *structs.PreparedQuery) error { // transaction. Otherwise, people could "steal" queries that they don't // have proper ACL rights to change. // - Session is optional and checked for integrity during the transaction. - // - Token is checked when a query is executed. + + // Token is checked when the query is executed, but we do make sure the + // user hasn't accidentally pasted-in the special redacted token name, + // which if we allowed in would be super hard to debug and understand. + if query.Token == redactedToken { + return fmt.Errorf("Bad Token '%s', it looks like a query definition with a redacted token was submitted", query.Token) + } // Parse the service query sub-structure. if err := parseService(&query.Service); err != nil { diff --git a/consul/prepared_query_endpoint_test.go b/consul/prepared_query_endpoint_test.go index 16c37cb54..a6c37ce88 100644 --- a/consul/prepared_query_endpoint_test.go +++ b/consul/prepared_query_endpoint_test.go @@ -539,6 +539,17 @@ func TestPreparedQuery_parseQuery(t *testing.T) { t.Fatalf("err: %v", err) } + query.Token = redactedToken + err = parseQuery(query) + if err == nil || !strings.Contains(err.Error(), "Bad Token") { + t.Fatalf("bad: %v", err) + } + + query.Token = "adf4238a-882b-9ddc-4a9d-5b6758e4159e" + if err := parseQuery(query); err != nil { + t.Fatalf("err: %v", err) + } + query.Service.Failover.NearestN = -1 err = parseQuery(query) if err == nil || !strings.Contains(err.Error(), "Bad NearestN") { diff --git a/website/source/docs/agent/http/query.html.markdown b/website/source/docs/agent/http/query.html.markdown index 3140e8f97..07ed2d9fe 100644 --- a/website/source/docs/agent/http/query.html.markdown +++ b/website/source/docs/agent/http/query.html.markdown @@ -167,8 +167,8 @@ queries and all consistency modes. If ACLs are enabled, then the client will only see prepared queries for which their token has `query` read privileges. A management token will be able to see all -prepared queries. Tokens will be displayed as `` unless a management token is -used. +prepared queries. Tokens will be redacted and displayed as `` unless a +management token is used. This returns a JSON list of prepared queries, which looks like: @@ -233,8 +233,8 @@ status code will be returned. If ACLs are enabled, then the client will only see prepared queries for which their token has `query` read privileges. A management token will be able to see all -prepared queries. Tokens will be displayed as `` unless a management token is -used. +prepared queries. Tokens will be redacted and displayed as `` unless a +management token is used. #### DELETE Method From 72f7c08a0aab5088ae958bfd4a2954ce5eab7f37 Mon Sep 17 00:00:00 2001 From: James Phillips Date: Wed, 24 Feb 2016 18:05:58 -0800 Subject: [PATCH 9/9] Cleans up the documents. --- .../docs/agent/http/query.html.markdown | 42 +++++++++---------- .../source/docs/internals/acl.html.markdown | 23 +++++----- 2 files changed, 31 insertions(+), 34 deletions(-) diff --git a/website/source/docs/agent/http/query.html.markdown b/website/source/docs/agent/http/query.html.markdown index 07ed2d9fe..e3cb32602 100644 --- a/website/source/docs/agent/http/query.html.markdown +++ b/website/source/docs/agent/http/query.html.markdown @@ -15,7 +15,7 @@ Prepared queries allow you to register a complex service query and then execute it later via its ID or name to get a set of healthy nodes that provide a given service. This is particularly useful in combination with Consul's [DNS Interface](/docs/agent/dns.html) as it allows for much richer queries than -would be possible given the limited syntax DNS provides. +would be possible given the limited entry points exposed by DNS. The following endpoints are supported: @@ -45,12 +45,11 @@ The general query endpoint supports the `POST` and `GET` methods. When using the `POST` method, Consul will create a new prepared query and return its ID if it is created successfully. -By default, the datacenter of the agent is queried; however, the dc can be +By default, the datacenter of the agent is queried; however, the `dc` can be provided using the "?dc=" query parameter. -If ACLs are enabled, then the client will need to supply a token with `query` -write privileges sufficient to match the service name being queried and the `Name` -given to the query, if any. See also the note about the `Token` field below. +If ACLs are enabled, the client will need to supply an ACL Token with `query` +write privileges for the `Name` of the query being created. The create operation expects a JSON request body that defines the prepared query, like this example: @@ -86,12 +85,13 @@ given session is invalidated. This is optional, and if not given the prepared query must be manually removed when no longer needed. -`Token` is a captured ACL Token to use when the query is executed. This allows -queries to be executed by clients with lesser or even no ACL Token, so this -should be used with care. The token itself can only be seen by clients with a -management token. If the `Token` field is left blank, the client's ACL Token -will be used to determine if they have access to the service being queried. If -the client does not supply an ACL Token, the anonymous token will be used. +`Token`, if specified, is a captured ACL Token that is reused as the ACL Token +every time the query is executed. This allows queries to be executed by clients +with lesser or even no ACL Token, so this should be used with care. The token +itself can only be seen by clients with a management token. If the `Token` +field is left blank or omitted, the client's ACL Token will be used to determine +if they have access to the service being queried. If the client does not supply +an ACL Token, the anonymous token will be used. Note that Consul version 0.6.3 and earlier would automatically capture the ACL Token for use in the future when prepared queries were executed and would @@ -161,7 +161,7 @@ a JSON body: When using the GET method, Consul will provide a listing of all prepared queries. -By default, the datacenter of the agent is queried; however, the dc can be +By default, the datacenter of the agent is queried; however, the `dc` can be provided using the "?dc=" query parameter. This endpoint supports blocking queries and all consistency modes. @@ -208,12 +208,11 @@ The query-specific endpoint supports the `GET`, `PUT`, and `DELETE` methods. The The `PUT` method allows an existing prepared query to be updated. -By default, the datacenter of the agent is queried; however, the dc can be +By default, the datacenter of the agent is queried; however, the `dc` can be provided using the "?dc=" query parameter. -If ACLs are enabled, then the client will need to supply a token with `query` -write privileges sufficient to match the service name being queried and the `Name` -given to the query, if any. +If ACLs are enabled, the client will need to supply an ACL Token with `query` +write privileges for the `Name` of the query being updated. The body is the same as is used to create a prepared query, as described above. @@ -223,7 +222,7 @@ If the API call succeeds, a 200 status code is returned. The `GET` method allows an existing prepared query to be fetched. -By default, the datacenter of the agent is queried; however, the dc can be +By default, the datacenter of the agent is queried; however, the `dc` can be provided using the "?dc=" query parameter. This endpoint supports blocking queries and all consistency modes. @@ -240,12 +239,11 @@ management token is used. The `DELETE` method is used to delete a prepared query. -By default, the datacenter of the agent is queried; however, the dc can be +By default, the datacenter of the agent is queried; however, the `dc` can be provided using the "?dc=" query parameter. -If ACLs are enabled, then the client will need to supply a token with `query` -write privileges sufficient to match the service name being queried and the `Name` -given to the query, if any. +If ACLs are enabled, the client will need to supply an ACL Token with `query` +write privileges for the `Name` of the query being deleted. No body is required as part of this request. @@ -257,7 +255,7 @@ The query execute endpoint supports only the `GET` method and is used to execute a prepared query. The \ argument is the ID or name of an existing prepared query. -By default, the datacenter of the agent is queried; however, the dc can be +By default, the datacenter of the agent is queried; however, the `dc` can be provided using the "?dc=" query parameter. This endpoint does not support blocking queries, but it does support all consistency modes. diff --git a/website/source/docs/internals/acl.html.markdown b/website/source/docs/internals/acl.html.markdown index 0bac34910..6d9888684 100644 --- a/website/source/docs/internals/acl.html.markdown +++ b/website/source/docs/internals/acl.html.markdown @@ -348,11 +348,6 @@ service, then the DNS interface will return no records when queried for it. ## Prepared Query ACLs -Prepared queries have a more complicated relationship with ACLs than most other -Consul resources because they have a lifecycle related to creating, reading, -updating, and deleting queries, and then a separate lifecycle related to -executing queries. - As we've gotten feedback from Consul users, we've evolved how prepared queries use ACLs. In this section we first cover the current implementation, and then we follow that with details about what's changed between specific versions of Consul. @@ -381,17 +376,16 @@ These variations are covered here, with examples: way to define more specific policies. Clients can list or read queries for which they have "read" access based on their prefix, and similar they can update any queries for which they have "write" access. An example use for - this type is a query with a well-known name (eg. prod-master-customer-db) + this type is a query with a well-known name (eg. `prod-master-customer-db`) that is used and known by many clients to provide geo-failover behavior for a database. #### Executing Pepared Queries -When prepared queries are executed via DNS lookups or HTTP requests, the -management ACL isn't used in any way. Instead, ACLs are applied to the service -being queried, similar to how other service lookups work, except that prepared -queries have some special behavior related to where the ACL Token comes -from: +When prepared queries are executed via DNS lookups or HTTP requests, the ACL +checks are run against the service being queried, similar to how ACLs work with +other service lookups. There are several ways the ACL token is selected for this +check: * If an ACL Token was captured when the prepared query was defined, it will be used to perform the service lookup. This allows queries to be executed by @@ -400,9 +394,14 @@ from: * If no ACL Token was captured, then the client's ACL Token will be used to perform the service lookup. -* If no ACL Token was captured, and the client has no ACL Token, then the +* If no ACL Token was captured and the client has no ACL Token, then the anonymous token will be used to perform the service lookup. +In the common case, the ACL Token of the invoker is used +to test the ability to look up a service. If a `Token` was specified when the +prepared query was created, the behavior changes and now the captured +ACL Token set by the definer of the query is used when lookup up a service. + Capturing ACL Tokens is analogous to [PostgreSQL’s](http://www.postgresql.org/docs/current/static/sql-createfunction.html) `SECURITY DEFINER` attribute which can be set on functions, and using the client's ACL