Page MenuHomePhabricator
Create Task
Maniphest T149150

OAuth api access on wikitech fails with consumed nonce error
Closed, ResolvedPublic
Actions

Description

I'm using the mwclient python library to try and make automated page edits to wikitech. The code I have works on a local test wiki, but it fails with an invalid nonce error when run against wikitech and labtestwikitech.

A reproduction boils down to this python snippet:

import mwclient
consumer_token = 'your consumer token here'
consumer_secret = 'your consumer secret here'
access_token = 'your access token here'
access_secret = 'your access secret here'
site = mwclient.Site(
    'labtestwikitech.wikimedia.org',
    consumer_token=consumer_token,
    consumer_secret=consumer_secret,
    access_token=access_token,
    access_secret=access_secret
)

When the Site object is created it calls /w/api.php?format=json&continue=&meta=siteinfo|userinfo|userinfo&action=query&siprop=general|namespaces&uiprop=groups|rights|blockinfo|hasmsg to get some basic metadata. This initial API call dies with the consumer nonce error.

With a bit more magic you can get full debug logs for the request into Logstash (set a X-Wikimedia-Debug: log header via the backing requests python library). That told me that BagOStuff::add() was being called twice for the same nonce, but didn't help figure out quite why. I live hacked a log with an exception trace into MemcachedPeclBagOStuff::add to find the call paths:

"exception": {
      "class": "Exception",
      "message": "0.42272100 1477437607",
      "code": 0,
      "file": "/srv/mediawiki/php-1.28.0-wmf.22/includes/libs/objectcache/MemcachedPeclBagOStuff.php:174",
      "trace": [
        "/srv/mediawiki/php-1.28.0-wmf.22/extensions/OAuth/backend/MWOAuthDataStore.php:108",
        "/srv/mediawiki/php-1.28.0-wmf.22/extensions/OAuth/lib/OAuth.php:759",
        "/srv/mediawiki/php-1.28.0-wmf.22/extensions/OAuth/lib/OAuth.php:707",
        "/srv/mediawiki/php-1.28.0-wmf.22/extensions/OAuth/lib/OAuth.php:611",
        "/srv/mediawiki/php-1.28.0-wmf.22/extensions/OAuth/backend/MWOAuthServer.php:160",
        "/srv/mediawiki/php-1.28.0-wmf.22/extensions/OAuth/api/MWOAuthSessionProvider.php:80",

        "/srv/mediawiki/php-1.28.0-wmf.22/includes/session/SessionManager.php:486",
        "/srv/mediawiki/php-1.28.0-wmf.22/includes/session/SessionManager.php:189",
        "/srv/mediawiki/php-1.28.0-wmf.22/includes/WebRequest.php:735",
        "/srv/mediawiki/php-1.28.0-wmf.22/includes/session/SessionManager.php:128",
        "/srv/mediawiki/php-1.28.0-wmf.22/includes/Setup.php:759",
        "/srv/mediawiki/php-1.28.0-wmf.22/includes/WebStart.php:137",
        "/srv/mediawiki/php-1.28.0-wmf.22/api.php:38",
        "/srv/mediawiki/w/api.php:3"
      ]
    }
  },
