Switches GETs to a filtering model for ACLs.

consul/filter.go

@@ -50,6 +50,35 @@ func FilterKeys(acl acl.ACL, keys []string) []string {
 	return keys[:FilterEntries(&kf)]
 }
 
+type txnResultsFilter struct {
+	acl     acl.ACL
+	results structs.TxnResults
+}
+
+func (t *txnResultsFilter) Len() int {
+	return len(t.results)
+}
+
+func (t *txnResultsFilter) Filter(i int) bool {
+	result := t.results[i]
+	if result.KV != nil {
+		return !t.acl.KeyRead(result.KV.Key)
+	} else {
+		return false
+	}
+}
+
+func (t *txnResultsFilter) Move(dst, src, span int) {
+	copy(t.results[dst:dst+span], t.results[src:src+span])
+}
+
+// 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 {
+	rf := txnResultsFilter{acl: acl, results: results}
+	return results[:FilterEntries(&rf)]
+}
+
 // Filter interface is used with FilterEntries to do an
 // in-place filter of a slice.
 type Filter interface {

consul/filter_test.go

@@ -8,7 +8,7 @@ import (
 	"github.com/hashicorp/consul/consul/structs"
 )
 
-func TestFilterDirEnt(t *testing.T) {
+func TestFilter_DirEnt(t *testing.T) {
 	policy, _ := acl.Parse(testFilterRules)
 	aclR, _ := acl.New(acl.DenyAll(), policy)
 
@@ -49,7 +49,7 @@ func TestFilterDirEnt(t *testing.T) {
 	}
 }
 
-func TestKeys(t *testing.T) {
+func TestFilter_Keys(t *testing.T) {
 	policy, _ := acl.Parse(testFilterRules)
 	aclR, _ := acl.New(acl.DenyAll(), policy)
 
@@ -80,6 +80,55 @@ func TestKeys(t *testing.T) {
 	}
 }
 
+func TestFilter_TxnResults(t *testing.T) {
+	policy, _ := acl.Parse(testFilterRules)
+	aclR, _ := acl.New(acl.DenyAll(), policy)
+
+	type tcase struct {
+		in  []string
+		out []string
+	}
+	cases := []tcase{
+		tcase{
+			in:  []string{"foo/test", "foo/priv/nope", "foo/other", "zoo"},
+			out: []string{"foo/test", "foo/other"},
+		},
+		tcase{
+			in:  []string{"abe", "lincoln"},
+			out: nil,
+		},
+		tcase{
+			in:  []string{"abe", "foo/1", "foo/2", "foo/3", "nope"},
+			out: []string{"foo/1", "foo/2", "foo/3"},
+		},
+	}
+
+	for _, tc := range cases {
+		results := structs.TxnResults{}
+		for _, in := range tc.in {
+			results = append(results, &structs.TxnResult{KV: &structs.DirEntry{Key: in}})
+		}
+
+		results = FilterTxnResults(aclR, results)
+		var outL []string
+		for _, r := range results {
+			outL = append(outL, r.KV.Key)
+		}
+
+		if !reflect.DeepEqual(outL, tc.out) {
+			t.Fatalf("bad: %#v %#v", outL, tc.out)
+		}
+	}
+
+	// Run a non-KV result.
+	results := structs.TxnResults{}
+	results = append(results, &structs.TxnResult{})
+	results = FilterTxnResults(aclR, results)
+	if len(results) != 1 {
+		t.Fatalf("should not have filtered non-KV result")
+	}
+}
+
 var testFilterRules = `
 key "" {
 	policy = "deny"

consul/kvs_endpoint.go

@@ -31,9 +31,13 @@ func kvsPreApply(srv *Server, acl acl.ACL, op structs.KVSOp, dirEnt *structs.Dir
 				return false, permissionDeniedErr
 			}
 
-		case structs.KVSGet,
+		case structs.KVSGet:
-			structs.KVSCheckSession,
+			// Filtering for GETs is done on the output side.
-			structs.KVSCheckIndex:
+
+		case structs.KVSCheckSession, structs.KVSCheckIndex:
+			// These could reveal information based on the outcome
+			// of the transaction, and they operate on individual
+			// keys so we check them here.
 			if !acl.KeyRead(dirEnt.Key) {
 				return false, permissionDeniedErr
 			}

consul/txn_endpoint.go

@@ -65,6 +65,9 @@ func (t *Txn) Apply(args *structs.TxnRequest, reply *structs.TxnResponse) error
 	// Convert the return type. This should be a cheap copy since we are
 	// just taking the two slices.
 	if txnResp, ok := resp.(structs.TxnResponse); ok {
+		if acl != nil {
+			txnResp.Results = FilterTxnResults(acl, txnResp.Results)
+		}
 		*reply = txnResp
 	} else {
 		return fmt.Errorf("unexpected return type %T", resp)
@@ -103,5 +106,8 @@ func (t *Txn) Read(args *structs.TxnReadRequest, reply *structs.TxnReadResponse)
 	// Run the read transaction.
 	state := t.srv.fsm.State()
 	reply.Results, reply.Errors = state.TxnRO(args.Ops)
+	if acl != nil {
+		reply.Results = FilterTxnResults(acl, reply.Results)
+	}
 	return nil
 }

consul/txn_endpoint_test.go

@@ -112,6 +112,16 @@ func TestTxn_Apply_ACLDeny(t *testing.T) {
 
 	testutil.WaitForLeader(t, s1.RPC, "dc1")
 
+	// Put in a key to read back.
+	state := s1.fsm.State()
+	d := &structs.DirEntry{
+		Key:   "nope",
+		Value: []byte("hello"),
+	}
+	if err := state.KVSSet(1, d); err != nil {
+		t.Fatalf("err: %v", err)
+	}
+
 	// Create the ACL.
 	var id string
 	{
@@ -139,7 +149,7 @@ func TestTxn_Apply_ACLDeny(t *testing.T) {
 				KV: &structs.TxnKVOp{
 					Verb: structs.KVSSet,
 					DirEnt: structs.DirEntry{
-						Key: "foo",
+						Key: "nope",
 					},
 				},
 			},
@@ -147,7 +157,7 @@ func TestTxn_Apply_ACLDeny(t *testing.T) {
 				KV: &structs.TxnKVOp{
 					Verb: structs.KVSDelete,
 					DirEnt: structs.DirEntry{
-						Key: "foo",
+						Key: "nope",
 					},
 				},
 			},
@@ -155,7 +165,7 @@ func TestTxn_Apply_ACLDeny(t *testing.T) {
 				KV: &structs.TxnKVOp{
 					Verb: structs.KVSDeleteCAS,
 					DirEnt: structs.DirEntry{
-						Key: "foo",
+						Key: "nope",
 					},
 				},
 			},
@@ -163,7 +173,7 @@ func TestTxn_Apply_ACLDeny(t *testing.T) {
 				KV: &structs.TxnKVOp{
 					Verb: structs.KVSDeleteTree,
 					DirEnt: structs.DirEntry{
-						Key: "foo",
+						Key: "nope",
 					},
 				},
 			},
@@ -171,7 +181,7 @@ func TestTxn_Apply_ACLDeny(t *testing.T) {
 				KV: &structs.TxnKVOp{
 					Verb: structs.KVSCAS,
 					DirEnt: structs.DirEntry{
-						Key: "foo",
+						Key: "nope",
 					},
 				},
 			},
@@ -179,7 +189,7 @@ func TestTxn_Apply_ACLDeny(t *testing.T) {
 				KV: &structs.TxnKVOp{
 					Verb: structs.KVSLock,
 					DirEnt: structs.DirEntry{
-						Key: "foo",
+						Key: "nope",
 					},
 				},
 			},
@@ -187,7 +197,7 @@ func TestTxn_Apply_ACLDeny(t *testing.T) {
 				KV: &structs.TxnKVOp{
 					Verb: structs.KVSUnlock,
 					DirEnt: structs.DirEntry{
-						Key: "foo",
+						Key: "nope",
 					},
 				},
 			},
@@ -227,8 +237,14 @@ func TestTxn_Apply_ACLDeny(t *testing.T) {
 
 	// Verify the transaction's return value.
 	var expected structs.TxnResponse
-	for i, _ := range arg.Ops {
+	for i, op := range arg.Ops {
-		expected.Errors = append(expected.Errors, &structs.TxnError{i, permissionDeniedErr.Error()})
+		switch op.KV.Verb {
+		case structs.KVSGet:
+			// These get filtered but won't result in an error.
+
+		default:
+			expected.Errors = append(expected.Errors, &structs.TxnError{i, permissionDeniedErr.Error()})
+		}
 	}
 	if !reflect.DeepEqual(out, expected) {
 		t.Fatalf("bad %v", out)
@@ -398,6 +414,16 @@ func TestTxn_Read_ACLDeny(t *testing.T) {
 
 	testutil.WaitForLeader(t, s1.RPC, "dc1")
 
+	// Put in a key to read back.
+	state := s1.fsm.State()
+	d := &structs.DirEntry{
+		Key:   "nope",
+		Value: []byte("hello"),
+	}
+	if err := state.KVSSet(1, d); err != nil {
+		t.Fatalf("err: %v", err)
+	}
+
 	// Create the ACL.
 	var id string
 	{
@@ -461,8 +487,14 @@ func TestTxn_Read_ACLDeny(t *testing.T) {
 			KnownLeader: true,
 		},
 	}
-	for i, _ := range arg.Ops {
+	for i, op := range arg.Ops {
-		expected.Errors = append(expected.Errors, &structs.TxnError{i, permissionDeniedErr.Error()})
+		switch op.KV.Verb {
+		case structs.KVSGet:
+			// These get filtered but won't result in an error.
+
+		default:
+			expected.Errors = append(expected.Errors, &structs.TxnError{i, permissionDeniedErr.Error()})
+		}
 	}
 	if !reflect.DeepEqual(out, expected) {
 		t.Fatalf("bad %v", out)