Implement socket=True in Container.exec_run via connection hijacking - #648
Conversation
d7bae46 to
28248df
Compare
inknos
left a comment
inknos
left a comment
There was a problem hiding this comment.
thanks for the PR. I saw you converted to draft so I'll stop reviewing, but here are a couple of comments.
I am also thinking if it will break if the socket is passed via ssh. The reason is, when you hijack the socket over http you can get a clean file descriptor for read/write operations. But when the connection is handled via ssh the python code overrides the read/write methods and you pipe to different ssh subprocess. This will give you different file descriptors and it will break.
So the SSH case needs to be handled with care. A quick solution would be to raise NotImplementedError for ssh connections to keep it simple.
| ) | ||
| start_resp.raise_for_status() | ||
|
|
||
| sock = start_resp.raw._connection.sock |
There was a problem hiding this comment.
I believe you need raw.connection instead of _connection
There was a problem hiding this comment.
also, I believe we need a None guard here in case the response is consumed or the socket is None for some reason
conn = start_resp.raw.connection
if conn is None or conn.sock is None:
raise RuntimeError("unable to extract socket from hijacked connection")
sock = conn.sock
not sure about the message, but I would avoid throwing a python error.
There was a problem hiding this comment.
Thanks for the review! I've addressed all three points:
- Fixed raw._connection → raw.connection (public property instead of the private urllib3 attribute).
- Added a None guard before extracting the socket:
conn = start_resp.raw.connection
if conn is None or conn.sock is None:
raise APIError(
f"/exec/{exec_id}/start",
explanation="Unable to extract socket from hijacked connection",
response=start_resp,
)
sock = conn.sockUsed APIError instead of a bare Python exception, consistent with the rest of the codebase.
- Added an explicit guard for SSH connections, raising NotImplementedError:
if self.client.api.base_url.scheme == "http+ssh":
raise NotImplementedError("exec_run(socket=True) is not supported over SSH")You're right that hijacking wouldn't work cleanly there since the socket ends up piped through an SSH subprocess rather than being a direct fd. Happy to look into proper SSH support as a follow-up if there's interest, but wanted to keep this PR focused on the UDS case.
Ready for another look whenever you have time.
There was a problem hiding this comment.
Used APIError
Makes sense
if self.client.api.base_url.scheme == "http+ssh":
raise NotImplementedError("exec_run(socket=True) is not supported over SSH")
Should be if self.client.base_url.scheme
Happy to look into proper SSH support as a follow-up if there's interest, but wanted to keep this PR focused on the UDS case
definitely, it might be/not be so trivial, but let's make it another PR
I'll give it another round of review tomorrow
There was a problem hiding this comment.
Should be
if self.client.base_url.scheme
Yes, I saw that and fixed it in the next commit, but thanks anyway.
9fd320e to
5ee8317
Compare
5ee8317 to
199974c
Compare
|
/packit test |
fed0694 to
4567fdd
Compare
|
The testing-farm:fedora-rawhide-x86_64:distro-fedora-all job fails deterministically (retriggered via /packit test, same result), but I'm unable to reproduce it locally: Full unit test suite passes on my machine (274 passed), with both pytest and unittest discover, including with the latest urllib3/requests/requests-mock All other testing-farm targets (fedora 43/44, centos-stream 14/15, epel, unittest-coverage) pass. The truncated failure log only shows urllib3 exhausting retries (raise reraise(...) in urllib3/util/retry.py). |
Implement connection hijacking over the exec start endpoint so exec_run(socket=True) returns the raw socket, matching docker-py's return contract (None, socket). The response is anchored on the socket object to prevent urllib3 from reclaiming the hijacked connection. Fixes containers#421 Signed-off-by: Giovanni <oligiovi2@gmail.com>
4567fdd to
9aa49c1
Compare
|
/packit test |
|
/packit retest-failed |
CI failure analysis:
|
|
your pr looks good, thanks! also, great investigation on the failures, I will go and take a look at podman 6.0.1 changes. |
on second thought, I believe it could be even infrastructure related. I couldn't reproduce locally on my system. I'll try a rawhide box to check if it's rawhide related |
If it turns out to be infrastructure-related, I'd be happy to help investigate further. |
it is and it's related to SELinux policies in ssh. I was checking, and I am almost at the bottom of it, I'll share what I found. I reproduced on a rawhide box, with SELinux enabled. If you set it to permissive you'll get the test passing with this line in the logs (or similar) I am not that fluent in SELinux, but I believe ssh forwardings run in the sshd-session binary, which Fedora's SELinux policy assigns to the new |
The test_ssh_ping CI failure has been root-caused as an SELinux issue unrelated to this PR. The OpenSSH 9.8+ split of sshd into sshd/sshd-session means SSH forwarding now runs as sshd_session_t, which lacked connectto on container_runtime_t sockets. Tracked and fixed in containers/container-selinux#478 and containers/container-selinux#479. Since the failure is confirmed environmental and independent of these changes, would you prefer to merge this PR now, or wait for the policy fix to land on the Testing Farm runners? |
|
ok, since we are gating podman-py again before the next release it can be merged |
thank you so much |
sure and thanks a lot for your contribution! |
2 participants
The libpod REST API supports upgrading the exec start request to a raw bidirectional stream (101 Switching Protocols). Send the Upgrade headers, keep the response unconsumed (stream=True) and hand the caller the underlying UDS socket, anchoring the response to keep the hijacked connection out of the urllib3 pool.
Fixes #421
Signed-off-by: Giovanni Oliverio oligiovi2@gmail.com
Fixes #421
Problem
Container.exec_run()accepts asocketparameter (documented as "Returnthe connection socket to allow custom read/write operations") but the
parameter is silently ignored: the call always blocks until the command
exits and returns the buffered output. This makes interactive exec
sessions (a shell with live stdin/stdout, terminal emulators, REPLs)
impossible with the native bindings, while the same use case works with
docker-py against Podman's compatibility socket.
The limitation is client-side only: the libpod REST API fully supports
hijacking the exec start request into a raw bidirectional stream.
Solution
When
socket=True, send thePOST /exec/{id}/startrequest withConnection: Upgrade/Upgrade: tcpheaders andstream=True(sorequestsdoes not consume the body). The server replies101 Switching Protocolsand the underlying connection stops being HTTP:it becomes a raw bidirectional byte channel attached to the exec
session's stdin/stdout/stderr (raw stream with
tty=True, multiplexedframes otherwise). The socket is extracted from the response via
response.raw._connection.sock—_connectionhere is podman-py's ownUDSConnection(podman/api/uds.py), which keeps a reference to itssocket, so the only private attribute traversed is urllib3's
HTTPResponse._connection.The response object is anchored on the returned socket
(
sock._hijacked_response) so the hijacked connection cannot be releasedback to the urllib3 pool while the caller is using it; a later request on
the same client would otherwise write HTTP into the exec session.
Detachis forced toFalsein this branch: a socket to a detachedsession is meaningless (consistent with docker-py behaviour).
Return contract matches docker-py:
(None, socket); the caller isresponsible for reading, writing and closing the socket.
Testing
socket=Trueissues the start request withthe Upgrade headers,
stream=TrueandDetach: false, and returns theextracted socket (extraction helper mocked, since the hijack cannot
happen against requests_mock).
/bin/shviaexec_run(socket=True), writes a command computing a marker and readsit back from the live socket.
podman-py 5.8.0: upgrade returns 101, the channel is bidirectional
while the command is still running, terminal resize via
/exec/{id}/resizeis honoured, exit codes are reported by/exec/{id}/json, and concurrent requests on the same client open aseparate pool connection without corrupting the hijacked stream.