Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Improve OIDC Compliance #734

Merged
merged 6 commits into from
Dec 2, 2019
Merged

Improve OIDC Compliance #734

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
89c0f5c
Improve OIDC Compliance
joshcanhelp Nov 23, 2019
993bf05
Mandatory nonce; max_age handing
joshcanhelp Nov 25, 2019
79eac9e
Ignore code coverage on PHP-SDK sourced classes
joshcanhelp Nov 25, 2019
06e1d3b
Add absint to max_age filter output
joshcanhelp Nov 25, 2019
5f767be
Decode token return type change
joshcanhelp Nov 25, 2019
6c8bc4b
More clear logic for expected token alg
joshcanhelp Dec 2, 2019
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
7 changes: 2 additions & 5 deletions .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,8 @@ commands:
run-formatting:
steps:
- run:
name: Running Linter and Formatting
name: Run i18n check and PHP compatibility
command: |
composer phpcbf
composer phpcbf-tests
composer phpcs-tests
composer phpcs-i18n
composer compat
run-tests:
Expand All @@ -55,7 +52,7 @@ commands:
jobs:
php_7:
docker:
- image: circleci/php:7.1
- image: circleci/php:7.0
- image: circleci/mysql:5.6
environment:
MYSQL_ALLOW_EMPTY_PASSWORD: true
Expand Down
12 changes: 2 additions & 10 deletions WP_Auth0.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@

define( 'WPA0_LANG', 'wp-auth0' ); // deprecated; do not use for translations

require_once 'vendor/autoload.php';
require_once 'functions.php';

/*
Expand Down Expand Up @@ -434,16 +435,6 @@ public static function uninstall() {
private function autoloader( $class ) {
$source_dir = WPA0_PLUGIN_DIR . 'lib/';

// Catch non-name-spaced classes that still need auto-loading.
switch ( $class ) {
case 'JWT':
case 'BeforeValidException':
case 'ExpiredException':
case 'SignatureInvalidException':
require_once $source_dir . 'php-jwt/' . $class . '.php';
return true;
}

// Anything that's not part of the above and not name-spaced can be skipped.
if ( 0 !== strpos( $class, 'WP_Auth0' ) ) {
return false;
Expand All @@ -457,6 +448,7 @@ private function autoloader( $class ) {
$source_dir . 'profile/',
$source_dir . 'wizard/',
$source_dir . 'initial-setup/',
$source_dir . 'token-verifier/',
];

foreach ( $paths as $path ) {
Expand Down
3 changes: 2 additions & 1 deletion composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@
"homepage": "https://auth0.com/wordpress",
"license": "GPLv2",
"require": {
"php": "^7.0"
"php": "^7.0",
"lcobucci/jwt": "^3.3.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previous library was not managed with Composer.

},
"require-dev": {
"dealerdirect/phpcodesniffer-composer-installer": "^0.5",
Expand Down
29 changes: 28 additions & 1 deletion functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
*/
function wp_auth0_generate_token() {
$token = WP_Auth0_Nonce_Handler::get_instance()->generate_unique( 64 );
return JWT::urlsafeB64Encode( $token );
return wp_auth0_url_base64_encode( $token );
}

/**
Expand Down Expand Up @@ -117,6 +117,33 @@ function wp_auth0_can_show_wp_login_form() {
return false;
}

/**
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both of these functions were pulled from the old JWT library.

* @param $input
*
* @return mixed
*
* @see https://github.com/firebase/php-jwt/blob/v5.0.0/src/JWT.php#L337
*/
function wp_auth0_url_base64_encode( $input ) {
return str_replace( '=', '', strtr( base64_encode( $input ), '+/', '-_' ) );
}

