-
Notifications
You must be signed in to change notification settings - Fork 13
Support for handling sha512-addressed repos in cfsctl and composefs-setup-root #199
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
base: main
Are you sure you want to change the base?
Conversation
Johan-Liebert1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had reviewed earlier, but forgot to press submit :|
91a1e94 to
c12eec3
Compare
c12eec3 to
b99fb36
Compare
|
Sorry missed another |
…etup-root Signed-off-by: Chaser Huang <huangkangjing@gmail.com>
b99fb36 to
66b2539
Compare
cgwalters
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I'm OK with this, but I do think we should add the repo metadata at some point.
In practice...because we're not shipping this project even on crates.io (much less as a standalone package/component) making "breaking" changes like this doesn't matter much.
But...we do want to eventually do that I think in order to replace the composefs-c project. But when we do that there will probably be deeper changes anyways.
| }; | ||
| let (image, insecure) = get_cmdline_composefs::<Sha256HashValue>(cmdline)?; | ||
|
|
||
| let (image_addr, insecure) = parse_image_address(cmdline)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also relates to the repo metadata thing. If we had that, then we would know precisely what sha digest length we should be expecting here.
| #[derive(Debug, Copy, Clone, PartialEq, Eq, ValueEnum, Default)] | ||
| enum HashType { | ||
| Sha256, | ||
| #[default] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So...this will increase compatibility with bootc when using the raw CLI, but it will also have the effect of changing the default here too, including AFAIK all CI jobs here. I think that's probably fine...but worth noting.
|
Most of the CI here was already broken for other reasons, but this creates a new problem:
I think it might be because it's missing |
This PR changes
cfsctlto use sha512-addressed composefs repo by default, this confronts to the behavior of bootc, making the two tools compatible with each other. An additional flag--use-sha256-object-idis also added to enablecfsctlto work with legacy repos.kernel cmdline parse logic in
composefs-setup-rootis also changed, both sha256 and sha512 length addresses would be attempted before returning an error, improving robustness within initramfs.Fixes #197