infra icon indicating copy to clipboard operation
infra copied to clipboard

Simplify access key query logic related to expiry time

Open dnephin opened this issue 1 year ago • 2 comments

Related to https://github.com/infrahq/infra/pull/3094#discussion_r961690050

Currently to exclude expired access keys from ListAccessKeys we have to check 6 conditions:

  • expiry > now
  • expiry not null
  • expiry not zero
  • extension deadline > now
  • extension deadline not null
  • extension deadline not zero

We could simplify this by:

  • making both expires_at and extension_deadline not nil fields
  • in create and update make sure no zero values are set (I believe we already require a non-zero expires_at, so we'd only need to set the extension deadline to the expiry time if there was no extension deadline set

This would allow us to check only a single condition, extension_deadline > now. I believe this change would also simplify ValidateAccessKey, because we could check only the extension deadline.

We'll need to write a migrate any existing access key.

dnephin avatar Sep 02 '22 18:09 dnephin

We need to clarify the semantics of extension deadline. As implemented today, the extension deadline is a time window within which the access key must be used otherwise it gets invalidated. This doesn't align with my idea of what an extension deadline means. In my mind, the extension deadline, or maybe some other name, is an extension of key lifetime whenever it's used.

Example, a key with an expiry of 1d and an extension of 1h will increase its expiry by 1h every time the key is used. The key will never expire as long as it's used within a 1h period. If the key isn't used within the 1h window, it expires and the client must get a new key.

In this interpretation, the extension deadline could be null/zero but it should not be part of the is_expired check since the extension deadline updates the expires_at value.

mxyng avatar Sep 02 '22 18:09 mxyng

That improvement is tracked in #2089. I agree we should look at improving the terminology here, and it's good to call out that these two issues could impact each other.

dnephin avatar Sep 02 '22 18:09 dnephin

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

stale[bot] avatar Nov 02 '22 09:11 stale[bot]