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 HSTS with IncludeSubdomains header by default for new apps #23852

Merged
merged 6 commits into from Feb 26, 2016

Conversation

@prathamesh-sonpatki
Copy link
Member

@prathamesh-sonpatki prathamesh-sonpatki commented Feb 24, 2016

Fixes #22663

@rafaelfranca
rafaelfranca reviewed Feb 24, 2016
View changes
railties/lib/rails/generators/rails/app/templates/config/initializers/hsts_subdomains.rb Outdated
@@ -0,0 +1 @@
Rails.application.config.action_dispatch.hsts_subdomains = true

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 24, 2016
Member

Instead of a new option can we set Rails.application.config.action_dispatch.ssl_options = { subdomains: true }?

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Feb 24, 2016
Author Member

@rafaelfranca That will mess up with existing ssl_options present in config/environments/production.rb right?

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 24, 2016
Member

Is there any existent in production.rb? I could not find it in the repository. If that is the case we can try to use merge with the already existent value.

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Feb 24, 2016
Author Member

Ah okey config.ssl_options is different that config.action_dispatch.ssl_options.

But you meant reusing config.ssl_options right?

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 24, 2016
Member

Right. config.ssl_options

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Feb 24, 2016
Author Member

@rafaelfranca We can do

Rails.application.config.ssl_options.merge!({ hsts: { subdomains: true } })

But that can mess up with setting the default hsts options here - https://github.com/prathamesh-sonpatki/rails/blob/master/actionpack/lib/action_dispatch/middleware/ssl.rb#L82.

Note that we are passing options only in case of https://github.com/prathamesh-sonpatki/rails/blob/master/actionpack/lib/action_dispatch/middleware/ssl.rb#L92.

options are config.ssl_options[:hsts]

So if we want to reuse config.ssl_options then we will have to update each branch in https://github.com/prathamesh-sonpatki/rails/blob/master/actionpack/lib/action_dispatch/middleware/ssl.rb#L82 to pass options[:subdomains].

It will override user's config.ssl_options[:hsts] though as we will have a default hash instead of true/false/nil/custom options.

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 24, 2016
Member

I don't get that is the problem. If we merged { expires: HSTS_EXPIRES_IN, subdomains: false, preload: false } with { subdomain: true } would not the result be { expires: HSTS_EXPIRES_IN, subdomains: true, preload: false }?

>> default = { expires: 1, subdomains: false, preload: false }
=> {:expires=>1, :subdomains=>false, :preload=>false}
>> default.merge(subdomains: true)
=> {:expires=>1, :subdomains=>true, :preload=>false}
@prathamesh-sonpatki prathamesh-sonpatki force-pushed the prathamesh-sonpatki:hsts-subdomains branch Feb 24, 2016
@prathamesh-sonpatki
prathamesh-sonpatki reviewed Feb 24, 2016
View changes
railties/lib/rails/generators/rails/app/templates/config/initializers/hsts_subdomains.rb Outdated
@@ -0,0 +1 @@
Rails.application.config.ssl_options.reverse_merge!({ hsts: { subdomains: true } })

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Feb 24, 2016
Author Member

@rafaelfranca Made the change as per our discussion. This will make sure that we don't mess up with user's config.ssl_options[:hsts]. It can be false or true or whatever, it will still remain that value after this initializer will get run.

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 24, 2016
Member

We can just set the ssl_option directly, not need to merge. This line will be generated only in new applications, so there is no ssl_options set anywhere. For existent applications we will let the deprecation warning handle it.

We will need to update the config_when_updating method in the generators to not generate this file.

Also it should be named ssl_options.rb

@prathamesh-sonpatki
prathamesh-sonpatki reviewed Feb 24, 2016
View changes
actionpack/lib/action_dispatch/middleware/ssl.rb Outdated
@@ -40,7 +40,7 @@ class SSL
HSTS_EXPIRES_IN = 15552000

def self.default_hsts_options
{ expires: HSTS_EXPIRES_IN, subdomains: false, preload: false }
{ expires: HSTS_EXPIRES_IN, subdomains: true, preload: false }
end

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Feb 24, 2016
Author Member

This is still shaky, thinking about how to handle it properly.

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 24, 2016
Member

The default should be false

homakov and others added 4 commits Dec 18, 2015
1) Because if you forget to add Secure; to the session cookie, it will leak to http:// subdomain in some cases
2) Because http:// subdomain can Cookie Bomb/cookie force main domain or be used for phishing.

That's why *by default* it must include subdomains as it's much more common scenario. Very few websites *intend* to leave their blog.app.com working over http:// while having everything else encrypted. 

Yes, many developers forget to add subdomains=true by default, believe me :)
…th subdomains

- We will reuse config.ssl_options for setting the HSTS settings.
- We will remove the initializer for old apps which are migrated to
  Rails 5 so that they are not affected by this breaking change.
- For old apps which are not setting any value for hsts[:subdomains],
  a deprecation warning will be shown saying that hsts[:subdomains] will
  be turned on by default in Rails 5.1. Currently it will be set to
  false for backward compatibility.
- Adjusted tests to reflect this change.
@prathamesh-sonpatki prathamesh-sonpatki force-pushed the prathamesh-sonpatki:hsts-subdomains branch to 31cf0f5 Feb 25, 2016
@prathamesh-sonpatki prathamesh-sonpatki changed the title [WIP] Enable HSTS with IncludeSubdomains by default for new apps Enable HSTS with IncludeSubdomains header by default for new apps Feb 25, 2016
@prathamesh-sonpatki
Copy link
Member Author

@prathamesh-sonpatki prathamesh-sonpatki commented Feb 25, 2016

r? @rafaelfranca Please review.

rafaelfranca added a commit that referenced this pull request Feb 26, 2016
Enable HSTS with IncludeSubdomains header by default for new apps
@rafaelfranca rafaelfranca merged commit 0e24fcc into rails:master Feb 26, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@prathamesh-sonpatki prathamesh-sonpatki deleted the prathamesh-sonpatki:hsts-subdomains branch Feb 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.