chore: migrate to crypto/ecdh

veraison / go-cose

go library for CBOR Object Signing and Encryption (COSE)

Mozilla Public License 2.0

49 stars 26 forks source link

chore: migrate to crypto/ecdh #178

Closed shizhMSFT closed 5 months ago

shizhMSFT commented 8 months ago

Resolves #168 by migrating to crypto/ecdh.

Here are notable changes:

Since crypto/ecdh is introduced in go 1.21, the minimum go version is thus bumped to 1.21 from 1.18.
Since crypto/ecdh does not expose low-level elliptic curve operations, custom curves are no longer supported. Thus the related test case is removed. /cc @qmuntal

shizhMSFT commented 8 months ago

The CI tests are failing since go-cose is not ready for go 1.21 yet as its current support windows is go [1.18, 1.19].

shizhMSFT commented 8 months ago

I would suggest reviewing and merging PR after the go 1.22 release (ETA: Feb 2024) so that we can move the support window to [1.21, 1.22] for crypto/ecdh.

OR13 commented 8 months ago

Seems like this PR is on a good track, once CI is passing, and our test coverage is recovered, this seems like a great improvement

SteveLasker commented 6 months ago

@shizhMSFT, with 1.22 released, are you ready to finalize this PR?

qmuntal commented 6 months ago

I'm not sold about this solution. x509.ParsePKCS8PrivateKey calls curve.ScalarBaseMult(d) under the hood, so all the additional work we do, namely mapping an ECDSA key to an ECDH key, then encoding a x509 and finally decoding the same key, boils down to avoiding a deprecated function call by using a higher level functions that uses the same deprecated function.

IMO, we should either remove support for ECDSA curves without public component, or assume that we have to call a deprecated function.

shizhMSFT commented 6 months ago

I'm not sold about this solution. x509.ParsePKCS8PrivateKey calls curve.ScalarBaseMult(d) under the hood, so all the additional work we do, namely mapping an ECDSA key to an ECDH key, then encoding a x509 and finally decoding the same key, boils down to avoiding a deprecated function call by using a higher level functions that uses the same deprecated function.

I agree with that.

assume that we have to call a deprecated function

I'm afraid of being scanned as a vulnerability by some scanners in the future although it is not.

remove support for ECDSA curves without public component

Can you elaborate more on this?

qmuntal commented 6 months ago

Can you elaborate more on this?

Do what @OR13 recommended here:

I'd recommend just throwing when compressed points are passed... and not doing the point compression, and that would eliminate the warning.

OR13 commented 6 months ago

Unless you are parsing a key that is inside a signature or encryption, developers can translate keys with compressed points to ones with uncompressed points.

Any library that doesn't support compressed points will need to throw if it cannot translate, because operations will fail that required both points.

shizhMSFT commented 5 months ago

Thanks for the elaboration.

remove support for ECDSA curves without public component

Do we want to go that way?

SteveLasker commented 5 months ago

Based on the above, we're going to close this PR and open a new issue for guidance.