Reproducible release tarballs #1550

patmaddox · 2024-12-13T22:19:35Z

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=283214

picking up the work from https://reviews.freebsd.org/D48010

TODO:

fix usr/share/man/mandoc.db non-determinism

brooksdavis

Looks pretty good. A few comments.

The existing @ prefixes are IMO bugs so you can drop them. I'm not sure why there are there when the surrounding code doesn't use them.

Makefile.inc1

release/Makefile

jrtc27 · 2024-12-13T23:47:07Z

I can't add myself as a reviewer but would like to review this properly in the coming days

jrtc27 · 2024-12-14T20:45:49Z

build: Default METALOG ownership to root/wheel

Release calls etcupdate.sh, which calls certctl. When building as an
unprivileged user, this resulted in the certs with uid of the building
user.

PR: 283214

Signed-off-by: Pat Maddox [email protected]

This isn't the right fix. The correct thing to do is to fix certctl, not paper over it and any other potential buggy build steps.

jrtc27 · 2024-12-14T20:50:57Z

build: Explicitly select debug files when distributing with -DNO_ROOT
Most of the debug files live in usr/lib/debug, but
usr/tests/sys/vm/stack/.debug/libsoxstack.so.debug
does not and was incorrectly being included in base.txz.

PR: 283214

Signed-off-by: Pat Maddox [email protected]

I feel like that's more of a sign the library is being installed to the wrong place and should be in /usr/tests/lib/sys/... or similar.

patmaddox · 2024-12-15T04:07:16Z

@jrtc27 you're right about the owner, fix in 526efd6

I'll see if there's a better spot for the test - but maybe someone more familiar with it knows off-hand?

@emaste @bsdimp I've submitted this single PR with a series of patches, but they're mostly independent. I didn't know if I should do one PR and let you cherry pick acceptable commits, or open separate PRs for each one. Let me know if I should split it up - or any general guidance you have, as this is my first time contributing to src.

patmaddox · 2024-12-15T07:55:03Z

on amd64

git checkout ac493dbe5ca707dba0ccc8c0f778e087f85ac32b
(as root) make -s -j$(sysctl -n hw.ncpu) -DWITH_REPRODUCIBLE_BUILD buildworld buildkernel && make -s -C release -j$(sysctl -n hw.ncpu) -DNOSRC -DNOPORTS -DWITH_REPRODUCIBLE_BUILD PKG_TIMESTAMP=1733729386 packagesystem

should produce kernel FreeBSD 15.0-CURRENT n274203-ac493dbe5ca7 GENERIC and the following MANIFEST:

base-dbg.txz    6adcb45f8e91fa84ae0888682df68407a495cf439483f0156cd052618d757493        1104    base_dbg        "Base system (Debugging)"       off
base.txz        ce100ab1671dc6bd754b92743e0bde96a39628a0b607955263c1cdb60f988735        29557   base    "Base system (MANDATORY)"       on
kernel-dbg.txz  60748997e8190fdeb230fa6e3dd9579c5fcf75cf515203d4c730ce4d9777436f        878     kernel_dbg      "Kernel (Debugging)"    on
kernel.txz      ef8d6e947d98f96c5caad22b7ca9d113cd63cec3e87f12649aa9c19eedbc6a4a        881     kernel  "Kernel (MANDATORY)"    on
lib32-dbg.txz   e7d18d478f38aa56c2550f9e26af6b9b1d972cda8b90a55fd78f7ea359ebaadf        243     lib32_dbg       "32-bit compatibility libraries (Debugging)"    off
lib32.txz       758203d29602a9aa37037850d8633ebf1b6ad93afdc0ba1bd8b048b9a781b30e        730     lib32   "32-bit compatibility libraries"        on
tests.txz       f1e3040026122424d58473f72a26f99f6777b16e5b4c1d6f639a44d31fd577ff        7350    tests   "Test suite"    off

patmaddox · 2024-12-15T16:04:31Z

Added certctl fix (thanks @jrtc27). Building and packaging as a non-privileged user reproduces the root build.

526efd6 with PKG_TIMESTAMP=1733729386 should produce FreeBSD 15.0-CURRENT n274204-526efd6c1d79 GENERIC with MANIFEST:

base-dbg.txz    6adcb45f8e91fa84ae0888682df68407a495cf439483f0156cd052618d757493        1104    base_dbg        "Base system (Debugging)"       off
base.txz        e3e2809ec6399a069cd25c7817c5a3d60d68f73acd4907f568db91d90fc8f6e2        29557   base    "Base system (MANDATORY)"       on
kernel-dbg.txz  9824cb629b5e991e171e08e01bf20bcebda9a9dd3c84e0e8fb9051bccfafe723        878     kernel_dbg      "Kernel (Debugging)"    on
kernel.txz      0453f33e8a14e7762ac0a20dd3fefff3d0ed058e4d0325cc3ef531ef572b02d1        881     kernel  "Kernel (MANDATORY)"    on
lib32-dbg.txz   e7d18d478f38aa56c2550f9e26af6b9b1d972cda8b90a55fd78f7ea359ebaadf        243     lib32_dbg       "32-bit compatibility libraries (Debugging)"    off
lib32.txz       758203d29602a9aa37037850d8633ebf1b6ad93afdc0ba1bd8b048b9a781b30e        730     lib32   "32-bit compatibility libraries"        on
tests.txz       f1e3040026122424d58473f72a26f99f6777b16e5b4c1d6f639a44d31fd577ff        7350    tests   "Test suite"    off

jrtc27 · 2024-12-15T17:27:32Z

Added certctl fix (thanks @jrtc27). Building and packaging as a non-privileged user reproduces the root build.

526efd6 with PKG_TIMESTAMP=1733729386 should produce FreeBSD 15.0-CURRENT n274204-526efd6c1d79 GENERIC with MANIFEST:

base-dbg.txz    6adcb45f8e91fa84ae0888682df68407a495cf439483f0156cd052618d757493        1104    base_dbg        "Base system (Debugging)"       off
base.txz        e3e2809ec6399a069cd25c7817c5a3d60d68f73acd4907f568db91d90fc8f6e2        29557   base    "Base system (MANDATORY)"       on
kernel-dbg.txz  9824cb629b5e991e171e08e01bf20bcebda9a9dd3c84e0e8fb9051bccfafe723        878     kernel_dbg      "Kernel (Debugging)"    on
kernel.txz      0453f33e8a14e7762ac0a20dd3fefff3d0ed058e4d0325cc3ef531ef572b02d1        881     kernel  "Kernel (MANDATORY)"    on
lib32-dbg.txz   e7d18d478f38aa56c2550f9e26af6b9b1d972cda8b90a55fd78f7ea359ebaadf        243     lib32_dbg       "32-bit compatibility libraries (Debugging)"    off
lib32.txz       758203d29602a9aa37037850d8633ebf1b6ad93afdc0ba1bd8b048b9a781b30e        730     lib32   "32-bit compatibility libraries"        on
tests.txz       f1e3040026122424d58473f72a26f99f6777b16e5b4c1d6f639a44d31fd577ff        7350    tests   "Test suite"    off

This will break building on Linux though as there's no wheel group there. This is what DB_FROM_SRC is for, certctl needs to pass -N /path/to/etc for wheel to have meaning.

patmaddox · 2024-12-15T18:11:05Z

This will break building on Linux though as there's no wheel group there. This is what DB_FROM_SRC is for, certctl needs to pass -N /path/to/etc for wheel to have meaning.

On FreeBSD, install -U simply writes the given values to the METALOG:

#!/bin/sh
set -eu

rm -rf METALOG foo bar installed

echo "this is foo" > foo
echo "this is bar" > bar

mkdir installed
install -U -M METALOG -o missing-user -g missing-group foo bar installed/
cat METALOG

This produces:

./installed/foo type=file uname=missing-user gname=missing-group mode=0755 size=12
./installed/bar type=file uname=missing-user gname=missing-group mode=0755 size=12

Are you saying that on Linux, install validates the user and group name before adding it to METALOG, so the above example would error?

patmaddox · 2024-12-15T18:30:43Z

Are you saying that on Linux, install validates the user and group name before adding it to METALOG, so the above example would error?

I just tested on debian, and it doesn't even have -U or -M.

So... check if install supports those flags / check if we're on FreeBSD, and only add them if it does? Because it already doesn't do the right thing on Linux (I assume). Thus the behavior would be improved on FreeBSD, and remain unchanged on Linux for the time being.

jrtc27 · 2024-12-15T18:32:21Z

This will break building on Linux though as there's no wheel group there. This is what DB_FROM_SRC is for, certctl needs to pass -N /path/to/etc for wheel to have meaning.

On FreeBSD, install -U simply writes the given values to the METALOG:
#!/bin/sh
set -eu

rm -rf METALOG foo bar installed

echo "this is foo" > foo
echo "this is bar" > bar

mkdir installed
install -U -M METALOG -o missing-user -g missing-group foo bar installed/
cat METALOG
This produces:
./installed/foo type=file uname=missing-user gname=missing-group mode=0755 size=12
./installed/bar type=file uname=missing-user gname=missing-group mode=0755 size=12
Are you saying that on Linux, install validates the user and group name before adding it to METALOG, so the above example would error?

Hm, it seems you're right that our install(1) (which will still be the one used on non-FreeBSD during the build) doesn't validate user/group names for -U. It's not documented though, and seems to be a bad idea to me, we should be validating them at install time, not at makefs time nor, in the case of distribution tarballs, untar, i.e. bsdinstall, time (since our tarballs use symbolic user/group names, and bsdtar doesn't get a -N equivalent yet packageworld works on Linux, so it can't possibly be validating it).

patmaddox · 2024-12-15T18:44:32Z

install(1) (which will still be the one used on non-FreeBSD during the build)

That's good to know - so build behavior is equivalent on both OSes.

we should be validating them at install time, not at makefs time nor, in the case of distribution tarballs, untar

Maybe... but I think there would need to be a flag to let you validate or not. It's plausible for a builder to configure a METALOG for users it doesn't have. In fact that's exactly something I'm going to do now that I know about install / metalog / tar combo. The builder tars up files with a METALOG assigning ownership to www, postgres, acme, etc - but the builder doesn't have any of those users defined locally. It also won't have access to a passwd db that defines them.

That may be a useful enhancement to install(1), but seems to me to be out of scope for this particular PR.

jrtc27 · 2024-12-15T18:52:16Z

install(1) (which will still be the one used on non-FreeBSD during the build)

That's good to know - so build behavior is equivalent on both OSes.

we should be validating them at install time, not at makefs time nor, in the case of distribution tarballs, untar

Maybe... but I think there would need to be a flag to let you validate or not. It's plausible for a builder to configure a METALOG for users it doesn't have. In fact that's exactly something I'm going to do now that I know about install / metalog / tar combo. The builder tars up files with a METALOG assigning ownership to www, postgres, acme, etc - but the builder doesn't have any of those users defined locally. It also won't have access to a passwd db that defines them.

That may be a useful enhancement to install(1), but seems to me to be out of scope for this particular PR.

My point is, throughout the build system, we, at least intend to, pass -N /path/to/etc to install(1), mtree(8) and makefs(8) whenever DB_FROM_SRC is set (in some cases even unconditionally), which is implied to NO_ROOT. So, just as we have INSTALLFLAGS+= -N ${.CURDIR}/etc in Makefile.inc1, we should ensure that certctl does the same during the build. It happens to not matter today, but that's an undocumented detail we're relying on, and is inconsistent with the rest of the build.

emaste · 2024-12-15T23:25:11Z

@emaste @bsdimp I've submitted this single PR with a series of patches, but they're mostly independent.

This is fine - I'm happy to grab individual commits out as I or others review them.

emaste · 2024-12-15T23:42:05Z

CC @brooksdavis and @kevans91 for certctl - this originated in 48e9fb8.

I think the patch in 526efd6 is a good step in the right direction, even if we should also have support for -N and fix PR283355.

emaste · 2024-12-15T23:47:45Z

Although that said I wonder if the -o and -u should be on each install invocation instead.

emaste · 2024-12-16T00:00:58Z

Makefile.inc1

-	@sed -e 's|^./kernel|.|' ${DESTDIR}/${DISTDIR}/kernel.premeta > \
-	    ${DESTDIR}/${DISTDIR}/kernel.meta
+	echo "#${MTREE_MAGIC}" > ${DESTDIR}/${DISTDIR}/kernel.meta
+	sed -e 's|^./kernel|.|' ${DESTDIR}/${DISTDIR}/kernel.premeta | \


does the unsortedness happen only with -j? I'd be a bit surprised by nondeterminism without -j.

I'm not sure - I've never had the patience to run without -j.

One potential source of non-determinism is when file lists are generated rather than declared. For example, some of the items returned by grep -R '=.*find' --include 'Makefile*' sys could potentially be non-determinstic. But I can't say for sure.

It looks like those are all in sys/contrib (and so not actually used in our build), but indeed anything that returns files in filesystem order could be an issue.

I expect -j will result in nondeterminism and so the sorting is necessary, mainly just wondering if this is the only source of nondeterminism here.

emaste · 2024-12-16T14:39:30Z

release/Makefile

-	    >> ${.OBJDIR}/${DISTDIR}/base.meta
+	sed -n 's,^\.,./var/db/etcupdate/current,p' \
+	    ${.OBJDIR}/${DISTDIR}/base/var/db/etcupdate/current/METALOG | \
+	    ${METALOG_SORT_CMD} >> ${.OBJDIR}/${DISTDIR}/base.meta


