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.

Sign up for GitHub

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

Jump to bottom

Extension repart definitions #3437

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Extension repart definitions #3437

wants to merge 1 commit into from
+49 −24

Conversation

hundeboll
Copy link
Contributor

No description provided.

DaanDeMeyer
DaanDeMeyer requested changes Jan 27, 2025
View reviewed changes
Copy link
Contributor

@DaanDeMeyer DaanDeMeyer left a comment

Choose a reason for hiding this comment

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

First commit looks good, second one less so. Making the repart definitions for these output types a free for all does not seem like a good idea.

Can't you achieve what you need with Format=disk and your own repart definitions instead?

@hundeboll
Copy link
Contributor Author

Can't you achieve what you need with Format=disk and your own repart definitions instead?

I wanted to benefit from the extension release file generated by mkosi... But if my use case is too niche, I can do with the Format=disk approach.

Another option would be a way to add --exclude-partitions=root-verity-sig to the systemd-repart command?

And while we're at it: Why not use systemd-repart --make-ddi=sysext instead of maintaining definition files?

@hundeboll hundeboll force-pushed the extension-repart-definitions branch from 05c3c90 to 2d0efc5 Compare January 28, 2025 08:36
@hundeboll
Copy link
Contributor Author

Another option would be a way to add --exclude-partitions=root-verity-sig to the systemd-repart command?

And while we're at it: Why not use systemd-repart --make-ddi=sysext instead of maintaining definition files?

Okay, pushed a proposal using the --make-ddi option. This changes the output image to always contain a verity data hash partition. Let me know if this has interest...

@hundeboll hundeboll force-pushed the extension-repart-definitions branch from 2d0efc5 to 610cfba Compare January 28, 2025 08:43
@DaanDeMeyer
Copy link
Contributor

@hundeboll This is much worse, now the repart definitions have to be installed in the root filesystem or tools tree. That's exactly why we don't use --make-ddi= in the first place.

@hundeboll
Copy link
Contributor Author

@hundeboll This is much worse, now the repart definitions have to be installed in the root filesystem or tools tree. That's exactly why we don't use --make-ddi= in the first place.

I see. I'll revert that part.

How about the other part - always using the signed repart definitions, and just exclude the signature partition when verity is off? Or would it be better with a Verity=hash-only setting ?

@DaanDeMeyer
Copy link
Contributor

@hundeboll Adding Verity=hash sounds good. You'll want to use config_make_enum_parser_with_boolean() for that.

@hundeboll hundeboll force-pushed the extension-repart-definitions branch 4 times, most recently from bd68892 to 64d0b7b Compare January 28, 2025 21:05
@hundeboll
Copy link
Contributor Author

I added the Verity=hash option. Should be ready for review.

Thanks for the hint on config_make_enum_parser_with_boolean 👍

DaanDeMeyer
DaanDeMeyer requested changes Jan 29, 2025
View reviewed changes
mkosi/config.py Outdated
@@ -363,6 +363,13 @@ def __bool__(self) -> bool:
return self != BuildSourcesEphemeral.no


class Verity(StrEnum):
yes = enum.auto()
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's name this signed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's okay to break existing users of --verity=yes ?

Copy link
Contributor

Choose a reason for hiding this comment

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

parse_boolean() will take care of the boolean values

mkosi/resources/man/mkosi.1.md Outdated
partition. If set to `auto` and a verity key and certificate are present,
**mkosi** will pass them to systemd-repart and expects the generated disk
image to contain verity partitions, but the build won't fail if no verity
partitions are found in the disk image produced by **systemd-repart**.

Note that explicitly disabling signed verity is not yet implemented
Copy link
Contributor

Choose a reason for hiding this comment

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

This also needs to mention that Verity=hash is not supported for the disk output yet.

@DaanDeMeyer
Copy link
Contributor

Setting don't-merge until we get another stable release out

@hundeboll
Support unsigned verity backed extension/portable images
f88f826 
Building an unsigned extension image with verity hashes provides data
integrity without needing a certificate on the target machine.

Note that systemd-dissect and systemd-sysext doesn't automatically
use the verity data has partition for validation. Both tools enables
validation if the user.verity.roothash xattr is set for the image.
For systemd-dissect, one can use the --root-hash option to enable the
validation.

The root hash can be obtained by concatenating the partition uuid's for
the root and the root-verity partitions.
@hundeboll hundeboll force-pushed the extension-repart-definitions branch from 64d0b7b to f88f826 Compare January 29, 2025 12:40
septatrix
septatrix suggested changes Jan 31, 2025
View reviewed changes
Copy link
Contributor

@septatrix septatrix left a comment

Choose a reason for hiding this comment

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

Thanks for working on this and implementing my proposal from #3121. However for consistency we should either reuse the nomenclature from either sd-repart or sd.image-policy. Those are off/hash/signature and off/verity/signed respectively

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DaanDeMeyer DaanDeMeyer DaanDeMeyer requested changes

@septatrix septatrix septatrix requested changes

Assignees
No one assigned
Labels
dont-merge
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hundeboll @DaanDeMeyer @septatrix