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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Group revamp #261

Merged
merged 7 commits into from Feb 25, 2022
Merged

Group revamp #261

merged 7 commits into from Feb 25, 2022

Conversation

daxpedda
Copy link
Contributor

@daxpedda daxpedda commented Jan 5, 2022

The Zeroize implementation and testing got out of hand here. I will improve on that later. See RustCrypto/utils#652, RustCrypto/utils#699 and RustCrypto/utils#700 on progress how we can improve on that.

  • Move de/serializing checks for keys to the KeGroup implementation (X25519 still outstanding).
  • Removed separate Hash associated type in CipherSuite and now the one from OprfGroup is used.
  • Removed P-256 implementation and replaced with generic one over RustCrypto traits.
  • Use ZeroizeOnDrop trait where appropriate.
  • Removed Key type and instead use type provided by KeGroup.
  • Change back permissions for all files to 644 (probably messed up by me because I was using a MacBook).
  • Removed documented experimental warning for P-256 馃帀.
  • Consolidated some CI runs and added more feature combinations to test.
  • Remove generic from SlowHash and move it to the method instead.
  • Updated MSRV to 1.57 because of dependencies on crypto-bigint.
  • Fix DeriveKeyPair implementation using the wrong group for HashToField.

Issues introduced 馃槄:

Fixes #254.
Fixes #256.
Fixes #258.
Fixes #267.

@daxpedda
Copy link
Contributor Author

This is currently broken for a couple of reasons:

  • The new ZeroizeOnDrop derive doesn't work for GenericArray (currently just implementing it by hand).
  • derive_where doesn't support cfg attributes on fields.
  • I didn't adjust tests yet. I will document all the changes I made here in the OP when I have working progress.

Will figure out how to fix/report this upstream.

@daxpedda daxpedda force-pushed the group-revamp branch 2 times, most recently from c4a7f92 to 1d5ba34 Compare February 24, 2022 00:22
@daxpedda
Copy link
Contributor Author

daxpedda commented Feb 24, 2022

@kevinlewi this should be ready to review. I will keep in draft because I'm going to push some more general code improvements, if that's alright.

Copy link
Contributor

@kevinlewi kevinlewi left a comment

Choose a reason for hiding this comment

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

Excellent, this was a big one, thanks so much!!

Should I wait to merge for the general improvements you wanted to add, or go ahead and merge now?

@daxpedda
Copy link
Contributor Author

I can make the general improvements as a follow-up, so go ahead!

@daxpedda daxpedda marked this pull request as ready for review February 24, 2022 22:10
@daxpedda
Copy link
Contributor Author

Pushed the improvements I had here for now, feel free to merge (or tell me to stop 馃槃) anytime.

@kevinlewi kevinlewi merged commit b2f1085 into facebook:main Feb 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants