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

Replace ring ecdsa implementation with rust cryptos #208

Merged
merged 1 commit into from
Mar 4, 2023

Conversation

Xynnn007
Copy link
Contributor

Hi, I'm working on the downstream crates and find that the ring crate used in this crate cannot be compiled in arch s390x. Thus I summit this PR to replace the ring with RustCrypto to implement Elliptic Curve Signatures.

Please take a look if I did anything contrary to the design of this module. Thanks!

Main change

  • Replaced ring elliptic curve signature implementations with RustCrypto
  • Added a compability test for previous ring implementation and current rust crypto to ensure the functionality of API does not change

cc @flavio who is the initial writer of the related code

@Xynnn007 Xynnn007 force-pushed the refactor-replace-ring branch 2 times, most recently from e60725a to 89d7dca Compare February 28, 2023 02:28
@Xynnn007
Copy link
Contributor Author

Update: delete ecdsa crate. I have test successfully locally and now it is ready to review

Copy link
Contributor

@flavio flavio left a comment

Choose a reason for hiding this comment

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

Overall LGTM. I think this would be a nice change to the library. When I wrote the initial code there was no p384 support outside of ring.
I'm glad we can switch to a pure rust implementation now 🥳

picky/src/key/ec.rs Outdated Show resolved Hide resolved
picky/src/key/ec.rs Outdated Show resolved Hide resolved
Replaced `ring` elliptic curve signature implementations
with RustCrypto

Added a compability test for previous ring implementation
and current rust crypto to ensure the functionality of API
does not change

Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>
Copy link
Contributor

@flavio flavio 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 good to me!

@CBenoit can you please take a look at this PR?

Copy link
Member

@CBenoit CBenoit left a comment

Choose a reason for hiding this comment

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

Thank you @Xynnn007 , I’m very happy with this change.
And thanks to you too @flavio, your review is really appreciated!
This is looking good to me 💯

@CBenoit CBenoit enabled auto-merge (squash) March 4, 2023 04:22
@CBenoit CBenoit merged commit f4be042 into Devolutions:master Mar 4, 2023
@Xynnn007 Xynnn007 deleted the refactor-replace-ring branch March 5, 2023 07:52
@Xynnn007
Copy link
Contributor Author

Xynnn007 commented Mar 6, 2023

Thanks for adopting this! @CBenoit When could this PR be included in the next release?

@CBenoit
Copy link
Member

CBenoit commented Mar 21, 2023

Thanks for adopting this! @CBenoit When could this PR be included in the next release?

I’ll take some time to publish a new version next week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants