Please fix reproducibility issues around JS minification

[Giszmo]

Every other build this app is not reproducible, with 10MB minified JS files differing. This issue persists since over a year. For a Bitcoin wallet this is kind of unacceptable as the release manager might get away with modifications in this file unless manual checks occur which I have no indication of, especially not by third-party experts. If security researchers would sign off on every release, ... but ideally reproducing the build should just work.

(With this latest release 3.15.0, collected on 2022-02-28 03:25:05 I was not even sure if I checked the correct revision as the respective tag was missing. It was added an hour ago.)

[AndreasGassmann]

As mentioned in a previous discussion, the root for this issue is most likely some kind of race condition in the build. This race condition will cause the code to be "concatenated" in a non-deterministic way. During the minification step, all variables are renamed to something shorter, which will start with something like "aaa", "aab" etc. Because the code was concatenated randomly, the outputs are no longer easily comparable.

We did try to look into this, but it is extremely hard because the issue appears randomly. We will have to dedicate some time to track this down because the problem probably lies somewhere deep in the build system.

While I understand the decision to no longer examine our builds, I do disagree. Because even though it is inconvenient and annoying, if the reproducibility does succeed once, it means that the build was ultimately reproducible, meaning all the security assumptions around reproducible builds apply. So it's more of a usability issue rather than a security issue.

If anyone has any insights here, it would be greatly appreciated.

[AbakumovAlexandr]

My 2 cent questions.

When reproducing builds, does WalletScrutiny use exactly the same versions of:

• OS
• NodeJS
• NPM

?

Also, do versions of all packages in node_modules locked, so exactly the same dependencies are used in both cases?

[Giszmo]

@AbakumovAlexandr if we don't, it's not the root of the issue. As Andreas explained, this issue happens randomly at compile-time and not consistently based on the module versions used.

I worked for Mycelium before and we had some similar issue. As release manager I built the app and others from the team ran the build process multiple times and even worked with a list of artefacts to reproduce, as often file X succeeded but Y not and on the next run Y succeeded but X not. This way we assured ourselves to not release stuff that wasn't in the agreed code but obviously with my WalletScrutiny.com hat on I can't be bothered to go to this length with maybe multiple projects, all needing different hacks. If AirGap wants to convince the users that are worried due to our verdict, we happily will link such a statement below the review - ideally involving a convincingly independent security researcher who is willing to do what it takes to reproduce the builds.

[AbakumovAlexandr]

@Giszmo Got you. But AFAIK, Mycelium is not using the Angular build toolchain, it uses Android SDK, since it's the Kotlin app.

As an Angular engineer, I would tell that having exactly the same environment (including OS, NodeJS, NPM, packages related to the Angular build toolchain) apart from the same sources, obviously, is the key and the first thing to check to exclude myriad of variables which could lead to the issue of minified code chunks jumping around.

As Andreas explained, this issue happens randomly at compile-time
First of all, I would ask: were those multiple builds performed one by one and on the same machine without any changes to the environment, project\source files or anything else in between?

Since, if the suspect is things like race conditions in the build toolchain - the first thing is to reproduce everything which might add in to threads execution enthropy.

[dim0xff]

During the minification step, all variables are renamed to something shorter, which will start with something like "aaa", "aab" etc. Because the code was concatenated randomly, the outputs are no longer easily comparable.
...
If anyone has any insights here, it would be greatly appreciated.

I'd like to ask - why do you use minification here in project? Did you benchmark size and loading speed of minified and not-minified versions?
IMO it is necessary in web (where you have network connection with differ speed, lags, google speed rank, etc.). But in application it is not necessary (apk is zip-archive, but installed app of course will take more size)

[AbakumovAlexandr]

I'd like to ask - why do you use minification here in project? Did you benchmark size and loading speed of minified and not-minified versions?

I think they doesn't even need to benchmark it - it's known in advance that minified vs non-minified apps differs in size significantly.

I would not consider disabling the minification at least until some basic reproduce-ability measures would be implemented.

[AbakumovAlexandr]

@AndreasGassmann I've tried to build the app and see that there's no package-lock.json to guarantee that WalletScrutiny would build using exactly the same packages as on your build machine. I am not sure how we could ensure the same build artifacts without this prerequisite.

[AndreasGassmann]

Thanks @AbakumovAlexandr for looking into it. We use yarn, that's why there is no package-lock.json, but instead a yarn.lock. (Note: We switched to yarn a few months ago, this issue existed before and after the switch). We also added --frozen-lockfile everywhere to make sure yarn uses the dependencies from the lockfile. (feel free to verify that we didn't miss it anywhere).

@AbakumovAlexandr WalletScrutiny uses our Dockerfile to build the app, so the environment should be exactly the same because we use the same Dockerfile to build the prod app in our CI.

@dim0xff I don't remember the exact numbers, but I think it was in the ballpark of 20MB+ extra size with all optimisations disabled. We did not benchmark how this affects performance. But we also didn't check if this actually resolves the reproducibility issues.

[AbakumovAlexandr]

@AndreasGassmann On this part:

We use yarn, that's why there is no package-lock.json, but instead a yarn.lock.

Build instructions says npm install:

Build
First follow the steps below to install the dependencies:
$ npm install -g @capacitor/cli
$ npm install

Is it an outdated build step? Don't you use npm install in your Dockerfile?

[AndreasGassmann]

