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

Add support for SRP Login #638

Merged
merged 4 commits into from
Oct 29, 2024
Merged

Conversation

abiligiri
Copy link
Contributor

  • Switch to use https://github.com/adam-fowler/swift-srp with some modifications that are local
    • Pad g value to equal size of N while calculating clientProof
  • Use SHA256(plain-text-password) while computing key using PBKDF2
  • Added a unit test with some sample values

Fixes #630

- Switch to use https://github.com/adam-fowler/swift-srp with some modifications
  that are local
  - Pad g value to equal size of N while calculating clientProof
- Use SHA256(plain-text-password) while computing key using PBKDF2
- Added a unit test with some sample values
@fiznool
Copy link

fiznool commented Oct 28, 2024

Personally, I like the approach of using a well-tested library for SRP, and the upstream library seems to provide this. So I would be in favour of this implementation vs #639

If I can make a suggestion, forking adam-fowler/swift-srp and publishing as a separate package with the necessary changes would be my preference for the following reasons:

  • any changes to the upstream code can be more easily incorporated into a fork
  • the shared package can be consumed by the Xcodes app and cli

@abiligiri
Copy link
Contributor Author

Note that both options use a library for SRP. This uses https://github.com/adam-fowler/swift-srp/blob/main/Sources/SRP/client.swift and #639 fixes the srp code that was already in repo most likely from https://github.com/Bouke/SRP. I see that Adam Fowler implementation is still active. Maybe that is another reason to use it.

@MattKiazyk I was able to download and install Xcode 16.1.0. Not sure what you mean by download doesn't work.

@abiligiri
Copy link
Contributor Author

May I also request merging this for the release #632

- Use from https://github.com/abiligiri/swift-srp, version 1.1.0
  This is based on latest from upstream with changes required
- Remove local copy of swift-srp
@abiligiri
Copy link
Contributor Author

I cleaned this up a bit. Using patched swift-srp from https://github.com/abiligiri/swift-srp and removed local copy. I have also opened MR for upstream adam-fowler/swift-srp#13

@abiligiri abiligiri mentioned this pull request Oct 28, 2024
@MattKiazyk
Copy link
Contributor

@abiligiri hope you don't mind I clean up some extra prints that were in there as well as moved the swift-srp dependency to an xcodes fork. I copied your changes from your branch into main on our fork.

@MattKiazyk MattKiazyk merged commit 29bf770 into XcodesOrg:matt/SRPLogin Oct 29, 2024
2 checks passed
@MattKiazyk MattKiazyk changed the title SRP Login works now Add support for SRP Login Oct 29, 2024
@MattKiazyk MattKiazyk added the bugfix Fixes a bug label Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants