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

Adds TOTP MFA for admins. #12363

Merged
merged 16 commits into from
Oct 24, 2021
Merged

Adds TOTP MFA for admins. #12363

merged 16 commits into from
Oct 24, 2021

Conversation

adamsong
Copy link
Contributor

@adamsong adamsong commented Sep 23, 2021

Document the changes in your pull request

This prevents a client from associating with their admin datum until they authenticate with 2FA. The user must provide a one time password from an app like Google Authenticator (Other apps will work, this is just a popular one), at which point they will be authenticated and readminned. The server will save a successful CID/IP pair for up to 30 days if the user so desires, so they don't have to re-enter the 2FA login every round.
In the event of a database failure, the last successful connection will be used.
If a user is not enrolled in 2FA when they try to readmin, they will be presented with a QR code, the raw TOTP seed, and a backup code. Most apps will be able to scan the QR code to get all the necessary information to generate 2FA codes, and the backup can be used in the event the 2FA device is lost. The server will ask for a code before saving these, to verify that the user has setup 2FA properly.
The permissions panel now requires a 2FA code to access, and can reset 2FA logins for any admin.

Images

Setup window
image
Google Authenticator after scanning the code
image
Entering the code
image
Save confirmation
image

Requires rust_g update with the hash feature enabled.

@adamsong adamsong requested a review from a team September 23, 2021 21:04
@adamsong adamsong marked this pull request as draft September 23, 2021 21:04
@github-actions github-actions bot added Admin This PR affects administrators Config Config files need to be changed on the host for this to work SQL This PR has SQL code within, likely database related labels Sep 23, 2021
@adamsong
Copy link
Contributor Author

WHY SQL WHY

@adamsong adamsong marked this pull request as ready for review September 23, 2021 23:14
@JamieD1 JamieD1 added the On hold - HD Should not be merged without HD Approval label Sep 23, 2021
@JamieD1
Copy link
Contributor

JamieD1 commented Sep 23, 2021

From Ash:

  1. Can we not use an existing standard for mfa, like whatever authenticator apps use? I don't think the maths behind these is super complex and it means we don't need to rely on multiple services communicating

  2. Is the session preserved for longer than a round? It probably should be or else it's going to be a pain on the admins

@adamsong
Copy link
Contributor Author

Can we not use an existing standard for mfa, like whatever authenticator apps use? I don't think the maths behind these is super complex and it means we don't need to rely on multiple services communicating

Then it becomes a pain when someone inevitably loses their 2FA device

Is the session preserved for longer than a round? It probably should be or else it's going to be a pain on the admins

Session is based on id + cid combo, if you have logged on before from the same computer/ip then you don't have to do 2fa

@JamieD1
Copy link
Contributor

JamieD1 commented Sep 23, 2021

  1. A solution that doesn't use yogbot is portable to other servers. So anything that uses the multifactor apps people already use would be great for portability

@adamsong
Copy link
Contributor Author

adamsong commented Sep 23, 2021

A solution that doesn't use yogbot is portable to other servers. So anything that uses the multifactor apps people already use would be great for portability

Other servers have done this already, but you would end up resetting a lot of people's 2FA when they lose their device. This is already decently portable as well, as it passes the MFA off with a webhook, and receives confirmation the same way, so another server wanting a different backend could still use the code, with a different backend.

@alexkar598
Copy link
Member

This is NOT mfa. MFA is as the name implies multi factor. This means you need atleast 2 of the following:
-Something you are
-Something you know
-Something you have

This meets the something you know twice but fails to enforce any other factor in a mandatory fashion (discord supports something you have in the form of their own 2fa but an admin could have it off)

@adamsong
Copy link
Contributor Author

While yes, technically the discord is something you know instead of something you have, so is email, and depending on the feature of your phone, sms which are popular 2nd factors. I could implement something like TOTP, to get a better second, however that puts the burden of resets onto us, and requires us to either develop a robust backup system, or manually reset, requiring staff time and effort every time some forgets to transfer their codes.

@Hopekz
Copy link
Contributor

Hopekz commented Sep 25, 2021

So admins can't come on anymore if yogbot is down?

@TheGamerdk
Copy link
Contributor

So admins can't come on anymore if yogbot is down?

Pretty sure admins wouldn't be able to connect for the first time on a new device.
Previous logins are cached, so you won't have to reauthenticate if you're on the same IP + CID

@adamsong
Copy link
Contributor Author

I'll swap this over to TOTP at some point, bit busy with school ATM

@adamsong adamsong changed the title Adds discord based MFA for admins. Adds TOTP MFA for admins. Oct 2, 2021
@adamsong
Copy link
Contributor Author

adamsong commented Oct 3, 2021

Might be good now

Copy link
Member

@alexkar598 alexkar598 left a comment

Choose a reason for hiding this comment

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

  • Give a way to clear all your sessions
  • Make sessions expire after let's say 30 days
  • Give a way to login without creating a session
  • Add a bloody index to the table, a ckey, ip, cid index would do nicely
  • (Maybe?) Refactor your code in a way that doesnt rely on /datum/admin for a general availability of the feature, doesnt have to be enabled for everyone but would be nice to have this be an option

Ideally we'd want to use a means of verifying the session by a means other than cid/ip but shrug

@adamsong
Copy link
Contributor Author

adamsong commented Oct 3, 2021

Add a bloody index to the table, a ckey, ip, cid index would do nicely

Hmm, interesting suggestion of index, would something like this work:

PRIMARY KEY (`ckey`,`ip`,`cid`)

@adamsong adamsong added the Awaiting - Vote - Council Awaiting the results of a council vote label Oct 4, 2021
@AffectedArc07
Copy link
Contributor

Why is this even up for debate
image

@adamsong
Copy link
Contributor Author

Enforcing 2FA for forums admins was a council vote (two actually), so I'm just keeping up with the tradition.

@adamsong adamsong added Approved Council Vote This PR is approved by council vote. and removed Awaiting - Vote - Council Awaiting the results of a council vote labels Oct 17, 2021
@JamieD1
Copy link
Contributor

JamieD1 commented Oct 24, 2021

Database updated

@JamieD1 JamieD1 merged commit d940c3e into master Oct 24, 2021
@JamieD1 JamieD1 deleted the add-2fa branch October 24, 2021 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Admin This PR affects administrators Approved Council Vote This PR is approved by council vote. Config Config files need to be changed on the host for this to work On hold - HD Should not be merged without HD Approval SQL This PR has SQL code within, likely database related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants