-
Notifications
You must be signed in to change notification settings - Fork 7.4k
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
base: main
Are you sure you want to change the base?
Conversation
@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? |
@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. |
Hi @mpugge, I understand your point of view.
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 |
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? |
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. 😎 |
There was a problem hiding this 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.
Is there an update on this one? @mpugge |
I'd also like to see this feature in Keycloak soon :) |
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.