test(tunnel): cover SSH over HTTPS CONNECT#953
Conversation
📝 WalkthroughWalkthroughAdds an SSH-over-HTTPS-CONNECT end-to-end test, an executable CONNECT proxy fixture, lifecycle XFAIL updates, and documentation covering service-in-Box tunnel paths, setup, execution, evidence, troubleshooting, and coverage gaps. ChangesTunnel E2E coverage
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
a0ce037 to
0ac7267
Compare
G4614
changed the title
8033bd6 to
8083362
Compare
G4614
changed the title
6babb23 to
b34e7de
Compare
G4614
changed the title
a6cdaef to
298f374
Compare
📦 BoxLite review — couldn't completepowered by BoxLite |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/test/e2e/cases/test_sdk_tunnel.py`:
- Around line 342-357: The SSH test command in the subprocess created by the
tunnel test targets the default port instead of the guest sshd on port 2222.
Update the command arguments around the endpoint hostname to include the SSH
port option set to 2222, preserving the existing ProxyCommand and connection
checks.
In `@scripts/test/e2e/fixtures/https_connect_proxy.py`:
- Around line 30-34: Update the CONNECT request construction in the proxy
connection flow to send the target authority in the Host header, reusing the
existing authority variable instead of proxy_host. Keep the CONNECT request line
and other headers unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 231a9f65-db30-4f89-b631-804f9657501a
📒 Files selected for processing (3)
docs/testing/service-in-box-e2e.mdscripts/test/e2e/cases/test_sdk_tunnel.pyscripts/test/e2e/fixtures/https_connect_proxy.py
| process = await asyncio.create_subprocess_exec( | ||
| "ssh", | ||
| "-i", | ||
| str(key_path), | ||
| "-o", | ||
| f"ProxyCommand={proxy_command}", | ||
| "-o", | ||
| "BatchMode=yes", | ||
| "-o", | ||
| "StrictHostKeyChecking=no", | ||
| "-o", | ||
| "UserKnownHostsFile=/dev/null", | ||
| "-o", | ||
| "ConnectTimeout=15", | ||
| f"root@{endpoint.hostname}", | ||
| "printf BOXLITE_SSH_CONNECT_OK", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
git ls-files | rg '(^|/)test_sdk_tunnel\.py$|(^|/)service-in-box-e2e\.md$|sshd|tunnel|ProxyCommand' || true
echo "== test file relevant lines =="
if [ -f scripts/test/e2e/cases/test_sdk_tunnel.py ]; then
wc -l scripts/test/e2e/cases/test_sdk_tunnel.py
sed -n '300,390p' scripts/test/e2e/cases/test_sdk_tunnel.py | cat -n -v
fi
echo "== docs relevant lines =="
if [ -f docs/testing/service-in-box-e2e.md ]; then
wc -l docs/testing/service-in-box-e2e.md
sed -n '1,120p' docs/testing/service-in-box-e2e.md | cat -n -v
fi
echo "== search proxy_command and 2222 usages =="
rg -n 'ProxyCommand|PROXY_COMMAND|2222|sshd:2222|BOXLITE_SSH_CONNECT_OK|CREATE_SUBPROCESS_EXEC|create_subprocess_exec' .Repository: boxlite-ai/boxlite
Length of output: 18089
🌐 Web query:
OpenSSH ssh Config ProxyCommand %p remote port default port behavior
💡 Result:
In OpenSSH configuration, the %p token used within a ProxyCommand string is a placeholder that OpenSSH automatically substitutes with the remote port number [1][2][3]. When you specify %p in a ProxyCommand, OpenSSH dynamically inserts the port it intends to connect to [4]. This behavior ensures that the command (such as nc or another proxy tool) uses the correct destination port, regardless of whether that port was explicitly provided by the user in the command line (e.g., via -p), defined in a Port configuration directive, or if it is relying on the system default [4]. Key aspects of this behavior include: - Automatic Resolution: OpenSSH determines the target port based on your connection request. If no specific port is provided, it uses the default SSH port (22) [1][4]. This resolved value is then passed to the ProxyCommand as the replacement for %p [2][5]. - Token Substitution: The %p token is part of a set of tokens (including %h for the host and %r for the remote user) designed to make ProxyCommand flexible [2][6]. When the ssh client executes the ProxyCommand, it performs the expansion of these tokens before the command is run [2][3]. - Consistency: This mechanism allows you to define generic proxy configurations, such as ProxyCommand /usr/bin/nc %h %p, which will correctly adapt to different hosts and ports without needing manual updates for each destination [2][5]. Essentially, %p serves as a bridge that passes the destination port context directly from the OpenSSH client to the utility handling the proxy connection [2][4].
Citations:
- 1: https://unix.stackexchange.com/questions/183951/what-do-the-h-and-p-do-in-this-command
- 2: https://man.openbsd.org/ssh_config
- 3: https://github.com/openssh/openssh-portable/blob/9f04903c50089acde55ef3ea7edd35161c5eac0c/ssh_config.5
- 4: https://github.com/openssh/openssh-portable/blob/44142068dc7ef783d135e91ff954e754d2ed432e/sshconnect.c
- 5: http://lists.mindrot.org/pipermail/openssh-bugs/2020-June/021618.html
- 6: https://man.openbsd.org/OpenBSD-current/man5/ssh_config.5
🌐 Web query:
OpenSSH ssh command line -p option ProxyCommand %p default port remote host
💡 Result:
In the OpenSSH ProxyCommand directive, the token %p is a placeholder that is automatically substituted with the destination port of the target host [1][2]. When you use %p in a command string, the SSH client replaces it with either the port explicitly specified in your SSH configuration (or via the command line) for that host, or the default SSH port (22) if no specific port is defined [3][2]. Key points regarding %p and ProxyCommand: - Token Substitution: %p (port), along with %h (host) and %r (remote user), are standard tokens supported by the ProxyCommand directive to dynamically pass connection details to the proxy command being executed [4][1]. - Functionality: ProxyCommand instructs the SSH client to use an arbitrary command (such as ssh -W or nc) to establish the connection [1][5]. By using %h and %p, you ensure that the proxy command correctly directs the traffic to the intended target host and port [6][7]. - Example: If you have Host target and Port 830 configured, using ProxyCommand ssh -W %h:%p jumphost will effectively execute as ssh -W target:830 jumphost [3][6]. - Note on Defaulting: If no port is specified for the target host in your configuration, %p defaults to 22 [3]. This behavior is automatic and intrinsic to how the SSH client processes the ProxyCommand string before execution [1].
Citations:
- 1: https://man.openbsd.org/ssh_config.5
- 2: https://unix.stackexchange.com/questions/183951/what-do-the-h-and-p-do-in-this-command
- 3: Using SSHConfig & ProxyCommand with non-port 22 host port paramiko/paramiko#1321
- 4: https://man.archlinux.org/man/ssh_config.5.uk.txt
- 5: https://goteleport.com/blog/ssh-proxyjump-ssh-proxycommand/
- 6: https://serverfault.com/questions/544545/how-does-ssh-proxycommand-actually-work
- 7: https://www.cyberciti.biz/faq/linux-unix-ssh-proxycommand-passing-through-one-host-gateway-server/
Connect SSH to the tunneled guest port.
The test creates sshd on 2222, and the tunnel is requested for guest port 2222, but the ssh command does not pass -p 2222. OpenSSH then uses the default SSH port internally pass-through for this connection, so the %p placeholder in ProxyCommand points at 22 instead of the running sshd. Add -p 2222 so the SSH client can complete to the intended guest port.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/test/e2e/cases/test_sdk_tunnel.py` around lines 342 - 357, The SSH
test command in the subprocess created by the tunnel test targets the default
port instead of the guest sshd on port 2222. Update the command arguments around
the endpoint hostname to include the SSH port option set to 2222, preserving the
existing ProxyCommand and connection checks.
| authority = f"{target_host}:{target_port}" | ||
| conn.sendall( | ||
| f"CONNECT {authority} HTTP/1.1\r\n" | ||
| f"Host: {proxy_host}\r\n" | ||
| "Proxy-Connection: Keep-Alive\r\n\r\n".encode() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Send the CONNECT authority in Host.
Line 33 sends the proxy host, while the CONNECT request targets authority. Proxies that validate Host against the CONNECT authority can reject this handshake. Use Host: {authority}.
Proposed fix
f"CONNECT {authority} HTTP/1.1\r\n"
- f"Host: {proxy_host}\r\n"
+ f"Host: {authority}\r\n"
"Proxy-Connection: Keep-Alive\r\n\r\n".encode()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| authority = f"{target_host}:{target_port}" | |
| conn.sendall( | |
| f"CONNECT {authority} HTTP/1.1\r\n" | |
| f"Host: {proxy_host}\r\n" | |
| "Proxy-Connection: Keep-Alive\r\n\r\n".encode() | |
| authority = f"{target_host}:{target_port}" | |
| conn.sendall( | |
| f"CONNECT {authority} HTTP/1.1\r\n" | |
| f"Host: {authority}\r\n" | |
| "Proxy-Connection: Keep-Alive\r\n\r\n".encode() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/test/e2e/fixtures/https_connect_proxy.py` around lines 30 - 34,
Update the CONNECT request construction in the proxy connection flow to send the
target authority in the Host header, reusing the existing authority variable
instead of proxy_host. Keep the CONNECT request line and other headers
unchanged.
1 participant
Adds real SSH-over-CONNECT dev coverage and documents the complete service-in-box test matrix and runbook.
Test plan:
3 passed in 20.53s1 passed in 13.43s4 xfailed in 28.84s2 passed in 2.47syarn --cwd apps prettier --check ../docs/testing/service-in-box-e2e.mdSummary by CodeRabbit