diff --git a/crates/orbyn-client/src/lib.rs b/crates/orbyn-client/src/lib.rs index e2db9f8..cd6f22d 100644 --- a/crates/orbyn-client/src/lib.rs +++ b/crates/orbyn-client/src/lib.rs @@ -360,8 +360,9 @@ pub struct RefusedEnvelope { } /// The engine's state for the host to store. It holds zone keys: a host must -/// seal it before it leaves the process (`orbyn-wasm` does). -#[derive(Clone, Debug, Default, PartialEq, Serialize, Deserialize)] +/// seal it before it leaves the process (`orbyn-wasm` does). Its `Debug` prints the account, the +/// device and the number of zones, and never a key or a record. +#[derive(Clone, Default, PartialEq, Serialize, Deserialize)] pub struct Snapshot { pub account_id: String, pub device_id: String, @@ -386,7 +387,9 @@ pub struct Snapshot { pub zones: BTreeMap, } -#[derive(Clone, Debug, Default, PartialEq, Serialize, Deserialize)] +/// One zone of a [`Snapshot`]: its keys (base64url), its records and its upload state. Its +/// `Debug` prints counts only, never a key or a record. +#[derive(Clone, Default, PartialEq, Serialize, Deserialize)] pub struct ZoneSnapshot { /// Key id to key, both base64url. pub keys: BTreeMap, @@ -406,6 +409,25 @@ pub struct ZoneSnapshot { refetch_restarts: u8, } +impl std::fmt::Debug for Snapshot { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("Snapshot") + .field("account_id", &self.account_id) + .field("device_id", &self.device_id) + .field("zones", &self.zones.len()) + .finish_non_exhaustive() + } +} + +impl std::fmt::Debug for ZoneSnapshot { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("ZoneSnapshot") + .field("keys", &self.keys.len()) + .field("records", &self.records.len()) + .finish_non_exhaustive() + } +} + /// One device's sync engine. pub struct Engine { keys: DeviceKeys, diff --git a/crates/orbyn-client/tests/snapshot_debug.rs b/crates/orbyn-client/tests/snapshot_debug.rs new file mode 100644 index 0000000..a58501b --- /dev/null +++ b/crates/orbyn-client/tests/snapshot_debug.rs @@ -0,0 +1,97 @@ +//! A snapshot holds zone keys and records. Printing it with `{:?}` or `{:#?}` must never show +//! either, in any encoding (the app's threat model, control 4). + +use base64::Engine as _; +use orbyn_client::{DeviceKeys, Engine, Fields, Snapshot}; +use serde_json::json; + +const ZONE: &str = "inbox"; +const SECRET_NOTE: &str = "a private note that must not be printed"; + +/// Every encoding a key could be printed in. +fn encodings(key: &[u8]) -> Vec { + let hex: String = key.iter().map(|b| format!("{b:02x}")).collect(); + let upper = hex.to_uppercase(); + let std = base64::engine::general_purpose::STANDARD.encode(key); + let std_nopad = base64::engine::general_purpose::STANDARD_NO_PAD.encode(key); + let url = base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(key); + let url_pad = base64::engine::general_purpose::URL_SAFE.encode(key); + let decimals: Vec = key.iter().map(u8::to_string).collect(); + let array = format!("[{}]", decimals.join(", ")); + let pretty = format!("[\n {},\n]", decimals.join(",\n ")); + let compact = decimals.join(","); + vec![ + hex, upper, std, std_nopad, url, url_pad, array, pretty, compact, + ] +} + +fn snapshot_with_a_key_and_a_record() -> (Snapshot, Vec) { + let mut engine = Engine::new( + DeviceKeys::generate().expect("keys"), + "0a0a0a0a-0000-4000-8000-000000000001", + "0000000a-0000-4000-8000-00000000000a", + ) + .expect("engine"); + let key_id = engine.create_zone_key(ZONE).expect("zone key"); + let stamp = engine.tick(1_700_000_000_000); + let mut fields = Fields::default(); + fields.set("note", json!(SECRET_NOTE), stamp); + engine + .stage(ZONE, "person_settings", "person", &fields) + .expect("stage"); + let snapshot = engine.snapshot(); + // The key as the snapshot stores it (base64url), decoded to its bytes. + let stored = &snapshot.zones[ZONE].keys[&key_id]; + let bytes = base64::engine::general_purpose::URL_SAFE_NO_PAD + .decode(stored) + .expect("base64url"); + assert_eq!(bytes.len(), 32); + (snapshot, bytes) +} + +#[test] +fn debug_of_a_snapshot_never_shows_a_zone_key_or_a_record() { + let (snapshot, key) = snapshot_with_a_key_and_a_record(); + let zone = &snapshot.zones[ZONE]; + let printed = [ + format!("{snapshot:?}"), + format!("{snapshot:#?}"), + format!("{:?}", Some(&snapshot)), + format!("{:#?}", Some(snapshot.clone())), + format!("{zone:?}"), + format!("{zone:#?}"), + format!("{:?}", snapshot.zones), + format!("{:#?}", snapshot.zones), + ]; + for text in &printed { + for encoding in encodings(&key) { + assert!( + !text.contains(&encoding), + "a key shows as {encoding}: {text}" + ); + } + assert!(!text.contains(SECRET_NOTE), "a record shows: {text}"); + } + // It still says whose snapshot it is and how big. + let text = format!("{snapshot:?}"); + assert!( + text.contains("0a0a0a0a-0000-4000-8000-000000000001"), + "{text}" + ); + assert!( + text.contains("0000000a-0000-4000-8000-00000000000a"), + "{text}" + ); + assert!(text.contains("zones: 1"), "{text}"); +} + +#[test] +fn the_check_finds_a_key_when_one_is_printed() { + // A derived Debug of the stored map would print the key: the encodings above catch it. + let (snapshot, key) = snapshot_with_a_key_and_a_record(); + let leaked = format!("{:?}", snapshot.zones[ZONE].keys); + assert!( + encodings(&key).iter().any(|e| leaked.contains(e)), + "the encodings must match what a derive would print: {leaked}" + ); +} diff --git a/crates/orbyn-device-client/src/client.rs b/crates/orbyn-device-client/src/client.rs index 836323d..e951bf1 100644 --- a/crates/orbyn-device-client/src/client.rs +++ b/crates/orbyn-device-client/src/client.rs @@ -4,6 +4,7 @@ use crate::api::{DeviceApi, Method, Request, Response}; use crate::clock::Clock; use crate::error::DeviceError; +use crate::password::Password; use crate::status::SyncStatus; use crate::store::{KeyStore, StoredIdentity, StoredSession}; use base64::Engine as _; @@ -40,9 +41,6 @@ struct SecondFactorBody<'a> { recovery_code: Option<&'a str>, } -/// A password. Wiped when dropped. -pub type Password = Zeroizing; - /// What the host says about this device. #[derive(Clone, Debug)] pub struct ClientConfig { diff --git a/crates/orbyn-device-client/src/lib.rs b/crates/orbyn-device-client/src/lib.rs index fda006e..196d934 100644 --- a/crates/orbyn-device-client/src/lib.rs +++ b/crates/orbyn-device-client/src/lib.rs @@ -9,6 +9,7 @@ mod client; mod clock; mod error; mod notices; +mod password; mod signup; mod status; mod store; @@ -17,12 +18,13 @@ mod sync; pub use api::{DeviceApi, HttpApi, Method, Request, Response, is_loopback_literal}; pub use approvals::{ApproverApproval, ApproverState, JoinedApproval}; pub use client::{ - ClientConfig, DeviceClient, Password, SecondFactor, SecondFactorChallenge, SessionInfo, - SignInOutcome, SignUpRequest, + ClientConfig, DeviceClient, SecondFactor, SecondFactorChallenge, SessionInfo, SignInOutcome, + SignUpRequest, }; pub use clock::{Clock, SystemClock}; pub use error::{DeviceError, StoreError}; pub use notices::{Notice, NoticeConfig, Notices}; +pub use password::Password; pub use signup::PendingSignUp; pub use status::{SyncState, SyncStatus}; pub use store::{KeyStore, MemoryStore, StoredIdentity, StoredSession}; diff --git a/crates/orbyn-device-client/src/password.rs b/crates/orbyn-device-client/src/password.rs new file mode 100644 index 0000000..38ac779 --- /dev/null +++ b/crates/orbyn-device-client/src/password.rs @@ -0,0 +1,38 @@ +//! The password type. + +use zeroize::Zeroizing; + +/// A password. Wiped when dropped, and its `Debug` prints `Password(..)` whatever version of +/// `zeroize` a build resolves (older ones print the contents of a bare `Zeroizing`). +#[derive(Clone)] +pub struct Password(Zeroizing); + +impl Password { + pub fn new(text: impl Into) -> Self { + Self(Zeroizing::new(text.into())) + } + + pub fn as_str(&self) -> &str { + &self.0 + } + + pub fn as_bytes(&self) -> &[u8] { + self.0.as_bytes() + } + + pub fn is_empty(&self) -> bool { + self.0.is_empty() + } +} + +impl From for Password { + fn from(text: String) -> Self { + Self::new(text) + } +} + +impl std::fmt::Debug for Password { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str("Password(..)") + } +} diff --git a/crates/orbyn-device-client/tests/client.rs b/crates/orbyn-device-client/tests/client.rs index e37b638..bfa3bf2 100644 --- a/crates/orbyn-device-client/tests/client.rs +++ b/crates/orbyn-device-client/tests/client.rs @@ -6,7 +6,7 @@ mod support; use orbyn_contracts::{DeviceApp, DevicePlatform, DeviceState}; use orbyn_device_client::{ ClientConfig, Clock, DeviceApi, DeviceClient, DeviceError, HttpApi, KeyStore, MemoryStore, - Method, Request, Response, SecondFactor, SignInOutcome, SignUpRequest, StoreError, + Method, Password, Request, Response, SecondFactor, SignInOutcome, SignUpRequest, StoreError, StoredIdentity, }; use std::sync::Arc; @@ -42,8 +42,8 @@ fn client(engine: &FakeEngine, name: &str) -> Client { ) } -fn password(text: &str) -> Zeroizing { - Zeroizing::new(text.to_owned()) +fn password(text: &str) -> Password { + Password::new(text) } fn request(invite: Option<&str>) -> SignUpRequest { @@ -214,13 +214,19 @@ async fn the_second_factor_is_asked_of_a_new_device_and_skipped_by_a_trusted_sta "nothing is kept before the factor" ); assert_eq!( - b.complete_second_factor(&challenge, SecondFactor::Code(password("000000"))) - .await - .err(), + b.complete_second_factor( + &challenge, + SecondFactor::Code(Zeroizing::new("000000".to_owned())) + ) + .await + .err(), Some(DeviceError::SecondFactorFailed) ); let outcome = b - .complete_second_factor(&challenge, SecondFactor::Code(password("123456"))) + .complete_second_factor( + &challenge, + SecondFactor::Code(Zeroizing::new("123456".to_owned())), + ) .await .expect("the right code"); assert!( @@ -633,6 +639,26 @@ fn flaky_client( ) } +#[test] +fn debug_of_a_password_never_shows_the_text() { + let secret = "correct horse battery staple"; + let password = Password::new(secret); + for text in [ + format!("{password:?}"), + format!("{password:#?}"), + format!("{:?}", Some(&password)), + format!("{:?}", vec![password.clone()]), + ] { + assert!(!text.contains(secret), "{text}"); + assert!(!text.contains("horse"), "{text}"); + assert!(text.contains("Password(..)"), "{text}"); + } + // It still works as a password. + assert_eq!(password.as_str(), secret); + assert_eq!(password.as_bytes(), secret.as_bytes()); + assert!(!password.is_empty() && Password::from(String::new()).is_empty()); +} + #[tokio::test(flavor = "multi_thread")] async fn a_failed_save_after_the_code_is_spent_keeps_the_keys_and_retry_save_finishes() { let engine = FakeEngine::new(); diff --git a/crates/orbyn-device-client/tests/local_stack.rs b/crates/orbyn-device-client/tests/local_stack.rs index 74b5c5b..5caf97b 100644 --- a/crates/orbyn-device-client/tests/local_stack.rs +++ b/crates/orbyn-device-client/tests/local_stack.rs @@ -12,12 +12,11 @@ use orbyn_contracts::{DeviceApp, DevicePlatform, DeviceState}; use orbyn_device_client::{ - ClientConfig, DeviceClient, DeviceError, HttpApi, MemoryStore, SignInOutcome, SignUpRequest, - SystemClock, + ClientConfig, DeviceClient, DeviceError, HttpApi, MemoryStore, Password, SignInOutcome, + SignUpRequest, SystemClock, }; use std::process::Command; use std::time::Duration; -use zeroize::Zeroizing; fn script(args: &[&str]) -> String { let path = concat!(env!("CARGO_MANIFEST_DIR"), "/../../scripts/local.sh"); @@ -51,7 +50,7 @@ async fn sign_up_second_device_re_login_and_sign_out_everywhere() { eprintln!("skipped: set ORBYN_LOCAL_STACK=1 with the local stack up (scripts/local.sh up)"); return; } - let password = Zeroizing::new("correct horse battery staple".to_owned()); + let password = Password::new("correct horse battery staple"); let email = format!("e2e-{}@example.test", uuid::Uuid::new_v4().simple()); let invite = script(&["invite-code", "device-client"]); assert_eq!(invite.len(), 19, "an invite code: {invite:?}"); @@ -94,7 +93,7 @@ async fn sign_up_second_device_re_login_and_sign_out_everywhere() { let third = client("Third"); assert_eq!( third - .sign_in(&email, &Zeroizing::new("wrong horse".to_owned())) + .sign_in(&email, &Password::new("wrong horse")) .await .err(), Some(DeviceError::LoginFailed) diff --git a/crates/orbyn-device-client/tests/single_crate.rs b/crates/orbyn-device-client/tests/single_crate.rs index 3ec66d1..8c0b764 100644 --- a/crates/orbyn-device-client/tests/single_crate.rs +++ b/crates/orbyn-device-client/tests/single_crate.rs @@ -4,9 +4,9 @@ use orbyn_device_client::{ Applied, ClientConfig, ClientError, Device, DeviceApp, DeviceKeys, DeviceList, DevicePlatform, - DevicePublic, DeviceRemoval, DeviceState, Engine, Fields, KeyStore, Local, MemoryStore, Peer, - PendingApproval, Quarantined, RefusedEnvelope, RejectedFields, Snapshot, Stamp, StoredIdentity, - SyncReport, Trashed, Zeroizing, ZoneReport, + DevicePublic, DeviceRemoval, DeviceState, Engine, Fields, KeyStore, Local, MemoryStore, + Password, Peer, PendingApproval, Quarantined, RefusedEnvelope, RejectedFields, Snapshot, Stamp, + StoredIdentity, SyncReport, Trashed, Zeroizing, ZoneReport, }; /// A host's `ClientConfig` is written with the re-exported platform and app types. @@ -87,6 +87,12 @@ fn a_host_drives_the_engine_and_a_key_store_with_the_re_exported_types() { assert!(store.load().expect("load").is_some()); } +#[test] +fn a_host_builds_a_password_from_this_crate_alone() { + let password = Password::new("hunter2 but longer"); + assert!(!format!("{password:?}").contains("hunter2")); +} + #[test] fn this_file_never_names_another_engine_crate() { let source = include_str!("single_crate.rs"); diff --git a/crates/orbyn-server/tests/device_client.rs b/crates/orbyn-server/tests/device_client.rs index baf4a80..07fd4d1 100644 --- a/crates/orbyn-server/tests/device_client.rs +++ b/crates/orbyn-server/tests/device_client.rs @@ -9,7 +9,8 @@ use orbyn_core::accounts::{AuthSecrets, Limit, Limits}; use orbyn_core::{Accounts, Database}; use orbyn_device_client::{ ApproverState, ClientConfig, DeviceApi, DeviceClient, DeviceError, HttpApi, KeyStore, - MemoryStore, Notice, NoticeConfig, SignInOutcome, SignUpRequest, SyncState, SystemClock, + MemoryStore, Notice, NoticeConfig, Password, SignInOutcome, SignUpRequest, SyncState, + SystemClock, }; use orbyn_server::{AppState, Readiness, router}; use orbyn_store::{Hub, Store}; @@ -113,8 +114,8 @@ impl Server { type Client = DeviceClient; -fn password() -> Zeroizing { - Zeroizing::new(PASSWORD.to_owned()) +fn password() -> Password { + Password::new(PASSWORD) } fn email() -> String { diff --git a/docs/device-client.md b/docs/device-client.md index 9689b8b..09707d3 100644 --- a/docs/device-client.md +++ b/docs/device-client.md @@ -131,12 +131,16 @@ The app may not depend on or name `orbyn-contracts` (rule D6). Every type of ano | `orbyn-client` | `Engine`, `Snapshot`, `Applied`, `RejectedFields`, `RefusedEnvelope`, `ClientError`, `Peer`, `Local`, `Quarantined`, `Trashed` | `open_engine`, `sync_all`, `SyncReport` and the engine's own return types | | `orbyn-sync` (through `orbyn-client`) | `Fields`, `Stamp` | staging and reading records | | `orbyn-crypto` (through `orbyn-client`) | `DeviceKeys`, `DevicePublic` | `StoredIdentity::keys()`, device keys | -| `zeroize` | `Zeroizing` | `Password`, `StoredIdentity` | +| `zeroize` | `Zeroizing` | `StoredIdentity`, `SecondFactor` | `orbyn-client` re-exports, besides those, the other contract types its `Engine` methods take or return (`ZoneList`, `UploadChanges`, `UploadResult`, `RemoveDevice`, `DeleteZone`, `KeyEnvelope`, `NewKeyEnvelope`, `KeyRotation`, `ApprovalNonce`, `CreateApproval`, `ConfirmApproval`, `ProtectionChain`, `ProtectionLink`, `ProtectionLevel`, `SetProtection`, `Record`, `RecordChanged`, `ZoneChanges`). Left out on purpose: the planner's `Item` and `ItemError` (planner only); the request and response bodies this crate sends and reads internally (`RegisterFinish`, `EmailVerify` and the like) and `orbyn-auth-client`, which no public signature uses; `ApproverPairing` and `DevicePairing`, which `ApproverApproval` and `JoinedApproval` wrap. `DeviceError` carries only strings, numbers and a duration. `tests/single_crate.rs` here and `orbyn-client/tests/reexports.rs` use these types through the one crate, and a check fails if the first names another Engine crate. +### Debug prints no secret + +A type that can hold secret material has a `Debug` that prints none: `Snapshot` (account, device and the number of zones) and `ZoneSnapshot` (counts) are manual, as are `StoredIdentity`, `StoredSession`, `SecondFactorChallenge`, `PendingSignUp`, `SymmetricKey` and `RecoveryKey`; `DeviceKeys`, `Engine`, `ApproverPairing`, `DevicePairing`, `SecondFactor`, `ApproverApproval` and `JoinedApproval` have no `Debug` at all. `Password` is **its own type** (`Password::new(text)`, `as_str`, `as_bytes`, `is_empty`, `From`; wiped on drop) whose `Debug` prints `Password(..)` and nothing else, so it does not depend on the `zeroize` version a build resolves: `zeroize` 1.9 prints `Zeroizing { .. }` for a bare `Zeroizing`, but 1.8 derives `Debug` and prints the contents. A host must still not format a bare `Zeroizing` (the second-factor code inside `SecondFactor::Code` is one, and `SecondFactor` has no `Debug`). `orbyn-client/tests/snapshot_debug.rs` checks that `{:?}` and `{:#?}` of a snapshot show neither a zone key (hex, base64 or an array) nor a record, and `tests/client.rs` that a `Password` never shows its text. `Local`, `Trashed` and `Quarantined` show the record they cache by design: it is content, not key material. + ### Tests `orbyn-server/tests/device_client.rs` runs the client over a real loopback socket against the real server on Postgres: two devices, the zone key, a sealed record that the server holds only as ciphertext, a conflict merged both ways, the snapshot round trip, a pending device refused, the whole approval through the client API (digits compared, confirmed and finished), a deny from either side, wrong digits typed, a relabelled listing refused by the server's commitment check, a removal and the last device that stays, offline work kept and sent later, and the notices stream hearing another device's change and being rejected when the session is revoked. `tests/notices.rs` serves the stream by hand to check the sign-in message, the pong, the reconnect and the stop.