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

Conversation 8 Commits 18 Files Changed 30

cooperaj commented 2023-08-14 16:11:19 +00:00

Owner

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.

👀 2 ❤️ 1

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

cooperaj commented 2023-08-14 21:33:32 +00:00

Poster

Owner

     

Screenshot 2023-08-14 at 22.32.28.png

52 KiB

Screenshot 2023-08-14 at 22.31.59.png

29 KiB

Screenshot 2023-08-14 at 22.31.03.png

23 KiB

Screenshot 2023-08-14 at 22.30.48.png

45 KiB

Screenshot 2023-08-14 at 22.30.10.png

46 KiB

Screenshot 2023-08-14 at 22.29.59.png

52 KiB

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

ernest commented 2023-08-16 07:44:59 +00:00

Owner

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

cooperaj commented 2023-08-16 08:50:43 +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.

👍 1

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

cooperaj commented 2023-09-08 13:43:30 +00:00

Poster

Owner

Backup codes functionality added.

Backup codes functionality added.   

Screenshot 2023-09-08 at 14.42.14.png

36 KiB

Screenshot 2023-09-08 at 14.41.28.png

113 KiB

Screenshot 2023-09-08 at 14.42.24.png

61 KiB

cooperaj added 1 commit 2023-09-08 13:46:09 +00:00

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

0d081a8675

Ensure rules are in right order

rideranton commented 2023-09-08 23:58:49 +00:00

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

👍 1

cooperaj force-pushed 2fa-implementation from 0d081a8675 to ae35d2a7a8 2023-09-12 14:20:08 +00:00 Compare

melroy89 commented 2023-09-12 21:21:23 +00:00

Owner

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

cooperaj commented 2023-09-13 10:58:51 +00:00

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 referenced this issue from a commit 2023-09-15 09:08:25 +00:00

Implement TOTP 2fa for user accounts (#1001)

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

melroy89 commented 2023-09-26 20:57:18 +00:00

Owner

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.

melroy89 referenced this pull request 2023-09-26 21:00:35 +00:00

Revert "Sync translations (#1166)" #1167

Sign in to join this conversation.

Reviewers

No reviewers

szsz

melroy89

ernest

Labels

Clear labels   

a11y

Accessibility   

ActivityPub

Incoming and/or outgoing federation of content   

admin

Functionality and updates to improve the experience for admins   

API

  

backend

  

bug

Something is not working   

community

  

conflicting

  

contribution welcome

Contributions are very welcome, get started here   

deployment

  

documentation

  

duplicate

This issue or pull request already exists   

enhancement

New feature or changes to existing functionally   

frontend

Styling, layout and other visual issues   

good first issue

Interested in contributing? Get started here.   

help wanted

Need some help   

high priority

  

instance config

  

low priority

  

mobile

  

moderation

  

more infomation needed

It's not possible to proceed without additional information   

needs feedback

Discussion is needed before action is taken.   

pr pending

This issue is either waiting on a pull request or has been approved and waiting deployment to production   

project setup

Questions / concerns relating to getting the project up and running   

question

A user is asking a question about /kbin, not necessarily reporting an issue   

search

  

security

  

translation

  

translations update needed

  

UI/UX

To do with user interface and/or user experience issues   

upstream

Related to an upstream repository, already reported there

No Label

a11y

ActivityPub

admin

API

backend

bug

community

conflicting

contribution welcome

deployment

documentation

duplicate

enhancement

frontend

good first issue

help wanted

high priority

instance config

low priority

mobile

moderation

more infomation needed

needs feedback

pr pending

project setup

question

search

security

translation

translations update needed

UI/UX

upstream

Milestone

Clear milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

No Assignees

4 Participants

Notifications

Subscribe

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.