Skip to content

Added PKCE support - #46

Open
almirbi wants to merge 3 commits into
mainfrom
pkce-support
Open

Added PKCE support#46
almirbi wants to merge 3 commits into
mainfrom
pkce-support

Conversation

@almirbi

@almirbi almirbi commented Sep 5, 2017

Copy link
Copy Markdown
Collaborator

#18

@almirbi
almirbi requested a review from rmccue September 5, 2017 20:34
rmccue
rmccue requested changes Sep 9, 2017
View reviewed changes

@rmccue rmccue left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Phew, massive review, but looking really good overall. Thanks so much for this PR!

Mostly minor things to fix up here; if you don't have time for them, I can follow them up instead.

Haven't tested this for functionality or against the spec yet, but will do so soonish.

Comment thread README.md

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

@rmccue rmccue Sep 9, 2017

Copy link
Copy Markdown
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.)

Comment thread inc/endpoints/class-token.php Outdated
}

$is_valid = $auth_code->validate();
$is_valid = $auth_code->validate( $request );

@rmccue rmccue Sep 9, 2017

Copy link
Copy Markdown
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.

Comment thread inc/tokens/class-authorization-code.php Outdated
return (int) $value['expiration'];
}

private function validate_code_verifier( $args ) {

@rmccue rmccue Sep 9, 2017

Copy link
Copy Markdown
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

Comment thread inc/tokens/class-authorization-code.php Outdated
$is_valid = $decoded === $value['code_challenge'];
break;
case 'plain':
$is_valid = $code_verifier === $value['code_challenge'];

@rmccue rmccue Sep 9, 2017

Copy link
Copy Markdown
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).

Comment thread inc/tokens/class-authorization-code.php Outdated

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

@rmccue rmccue Sep 9, 2017

Copy link
Copy Markdown
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?

Comment thread inc/utilities/class-oauth2-wp-cli.php
} 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 );

@rmccue rmccue Sep 9, 2017

Copy link
Copy Markdown
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.

Comment thread inc/utilities/class-oauth2-wp-cli.php Outdated
],
];

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

@rmccue rmccue Sep 9, 2017

Copy link
Copy Markdown
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.

Comment thread inc/utilities/class-oauth2-wp-cli.php Outdated
}
}

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

@rmccue rmccue Sep 9, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Unnecessary \

Comment thread inc/utilities/class-oauth2-wp-cli.php
$items = [
[
'code_verifier' => $random_seed,
'code_challenge = base64( sha256( code_verifier ) )' => $code_challenge,

@rmccue rmccue Sep 9, 2017

Copy link
Copy Markdown
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.

@almirbi almirbi Sep 16, 2017

Copy link
Copy Markdown
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

Comment thread inc/namespace.php Outdated

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

@rmccue rmccue Sep 9, 2017

Copy link
Copy Markdown
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.

@roborourke roborourke mentioned this pull request Aug 31, 2026
Open
@roborourke

roborourke commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Reviewed this against RFC 7636. The S256 transform has a bug that breaks interop: base64_encode( hash( 'sha256', $code_verifier ) ) base64-encodes the hex digest with standard base64, giving an 88-character value. RFC 7636 §4.2 wants base64url (unpadded) of the raw 32 bytes, 43 characters. Checked against the RFC's own Appendix B vector:

code_verifier  = dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk
code_challenge = E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM   (correct, RFC 7636 §4.2)

The fix is rtrim( strtr( base64_encode( hash( 'sha256', $verifier, true ) ), '+/', '-_' ), '=' ) — note the true for raw binary output. No client following the RFC could complete a flow against this as written. The charset regex allowing a literal space is a symptom of the same bug (it's what standard base64's + decodes to in a query string).

A few smaller things:

  • PKCE lives in Types\Base, so it also applies to the implicit grant, which mints no code for a challenge to bind to — worth scoping to the authorization_code type only.
  • The 43–128 length/charset check (RFC 7636 §4.1) is applied to code_challenge, but that's the verifier's ABNF, and the verifier itself is never validated.
  • handle_pkce() references $client_id, which isn't defined in that method, and passes it as WP_Error's third constructor argument — but that's $data, not a fourth positional arg, so the intended status/client_id error data is silently dropped on all four error paths.

We've reimplemented this with the above fixed (plus PKCE-required-per-client, RFC 7636 §4.4.1 redirect-based errors, and tests against the RFC vector) on our fork: humanmade#1. Happy for any of it to be pulled back here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rmccue rmccue rmccue requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@almirbi @roborourke @rmccue