While the client secret hashing feature can be a great addition, I've now also come to think it introduces a fairly disruptive change. There are quite a few use cases that rely on client secret in cleartext, and introducing new hashed format(s) would require flexibility to gracefully disable or adapt those.

Supporting Vault SPI for client secret (#13102) could potentially offer much less intrusive alternative to protect client secrets with minimal changes

• When Vault SPI is enabled, the client secret could be resolved via the SPI if it's a reference.
• The UI could be updated to allow setting a user-defined secret value (currently this is only possible via REST API).

I'd be happy to work on this.

When updating a client as "confidential", the secret is not returned. But Keycloak already has the endpoint to generate a new secret for the client: https://www.keycloak.org/docs-api/15.0/rest-api/index.html#_regeneratesecret
This returns a CredentialRepresentation which could provides the secret after generation (and storage of of hash).

Setting a client to "confidential" could either create a plain text secret (as is) or it could create no secret (maybe breaking change which needs compatibility mode?). Getting a hashed secret is then possible with the generate endpoint.

I like that idea. If hashing is not enabled it would just continue working as is. If hashing is enabled that is a "breaking change" anyways as you can no longer retrieve the secret for a client, so needs to be taken into account regardless. I'd say if hashing is enabled and it is a confidential client we could flag it as not having a valid secret, and require calling the regenerate secret separately. Not ideal, but I'm worried that changing the create client to return a full client representation would be a breaking change as well.

It is a breaking change but returning the client representation is what provides the best experience from a client's perspective. Perhaps, even from a UX too when using the administration console.

From a REST perspective, the specs are not 100% clear if returning the newly created resource in the response body is OK but use links to fetch the newly created resource. At the same time, we see a lot of APIs returning a payload in the response representing the newly created resource and we also have endpoints doing the same.

Sure, but that's for short/poor human rememberable passwords. Keycloak generates 32 character random client secrets, so there really is no point in salting something that's already random.

To salt, or not to salt, that is the question. I reckon we'll just do a salt, even though with long random secrets it shouldn't really be needed.

It shouldn't but salt is recommended as per https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html. Even though secrets are generated with enough entropy it seems a common approach to salt (as we do for user password) to make it harder to crack hashes or even correlate secrets from different clients as being the same.

I like the proposal about still allowing using plain text. For me, salt can be optional and not a default when hashing secrets.

We should be able to return the client's secret during creation, no? Not true though if you are querying the client.

Sure, creation is not a problem and we already return in the response to creation. This is per OIDC client registration specification where metadata about newly created client (including client secret) are returned https://openid.net/specs/openid-connect-registration-1_0.html#RegistrationResponse .

The problem is the GET request as I've mentioned. And also the UPDATE request (unless the client secret is rotated/re-generated during update request).

For those operations, I think it is OK to not return it in the response as long as people are aware about the implications.

The problem is the GET request as I've mentioned. And also the UPDATE request (unless the client secret is rotated/re-generated during update request).

I don't think we update the secret during updates, do we? Accordingly to https://www.rfc-editor.org/rfc/rfc7592#page-7, we should not allow clients to manage their secrets as this is a AS-only concern.

Yes, true.

However with client-secret-rotation enabled, the client secret can be automatically updated during client UPDATE request (to the new value generated by Keycloak server, not value provisioned by client himself). So in this case when client-secret-rotation is enabled, we will be able to return newly generated secret from the UPDATE request. When the client secret was not rotated (which is always with disabled client-secret-rotation), we won't be able to return client secret from UPDATE request.

But it's still a matter of plausible deniability: If auth server stores no client secrets, it's very plausible that the client is fully responsibe in case of secret leakage. Like with user passwords, nobody can claim that a provider lost clear text passwords or encrypted passwords if you can proof there were never saved like that in the first place.

That said, for the case where passwords are stored it's still very good to store them encrypted. Just want to point out that this approach still differs very much from storing hash only.

@mposolda I don't think it is an option and I share the arguments from @jbman. I would also add that it adds a lot of complexity and that will probably make life really hard when enabling the capability herein proposed.

Based on OWASP, encryption should not be used for password/secrets. And the reason is pretty much what @jbman mentioned. The original text can still be retrieved.

One thing I was wondering though, I know very few people use encryption at rest. Isn't this an alternative to not only client secrets but any other sensitive data? Including those that involve privacy?

@mposolda I don't think it is an option and I share the arguments from @jbman. I would also add that it adds a lot of complexity and that will probably make life really hard when enabling the capability herein proposed.

Based on OWASP, encryption should not be used for password/secrets. And the reason is pretty much what @jbman mentioned. The original text can still be retrieved.

@jbman @pedroigor Thanks for these points guys. Yes, the added complexity with "valve" is also something, which worries me...

One thing I was wondering though, I know very few people use encryption at rest. Isn't this an alternative to not only client secrets but any other sensitive data? Including those that involve privacy?

I am not sure what you mean? Could you please elaborate a bit more?

Database fully encrypted. Most vendors enabled this including cloud database services like RDS.

Yes, true. So with regards to it, I wonder what is exactly the priority of this task (considering that there are also disadvantages of this...)

I'd say this is still important, and I agree with client credentials being sensitive even if the credentials aren't re-used elsewhere. This especially apply to service accounts. End of the day there are a numbers of ways client credentials can be exposed one important one being through the Admin APIs/console, and is more secure to hash it than not.

Do you have any indication when this will be ready/started? Or where is this on the roadmap? We would really like this feature.

We don't have any plans on working on this anytime soon I'm afraid, but if anyone would like to contribute something that'd be great.

Ok thank you. Do you have any documentation on how to get a client credential flow working with x509 certificates? We have been struggling with finding the right resources to implement this. I thought this would be easy to find seeing this is the preferred way to implement this.

You need to add a CA certificate to the truststore, then just configure the x509 subject dn for the client. Look at the server guides for the first and the admin guide for the second.

I agree with signed jwt or x509 certificates being what should be recommended, as end of the day those are stronger credentials than a shared secret (hashed or not).

I don't think hashing secrets is that hard work though. It is a breaking change sure, which is why it would need to be optional.

Hi team, It's possible ?

Is what possible?