search

Nitrokey / nitrokey-hotp-verification

A command line C app to validate HOTP codes on Heads
GNU General Public License v3.0
11 stars 10 forks source link

Downstream issues with reproducibility due to submodules #13

Closed szszszsz closed 4 years ago

szszszsz commented 4 years ago

From: https://github.com/Nitrokey/nitrokey-hotp-verification/issues/12#issuecomment-578532190:

@szszszsz : latest (see created reproducibility issue here): "The trick applied upstream to fix commit IDs cannot be applied because it relies on hidapi. That needs to be fixed since, once again, upstreamed version changed last week and CI caches builds unless cache are flushed manually and restarted, which then builds from scratch."

Example of used solution: https://github.com/osresearch/heads/blob/master/modules/musl-cross#L7

szszszsz commented 4 years ago

@tlaurion @kylerankin While I think proper usage of the Git and submodules in downstream use should not be a concern of this project per se, as an alternative solution we could potentially move to git subtree to allow this. I have not used it, but as far as I understand this would include a snapshot of other repository commit into this one, which would make it available in a zip file as well, as downloaded in the example.

I leave to you the decision, implementation, and tests.

tlaurion commented 4 years ago

@kylerankin @szszszsz : For Heads, I would recommend creating a new hidapi module (could be a dumb module just like coreboot-blobs is defined under libremkey-hotp-verification just like coreboot module does it), on which libremkey-hotp-verification would then depend, using the same git archive download trick, fixing extracting strip depth accordingly in module with a variation of how it's done for coreboot-blobs.

Edit: point more directly to tricks currently used under Heads to fix current Purism/Nitrokey module reproducibility issues under Heads, permitting a change upstream in modules to download new git revision and build fresh. Otherwise, there is no reproducibility of builds, users/CI not redownloading sources, as proven in the past.

szszszsz commented 4 years ago

@tlaurion I understand your proposition is a no-action for this project, but instead to configure it downstream. @kylerankin Please close this ticket, if you agree.

tlaurion commented 4 years ago

@jans23 @szszszsz : Still an issue for crosscompiling that needs upstream help, see referenced issue.

szszszsz commented 4 years ago 
initrd.cpio.xz in artifacts for both builds.
xz -d initrd.cpio.xz
cpio --extract < tools.cpio

7fec08e7baa4d6b92533ba1bade4ce965c25f88b5acba293780efbe021cb01ab  ./bin/libremkey_hotp_verification

9b9655bfeeea6897b573d8e96635df2a8e8f7f58bf108c1273c06a497b4ebd77  ./bin/libremkey_hotp_verification

Latest build links:

szszszsz commented 4 years ago

Reopening, since the reproducibility problem still persist

szszszsz commented 4 years ago

Hi @tlaurion ! I have found the source of the differences. One comes from the assert messages, which are available in the debug build - simply changing to the release configuration will fix this. The other one is different format of git describe returned on both CI's - it could be removed by switch. Please retest by running the build with cmake .. -DADD_GIT_INFO=OFF -DCMAKE_BUILD_TYPE=Release

Full strings diff ``` 94c94 < /root/project/build/libremkey-hotp-verification-d4948fddd617022635129f9b00db3f98de01016f/device.c --- > /builds/tlaurion/heads/build/libremkey-hotp-verification-d4948fddd617022635129f9b00db3f98de01016f/device.c 119c119 < /root/project/build/libremkey-hotp-verification-d4948fddd617022635129f9b00db3f98de01016f/operations.c --- > /builds/tlaurion/heads/build/libremkey-hotp-verification-d4948fddd617022635129f9b00db3f98de01016f/operations.c 122c122 < /root/project/build/libremkey-hotp-verification-d4948fddd617022635129f9b00db3f98de01016f/base32.c --- > /builds/tlaurion/heads/build/libremkey-hotp-verification-d4948fddd617022635129f9b00db3f98de01016f/base32.c 134c134 < v0.2.0-758-g198029d --- > 198029d0 ```
tlaurion commented 4 years ago

@mrchromebox @szszszsz @kylerankin @jans23 @osresearch: Pipelines running suggested implemention change in heads module still produces different binaries (aside of bzImage reproducibility problem):

GitlabCI (ubuntu:18.04 docker image): df1ebcdbcc61c7dcd1ce3f8626e5389de47e9dc5143647d95492b1dab7b3b890 ./bin/libremkey_hotp_verification

CircleCI(Fedora:30 docker image): 9e6bd3647c0c7769e4ecd620243844666fa2c0d590e7e9812e2de6e71458dbc8 ./bin/libremkey_hotp_verification

Logs now available in downloadable artifacts: GitlabCI CircleCI

Note that this patch against upstream code should be merged upstream instead of patched against it.

Also note that librem-hotp-verification module is now based on commit d4948fddd617022635129f9b00db3f98de01016f

szszszsz commented 4 years ago

The initial change (setting the Release configuration) has helped, as the __FILE__ macro was used in the assertions, so the build path was part of the result. However there are still binary differences, which are not immediately obvious, e.g. the symbols for hidapi are moved exactly 0x1000 further in the binary while comparing the files (edit: that is binaries from CI builds from Gitlab and CircleCI) with diffoscope. So far I have not localized the direct cause for the current differences:

  1. The reproducible build tool reprotest, which injects intentionally changes to the file system, build path, system clock and locale, have not found any differences, while executed on the same Docker images, but with distro's current GCC.
  2. I have applied flags which are used in other projects built in heads project, but this has not changed the final sha256 sum in the local Docker tests. The used Hidapi library also receives the flags for stable build (confirmed with the compiler invocation), and is managed by the CMake.

This means the issue is occurring only during the Heads build, with that particular build-provided compiler configuration. Further ideas:

I will add in the further commit local build reproduction test with Docker and reprotest tool.

tlaurion commented 4 years ago

@osresearch? @flammit?

szszszsz commented 4 years ago

Object files artifacts from CI's (idea 1):

I plan to check this in the following days, unless someone would be faster.

Edit: note to myself: track the libusb use and build process, as a potential culprit.

szszszsz commented 4 years ago

I made a brief comparison and it appears the resulting code is not stable, as in this example: Screenshot from 2020-03-27 16-28-48 Here in the assembler instruction the arguments order is not changing the final result, however swapping them between builds breaks build reproducibility. Symbol table seems to not be stable as well. Overall only half of the build objects are different between CIs: affected-objects.txt.

I have disabled all optimization flags and run rebuild on CIs. Waiting for the results. Will look into it in the following days.

szszszsz commented 4 years ago

Attempt to turn off optimizations made it even worse. Now all files are with different checksums. Diffoscope reports, that symbols are not ordered. To check:

Builds:

Below difference example from version.c, which contains only application version. Screenshot from 2020-03-28 09-19-52

szszszsz commented 4 years ago

I am happy to report, that I got first SHA256 sums match between CIs while using Makefile as the builder, and perhaps solving the issue. The direct cause of the initial problem might be, that the CMake toolchain file for cross-compilation was missing some variable setting (either cross tool path, or flags) thus different end-result binaries. Other cause could be having different CMake binary and different flags set, as it seemed to not be rebuilt as opposing to Gnu Make, but taken from the OS repository.

Hashes:

Builds' links:

Edit: working hotp-verification commit: https://github.com/Nitrokey/nitrokey-hotp-verification/commit/809953b9b4bef97a4cffaa20d675bd7fe9d8da53

Additionally I had some problems on Gitlab CI, which were caused by invalid caching due to having the .git directory as ingredient, hence the problems downloading repo updates. Added temporary fix - to correct by caching only build/ directory.

tlaurion commented 4 years ago

@szszszsz : check the upstream circleci

tlaurion commented 4 years ago

@szszszsz thanks a lot for this, just saw the update.

Will look into details tomorrow, just started quick build during the night, changing simply the commit id to 06e5004 and retesting.

https://app.circleci.com/pipelines/github/tlaurion/heads/165/workflows/cb0c014b-6806-42cc-ba17-596713a48c78/jobs/178 https://gitlab.com/tlaurion/heads/-/jobs/560019205

szszszsz commented 4 years ago

Actually you need to change the build command too, to use Makefile instead of the CMake. I have not found the direct cause in the latter yet, however I believe it might not be needed and Makefile build is sufficient for Heads as-is. The whole Heads is based on Makefiles anyway, and all dependencies are properly provided (including stable GNU Make rebuild during the process), which does not seem to be for CMake. I will prepare proper diffs and PRs.

Will use the CI patch, thank you for the heads up!

tlaurion commented 4 years ago

don't hesitate to tag me back in, else not notified 😃

szszszsz commented 4 years ago

@tlaurion I have made a PR at https://github.com/tlaurion/heads/pull/5.

tlaurion commented 4 years ago

@szszszsz : please do against upstream, my branch is far from being up to date!

Just change the modules required. I see some magic being applied to CIs that would be interesting to push as seperate PRs if you found some interesting magic there!

If you do not care about ownership of those changes, I can play with them around, but would definitely prefer the maintainer of the mainstream project to maintain their modules inside of Heads.

Will comment on my branch meanwhile.

szszszsz commented 4 years ago

@tlaurion Sure! Now that I've learned the Heads better I can maintain the project's package there. Just ping me and create a ticket of you need anything.

About CI, I just added some defending code to remove old objects' cache (I've overlooked the cache path in the configuration) and added some debug artifacts, nothing fancy I think.

If I could suggest anything, perhaps it is worth considering extracting build script to external file, instead of building it up there. That would allow to run it locally easier as well. https://github.com/osresearch/heads/commit/7600ce4bff4075aeb4f490bcdcfff597cdfbea74

tlaurion commented 4 years ago

If I could suggest anything, perhaps it is worth considering extracting build script to external file, instead of building it up there. That would allow to run it locally easier as well. osresearch/heads@7600ce4

@szszszsz sorry I do not understand. What would you add?

szszszsz commented 4 years ago

Sorry, I meant extracting lines from the script array in Gitlab CI config file link to a separate file, which could help in readability potentially.