From f8eb0154d4092f711dbd7ede662603002193e319 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Lapeyre?= Date: Fri, 31 May 2024 15:58:03 +0200 Subject: [PATCH] Fix case handling in policyutil.EquivalentPolicies() (#16484) The previous logic would consider not normalize casing before comparing the policy names which meant that a token associated to a policy with an uppercase could not be renewed for the following auth methods: - AppID - Cert - GitHub - LDAP - Okta - Radius - Userpass Co-authored-by: Violet Hynes --- builtin/credential/cert/backend_test.go | 23 +++++----- changelog/16484.txt | 3 ++ sdk/helper/policyutil/policyutil.go | 24 ++++------ sdk/helper/policyutil/policyutil_test.go | 58 +++++++++++++++++------- 4 files changed, 64 insertions(+), 44 deletions(-) create mode 100644 changelog/16484.txt diff --git a/builtin/credential/cert/backend_test.go b/builtin/credential/cert/backend_test.go index 6260c368e8..e4affa3b52 100644 --- a/builtin/credential/cert/backend_test.go +++ b/builtin/credential/cert/backend_test.go @@ -843,7 +843,7 @@ func TestBackend_NonCAExpiry(t *testing.T) { time.Sleep(5 * time.Second) // Login attempt after certificate expiry should fail - resp, err = b.HandleRequest(context.Background(), loginReq) + _, err = b.HandleRequest(context.Background(), loginReq) if err == nil { t.Fatalf("expected error due to expired certificate") } @@ -2178,12 +2178,13 @@ func Test_Renew(t *testing.T) { Raw: map[string]interface{}{ "name": "test", "certificate": ca, - "policies": "foo,bar", + // Uppercase B should not cause an issue during renewal + "token_policies": "foo,Bar", }, Schema: pathCerts(b).Fields, } - resp, err := b.pathCertWrite(context.Background(), req, fd) + _, err = b.pathCertWrite(context.Background(), req, fd) if err != nil { t.Fatal(err) } @@ -2192,7 +2193,7 @@ func Test_Renew(t *testing.T) { Raw: map[string]interface{}{}, Schema: pathLogin(b).Fields, } - resp, err = b.pathLogin(context.Background(), req, empty_login_fd) + resp, err := b.pathLogin(context.Background(), req, empty_login_fd) if err != nil { t.Fatal(err) } @@ -2219,20 +2220,20 @@ func Test_Renew(t *testing.T) { } // Change the policies -- this should fail - fd.Raw["policies"] = "zip,zap" - resp, err = b.pathCertWrite(context.Background(), req, fd) + fd.Raw["token_policies"] = "zip,zap" + _, err = b.pathCertWrite(context.Background(), req, fd) if err != nil { t.Fatal(err) } - resp, err = b.pathLoginRenew(context.Background(), req, empty_login_fd) + _, err = b.pathLoginRenew(context.Background(), req, empty_login_fd) if err == nil { t.Fatal("expected error") } // Put the policies back, this should be okay - fd.Raw["policies"] = "bar,foo" - resp, err = b.pathCertWrite(context.Background(), req, fd) + fd.Raw["token_policies"] = "bar,foo" + _, err = b.pathCertWrite(context.Background(), req, fd) if err != nil { t.Fatal(err) } @@ -2251,7 +2252,7 @@ func Test_Renew(t *testing.T) { // Add period value to cert entry period := 350 * time.Second fd.Raw["period"] = period.String() - resp, err = b.pathCertWrite(context.Background(), req, fd) + _, err = b.pathCertWrite(context.Background(), req, fd) if err != nil { t.Fatal(err) } @@ -2272,7 +2273,7 @@ func Test_Renew(t *testing.T) { } // Delete CA, make sure we can't renew - resp, err = b.pathCertDelete(context.Background(), req, fd) + _, err = b.pathCertDelete(context.Background(), req, fd) if err != nil { t.Fatal(err) } diff --git a/changelog/16484.txt b/changelog/16484.txt new file mode 100644 index 0000000000..055f214dab --- /dev/null +++ b/changelog/16484.txt @@ -0,0 +1,3 @@ +```release-note:bug +auth/appid, auth/cert, auth/github, auth/ldap, auth/okta, auth/radius, auth/userpass: fixed an issue with policy name normalization that would prevent a token associated with a policy containing an uppercase character to be renewed. +``` diff --git a/sdk/helper/policyutil/policyutil.go b/sdk/helper/policyutil/policyutil.go index a5a8082e13..a8e6214ab0 100644 --- a/sdk/helper/policyutil/policyutil.go +++ b/sdk/helper/policyutil/policyutil.go @@ -79,33 +79,25 @@ func SanitizePolicies(policies []string, addDefault bool) []string { // EquivalentPolicies checks whether the given policy sets are equivalent, as in, // they contain the same values. The benefit of this method is that it leaves // the "default" policy out of its comparisons as it may be added later by core -// after a set of policies has been saved by a backend. +// after a set of policies has been saved by a backend and performs policy name +// normalization. func EquivalentPolicies(a, b []string) bool { - switch { - case a == nil && b == nil: - return true - case a == nil && len(b) == 1 && b[0] == "default": - return true - case b == nil && len(a) == 1 && a[0] == "default": - return true - case a == nil || b == nil: - return false - } - // First we'll build maps to ensure unique values and filter default - mapA := map[string]bool{} - mapB := map[string]bool{} + mapA := map[string]struct{}{} + mapB := map[string]struct{}{} for _, keyA := range a { + keyA := strings.ToLower(keyA) if keyA == "default" { continue } - mapA[keyA] = true + mapA[keyA] = struct{}{} } for _, keyB := range b { + keyB := strings.ToLower(keyB) if keyB == "default" { continue } - mapB[keyB] = true + mapB[keyB] = struct{}{} } // Now we'll build our checking slices diff --git a/sdk/helper/policyutil/policyutil_test.go b/sdk/helper/policyutil/policyutil_test.go index 2280ba93ee..04aedfae72 100644 --- a/sdk/helper/policyutil/policyutil_test.go +++ b/sdk/helper/policyutil/policyutil_test.go @@ -56,24 +56,48 @@ func TestParsePolicies(t *testing.T) { } func TestEquivalentPolicies(t *testing.T) { - a := []string{"foo", "bar"} - var b []string - if EquivalentPolicies(a, b) { - t.Fatal("bad") + testCases := map[string]struct { + A []string + B []string + Expected bool + }{ + "nil": { + A: nil, + B: nil, + Expected: true, + }, + "empty": { + A: []string{"foo", "bar"}, + B: []string{}, + Expected: false, + }, + "missing": { + A: []string{"foo", "bar"}, + B: []string{"foo"}, + Expected: false, + }, + "equal": { + A: []string{"bar", "foo"}, + B: []string{"bar", "foo"}, + Expected: true, + }, + "default": { + A: []string{"bar", "foo"}, + B: []string{"foo", "default", "bar"}, + Expected: true, + }, + "case-insensitive": { + A: []string{"test"}, + B: []string{"Test"}, + Expected: true, + }, } - b = []string{"foo"} - if EquivalentPolicies(a, b) { - t.Fatal("bad") - } - - b = []string{"bar", "foo"} - if !EquivalentPolicies(a, b) { - t.Fatal("bad") - } - - b = []string{"foo", "default", "bar"} - if !EquivalentPolicies(a, b) { - t.Fatal("bad") + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + if EquivalentPolicies(tc.A, tc.B) != tc.Expected { + t.Fatal("bad") + } + }) } }