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

cli: Initial version of Solar tool #305

Open
wants to merge 3 commits into
base: msft-main
Choose a base branch
from
Open

Conversation

jiria
Copy link
Member

@jiria jiria commented Feb 5, 2025

Solar tool is used for calculating root hashes of container image layers, signing them and producing a json manifest.

Merge Checklist
  • Followed patch format from upstream recommendation: https://github.com/kata-containers/community/blob/main/CONTRIBUTING.md#patch-format
    • Included a single commit in a given PR - at least unless there are related commits and each makes sense as a change on its own.
  • Aware about the PR to be merged using "create a merge commit" rather than "squash and merge" (or similar)
  • The upstream/missing label (or upstream/not-needed) has been set on the PR.
Summary

The Solar tool is an extension of genpolicy tool, that creates JSON manifests that hold layer digests and root hash signatures, that can be consumed by the tardev-snapshotter.

Test Methodology

The tool output hash been validated against the tardev-snapshotter.

Solar tool is used for calculating root hashes of container image layers, signing them and producing a json manifest.

Signed-off-by: Jiri Appl <[email protected]>
@jiria jiria requested review from a team as code owners February 5, 2025 23:43
@jiria
Copy link
Member Author

jiria commented Feb 5, 2025

Notes for reviewers:

  • registry.rs, registry_containerd.rs, verity.rs are forks of genpolicy codebase; it would be good to unify them eventually, to ensure fixes can be shared across the codebases
  • if there is a way to add some automated tests, please let me know and would love to add them

@christopherco christopherco requested a review from Copilot March 7, 2025 03:29
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

PR Overview

This PR introduces the initial version of the Solar tool, an extension of the genpolicy tool, that calculates root hashes of container image layers, signs them, and produces a JSON manifest consumable by the tardev-snapshotter.

  • Introduces new CLI functionality for signing OCI layer root hashes.
  • Adds a Cargo.toml with updated dependencies and new modules (main, version, utils, and verity).
  • Implements the core logic for computing and signing root hashes and for processing image lists.

Reviewed Changes

File Description
src/tools/sign-oci-layer-root-hashes/Cargo.toml Adds package definition and dependency configuration for the new tool.
src/tools/sign-oci-layer-root-hashes/src/version.rs.in Provides auto-generated version information.
src/tools/sign-oci-layer-root-hashes/src/main.rs Contains the CLI entry point and core logic for processing images and signing hashes.
src/tools/sign-oci-layer-root-hashes/src/utils.rs Implements CLI argument parsing and configuration management using clap.
src/tools/sign-oci-layer-root-hashes/src/verity.rs Implements the verity hashing logic used for generating and finalizing layer hashes.

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

src/tools/sign-oci-layer-root-hashes/src/main.rs:53

  • [nitpick] The printed tool name "SOLaR" differs in capitalization from the package name "solar". Consider using consistent capitalization for clarity.
println!("SOLaR tool: id: {}, version: {}, commit: {}", env!("CARGO_PKG_NAME"), env!("CARGO_PKG_VERSION"), version::COMMIT_INFO);

src/tools/sign-oci-layer-root-hashes/src/main.rs:100

  • Using .as_mut() on the temporary Vec from the collect operation with Vec::append can lead to issues. Consider replacing this with image_tags.extend( ... ) to append the values directly.
.collect::<Vec<String>>().as_mut(),

.as_mut(),
);
} else if let Some(images) = &config.image {
image_tags.append(images.clone().as_mut());
Copy link
Preview

Copilot AI Mar 7, 2025

Choose a reason for hiding this comment

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

Appending a cloned vector using .as_mut() is problematic because Vec::append expects a mutable vector reference. Use image_tags.extend(images.clone()) instead to correctly merge the vectors.

Suggested change
image_tags.append(images.clone().as_mut());
image_tags.extend(images.clone());

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

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.

1 participant