-
Notifications
You must be signed in to change notification settings - Fork 479
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
[WIP] OAuth2 Middleware: Store token in a cookie #2963
Conversation
Includes validation Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
Closing in favor of #2964 |
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.
Overall, looks good given the cookie limitations. I put some minor comments and a question inline.
Given the limitations, I think you still need a solution that uses a distributed cache or some shared session store server side.
} | ||
const ( | ||
// Timeout for authenticating with the IdP | ||
authenticationTimeout = 10 * time.Minute |
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.
This is named and documented as a timeout for the authentication request, but it is used as a TTL on the auth token / cookie.
Both may be necessary (the authenticationTimeout as a circuit breaker on the IdP).
Also, should these be configurable?
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.
Yes it's a timeout on authentication requests. Because we cannot influence the timeout on the IdP, we can only set an expiration on the cookie with the state token.
I am not sure this needs to be configurable. 10 mins should be plenty for someone to authenticate.
|
||
// Browsers have a maximum size of about 4KB, and if the cookie is larger than that (by looking at the entire value of the header), it is silently rejected | ||
// Some info: https://stackoverflow.com/a/4604212/192024 | ||
if len(cookieStr) > 4<<10 { |
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.
What happens if the backend service also adds cookie values on subsequent requests? Is the total limit still 4k?
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.
Yes, it's still 4KB per domain in most browsers. If the cookies go over the limit, they are silently dropped
Fixes #2635
Currently the OAuth 2 middleware suffers from scalability issues as reported in #2635. Tokens obtained from the authorization server are stored in-memory in the Dapr runtime (a session ID is stored in a cookie on the client), so they do not persist if the runtime is restarted, and make it impossible to scale the application horizontally.
The most common solution to this problem is to store the tokens in the clients, as self-contained tokens such as JWTs; because tokens are sensitive, they are encrypted (using encrypted JWTs).
Because cookies now store the token encrypted, users need to provide an encryption key via the new metadata property
cookieKey
:cookieKey
is not the actual encryption key used: the Dapr runtime deterministically derives both a signing key and an encryption key from thatWork In Progress
This PR is currently a WIP because of an issue with handling large tokens.
Solutions: