Opened 4 years ago

Closed 17 months ago

Last modified 17 months ago

#6932 enhancement closed duplicate (duplicate)

when using HTTPS, twisted.web session cookies should be "secure"

Reported by: Adi Roiban Owned by: Adi Roiban
Priority: normal Milestone:
Component: web Keywords:
Cc: jknight Branch: secure-cookie-6932-2
branch-diff, diff-cov, branch-cov, buildbot
Author: glyph

Description (last modified by Jean-Paul Calderone)

Cookies can be "secure" - ie, only transmitted over HTTPS connections.

This is probably a good thing for session cookies since these may grant access to an account that otherwise requires some kind of authentication (in other words, session cookies *are* a form of authentication).

https://en.wikipedia.org/wiki/HTTP_cookie#Secure_and_HttpOnly

Twisted Web should send "secure" session cookies over HTTPS connections by default. Maybe it should have an API to specify that *only* "secure" session cookies are allowed for an application.

Attachments (2)

6932-1.diff (3.2 KB) - added by Adi Roiban 4 years ago.
6932-2.diff (4.8 KB) - added by Adi Roiban 4 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 4 years ago by DefaultCC Plugin

Cc: jknight added

comment:2 Changed 4 years ago by Jean-Paul Calderone

Description: modified (diff)
Summary: Custom session cookie name and secure session cookiewhen using HTTPS, twisted.web session cookies should be "secure"

I am going to remove half of the things from this ticket. Please limit each ticket to a single feature. Thanks. :)

comment:3 Changed 4 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

Here is an initial implementation with test and news file.

Please suggest an API for overwriting the default behaviour.

I was thinking at

class Request(object):
    #: Set this to True to have new session cookies created over
    # HTTPS without the `secure` flag.
    __force_http_session_cookies__ = False

Diff web version: https://github.com/chevah/twisted/compare/6932-secure-session-cookie

Live diff here: https://github.com/chevah/twisted/compare/6932-secure-session-cookie.diff

I have attached the diff.

Thanks!

Changed 4 years ago by Adi Roiban

Attachment: 6932-1.diff added

comment:4 Changed 4 years ago by Adi Roiban

If code is ok, I think that this branch/ticket would also need to update the documentation and explain the default session behaviour.

comment:5 Changed 4 years ago by Adi Roiban

Please also check progress on #6933.

Instead of the current patch, I think that the IRequest should have a IRequest.makeNewSession() or IRequest.createNewSession() method which should take care of getting a new session from Site.makeSession() and setting the cookie.

In this way, users can overwrite this method and implement they own behavior.

Thanks!

comment:6 Changed 4 years ago by Glyph

Author: glyph
Branch: branches/secure-cookie-6932

(In [41728]) Branching to secure-cookie-6932.

comment:7 Changed 4 years ago by Glyph

Keywords: review removed
Owner: set to Adi Roiban

Wow, yet another patch from adiroiban! Thanks again!

Looks like there are some failing tests on Python 3.3, and a twistedchecker error related to malformatted docstrings.

Also, as you correctly point out, this is going to need some documentation.

Regarding your comments in #5 above: If you don't like the interface you're proposing in this patch... perhaps you should submit another patch with the interface you do like? It would be more productive to code-review that :).

comment:8 Changed 4 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

Hi,

Thanks for the review.

Asking new contributors to use twistedchecker is cruel. It is not documented and the code in trunk does not pass the twistedckecer. Filtering already exiting error is not fun.

trunk> twistedchecker twisted.web.server | wc -l
101

Also running tests on Python 2.7 and 3.3 is hard without providing a tox.ini file.

Looking at https://twistedmatrix.com/trac/wiki/TwistedDevelopment I could not find documentation about how to run tests on python 3.3. There is only a Plan/Python3 page.

No hard feelings. This is just a feedback as a new contributor.


In #6933 I wrote test for getSession method so here I will focus on tests for the new createSession method.

I tried my best to fix twistedchecker and py3.3 errors.

I have added some kind of documentation. Please advise how I would approach the documentation.

Please check latest patch.

Thanks!

Changed 4 years ago by Adi Roiban

Attachment: 6932-2.diff added

comment:9 Changed 4 years ago by Julian Berman

Is this a duplicate of #3461?

comment:10 Changed 4 years ago by Glyph

It appears to be.

I'm not going to close it as a duplicate because this one's in review and that one isn't, but I would encourage adiroiban to look at the earlier branch and see if it's worth preserving.

Also when this is landed one of the tickets should be closed as a duplicate.

comment:11 Changed 3 years ago by Glyph

Branch: branches/secure-cookie-6932branches/secure-cookie-6932-2

(In [42963]) Branching to secure-cookie-6932-2.

comment:12 Changed 3 years ago by Glyph

Keywords: review removed
Owner: set to Adi Roiban

Thanks adiroiban,

  1. There are some coding-standard issues and doc completeness issues that the twistedchecker buildbot found
  2. There should be 2 blank lines after makeRequest; for some reason twistedchecker didn't catch that. Hmm.
  3. The documentation says that it will be kept in a variable called TWISTED_SESSION, but it doesn't explain how you might choose something other than the default.
  4. There are a couple of places where you could use epytext markup to good effect.
    1. In makeRequest, you should use C{} rather than backticks to indicate request is an identifier.
    2. In createSession, the "See:" should be a @see: and the link should be in a U{} link.
  5. I think makeRawCookie wants to return bytes? It's not entirely clear whether it's operating on bytes or text.
  6. I tested this manually, using the following script:
    from twisted.internet import reactor
    from twisted.internet.ssl import PrivateCertificate
    from twisted.internet.endpoints import SSL4ServerEndpoint
    from twisted.web.resource import Resource
    from twisted.internet.endpoints import TCP4ServerEndpoint
    
    from twisted.web.server import Site
    
    with open("docs/core/examples/server.pem") as f:
        cert = PrivateCertificate.loadPEM(f.read())
    
    
    class CountingResource(Resource):
        """
        A resource that counts.
        """
        isLeaf = True
    
        def render_GET(self, request):
            """
            Render a GET.
            """
            session = request.getSession()
            session.value = getattr(session, "value", 0) + 1
            request.setHeader("content-type", "text/plain")
            return b"counter: " + unicode(session.value).encode("ascii")
    
    
    site = Site(CountingResource())
    secureEndpoint = SSL4ServerEndpoint(reactor, 8443, cert.options())
    secureEndpoint.listen(site)
    insecureEndpoint = TCP4ServerEndpoint(reactor, 8080)
    insecureEndpoint.listen(site)
    
    reactor.run()
    
    which reveals an unfortunate issue with this code. While I see that the session cookie is properly made https-only, practically, security has not improved much, since the HTTP-only session will override the HTTPS session as soon as you load any page (including a favicon.ico or a HSTS redirect) via plaintext. This seems like it could be a very common error, and would lead to some really horrible confusion.

Please address the issues above. As to the final point, you don't have to come up with a fully secure scheme for dealing with mixed HTTP/HTTPS sites where Twisted sessions might be used on both, but it would be good to at least have a plan for where that is going so that we can be sure this is the right kind of change to facilitate actual security.

Thanks for plugging away at this, and sorry the review queue is so backed up, I'm doing what I can.

comment:13 Changed 17 months ago by Adi Roiban

Resolution: duplicate
Status: newclosed

Duplicate of #3461

comment:14 Changed 17 months ago by Adi Roiban

Branch: branches/secure-cookie-6932-2secure-cookie-6932-2
Note: See TracTickets for help on using tickets.