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

Generate two keys with different touch policies #22

Open
joneskoo opened this issue May 12, 2020 · 15 comments
Open

Generate two keys with different touch policies #22

joneskoo opened this issue May 12, 2020 · 15 comments
Labels
enhancement New feature or request
Milestone

Comments

@joneskoo
Copy link

It should be possible to support two different keys with different touch policy on Yubikey. This would allow having some more sensitive services require a touch, but also enable other services to still use the hardware key but no touch required, only PIN per session.

Primary use case for me now is Github; I have auto-fetch on my editor and it keeps prompting for touch. It is making way too many (unidentified, #8 ) prompts for touch. If I could use a second slot on the same key for no-touch use cases, it would not be a problem, as long as SSH doesn't trigger signing and therefore touch for the passive other key with touch-required.

joneskoo added a commit to joneskoo/yubikey-agent that referenced this issue May 19, 2020
@joneskoo
Copy link
Author

I have a proof-of-concept of the second no-touch key in my fork. I implemented both key-generation and support for the ssh-agent. I don't mind if someone wants to take this idea and make a polished pull request. Feedback welcome.

[joneskoo@grant ~]$ ssh-add -L
ecdsa-sha2-nistp256 ... YubiKey #1337 PIV Slot 9a
ecdsa-sha2-nistp256 ... YubiKey #1337 PIV Slot 9d NO TOUCH REQUIRED

@joneskoo
Copy link
Author

@FiloSottile I know you're hesitant about adding more complexity to the setup but what do you think about the agent-side changes? Would it be ok to iterate all connected Yubikeys and all slots and return them all? That way the provisioning stays external but an unmodified agent could be used like this.

The PIN policies could be listed in the comment on the agent ssh-add -L based on the PIN/touch policy from attestation certificate based on actual configuration. This only works on device-generated keys and newer Yubikeys but we can easily discard the error from attestation and return just the slot in that case.

joneskoo added a commit to joneskoo/yubikey-agent that referenced this issue May 20, 2020
@hlidotbe
Copy link

hlidotbe commented Jun 5, 2020

It would be great to have that option. When running operations that touch ssh/git (composer, bundle, ...) multiple times it slows everything as you have to confirm each and every ssh connection. It's also very easy to mis-touch and input the 2FA string instead.

And as @joneskoo mentioned, auto-fetching tools like lazygit, GH Desktop, ... are much less useful.

@joneskoo
Copy link
Author

joneskoo commented Jun 5, 2020

@hlidotbe can you test #27?

@hlidotbe
Copy link

hlidotbe commented Jun 5, 2020

If I try as-is without resetting the key, it fails with agent 13: could not get public key: command failed: smart card error 6a82: data object or application not found

I'll have to test with another key as I'd rather not wipe this one and I don't have another one on hand here.

joneskoo added a commit to joneskoo/yubikey-agent that referenced this issue Jun 10, 2020
@joneskoo
Copy link
Author

@hlidotbe I tried to find an error and I can't see any way you'd get that error from my change. Maybe I'm missing something? Or, perhaps you have an older Yubikey and it started failing because of update of go-piv? Can you confirm that master works for you? I suspect you maybe hitting go-piv/piv-go#55 although I thought that was already fixed.

In any case I noticed I hadn't pushed all code before, now it should work. But testing it now I notice there's possibly 300ms performance penalty from getting all slots which we can avoid by caching the signers. Perhaps we need to do something to not slow down the happy path.

@FiloSottile would appreciate your comment, is it welcome to load certificates from all slots (or at least one extra slot for no-touch), and can we cache the signers (destroying cache when transaction closed)? It may affect setup, otherwise I don't see harm from caching.

@joneskoo
Copy link
Author

@hlidotbe Ok I was wrong. I suspect the error you see is because you haven't generated certificate on the other slot. I changed my PR to now load all 4 slots and cache the signers. Can you try again?

And you can use https://github.com/joneskoo/yubikey-agent/blob/master/contrib/add-second-key/main.go to generate a key on the other slot; should not affect the first slot but it hasn't been tested at all - so assume it will destroy the PIV content. (It shouldn't.)

@hlidotbe
Copy link

@joneskoo second key generated correctly without issue. I did a quick test and it seems to work well.

I'll run some more testing but so far so good, thanks!

@zachgersh
Copy link

I very much want to +1 this. Doing anything go.mod related when you have to authorize each SSH connection like @hlidotbe said is a real pain.

I'd hope this would be accepted!

@FiloSottile FiloSottile added the enhancement New feature or request label Jul 15, 2020
@RussellRollins
Copy link

I would also like to mention the same go.mod use case as @zachgersh, the first time I open a new project in VSCode and the automation starts trying to parse the go.mod file, it's just me tapping my YubiKey for a few minutes. Creating a specific SSH key for GitHub auth would solve that issue for me.

@smlx
Copy link

smlx commented Sep 9, 2020

Could this use-case be solved by the setting touch policy to "cache for 15 seconds"?

@drod3763
Copy link

drod3763 commented Sep 9, 2020

Honestly I don't want to enable touch confirm at all. As long as I have the key it should work, its not like its a biometric scanner.

@hlidotbe
Copy link

hlidotbe commented Sep 9, 2020

@smlx if it's a moving window that could help yes but @joneskoo solution works better for me, a touchless key for basic operations and a more secure one for critical operations

@joneskoo
Copy link
Author

I opened a new cleaner PR looking for feedback.

@9ary
Copy link

9ary commented Apr 10, 2023

I'd like to help out with this. While #57 is a good proof of concept, I think the approach should be revised.

One point is that we should definitely support (and even favor) using the 20 retired slots, which are pretty much free for arbitrary use, rather than (ab)use the 4 standard slots for this. Maybe it's also a good idea to filter certs based on the organization field, like age-plugin-yubikey does.

This brings us to the performance concern, because 24 slots is going to slow things down a lot more than 4. The proper solution in my opinion is to listen for hotplug with SCardGetStatusChange (which will most likely require support in go-piv). This would allow enumerating and caching the certs on a key when it's plugged in/inserted, and at that point support for multiple yubikeys almost comes for free.
Edit: seems like macOS does not support detecting reader hotplug unfortunately, so that's unfortunate. There might be other ways to detect hotplug of USB devices, but I don't know how macOS works. Comments I've found online suggest polling with a 1s interval which might be an acceptable workaround. https://ludovicrousseau.blogspot.com/2018/02/how-to-use-scardgetstatuschange.html

Lastly for the setup script, my preference would be a simple wrapper over ykman's python API. I've already used the ykman CLI to generate P384 keys (questionable decision, but it works), but more importantly so that I could use an AES management key.
This should be a contrib script for advanced users who want extra control, and it would absolutely not replace the existing go setup.

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

Successfully merging a pull request may close this issue.

8 participants