Wait, where is METALOG_SORT_CMD defined for relelase/* Makefiles?

oof... it is not.

And the reason this "worked" for removing the non-determinism is because echo "foo" | > file is valid shell, and results in empty file. So the etc entries aren't being sorted... they're just not being added at all.

fixed in fb482e3 and confirmed it packages the same on three systems

You still have to do what METALOG_SORT_CMD does or this won't be sound on systems with LC_COLLATE!=C.

Updated in 0d3b734

emaste · 2024-12-16T23:21:45Z

0d3b734 looks good except for the chicken and metalog files, will commit without those :)

patmaddox · 2024-12-17T01:16:47Z

0d3b734 looks good except for the chicken and metalog files, will commit without those :)

patmaddox · 2024-12-19T16:38:36Z

just as we have INSTALLFLAGS+= -N ${.CURDIR}/etc in Makefile.inc1

Updated in e482667

#1552 adds the actual validation. I've tested them together.

emaste · 2024-12-30T00:20:01Z

CC @allanjude @kevans91 if you'd like to look at the certctl change in e482667

allanjude

Reviewed by: allanjude

This sets the correct ownership values when building base.txz PR: 283340 Reviewed by: allanjude Pull request: #1550 Signed-off-by: Pat Maddox <[email protected]>

emaste · 2024-12-30T19:57:17Z

@allanjude did you review all of the changes or just certctl? (AFAIK there's no way to indicate review of a partial set of commits in a pull request.)

The metalog is produced by install -M, which is not sorted. This results in non-deterministic file ordering in kernel.txz. Order the files in kernel.txz to support reproducible builds. PR: 283214 Signed-off-by: Pat Maddox <[email protected]>

When using -DWITHOUT_REPRODUCIBLE_BUILD, files in *.txz will be added with their built timestamps. This results in non-deterministic tarballs. -DWITH_REPRODUCIBLE_BUILD sets the time in the METALOG, so that all files have the time defined by PKG_TIMESTAMP. kernel is added to its metalog via install -M, which does not support configuring the mtree keys. Rewrite the time for its metalog entry. PR: 283214 Signed-off-by: Pat Maddox <[email protected]>

distributeworld builds man pages using distribute, rather than install. Check for distribute, and wait to build etc. This ensures that makedb is called after the man pages have been distributed. PR: 283214 Signed-off-by: Pat Maddox <[email protected]>

PR: 283214 Signed-off-by: Pat Maddox <[email protected]>

emaste · 2025-01-06T15:08:23Z

Makefile.inc1

 	${_+_}cd ${KRNLOBJDIR}/${INSTALLKERNEL}; \
 	    ${IMAKEENV} ${IMAKE_INSTALL:S/METALOG/kernel.premeta/} \
 	    ${IMAKE_MTREE} PATH=${TMPPATH:Q} ${MAKE} KERNEL=${INSTKERNNAME} \
 	    DISTBASE=/kernel DESTDIR=${INSTALL_DDIR}/kernel \
 	    METALOG=${METALOG:S/METALOG/kernel.premeta/} \
 	    ${.TARGET:S/distributekernel/install/}
 .if defined(NO_ROOT)
-	@sed -e 's|^./kernel|.|' ${DESTDIR}/${DISTDIR}/kernel.premeta > \
-	    ${DESTDIR}/${DISTDIR}/kernel.meta
+	echo "#${MTREE_MAGIC}" > ${DESTDIR}/${DISTDIR}/kernel.meta


We want to keep the @s presumably

IMO the @s are bugs. These aren't steps with should be hiding from the caller.

emaste · 2025-01-06T15:57:11Z

Makefile.inc1

@@ -462,7 +462,7 @@ SUBDIR+=	${_DIR}
 # by calling 'makedb' in share/man.  This is only relevant for
 # install/distribute so they build the whatis file after every manpage is
 # installed.
-.if make(installworld) || make(install)
+.if make(installworld) || make(install) || make(distribute)


Should this be make(distributeworld)? I'm not sure, but I don't fully understand the distribute pseduo-target.

emaste · 2025-01-06T20:34:11Z

Sorting pushed in 58610d1.

I think build: Set METALOG time when using -DWITH_REPRODUCIBLE_BUILD is fine, although we ought to give install the ability to directly emit a reproducible time= entry. (I'd do that as a subsequent change.)

build: Wait to build mandoc.db when making distributeworld has an open question.

I've got some comments for the doc update in release: Document reproducible release tarballs but that change can wait until the others are solved anyway

emaste · 2025-01-06T20:35:45Z

share/man/man7/release.7

+Different builders can produce bit-identical release tarballs using
+.Va PKG_TIMESTAMP
+and
+.Va REVISION :


We probably want to differentiate PKG_TIMESTAMP from REVISION -- the former is an actual (literal) env variable used by Makefile*, but the latter is a placeholder in the example command below. I'm not sure off hand what the markup for that should be.

I wonder if we should have newvers.sh collect the commit time (e.g. from git show -s --format=%ct) and have the default SOURCE_DATE_EPOCH derived from that.

patmaddox force-pushed the reproducible-tarballs branch 2 times, most recently from 4c0cbd1 to 51dc538 Compare December 13, 2024 22:27

patmaddox changed the title ~~Reproducible tarballs~~ Reproducible release tarballs Dec 13, 2024

patmaddox force-pushed the reproducible-tarballs branch from 51dc538 to 05420f0 Compare December 13, 2024 23:20

brooksdavis reviewed Dec 13, 2024

View reviewed changes

Makefile.inc1 Outdated Show resolved Hide resolved

Makefile.inc1 Outdated Show resolved Hide resolved

Makefile.inc1 Outdated Show resolved Hide resolved

release/Makefile Outdated Show resolved Hide resolved

patmaddox force-pushed the reproducible-tarballs branch from 05420f0 to f7e2f46 Compare December 14, 2024 01:09

patmaddox requested a review from brooksdavis December 14, 2024 02:57

patmaddox force-pushed the reproducible-tarballs branch 2 times, most recently from 5542e70 to 3654c13 Compare December 14, 2024 20:15

patmaddox force-pushed the reproducible-tarballs branch from 3654c13 to c4b5544 Compare December 14, 2024 23:30

patmaddox force-pushed the reproducible-tarballs branch from c4b5544 to ac493db Compare December 15, 2024 07:26

emaste reviewed Dec 16, 2024

View reviewed changes

patmaddox force-pushed the reproducible-tarballs branch 2 times, most recently from 9da5a0f to b35bfb5 Compare December 16, 2024 21:54

patmaddox force-pushed the reproducible-tarballs branch from b35bfb5 to 3a06988 Compare December 17, 2024 00:58

patmaddox force-pushed the reproducible-tarballs branch from 3a06988 to e482667 Compare December 19, 2024 16:36

allanjude approved these changes Dec 30, 2024

View reviewed changes

freebsd-git pushed a commit that referenced this pull request Dec 30, 2024

certctl: Set METALOG ownership to root:wheel

10fa3f2

This sets the correct ownership values when building base.txz PR: 283340 Reviewed by: allanjude Pull request: #1550 Signed-off-by: Pat Maddox <[email protected]>

patmaddox added 4 commits December 30, 2024 12:34

release: Document reproducible release tarballs

6bc903d

PR: 283214 Signed-off-by: Pat Maddox <[email protected]>

patmaddox force-pushed the reproducible-tarballs branch from e482667 to 6bc903d Compare December 30, 2024 20:40

emaste reviewed Jan 6, 2025

View reviewed changes

Reproducible release tarballs #1550

Are you sure you want to change the base?

Reproducible release tarballs #1550

Conversation

patmaddox commented Dec 13, 2024 • edited Loading

brooksdavis left a comment

Choose a reason for hiding this comment

jrtc27 commented Dec 13, 2024

jrtc27 commented Dec 14, 2024

jrtc27 commented Dec 14, 2024

patmaddox commented Dec 15, 2024 • edited Loading

patmaddox commented Dec 15, 2024

patmaddox commented Dec 15, 2024

jrtc27 commented Dec 15, 2024

patmaddox commented Dec 15, 2024

patmaddox commented Dec 15, 2024

jrtc27 commented Dec 15, 2024

patmaddox commented Dec 15, 2024

jrtc27 commented Dec 15, 2024

emaste commented Dec 15, 2024

emaste commented Dec 15, 2024

emaste commented Dec 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patmaddox Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emaste commented Dec 16, 2024

patmaddox commented Dec 17, 2024

patmaddox commented Dec 19, 2024

emaste commented Dec 30, 2024

allanjude left a comment

Choose a reason for hiding this comment

emaste commented Dec 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emaste commented Jan 6, 2025

Choose a reason for hiding this comment

patmaddox commented Dec 13, 2024 •

edited

Loading

patmaddox commented Dec 15, 2024 •

edited

Loading

patmaddox Dec 16, 2024 •

edited

Loading