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: alternate implementation, based on @brndnmtthws changes #81

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Leandros
Copy link

@Leandros Leandros commented Jan 4, 2022

This is the continuation of #27 #28 and #29 from @brndnmtthws.

All changes have been rebased on master.

Leandros and others added 4 commits January 4, 2022 15:34
This implementation removes the use of unsigned integers (which is not
anywhere in the spec) and a bunch of un-specified mod operations.
BREAKING CHANGE: Replaces all BigUInt with BigInt and removes the modulo N
from each operation.

Based on @brndnmtthws changes.
Refs: RustCrypto#27 RustCrypto#28 RustCrypto#29
@tarcieri
Copy link
Member

tarcieri commented Jan 5, 2022

Thanks! I'll try to review this soon.

@jbis9051
Copy link
Contributor

jbis9051 commented Jan 6, 2022

  1. Could this be integrated into srp: rebuild library #79 ?

There's nothing in the spec that says you need to use unsigned integers, or that you need to mod n every operation, so I just removed that, and then everything was okay. - #29

As for the mod n, I'm pretty sure this is modular reduction, which is a mathematical shortcut (from what I understand). It's not needed however it speeds up operations, which is why removing it didn't cause issues, just likely slowed down the computations. Other SRP libraries seem to do the same thing, see 1Password/srp#6.

  1. If we want to change the proof to be RFC2945 complaint, we may want to create multiple proofs for maximum compatibility. For example, TSSRP6a and nimbus-srp both use the incorrect (current) proof mechanism.

@Leandros
Copy link
Author

Leandros commented Jan 6, 2022

@jbis9051:

  1. Sure, this can be integrated into srp: rebuild library #79. I can contribute a benchmark suite for the client implementation as well.

  2. I've got no big opinion on whether mod N makes sense or not. I've got to say this worked and that's what I was most concerned about.

  3. I'd definitely want to keep the proof to be RFC2945 compliant, for compliancy with other implementations. Further, I could contribute a client implementation written in Typescript which is fully compliant with this branch of SRP.

@jbis9051
Copy link
Contributor

jbis9051 commented Jan 6, 2022

@Leandros

  1. I think the H(A, B, K) scheme comes from the SRPv6 whitepaper http://srp.stanford.edu/srp6.ps

image

Therefore, I think it's reasonable for this library to support both implementations. In #79 this would be as simple as adding two methods for computing m1 in https://github.com/jbis9051/PAKEs/blob/master/srp/src/utils.rs.

@Leandros
Copy link
Author

Leandros commented Jan 7, 2022

Yes, makes sense. I'll see if I can integrate the proof into #79.

@tarcieri
Copy link
Member

FYI, #79 has been merged, so this needs to be rebased

@tarcieri tarcieri changed the title Alternate implementation, based on @brndnmtthws changes srp: alternate implementation, based on @brndnmtthws changes Jan 22, 2022
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.

4 participants