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.

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

Allow no nonce option #434

Merged
merged 8 commits into from
Apr 23, 2020
Merged

Allow no nonce option #434

merged 8 commits into from
Apr 23, 2020

Conversation

joshcanhelp
Copy link
Contributor

@joshcanhelp joshcanhelp commented Apr 21, 2020 •
edited
Loading

Description

  • Fix Auth0->renewTokens() to skip nonce checking for the ID token
  • Remove stored access token requirement from Auth0->renewTokens()
  • Remove id_token check from Auth0->renewTokens() response
  • Add TransientStoreHandler->isset() to check for stored transient value without getting/deleting it

References

Closes #432

Testing

647aaed - Fixes the renew token test to fail because of missing nonce, as it would (see issue above)

  • This change adds test coverage for new/changed/fixed functionality

joshcanhelp
joshcanhelp commented Apr 22, 2020
View reviewed changes
src/Auth0.php
@@ -564,10 +564,6 @@ public function exchange()
*/
public function renewTokens(array $options = [])
{
if (! $this->accessToken) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no good reason I can think of to keep this check.

joshcanhelp
joshcanhelp commented Apr 22, 2020
View reviewed changes
tests/Auth0Test.php
@@ -313,7 +294,7 @@ public function testThatRenewTokensSucceeds()

$mock = new MockHandler( [
// Code exchange response.
new Response( 200, self::$headers, '{"access_token":"1.2.3","refresh_token":"2.3.4"}' ),
new Response( 200, self::$headers, '{"access_token":"1.2.3","refresh_token":"2.3.4","id_token":"'.$id_token.'"}' ),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The refresh token response would also omit an ID token if the original code exchange did not include one. Leaving this out makes this test inaccurate for how the process would work in a real application. With this ID token added, the nonce is checked and removed from storage before the tokens can be renewed. This then causes the exception that was reported in #432

joshcanhelp
joshcanhelp commented Apr 22, 2020
View reviewed changes
src/Auth0.php Outdated
];

$verifierOptions[self::TRANSIENT_NONCE_KEY] = $this->transientHandler->getOnce(self::TRANSIENT_NONCE_KEY);
if (empty( $verifierOptions[self::TRANSIENT_NONCE_KEY] )) {
if (false !== $verifierOptions[self::TRANSIENT_NONCE_KEY] && empty( $verifierOptions[self::TRANSIENT_NONCE_KEY] )) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Setting the nonce key to false will now skip nonce checking in the ID token before setting.

@joshcanhelp joshcanhelp marked this pull request as ready for review April 22, 2020 03:55
@joshcanhelp joshcanhelp requested review from a team and lbalmaceda April 22, 2020 03:55
@joshcanhelp
Copy link
Contributor Author

@lbalmaceda - Adding you specifically since this is security-affecting.

stevehobbsdev
stevehobbsdev reviewed Apr 22, 2020
View reviewed changes
tests/Auth0Test.php Outdated Show resolved Hide resolved
joshcanhelp
joshcanhelp commented Apr 22, 2020
View reviewed changes
tests/Auth0Test.php Outdated Show resolved Hide resolved
lbalmaceda
lbalmaceda suggested changes Apr 22, 2020
View reviewed changes
src/Auth0.php Show resolved Hide resolved
src/Auth0.php Outdated
}

$this->setAccessToken($response['access_token']);

if (isset($response['id_token'])) {
$this->setIdToken($response['id_token']);
$this->setIdToken($response['id_token'], [self::TRANSIENT_NONCE_KEY => false]);
Copy link
Contributor

Choose a reason for hiding this comment

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

The outer world should probably not know about the verification options.

src/Auth0.php Show resolved Hide resolved
@jimmyjames jimmyjames mentioned this pull request Apr 22, 2020
Merged
lbalmaceda
lbalmaceda reviewed Apr 22, 2020
View reviewed changes
Copy link
Contributor

@lbalmaceda lbalmaceda left a comment

Choose a reason for hiding this comment

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

A few minor comments, nothing blocking. Ping me for the final approval

src/Auth0.php
'leeway' => $this->idTokenLeeway,
'max_age' => $this->transientHandler->getOnce('max_age') ?? $this->maxAge,
self::TRANSIENT_NONCE_KEY => $this->transientHandler->getOnce(self::TRANSIENT_NONCE_KEY)
Copy link
Contributor

Choose a reason for hiding this comment

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

It makes sense for the storage to know the value is stored under the name of self::TRANSIENT_NONCE_KEY. But this section on the left belongs to "verifierOptions". Do you need to use that name again or can you just use "nonce" here, knowing that that is what you are storing?

Suggested change
self::TRANSIENT_NONCE_KEY => $this->transientHandler->getOnce(self::TRANSIENT_NONCE_KEY)
'nonce' => $this->transientHandler->getOnce(self::TRANSIENT_NONCE_KEY)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That constant is just an enum so you don't have to remember the string.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's my point. For leeway and max_age you don't use any enum and still you're able to pick them up from the other side. It was a suggestion anyway 👍 the code works

tests/Auth0Test.php
* @throws ApiException
* @throws CoreException
*/
public function testThatExchangeFailsWithNoStoredNonce()
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this is the case because we don't allow to opt-out sending a nonce during authentication?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. Code exchange would only happen during login.

tests/Auth0Test.php
/**
* @throws CoreException
*/
public function testThatEmptyApplicationNonceFailsIdTokenValidation()
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this one be kept but with the condition/check inverted? e.g. id token validation when no nonce is stored should pass

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's handled in testThatRenewTokensSucceeds

@lbalmaceda lbalmaceda self-requested a review April 22, 2020 21:37
@joshcanhelp joshcanhelp added this to the 7.2.0 milestone Apr 23, 2020
@joshcanhelp joshcanhelp merged commit 660163b into master Apr 23, 2020
@joshcanhelp joshcanhelp deleted the allow-no-nonce-option branch April 23, 2020 00:20
@github-actions
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lbalmaceda lbalmaceda lbalmaceda approved these changes

@stevehobbsdev stevehobbsdev Awaiting requested review from stevehobbsdev

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
7.2.0
Development

Successfully merging this pull request may close these issues.

Renew Tokens throws nonce error
3 participants
@joshcanhelp @stevehobbsdev @lbalmaceda