"exception": {
      "class": "Exception",
      "message": "0.44190500 1477437607",
      "code": 0,
      "file": "/srv/mediawiki/php-1.28.0-wmf.22/includes/libs/objectcache/MemcachedPeclBagOStuff.php:174",
      "trace": [
        "/srv/mediawiki/php-1.28.0-wmf.22/extensions/OAuth/backend/MWOAuthDataStore.php:108",
        "/srv/mediawiki/php-1.28.0-wmf.22/extensions/OAuth/lib/OAuth.php:759",
        "/srv/mediawiki/php-1.28.0-wmf.22/extensions/OAuth/lib/OAuth.php:707",
        "/srv/mediawiki/php-1.28.0-wmf.22/extensions/OAuth/lib/OAuth.php:611",
        "/srv/mediawiki/php-1.28.0-wmf.22/extensions/OAuth/backend/MWOAuthServer.php:160",
        "/srv/mediawiki/php-1.28.0-wmf.22/extensions/OAuth/api/MWOAuthSessionProvider.php:80",
        "/srv/mediawiki/php-1.28.0-wmf.22/includes/session/SessionManager.php:486",
        "/srv/mediawiki/php-1.28.0-wmf.22/includes/session/SessionManager.php:189",
        "/srv/mediawiki/php-1.28.0-wmf.22/includes/WebRequest.php:735",
        "/srv/mediawiki/php-1.28.0-wmf.22/includes/user/User.php:3149",
        "/srv/mediawiki/php-1.28.0-wmf.22/includes/user/User.php:3448",
        "/srv/mediawiki/php-1.28.0-wmf.22/includes/user/LocalIdLookup.php:72",
        "/srv/mediawiki/php-1.28.0-wmf.22/includes/user/CentralIdLookup.php:156",
        "/srv/mediawiki/php-1.28.0-wmf.22/includes/user/CentralIdLookup.php:191",
        "/srv/mediawiki/php-1.28.0-wmf.22/extensions/OAuth/backend/MWOAuthUtils.php:279",
        "/srv/mediawiki/php-1.28.0-wmf.22/extensions/OAuth/api/MWOAuthSessionProvider.php:95",
        "/srv/mediawiki/php-1.28.0-wmf.22/includes/session/SessionManager.php:486",
        "/srv/mediawiki/php-1.28.0-wmf.22/includes/session/SessionManager.php:189",
        "/srv/mediawiki/php-1.28.0-wmf.22/includes/WebRequest.php:735",
        "/srv/mediawiki/php-1.28.0-wmf.22/includes/session/SessionManager.php:128",
        "/srv/mediawiki/php-1.28.0-wmf.22/includes/Setup.php:759",
        "/srv/mediawiki/php-1.28.0-wmf.22/includes/WebStart.php:137",
        "/srv/mediawiki/php-1.28.0-wmf.22/api.php:38",
        "/srv/mediawiki/w/api.php:3"
      ]
    }
  },

This points to MWOAuthSessionProvider::provideSessionInfo where the request is verified (first stack trace) and then MWOAuthUtils::getLocalUserFromCentralId is called to get the user object (second stack trace). During the getLocalUserFromCentralId call User::getRights is called which circles right back around to calling MWOAuthSessionProvider::provideSessionInfo a second time (and recursively).

This loop doesn't seem to happen on "normal" SUL wikis. I haven't tested for it on an other non-SUL wikis besides wikitech and labtestwikitech.

Details

SubjectRepoBranchLines +/-
Avoid suppressed-username permission check in local user lookupmediawiki/extensions/OAuthmaster+1 -1
Do not return unsafe users from CentralIdLookup::checkAudience()mediawiki/coremaster+15 -1
wikitech: Re-enable OAuth management interfacesoperations/mediawiki-configmaster+3 -2
wikitech: Set wgMWOAuthCentralWiki = falseoperations/mediawiki-configmaster+1 -1
Customize query in gerrit

Event Timeline

bd808 created this task.Oct 26 2016, 12:02 AM
Restricted Application added a project: Cloud-Services. · View Herald TranscriptOct 26 2016, 12:02 AM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Tgr subscribed.Oct 26 2016, 12:21 AM

The first nonce check is the usual one. The second happens because CentralAuth is not installed so central ID lookup happens via LocalIdLookup which checks whether the current user is allowed to see hidden usernames. That requires loading the current user from the session, which is an infinite loop (or would be if not for the nonce check).

On a general level, I don't see a way to fix this - session providers will just have to take care to always use AUDIENCE_RAW or AUDIENCE_PUBLIC when checking central IDs.

Specifically for OAuth, we can do that, and we can also track nonce checks - if the memcached check for a given nonce succeeded, there is no need to redo that check in the same request.

Specifically for LocalIdLookup, we can just check whether the user is fully loaded and assume AUDIENCE_PUBLIC otherwise.

gerritbot subscribed.Oct 26 2016, 12:36 AM

Change 318025 had a related patch set uploaded (by Gergő Tisza):
Do not return unsafe users from CentralIdLookup::checkAudience()

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

gerritbot added a project: Patch-For-Review.Oct 26 2016, 12:36 AM
gerritbot added a comment.Oct 26 2016, 12:47 AM

Change 318026 had a related patch set uploaded (by Gergő Tisza):
Avoid suppressed-username permission check in local user lookup

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

Tgr added a comment.Oct 26 2016, 12:52 AM
In T149150#2743597, @Tgr wrote:

...we can also track nonce checks - if the memcached check for a given nonce succeeded, there is no need to redo that check in the same request.

I decided to be lazy, that would involve too much messing with the external OAuth library that the extension uses. The other two fixes are less direct but should make the error go away nevertheless.

gerritbot added a comment.Oct 26 2016, 2:16 AM

Change 318036 had a related patch set uploaded (by BryanDavis):
wikitech: Set wgMWOAuthCentralWiki = false

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

gerritbot added a comment.Oct 26 2016, 1:03 PM

