Remove arti dependency in favor of tor proxy#319
Conversation
rustaceanrob
force-pushed
the
remove-arti-3-29
branch
from
3442ce3 to
a5f7a0b
Compare
rustaceanrob
force-pushed
the
remove-arti-3-29
branch
from
a5f7a0b to
4518685
Compare
rustaceanrob
force-pushed
the
remove-arti-3-29
branch
4 times, most recently
from
35b66d4 to
06eaf5a
Compare
The Arti project does not have an official MSRV, and the dependency list has a number of failed security audits caught by the CI job in this repository. The legacy Tor client is sufficient for desktop and some mobile users, and no developers have expressed interest in using Arti directly. A simple Socks5 proxy that connects to a Tor daemon allows for Tor connections without an explicit dependency that causes MSRV conflicts, security audits, and lib-sqlite version conflicts.
rustaceanrob
force-pushed
the
remove-arti-3-29
branch
from
06eaf5a to
64c9046
Compare
| impl_sourceless_error!(PeerError); | ||
|
|
||
| #[derive(Debug, Clone)] | ||
| pub(crate) enum Socks5Error { |
There was a problem hiding this comment.
More org bike shedding, but I find it strange to have the Socks5Error type defined outside of the socks module.
There was a problem hiding this comment.
Yeah, another all-or-nothing change though since I have the DNS errors here as well. I typically prefer anything that is an error just be thrown into the error module. I would consider it mostly a stylistic choice so I don't think I'll touch it for now
There was a problem hiding this comment.
Yea, I don't think it is mentioned explicitly in the official style guide. Just reminds me of OG java when we would put things in models and what not.
| const THIRTY_MINS: u64 = 60 * 30; | ||
| const CONNECTION_TIMEOUT: u64 = 2; | ||
|
|
||
| pub(crate) type StreamReader = Box<dyn AsyncRead + Send + Sync + Unpin>; |
There was a problem hiding this comment.
Is the overhead of dynamic dispatch worth it if this is pub(crate)? Usually the overhead is useful for flexibility for the caller, but here we are always the caller. Can't we just "hardcode" this to the types we are gonna use and keep static dispatch?
There was a problem hiding this comment.
When using Arti the most common thing between usual TCP and over Tor was the AsyncRead/Write traits, but this could be made concrete everywhere with OwnedReadHalf and OwnedWriteHalf. I will resist that refactor for now since web transport or something similar may come along and implement the AsyncRead/Write traits in the future (similar to Arti connections).
There was a problem hiding this comment.
But I do see what you mean, could be another refactor in the future where a peer is supplied a concrete transport enum that contains how they should write and read messages. Although, that's what dynamic dispatch is great for, since it will start to get unruly fast
There was a problem hiding this comment.
I have only recently started to think through this, so not a ton of experience, but I think I have a preference to start static and only introduce they dynamic flexibility when the complexity justifies it. Again, not a game changer here, but something I might experiment with later to just get a better feel for it.
| }; | ||
| // Begin the handshake by requesting a connection to the proxy. | ||
| let mut tcp_stream = timeout.map_err(|_| Socks5Error::ConnectionFailed)?; | ||
| tcp_stream.write_all(&[VERSION, METHODS, NOAUTH]).await?; |
There was a problem hiding this comment.
This handshake is pretty straight forward, wonder if we can split it out so it is unit testable. The tokio runtime is buyin a ton for cancellation cleanup so the tight integration makes sense, just thinking another internal function or two to cover the protocol logic.
There was a problem hiding this comment.
This function isn't inherently cancel safe but it's called directly from the main application loop and is not apart of a select statement, so it is guaranteed to execute in full. Unit tests get tricky because what is the source of truth for the server side? Would either be hardcoded, which feels like homework, add arti as a dev dependency - which I would rather not, or do some edgy shell commands to launch a Tor daemon, also not great.
There was a problem hiding this comment.
I was thinking a simple mock buffer just to exercise the state machine which is built in here. Don't think it would be too difficult and would probably make the handshake logic super clear. Don't need to refactor now, but might take a stab at it in the future.
2 participants
Having
artias a dependency has restricted the ability to make an MSRV guarantee for users. It is also difficult to say if it would even be used as a feature, but users can still take advantage of Tor withoutarti. Similar to the Electrum crates in the ecosystem, we can direct traffic to a Socks5 proxy that points at the Tor daemon. Again, the market is yet to be seen for using Tor specifically, so a simple local proxy pointing to127.0.0.1:9050should be sufficient in most cases.This allows for a reduction in the abstraction of network connections as well. Using a simple enumeration instead of dynamic dispatch should not only perform better, it should also make the code more legible.