From 4235173d78022182d4cd0da806bbffdf83b9fe1e Mon Sep 17 00:00:00 2001 From: Laura Bennett Date: Sun, 24 Jul 2016 22:27:41 -0400 Subject: [PATCH 1/6] initial commit for nonAssignablePolicies --- vault/policy_store.go | 29 +++++++++++++++++++++++++---- vault/token_store.go | 7 +++++++ 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/vault/policy_store.go b/vault/policy_store.go index 45439ebd60..f8422f5a17 100644 --- a/vault/policy_store.go +++ b/vault/policy_store.go @@ -36,6 +36,9 @@ var ( "root", cubbyholeResponseWrappingPolicyName, } + nonAssignablePolicies = []string{ + cubbyholeResponseWrappingPolicyName, + } ) // PolicyStore is used to provide durable storage of policy, and to @@ -89,7 +92,7 @@ func (c *Core) setupPolicyStore() error { // Ensure that the cubbyhole response wrapping policy exists policy, err = c.policyStore.GetPolicy(cubbyholeResponseWrappingPolicyName) if err != nil { - return errwrap.Wrapf("error fetching default policy from store: {{err}}", err) + return errwrap.Wrapf("error fetching cubbyhole response wrapping policy from store: {{err}}", err) } if policy == nil || policy.Raw != cubbyholeResponseWrappingPolicy { err := c.policyStore.createCubbyholeResponseWrappingPolicy() @@ -114,7 +117,7 @@ func (ps *PolicyStore) SetPolicy(p *Policy) error { if p.Name == "" { return fmt.Errorf("policy name missing") } - if strutil.StrListContains(immutablePolicies, p.Name) { + if strutil.StrListContains(immutablePolicies, p.Name) || strutil.StrListContains(nonAssignablePolicies, p.Name) { return fmt.Errorf("cannot update %s policy", p.Name) } @@ -210,13 +213,31 @@ func (ps *PolicyStore) ListPolicies() ([]string, error) { defer metrics.MeasureSince([]string{"policy", "list_policies"}, time.Now()) // Scan the view, since the policy names are the same as the // key names. - return CollectKeys(ps.view) + keys, err = CollectKeys(ps.view) + + for _, nonAssignable := range nonAssignablePolicies { + deleteIndex := -1 + //Find indices of non-assignable policies in keys + for index, key := range keys { + if key == nonAssignable { + // Delete collection outside the loop + deleteIndex = index + break + } + } + // Remove non-assignable policies when found + if deleteIndex != -1 { + keys = append(keys[:deleteIndex], keys[deleteIndex+1:]...) + } + } + + return keys, err } // DeletePolicy is used to delete the named policy func (ps *PolicyStore) DeletePolicy(name string) error { defer metrics.MeasureSince([]string{"policy", "delete_policy"}, time.Now()) - if strutil.StrListContains(immutablePolicies, name) { + if strutil.StrListContains(immutablePolicies, name) || strutil.StrListContains(nonAssignablePolicies, name) { return fmt.Errorf("cannot delete %s policy", name) } if name == "default" { diff --git a/vault/token_store.go b/vault/token_store.go index c628bc279b..3986855b55 100644 --- a/vault/token_store.go +++ b/vault/token_store.go @@ -1193,6 +1193,13 @@ func (ts *TokenStore) handleCreateCommon( return logical.ErrorResponse(err.Error()), logical.ErrInvalidRequest } + // Prevent internasl policies from being assigned to tokens + for _, policy := range te.Policies { + if strutil.StrListContains(nonAssignablePolicies, policy) { + return logical.ErrorResponse(fmt.Sprintf("cannot assign %s policy", policy)), nil + } + } + // Generate the response resp.Auth = &logical.Auth{ DisplayName: te.DisplayName, From 3c8e4da3597ba9eb044324675e5ce949d4ffa540 Mon Sep 17 00:00:00 2001 From: Laura Bennett Date: Sun, 24 Jul 2016 22:35:54 -0400 Subject: [PATCH 2/6] minor error correction --- vault/policy_store.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vault/policy_store.go b/vault/policy_store.go index f8422f5a17..531b5f5b84 100644 --- a/vault/policy_store.go +++ b/vault/policy_store.go @@ -213,7 +213,7 @@ func (ps *PolicyStore) ListPolicies() ([]string, error) { defer metrics.MeasureSince([]string{"policy", "list_policies"}, time.Now()) // Scan the view, since the policy names are the same as the // key names. - keys, err = CollectKeys(ps.view) + keys, err := CollectKeys(ps.view) for _, nonAssignable := range nonAssignablePolicies { deleteIndex := -1 From 1acea5246f65ba1b3f4e4ebda2920cc2eec424b2 Mon Sep 17 00:00:00 2001 From: Laura Bennett Date: Mon, 25 Jul 2016 09:46:10 -0400 Subject: [PATCH 3/6] edits based on comments in PR --- vault/policy_store.go | 6 +++--- vault/token_store.go | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/vault/policy_store.go b/vault/policy_store.go index 531b5f5b84..c409c124ec 100644 --- a/vault/policy_store.go +++ b/vault/policy_store.go @@ -92,7 +92,7 @@ func (c *Core) setupPolicyStore() error { // Ensure that the cubbyhole response wrapping policy exists policy, err = c.policyStore.GetPolicy(cubbyholeResponseWrappingPolicyName) if err != nil { - return errwrap.Wrapf("error fetching cubbyhole response wrapping policy from store: {{err}}", err) + return errwrap.Wrapf("error fetching response wrapping policy from store: {{err}}", err) } if policy == nil || policy.Raw != cubbyholeResponseWrappingPolicy { err := c.policyStore.createCubbyholeResponseWrappingPolicy() @@ -117,7 +117,7 @@ func (ps *PolicyStore) SetPolicy(p *Policy) error { if p.Name == "" { return fmt.Errorf("policy name missing") } - if strutil.StrListContains(immutablePolicies, p.Name) || strutil.StrListContains(nonAssignablePolicies, p.Name) { + if strutil.StrListContains(immutablePolicies, p.Name) { return fmt.Errorf("cannot update %s policy", p.Name) } @@ -237,7 +237,7 @@ func (ps *PolicyStore) ListPolicies() ([]string, error) { // DeletePolicy is used to delete the named policy func (ps *PolicyStore) DeletePolicy(name string) error { defer metrics.MeasureSince([]string{"policy", "delete_policy"}, time.Now()) - if strutil.StrListContains(immutablePolicies, name) || strutil.StrListContains(nonAssignablePolicies, name) { + if strutil.StrListContains(immutablePolicies, name) { return fmt.Errorf("cannot delete %s policy", name) } if name == "default" { diff --git a/vault/token_store.go b/vault/token_store.go index 3986855b55..a62efa3906 100644 --- a/vault/token_store.go +++ b/vault/token_store.go @@ -1193,7 +1193,7 @@ func (ts *TokenStore) handleCreateCommon( return logical.ErrorResponse(err.Error()), logical.ErrInvalidRequest } - // Prevent internasl policies from being assigned to tokens + // Prevent internal policies from being assigned to tokens for _, policy := range te.Policies { if strutil.StrListContains(nonAssignablePolicies, policy) { return logical.ErrorResponse(fmt.Sprintf("cannot assign %s policy", policy)), nil From d410787d0d8310fea83498cf13dbec39c16e7a9c Mon Sep 17 00:00:00 2001 From: Laura Bennett Date: Mon, 25 Jul 2016 13:29:57 -0400 Subject: [PATCH 4/6] minor edit for error statement --- vault/policy_store.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vault/policy_store.go b/vault/policy_store.go index c409c124ec..22750cf88f 100644 --- a/vault/policy_store.go +++ b/vault/policy_store.go @@ -92,7 +92,7 @@ func (c *Core) setupPolicyStore() error { // Ensure that the cubbyhole response wrapping policy exists policy, err = c.policyStore.GetPolicy(cubbyholeResponseWrappingPolicyName) if err != nil { - return errwrap.Wrapf("error fetching response wrapping policy from store: {{err}}", err) + return errwrap.Wrapf("error fetching response-wrapping policy from store: {{err}}", err) } if policy == nil || policy.Raw != cubbyholeResponseWrappingPolicy { err := c.policyStore.createCubbyholeResponseWrappingPolicy() From fffea5eab9608c6d4adbb09f8f46d21a462bc366 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Mon, 25 Jul 2016 15:59:02 -0400 Subject: [PATCH 5/6] Add test for non-assignable policies --- vault/logical_system_test.go | 12 ++++++------ vault/policy_store_test.go | 3 ++- vault/token_store_test.go | 26 ++++++++++++++++++++++++++ 3 files changed, 34 insertions(+), 7 deletions(-) diff --git a/vault/logical_system_test.go b/vault/logical_system_test.go index f354c2d037..d5c9a6eb71 100644 --- a/vault/logical_system_test.go +++ b/vault/logical_system_test.go @@ -614,8 +614,8 @@ func TestSystemBackend_policyList(t *testing.T) { } exp := map[string]interface{}{ - "keys": []string{"default", "response-wrapping", "root"}, - "policies": []string{"default", "response-wrapping", "root"}, + "keys": []string{"default", "root"}, + "policies": []string{"default", "root"}, } if !reflect.DeepEqual(resp.Data, exp) { t.Fatalf("got: %#v expect: %#v", resp.Data, exp) @@ -667,8 +667,8 @@ func TestSystemBackend_policyCRUD(t *testing.T) { } exp = map[string]interface{}{ - "keys": []string{"default", "foo", "response-wrapping", "root"}, - "policies": []string{"default", "foo", "response-wrapping", "root"}, + "keys": []string{"default", "foo", "root"}, + "policies": []string{"default", "foo", "root"}, } if !reflect.DeepEqual(resp.Data, exp) { t.Fatalf("got: %#v expect: %#v", resp.Data, exp) @@ -702,8 +702,8 @@ func TestSystemBackend_policyCRUD(t *testing.T) { } exp = map[string]interface{}{ - "keys": []string{"default", "response-wrapping", "root"}, - "policies": []string{"default", "response-wrapping", "root"}, + "keys": []string{"default", "root"}, + "policies": []string{"default", "root"}, } if !reflect.DeepEqual(resp.Data, exp) { t.Fatalf("got: %#v expect: %#v", resp.Data, exp) diff --git a/vault/policy_store_test.go b/vault/policy_store_test.go index 3fcd381068..4a0656e23b 100644 --- a/vault/policy_store_test.go +++ b/vault/policy_store_test.go @@ -138,7 +138,8 @@ func TestPolicyStore_Predefined(t *testing.T) { if err != nil { t.Fatalf("err: %v", err) } - if len(out) != 2 || out[0] != "default" || out[1] != "response-wrapping" { + // This shouldn't contain response-wrapping since it's non-assignable + if len(out) != 1 || out[0] != "default" { t.Fatalf("bad: %v", out) } diff --git a/vault/token_store_test.go b/vault/token_store_test.go index dfab0b0d1a..fcc7207bfd 100644 --- a/vault/token_store_test.go +++ b/vault/token_store_test.go @@ -503,6 +503,32 @@ func TestTokenStore_RevokeSelf(t *testing.T) { } } +func TestTokenStore_HandleRequest_NonAssignable(t *testing.T) { + _, ts, _, root := TestCoreWithTokenStore(t) + + req := logical.TestRequest(t, logical.UpdateOperation, "create") + req.ClientToken = root + req.Data["policies"] = []string{"default", "foo"} + + resp, err := ts.HandleRequest(req) + if err != nil { + t.Fatalf("err: %v %v", err, resp) + } + + req.Data["policies"] = []string{"default", "foo", cubbyholeResponseWrappingPolicyName} + + resp, err = ts.HandleRequest(req) + if err != nil { + t.Fatal(err) + } + if resp == nil { + t.Fatal("got a nil response") + } + if !resp.IsError() { + t.Fatalf("expected error; response is %#v", *resp) + } +} + func TestTokenStore_HandleRequest_CreateToken_DisplayName(t *testing.T) { _, ts, _, root := TestCoreWithTokenStore(t) From edc7baab025800780c0247c353bdcef126ccb366 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Mon, 25 Jul 2016 17:05:54 -0400 Subject: [PATCH 6/6] Fix tests --- http/sys_policy_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/http/sys_policy_test.go b/http/sys_policy_test.go index ed8339e620..f0250a01e7 100644 --- a/http/sys_policy_test.go +++ b/http/sys_policy_test.go @@ -17,8 +17,8 @@ func TestSysPolicies(t *testing.T) { var actual map[string]interface{} expected := map[string]interface{}{ - "policies": []interface{}{"default", "response-wrapping", "root"}, - "keys": []interface{}{"default", "response-wrapping", "root"}, + "policies": []interface{}{"default", "root"}, + "keys": []interface{}{"default", "root"}, } testResponseStatus(t, resp, 200) testResponseBody(t, resp, &actual) @@ -62,8 +62,8 @@ func TestSysWritePolicy(t *testing.T) { var actual map[string]interface{} expected := map[string]interface{}{ - "policies": []interface{}{"default", "foo", "response-wrapping", "root"}, - "keys": []interface{}{"default", "foo", "response-wrapping", "root"}, + "policies": []interface{}{"default", "foo", "root"}, + "keys": []interface{}{"default", "foo", "root"}, } testResponseStatus(t, resp, 200) testResponseBody(t, resp, &actual) @@ -100,8 +100,8 @@ func TestSysDeletePolicy(t *testing.T) { var actual map[string]interface{} expected := map[string]interface{}{ - "policies": []interface{}{"default", "response-wrapping", "root"}, - "keys": []interface{}{"default", "response-wrapping", "root"}, + "policies": []interface{}{"default", "root"}, + "keys": []interface{}{"default", "root"}, } testResponseStatus(t, resp, 200) testResponseBody(t, resp, &actual)