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

Make config.force_ssl less dangerous to try and easier to disable #21520

Merged
merged 1 commit into from Sep 8, 2015

Conversation

jeremy
Copy link
Member

@jeremy jeremy commented Sep 7, 2015

SSL redirect:

  • Move :host and :port options within redirect: { … }. Deprecate.
  • Introduce :status and :body to customize the redirect response.
    The 301 permanent default makes it difficult to test the redirect and
    back out of it since browsers remember the 301. Test with a 302 or 307
    instead, then switch to 301 once you're confident that all is well.

HTTP Strict Transport Security (HSTS):

  • Security. Include the header on http:// responses also. We immediately
    redirect http:// requests to https://, but the header needs to be set
    on the initial response, not just the https:// destination.
  • Shorter max-age. Shorten the default max-age from 1 year to 18 weeks180 days,
    the minimum to qualify for inclusion in browser preload lists.
  • Disabling HSTS. Setting hsts: false now sets hsts: { expires: 0 }
    instead of omitting the header. Omitting does nothing to disable HSTS
    since browsers hang on to your previous settings until they expire.
    Sending hsts: { expires: 0 } flushes out old browser settings and
    actually disables HSTS:
    http://tools.ietf.org/html/rfc6797#section-6.1.1
  • HSTS Preload. Introduce preload: true to set the preload flag,
    indicating that your site may be included in browser preload lists,
    including Chrome, Firefox, Safari, IE11, and Edge. Submit your site:
    https://hstspreload.appspot.com

class SSL
YEAR = 31536000
# Default to the minimum expiry needed to qualify for browser preload.
HSTS_EXPIRES_IN = 18.weeks
Copy link
Member

@rafaelfranca rafaelfranca Sep 7, 2015

Choose a reason for hiding this comment

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

Is not need to require the core_ext to get this working?

Copy link
Member Author

@jeremy jeremy Sep 7, 2015

Choose a reason for hiding this comment

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

Replaced with integer number of seconds 👍

@rafaelfranca
Copy link
Member

rafaelfranca commented Sep 7, 2015

Awesome! :shipit:

@host = options[:host]
@port = options[:port]
@app = RedirectInsecureRequests.new(app, **redirect)
@hsts = normalize_hsts_options(hsts)
Copy link
Contributor

@kaspth kaspth Sep 7, 2015

Choose a reason for hiding this comment

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

I don't see @hsts being used anywhere else than in this scope. Would it make sense to just have it as a local variable?

Copy link
Member Author

@jeremy jeremy Sep 7, 2015

Choose a reason for hiding this comment

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

Yes! 👍

@kaspth
Copy link
Contributor

kaspth commented Sep 7, 2015

Yeah, this is great ᕦ( ͡° ͜ʖ ͡°)ᕤ

HTTP Strict Transport Security (HSTS):
* Security. Include the header on http:// responses also. We immediately
redirect http:// requests to https://, but the header needs to be set
on the initial response, not just the https:// destination.
Copy link
Member

@matthewd matthewd Sep 7, 2015

Choose a reason for hiding this comment

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

https://tools.ietf.org/html/rfc6797#section-7.2:

An HSTS Host MUST NOT include the STS header field in HTTP responses conveyed over non-secure transport.

Copy link
Member Author

@jeremy jeremy Sep 7, 2015

Choose a reason for hiding this comment

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

Doh! Totally misinterpreted that.

Copy link
Member Author

@jeremy jeremy Sep 8, 2015

Choose a reason for hiding this comment

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

Updated to do the redirect before applying headers again.

@jeremy jeremy force-pushed the friendlier-force-ssl branch from f0604fc to f41b32b Compare Sep 8, 2015
@rafaelfranca
Copy link
Member

rafaelfranca commented Sep 8, 2015

👍

@nynhex
Copy link

nynhex commented Sep 8, 2015

This is really great work, wish I could get in on this but it looks very well factored thus far. You guys amaze me on a daily basis. Thanks so much!

SSL redirect:
* Move `:host` and `:port` options within `redirect: { … }`. Deprecate.
* Introduce `:status` and `:body` to customize the redirect response.
  The 301 permanent default makes it difficult to test the redirect and
  back out of it since browsers remember the 301. Test with a 302 or 307
  instead, then switch to 301 once you're confident that all is well.

HTTP Strict Transport Security (HSTS):
* Shorter max-age. Shorten the default max-age from 1 year to 180 days,
  the low end for https://www.ssllabs.com/ssltest/ grading and greater
  than the 18-week minimum to qualify for browser preload lists.
* Disabling HSTS. Setting `hsts: false` now sets `hsts: { expires: 0 }`
  instead of omitting the header. Omitting does nothing to disable HSTS
  since browsers hang on to your previous settings until they expire.
  Sending `{ hsts: { expires: 0 }}` flushes out old browser settings and
  actually disables HSTS:
    http://tools.ietf.org/html/rfc6797#section-6.1.1
* HSTS Preload. Introduce `preload: true` to set the `preload` flag,
  indicating that your site may be included in browser preload lists,
  including Chrome, Firefox, Safari, IE11, and Edge. Submit your site:
    https://hstspreload.appspot.com
@jeremy jeremy force-pushed the friendlier-force-ssl branch from f41b32b to f674922 Compare Sep 8, 2015
@jeremy
Copy link
Member Author

jeremy commented Sep 8, 2015

Updated to allow the app to provide its own Strict-Transport-Security header.

@rafaelfranca
Copy link
Member

rafaelfranca commented Sep 8, 2015

LGTM

jeremy added a commit that referenced this pull request Sep 8, 2015
Make `config.force_ssl` less dangerous to try and easier to disable
@jeremy jeremy merged commit a11571c into rails:master Sep 8, 2015
@jeremy jeremy deleted the friendlier-force-ssl branch Sep 8, 2015
#
# Configure HSTS with `hsts: { … }`:
# * `expires`: How long, in seconds, these settings will stick. Defaults to
# `18.weeks`, the minimum required to qualify for browser preload lists.

Choose a reason for hiding this comment

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

180.days now

Copy link
Contributor

@kaspth kaspth Sep 19, 2015

Choose a reason for hiding this comment

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

Already fixed on master: 7c47160 😁

Choose a reason for hiding this comment

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

Ah, missed that. Awesome.

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

6 participants