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

Add new algorithms supported in firmware 5.7.x #157

Merged
merged 3 commits into from
Oct 3, 2024

Conversation

maraino
Copy link
Contributor

@maraino maraino commented Sep 24, 2024

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

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 or importing X25519 keys is only supported with Go 1.20+,
which adds support for the crypto/ecdh package.
v2/piv/key_go120.go Outdated Show resolved Hide resolved
v2/piv/key_go120.go Outdated Show resolved Hide resolved
v2/piv/key_go120.go Outdated Show resolved Hide resolved
v2/piv/key_go120_test.go Outdated Show resolved Hide resolved
v2/piv/key_legacy.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@ericchiang ericchiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! A couple of comments, but otherwise lgtm

v2/piv/key_legacy.go Outdated Show resolved Hide resolved
v2/piv/key_go120.go Outdated Show resolved Hide resolved
v2/piv/key_go120.go Outdated Show resolved Hide resolved
v2/piv/key_go120.go Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto: let's remove this and drop support for earlier versions of Go

@AGWA
Copy link
Contributor

AGWA commented Oct 1, 2024

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)
}
  1. Rename SharedKey to ECDH.
  2. Add an ECDH function to ECDSAPrivateKey that satisfies this interface.
  3. Deprecate ECDSAPrivateKey.SharedKey
  4. 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
Copy link
Contributor Author

maraino commented Oct 2, 2024

@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
Copy link
Collaborator

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
Copy link
Contributor

AGWA commented Oct 2, 2024

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
Copy link
Collaborator

Thanks #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

This commit removes the code to support Go 1.16 to 1.19, requiring now
Go 1.20. With this requirement we can remove the build tags.

It also renames the X25519 SharedKey to ECDH.
@maraino
Copy link
Contributor Author

maraino commented Oct 2, 2024

@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
Copy link
Contributor

AGWA commented Oct 2, 2024

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

@ericchiang ericchiang merged commit 12c06a0 into go-piv:v2 Oct 3, 2024
4 checks passed
@ericchiang
Copy link
Collaborator

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

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.

Support new key types in Yubikey 5.7 firmware
4 participants