Skip to content
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

Magic link doesn't support PKCE #8

Open
L-E opened this issue Sep 16, 2022 · 22 comments
Open

Magic link doesn't support PKCE #8

L-E opened this issue Sep 16, 2022 · 22 comments
Assignees
Labels
enhancement New feature or request help wanted Extra attention is needed priority

Comments

@L-E
Copy link

L-E commented Sep 16, 2022

I was planning to the magic link extension with a standard Authorization Flow incl. PKCE (the use case is a mobile app logging in a user via email to access an API). In general everything works fine, but at the end I realised that it doesn't seem to work with PKCE, i.e., the code_verifier is ignored when sending it.

My assumption was that the magic-link extension doesn't really get involved in PKCE handling, but when I use something else (e.g., standard username+password) instead of the magic link it works fine.

Is this not possible when using the magic link or is it just a limitation/bug in the magic link extension... or am I doing something wrong?

Example request to initiate the flow:
https://my-keycloak/realms/test/protocol/openid-connect/auth?client_id=my-client&response_type=code&scope=email openid&redirect_uri=https://my-redirect-url/&state=my-random-state&nonce=my-random-nonce&code_challenge=Uz9stxVbetoK4mdkE8LLzl7UyMIPra5DyIputr9FWCA&code_challenge_method=S256&[email protected]

When clicking the magic link in the email https://my-keycloak/realms/test/login-actions/action-token?key=eyJhbGciOiJIUzI1NiIsIn....&client_id=my-client
I'm getting a redirect that only contains code and session_state, but not state or nonce.

Requesting the access_token using the code from the magic-link response with an INVALID code_verifier still gives me a valid access_token. Keycloak doesn't seem to expect a code_verifier and silently ignores it.

@xgp
Copy link
Member

xgp commented Sep 16, 2022

@L-E The current approach creates an entirely new login session, and it doesn't reuse the state/nonce values or carry the code_verifier to the action token. So currently a limitation. I can experiment with adding it. Thanks for submitting this, as I hadn't considered it before. If you want to take a look, the relevant code is in the MagicLinkAuthenticator and MagicLinkActionTokenHandler classes.

@yordis
Copy link

yordis commented Dec 4, 2022

Any updates @xgp? Thanks in advanced.

@xgp
Copy link
Member

xgp commented Dec 4, 2022

@yordis Happy to accept PRs!

@yordis
Copy link

yordis commented Dec 4, 2022

Believe me, I would do it by now if I even knew how to get started with Java 😭 ... I tried and I get lost in DX tools and whatnot, not counting the Keycloak abstraction on top of that 😢

@xgp xgp added the priority label Dec 5, 2022
@sweeneytr
Copy link

+1 to this, because state is an essential element in OAuth security this is unusable w/ the libraries we're using.

@xgp
Copy link
Member

xgp commented May 12, 2023

Agreed that this is important, as many use cases require PKCE. However, the problem in this implementation is that we use the action token to make it essentially stateless, whereas PKCE requires the server to store a value. I've looked at a few ideas, including storing the value as a temporary "credential" for the user. Any additional suggestions/ideas for how to support the PKCE flow are welcome.

@sweeneytr
Copy link

So, PKCE and state are different, so far as I understand it, that serve different purposes. state is an opaque value that is passed in the query-params when redirecting to the auth server, which needs to be passed when redirecting back to the app. It's the mechanism by which the app is aware that a GET /auth-callback was initiated by it.

Without state, you're vulnerable to someone sending a user a link like https://yoursite.com/auth-callback?code=<code_for_attacker>, and putting them into a compromised state. Auth0 has good docs on this. Most libraries we work with force the use of state, it'd be a PITA to work around.

I'm taking a crack at some code changes.

@xgp
Copy link
Member

xgp commented May 12, 2023

Where I'm hung up is whether or not it is safe to put any of the values (state, or code_challenge for PKCE) in the action token.

If we did put state in there then it would allow us to return it to the client in the callback. Similarly, if we put the code_challenge, then it would allow us to validate the code_verifier when the code exchange happened.

In any case (current or including those params) if attacker gets the action token, they're getting in, so it's not worse than today to include them.

