Skip to content

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.

@daxpedda daxpedda requested a review from kevinlewi February 24, 2022 22:21
@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