From 6336014a8606a647a63b3dbd5b475f283107480f Mon Sep 17 00:00:00 2001 From: Preetha Appan Date: Mon, 31 Jul 2017 13:40:21 -0500 Subject: [PATCH 1/2] Return 403 rather than a 404 when acls cause all results to be filtered out. This fixes #2637 --- agent/consul/filter.go | 24 ++++++++++++++++++------ agent/consul/filter_test.go | 27 +++++++++++++++++++++++---- agent/consul/kvs_endpoint.go | 13 +++++++++---- agent/consul/kvs_endpoint_test.go | 10 ++-------- agent/consul/txn_endpoint.go | 10 ++++++++-- 5 files changed, 60 insertions(+), 24 deletions(-) diff --git a/agent/consul/filter.go b/agent/consul/filter.go index 1e6806b28b44..ba6a01f7dcda 100644 --- a/agent/consul/filter.go +++ b/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 filtered, 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 filteredKeys, 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 filtered, errPermissionDenied + } + return filtered, nil } // Filter interface is used with FilterEntries to do an diff --git a/agent/consul/filter_test.go b/agent/consul/filter_test.go index 9568102cc8f3..161595e36b39 100644 --- a/agent/consul/filter_test.go +++ b/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{ @@ -67,6 +73,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 +82,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 +101,7 @@ func TestFilter_TxnResults(t *testing.T) { type tcase struct { in []string out []string + err error } cases := []tcase{ tcase{ @@ -99,6 +111,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 +125,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 +142,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") } diff --git a/agent/consul/kvs_endpoint.go b/agent/consul/kvs_endpoint.go index ef1c067fb780..e880a8a481e1 100644 --- a/agent/consul/kvs_endpoint.go +++ b/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 diff --git a/agent/consul/kvs_endpoint_test.go b/agent/consul/kvs_endpoint_test.go index 52b9a783d5cb..b03816b6e81a 100644 --- a/agent/consul/kvs_endpoint_test.go +++ b/agent/consul/kvs_endpoint_test.go @@ -206,7 +206,7 @@ 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{ @@ -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 { + 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) { diff --git a/agent/consul/txn_endpoint.go b/agent/consul/txn_endpoint.go index 7aefdc8cc3fa..e38882d021eb 100644 --- a/agent/consul/txn_endpoint.go +++ b/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 } From 4076c0d741d41d429ed547480879fa0f1ee47037 Mon Sep 17 00:00:00 2001 From: Preetha Appan Date: Mon, 31 Jul 2017 17:23:20 -0500 Subject: [PATCH 2/2] Return nil instead of empty list when returning a PermissionDenied error, updated unit test --- agent/consul/filter.go | 6 +++--- agent/consul/filter_test.go | 1 - 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/agent/consul/filter.go b/agent/consul/filter.go index ba6a01f7dcda..88be1b99c317 100644 --- a/agent/consul/filter.go +++ b/agent/consul/filter.go @@ -26,7 +26,7 @@ func FilterDirEnt(acl acl.ACL, ent structs.DirEntries) (structs.DirEntries, erro df := dirEntFilter{acl: acl, ent: ent} filtered := ent[:FilterEntries(&df)] if len(filtered) == 0 { - return filtered, errPermissionDenied + return nil, errPermissionDenied } return filtered, nil } @@ -53,7 +53,7 @@ func FilterKeys(acl acl.ACL, keys []string) ([]string, error) { kf := keyFilter{acl: acl, keys: keys} filteredKeys := keys[:FilterEntries(&kf)] if len(filteredKeys) == 0 { - return filteredKeys, errPermissionDenied + return nil, errPermissionDenied } return filteredKeys, nil } @@ -85,7 +85,7 @@ func FilterTxnResults(acl acl.ACL, results structs.TxnResults) (structs.TxnResul rf := txnResultsFilter{acl: acl, results: results} filtered := results[:FilterEntries(&rf)] if len(filtered) == 0 { - return filtered, errPermissionDenied + return nil, errPermissionDenied } return filtered, nil } diff --git a/agent/consul/filter_test.go b/agent/consul/filter_test.go index 161595e36b39..eb5218d53a76 100644 --- a/agent/consul/filter_test.go +++ b/agent/consul/filter_test.go @@ -72,7 +72,6 @@ func TestFilter_Keys(t *testing.T) { }, tcase{ in: []string{"abe", "lincoln"}, - out: []string{}, err: errPermissionDenied, }, tcase{