-
Notifications
You must be signed in to change notification settings - Fork 62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ARM architecture support. #99
base: release
Are you sure you want to change the base?
Conversation
Initial aarch64 support that allow this library to run tor on arm based systems. Signed-off-by: Zuev Aleksandr <[email protected]>
Exclude arm64 from test. Signed-off-by: Zuev Aleksandr <[email protected]>
Interesting concept. I really like the idea of supporting more platforms (similar to the macOS gap tracked here: #41). For ARM64, there are a couple of additional challenges:
Regarding repacking the archive format: it looks like the (unofficial) ARM64 binary distribution is the "browser bundle" rather than the "expert bundle". TorSharp used to handle both of these formats (see this diff as an example 673a359#diff-a6e7e09d5cdd0cb97db6eed09e1d45e7df7475fed489629a708c9321a1a3d7b5) so that could be reintroduced for ARM64. In other words, repacking should not be necessary. I don't have an ARM64 PC/board so I wouldn't be able to drive this implementation to completion. If you'd like to bring this support into the main TorSharp version, I have a couple of ideas:
|
Yes i think i can provide full arm support.
apt install tor
whereis tor
tor: /usr/bin/tor /usr/sbin/tor /etc/tor /usr/share/tor /usr/share/man/man1/tor.1.gz
whereis torrc
torrc: /usr/share/man/man5/torrc.5.gz
Also torrc in /etc/tor/torrc If it already running it show config path and it possible to parse port from config:
apt install privoxy
whereis privoxy
privoxy: /usr/sbin/privoxy /etc/privoxy /usr/share/privoxy /usr/share/man/man8/privoxy.8.gz
ps aux | grep privoxy
privoxy 15049 0.0 0.0 5468 3528 ? Ss 21:07 0:00 /usr/sbin/privoxy --pidfile /run/privoxy.pid --user privoxy /etc/privoxy/config Something like (both false by default): public class TorSharpSettings
{
public bool DetectSystemHaveTor { get; set; }
public bool DetectSystemHavePrivoxy { get; set; }
} If tor in system detected but cannot run skip it and go by default way. |
Using the system-wide Privoxy or Tor would be great. It's roughly supported in Privoxy, like this: This could be extended to work with Tor just like you mentioned. One tricky part is that the process and config should still be managed by TorSharp, it's just a matter of using the binary available on the system. So I guess you can try depending on tor in the path and then use You could also opt for this required system-installed Tor/Privoxy on ARM64 if you don't want to do the unofficial tool fetcher. I think the tool fetcher is very convenient so it's up to you how far you want to go. |
Oh and whatever new options you add, feel free to set the defaults to something that is nice and helpful on the ARM64 system. We shouldn't mess with existing platform defaults but new platforms can have new defaults that are useful and make sense on that plat. |
Automate binaries detection in linux. Signed-off-by: Zuev Aleksandr <[email protected]>
Armhf/Arm64 auto download feature. Signed-off-by: Zuev Aleksandr <[email protected]>
ARM archive correct extract. Signed-off-by: Zuev Aleksandr <[email protected]>
Fix unsecure Path.GetTempFileName(). Signed-off-by: Zuev Aleksandr <[email protected]>
process == null check goes after use methods inside variable. Signed-off-by: Zuev Aleksandr <[email protected]>
Remove useless null check. Simplify code. Make class static. Signed-off-by: Zuev Aleksandr <[email protected]>
src/TorSharp/ToolUtility.cs
Outdated
return false; | ||
|
||
// In linux most binaries exists in any bin directory, e.g /bin/ /sbin/ /usr/bin/ | ||
var toolPath = WhereIsUtility.WhereIs(toolSettings.TryFindExecutableName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious why whereis
is used instead of which
. Isn't the goal here just to find some system-provided executable? I think one benefit of using which
is that it only returns the first option by default. Consider a case where, prior to running the tool, the user adds tor or privoxy into PATH
manually which (generally) is a way to override which executable is used in subsequent steps of a script. It might be nice to use where
in such a case since I believe is documented to respect PATH.
Additionally, isn't enforcing /bin/
being in the path an unnecessary restriction? It seems to me that as long as where
returns a non-empty result for an executable name, we can just put that executable name in ExecutablePathOverride
and forget about the absolute full path to it (much like what privoxy does today with PrivoxySettings.ExecutablePathOverride
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's whereis simply because of linux command whereis.
Additionally, isn't enforcing
/bin/
being in the path an unnecessary restriction
Actually search in bin directory can be removed because whereis by default search in limited places as PATH variable.
But if user know where he put executable is it much easy for him just set ExecutablePathOverride them self, no?
By default whereis tries to find files from hard-coded paths, which are defined with glob
patterns. The command attempts to use the contents of $PATH and $MANPATH environment
variables as default search path. The easiest way to know what paths are in use is to add
the -l listing option. Effects of the -B, -M, and -S are displayed with -l.
https://manpages.ubuntu.com/manpages/bionic/man1/whereis.1.html
we can just put that executable name in ExecutablePathOverride
I think this is can be also target directory when binary will be searched.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if user know where he put executable is it much easy for him just set ExecutablePathOverride them self, no?
Yes, I agree. But from my play with whereis
it seems to provide a more complex set of output and behavior than the simple which
tool which is also available on all distros I've used (perhaps maybe not arm64 distros and that's what I'm missing)? Maybe I was naive in thinking "which" is the canonical (no pun intended) way to find the resolve path of an executable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems which
is better solution here. I now remember why regex here, it's because whereis
return more that just executable and regex needs to select from multiple matches. which
return only executable.
public static void CreateDirectoryIfNotExists(string path) | ||
{ | ||
if (!Directory.Exists(path)) | ||
Directory.CreateDirectory(path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Directory.CreateDirectory
already gracefully handles the case when the directory already exists. I think this CreateDirectoryIfNotExists
method is not necessary.
@@ -34,26 +34,18 @@ public Task StartAsync(Tool tool) | |||
startInfo.EnvironmentVariables[pair.Key] = pair.Value; | |||
} | |||
|
|||
Process process = Process.Start(startInfo); | |||
Process process = Process.Start(startInfo) ?? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch. I didn't know Process.Start
could return null but I see that in the docs now.
@@ -41,5 +41,10 @@ internal class Tool | |||
/// The full path to the tool's configuration file. | |||
/// </summary> | |||
public string ConfigurationPath { get; set; } | |||
|
|||
/// <summary> | |||
/// Automate detected in system. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Automate detected in system. | |
/// The tool was automatically detected in the PATH. |
(assuming we switch to which
)
src/TorSharp/Tools/Tor/TorFetcher.cs
Outdated
if (_settings.Architecture.HasFlag(TorSharpArchitecture.Arm)) | ||
{ | ||
if (!_settings.AllowUnofficialSources) | ||
throw new TorSharpException($"AllowUnofficialDistribution == false, there is no official tor distribution for {_settings.Architecture} platform."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great. Thanks!
#else | ||
OSPlatform = TorSharpOSPlatform.Windows; | ||
Architecture = Environment.Is64BitProcess ? TorSharpArchitecture.X64 : TorSharpArchitecture.X86; | ||
#endif | ||
|
||
PrivoxySettings = new TorSharpPrivoxySettings(); | ||
TorSettings = new TorSharpTorSettings(); | ||
|
||
#if NETSTANDARD | ||
if (OSPlatform == TorSharpOSPlatform.Linux) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a change in default behavior. Can we just scope this to the Arm32 and Arm64 architectures?
One drawback of defaulting to the system-provided on is that the Tor or Privoxy version on the system (i.e. provided by distro package maintainers) may not update to the latest version as quickly as what the tool fetcher in TorSharp can provide (which takes the latest build released by the upstream maintainers).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may not update to the latest version
Fair. But still bottom page of readme says why this is better for linux) Last version not always best and most dists e.g ubuntu or debian update security flaws in packages and yes it almost never latest version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good point. Using the system version is probably better on Linux.
If you want to change the defaults for non-ARM Linux, make an update to the README with an FAQ on how to revert to the previous behavior (2.14.0 and older, using some configuration setup) where the system-provided version is not used by default. That way people can choose to opt out of this behavior change if they want.
Assert.NotNull(result.Url); | ||
} | ||
|
||
//[Fact] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can delete commented code
|
||
using Xunit; | ||
|
||
namespace Knapcode.TorSharp.Tests.ARM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have a test suite for the system-provided privoxy and tor. I wonder if we can do an apt-get install for privoxy and tor in the GitHub Actions YAML and then test the system-provided flow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you can run any command inside. Docs says you can run apt install.
https://docs.github.com/en/actions/using-github-hosted-runners/customizing-github-hosted-runners
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great so far! I've left some comments about the system-provided behavior, testing on GitHub Actions, and some nitpicks.
I'd like to personally try this out on QEMU -- which I just learned is a thing -- and maybe my Raspberry pi (which I forgot I had 😂)
@@ -26,7 +26,7 @@ public async Task ApplySettings(Tool tool, TorSharpSettings settings) | |||
try | |||
{ | |||
// write first to a temporary file | |||
temporaryPath = Path.GetTempFileName(); | |||
temporaryPath = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting: https://rules.sonarsource.com/csharp/RSPEC-5445.
I never heard of this before. Thanks!
Change some variable names and comments. Signed-off-by: Zuev Aleksandr <[email protected]> Co-authored-by: Joel Verhagen <[email protected]>
Apply variable rename in code. Signed-off-by: Zuev Aleksandr <[email protected]>
Replace whereis with which. Add windows auto search.
This allow only run exists tor files (set TorSharpSettings.ZippedToolsDirectory and TorSharpSettings.UseExistingTools), because fetch and run code must be entire rewritten.
Arm version can be downloaded here https://sourceforge.net/projects/tor-browser-ports/files/ but they have different folder structure and it require to repack .tar.gz archive to get it work.
For example tor-browser-linux-arm64-12.0.3_ALL.tar.xz:
Tor and Data folders are located in tor-browser\Browser\TorBrowser.
Repack example:
tor-linux-aarch64-12.0.3.tar.gz
Maybe create nuget package to provide repacked tar.gz archives? Or rewrite current code to support auto download (those archive size larger that x86 bundles because they contains entire browser) and different folders structure?
Tested on Radxa Rock-5B board.