bitreq: add SOCKS5 proxy support

[FreeOnlineUser]

Add SOCKS5 proxy support (RFC 1928) alongside the existing HTTP CONNECT path.

Motivation

ldk-node needs to route HTTP calls (RGS, scoring, LNURL-auth) through Tor's SOCKS proxy when TorConfig is enabled. The existing proxy feature only supports HTTP CONNECT, which doesn't handle .onion addresses. See lightningdevkit/ldk-node#834.

SOCKS5 and HTTP CONNECT serve different use cases and now coexist behind the same proxy feature flag:

• SOCKS5: Tor and general proxying with domain-based addressing (DNS resolved at the proxy).
• HTTP CONNECT: forward proxies for HTTPS tunneling. Kept for the payjoin use case raised in feat(bitreq): HTTPS Proxy Support for HTTP CONNECT #472.

Public API

• Proxy::new_socks5(addr): unauthenticated SOCKS5. Accepts both socks5:// and socks5h:// URL schemes (identical behaviour, since destinations are always forwarded as ATYP 0x03 hostnames).
• Proxy::new_socks5_with_credentials(addr, user, pass): RFC 1929 username/password auth at construction. Tor uses these credentials for circuit isolation.
• Proxy::set_credentials(user, pass): mutate credentials on an existing Proxy. Lets a caller hold one Proxy and rotate credentials between connections to obtain fresh Tor circuits, without rebuilding from a URL.

Implementation

• New ProxyKind::Socks5 variant.
• Sync and async handshake paths share protocol logic via 5 extracted helpers (greeting, auth, connect framing). Only the I/O calls differ between sync and async.
• Connection layer branches on proxy.is_socks5() before the existing HTTP CONNECT path.

Tests

18 SOCKS5-specific tests:

• 10 parsing / credential: host+port parsing, socks5:// and socks5h:// schemes, default port (1080), wrong-protocol rejection, is_socks5 check, credential constructor, RFC 1929 length validation (constructor and setter), credential rotation.
• 8 mock handshake: success, .onion domain, server rejection, port encoding (big-endian), domain >255 bytes, RFC 1929 credential auth, credential rejection, no-auth fallback.

Unit tests, doctests, and clippy are all clean.

Notes

• Production tested on a live Lightning node routing through Arti's SOCKS proxy.
• Related: feat(bitreq): HTTPS Proxy Support for HTTP CONNECT #472 (original payjoin SOCKS5 ask), TorConfig: HTTP calls (RGS, scoring, LNURL-auth) bypass SOCKS proxy lightningdevkit/ldk-node#834 (downstream consumer).
• Read-timeout behaviour during the binary handshake matches the existing HTTP CONNECT path: both rely on the connect-time deadline and don't set per-read deadlines on the raw stream. Improving this for both paths is left as a follow-up.

---

Assisted by Joe (Claude Opus 4.7 under the hood).

[tnull]

Took a first very high-level look.

[tnull]

I think we should be good to just drop the HTTP proxy behavior? Not sure anybody is going to use that, and it might make things simpler?

[FreeOnlineUser]

HTTP CONNECT is what payjoin originally requested in #472, so I'd keep it. They serve different use cases: SOCKS5 for Tor/general proxying, HTTP CONNECT for corporate forward proxies and HTTPS tunneling. Happy to refactor if the maintainers agree it should go, but wanted to flag the other stakeholders first.

[tnull]

Note that Tor (mis-)uses user credentials to trigger circuit rebuilding/stream isolation. So we probably want to allow for an interface that at least lets the user refresh/change the credentials freely on the Proxy object, so that subsequent connections are using a new circuit.

[FreeOnlineUser]

Good call. I'll add username/password support (RFC 1929 sub-negotiation) to the SOCKS5 handshake. When credentials are set, the greeting advertises method 0x02 instead of 0x00, and Tor uses the unique credentials to isolate circuits. The Proxy API can accept optional credentials at construction or via a setter for rotation.

[FreeOnlineUser]

Updated: added Proxy::new_socks5_with_credentials for RFC 1929 auth (Tor circuit isolation), credential length validation, extracted shared protocol helpers to deduplicate sync/async, 15 tests (was 10).

