diff --git a/.claude/skills/sc-cli-reference/SKILL.md b/.claude/skills/sc-cli-reference/SKILL.md index 24c6f3fc..56dc1753 100644 --- a/.claude/skills/sc-cli-reference/SKILL.md +++ b/.claude/skills/sc-cli-reference/SKILL.md @@ -19,7 +19,7 @@ cargo run --bin sc -- demo --agents 4 --checkout # also materialize each workt # temp dir (the one disk-writing path), then clean up cargo run --bin sc -- import --repo # import a Git repo's HEAD bash demo/run_demo.sh # independent zero-residue proof -cargo run --bin sc -- keygen [--out ] # (default ~/.sc/identity; 0600 on unix only) v2 identity: one seed derives BOTH an X25519 +cargo run --bin sc -- keygen [--out ] # (default ~/.sc/identity; 0600 on unix, owner-only DACL on Windows) v2 identity: one seed derives BOTH an X25519 # encryption key and an Ed25519 signing key (P22); # a pre-P22 v1 (scl-sk-) identity still parses and # still encrypts, but cannot sign diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5d0869a3..a1723be8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -53,10 +53,11 @@ jobs: run: cargo doc --workspace --no-deps # Advisory (not a required context): proves the Windows build and the owner-only - # ACL on .sc/local-key (#214). Deliberately not `cargo test --workspace`; the - # wider suite still has unix-only assumptions. + # ACL on .sc/local-key, the `sc keygen` identity and the TLS serve key (#214, + # #281). Deliberately not `cargo test --workspace`; the wider suite still has + # unix-only assumptions. windows: - name: windows (check + key ACL tests) + name: windows (check + secret-file ACL tests) runs-on: windows-latest steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -68,8 +69,16 @@ jobs: run: cargo check --workspace --all-targets --keep-going - name: clippy run: cargo clippy --workspace --all-targets -- -D warnings - - name: key ACL and cache tests + - name: owner-only helper tests (scl-core) + run: cargo test -p scl-core --lib "owner_only::" + - name: local-key ACL and cache tests run: cargo test -p scl-repo --lib "cache::" + - name: TLS serve key ACL test + run: cargo test -p scl-repo --lib "minted_serve_tls_key" + - name: TLS identity injection tests (scl-tlsio) + run: cargo test -p scl-tlsio --lib "identity::" + - name: keygen identity ACL test + run: cargo test -p scl-cli --test keygen_overwrite desktop: name: desktop (typecheck + test + audit) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 551c55f9..2dad7e62 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -1120,7 +1120,7 @@ Trust is accept-new TOFU, the SSH `known_hosts` shape: the client pins `SC_HTTPS_FINGERPRINT` pre-pins for CI; `SC_HTTPS_STRICT=1` refuses unknown hosts). A pin mismatch always hard-fails. The server auto-mints a self-signed identity into -`.sc/serve-tls/` (key `0600`) unless PEM is supplied; `sc serve fingerprint` +`.sc/serve-tls/` (key `0600`; owner-only DACL on Windows, #281) unless PEM is supplied; `sc serve fingerprint` prints the pin an operator distributes out-of-band. Follow-ons: `sc tls list/forget/pin/path` manages the pin file (ADR-0053), and a client can opt into CA validation instead of pinning with `SC_HTTPS_CA_FILE=` or diff --git a/CLAUDE.md b/CLAUDE.md index fb85d5e7..0da660df 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -251,8 +251,11 @@ transport-adjacent. The rest, imperatively: transferred.** Cache saves are best-effort and ordered after ref moves; a cache failure may cause a spurious re-seal but never incorrectness. The key is owner-only on unix (`0600`) and Windows (single-ACE protected DACL, from the first byte; - `crates/repo/src/winacl.rs`); other targets, or a failed Windows ACL call, keep default - permissions and warn once (#214). + `crates/core/src/winacl.rs` behind `scl_core::owner_only`, the one creation primitive); other + targets, or a failed Windows ACL call, keep default permissions and warn once (#214). The + `sc keygen` identity and the TLS serve key use the same primitive (#281); `tlsio` is a leaf, so + `load_or_mint` takes the creator from `repo` (`load_or_mint_serve_identity`) and never calls + core. `windows-sys` and the sole Windows `unsafe` live in `scl-core` only. - **Revoke is standing-only.** A revoked recipient keeps decrypting ciphertext sealed before the revoke (they hold the old wraps); cryptographic cutover is `sc rewrap --rotate` at the tip (fresh key per protected blob; a plain diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index a66bd4dd..5d64f23c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -41,8 +41,9 @@ CI (`ci.yml`) runs `fmt --check`, `clippy -D warnings`, `cargo test --workspace --all-targets` (Linux and macOS) and `cargo doc` for the Rust workspace, plus a `desktop` job (`npm ci`, `npm audit --audit-level=high`, typecheck, test). An advisory `windows` job (`cargo check` and `clippy` for the workspace, plus -the `scl-repo` `cache::` tests, which verify the owner-only ACL on -`.sc/local-key`) is not a required check; keep it green when you touch +the `owner_only`, `cache::`, serve-key, `tlsio` identity and keygen tests, which +verify the owner-only ACL on `.sc/local-key`, the `sc keygen` identity and the TLS +serve key) is not a required check; keep it green when you touch Windows-specific code. Your PR must be green on all required jobs. Changes to `Cargo.toml` or `Cargo.lock` also run the RustSec `audit` workflow (it runs daily as well). CodeQL, Scorecard, the SBOM generation, the `vendor-provenance` check diff --git a/Cargo.lock b/Cargo.lock index fd8e669e..eb9eb05f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5369,6 +5369,7 @@ dependencies = [ "hex", "proptest", "thiserror 2.0.21", + "windows-sys 0.61.2", "zstd", ] @@ -5443,7 +5444,6 @@ dependencies = [ "serde", "thiserror 2.0.21", "toml 1.1.6+spec-1.1.0", - "windows-sys 0.61.2", ] [[package]] diff --git a/ROADMAP.md b/ROADMAP.md index 20148ee9..6824092b 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -690,8 +690,9 @@ across every phase. verified by the advisory `windows` CI job (#214). Windows keys created before that keep their inherited DACL and no longer warn; delete `.sc/local-key` to recreate it owner-only. `sc keygen` now refuses to - overwrite an existing identity on Windows too (#281 tracks owner-only - protection for the identity file and the TLS serve key). + overwrite an existing identity on Windows too. The `sc keygen` identity + file and the auto-minted TLS serve key get the same owner-only DACL through + one shared primitive, `scl_core::owner_only` (#281). ## Active @@ -833,12 +834,6 @@ smaller ones (protected-cache hygiene, theirs-side resolve carry, sparse and P28 review follow-ons, the port de-flake, the clippy allow-list) are recorded in `CLAUDE.md`, `ARCHITECTURE.md` and the ADRs. -- **Windows owner-only protection for the `sc keygen` identity file and the TLS - serve key (#281, follow-on to #214).** `.sc/local-key` gets an owner-only DACL; - the identity file (`sc keygen --out`, `0600` on unix only) and the serve-home - TLS key still get default permissions on non-unix. The helper lives in - `scl-repo` (`pub(crate)`), and `tlsio` is a leaf that must not depend on it, so - it moves to `core` or reaches `tlsio` as a callback. - **Remaining history-editing depth:** operation objects in the CAS (Jujutsu-deep upgrade to the file oplog), oplog entries for remote-tracking refs. diff --git a/crates/cli/src/main.rs b/crates/cli/src/main.rs index 286886ff..8f84dd5e 100644 --- a/crates/cli/src/main.rs +++ b/crates/cli/src/main.rs @@ -1082,7 +1082,25 @@ fn init_max_object_size(flag: Option<&str>) -> Result<()> { scl_core::set_max_object_size(n).map_err(|e| anyhow::anyhow!("{src}: {e}")) } +/// Windows gives the main thread a 1 MiB stack, which the clap parse and the +/// large command dispatch overflow in a debug build (`sc.exe` died with a stack +/// overflow before reaching any command). Run on a roomier thread there; other +/// platforms keep the main thread untouched. +#[cfg(windows)] fn main() -> Result<()> { + std::thread::Builder::new() + .stack_size(32 * 1024 * 1024) + .spawn(real_main)? + .join() + .unwrap_or_else(|panic| std::panic::resume_unwind(panic)) +} + +#[cfg(not(windows))] +fn main() -> Result<()> { + real_main() +} + +fn real_main() -> Result<()> { let cli = Cli::parse(); init_max_object_size(match &cli.cmd { Cmd::Serve { @@ -1669,8 +1687,8 @@ fn resolve_identity_path(flag: Option) -> PathBuf { /// `sc keygen`: generate a v2 identity — a single random seed from which both /// the X25519 encryption key and the Ed25519 signing key are deterministically /// derived (`scl_crypto::generate_identity_v2`). Only the seed (the `scl-id-` -/// line) is written to disk, 0600, and never echoed to stdout — it is the -/// whole private identity, encryption *and* signing, so it gets the same +/// line) is written to disk, owner-only (0600 / Windows DACL), and never +/// echoed to stdout — it is the whole private identity, encryption *and* signing, so it gets the same /// discipline the v1 private key always had (see the P5-scanner memory: an /// identity file is scanner-bait and must stay outside any working tree). /// Both PUBLIC halves are printed, with registration hints for both @@ -1699,17 +1717,14 @@ fn run_keygen(out: Option) -> Result<()> { .expect("generate_identity_v2 always derives a signing half"); let enc_pk = identity.enc.public(); let sig_pk = signing.public(); - // Create the key file 0600 *atomically* so the private key is never visible - // group/world-readable through the umask window between write and chmod. - // `create_new(true)` refuses to clobber an existing identity, on every - // platform. Windows has no owner-only protection for this file (#281). + // Create the key file owner-only *atomically* (unix 0600, Windows protected + // DACL) so the private key is never visible to other users through a + // window between write and chmod/ACL. `create_new` semantics refuse to + // clobber an existing identity, on every platform. If Windows cannot apply + // the ACL, the file gets default permissions and a one-time warning. { use std::io::Write; - let mut opts = std::fs::OpenOptions::new(); - opts.write(true).create_new(true); - #[cfg(unix)] - std::os::unix::fs::OpenOptionsExt::mode(&mut opts, 0o600); - let mut f = opts.open(&path).map_err(|e| { + let mut f = scl_core::owner_only::create_new_owner_only_or_warn(&path).map_err(|e| { if e.kind() == std::io::ErrorKind::AlreadyExists { anyhow::anyhow!( "identity already exists at {} (refusing to overwrite)", diff --git a/crates/cli/tests/keygen_overwrite.rs b/crates/cli/tests/keygen_overwrite.rs index 37a870e4..08bbaf52 100644 --- a/crates/cli/tests/keygen_overwrite.rs +++ b/crates/cli/tests/keygen_overwrite.rs @@ -31,3 +31,49 @@ fn keygen_refuses_to_overwrite_an_existing_identity() { std::fs::remove_dir_all(&dir).unwrap(); assert!(!dir.exists()); } + +/// Windows: the identity file is created with a single explicit, non-inherited +/// full-control ACE for the current user, read back with `icacls` (#281). On +/// unix it is 0600. +#[test] +fn keygen_identity_file_is_owner_only() { + let dir = std::env::temp_dir().join(format!("scl-cli-keygen-acl-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let id = dir.join("alice.id"); + let out = Command::new(env!("CARGO_BIN_EXE_sc")) + .args(["keygen", "--out", id.to_str().unwrap()]) + .current_dir(&dir) + .output() + .expect("sc runs"); + assert!(out.status.success(), "{out:?}"); + assert!( + !String::from_utf8_lossy(&out.stderr).contains("warning"), + "no fallback warning expected: {out:?}" + ); + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let mode = std::fs::metadata(&id).unwrap().permissions().mode(); + assert_eq!(mode & 0o777, 0o600); + } + #[cfg(windows)] + { + let out = Command::new("icacls").arg(&id).output().unwrap(); + assert!(out.status.success(), "icacls failed: {out:?}"); + let text = String::from_utf8_lossy(&out.stdout).to_string(); + let acl = text.replace(id.to_str().unwrap(), ""); + let aces: Vec<&str> = acl.lines().filter(|l| l.contains(":(")).collect(); + let user = std::env::var("USERNAME").unwrap().to_lowercase(); + assert_eq!(aces.len(), 1, "exactly one ACE expected:\n{text}"); + let ace = aces[0].trim().to_lowercase(); + assert!( + ace.contains(&format!("\\{user}:(")) && ace.contains("(f)") && !ace.contains("(i)"), + "ACE must be an explicit full-control grant to {user}:\n{text}" + ); + } + + std::fs::remove_dir_all(&dir).unwrap(); + assert!(!dir.exists()); +} diff --git a/crates/core/Cargo.toml b/crates/core/Cargo.toml index 8211b248..52ce30b6 100644 --- a/crates/core/Cargo.toml +++ b/crates/core/Cargo.toml @@ -11,6 +11,10 @@ thiserror = "2.0.18" hex = "0.4.3" zstd = "0.14.0" +# Owner-only DACL for secret files (#214, #281); the only unsafe is in src/winacl.rs. +[target.'cfg(windows)'.dependencies] +windows-sys = { version = "0.61", features = ["Win32_Foundation", "Win32_Security", "Win32_Security_Authorization", "Win32_Storage_FileSystem", "Win32_System_Threading"] } + [lints] workspace = true diff --git a/crates/core/src/lib.rs b/crates/core/src/lib.rs index fadf5822..ae2bae50 100644 --- a/crates/core/src/lib.rs +++ b/crates/core/src/lib.rs @@ -8,8 +8,11 @@ pub mod error; pub mod fsutil; pub mod id; pub mod object; +pub mod owner_only; pub mod pack; pub mod store; +#[cfg(windows)] +mod winacl; /// Default for the largest single object (decoded/decompressed canonical /// bytes) this system accepts from an untrusted source. The live value is diff --git a/crates/core/src/owner_only.rs b/crates/core/src/owner_only.rs new file mode 100644 index 00000000..ca86581d --- /dev/null +++ b/crates/core/src/owner_only.rs @@ -0,0 +1,209 @@ +//! Create a secret file readable by its owner only, from the first byte (#214, +//! #281). +//! +//! One implementation for every secret file `sc` writes: `.sc/local-key` +//! (`scl-repo`), the `sc keygen` identity (`scl-cli`) and the TLS serve key +//! (`scl-tlsio`, which receives [`create_new_owner_only_or_warn`] from its +//! caller because it may not depend on a workspace crate). +//! +//! * unix: `create_new` with mode `0o600`. +//! * Windows: a protected single-ACE DACL for the current user, applied +//! atomically by `CreateFileW` (`winacl.rs`, the only `unsafe`). +//! * other targets: [`io::ErrorKind::Unsupported`]. + +use std::fs::{File, OpenOptions}; +use std::io; +use std::path::Path; + +/// Create `path`, failing with `AlreadyExists` if present, readable and +/// writable by the current user only. Never falls back to wider permissions: +/// an error means no file was created. Callers that prefer a degraded file to +/// a failure use [`create_new_owner_only_or_warn`]. +pub fn create_new_owner_only(path: &Path) -> io::Result { + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o600) + .open(path) + } + #[cfg(windows)] + { + crate::winacl::create_new_owner_only(path) + } + #[cfg(not(any(unix, windows)))] + { + let _ = path; + Err(io::ErrorKind::Unsupported.into()) + } +} + +/// Like [`create_new_owner_only`], but where the owner-only create fails for a +/// reason other than `AlreadyExists` (Windows ACL call failed, or an +/// unsupported target), create the file with default permissions instead and +/// print a one-time warning naming it. The operation still proceeds (the key +/// must be written), but the operator is told the file is unprotected. On unix this is exactly +/// [`create_new_owner_only`]: there is nothing to degrade to. +pub fn create_new_owner_only_or_warn(path: &Path) -> io::Result { + #[cfg(unix)] + { + create_new_owner_only(path) + } + #[cfg(not(unix))] + { + with_default_fallback(create_new_owner_only(path), path, &WARNED) + } +} + +/// Once-per-process guard for the fallback warning. +#[cfg(not(unix))] +static WARNED: std::sync::atomic::AtomicBool = std::sync::atomic::AtomicBool::new(false); + +/// The fallback decision, separate from the platform call so unix tests can +/// drive it with an injected `restricted` result. `AlreadyExists` and success +/// pass through; any other error creates `path` with default permissions and +/// warns once via `warned`. +#[cfg(any(not(unix), test))] +fn with_default_fallback( + restricted: io::Result, + path: &Path, + warned: &std::sync::atomic::AtomicBool, +) -> io::Result { + match restricted { + Err(e) if e.kind() != io::ErrorKind::AlreadyExists => { + let f = OpenOptions::new().write(true).create_new(true).open(path)?; + if !warned.swap(true, std::sync::atomic::Ordering::Relaxed) { + eprintln!( + "warning: could not restrict {} to its owner ({e}); it was created \ + with default permissions and other local users may be able to read it", + path.display() + ); + } + Ok(f) + } + r => r, + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::io::Write; + use std::sync::atomic::{AtomicBool, Ordering::Relaxed}; + + fn tmp(tag: &str) -> std::path::PathBuf { + let d = std::env::temp_dir().join(format!("scl-core-owner-{tag}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&d); + std::fs::create_dir_all(&d).unwrap(); + d + } + + #[test] + fn creates_once_and_refuses_an_existing_file() { + let d = tmp("once"); + let p = d.join("secret"); + create_new_owner_only(&p) + .unwrap() + .write_all(b"s3cret") + .unwrap(); + assert_eq!(std::fs::read(&p).unwrap(), b"s3cret"); + let again = create_new_owner_only_or_warn(&p).unwrap_err(); + assert_eq!(again.kind(), io::ErrorKind::AlreadyExists); + assert_eq!(std::fs::read(&p).unwrap(), b"s3cret", "never clobbered"); + std::fs::remove_dir_all(&d).unwrap(); + assert!(!d.exists()); + } + + #[cfg(unix)] + #[test] + fn unix_mode_is_0600() { + use std::os::unix::fs::PermissionsExt; + let d = tmp("mode"); + let p = d.join("secret"); + create_new_owner_only_or_warn(&p).unwrap(); + let mode = std::fs::metadata(&p).unwrap().permissions().mode(); + assert_eq!(mode & 0o777, 0o600); + std::fs::remove_dir_all(&d).unwrap(); + } + + #[test] + fn failed_owner_only_create_falls_back_and_warns_once() { + let d = tmp("fallback"); + let warned = AtomicBool::new(false); + let denied = || Err(io::Error::from(io::ErrorKind::PermissionDenied)); + + let p = d.join("a"); + with_default_fallback(denied(), &p, &warned).unwrap(); + assert!(p.exists(), "fallback must still create the file"); + assert!(warned.load(Relaxed), "warned at creation"); + + // A concurrent creator's AlreadyExists is not a protection failure. + let warned = AtomicBool::new(false); + let exists = Err(io::Error::from(io::ErrorKind::AlreadyExists)); + let r = with_default_fallback(exists, &d.join("b"), &warned); + assert_eq!(r.unwrap_err().kind(), io::ErrorKind::AlreadyExists); + assert!(!d.join("b").exists() && !warned.load(Relaxed)); + + // If the default create fails too, that error surfaces unwarned. + let r = with_default_fallback(denied(), &p, &warned); + assert_eq!(r.unwrap_err().kind(), io::ErrorKind::AlreadyExists); + assert!(!warned.load(Relaxed)); + std::fs::remove_dir_all(&d).unwrap(); + } + + /// Windows: exactly one explicit, non-inherited full-control ACE for the + /// current user, read back with `icacls`. + #[cfg(windows)] + fn assert_owner_only_dacl(path: &Path) { + let out = std::process::Command::new("icacls") + .arg(path) + .output() + .unwrap(); + assert!(out.status.success(), "icacls failed: {out:?}"); + let text = String::from_utf8_lossy(&out.stdout).to_string(); + let acl = text.replace(path.to_str().unwrap(), ""); + let aces: Vec<&str> = acl.lines().filter(|l| l.contains(":(")).collect(); + let user = std::env::var("USERNAME").unwrap().to_lowercase(); + assert_eq!(aces.len(), 1, "exactly one ACE expected:\n{text}"); + let ace = aces[0].trim().to_lowercase(); + assert!( + ace.contains(&format!("\\{user}:(")) && ace.contains("(f)") && !ace.contains("(i)"), + "ACE must be an explicit full-control grant to {user}:\n{text}" + ); + } + + #[cfg(windows)] + #[test] + fn windows_dacl_is_owner_only_and_owner_can_use_it() { + let d = tmp("acl"); + let p = d.join("secret"); + create_new_owner_only(&p) + .expect("ACL path must work on the CI host") + .write_all(b"k") + .unwrap(); + assert_eq!(std::fs::read(&p).unwrap(), b"k", "owner can still read it"); + assert_owner_only_dacl(&p); + let again = create_new_owner_only(&p).unwrap_err(); + assert_eq!(again.kind(), io::ErrorKind::AlreadyExists); + std::fs::remove_dir_all(&d).unwrap(); + } + + /// Windows: a path past MAX_PATH still takes the ACL path (no fallback). + #[cfg(windows)] + #[test] + fn windows_acl_applies_to_a_path_over_260_chars() { + let top = tmp("long"); + let mut dir = top.clone(); + while dir.as_os_str().len() < 280 { + dir.push("d".repeat(40)); + } + std::fs::create_dir_all(&dir).unwrap(); + let p = dir.join("secret"); + assert!(p.as_os_str().len() > 260); + create_new_owner_only(&p).expect("long path must still get the ACL"); + assert!(p.exists()); + std::fs::remove_dir_all(&top).unwrap(); + } +} diff --git a/crates/repo/src/winacl.rs b/crates/core/src/winacl.rs similarity index 96% rename from crates/repo/src/winacl.rs rename to crates/core/src/winacl.rs index 8f038012..2cf47bdc 100644 --- a/crates/repo/src/winacl.rs +++ b/crates/core/src/winacl.rs @@ -1,6 +1,7 @@ -//! Windows owner-only file creation for `.sc/local-key` (#214). +//! Windows owner-only file creation (#214, #281): the `.sc/local-key`, the +//! `sc keygen` identity file and the TLS serve key. //! -//! The only Windows `unsafe` in `scl-repo` lives here, behind one safe function: +//! The only Windows `unsafe` in the workspace lives here, behind one safe function: //! [`create_new_owner_only`]. It builds a *protected* DACL holding a single //! ACE (`GENERIC_ALL` for the current user's SID) and hands it to `CreateFileW` //! as the file's security descriptor, so the file is owner-only from its first diff --git a/crates/repo/Cargo.toml b/crates/repo/Cargo.toml index aac1d326..d75f549c 100644 --- a/crates/repo/Cargo.toml +++ b/crates/repo/Cargo.toml @@ -19,9 +19,5 @@ scl-objio = { version = "0.1.0", path = "../objio" } hex = "0.4.3" blake3 = "1.8.5" -# Owner-only DACL for .sc/local-key (#214); the only unsafe is in src/winacl.rs. -[target.'cfg(windows)'.dependencies] -windows-sys = { version = "0.61", features = ["Win32_Foundation", "Win32_Security", "Win32_Security_Authorization", "Win32_Storage_FileSystem", "Win32_System_Threading"] } - [lints] workspace = true diff --git a/crates/repo/src/cache.rs b/crates/repo/src/cache.rs index c6960e56..70168acf 100644 --- a/crates/repo/src/cache.rs +++ b/crates/repo/src/cache.rs @@ -36,7 +36,7 @@ use crate::error::{Error, Result}; use crate::layout::Layout; /// Whether `.sc/local-key` can be created readable by its owner only: unix -/// (`mode(0o600)`) and Windows (owner-only DACL, `winacl`), both from the first +/// (`mode(0o600)`) and Windows (owner-only DACL, `scl_core::owner_only`), both from the first /// byte. Elsewhere, or after a Windows ACL failure, the key keeps default /// permissions and a warning is printed. Tests force `false` via /// `force_unrestricted_for_test`. @@ -142,12 +142,7 @@ fn write_key_0600(path: &Path, bytes: &[u8]) -> std::io::Result<()> { #[cfg(unix)] fn create_key_file(path: &Path) -> std::io::Result { - use std::os::unix::fs::OpenOptionsExt; - std::fs::OpenOptions::new() - .write(true) - .create_new(true) - .mode(0o600) - .open(path) + scl_core::owner_only::create_new_owner_only(path) } /// Windows: owner-only DACL from the first byte; if that cannot be applied, @@ -155,7 +150,7 @@ fn create_key_file(path: &Path) -> std::io::Result { #[cfg(windows)] fn create_key_file(path: &Path) -> std::io::Result { key_file_or_fallback( - crate::winacl::create_new_owner_only(path), + scl_core::owner_only::create_new_owner_only(path), || create_key_file_default(path), &ACL_FAILED, &WARNED, @@ -452,22 +447,7 @@ mod tests { "ACL path must not have fallen back" ); - let path = layout.local_key_path(); - let out = std::process::Command::new("icacls") - .arg(&path) - .output() - .unwrap(); - assert!(out.status.success(), "icacls failed: {out:?}"); - let text = String::from_utf8_lossy(&out.stdout).to_string(); - let acl = text.replace(path.to_str().unwrap(), ""); - let aces: Vec<&str> = acl.lines().filter(|l| l.contains(":(")).collect(); - let user = std::env::var("USERNAME").unwrap().to_lowercase(); - assert_eq!(aces.len(), 1, "exactly one ACE expected:\n{text}"); - let ace = aces[0].trim().to_lowercase(); - assert!( - ace.contains(&format!("\\{user}:(")) && ace.contains("(f)") && !ace.contains("(i)"), - "ACE must be an explicit full-control grant to {user}:\n{text}" - ); + crate::test_support::assert_owner_only_dacl(&layout.local_key_path()); cleanup(&layout); } diff --git a/crates/repo/src/http_transport.rs b/crates/repo/src/http_transport.rs index 885c1887..a6470087 100644 --- a/crates/repo/src/http_transport.rs +++ b/crates/repo/src/http_transport.rs @@ -978,13 +978,23 @@ fn resolve_tls( TlsMode::Off => return Ok(None), TlsMode::AutoMint => { require_repo_for_tls_identity(root)?; - scl_tlsio::load_or_mint(&crate::layout::Layout::at(root).serve_tls_dir())? + load_or_mint_serve_identity(&crate::layout::Layout::at(root).serve_tls_dir())? } TlsMode::Pem { cert, key } => scl_tlsio::load_pem(cert, key)?, }; Ok(Some(scl_tlsio::server_config(identity)?)) } +/// Load or mint the serve-TLS identity with its private key created owner-only +/// from the first byte (`scl_core::owner_only`; Windows DACL since #281). The +/// leaf `scl-tlsio` crate cannot depend on `scl-core`, so the creator is +/// injected here, the single place the key's permissions are decided. +pub(crate) fn load_or_mint_serve_identity( + dir: &std::path::Path, +) -> scl_tlsio::Result { + scl_tlsio::load_or_mint(dir, &scl_core::owner_only::create_new_owner_only_or_warn) +} + /// The serve-TLS identity lives in `/.sc/serve-tls/`, so minting one /// under a root that is not a repo would create a stray `.sc/` there (or fail /// obscurely). Refuse first, naming the fix. @@ -1005,7 +1015,7 @@ fn require_repo_for_tls_identity(root: &std::path::Path) -> Result<()> { /// server uses; no drift possible. pub fn serve_tls_fingerprint(root: &std::path::Path) -> Result { require_repo_for_tls_identity(root)?; - let id = scl_tlsio::load_or_mint(&crate::layout::Layout::at(root).serve_tls_dir())?; + let id = load_or_mint_serve_identity(&crate::layout::Layout::at(root).serve_tls_dir())?; Ok(scl_tlsio::fingerprint_hex(&id.spki_sha256)) } @@ -2845,9 +2855,32 @@ mod tests { // ── Task 4: TLS-wrapped server seam. ── + /// The auto-minted serve key is owner-only from its first byte: `0600` on + /// unix, a single explicit DACL ACE on Windows (#281). + #[test] + fn minted_serve_tls_key_is_owner_only() { + let root = std::env::temp_dir().join(format!("scl-serve-key-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&root); + let dir = root.join(".sc").join("serve-tls"); + let a = load_or_mint_serve_identity(&dir).unwrap(); + let b = load_or_mint_serve_identity(&dir).unwrap(); + assert_eq!(a.spki_sha256, b.spki_sha256, "second call loads"); + let key = dir.join("key.pem"); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let mode = std::fs::metadata(&key).unwrap().permissions().mode(); + assert_eq!(mode & 0o777, 0o600, "key.pem must be 0600"); + } + #[cfg(windows)] + crate::test_support::assert_owner_only_dacl(&key); + std::fs::remove_dir_all(&root).unwrap(); + assert!(!root.exists()); + } + /// Bind a TLS listener over a fresh repo; returns (addr, spki, join guard). fn spawn_tls_server(root: &std::path::Path) -> (String, [u8; 32]) { - let id = scl_tlsio::load_or_mint(&root.join(".sc").join("serve-tls")).unwrap(); + let id = load_or_mint_serve_identity(&root.join(".sc").join("serve-tls")).unwrap(); let spki = id.spki_sha256; let cfg = scl_tlsio::server_config(id).unwrap(); let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); diff --git a/crates/repo/src/lib.rs b/crates/repo/src/lib.rs index 94db0f8f..4dd9c4f0 100644 --- a/crates/repo/src/lib.rs +++ b/crates/repo/src/lib.rs @@ -67,8 +67,6 @@ pub mod transport; #[cfg(test)] mod user_file_safety; pub mod walfmt; -#[cfg(windows)] -mod winacl; pub mod wire; pub mod workspace; pub mod worktree; diff --git a/crates/repo/src/test_support.rs b/crates/repo/src/test_support.rs index f6ae71f1..438354a0 100644 --- a/crates/repo/src/test_support.rs +++ b/crates/repo/src/test_support.rs @@ -55,3 +55,24 @@ pub(crate) fn watchdog<'a>( } }); } + +/// Windows: assert `path` carries exactly one explicit, non-inherited +/// full-control ACE for the current user, read back with `icacls`. +#[cfg(windows)] +pub(crate) fn assert_owner_only_dacl(path: &std::path::Path) { + let out = std::process::Command::new("icacls") + .arg(path) + .output() + .unwrap(); + assert!(out.status.success(), "icacls failed: {out:?}"); + let text = String::from_utf8_lossy(&out.stdout).to_string(); + let acl = text.replace(path.to_str().unwrap(), ""); + let aces: Vec<&str> = acl.lines().filter(|l| l.contains(":(")).collect(); + let user = std::env::var("USERNAME").unwrap().to_lowercase(); + assert_eq!(aces.len(), 1, "exactly one ACE expected:\n{text}"); + let ace = aces[0].trim().to_lowercase(); + assert!( + ace.contains(&format!("\\{user}:(")) && ace.contains("(f)") && !ace.contains("(i)"), + "ACE must be an explicit full-control grant to {user}:\n{text}" + ); +} diff --git a/crates/tlsio/src/identity.rs b/crates/tlsio/src/identity.rs index 7f33e306..d93b2fc6 100644 --- a/crates/tlsio/src/identity.rs +++ b/crates/tlsio/src/identity.rs @@ -3,6 +3,8 @@ //! regenerate only when missing; the cert carries a far-future not_after so //! renewal never bites a pinned deployment (pins are on the SPKI anyway). +use std::fs::File; +use std::io::Write; use std::path::Path; use rustls::pki_types::{pem::PemObject, CertificateDer, PrivateKeyDer}; @@ -17,11 +19,18 @@ pub struct ServerIdentity { pub spki_sha256: [u8; 32], } -/// Load `dir/cert.pem` + `dir/key.pem`, minting both (key mode 0600) when -/// the CERT is absent. A dir with a cert but no key (or vice versa) is an +/// Load `dir/cert.pem` + `dir/key.pem`, minting both when the CERT is absent. +/// The private key is created through `create_key`, which must create a NEW +/// file (failing with `AlreadyExists` if present) readable by its owner only +/// from the first byte. This crate is a leaf that may not depend on a +/// workspace crate, so the owner-only primitive (unix `0600`, Windows DACL; +/// `scl_core::owner_only`) is injected by the caller rather than duplicated here. A dir with a cert but no key (or vice versa) is an /// error, not a silent re-mint — regenerating over half an identity would /// invalidate every client pin without the operator asking for it. -pub fn load_or_mint(dir: &Path) -> Result { +pub fn load_or_mint( + dir: &Path, + create_key: &dyn Fn(&Path) -> std::io::Result, +) -> Result { let cert_path = dir.join("cert.pem"); let key_path = dir.join("key.pem"); match (cert_path.exists(), key_path.exists()) { @@ -35,8 +44,11 @@ pub fn load_or_mint(dir: &Path) -> Result { let cert = params .self_signed(&key) .map_err(|e| Error::Mint(e.to_string()))?; + // Key first: its create-new is the arbiter between two racing + // minters, so the loser fails here before it can overwrite the + // winner's cert.pem with a cert for a key that lost the race. + create_key(&key_path)?.write_all(key.serialize_pem().as_bytes())?; std::fs::write(&cert_path, cert.pem())?; - write_key_0600(&key_path, key.serialize_pem().as_bytes())?; load_pem(&cert_path, &key_path) } _ => Err(Error::Mint(format!( @@ -65,27 +77,8 @@ pub fn load_pem(cert: &Path, key: &Path) -> Result { }) } -#[cfg(unix)] -fn write_key_0600(path: &Path, bytes: &[u8]) -> Result<()> { - use std::io::Write; - use std::os::unix::fs::OpenOptionsExt; - let mut f = std::fs::OpenOptions::new() - .write(true) - .create_new(true) - .mode(0o600) - .open(path)?; - f.write_all(bytes)?; - Ok(()) -} - -#[cfg(not(unix))] -fn write_key_0600(path: &Path, bytes: &[u8]) -> Result<()> { - std::fs::write(path, bytes)?; - Ok(()) -} - #[cfg(test)] -mod tests { +pub(crate) mod tests { use super::*; fn tmp(tag: &str) -> std::path::PathBuf { @@ -94,33 +87,53 @@ mod tests { d } + /// Stand-in for the caller's owner-only creator: this leaf crate only + /// checks that the key goes through it; the real one is tested in + /// `scl_core::owner_only` and via `scl-repo`'s serve identity. + pub(crate) fn plain_create(path: &Path) -> std::io::Result { + std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .open(path) + } + #[test] - fn mint_then_load_is_idempotent_and_key_is_0600() { + fn mint_then_load_is_idempotent_and_key_goes_through_the_creator() { let dir = tmp("mint"); - let a = load_or_mint(&dir).unwrap(); - let b = load_or_mint(&dir).unwrap(); + let calls = std::cell::RefCell::new(Vec::new()); + let spy = |p: &Path| { + calls.borrow_mut().push(p.to_path_buf()); + plain_create(p) + }; + let a = load_or_mint(&dir, &spy).unwrap(); + let b = load_or_mint(&dir, &spy).unwrap(); // Second call LOADS (same identity), never re-mints. assert_eq!(a.spki_sha256, b.spki_sha256); assert_eq!(a.certs, b.certs); - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - let mode = std::fs::metadata(dir.join("key.pem")) - .unwrap() - .permissions() - .mode(); - assert_eq!(mode & 0o777, 0o600, "key.pem must be 0600"); - } + assert_eq!(*calls.borrow(), vec![dir.join("key.pem")], "key only, once"); std::fs::remove_dir_all(&dir).unwrap(); assert!(!dir.exists()); } + #[test] + fn a_failing_creator_aborts_the_mint() { + let dir = tmp("deny"); + let deny = |_: &Path| Err(std::io::Error::from(std::io::ErrorKind::PermissionDenied)); + assert!(load_or_mint(&dir, &deny).is_err()); + assert!(!dir.join("key.pem").exists()); + assert!( + !dir.join("cert.pem").exists(), + "no half identity left behind" + ); + std::fs::remove_dir_all(&dir).unwrap(); + } + #[test] fn half_an_identity_errors_instead_of_silently_reminting() { let dir = tmp("half"); - load_or_mint(&dir).unwrap(); + load_or_mint(&dir, &plain_create).unwrap(); std::fs::remove_file(dir.join("key.pem")).unwrap(); - let err = load_or_mint(&dir).unwrap_err(); + let err = load_or_mint(&dir, &plain_create).unwrap_err(); assert!( err.to_string().contains("half a TLS identity"), "got: {err}" @@ -132,7 +145,7 @@ mod tests { #[test] fn load_pem_reads_a_minted_pair() { let dir = tmp("pem"); - let minted = load_or_mint(&dir).unwrap(); + let minted = load_or_mint(&dir, &plain_create).unwrap(); let loaded = load_pem(&dir.join("cert.pem"), &dir.join("key.pem")).unwrap(); assert_eq!(minted.spki_sha256, loaded.spki_sha256); std::fs::remove_dir_all(&dir).unwrap(); diff --git a/crates/tlsio/src/stream.rs b/crates/tlsio/src/stream.rs index f3c510b5..a7215799 100644 --- a/crates/tlsio/src/stream.rs +++ b/crates/tlsio/src/stream.rs @@ -299,7 +299,7 @@ mod tests { /// Loopback server: accept one connection, TLS-wrap it, echo one /// length-prefixed message back. Returns (addr, join handle, spki). fn echo_server(dir: &std::path::Path) -> (String, std::thread::JoinHandle<()>, [u8; 32]) { - let id = crate::load_or_mint(dir).unwrap(); + let id = crate::load_or_mint(dir, &crate::identity::tests::plain_create).unwrap(); let spki = id.spki_sha256; let cfg = server_config(id).unwrap(); let listener = TcpListener::bind("127.0.0.1:0").unwrap(); diff --git a/docs/THREAT-MODEL.md b/docs/THREAT-MODEL.md index e826c60a..bc4cdfa5 100644 --- a/docs/THREAT-MODEL.md +++ b/docs/THREAT-MODEL.md @@ -81,7 +81,8 @@ too, not just the src-control-side metadata. protected blob under a fresh key (ADR-0051), whereas a plain `sc rewrap` only replaces wrap lists and leaves the key a revoked holder already has. History keeps the old wraps and ciphertext regardless. -- **Local cache key is owner-only on unix and Windows (P33, #214):** the +- **Local cache key, `sc keygen` identity and TLS serve key are owner-only on + unix and Windows (P33, #214, #281):** the unchanged-detection cache is keyed by `.sc/local-key`, created owner-only from its first byte: mode `0600` on unix, a protected DACL with a single `GENERIC_ALL` ACE for the current user on Windows (no inherited entries; the @@ -90,19 +91,27 @@ too, not just the src-control-side metadata. them; delete `.sc/local-key` to have the next run recreate it owner-only (the cost is one round of spurious re-seals). The Windows DACL grants no explicit access to SYSTEM or Administrators, so backup and admin tooling can still take - ownership of the file, as root can read a `0600` file on unix. The `sc keygen` - identity file and the TLS serve key still have no Windows owner-only - protection (tracked in #281); keygen now refuses to overwrite an existing - identity on every platform. On any other target, or if the Windows - ACL call fails, the key and the cache files are written with inherited default - permissions, and `sc` prints a one-time stderr warning once a cache holds a - protected entry. The exposure then is small: checkout already writes protected - plaintext into the working tree with default permissions on every platform, and + ownership of the file, as root can read a `0600` file on unix. The same + creation primitive (`scl_core::owner_only`) makes the `sc keygen --out` + identity file and the auto-minted TLS serve key (`.sc/serve-tls/key.pem`) + owner-only from their first byte on both platforms (the `windows` CI job + reads each back with `icacls`); keygen refuses to overwrite an existing + identity on every platform. Windows identity files and serve keys created + before #281 keep their inherited DACL; restrict them yourself or regenerate + (a regenerated serve key changes the pinned fingerprint). A + `--tls-key` PEM supplied by the operator is theirs to protect. On any other + target, or if the Windows ACL call fails, the file is written with inherited + default permissions and `sc` prints a one-time stderr warning naming it + (for `.sc/local-key`, once a cache holds a protected entry). For `.sc/local-key` the exposure is small: checkout already + writes protected plaintext into the working tree with default permissions on every platform, and the cache holds tags only for files whose plaintext is on disk, so a reader of the key could read that plaintext directly. What remains is the tags of the last-committed version of a file edited since, and stale entries until the next save. A filesystem that ignores mode bits or ACLs (unix vfat, exFAT, some - network mounts; Windows FAT, exFAT, some SMB shares) is not detected. + network mounts; Windows FAT, exFAT, some SMB shares) is not detected. For an + identity file or TLS serve key the fallback is not small: it leaves the + unencrypted private key under inherited permissions, so other local users may + be able to read it. ## Private branches (`sc branch --private` / `sc branch publish`) — ADR-0044 diff --git a/docs/adr/0042-in-binary-tls-sc-https.md b/docs/adr/0042-in-binary-tls-sc-https.md index 730a9b80..32f3646a 100644 --- a/docs/adr/0042-in-binary-tls-sc-https.md +++ b/docs/adr/0042-in-binary-tls-sc-https.md @@ -107,9 +107,13 @@ Storage and knobs: `sc serve --http --tls [--tls-cert --tls-key ]`. Without PEM flags, the server auto-mints a self-signed identity into -`.sc/serve-tls/` (`cert.pem` + `key.pem`, key file `0600` — the key file IS -the server's identity, so its permissions matter the same way an SSH host key -does), regenerating only when the material is missing, `not_after` set to +`.sc/serve-tls/` (`cert.pem` + `key.pem`, key file `0600`, or a single-ACE +protected DACL on Windows since #281 — the key file IS the server's identity, so +its permissions matter the same way an SSH host key does; `tlsio` is a leaf and +receives the owner-only creator from `scl-repo` rather than depending on +`scl-core`; the key is created first, with `create_new` on every platform, so a race +between two first starts fails the loser before it writes a cert, instead of +pairing one process's cert with the other's key), regenerating only when the material is missing, `not_after` set to 2126 (a century out — self-signed + TOFU pinning makes expiry-driven rotation meaningless without a CA, so the cert's own validity window is not the trust mechanism; the pin is). `--tls` is refused together with `--stdio` (ssh diff --git a/docs/adr/0043-randomized-protected-encryption.md b/docs/adr/0043-randomized-protected-encryption.md index 4763cd2a..4ff086f2 100644 --- a/docs/adr/0043-randomized-protected-encryption.md +++ b/docs/adr/0043-randomized-protected-encryption.md @@ -103,7 +103,8 @@ file changed. Detection is a per-checkout stat cache (`crates/repo/src/cache.rs` world-readable between creation and permission-setting), created lazily, **never committed, never transferred**. On Windows (#214) it is created through `CreateFileW` with a protected DACL holding one `GENERIC_ALL` ACE for - the current user, also from the first byte (`crates/repo/src/winacl.rs`). Other + the current user, also from the first byte (`crates/core/src/winacl.rs`, behind `scl_core::owner_only`, which since #281 also + creates the `sc keygen` identity and the TLS serve key). Other targets, or a failed ACL call, keep default permissions with a one-time warning, because checkout already writes the same plaintext to the working tree with default permissions (see THREAT-MODEL.md). Windows keys created diff --git a/docs/security/third-party-inventory.md b/docs/security/third-party-inventory.md index 8e5655bf..d0922282 100644 --- a/docs/security/third-party-inventory.md +++ b/docs/security/third-party-inventory.md @@ -28,11 +28,11 @@ Architectural quarantine rules (which crate may use what) are in | Crate | Direct third-party deps | Role | |---|---|---| -| `scl-core` | blake3, hex, thiserror, zstd | Content addressing, object model | +| `scl-core` | blake3, hex, thiserror, zstd, windows-sys (Windows only) | Content addressing, object model; `windows-sys` gives secret files an owner-only DACL (`winacl.rs`, `owner_only`, #214, #281) | | `scl-vfs` | thiserror | In-RAM worktrees | | `scl-gitio` | gix, anyhow, thiserror | Git interop (gix quarantined here) | | `scl-crypto` | chacha20poly1305, x25519-dalek, ed25519-dalek, sha2, hkdf, rand_core, rand_chacha, zeroize, blake3, hex, thiserror | All cryptography (RustCrypto quarantined here) | -| `scl-repo` | blake3, hex, libc, regex, serde, thiserror, toml, windows-sys (Windows only) | Persistent store, merge, transports, policy; `windows-sys` gives `.sc/local-key` an owner-only DACL (`winacl.rs`, #214) | +| `scl-repo` | blake3, hex, libc, regex, serde, thiserror, toml | Persistent store, merge, transports, policy | | `scl-tlsio` | rustls, rustls-native-certs, rcgen, ring, rustls-pki-types, hex, thiserror | TLS (quarantined leaf); `rustls-native-certs` loads the platform roots for `SC_HTTPS_CA=system` (ADR-0054) | | `scl-objio` | aws-config, aws-sdk-s3, tokio, blake3, hex, thiserror | Object-store buckets for `sc+wal://` / `sc+s3://` (quarantined leaf; S3 SDK and its async runtime live only here, ADR-0046) | | `scl-cli` | clap, anyhow, serde, serde_json, toml, hex | The `sc` binary |