However, if we wanted to preserve them on the server, rather than in the action token, that would be a heavier lift, as it would require some mechanism of persisting them between the initial action token creation and the handling of the action token. If it's not clear why, this is because in Keycloak, these are technically two separate authentication sessions. We had to take this route in order to allow using the link on different devices.

@cedricjacobs
Copy link

So, I am struggling to implement it in my expressJS backend as I use passport in combination with openid-client.
The message I am seeing appearing is very similar (state missing from response).

So my question is: how do you configure it then? if it is not a PKCE flow, which type is it then?

@xgp
Copy link
Member

xgp commented May 13, 2023

@cedricjacobs it's a normal OIDC Authorization Code Flow without the state parameter.

@cedricjacobs
Copy link

The weird thing is, that it does everything quite perfectly, but for the callback. Is there like an example backend where this is implemented?
(just to be clear, it's a backend that serves HTML)

@xgp
Copy link
Member

xgp commented May 13, 2023

"but for the callback". Do you have more information? Can you capture a HAR of what is going on and post it? Or at least what your backend is getting as the redirect/callback?

@cedricjacobs
Copy link

I will post a full situation report later on today. I managed however to 'resolve' the matter :-)
I will include a gist for future reference for all to suggest/improve

@xgp
Copy link
Member

xgp commented May 15, 2023

@sweeneytr This approach solves the state/nonce support issue. #29 Let me know if you have the means to test from this in order to validate it works with the OIDC libs you are using.

@xgp xgp changed the title Magic link doesn't support PKCE or state/nonce values Magic link doesn't support PKCE May 22, 2023
@xgp
Copy link
Member

xgp commented May 22, 2023

state/nonce portion fixed in 50f2c0e

@xgp xgp added the help wanted Extra attention is needed label Jun 30, 2023
@nlsrchtr
Copy link

Thanks everyone in looking into this. We are currently also faced with this issue, since we would like to make logging into our mobile app as easy as possible.

@sweeneytr, were you able to move further with the additions @xgp was proposing?

Since I'm not a Java developer, I can offer my help in extending the documentation for this feature - if there is help needed.

Would be great to get this feature out of the door, since it enables really easy logins.

@xgp
Copy link
Member

xgp commented Dec 19, 2023

@nlsrchtr There is a PR #56 for PKCE support. Best thing anyone can do to help at this point is to test that branch https://github.com/p2-inc/keycloak-magic-link/tree/xgp/pkce . If I get the 👍 from enough people that it works, I'll merge it.

@embesozzi
Copy link

embesozzi commented Apr 5, 2024

The "problem" with the magic link that occurs in every IdP vendor is that these flows interrupt the login process, which then resumes when you click the link (a process that is not easy to implement in OAuth 2.0 with traditional flows). I must say that the xgp implementation is great in how it deals with this problem in KC.

Just to provide another perspective on solving the problem with state parameters: In other IdPs such as ForgeRock, the magic link redirects to the app (not to the IdP). The app then parses the link and sends it back to the IdP. Therefore, I would say it's a standard federation process that ultimately results in user authentication. Perhaps implementing it in that way will help in some scenarios.

@cbeckmann486
Copy link

@nlsrchtr There is a PR #56 for PKCE support. Best thing anyone can do to help at this point is to test that branch https://github.com/p2-inc/keycloak-magic-link/tree/xgp/pkce . If I get the 👍 from enough people that it works, I'll merge it.

Hi xgp. I tried this patch today, after doing a quick job of updating it for compatibility with 24.0.4. I'm sorry to say that even wtih this patch, I'm not seeing code_challenge or cc in the token generated.

@xgp
Copy link
Member

xgp commented Jun 20, 2024

@cbeckmann486 Thanks for testing. Can you post your client config and how you are setting up the login in your application? This doesn't set up PKCE for you. It only transits the appropriate variables so the code_challenge, etc transit in the magic link action token.

@Gryff
Copy link

Gryff commented Oct 25, 2024

Hey everyone, there's #91 to get this working. I've tested it with my keycloak setup using nextJS' next-auth (which requires PKCE) and I get no errors.

@Gryff
Copy link

Gryff commented Oct 25, 2024

#91 has been merged into #56 so try out #56 instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed priority
Projects
None yet
Development

No branches or pull requests

9 participants