https://dev.arvados.org/https://dev.arvados.org/favicon.ico?15576888422020-08-20T16:48:25ZArvadosArvados - Bug #16736: Token lifetime optionshttps://dev.arvados.org/issues/16736?journal_id=862032020-08-20T16:48:25ZLucas Di Pentimalucas.dipentima@curii.com
<ul></ul><p>Pushed a branch with a test: <code>16736-expiring-tokens-limits</code></p> Arvados - Bug #16736: Token lifetime optionshttps://dev.arvados.org/issues/16736?journal_id=862752020-08-24T20:31:20ZLucas Di Pentimalucas.dipentima@curii.com
<ul><li><strong>Status</strong> changed from <i>New</i> to <i>In Progress</i></li></ul> Arvados - Bug #16736: Token lifetime optionshttps://dev.arvados.org/issues/16736?journal_id=862762020-08-24T20:32:29ZLucas Di Pentimalucas.dipentima@curii.com
<ul><li><strong>Target version</strong> changed from <i>2020-09-09 Sprint</i> to <i>2020-08-26 Sprint</i></li></ul> Arvados - Bug #16736: Token lifetime optionshttps://dev.arvados.org/issues/16736?journal_id=863222020-08-26T15:32:23ZLucas Di Pentimalucas.dipentima@curii.com
<ul><li><strong>Target version</strong> changed from <i>2020-08-26 Sprint</i> to <i>2020-09-09 Sprint</i></li></ul> Arvados - Bug #16736: Token lifetime optionshttps://dev.arvados.org/issues/16736?journal_id=867572020-09-09T14:16:41ZLucas Di Pentimalucas.dipentima@curii.com
<ul><li><strong>Target version</strong> changed from <i>2020-09-09 Sprint</i> to <i>2020-09-23 Sprint</i></li></ul> Arvados - Bug #16736: Token lifetime optionshttps://dev.arvados.org/issues/16736?journal_id=868622020-09-10T15:29:24ZLucas Di Pentimalucas.dipentima@curii.com
<ul></ul><p>Updates at <a class="changeset" title="16736: Fixes permission to create check. Removes token expiration from fixtures Arvados-DCO-1.1-..." href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/386b60af1297d82b222b75aad2e1c5550f1c13a4">386b60af1</a> - branch <code>16736-expiring-tokens-limits</code><br />Test run: <a class="external" href="https://ci.arvados.org/job/developer-run-tests/2078/"<a href="https://ci.arvados.org/job/developer-run-tests/2078/">developer-run-tests: #2078 <img src="https://ci.arvados.org/buildStatus/icon?job=developer-run-tests&build=2078" alt="" /></a></a></p>
<ul>
<li>Adds tests exposing the bug.</li>
<li>Adds checks related to token's <code>expires_at</code> on create/update.</li>
<li>Fixes fixture removing expiration dates that were set far in the future.</li>
</ul> Arvados - Bug #16736: Token lifetime optionshttps://dev.arvados.org/issues/16736?journal_id=868832020-09-10T18:40:09ZPeter Amstutzpeter.amstutz@curii.com
<ul></ul><p>Lucas Di Pentima wrote:</p>
<blockquote>
<p>Updates at <a class="changeset" title="16736: Fixes permission to create check. Removes token expiration from fixtures Arvados-DCO-1.1-..." href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/386b60af1297d82b222b75aad2e1c5550f1c13a4">386b60af1</a> - branch <code>16736-expiring-tokens-limits</code><br />Test run: <a class="external" href="https://ci.arvados.org/job/developer-run-tests/2078/"<a href="https://ci.arvados.org/job/developer-run-tests/2078/">developer-run-tests: #2078 <img src="https://ci.arvados.org/buildStatus/icon?job=developer-run-tests&build=2078" alt="" /></a></a></p>
<ul>
<li>Adds tests exposing the bug.</li>
<li>Adds checks related to token's <code>expires_at</code> on create/update.</li>
<li>Fixes fixture removing expiration dates that were set far in the future.</li>
</ul>
</blockquote>
<pre>
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
</pre>
<p>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.</p>
<p>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?)</p>
<p>I think this should be:</p>
<pre>
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
</pre>
<p>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.</p>
<p>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.</p> Arvados - Bug #16736: Token lifetime optionshttps://dev.arvados.org/issues/16736?journal_id=869462020-09-14T20:13:48ZPeter Amstutzpeter.amstutz@curii.com
<ul></ul><p>update: I think you mentioned on gitter than <code>current_api_client_authorization</code> is nil in the case of the login process, which makes sense. We are creating a token, we don't have one yet.</p>
<p>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.</p>
<p>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.</p> Arvados - Bug #16736: Token lifetime optionshttps://dev.arvados.org/issues/16736?journal_id=870192020-09-17T20:01:40ZPeter Amstutzpeter.amstutz@curii.com
<ul></ul><p>When Login.TokenLifetime is set:</p>
<ul>
<li>Token from login uses "Login.TokenLifetime" expiration (existing behavior)</li>
<li>(valid) token from login can be used to create new tokens
<ul>
<li>If no expiration time is given, it is the "maximum" expiration (new configuration option API.MaximumTokenLifetime)</li>
<li>SystemRootToken can create tokens with any expiration (or nil)</li>
</ul>
</li>
<li>Token created to run a container doesn't have a set expire time, because it will expire when the container ends</li>
<li>Token used on a shell node is max lifetime, when about to expire it is rotated by the arvados-login-sync script</li>
<li>"Show current token" dialog in workbench should generate a new token instead
<ul>
<li>Change the label to something like "Get new user token" </li>
</ul>
</li>
<li>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.
<ul>
<li>Figure out which browser feature lets you do that</li>
</ul></li>
</ul> Arvados - Bug #16736: Token lifetime optionshttps://dev.arvados.org/issues/16736?journal_id=870222020-09-17T20:22:09ZPeter Amstutzpeter.amstutz@curii.com
<ul><li><strong>Release</strong> deleted (<del><i>25</i></del>)</li></ul> Arvados - Bug #16736: Token lifetime optionshttps://dev.arvados.org/issues/16736?journal_id=870332020-09-17T21:28:01ZPeter Amstutzpeter.amstutz@curii.com
<ul><li><strong>Subject</strong> changed from <i>Trusted client token with expiration can create tokens with any expiration date (or none at all)</i> to <i>Token default and maximum lifetime option</i></li></ul> Arvados - Bug #16736: Token lifetime optionshttps://dev.arvados.org/issues/16736?journal_id=870342020-09-17T22:02:18ZPeter Amstutzpeter.amstutz@curii.com
<ul><li><strong>Target version</strong> changed from <i>2020-09-23 Sprint</i> to <i>2020-10-07 Sprint</i></li><li><strong>Assigned To</strong> deleted (<del><i>Lucas Di Pentima</i></del>)</li><li><strong>Description</strong> updated (<a title="View differences" href="/journals/87034/diff?detail_id=83803">diff</a>)</li><li><strong>Subject</strong> changed from <i>Token default and maximum lifetime option</i> to <i>Token lifetime options</i></li></ul> Arvados - Bug #16736: Token lifetime optionshttps://dev.arvados.org/issues/16736?journal_id=872482020-09-23T16:15:07ZPeter Amstutzpeter.amstutz@curii.com
<ul><li><strong>Target version</strong> changed from <i>2020-10-07 Sprint</i> to <i>2020-10-21 Sprint</i></li></ul> Arvados - Bug #16736: Token lifetime optionshttps://dev.arvados.org/issues/16736?journal_id=878172020-10-07T16:09:20ZPeter Amstutzpeter.amstutz@curii.com
<ul><li><strong>Target version</strong> changed from <i>2020-10-21 Sprint</i> to <i>2020-11-04 Sprint</i></li></ul> Arvados - Bug #16736: Token lifetime optionshttps://dev.arvados.org/issues/16736?journal_id=881482020-10-21T14:08:50ZPeter Amstutzpeter.amstutz@curii.com
<ul><li><strong>Related to</strong> <i><a class="issue tracker-6 status-3 priority-4 priority-default closed behind-schedule" href="/issues/16520">Idea #16520</a>: GxP Qualification</i> added</li></ul> Arvados - Bug #16736: Token lifetime optionshttps://dev.arvados.org/issues/16736?journal_id=882072020-10-21T16:30:07ZPeter Amstutzpeter.amstutz@curii.com
<ul><li><strong>Assigned To</strong> set to <i>Peter Amstutz</i></li></ul> Arvados - Bug #16736: Token lifetime optionshttps://dev.arvados.org/issues/16736?journal_id=883802020-11-03T20:04:00ZPeter Amstutzpeter.amstutz@curii.com
<ul><li><strong>Target version</strong> changed from <i>2020-11-04 Sprint</i> to <i>2020-11-18</i></li></ul> Arvados - Bug #16736: Token lifetime optionshttps://dev.arvados.org/issues/16736?journal_id=887152020-11-16T18:42:27ZPeter Amstutzpeter.amstutz@curii.com
<ul><li><strong>Target version</strong> changed from <i>2020-11-18</i> to <i>2020-12-02 Sprint</i></li></ul> Arvados - Bug #16736: Token lifetime optionshttps://dev.arvados.org/issues/16736?journal_id=887752020-11-17T18:26:06ZPeter Amstutzpeter.amstutz@curii.com
<ul><li><strong>Story points</strong> set to <i>3.0</i></li></ul> Arvados - Bug #16736: Token lifetime optionshttps://dev.arvados.org/issues/16736?journal_id=888732020-11-18T16:45:53ZPeter Amstutzpeter.amstutz@curii.com
<ul><li><strong>Assigned To</strong> deleted (<del><i>Peter Amstutz</i></del>)</li></ul> Arvados - Bug #16736: Token lifetime optionshttps://dev.arvados.org/issues/16736?journal_id=888752020-11-18T16:47:46ZPeter Amstutzpeter.amstutz@curii.com
<ul><li><strong>Target version</strong> changed from <i>2020-12-02 Sprint</i> to <i>2020-12-16 Sprint</i></li></ul> Arvados - Bug #16736: Token lifetime optionshttps://dev.arvados.org/issues/16736?journal_id=892292020-12-02T16:53:12ZPeter Amstutzpeter.amstutz@curii.com
<ul><li><strong>Assigned To</strong> set to <i>Lucas Di Pentima</i></li></ul> Arvados - Bug #16736: Token lifetime optionshttps://dev.arvados.org/issues/16736?journal_id=894512020-12-16T14:43:09ZLucas Di Pentimalucas.dipentima@curii.com
<ul><li><strong>Target version</strong> changed from <i>2020-12-16 Sprint</i> to <i>2021-01-06 Sprint</i></li></ul> Arvados - Bug #16736: Token lifetime optionshttps://dev.arvados.org/issues/16736?journal_id=896082021-01-06T16:54:32ZLucas Di Pentimalucas.dipentima@curii.com
<ul><li><strong>Target version</strong> changed from <i>2021-01-06 Sprint</i> to <i>2021-01-20 Sprint</i></li></ul> Arvados - Bug #16736: Token lifetime optionshttps://dev.arvados.org/issues/16736?journal_id=900042021-01-20T15:18:01ZLucas Di Pentimalucas.dipentima@curii.com
<ul><li><strong>Target version</strong> changed from <i>2021-01-20 Sprint</i> to <i>2021-02-03 Sprint</i></li></ul> Arvados - Bug #16736: Token lifetime optionshttps://dev.arvados.org/issues/16736?journal_id=902962021-02-03T16:34:51ZLucas Di Pentimalucas.dipentima@curii.com
<ul><li><strong>Target version</strong> changed from <i>2021-02-03 Sprint</i> to <i>2021-02-17 sprint</i></li></ul> Arvados - Bug #16736: Token lifetime optionshttps://dev.arvados.org/issues/16736?journal_id=903812021-02-04T19:04:02ZLucas Di Pentimalucas.dipentima@curii.com
<ul></ul><p>Updates at <a class="changeset" title="16736: Updates arvados-login-sync to support expiring tokens. Arvados-DCO-1.1-Signed-off-by: Luc..." href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/bb5d1bb6158ebd6b9fc64ff465feba7c22eaa077">bb5d1bb61</a> - branch <code>16736-max-token-lifetime</code><br />Test run: <a class="external" href="https://ci.arvados.org/job/developer-run-tests/2301/"<a href="https://ci.arvados.org/job/developer-run-tests/2301/">developer-run-tests: #2301 <img src="https://ci.arvados.org/buildStatus/icon?job=developer-run-tests&build=2301" alt="" /></a></a></p>
<ul>
<li>Adds <code>API.MaxTokenLifetime</code> configuration.</li>
<li>Limits token's <code>expires_at</code> when the configuration is set and users don't provide a valid expiration date on creation or update.</li>
<li>Adds options to <code>arvados-login-sync</code> to allow the admin to set it up in a way that periodically rotates expiring tokens on shell sessions.</li>
</ul> Arvados - Bug #16736: Token lifetime optionshttps://dev.arvados.org/issues/16736?journal_id=904442021-02-05T22:06:17ZPeter Amstutzpeter.amstutz@curii.com
<ul></ul><ul>
<li>clamp_token_expiration:</li>
</ul>
<pre>
max_token_expiration = Time.now + Rails.configuration.API.MaxTokenLifetime
</pre>
<p>I think we want to be using <code>db_current_time</code> here?</p>
<p>Related, I see another use of <code>Time.now</code> in the <code>validate</code> method which I believe should also be <code>db_current_time</code></p>
<ul>
<li>I'm not sure I see the purpose of <code>elsif</code> here:</li>
</ul>
<pre>
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
</pre>
<p>Would this do the same thing?</p>
<pre>
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
</pre>
<ul>
<li>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 <code>assign_auth</code> method is guaranteed to be called by the system user).</li>
</ul> Arvados - Bug #16736: Token lifetime optionshttps://dev.arvados.org/issues/16736?journal_id=905282021-02-12T00:11:20ZLucas Di Pentimalucas.dipentima@curii.com
<ul></ul><p>Updates at <a class="changeset" title="16736: Replaces Time.now with db_current_time on token expiration handling code. Arvados-DCO-1.1..." href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/46a5e3958ce98fd1bca4f035dab83ea4c39e6666">46a5e39</a> (rebased) - branch <code>16736-max-token-lifetime</code><br />Test run: <a class="external" href="https://ci.arvados.org/job/developer-run-tests/2327/"<a href="https://ci.arvados.org/job/developer-run-tests/2327/">developer-run-tests: #2327 <img src="https://ci.arvados.org/buildStatus/icon?job=developer-run-tests&build=2327" alt="" /></a></a></p>
<p>Peter Amstutz wrote:</p>
<blockquote>
<p>I think we want to be using <code>db_current_time</code> here?<br />Related, I see another use of <code>Time.now</code> in the <code>validate</code> method which I believe should also be <code>db_current_time</code></p>
</blockquote>
<p>Yes, thank you, fixed!</p>
<blockquote>
<ul>
<li>I'm not sure I see the purpose of <code>elsif</code> here:<br />Would this do the same thing?</li>
</ul>
</blockquote>
<p>Fixed too.</p>
<blockquote>
<ul>
<li>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 <code>assign_auth</code> method is guaranteed to be called by the system user).</li>
</ul>
</blockquote>
<p>As I mentioned on today's standup: the <code>assign_auth</code> case doesn't get a clamped <code>expires_at</code> because that code is run with dispatcher credentials when the container is locked (added comment for clarification).<br />In the case of a federated call, the controller creates the runtime token so it also doesn't get clamped.<br />I added a test for the <code>assign_auth</code> case and expanded a preexisting one for the federated call case.</p> Arvados - Bug #16736: Token lifetime optionshttps://dev.arvados.org/issues/16736?journal_id=907002021-02-18T16:35:38ZLucas Di Pentimalucas.dipentima@curii.com
<ul><li><strong>Target version</strong> changed from <i>2021-02-17 sprint</i> to <i>2021-03-03 sprint</i></li></ul> Arvados - Bug #16736: Token lifetime optionshttps://dev.arvados.org/issues/16736?journal_id=907662021-02-19T20:28:51ZPeter Amstutzpeter.amstutz@curii.com
<ul></ul><p>Lucas Di Pentima wrote:</p>
<blockquote>
<p>Updates at <a class="changeset" title="16736: Replaces Time.now with db_current_time on token expiration handling code. Arvados-DCO-1.1..." href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/46a5e3958ce98fd1bca4f035dab83ea4c39e6666">46a5e39</a> (rebased) - branch <code>16736-max-token-lifetime</code><br />Test run: <a class="external" href="https://ci.arvados.org/job/developer-run-tests/2327/"<a href="https://ci.arvados.org/job/developer-run-tests/2327/">developer-run-tests: #2327 <img src="https://ci.arvados.org/buildStatus/icon?job=developer-run-tests&build=2327" alt="" /></a></a></p>
<p>Peter Amstutz wrote:</p>
<blockquote>
<p>I think we want to be using <code>db_current_time</code> here?<br />Related, I see another use of <code>Time.now</code> in the <code>validate</code> method which I believe should also be <code>db_current_time</code></p>
</blockquote>
<p>Yes, thank you, fixed!</p>
<blockquote>
<ul>
<li>I'm not sure I see the purpose of <code>elsif</code> here:<br />Would this do the same thing?</li>
</ul>
</blockquote>
<p>Fixed too.</p>
<blockquote>
<ul>
<li>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 <code>assign_auth</code> method is guaranteed to be called by the system user).</li>
</ul>
</blockquote>
<p>As I mentioned on today's standup: the <code>assign_auth</code> case doesn't get a clamped <code>expires_at</code> because that code is run with dispatcher credentials when the container is locked (added comment for clarification).<br />In the case of a federated call, the controller creates the runtime token so it also doesn't get clamped.<br />I added a test for the <code>assign_auth</code> case and expanded a preexisting one for the federated call case.</p>
</blockquote>
<p>This LGTM, thanks.</p> Arvados - Bug #16736: Token lifetime optionshttps://dev.arvados.org/issues/16736?journal_id=907742021-02-19T22:01:31ZAnonymous
<ul><li><strong>Status</strong> changed from <i>In Progress</i> to <i>Resolved</i></li></ul><p>Applied in changeset <a class="changeset" title="Merge branch '16736-max-token-lifetime' Closes #16736 Arvados-DCO-1.1-Signed-off-by: Lucas Di Pe..." href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/b3e4886cbbe195347179d0664621da9bc34e6170">arvados|b3e4886cbbe195347179d0664621da9bc34e6170</a>.</p> Arvados - Bug #16736: Token lifetime optionshttps://dev.arvados.org/issues/16736?journal_id=986042021-11-16T16:46:59ZPeter Amstutzpeter.amstutz@curii.com
<ul><li><strong>Release</strong> set to <i>41</i></li></ul>