A patch to https

[Wei]

Hi, all

I found my https server (powered by mochiweb) blocked and rejected new
connections after running for days. After capturing a coredump and
analyzing it, I found all 16 acceptor processes were blocked at
ssl:ssl_accept (mochiweb_socket:accept/1).

This problem can be reproduced like this:

wei@t-mingsong:~$ irb
irb(main):001:0> require 'socket'
=> true
irb(main):002:0> sockets = (1..16).map{|i| TCPSocket.open('127.0.0.1',
8080) }
=> [#<TCPSocket:0x7f9af3508a08>, ... , #<TCPSocket:0x7f9af3507388>]

At this time, all acceptors're blocked and hangs over incoming wget
request forever.

wei@t-mingsong:~$ wget https://127.0.0.1:8080 --no-check-certificate
--2011-12-12 16:29:52-- https://127.0.0.1:8080/
Connecting to 127.0.0.1:8080...

Here is my patch,

https://github.com/weicao/mochiweb/compare/fix_acceptor_blocked_in_ssl
https://github.com/weicao/mochiweb/compare/fix_acceptor_blocked_in_ssl.patch

[Rory Byrne]

On Mon, Dec 12, 2011 at 12:54:19AM -0800, Wei wrote:
> I found my https server (powered by mochiweb) blocked and rejected new
> connections after running for days. After capturing a coredump and
> analyzing it, I found all 16 acceptor processes were blocked at
> ssl:ssl_accept (mochiweb_socket:accept/1).
>

I can confirm this problem. I'm to blame for this one since I submitted
the initial ssl patch.

> Here is my patch,
>
> https://github.com/weicao/mochiweb/compare/fix_acceptor_blocked_in_ssl
> https://github.com/weicao/mochiweb/compare/fix_acceptor_blocked_in_ssl.patch

I would write the patch slightly differently (see attached) for the
following reasons:

1. The way you've added in the extra call to mochiweb_socket:after_accept/1
will result in all SSL errors getting ignored instead of being logged.

2. The purpose behind the mochiweb_socket module was to be able to treat
SSL and non-SSL sockets in the same way. The extra call to
mochiweb_socket:after_accept/1 only applies to SSL sockets, so
it goes against the spirit of the module in my opinion.

At a guess, I would say that Bob might be more likely to accept your patch
if you change it to being more like the one I've attached. Full credit to
you though Wei!

Thanks,

Rory

[Wei Cao]

2011/12/14 Rory Byrne <ro...@jinsky.com>

On Mon, Dec 12, 2011 at 12:54:19AM -0800, Wei wrote:
> I found my https server (powered by mochiweb) blocked and rejected new
> connections after running for days.  After capturing a coredump and
> analyzing it, I found all 16 acceptor processes were blocked at
> ssl:ssl_accept (mochiweb_socket:accept/1).
>

I can confirm this problem. I'm to blame for this one since I submitted
the initial ssl patch.

 

Your ssl patch is of great help and time-saving to add https layer to our web API server,

thanks a lot ~

 

> Here is my patch,
>
> https://github.com/weicao/mochiweb/compare/fix_acceptor_blocked_in_ssl
> https://github.com/weicao/mochiweb/compare/fix_acceptor_blocked_in_ssl.patch

I would write the patch slightly differently (see attached) for the
following reasons:

 1. The way you've added in the extra call to mochiweb_socket:after_accept/1
   will result in all SSL errors getting ignored instead of being logged

 

hm.. SSL errors can still be caught by the caller of mochiweb_socket:after_accept/1 in mochiweb_acceptor, and be logged if necessary, isn't it?

 

 2. The purpose behind the mochiweb_socket module was to be able to treat
   SSL and non-SSL sockets in the same way. The extra call to
   mochiweb_socket:after_accept/1 only applies to SSL sockets, so
   it goes against the spirit of the module in my opinion.

 

For SSL acceptor, the most efficient actions should be:

1. listen until a new connection in

2. send a message to notify mochiweb_socket_server to create a new acceptor

3. finish the SSL handshake

4. go to the main loop

 

Https server can avoid be blocked by idle connections only in this way, the key point

is we cannot wait to create new acceptor until handshake finishes.

 

yeah, it's a quite beautiful to treat SSL and non-SSL in the same way, it' a hight level

abstraction which looks like object oriented programming skills, here the problem is,

 the abstraction granularity of mochiweb_socket:accept/1 is a little coarse for SSL,

and it's better to split it into two steps (1 and 3) and inject step2 between them.

So I add an additional function after_accept/1 to handle SSL handshake, and it doesn't

violate the abstractions since nothing is done for non-SSL connection.

 

At a guess, I would say that Bob might be more likely to accept your patch
if you change it to being more like the one I've attached. Full credit to
you though Wei!

hm.. setting timeout to SSL handshake cannot solve the problem thoroughly.

 

Thanks,

Rory

--
You received this message because you are subscribed to the Google Groups "MochiWeb" group.
To post to this group, send email to moch...@googlegroups.com.
To unsubscribe from this group, send email to mochiweb+u...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/mochiweb?hl=en.

--

Best,

Wei Cao

[Rory Byrne]

On Wed, Dec 14, 2011 at 11:51:28PM +0800, Wei Cao wrote:
> 2011/12/14 Rory Byrne <ro...@jinsky.com>

> > 1. The way you've added in the extra call to
> > mochiweb_socket:after_accept/1
> > will result in all SSL errors getting ignored instead of being logged
>
>
> hm.. SSL errors can still be caught by the caller of
> mochiweb_socket:after_accept/1 in mochiweb_acceptor, and be logged if
> necessary, isn't it?
>

Was really just pointing out that up till now when ssl:ssl_accept/1
returned {error, reason()} the reason() part would be logged if it
wasn't 'closed', 'timeout' or 'esslaccept'.

>
> For SSL acceptor, the most efficient actions should be:
> 1. listen until a new connection in
> 2. send a message to notify mochiweb_socket_server to create a new acceptor
> 3. finish the SSL handshake
> 4. go to the main loop
>
> Https server can avoid be blocked by idle connections only in this way, the
> key point
> is we cannot wait to create new acceptor until handshake finishes.
>

Yep, you're totally right. My mistake.

Nice fix! :-)

[Bob Ippolito]

I created an issue to investigate this [1]. The original pull request is probably better because the latter patch of just adding a timeout to SSL accept is still susceptible to a "slowloris" style attack since it doesn't send the cast until after the SSL connection is established and an attacker can make that last up to the full timeout. I'll have to think about this a bit to see if there's a more elegant possible solution.

SSL is not something we do with mochiweb so it's never received much attention at Mochi. We do SSL with hardware load balancers.

[1] https://github.com/mochi/mochiweb/issues/70

-bob