8000 client-secret-rotation, allowing no expiration for new secret by mpugge · Pull Request #12937 · keycloak/keycloak · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
8000

client-secret-rotation, allowing no expiration for new secret #12937

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mpugge
Copy link
@mpugge mpugge commented Jul 6, 2022

Related to the discussion #9156 Client secret policies to support secret rotation, this change allows configuring no expiration for the new secrets generated after rotating the old ones, by using the value 0 in the realm policy.

@marcelomrwin
Copy link
Contributor

@mpugge thanks for your PR. I think that in addition to the discussion, it is necessary to observe the proposed design of the solution https://github.com/keycloak/keycloak-community/blob/main/design/client-secret-rotation.md. Note that the design does not include the possibility of creating a secret that never expires.

Also note that these are two totally different discussions, even though they can work together.

Realize that conceptually if the desire is to have a secret that never expires then it makes no sense to enable secret rotation. Hence I raise the question: Wouldn't it be more advantageous for a client that doesn't want the secret to expire to disable rotation?

@mpugge
Copy link
Author
mpugge commented Jul 6, 2022

@marcelomrwin well I think that the current proposal for secret rotation overlooks a very common use case, where there is the need to replace a possibly compromised secret.

Obviously if there is proof of abuse it would make sense to immediately invalidate the compromised secret, but in case of a proactive action why don't allow to rotate it without enforcing an expiration to the new one?

This will not allow to indefinitely keep around multiple valid secrets forever, it will simply give the option to replace a secret that's in use without disrupting the active clients.

With the current implementation the only way to do it is by manually removing records from the database and than clear the realm cache on all nodes.

Thanks much for your consideration.

@marcelomrwin
Copy link
Contributor

Hi @mpugge, I understand your point of view.

This will not allow to indefinitely keep around multiple valid secrets forever, it will simply give the option to replace a secret that's in use without disrupting the active clients.

Taking this your comment with the current implementation if the client already uses a secret that has just been rotated. The customer is not obliged to exchange immediately. During the rotation configuration, one of the parameters is to define how long a rotated secret is valid. This time must be suitable for the client so that there is enough time to make the switch to the new secret. Likewise, after rotating, it doesn't make sense to have a rotated secret with indefinite validity, otherwise, why was it rotated?

I still understand that the cases you mention are not applicable to secret rotation but to traditionally used secrets. The only disadvantage of this case is that once a new secret is generated, the old one immediately loses its validity.

Thanks again for your contribution

@mpugge
Copy link
Author
mpugge commented Jul 6, 2022

Yes my concerns are only related to the management of manually configured secrets for which no expiration is expected.

Keycloak does not allow to replace them, for any reason, without breaking all the clients using them.

I agree that it does not make sense to set no expiration for an auto rotated secret, but once I saw the new rotation design I thought it could have also be used to manually replace a permanent secret without disrupting the clients.

In the end, would it do any harm to allow configuring no expiration for the new secret?

@mehle
Copy link
mehle commented Sep 1, 2023

For what it’s worth, I would like to see this functionality merged in as well. I am looking at using Keycloak in a situation with a wide group of development teams both internal and external to my organization who are going to have different requirements for secret rotation. Rather than managing each policy for them, I would like to have the option of a blanket policy that allows them to rotate secrets on whatever timeframe they require without being their gatekeeper.

A secondary reason is that I don’t particularly want to get called at all hours because a secret expired in a production application. Sure, it’s the app owner’s responsibility to update the secret before it expires, but that doesn’t mean it won’t also rapidly become the IDP admin’s problem. Ask anyone who has worked with SAML certs for any appreciable amount of time. 😎

Copy link
Contributor
@stianst stianst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes total sense to allow the new secret to not expire. However, we'd need to have tests and documentation updates added to consider this PR.

@drewfreyling
Copy link

Is there an update on this one? @mpugge

@thomasdarimont
Copy link
Contributor

I'd also like to see this feature in Keycloak soon :)
A proper place to test that functionality is here: org.keycloak.testsuite.client.ClientSecretRotationTest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants
0