Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

x/build/cmd/relui: reproducibility diffs in darwin signed tar generation #61513

Closed
rsc opened this issue Jul 21, 2023 · 9 comments
Closed

x/build/cmd/relui: reproducibility diffs in darwin signed tar generation #61513

rsc opened this issue Jul 21, 2023 · 9 comments
Assignees
@heschi
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-Darwin
Milestone
Unreleased

Comments

@rsc
Copy link
Contributor

rsc commented Jul 21, 2023 •
edited
Loading

The Darwin tar files that get rebuilt to put in the signed binaries
are different from the cmd/dist-generated ones in a few ways.
For now I can teach the reproduction checker I'm working on to ignore
these differences, but could these divergences be corrected in relui?
It seems like they should be straightforward
(perhaps it requires packing the tar file in Go code instead of invoking the system tar though).

  • The standard cmd/dist-generated tar files have no uid/gid/uname/gname - they're 0/0/""/"".
    The signed ones say 501/0/gopher/wheel.
    Let's clear the signed ones too (or preserve the cmd/dist-provided metadata, either way).

  • The standard cmd/dist-generated tar files have no entries for directories.
    The signed ones add entries for directories, with semi-random time stamps, like go/ and go/api/ below (and more).
    Let's remove the tar entries for directories.

  • The mtime on signed binaries is changed to be different from the mtime used consistently in the rest of the archive.
    Let's keep the mtime the same on all files.

There is also an inconsistency in the file order but I think that one is cmd/distpack's fault and I will fix it there.

Thanks!

% tar tzvf cmddist.tgz | sed 10q
-rw-r--r--  0 0      0        1337 Jul 13 15:00 go/CONTRIBUTING.md
-rw-r--r--  0 0      0        1479 Jul 13 15:00 go/LICENSE
-rw-r--r--  0 0      0        1303 Jul 13 15:00 go/PATENTS
-rw-r--r--  0 0      0        1455 Jul 13 15:00 go/README.md
-rw-r--r--  0 0      0         419 Jul 13 15:00 go/SECURITY.md
-rw-r--r--  0 0      0          36 Jul 13 15:00 go/VERSION
-rw-r--r--  0 0      0        1142 Jul 13 15:00 go/api/README
-rw-r--r--  0 0      0       35424 Jul 13 15:00 go/api/except.txt
-rw-r--r--  0 0      0     2687115 Jul 13 15:00 go/api/go1.1.txt
-rw-r--r--  0 0      0       30821 Jul 13 15:00 go/api/go1.10.txt
% tar tzvf signed.tgz | sed 10q
drwxr-xr-x  0 0      0           0 Jul 13 17:49 go/
-rw-r--r--  0 gopher wheel    1337 Jul 13 15:00 go/CONTRIBUTING.md
-rw-r--r--  0 gopher wheel    1479 Jul 13 15:00 go/LICENSE
-rw-r--r--  0 gopher wheel    1303 Jul 13 15:00 go/PATENTS
-rw-r--r--  0 gopher wheel    1455 Jul 13 15:00 go/README.md
-rw-r--r--  0 gopher wheel     419 Jul 13 15:00 go/SECURITY.md
-rw-r--r--  0 gopher wheel      36 Jul 13 15:00 go/VERSION
drwxr-xr-x  0 gopher wheel       0 Jul 13 15:14 go/api/
-rw-r--r--  0 gopher wheel    1142 Jul 13 15:00 go/api/README
-rw-r--r--  0 gopher wheel   35424 Jul 13 15:00 go/api/except.txt
%
@gopherbot gopherbot added the Builders x/build issues (builders, bots, dashboards) label Jul 21, 2023
@gopherbot gopherbot added this to the Unreleased milestone Jul 21, 2023
@rsc
Copy link
Contributor Author

rsc commented Jul 21, 2023

https://go-review.googlesource.com/c/go/+/511977 is the distpack change to fix the file sort order to match what a system tar or fs.WalkDir would use.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/511977 mentions this issue: cmd/distpack: sort files in standard walk order

@rsc
Copy link
Contributor Author

rsc commented Jul 21, 2023 •
edited
Loading

One more issue - the mtimes on signed binaries are different from the mtime used on all the other files in the archive. Updated list in top comment.

@heschi heschi self-assigned this Jul 21, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/511759 mentions this issue: internal/relui: improve reproducibility of signed tarballs

gopherbot pushed a commit to golang/build that referenced this issue Jul 21, 2023
@heschi @gopherbot
internal/relui: improve reproducibility of signed tarballs
e32031f 
When we get the signed binaries back, the signing process has modified
the tarball somewhat. We want the files to match as much as possible, so
undo those changes. (And avoid making one change ourselves.)

- Don't add a directory entry for the go/ root dir. It's unnecessary and
  not included in distpacks. This will affect non-distpack builds, but
  nobody is scrutinizing those.
- Remove all other the directory entries too, which were inserted by the
  signing process.
- Set the timestamps back to the distribution's timestamps.
- Clear the user information on the modified files.

For golang/go#61513

Change-Id: I0b3508bc2547364e2a2b49e1c6ea7be8fe92b308
Reviewed-on: https://go-review.googlesource.com/c/build/+/511759
Reviewed-by: Dmitri Shuralyov <[email protected]>
Auto-Submit: Heschi Kreinick <[email protected]>
Run-TryBot: Heschi Kreinick <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
@dmitshur dmitshur changed the title x/build: reproducibility diffs in darwin signed tar generation x/build/cmd/relui: reproducibility diffs in darwin signed tar generation Jul 22, 2023
@dmitshur dmitshur moved this to In Progress in Go Release Jul 22, 2023
@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Jul 22, 2023
@rsc
Copy link
Contributor Author

