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

Fixes #10275 -Use secure cookies when SSL #2328

Closed
wants to merge 1 commit into from

Conversation

shlomizadok
Copy link
Member

In production, we must enforce ssl, so cookies will be secured :)
Tried on a local foreman (Satellite machine):
secure

This should be backported to 1.7 and 1.8

@theforeman-bot
Copy link
Member

There were the following issues with the commit message:

  • 357f9cb must be in the format Fixes/refs #redmine_number - brief description.

Guidelines are available on the Foreman wiki.


This message was auto-generated by Foreman's prprocessor

@shlomizadok shlomizadok changed the title Fixes 10275 - force using SSL on production Fixes #10275 - force using SSL on production Apr 28, 2015
@domcleal
Copy link
Contributor

Does this affect non-HTTPS usage? What if require_ssl's disabled?

Also the other cookies mentioned in my comment to the ticket should be fixed.

@@ -131,7 +131,7 @@
config.assets.precompile += javascript.map{|js| js + ".js"} + stylesheets + %w(background-size.htc)

# Force all access to the app over SSL, use Strict-Transport-Security, and use secure cookies.
# config.force_ssl = true
config.force_ssl = true
Copy link
Member

Choose a reason for hiding this comment

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

won't this break provisioning? (which needs http)

@domcleal
Copy link
Contributor

Setting back to WoC for answers to all of the above questions.

@shlomizadok
Copy link
Member Author

Added secure params to $.cookie on timezone and selected hosts cookies.
When in ssl:
foreman_dev_secure

When without ssl:

foreman_dev_insecure

One thing I am not sure about is provisioning. Any thoughts @domcleal ?

@domcleal
Copy link
Contributor

One thing I am not sure about is provisioning. Any thoughts @domcleal ?

I don't follow, what aren't you sure about?

@ohadlevy
Copy link
Member

@shlomizadok i guess you can use force_ssl:
http://api.rubyonrails.org/classes/ActionController/ForceSSL/ClassMethods.html

but, imho you would need to look at:

  1. disabling existin ssl redication code in foreman
  2. skipping it for unattended controller conditionally
  3. check that the smart proxy auth code still works as expected

anything else @domcleal ?

@@ -131,7 +131,7 @@
config.assets.precompile += javascript.map{|js| js + ".js"} + stylesheets + %w(background-size.htc)

# Force all access to the app over SSL, use Strict-Transport-Security, and use secure cookies.
# config.force_ssl = true
config.force_ssl = true if SETTINGS[:require_ssl]
Copy link
Member

Choose a reason for hiding this comment

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

config.force_ssl =  !!SETTINGS[:require_ssl]

Copy link
Member

Choose a reason for hiding this comment

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

That seems a bit dangerous if the file is misconfigured, 'false' gets evaluated to true.

@shlomizadok shlomizadok force-pushed the fix_10275 branch 2 times, most recently from 6668c02 to 6594736 Compare April 28, 2015 16:36
@@ -578,6 +578,10 @@ def test_unset_manage
Setting[:restrict_registered_smart_proxies] = true
Setting[:require_ssl_smart_proxies] = true
SETTINGS[:require_ssl] = true
# Adding https and certificates or else the request will be redirected
@request.env['HTTPS'] = 'on'
Copy link
Member Author

Choose a reason for hiding this comment

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

Since SETTINGS[:require_ssl] = true, I have to simulate an https call, or else I'll be redirected to https (and the response will be 301, and not 403 as expected)

Copy link
Contributor

Choose a reason for hiding this comment

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

That means you're no longer testing the scenario described on line 576? Instead it seems to test the SSL verification step over HTTPS.

That probably points to this PR needing to add something to disable the redirect in the Foreman::Controller::SmartProxyAuth concern, which currently disables the existing SSL redirect and checking require_ssl itself.

@domcleal
Copy link
Contributor

Does this obsolete our own require_ssl filter?

@shlomizadok
Copy link
Member Author

@domcleal - removed config.force_ssl option entirely, as we let the controllers decide on when to enforce ssl (because force_ssl on application.rb / will force it anyhow). Hope this helps.

@domcleal
Copy link
Contributor

@shlomizadok okay, but now we're back to the issue you're originally aiming to fix - the session cookie has no secure flag. See the link I mentioned in my last reply, about setting the session options - is that an alternative way?

(Currently this PR only fixes two non-session cookies, and simply refactors our redirects.)

@shlomizadok
Copy link
Member Author

@domcleal - another try at application.rb to set cookie to secure.

@@ -146,6 +146,12 @@ class Application < Rails::Application
child.helper helpers
end
end

# Secure cookies if the connection is via SSL
if SETTINGS[:require_ssl] == true
Copy link
Contributor

Choose a reason for hiding this comment

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

if !!SETTINGS[:require_ssl] for consistency

Copy link
Member Author

Choose a reason for hiding this comment

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

Chose the true option :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@shlomizadok shlomizadok force-pushed the fix_10275 branch 2 times, most recently from 58b043f to 4bf9e98 Compare May 11, 2015 14:54
@domcleal
Copy link
Contributor

Alright, looks good and tests well, thanks @shlomizadok.

Since this has evolved into two different things, would you mind moving the force_ssl changes into a separate PR against ticket http://projects.theforeman.org/issues/10471? We'll merge both, but that will probably go to 1.9.0 and this to our stable release(s) as we don't need it to fix the CVE.

Thanks!

@shlomizadok shlomizadok changed the title Fixes #10275 - force using SSL on production Fixes #10275 -Use secure cookies when SSL May 11, 2015
@shlomizadok
Copy link
Member Author

Done. #2373 is the second part

@domcleal
Copy link
Contributor

Merged as 0b03b9b, thanks @shlomizadok!

@domcleal domcleal closed this May 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants