From 376f9694f4b922ffb384a7cb69ee73d487334531 Mon Sep 17 00:00:00 2001 From: Armon Dadgar Date: Sun, 30 Nov 2014 20:12:44 -0700 Subject: [PATCH 1/5] website: Update ACL docs --- .../source/docs/internals/acl.html.markdown | 32 +++++++++++++++++-- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/website/source/docs/internals/acl.html.markdown b/website/source/docs/internals/acl.html.markdown index e2547513e..33994e3ac 100644 --- a/website/source/docs/internals/acl.html.markdown +++ b/website/source/docs/internals/acl.html.markdown @@ -114,6 +114,16 @@ key "foo/private/" { # Deny access to the private dir policy = "deny" } + +# Default all services to allowing registration +service "" { + policy = "write" +} + +service "secure" { + # Deny registration access to secure service + policy = "read" +} ``` This is equivalent to the following JSON input: @@ -122,14 +132,22 @@ This is equivalent to the following JSON input: { "key": { "": { - "policy": "read", + "policy": "read" }, "foo/": { - "policy": "write", + "policy": "write" }, "foo/private": { - "policy": "deny", + "policy": "deny" } + }, + "service": { + "": { + "policy": "write" + }, + "secure": { + "policy": "read" + } } } ``` @@ -139,3 +157,11 @@ using a longest-prefix match policy. This means we pick the most specific policy possible. The policy is either "read", "write" or "deny". A "write" policy implies "read", and there is no way to specify write-only. If there is no applicable rule, the `acl_default_policy` is applied. + +Services policies provide both a service name and a policy. The rules are +enforced using an exact match policy. The default rule is provided using +the empty string. The policy is either "read", "write", or "deny". A "write" +policy implies "read", and there is no way to specify write-only. If there +is no applicable rule, the `acl_default_policy` is applied. Currently, only +the "write" level is enforced for registration of services. + From b9810f774c2c63b76c893f2d087652e00eaf547e Mon Sep 17 00:00:00 2001 From: Armon Dadgar Date: Sun, 30 Nov 2014 20:18:16 -0700 Subject: [PATCH 2/5] acl: Support for service policies --- acl/policy.go | 37 ++++++++++++++++++++++++++++++++----- acl/policy_test.go | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 5 deletions(-) diff --git a/acl/policy.go b/acl/policy.go index 014ef51ac..569570a9d 100644 --- a/acl/policy.go +++ b/acl/policy.go @@ -2,20 +2,25 @@ package acl import ( "fmt" + "github.com/hashicorp/hcl" ) const ( - KeyPolicyDeny = "deny" - KeyPolicyRead = "read" - KeyPolicyWrite = "write" + KeyPolicyDeny = "deny" + KeyPolicyRead = "read" + KeyPolicyWrite = "write" + ServicePolicyDeny = "deny" + ServicePolicyRead = "read" + ServicePolicyWrite = "write" ) // Policy is used to represent the policy specified by // an ACL configuration. type Policy struct { - ID string `hcl:"-"` - Keys []*KeyPolicy `hcl:"key,expand"` + ID string `hcl:"-"` + Keys []*KeyPolicy `hcl:"key,expand"` + Services []*ServicePolicy `hcl:"service,expand"` } // KeyPolicy represents a policy for a key @@ -28,6 +33,16 @@ func (k *KeyPolicy) GoString() string { return fmt.Sprintf("%#v", *k) } +// 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) +} + // Parse is used to parse the specified ACL rules into an // intermediary set of policies, before being compiled into // the ACL @@ -53,5 +68,17 @@ func Parse(rules string) (*Policy, error) { return nil, fmt.Errorf("Invalid key policy: %#v", kp) } } + + // Validate the service policy + for _, sp := range p.Services { + switch sp.Policy { + case ServicePolicyDeny: + case ServicePolicyRead: + case ServicePolicyWrite: + default: + return nil, fmt.Errorf("Invalid service policy: %#v", sp) + } + } + return p, nil } diff --git a/acl/policy_test.go b/acl/policy_test.go index 0fc75e0fa..0c270e7d5 100644 --- a/acl/policy_test.go +++ b/acl/policy_test.go @@ -18,6 +18,12 @@ key "foo/bar/" { } key "foo/bar/baz" { policy = "deny" +} +service "" { + policy = "write" +} +service "foo" { + policy = "read" } ` exp := &Policy{ @@ -39,6 +45,16 @@ key "foo/bar/baz" { Policy: KeyPolicyDeny, }, }, + Services: []*ServicePolicy{ + &ServicePolicy{ + Name: "", + Policy: ServicePolicyWrite, + }, + &ServicePolicy{ + Name: "foo", + Policy: ServicePolicyRead, + }, + }, } out, err := Parse(inp) @@ -66,6 +82,14 @@ func TestParse_JSON(t *testing.T) { "foo/bar/baz": { "policy": "deny" } + }, + "service": { + "": { + "policy": "write" + }, + "foo": { + "policy": "read" + } } }` exp := &Policy{ @@ -87,6 +111,16 @@ func TestParse_JSON(t *testing.T) { Policy: KeyPolicyDeny, }, }, + Services: []*ServicePolicy{ + &ServicePolicy{ + Name: "", + Policy: ServicePolicyWrite, + }, + &ServicePolicy{ + Name: "foo", + Policy: ServicePolicyRead, + }, + }, } out, err := Parse(inp) From 7b8faf4cb3575c26e6fca49e93697297165a518d Mon Sep 17 00:00:00 2001 From: Armon Dadgar Date: Sun, 30 Nov 2014 20:33:46 -0700 Subject: [PATCH 3/5] acl: Expose service policy checks --- acl/acl.go | 69 ++++++++++++++++++++++++++++++++++- acl/acl_test.go | 97 +++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 160 insertions(+), 6 deletions(-) diff --git a/acl/acl.go b/acl/acl.go index 442837340..8a73674d0 100644 --- a/acl/acl.go +++ b/acl/acl.go @@ -46,6 +46,12 @@ 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 + // ACLList checks for permission to list all the ACLs ACLList() bool @@ -73,6 +79,14 @@ 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) ACLList() bool { return s.allowManage } @@ -119,20 +133,29 @@ type PolicyACL struct { // keyRules contains the key policies keyRules *radix.Tree + + // serviceRules contains the service policies + serviceRules map[string]string } // New is used to construct a policy based ACL from a set of policies // and a parent policy to resolve missing cases. func New(parent ACL, policy *Policy) (*PolicyACL, error) { p := &PolicyACL{ - parent: parent, - keyRules: radix.New(), + parent: parent, + keyRules: radix.New(), + serviceRules: make(map[string]string, len(policy.Services)), } // Load the key policy for _, kp := range policy.Keys { p.keyRules.Insert(kp.Prefix, kp.Policy) } + + // Load the service policy + for _, sp := range policy.Services { + p.serviceRules[sp.Name] = sp.Policy + } return p, nil } @@ -205,6 +228,48 @@ 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[name] + if !ok { + rule, ok = p.serviceRules[""] + } + if ok { + switch rule { + case ServicePolicyWrite: + return true + case ServicePolicyRead: + 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[name] + if !ok { + rule, ok = p.serviceRules[""] + } + if ok { + switch rule { + case ServicePolicyWrite: + return true + default: + return false + } + } + + // No matching rule, use the parent. + return p.parent.ServiceWrite(name) +} + // ACLList checks if listing of ACLs is allowed func (p *PolicyACL) ACLList() bool { return p.parent.ACLList() diff --git a/acl/acl_test.go b/acl/acl_test.go index 9be0388db..cecc870b9 100644 --- a/acl/acl_test.go +++ b/acl/acl_test.go @@ -41,6 +41,12 @@ func TestStaticACL(t *testing.T) { 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.ACLList() { t.Fatalf("should not allow") } @@ -54,6 +60,12 @@ func TestStaticACL(t *testing.T) { if none.KeyWrite("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.ACLList() { t.Fatalf("should not noneow") } @@ -67,6 +79,12 @@ func TestStaticACL(t *testing.T) { if !manage.KeyWrite("foobar") { t.Fatalf("should allow") } + if !manage.ServiceRead("foobar") { + t.Fatalf("should allow") + } + if !manage.ServiceWrite("foobar") { + t.Fatalf("should allow") + } if !manage.ACLList() { t.Fatalf("should allow") } @@ -96,19 +114,33 @@ func TestPolicyACL(t *testing.T) { Policy: KeyPolicyRead, }, }, + Services: []*ServicePolicy{ + &ServicePolicy{ + Name: "", + Policy: ServicePolicyWrite, + }, + &ServicePolicy{ + Name: "foo", + Policy: ServicePolicyRead, + }, + &ServicePolicy{ + Name: "bar", + Policy: ServicePolicyDeny, + }, + }, } acl, err := New(all, policy) if err != nil { t.Fatalf("err: %v", err) } - type tcase struct { + type keycase struct { inp string read bool write bool writePrefix bool } - cases := []tcase{ + cases := []keycase{ {"other", true, true, true}, {"foo/test", true, true, true}, {"foo/priv/test", false, false, false}, @@ -128,6 +160,26 @@ func TestPolicyACL(t *testing.T) { t.Fatalf("Write prefix fail: %#v", c) } } + + // Test the services + type servicecase struct { + inp string + read bool + write bool + } + scases := []servicecase{ + {"other", true, true}, + {"foo", true, false}, + {"bar", false, false}, + } + for _, c := range scases { + if c.read != acl.ServiceRead(c.inp) { + t.Fatalf("Read fail: %#v", c) + } + if c.write != acl.ServiceWrite(c.inp) { + t.Fatalf("Write fail: %#v", c) + } + } } func TestPolicyACL_Parent(t *testing.T) { @@ -143,6 +195,16 @@ func TestPolicyACL_Parent(t *testing.T) { Policy: KeyPolicyRead, }, }, + Services: []*ServicePolicy{ + &ServicePolicy{ + Name: "other", + Policy: ServicePolicyWrite, + }, + &ServicePolicy{ + Name: "foo", + Policy: ServicePolicyRead, + }, + }, } root, err := New(deny, policyRoot) if err != nil { @@ -164,19 +226,25 @@ func TestPolicyACL_Parent(t *testing.T) { Policy: KeyPolicyRead, }, }, + Services: []*ServicePolicy{ + &ServicePolicy{ + Name: "bar", + Policy: ServicePolicyDeny, + }, + }, } acl, err := New(root, policy) if err != nil { t.Fatalf("err: %v", err) } - type tcase struct { + type keycase struct { inp string read bool write bool writePrefix bool } - cases := []tcase{ + cases := []keycase{ {"other", false, false, false}, {"foo/test", true, true, true}, {"foo/priv/test", true, false, false}, @@ -194,4 +262,25 @@ func TestPolicyACL_Parent(t *testing.T) { t.Fatalf("Write prefix fail: %#v", c) } } + + // Test the services + type servicecase struct { + inp string + read bool + write bool + } + scases := []servicecase{ + {"fail", false, false}, + {"other", true, true}, + {"foo", true, false}, + {"bar", false, false}, + } + for _, c := range scases { + if c.read != acl.ServiceRead(c.inp) { + t.Fatalf("Read fail: %#v", c) + } + if c.write != acl.ServiceWrite(c.inp) { + t.Fatalf("Write fail: %#v", c) + } + } } From d74f79b3fa021bf9e34be4a315bb6be175556db1 Mon Sep 17 00:00:00 2001 From: Armon Dadgar Date: Sun, 30 Nov 2014 21:05:15 -0700 Subject: [PATCH 4/5] consul: Enforce service registration ACLs --- consul/catalog_endpoint.go | 19 ++++++- consul/catalog_endpoint_test.go | 56 +++++++++++++++++++ consul/leader.go | 8 +++ .../source/docs/internals/acl.html.markdown | 3 +- 4 files changed, 83 insertions(+), 3 deletions(-) diff --git a/consul/catalog_endpoint.go b/consul/catalog_endpoint.go index 9fb982e14..24995fdeb 100644 --- a/consul/catalog_endpoint.go +++ b/consul/catalog_endpoint.go @@ -2,10 +2,11 @@ package consul import ( "fmt" - "github.com/armon/go-metrics" - "github.com/hashicorp/consul/consul/structs" "sort" "time" + + "github.com/armon/go-metrics" + "github.com/hashicorp/consul/consul/structs" ) // Catalog endpoint is used to manipulate the service catalog @@ -35,6 +36,20 @@ func (c *Catalog) Register(args *structs.RegisterRequest, reply *struct{}) error 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. + 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) { + c.srv.logger.Printf("[WARN] consul.catalog: Register of service '%s' on '%s' denied due to ACLs", + args.Service.Service, args.Node) + return permissionDeniedErr + } + } } if args.Check != nil { diff --git a/consul/catalog_endpoint_test.go b/consul/catalog_endpoint_test.go index 4ba76ce01..9c4e86ea9 100644 --- a/consul/catalog_endpoint_test.go +++ b/consul/catalog_endpoint_test.go @@ -45,6 +45,56 @@ func TestCatalogRegister(t *testing.T) { }) } +func TestCatalogRegister_ACLDeny(t *testing.T) { + dir1, s1 := testServerWithConfig(t, func(c *Config) { + c.ACLDatacenter = "dc1" + c.ACLMasterToken = "root" + c.ACLDefaultPolicy = "deny" + c.ACLToken = "root" + }) + defer os.RemoveAll(dir1) + defer s1.Shutdown() + client := rpcClient(t, s1) + defer client.Close() + + testutil.WaitForLeader(t, client.Call, "dc1") + + // Create the ACL + arg := structs.ACLRequest{ + Datacenter: "dc1", + Op: structs.ACLSet, + ACL: structs.ACL{ + Name: "User token", + Type: structs.ACLTypeClient, + Rules: testRegisterRules, + }, + WriteRequest: structs.WriteRequest{Token: "root"}, + } + var out string + if err := client.Call("ACL.Apply", &arg, &out); err != nil { + t.Fatalf("err: %v", err) + } + id := out + + argR := structs.RegisterRequest{ + Datacenter: "dc1", + Node: "foo", + Address: "127.0.0.1", + Service: &structs.NodeService{ + Service: "db", + Tags: []string{"master"}, + Port: 8000, + }, + WriteRequest: structs.WriteRequest{Token: id}, + } + var outR struct{} + + err := client.Call("Catalog.Register", &argR, &outR) + if err == nil || !strings.Contains(err.Error(), permissionDenied) { + t.Fatalf("err: %v", err) + } +} + func TestCatalogRegister_ForwardLeader(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) @@ -722,3 +772,9 @@ func TestCatalogRegister_FailedCase1(t *testing.T) { t.Fatalf("Bad: %v", out2) } } + +var testRegisterRules = ` +service "foo" { + policy = "read" +} +` diff --git a/consul/leader.go b/consul/leader.go index b41f612cb..7f4f378a6 100644 --- a/consul/leader.go +++ b/consul/leader.go @@ -4,6 +4,7 @@ import ( "fmt" "net" "strconv" + "strings" "time" "github.com/armon/go-metrics" @@ -265,6 +266,11 @@ func (s *Server) reconcileMember(member serf.Member) error { if err != nil { s.logger.Printf("[ERR] consul: failed to reconcile member: %v: %v", member, err) + + // Permission denied should not bubble up + if strings.Contains(err.Error(), permissionDenied) { + return nil + } return err } return nil @@ -344,6 +350,7 @@ AFTER_CHECK: Status: structs.HealthPassing, Output: SerfCheckAliveOutput, }, + WriteRequest: structs.WriteRequest{Token: s.config.ACLToken}, } var out struct{} return s.endpoints.Catalog.Register(&req, &out) @@ -379,6 +386,7 @@ func (s *Server) handleFailedMember(member serf.Member) error { Status: structs.HealthCritical, Output: SerfCheckFailedOutput, }, + WriteRequest: structs.WriteRequest{Token: s.config.ACLToken}, } var out struct{} return s.endpoints.Catalog.Register(&req, &out) diff --git a/website/source/docs/internals/acl.html.markdown b/website/source/docs/internals/acl.html.markdown index 33994e3ac..b847e17da 100644 --- a/website/source/docs/internals/acl.html.markdown +++ b/website/source/docs/internals/acl.html.markdown @@ -163,5 +163,6 @@ enforced using an exact match policy. The default rule is provided using the empty string. The policy is either "read", "write", or "deny". A "write" policy implies "read", and there is no way to specify write-only. If there is no applicable rule, the `acl_default_policy` is applied. Currently, only -the "write" level is enforced for registration of services. +the "write" level is enforced for registration of services. The policy for +the "consul" service is always "write" as it is managed internally. From 402d5808632e6b7af5677bcaa272fc346fe7f42b Mon Sep 17 00:00:00 2001 From: Armon Dadgar Date: Sun, 30 Nov 2014 21:10:42 -0700 Subject: [PATCH 5/5] consul: Check that ACL also allows registration --- consul/catalog_endpoint_test.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/consul/catalog_endpoint_test.go b/consul/catalog_endpoint_test.go index 9c4e86ea9..e2674f12b 100644 --- a/consul/catalog_endpoint_test.go +++ b/consul/catalog_endpoint_test.go @@ -50,7 +50,6 @@ func TestCatalogRegister_ACLDeny(t *testing.T) { c.ACLDatacenter = "dc1" c.ACLMasterToken = "root" c.ACLDefaultPolicy = "deny" - c.ACLToken = "root" }) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -93,6 +92,12 @@ func TestCatalogRegister_ACLDeny(t *testing.T) { if err == nil || !strings.Contains(err.Error(), permissionDenied) { t.Fatalf("err: %v", err) } + + argR.Service.Service = "foo" + err = client.Call("Catalog.Register", &argR, &outR) + if err != nil { + t.Fatalf("err: %v", err) + } } func TestCatalogRegister_ForwardLeader(t *testing.T) { @@ -775,6 +780,6 @@ func TestCatalogRegister_FailedCase1(t *testing.T) { var testRegisterRules = ` service "foo" { - policy = "read" + policy = "write" } `