diff --git a/.planning/phases/10-key-material-hardening/deferred-items.md b/.planning/phases/10-key-material-hardening/deferred-items.md new file mode 100644 index 00000000..023da507 --- /dev/null +++ b/.planning/phases/10-key-material-hardening/deferred-items.md @@ -0,0 +1,31 @@ +# Deferred items — Phase 10 + +Out-of-scope discoveries found while executing this phase. Logged, not fixed. + +## From 10-01 (KEY-01 / F-01) + +**Flaky test: `credentials::operations::tests::test_list_credentials_no_filter`** + +- Discovered: 2026-08-02, during the post-plan full-suite run (`cargo test -p archipelago`). +- Symptom: `called Result::unwrap() on an Err value: UTF-8 credentials / invalid utf-8 sequence + of 1 bytes from index 3`. +- Root cause (read, not fixed): `credentials/store.rs:29` sniffs the FIRST BYTE of the stored + blob for `[` or `{` to distinguish a plaintext-JSON legacy store from the encrypted binary + one. When the encrypted ciphertext happens to begin with `0x5B` or `0x7B` — about a 1-in-128 + chance per run — the encrypted store is misread as plaintext and `String::from_utf8` fails. + This is a real bug in the migration sniffing, not just a test problem: a real node whose + credential ciphertext starts with one of those bytes cannot load its credentials. +- Why deferred: unrelated to KEY-01, different subsystem, untouched by this plan + (`git status` shows `credentials/` unmodified). Fixing it means adding a format marker or + version header to the store, which is an envelope change. +- Suggested fix: prepend an explicit magic/version byte on write and branch on that, keeping the + first-byte sniff only as the legacy fallback. +- **RESOLVED 2026-08-02** — fixed along the suggested lines, with one improvement. Writes are now + prefixed with a fixed `ARCHYCRED1` marker, which cannot collide with a random nonce. Legacy + unmarked files are detected by *successful AEAD decryption* rather than by a byte sniff: a + Poly1305 tag that verifies under the node key is a cryptographic discriminator (~2^-128 false + positive), strictly stronger than the structural sniff the fallback would have kept. Plaintext + JSON stays the last resort, and an undecodable file now errors instead of silently becoming an + empty store that the next save would overwrite. Legacy files upgrade on write, never on read. + Regression tests drive the collision deterministically via an explicit nonce (`0x5B`/`0x7B`) + instead of waiting on the 1-in-128 draw. diff --git a/core/archipelago/src/credentials/store.rs b/core/archipelago/src/credentials/store.rs index 0363506e..0d66dba0 100644 --- a/core/archipelago/src/credentials/store.rs +++ b/core/archipelago/src/credentials/store.rs @@ -18,6 +18,17 @@ fn store_path(data_dir: &Path) -> std::path::PathBuf { data_dir.join(CREDENTIALS_DIR).join("credentials.json") } +/// Magic prefix marking an encrypted credential store written by this version +/// onward. +/// +/// Historically the on-disk format was detected by sniffing the first byte for +/// `[`/`{`. Encrypted blobs begin with a *random* 12-byte nonce, so roughly 2 in +/// 256 (~1 in 128) saves produced a valid encrypted file whose first byte was +/// `[` (0x5B) or `{` (0x7B); those files were misread as plaintext JSON and +/// failed to load forever after. A fixed multi-byte marker cannot collide with a +/// random nonce, so detection is now exact rather than probabilistic. +const ENCRYPTED_MAGIC: &[u8] = b"ARCHYCRED1"; + pub async fn load_credentials(data_dir: &Path) -> Result { ensure_dir(data_dir).await?; let path = store_path(data_dir); @@ -25,27 +36,68 @@ pub async fn load_credentials(data_dir: &Path) -> Result { return Ok(CredentialStore::default()); } let raw = fs::read(&path).await.context("Reading credentials")?; - // Detect plaintext JSON (migration path) vs encrypted binary - if raw.first().is_some_and(|b| *b == b'[' || *b == b'{') { - let data = String::from_utf8(raw).context("UTF-8 credentials")?; - return serde_json::from_str(&data).context("Parsing credentials"); + decode_credentials(data_dir, &raw).await +} + +/// Decode any of the three on-disk credential formats that exist in the fleet. +/// +/// Detection order, and why each step is unambiguous: +/// +/// 1. **Current format** — `MAGIC ‖ nonce ‖ ciphertext`. The magic is a fixed +/// 10-byte literal, so this test has no false positives and no false +/// negatives. +/// 2. **Legacy encrypted** (no magic) — `nonce ‖ ciphertext`. Detected by +/// *successful AEAD decryption*, not by byte shape. ChaCha20-Poly1305 is +/// authenticated: a successful `decrypt` means the 16-byte Poly1305 tag +/// verified under the node key, which a non-ciphertext file passes only with +/// probability ~2^-128. This is a cryptographic discriminator, strictly +/// stronger than any structural sniff. +/// 3. **Legacy plaintext JSON** (original migration path) — reached only when +/// the bytes did not authenticate, then parsed strictly as a whole document. +/// +/// If none match we return an error rather than a default store, so a +/// transiently unreadable file is never silently replaced with empty +/// credentials on the next save (CLAUDE.md: migrations never destroy data). +async fn decode_credentials(data_dir: &Path, raw: &[u8]) -> Result { + // 1. Current format: explicit marker. + if let Some(body) = raw.strip_prefix(ENCRYPTED_MAGIC) { + let key = load_encryption_key(data_dir).await?; + let plaintext = decrypt_credentials(body, &key)?; + return serde_json::from_slice(&plaintext).context("Parsing decrypted credentials"); } - // Encrypted: decrypt using node key - let key = load_encryption_key(data_dir).await?; - let plaintext = decrypt_credentials(&raw, &key)?; - serde_json::from_slice(&plaintext).context("Parsing decrypted credentials") + + // 2. Legacy encrypted, unmarked. The node key may legitimately be absent on + // a node that only ever wrote plaintext, so a key-load failure falls + // through to the plaintext path instead of aborting. + if let Ok(key) = load_encryption_key(data_dir).await { + if let Ok(plaintext) = decrypt_credentials(raw, &key) { + return serde_json::from_slice(&plaintext) + .context("Parsing decrypted credentials (legacy unmarked)"); + } + } + + // 3. Legacy plaintext JSON migration path. + serde_json::from_slice(raw).context( + "Credentials file is not magic-prefixed encrypted data, does not authenticate \ + as a legacy encrypted blob, and is not valid plaintext JSON — refusing to \ + treat it as empty", + ) } pub async fn save_credentials(data_dir: &Path, store: &CredentialStore) -> Result<()> { ensure_dir(data_dir).await?; let path = store_path(data_dir); let data = serde_json::to_vec(store)?; - // Encrypt using node key + // Encrypt using node key. Always written in the current, magic-prefixed + // format — this is how legacy files are opportunistically upgraded: they are + // read in whatever format they are on disk, and the next save re-emits them + // marked. Nothing is ever rewritten from a read path. let key = load_encryption_key(data_dir).await?; let encrypted = encrypt_credentials(&data, &key)?; - fs::write(&path, encrypted) - .await - .context("Writing credentials") + let mut output = Vec::with_capacity(ENCRYPTED_MAGIC.len() + encrypted.len()); + output.extend_from_slice(ENCRYPTED_MAGIC); + output.extend_from_slice(&encrypted); + fs::write(&path, output).await.context("Writing credentials") } /// Derive a 32-byte encryption key from the node's identity key via SHA-256. @@ -65,8 +117,22 @@ async fn load_encryption_key(data_dir: &Path) -> Result<[u8; 32]> { } fn encrypt_credentials(data: &[u8], key: &[u8; 32]) -> Result> { - use chacha20poly1305::aead::{Aead, KeyInit}; let nonce_bytes: [u8; 12] = rand::random(); + encrypt_credentials_with_nonce(data, key, nonce_bytes) +} + +/// Encrypt with a caller-supplied nonce, returning `nonce ‖ ciphertext` (no +/// magic prefix — `save_credentials` adds that). +/// +/// Split out from [`encrypt_credentials`] so tests can construct a blob whose +/// first byte is a specific value and exercise format detection deterministically +/// instead of waiting on a 1-in-128 random draw. +fn encrypt_credentials_with_nonce( + data: &[u8], + key: &[u8; 32], + nonce_bytes: [u8; 12], +) -> Result> { + use chacha20poly1305::aead::{Aead, KeyInit}; let cipher = chacha20poly1305::ChaCha20Poly1305::new_from_slice(key) .map_err(|e| anyhow::anyhow!("Cipher init: {}", e))?; let ciphertext = cipher @@ -101,6 +167,46 @@ fn decrypt_credentials(data: &[u8], key: &[u8; 32]) -> Result> { #[cfg(test)] mod tests { use super::*; + use super::super::types::{CredentialProof, CredentialSubject, VerifiableCredential}; + + /// Tempdir with a deterministic `identity/node_key` so the encryption key + /// can be derived. Never a real key. + fn test_dir_with_node_key() -> tempfile::TempDir { + let dir = tempfile::tempdir().unwrap(); + let identity_dir = dir.path().join("identity"); + std::fs::create_dir_all(&identity_dir).unwrap(); + std::fs::write(identity_dir.join("node_key"), [0xAB; 32]).unwrap(); + std::fs::create_dir_all(dir.path().join(CREDENTIALS_DIR)).unwrap(); + dir + } + + fn sample_store(marker: &str) -> CredentialStore { + CredentialStore { + credentials: vec![VerifiableCredential { + context: vec!["https://www.w3.org/ns/credentials/v2".to_string()], + id: format!("urn:uuid:{marker}"), + credential_type: vec![ + "VerifiableCredential".to_string(), + "NodeOperator".to_string(), + ], + issuer: "did:key:issuer".to_string(), + credential_subject: CredentialSubject { + id: "did:key:subject".to_string(), + claims: serde_json::json!({"role": "admin"}), + }, + issuance_date: "2026-01-01T00:00:00Z".to_string(), + expiration_date: None, + proof: CredentialProof { + proof_type: "Ed25519Signature2020".to_string(), + created: "2026-01-01T00:00:00Z".to_string(), + verification_method: "did:key:issuer#key-1".to_string(), + proof_purpose: "assertionMethod".to_string(), + proof_value: "sig".to_string(), + }, + credential_status: None, + }], + } + } #[tokio::test] async fn test_load_credentials_returns_empty_when_no_file() { @@ -109,4 +215,153 @@ mod tests { assert!(store.credentials.is_empty()); assert!(dir.path().join(CREDENTIALS_DIR).exists()); } + + /// Regression: an encrypted blob whose random nonce happens to start with + /// `[` (0x5B) or `{` (0x7B) used to be misdetected as plaintext JSON, so + /// `String::from_utf8` failed and the store became permanently unreadable. + /// ~1 in 128 saves hit this. Both colliding bytes are exercised here + /// deterministically via an explicit nonce. + #[tokio::test] + async fn test_legacy_encrypted_blob_with_json_first_byte_still_loads() { + for (first_byte, label) in [(0x5Bu8, "open-bracket"), (0x7Bu8, "open-brace")] { + let dir = test_dir_with_node_key(); + let key = load_encryption_key(dir.path()).await.unwrap(); + + // Force the collision: nonce[0] is exactly the byte the old sniffer + // treated as "this file is plaintext JSON". + let mut nonce = [0u8; 12]; + nonce[0] = first_byte; + let plaintext = serde_json::to_vec(&sample_store(label)).unwrap(); + let blob = encrypt_credentials_with_nonce(&plaintext, &key, nonce).unwrap(); + assert_eq!(blob[0], first_byte, "test must actually trigger the collision"); + + // Written WITHOUT magic: this is the legacy on-disk population. + std::fs::write(store_path(dir.path()), &blob).unwrap(); + + let loaded = load_credentials(dir.path()) + .await + .unwrap_or_else(|e| panic!("{label}: colliding blob failed to load: {e:#}")); + assert_eq!(loaded.credentials.len(), 1, "{label}"); + assert_eq!(loaded.credentials[0].id, format!("urn:uuid:{label}")); + } + } + + /// Same collision, but in the current magic-prefixed format. + #[tokio::test] + async fn test_new_format_with_colliding_nonce_loads() { + for first_byte in [0x5Bu8, 0x7Bu8] { + let dir = test_dir_with_node_key(); + let key = load_encryption_key(dir.path()).await.unwrap(); + let mut nonce = [0u8; 12]; + nonce[0] = first_byte; + let plaintext = serde_json::to_vec(&sample_store("magic")).unwrap(); + let body = encrypt_credentials_with_nonce(&plaintext, &key, nonce).unwrap(); + let mut blob = ENCRYPTED_MAGIC.to_vec(); + blob.extend_from_slice(&body); + std::fs::write(store_path(dir.path()), &blob).unwrap(); + + let loaded = load_credentials(dir.path()).await.unwrap(); + assert_eq!(loaded.credentials[0].id, "urn:uuid:magic"); + } + } + + /// Population 1: legacy plaintext JSON from the original migration path. + #[tokio::test] + async fn test_legacy_plaintext_json_still_loads() { + let dir = test_dir_with_node_key(); + let json = serde_json::to_vec(&sample_store("plaintext")).unwrap(); + assert_eq!(json[0], b'{'); + std::fs::write(store_path(dir.path()), &json).unwrap(); + + let loaded = load_credentials(dir.path()).await.unwrap(); + assert_eq!(loaded.credentials[0].id, "urn:uuid:plaintext"); + } + + /// Legacy plaintext must still load on a node that has no node key at all + /// (pre-onboarding), where the encrypted path cannot even derive a key. + #[tokio::test] + async fn test_legacy_plaintext_json_loads_without_node_key() { + let dir = tempfile::tempdir().unwrap(); + std::fs::create_dir_all(dir.path().join(CREDENTIALS_DIR)).unwrap(); + let json = serde_json::to_vec(&sample_store("nokey")).unwrap(); + std::fs::write(store_path(dir.path()), &json).unwrap(); + + let loaded = load_credentials(dir.path()).await.unwrap(); + assert_eq!(loaded.credentials[0].id, "urn:uuid:nokey"); + } + + /// Population 2: legacy encrypted with an ordinary (non-colliding) nonce. + #[tokio::test] + async fn test_legacy_encrypted_without_magic_still_loads() { + let dir = test_dir_with_node_key(); + let key = load_encryption_key(dir.path()).await.unwrap(); + let plaintext = serde_json::to_vec(&sample_store("legacy-enc")).unwrap(); + let blob = encrypt_credentials_with_nonce(&plaintext, &key, [0x01; 12]).unwrap(); + std::fs::write(store_path(dir.path()), &blob).unwrap(); + + let loaded = load_credentials(dir.path()).await.unwrap(); + assert_eq!(loaded.credentials[0].id, "urn:uuid:legacy-enc"); + } + + /// Population 3: current format round-trips and is actually marked on disk. + #[tokio::test] + async fn test_new_format_roundtrip_and_is_magic_prefixed() { + let dir = test_dir_with_node_key(); + save_credentials(dir.path(), &sample_store("current")) + .await + .unwrap(); + + let on_disk = std::fs::read(store_path(dir.path())).unwrap(); + assert!(on_disk.starts_with(ENCRYPTED_MAGIC), "save must mark the file"); + // Still genuinely encrypted, not plaintext. + assert!(!on_disk.windows(4).any(|w| w == b"did:")); + + let loaded = load_credentials(dir.path()).await.unwrap(); + assert_eq!(loaded.credentials[0].id, "urn:uuid:current"); + } + + /// Opportunistic upgrade happens on write, never on read: reading a legacy + /// file must leave it byte-identical; the next save re-emits it marked. + #[tokio::test] + async fn test_legacy_file_upgraded_on_write_not_on_read() { + let dir = test_dir_with_node_key(); + let json = serde_json::to_vec(&sample_store("upgrade")).unwrap(); + std::fs::write(store_path(dir.path()), &json).unwrap(); + + let loaded = load_credentials(dir.path()).await.unwrap(); + // Read path must not have rewritten anything. + let after_read = std::fs::read(store_path(dir.path())).unwrap(); + assert_eq!(after_read, json, "read path must not rewrite the file"); + + save_credentials(dir.path(), &loaded).await.unwrap(); + let after_write = std::fs::read(store_path(dir.path())).unwrap(); + assert!(after_write.starts_with(ENCRYPTED_MAGIC)); + + let reloaded = load_credentials(dir.path()).await.unwrap(); + assert_eq!(reloaded.credentials[0].id, "urn:uuid:upgrade"); + } + + /// An unreadable file must surface an error, never a silent empty store — + /// otherwise the next save would overwrite recoverable user data. + #[tokio::test] + async fn test_undecodable_file_errors_instead_of_returning_empty() { + let dir = test_dir_with_node_key(); + std::fs::write(store_path(dir.path()), b"\x00\x01\x02 not json, not ours").unwrap(); + assert!(load_credentials(dir.path()).await.is_err()); + } + + /// A magic-prefixed file that fails authentication (tampered / wrong key) + /// must error rather than fall through to another format. + #[tokio::test] + async fn test_tampered_magic_file_errors() { + let dir = test_dir_with_node_key(); + save_credentials(dir.path(), &sample_store("tamper")) + .await + .unwrap(); + let mut blob = std::fs::read(store_path(dir.path())).unwrap(); + *blob.last_mut().unwrap() ^= 0x01; + std::fs::write(store_path(dir.path()), &blob).unwrap(); + + assert!(load_credentials(dir.path()).await.is_err()); + } }