SSL_CLOSE_NOTIFY goes undetected leaving sockets hanging

[baudehlo]

Currently in Node if the remote end calls SSL_shutdown(), node only notices that if the user polls pair.ssl.receivedShutdown. This seems like a bug. There should be some sort of event fired when shutdown is received.

The effect of this is a socket that is on SSL that receives a shutdown will hang indefinitely - no timeout will fire.

In Haraka I had to add setInterval() code to poll this flag to shutdown the socket:

        var interval_timer = setInterval(function () {
            if (pair.ssl.receivedShutdown) {
                log.logdebug("Received Shutdown!");
                clearInterval(interval_timer);
                socket.end();
            }
        }, 1000);

Obviously I'd rather not have a new setInterval running for every single TLS connection.

I'm not sure how this should be handled internally in Node (or perhaps libuv) to better receive notification when the shutdown occurs. At the very least an event should probably be fired that we can pickup.

I had to write perl code to emulate this problem as there's no way to send a shutdown from node land. The basic gist of it is using Net::SSLeay to do the upgrade (well documented elsewhere), and then you send:

    print "Shutting down SSL...\n";
    my $rc = Net::SSLeay::shutdown($ssl);
    print "Got back: $rc\n";

Ugly, but it works. I have a custom version of swaks (SMTP testing tool) that you can test with Haraka with TLS enabled if that helps: https://gist.github.com/baudehlo/7796228

Run with: ./swaks.pl -q MAIL -tls -s 127.0.0.1:25

With stock Haraka, swaks will hang at the point it sends the shutdown() and eventually time out. Haraka will never timeout or see any event on the socket or the tls pair. The "bug fix"/hack above is currently in the ssl_shutdown_bug branch on Haraka's github.

[indutny]

I'll look into it soon.

[baudehlo]

FWIW the timer/close thing above doesn't seem to work very cleanly at all - it seems to mess up the internals, but that may be because Haraka has to use a shim module to implement STARTTLS (because Node doesn't provide a way to upgrade from the server side).

[indutny]

@baudehlo may I ask you to give a try to this patch: #6661 ?

[baudehlo]

I applied the patch against v0.10.22 and it seemed to do the trick. My swaks gets very confused, but I'm OK with that :)

It remains to be seen what Exim does - what Exim expects I think is to be able to return to communicating in plain text (and possibly even issue STARTTLS again), not for the server to drop the connection.

[indutny]

Hrm? I think it should not drop connection now, and should emit 'end' event instead.

[baudehlo]

That would be preferable.

However isn't that problematic since you don't get any sort of event firing
from openssl when the shutdown event occurs (effectively you have to poll
for it)?

On Mon, Dec 9, 2013 at 4:09 PM, Fedor Indutny notifications@github.comwrote:

Hrm? I think it should not drop connection now, and should emit 'end'
event instead.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/6638#issuecomment-30173499
.

[indutny]

In fact the event isn't happening by itself - it happens only after we read data from socket and write it to openssl. So checking it after encOut/clearOut is perfectly ok.

[baudehlo]

Ok, as long as that won't slow things down too much.

On Mon, Dec 9, 2013 at 4:15 PM, Fedor Indutny notifications@github.comwrote:

In fact the event isn't happening by itself - it happens only after we
read data from socket and write it to openssl. So checking it after encOut
/clearOut is perfectly ok.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/6638#issuecomment-30173966
.

[indutny]

Believe me it doesn't :) Does it work with Exim?

[baudehlo]

Presumably, but Exim is slower than Haraka anyway :)

On Mon, Dec 9, 2013 at 4:37 PM, Fedor Indutny notifications@github.comwrote:

Believe me it doesn't :) Does it work with Exim?

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/6638#issuecomment-30175911
.

[indutny]

Gosh :) May I ask you to give it a try? ;)

[baudehlo]

Of course, but I don't see a patch for emitting an end event yet, is there?

On Mon, Dec 9, 2013 at 4:46 PM, Fedor Indutny notifications@github.comwrote:

Gosh :) May I ask you to give it a try? ;)

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/6638#issuecomment-30176715
.

[indutny]

.push(null) does it.

[baudehlo]

I can't quite seem to make this work (both from the Perl end and the Node end).

From Perl I'm sending SSL_shutdown() and Node receives 'close' and then 'end' events on the SSL socket (not sure that order is right btw?). But then from Perl I send QUIT over the plaintext socket, but the Node end never sees it.

I'm not sure if I'm doing something wrong on the Perl side or the Node side now.

Are you ever on IRC for some realtime debugging of this?

[indutny]

I'm on hanging around on #libuv channel, but... it'd be better if you'll just provide a test case. I have a lot of other stuff to do, not sure if I'll be able to react at the time suitable for you.

[baudehlo]

OK let me see how far I get with tcpdump/wireshark first.