From f838c6a7cdfbc8c5fe625728fcc21abdf7c29303 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Fri, 18 Aug 2017 16:50:31 -0400 Subject: [PATCH] Properly lowercase policy names. Previously we lowercased names on ingress but not on lookup or delete which could cause unexpected results. Now, just unilaterally lowercase policy names on write and delete. On get, to avoid the performance hit of always lowercasing when not necessary since it's in the critical path, we have a minor optimization -- we check the LRU first before normalizing. For tokens, because they're already normalized when adding policies during creation, this should always work; it might just be slower for API calls. Fixes #3187 --- command/policy_write.go | 3 ++- vault/acl_test.go | 4 ++-- vault/logical_system.go | 7 ++++--- vault/logical_system_test.go | 8 ++++++-- vault/policy_store.go | 9 +++++++++ vault/policy_store_test.go | 10 +++++----- 6 files changed, 28 insertions(+), 13 deletions(-) diff --git a/command/policy_write.go b/command/policy_write.go index 4f73ffe13ff8..59b26fb472e8 100644 --- a/command/policy_write.go +++ b/command/policy_write.go @@ -37,7 +37,8 @@ func (c *PolicyWriteCommand) Run(args []string) int { return 2 } - name := args[0] + // Policies are normalized to lowercase + name := strings.ToLower(args[0]) path := args[1] // Read the policy diff --git a/vault/acl_test.go b/vault/acl_test.go index 2d96b577dabf..638fed6d0c53 100644 --- a/vault/acl_test.go +++ b/vault/acl_test.go @@ -453,7 +453,7 @@ path "auth/token/create*" { ` var aclPolicy = ` -name = "dev" +name = "DeV" path "dev/*" { policy = "sudo" } @@ -482,7 +482,7 @@ path "foo/bar" { ` var aclPolicy2 = ` -name = "ops" +name = "OpS" path "dev/hide/*" { policy = "deny" } diff --git a/vault/logical_system.go b/vault/logical_system.go index 25a41f5097bd..0fab16f12145 100644 --- a/vault/logical_system.go +++ b/vault/logical_system.go @@ -1846,7 +1846,7 @@ func (b *SystemBackend) handlePolicyRead( return &logical.Response{ Data: map[string]interface{}{ - "name": name, + "name": policy.Name, "rules": policy.Raw, }, }, nil @@ -1873,8 +1873,9 @@ func (b *SystemBackend) handlePolicySet( return handleError(err) } - // Override the name - parse.Name = strings.ToLower(name) + if name != "" { + parse.Name = name + } // Update the policy if err := b.Core.policyStore.SetPolicy(parse); err != nil { diff --git a/vault/logical_system_test.go b/vault/logical_system_test.go index 69403b34b3bc..ecc5362eca34 100644 --- a/vault/logical_system_test.go +++ b/vault/logical_system_test.go @@ -1239,8 +1239,12 @@ func TestSystemBackend_policyCRUD(t *testing.T) { t.Fatalf("err: %v", err) } - if resp != nil { - t.Fatalf("err: expected nil response, got %#v", *resp) + exp = map[string]interface{}{ + "name": "foo", + "rules": rules, + } + if !reflect.DeepEqual(resp.Data, exp) { + t.Fatalf("got: %#v expect: %#v", resp.Data, exp) } // List the policies diff --git a/vault/policy_store.go b/vault/policy_store.go index bb2b56fd18e9..f3368d6e8dcd 100644 --- a/vault/policy_store.go +++ b/vault/policy_store.go @@ -200,6 +200,8 @@ func (ps *PolicyStore) SetPolicy(p *Policy) error { if p.Name == "" { return fmt.Errorf("policy name missing") } + // Policies are normalized to lower-case + p.Name = strings.ToLower(strings.TrimSpace(p.Name)) if strutil.StrListContains(immutablePolicies, p.Name) { return fmt.Errorf("cannot update %s policy", p.Name) } @@ -230,6 +232,7 @@ func (ps *PolicyStore) setPolicyInternal(p *Policy) error { // GetPolicy is used to fetch the named policy func (ps *PolicyStore) GetPolicy(name string) (*Policy, error) { defer metrics.MeasureSince([]string{"policy", "get_policy"}, time.Now()) + if ps.lru != nil { // Check for cached policy if raw, ok := ps.lru.Get(name); ok { @@ -237,6 +240,9 @@ func (ps *PolicyStore) GetPolicy(name string) (*Policy, error) { } } + // Policies are normalized to lower-case + name = strings.ToLower(strings.TrimSpace(name)) + // Special case the root policy if name == "root" { p := &Policy{Name: "root"} @@ -320,6 +326,9 @@ 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()) + + // Policies are normalized to lower-case + name = strings.ToLower(strings.TrimSpace(name)) if strutil.StrListContains(immutablePolicies, name) { return fmt.Errorf("cannot delete %s policy", name) } diff --git a/vault/policy_store_test.go b/vault/policy_store_test.go index dafca34c3f32..97107f148390 100644 --- a/vault/policy_store_test.go +++ b/vault/policy_store_test.go @@ -61,7 +61,7 @@ func TestPolicyStore_CRUD(t *testing.T) { func testPolicyStore_CRUD(t *testing.T, ps *PolicyStore) { // Get should return nothing - p, err := ps.GetPolicy("dev") + p, err := ps.GetPolicy("Dev") if err != nil { t.Fatalf("err: %v", err) } @@ -70,7 +70,7 @@ func testPolicyStore_CRUD(t *testing.T, ps *PolicyStore) { } // Delete should be no-op - err = ps.DeletePolicy("dev") + err = ps.DeletePolicy("deV") if err != nil { t.Fatalf("err: %v", err) } @@ -92,7 +92,7 @@ func testPolicyStore_CRUD(t *testing.T, ps *PolicyStore) { } // Get should work - p, err = ps.GetPolicy("dev") + p, err = ps.GetPolicy("dEv") if err != nil { t.Fatalf("err: %v", err) } @@ -110,13 +110,13 @@ func testPolicyStore_CRUD(t *testing.T, ps *PolicyStore) { } // Delete should be clear the entry - err = ps.DeletePolicy("dev") + err = ps.DeletePolicy("Dev") if err != nil { t.Fatalf("err: %v", err) } // Get should fail - p, err = ps.GetPolicy("dev") + p, err = ps.GetPolicy("deV") if err != nil { t.Fatalf("err: %v", err) }