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.

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

composefs: Change how we do signatures #2879

Merged

Conversation

alexlarsson
Copy link
Member

Currently we generate a signature for the actual composefs image, and
then we apply that when we enable fsverity on the composefs
image. However, there are some issues with this.

First of all, such a signed fs-verity image file can only be read if
the corresponding puiblic keyring is loaded into the fs-verity
keyring. In a typical secure setup we will have a per-commit key that
is loaded from the initrd. Additionally, the keyring is often sealed
to avoid loading more keys later.

This means you can only ever mount (or even look at) composefs images
from the current boot. While this is not a huge issue it is something
of a pain for example when debugging things.

Secondly, and more problematic, during a deploy we can't enable
fs-verity on the newly created composefs file, because and at that
point you need to pass in the signature. Unfortunately this will fail
if the matching public key is not in the keyring, which will fail for
similar reasons as the first issue.

The current workaround is to not enable fs-verity during deploy, but
write the signature to a file. Then the first time the particular
commit is booted we apply the signature to the iamge. This works
around issue two, but not issue one. But it causes us to do a lot of
writes and computation during the first boot as we need to write the
fs-verity merkle tree to disk. It would be much better and robust if
the merkle tree could be written during the deployment of the update
(i.e. before boot).

The new apporach is to always deploy an unsigned, but fs-verity
enabled composefs image. Then we create separate files that contain
the expected digest, and a signature of that file. On the first boot
we sign the digest file, and on further boots we can just verify
that it is signed before using it.

This fixes issue 1, since all deploys are always readable, and it
makes the workaround for issue 2 much less problematic, as we only
need to change a much smaller file on the first boot.

Long term I would like to avoid the first-boot writing totally, and
I've been chatting with David Howells (kernel keyring maintainer) and
he proposed adding a new keyring syscall that verifies a PKCS#7
signature from userspace directly. This would be exactly what
fs-verity does, except we wouldn't have to write the digest to disk
during boot, we would just read the digest file and the signature file
each boot and ask the kernel to verify it.

We will need the new fsverity computation helpers.
src/libostree/ostree-sysroot-deploy.c Show resolved Hide resolved
src/switchroot/ostree-prepare-root.c Outdated Show resolved Hide resolved
src/switchroot/ostree-prepare-root.c Outdated Show resolved Hide resolved
@cgwalters
Copy link
Member

we wouldn't have to write the digest to disk during boot, we would just read the digest file and the signature file each boot and ask the kernel to verify it.

That sounds way better indeed.

Currently we generate a signature for the actual composefs image, and
then we apply that when we enable fsverity on the composefs
image. However, there are some issues with this.

First of all, such a signed fs-verity image file can only be read if
the corresponding puiblic keyring is loaded into the fs-verity
keyring. In a typical secure setup we will have a per-commit key that
is loaded from the initrd. Additionally, the keyring is often sealed
to avoid loading more keys later.

This means you can only ever mount (or even look at) composefs images
from the current boot. While this is not a huge issue it is something
of a pain for example when debugging things.

Secondly, and more problematic, during a deploy we can't enable
fs-verity on the newly created composefs file, because and at that
point you need to pass in the signature. Unfortunately this will fail
if the matching public key is not in the keyring, which will fail for
similar reasons as the first issue.

The current workaround is to *not* enable fs-verity during deploy, but
write the signature to a file. Then the first time the particular
commit is booted we apply the signature to the iamge. This works
around issue two, but not issue one. But it causes us to do a lot of
writes and computation during the first boot as we need to write the
fs-verity merkle tree to disk. It would be much better and robust if
the merkle tree could be written during the deployment of the update
(i.e. before boot).

The new apporach is to always deploy an unsigned, but fs-verity
enabled composefs image. Then we create separate files that contain
the expected digest, and a signature of that file. On the first boot
we sign the digest file, and on further boots we can just verify
that it is signed before using it.

This fixes issue 1, since all deploys are always readable, and it
makes the workaround for issue 2 much less problematic, as we only
need to change a much smaller file on the first boot.

Long term I would like to avoid the first-boot writing totally, and
I've been chatting with David Howells (kernel keyring maintainer) and
he proposed adding a new keyring syscall that verifies a PKCS#7
signature from userspace directly. This would be exactly what
fs-verity does, except we wouldn't have to write the digest to disk
during boot, we would just read the digest file and the signature file
each boot and ask the kernel to verify it.
@alexlarsson alexlarsson force-pushed the composefs-new-signature-approach branch from 99ba5c4 to 2d47661 Compare June 10, 2023 15:14
@alexlarsson
Copy link
Member Author

Seems to have hit the same flake again...

@cgwalters
Copy link
Member

Yeah it's embarrassing, there's a race condition there but I haven't been able to figure it out or even reproduce yet outside of CI.
/override continuous-integration/jenkins/pr-merge

@alexlarsson alexlarsson merged commit c4591c2 into ostreedev:main Jun 10, 2023
19 checks passed
@openshift-ci
Copy link

openshift-ci bot commented Jun 10, 2023

@cgwalters: Overrode contexts on behalf of cgwalters: continuous-integration/jenkins/pr-merge

In response to this:

Yeah it's embarrassing, there's a race condition there but I haven't been able to figure it out or even reproduce yet outside of CI.
/override continuous-integration/jenkins/pr-merge

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.


/* By now we know its fs-verify enabled, also ensure it is signed
* with a key in the keyring */
ensure_digest_fd_is_signed (digest_fd);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why aren't you just storing the signature in the contents of the digestfile and verifying it in userspace here?

Unfortunately I wasn't able to get in front of the train wreck that is CONFIG_FS_VERITY_BUILTIN_SIGNATURES fast enough, but in summary it should not be used, as it is not a good way to do signatures.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's at the end of the commit message, right?

Long term I would like to avoid the first-boot writing totally, and
I've been chatting with David Howells (kernel keyring maintainer) and
he proposed adding a new keyring syscall that verifies a PKCS#7
signature from userspace directly. This would be exactly what
fs-verity does, except we wouldn't have to write the digest to disk
during boot, we would just read the digest file and the signature file
each boot and ask the kernel to verify it.

Or are you suggesting that it's simpler to just do full userspace verification here and not use the kernel keyring?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, just doing full userspace verification would be much simpler.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking at this by the way! Conceptually I think this is a composefs-level issue and not an ostree issue so I've moved this to containers/composefs#151

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants