Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
DPoP design doc #254
base: main
Are you sure you want to change the base?
DPoP design doc #254
Changes from 1 commit
baffdfd
45e3308
abdb40b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is this config option really needed? Just wondering that ideal would be to avoid many configuration options as each configuration option is adding some complexity (And Keycloak client already has really big amount of configuration options, but even if we do with client policies, one less config option is always good :)). For example JWTClientAuthenticator does not check that "iat" from the token is in the past (see https://github.com/keycloak/keycloak/blob/15.0.2/services/src/main/java/org/keycloak/authentication/authenticators/client/JWTClientAuthenticator.java) and hence it seems to me to have same behaviour for DPoP as well?
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.
Did you perhaps mean "in the future"? The only check that is performed against
iat
is this:That means, we're accepting
iat
values that are no more than 10 seconds into the past + values set in indefinite future.Unfortunately, this might not work for DPoP proofs. Let's revisit section 10.1 "DPoP Proof Replay":
Key points are:
jti
values;jti
values in memory only for the limited time window (aka DPoP proof lifetime), otherwise we'll quickly exhaust memory;nbf
norexp
claims, onlyiat
which is provided by the client; hence, we should enforce limited window of validity based on client-suppliediat
only;iat
s set in indefinite future, since the server will have to storejti
until (iat
+ lifetime) is reached, which creates possibility of conducting memory exhaustion attacks. Hence, allowed clock skew has to be finite.That said, we should have two server-side parameters, one for DPoP proof lifetime and another one for allowed clock skew. I decided to make them configurable, in order to better suit particular setups and security requirements, similar to what we have in another parts of Keycloak (configurable token lifetimes; configurable clock skews for adapters and IdPs).
The recent version of the DPoP draft offers an alternative mechanism of limiting DPoP proof lifetimes, which is based on server-supplied nonces rather than clock times (see 8. Authorization Server-Provided Nonce and 9. Resource Server-Provided Nonce) and is said to "yield intended results even in the face of arbitrarily large clock skews". However, this method is more complex and won't be featured in the initial PR. And even with this method we'll have to somehow configure lifetimes of server-supplied nonces.
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.
To simplify the implementation, it is only needed to add one client settings:
dpop_bound_access_tokens is a client metadata defined in the DPoP specification.
The boolean value is easy to handle compared with ternary (enabled/optional/disabled).
As for DPoP proof lifetime and clock skew, it might be better to use default values for all clients. It might be better not to add new client settings because keycloak has already a lot of such settings and these settings are difficult to be managed.
If we add client settings at this time and we add some method (e.g., client policies) to remove necessities of the settings afterwards, we cannnot delete these settings because we need to keep backward compatibility.
Therefore, at this time, it might be preferable to use default values only for DPoP proof lifetime and clock skew, and add some mechanism to change these values per client or set of clients without adding client settings afterwards.
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.
TBH, I would leave adapters as the lowest priority unless there is an explicit request for adding DPoP support to them. Do you have a need for adapters support for your deployment?
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.
Agreed; no, we do not have any requirements for the adapters.
I think we can focus this document solely on the AS side of things, as the RS side should probably belong to another design document. OK if I remove adapter-specific info and mention the current focus?
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.
As you pointed, this is quite challenging to support especially in the cluster environment. I would personally not support this requirement for now (Specification says "SHOULD") unless there is very strong requirement of it. I see there is some security gain, however the price to pay store additional data on the resource servers side is the challenge (especially for the clustering environment).
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.
Agreed. This should probably also belong to a RS-specific design document.