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.

Sign up for GitHub

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

Jump to bottom

Improve OIDC Compliance #734

Merged

joshcanhelp merged 6 commits into master from improved-oidc-compliance

Dec 2, 2019

Contributor

joshcanhelp commented Nov 23, 2019 •

edited

Changes

This update improves the SDK support for OpenID Connect. In particular, it modifies the sign in verification phase by substituting backchannel based checks with id_token validation.

Note to reviewers: This re-uses the majority of the PHP SDK token verifier work. Please see this draft PR to review the differences between the core verification classes. ~540 added lines are represented in that diff.

References

Testing

Manual testing:

In wp-admin > Auth0 > Settings, turn Implicit login flow off in settings and universal login page on.
Logout then log back in.
Make sure the redirect response from WP includes both state and nonce cookies:

Make sure the /authorize URL has all the expected params:

Log in at Auth0
Make sure you're logged in with no errors in the plugin error log
Make sure that the nonce and state cookies are deleted on the callback response:

I included manual testing steps above, if applicable
This change adds unit test coverage
This change has been tested on WP 5.3 and PHP 7.0

Checklist

All existing and new tests complete without errors
All code quality tools/guidelines in the Contribution guide have been run/followed
All relevant assets have been compiled as directed in the Contribution guide, if applicable
All active GitHub CI checks have passed


          Improve OIDC Compliance

89c0f5c

joshcanhelp commented

View reviewed changes

functions.php

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

Contributor Author

joshcanhelp Nov 25, 2019

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

joshcanhelp commented

View reviewed changes

composer.json

@@ @@ -4,7 +4,8 @@ @@
                   "homepage": "https://auth0.com/wordpress",
                   "license": "GPLv2",
                   "require": {
-                      "php": "^7.0"
+                      "php": "^7.0",
+                      "lcobucci/jwt": "^3.3.0"

Contributor Author

joshcanhelp Nov 25, 2019

Previous library was not managed with Composer.

joshcanhelp commented

View reviewed changes

lib/token-verifier/WP_Auth0_AsymmetricVerifier.php

		@@ -0,0 +1,54 @@
		<?php

Contributor Author

joshcanhelp Nov 25, 2019

See this diff for changes from approved PHP SDK ones.

joshcanhelp commented

View reviewed changes

lib/token-verifier/WP_Auth0_IdTokenVerifier.php

		@@ -0,0 +1,265 @@
		<?php

Contributor Author

joshcanhelp Nov 25, 2019

See this diff for changes from approved PHP SDK ones.

joshcanhelp commented

View reviewed changes

lib/token-verifier/WP_Auth0_JwksFetcher.php

		@@ -0,0 +1,76 @@
		<?php

Contributor Author

joshcanhelp Nov 25, 2019

See this diff for changes from approved PHP SDK ones.

joshcanhelp commented

View reviewed changes

lib/token-verifier/WP_Auth0_SignatureVerifier.php

		@@ -0,0 +1,87 @@
		<?php

Contributor Author

joshcanhelp Nov 25, 2019

See this diff for changes from approved PHP SDK ones.

joshcanhelp commented

View reviewed changes

lib/token-verifier/WP_Auth0_SymmetricVerifier.php

		@@ -0,0 +1,55 @@
		<?php

Contributor Author

joshcanhelp Nov 25, 2019

See this diff for changes from approved PHP SDK ones.

joshcanhelp added CH: Changed labels

joshcanhelp added this to the 4.0.0 milestone

joshcanhelp added review:medium and removed review:large labels

joshcanhelp added 3 commits

November 25, 2019 12:57


          Mandatory nonce; max_age handing

993bf05


          Ignore code coverage on PHP-SDK sourced classes

79eac9e


          Add absint to max_age filter output

06e1d3b

joshcanhelp added review:large and removed review:medium labels


          Decode token return type change

5f767be

joshcanhelp marked this pull request as ready for review

November 25, 2019 21:33

joshcanhelp requested a review from a team

November 25, 2019 21:33

joshcanhelp mentioned this pull request

Remove implicit login flow #735

Merged

5 tasks

lbalmaceda suggested changes

View reviewed changes

Contributor

lbalmaceda left a comment

Looks good in general. Please pay attention to the claim error messages. Should be wrapping the expected/received values between ( ) rather than " ". I see you're mixing them.

.circleci/config.yml Show resolved Hide resolved

lib/WP_Auth0_LoginManager.php Show resolved Hide resolved

lib/WP_Auth0_LoginManager.php

+              			$sigVerifier = new WP_Auth0_AsymmetricVerifier( $jwks );
+              		} elseif ( 'HS256' === $this->a0_options->get( 'client_signing_algorithm' ) ) {
+              			$sigVerifier = new WP_Auth0_SymmetricVerifier( $this->a0_options->get( 'client_secret' ) );
+              		}

Contributor

lbalmaceda Nov 28, 2019

"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?

lib/WP_Auth0_LoginManager.php Show resolved Hide resolved

lib/admin/WP_Auth0_Admin_Advanced.php Show resolved Hide resolved

lib/token-verifier/WP_Auth0_AsymmetricVerifier.php Outdated

+              	protected function checkSignature( Token $token ) : bool {
+              		$tokenKid = $token->getHeader( 'kid', false );
+              		if ( ! array_key_exists( $tokenKid, $this->jwks ) ) {
+              			throw new WP_Auth0_InvalidIdTokenException( 'ID token key ID "' . $tokenKid . '" was not found in the JWKS' );

Contributor

lbalmaceda Nov 28, 2019

Should be

Could not find a public key for Key ID (kid) "${decoded.header.kid}"

tests/testLoginManagerAuthParams.php Show resolved Hide resolved


          More clear logic for expected token alg

6c8bc4b

joshcanhelp requested a review from lbalmaceda

December 2, 2019 21:24

Contributor Author

joshcanhelp commented Dec 2, 2019

Please pay attention to the claim error messages. Should be wrapping the expected/received values between ( ) rather than " ". I see you're mixing them.

It looks like that was mapped over from the PHP SDK. We'll extract this out into it's own library in Phase 2 so this is consistent everywhere.

lbalmaceda approved these changes

View reviewed changes

joshcanhelp merged commit 1381fb1 into master

joshcanhelp deleted the improved-oidc-compliance branch

December 16, 2019 19:00

github-actions bot locked as resolved and limited conversation to collaborators

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