rsc commented Jul 22, 2023

Replied on the submitted CL, but since it is merged I will comment here too: distpack does not set AccessTime and ChangeTime, so I think those should be zeroed, not set to the mtime.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/512437 mentions this issue: internal/relui: improve reproducibility of signed tarballs, take 2

gopherbot pushed a commit to golang/build that referenced this issue Jul 24, 2023
@heschi @gopherbot
internal/relui: improve reproducibility of signed tarballs, take 2
1df5673 
Trying to predict the format of the distpack archives was probably not a
good idea. Use the same approach we use for the module zips: tweak the
contents of the files, and leave the metadata untouched.

For golang/go#61513

Change-Id: I6e498b573eade71ec907590af05e7d3c0f25eb57
Reviewed-on: https://go-review.googlesource.com/c/build/+/512437
Auto-Submit: Heschi Kreinick <[email protected]>
Run-TryBot: Heschi Kreinick <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
gopherbot pushed a commit that referenced this issue Jul 26, 2023
@rsc
cmd/distpack: sort files in standard walk order
3afa5da 
The code was sorting files in the archives entirely by path string,
but that's not what fs.WalkDir would do. In a directory with
subdirectory foo/bar and file foo/bar.go, foo/bar gets visited
first, so foo/bar/baz appears before foo/bar.go, even though
"foo/bar/baz" > "foo/bar.go".

This CL replaces the string comparison with a path-aware
comparison that places foo/bar/baz before foo/bar.go,
so that if the tar file is extracted and then repacked using
fs.WalkDir, the files will remain in the same order.

This will make it easier to compare the pristine distpack-produced
tgz for darwin against the rebuilt tgz with signed binaries.

Before:
% tar tzvf /tmp/cmddist.tgz | grep -C1 runtime/cgo.go
-rw-r--r--  0 0      0       11122 Jul 13 15:00 go/src/runtime/callers_test.go
-rw-r--r--  0 0      0        2416 Jul 13 15:00 go/src/runtime/cgo.go
-rw-r--r--  0 0      0        2795 Jul 13 15:00 go/src/runtime/cgo/abi_amd64.h

After:
% tar tzvf pkg/distpack/go1.21rsc.src.tar.gz | grep -C1 runtime/cgo.go
-rw-r--r--  0 0      0        1848 Dec 31  1969 go/src/runtime/cgo/signal_ios_arm64.s
-rw-r--r--  0 0      0        2416 Dec 31  1969 go/src/runtime/cgo.go
-rw-r--r--  0 0      0        2479 Dec 31  1969 go/src/runtime/cgo_mmap.go

For #24904.
For #61513.

Change-Id: Ib7374bc0d6324377f81c561bef57fd87b2111b98
Reviewed-on: https://go-review.googlesource.com/c/go/+/511977
Reviewed-by: Dmitri Shuralyov <[email protected]>
Reviewed-by: Heschi Kreinick <[email protected]>
Run-TryBot: Russ Cox <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
@heschi
Copy link
Contributor

heschi commented Aug 1, 2023

I've pushed relui and I believe we're done here. Let me know if I'm wrong.

@heschi heschi closed this as completed Aug 1, 2023
@github-project-automation github-project-automation bot moved this from In Progress to Done in Go Release Aug 1, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/515415 mentions this issue: cmd/gorebuild: check uid/gid/uname/gname/mtime fields in tgz files

gopherbot pushed a commit to golang/build that referenced this issue Aug 2, 2023
@dmitshur @gopherbot
cmd/gorebuild: check uid/gid/uname/gname/mtime fields in tgz files
1ddff20 
Issue 61513 is resolved so this path can be turned on now.
Confirmed to still pass now that go1.21rc4 is out. It was
the first release built using improvements from CL 512437.

For golang/go#57120.
For golang/go#58884.
For golang/go#61513.

Change-Id: Ie39765f8c7ba514dea2bfccf7c8ef8acc5822a22
Reviewed-on: https://go-review.googlesource.com/c/build/+/515415
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Russ Cox <[email protected]>
Auto-Submit: Russ Cox <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
Run-TryBot: Dmitri Shuralyov <[email protected]>
@dmitshur dmitshur mentioned this issue Aug 8, 2023
Closed
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/552015 mentions this issue: internal/relui: handle error from loadBinaries

gopherbot pushed a commit to golang/build that referenced this issue Dec 21, 2023
@dmitshur @gopherbot
internal/relui: handle error from loadBinaries
8014f11 
Caught while working on go.dev/issue/63147.

For golang/go#61513.

Change-Id: Ied9d3a6716759dfe40bf224f6b60bf733e07dc13
Cq-Include-Trybots: luci.golang.try:x_build-gotip-linux-amd64-longtest-race
Reviewed-on: https://go-review.googlesource.com/c/build/+/552015
Auto-Submit: Dmitri Shuralyov <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
Reviewed-by: Carlos Amedee <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
@golang golang locked and limited conversation to collaborators Dec 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@heschi heschi

Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-Darwin
Projects
Go Release
Archived in project
Milestone
Unreleased
Development

No branches or pull requests
6 participants
@rsc @dmitshur @bcmills @gopherbot @heschi and others