Auth: Add feature flag to move token rotation to client #65060
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kalleep
requested review from
tskarhed,
yaelleC,
andresmgot,
zserge,
mildwonkey and
suntala
and removed request for
a team
|
You have successfully added a new CodeQL configuration |
chri2547
reviewed
Mar 20, 2023
docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md
Outdated
Show resolved
Hide resolved
chri2547
requested changes
Mar 20, 2023
This can happen when token is deleted or we try to rotate an old token
roration is enabled
Co-authored-by: Ieva <ieva.vasiljeva@grafana.com>
Co-authored-by: Ieva <ieva.vasiljeva@grafana.com>
kalleep
force-pushed
the
kalleep/auth-token/rotate
branch
from
10fd5ef
to
d4e0a2d
Compare
gotjosh
pushed a commit
that referenced
this pull request
Mar 27, 2023
* FeatureToggle: Add toggle to use a new way of rotating tokens * API: Add endpoints to perform token rotation, one endpoint for api request and one endpoint for redirectsd * Auth: Aling not authorized handling between auth middleware and access control middleware * API: add utility function to get redirect for login * API: Handle token rotation redirect for login page * Frontend: Add job scheduling for token rotation and make call to token rotation as fallback in retry request * ContextHandler: Prevent in-request rotation if feature flag is enabled and check if token needs to be rotated * AuthN: Prevent in-request rotation if feature flag is enabled and check if token needs to be rotated * Cookies: Add option NotHttpOnly * AuthToken: Add helper function to get next rotation time and another function to check if token need to be rotated * AuthN: Add function to delete session cookie and set expiry cookie Co-authored-by: Ieva <ieva.vasiljeva@grafana.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What is this feature?
At the moment every request made to the grafana API can trigger a rotation. We want to move the responsibility of token rotation to the client instead.
This is all done behind a new feature toggle "ClientTokenRotation".
There are two scenarios we need to handle, page load and api request.
For page loads I have added checks in both Auth and AccessControl middleware where not authorized checks are performed. If the LookupTokenErr is ErrTokenNeedRotation we write a redirect cookie for current path and redirect to /user/auth-tokens/rotate. If token is successfully rotated we redirect the user batch to the patch set in redirect cookie. If the rotation failed (revoked, not found, expired) we redirect to login page and delete the cookie.
The second scenario is api request performed by the client. My first approach was to call the rotation endpoint if a request failed with 401 and retry the request if it was successful. This kinda works but have problems when making many concurrent request that all tries to rotate the token. If we would have a two token system (refresh and access) this would be no problem but with the current system a lot of concurrent request can cause unintended side effect, especially when running grafana in HA. We also have the problem that we cannot grantee that all plugins use backendSrv where this retry logic is implemented.
So Instead I decided to schedule the token rotation. This is done with the help of a new cookie we set that holds the number of seconds until next rotation, that I also subtract some leeway from. In order to try to not make all open tabs for a session run a token rotation I randomize a number between 1-20 and subtract that from the time we should run the rotation. When a rotation is triggered we first check if we have a new expiry time and reschedule the job in that case.
The out dated auth token stored in db can only make request for 1 minutes after a rotation has happen and then it will start getting 401:s
Fixes #
Special notes for your reviewer: