search

Pylons / pyramid

Pyramid - A Python web framework
https://trypyramid.com/
Other
3.98k stars 887 forks source link

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

Open luhn opened 4 years ago

luhn commented 4 years ago

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:

Cons:

Would a PR for this be welcome?

mmerickel commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

@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.