Merge pull request #3340 from hashicorp/issue_2637

This fixes issue #2637

agent/consul/filter.go

@@ -22,9 +22,13 @@ func (d *dirEntFilter) Move(dst, src, span int) {
 
 // FilterDirEnt is used to filter a list of directory entries
 // by applying an ACL policy
-func FilterDirEnt(acl acl.ACL, ent structs.DirEntries) structs.DirEntries {
+func FilterDirEnt(acl acl.ACL, ent structs.DirEntries) (structs.DirEntries, error) {
 	df := dirEntFilter{acl: acl, ent: ent}
-	return ent[:FilterEntries(&df)]
+	filtered := ent[:FilterEntries(&df)]
+	if len(filtered) == 0 {
+		return nil, errPermissionDenied
+	}
+	return filtered, nil
 }
 
 type keyFilter struct {
@@ -45,9 +49,13 @@ func (k *keyFilter) Move(dst, src, span int) {
 
 // FilterKeys is used to filter a list of keys by
 // applying an ACL policy
-func FilterKeys(acl acl.ACL, keys []string) []string {
+func FilterKeys(acl acl.ACL, keys []string) ([]string, error) {
 	kf := keyFilter{acl: acl, keys: keys}
-	return keys[:FilterEntries(&kf)]
+	filteredKeys := keys[:FilterEntries(&kf)]
+	if len(filteredKeys) == 0 {
+		return nil, errPermissionDenied
+	}
+	return filteredKeys, nil
 }
 
 type txnResultsFilter struct {
@@ -73,9 +81,13 @@ func (t *txnResultsFilter) Move(dst, src, span int) {
 
 // FilterTxnResults is used to filter a list of transaction results by
 // applying an ACL policy.
-func FilterTxnResults(acl acl.ACL, results structs.TxnResults) structs.TxnResults {
+func FilterTxnResults(acl acl.ACL, results structs.TxnResults) (structs.TxnResults, error) {
 	rf := txnResultsFilter{acl: acl, results: results}
-	return results[:FilterEntries(&rf)]
+	filtered := results[:FilterEntries(&rf)]
+	if len(filtered) == 0 {
+		return nil, errPermissionDenied
+	}
+	return filtered, nil
 }
 
 // Filter interface is used with FilterEntries to do an

agent/consul/filter_test.go

@@ -16,6 +16,7 @@ func TestFilter_DirEnt(t *testing.T) {
 	type tcase struct {
 		in  []string
 		out []string
+		err error
 	}
 	cases := []tcase{
 		tcase{
@@ -25,6 +26,7 @@ func TestFilter_DirEnt(t *testing.T) {
 		tcase{
 			in:  []string{"abe", "lincoln"},
 			out: nil,
+			err: errPermissionDenied,
 		},
 		tcase{
 			in:  []string{"abe", "foo/1", "foo/2", "foo/3", "nope"},
@@ -38,7 +40,10 @@ func TestFilter_DirEnt(t *testing.T) {
 			ents = append(ents, &structs.DirEntry{Key: in})
 		}
 
-		ents = FilterDirEnt(aclR, ents)
+		ents, err := FilterDirEnt(aclR, ents)
+		if err != tc.err {
+			t.Fatalf("Unexpected error, got %v, wanted %v", err, tc.err)
+		}
 		var outL []string
 		for _, e := range ents {
 			outL = append(outL, e.Key)
@@ -58,6 +63,7 @@ func TestFilter_Keys(t *testing.T) {
 	type tcase struct {
 		in  []string
 		out []string
+		err error
 	}
 	cases := []tcase{
 		tcase{
@@ -66,7 +72,7 @@ func TestFilter_Keys(t *testing.T) {
 		},
 		tcase{
 			in:  []string{"abe", "lincoln"},
-			out: []string{},
+			err: errPermissionDenied,
 		},
 		tcase{
 			in:  []string{"abe", "foo/1", "foo/2", "foo/3", "nope"},
@@ -75,10 +81,14 @@ func TestFilter_Keys(t *testing.T) {
 	}
 
 	for _, tc := range cases {
-		out := FilterKeys(aclR, tc.in)
+		out, err := FilterKeys(aclR, tc.in)
+		if tc.err != err {
+			t.Fatalf("Unexpected error, got %v, wanted %v", err, tc.err)
+		}
 		if !reflect.DeepEqual(out, tc.out) {
 			t.Fatalf("bad: %#v %#v", out, tc.out)
 		}
+
 	}
 }
 
@@ -90,6 +100,7 @@ func TestFilter_TxnResults(t *testing.T) {
 	type tcase struct {
 		in  []string
 		out []string
+		err error
 	}
 	cases := []tcase{
 		tcase{
@@ -99,6 +110,7 @@ func TestFilter_TxnResults(t *testing.T) {
 		tcase{
 			in:  []string{"abe", "lincoln"},
 			out: nil,
+			err: errPermissionDenied,
 		},
 		tcase{
 			in:  []string{"abe", "foo/1", "foo/2", "foo/3", "nope"},
@@ -112,7 +124,10 @@ func TestFilter_TxnResults(t *testing.T) {
 			results = append(results, &structs.TxnResult{KV: &structs.DirEntry{Key: in}})
 		}
 
-		results = FilterTxnResults(aclR, results)
+		results, err := FilterTxnResults(aclR, results)
+		if tc.err != err {
+			t.Fatalf("Unexpected error, got %v, wanted %v", err, tc.err)
+		}
 		var outL []string
 		for _, r := range results {
 			outL = append(outL, r.KV.Key)
@@ -126,7 +141,10 @@ func TestFilter_TxnResults(t *testing.T) {
 	// Run a non-KV result.
 	results := structs.TxnResults{}
 	results = append(results, &structs.TxnResult{})
-	results = FilterTxnResults(aclR, results)
+	results, err := FilterTxnResults(aclR, results)
+	if err != nil {
+		t.Fatalf("Unexpected error: %v", err)
+	}
 	if len(results) != 1 {
 		t.Fatalf("should not have filtered non-KV result")
 	}

agent/consul/kvs_endpoint.go

@@ -129,7 +129,7 @@ func (k *KVS) Get(args *structs.KeyRequest, reply *structs.IndexedDirEntries) er
 				return err
 			}
 			if acl != nil && !acl.KeyRead(args.Key) {
-				ent = nil
+				return errPermissionDenied
 			}
 			if ent == nil {
 				// Must provide non-zero index to prevent blocking
@@ -168,9 +168,11 @@ func (k *KVS) List(args *structs.KeyRequest, reply *structs.IndexedDirEntries) e
 				return err
 			}
 			if acl != nil {
-				ent = FilterDirEnt(acl, ent)
+				ent, err = FilterDirEnt(acl, ent)
+				if err != nil {
+					return err
+				}
 			}
-
 			if len(ent) == 0 {
 				// Must provide non-zero index to prevent blocking
 				// Index 1 is impossible anyways (due to Raft internals)
@@ -217,7 +219,10 @@ func (k *KVS) ListKeys(args *structs.KeyListRequest, reply *structs.IndexedKeyLi
 			}
 
 			if acl != nil {
-				keys = FilterKeys(acl, keys)
+				keys, err = FilterKeys(acl, keys)
+				if err != nil {
+					return err
+				}
 			}
 			reply.Keys = keys
 			return nil

agent/consul/kvs_endpoint_test.go

@@ -206,24 +206,18 @@ func TestKVS_Get_ACLDeny(t *testing.T) {
 	}
 	var out bool
 	if err := msgpackrpc.CallWithCodec(codec, "KVS.Apply", &arg, &out); err != nil {
-		t.Fatalf("err: %v", err)
+		t.Fatalf("Unexpected err: %v", err)
 	}
 
 	getR := structs.KeyRequest{
 		Datacenter: "dc1",
 		Key:        "zip",
 	}
 	var dirent structs.IndexedDirEntries
-	if err := msgpackrpc.CallWithCodec(codec, "KVS.Get", &getR, &dirent); err != nil {
+	if err := msgpackrpc.CallWithCodec(codec, "KVS.Get", &getR, &dirent); err.Error() != errPermissionDenied.Error() {
 		t.Fatalf("err: %v", 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) {

agent/consul/txn_endpoint.go

@@ -72,7 +72,10 @@ func (t *Txn) Apply(args *structs.TxnRequest, reply *structs.TxnResponse) error
 	// just taking the two slices.
 	if txnResp, ok := resp.(structs.TxnResponse); ok {
 		if acl != nil {
-			txnResp.Results = FilterTxnResults(acl, txnResp.Results)
+			txnResp.Results, err = FilterTxnResults(acl, txnResp.Results)
+			if err != nil {
+				return err
+			}
 		}
 		*reply = txnResp
 	} else {
@@ -113,7 +116,10 @@ func (t *Txn) Read(args *structs.TxnReadRequest, reply *structs.TxnReadResponse)
 	state := t.srv.fsm.State()
 	reply.Results, reply.Errors = state.TxnRO(args.Ops)
 	if acl != nil {
-		reply.Results = FilterTxnResults(acl, reply.Results)
+		reply.Results, err = FilterTxnResults(acl, reply.Results)
+		if err != nil {
+			return err
+		}
 	}
 	return nil
 }