Bug #18281

[login-sync] automatically replaces expired tokens

Added by Ward Vandewege 3 months ago. Updated 3 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
10/21/2021
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-
Release relationship:
Auto

Subtasks

Task #18282: Review 18281-login-sync-expired-tokensResolvedWard Vandewege

Associated revisions

Revision 242e740d
Added by Ward Vandewege 3 months ago

Merge branch 'main' into 18281-login-sync-expired-tokens

closes #18281

Arvados-DCO-1.1-Signed-off-by: Ward Vandewege <>

Revision dd5a2a0c
Added by Ward Vandewege 3 months ago

Merge branch '18281-login-sync-expired-tokens'

closes #18281

Arvados-DCO-1.1-Signed-off-by: Ward Vandewege <>

History

#1 Updated by Ward Vandewege 3 months ago

  • Status changed from New to In Progress

#2 Updated by Ward Vandewege 3 months ago

  • Release set to 45

#3 Updated by Ward Vandewege 3 months ago

Ready for review in ccb603fe5a8ca989d6db97cc723ccfcaba2781f5 on branch 18281-login-sync-expired-tokens

#4 Updated by Lucas Di Pentima 3 months 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 the tokenfile 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.

#5 Updated by Ward Vandewege 3 months 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 the tokenfile 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?

#6 Updated by Lucas Di Pentima 3 months 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. :)

#7 Updated by Ward Vandewege 3 months 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!

#8 Updated by Ward Vandewege 3 months ago

  • Status changed from In Progress to Resolved

#9 Updated by Ward Vandewege 3 months ago

  • Release changed from 45 to 42

Also available in: Atom PDF