feat: support TCP KeyExchange so logged-in clients >=1.4.1 can connect - #691
Open
cuongnq93 wants to merge 2 commits into
Open
feat: support TCP KeyExchange so logged-in clients >=1.4.1 can connect#691cuongnq93 wants to merge 2 commits into
cuongnq93 wants to merge 2 commits into
Conversation
Clients from 1.4.1 onwards call secure_tcp() against the rendezvous server whenever an account is logged in on an API server and a key is configured. The OSS server never answered the KeyExchange message, so those clients hung until 'Failed to secure tcp: deadline has elapsed'. Implements both handshake phases on port 21116/TCP: - phase 1: send this connection's box public key, signed with the server key - phase 2: open the client-sealed symmetric key and upgrade the channel Messages are then secretbox-encrypted with a sequence-number nonce, matching hbb_common::tcp::Encrypt on the client side. WebSocket connections are left untouched since wss already encrypts the transport. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds per-connection symmetric encryption for TCP rendezvous sessions through asymmetric key exchange, encrypted message handling, validation tests, and a multi-stage Docker image. WebSocket connections remain without this handshake. ChangesTCP connection encryption
Container packaging
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TCPClient
participant RendezvousServer
participant Sink
TCPClient->>RendezvousServer: Initiate KeyExchange
RendezvousServer->>TCPClient: Signed server public key
TCPClient->>RendezvousServer: Sealed symmetric key
RendezvousServer->>Sink: Install Encrypt
TCPClient->>RendezvousServer: Encrypted payload
RendezvousServer->>Sink: Decrypted message
Sink->>TCPClient: Encrypted response
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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…keypair Two issues from review: - handle_tcp serves both plain TCP and WebSocket, so a WebSocket client could install a secretbox layer on its own connection and make every later reply unreadable to itself. The client skips the handshake under wss anyway, so the message is now ignored there. - The box keypair was generated once per process, so anyone who later recovered that secret could unseal the symmetric key of every recorded session. It now lives on the Sink: generated per connection, taken (not borrowed) on use, and dropped when the connection ends. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/rendezvous_server.rs`:
- Around line 1355-1387: Update key_exchange_phase2 so it only reports success
after installing the negotiated Encrypt state into an available sink. Treat sink
being None as a failure: log the failed key installation, return false, and do
not log that the connection was secured; preserve the existing key validation
and installation behavior when sink is present.
🪄 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: 6d58388d-4a93-4ac8-8368-01c730b782f8
📒 Files selected for processing (1)
src/rendezvous_server.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Dockerfile.custom (1)
2-2: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin the base images for reproducible releases.
rust:1-bookwormanddebian:bookworm-slimcan change between builds. Use explicitly reviewed versions with immutable digests, while retaining a scheduled process for security updates.Also applies to: 11-11
🤖 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 `@Dockerfile.custom` at line 2, Pin the Dockerfile’s builder and runtime base images to explicitly reviewed version tags with immutable digests, replacing the floating rust:1-bookworm and debian:bookworm-slim references. Preserve the existing image roles and ensure the chosen versions remain compatible with the build while relying on the established scheduled process for future security updates.
🤖 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 `@Dockerfile.custom`:
- Around line 11-20: Update the final Dockerfile stage to create a dedicated
non-root system user with its home directory, replace the /root working
directory with that home directory, and add USER before CMD so hbbs runs under
the dedicated user.
---
Nitpick comments:
In `@Dockerfile.custom`:
- Line 2: Pin the Dockerfile’s builder and runtime base images to explicitly
reviewed version tags with immutable digests, replacing the floating
rust:1-bookworm and debian:bookworm-slim references. Preserve the existing image
roles and ensure the chosen versions remain compatible with the build while
relying on the established scheduled process for future security updates.
🪄 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: 2d9b4b5a-9ede-4970-b1a2-de6b81174be3
📒 Files selected for processing (2)
Dockerfile.customsrc/rendezvous_server.rs
Comment on lines
+11
to
+20
| FROM debian:bookworm-slim | ||
| RUN apt-get update && apt-get install -y --no-install-recommends ca-certificates \ | ||
| && rm -rf /var/lib/apt/lists/* | ||
| WORKDIR /root | ||
| COPY --from=builder /build/target/release/hbbs /usr/bin/hbbs | ||
| COPY --from=builder /build/target/release/hbbr /usr/bin/hbbr | ||
| # hbbs: 21115 (nat test), 21116 tcp+udp (rendezvous), 21118 (ws) | ||
| # hbbr: 21117 (relay), 21119 (ws relay) | ||
| EXPOSE 21115 21116 21116/udp 21117 21118 21119 | ||
| CMD ["hbbs"] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Run the server as a non-root user.
The final image defaults to root, and /root is used as the working directory. Create a dedicated system user/home directory, switch the working directory there, and add USER before CMD.
Proposed fix
FROM debian:bookworm-slim
RUN apt-get update && apt-get install -y --no-install-recommends ca-certificates \
- && rm -rf /var/lib/apt/lists/*
-WORKDIR /root
+ && rm -rf /var/lib/apt/lists/* \
+ && groupadd --system hbbs \
+ && useradd --system --gid hbbs --create-home --home-dir /var/lib/hbbs hbbs
+WORKDIR /var/lib/hbbs
COPY --from=builder /build/target/release/hbbs /usr/bin/hbbs
COPY --from=builder /build/target/release/hbbr /usr/bin/hbbr
+USER hbbs
CMD ["hbbs"]📝 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.
Suggested change
| FROM debian:bookworm-slim | |
| RUN apt-get update && apt-get install -y --no-install-recommends ca-certificates \ | |
| && rm -rf /var/lib/apt/lists/* | |
| WORKDIR /root | |
| COPY --from=builder /build/target/release/hbbs /usr/bin/hbbs | |
| COPY --from=builder /build/target/release/hbbr /usr/bin/hbbr | |
| # hbbs: 21115 (nat test), 21116 tcp+udp (rendezvous), 21118 (ws) | |
| # hbbr: 21117 (relay), 21119 (ws relay) | |
| EXPOSE 21115 21116 21116/udp 21117 21118 21119 | |
| CMD ["hbbs"] | |
| FROM debian:bookworm-slim | |
| RUN apt-get update && apt-get install -y --no-install-recommends ca-certificates \ | |
| && rm -rf /var/lib/apt/lists/* \ | |
| && groupadd --system hbbs \ | |
| && useradd --system --gid hbbs --create-home --home-dir /var/lib/hbbs hbbs | |
| WORKDIR /var/lib/hbbs | |
| COPY --from=builder /build/target/release/hbbs /usr/bin/hbbs | |
| COPY --from=builder /build/target/release/hbbr /usr/bin/hbbr | |
| # hbbs: 21115 (nat test), 21116 tcp+udp (rendezvous), 21118 (ws) | |
| # hbbr: 21117 (relay), 21119 (ws relay) | |
| EXPOSE 21115 21116 21116/udp 21117 21118 21119 | |
| USER hbbs | |
| CMD ["hbbs"] |
🤖 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 `@Dockerfile.custom` around lines 11 - 20, Update the final Dockerfile stage to
create a dedicated non-root system user with its home directory, replace the
/root working directory with that home directory, and add USER before CMD so
hbbs runs under the dedicated user.
Source: Linters/SAST tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Clients from 1.4.1 onwards call secure_tcp() against the rendezvous server whenever an account is logged in on an API server and a key is configured. The OSS server never answered the KeyExchange message, so those clients hung until 'Failed to secure tcp: deadline has elapsed'.
Implements both handshake phases on port 21116/TCP:
Messages are then secretbox-encrypted with a sequence-number nonce, matching hbb_common::tcp::Encrypt on the client side. WebSocket connections are left untouched since wss already encrypts the transport.
Summary by CodeRabbit
Greptile Summary
This PR implements the two-phase TCP key exchange handshake on port 21116 so that RustDesk clients ≥ 1.4.1 can connect without hanging when a server key and API account are both configured. The
Sinktype is refactored from a plain enum into a struct that carries both the transport variant and anArc<Mutex<Option<Encrypt>>>channel key, with secretbox encryption matchinghbb_common::tcp::Encrypt.Sink.exchange_skand discarded after phase 2.Encryptobject, and from that point all inbound frames are decrypted and all outboundsend_to_sinkcalls encrypt before writing.KeyExchangeon a WS connection is logged and silently ignored so the secretbox layer is never applied to WS frames.Confidence Score: 5/5
Safe to merge. The handshake logic is correct, ephemeral keypairs provide forward secrecy, and the WebSocket guard prevents mis-applying secretbox to WS frames.
The implementation is consistent with the hbb_common::tcp::Encrypt protocol: sequence numbers start at 0 and are incremented before use on both sides, nonces are correctly derived, and the bidirectional Arc<Mutex<Option>> is accessed safely through tokio's async mutex. Both previously raised concerns (static box keypair and unguarded WebSocket path) are addressed in the submitted code. Three unit tests cover the happy path, garbage rejection, and cross-connection key isolation.
Files Needing Attention: No files require special attention. The core changes in src/rendezvous_server.rs are well-structured and tested.
Important Files Changed
Sequence Diagram
sequenceDiagram participant C as Client (>=1.4.1) participant S as hbbs TCP :21116 C->>S: TCP connect Note over S: key_exchange_phase1()<br/>gen_keypair() per-connection S->>C: "KeyExchange { keys: [sign(box_pk, server_sk)] }" Note over C: verify signature,<br/>extract server box_pk,<br/>gen_key() symmetric,<br/>seal(sym_key, nonce=0, server_box_pk, client_sk) C->>S: "KeyExchange { keys: [client_box_pk, sealed_sym_key] }" Note over S: key_exchange_phase2()<br/>box::open -> sym_key<br/>Encrypt installed in Sink.key<br/>exchange_sk dropped Note over C,S: Channel now secretbox-encrypted<br/>(seqnum nonce, starting at 1) C->>S: "secretbox(PunchHoleRequest, nonce=1)" Note over S: recv loop: enc.dec() -> plaintext<br/>handle_tcp -> sink moved to tcp_punch S->>C: "secretbox(PunchHoleResponse, nonce=1)"Reviews (2): Last reviewed commit: "fix: ignore KeyExchange over WebSocket, ..." | Re-trigger Greptile