[tnull]

@FreeOnlineUser any update on this? Is this ready to take out of draft?

[daywalker90]

FYI: https://github.com/rust-bitcoin/bitcoin-payment-instructions needs socks5h support for remote DNS lookups.

[FreeOnlineUser]

@tnull yes, ready. Apologies for the radio silence. Pulling threads together:

HTTP CONNECT path (your March 20 question on dropping it): kept it. payjoin in #472 was the original requester for proxy support and they use HTTP CONNECT. Happy to drop in a follow-up PR if you'd rather, but it's separable from this work and doesn't block SOCKS5 going green.

Credential refresh interface (your March 20 ask for rotating credentials freely): added Proxy::set_credentials(user, pass). A caller can now hold one long-lived Proxy and rotate credentials per connection for Tor circuit isolation without rebuilding from a URL. Tested with a rotation case. Open to also adding a chained with_credentials(self, ...) builder if you'd prefer that shape too.

socks5h scheme (@daywalker90's hint about bitcoin-payment-instructions needing remote DNS): we already always do remote DNS at the proxy via ATYP 0x03 (DOMAIN). I've added socks5h:// as an accepted URL alias to match curl convention. Both schemes behave identically and the doc explicitly states this.

Hang risk on the binary handshake: this PR's read-timeout behaviour matches the existing HTTP CONNECT path, which also doesn't set per-read deadlines on the raw stream during proxy negotiation. Improving this for both paths is left as a follow-up so we don't expand scope here.

The description has been refreshed to reflect everything currently shipped (RFC 1929 credentials, socks5h://, set_credentials, 18 SOCKS5-specific tests). Out of draft.

[TheBlueMatt]

Mind refactoring this code so that it just always calls handshake... and doesn't care what type it is and that's abstracted by the proxy?

[TheBlueMatt]

please dont allocate to throw away <= 258 bytes.

[TheBlueMatt]

Can we DRY the handshake method? Maybe with a simple macro generating both the async and sync versions?

[FreeOnlineUser]

@TheBlueMatt thanks for the review. All three addressed in 8cfda31.

Allocation (proxy.rs:329): Switched the domain-ATYP arm to [u8; 257] with a slice. Both sync and async paths fixed; matches the IPv4 / IPv6 arms in the same match.

Polymorphism (connection.rs): Added Proxy::handshake_sync / Proxy::handshake_async pub(crate) dispatchers, with private http_connect_handshake_sync / _async methods owning the moved CONNECT logic. The connection layer now treats every proxy uniformly:

Some(proxy) => {
    let mut tcp = Self::tcp_connect(&proxy.server, proxy.port, timeout_at)?;
    proxy.handshake_sync(&mut tcp, params.host, params.port)?;
    Ok(tcp)
}

The is_socks5() predicate has no remaining lib callers and was dropped. Added two mock-server tests for the HTTP CONNECT path through the new dispatcher.

Sync/async DRY (proxy.rs:281): Done with two macro_rules! macros (socks5_handshake_body! and http_connect_handshake_body!) at module scope. Each takes the call-site idents (self, stream, target_host, target_port) and a token-tree $($maybe_await:tt)* that's empty for sync and .await for async, inserted at every I/O call. Macro hygiene means the parameter list is explicit, but the body is single-source.

The four handshake methods now look like:

pub(crate) fn socks5_handshake_sync(
    &self, stream: &mut std::net::TcpStream,
    target_host: &str, target_port: u16,
) -> Result<(), Error> {
    use std::io::{Read, Write};
    socks5_handshake_body!(self, stream, target_host, target_port;)
}

Net effect: -59 lines in proxy.rs versus the previous round, and any future protocol change happens in one place.

Also picked up a dead tokio::io::AsyncReadExt import that fell out of the connection.rs rewrite.

[TheBlueMatt]

Sorry for the delay, a few nits and one bug.

[TheBlueMatt]

nit: I believe this would be more correct with a ? not a * (which would ripple through the whole macro).

Suggested change

	($self:ident, $stream:ident, $target_host:ident, $target_port:ident; $($maybe_await:tt)*) => {{
	($self:ident, $stream:ident, $target_host:ident, $target_port:ident; $($maybe_await:tt)?) => {{

[FreeOnlineUser]

There's already a precedent for this in bitreq: response.rs defines a maybe_await! helper used by define_read_methods! to thread .await through a sync/async-templated body, with the outer macro taking the keyword via $(, $await: tt)?. Refactored both socks5_handshake_body! and http_connect_handshake_body! to use the same shape:

macro_rules! socks5_handshake_body {
    ($self:ident, $stream:ident, $target_host:ident, $target_port:ident $(, $await:tt)?) => {{
        ...
        maybe_await!($stream.write_all(&greeting), $($await)?).map_err(Error::IoError)?;
        ...
    }};
}

// sync:  socks5_handshake_body!(self, stream, target_host, target_port)
// async: socks5_handshake_body!(self, stream, target_host, target_port, await)

Aligns with the existing define_read_methods! convention. Pushed in 224343b.

[TheBlueMatt]

Doesn't really seem worth stubbing out to a function to check two bytes and return an error?

[FreeOnlineUser]

Inlined both 2-byte helpers (socks5_check_greeting, socks5_check_auth) into the handshake macro body and removed the standalone fns. Pushed in 224343b.

[TheBlueMatt]

No, it might just indicate we received one TCP packet and are waiting on more.

[FreeOnlineUser]

Worth raising the bigger question: should HTTP CONNECT come out entirely?

A few reasons:

• Making this read loop correct isn't trivial (Content-Length, chunked encoding, or read-until-close, each with their own edge cases).
• tnull floated removal back in March. I argued to keep it for payjoin-style HTTP-only callers at the time, but I don't have a concrete use case (ldk-node Tor is SOCKS5-only).
• Removing it deletes the second macro_rules! block, so the abstraction question on the SOCKS5 macro stands on its own.

I'm happy to either fix the response-read here or strip HTTP CONNECT in this PR. Slight lean toward removal. Let me know which way you'd prefer.

[TheBlueMatt]

That's a fair question, I wouldn't be too against removing it, but we do also actually have HTTP response-parsing in this crate....maybe we should find a way to use the Response type we already have to read the headers so that the code here can be ~trivial?

[FreeOnlineUser]

Took the latter path. Made parse_status_line pub(crate) and rewrote the handshake to read into an [u8; 8192] stack buffer until \r\n\r\n, then parse the status line through the shared helper:

let mut buf = [0u8; 8192];
let mut len = 0;
let header_end = loop {
    let n = maybe_await!($stream.read(&mut buf[len..]), $($await)?).map_err(Error::IoError)?;
    if n == 0 { return Err(Error::ProxyConnect); }
    len += n;
    if let Some(idx) = buf[..len].windows(4).position(|w| w == b"\r\n\r\n") {
        break idx;
    }
    if len == buf.len() { return Err(Error::ProxyConnect); }
};

let headers = core::str::from_utf8(&buf[..header_end]).map_err(|_| Error::ProxyConnect)?;
let status_line = headers.lines().next().ok_or(Error::ProxyConnect)?;
let (status_code, _) = crate::response::parse_status_line(status_line);

Drops verify_response (the partial-read-as-EOF bug goes with it) and the heap Vec. Added a handshake_handles_split_response test that writes the response in two TCP segments to exercise the loop. Pushed in 69b50bc.

[TheBlueMatt]

Please do not allocate a vec for < 8192 bytes. Same goes for socks5_auth_request.

[FreeOnlineUser]

Switched both to fixed-size stack buffers:

• socks5_auth_request → ([u8; 513], usize) (max 3 + 255 + 255)
• socks5_connect_request → ([u8; 262], usize) (max 4 + 1 + 255 + 2)

Sliced with the length at the call site. Pushed in 69b50bc.

[TheBlueMatt]

That's a fair question, I wouldn't be too against removing it, but we do also actually have HTTP response-parsing in this crate....maybe we should find a way to use the Response type we already have to read the headers so that the code here can be ~trivial?