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

ECDH for using P-384 keys for more than signing #79

Closed
tv42 opened this issue Sep 3, 2020 · 7 comments · Fixed by #80
Closed

ECDH for using P-384 keys for more than signing #79

tv42 opened this issue Sep 3, 2020 · 7 comments · Fixed by #80

Comments

@tv42
Copy link
Contributor

tv42 commented Sep 3, 2020

go-ykpiv's CGo wrapper of libykpiv has this:

https://godoc.org/github.com/go-piv/go-ykpiv#Slot.Decrypt

f the slot holds an EC key, then we will perform ECDH and return the shared secret. In this case, msg must be the peer's public key in octet form, as specified in section 4.3.6 of ANSI X9.62.

I've tested that to work. I would, however, like to use pure-Go libraries.

I couldn't figure out how to get ECDH working with piv-go. For the CGo version, sign and decrypt look very similar:

https://github.com/go-piv/go-ykpiv/blob/32315ce599d8e21e91880e6ecdce55a98420ddc0/decrypt.go#L68
https://github.com/go-piv/go-ykpiv/blob/32315ce599d8e21e91880e6ecdce55a98420ddc0/sign.go#L166

and so do libykpiv's two functions, they both call the same function:

https://github.com/Yubico/yubico-piv-tool/blob/0b27308560ab4f3df4bc29a179e1c91649ce0629/lib/ykpiv.c#L1144
https://github.com/Yubico/yubico-piv-tool/blob/0b27308560ab4f3df4bc29a179e1c91649ce0629/lib/ykpiv.c#L1126

go-ykpiv has a helpful test case here:

https://github.com/go-piv/go-ykpiv/blob/32315ce599d8e21e91880e6ecdce55a98420ddc0/ykpiv_test.go#L522

But I can't see where exactly signing and decrypting are different to make ECDH work with go-piv. Any thoughts?

@tv42
Copy link
Contributor Author

tv42 commented Sep 3, 2020

@tv42
Copy link
Contributor Author

tv42 commented Sep 3, 2020

I tried reproducing the 0x81->0x85 difference here

marshalASN1(0x81, digest)...)),

but that just broke with

command failed: smart card error 6a80: incorrect parameter in command data field

Same yubikey, same slot, works with ykpiv. I must be still doing something wrong.

@tv42
Copy link
Contributor Author

tv42 commented Sep 3, 2020

Ah, signing was silently truncating the input still. Commenting out this line kludges ykSignECDSA into a ECDH function.

digest = digest[:orderBytes]

@tv42
Copy link
Contributor Author

tv42 commented Sep 3, 2020

Now, obviously the above is not the right way to do it, just a proof of concept.

How should the real API look like?

I'm not thrilled by the idea of a Decrypt that doesn't really decrypt. Should YubiKey.PrivateKey return value learn an extra optional interface?

type KeyAgreement interface {
    // Perform a Diffie-Hellman key agreement with the peer.
    //
    // Peer's public key must use the same algorithm as
    // the key in this slot, or returns error ErrMismatchingAlgorithms.
    KeyAgreement(peer crypto.PublicKey) ([]byte, error)
}

@tv42
Copy link
Contributor Author

tv42 commented Sep 4, 2020

I guess that KeyAgreement should take rand io.Reader and opts crypto.SignerOpts too, even if ECDSA doesn't use them?

Any thoughts on the name of the interface, can't -er name it easily. KeyAgreementer. KeyAgreer. Are there good synonyms for the Diffie-Hellman operation? I already intentionally made it not say EC or DH, because I thought both of those were algorithm-specific details.

@tv42
Copy link
Contributor Author

tv42 commented Sep 4, 2020

I'll have a pull request ready as soon as I figure out how this library is tested.

@tv42
Copy link
Contributor Author

tv42 commented Sep 4, 2020

First stab is at https://github.com/tv42/piv-go/tree/wip-ecdh, without tests for now.

tv42 added a commit to tv42/piv-go that referenced this issue Sep 4, 2020
tv42 added a commit to tv42/piv-go that referenced this issue Sep 4, 2020
tv42 added a commit to tv42/piv-go that referenced this issue Sep 5, 2020
tv42 added a commit to tv42/piv-go that referenced this issue Sep 8, 2020
ericchiang pushed a commit that referenced this issue 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 a pull request may close this issue.

1 participant