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

Support ECDH for ECDSA keys #80

Merged
merged 1 commit into from
Sep 8, 2020
Merged

Support ECDH for ECDSA keys #80

merged 1 commit into from
Sep 8, 2020

Conversation

tv42
Copy link
Contributor

@tv42 tv42 commented Sep 4, 2020

Feedback on APIs would be most welcome.

Fixes #79

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.

Thanks for sending a PR :)

Keys implement an optional interface because it's what the standard library wants. The only prior art I could find for Diffie-Hellman was:

https://github.com/golang/go/blob/01a9cf8487df2b108f0dfd7060ff5ffbda972c3a/src/crypto/tls/key_schedule.go#L104-L110

I think this package shouldn't establishing a key agreement interface. Maybe just export keyECDSA as concrete type that you can type switch on?

type ECPrivetKey struct {}

func (k *ECPrivateKey) Public() crypto.Public {}
func (k *ECPrivateKey) Sign(rand io.Reader, digest []byte, opts crypto.SignerOpts) ([]byte, error)
func (k *ECPrivateKey) KeyAgreement(peer *ecdsa.PublicKey) ([]byte, error)

Then using the API looks like:

priv, err := yk.PrivateKey(slot, pub, auth)
if err != nil {
    // ...
}
ecPriv, ok := priv.(*piv.ECPrivateKey)
if !ok {
    // ...
}
sharedKey, err := ecPriv.KeyAgreement(peer)

If there's ever an actual key agreement interface to satisfy in the future, then ECPrivateKey can implement that later.

piv/key.go Outdated Show resolved Hide resolved
piv/key.go Outdated Show resolved Hide resolved
piv/key_test.go Outdated Show resolved Hide resolved
@tv42
Copy link
Contributor Author

tv42 commented Sep 5, 2020

Ooh I hadn't found the stdlib one. Of course it makes sense TLS would have one. I'll happily steal the SharedKey name for that.

The reason I exported an interface, not a concrete type, was that I was thinking there might be a non-ECDSA keytype that can do Diffie-Hellman on the yubikey. But yeah the interface is easy to add later, if and when. I can switch to a concrete type.

@tv42
Copy link
Contributor Author

tv42 commented Sep 5, 2020

Naming question: You used ECPrivateKey. Since this is specifically a P-nnn key that is ECDSA, and will refuse to do DH with non-ECDSA keys, I think it should be ECDSAPrivateKey then. We're not speaking on behalf of all elliptic curve crypto. Callers can always use handle multiple concrete types with interfaces.

@tv42 tv42 marked this pull request as draft September 5, 2020 16:05
@ericchiang
Copy link
Collaborator

ECDSAPrivateKey works for me

@tv42
Copy link
Contributor Author

tv42 commented Sep 5, 2020

Pushed fixups for all feedback above. If you're happy with the end result, I'll rebase them together and take the draft flag off this PR.

@tv42 tv42 requested a review from ericchiang September 5, 2020 16:52
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.

Couple comments, but other than that lgtm. Thanks for this!

piv/key.go Outdated Show resolved Hide resolved
piv/key.go Show resolved Hide resolved
piv/key.go Outdated
@@ -667,24 +671,82 @@ func (yk *YubiKey) PrivateKey(slot Slot, public crypto.PublicKey, auth KeyAuth)
}
}

type keyECDSA struct {
// ECDSAPrivateKey is used to access a ECDSA signing key.
Copy link
Collaborator

@ericchiang ericchiang Sep 5, 2020

Choose a reason for hiding this comment

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

nit: include user instructions to get a *ECDSAPrivateKey. Anyone reading the docs will probably appreciate that :)

// ECDSAPrivateKey is a crypto.PrivateKey implementation used to directly access ECDSA
// operations. Keys returned by PrivateKey() can be type casted to use these methods.
//
//     priv, err := yk.PrivateKey(slot, pub, auth)
//     if err != nil {
//         // ...
//     }
//     ecdsaPriv, ok := priv.(*piv. ECDSAPrivateKey)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wrote proper examples for SharedKey, that should also demonstrate this, and the documentation for the type referring there.

@ericchiang
Copy link
Collaborator

Oh an please squash your commits :) I can merge after

@tv42
Copy link
Contributor Author

tv42 commented Sep 5, 2020

Squashed, all ready to go. (Branch wip-ecdh has the commit-by-commit action, if you want to see the latest changes.)

@tv42 tv42 marked this pull request as ready for review September 5, 2020 18:52
@tv42
Copy link
Contributor Author

tv42 commented Sep 5, 2020

Oops, sorry, I want to clean up one more nit in the example.

@tv42 tv42 marked this pull request as draft September 5, 2020 18:54
@tv42
Copy link
Contributor Author

tv42 commented Sep 5, 2020

Nope, nevermind, what I needed to do was refresh my godoc window to see that I had already fixed the nit. All good!

@tv42 tv42 marked this pull request as ready for review September 5, 2020 18:56
piv/example_test.go Outdated Show resolved Hide resolved
piv/key.go Outdated Show resolved Hide resolved
@ericchiang ericchiang merged commit bdd9a61 into go-piv:master Sep 8, 2020
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.

ECDH for using P-384 keys for more than signing
2 participants