Repository navigation
Add DPoP support to keycloak-js - #253
Conversation
5cc24c1 to
eb42d36
Compare
|
@jonkoops Do you please have opportunity to review? I will try to review as well, but feedback from you would be welcome if you have a chance! |
|
Yes, I will take a look and review. Currently looking at the last bugs for 26.2 that I want to resolve, after that I am going to start merging in features and deprecations for 26.3. |
|
@sundayScoop can you fix the lint errors though |
eb42d36 to
8d65d02
Compare
|
@edewit just fixed the lint errors, let me know if there's anything else needed |
|
👍 |
There was a problem hiding this comment.
@sundayScoop Thanks, from a DPoP RFC point of view, it’s ok for me. I’ll leave to the others a more js-focused review.
|
@jonkoops Can you please reply here if you have a chance to review in the near future? Otherwise going to merge this without your review. @graziang Thanks for your review! @sundayScoop Thanks again for the PR! We may need also documentation for this feature in the keycloak.js documentation . See this directory https://github.com/keycloak/keycloak-js/tree/main/docs . Probably worth to introduce dedicated section about DPoP and document it here? Besides configuration options, it will be good to document:
|
|
Please hold off on merging this PR until we have done a final patch release. I intend to merge this in for 26.3.0, together with some other work for a feature release. |
@mposolda regarding the crypto API suggestion - there is this check in keycloak-dpop.js: if (typeof crypto === 'undefined' || !crypto.subtle) {
throw new Error('DPoP requires Web Crypto API (crypto.subtle) which is not available in this environment.')
}Were you thinking of explicitly showing a warning message to the user without an error? Or just improving the message in the error being thrown? I realise there's no mention of the https requirement in the error, so that might be good to add. Aside from that I've just finished the requested docs - so nearly there! |
|
We already have places where we detect the Crypto API and throw if it is not found: Lines 1623 to 1625 in fa52139 Lines 1689 to 1691 in fa52139 Lines 1946 to 1948 in fa52139 Keycloak JS relies on the Web Crypto API, and therefore only works in a secure context. Perhaps we can roll up this feature detection into a single utility function that is called in all the places where the API is called, to reduce repetition. |
@sundayScoop I was concerned just about documentation here. It can be good to explicitly mention in the keycloak.js documentation that DPoP would be available just if Crypto API is available. And use some warning/formulation in the docs about this similar to what I've mentioned. For example see some warning in the current keycloak.js documentation https://www.keycloak.org/securing-apps/javascript-adapter#_using_the_adapter (The warning in this section about "Silent check-sso functionality is limited in some modern browsers." ) |
@jonkoops Cool, Thanks! Any ETA when it could be merged approximately? And when you plan 26.3.0 release? |
At most in a couple of weeks. I am currently rather busy with my talk for BackstageCon, so my bandwidth is limited. |
8d65d02 to
fc1530a
Compare
|
@mposolda just added the requested docs. Sorry for the delay - had quite a busy few weeks. Let me know what you think of the docs |
There was a problem hiding this comment.
@sundayScoop Thanks for the documentation! I am approving this PR from my point of view. The merge might be delayed though (See comments above from @jonkoops ), but hopefully we would be able to do it in few weeks
Update: There is the issue in Build guides . Are you able to take a look?
|
Looking now |
|
Created #329 to discuss the v27 readiness. So we don't further pollute this PR with unrelated chatter. |
|
@jonkoops Sorry, but that did not answered my question from the comment #253 (comment) :-) Could you please share here or in that issue #329 what do you see as next steps to make it finally possible to merge this PR? |
There was a problem hiding this comment.
Preliminary review, some comments I already prepped a while ago, but never got around to finishing my review.
| /** | ||
| * Enables DPoP token auth flows and session key generation. | ||
| */ | ||
| useDPoP?: DPoPConfig |
There was a problem hiding this comment.
I'd rather not call an option that provides a config useX, as this implies a boolean value. Ideally this would be something like dpopConfig.
| useDPoP?: DPoPConfig | |
| dpopConfig?: DPoPConfig |
| } | ||
| } | ||
|
|
||
| secureFetch = async (url, init = {}) => { |
There was a problem hiding this comment.
This method is not annotated with JSDOC types like other functions, which is causing some type errors in IDEs. We're trying to add types wherever we can, so that this code can be easily converted to pure TypeScript at a later point.
There was a problem hiding this comment.
Also it would be better to just call it fetch as then it will then be keycloak.fetch
|
Thanks for taking the time to review my work guys! I'll look at it tomorrow so we can get this in ASAP |
f726d31 to
33de9b2
Compare
|
I've fixed the DCO sign-offs on my commits (they were missing https://github.com/edewit/keycloak-js/tree/fix/pr-253-dco Tree content is unchanged — only commit messages were updated to add sign-off on my four commits. @sundayScoop could you please update git fetch https://github.com/edewit/keycloak-js.git fix/pr-253-dco
git checkout dpop-support
git reset --hard FETCH_HEAD
git push --force-with-lease origin dpop-supportThat should clear the DCO check. Thanks! |
33de9b2 to
96b2a56
Compare
|
@sundayScoop @edewit could you rebase this branch on |
Closes keycloak#8 Signed-off-by: sundayScoop <killer.of.time@protonmail.com> Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Consolidate identical access/refresh token DPoP fetch paths, extract shared nonce validation and secureFetch proof building, and add reusable DPoP test helpers with parameterized algorithm tests. Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Flush and re-initialize DPoP keys in clearToken() to match logout behavior, expose strictStorage on useDPoP config, and add a test verifying key rotation after clearToken. Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Aligns the public API with review feedback: init option is dpopConfig and the DPoP-aware HTTP helper is keycloak.fetch(). Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Closes keycloak#8 Signed-off-by: sundayScoop <killer.of.time@protonmail.com> Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: sundayScoop <killer.of.time@protonmail.com> Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: sundayScoop <killer.of.time@protonmail.com> Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: sundayScoop <killer.of.time@protonmail.com> Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Head branch was pushed to by a user without write access
|
When it could be released? |
|
If it were up to me, ASAP. But unfortunately, @pskopek is the only one who can do releases. |
Add Demonstration of Proof-of-Possession (DPoP) token support as per the RFC, addressing issue #8.
Key Changes
secureFetch()wrapper so developers don't have to deal with DPoP flow management (e.g. new nonce provided)Testing
dpop.spec.tsNotes
Enabling DPoP Example
Closes #8