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

Yubikey support! #108

Closed
dlorenc opened this issue Mar 19, 2021 · 7 comments · Fixed by #256
Closed

Yubikey support! #108

dlorenc opened this issue Mar 19, 2021 · 7 comments · Fixed by #256

Comments

@dlorenc
Copy link
Member

dlorenc commented Mar 19, 2021

We should be able to support storing keys in yubikeys!

https://github.com/go-piv/piv-go
https://github.com/go-piv/go-ykpiv

@dlorenc
Copy link
Member Author

dlorenc commented Mar 20, 2021

@dlorenc dlorenc mentioned this issue Mar 25, 2021
@dlorenc
Copy link
Member Author

dlorenc commented Mar 26, 2021

We need to do a little bit of thinking on the design here before going too much further. I'll explain some background:

Yubikeys support 3 different levels of protection:

  • a 6 digit PIN
  • an 8 digit PUK
  • a 24 byte management key

The defaults can be found here:
image

There's a command line tool from yubikey to configure these, but it might be nice if we allow users to set it up directly in our tooling.

Here's what an ideal setup workflow might be:

$ cosign generate-key-pair --yubikey
Please enter the device management key, or press enter for the default:
Default management key selected. Would you like to change this? [y/N]:
Changing management key. Enter new key, up to 24 characters:
Confirm new management key:
Management key reset.

Would you like to set a key PIN? [y/N]: y
Please enter a 6 digit key PIN:
Confirm key pin:
Generating a private key on the device...

Key generated!
Public key written to cosign.pub
Device attestation certificate written to yubikey.crt

We could also allow configuring a PUK here, which is used to unlock the device if the pin is entered incorrectly too many times. It might be better to just bail out at that point and send them to the real yubikey tool though.

Same for resetting the entire thing.

@dlorenc
Copy link
Member Author

dlorenc commented Mar 27, 2021

I want to get the balance right between "I just took a yubikey out of a box and want to start using it to sign things" with more advanced yubikey users/workflows where they might be reusing the same hardware for FIDO2/U2F etc., or they have specially provisioned corporate yubikeys that have already been setup with a fixed management key.

@dlorenc
Copy link
Member Author

dlorenc commented Mar 27, 2021

Also, I'd hope most of this logic ends up in the http://github.com/sigstore/sigstore client/libraries so it can be used for all artifact types!

@lukehinds
Copy link
Member

lukehinds commented Mar 27, 2021

I see what you mean.

On the one hand it's nice to be able to set this up for users, that is a really good UX. On the other, if we get them to set up using the upstream tool, we won't get any blowback if they can no longer log into their bank.

Perhaps a warning?

WARNING: Setting a pin may cause loss of current key configurations
Would you like to set a key PIN? [y/N]: y
Please enter a 6 digit key PIN:
Confirm key pin:
Generating a private key on the device...

@OR13
Copy link

OR13 commented Mar 29, 2021

I know this is super hacky, but feel free to borrow any ideas from it:

https://github.com/OR13/lds-pgp2021/blob/main/bin/cli.js#L134

I have tested it with Yubikey.

This was referenced Apr 9, 2021
@axelsimon
Copy link
Contributor

I think one might be even more explicit and say something like:

WARNING: Setting a PIN may cause loss of current key configurations
WARNING: You will lose access to resources currently configured to use this key!
Would you like to set a key PIN? [y/N]: y
Please enter a 6 digit key PIN:
Confirm key pin:
Generating a private key on the device...

Yubikeys usually have two slots, so there's always the option of using slot #2 by default, but even then one can't assume it's not already configured :)

tommyd450 pushed a commit to tommyd450/cosign that referenced this issue Jan 16, 2024
Co-authored-by: red-hat-trusted-app-pipeline <123456+red-hat-trusted-app-pipeline[bot]@users.noreply.github.com>
tommyd450 pushed a commit to tommyd450/cosign that referenced this issue Jun 5, 2024
Co-authored-by: red-hat-trusted-app-pipeline <123456+red-hat-trusted-app-pipeline[bot]@users.noreply.github.com>
tommyd450 pushed a commit to tommyd450/cosign that referenced this issue Jun 5, 2024
Co-authored-by: red-hat-trusted-app-pipeline <123456+red-hat-trusted-app-pipeline[bot]@users.noreply.github.com>
tommyd450 pushed a commit to tommyd450/cosign that referenced this issue Jun 5, 2024
Co-authored-by: red-hat-trusted-app-pipeline <123456+red-hat-trusted-app-pipeline[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants