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

WIP: Support Ed25519 keys in setup #26

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nickray
Copy link

@nickray nickray commented May 18, 2020

Thoughts on this? Would adjust dependency if/when go-piv/piv-go#69 is accepted.

@FiloSottile
Copy link
Owner

This would be awesome! It will need to be autodetected though, we should not ask the user to pick.

What's the server-side compatibility situation for ed25519 keys?

@nickray
Copy link
Author

nickray commented May 18, 2020

RHEL 7, Debian Jessie and GitHub support them, need to go pretty far back (RHEL 6, Debian Wheezy), to lack server support (Jan 2014, https://www.openssh.com/txt/release-6.5). We're likely the only pseudo-PIV to support them though, as SP 800-78-4 doesn't include them. Given that, I see it as an experimental feature that merits a flag?

I don't know of any way to autodetect except trying to generate a 25519, on failure attempt a p256. Would that work? That would put the majority of existing keys (all Yubico) on unhappy path though.

If we do first-25519-then-256, would it make sense to add a --force-p256 for testing purposes, or user choice?

@FiloSottile
Copy link
Owner

Hmm, you can't issue a Version command or anything like that? I am really reticent to add flags, but first-25519-then-256 does seem too aggressive. If you could find a way to cleanly check, I'd prefer that (and no need for a flag to downgrade), otherwise for now the -ed25519 flag will do.

Please rebase now that the piv-go patch landed.

@matthewrees
Copy link

I believe Ed25519 support for Yubikey devices landed in firmware 5.2.3 and above, here are the details, so a firmware version check is likely the cleanest way to default to Ed25519 else 256. Thoughts?

@matthewrees
Copy link

Scratch the above, Ed25519 support has only been added for the OpenPGP applet, not for the PIV applet. I confirmed in a personal branch that a YubiKey with firmware 5.2.4 cannot accept the request for Ed25519 currently present in piv-go:
Failed to generate key: command failed: smart card error 6a80: incorrect parameter in command data field

This is confirmed by a comment in piv-go that it is currently only supported by SoloKeys devices and by the output of yubico-piv-tool that the ECD25519 support isn't available:

  -A, --algorithm=ENUM     What algorithm to use  (possible values="RSA1024",
                             "RSA2048", "ECCP256", "ECCP384"
                             default=`RSA2048')

@nickray
Copy link
Author

nickray commented Jun 27, 2020

I rebased, I hope go.sum is not a mess now.

To give some context on the flag and YubiKeys:

  • Ed25519 is not part of NIST PIV (and likely won't be for a while?)
  • I assume this is why Yubico added it to their OpenPGP (which officially has it) but not to PIV (they'd have to pick their own alg value)
  • piv-go kindly merged my suggested patch (using 0x22 as alg value), which however therefore can only currently be implemented by SoloKeys
  • @arekinath and us (SoloKeys) have chatted about drumming up some collaboration around open source/non-NIST PIV clarification and extension allocation (things like declaring supported algorithms in application property template, cf. piv: implement algorithm discovery go-piv/piv-go#1 (comment), or allocating non-official alg values in a principled way, for instance using non-interindustry class to definitely ensure no alg value clash ever), nothing to report yet though
  • since yubikey-agent does PIV via piv-go, it should be insulated from these matters
  • still, this Ed25519 support is experimental/non-NIST

So on the one hand I'd understand if you prefer not to merge this (although, why not ^^), on the other, for the above reasons I really think it should be an experimental flag and not an official feature.

If and when a community rally succeeds (perhaps even getting Yubico on board?), the algorithm discovery and a possibly amended alg value for Ed25519 could be considered more stable and used for auto-selection.

What do you think?

@FiloSottile
Copy link
Owner

Congrats on the launch of the Solo V2! I've been thinking about this a bit, and even if it's experimental it's in real hardware, and it's not going away from keys that have it, and keys are tied to the hardware anyway, so I am comfortable autodetecting it without any flag.

If you can give me a way to reliably detect supported SoloKeys, happy to switch them to Ed25519 setup by default.

Base automatically changed from master to main February 11, 2021 12:14
@zzywysm
Copy link

zzywysm commented Oct 15, 2021

Any movement on this? I would really like this feature. (Pretty please, with a 🍒 on top!)

@nickray
Copy link
Author

nickray commented Oct 23, 2021

I'm coordinating with other projects (PivApplet @arekinath, OpenFIPS201 @makinako) on which alg value to use. In retrospect, 0x22 was a somewhat unfortunate choice (it's in the range used for secure messaging). The word from NIST is that they won't be adding Ed255 (they're all about postquantum...), so we can't use official alg values and have to "guess" values that won't be clobbered by official ones. Current thinking is to use 0xE? and 0xF? (cf. arekinath/pivy#7).

That would entail a change in piv-go first, and then an update here. I propose waiting on some kind of consensus in the "open source PIV world" on the alg choices to avoid back and forth.

@michaelblyons
Copy link

Still blocked on cat-wrangling?

@makinako
Copy link

A few options:

  1. Define a 'GENERAL AUTHENTICATE EXTENDED' APDU which is identical except for the INS value (and we can then use any mechanism we like). This might seem like a downside because it breaks middleware, but any client/middleware that wants to make use of these extended algorithms needs to have functional awareness anyway.

  2. 0xE0-0xFF becomes the best guess at an 'unallocated' zone as Nick described. NIST appears to be philosophically against supporting extensible keys, mechanisms or data containers (most likely due to the interoperability problem) so I don't expect support for this now or in the future.

  3. Get a bit more greedy and say that any mechanism with the MSB set (0x80) set is a non-standard mechanism. See table below for the INCITS 504 mechanism table which is a superset of the one defined in SP800-78. Note also that it only reserves '2E-3F' as RFU which might give an insight into the numbers NIST will intend to use?
    image

  4. Define a new data container that contains an OID -> MECHANISM map. The standard PIV mechanisms will always be defined and then any supported extended mechanisms are mapped to whatever numbers we like. The up-side is that these can change in an extensible way and the down-side is it requires an additional call to find out the algorithm support (unless you cache card types you already know).

For simplicity, I'd probably vote for option 3 personally but I'm open to anything.

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 this pull request may close these issues.

6 participants