Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WarpTLS cannot teminate TCP connections if TLS handshake is not done #983

Closed
kazu-yamamoto opened this issue Apr 18, 2024 · 1 comment · Fixed by #984
Closed

WarpTLS cannot teminate TCP connections if TLS handshake is not done #983

kazu-yamamoto opened this issue Apr 18, 2024 · 1 comment · Fixed by #984
Assignees
Labels

Comments

@kazu-yamamoto
Copy link
Contributor

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.

@kazu-yamamoto
Copy link
Contributor Author

Uhhm, registerKillThread requires connClose conn.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants