Page MenuHomePhabricator

Security Readiness Review For OAuthRateLimiter
Closed, ResolvedPublic

Description

Project Information

Description of the tool/project:
The extension holds a single database table with a mapping from OAuth client ID to the rate limit tier. The rate limits for different tiers are statically configured in mediawiki-config. The tiers are assigned to the clients via a maintenance script. Dynamic tiers and UI for tier management might be implemented later if needed.

The ratelimit claims for the client is supplied to the OAuth extension via a new hook. The OAuth extension adds the claims to the access token JWT as private claims, which is then used by the envoy API Gateway to supply to the ratelimit service.

Currently the code depends on the fork of the oauth2-server library, that includes a single pull request which adds support for private claims. We're working with upstream to get the pull request accepted in the upstream library, and the need for the fork will eventually disappear.

Description of how the tool will be used at WMF:
We are developing an API Portal/Gateway. The work is described by the API Gateway initiative.

As part of this project, we plan to use the extension, OAuthRateLimiter, to add ratelimiter information to the OAuth token.

Dependencies

  • mediawiki-extensions-OAuth
  • Wikimedia/oauth2-server

Has this project been reviewed before?
No

Working test environment

  1. Download both OAuth and OAuthRateLimiter to extensions/ folder
  2. Checkout 613282 into OAuthRateLimiter
  3. Checkout 610335 into OAuth
  4. Run composer update to bring in fork of league/oauth2-server
  5. Add the following code at the bottom of your LocalSettings.php:
wfLoadExtension( 'OAuth' );
wfLoadExtension( 'OAuthRateLimiter' );
  1. Run the update script which will automatically create the necessary database tables that these extensions needs.
  2. Generate public and private keys
openssl genrsa -out private.key 2048
openssl rsa -in private.key -pubout -out public.key
  1. Configure user rights & general params:
// OAuth requires emails to be authenticated, this automatically authenticates an email added to user preference
$wgEmailAuthentication = false;

// Rights to add/update a consumer
$wgGroupPermissions['*']['mwoauthproposeconsumer'] = true;
$wgGroupPermissions['*']['mwoauthupdateownconsumer'] = true;

// location of private & public key 
$wgOAuth2PrivateKey = "/var/www/mediawiki/extensions/OAuth/private.key";
$wgOAuth2PublicKey = "/var/www/mediawiki/extensions/OAuth/public.key";



// OAuthRatelimiter configs
$wgOAuthRateLimiterDefaultClientTier = 'default';
$wgOAuthRateLimiterTierConfig = [
    'default' => [
        'ratelimit' => [
            'request_per_unit' => 1000,
            'unit'  => 'sec'
        ] 
    ],
    'Tier 1' => [
        'ratelimit' => [
            'request_per_unit' => 10000,
            'unit'  => 'sec'
        ]
    ]
];
  1. Follow OAuth registration steps to register an OAuth application. Make sure to choose OAuth 2.0 for OAuth protocol version and to save your consumer and private token for the next steps.
  2. Follow OAuth 2.0 authorization steps to authorize the client and get an access_token. Note: requests to /oauth2/access_token must be a POST.
  3. Use a website like https://jwt.io/ to decode the access_token. You should see the default rate limit information from $wgOAuthRateLimiterTierConfig
  4. To change a user’s client tier use the maintenance script: php setClientTierName.php --client=<your_client_id> --tier="Tier 1"
  5. To see the updated ratelimit in the access_token, you’ll need to rerun steps 10-11

Post-deployment
Platform Engineering will own the extension.

Event Timeline

Restricted Application added a project: secscrum. · View Herald TranscriptJul 14 2020, 2:45 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Where to find the code base to review? Where to find the issue tracker and issue tracker tag to report bugs?
Asking as there are no links on https://www.mediawiki.org/wiki/Extension:OAuthRateLimiter ...

sbassett closed this task as Resolved.Jul 20 2020, 4:59 PM
sbassett claimed this task.
sbassett triaged this task as Lowest priority.
sbassett moved this task from Incoming to Back Orders on the secscrum board.
sbassett added a subscriber: sbassett.

I'm going to stall this as lowest for now until we have:

  1. A more firm deployment date
  2. The rest of the above template filled out
  3. A commit sha at which to freeze the code base (whenever that may exist) for review
sbassett reopened this task as Open.Jul 20 2020, 4:59 PM
Pchelolo updated the task description. (Show Details)Jul 22 2020, 11:01 PM
Clarakosi updated the task description. (Show Details)Jul 23 2020, 6:32 PM
WDoranWMF raised the priority of this task from Lowest to High.Jul 23 2020, 7:25 PM

hi @sbassett, I believe the template should be fully complete now please let me know if we have elided anything.

Our release date is dependent on the security review. We would like to release by August 14th but, obviously, we will adjust dependent on scheduling from Security.

I have increased the priority to reflect *our priority level* BUT again please adjust that dependent on your team's timelines and we will adapt as needed.

WDoranWMF updated the task description. (Show Details)Jul 23 2020, 7:25 PM
Jcross added a subscriber: Jcross.EditedJul 29 2020, 4:22 PM

Hi @WDoranWMF - we're wondering if this code is finished and ready to be reviewed? Please let us know and we will get this started. Please feel free to touch base with me or @Reedy with any questions. Thanks!

sbassett reassigned this task from sbassett to Reedy.Jul 29 2020, 4:23 PM

Hi @WDoranWMF - we're wondering if this code is finished and ready to be reviewed? Please let us know and we will get this assigned and started. Thanks!

Yes. The codebase will not change going forward, maybe only some minor styling updates.

Note: This work ultimately depends on the contribution to upstream oauth2-server library, that is not merged yet, so once it's merged we will change references to the dependency, but there only code style changes left as well, no functional changes will be made.

@Pchelolo - just to clarify, the security review only needs to focus on the one currently-open patch set? Since it looks like what's currently on master is just a stubbed-out mw extension.

@Pchelolo - just to clarify, the security review only needs to focus on the one currently-open patch set? Since it looks like what's currently on master is just a stubbed-out mw extension.

Yes. The patchset. There sill be no more patchsets for this prior to deployment, and this one will not change.

sbassett moved this task from Back Orders to Waiting on the secscrum board.Aug 5 2020, 4:05 PM
Jcross added a comment.Aug 5 2020, 4:16 PM

Hi @Pchelolo - if you will merge we can go ahead and begin this review. Please just send a quick note when that's done. Thanks!

@Jcross is it possible to do the review of an unmerged gerrit patch?

This ultimately depends on vendoring a new version of a dependency for MediaWiki, and merging the whole dependency tree would mean deploying it, and we would like to have a bit more caution with it.

A bit more detail: OAuthRateLimit (the new extension under review, whole codebase except boilerplate is in linked gerrit change) depends on a new hook in OAuth extension. The new hook in OAuth, ultimately depends on new features added in oauth2-server package in this pull request. Currently the sequence of patches in gerrit is build on top of vendoring the version of the dependency with the pull request included - obviously we can't do that. The pull request in question is ready to be merged, but we're waiting for the maintainers of the upstream to cut the release branch, which they have promised to do shortly. As soon as that happens, we can vendor the new version properly, and then get to merging.

Alternatively, I can remove the dependency and just merge the code that uses a non-existent hook, but I thought demonstrating the whole chain of dependencies would be beneficial for the reviewers.

Reedy updated the task description. (Show Details)Aug 5 2020, 8:05 PM
Jcross added a comment.Aug 5 2020, 9:08 PM

@Pchelolo - thank you for the additional information. Sam will review and get back to you asap.

Reedy updated the task description. (Show Details)Aug 7 2020, 3:00 PM
Reedy moved this task from Waiting to Our Part Is Done on the secscrum board.Aug 7 2020, 3:49 PM

So, the thing is we've been burned numerous times being wanted to review moving targets/unmerged code. And to some extent, this chain has more than most, as it has upstream dependancies. "There'll be no more changes" is famous last words. You can't say no functional changes will be made when the upstream implementations aren't even finished/"stable".

