go-piv / piv-go

Keys and certificates for YubiKeys, written in Go
Apache License 2.0
366 stars 65 forks source link

Add new algorithms supported in firmware 5.7.x #157

Closed maraino closed 1 month ago

maraino commented 2 months ago

This commit supports the new algorithms supported on YubiKeys with a 5.7.x firmware. It adds support for RSA-3072, RSA-4096, Ed25519, and X25519.

Generating X25519 keys is only supported with Go 1.20+, which adds support for the crypto/ecdh package.

Note that this commit removes the support for Ed25519 on SoloKeys if the algorithm identifier is set to 0x22.

Fixes #143

AGWA commented 1 month ago

Instead of adding a new exported type X25519PrivateKey, I propose the following:

  1. Define the following interface (which is also satisfied by *ecdh.PrivateKey):
    type ECDHer interface {
    ECDH(*ecdh.PublicKey) ([]byte, error)
    }
  2. Rename SharedKey to ECDH.
  3. Add an ECDH function to ECDSAPrivateKey that satisfies this interface.
  4. Deprecate ECDSAPrivateKey.SharedKey
  5. Update the documentation for YubiKey.PrivateKey to say "The returned key implements crypto.Signer, crypto.Decrypter, and/or ECDHer" depending on the key type.

*ecdh.PrivateKeys and hardware-backed ECDH keys would implement the same interface, providing the same flexibility that signing and decrypting keys offer via crypto.Signer and crypto.Decrypter.

maraino commented 1 month ago

@ericchiang what do you think about @AGWA proposal? My initial implementation was like that, but I noticed that ECDSA was using SharedKey, so I changed it.

@AGWA I'm open to renaming SharedKey to ECDH if @ericchiang is ok with it. But it isn't necessary to add the interface here. The software using this package can create its own.

ericchiang commented 1 month ago

I agree that we shouldn't create new interfaces here. At the point where you use the interface, you have to be aware of piv-go anyway. If this is going to be a common definition, it probably belongs in https://pkg.go.dev/crypto, similar to crypto.Signer.

I'd prefer keeping "SharedKey" to be consistent within this package with ECDSAPrivateKey. Not thrilled with deprecating things for deprecation sake

AGWA commented 1 month ago

I agree that the interface doesn't need to be defined here, but I'm still hoping we can call the function ECDH for consistency with ecdh.PrivateKey.

I suggested deprecating ECDSAPrivateKey.SharedKey because doing ECDH with crypto/elliptic and crypto/ecdsa has been deprecated in the standard library in favor of crypto/ecdh. The new ECDSAPrivateKey.ECDH method would take a *ecdh.PublicKey instead of a *ecdsa.PublicKey. I've submitted a PR #158 for discussion. I'm not trying to create pointless churn here; I'm working on a project where I would like to use hardware and software ECDH keys interchangeably and having a common, modern interface for ECDH would be extremely convenient compared to ecdh.PrivateKey, ECDSAPrivateKey and X25519PrivateKey all having a different interface.

ericchiang commented 1 month ago

Thanks https://github.com/go-piv/piv-go/pull/158 is helpful! I didn't realize the proposal is to change the signature, not just the name. I'm fine with supporting the newer crypto/ecdh package keys through a "ECDH" method.

The parts of crypto/elliptic that were deprecated were those used to do ecdh, getting math/big out of the crypto paths for the standard library. piv-go doesn't use it this way (DH is done on the hardware). I don't see the need to deprecate the SharedKey method.

I'll merge #158. @maraino sorry for the flip flop, but the name "ECDH" does seem reasonable if we're accepting *ecdh.PublicKey

maraino commented 1 month ago

@ericchiang, I've fixed the issues with my latest commit cfec690. I didn't implement the func (*ECDSAPrivateKey) ECDH(*ecdh.PublicKey). I might do this in a new commit to fix the new issue created by @AGWA.

AGWA commented 1 month ago

@maraino no need to implement func (*ECDSAPrivateKey) ECDH(*ecdh.PublicKey) - my PR for that has already been merged.

ericchiang commented 1 month ago

Thanks everyone for the feedback here! I've gone ahead and tagged v2.2.0 including these changes https://github.com/go-piv/piv-go/releases/tag/v2.2.0