Add dynamic support for AES128/192/256 management keys #149
Conversation
Quantu
force-pushed
the
support_aes_managment_keys
branch
from
60628a9
to
e4fec72
Compare
|
I also have a branch where I've added the ability to set PIN/PUK retries, but it is based off the changes I've already made in this PR. I'll wait to make a PR for that functionality until after this PR is resolved one way or another. |
|
Thank you for this. Tested on a 5C NFC YubiKey on 5.7.1 firmware. Ran a number of operations like generating a private key, configuring PIV, and setting custom management keys without any issues. |
|
Thanks! I'm on vacation this week, so will look at this more after the holiday. One note is that because this PR changes an exported signature, we'll have to create a v2 release. I can setup the branches when I'm back. |
|
@ericchiang I did a little bit of additional cleanup/polish. Functionally unnecessary, but I got bored 🤷♂️ Whenever you get a chance to review this / set up the branches for v2, I'm done messing with it. |
|
I've gone ahead and started a v2 branch https://github.com/go-piv/piv-go/releases/tag/v2.0.0-alpha.2 Still confirming that it works with Go modules, but feel free to target that branch |
Quantu
force-pushed
the
support_aes_managment_keys
branch
from
a14338c
to
05f0241
Compare
|
Okay, rebased against v2 and updated PR to target v2 branch 👍 |
|
I have successfully tested updating to use the new version at |
…ting them. Prefer AES192 over 3DES when possible. Update supportsVersion to use *version and bytes instead of Version and ints to eliminate a lot of unnecessary calls to .Version()
Quantu
force-pushed
the
support_aes_managment_keys
branch
from
28bb133
to
08d04ae
Compare
|
Squashed! |
|
@ericchiang I don't know what happened, but I noticed v2 is different from my pre-squash code. Somehow when I squashed my commits, parts of the fixes I did to address your comments disappeared (really drawing attention to the danger of force pushing 😬). I am currently working on getting the missing changes restored to my branch, and then I can open a small PR to get those into v2. |
| func ykSetManagementKey(tx *scTx, key [24]byte, touch bool) error { | ||
| func ykSetManagementKey(tx *scTx, key []byte, touch bool, version *version) error { | ||
| var managementKeyType byte | ||
| if supportsVersion(version, 5, 4, 0) { |
There was a problem hiding this comment.
As described in #146 (comment), AES support was only added in 5.4.2, not 5.4.0.
There was a problem hiding this comment.
Yubico claims keys 5.4+ support AES keys. If it can be confirmed a 5.4.0 or 5.4.1 key doesn't support them, I'd say we should fix that. Otherwise, I went with their documentation here (perhaps there is a different page of their docs that conflicts this?): https://developers.yubico.com/PIV/Introduction/YubiKey_and_PIV.html
There was a problem hiding this comment.
Yeah there's conflicting information here, which is where I found 5.4.2. I think you'd have to find someone with a 5.4.0 or 5.4.1 key to confirm this.
There was a problem hiding this comment.
That's entertaining. That article links to the page I linked "for more details". My suspicion is that they don't actually sell keys with every point release of firmware. They may have added it in 5.4.2, but maybe 5.4.2 was the first 5.4 release to make it into keys they sold. This would effectively make both docs correct in saying all 5.4 keys support it, and that it was added in 5.4.2.
I may reach out to Yubico support to try to get clarification.
There was a problem hiding this comment.
It appears that 5.4.2 is in fact the first 5.4 release to be sold based on the following 2 articles. Ten days after announcing 5.4.2 as the initial release for yubikey 5 fips they announce the shipping of 5.4 keys.
5.4.2 is the initial firmware version for yubikey 5 fips keys (article 4 May, 2021)
https://support.yubico.com/hc/en-us/articles/360021443340-YubiKey-5-NFC-FIPS
5.4 started shipping on yubikey 5 series keys (article 14 May, 2021)
https://www.yubico.com/blog/yubikey-firmware-update-yubikey-5-series-with-firmware-5-4/
I think this version check is good to leave as-is.
Add dynamic support for AES128/192/256 management keys, including setting them. Prefer AES192 over 3DES when possible.
This also fixes the issue introduced by the default management key type change in yubikeys 5.7.0+