-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Authorization Code + PKCE #1572
Comments
Is your client public or private? |
Do you mean if the client has a secret set? No it has not. And |
Ok, could you show the full auth URL please, and the 302 redirect urls plus the last url? PKCE works fine. It also appears that here's a safari bug that could cause this: aaronpk/pkce-vanilla-js#1 |
Will do later. And this bug does not seem to be related to my case. |
Also please include hydra logs with LOG_LEVEL=trace |
|
I think there is a problem with the persiting of the PKCE verifier. If you do:
you get
which does not match your initial challenge:
so the code (hydra) appears to behave correctly |
you can probably debug this by |
I have no idea how you get |
seems you used sha1 not sha256. |
Oh yeah, my bad. The values you posted are indeed correct (PKCE does not use padding). I traced the error and it appears to stem from the default case: https://github.com/ory/fosite/blob/master/handler/pkce/handler.go#L206-L210 Which leads me to believe that the method is empty, which is weird, because it should be set during the first step of the PKCE flow and it should be retrieved here: https://github.com/ory/fosite/blob/master/handler/pkce/handler.go#L133 It should further be set correctly because we do not allow the plain method as you can see here: https://github.com/ory/fosite/blob/master/handler/pkce/handler.go#L99-L110 So this should be set correctly. Additionally, the PKCE session does/should properly store the challenge method as you can see here: https://github.com/ory/fosite/blob/master/handler/pkce/handler.go#L65-L70 Which database adapter are you using? in memory or sql? |
Whoops, my analysis was incorrect, I was looking at master instead of v0.29.7 - in that case fosite complains because the challenge computed form the verifier does not match the challenge itself: https://github.com/ory/fosite/blob/v0.29.7/handler/pkce/handler.go#L209-L212 however, I see that fosite expects the https://github.com/ory/fosite/blob/v0.29.7/handler/pkce/handler.go#L199-L202 So this would expect
instead of:
If I'm not mistaken, this is defined in the spec here: https://tools.ietf.org/html/rfc7636#section-4.1
|
Hm, I'm getting a bit unsure about this section:
I think this only means that that's one of the recommended ways how to generate a verifier, not that the verifier should generally be urldecoded before computing the challenge in the oauth2 server. I'm not entirely sure why noone has noticed this yet. PKCE is available since forever. Could you maybe help digging up some libraries (except the one posted) to see how they behave? We obviously want to do this in a way that works with the ecosystem best and hast the fewest breaking changes. |
Sorry for the comment flood, I'm a bit puzzled why this has come up just as of now. I remember quite a lot of people working with PKCE without issues. The node-oidc-provider only checks for the character set, it does not decode the challenge: |
What about the table I'm using MySQL for database (v8.0.17) |
Ok, so I had some time to properly look into the code. My previous assumption was wrong - PKCE is handled correctly by fosite which is also the reason why noone complained about the implementation of PKCE so far. I was confused by these lines: https://github.com/ory/fosite/blob/v0.29.7/handler/pkce/handler.go#L187-L202 However, those are there to ensure that the entropy is high enough - they are not actually used for the verification process itself. I also checked why Would it be possible for you to share a git repo with me that I can use for reproducing the issue? I'm now really clueless as to why this does not work. |
I seem to have gotten to the core of the problem - kind of. Seems like there is a problem with Chrome and XMLHttpRequest POST request on non-secure HTTP connection. I see from Inspector that correct data is being sent to the server but maybe some security layer in Chrome will block it? Or maybe it's because I'm using local domain with custom port and that will trigger some security layer shenanigans. I tried now same test application on Firefox and it works fine there. Thanks for you time. This was kind of odd thing to debug as everything seems to be alright on every step of the process. Even Chrome Inspector shows that correct data is being sent to the server. But I guess it's a kind of bug on their side? Or maybe a feature that is not well communicated in the Inspector. I dont really have the time to go digging in Chromes internals/issue tracker if this is already reported or expected behavior. But if someone has any knowledge of that then please let me know. |
It is possible that chrome or a privacy plugin is blocking localStore or some cookie required for the vanilljas repo to function properly |
I'm closing this because it is not an issue with this repo |
I dont think so. I don't have any plugins that could do that. Also localStorage is working fine. I see from Inspector what data is being sent to the server and it is all correct. It just seems that there is some layer after Chrome inspector that does something to the request. Just try it in Chrome with HTTP connection: https://github.com/aaronpk/pkce-vanilla-js |
The problem is in these lines of code:
The "hash.Sum" returns byte array of RAW digest not HEX encoded hash, therefore base64.RawURLEncoding.EncodeToString does not calculate a proper base64 value. You can test it with the following code
This would mean either the client uses sha256 hash using raw byte values (not hexed), or this code switches to HEX from RAW bytes |
Are you saying that the implementation in the code (fosite) is incorrect, or that the client library is handling this improperly? |
While that's correct, base64 takes raw digest, not hex encoded strings. That's the whole idea of base64. Hex encoded is already an encoding, so why would we need to base64 it, there is no good reason to do so. |
The fosite implementation is correct in my opinion i.e. "BASE64URL-ENCODE(SHA256(ASCII(code_verifier)))". The spec does not clearly state the following section, which confused me: I hope this save someone some time. Most documentation assumes that it is implicit in the use of base64. Note - I have updated my comment from two years ago (I did not notice a typo that might have caused further confusion rather than help people) - |
The spec has an example function in the appendix, which takes bytes (and not hex):
So I think the implementation is correct. https://tools.ietf.org/html/rfc7636#appendix-A I'm writing this because I'm experiencing the same error, and I'm pretty sure it's because my application overwrites the verifier (saved in the session) when multiple OAuth2-flows are executed in parallel. So after lots of debugging I'm pretty confident to say that Hydras implementation is correct. |
Describe the bug
I'm using simple native JS client from: https://github.com/aaronpk/pkce-vanilla-js to test my Hydra instance with Authorization Code + PKCE grant. Everything works fine untill the access_token request. I keep getting the following error:
I'm pretty sure challenge and verifier I send to the server are correct. I have also verifed that these values are really sent to the server.
code_verifier:
6e5ec79163ee989be79fa7a542f599d0e7c77038c8620aa44170217c
code_challenge:
ZTE4MTMxZGRlMjA2ZGJiZjAyNTMyZjU3NjM0MThmOTkxYTQ4Y2EyZjFhY2Y1NjRmOGI1YWE1ZjJhYTRkYTE3Ng
These values are being sent to the server and AFAIK they are totally correct. But I still get the error stating that these dont match. I have hit a wall on debugging this.
I also had a look into Hydra DB and the table hydra_oauth2_pkce is empty. Shouldn't session data from authorization code request be saved there? Maybe that's what is creating the problem? Is there something I can do from my side?
Environment
The text was updated successfully, but these errors were encountered: