Bug #18281
closed[login-sync] automatically replaces expired tokens
Added by Ward Vandewege about 3 years ago. Updated about 3 years ago.
Updated by Ward Vandewege about 3 years ago
- Status changed from New to In Progress
Updated by Ward Vandewege about 3 years ago
Ready for review in ccb603fe5a8ca989d6db97cc723ccfcaba2781f5 on branch 18281-login-sync-expired-tokens
Updated by Lucas Di Pentima about 3 years ago
- I'm not sure if I'm following correctly, but with this change, the
--rotate-tokens
flag seems to me to be superfluous: If thetokenfile
exists, it checks the token's validity; if it doesn't exist, it'll create a new one no matter if the--rotate-tokens
flag was used or not, right? - Your update will automatically detect token expirations, and I think that's a better approach than having the admin set up the flag on those clusters migrating towards token expiration.
- If my appreciation is correct, I think we can safely remove the flag.
Updated by Ward Vandewege about 3 years ago
Lucas Di Pentima wrote:
- I'm not sure if I'm following correctly, but with this change, the
--rotate-tokens
flag seems to me to be superfluous: If thetokenfile
exists, it checks the token's validity; if it doesn't exist, it'll create a new one no matter if the--rotate-tokens
flag was used or not, right?
Right - but if you give it --rotate-tokens
it will force a token rotation, even if there is an existing token and it is valid. I don't know why you'd want to do that, but I guess it could still be useful when you want to force-rotate all the tokens? The comment is misleading though, I've pushed e1c4967befc7b4dd273b3d9d047a4e4262f5ba2f to fix that.
- Your update will automatically detect token expirations, and I think that's a better approach than having the admin set up the flag on those clusters migrating towards token expiration.
- If my appreciation is correct, I think we can safely remove the flag.
If it's OK with you I'd keep it with the updated help text?
Updated by Lucas Di Pentima about 3 years ago
Ward Vandewege wrote:
Right - but if you give it
--rotate-tokens
it will force a token rotation, even if there is an existing token and it is valid. I don't know why you'd want to do that, but I guess it could still be useful when you want to force-rotate all the tokens? The comment is misleading though, I've pushed e1c4967befc7b4dd273b3d9d047a4e4262f5ba2f to fix that.
I kind of remember that the train of thought went something like this: On a expiring tokens installation, pass --token-lifetime N
and --rotate-tokens
while calling the script every N-X where X is a certain amount of minutes of acceptable grace period. But this doesn't work with auto-expiring tokens. (maybe this got lost when we changed the token expiration approach)
- Your update will automatically detect token expirations, and I think that's a better approach than having the admin set up the flag on those clusters migrating towards token expiration.
- If my appreciation is correct, I think we can safely remove the flag.
If it's OK with you I'd keep it with the updated help text?
I don't think it provides any benefit (other than don't error out on existing installations using the flag) besides what your updates provide, but also it doesn't do any harm to be left there, so it LGTM. :)
Updated by Ward Vandewege about 3 years ago
Lucas Di Pentima wrote:
I don't think it provides any benefit (other than don't error out on existing installations using the flag) besides what your updates provide, but also it doesn't do any harm to be left there, so it LGTM. :)
Thanks, merged!
Updated by Ward Vandewege about 3 years ago
- Status changed from In Progress to Resolved
Applied in changeset arvados|242e740db430759c3a09de3bf9ece89987c6b9b0.