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

docs: ADR to enable 2FA for course team accounts #574

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zainab-amir
Copy link
Contributor

This ADR describes the decision and process of adding a new security step for course team accounts. One decision is highlighted within the doc that needs review. Any possible addition or suggestion is welcome.

Ticket: https://2u-internal.atlassian.net/browse/VAN-942

@codecov
Copy link

codecov bot commented May 17, 2022

Codecov Report

Base: 76.93% // Head: 80.57% // Increases project coverage by +3.63% 🎉

Coverage data is based on head (a1aabc0) compared to base (bcece26).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #574      +/-   ##
==========================================
+ Coverage   76.93%   80.57%   +3.63%     
==========================================
  Files          88       87       -1     
  Lines        1834     1889      +55     
  Branches      492      531      +39     
==========================================
+ Hits         1411     1522     +111     
+ Misses        405      351      -54     
+ Partials       18       16       -2     
Impacted Files Coverage Δ
src/register/UsernameField.jsx 79.16% <0.00%> (-8.34%) ⬇️
src/data/reducers.js 0.00% <0.00%> (ø)
src/data/constants.js 100.00% <0.00%> (ø)
src/register/utils.js 59.09% <0.00%> (ø)
src/login/data/sagas.js 89.47% <0.00%> (ø)
src/login/data/actions.js 100.00% <0.00%> (ø)
src/welcome/data/sagas.js 0.00% <0.00%> (ø)
src/register/data/sagas.js 88.00% <0.00%> (ø)
src/login/data/selectors.js 100.00% <0.00%> (ø)
src/welcome/WelcomePage.jsx 88.05% <0.00%> (ø)
... and 48 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

******************************

- We could reduce session expiration time only for course team users. Currently session expiration time is four weeks and we can reduce this time to two weeks for educator accounts.
- We can also go with the first (2FA) and third (Reset password after a specific time period) option simultaneously.

Choose a reason for hiding this comment

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

I believe that the current best practice is to not reset passwords on a cadence, since that makes people more likely to pick simple passwords. (They should be using a password manager, but we can't force them to.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We enabled HIBP compliance on password reset and registration pages hence they cannot set weak passwords. Do you still think we should not ask them to reset password?

Choose a reason for hiding this comment

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

Strictly speaking, we can't keep people from setting weak passwords -- we can only prevent them from using specific known-to-be-weak passwords. If superman1 is disallowed, they might choose superm@n1 instead, which isn't much of an improvement.

HIBP will definitely help, but I do think that we should avoid doing password resets except in cases of known compromise.

Enhancements to above approach
******************************

- We could reduce session expiration time only for course team users. Currently session expiration time is four weeks and we can reduce this time to two weeks for educator accounts.

Choose a reason for hiding this comment

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

Two weeks doesn't make a big difference compared to four weeks if the course team user's account is compromised somehow. Should it be daily or at least has a configurable period for session expiration time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • We can make it configurable.
  • The only issue I see with making the session expiry time to 1 day would be that course team are normally making changes that might span over a few days and they might lose those edit if they don't save them as draft.

This change can be accommodated within our current login flow. A high level flow of this process is as follow:

- A user belonging to a course team will authenticate themselves like they currently do, either by providing login credentials or by SSO.
- Once they have authenticated successfully they will be sent an automatically generated authentication code to their primary email address.
Copy link

@timmc-edx timmc-edx Jun 13, 2022

Choose a reason for hiding this comment

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

Have you looked into other kinds of 2FA?

Email-based 2FA protects users in the situation where their Open edX password is compromised but their email account remains secure. This will be true for some users, but for others the passwords will unfortunately be the same in the two systems. Password re-use is tremendously common. For those people, email isn't truly a second factor, since they use the same password for two services (from the same device, even).

I think you can look at 2FA a few different ways, although some of these overlap:

  • Whether it protects against a password compromise due to A) reuse/just being weak, B) phishing (can get one TOTP token), or C) on-device malware (can capture full TOTP secret, if stored on same device)
  • Whether the password is reused for the second factor (e.g. securing the email account too)
  • Whether the password is entered on the same device where the second factor is secured (malware or a thief gets both in one go)

From that perspective, here's how I see some of the options:

  • Email token: Vulnerable if password is reused for email service, or if password was compromised via on-device malware, or in phishing. edX password is already equivalent in security to email account access due to reset-password functionality, so this is marginal as 2FA goes. Works OK if attack is based on password reuse but email account is secure.
  • SMS token: Vulnerable to targeted attacks, either social engineering (number porting) or technical (cloning), and to phishing. Probably OK against non-targeted attacks. Requires a cell phone.
  • TOTP: Vulnerable if login to edX also happens on the phone where the TOTP app is installed and malware is the attack method. (Unlikely for primarily-Studio users, much more likely for other sites.) Also vulnerable to phishing. Pretty secure otherwise, and doesn't require a phone—Yubikey can be used for TOTP, and in a pinch a desktop client can be used if the TOTP code is displayed in text under the QR code to allow manual enrollment (but this reduces the security in the malware case.)
  • Various apps like Duo that involve pressing a "confirm" button on the phone rather than typing in a code; probably not any better than TOTP, and require custom integrations. Not really recommended.
  • U2F: Vulnerable only to physical theft of hardware token—very secure. Even prevents phishing. But most people don't have these.

I think the email token is a good first step, but I'd like to see us support TOTP and U2F as the "preferred" methods (plus a "backup codes" feature), with SMS and email only provided as fallbacks when a user can't use the preferred methods. I suspect there's already a library that can do a good amount of this for us. Maybe I'm wrong, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@timmc-edx thank you for these suggestions. We are going to do more discovery and update ADR accordingly.

Copy link

Choose a reason for hiding this comment

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

Please keep in mind the support needs of different 2FA methods during discovery. I could imagine course staff needing help with installing 3rd party apps, with account recovery, with a change in phone access or email address, with university spam filters. Some of these are familiar support flows and some may need new tools/workflows.


This change can be accommodated within our current login flow. A high level flow of this process is as follow:

- A user belonging to a course team will authenticate themselves like they currently do, either by providing login credentials or by SSO.
Copy link

Choose a reason for hiding this comment

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

Is there a definition for the roles that would be appropriate for 2FA, and would that affect the best kind of 2FA for the platform?

As a user/support person, off the top of my head:

  • course staff, administrator, and course data researcher roles have high levels of access to sensitive data/tools
  • course discussion roles are orthogonal to the above, and give some access to discussion data only
  • beta testers do not have access to learner data, just early access to content
  • a couple roles may exist that are affiliated with an organization rather than a course ("universal course researcher")

Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, we are doing 2FA for course teams and I don't think it would have any effect on the best Kinds of 2FA for the platform, we have just updated the document, please have a look. Thankyou

@syedsajjadkazmii syedsajjadkazmii force-pushed the zamir/VAN-942/2fa_for_course_teams_adr branch 2 times, most recently from f7e2555 to 416c010 Compare September 5, 2022 08:05
@syedsajjadkazmii syedsajjadkazmii force-pushed the zamir/VAN-942/2fa_for_course_teams_adr branch from 416c010 to a1aabc0 Compare September 5, 2022 08:24
- Can use same U2F device for multiple sites
- - Needs a device
- Cost
- Support (only chrome supports U2F currently)

Choose a reason for hiding this comment

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

Support is broader than that -- Firefox supports U2F as well. (But agreed that it can't be the only supported method, so probably not something we want to start with.)

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.

None yet

5 participants