Yes, those build instructions are outdated. You can refer to the Dockerfile here for the latest build instructions: https://github.com/airgap-it/airgap-vault/blob/master/build/android/Dockerfile

Sidenote: I think npm actually uses the yarn.lock file if package-lock.json is not found.

[AndreasGassmann]

A small update: I didn't take a very deep look, but it seems that the issue comes from non-deterministic ordering when modules are concatenated by webpack 4. Angular 11, which we currently use, relies on webpack 4. I found mentions that webpack 5 improves this (eg. a module that deterministically orders imports is now enabled by default). I went ahead and updated our app to Angular 13, which uses webpack 5.

On my local machine, I rebuilt the old version 5 times and got 5 different outputs (basically I did yarn build:prod and moved the output folder, 5 times. Absolutely no other changes, eg. node_modules were exactly the same). As "expected", the ordering of some of the code was different.

I also rebuilt the new version with Angular 13, 5 times, and there I got exactly the same output every time. So there's definitely already an improvement. Sadly, when I built it in a docker container, the output was different compared to the one built outside the docker container. But this might be due to the environment being different (different node versions, etc). Hopefully, with the same environment inside the docker, the output will be the same on different machines.

I don't want to make any promises because the testing was minimal, but it does look like a step in the right direction.

The updated code is on github in the following branch: https://github.com/airgap-it/airgap-vault/tree/feat/ng-13

I will now build the APK of this branch on our CI and will try to reproduce it using the walletscrutiny pipeline. I will report the results here as soon as they are done.

[AndreasGassmann]

Looks like the upload to walletscrutiny was reproducible.

Hash: 49d8766bb0b536a3ea43bd04fd61909e40f81da7ad65895e37266fa954ae8355
Version Name: 3.17.0
Version Code: 41047
Date: 2022-04-25 06:24:25
Signature Verified: true
Reproducible: true
Test Result:
===== Begin Results =====
appId:          it.airgap.vault
signer:         
apkVersionName: 3.17.0
apkVersionCode: 41047
verdict:        reproducible
appHash:        49d8766bb0b536a3ea43bd04fd61909e40f81da7ad65895e37266fa954ae8355
commit:         1523ccdf4e366364e8749da6c8b1901d29004a55

Diff:

===== End Results =====

@Giszmo could you try locally and re-run the build on the server a couple of times?

If these tests are successful, we will finalize this branch and merge it into the next release. We can then decide how we handle it. We would obviously prefer if the status could be updated to reproducible again, and if it ever fails again, it could go back to not-reproducible. But we can also wait for a couple releases to make sure they are in fact consistently reproducible.

[AndreasGassmann]

I ran the build twice in our CI, same output. I uploaded the apk 3 times to walletscrutiny, it seems that all 3 builds were reproducible. Could you please confirm @Giszmo? Thanks

[Giszmo]

@AndreasGassmann that is great news! I can confirm that it also resulted "reproducible" locally. I downloaded the apk you had uploaded and ran:

$ ./test.sh --apk /path/to/it.airgap.vault_41047_49d8766bb0b536a3ea43bd04fd61909e40f81da7ad65895e37266fa954ae8355.apk --revision-override 1523ccd
...
===== Begin Results =====
appId:          it.airgap.vault
signer:         
apkVersionName: 3.17.0
apkVersionCode: 41047
verdict:        reproducible
appHash:        49d8766bb0b536a3ea43bd04fd61909e40f81da7ad65895e37266fa954ae8355
commit:         1523ccdf4e366364e8749da6c8b1901d29004a55

Diff:


Revision, tag (and its signature):
object 7df4dccebf60394cd0be4cb490f78807801d19b5
type commit
tag v3.17.0
tagger Mike Godenzi <m.godenzi@papers.ch> 1650367671 +0200

version 3.17.0
===== End Results =====

[AndreasGassmann]

Great to hear. We will merge this branch to master ASAP and I will upload a bunch of test versions before the release to make sure it's consistent.

[AndreasGassmann]

We have released 3.17.1 with the changes mentioned in this thread. I uploaded the APK to the walletscrutiny backend and it was reproducible.

[AndreasGassmann]

There was a regression in 3.17.1 (unrelated to the reproducibility, it was affecting account generation and signing because of a change that was introduced by Angular 13), we released 3.17.2 to fix address it.

[AndreasGassmann]

@Giszmo I wanted to follow up on this.

I have uploaded a couple of apks to your backend prior to the release of 3.17.2 and to my knowledge, all of them were reproducible.

Because it's an issue that appeared randomly, it's impossible to say for sure that we fixed it, but it looks promising so far.

How can we get the reproducible status back on walletscrutiny?

[Giszmo]

Hey @AndreasGassmann! Sorry for not having dedicated time to this in those past two weeks. To be honest, I'm thinking of dedicating my time to other projects after WalletScrutiny not getting significant donations in a whole year. It's almost been a full time job for more than two years and raised 1.9BTC which paid not only me. Guess my time is worth more than that.

I intend to update with what binaries I have. Please ping me if nothing happened by tomorrow.

[AndreasGassmann]

No problem about the delay.

It's sad to hear that the project does not seem to work out from a financial perspective. I think we can all agree that it does bring a lot of value to the space and the fact that we keep getting asked why our wallet status is not "reproducible" anymore does mean that at least some people care.

If you decide to move to other projects, I hope that you can find a way to keep the website up and allow apps to update their versions, or find a trustworthy person / group of people to hand the project over to.

[Giszmo]

Page is updated. Thanks for fixing this.