Expose socket connection interface in connection

[jborean93]

I've working on an custom httpx transport which uses Kerberos/NTLM/Negotiate authentication when talking to a peer. These authentication methods have a custom extension when talking over TLS called channel binding tokens (CBT) where you embed a structure that is based on information from the TLS context which in essence binds the authentication against the TLS context making it harder to achieve MitM attacks.

A version of CBT that I'm currently using is documented in RFC 5929 which is essentially tls-server-end-point:<cert hash>. In the past I've just send a request to the host, gotten the SSL object from the socket in the response but this has a few problems:

• You waste a round trip sending a request you know will fail
  • Somewhat more annoying because in TLS there are more than 1 roundtrips to set up the TLS context
• You need to have set up the connection before you can build the authentication context and generate the first auth token
• If the connection is closed in the auth request then the CBT data you used might not be correct anymore

The last point is problematic because in my scenario I send a blank request which sets up a connection and the response I get back is a 401. Typically I've seen a Connection: close header which has the *ConnectionPool classes drop the connection from the pool. This means the next request is a brand new connection with a brand new negotiated TLS context.

For the tls-server-end-point I could still get the certificate hash as that should be static between the connections but with new protocols like TLS 1.3 the draft proposal for CBT seems to indicate it's based on the unique connection set up Breaking authentication.

What I'm hoping is for is to expose the _open_socket method publically in some shape or form. This would allow me to create the socket connection and set up the TLS request before I send a request through (s)request on the connection. I feel like this could potentially be done by setting up the socket myself and passing it into the __init__() for *HTTPConnection or by just making that method more public by removing the _. This also hinges on my other question around whether connections are actually meant to be publicly exposed in #272.

Very interested to hear your thoughts on this as I do have a working prototype for all this but it relies on being able to access AsyncHTTPConnection and calling _open_socket manually before sending the request through (a)request.

[jborean93]

When thinking about this a bit more, it sounds like having an open/aopen method that is purely designed to open the connection. I'll see if I can create a PR to show you what I mean.

[jborean93]

I've just opened a draft PR that exposes the functionality I need as a public interface #274 in what I think is a suitable implementation. Happy to hear your thoughts on the approach and whether there's anything you wish to change or another approach you recommend.

[florimondmanca]

One thing that hits me looking at #274 and our code is that, although AsyncHTTPConnection is defined as a subclass of AsyncHTTPTransport (it "is" a transport), we don't use it as such internally.

See:

httpcore/httpcore/_async/connection_pool.py

Lines 201 to 226 in 12a25fd

	connection: Optional[AsyncHTTPConnection] = None
	while connection is None:
	async with self._connection_acquiry_lock:
	# We get-or-create a connection as an atomic operation, to ensure
	# that HTTP/2 requests issued in close concurrency will end up
	# on the same connection.
	logger.trace("get_connection_from_pool=%r", origin)
	connection = await self._get_connection_from_pool(origin)
	if connection is None:
	connection = self._create_connection(origin=origin)
	logger.trace("created connection=%r", connection)
	await self._add_to_pool(connection, timeout=timeout)
	else:
	logger.trace("reuse connection=%r", connection)
	try:
	response = await connection.arequest(
	method, url, headers=headers, stream=stream, ext=ext
	)
	except NewConnectionRequired:
	connection = None
	except Exception: # noqa: PIE786
	logger.trace("remove from pool connection=%r", connection)
	await self._remove_from_pool(connection)
	raise

In here we do a dance to acquire an existing connection from the pool, create one if needed, send a request, and then remove the connection from the pool.

If AsyncHTTPConnection is to be a proper transport, then we should use it like a transport, i.e. we should call into __aenter__(), arequest() and __aexit__()/aclose(). Right now we only do arequest() (and aclose() when the pool gets closed). That's a bit awkward, and worth looking into before looking into any new hooks, I think.

[florimondmanca]

@jborean93 About #274 and the "allow connections to open their sockets via a dedicated method" idea… Also, cc @tomchristie — happy to hear your thoughts about this?

Circling back to my last point above, actually transports are really thought of as classes with two methods: arequest(), and aclose(). Transports aren't really designed to "open things upon creation", at least not within the transport class itself. This is close enough to Trio's "async resource" idea, which is basically just an object with an aclose() method. In Trio, these "async resources" are typically created using async context managers, like open_something(), which manage the creation of the underlying I/O resources and pass them to the class constructor. Like this…

@asynccontextmanager
async def open_something():
    socket = await open_socket()
    something = Something(socket)
    try:
        yield something
    finally:
        await something.aclose()

Which is used like this:

async with open_something() as something:
    ...

But although our transports expose the same kind of "constructor + aclose()" style, we don't really use them with the proper semantics always.

