Page MenuHomePhabricator

Allow multiple different 2FA devices
Open, Needs TriagePublic

Description

Creating as an umbrella for T232336: Separate recovery codes into a separate MFA method and T230042: Allow multiple totp devices

It should be possible to have TOTP and WebAuthn enabled

[21:28:13] <AntiComposite> I know it was just deployed, but is there a reason that OATHAuth only supports TOTP _or_ WebAuthn (U2F)?
[21:29:40] <AntiComposite> Every other service I use that supports WebAuthn supports both at the same time, which means I can use my hardware key on my laptop but fall back to my TOTP generator for my phone, where WebAuthn isn't supported in the browser.

Event Timeline

The current db schema looks like:

CREATE TABLE oathauth_users (
 id INTEGER not null primary key, -- fk to user/central ID
 module TEXT not null, -- varchar(255) in non-sqlite
 data BLOB null -- JSON blob
 );

I would like to introduce a new column with autoincrement IDs to be the primary key, and allow id to be just a reference to the central ID but non-unique. I hope schema change wise it'll be simple, the code changes will require more work.

Change 873892 had a related patch set uploaded (by Majavah; author: Majavah):

[mediawiki/extensions/OATHAuth@master] Database-level support for multiple auth devices

https://gerrit.wikimedia.org/r/873892

Change 885284 had a related patch set uploaded (by Majavah; author: Majavah):

[mediawiki/extensions/OATHAuth@master] Api: Do not expose the module name in the output

https://gerrit.wikimedia.org/r/885284

Change 885284 merged by jenkins-bot:

[mediawiki/extensions/OATHAuth@master] API: Do not expose the module name in the output

https://gerrit.wikimedia.org/r/885284

Would this schema change be a good time to try and do/fix T242847: Include enrollment timestamp in oathauth_users.data?

It might make more sense to use/add a column rather than just bundling it into a json blob (as I originally suggested with the code as currently is)?

Would this schema change be a good time to try and do/fix T242847: Include enrollment timestamp in oathauth_users.data?

It might make more sense to use/add a column rather than just bundling it into a json blob (as I originally suggested with the code as currently is)?

This is a great opportunity to add the a column for that, but I think the actual implementation is best left for later. (I have a few local patches that still need some polishing but would make this somewhat easier.) Updated the patch for that.

Would this schema change be a good time to try and do/fix T242847: Include enrollment timestamp in oathauth_users.data?

It might make more sense to use/add a column rather than just bundling it into a json blob (as I originally suggested with the code as currently is)?

This is a great opportunity to add the a column for that, but I think the actual implementation is best left for later. (I have a few local patches that still need some polishing but would make this somewhat easier.) Updated the patch for that.

Oh yeah, I'm not bothered about getting it implemented (though if you were willing to later...). It's always going to have to have a "null" value for "we don't know when this person enrolled".

But yeah, including a useful schema change while is still cheap/easy (not that the DB tables are that large, but still), is a win all around.

Thanks!

Change 891833 had a related patch set uploaded (by Majavah; author: Majavah):

[operations/mediawiki-config@master] Set OATHAuthMultipleDevicesMigrationStage to MIGRATION_OLD

https://gerrit.wikimedia.org/r/891833

Change 891833 merged by jenkins-bot:

[operations/mediawiki-config@master] Set OATHAuthMultipleDevicesMigrationStage to MIGRATION_OLD

https://gerrit.wikimedia.org/r/891833

Mentioned in SAL (#wikimedia-operations) [2023-03-01T16:17:58Z] <taavi@deploy2002> Started scap: Backport for [[gerrit:891833|Set OATHAuthMultipleDevicesMigrationStage to MIGRATION_OLD (T242031)]]

Mentioned in SAL (#wikimedia-operations) [2023-03-01T16:20:00Z] <taavi@deploy2002> taavi: Backport for [[gerrit:891833|Set OATHAuthMultipleDevicesMigrationStage to MIGRATION_OLD (T242031)]] synced to the testservers: mwdebug2001.codfw.wmnet, mwdebug1002.eqiad.wmnet, mwdebug1001.eqiad.wmnet, mwdebug2002.codfw.wmnet

Mentioned in SAL (#wikimedia-operations) [2023-03-01T16:26:21Z] <taavi@deploy2002> Finished scap: Backport for [[gerrit:891833|Set OATHAuthMultipleDevicesMigrationStage to MIGRATION_OLD (T242031)]] (duration: 08m 23s)

Mentioned in SAL (#wikimedia-releng) [2023-03-01T16:32:51Z] <taavi> deployment-prep: block automatic/update.php execution of UpdateForMultipleDevicesSupport maintenance script in OATHAuth by pre-inserting rows in updatelog tables, will be run manually instead. T242031

Change 873892 merged by jenkins-bot:

[mediawiki/extensions/OATHAuth@master] Database-level support for multiple auth devices

https://gerrit.wikimedia.org/r/873892

Migration plan:

  • Create the new tables on beta
  • Write both for beta
  • Run the migration script on beta
  • Read new for beta
  • Write new for beta
  • Rename/drop old table on beta
  • Wait for the train to deploy new code
  • Create the new tables on production
  • Write both for production
  • Run the migration script on production
  • Read new for production
  • Write new for production
  • Rename/drop old table in production

Mentioned in SAL (#wikimedia-releng) [2023-03-22T20:34:32Z] <taavi> cat /srv/mediawiki/php-master/extensions/OATHAuth/sql/mysql/tables-generated.sql | sql centralauth --write # T242031

Change 902189 had a related patch set uploaded (by Majavah; author: Majavah):

[operations/mediawiki-config@master] [beta] Write both for OATHAuthMultipleDevicesMigrationStage

https://gerrit.wikimedia.org/r/902189

Change 902189 merged by jenkins-bot:

[operations/mediawiki-config@master] [beta] Write both for OATHAuthMultipleDevicesMigrationStage

https://gerrit.wikimedia.org/r/902189

Mentioned in SAL (#wikimedia-operations) [2023-03-22T21:06:26Z] <taavi@deploy2002> Started scap: Backport for [[gerrit:902188|Remove OATHAuthMultipleDevicesMigrationStage from CS]], [[gerrit:902189|[beta] Write both for OATHAuthMultipleDevicesMigrationStage (T242031)]]

Mentioned in SAL (#wikimedia-operations) [2023-03-22T21:08:09Z] <taavi@deploy2002> taavi: Backport for [[gerrit:902188|Remove OATHAuthMultipleDevicesMigrationStage from CS]], [[gerrit:902189|[beta] Write both for OATHAuthMultipleDevicesMigrationStage (T242031)]] synced to the testservers: mwdebug1001.eqiad.wmnet, mwdebug1002.eqiad.wmnet, mwdebug2001.codfw.wmnet, mwdebug2002.codfw.wmnet

Mentioned in SAL (#wikimedia-operations) [2023-03-22T21:13:55Z] <taavi@deploy2002> Finished scap: Backport for [[gerrit:902188|Remove OATHAuthMultipleDevicesMigrationStage from CS]], [[gerrit:902189|[beta] Write both for OATHAuthMultipleDevicesMigrationStage (T242031)]] (duration: 07m 29s)

Mentioned in SAL (#wikimedia-releng) [2023-03-22T21:14:15Z] <taavi> taavi@deployment-mwmaint02:~$ mwscript extensions/OATHAuth/maintenance/UpdateForMultipleDevicesSupport.php metawiki --force # T242031

Change 902198 had a related patch set uploaded (by Majavah; author: Majavah):

[operations/mediawiki-config@master] [beta] Read new for OATHAuthMultipleDevicesMigrationStage

https://gerrit.wikimedia.org/r/902198

Change 902198 merged by jenkins-bot:

[operations/mediawiki-config@master] [beta] Read new for OATHAuthMultipleDevicesMigrationStage

https://gerrit.wikimedia.org/r/902198

Migrating beta uncovered a couple of issues with the migration process that I somehow did not think about before. Essentially:

  • If someone enables 2fa during the "write both" phase, the migration script will create a duplicate row in the new table.
  • Since the migration script deletes the rows from the old table, two-factor authentication will not work properly once the migration script has not been ran but write mode is still set to old.

So I need to re-think the migration strategy here. :( I'll think about it and come back with an updated proposal/patch.

The way we do other data migrations is that we keep the duplicate data around and remove it with the drop of the old tables/fields. Basically my suggestion is to simply avoid deleting the rows in the old table. Would that fix your issue?