Use constant time comparison for client secret verification #1861
Conversation
xtremerui
force-pushed
the
pr/bcrypt-for-client-secret-sync
branch
from
8718f81
to
40dc061
Compare
xtremerui
force-pushed
the
pr/bcrypt-for-client-secret-sync
branch
from
40dc061
to
cbe0a17
Compare
- this assumes that the client is already bcrytped when passed to dex. Similar to user passwords. Signed-off-by: Josh Winters <jwinters@pivotal.io> Co-authored-by: Vikram Yadav <vyadav@pivotal.io>
* if enabled, it will make sure client secret is bcrypted correctly * if not, it falls back to old behaviour that allowing empty client secret and comparing plain text, though now it will do ConstantTimeCompare to avoid a timing attack. So in either way it should provide more secure of client secret verification. Co-authored-by: Alex Surraci <suraci.alex@gmail.com> Signed-off-by: Rui Yang <ruiya@vmware.com>
xtremerui
force-pushed
the
pr/bcrypt-for-client-secret-sync
branch
from
cbe0a17
to
d658c24
Compare
|
@xtremerui I agree that using |
the client secret coming in should be hashed and the one in storage is the one in plaintext Signed-off-by: Rui Yang <ruiya@vmware.com>
|
@tkleczek FYI I just pushed a commit to fix a bug in the comparison that client secret in request should be hashed and client secret in dex storage should be plain text. Though I don't think your question is specific on the implementation. I agree, not liking a password that could be used widely, a client secret is mostly specific to the client. Though I think it doesn't hurt to provide a way to hide client secret in request in case some one doesn't want client secret to be exposed anyway. For public client that has no secret, the original logic still applies. Thx for commenting! |
Not sure now, maybe I was mistaken.
I think it hurts mostly in the additional code that has to be maintained and excessive options in configuration. I don't think you have to protect client secret in transit more than standard TLS does. OIDC standard also does not mention this "extension". Btw, you might want to split this PR in two separate ones. I think using subtle.ConstantTimeCompare is has obvious advantages so it could hopefully go in quicker |
yes I think that makes sense. In our own use case, concourse CLI is actually a public client and we have the client secret in OSS github code. So technically this PR is also an option for us but not a must. I will update this PR to include only subtle.ConstanTimeCompare. Thx! |
constant time compare for client secret verification will be kept Signed-off-by: Rui Yang <ruiya@vmware.com>
xtremerui
changed the title
Overview:
This PR adds an option to Dex config to enable enforement of client secret encryption.Use subtle.ConstantTImeCompare for client secret verification to prevent a potential timing acttack
What problem does it solve?:
The current implementation for checking client secret is by plain text comparison, which is vulnerable to timing attack. Using constant time comparison will mitigate the threat.
Thx!