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

Use a secure session cookie for new installs #423

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
7 participants
@derekprior
Contributor

derekprior commented Apr 18, 2014

Turning secure_cookie on has the effect of making the cookie HTTPS-only, and
thus not susceptible to session hijacking. For security, this should be the
default in environments other than development and test.

The default initializer has been updated to reflect this. In Clearance 2.0,
this may become the default behavior in Clearance internals and thus no longer
depend on the initializer being present and containing the proper setting.

@derekprior

This comment has been minimized.

Show comment
Hide comment
@derekprior

derekprior Apr 18, 2014

Contributor

I'm wondering if the disconnect here between dev/test & production/staging is going to cause confusion.

Contributor

derekprior commented Apr 18, 2014

I'm wondering if the disconnect here between dev/test & production/staging is going to cause confusion.

@BlakeWilliams

This comment has been minimized.

Show comment
Hide comment
@BlakeWilliams

BlakeWilliams Apr 18, 2014

My first thought was the confusion that dev/test and production/staging behaving differently with little to no warning could cause.

My first thought was the confusion that dev/test and production/staging behaving differently with little to no warning could cause.

@derekprior

This comment has been minimized.

Show comment
Hide comment
@derekprior

derekprior Apr 18, 2014

Contributor

Any idea on how to address this? I could add an explanation to the default initializer, but I'm not sure people will look there.

I thought about turning it on for all environments but that seemed like it would cause even more issues.

Contributor

derekprior commented Apr 18, 2014

Any idea on how to address this? I could add an explanation to the default initializer, but I'm not sure people will look there.

I thought about turning it on for all environments but that seemed like it would cause even more issues.

@jferris

This comment has been minimized.

Show comment
Hide comment
@jferris

jferris Apr 18, 2014

Member

One idea to make this easier to debug: raise an exception if you try to fetch the current user when 1) the connection is not SSL and 2) Clearance's SSL-only setting is on.

That way, if you attempt to use staging/production on an HTTP connection, you'll get a 500 and a clear error in your logs instead of a mysteriously signed out session.

Member

jferris commented Apr 18, 2014

One idea to make this easier to debug: raise an exception if you try to fetch the current user when 1) the connection is not SSL and 2) Clearance's SSL-only setting is on.

That way, if you attempt to use staging/production on an HTTP connection, you'll get a 500 and a clear error in your logs instead of a mysteriously signed out session.

@BlakeWilliams

This comment has been minimized.

Show comment
Hide comment
@BlakeWilliams

BlakeWilliams Apr 18, 2014

It might also be a good idea to toss a notice in the generator similar to how Ember-Rails has done in the past with warn and add something in the readme.

It might also be a good idea to toss a notice in the generator similar to how Ember-Rails has done in the past with warn and add something in the readme.

@derekprior

This comment has been minimized.

Show comment
Hide comment
@derekprior

derekprior Jul 21, 2014

Contributor

@jferris, @BlakeWilliams: Would this error make sense to you?

        raise <<-ERROR.strip_heredoc
          Clearance has `secure_cookie` enabled in this environment but
          `current_user` was just accessed during a non-SSL request. Assuming
          this is a production environment, you probably want to insert the
          'Rack::SSL' middleware or otherwise force HTTPS connections.
        ERROR
Contributor

derekprior commented Jul 21, 2014

@jferris, @BlakeWilliams: Would this error make sense to you?

        raise <<-ERROR.strip_heredoc
          Clearance has `secure_cookie` enabled in this environment but
          `current_user` was just accessed during a non-SSL request. Assuming
          this is a production environment, you probably want to insert the
          'Rack::SSL' middleware or otherwise force HTTPS connections.
        ERROR
Encourage use of a secure session cookie
Turning `secure_cookie` on has the effect of making the cookie
HTTPS-only, and thus not susceptible to session hijacking. For
security, this should be the default in environments other than
development and test.

The default initializer has been updated to reflect this. New
applications that run the generator will have `secure_cookie` enabled
in all but development and test environments.

If `secure_cookie` is accessed having previously been unconfigured, a
warning is raised encouraging the developer to enable `secure_cookie` or
at least explicitly set it to false, thus opting in to the insecure
behavior.

Additionally, a descriptive application error is raised if
`secure_cookie` is enabled and yet `current_user` is accessed in a
non-ssl request. This is an indicator that the developer has forgotten
to enable SSL in this environment. This should help to address the
confusion that might arise from the generated initializer applying a
different configuration between development and production. Without the
application error, the end user would simply have a signed out session.
@derekprior

This comment has been minimized.

Show comment
Hide comment
@derekprior

derekprior Jul 23, 2014

Contributor

I've updated the pull request to take into account the previous feedback and to attempt to encourage existing users to update their clearance configurations to be more secure. I'll let my latest commit message do the talking here:

If secure_cookie is accessed having previously been unconfigured, a
warning is raised encouraging the developer to enable secure_cookie or
at least explicitly set it to false, thus opting in to the insecure
behavior.

Additionally, a descriptive application error is raised if
secure_cookie is enabled and yet current_user is accessed in a
non-ssl request. This is an indicator that the developer has forgotten
to enable SSL in this environment. This should help to address the
confusion that might arise from the generated initializer applying a
different configuration between development and production. Without the
application error, the end user would simply have a signed out session.

Contributor

derekprior commented Jul 23, 2014

I've updated the pull request to take into account the previous feedback and to attempt to encourage existing users to update their clearance configurations to be more secure. I'll let my latest commit message do the talking here:

If secure_cookie is accessed having previously been unconfigured, a
warning is raised encouraging the developer to enable secure_cookie or
at least explicitly set it to false, thus opting in to the insecure
behavior.

Additionally, a descriptive application error is raised if
secure_cookie is enabled and yet current_user is accessed in a
non-ssl request. This is an indicator that the developer has forgotten
to enable SSL in this environment. This should help to address the
confusion that might arise from the generated initializer applying a
different configuration between development and production. Without the
application error, the end user would simply have a signed out session.

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Jul 23, 2014

There should probably be some indication of what's going to happen outside of production. It's a poor UX to receive a 500 inexplicably when pushing to production. Especially considering this is an environment where you have to cat the logs to actually see the error. I'm not sure what the right solution is for that, however.

sgrif commented Jul 23, 2014

There should probably be some indication of what's going to happen outside of production. It's a poor UX to receive a 500 inexplicably when pushing to production. Especially considering this is an environment where you have to cat the logs to actually see the error. I'm not sure what the right solution is for that, however.

@derekprior

This comment has been minimized.

Show comment
Hide comment
@derekprior

derekprior Jul 23, 2014

Contributor

Another option would be to simply flip the default for secure_cookie to true in all environments and have the initializer opt-out dev and test. Developers already using SSL in production would have to update or create the clearance initializer to get their dev and test environments working. The generator would do this automatically for new apps.

Anyone using Clearance and not forcing ssl in production would have their application break, so this is likely a 2.0 release which would supersede any in-flight changes already made for 2.0 (those would now be targeted at 3.0).

If we pursued that I would remove the warning in configuration and the error raising.

Contributor

derekprior commented Jul 23, 2014

Another option would be to simply flip the default for secure_cookie to true in all environments and have the initializer opt-out dev and test. Developers already using SSL in production would have to update or create the clearance initializer to get their dev and test environments working. The generator would do this automatically for new apps.

Anyone using Clearance and not forcing ssl in production would have their application break, so this is likely a 2.0 release which would supersede any in-flight changes already made for 2.0 (those would now be targeted at 3.0).

If we pursued that I would remove the warning in configuration and the error raising.

@croaky

This comment has been minimized.

Show comment
Hide comment
@croaky

croaky Jul 23, 2014

Contributor

There should probably be some indication of what's going to happen outside of production.

This kind of reminds me of the attr_accessible days of Clearance. The hassle of breakages likely outweighs the opinionated opinion on security. Should we scale back to a warn instead of a raise in the controller?

Would it make sense to check if config.force_ssl = false is set at Rails boot time and warn then instead of, or in addition to, each request?

Contributor

croaky commented Jul 23, 2014

There should probably be some indication of what's going to happen outside of production.

This kind of reminds me of the attr_accessible days of Clearance. The hassle of breakages likely outweighs the opinionated opinion on security. Should we scale back to a warn instead of a raise in the controller?

Would it make sense to check if config.force_ssl = false is set at Rails boot time and warn then instead of, or in addition to, each request?

@@ -21,6 +21,16 @@ def authenticate(params)
end
def current_user
if !request.ssl? && Clearance.configuration.secure_cookie
raise <<-ERROR.strip_heredoc

This comment has been minimized.

@derekprior

derekprior Jul 23, 2014

Contributor

It may make more sense to do this on, say, authenticate. I can think of valid reasons where you may have secure_cookie enabled, be on a non-ssl connection, and still call current_user expecting it to return nil or guest in those cases.

This is probably not the right place for this error.

@derekprior

derekprior Jul 23, 2014

Contributor

It may make more sense to do this on, say, authenticate. I can think of valid reasons where you may have secure_cookie enabled, be on a non-ssl connection, and still call current_user expecting it to return nil or guest in those cases.

This is probably not the right place for this error.

This comment has been minimized.

@calebthompson

calebthompson Jul 23, 2014

