Skip to content

[WJ-1002] Add password and TOTP support #1048

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

Merged
merged 46 commits into from
Oct 27, 2022
Merged

[WJ-1002] Add password and TOTP support #1048

merged 46 commits into from
Oct 27, 2022

Conversation

emmiegit
Copy link
Member

@emmiegit emmiegit commented Jun 20, 2022

This adds the cryptographic basics for password verification (hashing via argon2), multi-factor authentication (MFA) using temporary one-time passwords (TOTP), and basic methods for setting each of those values. We will eventually need to encode the MFA secret in a QR code but that's out of scope at the moment.

The addition of cryptographic operations here provides a foundation for later authentication and related changes that will come as the account system develops.

The new services added are:

  • PasswordService -- wraps the argon2 crate by providing password hashing methods
  • MfaService -- validates TOTP and recovery codes to supplement password auth, if enabled
  • AuthenticationService -- calls the above services to perform high-level authentication operations

I have tried to be careful with my implementations, ensuring that I source random values from a CSPRNG, that any comparison operations are constant-time to avoid timing attacks, that failed authentications result in a constant time sleep to avoid brute-forcing, and that passwords are appropriately hashed before being stored in the database. (Relevant note there, the TOTP secret cannot be encrypted, as its exact value is needed on the server end. I did some research on this before and it seems the typical approach is to just store it as-is, like the password's salt.)

@emmiegit emmiegit changed the title [WJ-1002] Add TOTP support [WJ-1002] Add password and TOTP support Oct 27, 2022
@emmiegit emmiegit self-assigned this Oct 27, 2022
@emmiegit emmiegit marked this pull request as ready for review October 27, 2022 02:04
Not sure how this was missing it or it wasn't causing issues.
@emmiegit emmiegit requested a review from danieltharp as a code owner October 27, 2022 02:14
@codecov
Copy link

codecov bot commented Oct 27, 2022

Codecov Report

Merging #1048 (33caf9e) into develop (df324dd) will decrease coverage by 46.60%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #1048       +/-   ##
===========================================
- Coverage    48.84%   2.24%   -46.61%     
===========================================
  Files          296      96      -200     
  Lines         8985    3704     -5281     
===========================================
- Hits          4389      83     -4306     
+ Misses        4596    3621      -975     
Flag Coverage Δ
deepwell 2.24% <0.00%> (-0.13%) ⬇️
ftml ?
Impacted Files Coverage Δ
deepwell/src/api/internal.rs 0.00% <0.00%> (ø)
deepwell/src/methods/auth.rs 0.00% <0.00%> (ø)
deepwell/src/methods/mod.rs 0.00% <ø> (ø)
deepwell/src/services/authentication/service.rs 0.00% <0.00%> (ø)
deepwell/src/services/error.rs 0.00% <0.00%> (ø)
deepwell/src/services/mfa/service.rs 0.00% <0.00%> (ø)
deepwell/src/services/mfa/structs.rs 0.00% <0.00%> (ø)
deepwell/src/services/mod.rs 0.00% <ø> (ø)
deepwell/src/services/password.rs 0.00% <0.00%> (ø)
deepwell/src/services/user/service.rs 0.00% <0.00%> (ø)
... and 208 more

@emmiegit
Copy link
Member Author

Haven't gotten a response from TimDumol/rust-otp#4. If enough time passes without interaction from the maintainer, I'm wondering if we should fork it and maintain it ourselves.

@emmiegit
Copy link
Member Author

thanks @Yossipossi1

@emmiegit emmiegit merged commit 69a8fcd into develop Oct 27, 2022
@emmiegit emmiegit deleted the WJ-1002-totp branch October 27, 2022 18:41
@v4dkou v4dkou mentioned this pull request Nov 15, 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.

2 participants