Change 318036 merged by jenkins-bot:
wikitech: Set wgMWOAuthCentralWiki = false

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

Stashbot subscribed.Oct 26 2016, 1:11 PM

Mentioned in SAL (#wikimedia-operations) [2016-10-26T13:11:49Z] <hashar@tin> Synchronized wmf-config/CommonSettings.php: wikitech: Set wgMWOAuthCentralWiki = false - T149150 (duration: 00m 47s)

Anomie subscribed.Oct 26 2016, 1:52 PM
In T149150#2743597, @Tgr wrote:

The first nonce check is the usual one. The second happens because CentralAuth is not installed so central ID lookup happens via LocalIdLookup which checks whether the current user is allowed to see hidden usernames.

Correct so far.

That requires loading the current user from the session, which is an infinite loop (or would be if not for the nonce check).

Not quite. It requires loading the session for the request associated with the User object, and it's using the WebRequest even for users that aren't the WebRequest user.

What we probably really need to do is have User::getRights() only check ->getRequest()->getSession() if $this->mRequest && $this->isSafeToLoad() || $wgUser->isSafeToLoad() && $this->getName() === $wgUser->getName(). And if either of those ->isSafeToLoad() checks fails, we want to make sure that $this->mRights isn't used later when ->isSafeToLoad() becomes true.

On a general level, I don't see a way to fix this - session providers will just have to take care to always use AUDIENCE_RAW or AUDIENCE_PUBLIC when checking central IDs.

Note that this bug is using AUDIENCE_PUBLIC. Only AUDIENCE_RAW (or a User with an explicit non-WebRequest ->mRequest) is currently safe.

Specifically for OAuth, [...] we can also track nonce checks - if the memcached check for a given nonce succeeded, there is no need to redo that check in the same request.

Wouldn't hurt. You could probably wrap the MemcachedPeclBagOStuff in CachedBagOStuff.

Specifically for LocalIdLookup, we can just check whether the user is fully loaded and assume AUDIENCE_PUBLIC otherwise.

That wouldn't help here. The problem is in User::getRights(), not in User::loadFromSession().

bd808 added a comment.Oct 26 2016, 2:01 PM
In T149150#2744705, @Stashbot wrote:

Mentioned in SAL (#wikimedia-operations) [2016-10-26T13:11:49Z] <hashar@tin> Synchronized wmf-config/CommonSettings.php: wikitech: Set wgMWOAuthCentralWiki = false - T149150 (duration: 00m 47s)

Changing the configuration to $wgMWOAuthCentralWiki = false; for wikitech and labtestwikitech has fixed the problem on those wikis.

Maybe this is all just a case of "don't do that" for accidentally setting a wiki up so that it tries to use global user lookup when it really only has local users?

Anomie added a comment.Oct 26 2016, 2:24 PM
In T149150#2744874, @bd808 wrote:

Maybe this is all just a case of "don't do that" for accidentally setting a wiki up so that it tries to use global user lookup when it really only has local users?

LocalIdLookup should still work, IMO.

gerritbot added a comment.Oct 26 2016, 3:38 PM

Change 318110 had a related patch set uploaded (by BryanDavis):
wikitech: Re-enable OAuth management interfaces

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

gerritbot added a comment.Oct 26 2016, 6:02 PM

Change 318110 merged by jenkins-bot:
wikitech: Re-enable OAuth management interfaces

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

Stashbot added a comment.Oct 26 2016, 6:10 PM

Mentioned in SAL (#wikimedia-operations) [2016-10-26T18:10:16Z] <bd808@tin> Synchronized wmf-config/CommonSettings.php: wikitech: Re-enable OAuth management interfaces T149150 (duration: 00m 46s)

gerritbot added a comment.Oct 28 2016, 12:59 AM

Change 318025 abandoned by Gergő Tisza:
Do not return unsafe users from CentralIdLookup::checkAudience()

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

scfc subscribed.Feb 16 2017, 1:45 PM

@bd808: Is there anything left to do here?

bd808 closed this task as Resolved.Feb 16 2017, 9:35 PM
bd808 claimed this task.
bd808 removed a project: Patch-For-Review.
In T149150#3032939, @scfc wrote:

@bd808: Is there anything left to do here?

Let's call it done. Fixing the config made the problem go away.

Restricted Application added a project: User-bd808. · View Herald TranscriptFeb 16 2017, 9:35 PM
Aklapper removed a subscriber: Anomie.Oct 16 2020, 5:39 PM
Restricted Application added a subscriber: Danmichaelo. · View Herald TranscriptOct 16 2020, 5:39 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits