-
Notifications
You must be signed in to change notification settings - Fork 65
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
Implement import key functionality #83
Conversation
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.
For consistency this should probably be "SetPrivateKey"
Since there's already a struct that holds the policies, can the arguments be:
func (yk *YubiKey) SetPrivateKey(key [24]byte, slot Slot, priv crypto.PrivateKey, policy Key)
Or we can refactor this to:
type KeyPolicy struct {
PINPolicy PINPolicy
TouchPolicy TouchPolicy
// Opt-in required to use imported private keys?
InsecureAllowImportedPrivateKeys bool
}
func (yk *YubiKey) SetPrivateKey(key [24]byte, slot Slot, priv crypto.PrivateKey, policy KeyPolicy)
I can't review the rest without a link to the spec. You can see a few comments that have links to pages in the NIST pdf. Can you include the one that defines the key parameters:
https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-73-4.pdf#page=95
Finally, I think there should be warnings associated with this method:
// SetPrivateKey is an insecure method which imports a private key into the slot.
// Users should almost always use GeneratePrivateKey() instead.
//
// Importing a private key breaks functionality provided by this package, including
// AttestationCertificate() and Attest(). There are no stability guarantees for other
// methods for imported private keys.
//
// Keys generated outside of the YubiKey should not be considered hardware-backed,
// as there's no way to prove the key wasn't copied, exfiltrated, or replaced with malicious
// material before being imported.
func (yk *YubiKey) SetPrivateKey(...)
Also, wanted to say thanks for the PR! Apologies for some if some of the "insecure" comments seem dramatic. For security relevant code, I always find it helpful to optimize for reviewers (so they can see an "Insecure..." variable) and developers who are approaching this for the first time. |
Thank you for reviewing my PR! Adding an insecure warning is a good idea. The command is a Yubico extension to PIV. You can find the yubico-piv-tool implementation here and the documentation here. I don't think an InsecureAllowImportedPrivateKeys flag would work because we can't differentiate between an empty slot and an imported key. Would naming the function SetPrivateKeyInsecure be appropriate? |
"SetPrivateKeyInsecure" works for me |
Tests passed on my Macbook. lgtm. Can you squash your changes into a single commit? I'll merge after. |
Thanks for reviewing my changes! The tests also pass on Windows 10. |
Thanks for your contribution! This has been included in the v1.7.0 tag https://github.com/go-piv/piv-go/releases/tag/v1.7.0 |
This is to allow creating backups of the private key in case the YubiKey gets lost or damaged. The word "insecurely" is appended to the name of the switch and a long warning produced in order to make people who don't know what they're doing turn away and stop what they're doing. This is possible since piv-go 1.7.0[1] so I allowed myself to bump the version of this requirement. [1] go-piv/piv-go#83 (comment)
Issue #77 discusses whether this library should support this feature. My use case for implementing this is to be able to create backup Yubikeys with the same private key.
Tested on: Ubuntu 20.04.1 LTS