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

Enable PKCE extension in GDS OmniAuth Strategy #283

Merged
merged 1 commit into from
Aug 24, 2023
Merged

Enable PKCE extension in GDS OmniAuth Strategy #283

merged 1 commit into from
Aug 24, 2023

Conversation

chrisroos
Copy link
Contributor

@chrisroos chrisroos commented Aug 16, 2023

https://trello.com/c/59EBweBx

In alphagov/signon#2312 we enabled the OAuth2 PKCE extension in Signon.

In this PR we update the GDS OAuth2 OmniAuth Strategy to make use of the PKCE extension. This means that any of our apps using this Gem will benefit from the additional protection offered by the PKCE extension.

Testing this change in development

Create an app in Signon representing Whitehall Publisher.

$ cd ~/govuk/signon
$ gdr rails c
irb> app = Doorkeeper::Application.create!(name: 'Whitehall dev', redirect_uri: 'http://whitehall-admin.dev.gov.uk/auth/gds/callback')

Give our test user permission to access Whitehall Publisher.

irb> app = Doorkeeper::Application.find_by(name: 'Whitehall dev')
irb> user = User.find_by(email: 'test.user@gov.uk')
irb> user.supported_permissions << app.supported_permissions

Set the following environment variables for the whitehall-app container in govuk-docker/projects/whitehall/docker-compose.yml, where <oauth-id> is app.uid and <oauth-secret> is app.secret.

$ vim ~/govuk/govuk-docker/projects/whitehall/docker-compose.yml

GDS_SSO_OAUTH_ID: "<oauth-id>"
GDS_SSO_OAUTH_SECRET: "<oauth-secret>"
GDS_SSO_STRATEGY: "real"

Modify Whitehall's Gemfile and install Gems:

-gem "gds-sso"
+gem "gds-sso", git: "https://github.com/alphagov/gds-sso.git", branch: "add-pkce"

$ gdr bundle

Now when you sign in to Signon and access Whitehall Publisher in development you'll be able to see the following PKCE related params in the Signon logs:

  • the request for /oauth/authorize includes a code_challenge and code_challenge_method in the querystring
  • the request for /oauth/access_token includes the code_verifier in the POSTed body

@chrisroos chrisroos force-pushed the add-pkce branch 4 times, most recently from a1ca5a4 to 91aca87 Compare August 23, 2023 11:29
@chrisroos chrisroos marked this pull request as ready for review August 23, 2023 11:39
@chrisroos chrisroos changed the title WIP: Add PKCE Enable PKCE extension in GDS OmniAuth Strategy Aug 23, 2023
@floehopper floehopper self-requested a review August 24, 2023 08:11
Copy link
Contributor

@floehopper floehopper left a comment

Choose a reason for hiding this comment

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

Overall this looks good to me. I've added a few minor comments inline, but I'm happy to leave it to your discretion on whether to address them.

spec/system/authentication_and_authorisation_spec.rb Outdated Show resolved Hide resolved
spec/system/authentication_and_authorisation_spec.rb Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
https://trello.com/c/59EBweBx

In alphagov/signon#2312 we enabled the OAuth2
PKCE extension[1] in Signon.

In this commit we update our GDS OAuth2 OmniAuth Strategy to make use of
the PKCE extension. This means that any of our apps using this Gem will
benefit from the additional protection offered by the PKCE extension.

[1]: https://datatracker.ietf.org/doc/html/rfc7636
@chrisroos
Copy link
Contributor Author

Thanks, @floehopper. I've addressed your feedback and force pushed. Merging now.

@chrisroos chrisroos merged commit 162b39e into main Aug 24, 2023
16 checks passed
@chrisroos chrisroos deleted the add-pkce branch August 24, 2023 09:26
chrisroos added a commit that referenced this pull request Aug 24, 2023
- Enable PKCE extension in GDS OmniAuth Strategy #283
@chrisroos chrisroos mentioned this pull request Aug 24, 2023
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.

2 participants