For example, AsyncHTTPConnection deals with its own "open socket" internally by calling it upon first arequest(). That's kind of clunky. Ideally, we'd open a socket beforehand, and pass it to AsyncHTTPConnection(socket, ...). We don't do this right now (except for tunnel proxying, in which we pass an existing socket), and I think it's a problem that begs for a small refactor.

So, we could switch to a style in which we have AsyncConnectionPool manage the opening of sockets, and pass them down to connections. It would work, and I have a local branch that passes tests to prove it. :-) See #275

To customize "open socket" behavior, one would have to either override AsyncConnectionPool._open_socket() (if using connection pooling), or — if a single connection transport is enough — copy that code to open a socket before passing it to AsyncHTTPConnection(socket=...).

from httpcore._async.http import AsyncHTTPConnection

async def open_custom_socket(...):
    ...

socket = await open_custom_socket(...)
conn = AsyncHTTPConnection(socket=socket, origin=...)

response = await conn.arequest(...)

@jborean93 What would be the impact of this on your use case?

[jborean93]

If AsyncHTTPConnection is to be a proper transport, then we should use it like a transport, i.e. we should call into aenter(), arequest() and aexit()/aclose()

Yea there seems to be a mixture of transport behaviours but ultimately it does things right. The reason why I thought adding aopen() was because it can ensure that anything that opens a socket or some other resource for an endpoint can be controlled by both __aenter__() -> aopen() and __aexit__() -> aclose(). So if a transport uses a network resource like a socket it can open and close them as needed but if not then it just does nothing.

We don't do this right now (except for tunnel proxying, in which we pass an existing socket), and I think it's a problem that begs for a small refactor.

I can't really question how things work here as I'm mostly fumbling around but this is what I see with the 3 classes of transports being used by httpcore in a normal operation

• AsyncConnectionPool
  • This is public, seems to be the only public concrete (async) AsyncHTTPTransport implementation in httpcore
  • As you've said it maintains a pool of connections and just grabs/creates a AsyncHTTPConnection as needed
  • aclose() will call aclose() on all the connections in the pool
  • For my purposes it isn't usable as I have no public way to indicate a new connection was made
• AsyncHTTPConnection
  • This is effectively a wrapper around AsyncHTTP11Connection (and the other HTTP version specific connections)
  • It creates the socket and version specific connection using that socket on the first call to arequest()
  • aclose() will call aclose() on the HTTP version specific connection it had created
• AsyncHTTP11Connection
  • This one handles the HTTP logic itself and the direct reading/writing to the socket
  • It is initialised with an already connected socket
  • aclose() will close the H11 state machine and the socket that it is using

So we already do have actual instances where we pass in a socket (ignoring tunneled proxies) I was mostly just trying to mirror the close method with the relevant open functionality that a transprot may or may not implement.

So, we could switch to a style in which we have AsyncConnectionPool manage the opening of sockets, and pass them down to connections. It would work, and I have a local branch that passes tests to prove it. :-) See #275

Whatever route you would like to go forward I'm happy to follow, all I need is a way to open the connection and do the TLS handshake before I prepare my actual HTTP request. Right now that's impossible to do without calling some private methods, or re implementing the whole shebang. Still what you have in that PR will work for my scenario, I can manually create and connect to the socket myself, the next step is to just make the connection class public :) (totally understand why the project is not prepared to do that right now).

What would be the impact of this on your use case?

The method you have chosen works for my use case and I will be happy to adopt it once the underlying connection is made public. For now I've just re implemented (shamelessly copied) AsyncHTTP11Connection and have a thing transport over the top that manages the connection in a pool-less scenario https://github.com/jborean93/pypsrp/blob/asyncio/psrp/_wsman/_async.py. I'll probably copy your create the socket and open method in the parent transport and use that socket in the HTTP11 connection rather than the aopen() method I went with.

[tomchristie]

If we were going to support something like this my feeling is that we'd do so without changing which classes we expose, or exposing any additional methods, but instead by supporting a {"socket": ...} option in the extensions. So...

with httpcore.SyncConnectionPool() as http:
    socket = ...
    status_code, headers, stream, extensions = http.handle_request(
        method=b'GET',
        url=(b'https', b'example.org', 443, b'/'),
        headers=[(b'host', b'example.org'), (b'user-agent', b'httpcore')]
        stream=httpcore.ByteStream(b''),
        extensions={'socket': socket}
    )
    body = stream.read()
    print(status_code, body)

This kind of style then allows us to open advanced use cases up to httpx as well, so...

httpx.get("https://www.example.org/", extensions={"socket": socket})

[tomchristie]

There's a similar case wrt. supporting HTTP/2's never_indexed_headers...

https://github.com/encode/httpcore/discussions/301#discussioncomment-557209