Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow limited key validity window for authenticated keys #252

Merged
merged 25 commits into from
Nov 8, 2021
Merged

Conversation

maxgoedjen
Copy link
Owner

@maxgoedjen maxgoedjen commented Nov 6, 2021

Fixes #251

Screen Shot 2021-11-07 at 1 31 13 PM

Screen Shot 2021-11-07 at 1 30 28 PM

@maxgoedjen maxgoedjen added the rfc Request for comments label Nov 6, 2021
@maxgoedjen maxgoedjen added the enhancement New feature or request label Nov 6, 2021
@maxgoedjen maxgoedjen marked this pull request as ready for review November 8, 2021 01:35
@maxgoedjen maxgoedjen enabled auto-merge (squash) November 8, 2021 01:35
@maxgoedjen maxgoedjen merged commit 7d9fee0 into main Nov 8, 2021
@maxgoedjen maxgoedjen deleted the duration branch November 8, 2021 01:41
@taxilian
Copy link

This is awesome, but would you consider adding more options? The gap between "1 hour" and "1 day" is pretty extreme. I don't generally care about 1 minute vs 5 minutes -- for just 1-5 minutes it's not even worth the time to tell it to unlock -- but for my normal use case "1 hour" is too short and "1 day" is too long. Ideally I'd probably want 6-8 hours.

It would also be nice to be able to configure this in the UI rather than on the notification -- that's an annoying workflow and I'd rather be able to just add a setting that says "for this key when I unlock it leave it unlocked for __ minutes"

Let me know if this would be a separate issue request, but I'd also want it to lock itself again if the computer is locked -- maybe it already does that.

@maxgoedjen
Copy link
Owner Author

@taxilian the 1 min case is mostly for like "I need to run this script that will call 1000x within 3 seconds" type cases.

That being said, the 1 day case might be going away anyway due to technician issues, so I'll be playing with those durations. 4+ hours will probably be running into the same issues, but we'll see.

With regards to unlocking from UI, this is actually a bit of a weird quirk I'm dealing with now: there's no way to know ahead of time (that I can see) unless I recorded it elsewhere before creating the key. It's in the notification because I observe how long the signing process took: if it's more than a second or so, I can assume an authentication occurred.

@maxgoedjen
Copy link
Owner Author

Pretty sure the relock behavior is how it works now, but that's a good idea, I can double check that.

@maxgoedjen
Copy link
Owner Author

#332 for that one.

@taxilian
Copy link

Interesting; it's hard to tell from the user perspective (not being familiar with the APIs you're using) how much control you have over what is going on =]

@zachberger
Copy link

I've updated to 2.2.0 and this does not seem to work for me. I am able to see the option on the notification but I'm still prompted to re-unlock.

@maxgoedjen
Copy link
Owner Author

@zachberger When you select a duration, you'll be prompted once more to authorize the temporary unlock, but you should't any more after that point. Seeing something else?

@taxilian
Copy link

@zachberger also note that the unlock seems to be a "per-process" thing, or at least per-process-name. If you look at the window you'll see the name of the process requesting the unlock, if it doesn't match then you'll be prompted again.

I just tested with 2.2.0 and it's working for me, FWIW

@zachberger
Copy link

I don't seem to get the second prompt. Here is a quick video

secretive.mov

@maxgoedjen
Copy link
Owner Author

@zachberger thats definitely Not what it's supposed to be doing (thanks for the video, that was helpful). Tracking this separately in #354

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request rfc Request for comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validity window for "auth required" keys
3 participants