-
Notifications
You must be signed in to change notification settings - Fork 4
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
IAM 892 - login handler and authentication middleware implementation according to spec ID036 #338
Conversation
5a88794
to
3818bfe
Compare
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.
citing the Maxibon advertisement: "2 is meigl che 1"
happier with the split logic
minor comments but good to go imho
3818bfe
to
f4f1207
Compare
f4f1207
to
5659ad6
Compare
I went ahead and squashed the last commit |
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.
A couple of comments/questions, some may be off:
- Why store the access token in a cookie if it is not used? I suppose it is there to implement userinfo authentication, but we could instead use the id token on the introspection endpoint.
- Is it sensible to store the tokens in different cookies? It complicates the flow, by making it harder to reason (when examining the security of the flow, eg what happens if I mix and match cookies from different users?)
5659ad6
to
126ef68
Compare
126ef68
to
b1890ea
Compare
I replied to the first point in one of the comments, about the other point: unfortunately we have to rely on multiple cookies since the RFC(s) impose a 4096 bytes limit for cookie size, attributes included. A typical encrypted cookie of ours is more than 3k bytes, so it would not be possible to merge them as they are. |
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.
Minor fix, but otherwise lgtm
Description
This PR introduces the middleware functionalities to authenticate invocations via either a bearer token (access token only, no refresh) or ID and access token (with possible refresh in case one of the tokens is expired and a refresh token is available and valid).
This PR also changes the behavior of the callback endpoint which previously returned a json response with three tokens, now redirects to the content of
UIPrefix
as a URL. This needs to be fixed/improved, and I opened the issue to track this - #337.Middleware request authentication flow
Authorization
header (CLI use case)2.a if available, we rely on that as an access token: if validated we go on, otherwise return 401
3.a if both valid, we validate them and if all good we serve the request, otherwise
3.b if one of them is not valid and the error is
oidc.TokenExpiredError
we try to refresh3.b.1 if refresh is fine then cookies get set again and the request is served
3.c if one of them is not valid and the error is not a token expired error then return 401
NB: all encrypted token cookies now rely on the "refresh token" expiration (set via
OAUTH2_REFRESHTOKEN_COOKIE_TTL_SECONDS
env var): reason is that we need id token and access token be set as cookie for longer than their expiration, otherwise we would never getTokenExpiredError
which is needed to trigger the refresh flow. If for example id token cookie expiration is equal to the token expiration, the backend would not be able to receive the expired token (in the just expired cookie), so the backend would just return a 401 since no cookie would be found on the incoming request.About cookie attributes, some of them must be explicitly set, that's why we have this:
Also, both
expires
andmax-age
are set to support most browser implementations.Changes
OAuth2UserSessionCookieTTLSeconds
config