From f3d97e5877eda0aa873f60182f2335da22256375 Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Mon, 26 Oct 2026 16:00:57 +0000 Subject: [PATCH] tailor: validate and fingerprint signing credentials Include effective signing settings and role-specific credential contents in cache fingerprints, follow key symlinks, and reject invalid signing configuration. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6501a6e2-9ece-44f9-a91a-4d8e0c0e6d07 --- tools/tailor/crates/tailor-core/src/deps.rs | 31 ++- .../crates/tailor-core/src/fingerprint.rs | 132 +++++++++ .../crates/tailor-core/src/hashcache.rs | 2 +- .../crates/tailor-core/src/orchestrator.rs | 256 +++++++++++++++++- tools/tailor/docs/reference/image-yaml.md | 5 +- 5 files changed, 419 insertions(+), 7 deletions(-) diff --git a/tools/tailor/crates/tailor-core/src/deps.rs b/tools/tailor/crates/tailor-core/src/deps.rs index 6158ed1d8..1af437858 100644 --- a/tools/tailor/crates/tailor-core/src/deps.rs +++ b/tools/tailor/crates/tailor-core/src/deps.rs @@ -114,8 +114,11 @@ fn walk_dir( /// A per-file dependency hash: XXH3 over the file's relative path (rename sensitivity + machine /// portability) domain-separated from its content hash + size. The content hash comes from the shared -/// cache, so an unchanged file is not re-read. -fn hash_one( +/// cache, so an unchanged file is not re-read. Unlike [`hash_dependencies`]'s directory-walk dispatch, +/// this follows symlinks (via `fs::metadata`/`File::open` in [`hashcache::hash_file_cached`]) rather +/// than skipping them -- safe here because callers that know in advance they have exactly one +/// declared file (not a directory to walk) have no symlink-cycle risk to guard against. +pub(crate) fn hash_one( path: &Path, base_dir: &Path, cache_dir: Option<&Path>, @@ -126,6 +129,28 @@ fn hash_one( source, }) })?; + Ok(compose_hash(path, base_dir, &file)) +} + +/// Like [`hash_one`], but never consults the `(size, mtime)` cache -- always reads and hashes the +/// file fresh. Required for a symlinked declared path (e.g. a signing key/cert): the cache key is +/// scoped to the symlink's own path plus its *resolved target's* size/mtime, so rotating the symlink +/// to a different target file with coincidentally equal size and mtime would otherwise return the +/// previous target's stale cached hash. +pub(crate) fn hash_one_uncached( + path: &Path, + base_dir: &Path, +) -> Result<[u8; DEP_HASH_BYTES], CoreError> { + let file = hashcache::hash_file(path).map_err(|source| { + CoreError::Exec(ExecError::Io { + context: format!("failed to read dependency file `{}`", path.display()), + source, + }) + })?; + Ok(compose_hash(path, base_dir, &file)) +} + +fn compose_hash(path: &Path, base_dir: &Path, file: &hashcache::FileHash) -> [u8; DEP_HASH_BYTES] { let rel = path.strip_prefix(base_dir).unwrap_or(path); let rel_bytes = rel.to_string_lossy(); let mut hasher = Xxh3::new(); @@ -133,7 +158,7 @@ fn hash_one( hasher.update(rel_bytes.as_bytes()); hasher.update(&file.hash); hasher.update(&file.size.to_le_bytes()); - Ok(hasher.digest128().to_le_bytes()) + hasher.digest128().to_le_bytes() } #[cfg(test)] diff --git a/tools/tailor/crates/tailor-core/src/fingerprint.rs b/tools/tailor/crates/tailor-core/src/fingerprint.rs index d77e6aac2..e2f49ace5 100644 --- a/tools/tailor/crates/tailor-core/src/fingerprint.rs +++ b/tools/tailor/crates/tailor-core/src/fingerprint.rs @@ -39,6 +39,32 @@ pub struct FingerprintInputs<'a> { pub compression: Option, /// COSI compression level (`--cosi-compression-level`); a change rebuilds the COSI. pub cosi_compression_level: Option, + /// Effective signing profile's id (`meta/docs/2026-06-29-signing.md` §4); a change -- including + /// unsigned-to-signed or a profile swap -- rebuilds the cell. The reference fields below are + /// hashed separately (each under its own domain-separated tag, not concatenated into this or any + /// other single string) so that, say, a `:`-containing `certificate` value can never be + /// misattributed to `publish_ca_cert` or vice versa. + pub signing_profile_id: Option<&'a str>, + /// Effective signing profile's `backend`; a change (e.g. `keypair` -> `azure-key-vault` under the + /// same id) rebuilds the cell. + pub signing_backend: Option<&'a str>, + /// Effective signing profile's `vault` (azure-key-vault backend); a change rebuilds the cell. + pub signing_vault: Option<&'a str>, + /// Effective signing profile's `certificate` (azure-key-vault backend); a change rebuilds the + /// cell. + pub signing_certificate: Option<&'a str>, + /// Effective signing profile's `publishCaCert` path (local-test-ca backend); a change -- e.g. + /// redirecting where the enrollable CA certificate is written -- rebuilds the cell. + pub signing_publish_ca_cert: Option<&'a str>, + /// Content hash of the profile's local key file (XXH3-128, `deps.rs`, `keypair` backend only); a + /// change to the file *contents* alone (same declared path) still busts the fingerprint, unlike + /// the id or path strings above which only change when the config text itself changes. Kept + /// separate from `signing_cert_hash` (rather than merged into one sorted set) so swapping the + /// key and cert files changes the fingerprint instead of silently reusing a stale artifact. + pub signing_key_hash: Option<[u8; 16]>, + /// Content hash of the profile's local certificate file (XXH3-128, `deps.rs`, `keypair` backend + /// only); see `signing_key_hash`. + pub signing_cert_hash: Option<[u8; 16]>, } /// Compute the canonical fingerprint. Each field is domain-separated and length-prefixed so distinct @@ -100,6 +126,35 @@ pub fn fingerprint(inputs: &FingerprintInputs<'_>) -> Fingerprint { if let Some(level) = inputs.cosi_compression_level { field(&mut hasher, b"cosi-compression-level", &[level]); } + if let Some(id) = inputs.signing_profile_id { + field(&mut hasher, b"signing.profile.id", id.as_bytes()); + } + if let Some(backend) = inputs.signing_backend { + field(&mut hasher, b"signing.profile.backend", backend.as_bytes()); + } + if let Some(vault) = inputs.signing_vault { + field(&mut hasher, b"signing.profile.vault", vault.as_bytes()); + } + if let Some(certificate) = inputs.signing_certificate { + field( + &mut hasher, + b"signing.profile.certificate", + certificate.as_bytes(), + ); + } + if let Some(publish_ca_cert) = inputs.signing_publish_ca_cert { + field( + &mut hasher, + b"signing.profile.publish-ca-cert", + publish_ca_cert.as_bytes(), + ); + } + if let Some(hash) = inputs.signing_key_hash { + field(&mut hasher, b"signing.key", &hash); + } + if let Some(hash) = inputs.signing_cert_hash { + field(&mut hasher, b"signing.cert", &hash); + } Fingerprint(hasher.finalize().into()) } @@ -154,6 +209,13 @@ mod tests { extra_params: &[], compression: None, cosi_compression_level: None, + signing_profile_id: None, + signing_backend: None, + signing_vault: None, + signing_certificate: None, + signing_publish_ca_cert: None, + signing_key_hash: None, + signing_cert_hash: None, } } @@ -166,6 +228,76 @@ mod tests { assert_eq!(a, b); } + #[test] + fn fingerprint_changes_when_signing_profile_changes() { + let base = base(); + let cfg: Value = serde_yaml_ng::from_str("os:\n hostname: a\n").unwrap(); + let mut unsigned = inputs("cell", &cfg, &base); + let mut signed = inputs("cell", &cfg, &base); + signed.signing_profile_id = Some("release"); + assert_ne!(fingerprint(&unsigned), fingerprint(&signed)); + + unsigned.signing_profile_id = Some("release"); + let mut other_profile = inputs("cell", &cfg, &base); + other_profile.signing_profile_id = Some("dev"); + assert_ne!(fingerprint(&unsigned), fingerprint(&other_profile)); + } + + #[test] + fn fingerprint_changes_when_signing_profile_fields_are_reassigned_across_a_delimiter() { + // Regression: concatenating profile fields with a plain `:` was not collision-free -- + // `certificate: "ignored"` + `publish_ca_cert: "a:b"` produced the same joined string as + // `certificate: "ignored:a"` + `publish_ca_cert: "b"`. Each field must be hashed under its + // own domain-separated tag instead. + let base = base(); + let cfg: Value = serde_yaml_ng::from_str("os:\n hostname: a\n").unwrap(); + let mut a = inputs("cell", &cfg, &base); + a.signing_certificate = Some("ignored"); + a.signing_publish_ca_cert = Some("a:b"); + let mut b = inputs("cell", &cfg, &base); + b.signing_certificate = Some("ignored:a"); + b.signing_publish_ca_cert = Some("b"); + assert_ne!( + fingerprint(&a), + fingerprint(&b), + "reassigning bytes across the certificate/publish_ca_cert boundary must change the fingerprint" + ); + } + + #[test] + fn fingerprint_changes_when_signing_key_hash_changes() { + // A credential edit under the *same* profile id (e.g. rotating the keypair's PEM files in + // place) must still bust the fingerprint -- the id string alone doesn't change. + let base = base(); + let cfg: Value = serde_yaml_ng::from_str("os:\n hostname: a\n").unwrap(); + let mut a = inputs("cell", &cfg, &base); + a.signing_profile_id = Some("release:keypair::"); + a.signing_key_hash = Some([1; 16]); + a.signing_cert_hash = Some([2; 16]); + let mut b = inputs("cell", &cfg, &base); + b.signing_profile_id = Some("release:keypair::"); + b.signing_key_hash = Some([1; 16]); + b.signing_cert_hash = Some([9; 16]); + assert_ne!(fingerprint(&a), fingerprint(&b)); + } + + #[test] + fn fingerprint_changes_when_key_and_cert_hashes_are_swapped() { + // Swapping which file is the key and which is the cert must not be a no-op: merging both + // into one sorted hash set would make this pair indistinguishable from an unswapped one. + let base = base(); + let cfg: Value = serde_yaml_ng::from_str("os:\n hostname: a\n").unwrap(); + let mut a = inputs("cell", &cfg, &base); + a.signing_profile_id = Some("release:keypair::"); + a.signing_key_hash = Some([1; 16]); + a.signing_cert_hash = Some([2; 16]); + let mut swapped = inputs("cell", &cfg, &base); + swapped.signing_profile_id = Some("release:keypair::"); + swapped.signing_key_hash = Some([2; 16]); + swapped.signing_cert_hash = Some([1; 16]); + assert_ne!(fingerprint(&a), fingerprint(&swapped)); + } + #[test] fn config_change_changes_fingerprint() { let base = base(); diff --git a/tools/tailor/crates/tailor-core/src/hashcache.rs b/tools/tailor/crates/tailor-core/src/hashcache.rs index f038cbce5..23d019451 100644 --- a/tools/tailor/crates/tailor-core/src/hashcache.rs +++ b/tools/tailor/crates/tailor-core/src/hashcache.rs @@ -61,7 +61,7 @@ pub fn hash_file_cached(path: &Path, cache_dir: Option<&Path>) -> io::Result io::Result { +pub(crate) fn hash_file(path: &Path) -> io::Result { let mut file = File::open(path)?; let mut hasher = Xxh3::new(); let mut size = 0_u64; diff --git a/tools/tailor/crates/tailor-core/src/orchestrator.rs b/tools/tailor/crates/tailor-core/src/orchestrator.rs index be7a3ec1e..ecd242cb5 100644 --- a/tools/tailor/crates/tailor-core/src/orchestrator.rs +++ b/tools/tailor/crates/tailor-core/src/orchestrator.rs @@ -172,6 +172,15 @@ impl Orchestrator { hash_cache_dir, target.name(), )?; + let resolved_signing_profile = + tailor_config::resolve_signing(cell.signing.as_ref(), tool.signing.as_ref())?; + let signing_profile_descriptor_value = resolved_signing_profile + .as_ref() + .map(|(id, profile)| signing_profile_descriptor(id, profile)); + let (signing_key_hash, signing_cert_hash) = match &resolved_signing_profile { + Some((_, profile)) => signing_key_hashes(profile, &target.root, target.name())?, + None => (None, None), + }; let print = fingerprint(&FingerprintInputs { slug: cell.slug.as_ref(), toolchain_digest: &resolved_toolchain.ic_image_ref, @@ -185,6 +194,23 @@ impl Orchestrator { extra_params: &cell.extra_params, compression: cell.output.compression, cosi_compression_level: cell.output.cosi_compression_level, + signing_profile_id: signing_profile_descriptor_value + .as_ref() + .map(|d| d.id.as_str()), + signing_backend: signing_profile_descriptor_value + .as_ref() + .map(|d| d.backend.as_str()), + signing_vault: signing_profile_descriptor_value + .as_ref() + .map(|d| d.vault.as_str()), + signing_certificate: signing_profile_descriptor_value + .as_ref() + .map(|d| d.certificate.as_str()), + signing_publish_ca_cert: signing_profile_descriptor_value + .as_ref() + .map(|d| d.publish_ca_cert.as_str()), + signing_key_hash, + signing_cert_hash, }); let artifact = output_dir.join(published_artifact_name( cell.slug.as_ref(), @@ -390,6 +416,82 @@ fn resolved_tools_dir<'a>( .ok_or(CoreError::MissingResolvedToolsDir { reference: key }) } +/// Deterministic, domain-separated descriptors of a signing profile's build-affecting *reference* +/// settings -- everything except local key/cert file contents, which are content-hashed separately +/// (`signing_key_hashes`) since a path alone doesn't change when the file it names is edited in +/// place. Returned as separate owned fields -- each hashed under its own domain-separated tag in +/// `fingerprint()` -- rather than one concatenated string: joining fields with a plain `:` is not +/// collision-free (e.g. `certificate: "ignored"` + `publishCaCert: "a:b"` produces the same string +/// as `certificate: "ignored:a"` + `publishCaCert: "b"`). +struct SigningProfileDescriptor { + id: String, + backend: String, + vault: String, + certificate: String, + publish_ca_cert: String, +} + +fn signing_profile_descriptor( + id: &str, + profile: &tailor_config::SigningProfile, +) -> SigningProfileDescriptor { + SigningProfileDescriptor { + id: id.to_owned(), + backend: profile.backend.as_str().to_owned(), + vault: profile.vault.clone().unwrap_or_default(), + certificate: profile.certificate.clone().unwrap_or_default(), + publish_ca_cert: profile + .publish_ca_cert + .as_deref() + .map(Path::to_string_lossy) + .unwrap_or_default() + .into_owned(), + } +} + +/// Separate `(key_hash, cert_hash)` -- not a single merged-and-sorted set -- so the fingerprint +/// never confuses which file is the key and which is the certificate (see `signing_key_hashes`). +type SigningKeyHashes = (Option<[u8; 16]>, Option<[u8; 16]>); + +/// Content-hash a signing profile's local key and certificate files, keeping their roles distinct +/// (`keypair` backend only -- other backends ignore `key`/`cert` in `tailor_sign::build_signer`, +/// so stray values on a non-`keypair` profile are intentionally not hashed, and never required to +/// exist). Returned as `(key_hash, cert_hash)` rather than a single merged-and-sorted set: folding +/// both into one sorted set would make swapping the two files produce an identical fingerprint. +/// Uses [`deps::hash_one_uncached`] (not [`deps::hash_one`] or [`deps::hash_dependencies`]) so a +/// symlinked key/cert is both (a) hashed by its resolved target content -- the same file +/// `tailor_sign::build_signer` actually reads -- instead of being silently skipped the way a +/// directory-walk dependency would be, and (b) never served a stale cached hash: the `(size, mtime)` +/// cache's key is the symlink's own path plus the *resolved target's* size/mtime, so rotating the +/// symlink to a different target with a coincidentally equal size and mtime would otherwise return +/// the previous target's cached hash. +fn signing_key_hashes( + profile: &tailor_config::SigningProfile, + workspace_root: &Path, + image: &str, +) -> Result { + if profile.backend != tailor_config::SigningBackend::Keypair { + return Ok((None, None)); + } + let hash_one = |path: Option<&Path>| -> Result, CoreError> { + let Some(path) = path else { + return Ok(None); + }; + let absolute = tailor_config::absolutize(path, workspace_root); + if !absolute.exists() { + return Err(CoreError::MissingDependency { + image: image.to_owned(), + path: path.to_owned(), + }); + } + Ok(Some(deps::hash_one_uncached(&absolute, workspace_root)?)) + }; + Ok(( + hash_one(profile.key.as_deref())?, + hash_one(profile.cert.as_deref())?, + )) +} + fn resolve_tools_dir(target: &Target) -> Result, CoreError> { let Some(tools_dir) = &target.definition.tools_dir else { return Ok(None); @@ -942,7 +1044,7 @@ pub fn published_artifact_name( mod tests { use super::*; - use std::fs; + use std::{fs, path::PathBuf}; use indoc::indoc; use tempfile::TempDir; @@ -1756,4 +1858,156 @@ mod tests { "the locked base digest must win over the re-resolved tag" ); } + + #[test] + fn signing_key_hashes_follows_a_rotated_symlink() { + // Regression: a symlinked key/cert (e.g. a `current` symlink repointed at a rotated + // credential) must change the hash -- `hash_dependencies`'s directory-walk dispatch skips + // symlinks entirely (to avoid cycles), but `tailor_sign::build_signer` reads straight + // through them via `fs::read`, so skipping here would let a rotated credential silently + // reuse a stale signed artifact. + use std::os::unix::fs::symlink; + + let dir = TempDir::new().unwrap(); + let key_a = dir.path().join("key-a.pem"); + let key_b = dir.path().join("key-b.pem"); + fs::write(&key_a, b"key-version-a").unwrap(); + fs::write(&key_b, b"key-version-b").unwrap(); + let link = dir.path().join("current.key"); + symlink(&key_a, &link).unwrap(); + + let profile = tailor_config::SigningProfile { + backend: tailor_config::SigningBackend::Keypair, + key: Some(PathBuf::from("current.key")), + cert: None, + publish_ca_cert: None, + vault: None, + certificate: None, + }; + + let (before_key, _) = signing_key_hashes(&profile, dir.path(), "img").unwrap(); + assert!(before_key.is_some(), "a symlinked key must still be hashed"); + + // Rotate: repoint the symlink at the other key file, as a credential rotation would. + fs::remove_file(&link).unwrap(); + symlink(&key_b, &link).unwrap(); + + let (after_key, _) = signing_key_hashes(&profile, dir.path(), "img").unwrap(); + assert_ne!( + before_key, after_key, + "rotating the symlink target must change the hash" + ); + } + + #[test] + fn signing_key_hashes_detects_a_symlink_rotation_between_equal_size_and_mtime_targets() { + // Regression: the shared (size, mtime) hash cache's key for a single-file hash is the + // *declared path* plus the *resolved target's* size/mtime -- so if signing_key_hashes used + // the cache, rotating a symlink between two targets with an equal size and the same mtime + // would return the first target's stale cached hash. signing_key_hashes must always hash + // the resolved content fresh (never through the cache) so this can't happen. + use std::{ + os::unix::fs::symlink, + time::{Duration, SystemTime}, + }; + + let dir = TempDir::new().unwrap(); + let key_a = dir.path().join("key-a.pem"); + let key_b = dir.path().join("key-b.pem"); + fs::write(&key_a, b"AAAAAAAAAA").unwrap(); + fs::write(&key_b, b"BBBBBBBBBB").unwrap(); + assert_eq!( + fs::metadata(&key_a).unwrap().len(), + fs::metadata(&key_b).unwrap().len(), + "test fixture requires equal-size targets" + ); + let shared_mtime = SystemTime::UNIX_EPOCH + Duration::from_secs(1_700_000_000); + for path in [&key_a, &key_b] { + let file = fs::File::options().write(true).open(path).unwrap(); + file.set_modified(shared_mtime).unwrap(); + } + + let link = dir.path().join("current.key"); + symlink(&key_a, &link).unwrap(); + + let profile = tailor_config::SigningProfile { + backend: tailor_config::SigningBackend::Keypair, + key: Some(PathBuf::from("current.key")), + cert: None, + publish_ca_cert: None, + vault: None, + certificate: None, + }; + + let (before_key, _) = signing_key_hashes(&profile, dir.path(), "img").unwrap(); + + fs::remove_file(&link).unwrap(); + symlink(&key_b, &link).unwrap(); + let file = fs::File::options().write(true).open(&link).unwrap(); + file.set_modified(shared_mtime).unwrap(); + + let (after_key, _) = signing_key_hashes(&profile, dir.path(), "img").unwrap(); + assert_ne!( + before_key, after_key, + "a symlink rotation between equal-size/equal-mtime targets must still change the hash" + ); + } + + #[tokio::test] + async fn plan_errors_on_an_unknown_signing_profile_instead_of_building_unsigned() { + // Regression: `resolve_signing`'s `ConfigError` (unknown profile, missing default, + // malformed profile) must propagate, not be swallowed into `None` -- otherwise a typo'd + // signing reference silently produces an apparently-valid *unsigned* plan. + let (_tmp, target) = target_with(indoc! {" + name: solo + base: + oci: + uri: registry.example/base:edge + platform: linux/amd64 + signing: nope + config: + os: + hostname: solo + "}); + let tool: ToolConfig = serde_yaml_ng::from_str(indoc! {" + schemaVersion: 1 + toolchains: + default: ic + entries: + - name: ic + container: registry.example/imagecustomizer + version: 1.0.0 + signing: + default: release + profiles: + release: + backend: local-test-ca + "}) + .unwrap(); + let toolchains = resolved_toolchains(&tool); + let out = TempDir::new().unwrap(); + + let err = Orchestrator::new(FakeExecutor::default(), FakeResolver) + .plan( + &[target], + &[], + &tool, + &Lockfile::default(), + &toolchains, + &BTreeMap::new(), + &BuildSelection::from_selector(&Selector::default()), + out.path(), + None, + ) + .await + .unwrap_err(); + + assert!( + matches!( + err, + CoreError::Config(tailor_config::ConfigError::UnknownSigningProfile { .. }) + ), + "an unknown signing profile must be a config error, not a silently-unsigned plan: {err:?}" + ); + } } diff --git a/tools/tailor/docs/reference/image-yaml.md b/tools/tailor/docs/reference/image-yaml.md index 8b2c4e91a..bf90ba4de 100644 --- a/tools/tailor/docs/reference/image-yaml.md +++ b/tools/tailor/docs/reference/image-yaml.md @@ -269,12 +269,13 @@ entries whose kind is `image` add a build-order edge; each input `name` is uniqu outputs: - format: cosi cosiCompressionLevel: 6 - name: "${name}-${arch}" - format: vhd-fixed compression: zstd # tailor compresses the artifact → .vhd.zst ``` -`format` is required. `cosiCompressionLevel`, `compression`, and `name` are optional. +`format` is required. `cosiCompressionLevel` and `compression` are optional. The published +basename is always the cell's slug (`.`, clone-suffixed `_cloneN` under `--clones`) -- +there is no per-output name override. ### `compression`