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

srp: rebuild library #79

Merged
merged 8 commits into from
Jan 22, 2022
Merged

srp: rebuild library #79

merged 8 commits into from
Jan 22, 2022

Conversation

jbis9051
Copy link
Contributor

@jbis9051 jbis9051 commented Dec 20, 2021

This PR is a complete rewrite of the SRP library. It includes many improvements over the old library at the expense of backwards compatibility.

Improvements include:

  • Improved file and code organization
  • Access to individual SRP computations
  • Consistent sever and client API
  • Simpler API
  • Improved documentation with tests in documentation
  • New tests for compatibility with the RFC
  • Bumps dependencies
  • Timing safe verification comparisons
  • Modernized error handling
  • And much more...

@tarcieri
Copy link
Member

Interesting. Let me know when this is ready for review. I can leave some initial notes.

srp/Cargo.toml Outdated Show resolved Hide resolved
srp/Cargo.toml Outdated Show resolved Hide resolved
srp/tests/rfc5054.rs Outdated Show resolved Hide resolved
@newpavlov
Copy link
Member

@tarcieri
Would it be possible to use crypto-bigint for implementing SRP or is not not sufficiently advanced yet?

@tarcieri
Copy link
Member

tarcieri commented Dec 20, 2021

@newpavlov unfortunately not. The core functionality this needs is modular exponentiation (modpow). We attempted to add that to crypto-bigint but never got it working properly.

I might take a crack at it again soon for the purposes of expanding the functionality of ScalarCore in the elliptic-curve crate. The core algorithm isn't terribly complicated: use the existing mul_wide functionality to produce a wide output, then perform a Montgomery reduction, which requires precomputing -(m^{-1} mod m) mod m, but otherwise provides a relatively simple constant-time formula.

@jbis9051
Copy link
Contributor Author

jbis9051 commented Dec 20, 2021

crypto-bigint looks really interesting! Good luck with getting modular exponentiation to work.

@jbis9051
Copy link
Contributor Author

Looks like we will have to change minimum supported rust version

@tarcieri
Copy link
Member

That's no problem. I might suggest bumping edition = "2021" and setting rust-version = "1.56" in Cargo.toml.

@jbis9051
Copy link
Contributor Author

PR is complete. I will add some more features in a future.

@jbis9051 jbis9051 marked this pull request as ready for review December 20, 2021 23:33
@jbis9051
Copy link
Contributor Author

I'm not sure how to handle the failing GH workflows. It seems they aren't scoped to the proper sub-crate so SRP min rust version is interfering with spake2.

@tarcieri
Copy link
Member

Aah, this is the joy of having one 2021 edition crate in a workspace. It either needs to be split into its own workspace, or we should bump all of the crates to MSRV 1.56.

I can take care of that, if you'd like.

Clippy, on the other hand, is global to the whole workspace and needs an MSRV of the highest MSRV of any crate in the repo.

@jbis9051
Copy link
Contributor Author

jbis9051 commented Dec 20, 2021

@tarcieri

Could we do something like

- run: cd spake2 && cargo test --release

?

It either needs to be split into its own workspace, or we should bump all of the crates to MSRV 1.56.

If not, I'd say bump.

I can take care of that, if you'd like.

Sure. Whatever you think this best.

@tarcieri
Copy link
Member

It already does that:

https://github.com/RustCrypto/PAKEs/blob/master/.github/workflows/spake2.yml#L11-L13

...however you're running into things which are inherently global to a workspace, regardless of the CI config

@newpavlov
Copy link
Member

Yeah, I think the easiest solution would be to simply bump spake2's MSRV to 1.56.

@tarcieri
Copy link
Member

I bumped both spake2 and srp to Rust 2021 edition and did various other minor documentation and clippy fixups in #80.

@jbis9051 if you rebase, you should be good to go now

@jbis9051
Copy link
Contributor Author

\o/

@tarcieri
Copy link
Member

Success!

I’ll try to look through this in the coming weeks, but expect some delays there due to the holidays

@jbis9051
Copy link
Contributor Author

Sounds good and happy holidays!

@jbis9051
Copy link
Contributor Author

Hi. Any update on this?

@tarcieri
Copy link
Member

Will try to review it this weekend

Copy link
Member

@tarcieri tarcieri 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 like an improvement to me.

I say this with the caveat that I'm not particularly familiar with SRP, so I can't say my review was particularly careful or detailed, nor do I have any specific feedback beyond what's already been covered earlier.

I'll give @newpavlov a bit of time to review this, or if he's too busy I'll merge it next weekend.

@tarcieri tarcieri changed the title Rebuild Library srp: rebuild library Jan 16, 2022
@tarcieri tarcieri merged commit 6d96322 into RustCrypto:master Jan 22, 2022
@tarcieri
Copy link
Member

@jbis9051 thank you! I can go ahead and cut a release if you'd like

@jbis9051
Copy link
Contributor Author

@tarcieri Thank you! Sure that would be great.

P.S. This PR (and the last one) closed the following issues: #17, #18

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 this pull request may close these issues.

3 participants