Skip to content

ci: enforce built-vs-rootfs NVRC hash match#158

Closed
fidencio wants to merge 1 commit into
mainfrom
topic/ci-rootfs-hash-check
Closed

ci: enforce built-vs-rootfs NVRC hash match#158
fidencio wants to merge 1 commit into
mainfrom
topic/ci-rootfs-hash-check

Conversation

@fidencio
Copy link
Copy Markdown
Collaborator

Record the built NVRC sha256 and fail the workflow if the binary copied into rootfs (and still present after image build) does not match.

Record the built NVRC sha256 and fail the workflow if the binary copied
into rootfs (and still present after image build) does not match.

Signed-off-by: Fabiano Fidêncio <ffidencio@nvidia.com>
Copy link
Copy Markdown
Contributor

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.

Pull request overview

Adds CI safeguards to ensure the NVRC binary copied into the rootfs and present after image build matches the freshly-built artifact via sha256 comparison.

Changes:

  • Record sha256/size of built NVRC and write expected hash to a temp file.
  • Verify the rootfs copy matches immediately after cp.
  • Add a new post-image-build step to re-verify the packaged NVRC hash.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Contributor

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.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

@zvonkok
Copy link
Copy Markdown
Collaborator

zvonkok commented May 28, 2026

What is the concern here that cp does not work? The OS failing to copy a file? Wouldn't setting set -euo pipefail be enough?

@fidencio
Copy link
Copy Markdown
Collaborator Author

What is the concern here that cp does not work? The OS failing to copy a file? Wouldn't setting set -euo pipefail be enough?

To be honest, I just wanted to make sure that we won't deal with a stale binary and assume the tests are always passing.

@zvonkok
Copy link
Copy Markdown
Collaborator

zvonkok commented May 28, 2026

I have another idea what about something like this

fn main() {
    lockdown::set_panic_hook();
    // ... after kmsg::kernlog_setup() so logging is live:
    info!("NVRC {}", env!("CARGO_PKG_VERSION"));
    if log::log_enabled!(log::Level::Trace) {
        match std::fs::read("/proc/self/exe").map(|b| sha256_hex(&b)) {
            Ok(h)  => trace!("NVRC sha256={h}"),
            Err(e) => warn!("NVRC self-hash failed: {e}"),
        }
    }
    ...
}

During build we createh the sha256 sum, run the container capture what NVRC reported and bail out if it is not he same?

This prints the NVRC version plus hash over the binary

@zvonkok
Copy link
Copy Markdown
Collaborator

zvonkok commented May 28, 2026

Let me also think about not using a non-crypto fingerprint. Please also take a look at the CLAUDE.md especially the section on hardened_std moving forwared with your PRs.

I have a branch to roll this out.

@zvonkok
Copy link
Copy Markdown
Collaborator

zvonkok commented May 28, 2026

@fidencio See this: #161

Now, when NVRC executes, we can see NVRC version=<CARGO_PKG_VERSION> sha256=<hash>.
Hence, we can build->sha256 and use it to compare against the actual run and fail if they do not match, make sense?

We have one run that enables nvrc.log=debug so we will have the info. In CC setting where we disable the logs nothing should be deducable, hence no output.

@fidencio
Copy link
Copy Markdown
Collaborator Author

I like the idea of #161, let me close this one in favour of it.

@fidencio fidencio closed this May 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Ok to test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants