REST API: Add Authentication method based on Jetpack Signatures #5418

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

@@ -476,6 +483,11 @@ private function __construct() {
 			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

@@ -476,6 +483,11 @@ private function __construct() {
 			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

@@ -59,7 +59,7 @@ function sign_current_request( $override = array() ) {

 		$url = "{$scheme}://{$_SERVER['HTTP_HOST']}:{$port}" . stripslashes( $_SERVER['REQUEST_URI'] );

-		if ( array_key_exists( 'body', $override ) && !is_null( $override['body'] ) ) {
+		if ( array_key_exists( 'body', $override ) && ! is_null( $override['body'] ) && ! empty( $override['body'] ) ) {


! 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 · 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.

@nylen

* Add Authentication mechanism for REST API based on Jetpack user tokens * 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. * Clean up comments * Ensure that the request body is read at most once PHP versions prior to 5.6 cannot handle multiple reads from php://input * Only allow tested and known-working request parameters * Add translation calls to error messages * Add a missing require * Update the tests to set the request body as expected by the new code * Use 'determine_current_user' filter for authentication checks 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. * Remove vim swp file * Be nice and unset $HTTP_RAW_POST_DATA after each test * Only apply REST API authentication when ?_for=jetpack 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).

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.

CHangelog: add #5457 Changelog: add #5487 Changelog: add #5708 Changelog: add #5879 Changelog: add #5932 Changelog: add #5963 Changelog: add #5968 Changelog: add #5996 Changelog: add #5998 Changelog: add #5999 Changelog: add #6012 Changelog: add #6013 Changelog: add #6014 Changelog: add #6015 Changelog: add #6023 Changelog: add #6024 Changelog: add #6030 Changelog: add #5465 CHangelog: add #6063 Changelog: add #6025 Changelog: add #5974 Changelog: add #6059 Changelog: add #6046 Changelog: add #5418 Changelog: move things around and add missing information. Changelog: add #5565 Changelog: add #6087 Changelog: add #6095

@tyxla

Changelog: add #5867 Changelog: add #5874 Changelog: add #5905 Changelog: add #5906 Changelog: add #5931 Changelog: add #5933 Changelog: add #5934 Bring over 4.4.2 changelog from branch-4.4 @see 18012a3 Changelog: add #5976, #5978, #5983 Changelog: add #5917 Changelog: add #5832 Changelog: add 4.4.2 release post link. CHangelog: add #5457 Changelog: add #5487 Changelog: add #5708 Changelog: add #5879 Changelog: add #5932 Changelog: add #5963 Changelog: add #5968 Changelog: add #5996 Changelog: add #5998 Changelog: add #5999 Changelog: add #6012 Changelog: add #6013 Changelog: add #6014 Changelog: add #6015 Changelog: add #6023 Changelog: add #6024 Changelog: add #6030 Changelog: add #5465 CHangelog: add #6063 Changelog: add #6025 Changelog: add #5974 Changelog: add #6059 Changelog: add #6046 Changelog: add #5418 Changelog: move things around and add missing information. Changelog: add #5565 Changelog: add #6087 Changelog: add #6095 Readme: add @tyxla to the list of contributors. Improved changelog for your readability and enjoyment updated the release date finalizing the changelog with a few more edits

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

jeherve added [Type] 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 To request a review from fellow Jetpack developers. Label will be renamed soon. 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 To request a review from fellow Jetpack developers. Label will be renamed soon. labels Nov 23, 2016

roccotripaldi added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [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 removed the [Status] In Progress label Jan 10, 2017

nylen force-pushed the try/user_token-auth-for-rest-api branch 2 times, most recently from 4b794ab to b3aa973 Compare January 10, 2017 02:24

Remove vim swp file

616363b

nylen force-pushed the try/user_token-auth-for-rest-api branch from b294245 to 616363b Compare January 10, 2017 03:51

Be nice and unset $HTTP_RAW_POST_DATA after each test

dfa7008

samhotchkiss requested a review from vortfu January 11, 2017 18:54

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).

samhotchkiss merged commit 3a29df9 into master Jan 11, 2017

samhotchkiss removed the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Jan 11, 2017

nylen deleted the try/user_token-auth-for-rest-api branch January 11, 2017 23:03

jeherve added [Status] Has Changelog and removed [Status] Needs Changelog labels Jan 12, 2017

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

jeherve added the [Feature] WP REST API label Jan 24, 2017

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

REST API: Add Authentication method based on Jetpack Signatures #5418

REST API: Add Authentication method based on Jetpack Signatures #5418

oskosk commented Oct 26, 2016 •

edited by roccotripaldi

Loading

roccotripaldi left a comment

roccotripaldi Oct 31, 2016

roccotripaldi Oct 31, 2016

oskosk Oct 31, 2016

nylen commented Oct 31, 2016

oskosk commented Oct 31, 2016

nylen commented Nov 1, 2016 •

edited

Loading

nb commented Nov 15, 2016

mdawaffe commented Nov 16, 2016

nylen commented Nov 17, 2016

DanReyLop Nov 23, 2016

oskosk Nov 23, 2016 •

edited

Loading

roccotripaldi commented Nov 29, 2016

roccotripaldi commented Nov 29, 2016

tyxla left a comment •

edited

Loading

tyxla Nov 30, 2016

roccotripaldi Nov 30, 2016

nylen commented Dec 1, 2016

nylen commented Jan 10, 2017

roccotripaldi commented Jan 10, 2017

georgestephanis commented Jan 11, 2017 •

edited

Loading

samhotchkiss commented Jan 11, 2017

nylen commented Jan 11, 2017

samhotchkiss commented Jan 11, 2017

nylen commented Jan 11, 2017 •

edited

Loading

roccotripaldi commented Jan 12, 2017

REST API: Add Authentication method based on Jetpack Signatures #5418

REST API: Add Authentication method based on Jetpack Signatures #5418

Conversation

oskosk commented Oct 26, 2016 • edited by roccotripaldi Loading

Changes proposed in this Pull Request:

Remarks about this implementation

Testing instructions:

Running Unit tests

Why

Proposed changelog entry for your changes:

roccotripaldi left a comment

Choose a reason for hiding this comment

roccotripaldi Oct 31, 2016

Choose a reason for hiding this comment

roccotripaldi Oct 31, 2016

Choose a reason for hiding this comment

oskosk Oct 31, 2016

Choose a reason for hiding this comment

nylen commented Oct 31, 2016

oskosk commented Oct 31, 2016

nylen commented Nov 1, 2016 • edited Loading

nb commented Nov 15, 2016

mdawaffe commented Nov 16, 2016

nylen commented Nov 17, 2016

DanReyLop Nov 23, 2016

Choose a reason for hiding this comment

oskosk Nov 23, 2016 • edited Loading

Choose a reason for hiding this comment

roccotripaldi commented Nov 29, 2016

roccotripaldi commented Nov 29, 2016

tyxla left a comment • edited Loading

Choose a reason for hiding this comment

tyxla Nov 30, 2016

Choose a reason for hiding this comment

roccotripaldi Nov 30, 2016

Choose a reason for hiding this comment

nylen commented Dec 1, 2016

nylen commented Jan 10, 2017

roccotripaldi commented Jan 10, 2017

georgestephanis commented Jan 11, 2017 • edited Loading

samhotchkiss commented Jan 11, 2017

nylen commented Jan 11, 2017

samhotchkiss commented Jan 11, 2017

nylen commented Jan 11, 2017 • edited Loading

roccotripaldi commented Jan 12, 2017

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