From 3d76b4b314773d9a1670b05fc5a65e820da3dea3 Mon Sep 17 00:00:00 2001 From: Austin Gebauer Date: Tue, 7 Mar 2023 22:12:57 -0800 Subject: [PATCH 1/2] Invalidates WAL entry for static role if password policy has changed --- backend_test.go | 16 +++++++++-- path_static_roles_test.go | 25 ++++++++++++----- rotation.go | 51 ++++++++++++++++++----------------- rotation_test.go | 57 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 117 insertions(+), 32 deletions(-) diff --git a/backend_test.go b/backend_test.go index 9566155..3442e70 100644 --- a/backend_test.go +++ b/backend_test.go @@ -18,8 +18,12 @@ import ( ) var ( - defaultLeaseTTLVal = time.Hour * 12 - maxLeaseTTLVal = time.Hour * 24 + defaultLeaseTTLVal = time.Hour * 12 + maxLeaseTTLVal = time.Hour * 24 + testPasswordPolicy1 = "test_policy_1" + testPasswordPolicy2 = "test_policy_2" + testPasswordFromPolicy1 = "TestPolicy1Password" + testPasswordFromPolicy2 = "TestPolicy2Password" ) func getBackend(throwsErr bool) (*backend, logical.Storage) { @@ -29,6 +33,14 @@ func getBackend(throwsErr bool) (*backend, logical.Storage) { System: &logical.StaticSystemView{ DefaultLeaseTTLVal: defaultLeaseTTLVal, MaxLeaseTTLVal: maxLeaseTTLVal, + PasswordPolicies: map[string]logical.PasswordGenerator{ + testPasswordPolicy1: func() (string, error) { + return testPasswordFromPolicy1, nil + }, + testPasswordPolicy2: func() (string, error) { + return testPasswordFromPolicy2, nil + }, + }, }, StorageView: &logical.InmemStorage{}, } diff --git a/path_static_roles_test.go b/path_static_roles_test.go index 3f5d8d9..48482b4 100644 --- a/path_static_roles_test.go +++ b/path_static_roles_test.go @@ -741,16 +741,29 @@ func TestWALsDeletedOnRoleDeletion(t *testing.T) { func configureOpenLDAPMount(t *testing.T, b *backend, storage logical.Storage) { t.Helper() + + configureOpenLDAPMountWithPasswordPolicy(t, b, storage, "") +} + +func configureOpenLDAPMountWithPasswordPolicy(t *testing.T, b *backend, storage logical.Storage, policy string) { + t.Helper() + + data := map[string]interface{}{ + "binddn": "tester", + "bindpass": "pa$$w0rd", + "url": "ldap://138.91.247.105", + "certificate": validCertificate, + } + + if policy != "" { + data["password_policy"] = policy + } + resp, err := b.HandleRequest(context.Background(), &logical.Request{ Operation: logical.CreateOperation, Path: configPath, Storage: storage, - Data: map[string]interface{}{ - "binddn": "tester", - "bindpass": "pa$$w0rd", - "url": "ldap://138.91.247.105", - "certificate": validCertificate, - }, + Data: data, }) if err != nil || (resp != nil && resp.IsError()) { t.Fatalf("err:%s resp:%#v\n", err, resp) diff --git a/rotation.go b/rotation.go index e1ce38a..aed7a1b 100644 --- a/rotation.go +++ b/rotation.go @@ -123,11 +123,11 @@ func (b *backend) runTicker(ctx context.Context, s logical.Storage) { // setCredentialsWAL is used to store information in a WAL that can retry a // credential setting or rotation in the event of partial failure. type setCredentialsWAL struct { - NewPassword string `json:"new_password"` - RoleName string `json:"role_name"` - Username string `json:"username"` - DN string `json:"dn"` - + NewPassword string `json:"new_password"` + RoleName string `json:"role_name"` + Username string `json:"username"` + DN string `json:"dn"` + PasswordPolicy string `json:"password_policy"` LastVaultRotation time.Time `json:"last_vault_rotation"` // Private fields which will not be included in json.Marshal/Unmarshal. @@ -258,12 +258,13 @@ func (b *backend) findStaticWAL(ctx context.Context, s logical.Storage, id strin data := wal.Data.(map[string]interface{}) walEntry := setCredentialsWAL{ - walID: id, - walCreatedAt: wal.CreatedAt, - NewPassword: data["new_password"].(string), - RoleName: data["role_name"].(string), - Username: data["username"].(string), - DN: data["dn"].(string), + walID: id, + walCreatedAt: wal.CreatedAt, + NewPassword: data["new_password"].(string), + RoleName: data["role_name"].(string), + Username: data["username"].(string), + DN: data["dn"].(string), + PasswordPolicy: data["password_policy"].(string), } lvr, err := time.Parse(time.RFC3339, data["last_vault_rotation"].(string)) if err != nil { @@ -333,21 +334,22 @@ func (b *backend) setStaticAccountPassword(ctx context.Context, s logical.Storag } switch { - case wal != nil && wal.NewPassword != "": - newPassword = wal.NewPassword - default: - if wal == nil { - b.Logger().Error("expected role to have WAL, but WAL not found in storage", "role", input.RoleName, "WAL ID", output.WALID) - } else { - b.Logger().Error("expected WAL to have a new password set, but empty", "role", input.RoleName, "WAL ID", output.WALID) - err = framework.DeleteWAL(ctx, s, output.WALID) - if err != nil { - b.Logger().Warn("failed to delete WAL with no new password", "error", err, "WAL ID", output.WALID) - } + case wal == nil: + b.Logger().Error("expected role to have WAL, but WAL not found in storage", "role", input.RoleName, "WAL ID", output.WALID) + + // Generate a new WAL entry and credential + output.WALID = "" + case wal.NewPassword != "" && wal.PasswordPolicy != config.PasswordPolicy: + b.Logger().Debug("password policy changed, generating new password", "role", input.RoleName, "WAL ID", output.WALID) + if err := framework.DeleteWAL(ctx, s, output.WALID); err != nil { + b.Logger().Warn("failed to delete WAL", "error", err, "WAL ID", output.WALID) } - // If there's anything wrong with the WAL in storage, we'll need - // to generate a fresh WAL and password + + // Generate a new WAL entry and credential output.WALID = "" + default: + // Reuse the password from the existing WAL entry + newPassword = wal.NewPassword } } @@ -362,6 +364,7 @@ func (b *backend) setStaticAccountPassword(ctx context.Context, s logical.Storag DN: input.Role.StaticAccount.DN, NewPassword: newPassword, LastVaultRotation: input.Role.StaticAccount.LastVaultRotation, + PasswordPolicy: config.PasswordPolicy, }) b.Logger().Debug("wrote WAL", "role", input.RoleName, "WAL ID", output.WALID) if err != nil { diff --git a/rotation_test.go b/rotation_test.go index 76281d7..59b1973 100644 --- a/rotation_test.go +++ b/rotation_test.go @@ -117,6 +117,63 @@ func TestAutoRotate(t *testing.T) { }) } +// TestPasswordPolicyModificationInvalidatesWAL tests that modification of the +// password policy set on the config invalidates pre-generated passwords in WAL +// entries. WAL entries are used to roll forward during partial failure, but +// a password policy change should cause the WAL to be discarded and a new +// password to be generated using the updated policy. +func TestPasswordPolicyModificationInvalidatesWAL(t *testing.T) { + ctx := context.Background() + b, storage := getBackend(false) + defer b.Cleanup(ctx) + + configureOpenLDAPMountWithPasswordPolicy(t, b, storage, testPasswordPolicy1) + createRole(t, b, storage, "hashicorp") + + // Create a WAL entry from a partial failure to rotate + generateWALFromFailedRotation(t, b, storage, "hashicorp") + requireWALs(t, storage, 1) + + // The role password should still be the password generated from policy 1 + role, err := b.staticRole(ctx, storage, "hashicorp") + if err != nil { + t.Fatal(err) + } + if role.StaticAccount.Password != testPasswordFromPolicy1 { + t.Fatal(role.StaticAccount.Password, testPasswordFromPolicy1) + } + + // Update the password policy on the configuration + configureOpenLDAPMountWithPasswordPolicy(t, b, storage, testPasswordPolicy2) + + // Manually rotate the role. It should not use the password from the WAL entry + // created earlier. Instead, it should result in generation of a new password + // using the updated policy 2. + _, err = b.HandleRequest(ctx, &logical.Request{ + Operation: logical.UpdateOperation, + Path: "rotate-role/hashicorp", + Storage: storage, + }) + if err != nil { + t.Fatal(err) + } + + // The role password should be the password generated from policy 2 + role, err = b.staticRole(ctx, storage, "hashicorp") + if err != nil { + t.Fatal(err) + } + if role.StaticAccount.Password != testPasswordFromPolicy2 { + t.Fatal(role.StaticAccount.Password, testPasswordFromPolicy2) + } + if role.StaticAccount.LastPassword != testPasswordFromPolicy1 { + t.Fatal(role.StaticAccount.Password, testPasswordFromPolicy1) + } + + // The WAL entry should be deleted after the successful rotation + requireWALs(t, storage, 0) +} + func TestRollsPasswordForwardsUsingWAL(t *testing.T) { ctx := context.Background() b, storage := getBackend(false) From a77923480cae16ba8dc8ea02f3571c463fff2e85 Mon Sep 17 00:00:00 2001 From: Austin Gebauer Date: Wed, 8 Mar 2023 09:02:01 -0800 Subject: [PATCH 2/2] improve test output for failures --- rotation_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/rotation_test.go b/rotation_test.go index 59b1973..e5805b6 100644 --- a/rotation_test.go +++ b/rotation_test.go @@ -140,7 +140,7 @@ func TestPasswordPolicyModificationInvalidatesWAL(t *testing.T) { t.Fatal(err) } if role.StaticAccount.Password != testPasswordFromPolicy1 { - t.Fatal(role.StaticAccount.Password, testPasswordFromPolicy1) + t.Fatalf("expected %v, got %v", testPasswordFromPolicy1, role.StaticAccount.Password) } // Update the password policy on the configuration @@ -164,10 +164,10 @@ func TestPasswordPolicyModificationInvalidatesWAL(t *testing.T) { t.Fatal(err) } if role.StaticAccount.Password != testPasswordFromPolicy2 { - t.Fatal(role.StaticAccount.Password, testPasswordFromPolicy2) + t.Fatalf("expected %v, got %v", testPasswordFromPolicy2, role.StaticAccount.Password) } if role.StaticAccount.LastPassword != testPasswordFromPolicy1 { - t.Fatal(role.StaticAccount.Password, testPasswordFromPolicy1) + t.Fatalf("expected %v, got %v", testPasswordFromPolicy1, role.StaticAccount.LastPassword) } // The WAL entry should be deleted after the successful rotation