/**
* @param $input
*
* @return bool|string
*
* @see https://github.com/firebase/php-jwt/blob/v5.0.0/src/JWT.php#L320
*/
function wp_auth0_url_base64_decode( $input ) {
$remainder = strlen( $input ) % 4;
if ( $remainder ) {
$padlen = 4 - $remainder;
$input .= str_repeat( '=', $padlen );
}
return base64_decode( strtr( $input, '-_', '+/' ) );
}

if ( ! function_exists( 'get_auth0userinfo' ) ) {
/**
* Get the Auth0 profile from the database, if one exists.
Expand Down
62 changes: 0 additions & 62 deletions lib/WP_Auth0_Api_Client.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,68 +446,6 @@ public static function ConsentRequiredScopes() {
];
}

/**
* Convert a certificate to PEM format.
*
* @param string $cert - Certificate, like from .well-known/jwks.json.
*
* @return string
*/
protected static function convertCertToPem( $cert ) {
return '-----BEGIN CERTIFICATE-----' . PHP_EOL
. chunk_split( $cert, 64, PHP_EOL )
. '-----END CERTIFICATE-----' . PHP_EOL;
}

/**
* Get and cache a JWKS.
*
* @param string $domain - Issuer domain.
*
* @return array|bool|mixed
*/
public static function JWKfetch( $domain ) {

$a0_options = WP_Auth0_Options::Instance();

$endpoint = "https://$domain/.well-known/jwks.json";

if ( false === ( $secret = get_transient( WPA0_JWKS_CACHE_TRANSIENT_NAME ) ) ) {

$secret = [];

$response = wp_remote_get( $endpoint, [] );

if ( $response instanceof WP_Error ) {
WP_Auth0_ErrorManager::insert_auth0_error( __METHOD__, $response );
error_log( $response->get_error_message() );
return false;
}

if ( $response['response']['code'] != 200 ) {
WP_Auth0_ErrorManager::insert_auth0_error( __METHOD__, $response['body'] );
error_log( $response['body'] );
return false;
}

if ( $response['response']['code'] >= 300 ) {
return false;
}

$jwks = json_decode( $response['body'], true );

foreach ( $jwks['keys'] as $key ) {
$secret[ $key['kid'] ] = self::convertCertToPem( $key['x5c'][0] );
}

if ( $cache_expiration = $a0_options->get( 'cache_expiration' ) ) {
set_transient( WPA0_JWKS_CACHE_TRANSIENT_NAME, $secret, $cache_expiration * MINUTE_IN_SECONDS );
}
}

return $secret;
}

/**
* Return the grant types needed for new clients.
*
Expand Down
107 changes: 0 additions & 107 deletions lib/WP_Auth0_Id_Token_Validator.php
View file
Open in desktop

This file was deleted.

52 changes: 34 additions & 18 deletions lib/WP_Auth0_LoginManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,7 @@ public function login_auto() {
$auth_params = self::get_authorize_params( $connection );

WP_Auth0_State_Handler::get_instance()->set_cookie( $auth_params['state'] );

if ( isset( $auth_params['nonce'] ) ) {
WP_Auth0_Nonce_Handler::get_instance()->set_cookie();
}
WP_Auth0_Nonce_Handler::get_instance()->set_cookie( $auth_params['nonce'] );

$auth_url = self::build_authorize_url( $auth_params );
wp_redirect( $auth_url );
Expand Down Expand Up @@ -174,8 +171,7 @@ public function redirect_login() {
$refresh_token = isset( $data->refresh_token ) ? $data->refresh_token : null;

// Decode the incoming ID token for the Auth0 user.
$jwt_verifier = new WP_Auth0_Id_Token_Validator( $id_token, $this->a0_options );
$decoded_token = $jwt_verifier->decode();
$decoded_token = $this->decode_id_token( $id_token );

// Attempt to authenticate with the Management API, if allowed.
$userinfo = null;
Expand Down Expand Up @@ -232,8 +228,7 @@ public function implicit_login() {
$id_token_param = ! empty( $_POST['id_token'] ) ? $_POST['id_token'] : $_POST['token'];
$id_token = sanitize_text_field( wp_unslash( $id_token_param ) );

$jwt_verifier = new WP_Auth0_Id_Token_Validator( $id_token, $this->a0_options );
$decoded_token = $jwt_verifier->decode( true );
$decoded_token = $this->decode_id_token( $id_token );
$decoded_token = $this->clean_id_token( $decoded_token );

if ( $this->login_user( $decoded_token, $id_token ) ) {
Expand Down Expand Up @@ -460,24 +455,18 @@ public static function get_authorize_params( $connection = null, $redirect_to =
$opts = WP_Auth0_Options::Instance();
$lock_options = new WP_Auth0_Lock10_Options();
$is_implicit = (bool) $opts->get( 'auth0_implicit_workflow', false );
$nonce = WP_Auth0_Nonce_Handler::get_instance()->get_unique();

$params = [
'connection' => $connection,
'client_id' => $opts->get( 'client_id' ),
'scope' => self::get_userinfo_scope( 'authorize_url' ),
'nonce' => WP_Auth0_Nonce_Handler::get_instance()->get_unique(),
'max_age' => absint( apply_filters( 'auth0_jwt_max_age', null ) ),
'response_type' => $is_implicit ? 'id_token' : 'code',
'response_mode' => $is_implicit ? 'form_post' : 'query',
'redirect_uri' => $is_implicit ? $lock_options->get_implicit_callback_url() : $opts->get_wp_auth0_url(),
];

if ( $is_implicit ) {
$params['nonce'] = $nonce;
}

if ( ! empty( $connection ) ) {
$params['connection'] = $connection;
}

// Where should the user be redirected after logging in?
if ( empty( $redirect_to ) ) {
$redirect_to = empty( $_GET['redirect_to'] )
Expand All @@ -498,7 +487,7 @@ public static function get_authorize_params( $connection = null, $redirect_to =
$filtered_params['state'] = base64_encode( json_encode( $filtered_state ) );
}

return $filtered_params;
return array_filter( $filtered_params );
}

/**
Expand Down Expand Up @@ -573,6 +562,33 @@ protected function die_on_login( $msg = '', $code = 0 ) {
wp_die( $html );
}

/**
* @param $id_token
* @return object
* @throws WP_Auth0_InvalidIdTokenException
*/
private function decode_id_token( $id_token ) {
$expectedIss = 'https://' . $this->a0_options->get( 'domain' ) . '/';
$expectedAlg = $this->a0_options->get( 'client_signing_algorithm' );
if ( 'RS256' === $expectedAlg ) {
$jwks = ( new WP_Auth0_JwksFetcher() )->getKeys();
$sigVerifier = new WP_Auth0_AsymmetricVerifier( $jwks );
} elseif ( 'HS256' === $expectedAlg ) {
$sigVerifier = new WP_Auth0_SymmetricVerifier( $this->a0_options->get( 'client_secret' ) );
} else {
throw new WP_Auth0_InvalidIdTokenException( 'Signing algorithm of "' . $expectedAlg . '" is not supported.' );
}
Copy link
Contributor

Choose a reason for hiding this comment

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

"else" scenario????

Also, you're doing $this->a0_options->get( 'client_signing_algorithm' ) twice. Seems like it can be extracted. Does it have performance impact?


$verifierOptions = [
'nonce' => WP_Auth0_Nonce_Handler::get_instance()->get_once(),
'leeway' => absint( apply_filters( 'auth0_jwt_leeway', null ) ),
'max_age' => absint( apply_filters( 'auth0_jwt_max_age', null ) ),
];

$idTokenVerifier = new WP_Auth0_IdTokenVerifier( $expectedIss, $this->a0_options->get( 'client_id' ), $sigVerifier );
return (object) $idTokenVerifier->verify( $id_token, $verifierOptions );
}

/**
* Remove unnecessary ID token properties.
*
Expand Down
Loading