NIO SSL server infinite loops on close-notify

[jkeljo]

If a client sends a close-notify alert prior to closing the connection, the NIO HttpsServerHelper goes into an infinite loop in the ready loop in Connection.onSelected. This happens on both Android and JSE. The Android client appears to send a close-notify in some scenarios, and curl (on MacOSX) does it every time.

What appears to be happening is the following:

1. When the close-notify is received by the server, its SSLEngine returns status=CLOSED handshake_status=NEED_WRAP. (The NEED_WRAP is because it wants to send its own close-notify back to the client.)
2. In the post-processing of the network queue (ReadableSslChannel.postProcess), SslConnection.handleSslResult is called, which marks the connection Closing and clears the SSL result
3. In the post-processing of the application queue (HttpsServerInboundWay.postProcess), SslConnection.handleSslResult is called again. This time, it assumes a status of OK (because the result had been cleared), and calls handleSslHandshake().
4. handleSslHandshake() sees the NEED_WRAP handshake status and sets the outbound way's IO State to READY
5. The SSLEngine produces its own close-notify message, then immediately begins returning NOT_HANDSHAKING. It never returns FINISHED, so the state of the outbound way is never changed from READY, and the ready loop continues forever.

[jkeljo]

I don’t understand why handleSslResult is called both on post process of the network buffer (ReadableSslChannel) and the application buffer (HttpsOutboundWay). Each call clears the result, so the second call is guaranteed to always process the result as if it were OK. Anyone have input on that?

[jkeljo]

I see a few possible changes that would prevent the infinite loop, however I’m new to this code so I’m not too sure whether any of the ones I see are the “right fix”:

1. Treat a NOT_HANDSHAKING received after a NEED_WRAP as a FINISHED. This allows the server to send its own close-notify and then exit the ready loop.
2. Ignore handshake status for a given SSLEngine after the initial handshake is complete. With this solution, the server would not send its own close-notify, but it would still exit the ready loop.
3. Remove the handleSslResult calls during post processing of the application buffer, so that handleSslResult is called exactly once per SSL result. I haven’t tried this one yet, and it feels like a scarier change because it’s been that way for a long time and I don’t really understand why it was there in the first place.

I have tried the first two (as workarounds via my own SslContextFactory) and they do prevent the infinite loop. The first seems to me the way to go, but I really haven't grokked this code yet, so I could use some advice.

[jlouvel]

Thanks Jonathan for the investigation. I had a quick look at the code and I don't remember an obvious reason why handleSslResult is called several times.

I wouldn't be afraid to try the third change as well as long as it works in your use cases. This connector isn't considered stable yet, so no big worries about breaking things as long as unit test properly work.

Especially make sure you set "enableClientNio" to true in the SslBaseConnectorsTestCase class and that all SSL related test cases work properly. If this is the case, we should be fine.

Also, note that I just pushed some packages refactoring in Restlet engine and NIO extension. No impact on the actual logic, just clearer packages structure.

[jkeljo]

The third change didn't do it; the initial handshake was unable to complete without the extra call. However, even if that change had done it, it wouldn't have sent the close-notify and thus wouldn't have conformed to the SSL spec. More edits would have been required.

Since the first change addresses both this issue and the first part of #852, I decided to go with it in my pull request. See what you think...

[jlouvel]

Looks great, thanks Jonathan. Closing