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

2FA #7003

Closed
Bonapara opened this issue Sep 12, 2024 · 39 comments · Fixed by #9634
Closed

2FA #7003

Bonapara opened this issue Sep 12, 2024 · 39 comments · Fixed by #9634
Assignees

Comments

@Bonapara
Copy link
Member

Bonapara commented Sep 12, 2024

Desired behavior

We want to introduce 2FA to secure user sign-ins.

1. No 2FA configured

image

2. Configure your 2FA

When clicking on the card, you will be brought to the 2FA configuration page

image

image

Then save

3. 2FA configured

image

4. Deactivate 2FA or re-configure

image

Sign-in

image

Figma

Settings

https://www.figma.com/design/xt8O9mFeLl46C5InWwoMrN/Twenty?node-id=34845-125317&node-type=frame&t=emUxzrvX6zNaHyLZ-11

Onboarding

https://www.figma.com/design/xt8O9mFeLl46C5InWwoMrN/Twenty?node-id=39767-36921&node-type=frame&t=2jy2Es54Z3i9hDfJ-11

@Bonapara Bonapara added scope: back+front Issues requiring full-stack knowledge size: long prio: med labels Sep 12, 2024
@ehconitin
Copy link
Contributor

ehconitin commented Sep 13, 2024

@Bonapara can I work on this? I think I might need more time to get this right as this would need backend changes, but I would like to give it a try.

@sid0-0
Copy link
Contributor

sid0-0 commented Sep 13, 2024

@Bonapara
Will this will also require design and implementation for 2 FA verification on login?

@hozza
Copy link
Contributor

hozza commented Sep 30, 2024

I'm starting on this. TOTP incoming!

@Bonapara
Copy link
Member Author

Thanks @hozza!

@FelixMalfait
Copy link
Member

/oss.gg 3000

Copy link

oss-gg bot commented Oct 13, 2024

Thanks for opening an issue! It's live on oss.gg!

@DeepaPrasanna
Copy link
Contributor

/assign

Copy link

oss-gg bot commented Oct 13, 2024

Assigned to @DeepaPrasanna! Please open a draft PR linking this issue within 48h ⚠️ If we can't detect a PR from you linking this issue in 48h, you'll be unassigned automatically 🕹️ Excited to have you ship this 🚀

@DeepaPrasanna DeepaPrasanna removed their assignment Oct 13, 2024
@sudarshana133
Copy link

/assign

Copy link

oss-gg bot commented Oct 13, 2024

Assigned to @sudarshana133! Please open a draft PR linking this issue within 48h ⚠️ If we can't detect a PR from you linking this issue in 48h, you'll be unassigned automatically 🕹️ Excited to have you ship this 🚀

@sudarshana133
Copy link

/unassign

Copy link

oss-gg bot commented Oct 13, 2024

Issue unassigned.

@dshinde96
Copy link

/assign

Copy link

oss-gg bot commented Oct 13, 2024

Assigned to @dshinde96! Please open a draft PR linking this issue within 48h ⚠️ If we can't detect a PR from you linking this issue in 48h, you'll be unassigned automatically 🕹️ Excited to have you ship this 🚀

@Pushpender1122
Copy link
Contributor

/assign

Copy link

oss-gg bot commented Oct 15, 2024

This issue is already assigned to another person. Please find more issues here.

@Anky9972
Copy link

/assign

Copy link

oss-gg bot commented Oct 16, 2024

@dshinde96, Just a little reminder: Please open a draft PR linking this issue within 12 hours. If we can't detect a PR in 12h, you will be unassigned automatically.

@BOHEUS
Copy link
Contributor

BOHEUS commented Nov 22, 2024

@Bonapara what do you think about this project? Will it be suitable here? https://www.better-auth.com/

@FelixMalfait
Copy link
Member

FelixMalfait commented Nov 23, 2024

@BOHEUS we would most likely use https://www.passportjs.org/packages/passport-totp/ since we use Passport for other auth concerns already

@DeepaPrasanna
Copy link
Contributor

Is this open for contribution?

@FelixMalfait
Copy link
Member

@DeepaPrasanna yes contribution most welcome!!! passport-totp is the right one I think, I edited my above message.
See also: https://github.com/guilhermerodz/input-otp

Thanks a lot

@DeepaPrasanna
Copy link
Contributor

Thank you! I would love to work on it

@FelixMalfait
Copy link
Member

Thanks a lot @DeepaPrasanna! Please ping me if you have any question (here on on Discord)

@sid0-0
Copy link
Contributor

sid0-0 commented Jan 10, 2025

Hey this has been dormant for a while now, I would like to work on it if it's cool (and not deprioritized).

I had some questions then:

  1. For 2FA we will need to store a totp secret in the user db. I'm not sure what changes will need to be made (haven't really worked with Graphql & nest before). If you could provide some sort of a reference PR for adding a new field to users db and APIs to access them, that would be great.
  2. There are 2 ways to generate totp and store
    1. (low effort, less secure) Generate a secret on UI, once user confirms/validates, we make an api call to store it in db.
    2. (more effort/complex, slightly more secure) Call backend to generate and store the secret in users db in some sort of temporary state (can be a different db column or the same column with prefixed value), send it to UI to generate QR code, then once user confirms the code, we can mark it as validate/move to permanent column/remove the prefix. We can also store it permanently on generation and then run a cron to remove it after some time and once user confirms, cancel the cron but this seems like an overkill to me.

@FelixMalfait
Copy link
Member

Hey @sid0-0, thanks! I believe @nicolasrouanne might work on it (discussed with him via Discord, not sure yet)

We have an appTokens table we use to store similar things (like the invitation code when you're added to a workspace).
But we do need to create a field to store 2FA enablement/config yes.

2FA settings should be stored in the userWorkspace table imo.

I'm not an expert in 2FA but I believe that the code is computed independently by the app without server connection so you don't need to generate/store totp in DB. Hope the passport package takes care of that!

@nicolasrouanne
Copy link
Contributor

Hey @sid0-0 and @FelixMalfait, yes I will look into it early next week, but I'm open for suggestions and joint work if you want @sid0-0 .

I'll draft out the general engineering directions in this issue before creating a PR; I will start from the backend obviously.

@nicolasrouanne
Copy link
Contributor

nicolasrouanne commented Jan 10, 2025

Here are my thoughts about the general direction

I'm thinking of using otplib package to generate QR codes, TOTP secret and validating tokens.
There are 2 main passport strategies available, and referenced in the passport website, passport-totp (GitHub) and passport-2fa-totp (GitHub) but neither have been maintained in the last 10 years.

Note

2FA is tightly coupled with storing "authorized" client sessions, and not asking for 2FA upon every login attempt. This would mean storing and displaying "authorized" clients (e.g. a Webbrowser fingerprint). Although displaying these clients is optional.
Maybe we can start with asking for the OTP on each login attempt? If it's a too deceptive UX, we'll add those authorized clients.

We need to store:

  • the secret used to generate the OTP, one secret per user using 2FA, probably in the users table, could be named totpSecret for instance. It could be considered like a password which is stored in this table. It can't be hashed because it has to be available to check against the OTP code upon authentication.
  • wether the user has 2FA enabled; although this can already be inferred by the fact that a totpSecret exists. Hence, I suggest not storing it and relying on !!user.totpSecret to know if a user has 2FA enabled.

A totpSecret is this kind of thing:

// manually created
const secret = 'KVKFKRCPNZQUYMLXOVYDSQKJKZDTSRLD';

// it can also be generated by `otplib`
import { authenticator } from 'otplib';
const secret = authenticator.generateSecret();

I think storing totpSecret makes more sense in the users table than in the userWorkspaces table, since a user can have many workspaces, but 2FA is enabled for the user, not for the user-workspace tuple. It is also very different from appTokens which are an access token / refresh token pair in the OAuth2 space ; one to many relation with a user, can be revoked, etc.

We would then probably need:

  • a TOTPAuthStrategy and a TOTPAuthGuard
  • routes and controllers to display the QR code, to check a OTP code

Will elaborate later. What do you think @FelixMalfait?

Sources:

@FelixMalfait
Copy link
Member

I'm thinking of using otplib package to generate QR codes, TOTP secret and validating tokens.

Good point, I agree, the lib seems quite light https://github.com/jaredhanson/passport-totp/blob/master/lib/strategy.js so even if we wanted to use it, copy/pasting would make more sense for a lib that hasn't been used for more than 8 years.

I think storing totpSecret makes more sense in the users table than in the userWorkspaces table, since a user can have many workspaces, but 2FA is enabled for the user, not for the user-workspace tuple

I get your point but this is really a tradeoff between simplicity and enterprise requirements. With SSO and subdomains we went a path where we are tending to isolate concerns between workspaces. So you could login to a workspace with Google and then an other workspace could require you to login via SSO only. We used to log you in automatically in the "workspace switcher" but now we don't anymore and you have to re-login separately for each domain.
One advantage of putting 2FA at a userWorkspace level is that we could allow the workspace admins to reset/disable your 2FA if you lost your phone, while we couldn't if we put it at the user level.

Maybe we can start with asking for the OTP on each login attempt

Yes I think it's the right behavior. We have quite a long TTL for the Refresh Token so that shouldn't be a problem.
Would be very nice one day to have fingerprint/device logs but probably an even bigger task than 2FA 😅

the secret used to generate the OTP, one secret per user using 2FA, probably in the users table, could be named totpSecret for instance

What about doing something similar to the generateAppSecret function?
The token could be the hash of "{userId} + {APP_SECRET} + TOTP" ; that way we have a unique secret hash for each user but don't need to store it anywhere
In that case we would need to store is2FAEnabled yes (or something like that)

@sid0-0
Copy link
Contributor

sid0-0 commented Jan 12, 2025

Hey @nicolasrouanne

Would love to work on it.

I was taking an approach very similar to as you described. Here is a sample PR in my fork with the changes for reference.

The biggest difference is that in this I am generating a secret and URI on UI itself as I don't see any real benefit to generating it on server, reason being server will need to share a QR image or the secret itself for user to scan anyway. Also did not want to add a library for 1 simple random generation function (if Passport can generate one, no reason to not use it). QR generation and scanning is tested working. Validation should be with some library on backend anyway though.

I was mainly blocked on adding new APIs and mutation requests and I have altered table in local but not sure how the migration works in prod.

Also adding OTP verification on login can be a subtask I believe, since this is a feature with multiple potential use cases, so I haven't done anything on that.

@nicolasrouanne
Copy link
Contributor

nicolasrouanne commented Jan 15, 2025

@FelixMalfait Thanks a lot for the detailed feedback and guidance.
@sid0-0 I'm going to open a draft PR with migrations and checking the OTP upon login, some thoughts:

  • I think checking OTP upon login is tightly coupled to 2FA, otherwise users would be confused and think it's broken
  • I have not yet decided where secrets will be generated, I'll go fore more straightforward solution

Where to store the setting?

I'm still confused about storing the setting in the user_workspace table, which is a joint table. And I don't really understand why storing it in the user table would be in contradiction with forcing the SSO...
It feels really weird to me that the TOTP isn't tied to the password. The user - user_workspace relation is N-N, so a user could in theory have multiple TOTP while having one password.

However you know the project architecture, much better than I do, so I will try and implement it this way

Computing the totpSecret

What about doing something similar to the generateAppSecret function?

I was initially enthusiastic about this, and started with:

generateUserSecret(type: UserTokenType, workspaceId: string, userId: string);

However while testing it, I figured out the totpSecret can't be "static". What happens if the user's 2FA device is stolen or the 2FA secret is compromised? It has to be revokable by the user, like a password, so we need to store "something". We could store a seed that we generate a sha from, but we might as well store the secret.

We probably want something like this behavior:

  • see the secret and the QR code
  • attach device
  • detach device
  • reattach with another secret/QR code

Image

Login flow

About the login flow, I searched about the "correct" way to implement 2FA without being able to replay the login attempt:

  • user submits email/password
  • backend checks if email/password are valid, and waits for a OTP code
    -> how do we check email/password validity, when submitting the OTP code?

A solution could be to send a temporary 2fa_token after a successful email/password submit, but awaiting second factor. That 2fa_token would be sent along the OTP and exchanged for a proper access_token. However, I couldn't find any standardized way to do that.

So I looked what other OSS projects where doing and I offer to do it the Cal.com way:

@nicolasrouanne nicolasrouanne mentioned this issue Jan 15, 2025
1 task
@FelixMalfait
Copy link
Member

I'm still confused about storing the setting in the user_workspace table, which is a joint table. And I don't really understand why storing it in the user table would be in contradiction with forcing the SSO...
It feels really weird to me that the TOTP isn't tied to the password. The user - user_workspace relation is N-N, so a user could in theory have multiple TOTP while having one password.

I get your point. If we were to rebuild the architecture today we might store password at a userWorkspace level yes. One argument against it asking for a password is a nice protection to access "workspace discovery" (i.e. redirect you to the right workspace if you have one or maybe show a screen with the list of workspaces you're part of). If we were to put password at a userWorkspace level then it would probably be impossible to redirect a user to his workspace if he doesn't have the url (or we could but then maybe it's a higher security risk we're introducing?).

The rational for putting things at a userWorkspace level is to give freedom to the workspace admin to affect those parameter. For example in Google Workspace an admin can reset a password, disable the 2FA etc. That way if someone in our team has lost their phone we don't have to contact Google's support but can act independently. If that setting is managed at a user-level then a workspace admin shouldn't be allowed to touch it.

A workspace admin cannot reset a user's password but this is less of an issue because this is the first step (2FA only comes after) and therefore can be performed without being logged in / independently.

Computing the totpSecret

Agree with your point. You could almost argue that it should be a table for all 2FA methods and only one defined as "active". But I'm afraid this will increase the size of your PR which is probably already big. If we were to go with a table we could keep the same generateSecret method but use the id of the 2FA method instead of the id of the user.

A solution could be to send a temporary 2fa_token after a successful email/password submit, but awaiting second factor. That 2fa_token would be sent along the OTP and exchanged for a proper access_token. However, I couldn't find any standardized way to do that.

What you describe here seems very similar to the loginToken concept we have. We already try to unify all flows with a loginToken. When you come from Google or Microsoft or Password, we first issue a short-lived loginToken and then only you can exchange that against an access+refreshToken pair

@nicolasrouanne
Copy link
Contributor

nicolasrouanne commented Jan 15, 2025

OK, it seems a lot clearer now, thanks. Some key takeaways (trying to keep it short):

  • will go for the loginToken flow: user + password | SSO -> loginToken -> if twoFactorEnabled OTP input then loginToken + OTP sent to verify
  • let's save the totpSecret OR a table with 2FA methods - tell me which one you prefer, I don't care - in the user-workspace realm. If we go for the table, it would be something like twoFactorDevices (open for naming suggestions)
    • userWorkspaces (ManyToOne)
    • active (boolean) or could be replaced by a deactivated_at timestamp

But I don't see much more in that table.

@FelixMalfait
Copy link
Member

great points @nicolasrouanne! twoFactorMethods ? And maybe deletedAt instead of introducing a new deactivatedAt?

DeepaPrasanna pushed a commit to DeepaPrasanna/twenty that referenced this issue Jan 27, 2025
# Description
Closes twentyhq#7003 
Implements 2FA with TOTP. 

>[!WARNING]
> This is a draft PR, with only partial changes, made as a mean of
discussion about twentyhq#7003 (it's easier to reason about real code)

## Behaviour
- a `totpSecret` is stored for each user
- use [`otplib`](https://github.com/yeojz/otplib/tree/master) to create
a QR code and to validate an `otp` against an `totpSecret` (great [demo
website](https://otplib.yeojz.dev/) by `otplib`)
- OTP is asked upon each login attempt

## Source
Inspired by:
- [RFC 6238](https://datatracker.ietf.org/doc/html/rfc6238)
- Cal.com's implementation of 2FA, namely
- [raising a
401](https://github.com/calcom/cal.com/blob/c21ba636d2bec4ed55775f0b058f70fdc371c410/packages/features/auth/lib/next-auth-options.ts#L188-L190)
when missing OTP and 2FA is enabled, with a [specific error
code](https://github.com/calcom/cal.com/blob/c21ba636d2bec4ed55775f0b058f70fdc371c410/packages/features/auth/lib/ErrorCode.ts#L9)
- [catching the
401](https://github.com/calcom/cal.com/blob/c21ba636d2bec4ed55775f0b058f70fdc371c410/apps/web/modules/auth/login-view.tsx#L160)
in the frontend and
[displaying](https://github.com/calcom/cal.com/blob/c21ba636d2bec4ed55775f0b058f70fdc371c410/apps/web/modules/auth/login-view.tsx#L276)
the OTP input

## Remaining
- [ ] encrypt `totpSecret` at rest using a symetric algorithm

---------

Co-authored-by: Félix Malfait <[email protected]>
Co-authored-by: Félix Malfait <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.