yesodweb/wai

WarpTLS cannot teminate TCP connections if TLS handshake is not done

kazu-yamamoto opened this issue · 1 comments

Warp can terminate TCP connections even if no bytes are sent from clients.

% gtelnet 127.0.0.1 80
Trying 127.0.0.1...
Connected to 127.0.0.1.
Escape character is '^]'.
Connection closed by foreign host.

Unfortunately, WarpTLS cannot:

% gtelnet 127.0.0.1 443
Trying 127.0.0.1...
Connected to 127.0.0.1.
Escape character is '^]'.
... (waiting forever)

The difference is their mkConn functions. WarpTLS's one returns only after TLS handshake is done while Warp's one can immediately return.

Run.hs has the following function:

fork set mkConn addr app counter ii = settingsFork set $ \unmask ->
    -- Call the user-supplied on exception code if any
    -- exceptions are thrown.
    --
    -- Intentionally using Control.Exception.handle, since we want to
    -- catch all exceptions and avoid them from propagating, even
    -- async exceptions. See:
    -- https://github.com/yesodweb/wai/issues/850
    Control.Exception.handle (settingsOnException set Nothing) $
        -- Run the connection maker to get a new connection, and ensure
        -- that the connection is closed. If the mkConn call throws an
        -- exception, we will leak the connection. If the mkConn call is
        -- vulnerable to attacks (e.g., Slowloris), we do nothing to
        -- protect the server. It is therefore vital that mkConn is well
        -- vetted.
        --
        -- We grab the connection before registering timeouts since the
        -- timeouts will be useless during connection creation, due to the
        -- fact that async exceptions are still masked.
        UnliftIO.bracket mkConn cleanUp (serve unmask)
  where
    cleanUp (conn, _) =
        connClose conn `UnliftIO.finally` do
            writeBuffer <- readIORef $ connWriteBuffer conn
            bufFree writeBuffer

    -- We need to register a timeout handler for this thread, and
    -- cancel that handler as soon as we exit.
    serve unmask (conn, transport) = UnliftIO.bracket register cancel $ \th -> do
        -- We now have fully registered a connection close handler in
        -- the case of all exceptions, so it is safe to once again
        -- allow async exceptions.
        unmask
            .
            -- Call the user-supplied code for connection open and
            -- close events
            UnliftIO.bracket (onOpen addr) (onClose addr)
            $ \goingon ->
                -- Actually serve this connection.  bracket with closeConn
                -- above ensures the connection is closed.
                when goingon $ serveConnection conn ii th addr transport set app
      where
        register = T.registerKillThread (timeoutManager ii) (connClose conn)
        cancel = T.cancel

    onOpen adr = increase counter >> settingsOnOpen set adr
    onClose adr _ = decrease counter >> settingsOnClose set adr

The time handle is created after mkConn returns.
As the comment says, asynchronous exceptions do not reach to the resource acquire handler of bracket.
I guess that we should register timeout first then call mkConn without bracket.

Uhhm, registerKillThread requires connClose conn.