Support using composefs signatures also with bootc commits #3523
Conversation
|
I know this is the "old" approach and there is work on sealing images with the new native composefs backend. However, this is a minor change and lets existing users move to bootc easily while keeping this working. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for using composefs signatures from a base commit, which is useful for bootc generated images. The changes in otcore-prepare-root.c correctly implement a fallback mechanism to load the base commit and its metadata if the main commit is not signed. My review focuses on improving the robustness of the implementation, reducing code duplication, and ensuring logging consistency. I've identified a potential issue with the regular expression used for parsing JSON, which could be brittle. I've also suggested refactoring duplicated error handling logic and improving logging and conditional checks for better code clarity and maintainability.
alexlarsson
force-pushed
the
signed-composefs-with-bootc
branch
from
d2d2cb9 to
6461363
Compare
|
Not reviewing the code in detail yet...yeah I totally understand this and have thought about it myself... But...there's obviously a lot of suboptimal things about this (many of which get fixed with the big composefs-native work of course). In the short term though, I think we could change bootc to store the manifest and config data out of band if the input image is "fully" ostree (i.e. does not have any non-ostree layers). |
|
@cgwalters You mean re-use the existing commit in that case? And add the manifest/commit to the commitmeta? |
|
I wish we had signed the composefs digest instead of the complete commit. Then we could just have copied the composefs digest + its signature into the new commit. |
Yep |
src/libotcore/otcore-prepare-root.c
Outdated
| return FALSE; | ||
|
|
||
| /* In case the commit is one created by bootc when importing a container, it will not | ||
| be signed. However, we can still look at the base commit which may be. */ |
There was a problem hiding this comment.
Haven't tried to follow the logic, but have you made sure this doesn't break layering:
- rpm-ostree local layering (not sure if metadata like
ostree.container.image-configis copied between commit when doing local layering) - container layering (
LABELare kept when adding layers I think)
There was a problem hiding this comment.
Remember that people doing sealed images don't want either of those things to work.
There was a problem hiding this comment.
Sure, but silently ignoring the extra layers would be super surprising behavior.
You can imagine a vendor delivering a signed commit encapsulated in a container, and a customer layering their favorite security/monitoring agent.
This should error out (during deployment ?), telling the customer they must disable signature checks
There was a problem hiding this comment.
Well, sealing fundamentally breaks layering. In this case, the boot will read the expected composefs digest from the base ostree commit, and if there were added layers the deployed composefs image will have a different commit, so it will fail to boot with an "invalid digest". But this is sort of the point of sealing it in the first place.
If you truly want to layer something you need to at the end run something that re-signs the completed image, and that could if you wanted re-create a full signed ostree commit to make it boot again.
There was a problem hiding this comment.
If you truly want to layer something you need to at the end run something that re-signs the completed image, and that could if you wanted re-create a full signed ostree commit to make it boot again.
Or disable signature checking at runtime, because you just want to run a quick test and don't have the signing key.
IIUC, I'm not sure we could do this by default. Wouldn't this change the output of Well... I guess we could change |
Probably but I think things that are doing that are broken and should have been instead using the |
I'm not sure I buy that. :) Anyway, we could adapt obviously (and I actually much prefer if we had it detached from the start), though given that this is all pretty stable stuff at this point, we need to be careful. I'd prefer having it opt-in to start. |
Motivated by ostreedev/ostree#3523 This is an obvious and trivially easy thing to do here, and makes dereferencing from "merge -> base" client side also trivial which is especially important in the initramfs.
|
I did bootc-dev/bootc#1600 which should dramatically simplify this.
Yes. I think a clear next enhancement to bootc (ostree-ext) is something like |
|
Let's go back to the start here though:
This isn't true. The problem solely lies in how bootc always creates a local commit (in order to handle non-ostree layers). |
Motivated by ostreedev/ostree#3523 This is an obvious and trivially easy thing to do here, and makes dereferencing from "merge -> base" client side also trivial which is especially important in the initramfs. Signed-off-by: Colin Walters <[email protected]>
|
The more I think about this the more I feel this all just needs to be fixed on the bootc side. |
|
I feel the opposite. This is a very localized hack that makes it work with minimal changes. Changing the format of the commits is a much more major change. Especially given that we want to do something else long-term. |
|
One thing I was considering, is could we have bootc embedd the original commit and commitmeta variants as metadata keys in the bootc created commit as "base-commit" and "base-commitmeta"? That would make this workaround much cleaner. |
Not sure where the above comment came from, but that is not quite true. The local commit will be used to deploy. The base commit is only used to find the expected composefs digest, so what will happen is that you deploy something that will then not boot. I mean, this isn't exactly great either, but its not just "silently ignoring" some content. Also, it is possible to derive from the container if you either override the composefs=signed option in the derived image. Or if you later use some tool to re-sign it (convert it back to a pure ostree image). |
|
I took a look at it, it's not that major...did some prep work in bootc-dev/bootc#1602 Basically in this flow we'd skip Something to bear in mind of course here is that in theory in this case, the client side contents should be the same as the server side generated contents...but we're definitely not strictly validating that right now. Hmm, let me try out adding an assertion that they're content identical and see... There's some potentially kind of tricky skew here if what we do at mount time is different from the filesystem/commit used by all of the rest of the code. I'm OK to do something here but but the regex parsing JSON feels a bit too hacky for something security sensitive. How about the logic is:
? |
I think this is good. Although i think looking for the |
I mean, that MR makes this code much cleaner, so i love it, but what I mean was embedding the actual commit data, not the digests. |
Hmm. But if we just copied the composefs digest, I think we'd want to verify that they're actually the same thing. They should be the same thing...but now that I look they're not. Looking at current What's a bit more concerning there is the labels seem different e.g. Where the base labels look wrong. Actually though, This is probably worth chasing - must be an rpm-ostree side issue. Although it might somehow be specific to the container build path we're using; do you see the |
|
|
I don't see that difference. I did see some differences due to coreos/rpm-ostree#5485 but when I fixed that i get identical content checksums. Honestly, having this break something is pretty good, because these shouldn't diverge. Any such divergence is some kind of bug. In my builds, the only default_t is: |
This is a minor preparation for a later change. Instead of hand-rolling the G_FILE_ERROR_NOENT error check we add a new allow_noent option. Additionally, we move the handling of a no commitmeta being an error to the caller of load_commit_for_deploy(), because this check will be slightly more complex in the future.
When using bootc, if you convert a signed ostree commit into an OCI image `rpm-ostree compose container-encapsulate` you end up with a new commit that isn't signed. However, the base commit object, and its commitmeta are still in the image and will end up the repo, and since bootc-dev/bootc#1600 the base commit id is available as the parent commit. So, we change ostree-prepare-root to fall back to using the base commit+commitmeta to find the expected composefs digest if the main commit is not signed. Note: This will only work with ostree-only commits. If you have any layered data, then the content will change, and the composefs digest in the base commit will not match the deployed one. This is expected with such sealed commits though. If you want to layer, either disable sealing, or create a new sealed ostree commit for the new image.
alexlarsson
force-pushed
the
signed-composefs-with-bootc
branch
from
6461363 to
92f6d8e
Compare
|
I updated this to be based on the new "parent commit". Also, i split out some helper work in a separate commit to make it more reviewable. |
4 participants
When using bootc, if you convert a signed ostree commit into an OCI image
rpm-ostree compose container-encapsulateyou end up with a new commit that isn't signed. However, the base commit object, and its commitmeta are still in the image and will end up the repo.The base commit id is available in the container image config as a Label. So, we change ostree-prepare-root to fall back to using this base commit+commitmeta to find the expected composefs digest if the main commit is not signed.