Skip to content

Conversation

@justus-camp-microsoft
Copy link
Contributor

not ready for review, testing if my install_nix node works properly and what's missing downstream for our build

Copilot AI review requested due to automatic review settings December 9, 2025 18:52
@justus-camp-microsoft justus-camp-microsoft requested review from a team as code owners December 9, 2025 18:52
Copy link
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

This WIP PR adds experimental Nix package manager support to OpenVMM's build infrastructure for use in CI. The primary goal is to enable reproducible builds using Nix to manage build dependencies instead of downloading them dynamically.

Key Changes

  • Adds Nix package definitions for build dependencies (protoc, mdbook tools, openvmm-deps, UEFI firmware, OpenHCL kernel)
  • Implements Nix installation and detection logic in the flowey build system
  • Refactors dependency resolution to support both download and local path modes
  • Modifies cargo build orchestration to wrap builds in nix-shell when USING_NIX=1
  • Updates GitHub Actions workflows to install Nix on CI runners

Reviewed changes

Copilot reviewed 41 out of 41 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
uefi_mu_msvm.nix Nix package definition for UEFI firmware (has incorrect pname)
shell.nix Main Nix shell environment with all build dependencies
protoc.nix Nix package for protobuf compiler
openvmm_deps.nix Nix package for OpenVMM dependencies archive
openhcl_kernel.nix Nix package for OpenHCL Linux kernel (has empty hash for one config)
mdbook*.nix Nix packages for mdbook documentation tools
lxutil.nix Nix package for WSL lxutil utility
flowey/flowey_lib_common/src/install_nix.rs New module to install Nix package manager (has security issues)
flowey/flowey_lib_common/src/run_cargo_build.rs Modified to wrap cargo in nix-shell (has command injection vulnerability)
flowey/flowey_lib_common/src/download_protoc.rs Added LocalPath support for using local protoc
flowey/flowey_lib_hvlite/src/resolve_openvmm_deps.rs Renamed from download_openvmm_deps, added LocalPath support
flowey/flowey_lib_hvlite/src/_jobs/cfg_versions.rs Added Local request type for specifying local dependency paths
flowey/flowey_lib_hvlite/src/init_openvmm_magicpath_protoc.rs Falls back to copy when hardlink fails (for Nix store)
flowey/flowey_lib_hvlite/src/init_cross_build.rs Added Nix distro support for cross-compilation
flowey/flowey_lib_hvlite/src/run_split_debug_info.rs Added Nix distro support for objcopy
flowey/flowey_lib_common/src/install_dist_pkg.rs Added Nix package management (warns instead of installing)
flowey/flowey_core/src/pipeline.rs Detects Nix environment via USING_NIX env var
flowey/flowey_core/src/node.rs Added Nix variant to FlowPlatformLinuxDistro enum
flowey/flowey_hvlite/src/pipelines/*.rs Updated to use Download vs Local requests
.github/workflows/*.yaml Generated workflow files with Nix installation steps
Cargo.toml Bumped rust-version from 1.91 to 1.91.1
Comments suppressed due to low confidence (1)

shell.nix:3

  • Security concern: Fetching from a mutable reference (master branch) without pinning to a specific commit introduces supply chain risk. An attacker who compromises the rust-overlay repository could inject malicious code that would be automatically pulled into builds. Consider pinning to a specific commit hash or release tag.

// Download and run the Nix installer script (single-user mode)
xshell::cmd!(
sh,
"sh -c 'curl --proto =https --tlsv1.2 -L https://nixos.org/nix/install | sh -s -- --no-daemon'"
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

The command syntax has a typo: --proto =https should be --proto '=https' (with quotes). The space before =https might cause curl to interpret it incorrectly.

Suggested change
"sh -c 'curl --proto =https --tlsv1.2 -L https://nixos.org/nix/install | sh -s -- --no-daemon'"
"sh -c 'curl --proto '=https' --tlsv1.2 -L https://nixos.org/nix/install | sh -s -- --no-daemon'"

Copilot uses AI. Check for mistakes.
Comment on lines +83 to +104
xshell::cmd!(
sh,
"sh -c 'curl --proto =https --tlsv1.2 -L https://nixos.org/nix/install | sh -s -- --no-daemon'"
)
.run()?;
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

Security concern: Piping curl output directly to sh without verification is risky. Consider adding checksum verification or using a specific version/commit of the installer script. This is especially important since the PR description indicates this is for CI use.

Copilot uses AI. Check for mistakes.
Comment on lines +18 to +20
glibc_2_39_52 = import (fetchTarball
"https://github.com/NixOS/nixpkgs/archive/ab7b6889ae9d484eed2876868209e33eb262511d.tar.gz")
{ };
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

The glibc_2_39_52 variable is defined but never used in this file. Consider removing it or documenting why it's needed for future use.

Copilot uses AI. Check for mistakes.
Comment on lines +63 to +65
if fs_err::hard_link(&src, &dst).is_err() {
fs_err::copy(&src, &dst)?;
}
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

The error from fs_err::hard_link is silently ignored (line 63) without logging, which could make debugging harder when the hard link fails for reasons other than cross-filesystem or read-only filesystem issues. Consider logging when falling back to copy, e.g., log::debug!("Hard link failed, falling back to copy: {}", e).

Copilot uses AI. Check for mistakes.
Comment on lines 297 to 311
// Build the full command string with environment variables
let env_prefix = with_env
.iter()
.map(|(k, v)| format!("{k}={v}"))
.collect::<Vec<_>>()
.join(" ");

let cargo_cmd = format!("{} {}", argv0, all_params.join(" "));
let full_cmd = if env_prefix.is_empty() {
cargo_cmd
} else {
format!("{env_prefix} {cargo_cmd}")
};

xshell::cmd!(sh, "nix-shell --pure --run {full_cmd}")
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

Security concern: Building a shell command string by concatenating environment variables and parameters (lines 298-309) without proper shell escaping can lead to command injection vulnerabilities. Environment variable values or parameters containing special shell characters (e.g., spaces, quotes, semicolons) could break out of the intended command structure. Use xshell's structured command building instead, or apply proper shell escaping to all interpolated values.

Copilot uses AI. Check for mistakes.
"sha256-wJeRZC6sd+tNSYHdyyN4Qj/sn5puT6R8eagFlHa6pP4=";

in stdenv.mkDerivation {
pname = "openvmm-deps";
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

The pname is set to "openvmm-deps" but this file is downloading UEFI firmware from mu_msvm. The package name should be something like "uefi-mu-msvm" to match the filename and content.

Copilot uses AI. Check for mistakes.
{ system, stdenv, fetchzip, is_dev ? false, is_cvm ? false, }:

let
version = if is_dev then "6.12.44.1" else "6.12.44.1";
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

The version assignment is redundant - both branches of the conditional set the same value "6.12.44.1". Either the versions should be different, or this can be simplified to version = "6.12.44.1";

Copilot uses AI. Check for mistakes.
hcl-dev = {
std = {
x64 = "sha256-Ow9piuc2IDR4RPISKY5EAQ5ykjitem4CXS9974lvwPE=";
arm64 = "";
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

Empty hash string for hcl-dev std arm64 (line 24) will cause Nix to fail when this configuration is used. If this combination is not supported, consider removing it from the hash table or adding a runtime check with a clear error message.

Copilot uses AI. Check for mistakes.
Comment on lines +18 to +20
glibc_2_39_52 = import (fetchTarball
"https://github.com/NixOS/nixpkgs/archive/ab7b6889ae9d484eed2876868209e33eb262511d.tar.gz")
{ };
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

fetchTarball is also used to import a nixpkgs snapshot without specifying a sha256, so the tarball is trusted solely based on HTTPS and GitHub serving the expected content. If the download is ever tampered with (e.g. supply‑chain compromise or MITM at the TLS/CA layer), your build environment could run modified code while Nix has no way to detect the corruption. Please change this to a fixed‑output fetch with an explicit hash (e.g. builtins.fetchTarball { url = "…"; sha256 = "…"; }) so Nix enforces content integrity.

Copilot uses AI. Check for mistakes.
Comment on lines +2 to +3
rust_overlay = import (builtins.fetchTarball
"https://github.com/oxalica/rust-overlay/archive/master.tar.gz");
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

builtins.fetchTarball is used to import the rust_overlay from the moving master branch without a pinned hash, so every shell evaluation can execute arbitrary new code from GitHub with only TLS as a safeguard. If that repository or the connection is compromised, an attacker could ship a malicious overlay that runs in your build environment and potentially tamper with build outputs. Please pin this dependency by fetching a specific tag/commit and providing an explicit sha256 (e.g. builtins.fetchTarball { url = "…"; sha256 = "…"; }) so the content is integrity-checked and immutable.

Copilot uses AI. Check for mistakes.
@justus-camp-microsoft justus-camp-microsoft requested a review from a team as a code owner December 11, 2025 23:51
@justus-camp-microsoft justus-camp-microsoft force-pushed the working_nix_build_igvm branch 4 times, most recently from 547a21e to 18f0e2f Compare December 12, 2025 00:49
@github-actions
Copy link

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