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

Concurrent requests for token endpoint on auth-code flow with same code succeed. #778

Open
3 of 5 tasks
tn185075 opened this issue Nov 28, 2023 · 8 comments
Open
3 of 5 tasks
Labels
bug Something is not working.

Comments

@tn185075
Copy link

tn185075 commented Nov 28, 2023

Preflight checklist

Ory Network Project

No response

Describe the bug

When an authorization code is issued to the client, if the client makes two concurrent requests for token endpoint using the same auth code, it results in two tokens, as the code is not invalidated in PopulateTokenEndpointResponse before the other request reaches the HandleTokenEndpointRequest method.

Reproducing the bug

  1. Run the auth code flow with a registered client.
  2. Make two concurrent requests (can use goroutines) on token endpoint with the same auth code.
  3. We can get the token for both the requests.

Relevant log output

No response

Relevant configuration

No response

Version

v0.42.2

On which operating system are you observing this issue?

Linux

In which environment are you deploying?

Kubernetes

Additional Context

No response

@tn185075 tn185075 added the bug Something is not working. label Nov 28, 2023
@tn185075
Copy link
Author

Bump ‼️ ‼️

@vivshankar
Copy link
Contributor

vivshankar commented Mar 19, 2024

Won't this depend on the storage implementation? In my implementation, we use redis for such short term storage and treat the Get as a pop. The choice of storage doesn't matter too much.

I expect that is the only way to atomically implement this.

@james-d-elliott
Copy link
Contributor

There are quite a few options in Postgres to deal with this from memory.

@mitar
Copy link
Contributor

mitar commented Mar 19, 2024

I think you need at least repeatable read isolation levels, but you can just go with serializable.

@vivshankar
Copy link
Contributor

vivshankar commented Mar 19, 2024

@tn185075 are you concerned about the implementation in fosite that uses the memory store? At the moment, the InvalidateAuthorizeCodeSession is a no-op for me. I expect in any practical implementation, that would be the case. The GetAuthorizeCodeSession should pop, i.e. get and delete atomically.

I haven't looked at Hydra.

@tn185075
Copy link
Author

tn185075 commented Mar 19, 2024

popping it out on GET is a probable solution, but has it's own limitations. As we need to keep that record, in case if we fail to reach the populateTokenEndpointResponse, client should be able to use the auth code again to fetch the token.

Also it's one of the key info, which we can use to detect malicious activities against our providers I believe.

I do not think it is a storage problem, as we are making a get in HandletokenEndpointRequest, but invalidation happens in PopulateTokenEndpointResponse. This is a clear case of race/timing conditions, in my opinion, even if we manage to wrap the block invalidating auth code over mutex :).

@tn185075
Copy link
Author

If I remember rightly, I believe I was able to replicate this with Hydra :).

@tn185075
Copy link
Author

Any updates here? ‼️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is not working.
Projects
None yet
Development

No branches or pull requests

4 participants