Bug #16736
closedToken lifetime options
Added by Lucas Di Pentima about 4 years ago. Updated almost 3 years ago.
Description
Add a new option API.MaximumTokenLifetime:
- If no expiration time is given in the create call, it is the "maximum" expiration (new configuration option API.MaximumTokenLifetime)
- For regular users, token expires_at is clamped to MaximumTokenLifetime for create/update
- Admins can create tokens with any expiration time.
- Tokens created to run a container do not have a set expire time (because it will expire when the container ends)
- Tokens created for use on a shell node by arvados-login-sync script have max lifetime, and are rotated by the script on some interval (like MaximumTokenLifetime/2)
Tokens created through login use Login.TokenLifetime (existing behavior).
Related issues
Updated by Lucas Di Pentima about 4 years ago
Pushed a branch with a test: 16736-expiring-tokens-limits
Updated by Lucas Di Pentima almost 4 years ago
- Status changed from New to In Progress
Updated by Lucas Di Pentima almost 4 years ago
- Target version changed from 2020-09-09 Sprint to 2020-08-26 Sprint
Updated by Lucas Di Pentima almost 4 years ago
- Target version changed from 2020-08-26 Sprint to 2020-09-09 Sprint
Updated by Lucas Di Pentima almost 4 years ago
- Target version changed from 2020-09-09 Sprint to 2020-09-23 Sprint
Updated by Lucas Di Pentima almost 4 years ago
Updates at 386b60af1 - branch 16736-expiring-tokens-limits
Test run: developer-run-tests: #2078
- Adds tests exposing the bug.
- Adds checks related to token's
expires_at
on create/update. - Fixes fixture removing expiration dates that were set far in the future.
Updated by Peter Amstutz almost 4 years ago
Lucas Di Pentima wrote:
Updates at 386b60af1 - branch
16736-expiring-tokens-limits
Test run: developer-run-tests: #2078
- Adds tests exposing the bug.
- Adds checks related to token's
expires_at
on create/update.- Fixes fixture removing expiration dates that were set far in the future.
def permission_to_create current_user.andand.is_admin or ((current_user.andand.id == self.user_id) and (current_api_client_authorization.andand.expires_at.nil? or (self.expires_at and current_api_client_authorization.expires_at >= self.expires_at))) end
I don't think this is quite right, this will allow an admin to always create a token even if their token is about to expire. But that's not right. Admins can create tokens for other users, but the tokens are still capped by expires_at.
Also, instead of "current_api_client_authorization.andand", if it is possible for current_api_client_authorization to be nil I don't think we allow tokens to be manipulated (is there a special case, like tests?)
I think this should be:
def permission_to_create (current_user.andand.is_admin || current_user.andand.id == self.user_id) and !current_api_client_authorization.nil? and (current_api_client_authorization.expires_at.nil? || (!self.expires_at.nil? && current_api_client_authorization.expires_at >= self.expires_at))) end
By the way, I looked this up recently, the difference between "and"/"or" and "&&"/"||" in Ruby has to do with operator precedence, "and"/"or" have lower precedence than "&&"/"||" so the convention is to use the and/or for the "outer" clauses and &&/|| for the "inner" clauses although when grouping with parenthesis it doesn't really matter.
Arvados::V1::ApiClientAuthorizationsController#find_objects_for_index should include a filter on expires_at when current_api_client_authorization is non-nil, and there should be a controller test for that.
Updated by Peter Amstutz almost 4 years ago
update: I think you mentioned on gitter than current_api_client_authorization
is nil in the case of the login process, which makes sense. We are creating a token, we don't have one yet.
However, if there was a bug that allowed some other way of calling token creation without setting current_api_client_authorization, that would be a serious security hole.
I wonder if there's a way to handle login as a special case. The value of current_api_client_authorization comes from Thread.current[:api_client_authorization], so the login code could assign a fake (not persisted to database) ApiClientAuthorization object.
Updated by Peter Amstutz almost 4 years ago
When Login.TokenLifetime is set:
- Token from login uses "Login.TokenLifetime" expiration (existing behavior)
- (valid) token from login can be used to create new tokens
- If no expiration time is given, it is the "maximum" expiration (new configuration option API.MaximumTokenLifetime)
- SystemRootToken can create tokens with any expiration (or nil)
- Token created to run a container doesn't have a set expire time, because it will expire when the container ends
- Token used on a shell node is max lifetime, when about to expire it is rotated by the arvados-login-sync script
- "Show current token" dialog in workbench should generate a new token instead
- Change the label to something like "Get new user token"
- Auto-logout should coordinate across browser tabs/windows (using the same token?) so that it only logs out when all of them have been idle.
- Figure out which browser feature lets you do that
Updated by Peter Amstutz almost 4 years ago
- Subject changed from Trusted client token with expiration can create tokens with any expiration date (or none at all) to Token default and maximum lifetime option
Updated by Peter Amstutz almost 4 years ago
- Target version changed from 2020-09-23 Sprint to 2020-10-07 Sprint
- Assigned To deleted (
Lucas Di Pentima) - Description updated (diff)
- Subject changed from Token default and maximum lifetime option to Token lifetime options
Updated by Peter Amstutz almost 4 years ago
- Target version changed from 2020-10-07 Sprint to 2020-10-21 Sprint
Updated by Peter Amstutz almost 4 years ago
- Target version changed from 2020-10-21 Sprint to 2020-11-04 Sprint
Updated by Peter Amstutz almost 4 years ago
- Related to Idea #16520: GxP Qualification added
Updated by Peter Amstutz almost 4 years ago
- Target version changed from 2020-11-04 Sprint to 2020-11-18
Updated by Peter Amstutz almost 4 years ago
- Target version changed from 2020-11-18 to 2020-12-02 Sprint
Updated by Peter Amstutz almost 4 years ago
- Assigned To deleted (
Peter Amstutz)
Updated by Peter Amstutz almost 4 years ago
- Target version changed from 2020-12-02 Sprint to 2020-12-16 Sprint
Updated by Lucas Di Pentima over 3 years ago
- Target version changed from 2020-12-16 Sprint to 2021-01-06 Sprint
Updated by Lucas Di Pentima over 3 years ago
- Target version changed from 2021-01-06 Sprint to 2021-01-20 Sprint
Updated by Lucas Di Pentima over 3 years ago
- Target version changed from 2021-01-20 Sprint to 2021-02-03 Sprint
Updated by Lucas Di Pentima over 3 years ago
- Target version changed from 2021-02-03 Sprint to 2021-02-17 sprint
Updated by Lucas Di Pentima over 3 years ago
Updates at bb5d1bb61 - branch 16736-max-token-lifetime
Test run: developer-run-tests: #2301
- Adds
API.MaxTokenLifetime
configuration. - Limits token's
expires_at
when the configuration is set and users don't provide a valid expiration date on creation or update. - Adds options to
arvados-login-sync
to allow the admin to set it up in a way that periodically rotates expiring tokens on shell sessions.
Updated by Peter Amstutz over 3 years ago
- clamp_token_expiration:
max_token_expiration = Time.now + Rails.configuration.API.MaxTokenLifetime
I think we want to be using db_current_time
here?
Related, I see another use of Time.now
in the validate
method which I believe should also be db_current_time
- I'm not sure I see the purpose of
elsif
here:
if self.new_record? && (self.expires_at.nil? || self.expires_at > max_token_expiration) self.expires_at = max_token_expiration elsif !self.new_record? && self.expires_at_changed? && (self.expires_at.nil? || self.expires_at > max_token_expiration) self.expires_at = max_token_expiration end
Would this do the same thing?
if (self.new_record? || self.expires_at_changed?) && (self.expires_at.nil? || self.expires_at > max_token_expiration) self.expires_at = max_token_expiration end
- Did you confirm that tokens created for containers do not have an expiration set? Is there a test to check that? (it is unclear if the container
assign_auth
method is guaranteed to be called by the system user).
Updated by Lucas Di Pentima over 3 years ago
Updates at 46a5e39 (rebased) - branch 16736-max-token-lifetime
Test run: developer-run-tests: #2327
Peter Amstutz wrote:
I think we want to be using
db_current_time
here?
Related, I see another use ofTime.now
in thevalidate
method which I believe should also bedb_current_time
Yes, thank you, fixed!
- I'm not sure I see the purpose of
elsif
here:
Would this do the same thing?
Fixed too.
- Did you confirm that tokens created for containers do not have an expiration set? Is there a test to check that? (it is unclear if the container
assign_auth
method is guaranteed to be called by the system user).
As I mentioned on today's standup: the assign_auth
case doesn't get a clamped expires_at
because that code is run with dispatcher credentials when the container is locked (added comment for clarification).
In the case of a federated call, the controller creates the runtime token so it also doesn't get clamped.
I added a test for the assign_auth
case and expanded a preexisting one for the federated call case.
Updated by Lucas Di Pentima over 3 years ago
- Target version changed from 2021-02-17 sprint to 2021-03-03 sprint
Updated by Peter Amstutz over 3 years ago
Lucas Di Pentima wrote:
Updates at 46a5e39 (rebased) - branch
16736-max-token-lifetime
Test run: developer-run-tests: #2327Peter Amstutz wrote:
I think we want to be using
db_current_time
here?
Related, I see another use ofTime.now
in thevalidate
method which I believe should also bedb_current_time
Yes, thank you, fixed!
- I'm not sure I see the purpose of
elsif
here:
Would this do the same thing?Fixed too.
- Did you confirm that tokens created for containers do not have an expiration set? Is there a test to check that? (it is unclear if the container
assign_auth
method is guaranteed to be called by the system user).As I mentioned on today's standup: the
assign_auth
case doesn't get a clampedexpires_at
because that code is run with dispatcher credentials when the container is locked (added comment for clarification).
In the case of a federated call, the controller creates the runtime token so it also doesn't get clamped.
I added a test for theassign_auth
case and expanded a preexisting one for the federated call case.
This LGTM, thanks.
Updated by Anonymous over 3 years ago
- Status changed from In Progress to Resolved
Applied in changeset arvados|b3e4886cbbe195347179d0664621da9bc34e6170.