From 0f94a7a32648030bf15f0f2f2d6af8e498952cf2 Mon Sep 17 00:00:00 2001 From: James Phillips Date: Fri, 13 May 2016 15:58:55 -0700 Subject: [PATCH] Switches GETs to a filtering model for ACLs. --- consul/filter.go | 29 ++++++++++++++++++++ consul/filter_test.go | 53 ++++++++++++++++++++++++++++++++++-- consul/kvs_endpoint.go | 10 ++++--- consul/txn_endpoint.go | 6 +++++ consul/txn_endpoint_test.go | 54 +++++++++++++++++++++++++++++-------- 5 files changed, 136 insertions(+), 16 deletions(-) diff --git a/consul/filter.go b/consul/filter.go index 946508e31..322cd353a 100644 --- a/consul/filter.go +++ b/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 { diff --git a/consul/filter_test.go b/consul/filter_test.go index ce419bb97..10ee367e1 100644 --- a/consul/filter_test.go +++ b/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" diff --git a/consul/kvs_endpoint.go b/consul/kvs_endpoint.go index 0c2a05e1c..ebe109d19 100644 --- a/consul/kvs_endpoint.go +++ b/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, - structs.KVSCheckSession, - structs.KVSCheckIndex: + case structs.KVSGet: + // Filtering for GETs is done on the output side. + + 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 } diff --git a/consul/txn_endpoint.go b/consul/txn_endpoint.go index 72f81f729..d5125a7d5 100644 --- a/consul/txn_endpoint.go +++ b/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 } diff --git a/consul/txn_endpoint_test.go b/consul/txn_endpoint_test.go index e15e42c4d..797718433 100644 --- a/consul/txn_endpoint_test.go +++ b/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 { - expected.Errors = append(expected.Errors, &structs.TxnError{i, permissionDeniedErr.Error()}) + for i, op := range arg.Ops { + 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 { - expected.Errors = append(expected.Errors, &structs.TxnError{i, permissionDeniedErr.Error()}) + for i, op := range arg.Ops { + 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)