Contributor

current_user also seems like a likely candidate for a method that's been overridden.

I'm not sure there's much to be done about that in any case with Clearance's setup, but possibly pushing this to be more internal would help.

@calebthompson

calebthompson Jul 23, 2014

Contributor

current_user also seems like a likely candidate for a method that's been overridden.

I'm not sure there's much to be done about that in any case with Clearance's setup, but possibly pushing this to be more internal would help.

This comment has been minimized.

@derekprior

derekprior Jul 23, 2014

Contributor

In cases where it's overridden it should still be calling super otherwise you're on your own anyway.

@derekprior

derekprior Jul 23, 2014

Contributor

In cases where it's overridden it should still be calling super otherwise you're on your own anyway.

@derekprior

This comment has been minimized.

Show comment
Hide comment
@derekprior

derekprior Jul 23, 2014

Contributor

The hassle of breakages likely outweighs the opinionated opinion on security.

I don't think I'd call this an opinion. We're dealing with an insecure default. I suspect that the majority of Clearance applications in production are susceptible to relatively simple session hijacking. I think we need to address this one way or another.

Contributor

derekprior commented Jul 23, 2014

The hassle of breakages likely outweighs the opinionated opinion on security.

I don't think I'd call this an opinion. We're dealing with an insecure default. I suspect that the majority of Clearance applications in production are susceptible to relatively simple session hijacking. I think we need to address this one way or another.

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Jul 23, 2014

I think a better solution would be to have the generator default to true in
the config, and raise in any environment at boot if the config option is
not set.
On Jul 22, 2014 9:31 PM, "Derek Prior" notifications@github.com wrote:

The hassle of breakages likely outweighs the opinionated opinion on
security.

I don't think I'd call this an opinion. We're dealing with an insecure
default. I suspect that the majority of Clearance applications in
production are susceptible to relatively simple session hijacking. I think
we need to address this one way or another.


Reply to this email directly or view it on GitHub
#423 (comment).

sgrif commented Jul 23, 2014

I think a better solution would be to have the generator default to true in
the config, and raise in any environment at boot if the config option is
not set.
On Jul 22, 2014 9:31 PM, "Derek Prior" notifications@github.com wrote:

The hassle of breakages likely outweighs the opinionated opinion on
security.

I don't think I'd call this an opinion. We're dealing with an insecure
default. I suspect that the majority of Clearance applications in
production are susceptible to relatively simple session hijacking. I think
we need to address this one way or another.


Reply to this email directly or view it on GitHub
#423 (comment).

@mike-burns

This comment has been minimized.

Show comment
Hide comment
@mike-burns

mike-burns Jul 24, 2014

Member

If there's something the user can do that makes them susceptible to being attacked by something as simple as Firesheep, we should not make that an option. We simply shouldn't allow that.

Member

mike-burns commented Jul 24, 2014

If there's something the user can do that makes them susceptible to being attacked by something as simple as Firesheep, we should not make that an option. We simply shouldn't allow that.

@derekprior

This comment has been minimized.

Show comment
Hide comment
@derekprior

derekprior Jul 24, 2014

Contributor

So that's a vote for removing 'secure_cookie' as an option entirely -- or at least ignoring what the user tries to set it to?

That'd be a breaking change, so it'd go to 2.0. I think that'd be the only change introduced in 2.0. I think I'd even leave the previously deprecated stuff in just so as many people as possible could upgrade.

People that can't upgrade would be covered by enabling secure_cookie provided that are on at least 1.0.0. Users of older versions would have to upgrade, I think. I'm not positive how cookies were handled in older versions, but the fact that secure_cookie came in 1.0.0 makes me think they are insecure.

Contributor

derekprior commented Jul 24, 2014

So that's a vote for removing 'secure_cookie' as an option entirely -- or at least ignoring what the user tries to set it to?

That'd be a breaking change, so it'd go to 2.0. I think that'd be the only change introduced in 2.0. I think I'd even leave the previously deprecated stuff in just so as many people as possible could upgrade.

People that can't upgrade would be covered by enabling secure_cookie provided that are on at least 1.0.0. Users of older versions would have to upgrade, I think. I'm not positive how cookies were handled in older versions, but the fact that secure_cookie came in 1.0.0 makes me think they are insecure.

@mike-burns

This comment has been minimized.

Show comment
Hide comment
@mike-burns

mike-burns Jul 24, 2014

Member

Yes: remove secure_cookie, 2.0 should only be this security fix, and users who choose not to upgrade are choosing against security.

Member

mike-burns commented Jul 24, 2014

Yes: remove secure_cookie, 2.0 should only be this security fix, and users who choose not to upgrade are choosing against security.

@derekprior

This comment has been minimized.

Show comment
Hide comment
@derekprior

derekprior Jul 24, 2014

Contributor

That would mean you'd need to run SSL in dev and test. Maybe this is actually desired -- you'll be running SSL in production, after all -- but it's not exactly common practice.

Maybe we default the setting to true and allow it to be overridden explicitly? The generator could still set it to false for dev and test. But then we're back in a situation where there's potentially confusing behavior when you go to prod.

Contributor

derekprior commented Jul 24, 2014

That would mean you'd need to run SSL in dev and test. Maybe this is actually desired -- you'll be running SSL in production, after all -- but it's not exactly common practice.

Maybe we default the setting to true and allow it to be overridden explicitly? The generator could still set it to false for dev and test. But then we're back in a situation where there's potentially confusing behavior when you go to prod.

@derekprior

This comment has been minimized.

Show comment
Hide comment
@derekprior

derekprior Jul 24, 2014

Contributor

Here's the best plan I can come up with so far:

  1. Release Clearance 2.0 which:
    • defaults secure_cookie to true when not set.
    • updates the generated initializer to set secure_cookie to false in dev and test.
    • Raises an error if secure_cookie is true and you call authenticate.
    • Add a post-install message to the gem telling people they need to enable SSL in non dev/test environments.
    • Add a similar message to the text you see after running rails generate clearance:install
  2. Blog about this - explain the problem and the difficulties in finding a perfect solution.
  3. Release a CVE for the insecure default in Clearance < 2.0

There's going to be some confusion about the disconnect between dev and prod, but I cannot think of a good way to handle this. Updating current_user as I did in this PR is not valid in all scenarios and may cause errors for sites that are perfectly secure, but don't force SSL on all pages. Moving the error to authenticate seems correct to me, though there's always a chance you are using SSL on pages that do authentication but not on the rest of the site.

I wish it was standard practice to run SSL in dev and test if you are running it in prod, but it's simply not.

Contributor

derekprior commented Jul 24, 2014

Here's the best plan I can come up with so far:

  1. Release Clearance 2.0 which:
    • defaults secure_cookie to true when not set.
    • updates the generated initializer to set secure_cookie to false in dev and test.
    • Raises an error if secure_cookie is true and you call authenticate.
    • Add a post-install message to the gem telling people they need to enable SSL in non dev/test environments.
    • Add a similar message to the text you see after running rails generate clearance:install
  2. Blog about this - explain the problem and the difficulties in finding a perfect solution.
  3. Release a CVE for the insecure default in Clearance < 2.0

There's going to be some confusion about the disconnect between dev and prod, but I cannot think of a good way to handle this. Updating current_user as I did in this PR is not valid in all scenarios and may cause errors for sites that are perfectly secure, but don't force SSL on all pages. Moving the error to authenticate seems correct to me, though there's always a chance you are using SSL on pages that do authentication but not on the rest of the site.

I wish it was standard practice to run SSL in dev and test if you are running it in prod, but it's simply not.

@mike-burns

This comment has been minimized.

Show comment
Hide comment
@mike-burns

mike-burns Jul 24, 2014

Member

I honestly don't know the current easiest way to run dev and test using TLS. Is there a blog post we can point to when announcing things?

Member

mike-burns commented Jul 24, 2014

I honestly don't know the current easiest way to run dev and test using TLS. Is there a blog post we can point to when announcing things?

@derekprior

This comment has been minimized.

Show comment
Hide comment
@derekprior

derekprior Jul 25, 2014

Contributor

Some new information after looking at the source of Rack:SSL/ActionDispatch::SSL:

That middleware rewrites cookies (all of them) to be secure-only. So if you use clearance and enable site-wide SSL in your environment configurations then it doesn't matter what you have Clearance's secure_cookie set to. The cookie will be set as secure.

This means the scope of sites effected by the security issue I call out here is limited to those that do not enable SSL at all or enable it selectively or with something other than the rails/rack middleware.

Contributor

derekprior commented Jul 25, 2014

Some new information after looking at the source of Rack:SSL/ActionDispatch::SSL:

That middleware rewrites cookies (all of them) to be secure-only. So if you use clearance and enable site-wide SSL in your environment configurations then it doesn't matter what you have Clearance's secure_cookie set to. The cookie will be set as secure.

This means the scope of sites effected by the security issue I call out here is limited to those that do not enable SSL at all or enable it selectively or with something other than the rails/rack middleware.

@derekprior derekprior closed this Aug 29, 2014

@derekprior derekprior deleted the dp-initialize-secure-cookie branch Jan 6, 2015

@derekprior derekprior referenced this pull request Feb 21, 2017

Closed

SSL as the default? #738

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment