Skip to content

Don't let crypton-connection resolve hostname #585

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.

Sign up for GitHub

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

Open
sol wants to merge 1 commit into
base: main
Choose a base branch
from
+7 −6
Open

Don't let crypton-connection resolve hostname #585

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions http-client-tls/Network/HTTP/Client/TLS.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,14 +125,14 @@ getTlsConnection :: Maybe NC.ConnectionContext
-> IO (Maybe HostAddress -> String -> Int -> IO Connection)
getTlsConnection mcontext tls sock = do
context <- maybe NC.initConnectionContext return mcontext
return $ \_ha host port -> do
return $ \ha host port -> do
let params = NC.ConnectionParams
{ NC.connectionHostname = strippedHostName host
, NC.connectionPort = fromIntegral port
, NC.connectionUseSecure = tls
, NC.connectionUseSocks = sock
}
withConnection context params
withConnection context params ha host port
convertConnection

getTlsProxyConnection
Expand All @@ -142,7 +142,7 @@ getTlsProxyConnection
-> IO (S.ByteString -> (Connection -> IO ()) -> String -> Maybe HostAddress -> String -> Int -> IO Connection)
getTlsProxyConnection mcontext tls sock = do
context <- maybe NC.initConnectionContext return mcontext
return $ \connstr checkConn serverName _ha host port -> do
return $ \connstr checkConn serverName ha host port -> do
let params = NC.ConnectionParams
{ NC.connectionHostname = strippedHostName serverName
, NC.connectionPort = fromIntegral port
Expand All @@ -152,7 +152,7 @@ getTlsProxyConnection mcontext tls sock = do
Just _ -> error "Cannot use SOCKS and TLS proxying together"
Nothing -> Just $ NC.OtherProxy (strippedHostName host) $ fromIntegral port
}
withConnection context params $ \conn -> do
withConnection context params ha host port $ \conn -> do
NC.connectionPut conn connstr
conn' <- convertConnection conn

Expand All @@ -162,8 +162,9 @@ getTlsProxyConnection mcontext tls sock = do

return conn'

withConnection :: NC.ConnectionContext -> NC.ConnectionParams -> (NC.Connection -> IO a) -> IO a
withConnection context params = bracketOnError (NC.connectTo context params) NC.connectionClose
withConnection :: NC.ConnectionContext -> NC.ConnectionParams -> Maybe HostAddress -> String -> Int -> (NC.Connection -> IO a) -> IO a
withConnection context params ha host port action = withSocket (const $ pure ()) ha host port $ \socket -> do
NC.connectFromSocket context socket params >>= action
Copy link
Copy Markdown
Collaborator Author

@sol sol Jun 7, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Yuras regarding your original comment:

-- We won't send TLS bye in case of exception, but that's OK

That relates to NC.connectionClose, I guess. So I assume we don't properly terminate the connection, but we close the socket, which is technically all we care about.

Comparing connectTo to withSocket, there seems to be a lot of things going on in connectTo, including things related to SOCKS. So I can't trivially convince myself that this won't break the proxy code path (+ apparently that code path is also not covered by the test suite). I'll either have to look at it again when I got more sleep, or somebody has to test it. 😅

https://github.com/kazu-yamamoto/crypton-connection/blob/1af72aad1733046195e6f0025adcb7944fa75dad/Network/Connection.hs#L219


convertConnection :: NC.Connection -> IO Connection
convertConnection conn = makeConnection
Expand Down
Loading