REST API: Add Authentication method based on Jetpack Signatures by oskosk · Pull Request #5418 · Automattic/jetpack

oskosk · 2016-10-26T13:56:53Z

Changes proposed in this Pull Request:

Adds a token based authentication mechanism for the REST API.

We already generate tokens for the users when they connect thir self-hosted account to a WordPress.com account by means of the Jetpack connection process.

Remarks about this implementation

This implementation currently accepts WP API requests from the Jetpack server only

Testing instructions:

Ping Rocco on slack for running tests from the Jetpack server

Running Unit tests

In a proper WordPress testing environment, try this command:
phpunit --filter=test_jetpack_rest_api_authentication --debug

Why

We're going to implement Jetpack module settings in Calypso and this communication mechanism ensures that we can update the Jetpack site in real time without the need of going through the XML-RPC API.

Proposed changelog entry for your changes:

Adds a token based authentication mechanism to the REST API in order for the site to be able to receive authenticated requests from WordPress.com

roccotripaldi

I haven't actually tested this yet. But it seems like a great start. I left a few comments.

roccotripaldi · 2016-10-31T17:27:53Z

class.jetpack.php

 			Jetpack_Heartbeat::init();
 		}

+		if ( Jetpack::is_active() ) {


Perhaps we should do
if ( Jetpack::is_active() && defined( 'REST_REQUEST' ) ) {

to limit these filters to rest request.

roccotripaldi · 2016-10-31T17:28:06Z

class.jetpack.php

+	 * Authenticates REST API via user_tokens passed as GET query parameter
+	 */
+	function authenticate_user_tokens( $user_id ) {
+		$get_token = isset( $_GET[ 'user_token' ] ) ? $_GET[ 'user_token' ] : false;


Does it have to be a $_GET param?
Could it instead be an actual authorization header? Should we support both?

I'd vote for the header alternative

nylen · 2016-10-31T20:38:04Z

It looks like this sends user tokens unencrypted, which seems problematic for non-HTTPS sites. Does the existing XMLRPC-based authentication do this too?

oskosk · 2016-10-31T21:59:34Z

Thanks for the recommendations. I've just updated this PR description to take in account latest comments.

Note This PR was created as a kick-off for an implementation of securing the REST API with a mechanism that is currently not supported, so if you reach here please take in account those observations under Remarks about this implementation as they represent things that would need to be addressed for this implementation to be solid.

nylen · 2016-11-01T00:01:37Z

this practice sends the token unencrypted even when using SSL

GET parameters, along with the request URL itself, are encrypted in SSL requests.

sending it in a header, as @roccotripaldi mentions is the way to go

Using a header, the token would still be sent in plaintext over HTTP, which is not a great idea.

I assume that our existing XMLRPC auth strategy has some form of encryption and/or request signing for this reason. Unfortunately I'm not the person to ask about how that works, but this possible lack of transport-level encryption is why the OAuth1 flow and request signing mechanism look the way they do.

nb · 2016-11-15T09:04:49Z

We should definitely try and re-use as much as possible from the XML-RPC authentication. @mdawaffe might be able to help out with some specifics.

mdawaffe · 2016-11-16T22:32:31Z

These Jetpack tokens can't be included "raw" in requests, for the reasons others have outlined above.

This is why Jetpack uses the Jetpack_Signature class both when making requests from the site to WP.com and when making requests from WP.com to the site. It's similar to OAuth1 signatures in that the secrets are never transmitted, only HMACs are.

(The full background: Jetpack_Signature is an implementation of OAuth2 MAC, which never took off, with some tiny differences since MAC was only ever envisioned as a means of one way communication, whereas Jetpack needs to be able to make requests in either direction.)

Though we mostly only use Jetpack_Signature for XML-RPC, we should be able to use it for REST requests as well.

A few things that will need to happen:

Make sure $HTTP_RAW_POST_DATA is available when we need it. For XML-RPC requests, we have some hacks to make sure it's available. It's possible the file_get_contents( 'php://input' ) call is good enough, but I'm not sure - it will need lots of testing in various environments.
The Jetpack::verify_xml_rpc_signature() method should work fine for incoming REST API requests, though we should consider renaming it and some of its variables.
Jetpack_Client::remote_request() in the plugin can be used to build a signed request to WP.com, and Jetpack_Client::remote_request() on WP.com (largely the same code) can be used to build a signed request to the Jetpack site.

nylen · 2016-11-17T17:26:23Z

Make sure $HTTP_RAW_POST_DATA is available when we need it

The method to use for this is WP_REST_Request::get_body() which is populated by WP_REST_Server::get_raw_data().

If there are further changes needed there for different environments, let's get these changes into WP core as well.

DanReyLop · 2016-11-23T17:18:12Z

class.jetpack.php

 			Jetpack_Heartbeat::init();
 		}

+		if ( Jetpack::is_active() && defined( 'REST_REQUEST' ) ) {


I was using WordPress 4.5.4 and REST_REQUEST was not defined. Same thing happened in WordPress 4.6.1.

I've put some var_dump() here and there, and what is happening is that this code is being executed, and after that, wp-includes/rest-api.php#rest_api_loaded() is executed (and the REST_REQUEST constant is set).

Which version of WordPress did you use to test this?

I was using 4.5 But I didn't update the code here to move the hooking to a safe place yet. I've just labeled this PR to "In Progress" as there's a lot to be changed.

roccotripaldi · 2016-11-29T02:28:02Z

I added a commit to address @DanReyLop 's feedback.

I get this when trying to run the curl request in the testing instructions above:
{"code":"rest_forbidden","message":"Sorry, you are not allowed to do that.","data":{"status":403}}

But i was also getting that prior to my commit. I think if we start addressing the feedback from @nb and @mdawaffe we will be on the right track. i'm happy to help out this week. :)

roccotripaldi · 2016-11-29T21:48:33Z

Testing instructions have been updated. Please ping me on Slack for how to run tests from the Jetpack server.

tyxla

Added a small comment, but this worked just fine for me, and looks great 👍

tyxla · 2016-11-30T12:59:25Z

class.jetpack-signature.php

! empty() will include a check for null values, so the ! is_null() check is now obsolete.

nylen · 2016-12-01T17:00:37Z

This needs unit tests for requests with both valid and invalid signatures.

nylen · 2016-12-22T09:10:30Z

I thought of a couple more small improvements to make, see above.

Also, sorry for the noise, but I'm switching this back to In Progress since I think there are some remaining issues still.

It looks like we need to pay more attention to this earlier comment:

Make sure $HTTP_RAW_POST_DATA is available when we need it. For XML-RPC requests, we have some hacks to make sure it's available. It's possible the file_get_contents( 'php://input' ) call is good enough, but I'm not sure - it will need lots of testing in various environments.

$HTTP_RAW_POST_DATA is not always set - it was removed in PHP 7, and there's an ini setting that can turn it off.

If it's not set, we'll end up calling file_get_contents( 'php://input' ) twice: once here and once here.

But, from the php://input docs:

php://input is a read-only stream that allows you to read raw data from the request body. [...] Note: Prior to PHP 5.6, a stream opened with php://input could only be read once

So I think this is probably a bit broken in, say, PHP 5.5 with always_populate_raw_post_data set to something other than TRUE. In order to solve it, we need to make sure we only read php://input at most once, which is probably not too bad (the testing is the hard part).

This also made me think of where else we might have problems with handling of request bodies. Basically we need to make sure that the signature verification code in Jetpack::verify_xml_rpc_signature handles request bodies the same way as the REST API, and I'm no longer convinced that that's the case. So we need to go through that block and test out the different variations more thoroughly.

roccotripaldi · 2016-12-27T19:26:25Z

Since this PR is getting rather large and unwieldy, I propose we create issues for the following improvements:

Ensure we are only reading php://input once at most
Ensure that Jetpack::verify_xml_rpc_signature handles request bodies identical to REST API

We can merge this PR, and work on improvements in separate PRs. Keeping this unmerged is slowing down work on Jetpack server enhancements.

nylen · 2016-12-28T00:57:21Z

The first issue is fine for a separate improvement.

The second issue needs to be handled here before anything is merged. We need to make sure it is not possible to send a request parameter that is not checked as part of request signing.

oskosk · 2017-01-05T13:19:58Z

@roccotripaldi @nylen I've just squashed this branch to have a single commit (plus a debugging one from last night by Rocco) so we can eventually simplify the merging task when wrapping up the release that will include this PR.

By doing that, I totally owned your 30+ commits :( . I left the relevant commit messages though, but I forgot to add references to the authors.

georgestephanis · 2017-01-06T20:27:08Z

class.jetpack.php

This feels like an awkward way to log errors -- wouldn't it be simpler to concatenate the print_r( $body, true ) onto the string, rather than making a bigger array?

This isn't sticking around in the final version.

georgestephanis · 2017-01-06T20:30:58Z

class.jetpack.php

Would it be more readable to do empty() instead of ! isset() ? Or do we want to accept falsey empty strings?

Doesn't matter - this can only ever be blog or user. See $token_type in verify_xml_rpc_signature.

georgestephanis · 2017-01-06T20:31:23Z

class.jetpack.php

Should this have translation __() calls around it?

Probably - will go through and add these.

georgestephanis · 2017-01-06T20:33:36Z

class.jetpack.php

Is this the best place to handle authentication? I had thought it was better to use determine_current_user for that?

Good catch - I think you're right. This would cause auth here to override the user set by a cookie, for example. WP_REST_OAuth1 only uses rest_authentication_errors to report errors that were stored previously.

@nylen

* Check if we're on a REST API request before enabling the token-based auth method for the rest API * Do some Jetpack specific stuff on action `wp_json_init` instead of checking for `REST_REQUEST` constant. * Use the existing XMLRPC signature verification to validate WP API requests coming from the Jetpack server. Modifies the algorithm to handle a request containing an empty string body. * Ensure that the authorization object is not a WP_Error before attempting to use it. * To prove that we can use this mechanism to post complex objects, I've modified the bulk module activation endpoint to support JSON in POST body. * The endpoint should still work with multi-part form data as it did before. * In core's phpunit suites, `WP_Test_REST_Controller_Testcase` is already defined. Jetpack should define it if the class already exists. * Updating auth logic so that we do not overwrite any work from other plugins. props @nylen * improve coverage of Jetpack::wp_rest_authenticate to cover generic errors as well as errors from verify_xml_rpc_signature * REST Authentication Logic - WP API returns simply `true` for generic auth errors - Jetpack's authorization will account for this by also returning `true` in these cases - Jetpack's authorizaiton will also account for other plugins that may be authenticating, by giving their errors priority * Update modules list endpoint parameters for WP 4.7 * Run new API tests under multisite too * Improve error message when signature verification fails * Once we decide to handle authentication, either succeed or fail: Eliminate undecided states caused by successful signature verification without a token that we know how to deal with. It looks like this could happen if we successfully authenticated with a token of type 'blog' - let's make this (and any other such conditions) unsupported (and return an error) instead. * When receiving a POST request with no body, authentication fails with body-hash errors. This is because our signature verification is looking explicitly for `null` in regards to body. If, after all of Jetpack's sniffing, we still find an empty body, we should explicitly set it to `null`. Not all POST requests will require a body.

PHP versions prior to 5.6 cannot handle multiple reads from php://input

This also means we need to change up the logic to parse the request body, because we are now running *before* the WP REST API body-parsing code rather than *after* it.

nylen · 2017-01-10T02:05:05Z

With the latest changes this is ready for review again.

roccotripaldi · 2017-01-10T16:47:00Z

High fives @nylen and @georgestephanis ! This looks good and works well. I think all concerns have been addressed. If George would give it a thumbs up, I am completely confident to merge.

georgestephanis · 2017-01-11T04:11:13Z

@nylen @roccotripaldi -- Last thing -- one thing I had to do with Application Passwords was this -- WordPress/application-passwords@4226502 -- because I was only allowing my override of determine_current_user to happen on rest api requests where the constant was defined -- which occasionally would happen before the constant was defined, so the old lack of a current user would get cached in the global. I don't /think/ that would be the case here -- would it?

edit: the code in the changeset linked above eventually changed to https://github.com/georgestephanis/application-passwords/blob/c975c312b4f448d6dab0d016e152ab75354d38cd/class.application-passwords.php#L40-L67

Apart from that, 💯

samhotchkiss · 2017-01-11T18:54:37Z

@vortfu is going to be reviewing shortly, once he signs off, let's go ahead and merge to master and branch-4.5

nylen · 2017-01-11T19:02:40Z

Discussed #5418 (comment) a bit with George via DM. This comment highlights the fact that we are currently running our auth logic on every request, but we should probably only be running it for a REST_REQUEST like Application Passwords does.

As George points out, this is a bit tricky, because the determine_current_user filter runs before the REST_REQUEST constant is defined. To get around that, we can unset the $current_user global soon after the constant is defined, which will cause the next call to wp_get_current_user() to call determine_current_user again (ref).

@samhotchkiss Let me know if y'all would like to fix this here or in a separate PR, either one should be fine.

samhotchkiss · 2017-01-11T21:12:02Z

@nylen -- if you can fix here in the next couple of hours, that's 👍

This is the simplest way to make sure that we only apply this authentication method when intended (i.e. for requests from the WP.com servers).

nylen · 2017-01-11T22:36:34Z

So, I am a little bit uncomfortable with adding the $current_user hackery without further testing of this PR. Most requests would have been fine as-is, because they wouldn't have ?token or ?signature params so they would quickly be ignored. And XMLRPC requests are fine because determine_current_user does not run under XMLRPC: https://github.com/WordPress/wordpress-develop/blob/4.7.0/src/wp-includes/user.php#L2506-L2509

Where we might have run into problems is if someone did another API request (unrelated to Jetpack) that happened to contain a ?token or ?signature parameter. These would have caused our new auth method to kick in and raise an error.

In 1937dcd I addressed this by following the existing convention of a ?for=jetpack parameter, but I'm calling it ?_for=jetpack instead because the leading underscore denotes a WP REST API "meta" or "internal" parameter that individual API endpoints shouldn't care about.

Note - this new parameter will require changes to existing WP.com server code. See D3926-code.

I still think this is good to ship. Possible future enhancements:

Only run our determine_current_user filter if the REST_REQUEST constant is defined, using a similar approach to the application passwords plugin. This is more complicated, though, and for what we will need this code to do, I think ?_for=jetpack will be fine.
Make it possible to pass token and signature parameters (and a few others like body-hash) to WP REST API endpoints. This will currently be problematic because these parameters are used by the authentication scheme. We can either hack around this later or just avoid using these parameter names.

roccotripaldi · 2017-01-12T01:48:23Z

Note - this new parameter will require changes to existing WP.com server code. See D3926-code.

@nylen : noted. Thanks.

oskosk added the [Status] In Progress label Oct 26, 2016

jeherve added Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] WPCOM API labels Oct 26, 2016

roccotripaldi reviewed Oct 31, 2016

View reviewed changes

oskosk added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Oct 31, 2016

oskosk added the [Status] Needs Changelog label Oct 31, 2016

oskosk mentioned this pull request Nov 6, 2016

Implement Jetpack Module Settings Automattic/wp-calypso#9171

Closed

55 tasks

jeherve added this to the Not Currently Planned milestone Nov 15, 2016

oskosk changed the title ~~Try: Add Authentication mechanism for REST API based on Jetpack user tokens~~ Try: Add Authentication method for REST API based on Jetpack user tokens Nov 16, 2016

DanReyLop reviewed Nov 23, 2016

View reviewed changes

oskosk added [Status] In Progress and removed [Status] Needs Review This PR is ready for review. labels Nov 23, 2016

roccotripaldi added [Status] Needs Review This PR is ready for review. [Status] Needs Testing We need to add this change to the testing call for this month's release and removed [Status] In Progress labels Nov 29, 2016

tyxla reviewed Nov 30, 2016

View reviewed changes

tyxla mentioned this pull request Dec 6, 2016

Jetpack Settings: Add actions, reducer, selectors and query components for module settings Automattic/wp-calypso#9847

Merged

roccotripaldi force-pushed the try/user_token-auth-for-rest-api branch from 563460e to 0307a06 Compare December 6, 2016 18:51

nylen force-pushed the try/user_token-auth-for-rest-api branch from 7ddacda to 03ac039 Compare December 22, 2016 08:34

tyxla mentioned this pull request Dec 28, 2016

Jetpack Settings: Implement bulk settings management into Redux Automattic/wp-calypso#10123

Merged

georgestephanis reviewed Jan 6, 2017

View reviewed changes

roccotripaldi and others added 8 commits January 9, 2017 19:27

Clean up comments

0fae0f5

Ensure that the request body is read at most once

ed86d0a

PHP versions prior to 5.6 cannot handle multiple reads from php://input

Only allow tested and known-working request parameters

fc1d665

Add translation calls to error messages

4d852ae

Add a missing require

9e9dd7c

Update the tests to set the request body as expected by the new code

8e6cf22

Use 'determine_current_user' filter for authentication checks

b3aa973

This also means we need to change up the logic to parse the request body, because we are now running *before* the WP REST API body-parsing code rather than *after* it.

nylen added 2 commits January 9, 2017 22:51

Remove vim swp file

616363b

Be nice and unset $HTTP_RAW_POST_DATA after each test

dfa7008

Only apply REST API authentication when ?_for=jetpack

1937dcd

This is the simplest way to make sure that we only apply this authentication method when intended (i.e. for requests from the WP.com servers).

This was referenced Jan 12, 2017

Sites: Add siteSupportsCalypsoSettingsUI() selector Automattic/wp-calypso#10584

Merged

REST API: Add support for HTTP_CONTENT_TYPE in authentication #6095

Merged

nylen mentioned this pull request Jan 30, 2017

Reverse permission check in REST API auth #6235

Merged

roccotripaldi mentioned this pull request Apr 3, 2017

Allow HTTP PUT requests sent from Jetpack to WPCOM #6853

Merged

Conversation

oskosk commented Oct 26, 2016 • edited by roccotripaldi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this Pull Request:

Remarks about this implementation

Testing instructions:

Running Unit tests

Why

Proposed changelog entry for your changes:

Uh oh!

roccotripaldi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nylen commented Oct 31, 2016

Uh oh!

oskosk commented Oct 31, 2016

Uh oh!

nylen commented Nov 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nb commented Nov 15, 2016

Uh oh!

mdawaffe commented Nov 16, 2016

Uh oh!

nylen commented Nov 17, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oskosk Nov 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roccotripaldi commented Nov 29, 2016

Uh oh!

roccotripaldi commented Nov 29, 2016

Uh oh!

tyxla left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nylen commented Dec 1, 2016

Uh oh!

nylen commented Dec 22, 2016

Uh oh!

roccotripaldi commented Dec 27, 2016

Uh oh!

nylen commented Dec 28, 2016

Uh oh!

oskosk commented Jan 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nylen commented Jan 10, 2017

Uh oh!

roccotripaldi commented Jan 10, 2017

Uh oh!

oskosk commented Oct 26, 2016 •

edited by roccotripaldi

Loading

nylen commented Nov 1, 2016 •

edited

Loading

oskosk Nov 23, 2016 •

edited

Loading

tyxla left a comment •

edited

Loading

georgestephanis commented Jan 11, 2017 •

edited

Loading

nylen commented Jan 11, 2017 •

edited

Loading