Page MenuHomePhabricator

MW REST Framework support for authenticated CORS
Closed, ResolvedPublic

Description

For more context, please look into all the discussions on the parent task, as well as on T232176 and T261053

TLDR: It's been decided that by default REST API will set access-control-allow-credentials: false thus disallowing CORS requests with cookie authentication.

However, for the use-case described in the parent task, we need to be able to enable it on a per-handler basis.

Proposal

Introduce a new method in handler:

/**
  * Returns a list of domains for which to allow CORS requests with credentials.
  * @see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Credentials
  *
  * @return string[]
  * @internal
  */
public allowCorsWithCredentialsForOrigins() : array { // Name TBD, I'm notoriously bad in naming.
  return [];
}

By default the method would return an empty array, thus disallowing a cookie-authenticated CORS request. If the specific route handler needs it, it has to override the method, providing a list of origins allowed to do the requests.

Under the hood, if the returned list is not empty, the REST Framework will, for every request (including OPTIONS), check if the Origin header is present and is in the allowed list, and if it is, set Access-Control-Allow-Credentials: true and Access-Control-Allow-Origin: <origin> - the origin needs to specifically match the Origin header, wildcards not allowed here by the standard.

Event Timeline

Pchelolo created this task.Aug 31 2020, 9:09 PM

Change 623461 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/core@master] Implement REST framework support for CORS with credentials.

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

TLDR: It's been decided that by default REST API will set access-control-allow-credentials: false thus disallowing CORS requests with cookie authentication.

This should be unset by default not false.

providing a list of origins allowed to do the requests.

This doesn't provide any additional benefit from simply allowing all of the origins specified in $wgCrossSiteAJAXdomains. If you have more than one Origin you have to add a Vary: Origin at which point, there is no benefit of providing a different list of trusted origins. Even if you had a single origin, you would still want to set * for anon requests which would force the Vary: Origin again.

I think a better implementation might be something like this:

/**
  * Allows cross-origin requests using cookies for authorization.
  *
  * Enabling this option has a performance impact and should only be done if
  * using other authorization mechanism (like Oath) is impossible for this route.
  *
  * @return bool
  * @internal
  */
public allowCrossOriginCookieAuth() : bool {
  return false;
}

I think this does create a slippery slope where it's not clear why some endpoints would allow this and others do. Clients should work to remove the performance impact by using a different authorization method for cross-origin requests.

Another idea, would be to allow Cookie Auth for all requests, but configure it per wiki. Basically Meta would allow such requests (by configuration) but all other wikis would not. Which I guess kind of makes sense conceptually. Meta is the "central" wiki for lots of things and should allow access all the time, but not the other way around.

This doesn't provide any additional benefit from simply allowing all of the origins specified in $wgCrossSiteAJAXdomains.

The only reason was to be able to narrow it down even more then $wgCrossSiteAJAXdomains. I agree that it's not really necessary and a boolean would work just fine. The idea was to make this as narrow as absolutely possible. Since this is supposed to be a one-off only for very specific endpoints, we should not make it too convenient. Handler can easily provide $wgCrossSiteAJAXdomains as a result of the method, but with a boolean approach it's less flexible since it can't provide anything other then $wgCrossSiteAJAXdomains.

If you have more than one Origin you have to add a Vary: Origin

You mean more then zero origins - if we have anything in the allowed list we need vary: origin

would be to allow Cookie Auth for all requests, but configure it per wiki

Is still feel like the more narrow the surface is where this is enabled the better. Also, you can vary the config by wiki, so theoretically you can achieve varying it by wiki. The plan is to have it enabled only for meta and only with a single domain in the special allow list.

would be to allow Cookie Auth for all requests, but configure it per wiki

We would still want to respect wgCrossSiteAJAXdomains, so we would still need to Vary: Origin, but now for all endpoints, thus creating a lot of unnecessary performance impact.

dbarratt added a comment.EditedSep 1 2020, 3:38 PM

I think this whole thing is trying to make an exception for a single client. I don't think this is a good idea. But I do understand if a wiki wanted to take the performance impact for all routes. I just don't see why a wiki would want to allow it on some routes, but not others. It implies that the routes they want to allow use are known, when they might not be. We'd have to add configuration to allow a wiki to specify which routes are allowed.

Therefore, if you wanted to limit the scope, you could provide a config option like:

$wgRestAllowCrossOriginCookieAuth = [];

which specified an array of routes (by path? name? id?) that are allowed to use cross origin cookie auth. And perhaps they can specify * to mean all routes?

I really don't see why this would be up to the implementer of the endpoint and not the wiki themselves. The wiki is taking the risk, not the implementer and most wikis wont need to allow that, because their clients will use OAuth. However if the wiki does have some routes it needs to use cookie auth on, it can specify that (and will use wgCrossSiteAJAXdomains to determine which domains are allowed to do that).

@sbassett in the parent task has indicated that the approach is fine from the security perspective if we are restricting both the number of routes we enable this on and the origins, so we would need to ask for input again if we are to switch to the approach you're proposing.

Honestly, yes, thinking about it more, simpler configuration does make sense, so introducing something as simple as

$wgRestAllowCrossOriginCookieAuth = true;

which would enable CORS with credentials across all routes for origins specified in $wgCrossSiteAJAXdomains would be the simplest and most obvious way to go.

The downsides of the approach are:

  • Performance impact across all routes, not just the ones used cross origin
  • Much broader attack surface (not sure there's any attack surface existing at all, but if there is, it's much broader)

On the upside:

  • Action API takes this approach, it's either enabled or disabled for everything
  • Indeed CORS it's not a concern for the handler implementer, so from a framework perspective it would make more sense

As for the proposal to make the $wgRestAllowCrossOriginCookieAuth an array of routes... It's a nice middle ground from the behavior perspective, but it just smells fishy with copy-pasting the route-names into this config...

@Pchelolo That makes sense to me. This task should be resolved when T232176 is resolved correct? Assuming we make it configurable as described in T232176#6427076?

Based on discussion with Security, @JBennett, @sbassett and for Security Teams reference I'm noting here that as a manager I approve the risk level they have identified of medium for this work.

Change 623461 abandoned by Ppchelko:
[mediawiki/core@master] Implement REST framework support for CORS with credentials.

Reason:
I128b4bdbec4f6bea35142153c951fd7b79617106

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

sbassett added a comment.EditedOct 2 2020, 8:08 PM

@WDoranWMF et al - Sorry for the delay in acknowledging this. I've added an entry within our risk register for your acceptance at T261696#6450752. I went with medium risk for now, since https://gerrit.wikimedia.org/r/621900 is a bit less granular (even if it may currently be restricted within production for a specific use case) than what I had discussed at T261358#6424759.