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

Only refresh tokens should be allowed to reauthenticate #253

Merged
merged 4 commits into from Feb 27, 2023
Merged

Conversation

bbengfort
Copy link
Contributor

@bbengfort bbengfort commented Feb 25, 2023

Scope of changes

Adds another audience to refresh tokens that identifies them as refresh tokens and verifies them in the Refresh handler.

The basic fix is as follows, the aud claim in a JWT token can have multiple items. Access tokens have the audience claim [https://api.rotational.app] but refresh tokens now have the audience claim [https://api.rotational.app, https://auth.rotational.app/v1/refresh]. This means that the refresh tokens go through two checks; one that the audience claim https://api.rotational.app is good and then that https://auth.rotational.app/v1/refresh is in the token.

I've added a test that shows that valid access tokens are rejected since they don't have this audience.

Fixes SC-13078

Type of change

  • new feature
  • bug fix
  • documentation
  • testing
  • technical debt
  • other (describe)

Acceptance criteria

I'm worried about the security of this model. Should we add a check that requires the access token, possibly expired be in the Authorization header then match the IDs of the two claims before we refresh?

I kind of feel like we should; particularly if we put the access token in a Cookie that is http only and secure.

@masskoder @pdamodaran any thoughts on this?

Author checklist

  • I have manually tested the change and/or added automation in the form of unit tests or integration tests
  • I have updated the dependencies list
  • I have recompiled and included new protocol buffers to reflect changes I made
  • I have added new test fixtures as needed to support added tests
  • Check this box if a reviewer can merge this pull request after approval (leave it unchecked if you want to do it yourself)
  • I have moved the associated Shortcut story to "Ready for Review"

Reviewer(s) checklist

  • Any new user-facing content that has been added for this PR has been QA'ed to ensure correct grammar, spelling, and understandability.
  • Are there any TODOs in this PR that should be turned into stories?
  • Should we add another test ensuring that the new access/refresh token have the same claims, particularly the audience? Just to make sure that it doesn't accidentally change in the future?

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #13078: Only refresh tokens should be allowed to reauthenticate.

@bbengfort bbengfort marked this pull request as ready for review February 25, 2023 20:04
Copy link
Contributor

@pdamodaran pdamodaran left a comment

Choose a reason for hiding this comment

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

Looks good @bbengfort - I just added one suggested change for an additional check in the test.

pkg/quarterdeck/tokens/tokens_test.go Show resolved Hide resolved
bbengfort and others added 2 commits February 27, 2023 16:44
Co-authored-by: pdamodaran <pdamodaran@users.noreply.github.com>
@bbengfort bbengfort merged commit d861546 into main Feb 27, 2023
@bbengfort bbengfort deleted the sc-13078 branch February 27, 2023 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants