diff mbox series

sstate.bbclass: Only sign packages at the time of their creation

Message ID 20231222060932.1504845-2-tobias.hagelborn@axis.com
State New
Headers show
Series sstate.bbclass: Only sign packages at the time of their creation | expand

Commit Message

Tobias Hagelborn Dec. 22, 2023, 6:09 a.m. UTC
From: Tobias Hagelborn <tobiasha@axis.com>

The purpose of the change is to never sign a package not created by
the build itself.

sstate_create_package is refactored into Python and re-designed
to handle signing inside the function. Thus, the signing should never
apply to existing sstate packages. The function is therefore renamed
into sstate_create_and_sign_package.
The creation of the archive remains in a separate shellscript function.

Co-authored-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
Signed-off-by: Tobias Hagelborn <tobias.hagelborn@axis.com>
---
 meta/classes-global/sstate.bbclass | 128 ++++++++++++++++++++---------
 1 file changed, 87 insertions(+), 41 deletions(-)

Comments

Richard Purdie Dec. 22, 2023, 9:32 a.m. UTC | #1
On Fri, 2023-12-22 at 07:09 +0100, Tobias Hagelborn wrote:
> From: Tobias Hagelborn <tobiasha@axis.com>
> 
> The purpose of the change is to never sign a package not created by
> the build itself.
> 
> sstate_create_package is refactored into Python and re-designed
> to handle signing inside the function. Thus, the signing should never
> apply to existing sstate packages. The function is therefore renamed
> into sstate_create_and_sign_package.
> The creation of the archive remains in a separate shellscript function.
> 
> Co-authored-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
> Signed-off-by: Tobias Hagelborn <tobias.hagelborn@axis.com>
> ---
>  meta/classes-global/sstate.bbclass | 128 ++++++++++++++++++++---------
>  1 file changed, 87 insertions(+), 41 deletions(-)

This code is quite critical so review on any change here needs to be
very careful and that is why I might seem to be a little hard on this
patch. The fact this completely rewrites the critical section worries
me a lot and means it isn't an easy to review change.

We tried to keep this code simple deliberately due to the number of
problems we've had with it in the past. You have to keep in mind it is
one of the few shared directories we have where other builds can be
reading/writing the files at the same time as us without locks.

The fact that you have the force mode of operation is setting off alarm
bells since we have seen build failures where an existing file is
changed half way though a build.

Another example of how this could re-introduce issues is that it looks
like it will make it easy to break:

https://git.yoctoproject.org/poky/commit/?id=47cc6288280bd38f42851d6423a5ffc95a46eea4

again. The code:

+    if not sstate_pkg.parent.is_dir():
+        sstate_pkg.parent.mkdir(parents=True, exist_ok=True)

will happen to work as the previous code should have already created
the directories correctly so this line will do nothing. The fact it is
there will make it all too easy for someone to break this in future
though.

When I added that I deliberately didn't rewrite other bits in python
because I knew what the race issues were like.

The directory creation and then moving files from the directory is also
a concern as it is potentially going to be much slower. If you have
something like the sstate on the autobuilder where the directory is
NFS, creating temporary directories and moving files in/out of them is
expensive compared to just renaming files.

Another race that the force copy of the sig as written isn't as safe as
you might think as it can still race with someone else copying the file
at the same time. You probably need to move the file into place, then
check that the resulting file is the one you placed there and if it is,
only then move the sig in.

I'm also not thrilled at the need for copies of the datastore. From a
style standpoint, we haven't really used Path() in OE/bitbake so that
is a bit out of place but I guess people are going to do this
inevitably. The variable naming "verify_sig" isn't really true /clear
when you're actually signing it either.

Cheers,

Richard
diff mbox series

Patch

diff --git a/meta/classes-global/sstate.bbclass b/meta/classes-global/sstate.bbclass
index 9330433bb2..bc7bdbe7d5 100644
--- a/meta/classes-global/sstate.bbclass
+++ b/meta/classes-global/sstate.bbclass
@@ -703,9 +703,7 @@  def sstate_package(ss, d):
     if d.getVar('SSTATE_SKIP_CREATION') == '1':
         return
 
-    sstate_create_package = ['sstate_report_unihash', 'sstate_create_pkgdirs', 'sstate_create_package']
-    if d.getVar('SSTATE_SIG_KEY'):
-        sstate_create_package.append('sstate_sign_package')
+    sstate_create_package = ['sstate_report_unihash', 'sstate_create_pkgdirs', 'sstate_create_and_sign_package']
 
     for f in (d.getVar('SSTATECREATEFUNCS') or '').split() + \
              sstate_create_package + \
@@ -817,19 +815,91 @@  python sstate_create_pkgdirs () {
         bb.utils.mkdirhier(os.path.dirname(d.getVar('SSTATE_PKG')))
 }
 
