- Notifications
You must be signed in to change notification settings - Fork928
feat: add session expiry control flags#5976
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Adds --session-duration which lets admins customize the default sessionexpiration for browser sessions.Adds --disable-session-expiry-refresh which allows admins to preventsession expiry from being automatically bumped upon the API key beingused.
@deansheather is there any issue you can link with this pull request? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Left two questions but in general LGTM. I'm wondering if we should hide security-related flags under a commonsecurity umbrella.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Code changes LGTM, but I leave the product decision to Kyle.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for addressing the comments. Feel free to merge if the CI is happy.
Uh oh!
There was an error while loading.Please reload this page.
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