[go: up one dir, main page]

Page MenuHomePhabricator

Invalid client secrets in API Portal
Closed, ResolvedPublic2 Estimated Story PointsBUG REPORT

Description

Expected behavior

OAuth client secrets created or reset through the API Portal are valid and unique.

Observed behavior

OAuth client secrets recently created or reset through the API Portal are shared between clients and result in an invalid client error when fetching an access token.

# Get auth code
https://meta.wikimedia.org/w/rest.php/oauth2/authorize?client_id=CLIENT_ID_HERE&response_type=code

# Get access token
curl -X POST -F 'grant_type=authorization_code' \
-F 'code=AUTH_CODE_HERE' \
-F 'client_id=CLIENT_ID_HERE' \
-F 'client_secret=CLIENT_SECRET_HERE' \
https://meta.wikimedia.org/w/rest.php/oauth2/access_token

# Error
{"error":"invalid_client","error_description":"Client authentication failed","message":"Client authentication failed"}

I was able to replicate this error with secrets that were reset on May 24, 2021. The last time I tested this successfully was February 8, 2021. Client secrets reset via Meta work as expected. Could be related to similar past issue T264457: Client secret shared between clients

Event Timeline

sdkim changed the subtype of this task from "Task" to "Bug Report".Jun 10 2021, 2:49 PM
sdkim set the point value for this task to 2.Dec 2 2021, 5:13 PM
sdkim moved this task from Needs Grooming to Must do now on the API Platform board.
DAbad raised the priority of this task from Medium to High.Jan 27 2022, 5:15 PM
DAbad moved this task from Must do now to Should do next on the API Platform board.
DAbad moved this task from Should do next to Must do now on the API Platform board.

I've reproduced what seems to be the same behavior on my local.

Assuming I've reproduced it correctly, then the $clientAccess->getSecretKey() call on line 60 of AbstractClientHandler is actually returning an error message (mwoauth-field-private) and not the actual secret key. The Utils::hmacDBSecret() call on that same line therefore receives an instance of Message rather than a key.

Because the Message instance is the same in all cases, it happily hashes to the same value for all clients (and is equally useless as a secret key for all of them).

Next steps:

  1. figure out why we're getting an error instead of the actual key (and fix)
  2. make this code more robust so that it recognizes errors instead of hashing Message objects

Followup:

Looks like this was already fixed once here:
https://gerrit.wikimedia.org/r/c/mediawiki/extensions/OAuth/+/633551

But then that fix was (maybe inadvertently?) reverted as part of a larger change here:
https://gerrit.wikimedia.org/r/c/mediawiki/extensions/OAuth/+/657386

In fact, the logic in the section of interest can be simplified a bit, as the test for valid client is already covered by the $status->isGood() test, and we already return to the caller an appropriate error message in that case.

Patch incoming.

Change 759753 had a related patch set uploaded (by BPirkle; author: BPirkle):

[mediawiki/extensions/OAuth@master] Restore fix from https://gerrit.wikimedia.org/r/c/mediawiki/extensions/OAuth/+/633551 that was (maybe inadvertently?) reverted in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/OAuth/+/657386

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

Change 759753 merged by jenkins-bot:

[mediawiki/extensions/OAuth@master] Fix client secret display after reset on API Portal

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

Verified that this is fixed in production. Thanks all!