Merge pull request #3523 from hashicorp/issue_3511

Introduces new 'list' permission that applies to KV store recursive reads

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

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) {

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
@@ -175,7 +176,7 @@ func Parse(rules string, sentinel sentinel.Evaluator) (*Policy, error) {
 
 	// Validate the key policy
 	for _, kp := range p.Keys {
-		if !isPolicyValid(kp.Policy) {
+		if kp.Policy != PolicyList && !isPolicyValid(kp.Policy) {
 			return nil, fmt.Errorf("Invalid key policy: %#v", kp)
 		}
 		if err := isSentinelValid(sentinel, kp.Policy, kp.Sentinel); err != nil {

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
 	}

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),

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"`

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

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",

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

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

agent/consul/kvs_endpoint_test.go

@@ -10,6 +10,7 @@ import (
 	"github.com/hashicorp/consul/api"
 	"github.com/hashicorp/consul/testrpc"
 	"github.com/hashicorp/net-rpc-msgpackrpc"
+	"github.com/pascaldekloe/goe/verify"
 )
 
 func TestKVS_Apply(t *testing.T) {
@@ -214,16 +215,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 +474,134 @@ 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
+	getReq := structs.KeyRequest{
+		Datacenter:   "dc1",
+		Key:          "",
+		QueryOptions: structs.QueryOptions{Token: id},
+	}
+	var dirent structs.IndexedDirEntries
+	if err := msgpackrpc.CallWithCodec(codec, "KVS.List", &getReq, &dirent); !acl.IsErrPermissionDenied(err) {
+		t.Fatalf("expected %v but got err: %v", acl.ErrPermissionDenied, err)
+	}
+
+	//listing keys on empty prefix should fail
+	getKeysReq := structs.KeyListRequest{
+		Datacenter:   "dc1",
+		Prefix:       "",
+		Seperator:    "/",
+		QueryOptions: structs.QueryOptions{Token: id},
+	}
+	var keyList structs.IndexedKeyList
+	if err := msgpackrpc.CallWithCodec(codec, "KVS.ListKeys", &getKeysReq, &keyList); !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
+	getReq2 := structs.KeyRequest{
+		Datacenter:   "dc1",
+		Key:          "bar",
+		QueryOptions: structs.QueryOptions{Token: id},
+	}
+	if err := msgpackrpc.CallWithCodec(codec, "KVS.List", &getReq2, &dirent); err != nil {
+		t.Fatalf("err: %v", err)
+	}
+
+	expectedKeys := []string{"bar/bar1", "bar/bar2"}
+	var actualKeys []string
+	for _, entry := range dirent.Entries {
+		actualKeys = append(actualKeys, entry.Key)
+	}
+
+	verify.Values(t, "", actualKeys, expectedKeys)
+
+	// list keys with a prefix that has list permissions should succeed
+	getKeysReq2 := structs.KeyListRequest{
+		Datacenter:   "dc1",
+		Prefix:       "bar",
+		QueryOptions: structs.QueryOptions{Token: id},
+	}
+	if err := msgpackrpc.CallWithCodec(codec, "KVS.ListKeys", &getKeysReq2, &keyList); err != nil {
+		t.Fatalf("err: %v", err)
+	}
+
+	actualKeys = []string{}
+
+	for _, key := range keyList.Keys {
+		actualKeys = append(actualKeys, key)
+	}
+
+	verify.Values(t, "", actualKeys, expectedKeys)
+
+}
+
 func TestKVSEndpoint_ListKeys(t *testing.T) {
 	t.Parallel()
 	dir1, s1 := testServer(t)

website/source/docs/guides/acl.html.md

@@ -671,6 +671,32 @@ the example above, the rules allow read-only access to any key name with the emp
 read-write access to any key name that starts with "foo", and deny all access to any key name that
 starts with "bar".
 
+#### List Policy for Keys
+
+Consul 1.0 introduces a new `list` policy for keys that is only enforced when opted in via the boolean config param "acl_enable_key_list_policy".
+`list` controls access to recursively list entries and keys, and enables more fine grained policies. With "acl_enable_key_list_policy",
+recursive reads via [the KV API](/api/kv.html#recurse) with an invalid token result in a 403. Example:
+
+```text
+key "" {
+ policy = "deny"
+}
+
+key "bar" {
+ policy = "list"
+}
+
+key "baz" {
+ policy = "read"
+}
+```
+
+In the example above, the rules allow reading the key "baz", and only allow recursive reads on the prefix "bar".
+
+A token with `write` access on a prefix also has `list` access. A token with `list` access on a prefix also has `read` access on all its suffixes.
+
+#### Sentinel Integration
+
 Consul Enterprise supports additional optional fields for key write policies for
 [Sentinel](https://docs.hashicorp.com/sentinel/app/consul/) integration. An example key rule with a
 Sentinel code policy looks like this: