Add a /config/rotate-root path to the ldap auth backend #24099

kpcraig · 2023-11-10T18:21:38Z

This PR adds a rotate-root functionality to the LDAP auth backend.

github-actions · 2023-11-10T18:47:28Z

Build Results:
All builds succeeded! ✅

github-actions · 2023-11-10T18:52:05Z

CI Results:
All Go tests succeeded! ✅

raymonstah

Are there any tests we can add to validate the new path?

raymonstah · 2023-11-10T18:55:04Z

builtin/credential/ldap/path_config_rotate_root.go

+	// grab our ldap client
+	client := ldaputil.Client{
+		Logger: b.Logger(),
+		LDAP:   ldaputil.NewLDAP(),
+	}
+


Can this live on the *backend struct so we can reuse it or does the client need to be created every time?
Also, there's a newer cap/ldap client that we're trying to migrate to. Any reason not to use this one instead?

+1 on potentially adding a getClient helper on the backend struct. That would also match up with other plugins

i thought about this - this is the only place this particular is currently used in this (credential/)ldap, and i want to avoid confusion with the other similarly named one (cap/ldap/Client).

Possibly whoever adds the second use of this client can add the helper?

Not overly concerned with having a helper method, but more so wondering why we can't reuse the same instance of the ldap client. Probably not too concerning though, assuming that root rotation isn't a high-volume endpoint.

I didn't notice the earlier mention of the cap client the first time around - the issue i encountered was that it wasn't interacting with LDAP at the level I needed to make Modify calls to the server, although maybe there was a way to do it that I missed.

I'm not too worried about multiple instantiations of the ldap client - whatever we're allocating is pretty thin each time, and rotate-root is going to be pretty low volume.

builtin/credential/ldap/path_config_rotate_root.go

builtin/credential/ldap/backend.go

builtin/credential/ldap/path_config.go

builtin/credential/ldap/path_config_rotate_root.go

fairclothjm

Are there any tests we could add to this?

kpcraig · 2023-11-16T17:52:53Z

Are there any tests we could add to this?

I added a test cribbing from backend_test. As you might imagine, it's based on having an external ldap set up, but i matched the expectations to the one from the backend tests.

builtin/credential/ldap/path_config_rotate_root_test.go

builtin/credential/ldap/path_config_rotate_root.go

builtin/credential/ldap/path_config_rotate_root_test.go

changelog/24099.txt

austingebauer

Looking good! Will ✅ after the comments are addressed.

builtin/credential/ldap/path_config_rotate_root.go

builtin/credential/ldap/path_config_rotate_root_test.go

vinay-gopalan

LGTM! Had a small additional nit around copywrite headers for getting CI cleared

builtin/credential/ldap/path_config_rotate_root.go

austingebauer

🎉

kpcraig added 2 commits November 9, 2023 16:32

rotate-root

0502420

Merge branch 'main' into VAULT-21415/ldap-auth-rotate-root

2d66dfd

kpcraig requested a review from a team as a code owner November 10, 2023 18:21

github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Nov 10, 2023

add changelog, remove debug response

b052fc3

raymonstah reviewed Nov 10, 2023

View reviewed changes

austingebauer reviewed Nov 10, 2023

View reviewed changes

builtin/credential/ldap/path_config.go Show resolved Hide resolved

builtin/credential/ldap/path_config_rotate_root.go Show resolved Hide resolved

builtin/credential/ldap/path_config_rotate_root.go Outdated Show resolved Hide resolved

vinay-gopalan reviewed Nov 10, 2023

View reviewed changes

builtin/credential/ldap/path_config_rotate_root.go Outdated Show resolved Hide resolved

kpcraig added 8 commits November 13, 2023 13:42

remove stray log line

2d0e236

more stray log lines

613bf5a

basic storage of new password

92e6d5f

return policy (name) with config read request

6264368

add ForwardStandby, etc

a32e45e

add write mutex for rotate-root

958b5c5

add read/write lock on more config accesses

ef2f8ec

Merge branch 'main' into VAULT-21415/ldap-auth-rotate-root

02a85a1

kpcraig added this to the 1.16.0-rc1 milestone Nov 16, 2023

fairclothjm reviewed Nov 16, 2023

View reviewed changes

add a basic test, cribbing from backend_test

5727653

maxcoulombe reviewed Nov 16, 2023

View reviewed changes

builtin/credential/ldap/path_config_rotate_root_test.go Show resolved Hide resolved

vinay-gopalan reviewed Nov 16, 2023

View reviewed changes

builtin/credential/ldap/path_config_rotate_root.go Outdated Show resolved Hide resolved

vinay-gopalan reviewed Nov 16, 2023

View reviewed changes

builtin/credential/ldap/path_config_rotate_root_test.go Show resolved Hide resolved

add useful error message on a nil config

88a791a

fairclothjm reviewed Nov 17, 2023

View reviewed changes

changelog/24099.txt Outdated Show resolved Hide resolved

additionally check the response in the test for an error

1223cab

kpcraig force-pushed the VAULT-21415/ldap-auth-rotate-root branch from 49ce79f to 1223cab Compare November 17, 2023 17:46

somehow we lost the changelog

9bdeb18

reformat to use PrepareTestContainer

f94e892

raymonstah approved these changes Nov 22, 2023

View reviewed changes

austingebauer self-requested a review November 22, 2023 17:28

austingebauer reviewed Nov 22, 2023

View reviewed changes

vinay-gopalan approved these changes Nov 22, 2023

View reviewed changes

builtin/credential/ldap/path_config_rotate_root.go Show resolved Hide resolved

kpcraig added 4 commits November 22, 2023 15:24

return a descriptive error to the caller on nil config

6dd9bfe

add license to new files

7ba417c

fix test

21d7f1f

clarify the preconditions for the test

0439a51

austingebauer approved these changes Nov 22, 2023

View reviewed changes

kpcraig merged commit 9b7d068 into main Nov 27, 2023
109 checks passed

kpcraig deleted the VAULT-21415/ldap-auth-rotate-root branch November 27, 2023 20:48

Monkeychip mentioned this pull request Nov 28, 2023

Fix failing LDAP test with new attribute #24273

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a /config/rotate-root path to the ldap auth backend #24099

Add a /config/rotate-root path to the ldap auth backend #24099

kpcraig commented Nov 10, 2023

github-actions bot commented Nov 10, 2023 •

edited

github-actions bot commented Nov 10, 2023 •

edited

raymonstah left a comment

raymonstah Nov 10, 2023

vinay-gopalan Nov 10, 2023

kpcraig Nov 16, 2023

raymonstah Nov 17, 2023

kpcraig Nov 21, 2023

fairclothjm left a comment

kpcraig commented Nov 16, 2023

austingebauer left a comment

vinay-gopalan left a comment

austingebauer left a comment

Add a /config/rotate-root path to the ldap auth backend #24099

Add a /config/rotate-root path to the ldap auth backend #24099

Conversation

kpcraig commented Nov 10, 2023

github-actions bot commented Nov 10, 2023 • edited

github-actions bot commented Nov 10, 2023 • edited

raymonstah left a comment

Choose a reason for hiding this comment

raymonstah Nov 10, 2023

Choose a reason for hiding this comment

vinay-gopalan Nov 10, 2023

Choose a reason for hiding this comment

kpcraig Nov 16, 2023

Choose a reason for hiding this comment

raymonstah Nov 17, 2023

Choose a reason for hiding this comment

kpcraig Nov 21, 2023

Choose a reason for hiding this comment

fairclothjm left a comment

Choose a reason for hiding this comment

kpcraig commented Nov 16, 2023

austingebauer left a comment

Choose a reason for hiding this comment

vinay-gopalan left a comment

Choose a reason for hiding this comment

austingebauer left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 10, 2023 •

edited

github-actions bot commented Nov 10, 2023 •

edited