feat: add session expiry control flags #5976
Conversation
Adds --session-duration which lets admins customize the default session expiration for browser sessions. Adds --disable-session-expiry-refresh which allows admins to prevent session expiry from being automatically bumped upon the API key being used.
|
@deansheather is there any issue you can link with this pull request? |
There was a problem hiding this comment.
You're going to hate my response here, but can we check if this is how GitLab and Vault handle these too?
Adding more user knobs also makes it easy for users to get themselves in an insecure situation where it'd be easier for an attacker to be malicious. If we give the knobs, it's still our responsibility.
In the case these are standard, I'm good with it... just sketched out adding even more options.
|
It's a finding from the penetration test, I don't believe there's an issue for it but I could make one if you want. |
|
GitLab does not seem to have a knob for max session duration, but Vault does. I'm also aware of other software that does it like Mattermost. The second flag can't reduce security and can only increase it. |
coderd/apikey_test.go
Outdated
| dc.SessionDuration.Value = time.Second | ||
|
|
||
| userClient := coderdtest.CreateAnotherUser(t, adminClient, adminUser.OrganizationID) | ||
| time.Sleep(dc.SessionDuration.Value + time.Second) |
There was a problem hiding this comment.
Do you think that you can somehow mock the timer or refactor that part? We have enough of time-dependent tests.
There was a problem hiding this comment.
Do you have any suggestions? Since we don't allow db access from tests I can't force the API key to have a specific "last used at" time and have to rely on sleep in this test.
There was a problem hiding this comment.
Maybe it's fine to just test it on the lower level where we can modify the timestamp? or create an "already expired" token
There was a problem hiding this comment.
I refactored the test to use the database to verify and force API key states.
|
I see this from the pen-test, but I wonder if this will make us focus on security less for session tokens or consider reducing the duration as a reliable method to reduce the attack surface. From my perspective, it's security through obscurity. It makes the user-experience worse (especially if admins specify a really low duration). Before merge I'd like @ammario's thoughts, because maybe I'm wrong. @ElliotG would you consider these options standard? |
|
Regardless of whether we think reducing the session expiry limit is security by obscurity or security theater, this will most likely be a customer requirement from some of our highly secure customers. |
I would look at it from the other perspective. Users are as secure as their admins set the security level. Having a knob to control and fine-tune the timeout might be an option to pass any internal security certification processes, as some may have stricter rules than others. What we definitely shouldn't do, and we don't intend to, is give the option to the end-user to create an infinite token. |
coderd/apikey_test.go
Outdated
| dc.SessionDuration.Value = time.Second | ||
|
|
||
| userClient := coderdtest.CreateAnotherUser(t, adminClient, adminUser.OrganizationID) | ||
| time.Sleep(dc.SessionDuration.Value + time.Second) |
There was a problem hiding this comment.
Maybe it's fine to just test it on the lower level where we can modify the timestamp? or create an "already expired" token
|
@bpmct says this is a common request, e.g. for a Yubikey workflow where the admin wants to physically verify all access every X days. Thus, I begrudgingly accept this PR. There is a greater issue of |
Adds --session-duration which lets admins customize the default session expiration for browser sessions.
Adds --disable-session-expiry-refresh which allows admins to prevent session expiry from being automatically bumped upon the API key being used.
Closes #5988
cc @ElliotG