Opened 6 months ago

Closed 5 months ago

#29981 closed task (fixed)

Add option to build without using containers

Reported by: boklm Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-rbm, TorBrowserTeam201904R, tbb-8.5
Cc: hans@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

By default the Tor Browser build is done inside containers, which we run using runc, which require root access.

In some cases, such as F-Droid builds (#27539), this can be a problem. I think we should be able to add an option to do the builds without using containers.

Child Tickets

TicketStatusOwnerSummaryComponent
#30089closedtbb-teamUse apksigner instead of jarsignerApplications/Tor Browser

Attachments (4)

liblgpllibs_1.so (16.3 KB) - added by gk 5 months ago.
liblgpllibs_2.so (18.5 KB) - added by gk 5 months ago.
pkg_list.txt (81.8 KB) - added by boklm 5 months ago.
List of packages installed on my build machine.
pkg_list_tpo (116.6 KB) - added by gk 5 months ago.

Download all attachments as: .zip

Change History (33)

comment:1 in reply to:  description ; Changed 6 months ago by gk

Replying to boklm:

By default the Tor Browser build is done inside containers, which we run using runc, which require root access.

In some cases, such as F-Droid builds (#27539), this can be a problem. I think we should be able to add an option to do the builds without using containers.

How would that help the F-Droid case given that F-Droid is taking just the signature for an app and applying that one to the built they made themselves? I mean the non-container build would then need to match our build done in a containerized setup so that F-Droid would get a bit-for-bit identical output. Maybe I am too pessimistic here but I'd be surprised if that worked out-of-the-box...

comment:2 in reply to:  1 Changed 6 months ago by boklm

Replying to gk:

Replying to boklm:

By default the Tor Browser build is done inside containers, which we run using runc, which require root access.

In some cases, such as F-Droid builds (#27539), this can be a problem. I think we should be able to add an option to do the builds without using containers.

How would that help the F-Droid case given that F-Droid is taking just the signature for an app and applying that one to the built they made themselves? I mean the non-container build would then need to match our build done in a containerized setup so that F-Droid would get a bit-for-bit identical output. Maybe I am too pessimistic here but I'd be surprised if that worked out-of-the-box...

If the F-Droid build VM is using Debian Strech too, then I think the main differences between our build environment and the F-Droid build environment would be:

  • the username of the build user. In the rbm build we use an rbm user, but I think we can add an option to change it if that is an issue.
  • the list of packages installed. In our case we build each component with only the minimum set of dependencies required to build this component, while in the F-Droid case they would always have the dependencies required to build all of the components. I think in most cases the additional packages should not change the output of the build. Maybe in some cases it will requires some changes to avoid the effect of some additional package.

Unless I'm forgetting an other important difference, I think it should not be very difficult to get matching builds. Although I did not try it, so maybe I'm too optimistic.

comment:3 Changed 6 months ago by boklm

I started working on a patch for that, and after comparing a build done without containers, and a normal build with containers, I get an almost matching apk. The only difference is:

Binary files 1/META-INF/ANDROIDQ.RSA and 2/META-INF/ANDROIDQ.RSA differ
diff -r 1/META-INF/ANDROIDQ.SF 2/META-INF/ANDROIDQ.SF
2,4c2,4
< SHA1-Digest-Manifest-Main-Attributes: nJdsdwNyiHipRN3sUz498qUG+L0=
< SHA1-Digest-Manifest: vF7lnm0ro+G4diQqON1e3X8+snc=
< Created-By: 1.8.0_171 (Oracle Corporation)
---
> SHA1-Digest-Manifest-Main-Attributes: E8IbsJM7v8R6d8dy6OqA+g4Q9EE=
> SHA1-Digest-Manifest: cBYG++7ArsIP93o3h2mGE0OM4WI=
> Created-By: 1.8.0_212 (Oracle Corporation)
diff -r 1/META-INF/MANIFEST.MF 2/META-INF/MANIFEST.MF
2c2
< Created-By: 1.8.0_171 (Oracle Corporation)
---
> Created-By: 1.8.0_212 (Oracle Corporation)

It seems to be because the tor-browser step is done in a buster container. The debug signature is also causing the build without containers to stall.

So I think if we find an other workaround for the debug signature that doesn't require using buster, we should be able to get matching build without and without containers.

comment:4 Changed 6 months ago by eighthave

Wow, you have achieved reproducible builds then! The oracle version number diff can be solved by using apksigner rather than jarsigner to make the APK Signatures. apksigner is what the Android SDK has been using by default for about 2 years now, and is reproducibly built in Debian stretch and buster:

https://tests.reproducible-builds.org/debian/rb-pkg/stretch/amd64/android-platform-tools-apksig.html

comment:5 Changed 6 months ago by eighthave

If you need the newer apksigner in Debian/stretch, I can backport it. As far as I remember, the changes are only to support APK v2 and v3 signatures better, and were not security fixes. But it might be worth double checking my memory.

comment:6 Changed 6 months ago by eighthave

Cc: hans@… added

comment:7 Changed 6 months ago by sisbell

Technically, you don't need to update to apksigner although there is nothing wrong with it.

You can override the default manifest Oracle. First create the a manifest.txt to use to override entries

Created-By: Tor Build

Then use the 'm' flag

jar -cvfm tor-browser.jar manifest.txt

The generated manifest will be

Manifest-Version: 1.0
Created-By: Tor Build

comment:8 in reply to:  7 Changed 6 months ago by boklm

Replying to sisbell:

Technically, you don't need to update to apksigner although there is nothing wrong with it.

You can override the default manifest Oracle. First create the a manifest.txt to use to override entries

Created-By: Tor Build

Then use the 'm' flag

jar -cvfm tor-browser.jar manifest.txt

The generated manifest will be

Manifest-Version: 1.0
Created-By: Tor Build

The main issue is not that some entries in the manifest are different, but that the signing is stalling when we do it with faketime on stretch, which is the reason we currently have to do it in a buster container.

If apksigner does not stall when used with faketime on stretch (or doesn't need faketime to produce reproducible output), then that would solve the issue.

comment:9 Changed 6 months ago by eighthave

While you are at it, you should also drop faketime for signing. The Android tools have been zeroing out the dates in the ZIP header for a while now, like 2+ years. If TBA doesn't use a version of the Android tools that does that, it is not hard to do it with a tool or with Python or something as a post-build, pre-signing process.

comment:10 Changed 6 months ago by boklm

Using the patch from branch bug_29981_v3 I have been able to do a build without containers:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_29981_v3&id=ab3c5d96be66d09d4dc98be385e495244bef48af

The apk I got does not match the one built using container, but the only difference is:

--- tor-browser-8.5a10-android-armv7-multi-qa.apk
+++ ../../testbuild/tor-browser-8.5a10-android-armv7-multi-qa.apk
├── zipinfo {}
│ @@ -1502,13 +1502,13 @@
│  -rwxr-xr-x  2.0 unx     5596 b- defX 10-Jan-01 00:00 lib/armeabi-v7a/libplugin-container.so
│  -rw-r--r--  2.0 unx      382 b- defX 10-Jan-01 00:00 application.ini
│  -rw-r--r--  2.0 unx       32 b- defX 10-Jan-01 00:00 package-name.txt
│  -rw-r--r--  2.0 unx      795 b- defX 10-Jan-01 00:00 ua-update.json
│  -rw-r--r--  2.0 unx       48 b- defX 10-Jan-01 00:00 platform.ini
│  -rw-r--r--  2.0 unx       97 b- defX 10-Jan-01 00:00 removed-files
│  -rw-r--r--  2.0 unx 10641927 b- stor 10-Jan-01 00:00 assets/omni.ja
│ --rw-r--r--  3.0 unx   502590 b- defN 00-Jan-01 00:00 assets/distribution/extensions/{73a6fe31-595d-460b-a920-fcc0f8843232}.xpi
│  -rw-r--r--  3.0 unx  1765652 b- defN 00-Jan-01 00:00 assets/distribution/extensions/https-everywhere-eff@eff.org.xpi
│ +-rw-r--r--  3.0 unx   502590 b- defN 00-Jan-01 00:00 assets/distribution/extensions/{73a6fe31-595d-460b-a920-fcc0f8843232}.xpi
│  -rw----     2.0 fat   144465 b- defN 10-Jan-01 00:00 META-INF/ANDROIDQ.SF
│  -rw----     2.0 fat     1194 b- defN 10-Jan-01 00:00 META-INF/ANDROIDQ.RSA
│  -rw----     2.0 fat   144360 b- defN 10-Jan-01 00:00 META-INF/MANIFEST.MF
│  1512 files, 58292874 bytes uncompressed, 44899152 bytes compressed:  23.0%

I think the cause of this is this line in projects/tor-browser/build.android:

zip -r -X $apk $ext_dir

Which is not adding the extensions to the .apk in a sorted order.

comment:11 Changed 6 months ago by eighthave

Wow, great progress! That sort order issue is frustrating. My experience is that doing post-processing of APKs with zip or other tools usually leads to random repro issues like that. The Android tools have been getting better, but they still have repro quirks. It seems that the zip commands are used for adding and removing static assets. I can help move that to the pre-packaging phase of the Android build, if gradle is in use.

comment:12 Changed 5 months ago by gk

Keywords: tbb-8.5-must added

comment:13 Changed 5 months ago by boklm

Keywords: TorBrowserTeam201904R added; TorBrowserTeam201904 removed
Status: newneeds_review

There are two patches for review in branch bug_29981_v5:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_29981_v5&id=069c880961e93fe2f5052ae6d428b2f9efdd1d4e

I have been able to get matching builds from this branch for the android-x86 and android-armv7 builds.

The packages that I installed on a stretch machine to be able to do the build are:

build-essential python bison automake libtool zip unzip autoconf2.13 yasm openjdk-8-jdk gettext-base autotools-dev automake autoconf libtool autopoint libssl-dev pkg-config zlib1g-dev libparallel-forkmanager-perl libfile-slurp-perl bzip2 xz-utils apksigner

comment:14 Changed 5 months ago by gk

Keywords: TorBrowserTeam201904 added; TorBrowserTeam201904R removed
Status: needs_reviewneeds_revision

While still testing the changes here come some preliminary comments:

commit e38b5d94a19caff488382d92968dbd8cd85b18eb

The change to the README says "Adding the no_containers target will disable the use of containers", but the first thing that happens is actually building a container (called "empty"). So, we might want to be a bit more verbose explaining what is happening here to avoid confusion.

"and install build dependencies for all the components that are built" We are listing build dependencies in the README file. I think we should list all the additional build dependencies needed for building without containers as well while we are talking about them. There should be no need for builders that want to build without containers to try figuring that out themselves.

commit 069c880961e93fe2f5052ae6d428b2f9efdd1d4e

"Add the extensions in sorted order" it's not only extensions it seems? So, please adjust the commit message accordingly.

I wonder why we need container/use_container and container/disable as two separate options. If use_container is false then cleary I build without containers, no? In other words: why can't we just use/amend the already existing use_container option to avoid creating another one that seems to do the same thing but just (slightly) differently?

comment:15 Changed 5 months ago by gk

Alright, I started a build on two different machines, both on commit 069c880961e93fe2f5052ae6d428b2f9efdd1d4e (boklm/bug_29981_v5). One machine is building with containers and one without. While this setup builds reproducibly on each machine the *.apks on one machine differ from those on the second one.

The differing parts are essentially all the Firefox .so files in assets/ or lib/. Both machines built from the same tor-browser commit (c722d57604db58695140d95565a78433989fe9ca).

I attached two .so files showing the issue.

FWIW: I built on the bare stretch machine without installing bison, yasm, and openjdk-8-jdk. (However, openjdk-8-jre is installed it seems)

Changed 5 months ago by gk

Attachment: liblgpllibs_1.so added

Changed 5 months ago by gk

Attachment: liblgpllibs_2.so added

Changed 5 months ago by boklm

Attachment: pkg_list.txt added

List of packages installed on my build machine.

comment:16 in reply to:  15 ; Changed 5 months ago by boklm

Replying to gk:

FWIW: I built on the bare stretch machine without installing bison, yasm, and openjdk-8-jdk. (However, openjdk-8-jre is installed it seems)

I'm wondering if the difference in the build could be caused by some different versions of packages installed, or maybe additional or missing packages.

In my case, the build that I did without containers is matching the one done with containers. I attached the list of packages installed on this machine (the output from dpkg -l). Is there any difference with the list of packages installed on your build machine, other than bison, yasm and openjdk-8-jdk?

comment:17 in reply to:  16 ; Changed 5 months ago by gk

Replying to boklm:

Replying to gk:

FWIW: I built on the bare stretch machine without installing bison, yasm, and openjdk-8-jdk. (However, openjdk-8-jre is installed it seems)

I'm wondering if the difference in the build could be caused by some different versions of packages installed, or maybe additional or missing packages.

In my case, the build that I did without containers is matching the one done with containers. I attached the list of packages installed on this machine (the output from dpkg -l). Is there any difference with the list of packages installed on your build machine, other than bison, yasm and openjdk-8-jdk?

Okay, installing openjdk-8-jdk does not make a difference. I attached the package list of the stretch machine I used. In fact, you have access to it, too, it's the TPO build machine. In case you want to make more elaborate queries to figure out what the important difference here is.

Changed 5 months ago by gk

Attachment: pkg_list_tpo added

comment:18 Changed 5 months ago by gk

Keywords: tbb-8.5 added; tbb-8.5-must removed

Given the reproducibility issues and the work to get this properly going on F-Droid itself we might need to resort to hc helping us again for 8.5 given that we don't want to postpone it further and test the native F-Droid setup in an alpha build.

comment:19 in reply to:  17 ; Changed 5 months ago by boklm

Replying to gk:

Okay, installing openjdk-8-jdk does not make a difference. I attached the package list of the stretch machine I used. In fact, you have access to it, too, it's the TPO build machine. In case you want to make more elaborate queries to figure out what the important difference here is.

I did a build without containers on build-sunet-a, with commit 069c880961e93fe2f5052ae6d428b2f9efdd1d4e, after installing all the packages from comment 13, I have x86 and armv7 apk files matching the ones I built on an other machine with containers.

Can you try to do an other build on this machine?

Also, did you do alpha or nightly builds? In my case I did alpha builds, as it makes it easier to be sure both machines are using the same commits.

This is the apk files that I get:

e88fcd32929f86765e17ef0ccf1979c64a1fe9b2d6e2099590bac3b061f31487  testbuild/tor-browser-8.5a11-android-armv7-multi-qa.apk
df0823edbc7aeb3c19a33bada6edde6743958d4b71629a24e6fed8d999ff4814  testbuild/tor-browser-8.5a11-android-x86-multi-qa.apk

comment:20 in reply to:  19 Changed 5 months ago by gk

Replying to boklm:

Replying to gk:

Okay, installing openjdk-8-jdk does not make a difference. I attached the package list of the stretch machine I used. In fact, you have access to it, too, it's the TPO build machine. In case you want to make more elaborate queries to figure out what the important difference here is.

I did a build without containers on build-sunet-a, with commit 069c880961e93fe2f5052ae6d428b2f9efdd1d4e, after installing all the packages from comment 13, I have x86 and armv7 apk files matching the ones I built on an other machine with containers.

Can you try to do an other build on this machine?

Will do.

Also, did you do alpha or nightly builds? In my case I did alpha builds, as it makes it easier to be sure both machines are using the same commits.

Nightlies. But that should not matter much here as the Firefox libs where different and I used the same Firefox commit for both builds. But I can try doing an alpha one.

comment:21 in reply to:  14 Changed 5 months ago by boklm

Keywords: TorBrowserTeam201904R added; TorBrowserTeam201904 removed
Status: needs_revisionneeds_review

Replying to gk:

While still testing the changes here come some preliminary comments:

commit e38b5d94a19caff488382d92968dbd8cd85b18eb

The change to the README says "Adding the no_containers target will disable the use of containers", but the first thing that happens is actually building a container (called "empty"). So, we might want to be a bit more verbose explaining what is happening here to avoid confusion.

I added something about that in the README, and renamed the file empty to containers_disabled.

"and install build dependencies for all the components that are built" We are listing build dependencies in the README file. I think we should list all the additional build dependencies needed for building without containers as well while we are talking about them. There should be no need for builders that want to build without containers to try figuring that out themselves.

commit 069c880961e93fe2f5052ae6d428b2f9efdd1d4e

"Add the extensions in sorted order" it's not only extensions it seems? So, please adjust the commit message accordingly.

I pushed a new revision of the commits fixing that in branch bug_29981_v6:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_29981_v6&id=718d1ab83e497cc9492d3c8b1fc101095e984e81

I wonder why we need container/use_container and container/disable as two separate options. If use_container is false then cleary I build without containers, no? In other words: why can't we just use/amend the already existing use_container option to avoid creating another one that seems to do the same thing but just (slightly) differently?

container/use_container is defined in projects configs, which have priority over definitions in rbm.conf. So if we want to change the value of container/use_container for the no_containers target, we would have to do it in all projects/*/config files where it is defined. I have renamed var/container/disable to var/container/global_disable to try to make it less confusing.

comment:22 Changed 5 months ago by gk

Thanks for the update. How did you get the list of packages to install? E.g. I did not need to install bison nor yasm to get the mobile browser built, so it seems those are not actually needed.

comment:23 in reply to:  22 Changed 5 months ago by boklm

Replying to gk:

Thanks for the update. How did you get the list of packages to install? E.g. I did not need to install bison nor yasm to get the mobile browser built, so it seems those are not actually needed.

I looked at the list of packages we list as dependencies for android in the various projects/*/config files, and rbm.conf.

For bison, we have it in rbm.conf. We added it to the default list of packages for the other platforms with commit 78a5009fa4e0fc3e7e4ccd854df7dd3833b6b174, as it seems it is needed to build binutils, and adding it to the default list of packages avoids creating and storing a specific container for the binutils build. Then we probably copied it to the android list of dependencies without checking that it was really required. We could probably remove it from the osx-x86_64 build too as we don't need to build binutils there. I opened #30325 for that.

For yasm, we have it in the list of packages needed to build firefox on all platforms, although it seems it's not needed for the android build. I opened #30326 to fix that.

In branch bug_29981_v7 I updated the README to remove bison and yasm from the list of packages to install:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_29981_v7&id=0fc15a39f2adcf029676810c2ec37c566886879e

comment:24 Changed 5 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Thanks for the detailed analysis! The new branch looks good to me. I've merged it to master (commits 9072578261fc2e4198e39b81313683bf2674c5b4 and 0fc15a39f2adcf029676810c2ec37c566886879e).

comment:25 Changed 5 months ago by boklm

Resolution: fixed
Status: closedreopened

So it seems yasm is not needed for the android-armv7 build, but still needed for the android-x86 one.

In branch bug_29981_v8 I added a new commit mentioning that in the README:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_29981_v8&id=1959d1c9dbe993b8fb4732b333892cb04f7ccfae

comment:26 Changed 5 months ago by boklm

Status: reopenedneeds_review

comment:27 in reply to:  25 ; Changed 5 months ago by gk

Keywords: TorBrowserTeam201904 added; TorBrowserTeam201904R removed
Status: needs_reviewneeds_revision

Replying to boklm:

So it seems yasm is not needed for the android-armv7 build, but still needed for the android-x86 one.

In branch bug_29981_v8 I added a new commit mentioning that in the README:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_29981_v8&id=1959d1c9dbe993b8fb4732b333892cb04f7ccfae

The previous apt-get install comment does not talk about android-armv7 specifically, thus starting now talking of android-x86 seems a bit weird to me. I think *if* we go with just installing yasm everywhere to not create an extra container (I agree with that idea) then we should treat it as a first-class citizen here and just add it to the other packages. Otherwise we are starting to divert regarding installed packages between container/non-container builds (think of folks thinking "I only want to build android-armv7 packages, thus no yasm for me`) which might fall onto our feet at some point.

Maybe a note in the commit message would be enough to help us remembering in case we stumble over that again in the future? Otherwise I could think of "This can be done
with the following command (note: yasm is only needed for android-x86 builds):" to make our point clear while minimizing the risk for deviating from the packages installed in our container builds.

comment:28 in reply to:  27 Changed 5 months ago by boklm

Keywords: TorBrowserTeam201904R added; TorBrowserTeam201904 removed
Status: needs_revisionneeds_review

Replying to gk:

Maybe a note in the commit message would be enough to help us remembering in case we stumble over that again in the future?

Yes, I think it's probably enough. I did that in branch bug_29981_v9:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_29981_v9&id=f7ae040da39ac0009776802371e088f15531b9e7

comment:29 Changed 5 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Thanks. Merged to master (commit f7ae040da39ac0009776802371e088f15531b9e7) and maint-8.5 (commit 7e784c57f614b97a9b2f614bc0550fd5ade1e735).

Note: See TracTickets for help on using tickets.