Add Socks5 proxy transport for RPC blockchain #493

rajarshimaitra · 2021-12-03T13:05:47Z

Description

This attempts to solve #373.

Because bitcoincore_rpc::Client doesn't support socks5 proxy internally, the easiest option for us is to implement our own
socks5 transport and make it compatible with bitcoincore_rpc::Client.

The Transport trait is implemented for ProxyTransport to make it compatible with core rpc Client. ProxyTransport is a simple Socks5 wrapper over TcpStream and handles HTTP data like regular connection, but passes it through a given proxy.

I have implemented the basic structure but getting a generic error while trying to connect through local tor proxy running at 127.0.0.1:9050.

Error

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: RpcClient(JsonRpc(Transport(Io(Custom { kind: Other, error: "general SOCKS server failure" }))))', src/main.rs:77:62
stack backtrace:

The sample test code is here https://github.com/rajarshimaitra/bdk-example/blob/main/src/main.rs

I am investigating on the error.

Notes to the reviewers

Making the proxy transport probably warrant a dedicated module for rpc. Once the current approach is finalized, that refactoring can be done on a follow up PR. Suggestions welcome.

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing

New Features:

I've added tests for the new feature
I've added docs for the new feature
I've updated CHANGELOG.md

The `ProxyTransport` implements a socks5 stream connection with the core rpc host, and request/receive http data from it. `Transport` trait is implemented on `ProxyTransport` to make it compatible with existing `bitcoincore-rpc::Client`.

Handles proxy options to create a socks5 stream through proxy to rpc host. Creates regular http connection otherwise.

afilini

It seems a bit redundant to reimplement an http client in here: can you first try opening a pr to the jsonrpc library to see if they somehow let you build a Client from an existing TcpStream?

afilini · 2021-12-07T13:18:01Z

src/blockchain/rpc_proxy.rs

+        // Fetch the RPC address:port and wallet path from url
+        let (target_addr, wallet_path) = {
+            // the url will be of form "http://<rpc-host>:<rpc-port>/wallet/<wallet-file-name>"
+            (rpc_url[7..22].to_owned(), rpc_url[22..].to_owned())


Ideally here I would parse the url entirely, it's not reliable to just index into the string

Yes this does feel clunky. But this seemed easier for now without brain storming too much on a parser. Can you suggest a better way? Would update that in next revise.

A quick way without any extra dependencies could be:

check the prefix. It should be either "http://" or nothing

replace "http://" with ""

splitn with n = 2, separator = "/"

first part is the host, second part the wallet path

afilini · 2021-12-07T13:18:37Z

src/blockchain/rpc_proxy.rs

+        // fetch username password from rpc authentication
+        let rpc_auth = {
+            if let (Some(user), Some(pass)) = Self::get_user_pass(rpc_auth)? {
+                let mut auth = user;


Why not format!("{}:{}", user, pass)?

This is because the RPC server expects the auth string to be formatted in a certain way which is base64(b"Basic {user}:{pass}"). I just broke down the steps. This can be written in shorter lines.

Yeah it just seems a bit redundant here since the steps are so simple

src/blockchain/rpc_proxy.rs

afilini · 2021-12-07T13:22:04Z

src/blockchain/rpc_proxy.rs

+        while deadline > Instant::now() {
+            match reader.read_line(&mut line) {
+                // EOF reached for now, try again later
+                Ok(0) => std::thread::sleep(Duration::from_millis(5)),


When EOF is reached there's nothing more to wait for, you should return an error here

The function is blocking, so it will internally wait for data and only return a string when there's something: https://doc.rust-lang.org/std/io/trait.BufRead.html#method.read_line

I wrote it this way because I wanted to introduce a timeout into it. Socks doesn't work with timeouts. Is there a way we can do timeouts without this?

I don't think so, read_line() is blocking and won't return until either the fd is closed or there's a new line available.

The timeout is usually set on the TcpStream, if that's not exposed by the socks library we can't do anything.

rajarshimaitra · 2021-12-08T12:49:19Z

Thanks @afilini for the look.

It seems a bit redundant to reimplement an http client in here: can you first try opening a pr to the jsonrpc library to see if they somehow let you build a Client from an existing TcpStream?

I was looking for ways of how to do this easier in one of our upstream bitcoincorerpc or jsonrpc. But unfortunately the way jsonrpc::Client is written, it doesn't allow putting a TcpStream inside.

https://github.com/apoelstra/rust-jsonrpc/blob/ee78ede9a1eb543e0d64c4658019a6e4f7868d09/src/client.rs#L46-L53

The Client is wrapper over a Transport trait object. I think they did it make the lib flexible. So if we want to put a TcpStream in it, we have to make it Transport, which is essentially what I did in this PR with a Socks5Stream.

So weather we wanna do it in jsonrpc or anywhere, the approach kinda needs to be same, unless we propose to heavily refactor jsonrpc::Client which seems infeasible to me.

So instead of putting the whole thing in the upstream, I think its better to maintain it in our own repo for our need. The upstreams (both bitcoincorerpc and jsonrpc) provides specific APIs (from_jsonrpc and with_transport respectively) to enable this customization. So I think that's also their way of saying, "use your own Transport if you don't wanna use the default"?

afilini · 2021-12-09T10:15:40Z

Sorry I got confused, I meant the http client: https://github.com/apoelstra/rust-jsonrpc/blob/ee78ede9a1eb543e0d64c4658019a6e4f7868d09/src/simple_http.rs

Looking at it they don't store the TcpStream, so maybe that's not feasible.. but it sucks to have to just repeat the same code twice (also, not sure if there are licensing issues if we just copy their stuff in here).

rajarshimaitra · 2021-12-10T12:55:46Z

Looking at it they don't store the TcpStream, so maybe that's not feasible.. but it sucks to have to just repeat the same code twice (also, not sure if there are licensing issues if we just copy their stuff in here).

The only thing similar with their simplehttp is the http read write part. And that's generic http stuffs. But ya it sucks to have the same http logic twice.

There is another way I can think of is to propose changing their simplehttp client to include Socks5Stream inside depending on weather proxy options are provided. Then jsonrpc itself will be able provide proxy support. Do you think that would be a better approach?

Also any clue on the generic socks failure error? I can't seem to find any concrete reference in open net, and debugging inside isn't giving any more clue than this vague error message.

I have checked that my local tor proxy at port 9050 is working fine with curl.

notmandatory · 2021-12-16T18:11:46Z

Now that #501 is in you can rebase on master to fix the CI issue.

rajarshimaitra · 2022-04-04T10:56:11Z

Now that #501 is in you can rebase on master to fix the CI issue.

Yeah but I am still not sure if this is the best way to go for this..

notmandatory · 2022-04-05T00:38:30Z

No problem, we can leave as draft until you decide how you want to proceed.

notmandatory · 2022-06-07T19:04:28Z

My 2 cents on this is I don't think there is a compelling use-case for it. For mobile devices I think the preferred method will be to connect to public or home hosted bitcoind+electrs servers, or use compact block filters. But I'm not completely opposed to it if you have other uses in mind and/or find others who would use it.

rajarshimaitra · 2022-06-07T19:29:35Z

One use case I can have in mind, is a personal mobile wallet connected to a home Umbrel node.. Umbrel can't be exposed outside home network without Tor and electrum server overhead just for personal usage doesn't make sense.. Bitcoin RPC is enough to operate a wallet that only handles someone's personal data.. Electrums are useful when they wanna expose blockchain service to public.. And then also users need trusted reputed 3rd party Electrum servers and that server have user transaction data available, so that creates potential privacy leak.. (Tor doesn't help there, because the endpoint itself is the point of failure)

Many of the home use cases with vanilla core (Umbrel or something else) with mobile wallet integration will require Tor support for our RPC blcockhain.. They ideally will need Tor in the mobile too.. But not having Tor support for RPC restricts them to

Either connect only in local home network.
Or delpoy electrum server too and then route to it via Tor. (In this case the wallet devs falls back on running their own electrum service, because its too much too ask to wallet users).

I personally don't find the electrum way friendly especially for home use cases.. Its an extra overhead (another 400 gb of storage) for home node runners with no added benefits..

I forgot which one but Specter/Sparrow wallet allows to connect to a home RPC over Tor.. That makes it very useful to have it operated from anywhere in the world.. And it doesn't need electrum too..

notmandatory · 2022-06-07T20:28:24Z

According to the electrs features doc the index overhead should only be about 10%, you may be thinking of esplora which does use a lot more disk space and is harder to run on a home single board computer. And if someone does want to run only a simple bitcoind node and access it remotely this can also be done with BDK via compact block filters since we do support a single peer. I don't recall if we support the TOR proxy for those connections, if not I think that'd be more useful since it would also allow you to connect to a (single) public node with a little better privacy than the electrum protocol.

All that said, if you really think this is something folks will use then I'm not against it on any technical grounds, my primary concern is that it won't be used and doesn't look like a simple thing to add.

EDIT: The compact_filters Peer looks like it does already support connecting via a TOR proxy.

rajarshimaitra · 2022-06-08T08:03:38Z

Yes, the Compact Filter way seems like another possibility, even though isn't meant for this purpose. Users also have to configure their node to allow compact filters since by default its not active for core. But then, it would open compact filter service of the node to public too, and that will cost them good bandwidth (recently pointed out by Murch in a core review, as there are not much CBF nodes out there, the burden of existing node is more, hopefully that will improve in future). But most home node setup packages comes with CBF disabled.

Yes I agree, its not a easy thing to get into and also not dependent on BDK in any way. Tor support for RPC has to be done in lower level crates, and I am not sure if it will ever get done there too. Its not difficult to implement, but its difficult to organize PR and reviews in all those crates.

But pushing the ball on that front in apoelstra/rust-jsonrpc#57

Lets see if it can get somewhere.

I really think it will be very useful. Otherwise the RpcBlockhain has very few limited application (only when the wallet and node are in same local network). With some upcoming changes in core, our RPC sync logic can be made more powerful and faster. But it would be sad if we can't use it in its full form in all home applications, for just this one missing piece.

notmandatory · 2022-11-16T14:53:57Z

@rajarshimaitra here's an example of a socks proxy server, and is even in rust, you may be able to use to help with integration testing this feature without involving TOR. You'd have to spin up a socks5-server instance that proxies back to your localhost bitcoind. At least you should be able to verify the socks5 proxy protocol and auth is working (as long as socks5-server is correctly implementing the protocol from the server side).

https://crates.io/crates/socks5-server
~~https://github.com/shadowsocks/shadowsocks-rust~~ nevermind this one.. has non-standard client/server encryption

rajarshimaitra · 2022-11-17T14:12:44Z

Thanks @notmandatory , I think this PR can be closed if you wanna clean up the PR list.. We are not going with this approach.. And I will try to test the socks5 proxy manually with your suggestion..

Update:

It worked 🎉 .. rust-bitcoin/rust-bitcoincore-rpc#249 (comment)

Thanks for the suggestion.. Opened the upstream PR for review.. :)

notmandatory · 2022-11-19T20:11:56Z

Glad to hear the socks5 proxy suggestion helped. Closing this PR in favor of your rust-bitcoincore-rpc PR.

rajarshimaitra added 2 commits December 3, 2021 18:12

Add Socks5 proxy transport for RPC

42b3df9

The `ProxyTransport` implements a socks5 stream connection with the core rpc host, and request/receive http data from it. `Transport` trait is implemented on `ProxyTransport` to make it compatible with existing `bitcoincore-rpc::Client`.

Add RPC proxy options to RpcConfig

a9ce6a3

Handles proxy options to create a socks5 stream through proxy to rpc host. Creates regular http connection otherwise.

rajarshimaitra requested review from notmandatory and afilini December 3, 2021 13:06

rajarshimaitra changed the title ~~[WIP] Add a socks5 proxy transport for RPC blockchain~~ [WIP] Add Socks5 proxy transport for RPC blockchain Dec 3, 2021

Add ProxyTransport error into Global Error.

b99f1cf

rajarshimaitra force-pushed the proxy-in-rpc branch from 6f7af02 to b99f1cf Compare December 4, 2021 04:42

rajarshimaitra marked this pull request as ready for review December 7, 2021 05:59

rajarshimaitra changed the title ~~[WIP] Add Socks5 proxy transport for RPC blockchain~~ Add Socks5 proxy transport for RPC blockchain Dec 7, 2021

afilini reviewed Dec 7, 2021

View reviewed changes

notmandatory marked this pull request as draft February 8, 2022 20:20

notmandatory assigned rajarshimaitra Apr 4, 2022

rajarshimaitra mentioned this pull request Jun 7, 2022

[Feature Request] Enable Socks5 proxy support at Transport level apoelstra/rust-jsonrpc#57

Closed

notmandatory closed this Nov 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Socks5 proxy transport for RPC blockchain #493

Add Socks5 proxy transport for RPC blockchain #493

rajarshimaitra commented Dec 3, 2021

afilini left a comment

afilini Dec 7, 2021

rajarshimaitra Dec 8, 2021

afilini Dec 9, 2021

afilini Dec 7, 2021

rajarshimaitra Dec 8, 2021

afilini Dec 9, 2021

afilini Dec 7, 2021

afilini Dec 7, 2021

rajarshimaitra Dec 8, 2021 •

edited

Loading

afilini Dec 9, 2021

rajarshimaitra commented Dec 8, 2021 •

edited

Loading

afilini commented Dec 9, 2021

rajarshimaitra commented Dec 10, 2021

notmandatory commented Dec 16, 2021

rajarshimaitra commented Apr 4, 2022

notmandatory commented Apr 5, 2022

notmandatory commented Jun 7, 2022 •

edited

Loading

rajarshimaitra commented Jun 7, 2022

notmandatory commented Jun 7, 2022 •

edited

Loading

rajarshimaitra commented Jun 8, 2022 •

edited

Loading

notmandatory commented Nov 16, 2022 •

edited

Loading

rajarshimaitra commented Nov 17, 2022 •

edited

Loading

notmandatory commented Nov 19, 2022

Add Socks5 proxy transport for RPC blockchain #493

Add Socks5 proxy transport for RPC blockchain #493

Conversation

rajarshimaitra commented Dec 3, 2021

Description

Notes to the reviewers

Checklists

All Submissions:

New Features:

afilini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajarshimaitra Dec 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajarshimaitra commented Dec 8, 2021 • edited Loading

afilini commented Dec 9, 2021

rajarshimaitra commented Dec 10, 2021

notmandatory commented Dec 16, 2021

rajarshimaitra commented Apr 4, 2022

notmandatory commented Apr 5, 2022

notmandatory commented Jun 7, 2022 • edited Loading

rajarshimaitra commented Jun 7, 2022

notmandatory commented Jun 7, 2022 • edited Loading

rajarshimaitra commented Jun 8, 2022 • edited Loading

notmandatory commented Nov 16, 2022 • edited Loading

rajarshimaitra commented Nov 17, 2022 • edited Loading

notmandatory commented Nov 19, 2022

rajarshimaitra Dec 8, 2021 •

edited

Loading

rajarshimaitra commented Dec 8, 2021 •

edited

Loading

notmandatory commented Jun 7, 2022 •

edited

Loading

notmandatory commented Jun 7, 2022 •

edited

Loading

rajarshimaitra commented Jun 8, 2022 •

edited

Loading

notmandatory commented Nov 16, 2022 •

edited

Loading

rajarshimaitra commented Nov 17, 2022 •

edited

Loading