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

Allauth: 2FA #11524

Merged
merged 5 commits into from
Aug 22, 2024
Merged

Allauth: 2FA #11524

merged 5 commits into from
Aug 22, 2024

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Aug 7, 2024

  • allauth saves the seed that is used to generate the recovery codes in plain text, but it offers a way to encryp it. I thought django provided an interface to encrypt things with the secrey key, but they only provide that for signing, but encrypting it using cryptography shouldn't be hard. But also, do we want to encrypt the seed? Encrypting the seed will provide protection If there is a breach, and users are using a weak password with 2FA enabled.
  • If we want to send users emails about changes in 2FA like
Hello,

You are receiving this mail because the following change was made to your account:

A new set of Two-Factor Authentication recovery codes has been generated.

If you do not recognize this change then please take proper security precautions immediately. The change to your account originates from:

- IP address: ...
- Browser: ...
- Date: ..

Keep documenting,
Read the Docs

We need #11344

ref #3523

@humitos
Copy link
Member

humitos commented Aug 8, 2024

allauth saves the seed that is used to generate the recovery codes in plain text, but it offers a way to encryp it

I wouldn't jump into this field yet. At least, without reviewing all the conversations we've had in the past and trying to find a generic/global solution for encrypting some fields into the database.

Actually, it seems we encrypt SSHKey.private_key (#4357) using an external dependency, so we should probably re-use that work if we are going in this direction.

@stsewd
Copy link
Member Author

stsewd commented Aug 8, 2024

I wouldn't jump into this field yet. At least, without reviewing all the conversations we've had in the past and trying to find a generic/global solution for encrypting some fields into the database.

Don't think we need to solve all the issues mentioned there or overthink this. In any case, looks like we already have a solution in place. But, that is to declare model fields as encrypted, here we just have access to the field value, but looks like we can re-use that lib https://gitlab.com/lansharkconsulting/django/django-encrypted-model-fields/-/blob/3a68fa64428d9e64bc298d9c9c9ce21d54d4263d/encrypted_model_fields/fields.py#L48-56.

@humitos
Copy link
Member

humitos commented Aug 9, 2024

Don't think we need to solve all the issues mentioned there or overthink this

Oh, yeah, no. I'm not saying we have to solve all those issues, no. I'm saying that we have talked about this in different opportunities and there is really good information about a different proposals there (with pros/cons) that we should consider here before implementing a new different approach for something that we've already discussed and analyzed in the past. We can re-use all those discussions here.

@stsewd
Copy link
Member Author

stsewd commented Aug 21, 2024

We decided to move on without encrypting the seed for now. And to ship this without the templates nor exposing it to users for now. We will expose this to users once we have the templates ready (readthedocs/ext-theme#443).

The only question left if about email notifications (#11344), but we can start discussing that once we have shipped this feature.

Another thing I guess is, we aren't exposing this in the UI yet, but anyone with the link will be able to use it, are we okay with that or should we enforce access with a feature flag?

@stsewd stsewd marked this pull request as ready for review August 21, 2024 17:08
@stsewd stsewd requested a review from a team as a code owner August 21, 2024 17:08
@stsewd stsewd requested a review from humitos August 21, 2024 17:08
Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

We should create a few issues for follow up required work:

  • Work on templates related to MFA
  • Make sure our Site objects are correct and they says Read the Docs Community and Read the Docs for Business
  • Add the UI elements to users can setup MFA

@stsewd stsewd merged commit f808dcd into main Aug 22, 2024
8 checks passed
@stsewd stsewd deleted the mfa branch August 22, 2024 16:12
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