Bug #16736

Token lifetime options

Added by Lucas Di Pentima 9 months ago. Updated 3 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
API
Target version:
Start date:
09/10/2020
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
3.0

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).


Subtasks

Task #16754: Review 16736-expiring-tokens-limitsClosed

Task #17034: Review 16736-max-token-lifetimeResolvedPeter Amstutz


Related issues

Related to Arvados Epics - Story #16520: GxP QualificationIn Progress08/01/202004/30/2021

Associated revisions

Revision b3e4886c
Added by Lucas Di Pentima 3 months ago

Merge branch '16736-max-token-lifetime'
Closes #16736

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <>

History

#1 Updated by Lucas Di Pentima 9 months ago

Pushed a branch with a test: 16736-expiring-tokens-limits

#2 Updated by Lucas Di Pentima 9 months ago

  • Status changed from New to In Progress

#3 Updated by Lucas Di Pentima 9 months ago

  • Target version changed from 2020-09-09 Sprint to 2020-08-26 Sprint

#4 Updated by Lucas Di Pentima 9 months ago

  • Target version changed from 2020-08-26 Sprint to 2020-09-09 Sprint

#5 Updated by Lucas Di Pentima 8 months ago

  • Target version changed from 2020-09-09 Sprint to 2020-09-23 Sprint

#6 Updated by Lucas Di Pentima 8 months ago

Updates at 386b60af1 - branch 16736-expiring-tokens-limits
Test run: https://ci.arvados.org/job/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.

#7 Updated by Peter Amstutz 8 months ago

Lucas Di Pentima wrote:

Updates at 386b60af1 - branch 16736-expiring-tokens-limits
Test run: https://ci.arvados.org/job/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.

#8 Updated by Peter Amstutz 8 months 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.

#9 Updated by Peter Amstutz 8 months 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

#10 Updated by Peter Amstutz 8 months ago

  • Release deleted (25)

#11 Updated by Peter Amstutz 8 months 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

#12 Updated by Peter Amstutz 8 months 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

#13 Updated by Peter Amstutz 8 months ago

  • Target version changed from 2020-10-07 Sprint to 2020-10-21 Sprint

#14 Updated by Peter Amstutz 7 months ago

  • Target version changed from 2020-10-21 Sprint to 2020-11-04 Sprint

#15 Updated by Peter Amstutz 7 months ago

#16 Updated by Peter Amstutz 7 months ago

  • Assigned To set to Peter Amstutz

#17 Updated by Peter Amstutz 6 months ago

  • Target version changed from 2020-11-04 Sprint to 2020-11-18

#18 Updated by Peter Amstutz 6 months ago

  • Target version changed from 2020-11-18 to 2020-12-02 Sprint

#19 Updated by Peter Amstutz 6 months ago

  • Story points set to 3.0

#20 Updated by Peter Amstutz 6 months ago

  • Assigned To deleted (Peter Amstutz)

#21 Updated by Peter Amstutz 6 months ago

  • Target version changed from 2020-12-02 Sprint to 2020-12-16 Sprint

#22 Updated by Peter Amstutz 5 months ago

  • Assigned To set to Lucas Di Pentima

#23 Updated by Lucas Di Pentima 5 months ago

  • Target version changed from 2020-12-16 Sprint to 2021-01-06 Sprint

#24 Updated by Lucas Di Pentima 4 months ago

  • Target version changed from 2021-01-06 Sprint to 2021-01-20 Sprint

#25 Updated by Lucas Di Pentima 4 months ago

  • Target version changed from 2021-01-20 Sprint to 2021-02-03 Sprint

#26 Updated by Lucas Di Pentima 3 months ago

  • Target version changed from 2021-02-03 Sprint to 2021-02-17 sprint

#27 Updated by Lucas Di Pentima 3 months ago

Updates at bb5d1bb61 - branch 16736-max-token-lifetime
Test run: https://ci.arvados.org/job/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.

#28 Updated by Peter Amstutz 3 months 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).

#29 Updated by Lucas Di Pentima 3 months ago

Updates at 46a5e39 (rebased) - branch 16736-max-token-lifetime
Test run: https://ci.arvados.org/job/developer-run-tests/2327/

Peter Amstutz wrote:

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

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.

#30 Updated by Lucas Di Pentima 3 months ago

  • Target version changed from 2021-02-17 sprint to 2021-03-03 sprint

#31 Updated by Peter Amstutz 3 months ago

Lucas Di Pentima wrote:

Updates at 46a5e39 (rebased) - branch 16736-max-token-lifetime
Test run: https://ci.arvados.org/job/developer-run-tests/2327/

Peter Amstutz wrote:

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

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.

This LGTM, thanks.

#32 Updated by Anonymous 3 months ago

  • Status changed from In Progress to Resolved

Also available in: Atom PDF