feat: incoming proxy protocol v2 support on listener port#15
Conversation
|
Lost in the diff? Review this PR in Change Stack to follow the change map from intent to exact ranges. Warning Review limit reached
More reviews will be available in 44 minutes and 6 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (13)
📝 WalkthroughWalkthroughMinecraft packet fields are refactored to use strongly-typed ChangesMinecraft Packet Field Type Wrapping
HAProxy PROXY Protocol v2 Support
JSON Field Updates & Formatting
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/bin/session_traffic_test.rs (1)
105-119:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEhh~? Using a hardcoded
758when you've got the wrapper right there?Line 119 passes the raw literal
758toencode_body_with_version, but the other test binaries (epoll_stat_probe.rsline 277,instance_tcp_test.rsline 437) properly dereference the wrapper with*hs.protocol_version. This inconsistency is kinda sloppy, don'tcha think? ♪Keeping the pattern consistent makes maintenance way easier~
✨ Suggested fix to stay consistent with the other test files
let mut hs_raw = Vec::new(); let _ = encode_packet(&mut hs_raw, &hs); let mut login_body = Vec::new(); - let _ = login.encode_body_with_version(&mut login_body, 758); + let _ = login.encode_body_with_version(&mut login_body, *hs.protocol_version); let mut login_raw = Vec::new();🤖 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 `@src/bin/session_traffic_test.rs` around lines 105 - 119, The test uses a hardcoded protocol version (758) when calling login.encode_body_with_version; update the call to use the HandshakeC2s wrapper by dereferencing hs.protocol_version (i.e., pass *hs.protocol_version) so it matches the pattern used elsewhere (see HandshakeC2s, hs.protocol_version and the call to encode_body_with_version on LoginStartC2s).src/proxy/mod.rs (2)
349-372:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe rate limiter is still bucketing the load balancer, not the player.
Heh, with
proxy_protocolenabled you already decode the real client IP, but the limiter still runs onaddr.ip()before that happens. That makes every player behind one HAProxy instance share a single quota, which is a nasty self-DoS footgun and pretty useless for per-client abuse control. Move the check after a successful PPv2 parse and key it onclient_addr.ip().Also applies to: 508-531
🤖 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 `@src/proxy/mod.rs` around lines 349 - 372, The rate limiter is currently applied to the upstream/load‑balancer address `addr.ip()` before you decode the real client IP with `read_proxy_protocol_addr`, causing shared buckets; move the `rate_limiter.check(...)` logic so it runs after you compute `client_addr` (use `client_addr.ip()` as the key) and keep the same handling (call `LureLogger::rate_limited(&client_addr.ip())`, `drop(client)`, `continue`) on disallowed results; make the identical change for the other occurrence that mirrors the same pattern (the block that currently mirrors 508-531) so both proxy‑protocol code paths use the decoded client IP for rate limiting.
1162-1175:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't collapse the queried hostname into a resolved
SocketAddr.You resolve
handshake.server_addresshere and only send that socket address throughrequest_query_from_agent. On the tunnel side the status handshake is rebuilt fromquery.server_address.to_string(), so the backend never sees the hostname the player actually asked for. Any vhost / forced-host / status plugin keyed off the handshake host can answer with the wrong status. This request needs to carry the original host string and port separately from the resolved target.🤖 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 `@src/proxy/mod.rs` around lines 1162 - 1175, The code collapses handshake.server_address and handshake.server_port into a resolved SocketAddr (via resolve_socket_addr) and passes only server_address into request_query_from_agent, which loses the original hostname; change request handling so the original host string and port are preserved and sent alongside (or instead of) the resolved SocketAddr: update the call/site around resolve_socket_addr, request_query_from_agent, and any Query/Status handshake construction to include handshake.server_address (string) and handshake.server_port (u16) separately (or add fields like original_host/original_port to the query struct) so the tunnel/backend can rebuild the handshake from the original hostname (see query.server_address.to_string() usage) rather than the resolved socket address.
🤖 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/proxy/mod.rs`:
- Around line 570-573: The parsed PPv2 client_addr is dropped after
LureLogger::new_connection and handle_handshake; subsequent code (handle_status,
handle_proxy and any logic using client.as_inner().addr()) therefore uses the
HAProxy peer IP — fix by threading the parsed client_addr through the call chain
or storing it on the connection wrapper: update handle_handshake, handle_status,
handle_proxy signatures (and any callers) to accept a client_addr parameter (or
add a field like parsed_addr on the connection type and set it in
handle_handshake), then replace uses of client.as_inner().addr() where real
client identity is required with the parsed client_addr/connection.parsed_addr
and adjust logging (e.g., LureLogger::new_connection) to use the parsed address.
- Around line 57-87: The code currently discards any bytes read beyond the PROXY
v2 header; preserve leftovers by collecting buf[..total_len] for parsing and
capturing buf[total_len..] as leftover bytes instead of dropping them. After
parsing with net::ha::parse, return or forward the leftover slice (e.g., a
Vec<u8>) together with the real address from read_ingress_hello (or push
leftovers back into client/read_chunk buffer) so downstream code can resume
decoding from the correct position; update the read_ingress_hello signature and
callers to accept the leftover bytes and ensure client.read_chunk remains the
single reader.
---
Outside diff comments:
In `@src/bin/session_traffic_test.rs`:
- Around line 105-119: The test uses a hardcoded protocol version (758) when
calling login.encode_body_with_version; update the call to use the HandshakeC2s
wrapper by dereferencing hs.protocol_version (i.e., pass *hs.protocol_version)
so it matches the pattern used elsewhere (see HandshakeC2s, hs.protocol_version
and the call to encode_body_with_version on LoginStartC2s).
In `@src/proxy/mod.rs`:
- Around line 349-372: The rate limiter is currently applied to the
upstream/load‑balancer address `addr.ip()` before you decode the real client IP
with `read_proxy_protocol_addr`, causing shared buckets; move the
`rate_limiter.check(...)` logic so it runs after you compute `client_addr` (use
`client_addr.ip()` as the key) and keep the same handling (call
`LureLogger::rate_limited(&client_addr.ip())`, `drop(client)`, `continue`) on
disallowed results; make the identical change for the other occurrence that
mirrors the same pattern (the block that currently mirrors 508-531) so both
proxy‑protocol code paths use the decoded client IP for rate limiting.
- Around line 1162-1175: The code collapses handshake.server_address and
handshake.server_port into a resolved SocketAddr (via resolve_socket_addr) and
passes only server_address into request_query_from_agent, which loses the
original hostname; change request handling so the original host string and port
are preserved and sent alongside (or instead of) the resolved SocketAddr: update
the call/site around resolve_socket_addr, request_query_from_agent, and any
Query/Status handshake construction to include handshake.server_address (string)
and handshake.server_port (u16) separately (or add fields like
original_host/original_port to the query struct) so the tunnel/backend can
rebuild the handshake from the original hostname (see
query.server_address.to_string() usage) rather than the resolved socket address.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a9b665a6-2822-42ef-8993-c5465b074531
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
src/bin/epoll_stat_probe.rssrc/bin/instance_tcp_test.rssrc/bin/session_traffic_test.rssrc/connection.rssrc/packet/mod.rssrc/proxy/backend/mod.rssrc/proxy/helpers.rssrc/proxy/mod.rssrc/proxy/query.rssrc/tunnel/registry.rssrc/utils/logging.rstun/src/client/runtime.rstun/src/lib.rs
Incoming HAProxy PROXY protocol v2 parsing on the listener port. When proxy_protocol = true in settings.toml, the PPv2 header is read from the accepted connection and the real client address is used throughout the proxy pipeline. Also fixes API compatibility with net crate commit 2b02d51 which changed Minecraft packet fields to wrapper types (VarInt, BoundedStr, BEu16, PacketDecode/Encode traits).
- Thread parsed PPv2 client_addr through to handle_status/handle_proxy instead of dropping it (they now use the real client IP, not proxy IP) - Preserve leftover bytes after PPv2 header and pass them to read_ingress_hello so pipelined MC handshake data isn't lost - Move rate limiter check after PPv2 parsing so it keys on the real client IP, not the load-balancer IP - session_traffic_test: deref hs.protocol_version instead of hardcoding 758 Fix 5 (original hostname in tunnel query path) skipped: server_address resolves to the backend IP which is functionally correct for the handshake; fixing it would require adding a wire field to the tunnel protocol, disproportionate for this change.
hUwUtao
force-pushed
the
feat/proxy-protocol
branch
from
56477d5 to
8b8e981
Compare
1 participant
Summary by CodeRabbit
New Features
Improvements