From 8ae9e17dff67cb47468f33837b7bd69098104556 Mon Sep 17 00:00:00 2001 From: James Phillips Date: Thu, 1 Dec 2016 19:14:08 -0800 Subject: [PATCH 1/9] Adds an opt-in for new ACL policies and features coming in Consul 0.8. --- command/agent/config.go | 18 +++++++++++++----- command/agent/config_test.go | 17 +++++++++++++++++ .../source/docs/agent/options.html.markdown | 7 +++++++ 3 files changed, 37 insertions(+), 5 deletions(-) diff --git a/command/agent/config.go b/command/agent/config.go index 8c17f91182..fc80b4bf1a 100644 --- a/command/agent/config.go +++ b/command/agent/config.go @@ -525,6 +525,10 @@ type Config struct { // other than the ACLDatacenter. ACLReplicationToken string `mapstructure:"acl_replication_token" json:"-"` + // ACLEnforceVersion8 is used to gate a set of ACL policy features that + // are opt-in prior to Consul 0.8 and opt-out in Consul 0.8 and later. + ACLEnforceVersion8 *bool `mapstructure:"acl_enforce_version_8"` + // Watches are used to monitor various endpoints and to invoke a // handler to act appropriately. These are managed entirely in the // agent layer using the standard APIs. @@ -705,11 +709,12 @@ func DefaultConfig() *Config { SyncCoordinateRateTarget: 64.0, // updates / second SyncCoordinateIntervalMin: 15 * time.Second, - ACLTTL: 30 * time.Second, - ACLDownPolicy: "extend-cache", - ACLDefaultPolicy: "allow", - RetryInterval: 30 * time.Second, - RetryIntervalWan: 30 * time.Second, + ACLTTL: 30 * time.Second, + ACLDownPolicy: "extend-cache", + ACLDefaultPolicy: "allow", + ACLEnforceVersion8: Bool(false), + RetryInterval: 30 * time.Second, + RetryIntervalWan: 30 * time.Second, } } @@ -1480,6 +1485,9 @@ func MergeConfig(a, b *Config) *Config { if b.ACLReplicationToken != "" { result.ACLReplicationToken = b.ACLReplicationToken } + if b.ACLEnforceVersion8 != nil { + result.ACLEnforceVersion8 = b.ACLEnforceVersion8 + } if len(b.Watches) != 0 { result.Watches = append(result.Watches, b.Watches...) } diff --git a/command/agent/config_test.go b/command/agent/config_test.go index e2a04ae98a..48a615f974 100644 --- a/command/agent/config_test.go +++ b/command/agent/config_test.go @@ -674,6 +674,22 @@ func TestDecodeConfig(t *testing.T) { t.Fatalf("bad: %#v", config) } + // ACL flag for Consul version 0.8 features (broken out since we will + // eventually remove this). We first verify this is opt-out. + config = DefaultConfig() + if *config.ACLEnforceVersion8 != false { + t.Fatalf("bad: %#v", config) + } + + input = `{"acl_enforce_version_8": true}` + config, err = DecodeConfig(bytes.NewReader([]byte(input))) + if err != nil { + t.Fatalf("err: %s", err) + } + if *config.ACLEnforceVersion8 != true { + t.Fatalf("bad: %#v", config) + } + // Watches input = `{"watches": [{"type":"keyprefix", "prefix":"foo/", "handler":"foobar"}]}` config, err = DecodeConfig(bytes.NewReader([]byte(input))) @@ -1552,6 +1568,7 @@ func TestMergeConfig(t *testing.T) { ACLDownPolicy: "deny", ACLDefaultPolicy: "deny", ACLReplicationToken: "8765309", + ACLEnforceVersion8: Bool(true), Watches: []map[string]interface{}{ map[string]interface{}{ "type": "keyprefix", diff --git a/website/source/docs/agent/options.html.markdown b/website/source/docs/agent/options.html.markdown index 49253e30b7..ea6fb252c6 100644 --- a/website/source/docs/agent/options.html.markdown +++ b/website/source/docs/agent/options.html.markdown @@ -377,6 +377,13 @@ Consul will not enable TLS for the HTTP API unless the `https` port has been ass all operations, and "extend-cache" allows any cached ACLs to be used, ignoring their TTL values. If a non-cached ACL is used, "extend-cache" acts like "deny". +* `acl_enforce_version_8` - + Used for clients and servers to determine if enforcement should occur for new ACL policies being + previewed before Consul 0.8. Added in Consul 0.7.2, this will default to false in versions of + Consul prior to 0.8, and will default to true in Consul 0.8 and later. This helps ease the + transition to the new ACL features by allowing policies to be in place before enforcement begins. + Please see the [ACL internals guide](/docs/internals/acl.htmlXS) for more details. + * `acl_master_token` - Only used for servers in the [`acl_datacenter`](#acl_datacenter). This token will be created with management-level permissions if it does not exist. It allows operators to bootstrap the ACL system From 9b4f316b210a47d09068df9e9123f5c2cab3c3c5 Mon Sep 17 00:00:00 2001 From: James Phillips Date: Thu, 1 Dec 2016 20:31:50 -0800 Subject: [PATCH 2/9] Sorts all the ACl policy handlers for easier navigation (no functional changes). --- acl/acl.go | 316 ++++++++++++++++++++++----------------------- acl/acl_test.go | 176 ++++++++++++------------- acl/policy_test.go | 174 ++++++++++++------------- 3 files changed, 333 insertions(+), 333 deletions(-) diff --git a/acl/acl.go b/acl/acl.go index f0180d60e7..2a09ad6b15 100644 --- a/acl/acl.go +++ b/acl/acl.go @@ -35,6 +35,18 @@ func init() { // ACL is the interface for policy enforcement. type ACL interface { + // ACLList checks for permission to list all the ACLs + ACLList() bool + + // ACLModify checks for permission to manipulate ACLs + ACLModify() bool + + // EventRead determines if a specific event can be queried. + EventRead(string) bool + + // EventWrite determines if a specific event may be fired. + EventWrite(string) bool + // KeyRead checks for permission to read a given key KeyRead(string) bool @@ -46,26 +58,6 @@ type ACL interface { // that deny a write. KeyWritePrefix(string) bool - // ServiceWrite checks for permission to read a given service - ServiceWrite(string) bool - - // ServiceRead checks for permission to read a given service - ServiceRead(string) bool - - // EventRead determines if a specific event can be queried. - EventRead(string) bool - - // 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 @@ -81,11 +73,19 @@ type ACL interface { // functions can be used. OperatorWrite() bool - // ACLList checks for permission to list all the ACLs - ACLList() bool + // PrepardQueryRead determines if a specific prepared query can be read + // to show its contents (this is not used for execution). + PreparedQueryRead(string) bool - // ACLModify checks for permission to manipulate ACLs - ACLModify() bool + // PreparedQueryWrite determines if a specific prepared query can be + // created, modified, or deleted. + PreparedQueryWrite(string) bool + + // ServiceRead checks for permission to read a given service + ServiceRead(string) bool + + // ServiceWrite checks for permission to read a given service + ServiceWrite(string) bool // Snapshot checks for permission to take and restore snapshots. Snapshot() bool @@ -99,24 +99,12 @@ type StaticACL struct { defaultAllow bool } -func (s *StaticACL) KeyRead(string) bool { - return s.defaultAllow +func (s *StaticACL) ACLList() bool { + return s.allowManage } -func (s *StaticACL) KeyWrite(string) bool { - return s.defaultAllow -} - -func (s *StaticACL) KeyWritePrefix(string) bool { - return s.defaultAllow -} - -func (s *StaticACL) ServiceRead(string) bool { - return s.defaultAllow -} - -func (s *StaticACL) ServiceWrite(string) bool { - return s.defaultAllow +func (s *StaticACL) ACLModify() bool { + return s.allowManage } func (s *StaticACL) EventRead(string) bool { @@ -127,11 +115,15 @@ func (s *StaticACL) EventWrite(string) bool { return s.defaultAllow } -func (s *StaticACL) PreparedQueryRead(string) bool { +func (s *StaticACL) KeyRead(string) bool { return s.defaultAllow } -func (s *StaticACL) PreparedQueryWrite(string) bool { +func (s *StaticACL) KeyWrite(string) bool { + return s.defaultAllow +} + +func (s *StaticACL) KeyWritePrefix(string) bool { return s.defaultAllow } @@ -151,12 +143,20 @@ func (s *StaticACL) OperatorWrite() bool { return s.defaultAllow } -func (s *StaticACL) ACLList() bool { - return s.allowManage +func (s *StaticACL) PreparedQueryRead(string) bool { + return s.defaultAllow } -func (s *StaticACL) ACLModify() bool { - return s.allowManage +func (s *StaticACL) PreparedQueryWrite(string) bool { + return s.defaultAllow +} + +func (s *StaticACL) ServiceRead(string) bool { + return s.defaultAllow +} + +func (s *StaticACL) ServiceWrite(string) bool { + return s.defaultAllow } func (s *StaticACL) Snapshot() bool { @@ -260,6 +260,50 @@ func New(parent ACL, policy *Policy) (*PolicyACL, error) { return p, nil } +// ACLList checks if listing of ACLs is allowed +func (p *PolicyACL) ACLList() bool { + return p.parent.ACLList() +} + +// ACLModify checks if modification of ACLs is allowed +func (p *PolicyACL) ACLModify() bool { + return p.parent.ACLModify() +} + +// Snapshot checks if taking and restoring snapshots is allowed. +func (p *PolicyACL) Snapshot() bool { + return p.parent.Snapshot() +} + +// EventRead is used to determine if the policy allows for a +// specific user event to be read. +func (p *PolicyACL) EventRead(name string) bool { + // Longest-prefix match on event names + if _, rule, ok := p.eventRules.LongestPrefix(name); ok { + switch rule { + case PolicyRead, PolicyWrite: + return true + default: + return false + } + } + + // Nothing matched, use parent + return p.parent.EventRead(name) +} + +// EventWrite is used to determine if new events can be created +// (fired) by the policy. +func (p *PolicyACL) EventWrite(name string) bool { + // Longest-prefix match event names + if _, rule, ok := p.eventRules.LongestPrefix(name); ok { + return rule == PolicyWrite + } + + // No match, use parent + return p.parent.EventWrite(name) +} + // KeyRead returns if a key is allowed to be read func (p *PolicyACL) KeyRead(key string) bool { // Look for a matching rule @@ -327,109 +371,6 @@ func (p *PolicyACL) KeyWritePrefix(prefix string) bool { return p.parent.KeyWritePrefix(prefix) } -// ServiceRead checks if reading (discovery) of a service is allowed -func (p *PolicyACL) ServiceRead(name string) bool { - // Check for an exact rule or catch-all - _, rule, ok := p.serviceRules.LongestPrefix(name) - - if ok { - switch rule { - case PolicyRead, PolicyWrite: - return true - default: - return false - } - } - - // No matching rule, use the parent. - return p.parent.ServiceRead(name) -} - -// ServiceWrite checks if writing (registering) a service is allowed -func (p *PolicyACL) ServiceWrite(name string) bool { - // Check for an exact rule or catch-all - _, rule, ok := p.serviceRules.LongestPrefix(name) - - if ok { - switch rule { - case PolicyWrite: - return true - default: - return false - } - } - - // No matching rule, use the parent. - return p.parent.ServiceWrite(name) -} - -// EventRead is used to determine if the policy allows for a -// specific user event to be read. -func (p *PolicyACL) EventRead(name string) bool { - // Longest-prefix match on event names - if _, rule, ok := p.eventRules.LongestPrefix(name); ok { - switch rule { - case PolicyRead, PolicyWrite: - return true - default: - return false - } - } - - // Nothing matched, use parent - return p.parent.EventRead(name) -} - -// EventWrite is used to determine if new events can be created -// (fired) by the policy. -func (p *PolicyACL) EventWrite(name string) bool { - // Longest-prefix match event names - if _, rule, ok := p.eventRules.LongestPrefix(name); ok { - 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 { @@ -472,17 +413,76 @@ func (p *PolicyACL) OperatorWrite() bool { return p.parent.OperatorWrite() } -// ACLList checks if listing of ACLs is allowed -func (p *PolicyACL) ACLList() bool { - return p.parent.ACLList() +// 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) } -// ACLModify checks if modification of ACLs is allowed -func (p *PolicyACL) ACLModify() bool { - return p.parent.ACLModify() +// 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) } -// Snapshot checks if taking and restoring snapshots is allowed. -func (p *PolicyACL) Snapshot() bool { - return p.parent.Snapshot() +// ServiceRead checks if reading (discovery) of a service is allowed +func (p *PolicyACL) ServiceRead(name string) bool { + // Check for an exact rule or catch-all + _, rule, ok := p.serviceRules.LongestPrefix(name) + + if ok { + switch rule { + case PolicyRead, PolicyWrite: + return true + default: + return false + } + } + + // No matching rule, use the parent. + return p.parent.ServiceRead(name) +} + +// ServiceWrite checks if writing (registering) a service is allowed +func (p *PolicyACL) ServiceWrite(name string) bool { + // Check for an exact rule or catch-all + _, rule, ok := p.serviceRules.LongestPrefix(name) + + if ok { + switch rule { + case PolicyWrite: + return true + default: + return false + } + } + + // No matching rule, use the parent. + return p.parent.ServiceWrite(name) } diff --git a/acl/acl_test.go b/acl/acl_test.go index c6ce5789ab..9a6e710f41 100644 --- a/acl/acl_test.go +++ b/acl/acl_test.go @@ -35,17 +35,11 @@ func TestStaticACL(t *testing.T) { t.Fatalf("expected static") } - if !all.KeyRead("foobar") { - t.Fatalf("should allow") + if all.ACLList() { + t.Fatalf("should not allow") } - if !all.KeyWrite("foobar") { - t.Fatalf("should allow") - } - if !all.ServiceRead("foobar") { - t.Fatalf("should allow") - } - if !all.ServiceWrite("foobar") { - t.Fatalf("should allow") + if all.ACLModify() { + t.Fatalf("should not allow") } if !all.EventRead("foobar") { t.Fatalf("should allow") @@ -53,10 +47,10 @@ func TestStaticACL(t *testing.T) { if !all.EventWrite("foobar") { t.Fatalf("should allow") } - if !all.PreparedQueryRead("foobar") { + if !all.KeyRead("foobar") { t.Fatalf("should allow") } - if !all.PreparedQueryWrite("foobar") { + if !all.KeyWrite("foobar") { t.Fatalf("should allow") } if !all.KeyringRead() { @@ -71,26 +65,26 @@ func TestStaticACL(t *testing.T) { if !all.OperatorWrite() { t.Fatalf("should allow") } - if all.ACLList() { - t.Fatalf("should not allow") + if !all.PreparedQueryRead("foobar") { + t.Fatalf("should allow") } - if all.ACLModify() { - t.Fatalf("should not allow") + if !all.PreparedQueryWrite("foobar") { + t.Fatalf("should allow") + } + if !all.ServiceRead("foobar") { + t.Fatalf("should allow") + } + if !all.ServiceWrite("foobar") { + t.Fatalf("should allow") } if all.Snapshot() { t.Fatalf("should not allow") } - if none.KeyRead("foobar") { + if none.ACLList() { t.Fatalf("should not allow") } - if none.KeyWrite("foobar") { - t.Fatalf("should not allow") - } - if none.ServiceRead("foobar") { - t.Fatalf("should not allow") - } - if none.ServiceWrite("foobar") { + if none.ACLModify() { t.Fatalf("should not allow") } if none.EventRead("foobar") { @@ -105,10 +99,10 @@ func TestStaticACL(t *testing.T) { if none.EventWrite("") { t.Fatalf("should not allow") } - if none.PreparedQueryRead("foobar") { + if none.KeyRead("foobar") { t.Fatalf("should not allow") } - if none.PreparedQueryWrite("foobar") { + if none.KeyWrite("foobar") { t.Fatalf("should not allow") } if none.KeyringRead() { @@ -123,26 +117,26 @@ func TestStaticACL(t *testing.T) { if none.OperatorWrite() { t.Fatalf("should not allow") } - if none.ACLList() { + if none.PreparedQueryRead("foobar") { t.Fatalf("should not allow") } - if none.ACLModify() { + if none.PreparedQueryWrite("foobar") { + t.Fatalf("should not allow") + } + if none.ServiceRead("foobar") { + t.Fatalf("should not allow") + } + if none.ServiceWrite("foobar") { t.Fatalf("should not allow") } if none.Snapshot() { t.Fatalf("should not allow") } - if !manage.KeyRead("foobar") { + if !manage.ACLList() { t.Fatalf("should allow") } - if !manage.KeyWrite("foobar") { - t.Fatalf("should allow") - } - if !manage.ServiceRead("foobar") { - t.Fatalf("should allow") - } - if !manage.ServiceWrite("foobar") { + if !manage.ACLModify() { t.Fatalf("should allow") } if !manage.EventRead("foobar") { @@ -151,10 +145,10 @@ func TestStaticACL(t *testing.T) { if !manage.EventWrite("foobar") { t.Fatalf("should allow") } - if !manage.PreparedQueryRead("foobar") { + if !manage.KeyRead("foobar") { t.Fatalf("should allow") } - if !manage.PreparedQueryWrite("foobar") { + if !manage.KeyWrite("foobar") { t.Fatalf("should allow") } if !manage.KeyringRead() { @@ -169,10 +163,16 @@ func TestStaticACL(t *testing.T) { if !manage.OperatorWrite() { t.Fatalf("should allow") } - if !manage.ACLList() { + if !manage.PreparedQueryRead("foobar") { t.Fatalf("should allow") } - if !manage.ACLModify() { + if !manage.PreparedQueryWrite("foobar") { + t.Fatalf("should allow") + } + if !manage.ServiceRead("foobar") { + t.Fatalf("should allow") + } + if !manage.ServiceWrite("foobar") { t.Fatalf("should allow") } if !manage.Snapshot() { @@ -183,6 +183,20 @@ func TestStaticACL(t *testing.T) { func TestPolicyACL(t *testing.T) { all := AllowAll() policy := &Policy{ + Events: []*EventPolicy{ + &EventPolicy{ + Event: "", + Policy: PolicyRead, + }, + &EventPolicy{ + Event: "foo", + Policy: PolicyWrite, + }, + &EventPolicy{ + Event: "bar", + Policy: PolicyDeny, + }, + }, Keys: []*KeyPolicy{ &KeyPolicy{ Prefix: "foo/", @@ -201,38 +215,6 @@ func TestPolicyACL(t *testing.T) { Policy: PolicyRead, }, }, - Services: []*ServicePolicy{ - &ServicePolicy{ - Name: "", - Policy: PolicyWrite, - }, - &ServicePolicy{ - Name: "foo", - Policy: PolicyRead, - }, - &ServicePolicy{ - Name: "bar", - Policy: PolicyDeny, - }, - &ServicePolicy{ - Name: "barfoo", - Policy: PolicyWrite, - }, - }, - Events: []*EventPolicy{ - &EventPolicy{ - Event: "", - Policy: PolicyRead, - }, - &EventPolicy{ - Event: "foo", - Policy: PolicyWrite, - }, - &EventPolicy{ - Event: "bar", - Policy: PolicyDeny, - }, - }, PreparedQueries: []*PreparedQueryPolicy{ &PreparedQueryPolicy{ Prefix: "", @@ -251,6 +233,24 @@ func TestPolicyACL(t *testing.T) { Policy: PolicyWrite, }, }, + Services: []*ServicePolicy{ + &ServicePolicy{ + Name: "", + Policy: PolicyWrite, + }, + &ServicePolicy{ + Name: "foo", + Policy: PolicyRead, + }, + &ServicePolicy{ + Name: "bar", + Policy: PolicyDeny, + }, + &ServicePolicy{ + Name: "barfoo", + Policy: PolicyWrite, + }, + }, } acl, err := New(all, policy) if err != nil { @@ -369,16 +369,6 @@ func TestPolicyACL_Parent(t *testing.T) { Policy: PolicyRead, }, }, - Services: []*ServicePolicy{ - &ServicePolicy{ - Name: "other", - Policy: PolicyWrite, - }, - &ServicePolicy{ - Name: "foo", - Policy: PolicyRead, - }, - }, PreparedQueries: []*PreparedQueryPolicy{ &PreparedQueryPolicy{ Prefix: "other", @@ -389,6 +379,16 @@ func TestPolicyACL_Parent(t *testing.T) { Policy: PolicyRead, }, }, + Services: []*ServicePolicy{ + &ServicePolicy{ + Name: "other", + Policy: PolicyWrite, + }, + &ServicePolicy{ + Name: "foo", + Policy: PolicyRead, + }, + }, } root, err := New(deny, policyRoot) if err != nil { @@ -410,18 +410,18 @@ func TestPolicyACL_Parent(t *testing.T) { Policy: PolicyRead, }, }, - Services: []*ServicePolicy{ - &ServicePolicy{ - Name: "bar", - Policy: PolicyDeny, - }, - }, PreparedQueries: []*PreparedQueryPolicy{ &PreparedQueryPolicy{ Prefix: "bar", Policy: PolicyDeny, }, }, + Services: []*ServicePolicy{ + &ServicePolicy{ + Name: "bar", + Policy: PolicyDeny, + }, + }, } acl, err := New(root, policy) if err != nil { diff --git a/acl/policy_test.go b/acl/policy_test.go index 7f31bf8608..f086c4d13f 100644 --- a/acl/policy_test.go +++ b/acl/policy_test.go @@ -8,6 +8,15 @@ import ( func TestACLPolicy_Parse_HCL(t *testing.T) { inp := ` +event "" { + policy = "read" +} +event "foo" { + policy = "write" +} +event "bar" { + policy = "deny" +} key "" { policy = "read" } @@ -20,21 +29,14 @@ key "foo/bar/" { key "foo/bar/baz" { policy = "deny" } +keyring = "deny" +operator = "deny" service "" { policy = "write" } service "foo" { policy = "read" } -event "" { - policy = "read" -} -event "foo" { - policy = "write" -} -event "bar" { - policy = "deny" -} query "" { policy = "read" } @@ -44,10 +46,23 @@ query "foo" { query "bar" { policy = "deny" } -keyring = "deny" -operator = "deny" ` exp := &Policy{ + Events: []*EventPolicy{ + &EventPolicy{ + Event: "", + Policy: PolicyRead, + }, + &EventPolicy{ + Event: "foo", + Policy: PolicyWrite, + }, + &EventPolicy{ + Event: "bar", + Policy: PolicyDeny, + }, + }, + Keyring: PolicyDeny, Keys: []*KeyPolicy{ &KeyPolicy{ Prefix: "", @@ -66,30 +81,7 @@ operator = "deny" Policy: PolicyDeny, }, }, - Services: []*ServicePolicy{ - &ServicePolicy{ - Name: "", - Policy: PolicyWrite, - }, - &ServicePolicy{ - Name: "foo", - Policy: PolicyRead, - }, - }, - Events: []*EventPolicy{ - &EventPolicy{ - Event: "", - Policy: PolicyRead, - }, - &EventPolicy{ - Event: "foo", - Policy: PolicyWrite, - }, - &EventPolicy{ - Event: "bar", - Policy: PolicyDeny, - }, - }, + Operator: PolicyDeny, PreparedQueries: []*PreparedQueryPolicy{ &PreparedQueryPolicy{ Prefix: "", @@ -104,8 +96,16 @@ operator = "deny" Policy: PolicyDeny, }, }, - Keyring: PolicyDeny, - Operator: PolicyDeny, + Services: []*ServicePolicy{ + &ServicePolicy{ + Name: "", + Policy: PolicyWrite, + }, + &ServicePolicy{ + Name: "foo", + Policy: PolicyRead, + }, + }, } out, err := Parse(inp) @@ -120,6 +120,17 @@ operator = "deny" func TestACLPolicy_Parse_JSON(t *testing.T) { inp := `{ + "event": { + "": { + "policy": "read" + }, + "foo": { + "policy": "write" + }, + "bar": { + "policy": "deny" + } + }, "key": { "": { "policy": "read" @@ -134,25 +145,8 @@ func TestACLPolicy_Parse_JSON(t *testing.T) { "policy": "deny" } }, - "service": { - "": { - "policy": "write" - }, - "foo": { - "policy": "read" - } - }, - "event": { - "": { - "policy": "read" - }, - "foo": { - "policy": "write" - }, - "bar": { - "policy": "deny" - } - }, + "keyring": "deny", + "operator": "deny", "query": { "": { "policy": "read" @@ -164,10 +158,31 @@ func TestACLPolicy_Parse_JSON(t *testing.T) { "policy": "deny" } }, - "keyring": "deny", - "operator": "deny" + "service": { + "": { + "policy": "write" + }, + "foo": { + "policy": "read" + } + } }` exp := &Policy{ + Events: []*EventPolicy{ + &EventPolicy{ + Event: "", + Policy: PolicyRead, + }, + &EventPolicy{ + Event: "foo", + Policy: PolicyWrite, + }, + &EventPolicy{ + Event: "bar", + Policy: PolicyDeny, + }, + }, + Keyring: PolicyDeny, Keys: []*KeyPolicy{ &KeyPolicy{ Prefix: "", @@ -186,30 +201,7 @@ func TestACLPolicy_Parse_JSON(t *testing.T) { Policy: PolicyDeny, }, }, - Services: []*ServicePolicy{ - &ServicePolicy{ - Name: "", - Policy: PolicyWrite, - }, - &ServicePolicy{ - Name: "foo", - Policy: PolicyRead, - }, - }, - Events: []*EventPolicy{ - &EventPolicy{ - Event: "", - Policy: PolicyRead, - }, - &EventPolicy{ - Event: "foo", - Policy: PolicyWrite, - }, - &EventPolicy{ - Event: "bar", - Policy: PolicyDeny, - }, - }, + Operator: PolicyDeny, PreparedQueries: []*PreparedQueryPolicy{ &PreparedQueryPolicy{ Prefix: "", @@ -224,8 +216,16 @@ func TestACLPolicy_Parse_JSON(t *testing.T) { Policy: PolicyDeny, }, }, - Keyring: PolicyDeny, - Operator: PolicyDeny, + Services: []*ServicePolicy{ + &ServicePolicy{ + Name: "", + Policy: PolicyWrite, + }, + &ServicePolicy{ + Name: "foo", + Policy: PolicyRead, + }, + }, } out, err := Parse(inp) @@ -276,12 +276,12 @@ operator = "" func TestACLPolicy_Bad_Policy(t *testing.T) { cases := []string{ - `key "" { policy = "nope" }`, - `service "" { policy = "nope" }`, `event "" { policy = "nope" }`, - `query "" { policy = "nope" }`, + `key "" { policy = "nope" }`, `keyring = "nope"`, `operator = "nope"`, + `query "" { policy = "nope" }`, + `service "" { policy = "nope" }`, } for _, c := range cases { _, err := Parse(c) From 7fa4ab3fd1a525753787995d182406341bc5a37b Mon Sep 17 00:00:00 2001 From: James Phillips Date: Tue, 6 Dec 2016 20:05:15 -0800 Subject: [PATCH 3/9] Adds support to ACL package for node policies. --- acl/acl.go | 63 +++++++++++++++++++++++++++- acl/acl_test.go | 101 +++++++++++++++++++++++++++++++++++++++++++++ acl/policy.go | 28 ++++++++++--- acl/policy_test.go | 49 ++++++++++++++++++++++ 4 files changed, 235 insertions(+), 6 deletions(-) diff --git a/acl/acl.go b/acl/acl.go index 2a09ad6b15..5b2be3f989 100644 --- a/acl/acl.go +++ b/acl/acl.go @@ -65,6 +65,13 @@ type ACL interface { // KeyringWrite determines if the keyring can be manipulated KeyringWrite() bool + // NodeRead checks for permission to read (discover) a given node. + NodeRead(string) bool + + // NodeWrite checks for permission to create or update (register) a + // given node. + NodeWrite(string) bool + // OperatorRead determines if the read-only Consul operator functions // can be used. OperatorRead() bool @@ -84,7 +91,8 @@ type ACL interface { // ServiceRead checks for permission to read a given service ServiceRead(string) bool - // ServiceWrite checks for permission to read a given service + // ServiceWrite checks for permission to create or update a given + // service ServiceWrite(string) bool // Snapshot checks for permission to take and restore snapshots. @@ -135,6 +143,14 @@ func (s *StaticACL) KeyringWrite() bool { return s.defaultAllow } +func (s *StaticACL) NodeRead(string) bool { + return s.defaultAllow +} + +func (s *StaticACL) NodeWrite(string) bool { + return s.defaultAllow +} + func (s *StaticACL) OperatorRead() bool { return s.defaultAllow } @@ -202,6 +218,9 @@ type PolicyACL struct { // keyRules contains the key policies keyRules *radix.Tree + // nodeRules contains the node policies + nodeRules *radix.Tree + // serviceRules contains the service policies serviceRules *radix.Tree @@ -226,6 +245,7 @@ func New(parent ACL, policy *Policy) (*PolicyACL, error) { p := &PolicyACL{ parent: parent, keyRules: radix.New(), + nodeRules: radix.New(), serviceRules: radix.New(), eventRules: radix.New(), preparedQueryRules: radix.New(), @@ -236,6 +256,11 @@ func New(parent ACL, policy *Policy) (*PolicyACL, error) { p.keyRules.Insert(kp.Prefix, kp.Policy) } + // Load the node policy + for _, np := range policy.Nodes { + p.nodeRules.Insert(np.Name, np.Policy) + } + // Load the service policy for _, sp := range policy.Services { p.serviceRules.Insert(sp.Name, sp.Policy) @@ -404,6 +429,42 @@ func (p *PolicyACL) OperatorRead() bool { } } +// NodeRead checks if reading (discovery) of a node is allowed +func (p *PolicyACL) NodeRead(name string) bool { + // Check for an exact rule or catch-all + _, rule, ok := p.nodeRules.LongestPrefix(name) + + if ok { + switch rule { + case PolicyRead, PolicyWrite: + return true + default: + return false + } + } + + // No matching rule, use the parent. + return p.parent.NodeRead(name) +} + +// NodeWrite checks if writing (registering) a node is allowed +func (p *PolicyACL) NodeWrite(name string) bool { + // Check for an exact rule or catch-all + _, rule, ok := p.nodeRules.LongestPrefix(name) + + if ok { + switch rule { + case PolicyWrite: + return true + default: + return false + } + } + + // No matching rule, use the parent. + return p.parent.NodeWrite(name) +} + // OperatorWrite determines if the state-changing operator functions are // allowed. func (p *PolicyACL) OperatorWrite() bool { diff --git a/acl/acl_test.go b/acl/acl_test.go index 9a6e710f41..e398e50c90 100644 --- a/acl/acl_test.go +++ b/acl/acl_test.go @@ -59,6 +59,12 @@ func TestStaticACL(t *testing.T) { if !all.KeyringWrite() { t.Fatalf("should allow") } + if !all.NodeRead("foobar") { + t.Fatalf("should allow") + } + if !all.NodeWrite("foobar") { + t.Fatalf("should allow") + } if !all.OperatorRead() { t.Fatalf("should allow") } @@ -111,6 +117,12 @@ func TestStaticACL(t *testing.T) { if none.KeyringWrite() { t.Fatalf("should not allow") } + if none.NodeRead("foobar") { + t.Fatalf("should not allow") + } + if none.NodeWrite("foobar") { + t.Fatalf("should not allow") + } if none.OperatorRead() { t.Fatalf("should now allow") } @@ -157,6 +169,12 @@ func TestStaticACL(t *testing.T) { if !manage.KeyringWrite() { t.Fatalf("should allow") } + if !manage.NodeRead("foobar") { + t.Fatalf("should allow") + } + if !manage.NodeWrite("foobar") { + t.Fatalf("should allow") + } if !manage.OperatorRead() { t.Fatalf("should allow") } @@ -560,3 +578,86 @@ func TestPolicyACL_Operator(t *testing.T) { } } } + +func TestPolicyACL_Node(t *testing.T) { + deny := DenyAll() + policyRoot := &Policy{ + Nodes: []*NodePolicy{ + &NodePolicy{ + Name: "root-nope", + Policy: PolicyDeny, + }, + &NodePolicy{ + Name: "root-ro", + Policy: PolicyRead, + }, + &NodePolicy{ + Name: "root-rw", + Policy: PolicyWrite, + }, + &NodePolicy{ + Name: "override", + Policy: PolicyDeny, + }, + }, + } + root, err := New(deny, policyRoot) + if err != nil { + t.Fatalf("err: %v", err) + } + + policy := &Policy{ + Nodes: []*NodePolicy{ + &NodePolicy{ + Name: "child-nope", + Policy: PolicyDeny, + }, + &NodePolicy{ + Name: "child-ro", + Policy: PolicyRead, + }, + &NodePolicy{ + Name: "child-rw", + Policy: PolicyWrite, + }, + &NodePolicy{ + Name: "override", + Policy: PolicyWrite, + }, + }, + } + acl, err := New(root, policy) + if err != nil { + t.Fatalf("err: %v", err) + } + + type nodecase struct { + inp string + read bool + write bool + } + cases := []nodecase{ + {"nope", false, false}, + {"root-nope", false, false}, + {"root-ro", true, false}, + {"root-rw", true, true}, + {"root-nope-prefix", false, false}, + {"root-ro-prefix", true, false}, + {"root-rw-prefix", true, true}, + {"child-nope", false, false}, + {"child-ro", true, false}, + {"child-rw", true, true}, + {"child-nope-prefix", false, false}, + {"child-ro-prefix", true, false}, + {"child-rw-prefix", true, true}, + {"override", true, true}, + } + for _, c := range cases { + if c.read != acl.NodeRead(c.inp) { + t.Fatalf("Read fail: %#v", c) + } + if c.write != acl.NodeWrite(c.inp) { + t.Fatalf("Write fail: %#v", c) + } + } +} diff --git a/acl/policy.go b/acl/policy.go index ae69067fea..c01dbaea18 100644 --- a/acl/policy.go +++ b/acl/policy.go @@ -17,6 +17,7 @@ const ( type Policy struct { ID string `hcl:"-"` Keys []*KeyPolicy `hcl:"key,expand"` + Nodes []*NodePolicy `hcl:"node,expand"` Services []*ServicePolicy `hcl:"service,expand"` Events []*EventPolicy `hcl:"event,expand"` PreparedQueries []*PreparedQueryPolicy `hcl:"query,expand"` @@ -34,14 +35,24 @@ func (k *KeyPolicy) GoString() string { return fmt.Sprintf("%#v", *k) } +// NodePolicy represents a policy for a node +type NodePolicy struct { + Name string `hcl:",key"` + Policy string +} + +func (n *NodePolicy) GoString() string { + return fmt.Sprintf("%#v", *n) +} + // ServicePolicy represents a policy for a service type ServicePolicy struct { Name string `hcl:",key"` Policy string } -func (k *ServicePolicy) GoString() string { - return fmt.Sprintf("%#v", *k) +func (s *ServicePolicy) GoString() string { + return fmt.Sprintf("%#v", *s) } // EventPolicy represents a user event policy. @@ -60,8 +71,8 @@ type PreparedQueryPolicy struct { Policy string } -func (e *PreparedQueryPolicy) GoString() string { - return fmt.Sprintf("%#v", *e) +func (p *PreparedQueryPolicy) GoString() string { + return fmt.Sprintf("%#v", *p) } // isPolicyValid makes sure the given string matches one of the valid policies. @@ -100,7 +111,14 @@ func Parse(rules string) (*Policy, error) { } } - // Validate the service policy + // Validate the node policies + for _, np := range p.Nodes { + if !isPolicyValid(np.Policy) { + return nil, fmt.Errorf("Invalid node policy: %#v", np) + } + } + + // Validate the service policies for _, sp := range p.Services { if !isPolicyValid(sp.Policy) { return nil, fmt.Errorf("Invalid service policy: %#v", sp) diff --git a/acl/policy_test.go b/acl/policy_test.go index f086c4d13f..23504ddbec 100644 --- a/acl/policy_test.go +++ b/acl/policy_test.go @@ -30,6 +30,15 @@ key "foo/bar/baz" { policy = "deny" } keyring = "deny" +node "" { + policy = "read" +} +node "foo" { + policy = "write" +} +node "bar" { + policy = "deny" +} operator = "deny" service "" { policy = "write" @@ -81,6 +90,20 @@ query "bar" { Policy: PolicyDeny, }, }, + Nodes: []*NodePolicy{ + &NodePolicy{ + Name: "", + Policy: PolicyRead, + }, + &NodePolicy{ + Name: "foo", + Policy: PolicyWrite, + }, + &NodePolicy{ + Name: "bar", + Policy: PolicyDeny, + }, + }, Operator: PolicyDeny, PreparedQueries: []*PreparedQueryPolicy{ &PreparedQueryPolicy{ @@ -146,6 +169,17 @@ func TestACLPolicy_Parse_JSON(t *testing.T) { } }, "keyring": "deny", + "node": { + "": { + "policy": "read" + }, + "foo": { + "policy": "write" + }, + "bar": { + "policy": "deny" + } + }, "operator": "deny", "query": { "": { @@ -201,6 +235,20 @@ func TestACLPolicy_Parse_JSON(t *testing.T) { Policy: PolicyDeny, }, }, + Nodes: []*NodePolicy{ + &NodePolicy{ + Name: "", + Policy: PolicyRead, + }, + &NodePolicy{ + Name: "foo", + Policy: PolicyWrite, + }, + &NodePolicy{ + Name: "bar", + Policy: PolicyDeny, + }, + }, Operator: PolicyDeny, PreparedQueries: []*PreparedQueryPolicy{ &PreparedQueryPolicy{ @@ -279,6 +327,7 @@ func TestACLPolicy_Bad_Policy(t *testing.T) { `event "" { policy = "nope" }`, `key "" { policy = "nope" }`, `keyring = "nope"`, + `node "" { policy = "nope" }`, `operator = "nope"`, `query "" { policy = "nope" }`, `service "" { policy = "nope" }`, From 66b437ca338b1e3d9d7f906adff9c5f975bc73cb Mon Sep 17 00:00:00 2001 From: James Phillips Date: Wed, 7 Dec 2016 17:58:23 -0800 Subject: [PATCH 4/9] Removes the exception for the "consul" service in the catalog. --- command/agent/agent.go | 4 ++++ consul/catalog_endpoint.go | 22 +++++++++++++--------- consul/catalog_endpoint_test.go | 18 +++++++++++++++++- consul/config.go | 4 ++++ 4 files changed, 38 insertions(+), 10 deletions(-) diff --git a/command/agent/agent.go b/command/agent/agent.go index eef2992d8a..23c7f4d557 100644 --- a/command/agent/agent.go +++ b/command/agent/agent.go @@ -227,6 +227,7 @@ func Create(config *Config, logOutput io.Writer, logWriter *logger.LogWriter, Port: agent.config.Ports.Server, Tags: []string{}, } + // TODO (slackpad) - Plumb the "acl_agent_token" into here. agent.state.AddService(&consulService, "") } else { err = agent.setupClient() @@ -381,6 +382,9 @@ func (a *Agent) consulConfig() *consul.Config { if a.config.ACLReplicationToken != "" { base.ACLReplicationToken = a.config.ACLReplicationToken } + if a.config.ACLEnforceVersion8 != nil { + base.ACLEnforceVersion8 = *a.config.ACLEnforceVersion8 + } if a.config.SessionTTLMinRaw != "" { base.SessionTTLMin = a.config.SessionTTLMin } diff --git a/consul/catalog_endpoint.go b/consul/catalog_endpoint.go index 06f81993b9..7b85aeec94 100644 --- a/consul/catalog_endpoint.go +++ b/consul/catalog_endpoint.go @@ -26,6 +26,12 @@ func (c *Catalog) Register(args *structs.RegisterRequest, reply *struct{}) error return fmt.Errorf("Must provide node and address") } + // Fetch the ACL token, if any. + acl, err := c.srv.resolveToken(args.Token) + if err != nil { + return err + } + if args.Service != nil { // If no service id, but service name, use default if args.Service.ID == "" && args.Service.Service != "" { @@ -37,14 +43,12 @@ func (c *Catalog) Register(args *structs.RegisterRequest, reply *struct{}) error return fmt.Errorf("Must provide service name with ID") } - // Apply the ACL policy if any - // The 'consul' service is excluded since it is managed - // automatically internally. - if args.Service.Service != ConsulServiceName { - acl, err := c.srv.resolveToken(args.Token) - if err != nil { - return err - } else if acl != nil && !acl.ServiceWrite(args.Service.Service) { + // Apply the ACL policy if any. The 'consul' service is excluded + // since it is managed automatically internally (that behavior + // is going away after version 0.8). + if c.srv.config.ACLEnforceVersion8 || + (args.Service.Service != ConsulServiceName) { + if acl != nil && !acl.ServiceWrite(args.Service.Service) { c.srv.logger.Printf("[WARN] consul.catalog: Register of service '%s' on '%s' denied due to ACLs", args.Service.Service, args.Node) return permissionDeniedErr @@ -65,7 +69,7 @@ func (c *Catalog) Register(args *structs.RegisterRequest, reply *struct{}) error } } - _, err := c.srv.raftApply(structs.RegisterRequestType, args) + _, err = c.srv.raftApply(structs.RegisterRequestType, args) if err != nil { c.srv.logger.Printf("[ERR] consul.catalog: Register failed: %v", err) return err diff --git a/consul/catalog_endpoint_test.go b/consul/catalog_endpoint_test.go index 8171551121..4282eea031 100644 --- a/consul/catalog_endpoint_test.go +++ b/consul/catalog_endpoint_test.go @@ -46,11 +46,12 @@ func TestCatalogRegister(t *testing.T) { }) } -func TestCatalogRegister_ACLDeny(t *testing.T) { +func TestCatalogRegister_ACLDeny_Service(t *testing.T) { dir1, s1 := testServerWithConfig(t, func(c *Config) { c.ACLDatacenter = "dc1" c.ACLMasterToken = "root" c.ACLDefaultPolicy = "deny" + c.ACLEnforceVersion8 = false }) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -99,6 +100,21 @@ func TestCatalogRegister_ACLDeny(t *testing.T) { if err != nil { t.Fatalf("err: %v", err) } + + // Try the special case for the "consul" service. + argR.Service.Service = "consul" + err = msgpackrpc.CallWithCodec(codec, "Catalog.Register", &argR, &outR) + if err != nil { + t.Fatalf("err: %v", err) + } + + // Make sure the exception goes away when we turn on version 8 ACL + // enforcement. + s1.config.ACLEnforceVersion8 = true + err = msgpackrpc.CallWithCodec(codec, "Catalog.Register", &argR, &outR) + if err == nil || !strings.Contains(err.Error(), permissionDenied) { + t.Fatalf("err: %v", err) + } } func TestCatalogRegister_ForwardLeader(t *testing.T) { diff --git a/consul/config.go b/consul/config.go index 0e094f305b..0259b3d31d 100644 --- a/consul/config.go +++ b/consul/config.go @@ -200,6 +200,10 @@ type Config struct { // used to limit the amount of Raft bandwidth used for replication. ACLReplicationApplyLimit int + // ACLEnforceVersion8 is used to gate a set of ACL policy features that + // are opt-in prior to Consul 0.8 and opt-out in Consul 0.8 and later. + ACLEnforceVersion8 bool + // TombstoneTTL is used to control how long KV tombstones are retained. // This provides a window of time where the X-Consul-Index is monotonic. // Outside this window, the index may not be monotonic. This is a result From 800c67c58aff28389e78f12bd3aa6f58efdbacd6 Mon Sep 17 00:00:00 2001 From: James Phillips Date: Thu, 8 Dec 2016 16:01:01 -0800 Subject: [PATCH 5/9] Adds complete ACL coverage for /v1/catalog/register. --- consul/acl.go | 100 +++++++++++++ consul/acl_test.go | 239 +++++++++++++++++++++++++++++++- consul/catalog_endpoint.go | 27 +++- consul/catalog_endpoint_test.go | 27 +++- consul/state/state_store.go | 6 + consul/structs/structs.go | 19 +++ consul/structs/structs_test.go | 38 +++++ 7 files changed, 442 insertions(+), 14 deletions(-) diff --git a/consul/acl.go b/consul/acl.go index 278395f84e..cc3b35cf3d 100644 --- a/consul/acl.go +++ b/consul/acl.go @@ -544,3 +544,103 @@ func (s *Server) filterACL(token string, subj interface{}) error { return nil } + +// vetRegisterWithACL applies the given ACL's policy to the catalog update and +// determines if it is allowed. Since the catalog register request is so +// dynamic, this is a pretty complex algorithm and was worth breaking out of the +// endpoint. +// +// This is a bit racy because we have to check the state store outside of a +// transaction. It's the best we can do because we don't want to flow ACL +// checking down there. The node information doesn't change in practice, so this +// will be fine. If we expose ways to change node addresses in a later version, +// then we should split the catalog API at the node and service level so we can +// address this race better (even then it would be super rare, and would at +// worst let a service update revert a recent node update, so it doesn't open up +// too much abuse). +func vetRegisterWithACL(acl acl.ACL, subj *structs.RegisterRequest, ns *structs.NodeServices) error { + // Fast path if ACLs are not enabled. + if acl == nil { + return nil + } + + // Vet the node info. This allows service updates to re-post the required + // node info for each request without having to have node "write" + // privileges. + needsNode := ns == nil || subj.ChangesNode(ns.Node) + if needsNode && !acl.NodeWrite(subj.Node) { + return permissionDeniedErr + } + + // Vet the service change. This includes making sure they can register + // the given service, and that we can write to any existing service that + // is being modified by id (if any). + if subj.Service != nil { + if !acl.ServiceWrite(subj.Service.Service) { + return permissionDeniedErr + } + + if ns != nil { + other, ok := ns.Services[subj.Service.ID] + if ok && !acl.ServiceWrite(other.Service) { + return permissionDeniedErr + } + } + } + + // Make sure that the member was flattened before we got there. This + // keeps us from having to verify this check as well. + if subj.Check != nil { + return fmt.Errorf("check member must be nil") + } + + // Vet the checks. Node-level checks require node write, and + // service-level checks require service write. + for _, check := range subj.Checks { + // Make sure that the node matches - we don't allow you to mix + // checks from other nodes because we'd have to pull a bunch + // more state store data to check this. If ACLs are enabled then + // we simply require them to match in a given request. There's a + // note in state_store.go to ban this down there in Consul 0.8, + // but it's good to leave this here because it's required for + // correctness wrt. ACLs. + if check.Node != subj.Node { + return fmt.Errorf("Node '%s' for check '%s' doesn't match register request node '%s'", + check.Node, check.CheckID, subj.Node) + } + + // Node-level check. + if check.ServiceID == "" { + if !acl.NodeWrite(subj.Node) { + return permissionDeniedErr + } + continue + } + + // Service-level check, check the common case where it + // matches the service part of this request, which has + // already been vetted above, and might be being registered + // along with its checks. + if subj.Service != nil && subj.Service.ID == check.ServiceID { + continue + } + + // Service-level check for some other service. Make sure they've + // got write permissions for that service. + if ns == nil { + return fmt.Errorf("Unknown service '%s' for check '%s'", + check.ServiceID, check.CheckID) + } else { + other, ok := ns.Services[check.ServiceID] + if !ok { + return fmt.Errorf("Unknown service '%s' for check '%s'", + check.ServiceID, check.CheckID) + } + if !acl.ServiceWrite(other.Service) { + return permissionDeniedErr + } + } + } + + return nil +} diff --git a/consul/acl_test.go b/consul/acl_test.go index 6eb9308bd6..1d36aaa1bb 100644 --- a/consul/acl_test.go +++ b/consul/acl_test.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "reflect" + "strings" "testing" "github.com/hashicorp/consul/acl" @@ -12,6 +13,15 @@ import ( "github.com/hashicorp/consul/testutil" ) +var testACLPolicy = ` +key "" { + policy = "deny" +} +key "foo/" { + policy = "write" +} +` + func TestACL_Disabled(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) @@ -1106,11 +1116,228 @@ func TestACL_unhandledFilterType(t *testing.T) { srv.filterACL(token, &structs.HealthCheck{}) } -var testACLPolicy = ` -key "" { - policy = "deny" +func TestACL_vetRegisterWithACL(t *testing.T) { + args := &structs.RegisterRequest{ + Node: "nope", + Address: "127.0.0.1", + } + + // With a nil ACL, the update should be allowed. + if err := vetRegisterWithACL(nil, args, nil); err != nil { + t.Fatalf("err: %v", err) + } + + // Create a basic node policy. + policy, err := acl.Parse(` +node "node" { + policy = "write" } -key "foo/" { - policy = "write" +`) + if err != nil { + t.Fatalf("err %v", err) + } + perms, err := acl.New(acl.DenyAll(), policy) + if err != nil { + t.Fatalf("err: %v", err) + } + + // With that policy, the update should now be blocked for node reasons. + err = vetRegisterWithACL(perms, args, nil) + if err == nil || !strings.Contains(err.Error(), permissionDenied) { + t.Fatalf("bad: %v", err) + } + + // Now use a permitted node name. + args.Node = "node" + if err := vetRegisterWithACL(perms, args, nil); err != nil { + t.Fatalf("err: %v", err) + } + + // Build some node info that matches what we have now. + ns := &structs.NodeServices{ + Node: &structs.Node{ + Node: "node", + Address: "127.0.0.1", + }, + Services: make(map[string]*structs.NodeService), + } + + // Try to register a service, which should be blocked. + args.Service = &structs.NodeService{ + Service: "service", + ID: "my-id", + } + err = vetRegisterWithACL(perms, args, ns) + if err == nil || !strings.Contains(err.Error(), permissionDenied) { + t.Fatalf("bad: %v", err) + } + + // Chain on a basic service policy. + policy, err = acl.Parse(` +service "service" { + policy = "write" +} +`) + if err != nil { + t.Fatalf("err %v", err) + } + perms, err = acl.New(perms, policy) + if err != nil { + t.Fatalf("err: %v", err) + } + + // With the service ACL, the update should go through. + if err := vetRegisterWithACL(perms, args, ns); err != nil { + t.Fatalf("err: %v", err) + } + + // Add an existing service that they are clobbering and aren't allowed + // to write to. + ns.Services["my-id"] = &structs.NodeService{ + Service: "other", + ID: "my-id", + } + err = vetRegisterWithACL(perms, args, ns) + if err == nil || !strings.Contains(err.Error(), permissionDenied) { + t.Fatalf("bad: %v", err) + } + + // Chain on a policy that allows them to write to the other service. + policy, err = acl.Parse(` +service "other" { + policy = "write" +} +`) + if err != nil { + t.Fatalf("err %v", err) + } + perms, err = acl.New(perms, policy) + if err != nil { + t.Fatalf("err: %v", err) + } + + // Now it should go through. + if err := vetRegisterWithACL(perms, args, ns); err != nil { + t.Fatalf("err: %v", err) + } + + // Try creating the node and the service at once by having no existing + // node record. This should be ok since we have node and service + // permissions. + if err := vetRegisterWithACL(perms, args, nil); err != nil { + t.Fatalf("err: %v", err) + } + + // Add a node-level check to the member, which should be rejected. + args.Check = &structs.HealthCheck{ + Node: "node", + } + err = vetRegisterWithACL(perms, args, ns) + if err == nil || !strings.Contains(err.Error(), "check member must be nil") { + t.Fatalf("bad: %v", err) + } + + // Move the check into the slice, but give a bad node name. + args.Check.Node = "nope" + args.Checks = append(args.Checks, args.Check) + args.Check = nil + err = vetRegisterWithACL(perms, args, ns) + if err == nil || !strings.Contains(err.Error(), "doesn't match register request node") { + t.Fatalf("bad: %v", err) + } + + // Fix the node name, which should now go through. + args.Checks[0].Node = "node" + if err := vetRegisterWithACL(perms, args, ns); err != nil { + t.Fatalf("err: %v", err) + } + + // Add a service-level check. + args.Checks = append(args.Checks, &structs.HealthCheck{ + Node: "node", + ServiceID: "my-id", + }) + if err := vetRegisterWithACL(perms, args, ns); err != nil { + t.Fatalf("err: %v", err) + } + + // Try creating everything at once. This should be ok since we have all + // the permissions we need. It also makes sure that we can register a + // new node, service, and associated checks. + if err := vetRegisterWithACL(perms, args, nil); err != nil { + t.Fatalf("err: %v", err) + } + + // Nil out the service registration, which'll skip the special case + // and force us to look at the ns data (it will look like we are + // writing to the "other" service which also has "my-id"). + args.Service = nil + if err := vetRegisterWithACL(perms, args, ns); err != nil { + t.Fatalf("err: %v", err) + } + + // Chain on a policy that forbids them to write to the other service. + policy, err = acl.Parse(` +service "other" { + policy = "deny" +} +`) + if err != nil { + t.Fatalf("err %v", err) + } + perms, err = acl.New(perms, policy) + if err != nil { + t.Fatalf("err: %v", err) + } + + // This should get rejected. + err = vetRegisterWithACL(perms, args, ns) + if err == nil || !strings.Contains(err.Error(), permissionDenied) { + t.Fatalf("bad: %v", err) + } + + // Change the existing service data to point to a service name they + // car write to. This should go through. + ns.Services["my-id"] = &structs.NodeService{ + Service: "service", + ID: "my-id", + } + if err := vetRegisterWithACL(perms, args, ns); err != nil { + t.Fatalf("err: %v", err) + } + + // Chain on a policy that forbids them to write to the node. + policy, err = acl.Parse(` +node "node" { + policy = "deny" +} +`) + if err != nil { + t.Fatalf("err %v", err) + } + perms, err = acl.New(perms, policy) + if err != nil { + t.Fatalf("err: %v", err) + } + + // This should get rejected because there's a node-level check in here. + err = vetRegisterWithACL(perms, args, ns) + if err == nil || !strings.Contains(err.Error(), permissionDenied) { + t.Fatalf("bad: %v", err) + } + + // Change the node-level check into a service check, and then it should + // go through. + args.Checks[0].ServiceID = "my-id" + if err := vetRegisterWithACL(perms, args, ns); err != nil { + t.Fatalf("err: %v", err) + } + + // Finally, attempt to update the node part of the data and make sure + // that gets rejected since they no longer have permissions. + args.Address = "127.0.0.2" + err = vetRegisterWithACL(perms, args, ns) + if err == nil || !strings.Contains(err.Error(), permissionDenied) { + t.Fatalf("bad: %v", err) + } } -` diff --git a/consul/catalog_endpoint.go b/consul/catalog_endpoint.go index 7b85aeec94..6c49417580 100644 --- a/consul/catalog_endpoint.go +++ b/consul/catalog_endpoint.go @@ -21,7 +21,7 @@ func (c *Catalog) Register(args *structs.RegisterRequest, reply *struct{}) error } defer metrics.MeasureSince([]string{"consul", "catalog", "register"}, time.Now()) - // Verify the args + // Verify the args. if args.Node == "" || args.Address == "" { return fmt.Errorf("Must provide node and address") } @@ -32,30 +32,31 @@ func (c *Catalog) Register(args *structs.RegisterRequest, reply *struct{}) error return err } + // Handle a service registration. if args.Service != nil { // If no service id, but service name, use default if args.Service.ID == "" && args.Service.Service != "" { args.Service.ID = args.Service.Service } - // Verify ServiceName provided if ID + // Verify ServiceName provided if ID. if args.Service.ID != "" && args.Service.Service == "" { return fmt.Errorf("Must provide service name with ID") } // Apply the ACL policy if any. The 'consul' service is excluded // since it is managed automatically internally (that behavior - // is going away after version 0.8). - if c.srv.config.ACLEnforceVersion8 || - (args.Service.Service != ConsulServiceName) { + // is going away after version 0.8). We check this same policy + // later if version 0.8 is enabled, so we can eventually just + // delete this and do all the ACL checks down there. + if args.Service.Service != ConsulServiceName { if acl != nil && !acl.ServiceWrite(args.Service.Service) { - c.srv.logger.Printf("[WARN] consul.catalog: Register of service '%s' on '%s' denied due to ACLs", - args.Service.Service, args.Node) return permissionDeniedErr } } } + // Move the old format single check into the slice, and fixup IDs. if args.Check != nil { args.Checks = append(args.Checks, args.Check) args.Check = nil @@ -69,6 +70,18 @@ func (c *Catalog) Register(args *structs.RegisterRequest, reply *struct{}) error } } + // Check the complete register request against the given ACL policy. + if c.srv.config.ACLEnforceVersion8 { + state := c.srv.fsm.State() + _, ns, err := state.NodeServices(args.Node) + if err != nil { + return fmt.Errorf("Node lookup failed: %v", err) + } + if err := vetRegisterWithACL(acl, args, ns); err != nil { + return err + } + } + _, err = c.srv.raftApply(structs.RegisterRequestType, args) if err != nil { c.srv.logger.Printf("[ERR] consul.catalog: Register failed: %v", err) diff --git a/consul/catalog_endpoint_test.go b/consul/catalog_endpoint_test.go index 4282eea031..20c5efdd0d 100644 --- a/consul/catalog_endpoint_test.go +++ b/consul/catalog_endpoint_test.go @@ -30,6 +30,9 @@ func TestCatalogRegister(t *testing.T) { Tags: []string{"master"}, Port: 8000, }, + Check: &structs.HealthCheck{ + ServiceID: "db", + }, } var out struct{} @@ -46,7 +49,7 @@ func TestCatalogRegister(t *testing.T) { }) } -func TestCatalogRegister_ACLDeny_Service(t *testing.T) { +func TestCatalogRegister_ACLDeny(t *testing.T) { dir1, s1 := testServerWithConfig(t, func(c *Config) { c.ACLDatacenter = "dc1" c.ACLMasterToken = "root" @@ -115,6 +118,28 @@ func TestCatalogRegister_ACLDeny_Service(t *testing.T) { if err == nil || !strings.Contains(err.Error(), permissionDenied) { t.Fatalf("err: %v", err) } + + // Register a db service using the root token. + argR.Service.Service = "db" + argR.Service.ID = "my-id" + argR.Token = "root" + err = msgpackrpc.CallWithCodec(codec, "Catalog.Register", &argR, &outR) + if err != nil { + t.Fatalf("err: %v", err) + } + + // Prove that we are properly looking up the node services and passing + // that to the ACL helper. We can vet the helper independently in its + // own unit test after this. This is trying to register over the db + // service we created above, which is a check that depends on looking + // at the existing registration data with that service ID. + argR.Service.Service = "foo" + argR.Service.ID = "my-id" + argR.Token = id + err = msgpackrpc.CallWithCodec(codec, "Catalog.Register", &argR, &outR) + if err == nil || !strings.Contains(err.Error(), permissionDenied) { + t.Fatalf("err: %v", err) + } } func TestCatalogRegister_ForwardLeader(t *testing.T) { diff --git a/consul/state/state_store.go b/consul/state/state_store.go index a2c94b7df6..68c786f3a9 100644 --- a/consul/state/state_store.go +++ b/consul/state/state_store.go @@ -438,6 +438,12 @@ func (s *StateStore) ensureRegistrationTxn(tx *memdb.Txn, idx uint64, watches *D } } + // TODO (slackpad) In Consul 0.8 ban checks that don't have the same + // node as the top-level registration. This is just weird to be able to + // update unrelated nodes' checks from in here. In 0.7.2 we banned this + // up in the ACL check since that's guarded behind an opt-in flag until + // Consul 0.8. + // Add the checks, if any. if req.Check != nil { if err := s.ensureCheckTxn(tx, idx, watches, req.Check); err != nil { diff --git a/consul/structs/structs.go b/consul/structs/structs.go index 9c28077c38..30b0be378c 100644 --- a/consul/structs/structs.go +++ b/consul/structs/structs.go @@ -183,6 +183,25 @@ func (r *RegisterRequest) RequestDatacenter() string { return r.Datacenter } +// ChangesNode returns true if the given register request changes the given +// node, which can be nil. This only looks for changes to the node record itself, +// not any of the health checks. +func (r *RegisterRequest) ChangesNode(node *Node) bool { + // This means it's creating the node. + if node == nil { + return true + } + + // Check if any of the node-level fields are being changed. + if r.Node != node.Node || + r.Address != node.Address || + !reflect.DeepEqual(r.TaggedAddresses, node.TaggedAddresses) { + return true + } + + return false +} + // DeregisterRequest is used for the Catalog.Deregister endpoint // to deregister a node as providing a service. If no service is // provided the entire node is deregistered. diff --git a/consul/structs/structs_test.go b/consul/structs/structs_test.go index 11c5b2aea3..efceeace3f 100644 --- a/consul/structs/structs_test.go +++ b/consul/structs/structs_test.go @@ -105,6 +105,44 @@ func TestStructs_ACL_IsSame(t *testing.T) { check(func() { other.Rules = "" }, func() { other.Rules = "service \"\" { policy = \"read\" }" }) } +func TestStructs_RegisterRequest_ChangesNode(t *testing.T) { + req := &RegisterRequest{ + Node: "test", + Address: "127.0.0.1", + TaggedAddresses: make(map[string]string), + } + + node := &Node{ + Node: "test", + Address: "127.0.0.1", + TaggedAddresses: make(map[string]string), + } + + check := func(twiddle, restore func()) { + if req.ChangesNode(node) { + t.Fatalf("should not change") + } + + twiddle() + if !req.ChangesNode(node) { + t.Fatalf("should change") + } + + restore() + if req.ChangesNode(node) { + t.Fatalf("should not change") + } + } + + check(func() { req.Node = "nope" }, func() { req.Node = "test" }) + check(func() { req.Address = "127.0.0.2" }, func() { req.Address = "127.0.0.1" }) + check(func() { req.TaggedAddresses["wan"] = "nope" }, func() { delete(req.TaggedAddresses, "wan") }) + + if !req.ChangesNode(nil) { + t.Fatalf("should change") + } +} + // testServiceNode gives a fully filled out ServiceNode instance. func testServiceNode() *ServiceNode { return &ServiceNode{ From 9b7564490c4ce919e94c5afe6fe5e52e901deb53 Mon Sep 17 00:00:00 2001 From: James Phillips Date: Fri, 9 Dec 2016 19:15:44 -0800 Subject: [PATCH 6/9] Adds complete ACL coverage for /v1/catalog/deregister. This included some state store helpers to make this more efficient. --- consul/acl.go | 50 +++++- consul/acl_test.go | 105 +++++++++++++ consul/catalog_endpoint.go | 36 ++++- consul/catalog_endpoint_test.go | 253 ++++++++++++++++++++++++++++--- consul/fsm.go | 4 +- consul/state/state_store.go | 47 +++++- consul/state/state_store_test.go | 35 ++++- 7 files changed, 498 insertions(+), 32 deletions(-) diff --git a/consul/acl.go b/consul/acl.go index cc3b35cf3d..d622e5ef2a 100644 --- a/consul/acl.go +++ b/consul/acl.go @@ -548,7 +548,8 @@ func (s *Server) filterACL(token string, subj interface{}) error { // vetRegisterWithACL applies the given ACL's policy to the catalog update and // determines if it is allowed. Since the catalog register request is so // dynamic, this is a pretty complex algorithm and was worth breaking out of the -// endpoint. +// endpoint. The NodeServices record for the node must be supplied, and can be +// nil. // // This is a bit racy because we have to check the state store outside of a // transaction. It's the best we can do because we don't want to flow ACL @@ -558,7 +559,8 @@ func (s *Server) filterACL(token string, subj interface{}) error { // address this race better (even then it would be super rare, and would at // worst let a service update revert a recent node update, so it doesn't open up // too much abuse). -func vetRegisterWithACL(acl acl.ACL, subj *structs.RegisterRequest, ns *structs.NodeServices) error { +func vetRegisterWithACL(acl acl.ACL, subj *structs.RegisterRequest, + ns *structs.NodeServices) error { // Fast path if ACLs are not enabled. if acl == nil { return nil @@ -644,3 +646,47 @@ func vetRegisterWithACL(acl acl.ACL, subj *structs.RegisterRequest, ns *structs. return nil } + +// vetDeregisterWithACL applies the given ACL's policy to the catalog update and +// determines if it is allowed. Since the catalog deregister request is so +// dynamic, this is a pretty complex algorithm and was worth breaking out of the +// endpoint. The NodeService for the referenced service must be supplied, and can +// be nil; similar for the HealthCheck for the referenced health check. +func vetDeregisterWithACL(acl acl.ACL, subj *structs.DeregisterRequest, + ns *structs.NodeService, nc *structs.HealthCheck) error { + // Fast path if ACLs are not enabled. + if acl == nil { + return nil + } + + // This order must match the code in applyRegister() in fsm.go since it + // also evaluates things in this order, and will ignore fields based on + // this precedence. This lets us also ignore them from an ACL perspective. + if subj.ServiceID != "" { + if ns == nil { + return fmt.Errorf("Unknown service '%s'", subj.ServiceID) + } + if !acl.ServiceWrite(ns.Service) { + return permissionDeniedErr + } + } else if subj.CheckID != "" { + if nc == nil { + return fmt.Errorf("Unknown check '%s'", subj.CheckID) + } + if nc.ServiceID != "" { + if !acl.ServiceWrite(nc.ServiceName) { + return permissionDeniedErr + } + } else { + if !acl.NodeWrite(subj.Node) { + return permissionDeniedErr + } + } + } else { + if !acl.NodeWrite(subj.Node) { + return permissionDeniedErr + } + } + + return nil +} diff --git a/consul/acl_test.go b/consul/acl_test.go index 1d36aaa1bb..d1baa1a8d4 100644 --- a/consul/acl_test.go +++ b/consul/acl_test.go @@ -1341,3 +1341,108 @@ node "node" { t.Fatalf("bad: %v", err) } } + +func TestACL_vetDeregisterWithACL(t *testing.T) { + args := &structs.DeregisterRequest{ + Node: "nope", + } + + // With a nil ACL, the update should be allowed. + if err := vetDeregisterWithACL(nil, args, nil, nil); err != nil { + t.Fatalf("err: %v", err) + } + + // Create a basic node policy. + policy, err := acl.Parse(` +node "node" { + policy = "write" +} +service "service" { + policy = "write" +} +`) + if err != nil { + t.Fatalf("err %v", err) + } + perms, err := acl.New(acl.DenyAll(), policy) + if err != nil { + t.Fatalf("err: %v", err) + } + + // With that policy, the update should now be blocked for node reasons. + err = vetDeregisterWithACL(perms, args, nil, nil) + if err == nil || !strings.Contains(err.Error(), permissionDenied) { + t.Fatalf("bad: %v", err) + } + + // Now use a permitted node name. + args.Node = "node" + if err := vetDeregisterWithACL(perms, args, nil, nil); err != nil { + t.Fatalf("err: %v", err) + } + + // Try an unknown check. + args.CheckID = "check-id" + err = vetDeregisterWithACL(perms, args, nil, nil) + if err == nil || !strings.Contains(err.Error(), "Unknown check") { + t.Fatalf("bad: %v", err) + } + + // Now pass in a check that should be blocked. + nc := &structs.HealthCheck{ + Node: "node", + CheckID: "check-id", + ServiceID: "service-id", + ServiceName: "nope", + } + err = vetDeregisterWithACL(perms, args, nil, nc) + if err == nil || !strings.Contains(err.Error(), permissionDenied) { + t.Fatalf("bad: %v", err) + } + + // Change it to an allowed service, which should go through. + nc.ServiceName = "service" + if err := vetDeregisterWithACL(perms, args, nil, nc); err != nil { + t.Fatalf("err: %v", err) + } + + // Switch to a node check that should be blocked. + args.Node = "nope" + nc.Node = "nope" + nc.ServiceID = "" + nc.ServiceName = "" + err = vetDeregisterWithACL(perms, args, nil, nc) + if err == nil || !strings.Contains(err.Error(), permissionDenied) { + t.Fatalf("bad: %v", err) + } + + // Switch to an allowed node check, which should go through. + args.Node = "node" + nc.Node = "node" + if err := vetDeregisterWithACL(perms, args, nil, nc); err != nil { + t.Fatalf("err: %v", err) + } + + // Try an unknown service. + args.ServiceID = "service-id" + err = vetDeregisterWithACL(perms, args, nil, nil) + if err == nil || !strings.Contains(err.Error(), "Unknown service") { + t.Fatalf("bad: %v", err) + } + + // Now pass in a service that should be blocked. + ns := &structs.NodeService{ + ID: "service-id", + Service: "nope", + } + err = vetDeregisterWithACL(perms, args, ns, nil) + if err == nil || !strings.Contains(err.Error(), permissionDenied) { + t.Fatalf("bad: %v", err) + } + + // Change it to an allowed service, which should go through. + ns.Service = "service" + if err := vetDeregisterWithACL(perms, args, ns, nil); err != nil { + t.Fatalf("err: %v", err) + } +} diff --git a/consul/catalog_endpoint.go b/consul/catalog_endpoint.go index 6c49417580..ce61547b3a 100644 --- a/consul/catalog_endpoint.go +++ b/consul/catalog_endpoint.go @@ -71,7 +71,7 @@ func (c *Catalog) Register(args *structs.RegisterRequest, reply *struct{}) error } // Check the complete register request against the given ACL policy. - if c.srv.config.ACLEnforceVersion8 { + if acl != nil && c.srv.config.ACLEnforceVersion8 { state := c.srv.fsm.State() _, ns, err := state.NodeServices(args.Node) if err != nil { @@ -84,7 +84,6 @@ func (c *Catalog) Register(args *structs.RegisterRequest, reply *struct{}) error _, err = c.srv.raftApply(structs.RegisterRequestType, args) if err != nil { - c.srv.logger.Printf("[ERR] consul.catalog: Register failed: %v", err) return err } @@ -103,9 +102,38 @@ func (c *Catalog) Deregister(args *structs.DeregisterRequest, reply *struct{}) e return fmt.Errorf("Must provide node") } - _, err := c.srv.raftApply(structs.DeregisterRequestType, args) + // Fetch the ACL token, if any. + acl, err := c.srv.resolveToken(args.Token) if err != nil { - c.srv.logger.Printf("[ERR] consul.catalog: Deregister failed: %v", err) + return err + } + + // Check the complete deregister request against the given ACL policy. + if acl != nil && c.srv.config.ACLEnforceVersion8 { + state := c.srv.fsm.State() + + var ns *structs.NodeService + if args.ServiceID != "" { + _, ns, err = state.NodeService(args.Node, args.ServiceID) + if err != nil { + return fmt.Errorf("Service lookup failed: %v", err) + } + } + + var nc *structs.HealthCheck + if args.CheckID != "" { + _, nc, err = state.NodeCheck(args.Node, args.CheckID) + if err != nil { + return fmt.Errorf("Check lookup failed: %v", err) + } + } + + if err := vetDeregisterWithACL(acl, args, ns, nc); err != nil { + return err + } + } + + if _, err := c.srv.raftApply(structs.DeregisterRequestType, args); err != nil { return err } return nil diff --git a/consul/catalog_endpoint_test.go b/consul/catalog_endpoint_test.go index 20c5efdd0d..bc01084d3a 100644 --- a/consul/catalog_endpoint_test.go +++ b/consul/catalog_endpoint_test.go @@ -63,22 +63,25 @@ func TestCatalogRegister_ACLDeny(t *testing.T) { testutil.WaitForLeader(t, s1.RPC, "dc1") - // Create the ACL + // Create the ACL. arg := structs.ACLRequest{ Datacenter: "dc1", Op: structs.ACLSet, ACL: structs.ACL{ - Name: "User token", - Type: structs.ACLTypeClient, - Rules: testRegisterRules, + Name: "User token", + Type: structs.ACLTypeClient, + Rules: ` +service "foo" { + policy = "write" +} +`, }, WriteRequest: structs.WriteRequest{Token: "root"}, } - var out string - if err := msgpackrpc.CallWithCodec(codec, "ACL.Apply", &arg, &out); err != nil { + var id string + if err := msgpackrpc.CallWithCodec(codec, "ACL.Apply", &arg, &id); err != nil { t.Fatalf("err: %v", err) } - id := out argR := structs.RegisterRequest{ Datacenter: "dc1", @@ -93,18 +96,22 @@ func TestCatalogRegister_ACLDeny(t *testing.T) { } var outR struct{} + // This should fail since we are writing to the "db" service, which isn't + // allowed. err := msgpackrpc.CallWithCodec(codec, "Catalog.Register", &argR, &outR) if err == nil || !strings.Contains(err.Error(), permissionDenied) { t.Fatalf("err: %v", err) } + // The "foo" service should work, though. argR.Service.Service = "foo" err = msgpackrpc.CallWithCodec(codec, "Catalog.Register", &argR, &outR) if err != nil { t.Fatalf("err: %v", err) } - // Try the special case for the "consul" service. + // Try the special case for the "consul" service that allows it no matter + // what with pre-version 8 ACL enforcement. argR.Service.Service = "consul" err = msgpackrpc.CallWithCodec(codec, "Catalog.Register", &argR, &outR) if err != nil { @@ -132,7 +139,8 @@ func TestCatalogRegister_ACLDeny(t *testing.T) { // that to the ACL helper. We can vet the helper independently in its // own unit test after this. This is trying to register over the db // service we created above, which is a check that depends on looking - // at the existing registration data with that service ID. + // at the existing registration data with that service ID. This is a new + // check for version 8. argR.Service.Service = "foo" argR.Service.ID = "my-id" argR.Token = id @@ -250,6 +258,217 @@ func TestCatalogDeregister(t *testing.T) { } } +func TestCatalogDeregister_ACLDeny(t *testing.T) { + dir1, s1 := testServerWithConfig(t, func(c *Config) { + c.ACLDatacenter = "dc1" + c.ACLMasterToken = "root" + c.ACLDefaultPolicy = "deny" + c.ACLEnforceVersion8 = false + }) + defer os.RemoveAll(dir1) + defer s1.Shutdown() + codec := rpcClient(t, s1) + defer codec.Close() + + testutil.WaitForLeader(t, s1.RPC, "dc1") + + // Create the ACL. + arg := structs.ACLRequest{ + Datacenter: "dc1", + Op: structs.ACLSet, + ACL: structs.ACL{ + Name: "User token", + Type: structs.ACLTypeClient, + Rules: ` +node "node" { + policy = "write" +} + +service "service" { + policy = "write" +} +`, + }, + WriteRequest: structs.WriteRequest{Token: "root"}, + } + var id string + if err := msgpackrpc.CallWithCodec(codec, "ACL.Apply", &arg, &id); err != nil { + t.Fatalf("err: %v", err) + } + + // Register a node, node check, service, and service check. + argR := structs.RegisterRequest{ + Datacenter: "dc1", + Node: "node", + Address: "127.0.0.1", + Service: &structs.NodeService{ + Service: "service", + Port: 8000, + }, + Checks: structs.HealthChecks{ + &structs.HealthCheck{ + Node: "node", + CheckID: "node-check", + }, + &structs.HealthCheck{ + Node: "node", + CheckID: "service-check", + ServiceID: "service", + }, + }, + WriteRequest: structs.WriteRequest{Token: id}, + } + var outR struct{} + if err := msgpackrpc.CallWithCodec(codec, "Catalog.Register", &argR, &outR); err != nil { + t.Fatalf("err: %v", err) + } + + // First pass with version 8 ACL enforcement disabled, we should be able + // to deregister everything even without a token. + var err error + var out struct{} + err = msgpackrpc.CallWithCodec(codec, "Catalog.Deregister", + &structs.DeregisterRequest{ + Datacenter: "dc1", + Node: "node", + CheckID: "service-check"}, &out) + if err != nil { + t.Fatalf("err: %v", err) + } + err = msgpackrpc.CallWithCodec(codec, "Catalog.Deregister", + &structs.DeregisterRequest{ + Datacenter: "dc1", + Node: "node", + CheckID: "node-check"}, &out) + if err != nil { + t.Fatalf("err: %v", err) + } + err = msgpackrpc.CallWithCodec(codec, "Catalog.Deregister", + &structs.DeregisterRequest{ + Datacenter: "dc1", + Node: "node", + ServiceID: "service"}, &out) + if err != nil { + t.Fatalf("err: %v", err) + } + err = msgpackrpc.CallWithCodec(codec, "Catalog.Deregister", + &structs.DeregisterRequest{ + Datacenter: "dc1", + Node: "node"}, &out) + if err != nil { + t.Fatalf("err: %v", err) + } + + // Turn on version 8 ACL enforcement and put the catalog entry back. + s1.config.ACLEnforceVersion8 = true + if err := msgpackrpc.CallWithCodec(codec, "Catalog.Register", &argR, &outR); err != nil { + t.Fatalf("err: %v", err) + } + + // Second pass with version 8 ACL enforcement enabled, these should all + // get rejected. + err = msgpackrpc.CallWithCodec(codec, "Catalog.Deregister", + &structs.DeregisterRequest{ + Datacenter: "dc1", + Node: "node", + CheckID: "service-check"}, &out) + if err == nil || !strings.Contains(err.Error(), permissionDenied) { + t.Fatalf("err: %v", err) + } + err = msgpackrpc.CallWithCodec(codec, "Catalog.Deregister", + &structs.DeregisterRequest{ + Datacenter: "dc1", + Node: "node", + CheckID: "node-check"}, &out) + if err == nil || !strings.Contains(err.Error(), permissionDenied) { + t.Fatalf("err: %v", err) + } + err = msgpackrpc.CallWithCodec(codec, "Catalog.Deregister", + &structs.DeregisterRequest{ + Datacenter: "dc1", + Node: "node", + ServiceID: "service"}, &out) + if err == nil || !strings.Contains(err.Error(), permissionDenied) { + t.Fatalf("err: %v", err) + } + err = msgpackrpc.CallWithCodec(codec, "Catalog.Deregister", + &structs.DeregisterRequest{ + Datacenter: "dc1", + Node: "node"}, &out) + if err == nil || !strings.Contains(err.Error(), permissionDenied) { + t.Fatalf("err: %v", err) + } + + // Third pass these should all go through with the token set. + err = msgpackrpc.CallWithCodec(codec, "Catalog.Deregister", + &structs.DeregisterRequest{ + Datacenter: "dc1", + Node: "node", + CheckID: "service-check", + WriteRequest: structs.WriteRequest{ + Token: id, + }}, &out) + if err != nil { + t.Fatalf("err: %v", err) + } + err = msgpackrpc.CallWithCodec(codec, "Catalog.Deregister", + &structs.DeregisterRequest{ + Datacenter: "dc1", + Node: "node", + CheckID: "node-check", + WriteRequest: structs.WriteRequest{ + Token: id, + }}, &out) + if err != nil { + t.Fatalf("err: %v", err) + } + err = msgpackrpc.CallWithCodec(codec, "Catalog.Deregister", + &structs.DeregisterRequest{ + Datacenter: "dc1", + Node: "node", + ServiceID: "service", + WriteRequest: structs.WriteRequest{ + Token: id, + }}, &out) + if err != nil { + t.Fatalf("err: %v", err) + } + err = msgpackrpc.CallWithCodec(codec, "Catalog.Deregister", + &structs.DeregisterRequest{ + Datacenter: "dc1", + Node: "node", + WriteRequest: structs.WriteRequest{ + Token: id, + }}, &out) + if err != nil { + t.Fatalf("err: %v", err) + } + + // Try a few error cases. + err = msgpackrpc.CallWithCodec(codec, "Catalog.Deregister", + &structs.DeregisterRequest{ + Datacenter: "dc1", + Node: "node", + ServiceID: "nope", + WriteRequest: structs.WriteRequest{ + Token: id, + }}, &out) + if err == nil || !strings.Contains(err.Error(), "Unknown service") { + t.Fatalf("err: %v", err) + } + err = msgpackrpc.CallWithCodec(codec, "Catalog.Deregister", + &structs.DeregisterRequest{ + Datacenter: "dc1", + Node: "node", + CheckID: "nope", + WriteRequest: structs.WriteRequest{ + Token: id, + }}, &out) + if err == nil || !strings.Contains(err.Error(), "Unknown check") { + t.Fatalf("err: %v", err) + } +} + func TestCatalogListDatacenters(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) @@ -1083,9 +1302,13 @@ func testACLFilterServer(t *testing.T) (dir, token string, srv *Server, codec rp Datacenter: "dc1", Op: structs.ACLSet, ACL: structs.ACL{ - Name: "User token", - Type: structs.ACLTypeClient, - Rules: testRegisterRules, + Name: "User token", + Type: structs.ACLTypeClient, + Rules: ` +service "foo" { + policy = "write" +} +`, }, WriteRequest: structs.WriteRequest{Token: "root"}, } @@ -1230,9 +1453,3 @@ func TestCatalog_NodeServices_FilterACL(t *testing.T) { t.Fatalf("bad: %#v", reply.NodeServices) } } - -var testRegisterRules = ` -service "foo" { - policy = "write" -} -` diff --git a/consul/fsm.go b/consul/fsm.go index 6694b87f79..8e098eb3e1 100644 --- a/consul/fsm.go +++ b/consul/fsm.go @@ -127,7 +127,9 @@ func (c *consulFSM) applyDeregister(buf []byte, index uint64) interface{} { panic(fmt.Errorf("failed to decode request: %v", err)) } - // Either remove the service entry or the whole node + // Either remove the service entry or the whole node. The precedence + // here is also baked into vetDeregisterWithACL() in acl.go, so if you + // make changes here, be sure to also adjust the code over there. if req.ServiceID != "" { if err := c.state.DeleteService(index, req.Node, req.ServiceID); err != nil { c.logger.Printf("[INFO] consul.fsm: DeleteNodeService failed: %v", err) diff --git a/consul/state/state_store.go b/consul/state/state_store.go index 68c786f3a9..3316e4f77e 100644 --- a/consul/state/state_store.go +++ b/consul/state/state_store.go @@ -349,9 +349,9 @@ func (s *StateStore) getWatchTables(method string) []string { return []string{"nodes"} case "Services": return []string{"services"} - case "ServiceNodes", "NodeServices": + case "NodeService", "NodeServices", "ServiceNodes": return []string{"nodes", "services"} - case "NodeChecks", "ServiceChecks", "ChecksInState": + case "NodeCheck", "NodeChecks", "ServiceChecks", "ChecksInState": return []string{"checks"} case "CheckServiceNodes", "NodeInfo", "NodeDump": return []string{"nodes", "services", "checks"} @@ -860,6 +860,28 @@ func (s *StateStore) parseServiceNodes(tx *memdb.Txn, services structs.ServiceNo return results, nil } +// NodeService is used to retrieve a specific service associated with the given +// node. +func (s *StateStore) NodeService(nodeID string, serviceID string) (uint64, *structs.NodeService, error) { + tx := s.db.Txn(false) + defer tx.Abort() + + // Get the table index. + idx := maxIndexTxn(tx, s.getWatchTables("NodeService")...) + + // Query the service + service, err := tx.First("services", "id", nodeID, serviceID) + if err != nil { + return 0, nil, fmt.Errorf("failed querying service for node %q: %s", nodeID, err) + } + + if service != nil { + return idx, service.(*structs.ServiceNode).ToNodeService(), nil + } else { + return idx, nil, nil + } +} + // NodeServices is used to query service registrations by node ID. func (s *StateStore) NodeServices(nodeID string) (uint64, *structs.NodeServices, error) { tx := s.db.Txn(false) @@ -1062,6 +1084,27 @@ func (s *StateStore) ensureCheckTxn(tx *memdb.Txn, idx uint64, watches *DumbWatc return nil } +// NodeCheck is used to retrieve a specific check associated with the given +// node. +func (s *StateStore) NodeCheck(nodeID string, checkID types.CheckID) (uint64, *structs.HealthCheck, error) { + tx := s.db.Txn(false) + defer tx.Abort() + + // Get the table index. + idx := maxIndexTxn(tx, s.getWatchTables("NodeCheck")...) + + // Return the check. + check, err := tx.First("checks", "id", nodeID, string(checkID)) + if err != nil { + return 0, nil, fmt.Errorf("failed check lookup: %s", err) + } + if check != nil { + return idx, check.(*structs.HealthCheck), nil + } else { + return idx, nil, nil + } +} + // NodeChecks is used to retrieve checks associated with the // given node from the state store. func (s *StateStore) NodeChecks(nodeID string) (uint64, structs.HealthChecks, error) { diff --git a/consul/state/state_store_test.go b/consul/state/state_store_test.go index 246dfbc80b..1354d8920e 100644 --- a/consul/state/state_store_test.go +++ b/consul/state/state_store_test.go @@ -284,11 +284,24 @@ func TestStateStore_EnsureRegistration(t *testing.T) { if len(out.Services) != 1 { t.Fatalf("bad: %#v", out.Services) } - s := out.Services["redis1"] - if s.ID != "redis1" || s.Service != "redis" || - s.Address != "1.1.1.1" || s.Port != 8080 || - s.CreateIndex != created || s.ModifyIndex != modified { - t.Fatalf("bad service returned: %#v", s) + r := out.Services["redis1"] + if r == nil || r.ID != "redis1" || r.Service != "redis" || + r.Address != "1.1.1.1" || r.Port != 8080 || + r.CreateIndex != created || r.ModifyIndex != modified { + t.Fatalf("bad service returned: %#v", r) + } + + idx, r, err = s.NodeService("node1", "redis1") + if err != nil { + t.Fatalf("err: %s", err) + } + if idx != modified { + t.Fatalf("bad index: %d", idx) + } + if r == nil || r.ID != "redis1" || r.Service != "redis" || + r.Address != "1.1.1.1" || r.Port != 8080 || + r.CreateIndex != created || r.ModifyIndex != modified { + t.Fatalf("bad service returned: %#v", r) } } verifyNode(1, 2) @@ -321,6 +334,18 @@ func TestStateStore_EnsureRegistration(t *testing.T) { c.CreateIndex != created || c.ModifyIndex != modified { t.Fatalf("bad check returned: %#v", c) } + + idx, c, err = s.NodeCheck("node1", "check1") + if err != nil { + t.Fatalf("err: %s", err) + } + if idx != modified { + t.Fatalf("bad index: %d", idx) + } + if c.Node != "node1" || c.CheckID != "check1" || c.Name != "check" || + c.CreateIndex != created || c.ModifyIndex != modified { + t.Fatalf("bad check returned: %#v", c) + } } verifyNode(1, 3) verifyService(2, 3) From 9d6567ff083a279d6a4f77b7e14d5b4e9507f7cb Mon Sep 17 00:00:00 2001 From: James Phillips Date: Fri, 9 Dec 2016 21:04:00 -0800 Subject: [PATCH 7/9] Adds complete ACL coverage for /v1/catalog/node/. --- consul/catalog_endpoint.go | 15 +++++++++ consul/catalog_endpoint_test.go | 60 ++++++++++++++++++++++++++++++++- 2 files changed, 74 insertions(+), 1 deletion(-) diff --git a/consul/catalog_endpoint.go b/consul/catalog_endpoint.go index ce61547b3a..a943aa91cd 100644 --- a/consul/catalog_endpoint.go +++ b/consul/catalog_endpoint.go @@ -267,6 +267,21 @@ func (c *Catalog) NodeServices(args *structs.NodeSpecificRequest, reply *structs if err != nil { return err } + + // Node read access is required with version 8 ACLs. We + // just return the same response as if the node doesn't + // exist, which is consistent with how the rest of the + // catalog filtering works and doesn't disclose the node. + acl, err := c.srv.resolveToken(args.Token) + if err != nil { + return err + } + if acl != nil && c.srv.config.ACLEnforceVersion8 { + if !acl.NodeRead(args.Node) { + return permissionDeniedErr + } + } + reply.Index, reply.NodeServices = index, services return c.srv.filterACL(args.Token, reply) }) diff --git a/consul/catalog_endpoint_test.go b/consul/catalog_endpoint_test.go index bc01084d3a..afb41a34b5 100644 --- a/consul/catalog_endpoint_test.go +++ b/consul/catalog_endpoint_test.go @@ -1191,7 +1191,7 @@ func TestCatalogListServiceNodes_DistanceSort(t *testing.T) { } } -func TestCatalogNodeServices(t *testing.T) { +func TestCatalog_NodeServices(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -1424,6 +1424,64 @@ func TestCatalog_ServiceNodes_FilterACL(t *testing.T) { } } +func TestCatalog_NodeServices_ACLDeny(t *testing.T) { + dir1, s1 := testServerWithConfig(t, func(c *Config) { + c.ACLDatacenter = "dc1" + c.ACLMasterToken = "root" + c.ACLDefaultPolicy = "deny" + c.ACLEnforceVersion8 = false + }) + defer os.RemoveAll(dir1) + defer s1.Shutdown() + codec := rpcClient(t, s1) + defer codec.Close() + + testutil.WaitForLeader(t, s1.RPC, "dc1") + + // Prior to version 8, the node policy should be ignored. + args := structs.NodeSpecificRequest{ + Datacenter: "dc1", + Node: s1.config.NodeName, + } + reply := structs.IndexedNodeServices{} + if err := msgpackrpc.CallWithCodec(codec, "Catalog.NodeServices", &args, &reply); err != nil { + t.Fatalf("err: %v", err) + } + + // Now turn on version 8 enforcement and try again. + s1.config.ACLEnforceVersion8 = true + err := msgpackrpc.CallWithCodec(codec, "Catalog.NodeServices", &args, &reply) + if err == nil || !strings.Contains(err.Error(), permissionDenied) { + t.Fatalf("err: %v", err) + } + + // Create an ACL that can read the node. + arg := structs.ACLRequest{ + Datacenter: "dc1", + Op: structs.ACLSet, + ACL: structs.ACL{ + Name: "User token", + Type: structs.ACLTypeClient, + Rules: fmt.Sprintf(` +node "%s" { + policy = "write" +} +`, s1.config.NodeName), + }, + WriteRequest: structs.WriteRequest{Token: "root"}, + } + var id string + if err := msgpackrpc.CallWithCodec(codec, "ACL.Apply", &arg, &id); err != nil { + t.Fatalf("err: %v", err) + } + + // Now try with the token and it will go through. + args.Token = id + if err := msgpackrpc.CallWithCodec(codec, "Catalog.NodeServices", &args, &reply); err != nil { + t.Fatalf("err: %v", err) + } +} + func TestCatalog_NodeServices_FilterACL(t *testing.T) { dir, token, srv, codec := testACLFilterServer(t) defer os.RemoveAll(dir) From 8038f2168479328c6293614115bd7b762337f1aa Mon Sep 17 00:00:00 2001 From: James Phillips Date: Sat, 10 Dec 2016 16:00:11 -0800 Subject: [PATCH 8/9] Adds complete ACL coverage for /v1/catalog/nodes. --- consul/acl.go | 60 +++++++++++---- consul/acl_test.go | 67 ++++++++++++----- consul/catalog_endpoint.go | 3 + consul/catalog_endpoint_test.go | 127 ++++++++++++++++++++++++++------ 4 files changed, 202 insertions(+), 55 deletions(-) diff --git a/consul/acl.go b/consul/acl.go index d622e5ef2a..1ab8f435b7 100644 --- a/consul/acl.go +++ b/consul/acl.go @@ -311,16 +311,21 @@ func (c *aclCache) useACLPolicy(id, authDC string, cached *aclCacheEntry, p *str // aclFilter is used to filter results from our state store based on ACL rules // configured for the provided token. type aclFilter struct { - acl acl.ACL - logger *log.Logger + acl acl.ACL + logger *log.Logger + enforceVersion8 bool } // newAclFilter constructs a new aclFilter. -func newAclFilter(acl acl.ACL, logger *log.Logger) *aclFilter { +func newAclFilter(acl acl.ACL, logger *log.Logger, enforceVersion8 bool) *aclFilter { if logger == nil { logger = log.New(os.Stdout, "", log.LstdFlags) } - return &aclFilter{acl, logger} + return &aclFilter{ + acl: acl, + logger: logger, + enforceVersion8: enforceVersion8, + } } // filterService is used to determine if a service is accessible for an ACL. @@ -432,6 +437,26 @@ func (f *aclFilter) filterNodeDump(dump *structs.NodeDump) { *dump = nd } +// filterNodes is used to filter through all parts of a node list and remove +// elements the provided ACL token cannot access. +func (f *aclFilter) filterNodes(nodes *structs.Nodes) { + if !f.enforceVersion8 { + return + } + + n := *nodes + for i := 0; i < len(n); i++ { + node := n[i].Node + if f.acl.NodeRead(node) { + continue + } + f.logger.Printf("[DEBUG] consul: dropping node %q from result due to ACLs", node) + n = append(n[:i], n[i+1:]...) + i-- + } + *nodes = n +} + // redactPreparedQueryTokens will redact any tokens unless the client has a // management token. This eases the transition to delegated authority over // prepared queries, since it was easy to capture management tokens in Consul @@ -506,31 +531,34 @@ func (s *Server) filterACL(token string, subj interface{}) error { } // Create the filter - filt := newAclFilter(acl, s.logger) + filt := newAclFilter(acl, s.logger, s.config.ACLEnforceVersion8) switch v := subj.(type) { + case *structs.CheckServiceNodes: + filt.filterCheckServiceNodes(v) + + case *structs.IndexedCheckServiceNodes: + filt.filterCheckServiceNodes(&v.Nodes) + case *structs.IndexedHealthChecks: filt.filterHealthChecks(&v.HealthChecks) - case *structs.IndexedServices: - filt.filterServices(v.Services) + case *structs.IndexedNodeDump: + filt.filterNodeDump(&v.Dump) - case *structs.IndexedServiceNodes: - filt.filterServiceNodes(&v.ServiceNodes) + case *structs.IndexedNodes: + filt.filterNodes(&v.Nodes) case *structs.IndexedNodeServices: if v.NodeServices != nil { filt.filterNodeServices(v.NodeServices) } - case *structs.IndexedCheckServiceNodes: - filt.filterCheckServiceNodes(&v.Nodes) + case *structs.IndexedServiceNodes: + filt.filterServiceNodes(&v.ServiceNodes) - case *structs.CheckServiceNodes: - filt.filterCheckServiceNodes(v) - - case *structs.IndexedNodeDump: - filt.filterNodeDump(&v.Dump) + case *structs.IndexedServices: + filt.filterServices(v.Services) case *structs.IndexedPreparedQueries: filt.filterPreparedQueries(&v.Queries) diff --git a/consul/acl_test.go b/consul/acl_test.go index d1baa1a8d4..c9232b39e3 100644 --- a/consul/acl_test.go +++ b/consul/acl_test.go @@ -818,14 +818,14 @@ func TestACL_filterHealthChecks(t *testing.T) { } // Try permissive filtering - filt := newAclFilter(acl.AllowAll(), nil) + filt := newAclFilter(acl.AllowAll(), nil, false) filt.filterHealthChecks(&hc) if len(hc) != 1 { t.Fatalf("bad: %#v", hc) } // Try restrictive filtering - filt = newAclFilter(acl.DenyAll(), nil) + filt = newAclFilter(acl.DenyAll(), nil, false) filt.filterHealthChecks(&hc) if len(hc) != 0 { t.Fatalf("bad: %#v", hc) @@ -840,14 +840,14 @@ func TestACL_filterServices(t *testing.T) { } // Try permissive filtering - filt := newAclFilter(acl.AllowAll(), nil) + filt := newAclFilter(acl.AllowAll(), nil, false) filt.filterServices(services) if len(services) != 2 { t.Fatalf("bad: %#v", services) } // Try restrictive filtering - filt = newAclFilter(acl.DenyAll(), nil) + filt = newAclFilter(acl.DenyAll(), nil, false) filt.filterServices(services) if len(services) != 0 { t.Fatalf("bad: %#v", services) @@ -864,14 +864,14 @@ func TestACL_filterServiceNodes(t *testing.T) { } // Try permissive filtering - filt := newAclFilter(acl.AllowAll(), nil) + filt := newAclFilter(acl.AllowAll(), nil, false) filt.filterServiceNodes(&nodes) if len(nodes) != 1 { t.Fatalf("bad: %#v", nodes) } // Try restrictive filtering - filt = newAclFilter(acl.DenyAll(), nil) + filt = newAclFilter(acl.DenyAll(), nil, false) filt.filterServiceNodes(&nodes) if len(nodes) != 0 { t.Fatalf("bad: %#v", nodes) @@ -893,14 +893,14 @@ func TestACL_filterNodeServices(t *testing.T) { } // Try permissive filtering - filt := newAclFilter(acl.AllowAll(), nil) + filt := newAclFilter(acl.AllowAll(), nil, false) filt.filterNodeServices(&services) if len(services.Services) != 1 { t.Fatalf("bad: %#v", services.Services) } // Try restrictive filtering - filt = newAclFilter(acl.DenyAll(), nil) + filt = newAclFilter(acl.DenyAll(), nil, false) filt.filterNodeServices(&services) if len(services.Services) != 0 { t.Fatalf("bad: %#v", services.Services) @@ -929,7 +929,7 @@ func TestACL_filterCheckServiceNodes(t *testing.T) { } // Try permissive filtering - filt := newAclFilter(acl.AllowAll(), nil) + filt := newAclFilter(acl.AllowAll(), nil, false) filt.filterCheckServiceNodes(&nodes) if len(nodes) != 1 { t.Fatalf("bad: %#v", nodes) @@ -939,7 +939,7 @@ func TestACL_filterCheckServiceNodes(t *testing.T) { } // Try restrictive filtering - filt = newAclFilter(acl.DenyAll(), nil) + filt = newAclFilter(acl.DenyAll(), nil, false) filt.filterCheckServiceNodes(&nodes) if len(nodes) != 0 { t.Fatalf("bad: %#v", nodes) @@ -968,7 +968,7 @@ func TestACL_filterNodeDump(t *testing.T) { } // Try permissive filtering - filt := newAclFilter(acl.AllowAll(), nil) + filt := newAclFilter(acl.AllowAll(), nil, false) filt.filterNodeDump(&dump) if len(dump) != 1 { t.Fatalf("bad: %#v", dump) @@ -981,7 +981,7 @@ func TestACL_filterNodeDump(t *testing.T) { } // Try restrictive filtering - filt = newAclFilter(acl.DenyAll(), nil) + filt = newAclFilter(acl.DenyAll(), nil, false) filt.filterNodeDump(&dump) if len(dump) != 1 { t.Fatalf("bad: %#v", dump) @@ -994,6 +994,39 @@ func TestACL_filterNodeDump(t *testing.T) { } } +func TestACL_filterNodes(t *testing.T) { + // Create a nodes list. + nodes := structs.Nodes{ + &structs.Node{ + Node: "foo", + }, + &structs.Node{ + Node: "bar", + }, + } + + // Try permissive filtering. + filt := newAclFilter(acl.AllowAll(), nil, true) + filt.filterNodes(&nodes) + if len(nodes) != 2 { + t.Fatalf("bad: %#v", nodes) + } + + // Try restrictive filtering but with version 8 enforcement turned off. + filt = newAclFilter(acl.DenyAll(), nil, false) + filt.filterNodes(&nodes) + if len(nodes) != 2 { + t.Fatalf("bad: %#v", nodes) + } + + // Try restrictive filtering with version 8 enforcement turned on. + filt = newAclFilter(acl.DenyAll(), nil, true) + filt.filterNodes(&nodes) + if len(nodes) != 0 { + t.Fatalf("bad: %#v", nodes) + } +} + func TestACL_redactPreparedQueryTokens(t *testing.T) { query := &structs.PreparedQuery{ ID: "f004177f-2c28-83b7-4229-eacc25fe55d1", @@ -1007,7 +1040,7 @@ func TestACL_redactPreparedQueryTokens(t *testing.T) { // Try permissive filtering with a management token. This will allow the // embedded token to be seen. - filt := newAclFilter(acl.ManageAll(), nil) + filt := newAclFilter(acl.ManageAll(), nil, false) filt.redactPreparedQueryTokens(&query) if !reflect.DeepEqual(query, expected) { t.Fatalf("bad: %#v", &query) @@ -1019,7 +1052,7 @@ func TestACL_redactPreparedQueryTokens(t *testing.T) { // Now try permissive filtering with a client token, which should cause // the embedded token to get redacted. - filt = newAclFilter(acl.AllowAll(), nil) + filt = newAclFilter(acl.AllowAll(), nil, false) filt.redactPreparedQueryTokens(&query) expected.Token = redactedToken if !reflect.DeepEqual(query, expected) { @@ -1065,7 +1098,7 @@ func TestACL_filterPreparedQueries(t *testing.T) { // Try permissive filtering with a management token. This will allow the // embedded token to be seen. - filt := newAclFilter(acl.ManageAll(), nil) + filt := newAclFilter(acl.ManageAll(), nil, false) filt.filterPreparedQueries(&queries) if !reflect.DeepEqual(queries, expected) { t.Fatalf("bad: %#v", queries) @@ -1078,7 +1111,7 @@ func TestACL_filterPreparedQueries(t *testing.T) { // Now try permissive filtering with a client token, which should cause // the embedded token to get redacted, and the query with no name to get // filtered out. - filt = newAclFilter(acl.AllowAll(), nil) + filt = newAclFilter(acl.AllowAll(), nil, false) filt.filterPreparedQueries(&queries) expected[2].Token = redactedToken expected = append(structs.PreparedQueries{}, expected[1], expected[2]) @@ -1092,7 +1125,7 @@ func TestACL_filterPreparedQueries(t *testing.T) { } // Now try restrictive filtering. - filt = newAclFilter(acl.DenyAll(), nil) + filt = newAclFilter(acl.DenyAll(), nil, false) filt.filterPreparedQueries(&queries) if len(queries) != 0 { t.Fatalf("bad: %#v", queries) diff --git a/consul/catalog_endpoint.go b/consul/catalog_endpoint.go index a943aa91cd..35d00dd470 100644 --- a/consul/catalog_endpoint.go +++ b/consul/catalog_endpoint.go @@ -169,6 +169,9 @@ func (c *Catalog) ListNodes(args *structs.DCSpecificRequest, reply *structs.Inde } reply.Index, reply.Nodes = index, nodes + if err := c.srv.filterACL(args.Token, reply); err != nil { + return err + } return c.srv.sortNodesByDistanceFrom(args.Source, reply.Nodes) }) } diff --git a/consul/catalog_endpoint_test.go b/consul/catalog_endpoint_test.go index afb41a34b5..76a3e2703c 100644 --- a/consul/catalog_endpoint_test.go +++ b/consul/catalog_endpoint_test.go @@ -14,7 +14,7 @@ import ( "github.com/hashicorp/net-rpc-msgpackrpc" ) -func TestCatalogRegister(t *testing.T) { +func TestCatalog_Register(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -49,7 +49,7 @@ func TestCatalogRegister(t *testing.T) { }) } -func TestCatalogRegister_ACLDeny(t *testing.T) { +func TestCatalog_Register_ACLDeny(t *testing.T) { dir1, s1 := testServerWithConfig(t, func(c *Config) { c.ACLDatacenter = "dc1" c.ACLMasterToken = "root" @@ -150,7 +150,7 @@ service "foo" { } } -func TestCatalogRegister_ForwardLeader(t *testing.T) { +func TestCatalog_Register_ForwardLeader(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -197,7 +197,7 @@ func TestCatalogRegister_ForwardLeader(t *testing.T) { } } -func TestCatalogRegister_ForwardDC(t *testing.T) { +func TestCatalog_Register_ForwardDC(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -233,7 +233,7 @@ func TestCatalogRegister_ForwardDC(t *testing.T) { } } -func TestCatalogDeregister(t *testing.T) { +func TestCatalog_Deregister(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -258,7 +258,7 @@ func TestCatalogDeregister(t *testing.T) { } } -func TestCatalogDeregister_ACLDeny(t *testing.T) { +func TestCatalog_Deregister_ACLDeny(t *testing.T) { dir1, s1 := testServerWithConfig(t, func(c *Config) { c.ACLDatacenter = "dc1" c.ACLMasterToken = "root" @@ -469,7 +469,7 @@ service "service" { } } -func TestCatalogListDatacenters(t *testing.T) { +func TestCatalog_ListDatacenters(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -506,7 +506,7 @@ func TestCatalogListDatacenters(t *testing.T) { } } -func TestCatalogListDatacenters_DistanceSort(t *testing.T) { +func TestCatalog_ListDatacenters_DistanceSort(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -550,7 +550,7 @@ func TestCatalogListDatacenters_DistanceSort(t *testing.T) { } } -func TestCatalogListNodes(t *testing.T) { +func TestCatalog_ListNodes(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -592,7 +592,7 @@ func TestCatalogListNodes(t *testing.T) { } } -func TestCatalogListNodes_StaleRaad(t *testing.T) { +func TestCatalog_ListNodes_StaleRaad(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -660,7 +660,7 @@ func TestCatalogListNodes_StaleRaad(t *testing.T) { } } -func TestCatalogListNodes_ConsistentRead_Fail(t *testing.T) { +func TestCatalog_ListNodes_ConsistentRead_Fail(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -710,7 +710,7 @@ func TestCatalogListNodes_ConsistentRead_Fail(t *testing.T) { } } -func TestCatalogListNodes_ConsistentRead(t *testing.T) { +func TestCatalog_ListNodes_ConsistentRead(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -758,7 +758,7 @@ func TestCatalogListNodes_ConsistentRead(t *testing.T) { } } -func TestCatalogListNodes_DistanceSort(t *testing.T) { +func TestCatalog_ListNodes_DistanceSort(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -846,7 +846,84 @@ func TestCatalogListNodes_DistanceSort(t *testing.T) { } } -func BenchmarkCatalogListNodes(t *testing.B) { +func TestCatalog_ListNodes_ACLFilter(t *testing.T) { + dir1, s1 := testServerWithConfig(t, func(c *Config) { + c.ACLDatacenter = "dc1" + c.ACLMasterToken = "root" + c.ACLDefaultPolicy = "deny" + c.ACLEnforceVersion8 = false + }) + defer os.RemoveAll(dir1) + defer s1.Shutdown() + codec := rpcClient(t, s1) + defer codec.Close() + + testutil.WaitForLeader(t, s1.RPC, "dc1") + + // We scope the reply in each of these since msgpack won't clear out an + // existing slice if the incoming one is nil, so it's best to start + // clean each time. + + // Prior to version 8, the node policy should be ignored. + args := structs.DCSpecificRequest{ + Datacenter: "dc1", + } + { + reply := structs.IndexedNodes{} + if err := msgpackrpc.CallWithCodec(codec, "Catalog.ListNodes", &args, &reply); err != nil { + t.Fatalf("err: %v", err) + } + if len(reply.Nodes) != 1 { + t.Fatalf("bad: %v", reply.Nodes) + } + } + + // Now turn on version 8 enforcement and try again. + s1.config.ACLEnforceVersion8 = true + { + reply := structs.IndexedNodes{} + if err := msgpackrpc.CallWithCodec(codec, "Catalog.ListNodes", &args, &reply); err != nil { + t.Fatalf("err: %v", err) + } + if len(reply.Nodes) != 0 { + t.Fatalf("bad: %v", reply.Nodes) + } + } + + // Create an ACL that can read the node. + arg := structs.ACLRequest{ + Datacenter: "dc1", + Op: structs.ACLSet, + ACL: structs.ACL{ + Name: "User token", + Type: structs.ACLTypeClient, + Rules: fmt.Sprintf(` +node "%s" { + policy = "read" +} +`, s1.config.NodeName), + }, + WriteRequest: structs.WriteRequest{Token: "root"}, + } + var id string + if err := msgpackrpc.CallWithCodec(codec, "ACL.Apply", &arg, &id); err != nil { + t.Fatalf("err: %v", err) + } + + // Now try with the token and it will go through. + args.Token = id + { + reply := structs.IndexedNodes{} + if err := msgpackrpc.CallWithCodec(codec, "Catalog.ListNodes", &args, &reply); err != nil { + t.Fatalf("err: %v", err) + } + if len(reply.Nodes) != 1 { + t.Fatalf("bad: %v", reply.Nodes) + } + } +} + +func Benchmark_Catalog_ListNodes(t *testing.B) { dir1, s1 := testServer(nil) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -869,7 +946,7 @@ func BenchmarkCatalogListNodes(t *testing.B) { } } -func TestCatalogListServices(t *testing.T) { +func TestCatalog_ListServices(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -919,7 +996,7 @@ func TestCatalogListServices(t *testing.T) { } } -func TestCatalogListServices_Blocking(t *testing.T) { +func TestCatalog_ListServices_Blocking(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -977,7 +1054,7 @@ func TestCatalogListServices_Blocking(t *testing.T) { } } -func TestCatalogListServices_Timeout(t *testing.T) { +func TestCatalog_ListServices_Timeout(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -1018,7 +1095,7 @@ func TestCatalogListServices_Timeout(t *testing.T) { } } -func TestCatalogListServices_Stale(t *testing.T) { +func TestCatalog_ListServices_Stale(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -1055,7 +1132,7 @@ func TestCatalogListServices_Stale(t *testing.T) { } } -func TestCatalogListServiceNodes(t *testing.T) { +func TestCatalog_ListServiceNodes(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -1104,7 +1181,7 @@ func TestCatalogListServiceNodes(t *testing.T) { } } -func TestCatalogListServiceNodes_DistanceSort(t *testing.T) { +func TestCatalog_ListServiceNodes_DistanceSort(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -1241,7 +1318,7 @@ func TestCatalog_NodeServices(t *testing.T) { } // Used to check for a regression against a known bug -func TestCatalogRegister_FailedCase1(t *testing.T) { +func TestCatalog_Register_FailedCase1(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -1447,6 +1524,9 @@ func TestCatalog_NodeServices_ACLDeny(t *testing.T) { if err := msgpackrpc.CallWithCodec(codec, "Catalog.NodeServices", &args, &reply); err != nil { t.Fatalf("err: %v", err) } + if reply.NodeServices == nil { + t.Fatalf("should not be nil") + } // Now turn on version 8 enforcement and try again. s1.config.ACLEnforceVersion8 = true @@ -1464,7 +1544,7 @@ func TestCatalog_NodeServices_ACLDeny(t *testing.T) { Type: structs.ACLTypeClient, Rules: fmt.Sprintf(` node "%s" { - policy = "write" + policy = "read" } `, s1.config.NodeName), }, @@ -1480,6 +1560,9 @@ node "%s" { if err := msgpackrpc.CallWithCodec(codec, "Catalog.NodeServices", &args, &reply); err != nil { t.Fatalf("err: %v", err) } + if reply.NodeServices == nil { + t.Fatalf("should not be nil") + } } func TestCatalog_NodeServices_FilterACL(t *testing.T) { From 2ace618bf9cf4ec2869857d7e8dfd0246ae166bb Mon Sep 17 00:00:00 2001 From: James Phillips Date: Sun, 11 Dec 2016 13:22:14 -0800 Subject: [PATCH 9/9] Adds complete ACL coverage for /v1/catalog/service/. --- consul/acl.go | 26 ++++++--- consul/acl_test.go | 98 +++++++++++++++++++++++++++------ consul/catalog_endpoint_test.go | 7 +++ 3 files changed, 106 insertions(+), 25 deletions(-) diff --git a/consul/acl.go b/consul/acl.go index 1ab8f435b7..cfbf744f77 100644 --- a/consul/acl.go +++ b/consul/acl.go @@ -328,8 +328,16 @@ func newAclFilter(acl acl.ACL, logger *log.Logger, enforceVersion8 bool) *aclFil } } -// filterService is used to determine if a service is accessible for an ACL. -func (f *aclFilter) filterService(service string) bool { +// allowNode is used to determine if a node is accessible for an ACL. +func (f *aclFilter) allowNode(node string) bool { + if !f.enforceVersion8 { + return true + } + return f.acl.NodeRead(node) +} + +// allowService is used to determine if a service is accessible for an ACL. +func (f *aclFilter) allowService(service string) bool { if service == "" || service == ConsulServiceID { return true } @@ -342,7 +350,7 @@ func (f *aclFilter) filterHealthChecks(checks *structs.HealthChecks) { hc := *checks for i := 0; i < len(hc); i++ { check := hc[i] - if f.filterService(check.ServiceName) { + if f.allowService(check.ServiceName) { continue } f.logger.Printf("[DEBUG] consul: dropping check %q from result due to ACLs", check.CheckID) @@ -355,7 +363,7 @@ func (f *aclFilter) filterHealthChecks(checks *structs.HealthChecks) { // filterServices is used to filter a set of services based on ACLs. func (f *aclFilter) filterServices(services structs.Services) { for svc, _ := range services { - if f.filterService(svc) { + if f.allowService(svc) { continue } f.logger.Printf("[DEBUG] consul: dropping service %q from result due to ACLs", svc) @@ -369,7 +377,7 @@ func (f *aclFilter) filterServiceNodes(nodes *structs.ServiceNodes) { sn := *nodes for i := 0; i < len(sn); i++ { node := sn[i] - if f.filterService(node.ServiceName) { + if f.allowNode(node.Node) && f.allowService(node.ServiceName) { continue } f.logger.Printf("[DEBUG] consul: dropping node %q from result due to ACLs", node.Node) @@ -382,7 +390,7 @@ func (f *aclFilter) filterServiceNodes(nodes *structs.ServiceNodes) { // filterNodeServices is used to filter services on a given node base on ACLs. func (f *aclFilter) filterNodeServices(services *structs.NodeServices) { for svc, _ := range services.Services { - if f.filterService(svc) { + if f.allowService(svc) { continue } f.logger.Printf("[DEBUG] consul: dropping service %q from result due to ACLs", svc) @@ -395,7 +403,7 @@ func (f *aclFilter) filterCheckServiceNodes(nodes *structs.CheckServiceNodes) { csn := *nodes for i := 0; i < len(csn); i++ { node := csn[i] - if f.filterService(node.Service.Service) { + if f.allowService(node.Service.Service) { continue } f.logger.Printf("[DEBUG] consul: dropping node %q from result due to ACLs", node.Node.Node) @@ -415,7 +423,7 @@ func (f *aclFilter) filterNodeDump(dump *structs.NodeDump) { // Filter services for i := 0; i < len(info.Services); i++ { svc := info.Services[i].Service - if f.filterService(svc) { + if f.allowService(svc) { continue } f.logger.Printf("[DEBUG] consul: dropping service %q from result due to ACLs", svc) @@ -426,7 +434,7 @@ func (f *aclFilter) filterNodeDump(dump *structs.NodeDump) { // Filter checks for i := 0; i < len(info.Checks); i++ { chk := info.Checks[i] - if f.filterService(chk.ServiceName) { + if f.allowService(chk.ServiceName) { continue } f.logger.Printf("[DEBUG] consul: dropping check %q from result due to ACLs", chk.CheckID) diff --git a/consul/acl_test.go b/consul/acl_test.go index c9232b39e3..221e09fe97 100644 --- a/consul/acl_test.go +++ b/consul/acl_test.go @@ -855,26 +855,92 @@ func TestACL_filterServices(t *testing.T) { } func TestACL_filterServiceNodes(t *testing.T) { - // Create some service nodes - nodes := structs.ServiceNodes{ - &structs.ServiceNode{ - Node: "node1", - ServiceName: "foo", - }, + // Create some service nodes. + fill := func() structs.ServiceNodes { + return structs.ServiceNodes{ + &structs.ServiceNode{ + Node: "node1", + ServiceName: "foo", + }, + } } - // Try permissive filtering - filt := newAclFilter(acl.AllowAll(), nil, false) - filt.filterServiceNodes(&nodes) - if len(nodes) != 1 { - t.Fatalf("bad: %#v", nodes) + // Try permissive filtering. + { + nodes := fill() + filt := newAclFilter(acl.AllowAll(), nil, false) + filt.filterServiceNodes(&nodes) + if len(nodes) != 1 { + t.Fatalf("bad: %#v", nodes) + } } - // Try restrictive filtering - filt = newAclFilter(acl.DenyAll(), nil, false) - filt.filterServiceNodes(&nodes) - if len(nodes) != 0 { - t.Fatalf("bad: %#v", nodes) + // Try restrictive filtering. + { + nodes := fill() + filt := newAclFilter(acl.DenyAll(), nil, false) + filt.filterServiceNodes(&nodes) + if len(nodes) != 0 { + t.Fatalf("bad: %#v", nodes) + } + } + + // Allowed to see the service but not the node. + policy, err := acl.Parse(` +service "foo" { + policy = "read" +} +`) + if err != nil { + t.Fatalf("err %v", err) + } + perms, err := acl.New(acl.DenyAll(), policy) + if err != nil { + t.Fatalf("err: %v", err) + } + + /// This will work because version 8 ACLs aren't being enforced. + { + nodes := fill() + filt := newAclFilter(perms, nil, false) + filt.filterServiceNodes(&nodes) + if len(nodes) != 1 { + t.Fatalf("bad: %#v", nodes) + } + } + + // But with version 8 the node will block it. + { + nodes := fill() + filt := newAclFilter(perms, nil, true) + filt.filterServiceNodes(&nodes) + if len(nodes) != 0 { + t.Fatalf("bad: %#v", nodes) + } + } + + // Chain on access to the node. + policy, err = acl.Parse(` +node "node1" { + policy = "read" +} +`) + if err != nil { + t.Fatalf("err %v", err) + } + perms, err = acl.New(perms, policy) + if err != nil { + t.Fatalf("err: %v", err) + } + + // Now it should go through. + { + nodes := fill() + filt := newAclFilter(perms, nil, true) + filt.filterServiceNodes(&nodes) + if len(nodes) != 1 { + t.Fatalf("bad: %#v", nodes) + } } } diff --git a/consul/catalog_endpoint_test.go b/consul/catalog_endpoint_test.go index 76a3e2703c..6593fdf848 100644 --- a/consul/catalog_endpoint_test.go +++ b/consul/catalog_endpoint_test.go @@ -1369,6 +1369,7 @@ func testACLFilterServer(t *testing.T) (dir, token string, srv *Server, codec rp c.ACLDatacenter = "dc1" c.ACLMasterToken = "root" c.ACLDefaultPolicy = "deny" + c.ACLEnforceVersion8 = false }) codec = rpcClient(t, srv) @@ -1499,6 +1500,12 @@ func TestCatalog_ServiceNodes_FilterACL(t *testing.T) { t.Fatalf("bad: %#v", reply.ServiceNodes) } } + + // We've already proven that we call the ACL filtering function so we + // test node filtering down in acl.go for node cases. This also proves + // that we respect the version 8 ACL flag, since the test server sets + // that to false (the regression value of *not* changing this is better + // for now until we change the sense of the version 8 ACL flag). } func TestCatalog_NodeServices_ACLDeny(t *testing.T) {