Add support for additional FIPS-certified Yubikeys #98
Conversation
piv/key.go
Outdated
| FormfactorUSBANano | ||
| FormfactorUSBCKeychain | ||
| FormfactorUSBCNano | ||
| FormfactorUSBCLightningKeychain | ||
|
|
||
| FormfactorUSBAKeychainFIPS = iota + 0x81 |
There was a problem hiding this comment.
The reasoning for resetting the iota increment here (apart from making the values here explicitly equivalent to their vendor values) is that it's possible that we'll want to add to the middle and end of this list if new regular and FIPS variants appear, in which case keeping compatibility with the implied public interface would require that we mix-and-match the names.
There was a problem hiding this comment.
See my other comment, let's drop the iota
There was a problem hiding this comment.
Awesome, thanks for the review! I'll give this a do-over and write some tests -- would you rather I rebase or are the intermediate commits good to keep?
There was a problem hiding this comment.
Please ensure there's only one commit. So rebasing sgtm.
Tests might be overkill? Testing that a constant is a specific value seems like it's covered by the constant declaration :)
piv/key.go
Outdated
| case 0x05: | ||
| a.Formfactor = FormfactorUSBCLightningKeychain | ||
| switch { | ||
| case e.Value[0] >= FormfactorUSBAKeychain && e.Value[0] <= FormfactorUSBCLightningKeychain: |
There was a problem hiding this comment.
This is terrible, but again the reasoning here is that adding new variants would only require a change to that final FormfactorUSBCLightningKeychain name.
There was a problem hiding this comment.
Feel free to remove this switch. It was there as a safeguard when I was developing this package, but I think we can let users handle the unknown. :)
Will also prevent users from having to send PRs every time there's a new form factor.
e.Formfactor = Formfactor(e.Value[0])
There was a problem hiding this comment.
Bit of background. Originally these didn't correspond to the values presented by the YubiKey, later we switched them to match: #73
We probably should remove the "iota" in #74, but let's do that now. Can you update this to not use an iota?
const (
FormfactorUSBAKeychain = 0x01
FormfactorUSBANano = 0x02
// ...
FormfactorUSBAKeychainFIPS = 0x81
FormfactorUSBANanoFIPS = 0x82
)Probably good to add a String() method so these print nicely?
var formFactorStrings = map[Formfactor]string{
FormfactorUSBAKeychain: "USB-A keychain",
FormfactorUSBANano : "USB-A nano",
}
func (f Formfactor) String() string {
if s, ok := formFactorStrings[f]; ok {
return s
}
return fmt.Sprintf("unknown(0x%02x)", int(f))
}
piv/key.go
Outdated
| case 0x05: | ||
| a.Formfactor = FormfactorUSBCLightningKeychain | ||
| switch { | ||
| case e.Value[0] >= FormfactorUSBAKeychain && e.Value[0] <= FormfactorUSBCLightningKeychain: |
There was a problem hiding this comment.
Feel free to remove this switch. It was there as a safeguard when I was developing this package, but I think we can let users handle the unknown. :)
Will also prevent users from having to send PRs every time there's a new form factor.
e.Formfactor = Formfactor(e.Value[0])
This commit adds support for all known existing FIPS-certified Yubikey variants, and makes adding more of these additional variants simpler by ensuring that values for Formfactor constants follow those for the form-factors emitted by Yubikeys themselves.
|
Fixed and pushed, let's see if tests pass! |
|
Thank you! Do you plan on tagging a release soon? I'm looking to enable |
|
That's awesome, thank you so much! |
This commit adds support for all known existing FIPS-certified Yubikey variants, and makes
adding more of these additional variants simpler by ensuring that values for
Formfactorconstants follow those for the form-factors emitted by Yubikeys themselves.