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

Should cookie-based sessions default to secure if using HTTPS? #3610

Open
luhn opened this issue Sep 1, 2020 · 7 comments
Open

Should cookie-based sessions default to secure if using HTTPS? #3610

luhn opened this issue Sep 1, 2020 · 7 comments

Comments

@luhn
Copy link
Contributor

luhn commented Sep 1, 2020

Pretty simple change that I can whip up a PR for, figured now's the time to shoehorn it in before 2.0 release.

Basically, default SignedCookieSessionFactory would default the "secure" parameter to request.scheme == 'https'. So by default if the request is HTTPS, sessions will use a secure cookie. Implementation looks something like this.

Pros:

  • A more secure default.
  • Works nicely OotB with HTTP for localhost and HTTPS for production.

Cons:

  • Not backwards compatible, may cause problems if sites are using both HTTP and HTTPS.

Would a PR for this be welcome?

@mmerickel
Copy link
Member

I think I'm ok with this but I think it should be done universally for our various cookie-generating APIs (auth, sessions, csrf). I suspect the API would be that if secure=None then it is "dynamic", versus a truthy or falsey value that is explicit? I want to know what @bertjwregeer thinks as well.

I'm not sure what is considered better - "dynamic / smart" security policies or more explicit ones like what we have right now.

@luhn
Copy link
Contributor Author

luhn commented Sep 1, 2020

I think I'm ok with this but I think it should be done universally for our various cookie-generating APIs (auth, sessions, csrf).

Sounds good, I can include those in a PR.

I suspect the API would be that if secure=None then it is "dynamic", versus a truthy or falsey value that is explicit?

Yeah, that's what I was imagining.

I'm not sure what is considered better - "dynamic / smart" security policies or more explicit ones like what we have right now.

Generally I would prefer explicit, but in this case secure=True is untenable because it would be broken on localhost, and I prefer "smart" to secure=False.

Speaking of explicit—maybe a topic for another issue—but should we change default of samesite to Strict and httponly to True? A bit more radical, since that has a higher chance of breaking sites, but you only have 2.0 once.

@mmerickel
Copy link
Member

In the interest of alternatives that do not require changing Pyramid to solve this problem - all the apps I write source these settings from the ini file. So I commonly support settings like session.secure, session.cookie_name, and session.secret while hard-coding samesite, httponly etc based on how the app expects those to be used.

@stevepiercy
Copy link
Member

Considering both a docs/tutorials/cookie cutter updating and getting started/newbie perspectives, my preferences would be in order:

  1. Using an .ini (implicitly leaving Pyramid as is)
  2. Flat out leave it as is
  3. Detect whether the request is over http versus https and uses the correct setting accordingly

Forcing it to prefer https seems harmful to me.

I would strongly support some added documentation about how to use https throughout the entire stack, even though that is primarily the responsibility of the web server, because secure cookies touches on this topic. At the very least, a list of requirements with references for more information. Currently on master there is no mention of "certificate" in the docs.

@digitalresistor
Copy link
Member

@luhn you may also want to take a look at CookieProfile and the cookie handling in WebOb as I think that is where we would want to try and make such a change happen, if at all possible.

I am okay with setting the cookie to secure if the origin is reached over HTTPS.

@luhn
Copy link
Contributor Author

luhn commented Sep 7, 2020

In the interest of alternatives that do not require changing Pyramid to solve this problem - all the apps I write source these settings from the ini file.

It would be convenient to have a SignedCookieSessionFactory.from_settings classmethod. I'd prefer to have secure defaults, but making is easy to turn secure on/off from the settings is a good thing to have.

you may also want to take a look at CookieProfile and the cookie handling in WebOb as I think that is where we would want to try and make such a change happen, if at all possible.

Are you saying we should remove Pyramid's implementation of signed cookies and use WebOb's instead?

@digitalresistor
Copy link
Member

@luhn we already used the serializers from WebOb, I thought that the BaseCookieSessionFactory was also using webob.cookies.SignedCookieProfile to do the cookie setting/unsetting.

Since we aren't, we should see if we can change it to use the SignedCookieProfile instead, so we have a single place to make these sorts of improvements.

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

No branches or pull requests

4 participants