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

require POSTs during sign-in (CVE-2015-9284) #1815

Merged
merged 3 commits into from
Jun 17, 2019
Merged

Conversation

robbkidd
Copy link
Contributor

@robbkidd robbkidd commented Jun 14, 2019

Description

CVE-2015-9284 was a bit of a doozy. Essentially, a service provider (oauth "client") ought to accept authentication initiations by POST with a token from itself to protect against cross-site forgeries. Details so far on omniauth/omniauth issue 809.

This change does the following:

  1. upgrades omniauth and adds a new omniauth-rails_csrf_protection gem

The new gem twiddles with OmniAuth's middleware to require POSTing to
the auth endpoints it adds to the app and wires up checking for a valid
token during the middleware processing. The omniauth gem itself is
framework agnostic, so the current solution is a new Rails-flavored
library.

  1. updates all Supermarket's links to OAuth2 providers to use POST

Pretty easy update to link_to "buttons" in the views.

  1. display a page on /sign-in with a login button

Because the endpoint that actually signs a user is /auth/chef-oauth2, a
route and sessions controller action was made to redirect there for a
friendlier URL path. But you can't redirect POSTs, yo. So, now that
we're POSTing, if someone lands on /sign-in some how, display a button
for logging in via Chef Server. This can be the place where more buttons
are added if we ever decide to implement multi-auth.

new /sign-in page

  1. redirects to /sign-in changed from 301 to 302

Browsers take permanent redirects at their word and seem to never forget
them. Nothing is really permanent on the internet.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (non-breaking change that does not add functionality or fix an issue)

Checklist:

  • I have read the CONTRIBUTING document.
  • I have run the pre-merge tests locally and they pass.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commits have been signed-off for the Developer Certificate of Origin.

CVE-2015-9284 was a bit of a doozy. Essentially, a service provider
(oauth "client") ought to accept authentication initiations by POST with
a token from itself to protect against cross-site forgeries. Details so
far on omniauth/omniauth issue 809.

This change does the following:

1. upgrades omniauth and adds a new omniauth-rails_csrf_protection gem

The new gem twiddles with OmniAuth's middleware to require POSTing to
the auth endpoints it adds to the app and wires up checking for a valid
token during the middleware processing. The omniauth gem itself is
framework agnostic, so the current solution is a new Rails-flavored
library.

2. updates all Supermarket's links to OAuth2 providers to use POST

Pretty easy update to link_to "buttons" in the views.

3. display a page on /sign-in with a login button

Because the endpoint that actually signs a user is /auth/chef-oauth2, a
route and sessions controller action was made to redirect there for a
friendlier URL path. But you can't redirect POSTs, yo. So, now that
we're POSTing, if someone lands on /sign-in some how, display a button
for logging in via Chef Server. This can be the place where more buttons
are added if we ever decide to implement multi-auth.

4. redirects to /sign-in changed from 301 to 302

Browsers take permanent redirects at their word and seem to never forget
them.  Nothing is really permanent on the internet.

Signed-off-by: Robb Kidd <rkidd@chef.io>
Signed-off-by: Robb Kidd <rkidd@chef.io>
@robbkidd robbkidd requested a review from a team as a code owner June 14, 2019 21:48
Signed-off-by: Robb Kidd <rkidd@chef.io>
@robbkidd robbkidd added Aspect: Security Can an unwanted third party affect the stability or look at privileged information? Priority: Medium labels Jun 17, 2019
@robbkidd robbkidd merged commit 931881e into master Jun 17, 2019
@chef-ci chef-ci deleted the robb/auth-update branch June 17, 2019 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Aspect: Security Can an unwanted third party affect the stability or look at privileged information? Priority: Medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants