diff --git a/Cargo.lock b/Cargo.lock index a76b215..a06a5e7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -436,7 +436,7 @@ dependencies = [ [[package]] name = "da-core" -version = "0.1.3" +version = "0.1.4" dependencies = [ "argon2", "base32", @@ -461,7 +461,7 @@ dependencies = [ [[package]] name = "da-discord" -version = "0.1.3" +version = "0.1.4" dependencies = [ "axum", "da-core", @@ -478,7 +478,7 @@ dependencies = [ [[package]] name = "da-http-api" -version = "0.1.3" +version = "0.1.4" dependencies = [ "axum", "base64 0.22.1", @@ -500,7 +500,7 @@ dependencies = [ [[package]] name = "da-render" -version = "0.1.3" +version = "0.1.4" dependencies = [ "image", "resvg", @@ -512,7 +512,7 @@ dependencies = [ [[package]] name = "da-webui" -version = "0.1.3" +version = "0.1.4" [[package]] name = "data-url" @@ -542,7 +542,7 @@ dependencies = [ [[package]] name = "discord-authenticator" -version = "0.1.3" +version = "0.1.4" dependencies = [ "anyhow", "axum", diff --git a/Cargo.toml b/Cargo.toml index c0267f5..b82c47e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -10,7 +10,7 @@ members = [ ] [workspace.package] -version = "0.1.3" +version = "0.1.4" edition = "2021" rust-version = "1.90" license = "MIT OR Apache-2.0" diff --git a/README.md b/README.md index 64875a6..72a64c2 100644 --- a/README.md +++ b/README.md @@ -392,19 +392,36 @@ accounts are loaded from it, the damaged file is moved to `vault.json.corrupt-`, and a fresh `vault.json` is written. You will see one `ERROR` line saying so, then normal startup. -If it still refuses, both files are unreadable and there is nothing to recover -from automatically. Get the file out of the way and restart: +If it still refuses, the message names exactly which situation you are in, +because they need different responses: + +- **"there is no backup at …"** — no `.bak` was ever created, which means this + vault has not completed a single write since backups were introduced. Nothing + on disk is recoverable; the unreadable file is all there is. +- **"exists but could not be parsed either"** — both copies are gone. +- **"is empty as well"** — both are zero bytes. + +Either way, the service will not boot with an empty vault on its own, because +that looks identical to a vault you emptied on purpose. Two ways forward: ```bash +# 1. by hand cd /mnt/user/appdata/discord-authenticator mv vault.json "vault.json.broken-$(date +%s)" docker restart Discord-Authenticator ``` -The service will start with an empty vault, so re-import your accounts from -your own backups (`otpauth://` links or a Google Authenticator export). Keep -`vault.key` — without it, nothing in the old file can be decrypted even if the -bytes are recovered later. +Or set `DA_VAULT_RESET=1` in the Unraid template (Docker → the container → Add +another Path, Variable, Port or Label → Variable, Key `DA_VAULT_RESET`, Value +`1`) and restart. The service boots empty and moves the unreadable file to +`vault.json.corrupt-` before writing a fresh vault, so nothing is +overwritten. **Remove the variable afterwards** — it only has an effect on the +failure path, but leaving it set means the next genuinely-broken vault boots +empty instead of telling you about it. + +Then re-import your accounts from your own backups (`otpauth://` links or a +Google Authenticator export). Keep `vault.key` — without it, nothing in an old +vault file can be decrypted even if the bytes turn up later. **If you hit this, upgrading matters more than the workaround.** The cause was a rename being journalled ahead of the data it published, which is fixed by diff --git a/crates/core/src/vault.rs b/crates/core/src/vault.rs index 4b1989f..42fbe17 100644 --- a/crates/core/src/vault.rs +++ b/crates/core/src/vault.rs @@ -251,6 +251,8 @@ impl Vault { pub fn load(work_dir: &Path, key: VaultKey) -> Result { let path = work_dir.join("vault.json"); let backup_path = backup_of(&path); + // `load_accounts` decides on DA_VAULT_RESET itself, because that is + // where the description of what went wrong is built. let (accounts, recovered) = load_accounts(&path, &backup_path)?; tracing::info!(target: "da::vault", count = accounts.len(), "vault loaded"); @@ -790,6 +792,34 @@ impl Vault { } } +/// Has the operator explicitly asked to start with an empty vault? +/// +/// Set `DA_VAULT_RESET=1` to boot an unreadable vault as empty instead of +/// refusing. Checked only on the failure path, so it has no effect on a vault +/// that loads normally. +fn reset_requested() -> bool { + std::env::var("DA_VAULT_RESET") + .map(|v| looks_like_reset(&v)) + .unwrap_or(false) +} + +/// Split out so the accepted spellings can be tested without touching the +/// process environment. +fn looks_like_reset(value: &str) -> bool { + let v = value.trim(); + !v.is_empty() && v != "0" && !v.eq_ignore_ascii_case("false") +} + +/// Serializes the tests that write process environment variables. Rust runs +/// tests in parallel threads within one process, so an env var set by one test +/// is visible to every other test running at that moment. +#[cfg(test)] +fn env_guard() -> std::sync::MutexGuard<'static, ()> { + static LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + LOCK.lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) +} + /// `vault.json` → `vault.json.bak`. fn backup_of(path: &Path) -> PathBuf { let mut name = path @@ -806,8 +836,19 @@ fn backup_of(path: &Path) -> PathBuf { /// between "won't parse" and "unrecoverable", and the backup lives in it: /// whenever one exists and does parse, the honest move is to use it and say so /// loudly, keeping the damaged original for inspection. +/// +/// When *nothing* is recoverable the refusal has to say why precisely, because +/// the two cases need opposite responses from the operator. A backup that is +/// present and unreadable means both copies are gone and the accounts have to +/// be re-added; a backup that was never created (a container whose first write +/// never completed, so this feature has not run once) means the same file is +/// fine and there is simply nothing on disk to recover. The old wording said +/// "the backup could not be used either" for both, which sent the operator +/// looking for a file that did not exist. fn load_accounts(path: &Path, backup_path: &Path) -> Result<(Vec, bool)> { let text = match std::fs::read_to_string(path) { + // A missing vault is a first run, not a failure: there is nothing to + // recover and the service creates one on its first write. Ok(text) => text, Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok((Vec::new(), false)), Err(e) => return Err(e.into()), @@ -815,36 +856,116 @@ fn load_accounts(path: &Path, backup_path: &Path) -> Result<(Vec, bool) match serde_json::from_str::(&text) { Ok(file) => Ok((file.accounts, false)), - Err(primary_err) => match read_backup(backup_path) { - Some(accounts) => { - let quarantined = quarantine(path); - tracing::error!( + Err(primary_err) => { + match read_backup(backup_path) { + Some(accounts) => { + let quarantined = quarantine(path); + tracing::error!( + target: "da::vault", + path = %path.display(), + backup = %backup_path.display(), + recovered = accounts.len(), + quarantined = %quarantined.as_deref().map_or_else( + || "could not move aside".to_string(), + |p| p.display().to_string() + ), + "vault.json could not be parsed ({primary_err}); recovered the accounts \ + from the backup. The damaged file is kept alongside for inspection — \ + compare it against the backup if anything you added recently is missing." + ); + return Ok((accounts, true)); + } + None => {} + } + let damage = describe_damage(path, backup_path, &primary_err.to_string()); + + // The refusal is the right default, but it must not be a dead end: + // an operator who has accepted that the accounts are gone needs a + // way forward that does not involve editing files inside a + // container that will not start. The variable has to be set + // deliberately, so this can never look like the silent-empty + // fallback the refusal exists to prevent. + if reset_requested() { + // Move the unreadable file aside *before* the empty vault is + // written, or the caller's `persist` would overwrite it and + // then take the backup from the replacement — destroying the + // only copy of whatever might still have been salvageable. + let kept = quarantine(path); + tracing::warn!( target: "da::vault", - path = %path.display(), - backup = %backup_path.display(), - recovered = accounts.len(), - quarantined = %quarantined.as_deref().map_or_else( - || "could not move aside".to_string(), + preserved = %kept.as_deref().map_or_else( + || "NOTHING — it could not be moved aside and will be overwritten" + .to_string(), |p| p.display().to_string() ), - "vault.json could not be parsed ({primary_err}); recovered the accounts \ - from the backup. The damaged file is kept alongside for inspection — \ - compare it against the backup if anything you added recently is missing." + "DA_VAULT_RESET is set, so starting with an empty vault. {damage}" ); - Ok((accounts, true)) + return Ok((Vec::new(), true)); } - None => Err(Error::invalid(format!( - "{} exists but could not be parsed ({primary_err}), and the backup at {} \ - could not be used either. Refusing to start with an empty vault — move the \ - file aside to start fresh.", - path.display(), - backup_path.display() - ))), - }, + + Err(Error::invalid(format!( + "{damage} Refusing to start with an empty vault, because that would look \ + exactly like a vault you had emptied on purpose. If those accounts are gone, \ + either move the files aside to start fresh, or set DA_VAULT_RESET=1 to do the \ + same thing from the Unraid template — either way a copy of the unreadable file \ + is kept." + ))) + } } } -/// Parse the backup, or `None` if it is missing or not parseable as a vault. +/// Describe exactly what is on disk and why nothing could be loaded. +/// +/// The wording has to distinguish the cases, because they need opposite +/// responses. A backup that is present and unreadable means both copies are +/// gone and the accounts must be re-added. A backup that was never created — +/// a container that has not completed a single write since backups were +/// introduced — means there is simply nothing on disk to recover. The previous +/// message said "the backup could not be used either" for both, which sent an +/// operator hunting for a file that did not exist. +/// +/// Returns only the description. The decision to refuse, and the advice that +/// goes with it, belongs to the caller: a caller that has been told to reset is +/// not refusing anything, and must not say that it is. +fn describe_damage(path: &Path, backup_path: &Path, primary_err: &str) -> String { + let backup_note = match std::fs::metadata(backup_path) { + Ok(m) if m.len() == 0 => format!( + "the backup at {} is empty as well, so there is nothing to recover", + backup_path.display() + ), + Ok(_) => format!( + "the backup at {} exists but could not be parsed either", + backup_path.display() + ), + Err(_) => format!( + "there is no backup at {} — this vault has not been written since \ + backups were introduced, so none was ever created", + backup_path.display() + ), + }; + + // A quarantine file means a previous boot already recovered this vault and + // moved a damaged copy aside. That copy is the only other candidate left. + let quarantined: Vec = std::fs::read_dir(path.parent().unwrap_or(Path::new("."))) + .into_iter() + .flatten() + .flatten() + .map(|e| e.file_name().to_string_lossy().into_owned()) + .filter(|n| n.starts_with("vault.json.corrupt-")) + .collect(); + let quarantine_note = if quarantined.is_empty() { + String::new() + } else { + format!( + " Files kept from earlier recoveries that may be worth inspecting: {}.", + quarantined.join(", ") + ) + }; + + format!("vault.json could not be parsed ({primary_err}), and {backup_note}.{quarantine_note}") +} + +/// Parse the backup, or `None` if it is absent or not parseable as a vault. fn read_backup(backup_path: &Path) -> Option> { let text = std::fs::read_to_string(backup_path).ok()?; serde_json::from_str::(&text) @@ -1318,6 +1439,120 @@ mod tests { ); } + /// The refusal has to say which of the two situations the operator is in: + /// both copies gone, or a backup that was never created. The old wording + /// said "the backup could not be used either" for both, which sent someone + /// hunting for a file that did not exist. + #[test] + fn the_refusal_distinguishes_a_missing_backup_from_a_broken_one() { + let dir = tempdir().unwrap(); + std::fs::write(dir.path().join("vault.json"), b"").unwrap(); + + let missing = Vault::load(dir.path(), VaultKey::from_bytes(&[1u8; 32]).unwrap()) + .unwrap_err() + .to_string(); + // No .bak at all: this is the state a container is in when it has not + // completed a single write since backups were introduced. + assert!( + missing.contains("no backup at") && missing.contains("has not been written since"), + "a missing backup must be named as missing: {missing}" + ); + + std::fs::write(dir.path().join("vault.json.bak"), b"garbage").unwrap(); + let broken = Vault::load(dir.path(), VaultKey::from_bytes(&[1u8; 32]).unwrap()) + .unwrap_err() + .to_string(); + assert!( + broken.contains("exists but could not be parsed"), + "a backup that is present but unreadable must say so: {broken}" + ); + } + + #[test] + fn an_empty_backup_is_reported_as_empty_not_as_broken() { + let dir = tempdir().unwrap(); + std::fs::write(dir.path().join("vault.json"), b"").unwrap(); + std::fs::write(dir.path().join("vault.json.bak"), b"").unwrap(); + + let msg = Vault::load(dir.path(), VaultKey::from_bytes(&[1u8; 32]).unwrap()) + .unwrap_err() + .to_string(); + assert!( + msg.contains("is empty as well"), + "zero bytes is not the same as unparseable: {msg}" + ); + } + + #[test] + fn the_refusal_points_at_anything_kept_by_an_earlier_recovery() { + let dir = tempdir().unwrap(); + std::fs::write(dir.path().join("vault.json"), b"").unwrap(); + std::fs::write( + dir.path().join("vault.json.corrupt-1700000000"), + b"an earlier damaged copy", + ) + .unwrap(); + + let msg = Vault::load(dir.path(), VaultKey::from_bytes(&[1u8; 32]).unwrap()) + .unwrap_err() + .to_string(); + assert!( + msg.contains("vault.json.corrupt-1700000000"), + "a copy from an earlier recovery is the only other thing worth \ + looking at, so it should be named: {msg}" + ); + } + + #[test] + fn reset_is_only_requested_by_a_deliberate_value() { + assert!(looks_like_reset("1")); + assert!(looks_like_reset("yes")); + assert!(looks_like_reset(" true ")); + // Explicitly off, and the ways an unset variable shows up. + assert!(!looks_like_reset("0")); + assert!(!looks_like_reset("false")); + assert!(!looks_like_reset("FALSE")); + assert!(!looks_like_reset("")); + assert!(!looks_like_reset(" ")); + } + + /// The escape hatch itself. Serialized against the greeting test because + /// both write process environment variables, which are shared by every + /// test in the binary. + #[test] + fn da_vault_reset_boots_empty_and_preserves_the_unreadable_file() { + let _guard = env_guard(); + let dir = tempdir().unwrap(); + std::fs::write(dir.path().join("vault.json"), b"").unwrap(); + std::fs::write(dir.path().join("vault.json.bak"), b"").unwrap(); + + std::env::set_var("DA_VAULT_RESET", "1"); + let loaded = Vault::load(dir.path(), VaultKey::from_bytes(&[1u8; 32]).unwrap()); + std::env::remove_var("DA_VAULT_RESET"); + + let v = loaded.expect("DA_VAULT_RESET must get an operator past a dead vault"); + assert!(v.is_empty()); + + // The unreadable original must not have been silently overwritten — + // it is the only thing anyone could still try to salvage. + let kept: Vec<_> = std::fs::read_dir(dir.path()) + .unwrap() + .flatten() + .map(|e| e.file_name().to_string_lossy().into_owned()) + .filter(|n| n.starts_with("vault.json.corrupt-")) + .collect(); + assert_eq!(kept.len(), 1, "the original must be kept: {kept:?}"); + } + + #[test] + fn without_reset_a_dead_vault_still_refuses() { + let _guard = env_guard(); + std::env::remove_var("DA_VAULT_RESET"); + let dir = tempdir().unwrap(); + std::fs::write(dir.path().join("vault.json"), b"").unwrap(); + assert!(Vault::load(dir.path(), VaultKey::from_bytes(&[1u8; 32]).unwrap()).is_err()); + } + #[test] fn a_fresh_install_is_not_an_error() { let dir = tempdir().unwrap(); diff --git a/docs/releases/v0.1.4.md b/docs/releases/v0.1.4.md new file mode 100644 index 0000000..b58eca0 --- /dev/null +++ b/docs/releases/v0.1.4.md @@ -0,0 +1,52 @@ +## An accurate message, and a way out + +No functional change to how the vault is written or recovered. This fixes what +the service tells you when recovery is not possible, and adds an escape hatch +for the case it is not. + +### The message said the wrong thing + +When neither `vault.json` nor `vault.json.bak` could be loaded, the error read: + +> ...and the backup at /var/lib/discord-authenticator/vault.json.bak could not +> be used either. + +That is true for three quite different situations, and it reads as though a +backup exists. The case that actually bites is the one where **no backup was +ever created**: a vault that has not completed a single write since v0.1.3 +introduced backups has no `.bak` at all, so there is nothing on disk to recover +and no file to go looking for. The old wording sent you looking anyway. + +It now says which situation you are in: + +- `there is no backup at … — this vault has not been written since backups were + introduced, so none was ever created` +- `the backup at … exists but could not be parsed either` +- `the backup at … is empty as well, so there is nothing to recover` + +It also lists any `vault.json.corrupt-` files left by earlier +recoveries, since those are the only other thing worth inspecting. + +### DA_VAULT_RESET + +Refusing to start on an unreadable vault is still the default — booting empty +would look identical to a vault you had emptied on purpose, and the whole point +is that the service cannot tell the difference. But refusing must not be a dead +end, and having to edit files by hand inside a container that will not start is +a poor answer at 2am. + +Set `DA_VAULT_RESET=1` and the service boots with an empty vault instead. It +moves the unreadable file to `vault.json.corrupt-` *before* writing +the fresh one, so nothing is overwritten, and the log line names where it went. +Remove the variable afterwards: it has no effect on a vault that loads, but +leaving it set means the next genuinely-broken vault boots empty rather than +telling you about it. + +In the Unraid template: Docker → the container → **Add another Path, Variable, +Port or Label** → Variable, Key `DA_VAULT_RESET`, Value `1`. + +### Upgrading + +Nothing to do. Vault format unchanged, no accounts to re-import. If you are +currently stuck on the old message, this release does not recover the accounts +either — it just tells you why, and lets you start clean without leaving the UI.