-#
-# Shell function to generate a sstate package from a directory
-# set as SSTATE_BUILDDIR. Will be run from within SSTATE_BUILDDIR.
-#
-sstate_create_package () {
-	# Exit early if it already exists
-	if [ -e ${SSTATE_PKG} ]; then
-		touch ${SSTATE_PKG} 2>/dev/null || true
-		return
-	fi
+# Create sstate package
+# If enabled, sign the package.
+# Package and signature are created in a sub-directory
+# and renamed in place once created.
+python sstate_create_and_sign_package () {
+    from pathlib import Path
+
+    # Best effort touch
+    def touch(file):
+        try:
+            file.touch()
+        except:
+            pass
+
+    def update_file(src, dst, force=False):
+        if dst.is_symlink() and not dst.exists():
+            force=True
+        try:
+            # This relies on that src is a temporary file that can be renamed
+            # or left as is.
+            if force:
+                src.rename(dst)
+            else:
+                os.link(src, dst)
+            return True
+        except:
+            pass
+
+        if dst.exists():
+            touch(dst)
+
+        return False
 
-	TFILE=`mktemp ${SSTATE_PKG}.XXXXXXXX`
+    verify_sig = (
+        bb.utils.to_boolean(d.getVar("SSTATE_VERIFY_SIG")) and
+        bool(d.getVar("SSTATE_SIG_KEY"))
+    )
 
+    sstate_pkg = Path(d.getVar("SSTATE_PKG"))
+    sstate_pkg_sig = Path(str(sstate_pkg) + ".sig")
+    if verify_sig:
+        if sstate_pkg.exists() and sstate_pkg_sig.exists():
+            touch(sstate_pkg)
+            touch(sstate_pkg_sig)
+            return
+    else:
+        if sstate_pkg.exists():
+            touch(sstate_pkg)
+            return
+
+    from tempfile import TemporaryDirectory
+    if not sstate_pkg.parent.is_dir():
+        sstate_pkg.parent.mkdir(parents=True, exist_ok=True)
+
+    with TemporaryDirectory(dir=sstate_pkg.parent) as tmp_dir:
+        tmp_pkg = Path(tmp_dir) / sstate_pkg.name
+        localdata = d.createCopy()
+        localdata.setVar("SSTATE_PKG", str(tmp_pkg))
+        bb.build.exec_func('sstate_archive_package', localdata)
+
+        force = False
+        if verify_sig:
+            from oe.gpg_sign import get_signer
+            signer = get_signer(d, 'local')
+            signer.detach_sign(str(tmp_pkg), d.getVar('SSTATE_SIG_KEY'), None,
+                                d.getVar('SSTATE_SIG_PASSPHRASE'), armor=False)
+
+            tmp_pkg_sig = Path(tmp_dir) / sstate_pkg_sig.name
+            if not update_file(tmp_pkg_sig, sstate_pkg_sig):
+                # If the created signature file could not be copied into place,
+                # then we should not use the sstate package either.
+                return
+
+            # If the .sig file was updated, then the sstate package must also
+            # be updated.
+            force = True
+
+        update_file(tmp_pkg, sstate_pkg, force)
+}
+
+# Shell function to generate a sstate package from a directory
+# set as SSTATE_BUILDDIR. Will be run from within SSTATE_BUILDDIR.
+# The calling function handles moving the sstate package into the final
+# destination.
+sstate_archive_package () {
 	OPT="-cS"
 	ZSTD="zstd -${SSTATE_ZSTD_CLEVEL} -T${ZSTD_THREADS}"
 	# Use pzstd if available
@@ -840,42 +910,18 @@  sstate_create_package () {
 	# Need to handle empty directories
 	if [ "$(ls -A)" ]; then
 		set +e
-		tar -I "$ZSTD" $OPT -f $TFILE *
+		tar -I "$ZSTD" $OPT -f ${SSTATE_PKG} *
 		ret=$?
 		if [ $ret -ne 0 ] && [ $ret -ne 1 ]; then
 			exit 1
 		fi
 		set -e
 	else
-		tar -I "$ZSTD" $OPT --file=$TFILE --files-from=/dev/null
-	fi
-	chmod 0664 $TFILE
-	# Skip if it was already created by some other process
-	if [ -h ${SSTATE_PKG} ] && [ ! -e ${SSTATE_PKG} ]; then
-		# There is a symbolic link, but it links to nothing.
-		# Forcefully replace it with the new file.
-		ln -f $TFILE ${SSTATE_PKG} || true
-	elif [ ! -e ${SSTATE_PKG} ]; then
-		# Move into place using ln to attempt an atomic op.
-		# Abort if it already exists
-		ln $TFILE ${SSTATE_PKG} || true
-	else
-		touch ${SSTATE_PKG} 2>/dev/null || true
+		tar -I "$ZSTD" $OPT --file=${SSTATE_PKG} --files-from=/dev/null
 	fi
-	rm $TFILE
+	chmod 0664 ${SSTATE_PKG}
 }
 
-python sstate_sign_package () {
-    from oe.gpg_sign import get_signer
-
-
-    signer = get_signer(d, 'local')
-    sstate_pkg = d.getVar('SSTATE_PKG')
-    if os.path.exists(sstate_pkg + '.sig'):
-        os.unlink(sstate_pkg + '.sig')
-    signer.detach_sign(sstate_pkg, d.getVar('SSTATE_SIG_KEY', False), None,
-                       d.getVar('SSTATE_SIG_PASSPHRASE'), armor=False)
-}
 
 python sstate_report_unihash() {
     report_unihash = getattr(bb.parse.siggen, 'report_unihash', None)