bugfix: setkeepalive failure on TLSv1.3

[catbro666]

When TLSv1.3 is used, the server may send a NewSessionTicket message after the handshake. While this message is ssl-layer data, tcpsock:sslhandshake does not consume it.

In the implementation of setkeepalive, recv is used to confirm the connection is still open and there is no unread data in the buffer. But it treats the NewSessionTicket message as application layer data and then setkeepalive fails with this error connection in dubious state.

In fact we don't need to peek here, because if the application data is read successfully then the connection is going to be closed anyway. Therefore, c->recv can be used instead which will consume the ssl-layer data implicitly.

A quick reproduction:

openssl s_server --debug -msg -port 11443 -cert server.crt -key server.key

$ cat test.lua
local host = "127.0.0.1"
local port = 11443
local say = ngx.say

local sock = ngx.socket.tcp()
sock:settimeouts(60, 60, 1)

for i=1, 2 do
  assert(sock:connect(host, port))

  local times = assert(sock:getreusedtimes())
  say("reusedtime " .. times)

  if true and times == 0 then
    assert(sock:sslhandshake(false, nil, false))
  end

  local message = "hello"
  assert(sock:send(message .. "\n"))
  say("sent: ", message)

  -- sleep a while to make sure the NewSessionTicket message has arrived
  ngx.sleep(2)

  local ok, err = sock:setkeepalive(60000, 200)
  if not ok then
    say("failed to keepalive to ", host, ":", tostring(port), ": ", err)
    sock:close()
  end
end

$ resty --http-conf 'lua_ssl_protocols TLSv1.3;' test.lua
reusedtime 0
sent: hello
failed to keepalive to 127.0.0.1:11443: connection in dubious state
reusedtime 0
sent: hello
failed to keepalive to 127.0.0.1:11443: connection in dubious state

After the fix:

$ resty --http-conf 'lua_ssl_protocols TLSv1.3;' test.lua
reusedtime 0
sent: hello
reusedtime 1
sent: hello

I hereby granted the copyright of the changes in this pull request
to the authors of this lua-nginx-module project.

[mergify]

This pull request is now in conflict :(

[catbro666]

@zhuizhuhaomeng I just noticed there was already a corresponding fix in 0.10.27, but I still changed the code to make it simpler and added a test.

It seems the original fix has a risk:

    n = recv(c->fd, buf, 1, MSG_PEEK);
    err = ngx_socket_errno;
#if (NGX_HTTP_SSL)
    /* ignore ssl protocol data like change cipher spec */
    if (n == 1 && c->ssl != NULL) {
        n = c->recv(c, (unsigned char *) buf, 1);
        if (n == NGX_AGAIN) {
            n = -1;
            err = NGX_EAGAIN;
        }
    }
#endif

    if (n == -1 && err == NGX_EAGAIN) {
        /* stale event */

If recv returns 1, ngx_socket_errno will be a random value which may be NGX_EAGAIN. And if c->recv returns NGX_ERROR which equals -1, then the condition n == -1 && err == NGX_EAGAIN will be true.