Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added PKCE support #46

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,7 @@ _book
*.epub
*.mobi
*.pdf
.idea
.idea
.vscode
vendor
composer.lock
20 changes: 20 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,26 @@ This plugin uses the OAuth 2 protocol to allow delegated authorization; that is,

This plugin only supports WordPress >= 4.8.

## Proof Key for Code Exchange
OAuth2 plugin supports PKCE as a protection against authorization code interception attack (relevant only for authorization code grant type). In order to use PKCE, on the initial authorization request, add two fields:
* _code_challenge_
* _code_challenge_method_ (optional).

Code verifier is a 43-128 length random string consisting of [A-Z] / [a-z] / [0-9] / "-" / "." / "_" / "~". Code challenge is derived from the code verifier depending on the challenge method. Two types are supported, 's256' and 'plain'. Plain is just code_challenge = code_verifier. s256 method uses SHA256 to hash the code verifier and then do a base64 encoding of the resulting hash. e.g.

code_verifier = 052edd3941bb8040ecac75d2359d7cd1abe2518911b<br>
code_challenge = base64( sha256( code_verifier ) ) = MmNmZTJlNGZhYmNmYzQ3YTI4MmRhY2Q1NGEwZDUzZTFiZGFhNTNlODI4MGY3NjM0YWUwNjA1YjYzMmQwNDMxNQ==<br>
code_challenge_method = s256
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be wrapped in a code block. (Actually, we should eventually move into the proper docs, but that can happen later.)


In the next step, when using the code received from the server to obtain an access token, code_verifier must be passed in as an additional field to the request, and it must be using the code_verifier value that was used to calculate the code_challenge in the initial request.

## CLI Commands

### PKCE

A custom WP CLI helper command to generate a random code verifier and a code challenge.

```wp oauth2 generate-code-challenge```

## Warning

Expand Down
4 changes: 2 additions & 2 deletions inc/class-client.php
Original file line number Diff line number Diff line change
Expand Up @@ -229,8 +229,8 @@ public function check_redirect_uri( $uri ) {
*
* @return Authorization_Code|WP_Error
*/
public function generate_authorization_code( WP_User $user ) {
return Authorization_Code::create( $this, $user );
public function generate_authorization_code( WP_User $user, $data ) {
return Authorization_Code::create( $this, $user, $data );
}

/**
Expand Down
7 changes: 6 additions & 1 deletion inc/endpoints/class-token.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@ public function register_routes() {
'type' => 'string',
'validate_callback' => 'rest_validate_request_arg',
],
'code_verifier' => [
'required' => false,
'type' => 'string',
'validate_callback' => 'rest_validate_request_arg',
],
],
] );
}
Expand Down Expand Up @@ -71,7 +76,7 @@ public function exchange_token( WP_REST_Request $request ) {
return $auth_code;
}

$is_valid = $auth_code->validate();
$is_valid = $auth_code->validate( $request );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather pass the args in separately here to avoid having the validate() method depend on the request parameter names.

if ( is_wp_error( $is_valid ) ) {
// Invalid request, but code itself exists, so we should delete
// (and silently ignore errors).
Expand Down
6 changes: 6 additions & 0 deletions inc/namespace.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,12 @@ function bootstrap() {
// Admin-related.
add_action( 'init', __NAMESPACE__ . '\\rest_oauth2_load_authorize_page' );
add_action( 'admin_menu', __NAMESPACE__ . '\\Admin\\register' );

// WP-Cli
if ( class_exists( __NAMESPACE__ . '\\Utilities\\Oauth2_Wp_Cli' ) ) {
\WP_CLI::add_command( 'oauth2', __NAMESPACE__ . '\\Utilities\\Oauth2_Wp_Cli' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WP_CLI should be used at the top of the file instead of an absolute reference.

}

Admin\Profile\bootstrap();
}

Expand Down
46 changes: 42 additions & 4 deletions inc/tokens/class-authorization-code.php
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,36 @@ public function get_expiration() {
return (int) $value['expiration'];
}

private function validate_code_verifier( $args ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be protected, not private

$value = $this->get_value();

if ( empty( $value['code_challenge'] ) ) {
return true;
}

$code_verifier = $args['code_verifier'];
$is_valid = false;

switch ( strtolower( $value['code_challenge_method'] ) ) {
case 's256':
$decoded = base64_encode( hash( 'sha256', $code_verifier ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be $encoded instead?

$is_valid = $decoded === $value['code_challenge'];
break;
case 'plain':
$is_valid = $code_verifier === $value['code_challenge'];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both this equality check and the one above should use hash_equals() to ensure constant-time string comparison (to avoid timing attacks).

break;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add a default case for unsupported algorithms; §4.4.1 states that the description SHOULD disambiguate this case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"If the server supporting PKCE does not support the requested
transformation, the authorization endpoint MUST return the
authorization error response with "error" value set to
"invalid_request"" - inc/types/class-base/php line 192 - the challenge_method cannot be anything else than plain | s256. However, I edited the error message according to the standard and added a default throwing an exception nonetheless.

}

if ( ! $is_valid ) {
return new WP_Error(
'oauth2.tokens.authorization_code.validate_code_verifier.invalid_grant',
__( 'Invalid code verifier.', 'oauth2' )
);
}

return true;
}

/**
* Validate the code for use.
*
Expand All @@ -129,7 +159,15 @@ public function validate( $args = [] ) {
);
}

return true;
$code_verifier = $this->validate_code_verifier( [
'code_verifier' => $args['code_verifier'],
]
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatting is a little funky here; should be

$code_verifier = $this->validate_code_verifier( [
	'code_verifier' => ...,
] );

if ( is_wp_error( $code_verifier ) ) {
return $code_verifier;
}

return $code_verifier;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The if isn't necessary, as we're returning the value of $code_verifier in both cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather just return true at the bottom and keep if if, if there are any validations to add later it's easier. Is that fine?

}

/**
Expand Down Expand Up @@ -183,13 +221,13 @@ public static function get_by_code( Client $client, $code ) {
*
* @return Authorization_Code|WP_Error Authorization code instance, or error on failure.
*/
public static function create( Client $client, WP_User $user ) {
public static function create( Client $client, WP_User $user, $data ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$data should be documented here.

$code = wp_generate_password( static::KEY_LENGTH, false );
$meta_key = static::KEY_PREFIX . $code;
$data = [
$data = \array_merge( [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial \ is unnecessary here.

'user' => (int) $user->ID,
'expiration' => time() + static::MAX_AGE,
];
], $data );
$result = add_post_meta( $client->get_post_id(), wp_slash( $meta_key ), wp_slash( $data ), true );
if ( ! $result ) {
return new WP_Error(
Expand Down
2 changes: 1 addition & 1 deletion inc/types/class-authorization-code.php
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ protected function handle_authorization_submission( $submit, Client $client, $da
case 'authorize':
// Generate authorization code and redirect back.
$user = wp_get_current_user();
$code = $client->generate_authorization_code( $user );
$code = $client->generate_authorization_code( $user, $data );
if ( is_wp_error( $code ) ) {
return $code;
}
Expand Down
65 changes: 59 additions & 6 deletions inc/types/class-base.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
use WP\OAuth2\Client;

abstract class Base implements Type {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be here :)

/**
* Handle submission of authorisation page.
*
Expand All @@ -25,6 +26,8 @@ abstract protected function handle_authorization_submission( $submit, Client $cl
* Handle authorisation page.
*/
public function handle_authorisation() {
// Should probably keep this as an option in the application (e.g. force PKCE)
$pkce = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is a todo, we should file as an issue for later follow-up.


if ( empty( $_GET['client_id'] ) ) {
return new WP_Error(
Expand All @@ -34,10 +37,20 @@ public function handle_authorisation() {
}

// Gather parameters.
$client_id = wp_unslash( $_GET['client_id'] );
$redirect_uri = isset( $_GET['redirect_uri'] ) ? wp_unslash( $_GET['redirect_uri'] ) : null;
$scope = isset( $_GET['scope'] ) ? wp_unslash( $_GET['scope'] ) : null;
$state = isset( $_GET['state'] ) ? wp_unslash( $_GET['state'] ) : null;
$client_id = wp_unslash( $_GET['client_id'] );
$redirect_uri = isset( $_GET['redirect_uri'] ) ? wp_unslash( $_GET['redirect_uri'] ) : null;
$scope = isset( $_GET['scope'] ) ? wp_unslash( $_GET['scope'] ) : null;
$state = isset( $_GET['state'] ) ? wp_unslash( $_GET['state'] ) : null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alignment is off here.


if ( $pkce ) {
$pkce_data = $this->handle_pkce();
if ( is_wp_error( $pkce_data ) ) {
return $pkce_data;
}

$code_challenge = $pkce_data['code_challenge'];
$code_challenge_method = $pkce_data['code_challenge_method'];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could just merge $pkce_data into $data later on?

}

$client = Client::get_by_id( $client_id );
if ( empty( $client ) ) {
Expand Down Expand Up @@ -70,7 +83,7 @@ public function handle_authorisation() {

// Check nonce.
$nonce_action = $this->get_nonce_action( $client );
if ( ! wp_verify_nonce( wp_unslash( $_POST['_wpnonce'] ), $none_action ) ) {
if ( ! wp_verify_nonce( wp_unslash( $_POST['_wpnonce'] ), $this->get_nonce_action( $client ) ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use $nonce_action just above (and ouch, that misspelling!)

return new WP_Error(
'oauth2.types.authorization_code.handle_authorisation.invalid_nonce',
__( 'Invalid nonce.', 'oauth2' )
Expand All @@ -93,7 +106,7 @@ public function handle_authorisation() {

$submit = wp_unslash( $_POST['wp-submit'] );

$data = compact( 'redirect_uri', 'scope', 'state' );
$data = compact( 'redirect_uri', 'scope', 'state', 'code_challenge', 'code_challenge_method' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should have default values (or just merge $pkce_data if it's set)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest of the code assumes if there is no code_challenge set, PKCE is not being used. If PKCE is forced, it will throw an error before (just added that). I'll merge $pkce_data if set.

return $this->handle_authorization_submission( $submit, $client, $data );
}

Expand Down Expand Up @@ -152,4 +165,44 @@ protected function render_form( Client $client, WP_Error $errors = null ) {
protected function get_nonce_action( Client $client ) {
return sprintf( 'oauth2_authorize:%s', $client->get_id() );
}

/**
* Get and validate PKCE parameters from a request.
*
* @return string[] code_challenge and code_challenge_method
*/
private function handle_pkce() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

protected

$code_challenge = isset( $_GET['code_challenge'] ) ? wp_unslash( $_GET['code_challenge'] ) : null;
$code_challenge_method = isset( $_GET['code_challenge_method'] ) ? wp_unslash( $_GET['code_challenge_method'] ) : null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we instead pass wp_unslash( $_GET ) as a parameter into this function to separate the globals out?


if ( ! is_null( $code_challenge ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be inverted to return early instead.

if ( '' === \trim( $code_challenge ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary \

return new WP_Error(
'oauth2.types.authorization_code.handle_authorisation.code_challenge_empty',
sprintf( __( 'Code challenge cannot be empty', 'oauth2' ), $client_id ),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary sprintf, we're not using $client_id in the text here.

[
'status' => WP_Http::BAD_REQUEST,
'client_id' => $client_id,
]
);
}

$code_challenge_method = is_null( $code_challenge_method ) ? 'plain' : $code_challenge_method;

if ( ! \in_array( \strtolower( $code_challenge_method ), [ 'plain', 's256' ], true ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary \s here

return new WP_Error(
'oauth2.types.authorization_code.handle_authorisation.wrong_challenge_method',
sprintf( __( 'Challenge method must be S256 or plain', 'oauth2' ), $client_id ),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary sprintf

[
'status' => WP_Http::BAD_REQUEST,
'client_id' => $client_id,
]
);
}

return [ 'code_challenge' => $code_challenge, 'code_challenge_method' => $code_challenge_method ];
}

return false;
}
}
67 changes: 67 additions & 0 deletions inc/utilities/class-oauth2-wp-cli.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
<?php

namespace WP\OAuth2\Utilities;

class Oauth2_Wp_Cli extends \WP_CLI_Command {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class should just be called something like Command instead, no need to prefix inside the namespace. Also, WP_CLI_Command should be use'd at the top of the file rather than using an absolute reference.

/**
* Generate a code challenge.
*
* ## OPTIONS
*
* [<random_string>]
* : The string to be hashed.
*
*
* [--length=<length>]
* : The length of the random seed string.
* ---
* default: 64
* ---
*
* ## EXAMPLES
*
* wp oauth2 generate-code-challenge --length=64
*
* @alias generate-code-challenge
*/
function generate_code_challenge( $args, $assoc_args ) {
if ( ! empty( $args[0] ) && ! empty( $assoc_args['length'] ) ) {
\WP_CLI::warning( 'Length parameter will be ignored since the input string was provided.' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WP_CLI should be used at the top of the file rather than absolute referenced

}

$length = empty( $assoc_args['length'] ) ? 64 : intval( $assoc_args['length'] );

if ( $length < 43 || $length > 128 ) {
\WP_CLI::error( 'Length should be >= 43 and <= 128.' );
}

if ( ! empty( $args[0] ) ) {
$random_seed = $args[0];

if ( strlen( $random_seed ) < 43 || strlen( $random_seed ) > 128 ) {
\WP_CLI::error( 'Length of the provided random seed should be >= 43 and <= 128.' );
}

\WP_CLI::warning( "Using provided string {$random_seed} as a random seed. It is recommended to use this command without parameters, 64 characters long random key will be generated automatically." );
} else {
$is_strong_crypto = true;
$random_seed = \bin2hex( \openssl_random_pseudo_bytes( $length / 2 + $length % 2, $is_strong_crypto ) );
$random_seed = \substr( $random_seed, 0, $length );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The \s here are unnecessary.


if ( ! $is_strong_crypto ) {
\WP_CLI::error( 'openssl_random_pseudo_bytes failed to generate a cryptographically strong random string.' );
}
}

$code_challenge = \base64_encode( hash( 'sha256', $random_seed ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary \


$items = [
[
'code_verifier' => $random_seed,
'code_challenge = base64( sha256( code_verifier ) )' => $code_challenge,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep the title a little shorter, but not sure what this actually looks like in practice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The keys are longer anyways

],
];

\WP_CLI\Utils\format_items( 'table', $items, [ 'code_verifier', 'code_challenge = base64( sha256( code_verifier ) )' ] );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use WP_CLI\Utils here too.

}
}
4 changes: 4 additions & 0 deletions plugin.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,8 @@
require __DIR__ . '/inc/admin/namespace.php';
require __DIR__ . '/inc/admin/profile/namespace.php';

if ( defined( 'WP_CLI' ) && WP_CLI ) {
require __DIR__ . '/inc/utilities/class-oauth2-wp-cli.php';
}

bootstrap();