ostree-prepare-root: Validate ed25519 signatures when requested

[alexlarsson]

NOTE: This is WIP, I have not yet tried the boot part of this.

This is a continuation of #2891 which dropped the use of fs-verity built-in signatures in favour of user-space signatures (as discussed in containers/composefs#151).

The new approach is to the the more modern ed25519 detached signatures. We pass in a (typically one-use throwaway) secret key to commit, which is used to sign the composefs image digest. This is then put in the ostree metadata. During deploy this is extracted and stored next to the regenerated image. During boot, the initrd (ostree-prepare-root) loads the public key from a fixed place in the initrd, exctracs the digest of the composefs files and validates it according to the signature.

This way we can have a single-use public key that ties the initrd (which is validated with secureboot) and the ostree commit /usr part together (so you can't mix and match).

Note that the CLI for passing the key to commit uses the path to the key file rather than the passing the secret key in directly (as --sign does), because that seems very risky, and will likely end up in some logfile by accident.

[alexlarsson]

Feedback from @cgwalters: We can just read the signatures from the commit object, there is no need for the separate signature file. Especially now that prepare-root links to glib and can easily use gvariant.

[cgwalters]

Right. Don't get me wrong, I know the existing GPG signature stuff is heavyweight. And AFAIK some users just want to avoid GPL3 entirely that motivated the ed25519 signatures. But...hopefully the existing ed25519 stuff is light enough for those who want that. For CoreOS today our initramfs is so huge already that adding gpg probably wouldn't be too bad...though I haven't evaluated it.

As you mentioned before we have the libcurl/libsoup dependencies (and libcurl is transitively big) but we could likely compile just the signature stuff into a new static library linked into ostree-prepare-root.

[alexlarsson]

I don't necessarily disagree with gpg use due to the code size alone, but rather due to the complexity of its key/crypto mechanisms and ecosystem. And it really is a total pain in the butt to work with. ed25519 is just super simple, doing one thing and just one thing, which is the thing we want.

Also, again, I'd like to emphasize that the signature of the commit itself (used during pull, and is the same for all releases from that software published, with the private key hidden deep in some signing hardware) and the signature of the composefs image (new one generated each build, never reused, with secure key easy and cheap to generate/use and then delete) are very different things.

[cgwalters]

On Thu, Jul 6, 2023, at 5:49 PM, Alexander Larsson wrote: I don't necessarily disagree with gpg use due to the code size alone, but rather due to the complexity of its key/crypto mechanisms and ecosystem. And it really is a total pain in the but to work with. ed25519 is just super simple, doing one thing and just one thing, which is the thing we want.

Agreed on this.

Also, again, I'd like to emphasize that the signature of the commit itself (used during pull, and is the same for all releases from that software published, with the private key hidden deep in some signing hardware) and the signature of the composefs image (new one generated each build, never reused, with secure key easy and cheap to generate/use and then delete) are very different things.

Hmm. Ok interesting. So you are seeing this key as simply a way to authenticate a chain from exactly one initramfs/uki to exactly one rootfs, right? It’s something we could do without asymmetric crypto at all, ie we could just use a sha256 hash, except for the circular dependency of having the uki/initramfs shipped in the root file system tree and ostree commit. But…that’s solvable in theory by splitting out the kernel/initramfs into something shipped as a distinct unit but lifecycled together. Hmm. Actually, couldn’t we just omit /usr/lib/modules/$ver/vmlinuz from the composefs digest computation? And also we don’t see it at runtime too. Then in a UKI scenario, we can embed the desired rootfs composefs digest in it. Note a consequence of “strict root and uki binding” (whether implemented via asymmetric crypto or a digest) is that updating the rootfs requires updating the kernel image too. Whereas reusing the pull signatures (or I guess more generally signing the composefs digest with a key reused across builds) allows us to avoid changing the kernel binary for pure userspace changes. This seems like a somewhat desirable property, right?

[alexlarsson]

Yes. I would like to tie initrd to ostree commit tightly. It could be a security problem if you can mix and match kernels and userspace. For example, you could boot a new signed kernel with and old unsecure userspace.

I agree that if we ignore the circular dependency we wouldn't have to use a public key. But we can't just ignore that. The reason for the public key is that we can pre-generate it and store the in the initrd without cycles. I've been trying to avoid this in various ways, but I think it is really the easiest and best way.

In theory, if strip out the initrd file when we create the composefs image, we could then embed a hash in it and avoid the cycle, but computing such an initrd sound like a gigantic hairball. You have to have the complete commit ready and then go back and tweak the initrd.

As for this tying the initrd/kernel with the userspace. Yes, that is a problem (and it also happens if we do the above digest approach). It is a slight cost, yes, but in practice I think it is a cost you must bear if you want a secureboot approach. Allowing users to mix and match between kernels and userspace is just not secure enough.

[cgwalters]

For example, you could boot a new signed kernel with and old unsecure userspace.

Yes, though there's many things one could do here; e.g. we still include the kernel version in the ostree commit metadata, so we can verify before entering that we're booted with the kernel we expect.

Also in practice many generic use cases have important loadable modules, and that will also restrict things to userspace trees with exact matching kernel versions.

Hmm...I'm tempted to add the kver validation just on general principle.

In theory, if strip out the initrd file when we create the composefs image, we could then embed a hash in it and avoid the cycle, but computing such an initrd sound like a gigantic hairball. You have to have the complete commit ready and then go back and tweak the initrd.

I don't see what's so bad about that; an important thing here is to s/commit/filesystem tree/ - the ostree commit is really just metadata that we generate at the end. For the split initramfs case because the Linux kernel will accept a chain of cpio blobs it's just something like echo $composefs_digest > tmproot/etc/ostree/prepare-root-composefs-digest && find tmproot | cpio -Hnewc | gzip >> root/usr/lib/modules/*/initramfs.img. (The UKI case just needs to delay synthesizing the UKI until the end).

Anyways backing up a bit...I'm not opposed to these patches and I think with a bit of cleanup it could make sense as an option. But I don't think it's the only viable option at all, so perhaps we call this "transient composefs digest signatures" or so?

[cgwalters]

One goal I had in mind with re-using the pull signatures is that I think this is the model we'll need to take for integrating with the OCI ecosystem anyways in general (so this relates more to containers/composefs#151 ) - basically we inject the computed composefs digest into the manifest, and then it can be covered by sigstore/cosign. This is just very clear and easy to explain and test and debug. Which is also true of ostree using the pull signatures, however in the ostree case we do have the more special nature of the kernel/initramfs dance and chaining from SB.

[cgwalters]

In looking at this quckly; omitting the kernel/initramfs from ostree+composefs would need to be done in both ostree_repo_commit_add_composefs_metadata and in ostree_repo_checkout_composefs.

[alexlarsson]

In looking at this quckly; omitting the kernel/initramfs from ostree+composefs would need to be done in both ostree_repo_commit_add_composefs_metadata and in ostree_repo_checkout_composefs.

That is the easy part though. The hard part is that during ostree commit (or as a step just before it) you have to:

• take the root OstreeRepoFile and compute the digest of a filtered composefs image
• insert the computed digest into the initrd from the OstreeRepoFile
• combine the new initrd with the kernel into a new UKI
• sign the new UKI with the private key (which is offboard in some custom signing hardware that may need manual work to run)
• re-inject the UKI and initrd into a new root OstreeRepoFile (ensuring that the filtered composefs digest is 100% unaltered.
• Use the new OstreeRepoFile for the commit instead of the old one.

This stuff is hard, as it is very specific to how the UKI and initrd looks, how it is buillt and how it is signed. Especially the uefi or aboot UKI signing is very complex, due to the security apparatus around it.

All of this goes away if we use a per-build keypair, and it also fixes the security issues around kernel/userspace mix-and-match. If that is not fixed like that we have to add a lot of custom code around signatures so that we can handle revoking, burning hardware fuses for old keys, etc.

I'm pretty sure this is unworkable.

[alexlarsson]

What I think makes most sense is to:

• During build, create transient key-pair
• During build, insert transient pubkey into initrd
• During commit, sign commit with both the long-term key (be it gpg or ed25519) and the transient seckey
• In ostree-prepare-root, read the commit and detached metadata, validate commit against the transient pubkey in the initrd
• Extract composefs digest from commit

[alexlarsson]

Updated to new approach, which is much smaller. Will start doing some testing.

[alexlarsson]

This new version has the commits from #2922 as well

[cgwalters]

That is the easy part though. The hard part is that during ostree commit (or as a step just before it) you have to:

• take the root OstreeRepoFile and compute the digest of a filtered composefs image

We already landed a handy API for that.

• insert the computed digest into the initrd from the OstreeRepoFile

Only into the on-disk version; there is no initrd in the OstreeRepoFile. (Sorry I was wrong. I think basically in my proposal we need to just not use OstreeRepoFile to start)

• combine the new initrd with the kernel into a new UKI
• sign the new UKI with the private key (which is offboard in some custom signing hardware that may need manual work to run)

Sure, but in this proposed "transient linkage key" flow, these steps are still required, right? It's just that you can do them asynchronously/decoupled.

• re-inject the UKI and initrd into a new root OstreeRepoFile (ensuring that the filtered composefs digest is 100% unaltered.
• Use the new OstreeRepoFile for the commit instead of the old one.

I don't think there's any "re-inject". We don't need an OstreeRepoFile until the very end.

The flow can be:

• Compute on-disk userspace filesystem tree (this may or may not involve ostree-style hardlinks and the devino cache, containerization and how that stuff intersects with composefs is a whole other really interesting sub-topic)
• There is no initramfs yet
• Compute composefs digest (and in fact, this could use mkcomposefs --print-digest-only)
• build UKI (possibly via remote service; a huge interesting topic here is whether the userspace tree for that UKI comes from the exact same versions of things or whether it's distinct) - but either way the composefs digest must be passed
• UKI is returned and injected into /usr/lib/modules/$kver/vmlinuz
• ostree commit is generated
• ostree commit is signed as usual with "pull key" which covers UKI too

I'm pretty sure [injecting via digest] is unworkable.

Assuming I'm correct in replacing "this" with "[injecting via digest]" here - I don't agree it's unworkable. I'm tempted to Just Do It in rpm-ostree by default. (Though practically speaking we need to enable composefs support in Fedora at least by default...I can't think of a big reason not to?)

I do agree that "transient linkage key" has some stronger semantics and some potential build process advantages.

During build, create transient key-pair

But I want to dig into this more...you're arguing that "transient linkage key" is easier for builders who may have special processes around the UKI - but the transient key still must be injected and built into it. Let's assume there's a distinct "build server/service" for this that operates over RPC. Kind of like how Fedora's robosignatory works.

During build, insert transient pubkey into initrd

You're arguing there's two "build" processes right? Let's call them "build-uki" and "build-userspace". This then is "build-uki", right?

So the output of build-uki actually returns two things:

• vmlinuz binary
• private key half of the "linkage key"

During commit, sign commit with both the long-term key (be it gpg or ed25519) and the transient seckey

OK, sure.

In ostree-prepare-root, read the commit and detached metadata, validate commit against the transient pubkey in the initrd

Yes, OK. (Though ISTM it would perhaps make sense to validate both keys? Just...on general principle?)

Extract composefs digest from commit

(And pass it to mount.composefs.) Yep.

[cgwalters]

OK cool I like the direction here. I have some refactoring requests which I think will make longer term maintenance easier. I'd be happy to try doing them if you prefer!

[cgwalters]

I can see some cases where we want to revalidate the "pull signature" on the commit before mounting. So in theory this could be a separate option; perhaps something like ostree config set sysroot.verify-signatures-before-mount true or so?

But we'd forcibly require that to happen in the composefs-signed case.

[cgwalters]

I think I'd like to do this still. But I also think we can probably just merge this and do followups.

[alexlarsson]

OK cool I like the direction here. I have some refactoring requests which I think will make longer term maintenance easier. I'd be happy to try doing them if you prefer!

Feel free to try it, as I'm on my way out. Otherwise I'll continue with that on monday.

[alexlarsson]

Ok, this discussion is becoming a bit long and hard to follow. I'll do some thread-sniping below, but overall I guess I come from a slightly different perspective, where I'm working both with an existing osbuild-based workflow (not fcos), as well as an additional different UKI format (android boot, not UEFI). I think that colors my perception of this a bit.

• take the root OstreeRepoFile and compute the digest of a filtered composefs image

We already landed a handy API for that.

What do you mean here?

• insert the computed digest into the initrd from the OstreeRepoFile

Only into the on-disk version; there is no initrd in the OstreeRepoFile. (Sorry I was wrong. I think basically in my proposal we need to just not use OstreeRepoFile to start)

• combine the new initrd with the kernel into a new UKI
• sign the new UKI with the private key (which is offboard in some custom signing hardware that may need manual work to run)

Sure, but in this proposed "transient linkage key" flow, these steps are still required, right? It's just that you can do them asynchronously/decoupled.

Yes, independent of anything else, we do need a way to allow somewhere during the build to essentially call out to some flexible signature mechanism. However, I would like that to be as independent from ostree as possible, so it is easy to integrate with the build, both when building ostree and for non-ostree images.

• re-inject the UKI and initrd into a new root OstreeRepoFile (ensuring that the filtered composefs digest is 100% unaltered.
• Use the new OstreeRepoFile for the commit instead of the old one.

I don't think there's any "re-inject". We don't need an OstreeRepoFile until the very end.

The flow can be:

* Compute on-disk userspace filesystem tree (this may or may not involve ostree-style hardlinks and the devino cache, containerization and how _that_ stuff intersects with composefs is a whole other really interesting sub-topic)

* There is no initramfs yet

* Compute composefs digest (and in fact, this could use `mkcomposefs --print-digest-only`)

Who would compute this digest? Its kinda tricky as it has to produce exactly the same metadata as the later commit. For example, you can't just run it on the on-disk userspace filesystem tree without a deep knowledge of how that would be converted to ostree metadata (canonicalization, timestamps, commit filter options, etc).

* build UKI (possibly via remote service; a huge interesting topic here is whether the userspace tree for that UKI comes from the exact same versions of things or whether it's distinct) - but either way the composefs digest must be passed

* UKI is returned and injected into `/usr/lib/modules/$kver/vmlinuz`

* ostree commit is generated

* ostree commit is signed as usual with "pull key" which covers UKI too

I'm pretty sure [injecting via digest] is unworkable.

Assuming I'm correct in replacing "this" with "[injecting via digest]" here - I don't agree it's unworkable. I'm tempted to Just Do It in rpm-ostree by default. (Though practically speaking we need to enable composefs support in Fedora at least by default...I can't think of a big reason not to?)

I do agree that "transient linkage key" has some stronger semantics and some potential build process advantages.

During build, create transient key-pair

But I want to dig into this more...you're arguing that "transient linkage key" is easier for builders who may have special processes around the UKI - but the transient key still must be injected and built into it. Let's assume there's a distinct "build server/service" for this that operates over RPC. Kind of like how Fedora's robosignatory works.

There is no need for the "transient" key to have the same special processes around it. It can be created locally as a regular file on the machine during the build/compose, and then deleted at the end. This is very similar to how the kernel rpm build generates a throwaway key to sign the kernel modules, and then builds the public part into the kernel image itself.

During build, insert transient pubkey into initrd

You're arguing there's two "build" processes right? Let's call them "build-uki" and "build-userspace". This then is "build-uki", right?

So the output of build-uki actually returns two things:

* vmlinuz binary

* private key half of the "linkage key"

I imagine the setup to be ordered differently. The key isn't really generated by build-uki, but already exists when it is invoked.

Basically, before you even do any userspace or initrd building you generate a key-pair. Then you put the public key in your userspace rootfs (say as /etc/boot.key. Then you pass --install /etc/boot.key to dracut when generating the initrd and pass --sign=$privkey when you commit the result.

[cgwalters]

Its kinda tricky as it has to produce exactly the same metadata as the later commit. For example, you can't just run it on the on-disk userspace filesystem tree without a deep knowledge of how that would be converted to ostree metadata (canonicalization, timestamps, commit filter options, etc).

Yeah, definitely true. Of these, I think SELinux labeling is the hardest; ostree wants to own this. But...we could move it into mkcomposefs too...

[cgwalters]

@alexlarsson OK only compile tested so far...but I think this will work, WDYT?

From 89170f1b01d4599995e35d35496d45d98365d877 Mon Sep 17 00:00:00 2001
From: Colin Walters <[email protected]>
Date: Fri, 7 Jul 2023 16:31:58 -0400
Subject: [PATCH] wip: factor out a libotcore

This will contain logic shared between ostree-prepare-root
and libostree-1.so.  It will just link to libgio.so, so as
to avoid pulling in e.g. libcurl and other things.
---
 Makefile-libostree.am                 |   4 +-
 Makefile-otcore.am                    |  28 +++++++
 Makefile-switchroot.am                |  14 +---
 Makefile.am                           |   1 +
 src/libostree/ostree-sign-ed25519.c   |  58 +++-----------
 src/libotcore/README.md               |   1 +
 src/libotcore/otcore-ed25519-verify.c | 109 ++++++++++++++++++++++++++
 src/libotcore/otcore.h                |  38 +++++++++
 src/switchroot/ostree-prepare-root.c  |  62 +++++----------
 9 files changed, 211 insertions(+), 104 deletions(-)
 create mode 100644 Makefile-otcore.am
 create mode 100644 src/libotcore/README.md
 create mode 100644 src/libotcore/otcore-ed25519-verify.c
 create mode 100644 src/libotcore/otcore.h

diff --git a/Makefile-libostree.am b/Makefile-libostree.am
index 8edd7f4d..47eb3055 100644
--- a/Makefile-libostree.am
+++ b/Makefile-libostree.am
@@ -185,12 +185,12 @@ EXTRA_DIST += \
 	$(top_srcdir)/src/libostree/libostree-released.sym \
 	$(NULL)
 
-libostree_1_la_CFLAGS = $(AM_CFLAGS) -I$(srcdir)/bsdiff -I$(srcdir)/libglnx -I$(srcdir)/composefs -I$(srcdir)/src/libotutil -I$(srcdir)/src/libostree -I$(builddir)/src/libostree \
+libostree_1_la_CFLAGS = $(AM_CFLAGS) -I$(srcdir)/bsdiff -I$(srcdir)/libglnx -I$(srcdir)/composefs -I$(srcdir)/src/libotutil -I$(srcdir)/src/libotcore -I$(srcdir)/src/libostree -I$(builddir)/src/libostree \
 	$(OT_INTERNAL_GIO_UNIX_CFLAGS) $(OT_INTERNAL_GPGME_CFLAGS) $(OT_DEP_LZMA_CFLAGS) $(OT_DEP_ZLIB_CFLAGS) $(OT_DEP_CRYPTO_CFLAGS) \
 	-fvisibility=hidden '-D_OSTREE_PUBLIC=__attribute__((visibility("default"))) extern' \
 	-DPKGLIBEXECDIR=\"$(pkglibexecdir)\"
 libostree_1_la_LDFLAGS = -version-number 1:0:0 -Bsymbolic-functions $(addprefix $(wl_versionscript_arg),$(symbol_files))
-libostree_1_la_LIBADD = libotutil.la libglnx.la libbsdiff.la $(OT_INTERNAL_GIO_UNIX_LIBS) $(OT_INTERNAL_GPGME_LIBS) \
+libostree_1_la_LIBADD = libotutil.la libotcore.la libglnx.la libbsdiff.la $(OT_INTERNAL_GIO_UNIX_LIBS) $(OT_INTERNAL_GPGME_LIBS) \
                         $(OT_DEP_LZMA_LIBS) $(OT_DEP_ZLIB_LIBS) $(OT_DEP_CRYPTO_LIBS)
 # Some change between rust-1.21.0-1.fc27 and rust-1.22.1-1.fc27.x86_64
 libostree_1_la_LIBADD += $(bupsplitpath)
diff --git a/Makefile-otcore.am b/Makefile-otcore.am
new file mode 100644
index 00000000..4675f261
--- /dev/null
+++ b/Makefile-otcore.am
@@ -0,0 +1,28 @@
+# Makefile for C source code
+#
+# Copyright (C) 2023 Colin Walters <[email protected]>
+#
+# SPDX-License-Identifier: LGPL-2.0+
+#
+# This library is free software; you can redistribute it and/or
+# modify it under the terms of the GNU Lesser General Public
+# License as published by the Free Software Foundation; either
+# version 2 of the License, or (at your option) any later version.
+#
+# This library is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this library. If not, see <https://www.gnu.org/licenses/>.
+
+noinst_LTLIBRARIES += libotcore.la
+
+libotcore_la_SOURCES = \
+	src/libotcore/otcore.h \
+    src/libotcore/otcore-ed25519-verify.c \
+	$(NULL)
+
+libotcore_la_CFLAGS = $(AM_CFLAGS) -I$(srcdir)/libglnx -I$(srcdir)/src/libotutil -DLOCALEDIR=\"$(datadir)/locale\" $(OT_INTERNAL_GIO_UNIX_CFLAGS) $(OT_INTERNAL_GPGME_CFLAGS) $(OT_DEP_CRYPTO_LIBS) $(LIBSYSTEMD_CFLAGS)
+libotcore_la_LIBADD = $(OT_INTERNAL_GIO_UNIX_LIBS) $(OT_INTERNAL_GPGME_LIBS) $(LIBSYSTEMD_LIBS) $(OT_DEP_CRYPTO_LIBS)
\ No newline at end of file
diff --git a/Makefile-switchroot.am b/Makefile-switchroot.am
index 9833f7e0..75005c5e 100644
--- a/Makefile-switchroot.am
+++ b/Makefile-switchroot.am
@@ -49,20 +49,10 @@ ostree-prepare-root : $(ostree_prepare_root_SOURCES)
 CLEANFILES += ostree-prepare-root
 else
 ostree_boot_PROGRAMS += ostree-prepare-root
-ostree_prepare_root_CFLAGS += $(AM_CFLAGS) -Isrc/switchroot -I$(srcdir)/composefs -I$(srcdir)/src/libostree -I$(srcdir)/src/libotutil
+ostree_prepare_root_CFLAGS += $(AM_CFLAGS) -Isrc/switchroot -I$(srcdir)/composefs -I$(srcdir)/src/libostree -I$(srcdir)/src/libotutil -I$(srcdir)/src/libotcore
 ostree_prepare_root_SOURCES += src/switchroot/ostree-prepare-root.c
 ostree_prepare_root_CPPFLAGS += $(OT_INTERNAL_GIO_UNIX_CFLAGS) -I $(srcdir)/libglnx
-ostree_prepare_root_LDADD += $(AM_LDFLAGS) $(OT_INTERNAL_GIO_UNIX_LIBS) libotutil.la libglnx.la
-if USE_LIBSODIUM
-ostree_prepare_root_CFLAGS += $(OT_DEP_LIBSODIUM_CFLAGS)
-ostree_prepare_root_LDADD += $(OT_DEP_LIBSODIUM_LIBS)
-else
-if USE_OPENSSL
-ostree_prepare_root_CFLAGS += $(OT_DEP_CRYPTO_CFLAGS)
-ostree_prepare_root_LDADD += $(OT_DEP_CRYPTO_LIBS)
-endif # USE_OPENSSL
-endif # USE_LIBSODIUM
-
+ostree_prepare_root_LDADD += $(AM_LDFLAGS) $(OT_INTERNAL_GIO_UNIX_LIBS) libotutil.la libotcore.la libglnx.la
 endif # BUILDOPT_USE_STATIC_COMPILER
 
 
diff --git a/Makefile.am b/Makefile.am
index ca7dec9e..19abc0c1 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -127,6 +127,7 @@ noinst_LTLIBRARIES += libcomposefs.la
 endif
 
 include Makefile-otutil.am
+include Makefile-otcore.am
 include Makefile-libostree.am
 include Makefile-ostree.am
 include Makefile-switchroot.am
diff --git a/src/libostree/ostree-sign-ed25519.c b/src/libostree/ostree-sign-ed25519.c
index 6ff13260..b7682416 100644
--- a/src/libostree/ostree-sign-ed25519.c
+++ b/src/libostree/ostree-sign-ed25519.c
@@ -24,21 +24,10 @@
 #include "config.h"
 
 #include "ostree-sign-ed25519.h"
+#include "otcore.h"
 #include <libglnx.h>
 #include <ot-checksum-utils.h>
 
-#ifdef HAVE_LIBSODIUM
-#include <sodium.h>
-#define USE_LIBSODIUM
-#else
-
-#if defined(HAVE_OPENSSL)
-#include <openssl/evp.h>
-#define USE_OPENSSL
-#endif
-
-#endif
-
 #undef G_LOG_DOMAIN
 #define G_LOG_DOMAIN "OSTreeSign"
 
@@ -47,8 +36,6 @@
 #define OSTREE_SIGN_METADATA_ED25519_KEY "ostree.sign.ed25519"
 #define OSTREE_SIGN_METADATA_ED25519_TYPE "aay"
 
-#define OSTREE_SIGN_ED25519_SIG_SIZE 64U
-#define OSTREE_SIGN_ED25519_PUBKEY_SIZE 32U
 #define OSTREE_SIGN_ED25519_SEED_SIZE 32U
 #define OSTREE_SIGN_ED25519_SECKEY_SIZE \
   (OSTREE_SIGN_ED25519_SEED_SIZE + OSTREE_SIGN_ED25519_PUBKEY_SIZE)
@@ -108,12 +95,11 @@ _ostree_sign_ed25519_init (OstreeSignEd25519 *self)
   self->public_keys = NULL;
   self->revoked_keys = NULL;
 
-#if defined(USE_LIBSODIUM)
-  if (sodium_init () < 0)
-    self->state = ED25519_FAILED_INITIALIZATION;
-#elif defined(USE_OPENSSL)
-#else
+#if !(defined(USE_OPENSSL) || defined(USE_LIBSODIUM))
   self->state = ED25519_NOT_SUPPORTED;
+#else
+  if (!otcore_ed25519_init ())
+    self->state = ED25519_FAILED_INITIALIZATION;
 #endif
 }
 
@@ -232,7 +218,6 @@ ostree_sign_ed25519_data_verify (OstreeSign *self, GBytes *data, GVariant *signa
     {
       g_autoptr (GVariant) child = g_variant_get_child_value (signatures, i);
       g_autoptr (GBytes) signature = g_variant_get_data_as_bytes (child);
-      gboolean valid = FALSE;
 
       if (g_bytes_get_size (signature) != OSTREE_SIGN_ED25519_SIG_SIZE)
         return glnx_throw (
@@ -246,7 +231,6 @@ ostree_sign_ed25519_data_verify (OstreeSign *self, GBytes *data, GVariant *signa
 
       for (GList *public_key = sign->public_keys; public_key != NULL; public_key = public_key->next)
         {
-
           /* TODO: use non-list for tons of revoked keys? */
           if (g_list_find_custom (sign->revoked_keys, public_key->data, _compare_ed25519_keys)
               != NULL)
@@ -256,32 +240,12 @@ ostree_sign_ed25519_data_verify (OstreeSign *self, GBytes *data, GVariant *signa
               continue;
             }
 
-#if defined(USE_LIBSODIUM)
-          valid = crypto_sign_verify_detached ((guchar *)g_variant_get_data (child),
-                                               g_bytes_get_data (data, NULL),
-                                               g_bytes_get_size (data), public_key->data)
-                  == 0;
-#elif defined(USE_OPENSSL)
-          EVP_MD_CTX *ctx = EVP_MD_CTX_new ();
-          if (!ctx)
-            return glnx_throw (error, "openssl: failed to allocate context");
-          EVP_PKEY *pkey = EVP_PKEY_new_raw_public_key (EVP_PKEY_ED25519, NULL, public_key->data,
-                                                        OSTREE_SIGN_ED25519_PUBKEY_SIZE);
-          if (!pkey)
-            {
-              EVP_MD_CTX_free (ctx);
-              return glnx_throw (error, "openssl: Failed to initialize ed5519 key");
-            }
-
-          valid = EVP_DigestVerifyInit (ctx, NULL, NULL, NULL, pkey) != 0
-                  && EVP_DigestVerify (ctx, g_bytes_get_data (signature, NULL),
-                                       g_bytes_get_size (signature), g_bytes_get_data (data, NULL),
-                                       g_bytes_get_size (data))
-                         != 0;
-
-          EVP_PKEY_free (pkey);
-          EVP_MD_CTX_free (ctx);
-#endif
+          bool valid = false;
+          // Wrap the pubkey in a GBytes as that's what this API wants
+          g_autoptr (GBytes) public_key_bytes
+              = g_bytes_new_static (public_key->data, OSTREE_SIGN_ED25519_PUBKEY_SIZE);
+          if (!otcore_validate_ed25519_signature (data, public_key_bytes, signature, &valid, error))
+            return FALSE;
           if (!valid)
             {
               /* Incorrect signature! */
diff --git a/src/libotcore/README.md b/src/libotcore/README.md
new file mode 100644
index 00000000..da86c9d2
--- /dev/null
+++ b/src/libotcore/README.md
@@ -0,0 +1 @@
+This library is shared between `libostree-1.so` and `ostree-prepare-root`.
diff --git a/src/libotcore/otcore-ed25519-verify.c b/src/libotcore/otcore-ed25519-verify.c
new file mode 100644
index 00000000..6d2830bc
--- /dev/null
+++ b/src/libotcore/otcore-ed25519-verify.c
@@ -0,0 +1,109 @@
+/*
+ * SPDX-License-Identifier: LGPL-2.0+
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see <https://www.gnu.org/licenses/>.
+ */
+
+#include "config.h"
+
+#include "otcore.h"
+
+/* Initialize global state; may be called multiple times and is idempotent. */
+bool
+otcore_ed25519_init (void)
+{
+#if defined(HAVE_LIBSODIUM)
+  static int initstate;
+  static bool success;
+  if (g_once_init_enter (&initstate))
+    {
+      int val = sodium_init () >= 0 ? 1 : -1;
+      g_once_init_leave (&initialized, initstate);
+    }
+  switch
+    initstate
+    {
+    case 1:
+      return true;
+    case -1:
+      return false;
+    default:
+      g_assert_not_reached ();
+    }
+#else
+  return true;
+#endif
+}
+
+gboolean
+otcore_validate_ed25519_signature (GBytes *data, GBytes *public_key, GBytes *signature,
+                                   bool *out_valid, GError **error)
+{
+  // Since this is signature verification code, let's verify preconditions.
+  g_assert (data);
+  g_assert (public_key);
+  g_assert (signature);
+  g_assert (out_valid);
+  // It is OK for error to be NULL, though according to GError rules.
+
+#if defined(HAVE_LIBSODIUM) || defined(HAVE_OPENSSL)
+  // And strictly verify pubkey and signature lengths
+  if (g_bytes_get_size (public_key) != OSTREE_SIGN_ED25519_PUBKEY_SIZE)
+    return glnx_throw (error, "Invalid public key of %" G_GSIZE_FORMAT " expected %" G_GSIZE_FORMAT,
+                       (gsize)g_bytes_get_size (public_key),
+                       (gsize)OSTREE_SIGN_ED25519_PUBKEY_SIZE);
+  const guint8 *public_key_buf = g_bytes_get_data (public_key, NULL);
+  if (g_bytes_get_size (signature) != OSTREE_SIGN_ED25519_SIG_SIZE)
+    return glnx_throw (
+        error, "Invalid signature length of %" G_GSIZE_FORMAT " bytes, expected %" G_GSIZE_FORMAT,
+        (gsize)g_bytes_get_size (signature), (gsize)OSTREE_SIGN_ED25519_SIG_SIZE);
+  const guint8 *signature_buf = g_bytes_get_data (signature, NULL);
+
+#endif
+
+#if defined(HAVE_LIBSODIUM)
+  // Note that libsodium assumes the passed byte arrays for the signature and public key
+  // have at least the expected length, but we checked that above.
+  if (crypto_sign_verify_detached (signature_buf, g_bytes_get_data (data, NULL),
+                                   g_bytes_get_size (data), public_key_buf)
+      == 0)
+    {
+      *out_valid = true;
+    }
+  return TRUE;
+#elif defined(HAVE_OPENSSL)
+  EVP_MD_CTX *ctx = EVP_MD_CTX_new ();
+  if (!ctx)
+    return glnx_throw (error, "openssl: failed to allocate context");
+  EVP_PKEY *pkey = EVP_PKEY_new_raw_public_key (EVP_PKEY_ED25519, NULL, public_key_buf,
+                                                OSTREE_SIGN_ED25519_PUBKEY_SIZE);
+  if (!pkey)
+    {
+      EVP_MD_CTX_free (ctx);
+      return glnx_throw (error, "openssl: Failed to initialize ed5519 key");
+    }
+  if (EVP_DigestVerifyInit (ctx, NULL, NULL, NULL, pkey) != 0
+      && EVP_DigestVerify (ctx, signature_buf, OSTREE_SIGN_ED25519_SIG_SIZE,
+                           g_bytes_get_data (data, NULL), g_bytes_get_size (data))
+             != 0)
+    {
+      *out_valid = true;
+    }
+  EVP_PKEY_free (pkey);
+  EVP_MD_CTX_free (ctx);
+  return TRUE;
+#else
+  return glnx_throw (error, "ed25519 signature validation requested, but support not compiled in");
+#endif
+}
\ No newline at end of file
diff --git a/src/libotcore/otcore.h b/src/libotcore/otcore.h
new file mode 100644
index 00000000..8527e03c
--- /dev/null
+++ b/src/libotcore/otcore.h
@@ -0,0 +1,38 @@
+/*
+ * SPDX-License-Identifier: LGPL-2.0+
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see <https://www.gnu.org/licenses/>.
+ */
+
+#pragma once
+
+#include "config.h"
+
+#include "otutil.h"
+#include <stdbool.h>
+
+#ifdef HAVE_LIBSODIUM
+#include <sodium.h>
+#define USE_LIBSODIUM
+#elif defined(HAVE_OPENSSL)
+#include <openssl/evp.h>
+#define USE_OPENSSL
+#endif
+
+#define OSTREE_SIGN_ED25519_SIG_SIZE 64U
+#define OSTREE_SIGN_ED25519_PUBKEY_SIZE 32U
+
+bool otcore_ed25519_init (void);
+gboolean otcore_validate_ed25519_signature (GBytes *data, GBytes *pubkey, GBytes *signature,
+                                            bool *out_valid, GError **error);
diff --git a/src/switchroot/ostree-prepare-root.c b/src/switchroot/ostree-prepare-root.c
index 8c007da8..c57e1539 100644
--- a/src/switchroot/ostree-prepare-root.c
+++ b/src/switchroot/ostree-prepare-root.c
@@ -70,6 +70,7 @@
 #include <sys/types.h>
 #include <unistd.h>
 
+#include "otcore.h"
 #include <ostree-core.h>
 #include <ostree-repo-private.h>
 
@@ -226,47 +227,6 @@ load_commit_for_deploy (const char *root_mountpoint, const char *deploy_path, GV
   return TRUE;
 }
 
-static gboolean
-validate_signature (GBytes *data, GVariant *signatures, const guchar *pubkey, size_t pubkey_size)
-{
-#if defined(USE_LIBSODIUM)
-  if (sodium_init () < 0)
-    err (EXIT_FAILURE, "Failed to init libsodiume");
-#endif
-
-  for (gsize i = 0; i < g_variant_n_children (signatures); i++)
-    {
-      g_autoptr (GVariant) child = g_variant_get_child_value (signatures, i);
-      g_autoptr (GBytes) signature = g_variant_get_data_as_bytes (child);
-      gboolean valid = FALSE;
-
-#if defined(USE_LIBSODIUM)
-      valid = crypto_sign_verify_detached (g_bytes_get_data (signature, NULL),
-                                           g_bytes_get_data (data, NULL), g_bytes_get_size (data),
-                                           pubkey)
-              == 0;
-#elif defined(USE_OPENSSL)
-      EVP_MD_CTX *ctx = EVP_MD_CTX_new ();
-      EVP_PKEY *pkey = EVP_PKEY_new_raw_public_key (EVP_PKEY_ED25519, NULL, pubkey, pubkey_size);
-      valid = ctx != NULL && pkey != NULL && EVP_DigestVerifyInit (ctx, NULL, NULL, NULL, pkey) != 0
-              && EVP_DigestVerify (ctx, g_bytes_get_data (signature, NULL),
-                                   g_bytes_get_size (signature), g_bytes_get_data (data, NULL),
-                                   g_bytes_get_size (data))
-                     != 0;
-      if (pkey)
-        EVP_PKEY_free (pkey);
-      if (ctx)
-        EVP_MD_CTX_free (ctx);
-#else
-      errx (EXIT_FAILURE, "Signature validation requested, but support not compiled in");
-#endif
-      if (valid)
-        return TRUE;
-    }
-
-  return FALSE;
-}
-
 int
 main (int argc, char *argv[])
 {
@@ -407,8 +367,24 @@ main (int argc, char *argv[])
             errx (EXIT_FAILURE, "Signature validation requested, but no signatures in commit");
 
           g_autoptr (GBytes) commit_data = g_variant_get_data_as_bytes (commit);
-          if (!validate_signature (commit_data, signatures, (guchar *)pubkey, pubkey_size))
-            errx (EXIT_FAILURE, "No valid signatures found for public key");
+          g_autoptr (GBytes) pubkey_buf = g_bytes_new_static (pubkey, pubkey_size);
+          bool valid_signature = false;
+          if (!otcore_ed25519_init ())
+            errx (EXIT_FAILURE, "Failed to initialize ed25519");
+          for (gsize i = 0; i < g_variant_n_children (signatures); i++)
+            {
+              g_autoptr (GVariant) child = g_variant_get_child_value (signatures, i);
+              g_autoptr (GBytes) signature = g_variant_get_data_as_bytes (child);
+              if (!otcore_validate_ed25519_signature (commit_data, pubkey_buf, signature,
+                                                      &valid_signature, &local_error))
+                errx (EXIT_FAILURE, "Failed to verify signature: %s", local_error->message);
+              if (valid_signature)
+                break;
+            }
+          if (!valid_signature)
+            {
+              errx (EXIT_FAILURE, "No valid signatures found");
+            }
 
 #ifdef USE_LIBSYSTEMD
           sd_journal_send ("MESSAGE=Validated commit signature using '%s'", composefs_pubkey, NULL);
-- 
2.40.1

[cgwalters]

OK I updated this with a prep cleanup commit, a commit which introduces libotcore.la and factors out a signature verification helper, then your commit, then finally a fixup! that demonstrates how we now de-deuplicate the signature verification between libostree-1.so and ostree-prepare-root using the new libotcore.la. (edit: and another inevitable fixup! for clang-format)

[alexlarsson]

These looks good to me, so i squashed them. Will do some testing today.

[cgwalters]

Can you also add at least a stub to doc/composefs.md describing this? Doesn't have to be super polished, we can collaborate on it and iterate.

[cgwalters]

Having the pubkey on the kernel commandline is probably fine, but longer term it feels like what we want instead is to have this baked into the initramfs as e.g. /usr/lib/ostree/rootfs-initramfs-binding-key.pub or so?

Basically in general I think kernel arguments should primarily be for things interpreted by the Linux kernel. (Of course the historical use case of passing arguments to init (systemd) will probably always be there)

And this is actually argument that even ostree= should work this way.

[alexlarsson]

Do you mean, that instead of checking the commandline we just check for the existence of this file in the initrd, and if it is there we assume we need to validate against it?

[cgwalters]

Right.

BTW one thing that might be nice here is to write the state into /run/ostree-booted - we could make that an a{sv} and write e.g. composefs = "signed" into it and that way e.g. ostree admin status and equivalent can show that the composefs was handled in a signed way (instead of these tools parsing kernel arguments, and also so they're explicitly verifying the post-execution state instead of the configuration-to-be-applied).

[cgwalters]

I edited the PR title to be a bit clearer, can you do the same for the commit message?

(A minor procedural note; I'd recommend using Github PR "draft" state instead of adding "WIP:" into the title, because I've seen more than one PR merge with the commit message changed but the PR title still in "WIP:" state, and that PR title ends up in the merge commit message forever 😄)

[cgwalters]

This is very similar to how the kernel rpm build generates a throwaway key to sign the kernel modules, and then builds the public part into the kernel image itself.

Thanks, I wasn't aware of how that worked. That's actually a very strong comparison. In fact the more I think about it, the more this is basically extending the kernel module signatures to the rest of the userspace filesystem tree. Right?

(If one has an LSM configured and enforcing such that kmods can only be loaded from a filesystem tree with verified integrity such as dm-verity or composefs, then we don't need the distinct kernel key at all, right?)

[alexlarsson]

This is very similar to how the kernel rpm build generates a throwaway key to sign the kernel modules, and then builds the public part into the kernel image itself.

Thanks, I wasn't aware of how that worked. That's actually a very strong comparison. In fact the more I think about it, the more this is basically extending the kernel module signatures to the rest of the userspace filesystem tree. Right?

Yeah, thats probably the right way to see it.

(If one has an LSM configured and enforcing such that kmods can only be loaded from a filesystem tree with verified integrity such as dm-verity or composefs, then we don't need the distinct kernel key at all, right?)

You need a slightly tighter rule. Like you can't allow anyone to just download and loopback mount some random dm-verity image they control. It has to be in the trusted path. But, other than that, yeah.

[alexlarsson]

(Oh, and you have to also allow modules from the initrd obviously)

[alexlarsson]

Ok, I managed to boot an automotive system with this:
https://gitlab.com/CentOS/automotive/sample-images/-/merge_requests/333
This is based on some new osbuild stages i added here:
osbuild/osbuild#1343

The resulting initrd boot debug output:

Jul 11 09:40:48 localhost kernel: Kernel command line: BOOT_IMAGE=(hd0,gpt2)/ostree/centos-763e3d8d9dfd809065c39af0897c20ca38a6f327e09fa9cfb8663be1c0789d54/vmlinuz-5.14.0-10001.275.test.el9.x86_64 root=LABEL=root ro loglevel=4 efi=runtime libahci.ignore_sss=1 console=ttyS0 ot-composefs=signed=/etc/ostree/commit.key ostree=/ostree/boot.1/centos/763e3d8d9dfd809065c39af0897c20ca38a6f327e09fa9cfb8663be1c0789d54/0
Jul 11 09:40:48 localhost ostree-prepare-root[452]: preparing sysroot at /sysroot
Jul 11 09:40:48 localhost ostree-prepare-root[452]: Resolved OSTree target to: /sysroot/ostree/deploy/centos/deploy/2eec6251265b9792709f71956c536714b2eb82d98471e30de9443f262df36144.0
Jul 11 09:40:48 localhost ostree-prepare-root[452]: Validated commit signature using '/etc/ostree/commit.key'
Jul 11 09:40:48 localhost ostree-prepare-root[452]: Mounting composefs rootfs with expected digest 'f883e926af566f777d60a927216b0dd5e23c88d3be3ace95052168c3cff2cbde'
Jul 11 09:40:48 localhost ostree-prepare-root[452]: Mounted composefs

[cgwalters]

OK yeah, there's stuff I'd like to change but since all this is still experimental and to preserve momentum let's get this in now.

[cgwalters]

Ah yes, make syntax-check was failing on a missing trailing newline. Grr. Tempted to remove that.