Implement TOTP 2fa for user accounts #1001

Merged
cooperaj merged 18 commits from cooperaj/kbin-core:2fa-implementation into develop 2023-09-15 09:08:28 +00:00

Using standard Symfony recommended libraries.

When a user has enabled 2fa on their account via their newly altered "password & 2fa" settings page they will be prompted for that code when logging in. Accounts with 2fa enabled cannot be accessed without entering a correct 2fa code.

TODO

  • Allow a user to remove 2fa from their account
  • Allow an admin to remove 2fa from a users account
  • Enable backup codes functionality
  • Testing testing testing

Users are able to add 2fa to their account, and remove it. Admin's are also able to remove 2fa from a users account using the normal user admin toolbar and can view accounts that have 2fa enabled in the user listing view.

Using standard Symfony recommended libraries. When a user has enabled 2fa on their account via their newly altered "password & 2fa" settings page they will be prompted for that code when logging in. Accounts with 2fa enabled cannot be accessed without entering a correct 2fa code. **TODO** - [x] Allow a user to remove 2fa from their account - [x] Allow an admin to remove 2fa from a users account - [x] Enable backup codes functionality - [ ] Testing testing testing Users are able to add 2fa to their account, and remove it. Admin's are also able to remove 2fa from a users account using the normal user admin toolbar and can view accounts that have 2fa enabled in the user listing view.
cooperaj added the
enhancement
translations update needed
labels 2023-08-14 16:14:13 +00:00
cooperaj changed title from WIP: Implement TOTP 2fa for user accounts to Implement TOTP 2fa for user accounts 2023-08-14 21:27:48 +00:00
Poster
Owner

Screenshot 2023-08-14 at 22.29.59.png
Screenshot 2023-08-14 at 22.30.10.png
Screenshot 2023-08-14 at 22.30.48.png
Screenshot 2023-08-14 at 22.31.03.png
Screenshot 2023-08-14 at 22.31.59.png
Screenshot 2023-08-14 at 22.32.28.png

![Screenshot 2023-08-14 at 22.29.59.png](/attachments/79ea5e3e-5a86-4bed-83fd-dcb65b3ece77) ![Screenshot 2023-08-14 at 22.30.10.png](/attachments/926a5be7-e7da-4722-add4-5cb3bf3924b6) ![Screenshot 2023-08-14 at 22.30.48.png](/attachments/35873045-a537-4a8a-b0b7-f063396595a3) ![Screenshot 2023-08-14 at 22.31.03.png](/attachments/31b517b8-997b-4559-959f-d9c42231a315) ![Screenshot 2023-08-14 at 22.31.59.png](/attachments/44ed671d-c3cb-432e-b435-2b7abf3639ed) ![Screenshot 2023-08-14 at 22.32.28.png](/attachments/ed152da6-4774-4b37-add6-d98c9560ddd4)
cooperaj requested review from ernest 2023-08-14 21:34:17 +00:00
cooperaj requested review from szsz 2023-08-14 21:34:17 +00:00
cooperaj requested review from melroy89 2023-08-14 21:34:33 +00:00

holy moly, this is another big thing.

holy moly, this is another big thing.
ernest changed title from Implement TOTP 2fa for user accounts to WIP: Implement TOTP 2fa for user accounts 2023-08-16 07:56:36 +00:00
Poster
Owner

Only thing missing I think is the backup codes implementation. To reduce "help me, i've lost my phone" email spam to admins it is probably worth having.

Only thing missing I think is the backup codes implementation. To reduce "help me, i've lost my phone" email spam to admins it is probably worth having.
cooperaj force-pushed 2fa-implementation from 384e4d8f67 to 9426fbe6c0 2023-08-16 09:22:54 +00:00 Compare
cooperaj force-pushed 2fa-implementation from b5d77e0ea4 to da8c4196e9 2023-08-16 09:37:32 +00:00 Compare
cooperaj force-pushed 2fa-implementation from daf2ad441c to ab0d34c964 2023-08-16 13:25:21 +00:00 Compare
ernest added the
security
label 2023-08-16 16:01:43 +00:00
cooperaj force-pushed 2fa-implementation from ab0d34c964 to e437793323 2023-09-05 16:01:19 +00:00 Compare
cooperaj force-pushed 2fa-implementation from e437793323 to bfe65c055c 2023-09-08 13:40:58 +00:00 Compare
Poster
Owner

Backup codes functionality added.

Screenshot 2023-09-08 at 14.41.28.png
Screenshot 2023-09-08 at 14.42.14.png
Screenshot 2023-09-08 at 14.42.24.png

Backup codes functionality added. ![Screenshot 2023-09-08 at 14.41.28.png](/attachments/fd893c84-f1f1-45fb-9145-1231c4ae3628) ![Screenshot 2023-09-08 at 14.42.14.png](/attachments/9864a588-5651-49ca-80d1-ce7ac6ae195a) ![Screenshot 2023-09-08 at 14.42.24.png](/attachments/3e144c97-e2a5-495c-9952-240f17af0c52)
cooperaj added 1 commit 2023-09-08 13:46:09 +00:00
Kbin CI/CD pipeline / unit-test (pull_request) Successful in 26s Details
Kbin CI/CD pipeline / fixer-dry-run (pull_request) Successful in 30s Details
Kbin CI/CD pipeline / build (pull_request) Successful in 45s Details
6a902d9abf
Lock file update and cs-fixer fixes.
cooperaj changed title from WIP: Implement TOTP 2fa for user accounts to Implement TOTP 2fa for user accounts 2023-09-08 13:47:49 +00:00
cooperaj added 1 commit 2023-09-08 15:40:24 +00:00
Kbin CI/CD pipeline / unit-test (pull_request) Successful in 21s Details
Kbin CI/CD pipeline / fixer-dry-run (pull_request) Successful in 29s Details
Kbin CI/CD pipeline / build (pull_request) Successful in 40s Details
0d081a8675
Ensure rules are in right order

I've double checked the API oauth flow using this branch on my dev machine, and it seems to work fine when retrieving a token for an account with 2FA enabled

I've double checked the API oauth flow using this branch on my dev machine, and it seems to work fine when retrieving a token for an account with 2FA enabled
cooperaj force-pushed 2fa-implementation from 0d081a8675 to ae35d2a7a8 2023-09-12 14:20:08 +00:00 Compare

When reading RFC 6238:

We also RECOMMEND storing the keys securely in the validation system,
and, more specifically, encrypting them using tamper-resistant
hardware encryption and exposing them only when required: for
example, the key is decrypted when needed to verify an OTP value, and
re-encrypted immediately to limit exposure in the RAM to a short
period of time.

https://datatracker.ietf.org/doc/html/rfc6238#section-5

When reading RFC 6238: > We also RECOMMEND storing the keys securely in the validation system, > and, more specifically, encrypting them using tamper-resistant > hardware encryption and exposing them only when required: for > example, the key is decrypted when needed to verify an OTP value, and > re-encrypted immediately to limit exposure in the RAM to a short > period of time. > https://datatracker.ietf.org/doc/html/rfc6238#section-5
Poster
Owner

Yeah, I'm pretty certain thats a recommendation that no one actually follows as it's incredibly hard to get right/correct. PHP's nature as a request based runtime means it's mostly immune to memory based leakage and since you wouldn't want to slow things down too much pulling the decryption keys on every request you'd want to cache those somehow, which means any implementation would just be an exercise in obfuscation.

If we were going to start encryption of credentials in the database we'd also want to take a look at the private keys. So mostly the recommendation for this sort of thing is going to be to have an encrypted filesystem to cover your "at rest" issues.

Edit. Have created #1123 to track this possible effort.

Yeah, I'm pretty certain thats a recommendation that no one actually follows as it's incredibly hard to get right/correct. PHP's nature as a request based runtime means it's mostly immune to memory based leakage and since you wouldn't want to slow things down too much pulling the decryption keys on every request you'd want to cache those somehow, which means any implementation would just be an exercise in obfuscation. If we were going to start encryption of credentials in the database we'd also want to take a look at the private keys. So mostly the recommendation for this sort of thing is going to be to have an encrypted filesystem to cover your "at rest" issues. Edit. Have created #1123 to track this possible effort.
cooperaj force-pushed 2fa-implementation from ae35d2a7a8 to 525a123990 2023-09-15 08:43:35 +00:00 Compare
cooperaj force-pushed 2fa-implementation from 525a123990 to 3c22611a66 2023-09-15 08:51:34 +00:00 Compare
ernest approved these changes 2023-09-15 09:06:31 +00:00
cooperaj merged commit fa87239ea3 into develop 2023-09-15 09:08:28 +00:00
cooperaj deleted branch 2fa-implementation 2023-09-15 09:08:29 +00:00

Causing regression on translation files, since the translation strings were added to Webplate.

Causing regression on translation files, since the translation strings were added to Webplate.
Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: Kbin/kbin-core#1001
There is no content yet.