Allow TorService to be used in a separate process

[grote]

Running TorService in a separate process has a couple of advantages:

• when Tor crashes, the app doesn't crash with it
• Tor can be restarted frequently without crashing (e.g. tor#23847 tor#23848)
• might improve security

This PR enables that.

Note that this also upgrades jtorctl.

[grote]

This is to ensure that Tor doesn't outlive the app (for more than 15sec) if it crashes. Otherwise, it would continue idling on the system (potentially indefinitely) consuming resources.

[eighthave]

Android has native mechanisms for ensuring that a Service does not outlive its app. Binding is one of them. TorService works much more in the Android space than tor daemon ever did. This kind of thing seems like a holdover from running tor as a daemon. (If we eliminate the control port socket, then it will be even more in the direction of native Android.)

[eighthave]

I read up a bit on this option, it doesn't seem like something that should be built into TorService because it is about the daemon-style lifecycle and the control port. If it is something you want to use, you can generate your own torrc and TorService will use that file.

[akwizgran]

We use this option in Briar to prevent the Tor process from outliving the controller process, which was a recurrent source of bugs for both Briar and Orbot in the past. When owingProcessPid != -1 the controller and Tor library are running in different processes, so that possibility resurfaces.

As far as I know, Android doesn't guarantee that a service will be destroyed if a bound client in another process crashes. The service may be cached, for example. So I don't think it's wise to rely on Android destroying the service.

[akwizgran]

Another quick thought here: when TorService is running as a foreground service (which we're doing in OnionShare because Android gives various power management advantages to foreground services), we've found than simply unbinding is not enough to stop or destroy the service. I don't know whether the same applies when a bound client dies, rather than unbinding, but I think it would at least be worth checking that the service gets destroyed before we stop using __OwningControllerProcess.

[akwizgran]

I don't think we'll want to use WorkManager for OnionShare, as the tasks started by the app are expected to start immediately, not to be scheduled for later execution when some constraints are met.

Those other tasks should be in a Service instance that is separate from TorService. OnionShare could have multiple uploads running at the same time. Each of those would run in an instance of UploadService that is bound to OnionShare's unique TorService. Or there could be a CoordinatorService which manages the notifications and is set as a foreground service.

That's one possible way of structuring things, yes. But it would be nice if the library didn't impose a specific architecture on the application.

[eighthave]

WorkManager now considers foreground processing a first class use case: https://www.droidcon.com/2021/11/17/workmanager-back-to-the-foreground/ see at 6:30. WorkManager is clearly the way forward with Tor on Android.

That's one possible way of structuring things, yes. But it would be nice if the library didn't impose a specific architecture on the application.

This library does not impose that kind of architecture, Android does. A central goal of this library is to make Tor behave natively on Android. Android works quite differently than UNIX, Windows, and macOS. Using desktop lifecycle constructs on Android and iOS works quite poorly, that's why this is a goal.

[grote]

If you refer to Expedited work then that is "for tasks that are important to the user and which complete within a few minutes." This not the case for the OnionShare use-case where you might want to leave the share running for more than a day without the fear of yet another system service killing you early.

[grote]

See also the docs on quotas: https://developer.android.com/topic/libraries/architecture/workmanager/how-to/define-work#quotas

If you want a library to be used, you really shouldn't impose a specific architecture on the application. If you want an easy way to use this with WorkManager, it is fine to add this as an extra wrapper around the general solution.

IMHO even offering only a Service and broadcasts is already going to far into a certain direction.

[eighthave]

From the link you sent:

While your app is in the foreground, quotas won’t limit the execution of expedited work.

Here's more on using WorkManager for long running services, "Example use-cases for this new feature include bulk uploads or downloads":
https://developer.android.com/topic/libraries/architecture/workmanager/advanced/long-running

(github messed up the threading so this is a repost)

[eighthave]

As discussed elsewhere, I think it is quite risky to rely on UNIX domain sockets for IPC here. Android does not official support them, the Java code never included support for them, and the Android team has been locking down all sorts of things. For example, they are blocking reflection entirely in many cases. They are also blocking ioctl and other things. Even if Chromium uses UNIX domain sockets (and not Linux abstract sockets), they might implement a permission needed to get access to them, and then not allow third party apps with that permission into Google Play. The Android team has been breaking functionality in their drive for locking things down.

The Android ways of IPC are:

• Sending Intent instances to Service and Activity instances.
• Binder
• AIDL

AIDL sucks but I think Binder and Intent are both workable here. I had sketched out some stuff using Binder from an app to the TorServices app, and it seemed like a good approach.

[eighthave]

Also, I think using UNIX domain sockets between processes will be brittle, and that's one thing I have been focused on improving with TorService. For example, the client will have to be able to handle the case if TorService crashes or gets killed, and the socket goes away. Or maybe the socket stays up but hangs. Binder and Intent both provide tools to handle that kind of stuff.

[akwizgran]

As discussed elsewhere, I think it is quite risky to rely on UNIX domain sockets for IPC here. Android does not official support them, the Java code never included support for them, and the Android team has been locking down all sorts of things.

TorService already uses a Unix domain socket to talk to the Tor library's control port. Do you think it's likely that Unix sockets will continue to be supported for in-process communication while being blocked for inter-process communication (between processes with the same UID)? That seems unlikely to me, but even if it does happen, perhaps we should cross that bridge when we come to it?

[eighthave]

When I implemented the UNIX domain socket hack, I tried first just making the socket, then decided that the hack was the only safe way. I don't remember the details there, it was a while ago. But I do remember that Android seems to be built around Linux abstract sockets, which still use something called a "path" but have no file on the filesystem and no access controls. I still haven't seen something confirming that it is possible to use UNIX domains sockets in Java on the filesystem with access controls without doing a JNI hack. That kind of JNI hack is the kind of thing they are locking down. For example, they already locked down the Java Reflection that would have worked in place of the JNI hack.

There are lots of official Android docs about IPC, sockets are never mentioned. The only Android app I've ever seen that using UNIX domain sockets is Chromium. It has special status in Android and Google, so they get special permissions as needed. Also, it has an elaborate IPC auth system to enforce access controls. A key goal with TorService was getting fast, stable startup. Eliminating the auth helped a lot there.

[akwizgran]

A key goal with TorService was getting fast, stable startup. Eliminating the auth helped a lot there.

Yes, I think using a Unix socket rather than a TCP socket for the control port is a big improvement in that regard.

If I understand right, you'll keep using the JNI approach internally for now, but you'd prefer not to expose it in the API of the library in case it breaks in future and you need to find a new way of connecting to the control port?

That's fine from our side. We've found a way of using LocalSocket to connect to Tor's control port from our own code without JNI. You might be able to use this approach inside the library too. Create a LocalSocketAddress using the path of the control socket and namespace = LocalSocketNamespace.FILESYSTEM, then get its file descriptor and use that to open a FileInputStream and FileOutputStream as before.

This is all documented stuff that's been available since API 1, so I doubt it will change without warning.

So we can remove the part of the patch that exposes the FileDescriptor via the library's API, if you prefer.

[eighthave]

Did you confirm with a test that there is a file on the filesystem with proper access controls?

[akwizgran]

Yes, this uses the same file as the JNI method.

[eighthave]

Ok, I hope it works out. When I was implementing TorService, there was something I found that spooked me and gave me the feeling that I couldn't be sure that access controls would always work using `LocalSocket`. I can't remember more than that now, I wish I had documented it...

[akwizgran]

I cannot see why. Service binding is not complex or hard to use. And it is well documented.

It adds complexity because now we need two services: TorService itself, and a second service whose only purpose is to bind to TorService in order to ensure that TorService gets destroyed if the client dies. With __OwningControllerProcess TorService would be responsible for making sure it gets destroyed if the client dies, which seems like a more convenient library to me.

[grote]

As starting/stopping Tor within the same process is known to cause problems and crashes, I think this library should make it as easy as possible to be run in a dedicated process, even encourage it. Needing another service and cross-process binding isn't exactly the most easy way to achieve this.

[eighthave]

Are you still seeing the crashes with 0.4.6.8? Have you tried 0.4.6.9? I agree that it should be easy to run TorService in a separate process. That was a core design goal. It does not require another Service to achieve this. I'm recommending small, lightweight Services that bind to each other as a way to make it work most reliably on Android. That was also a core design goal, and I based on the Android docs as much as I could.

[grote]

Are you still seeing the crashes with 0.4.6.8?

The double close and thread-safety issues have been resolved with 0.4.6.8.

However, when run in process, the random Tor crashes when restarted frequently persist.

Have you tried 0.4.6.9?

Is that even out, yet? i don't see a tag for it.

It does not require another Service to achieve this.

Then how do you suggest to ensure that Tor gets taken down when the app crashes or gets killed? Ideally, the library doesn't allow you to shoot yourself in the foot that easily and take care of such things out of the box.

[eighthave]

FYI, you can use __OwningControllerProcess now, just do this right before starting TorService:

File torrc = TorService.getTorrc(this);
FileUtils.writeStringToFile(torrc, "__OwningControllerProcess " + android.os.Process.myPid());
// start TorService 

And you can get TorService.getControlSocket() via reflection.

[eighthave]

Then how do you suggest to ensure that Tor gets taken down when the app crashes or gets killed? Ideally, the library doesn't allow you to shoot yourself in the foot that easily and take care of such things out of the box.

I don't understand why Tor must be shutdown in that scenario. If TorService hasn't been killed by Android, then it seems fine to reconnect to it and use it again. Or is reusing it triggering the crashes that you are seeing? Also, Tor provides Dormant Mode for making sure it doesn't stay active when it is not being used. I think it would make sense here. @n8fr8 told me it is used by Orbot via DormantClientTimeout.

If you are using the UNIX domain socket and __OwningControllerProcess works for you, seems fine to use it. I have no experience with it.

v0.4.6.9 is up on Maven Central.

[eighthave]

I don't think we'll want to use WorkManager for OnionShare, as the tasks started by the app are expected to start immediately, not to be scheduled for later execution when some constraints are met. WorkManager starts fast enough for things like uploads or messaging sending, but not for things like rendering the UI for the current screen. It provides really good scheduling and background options, as well as lifecycle management. Like the network requirement. For example, if a job is marked as requiring network, and the devices looses network. The job should exit saying it did not succeed. Then WorkManager will start it up again once the network is available again. On top of that, it is much easier to test and provides the best debug logging I've seen for this kind of thing on Android. This example seems quite close to OnionShare: https://medium.com/androiddevelopers/workmanager-basics-beba51e94048 That's one possible way of structuring things, yes. But it would be nice if the library didn't impose a specific architecture on the application. TorService is meant to be a native Android Service while imposing as little as possible. Things like the Service lifecycle and Binder for IPC come from Android, not TorService. TorService was designed around the Android-recommended practices for running things in background. As far as I can tell, you can do your UNIX daemon approach with TorService right now. Use reflection to get the domain socket path and set config options via torrc. Setting command line flags is not possible with TorService, but those can be set via torrc. Managing command line executables is definitely not recommended on Android, and really only accidentally possible.

[eighthave]

Just went through this with @grote, it seems that 0.4.6.9 covers the issues that this merge request was originally intended to address, so I'm closing this. Please open new issues if anything still needs work.

[eighthave]

From the link you sent: While your app is in the foreground, quotas won’t limit the execution of expedited work. Here's more on using WorkManager for long running services, "Example use-cases for this new feature include bulk uploads or downloads": https://developer.android.com/topic/libraries/architecture/workmanager/advanced/long-running