Use secure session cookie when connection is secure

[mthuurne]

Currently, Request.getSession() returns a cookie that is not marked as secure, even if the request was made over HTTPS. This means that for example someone in control of a WiFi access point can trick the browser into sending the session cookie unencrypted. Since session cookies are often used to remember a user who was already authenticated, this would be bad.

I think it would be useful to add the notion of a "secure session" to the Session class. A secure session would use a session cookie that is marked as secure, so it will only be transmitted over a secure connection. If a Session object is created from an HTTPS request, it should be a secure session by default.

[steiza]

Proposed fix

[glyph]

Thanks so much for your contribution. There are a few issues which need to be addressed before it's merged.

(Some of these comments interact with each other, so you should probably read the whole thing before starting to respond.)

1. The  twisted coding standard predates (and differs from) PEP8. (Although I suppose everything is PEP8 compliant, insofar as PEP8 says stuff like "Consistency with this style guide is important. Consistency within a project is more important." and "mixedCase is allowed ... where that's already the prevailing style ... to retain backwards compatibility") In particular, attribute names must be camel-case, so secure_session should be secureSession.
   1. very minor point: in new code, there should be 2 blank lines between methods. I notice that you actually deleted a thusly required blank line in test_web.
   2. cookie_string should also be cookieString.
2. Do we really need a secure parameter to getSession? Shouldn't it just use Request.isSecure to determine which one is appropriate? If for some reason we want to provide access from a secure request to an insecure session (which just seems like a weaker form of the same bug to me, but maybe there's a use-case I haven't considered), the default value for secure should be None, so that it will do the right thing by default (inspect isSecure if the flag is None to determine whether it should be true or false).
3. Thanks for the coding-standard updates on the test_ method names.
4. The style for test docstrings is really important. In particular, you shouldn't use phrases like "ensure that" or "verify that" or "test that" or "behaves correctly". Just describe the behavior, say what happens. If the test fails, the reader should infer that the declarations you're making are now false and they should fix the code until they are true again :). For example: test_sessionDifferentFromSecureSession. The docstring should say something like "L{Request.session} and L{Request.secure_session} are different." But, when phrased this way it's clear that this is not enough information: when are they different? Why are they different? Why is it important that they're different?
5. It was a mistake that the session attribute was public. Actually, if you don't mind, could you file a ticket for deprecating it and eventually making it private? The right way to get at the session is to call getSession, not to poke the session attribute directly, since it might not be set. Therefore we shouldn't be adding new, public attributes in this style. secure_session should really be _secureSession.
6. SecurityIssue is a really weird name for an exception. It's not really that general; it's a very specific kind of security issue. WouldLeakInformation, maybe? It's not a security issue yet, it's saying "I won't do this because it would cause this problem", not "I did this, and now you have a security problem" which is what the name SecurityIssue implies to me.

Again, thanks a lot, and I look forward to your updated patch!

[steiza]

Updated fix

[steiza]

1. Updated!

2. Updated, I was thinking most webpages start out as HTTP and then go to HTTPS. When a user transitions from HTTP -> HTTPS you might want access to information or preferences that were set in their insecure session. The exception was not a great idea, instead, I changed the method to take a kwarg forceNotSecure. That way the user can still access the insecure session if they really need to, but they can't call the method in a way that will throw an exception. : )

4. Updated, thanks for clarifying. I'm still kind of a twisted-dev newbie.

5. Updated, see http://twistedmatrix.com/trac/ticket/5026

6. Yeah, see #2. I think this way is much better.

[spiv]

This is not a complete review (so I'm leaving the "review" keyword intact), just some quick comments: use assertNotEqual(session, None) rather than self.assertTrue(session != None). It will give considerably more helpful errors if it does fail :)

Also this looks pretty good in terms of Twisted's style guide now, but you still need to make sure there's two blank lines between methods, as glyph mentioned in his review.

Finally, this ought to have a news file, http://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles.

[exarkun]

(In [31644]) Branching to 'secure-session-3461'

[exarkun]

(In [31645]) Apply issue.3461.1.patch

refs #3461

[exarkun]

Thanks for the updated patch! In addition to spiv's earlier comment:

1. Test coverage for getSession is incomplete. coverage.py is a nice tool for finding uncovered cases.
2. It would be really great to document these security properties in the long form twisted web docs.
3. I also wonder about the backwards compatibility concerns here. Before, if you set a cookie on a client over either HTTP or HTTPS, they could come back over either HTTP or HTTPS and you'd recognize their session. Now it looks like if they initially show up over HTTPS and then come back over HTTP, you won't recognize them (because they won't send the cookie back)? Is that only case that changes? This will break certain existing uses, but they're all uses that leaked credentials material which you really don't ever want to leak, right?
4. It wouldn't hurt to replace the string module usage in this code with string methods, as long as the code is changing.
5. The new _secureSession attribute should be covered in the class docstring as well

I applied your latest patch to the branch mentioned a couple comments up. Please make future patches against that branch. Thanks again.