From d5acfc39829fbf81fcb6543d77d0445b9923b631 Mon Sep 17 00:00:00 2001 From: Preetha Appan Date: Mon, 2 Oct 2017 17:10:21 -0500 Subject: [PATCH] Introduces new 'list' permission that applies to KV store recursive reads, and enforced only when opted in. --- acl/acl.go | 27 +++- acl/acl_test.go | 20 ++- acl/policy.go | 3 + agent/agent.go | 3 + agent/config/builder.go | 23 +-- agent/config/config.go | 1 + agent/config/runtime.go | 1 + agent/config/runtime_test.go | 4 + agent/consul/config.go | 5 + agent/consul/kvs_endpoint.go | 22 ++- agent/consul/kvs_endpoint_test.go | 224 ++++++++++++++++++++++++++++-- 11 files changed, 299 insertions(+), 34 deletions(-) diff --git a/acl/acl.go b/acl/acl.go index 3f867dbd9..72a2e78f2 100644 --- a/acl/acl.go +++ b/acl/acl.go @@ -60,6 +60,9 @@ type ACL interface { // EventWrite determines if a specific event may be fired. EventWrite(string) bool + // KeyList checks for permission to list keys under a prefix + KeyList(string) bool + // KeyRead checks for permission to read a given key KeyRead(string) bool @@ -155,6 +158,10 @@ func (s *StaticACL) KeyRead(string) bool { return s.defaultAllow } +func (s *StaticACL) KeyList(string) bool { + return s.defaultAllow +} + func (s *StaticACL) KeyWrite(string, sentinel.ScopeFn) bool { return s.defaultAllow } @@ -455,7 +462,7 @@ func (p *PolicyACL) KeyRead(key string) bool { if ok { pr := rule.(PolicyRule) switch pr.aclPolicy { - case PolicyRead, PolicyWrite: + case PolicyRead, PolicyWrite, PolicyList: return true default: return false @@ -466,6 +473,24 @@ func (p *PolicyACL) KeyRead(key string) bool { return p.parent.KeyRead(key) } +// KeyList returns if a key is allowed to be listed +func (p *PolicyACL) KeyList(key string) bool { + // Look for a matching rule + _, rule, ok := p.keyRules.LongestPrefix(key) + if ok { + pr := rule.(PolicyRule) + switch pr.aclPolicy { + case PolicyList, PolicyWrite: + return true + default: + return false + } + } + + // No matching rule, use the parent. + return p.parent.KeyList(key) +} + // KeyWrite returns if a key is allowed to be written func (p *PolicyACL) KeyWrite(key string, scope sentinel.ScopeFn) bool { // Look for a matching rule diff --git a/acl/acl_test.go b/acl/acl_test.go index c8fbb3d77..02ae0efb4 100644 --- a/acl/acl_test.go +++ b/acl/acl_test.go @@ -268,6 +268,10 @@ func TestPolicyACL(t *testing.T) { Prefix: "zip/", Policy: PolicyRead, }, + &KeyPolicy{ + Prefix: "zap/", + Policy: PolicyList, + }, }, PreparedQueries: []*PreparedQueryPolicy{ &PreparedQueryPolicy{ @@ -316,15 +320,17 @@ func TestPolicyACL(t *testing.T) { read bool write bool writePrefix bool + list bool } cases := []keycase{ - {"other", true, true, true}, - {"foo/test", true, true, true}, - {"foo/priv/test", false, false, false}, - {"bar/any", false, false, false}, - {"zip/test", true, false, false}, - {"foo/", true, true, false}, - {"", true, true, false}, + {"other", true, true, true, true}, + {"foo/test", true, true, true, true}, + {"foo/priv/test", false, false, false, false}, + {"bar/any", false, false, false, false}, + {"zip/test", true, false, false, false}, + {"foo/", true, true, false, true}, + {"", true, true, false, true}, + {"zap/test", true, false, false, true}, } for _, c := range cases { if c.read != acl.KeyRead(c.inp) { diff --git a/acl/policy.go b/acl/policy.go index 9df0020a3..4da26f45b 100644 --- a/acl/policy.go +++ b/acl/policy.go @@ -11,6 +11,7 @@ const ( PolicyDeny = "deny" PolicyRead = "read" PolicyWrite = "write" + PolicyList = "list" ) // Policy is used to represent the policy specified by @@ -118,6 +119,8 @@ func isPolicyValid(policy string) bool { return true case PolicyWrite: return true + case PolicyList: + return true default: return false } diff --git a/agent/agent.go b/agent/agent.go index 7ee709732..c6d3a1a62 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -676,6 +676,9 @@ func (a *Agent) consulConfig() (*consul.Config, error) { if a.config.ACLEnforceVersion8 { base.ACLEnforceVersion8 = a.config.ACLEnforceVersion8 } + if a.config.ACLEnableKeyListPolicy { + base.ACLEnableKeyListPolicy = a.config.ACLEnableKeyListPolicy + } if a.config.SessionTTLMin != 0 { base.SessionTTLMin = a.config.SessionTTLMin } diff --git a/agent/config/builder.go b/agent/config/builder.go index 0bd383e29..d8c33cef3 100644 --- a/agent/config/builder.go +++ b/agent/config/builder.go @@ -481,17 +481,18 @@ func (b *Builder) Build() (rt RuntimeConfig, err error) { ConsulServerHealthInterval: b.durationVal("consul.server.health_interval", c.Consul.Server.HealthInterval), // ACL - ACLAgentMasterToken: b.stringVal(c.ACLAgentMasterToken), - ACLAgentToken: b.stringVal(c.ACLAgentToken), - ACLDatacenter: strings.ToLower(b.stringVal(c.ACLDatacenter)), - ACLDefaultPolicy: b.stringVal(c.ACLDefaultPolicy), - ACLDownPolicy: b.stringVal(c.ACLDownPolicy), - ACLEnforceVersion8: b.boolVal(c.ACLEnforceVersion8), - ACLMasterToken: b.stringVal(c.ACLMasterToken), - ACLReplicationToken: b.stringVal(c.ACLReplicationToken), - ACLTTL: b.durationVal("acl_ttl", c.ACLTTL), - ACLToken: b.stringVal(c.ACLToken), - EnableACLReplication: b.boolVal(c.EnableACLReplication), + ACLAgentMasterToken: b.stringVal(c.ACLAgentMasterToken), + ACLAgentToken: b.stringVal(c.ACLAgentToken), + ACLDatacenter: strings.ToLower(b.stringVal(c.ACLDatacenter)), + ACLDefaultPolicy: b.stringVal(c.ACLDefaultPolicy), + ACLDownPolicy: b.stringVal(c.ACLDownPolicy), + ACLEnforceVersion8: b.boolVal(c.ACLEnforceVersion8), + ACLEnableKeyListPolicy: b.boolVal(c.ACLEnableKeyListPolicy), + ACLMasterToken: b.stringVal(c.ACLMasterToken), + ACLReplicationToken: b.stringVal(c.ACLReplicationToken), + ACLTTL: b.durationVal("acl_ttl", c.ACLTTL), + ACLToken: b.stringVal(c.ACLToken), + EnableACLReplication: b.boolVal(c.EnableACLReplication), // Autopilot AutopilotCleanupDeadServers: b.boolVal(c.Autopilot.CleanupDeadServers), diff --git a/agent/config/config.go b/agent/config/config.go index 1ace6779f..20a291db9 100644 --- a/agent/config/config.go +++ b/agent/config/config.go @@ -128,6 +128,7 @@ type Config struct { ACLDatacenter *string `json:"acl_datacenter,omitempty" hcl:"acl_datacenter" mapstructure:"acl_datacenter"` ACLDefaultPolicy *string `json:"acl_default_policy,omitempty" hcl:"acl_default_policy" mapstructure:"acl_default_policy"` ACLDownPolicy *string `json:"acl_down_policy,omitempty" hcl:"acl_down_policy" mapstructure:"acl_down_policy"` + ACLEnableKeyListPolicy *bool `json:"acl_enable_key_list_policy,omitempty" hcl:"acl_enable_key_list_policy" mapstructure:"acl_enable_key_list_policy"` ACLEnforceVersion8 *bool `json:"acl_enforce_version_8,omitempty" hcl:"acl_enforce_version_8" mapstructure:"acl_enforce_version_8"` ACLMasterToken *string `json:"acl_master_token,omitempty" hcl:"acl_master_token" mapstructure:"acl_master_token"` ACLReplicationToken *string `json:"acl_replication_token,omitempty" hcl:"acl_replication_token" mapstructure:"acl_replication_token"` diff --git a/agent/config/runtime.go b/agent/config/runtime.go index fd8ad4744..28cb13ac8 100644 --- a/agent/config/runtime.go +++ b/agent/config/runtime.go @@ -54,6 +54,7 @@ type RuntimeConfig struct { ACLDefaultPolicy string ACLDownPolicy string ACLEnforceVersion8 bool + ACLEnableKeyListPolicy bool ACLMasterToken string ACLReplicationToken string ACLTTL time.Duration diff --git a/agent/config/runtime_test.go b/agent/config/runtime_test.go index e77c511f4..b63ecc14a 100644 --- a/agent/config/runtime_test.go +++ b/agent/config/runtime_test.go @@ -1932,6 +1932,7 @@ func TestFullConfig(t *testing.T) { "acl_default_policy": "ArK3WIfE", "acl_down_policy": "vZXMfMP0", "acl_enforce_version_8": true, + "acl_enable_key_list_policy": true, "acl_master_token": "C1Q1oIwh", "acl_replication_token": "LMmgy5dO", "acl_token": "O1El0wan", @@ -2347,6 +2348,7 @@ func TestFullConfig(t *testing.T) { acl_default_policy = "ArK3WIfE" acl_down_policy = "vZXMfMP0" acl_enforce_version_8 = true + acl_enable_key_list_policy = true acl_master_token = "C1Q1oIwh" acl_replication_token = "LMmgy5dO" acl_token = "O1El0wan" @@ -2905,6 +2907,7 @@ func TestFullConfig(t *testing.T) { ACLDefaultPolicy: "ArK3WIfE", ACLDownPolicy: "vZXMfMP0", ACLEnforceVersion8: true, + ACLEnableKeyListPolicy: true, ACLMasterToken: "C1Q1oIwh", ACLReplicationToken: "LMmgy5dO", ACLTTL: 18060 * time.Second, @@ -3592,6 +3595,7 @@ func TestSanitize(t *testing.T) { "ACLDefaultPolicy": "", "ACLDisabledTTL": "0s", "ACLDownPolicy": "", + "ACLEnableKeyListPolicy": false, "ACLEnforceVersion8": false, "ACLMasterToken": "hidden", "ACLReplicationToken": "hidden", diff --git a/agent/consul/config.go b/agent/consul/config.go index 1b62af96c..1b904593e 100644 --- a/agent/consul/config.go +++ b/agent/consul/config.go @@ -257,6 +257,11 @@ type Config struct { // are opt-in prior to Consul 0.8 and opt-out in Consul 0.8 and later. ACLEnforceVersion8 bool + // ACLEnableKeyListPolicy is used to gate enforcement of the new "list" policy that + // protects listing keys by prefix. This behavior is opt-in + // by default in Consul 1.0 and later. + ACLEnableKeyListPolicy 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 diff --git a/agent/consul/kvs_endpoint.go b/agent/consul/kvs_endpoint.go index 78a631f88..19591e8c6 100644 --- a/agent/consul/kvs_endpoint.go +++ b/agent/consul/kvs_endpoint.go @@ -133,7 +133,7 @@ func (k *KVS) Get(args *structs.KeyRequest, reply *structs.IndexedDirEntries) er return err } if aclRule != nil && !aclRule.KeyRead(args.Key) { - ent = nil + return acl.ErrPermissionDenied } if ent == nil { @@ -159,11 +159,15 @@ func (k *KVS) List(args *structs.KeyRequest, reply *structs.IndexedDirEntries) e return err } - acl, err := k.srv.resolveToken(args.Token) + aclToken, err := k.srv.resolveToken(args.Token) if err != nil { return err } + if aclToken != nil && k.srv.config.ACLEnableKeyListPolicy && !aclToken.KeyList(args.Key) { + return acl.ErrPermissionDenied + } + return k.srv.blockingQuery( &args.QueryOptions, &reply.QueryMeta, @@ -172,8 +176,8 @@ func (k *KVS) List(args *structs.KeyRequest, reply *structs.IndexedDirEntries) e if err != nil { return err } - if acl != nil { - ent = FilterDirEnt(acl, ent) + if aclToken != nil { + ent = FilterDirEnt(aclToken, ent) } if len(ent) == 0 { @@ -199,11 +203,15 @@ func (k *KVS) ListKeys(args *structs.KeyListRequest, reply *structs.IndexedKeyLi return err } - acl, err := k.srv.resolveToken(args.Token) + aclToken, err := k.srv.resolveToken(args.Token) if err != nil { return err } + if aclToken != nil && k.srv.config.ACLEnableKeyListPolicy && !aclToken.KeyList(args.Prefix) { + return acl.ErrPermissionDenied + } + return k.srv.blockingQuery( &args.QueryOptions, &reply.QueryMeta, @@ -221,8 +229,8 @@ func (k *KVS) ListKeys(args *structs.KeyListRequest, reply *structs.IndexedKeyLi reply.Index = index } - if acl != nil { - keys = FilterKeys(acl, keys) + if aclToken != nil { + keys = FilterKeys(aclToken, keys) } reply.Keys = keys return nil diff --git a/agent/consul/kvs_endpoint_test.go b/agent/consul/kvs_endpoint_test.go index daaa237bc..5a72bd0a2 100644 --- a/agent/consul/kvs_endpoint_test.go +++ b/agent/consul/kvs_endpoint_test.go @@ -214,16 +214,10 @@ func TestKVS_Get_ACLDeny(t *testing.T) { Key: "zip", } var dirent structs.IndexedDirEntries - if err := msgpackrpc.CallWithCodec(codec, "KVS.Get", &getR, &dirent); err != nil { - t.Fatalf("err: %v", err) + if err := msgpackrpc.CallWithCodec(codec, "KVS.Get", &getR, &dirent); !acl.IsErrPermissionDenied(err) { + t.Fatalf("Expected %v, got err: %v", acl.ErrPermissionDenied, err) } - if dirent.Index == 0 { - t.Fatalf("Bad: %v", dirent) - } - if len(dirent.Entries) != 0 { - t.Fatalf("Bad: %v", dirent) - } } func TestKVSEndpoint_List(t *testing.T) { @@ -479,6 +473,116 @@ func TestKVSEndpoint_List_ACLDeny(t *testing.T) { } } +func TestKVSEndpoint_List_ACLEnableKeyListPolicy(t *testing.T) { + t.Parallel() + dir1, s1 := testServerWithConfig(t, func(c *Config) { + c.ACLDatacenter = "dc1" + c.ACLMasterToken = "root" + c.ACLDefaultPolicy = "deny" + c.ACLEnableKeyListPolicy = true + }) + defer os.RemoveAll(dir1) + defer s1.Shutdown() + codec := rpcClient(t, s1) + defer codec.Close() + + testrpc.WaitForLeader(t, s1.RPC, "dc1") + + keys := []string{ + "abe", + "bar/bar1", + "bar/bar2", + "zip", + } + + for _, key := range keys { + arg := structs.KVSRequest{ + Datacenter: "dc1", + Op: api.KVSet, + DirEnt: structs.DirEntry{ + Key: key, + Flags: 1, + }, + WriteRequest: structs.WriteRequest{Token: "root"}, + } + var out bool + if err := msgpackrpc.CallWithCodec(codec, "KVS.Apply", &arg, &out); err != nil { + t.Fatalf("err: %v", err) + } + } + + //write acl policy that denies recursive reads on "" + var testListRules1 = ` +key "" { + policy = "deny" +} +key "bar" { + policy = "list" +} +key "zip" { + policy = "read" +} +` + + arg := structs.ACLRequest{ + Datacenter: "dc1", + Op: structs.ACLSet, + ACL: structs.ACL{ + Name: "User token", + Type: structs.ACLTypeClient, + Rules: testListRules1, + }, + WriteRequest: structs.WriteRequest{Token: "root"}, + } + var out string + if err := msgpackrpc.CallWithCodec(codec, "ACL.Apply", &arg, &out); err != nil { + t.Fatalf("err: %v", err) + } + id := out + + //recursive read on empty prefix should fail + getR := structs.KeyRequest{ + Datacenter: "dc1", + Key: "", + QueryOptions: structs.QueryOptions{Token: id}, + } + var dirent structs.IndexedDirEntries + if err := msgpackrpc.CallWithCodec(codec, "KVS.List", &getR, &dirent); !acl.IsErrPermissionDenied(err) { + t.Fatalf("expected %v but got err: %v", acl.ErrPermissionDenied, err) + } + + // recursive read with a prefix that has list permissions should succeed + getR2 := structs.KeyRequest{ + Datacenter: "dc1", + Key: "bar", + QueryOptions: structs.QueryOptions{Token: id}, + } + if err := msgpackrpc.CallWithCodec(codec, "KVS.List", &getR2, &dirent); err != nil { + t.Fatalf("err: %v", err) + } + + if dirent.Index == 0 { + t.Fatalf("Bad: %v", dirent) + } + if len(dirent.Entries) != 2 { + t.Fatalf("Bad: %v", dirent.Entries) + } + for i := 0; i < len(dirent.Entries); i++ { + d := dirent.Entries[i] + switch i { + case 0: + if d.Key != "bar/bar1" { + t.Fatalf("bad key %v", d.Key) + } + case 1: + if d.Key != "bar/bar2" { + t.Fatalf("bad key %v", d.Key) + } + } + } + +} + func TestKVSEndpoint_ListKeys(t *testing.T) { t.Parallel() dir1, s1 := testServer(t) @@ -628,6 +732,110 @@ func TestKVSEndpoint_ListKeys_ACLDeny(t *testing.T) { } } +func TestKVSEndpoint_ListKeys_ACLEnableKeyListPolicy(t *testing.T) { + t.Parallel() + dir1, s1 := testServerWithConfig(t, func(c *Config) { + c.ACLDatacenter = "dc1" + c.ACLMasterToken = "root" + c.ACLDefaultPolicy = "deny" + c.ACLEnableKeyListPolicy = true + }) + defer os.RemoveAll(dir1) + defer s1.Shutdown() + codec := rpcClient(t, s1) + defer codec.Close() + + testrpc.WaitForLeader(t, s1.RPC, "dc1") + + keys := []string{ + "abe", + "bar/bar1", + "bar/bar2", + "zip", + } + + for _, key := range keys { + arg := structs.KVSRequest{ + Datacenter: "dc1", + Op: api.KVSet, + DirEnt: structs.DirEntry{ + Key: key, + Flags: 1, + }, + WriteRequest: structs.WriteRequest{Token: "root"}, + } + var out bool + if err := msgpackrpc.CallWithCodec(codec, "KVS.Apply", &arg, &out); err != nil { + t.Fatalf("err: %v", err) + } + } + + //write acl policy that denies recursive reads on "" + var testListRules1 = ` +key "" { + policy = "deny" +} +key "bar" { + policy = "list" +} +key "zip" { + policy = "read" +} +` + + arg := structs.ACLRequest{ + Datacenter: "dc1", + Op: structs.ACLSet, + ACL: structs.ACL{ + Name: "User token", + Type: structs.ACLTypeClient, + Rules: testListRules1, + }, + WriteRequest: structs.WriteRequest{Token: "root"}, + } + var out string + if err := msgpackrpc.CallWithCodec(codec, "ACL.Apply", &arg, &out); err != nil { + t.Fatalf("err: %v", err) + } + id := out + + //recursive read on empty prefix should fail + getR := structs.KeyListRequest{ + Datacenter: "dc1", + Prefix: "", + Seperator: "/", + QueryOptions: structs.QueryOptions{Token: id}, + } + var dirent structs.IndexedKeyList + if err := msgpackrpc.CallWithCodec(codec, "KVS.ListKeys", &getR, &dirent); !acl.IsErrPermissionDenied(err) { + t.Fatalf("expected %v but got err: %v", acl.ErrPermissionDenied, err) + } + + // recursive read with a prefix that has list permissions should succeed + getR2 := structs.KeyListRequest{ + Datacenter: "dc1", + Prefix: "bar", + QueryOptions: structs.QueryOptions{Token: id}, + } + if err := msgpackrpc.CallWithCodec(codec, "KVS.ListKeys", &getR2, &dirent); err != nil { + t.Fatalf("err: %v", err) + } + + if dirent.Index == 0 { + t.Fatalf("Bad: %v", dirent) + } + if len(dirent.Keys) != 2 { + t.Fatalf("Bad: %v", dirent.Keys) + } + if dirent.Keys[0] != "bar/bar1" { + t.Fatalf("Bad: %v", dirent.Keys) + } + if dirent.Keys[1] != "bar/bar2" { + t.Fatalf("Bad: %v", dirent.Keys) + } + +} + func TestKVS_Apply_LockDelay(t *testing.T) { t.Parallel() dir1, s1 := testServer(t)