Page MenuHomePhabricator

Cannot enable 2FA on testwiki
Closed, ResolvedPublic

Description

Since I’m an admin on test.wikipedia.org (discussion), I wanted to (ab?)use this privilege to enable two-factor authentication for my account. (I’m also applying to become an interface administrator on Commons, so I want my account to be strongly protected, ideally before being granted that right.) However, it didn’t work – despite trying multiple times, Special:OATH rejected all the codes generated by my phone with the error “Failed to validate two-factor credentials”.

On test-commons.wikimedia.org (testcommonswiki, where I’m also an admin thanks to T233056), on the other hand, I was able to enable 2FA without issue.

Details

Related Gerrit Patches:
mediawiki/extensions/OATHAuth : masterDo not store proper objects in session data
mediawiki/extensions/WebAuthn : REL1_34Do not store proper objects in session data
mediawiki/extensions/OATHAuth : REL1_34Do not store proper objects in session data
mediawiki/extensions/WebAuthn : masterDo not store proper objects in session data

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 17 2019, 7:07 PM
Reedy added a subscriber: Reedy.EditedSep 17 2019, 8:44 PM

What is curious... Is both wikis were on the same MW version.... (.22)

Confirmed that I can't enable 2FA on testwiki on .23 either on a test account

https://github.com/wikimedia/mediawiki-extensions-OATHAuth/commit/ae53dc5c60ecade85293656af4ab199d3534aa4e (from T231786) would be the suspect commit... but why isn't breaking on other wikis?

Will have to put the revert onto prod and test it...

Reedy triaged this task as High priority.Sep 17 2019, 10:25 PM
Reedy added a project: Operations.
Reedy added a subscriber: ItSpiderman.

And as T231786 was only on testwiki... That makes it all the more suspect

Not necessarily a bug in OATHAuth... But maybe something to do with servers/caching?

I did note on testwiki... That when I entered a code, when it refreshed, the Two-factor authentication secret key changed... Which suggests the code in https://github.com/wikimedia/mediawiki-extensions-OATHAuth/commit/ae53dc5c60ecade85293656af4ab199d3534aa4e is being hit every run, invalidating the key, so of course it's not going to be able to validate

If I enter a bogus code on test commons (123456), I still get the same Two-factor authentication secret key

If we look at the original error in T231786...

[XWzSlgpAMFgAAJHPyUAAAABG] /w/index.php?title=Special:Manage_Two-factor_authentication&action=enable&module=totp BadMethodCallException from line 36 of /srv/mediawiki/php-1.34.0-wmf.20/extensions/OATHAuth/src/HTMLForm/TOTPEnableForm.php: Call to a member function getSecret() on a non-object (array)

Why is it an array?

I'm getting an early night (for me!) and will look at this again sometime tomorrow. I'll steal a mwdebug server and have a look at what said array apparently includes

Hm, it cant be an array, key can only be generated from

TOTPKey::newFromRandom()

which cannot return an array. Thats weird, im very interested in what that array contains

Reedy added a comment.Sep 18 2019, 3:59 PM

Hm, it cant be an array, key can only be generated from

TOTPKey::newFromRandom()

which cannot return an array. Thats weird, im very interested in what that array contains

It's definitely an array :(

So using this simple "test":

reedy@deploy1001:/srv/mediawiki-staging/php-1.34.0-wmf.23/extensions/OATHAuth$ git diff
diff --git a/src/HTMLForm/TOTPEnableForm.php b/src/HTMLForm/TOTPEnableForm.php
index a3dd186..326d455 100644
--- a/src/HTMLForm/TOTPEnableForm.php
+++ b/src/HTMLForm/TOTPEnableForm.php
@@ -30,6 +30,7 @@ class TOTPEnableForm extends OATHAuthOOUIHTMLForm implements IManageForm {
                $key = $this->getRequest()->getSessionData( 'oathauth_totp_key' );
 
                if ( !$key instanceof TOTPKey ) {
+                       var_dump( $key );
                        $key = TOTPKey::newFromRandom();
                        $this->getRequest()->setSessionData( 'oathauth_totp_key', $key );
                }

It's getting an empty array (see the top left)

This really doesn't feel like an OATHAuth bug... So it's possibly something in MW.. Or possibly something related to testwiki that falls under SRE's purview

@LucasWerkmeister OOI, what browser are you using? I'm on Chrome Version 77.0.3865.75 on macOS

LucasWerkmeister added a comment.EditedSep 18 2019, 4:00 PM

Firefox Developer Edition 70.0b5 (64-bit) on Arch Linux.

Reedy added a subscriber: daniel.EditedSep 19 2019, 9:29 PM

So, yeah, it's Kask (even if indirectly), and the way MW uses it is breaking this behaviour. It worked not using Kask. So the next question is does this just break OATHAuth? Or do other extensions put objects in and therefore will expect it to work?

Note that per T161647: RFC: Deprecate using php serialization inside MediaWiki we should not be using PHP serialization for this purpose. It's insecure and brittle against code changes. Only "dumb" objects should get serialized (associative arrays, essentially). Any code relying on "proper" objects coming back from serialization should be fixed.

@ItSpiderman So we can't seriali[sz]e objects. You need to swap it for an associative array

Change 538154 had a related patch set uploaded (by ItSpiderman; owner: ItSpiderman):
[mediawiki/extensions/OATHAuth@master] Do not store proper objects in session data

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

Change 538155 had a related patch set uploaded (by ItSpiderman; owner: ItSpiderman):
[mediawiki/extensions/WebAuthn@master] Do not store proper objects in session data

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

Switched to jsonSerializing objects before putting them into session.
These changes are made on top of existing changes to avoid merge conflicts

Change 538154 merged by jenkins-bot:
[mediawiki/extensions/OATHAuth@master] Do not store proper objects in session data

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

Change 538155 merged by jenkins-bot:
[mediawiki/extensions/WebAuthn@master] Do not store proper objects in session data

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

Change 543461 had a related patch set uploaded (by Reedy; owner: ItSpiderman):
[mediawiki/extensions/OATHAuth@REL1_34] Do not store proper objects in session data

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

Change 543462 had a related patch set uploaded (by Reedy; owner: ItSpiderman):
[mediawiki/extensions/WebAuthn@REL1_34] Do not store proper objects in session data

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

Change 543461 merged by jenkins-bot:
[mediawiki/extensions/OATHAuth@REL1_34] Do not store proper objects in session data

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

Change 543462 merged by jenkins-bot:
[mediawiki/extensions/WebAuthn@REL1_34] Do not store proper objects in session data

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

Reedy closed this task as Resolved.Oct 16 2019, 2:03 PM
Reedy claimed this task.
SD0001 added a subscriber: SD0001.Oct 31 2019, 5:18 AM
SD0001 removed a subscriber: SD0001.