Support Launching TorService Using JNI
Activity
A working version of this is here: https://gitlab.com/eighthave/tor-android
I'll be working to get it integrated into https://gitlab.com/guardianproject/tor-android
Trac:
Keywords: N/A deleted, Android added
Cc: gk, sysrqb to gk, sysrqb, hans@guardianproject.infoI assume some of the native methods are custom for tor-android so that we can run in embedded. If we are using tor from the tor browser build, are there any hoops we need to go through to get this integrated?
Are the native methods only supported on Android or all platforms? I'm asking this since TOPL is desktop/mobile so this good info to have for planning whether we can continue support across the board.
The version of https://gitlab.com/eighthave/tor-android is not referencing the version of tor that contains the JNI classes so app fails at runtime. I did find the correct JNI code in the following commit that I used for review.
https://gitlab.com/eighthave/tor/commit/a453e9faf3e4be946506111754b18f9fcf1ac595
I realize its early development. Some review comments.
-
I still see that we need to wait for the control port creation file. This is what was causing some earlier problems. Will this be fixed with the JNI code in the future or does the problem lie in the tor code itself?
-
Authentication appears to just be default, with no cookies. If we add cookie base auth, I think we would also need to wait for that file creation as well.
I bring up (1) and (2) specifically since these were pain points in the past and don't seem to be mitigated in the current code.
-
The JNI code will lock down the implementation class as TorService, which is also the Android service class. I think it would be better to move the JNI interaction to a different class, which is then called by TorService class. This will make it more reusable by third-parties.
-
We are using file descriptor for the control port with the JNI version. This is good. (It does mean this only support *nix based systems).
I agree we should move forward with JNI integration since we shouldn't be spawning processes on Android. However, outside of this general principle, I don't see any additional JNI advantages at this point.
-
-
Are the native methods only supported on Android or all platforms? I'm asking this since TOPL is desktop/mobile so this good info to have for planning whether we can continue support across the board.
The JNI code should work on any Java or Android version. I tested them on Java only in the beginning, so I don't know the status there. But any breakage there is most likely trivial. Is Desktop TBB using TOPL?
- I still see that we need to wait for the control port creation file. This is what was causing some earlier problems. Will this be fixed with the JNI code in the future or does the problem lie in the tor code itself?
The best solution for this is to create a pipe/stdin/stdout interface for the ControlPort. Then Java code can interact directly with it, without any kind of socket. This current solution should be better than before because it is based on a UNIX domain socket rather than a TCP socket. So it uses the Linux inotify API, which is quite reliable, to get an event when the socket file is created.
- Authentication appears to just be default, with no cookies. If we add cookie base auth, I think we would also need to wait for that file creation as well.
I bring up (1) and (2) specifically since these were pain points in the past and don't seem to be mitigated in the current code.
With a pipe/stdin/stdout interface for the ControlPort, cookie auth is not needed. On Android, creating the UNIX socket file within the app's private file dirs should eliminate the need for the cookie auth, based on standard Android file permissions. Setting the permissions on the socket file itself further ensures that. I have the cookie auth working as well, with an inotify wait, if that's needed. But that'll likely be more brittle, given more moving parts.
The change here is switching from TCP socket to UNIX domain socket on the filesystem. That will eliminate a lot of pain points:
- no more TCP port conflicts
- access control via standard Android file/dir permissions
- detect controlport creation using Linux inotify
- The JNI code will lock down the implementation class as TorService, which is also the Android service class. I think it would be better to move the JNI interaction to a different class, which is then called by TorService class. This will make it more reusable by third-parties.
I agree that the JNI code should work on Java and Android. There is nothing about src/feature/api/org_torproject_jni_TorService.c that locks it to Android. JNI files do not normally declare a Java class, they implement methods for a class declared in a .java file. tor-android-binary/src/main/java/org/torproject/jni/TorService.java is the Android setup. There should be a separate TorService.java for Java. There isn't enough in the TorService.java that is shareable between Android and Java. Trying to make a shared super class will make the code really confusing and hard to follow, while only sharing some trivial bits of code.
- We are using file descriptor for the control port with the JNI version. This is good. (It does mean this only support *nix based systems).
The UNIX domain socket code in
Java_org_torproject_jni_TorService_prepareFileDescriptor()might only be relevant on Android, but it probably works on Java. I think there are probably better ways to do this on Java. Desktop TBB is not using TOPL. I think its fine targeting Android for the JNI work, as that is the primary platform we are interested in. We can just stick with your implementation of the unix socket for the control port.
As we break apart everything, we can pick and choose which components it makes sense to share between the projects. I'm still interested in creating pure Java components where we can, as this would be more useful to the general community.
The Android specific pieces deal mostly around broadcasts and shared preferences so those can be abstracted out without complicating the code. Orbot already has abstractions for the data so its just a matter of standardizing all of that similar to what we are talking about with jtorctl.
-
I agree that having Java support is good, plus I don't think it'll be much work. I've been working on TorService and jtorctl always with supporting both Java and Android in mind. The JNI code should already support Java, its just not tested there for a while.
The clear border of sharing is src/feature/api/org_torproject_jni_TorService.c instead of TorService.java. The biggest block of logic in TorService.java is the startup procedure, and it seems very unlikely that the startup procedure should be shared between Java and Android. They will have similarities, but not close enough that code sharing makes sense. So if that core stuff isn't shared, then separating the Android-specific stuff like broadcasts from the Android-specific startup logic would only make the code more unreadable and unmaintainable.
I'm not in exact agreement that startup can't be shared without making the code unmaintainable (although this has some subjectivity). I'm using a base broadcaster, followed by an Android broadcaster that encapsulates the logic. The main class is then just invoking the interface implementation.
This all doesn't have to be decided up-front. It looks good enough for now. After getting the Android tor variant integrated into the build system, the next step will be to pick this up to include the embedded JNI interface and implementation. After that, we can circle back on more integration specifics.
Replying to eighthave:
I agree that having Java support is good, plus I don't think it'll be much work. I've been working on TorService and jtorctl always with supporting both Java and Android in mind. The JNI code should already support Java, its just not tested there for a while.
The clear border of sharing is src/feature/api/org_torproject_jni_TorService.c instead of TorService.java. The biggest block of logic in TorService.java is the startup procedure, and it seems very unlikely that the startup procedure should be shared between Java and Android. They will have similarities, but not close enough that code sharing makes sense. So if that core stuff isn't shared, then separating the Android-specific stuff like broadcasts from the Android-specific startup logic would only make the code more unreadable and unmaintainable.
-
I agree that the broadcasting stuff can be shared. I think that the EventListener/RawEventListener class to jtorctl can serve as the shared base class.
About the startup stuff, Android has such different handling of filesystem interactions and process startup that it seems there is always inevitable quirks between things that seem like they could be the same. I wish I could remember concrete examples, I'll try to update this thread if I think of any.
This is still going to be supported. The prerequisite is that we get all the issues related to getting tor built in tbb. Once that is done, we can either patch the tor project or commit needed changes directly to support JNI.