See also the CR comments left... I've identified one bug so far that is going to require a code change :)

The changes/implementation of the extension itself is fairly standalone, but then we've got the dependancies on the OAuth extension, which depends on an upstream change "a single pull request" (which has 28 commits). It certainly comes across as being a way away from being merged; radio silence on requests for changes for 10 days.

It would seem the target deployment of 14/8 isn't going to happen, purely based on upstream changes not being finished and merged. Never mind being tagged in a release. It also sounds like it's going to land in 9.x (not in 8.x which is current stable/release), so there's a question of when that's going to be finished too. Unless we end up using a fork for our (short term) purposes.

In this case, there's no real issue with merging the change for this extension (IMHO), it's not like it's deployed yet, for example. Even if it was, the lack of anything calling the hook makes it a no-op. Conversely, it's "simple enough" to review in gerrit.

I know you're not proposing to actually merge the vendor change with the patches using the fork where it's being developed, but for example, that is someone with apparently no prior contributions to the project, followed up by partial revertions by someone who has.

Then the RFC it is being implemented against is a now expired draft - https://tools.ietf.org/id/draft-polli-ratelimit-headers-00.html (Expires: March 8, 2020) with many issued filed (10 currently open) at https://github.com/ioggstream/draft-polli-ratelimit-headers/issues. Plus we also have https://github.com/envoyproxy/envoy/pull/12410/, again implemented against the draft RFC (but accepted and merged).

I know it's "common enough" that software implements draft RFC's and then has to chase targets if they change, or they don't, and the implementations become de-facto (even if the RFC's haven't actually been accepted).

Do we actually know if the parts of the RFC we're implementing against should be considered "stable", for example?

Obviously, this all results in risk. Sure, it's good that we're getting the changes implemented upstream, so relieves some of the maintenance burden for ourselves, but that code is certainly subject to change. We then have to adapt to those, if/when they happen.


There's very little to really review security wise in this extension itself (and therefore is now done). It is not much more than a slim wapper around a simple database table.

But there's definitely some risk (even if low) as identified above from the tree of changes. They potentially don't actually apply to this extension directly, but more to the OAuth changes (T254948: Security Readiness Review For Enhancements to OAuth Extension), and the ApiPortal as a whole down the road.

It would seem the target deployment of 14/8 isn't going to happen, purely based on upstream changes not being finished and merged. Never mind being tagged in a release. It also sounds like it's going to land in 9.x (not in 8.x which is current stable/release), so there's a question of when that's going to be finished too. Unless we end up using a fork for our (short term) purposes.

I think we will end up with a fork in the short term, we were trying to avoid a fork and wait for that PR to get merged for as long as possible, that's why unmerged dependency tree.

Then the RFC it is being implemented against is a now expired draft - https://tools.ietf.org/id/draft-polli-ratelimit-headers-00.html (Expires: March 8, 2020) with many issued filed (10 currently open) at https://github.com/ioggstream/draft-polli-ratelimit-headers/issues. Plus we also have https://github.com/envoyproxy/envoy/pull/12410/, again implemented against the draft RFC (but accepted and merged).

This draft RFC has absolutely nothing to do with the OAuthRateLimiter extension. It is an entirely separate feature, tracked under T246278.

OAuthRateLimiter provides ratelimit claim for the JWT access token emitted by OAuth extension. The claim is private and is not covered by any standard - OAuth protocol treats access token as an opaque value. The 'ratelimit' claim will then be interpreted by our envoy configuration to pass it along to the rate limiting service. Our own envoy is the only consumer of the claim that is set here, so we are free in our choice of name for it.

There's very little to really review security wise in this extension itself (and therefore is now done). It is not much more than a slim wapper around a simple database table.

Would you recommend breaking the dependency chain in gerrit, merging the OAuthRateLimiter extension after addressing your comments, and dealing with the dependency fork separately?

They potentially don't actually apply to this extension directly, but more to the OAuth changes (T254948: Security Review Request for Enhancements to OAuth Extension), and the ApiPortal as a whole down the road.

I would like to note that those changes are entirely independent from this tree.

Reedy added a comment.Aug 7 2020, 4:24 PM

I will note, the whole dependancy tree for this project is somewhat confusing.

There's numerous parts with outwardly very similar purposes (ie rate limiting). It feels odd that they're all being done around the same time, but apparently aren't related?

Similarly, looking at T254948: Security Readiness Review For Enhancements to OAuth Extension, I cannot tell what "enhancements" these are (I've got to look at the gerrit patch), so, my fault there, sure, but again, multiple changes in a similar area at a similar time...

I will note, the whole dependancy tree for this project is somewhat confusing.

yeah... I agree. Sorry about that, we're working on a bunch of different pieces simultaneously.

Reedy added a comment.Aug 7 2020, 5:07 PM

I guess for clarity... Where is https://gerrit.wikimedia.org/r/c/mediawiki/extensions/OAuth/+/610335 supposed to be reviewed? Are they wanted to be reviewed?

Obviously it's a dependancy of the OAuthRateLimiter extension itself... But isn't called out specifically for review on this task (just a testing dependancy). Hence my confusion with T254948: Security Readiness Review For Enhancements to OAuth Extension, and it obviously isn't T254947: Security Review Request for WikimediaApiPortalOAuth Extension or the older T239940: Security review of OAuth 2.0 patches either...

I guess for clarity... Where is https://gerrit.wikimedia.org/r/c/mediawiki/extensions/OAuth/+/610335 supposed to be reviewed? Are they wanted to be reviewed?

As I understand our policies, only new extensions are required to be reviewed, thus no separate for an OAuth change, but we would definitely appreciate your feedback on the changes in the dependency tree of the new extension.

As I understand our policies, only new extensions are required to be reviewed, thus no separate for an OAuth change, but we would definitely appreciate your feedback on the changes in the dependency tree of the new extension.

Current language from our SOP:

Before a new or significant change to an existing feature, extension, service, or library is deployed (or intended to be deployed) to production on SRE-managed hardware, it MUST have gone through the security readiness review process defined within this SOP.

Work product for which this SHALL NOT be relevant:

* Routine changes which are typically submitted through Gerrit and handled via standard code review
* Security patches or other discreetly-deployed code
* Applications running under Cloud VPS, even high-visibility apps like Quarry, etc.
* Any user-JavaScript or Gadgets which may run on various Wikimedia wikis

If there are any questions as to what constitutes the requirement for a security review, please contact the Security Team (security-help@wikimedia.org).
Jcross moved this task from Our Part Is Done to Waiting on the secscrum board.Aug 7 2020, 6:07 PM

@Reedy We've created a Wikimedia fork of the changes to oauth2-server and have updated the dependency tree accordingly.

I am not sure if we need separate security tickets for the review of the vendor and OAuth changes but we'd appreciate a review of both.

Hi @Clarakosi - we'll be able to start progress on this ticket, but please note that we will need a new ticket/s for the review of vendor and Oauth. Also, please note that while these new reviews do not appear to be a heavy lift, they will be new tickets in our workflow and will be triaged as such. We'll do our best to address them in a timely manner. Please let us know what your new deployment date looks like in light of this ticket being ready for review and all of the confusion sorted out only very recently, and if there is anything else I can do to help. @Reedy will be in touch with any comments or concerns as he moves forward with the ticket. Thank so much!

Jcross moved this task from Waiting to In Progress on the secscrum board.Aug 17 2020, 4:46 PM
Clarakosi updated the task description. (Show Details)Aug 17 2020, 6:18 PM

Hi @Jcross - Thanks for clarifying the process! I've created tickets for the review of the vendor and OAuth changes. Please let us know if we missed anything.

@Clarakosi - received and we will triage and get in queue tomorrow! Thanks :)

Reedy closed this task as Resolved.Aug 27 2020, 9:41 PM
Reedy moved this task from In Progress to Our Part Is Done on the secscrum board.

This stack is good to go