-
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
[1.13] New OAuth2 middleware with token persistence #2967
base: main
Are you sure you want to change the base?
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]>
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]>
Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
…nto oauth2-mw-persistence3 Signed-off-by: ItalyPaleAle <[email protected]>
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.
Doing the very first pass. Will try to review it more thoroughly.
m.setTokenFn = m.cookieModeSetTokenInResponse | ||
m.claimsForAuthFn = m.cookieModeClaimsForAuth | ||
case modeHeader: | ||
m.getTokenFn = m.headerModeGetClaimsFromHeader |
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.
Should we change the name headerModeGetClaimsFromHeader
to GetClaimsFromHeader
, so that it is in sync with GetClaimsFromCookie
?
m.claimsForAuthFn = m.cookieModeClaimsForAuth | ||
case modeHeader: | ||
m.getTokenFn = m.headerModeGetClaimsFromHeader | ||
m.setTokenFn = m.headerModeSetTokenResponse |
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.
m.setTokenFn = m.headerModeSetTokenResponse | |
m.setTokenFn = m.headerModeSetTokenInResponse |
or cookieModeSetTokenInResponse
to cookieModeSetTokenResponse
state := r.URL.Query().Get("state") | ||
if code != "" && state != "" { | ||
// Always get the claims from the cookies in this case | ||
claims, err := m.GetClaimsFromCookie(r) |
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.
Trying to understand: Why claims are always fetched from cookie and not from header, if code and state are provided?
|
||
url := conf.AuthCodeURL(idStr, oauth2.AccessTypeOffline) | ||
httputils.RespondWithRedirect(w, http.StatusFound, url) | ||
m.exchangeAccessCode(w, r, claims, code, state) |
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.
Here, we rely on code
being not blank to exchange it to get access token. Should we also check response_type
somehow before doing so?
Ref: https://darutk.medium.com/diagrams-and-movies-of-all-the-oauth-2-0-flows-194f3c3ade85 and https://stackoverflow.com/a/74547318/18848251
I think we should push this to 1.13 for when internal actors for components (dapr/dapr#6538) is available. At this stage, it's highly unlikely internal actors for components are available in 1.12. The reason is that in the current case, it fails when tokens are large, such as with Azure AD. Although this PR does fix a very important issue, it may be a lesser issue than the alternative. Thoughts? @dapr/maintainers-components-contrib |
I'm fine postponing this to 1.13. If so, do you want to mark it as a draft PR and add 1.13 to the title for now @ItalyPaleAle ? |
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
…o oauth2-mw-persistence3 Signed-off-by: ItalyPaleAle <[email protected]>
Fixes #2635
Replaces #2963 and #2964
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).
This PR rewrites the OAuth2 middleware to allow for 2 different behaviors based on the new
mode
metadata property:cookie
(default)header
:Authorization
headerAuthorization
and the value of the Authorization header that clients must passWhy 2 modes?
The reason why we have two modes is due to the issue described in #2963. TLDR: tokens returned by Azure AD can be too large to be stored in a cookie.
However, using headers is not a perfect 1:1 replacement:
Token encryption key
Because cookies now store the token encrypted, users need to provide an encryption key via the new metadata property
tokenEncryptionKey
:tokenEncryptionKey
is not the actual encryption key used: the Dapr runtime deterministically derives both a signing key and an encryption key from thatIncluded in this PR
Future work
middleware.http.oauth2clientcredentials
has the same issues with scalability as the OAuth2, and needs to be updated as well (and perhaps we can re-use the shared implementation of this middleware).