Foxboron
Initial yubikey backend keytype support
Same PR for initial yubikey piv support with the intermediate commits removed.
I also removed:
- changes to the config parsing (will address in another pr)
- prompting for yubikey pin (right now uses the default PIN)
Hopefully this helps for reviewing / testing. I think I also addressed all your comments from the previous review. Thank you for your help!
I do think the prompting for the PIN is important to avoid needing to store the pin anywhere, so once we get this PR done I can rework and submit that one separately
Please stop making new PRs :) Rebase and force-push the branch in-place is much easier instead of leaving 3 PRs around in the project.
Sure, I'll do that going forward, I didn't know that worked in a github PR my bad!
Would it be helpful if rebased and force pushed this commit PR over https://github.com/Foxboron/sbctl/pull/413? I can then use this one for the prompt support PR
Would it be helpful if rebased and force pushed this commit PR over https://github.com/Foxboron/sbctl/pull/413? I can then use this one for the prompt support PR
Nah, no we can work in this branch :)
Updated with the requested changes, I also verified that using sbctl to create a key with the yubikey type and signing files is working (for me at least) with the default pin
I saw down to try this change and I can't seem to get it to work. The yubikey blinks for a press, but nothing happens when I press. I suspect the issue is on my side, but I found this a bit weird. Have to spend a little bit of time to figure out why this happens and the potential failure case.
Interesting, I have not encountered that behavior before with my yubikey.
Are you able to create any key on your yubikey in the piv slot with ykman?
ykman piv info
# you can set the touch/pin policy too
ykman piv keys generate --algorithm RSA2048 9a pubkey.test
What happens if you update yubikey.go and change the PIN/Touch policy to never?:
piv.Key{
Algorithm: piv.AlgorithmRSA2048,
PINPolicy: piv.PINPolicyNever,
TouchPolicy: piv.TouchPolicyNever,
}
I just tested after resetting my yubikey piv data and it worked as expected:
$ ykman info
Device type: YubiKey 5C
Serial number: 12341234
Firmware version: 5.7.1
Form factor: Keychain (USB-C)
Enabled USB interfaces: OTP, FIDO, CCID
Applications
Yubico OTP Enabled
FIDO U2F Enabled
FIDO2 Enabled
OATH Enabled
PIV Enabled
OpenPGP Enabled
YubiHSM Auth Enabled
$ ykman piv reset
WARNING! This will delete all stored PIV data and restore factory settings. Proceed? [y/N]: y
Resetting PIV data...
Reset complete. All PIV data has been cleared from the YubiKey.
Your YubiKey now has the default PIN, PUK and Management Key:
PIN: 123456
PUK: 12345678
Management Key: 010203040506070801020304050607080102030405060708
$ ./sbctl create-keys --keytype yubikey
Created Owner UUID 865d27c1-27a4-4b1d-bc5b-36ebf1d39124
Creating RSA2048 key...
Please press Yubikey to confirm presense
Created RSA2048 key MD5: 9a50e63ad7c616fb6aa8f0f5b6ebb680
Creating PK key...
Please press Yubikey to confirm presence for key RSA2048 MD5: 9a50e63ad7c616fb6aa8f0f5b6ebb680
Creating KEK key...
Please press Yubikey to confirm presence for key RSA2048 MD5: 9a50e63ad7c616fb6aa8f0f5b6ebb680
Creating db key...
Please press Yubikey to confirm presence for key RSA2048 MD5: 9a50e63ad7c616fb6aa8f0f5b6ebb680
✓
Secure boot keys created!
How far did it get when you tested? I wonder if there is a missing dependency, do you have pcsclite installed?
Any luck testing? If you were unable to get it working I can take a look on a few different computers I have and try to reproduce the issue.
Sorry!
I haven't managed to look at this again but I'll take a stab at this within the next couple of days :) Thanks!
Hmmmmm, ran the same commands as you and reset the yubikey.
couldn't initialize secure boot: command failed: smart card error 6d00
I suspect there is something wrong with scdaemon when it comes to exclusive vs shared access? I'll try and figure this out on my end
Hmm, I'll do some trouble shooting as well. What yubikey / firmware version are you using?
Device type: YubiKey 5 NFC
Serial number: 9027747
Firmware version: 5.1.1
Form factor: Keychain (USB-A)
Enabled USB interfaces: OTP, FIDO, CCID
NFC transport is enabled
Applications USB NFC
Yubico OTP Enabled Enabled
FIDO U2F Enabled Enabled
FIDO2 Enabled Enabled
OATH Enabled Enabled
PIV Enabled Disabled
OpenPGP Enabled Enabled
YubiHSM Auth Not available Not available
The one I tried with
It seems like Yubico does not support upgrading/downgrading yubikey firmware, I haven't been able to trace down or reproduce your issue yet. Does your yubikey work as expected with ykman?
ykman piv info
ykman piv keys generate --algorithm RSA2048 9a pubkey.test
I don't suppose you've had any luck with trying a different yubikey? I don't think I can easily get a yubikey 5 NFC on the older 5.1.1 to test against. I can keep testing/making changes if you think it's helpful, otherwise I may just keep working on on my own fork and we can close out the pr
I'll try and pick this up again. I do really want yubikey support solved for sbctl but I suspect I need to set off a bit more time to figure out why things are not working as expected.
I tried with a different Yubikey (5 USB A) to make sure everything was working on the branch still and it worked:
$ git clone [email protected]:tomis007/sbctl.git -b initial-yubikey-support
$ make
a2x --no-xmllint --asciidoc-opts="-f docs/asciidoc.conf" -d manpage -f manpage -D docs docs/sbctl.8.txt
a2x --no-xmllint --asciidoc-opts="-f docs/asciidoc.conf" -d manpage -f manpage -D docs docs/sbctl.conf.5.txt
fatal: No names found, cannot describe anything.
go build -ldflags="-X github.com/foxboron/sbctl.Version=" -o sbctl ./cmd/sbctl
$ sudo ./sbctl create-keys --keytype yubikey
Created Owner UUID 018ba2df-8734-4889-bcb5-43f3a52fc21f
Creating RSA2048 key...
Please press Yubikey to confirm presense
Created RSA2048 key MD5: 99bec93ddf3d20fcc208583b611700b2
Creating PK key...
Please press Yubikey to confirm presence for key RSA2048 MD5: 99bec93ddf3d20fcc208583b611700b2
Creating KEK key...
Please press Yubikey to confirm presence for key RSA2048 MD5: 99bec93ddf3d20fcc208583b611700b2
Creating db key...
Please press Yubikey to confirm presence for key RSA2048 MD5: 99bec93ddf3d20fcc208583b611700b2
✓
Secure boot keys created!
$ ykman info
Device type: YubiKey 5 NFC
Serial number: 52734083
Firmware version: 5.7.4
Form factor: Keychain (USB-A)
Enabled USB interfaces: OTP, FIDO, CCID
NFC transport is enabled
Applications USB NFC
Yubico OTP Enabled Enabled
FIDO U2F Enabled Enabled
FIDO2 Enabled Enabled
OATH Enabled Enabled
PIV Enabled Enabled
OpenPGP Enabled Enabled
YubiHSM Auth Enabled Enabled
This was the same error that you described but your Yubikey seems newer than the ones in the issue: https://github.com/go-piv/piv-go/issues/55. I haven't been able to reproduce the issue unfortunately and I have no older yubikeys to test against
Okay, I did some hacking and figured out that KeyInfo() is only supported on yubikeys above version 5.3.0 while all my keys are 5.1.1. So I'll see if we can work around this.
https://github.com/go-piv/piv-go/blob/0383b0aa884b2b642e9e3446ea01ba22ccadc83a/v2/piv/key.go#L673
Alternatively we can merge this as-is and just declare newer yubikeys supported? I'd just need to do another review to make sure I understand it, and I would have problems maintaing this support until I get myself a newer yubikey :melting_face:
Judging by https://github.com/FiloSottile/yubikey-agent/tree/main I'm wondering if we should omit KeyInfo and just save the public key and make assumptions about key policies and so on. Would that work?
We could alternatively store it if the yubikey is of a recent version.
Oh nice find on the cause of the issue. I think it'd be ideal to ensure it works on older firmware versions too rather than just supporting newer ones. I'll take a look at how yubikey-agent manages older keys. It might take me a few days to get around to updating the pr
@Foxboron I updated and refactored to remove the KeyInfo calls so it should hopefully work with your yubikey now. I did some more refactoring to improve the code and the yubikey connection handling. It is ready for a review/testing. The YubikeyReader type in config.go might belong in a better place or perhaps its own file in that directory
Generally I think this is shaping up nicely. I'm a bit concerned as there is not a good way to test this at all. So I might merge this and see if I can find some time to check if I can do a small test setup before release.
Else I'll be a little bit dependent on you being willing to help debug issues :)
I added all the requested changes, except the polling on the yubikey. I am happy to help with future issues! Let me know what you think on the updated code
Were you able to create / sign a uki with your yubikey?
One thing that I am tracking is that I think if the pin is non default there may be an issue with the hook after a uki rebuild gets triggered. The hook would need the correct environment variable to be set otherwise signing will fail. I think that's a future issue to figure out though as this pr is already quite large and eventually I'd like to be able to prompt for the pin instead of storing it
I think that's a future issue to figure out though as this pr is already quite large and eventually I'd like to be able to prompt for the pin instead of storing it
Yes. If we just keep this to the basic yubikey functionality and then try to expend with pin reading later it will be much simpler for us :)
The entire process of prompting for information when we are trying to run interactively is probably going to be tricky. Which was my primary motivation for splitting this up.
Requested changes are addressed and I confirmed yubikey creation/signing is working for my two yubikeys (both are on 5.7.X)
Thanks.
I'll probably go ahead and merge this before the weekend, we can polish this going forward as I won't strive for perfect.
Do you want to clean up the history a bit, merge the fixup commits into logical commits. Or I can do this before I merge.
I cleaned up the commit history. I thought about breaking it into a few different commits, but the yubikey support did not have a clean breakdown and the commits in between were not really functional and in a buggy/incomplete state. I ended up just rebaseing to the current state and fixuped the yubikey specific changes.
I think that is completely fair :)
I'll do a last review and test, hopefully tonight.