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

ostree-ext: surface libostree signature verification text #1028

Merged
merged 4 commits into from
Jan 14, 2025

Conversation

jlebon
Copy link
Contributor

@jlebon jlebon commented Jan 14, 2025

Right now when using OSTree remote verification, there's no indication
that the OSTree commit was successfully verified.

Capture the return string from OstreeRepo.signature_verify_commit_data
and return it as part of the public LayeredImageState object.

We could return a more structured signature object here, but this is
sufficient for our purposes for now and saves clients from having to
regenerate similar looking text.

In the ostree container CLI, print this verification text.

@github-actions github-actions bot added the area/install Issues related to `bootc install` label Jan 14, 2025
@jlebon
Copy link
Contributor Author

jlebon commented Jan 14, 2025

Haven't added any tests to this. Didn't really look at the testing setup in this repo yet.

The `install` and `switch` verbs supported passing an OSTree remote to
use to verify the embedded OSTree commit. This option in fact did not
function correctly and no verification was actually performace.

While it's possible someone was using this feature, it seems quite
unlikely since the UX is more geared towards native OCI signatures. As a
result, we have decided not to file a CVE for this.

And in fact, since we're planning to keep moving away from ostree,
instead of fixing this bug, just completely rip out support for passing
and OSTree remote.

Note this is distinct from the ostree-ext code, which still does support
signature verification of the embedded OSTree commit. E.g. Fedora
CoreOS is planning to make use of that until it can move to proper OCI
signatures.

While we're here, drop the negation around the container sigpolicy to
make it easier to follow the logic.

Signed-off-by: Jonathan Lebon <[email protected]>
We don't actually mutate anything in `self` in this function.

Signed-off-by: Jonathan Lebon <[email protected]>
Right now when using OSTree remote verification, there's no indication
that the OSTree commit was successfully verified.

Capture the return string from `OstreeRepo.signature_verify_commit_data`
and return it as part of the public `LayeredImageState` object.

We could return a more structured signature object here, but this is
sufficient for our purposes for now and saves clients from having to
regenerate similar looking text.

In the `ostree container` CLI, print this verification text.

Signed-off-by: Jonathan Lebon <[email protected]>
Copy link
Collaborator

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

Also of note at some point IMO we should do the same for skopeo re surfacing verification status, although I think the maintainer felt that was an anti-pattern I don't agree.

@@ -111,10 +111,6 @@ pub(crate) struct SwitchOpts {
#[clap(long)]
pub(crate) enforce_container_sigpolicy: bool,

/// Enable verification via an ostree remote
#[clap(long)]
pub(crate) ostree_remote: Option<String>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Technically we claimed public CLI/API stability but I can't find any users of this on public source code, and I can't imagine there are any private ones.

To be clear: Everyone should use container signing, not ostree commit signatures.

@cgwalters cgwalters merged commit a2ecc4b into containers:main Jan 14, 2025
24 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/install Issues related to `bootc install`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants