Opened 10 years ago

Closed 5 years ago

#764 defect closed duplicate (duplicate)

SSL connection hangs open with firefox, can't reverseproxy things on a HTTPS server.

Reported by: Tv Owned by: Tv
Priority: high Milestone:
Component: core Keywords:
Cc: exarkun, itamarst, Tv, jknight, thijs, ivank, jesstess Branch:
Author: Launchpad Bug:

Description


Attachments (2)

Twistedssl.diff (9.2 KB) - added by jknight 10 years ago.
pyOpenSSL.diff (2.1 KB) - added by jknight 10 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 10 years ago by Tv

[see http://divmod.org/svn/Nevow/sandbox/tv/https-proxy-hang for the code]

There's a bug somewhere that makes firefox hang in some situations
when talking to https URLs.

First of all, run "./createkey" to set up the SSL keys.

There are two ways to reproduce it:


The original way
================

This is how I discovered this bug.

A "public" server proxies some requests over to "private",
in effect

  browser --HTTP--> public --HTTP--> private

now, when browser uses HTTPS to talk to public, the page
never finishes loading.

  browser --HTTPS--> public --HTTP--> private

twistd --pidfile public.pid -noy public.tac
twistd --pidfile private.pid -noy private.tac

Use firefox to browse to http://localhost:8080/

Click on the plaintext link, see how the page completes loading.

Click on the HTTPS link, see how the spinner keeps spinning and the
mouse pointer has a clock. The connection never closes down.


The minimal way
===============

I _hope_ this is the same bug.

twistd --pidfile minimal.pid -noy minimal.tac

Browse to http://localhost:8084/ and see how the browser quickly
aborts loading the page.

Browse to https://localhost:8083/ and see how it hangs.


A Clue
======

I've traced the bug far enough to know the patch in makes-it-work.diff
eliminates this symptom.

comment:2 Changed 10 years ago by Tv

<foom> Tv: using ssltap, I see that firefox never sends back an SSL alert
<Tv> foom: I know that.
<foom> Tv: okay. So I guess the real question is, what does apache do. :)
<Tv> foom: Also, what's different from the case with no proxy -- because then
there is no hang.
<Tv> just normal apache2 does so much keep-alive that seeing the same thing
happen is quite hard.
<Tv> So is this about no content-length?
<Tv> That would explain why firefox shows busy -- it doesn't know it received
everything already.
<Tv> Hmm.
<foom> Tv: the difference with proxy is that the proxy forces connection close,
I think
<Tv> foom: Yeah, it seems to do that, but I thought that was just because it was
giving a non-chunked streaming result.
<foom> Tv: when firefox talks to apache it sends back a close alert just fine
<Tv> I bet the apache content had a content-length in it.
<foom> er actually I lie.
<foom> Apache sends a close alert and immediately shuts the socket
<Tv> foom: A-ha!
<foom> and *then* firefox responds with a close alert. :P
<Tv> foom: I think it does a shutdown and not a real close, then.
<Tv> I don't see why you couldn't half-close the socket after saying you will
never say anything again
<Tv> Is there something against that in the SSL spec?
<foom> Tv: i'm using a proxy, so, I don't know if it ever gets back to apache or
not.
<foom> Tv: but I don't think a shutdown works because ssl needs two way
communications even for one way sending, IIRC
<Tv> Even for the final alert?
<Tv> I don't see any two-way for that in ethereal.
<Tv> Way too long keep-alive, this is horrible to debug with apache2<->firefox.
<Tv> The connection doesn't close for atleast a minute.
<foom> Tv: you can turn off keepalive in apache config somehow
<foom> okay, here we go:
<foom>     else if (ap_ctx_get(conn->client->ctx,
"ssl::flag::accurate-shutdown") == PTRUE) {
<foom>         /* send close notify and wait for clients close notify          
                                  
<foom>            (standard compliant, but usually causes connection hangs) */
<foom>     else {
<foom>         /* send close notify, but don't wait for clients close notify   
                                  
<foom>            (standard compliant and safe, so it's the DEFAULT!) */
<foom> so basically, firefox (and likely all mozillas before it) is a piece of
crap, so apache works around
<Tv> foom: I just talked to https://sourceforge.net/ with firefox, and I see
e.g. alert and FIN in the same package.
<Tv> foom: Hmm, that says that both modes of operation are standards compliant.
<foom> aha.
<foom> according to the TLS RFC, it is acceptable to only send the ``close
notify'' alert but to not wait for the peer's answer, when the underlying
connection is closed
<foom> (according to openssl docs)
<foom> I believe that more than I believe mod_ssl source.
<foom> In that case, I think we can remove the wait-for-other-side-to-send-close
bit from twisted.
<foom> but, pyopenssl is missing the function we need
<foom> :(
<foom> twisted should call SSL_set_shutdown(SSL_RECEIVED_SHUTDOWN) and then call
SSL_shutdown
<foom> but it can't do the first
<foom> perhaps you'd like to lobby for that function to be added to pyopenssl
<foom> actually, there's a .6
<foom> lemme see if that has it
<foom> nope.
<Tv> foom: Err, what does that mean? That the functionality cannot be
implemented without adding a wrapper for one more C function?
<foom> Tv: yes./
<Tv> foom: Err, surely it _can_. I mean, I already kludged it :) You mean
without that extra function the session state is lost and must be renegotiated?
<Tv> foom: (so you can detect whether pyopenssl has it, and use it if not, and
just break SSL sessions if pyopenssl is too old to have it)
<Tv> s/not/it has/
<foom> Tv: i'm not entirely sure what breaks if you don't set that, but mod_ssl
does it that way, so I'm assuming it's for a reason. 
<foom> Tv: I think because SSL_shutdown uses that flag to determine when it's done
<Tv> The worst you can loose is speedy startup of next TCP connection.
<foom> Tv: you could also lose data
<Tv> Hmm.
<foom> Tv: if you send a bunch of data and then immediately close the
connection, you could lose some of it
<Tv> No, wait.
<Tv> What is the part that makes SSL wait for the incoming shutdown?
<Tv> Does it do that always if SSL_shutdown is called and it so far hasn't seen one?
<Tv> <foom> twisted should call SSL_set_shutdown(SSL_RECEIVED_SHUTDOWN) and then
call SSL_shutdown
<Tv> and that basically lies to it?
<foom> Tv: yeah, SSL_shutdown returns 0 which means not done yet
<Tv> foom: I think what your saying is not 100% in agreement with man SSL_shutdown
<foom> Tv: maybe mod_ssl is being overprotective. let me go read the ssl docs again.
<Tv> I don't see why you'd need to set SSL_RECEIVED_SHUTDOWN manually.
<Tv> foom: I mean, the man page says one call for unidirectional (what we want),
two calls for bidirectional (what twisted does now).
<foom> Tv: yes, i concur.
<Tv> foom: But I don't know how one ensures SSL has flushed data, in general.
That's still needed after SSL_shutdown
<foom> Tv: I think you can assume it has after shutdown returns 0 or 1
<Tv> But it seems there's nothing special in that, it's the same way as with any
normal data..
<foom> Tv: i notice we don't properly handle the error conditino
<Tv> Heh.
<foom> Tv: SSL_ERROR_WANT_WRITE/READ from SSL_shutdown is unhandled
<foom> Tv: i think that means if you send a lot of data, and then lose the
connection, the shutdown will not occur and furthermore the socket will be
closed without finishing sending the data.

comment:3 Changed 10 years ago by Tv

<foom> Tv: okay, I've got it now. :) _postLoseConnection should send the
shutdown (and if it fails, retry when socket becomes readable/writable), and
lose the socket. _closeWriteConnection (from halfCloseConnection write=True)
should call shutdown (and retry etc) but not lose the socket.

Changed 10 years ago by jknight

comment:4 Changed 10 years ago by jknight

Working on it. Turns out SSL_shutdown is a piece of crap, so some workaround is necessary. Attached
patch may do the right thing. Also need the patch for pyOpenSSL .6 to have get/set_shutdown. I 
submitted it to martin strakt, hopefully it'll get in next pyOpenSSL.

I don't think there's a way to do the right thing without that call.

Changed 10 years ago by jknight

comment:5 Changed 10 years ago by jknight

Tv: Please verify fixed in trunk and resolve if it's good now.

comment:6 Changed 8 years ago by exarkun

  • Component set to core

Ping. Earth to Tv? Any comments on this?

comment:7 Changed 8 years ago by jknight

I don't believe above patch ever got into pyOpenSSL, although I admit to not having checked very recently. :(

comment:8 Changed 8 years ago by exarkun

I think Strakt is maintaining PyOpenSSL now. Maybe we should toss the patch over to someone there?

comment:9 follow-up: Changed 6 years ago by exarkun

The PyOpenSSL patch has been applied, at least. It's in the PyOpenSSL repository now hosted on Launchpad. As soon as I get a release together, it will be part of it.

comment:10 in reply to: ↑ 9 Changed 6 years ago by collab

  • Cc collab added

Replying to exarkun:

The PyOpenSSL patch has been applied, at least. It's in the PyOpenSSL repository now hosted on Launchpad. As soon as I get a release together, it will be part of it.

Guess this one can be closed, the patch was included in PyOpenSSL 0.7, released on 2008-04-11.

comment:11 Changed 6 years ago by exarkun

I think Twisted is missing test coverage for this feature. Also, it's broken. See #3218

comment:12 Changed 6 years ago by thijs

  • Cc thijs added; collab removed

comment:13 Changed 5 years ago by ivank

  • Cc ivank added

comment:14 Changed 5 years ago by exarkun

I think #3218 was kind of a duplicate of this ticket? I don't remember why I didn't comment about that fact earlier. If that's right, then since #3218 is resolved now, perhaps this is resolved as well. #3218 made the SSL_shutdown code work (by fixing the bad select() interaction) and added test coverage for the cases where it's necessary.

Anyone agree or disagree?

comment:15 Changed 5 years ago by jesstess

  • Cc jesstess added

comment:16 Changed 5 years ago by exarkun

  • Resolution set to duplicate
  • Status changed from new to closed

Well then. I'll go with that last thing I said.

Note: See TracTickets for help on using tickets.