diff --git a/crates/trusted-server-adapter-fastly/src/ec_kv.rs b/crates/trusted-server-adapter-fastly/src/ec_kv.rs index 229c6c2d3..031a867ec 100644 --- a/crates/trusted-server-adapter-fastly/src/ec_kv.rs +++ b/crates/trusted-server-adapter-fastly/src/ec_kv.rs @@ -9,6 +9,7 @@ use fastly::kv_store::{InsertMode, KVStore}; use trusted_server_core::ec::kv_backend::{ EcKvLookup, EcKvStore, EcKvWrite, EcKvWriteMode, EcKvWriteOutcome, }; +use trusted_server_core::ec::log_id; use trusted_server_core::error::TrustedServerError; /// Fastly KV Store backend for the EC identity graph. @@ -56,7 +57,7 @@ impl EcKvStore for FastlyEcKvStore { return Err( Report::new(err).change_context(TrustedServerError::KvStore { store_name: self.store_name.clone(), - message: format!("Failed to read key '{key}'"), + message: format!("Failed to read key '{}'", log_id(key),), }), ); } @@ -98,7 +99,7 @@ impl EcKvStore for FastlyEcKvStore { Err(err) => Err( Report::new(err).change_context(TrustedServerError::KvStore { store_name: self.store_name.clone(), - message: format!("Failed to write entry for key '{key}'"), + message: format!("Failed to write entry for key '{}'", log_id(key)), }), ), } @@ -117,10 +118,7 @@ impl EcKvStore for FastlyEcKvStore { .execute() .change_context(TrustedServerError::KvStore { store_name: self.store_name.clone(), - message: format!( - "Failed to list keys with prefix '{}'", - prefix.get(..8).unwrap_or(prefix), - ), + message: format!("Failed to list keys with prefix '{}'", log_id(prefix),), })?; #[allow(clippy::cast_possible_truncation)] @@ -134,7 +132,180 @@ impl EcKvStore for FastlyEcKvStore { .delete(key) .change_context(TrustedServerError::KvStore { store_name: self.store_name.clone(), - message: format!("Failed to delete key '{key}'"), + message: format!("Failed to delete key '{}'", log_id(key)), }) } } + +#[cfg(test)] +mod tests { + use std::time::Duration; + + use super::*; + + /// KV store declared for the local simulator in `fastly.toml`. + const TEST_STORE: &str = "ec_identity_store"; + + /// Entry metadata. Opaque to the backend, which only round-trips bytes. + const METADATA: &str = "entry-metadata"; + + fn store() -> FastlyEcKvStore { + FastlyEcKvStore::new(TEST_STORE) + } + + fn write(key: &str, body: &str, mode: EcKvWriteMode) -> EcKvWriteOutcome { + store() + .insert( + key, + EcKvWrite { + body, + metadata: METADATA, + ttl: Duration::from_secs(60), + mode, + }, + ) + .expect("should reach the store") + } + + #[test] + fn opening_a_store_this_service_does_not_have_is_an_error() { + let error = FastlyEcKvStore::new("no_such_store") + .lookup("any-key") + .expect_err("should not resolve against a store that is not linked"); + + assert!( + matches!( + error.current_context(), + TrustedServerError::KvStore { store_name, .. } if store_name == "no_such_store" + ), + "should name the store it could not open: {error:?}" + ); + } + + #[test] + fn a_missing_key_is_absent_rather_than_an_error() { + let absent = format!("{}.ABC123", "1".repeat(64)); + + assert!( + store() + .lookup(&absent) + .expect("should reach the store") + .is_none(), + "a key the store does not hold is absent, not a failure" + ); + } + + #[test] + fn an_entry_round_trips_through_insert_lookup_and_delete() { + let key = format!("{}.ABC123", "2".repeat(64)); + let backend = store(); + + assert_eq!( + write(&key, "entry-body-1", EcKvWriteMode::Overwrite), + EcKvWriteOutcome::Written, + "should write the entry" + ); + + let found = backend + .lookup(&key) + .expect("should reach the store") + .expect("should hold the entry just written"); + assert_eq!(found.body, b"entry-body-1", "should read back the body"); + assert_eq!( + found.metadata.as_deref(), + Some(METADATA.as_bytes()), + "should read back the metadata" + ); + + backend.delete(&key).expect("should delete the entry"); + assert!( + backend + .lookup(&key) + .expect("should reach the store") + .is_none(), + "a deleted key is absent" + ); + } + + #[test] + fn add_mode_refuses_a_key_that_already_exists() { + let key = format!("{}.ABC123", "3".repeat(64)); + let backend = store(); + + assert_eq!( + write(&key, "entry-body-1", EcKvWriteMode::Add), + EcKvWriteOutcome::Written, + "should create a key nothing holds" + ); + assert_eq!( + write(&key, "entry-body-2", EcKvWriteMode::Add), + EcKvWriteOutcome::PreconditionFailed, + "a precondition failure is control flow, not an error" + ); + + backend.delete(&key).expect("should delete the entry"); + } + + #[test] + fn a_generation_mismatch_is_reported_as_a_precondition_failure() { + let key = format!("{}.ABC123", "4".repeat(64)); + let backend = store(); + + write(&key, "entry-body-1", EcKvWriteMode::Overwrite); + let generation = backend + .lookup(&key) + .expect("should reach the store") + .expect("should hold the entry just written") + .generation; + + assert_eq!( + write( + &key, + "entry-body-2", + EcKvWriteMode::IfGenerationMatch(generation) + ), + EcKvWriteOutcome::Written, + "should write when the generation still matches" + ); + assert_eq!( + write( + &key, + "entry-body-3", + EcKvWriteMode::IfGenerationMatch(generation) + ), + EcKvWriteOutcome::PreconditionFailed, + "the generation moved on with the previous write" + ); + + backend.delete(&key).expect("should delete the entry"); + } + + #[test] + fn counting_a_prefix_counts_only_the_keys_under_it() { + let hash = "5".repeat(64); + let backend = store(); + let keys = [format!("{hash}.AAA111"), format!("{hash}.BBB222")]; + for key in &keys { + write(key, "entry-body-1", EcKvWriteMode::Overwrite); + } + + assert_eq!( + backend + .count_keys_with_prefix(&hash, 100) + .expect("should list the prefix"), + 2, + "should count both keys issued under this hash" + ); + assert_eq!( + backend + .count_keys_with_prefix(&"6".repeat(64), 100) + .expect("should list the prefix"), + 0, + "should count nothing under a hash nothing was issued for" + ); + + for key in &keys { + backend.delete(key).expect("should delete the entry"); + } + } +} diff --git a/crates/trusted-server-core/src/ec/admin.rs b/crates/trusted-server-core/src/ec/admin.rs index 6219af7a9..38033c533 100644 --- a/crates/trusted-server-core/src/ec/admin.rs +++ b/crates/trusted-server-core/src/ec/admin.rs @@ -677,6 +677,8 @@ mod tests { use base64::{Engine as _, engine::general_purpose::STANDARD as BASE64}; + use crate::ec::kv::TombstoneOutcome; + use super::*; use crate::ec::kv_backend::test_support::InMemoryEcKv; use crate::ec::kv_backend::{EcKvStore as _, EcKvWrite, EcKvWriteMode}; @@ -1114,9 +1116,18 @@ mod tests { #[test] fn reports_tombstone_entries() { let ec_id = test_ec_id(); - let kv = KvIdentityGraph::in_memory("test-store"); - kv.write_withdrawal_tombstone(&ec_id) - .expect("should write tombstone"); + // Only an identity the store already holds can be tombstoned, so seed + // the live entry the withdrawal replaces. + let kv = kv_with_entry( + &ec_id, + &KvEntry::minimal("bidstream.example", "uid-live", 1_741_824_000), + ); + assert_eq!( + kv.write_withdrawal_tombstone(&ec_id) + .expect("should write tombstone"), + TombstoneOutcome::Written, + "should tombstone the seeded identity" + ); let req = get_request(&format!("/_ts/admin/ec/{ec_id}")); let response = handle_admin_ec_lookup(Some(&kv), &test_registry(), &req) diff --git a/crates/trusted-server-core/src/ec/finalize.rs b/crates/trusted-server-core/src/ec/finalize.rs index a553bb7a7..20433e34a 100644 --- a/crates/trusted-server-core/src/ec/finalize.rs +++ b/crates/trusted-server-core/src/ec/finalize.rs @@ -6,15 +6,17 @@ use std::collections::HashSet; use edgezero_core::body::Body as EdgeBody; +use error_stack::Report; use http::Response; use super::consent::{ec_consent_granted, ec_consent_withdrawn}; +use crate::error::TrustedServerError; use crate::settings::Settings; use super::EcContext; use super::cookies::{expire_ec_cookie, set_ec_cookie}; use super::generation::is_valid_ec_id; -use super::kv::KvIdentityGraph; +use super::kv::{KvIdentityGraph, TombstoneOutcome}; use super::log_id; use super::prebid_eids::ingest_eid_cookies; use super::registry::PartnerRegistry; @@ -51,34 +53,14 @@ pub fn ec_finalize_response( let consent_withdrawn = ec_consent_withdrawn(ec_context.consent()); if !consent_allows_ec { - // Always strip EC-specific response headers when consent is not - // currently usable for this request. This covers both explicit - // revocation and fail-closed cases such as missing geo or undecodable - // consent input. - clear_ec_headers_on_response(response, Some(registry)); - - // Only expire the browser cookie and tombstone the identity-graph row - // when the request carries an explicit withdrawal signal. - if consent_withdrawn && ec_context.cookie_was_present() { - expire_ec_cookie(settings, response); - - // Compute once for the authoritative identity-graph tombstones. - let ids_to_withdraw = withdrawal_ec_ids(ec_context); - - // The identity-graph tombstone is the authoritative withdrawal marker - // for subsequent EC behavior. - if let Some(graph) = kv { - apply_withdrawal_tombstones(&ids_to_withdraw, |ec_id| { - if let Err(err) = graph.write_withdrawal_tombstone(ec_id) { - log::error!( - "Failed to write withdrawal tombstone for EC ID '{}': {err:?}", - log_id(ec_id), - ); - } - }); - } - } - + finalize_unusable_consent( + settings, + ec_context, + kv, + registry, + consent_withdrawn, + response, + ); return; } @@ -152,6 +134,74 @@ pub fn clear_ec_on_response(settings: &Settings, response: &mut Response, + registry: &PartnerRegistry, + consent_withdrawn: bool, + response: &mut Response, +) { + clear_ec_headers_on_response(response, Some(registry)); + + if !(consent_withdrawn && ec_context.cookie_was_present()) { + return; + } + + expire_ec_cookie(settings, response); + + // Compute once for the authoritative identity-graph tombstones. + let ids_to_withdraw = withdrawal_ec_ids(ec_context); + + // The identity-graph tombstone is the authoritative withdrawal marker + // for subsequent EC behavior. + if let Some(graph) = kv { + apply_withdrawal_tombstones(&ids_to_withdraw, |ec_id| { + log_tombstone_outcome(ec_id, graph.write_withdrawal_tombstone(ec_id)); + }); + } +} + +/// Records what happened to one withdrawal tombstone. +/// +/// An unknown identity is expected traffic rather than a fault: the identifier +/// comes from a client-supplied cookie, so it may name something this +/// deployment never issued. An error is different: nothing was recorded, so a +/// real row may have gone unmarked, and that is logged as a fault. The browser +/// cookie is expired in every case, and that is the primary enforcement. +fn log_tombstone_outcome( + ec_id: &str, + outcome: Result>, +) { + match outcome { + Ok(TombstoneOutcome::Written) => {} + Ok(TombstoneOutcome::UnknownIdentity) => { + log::debug!( + "Skipping withdrawal tombstone for unknown EC ID '{}'", + log_id(ec_id), + ); + } + Err(err) => { + // Covers both a failed write and a check that could not determine + // whether the identity exists. Either way no marker was recorded, + // so a withdrawal may go unrecorded for the batch-sync window; the + // browser cookie is expired regardless. + log::error!( + "Could not record the withdrawal of EC ID '{}', so it may go unrecorded \ + for the batch-sync window; the browser cookie is still expired: {err:?}", + log_id(ec_id), + ); + } + } +} + fn withdrawal_ec_ids(ec_context: &EcContext) -> HashSet { let mut hashes = HashSet::new(); @@ -391,6 +441,128 @@ mod tests { ); } + #[test] + fn finalize_withdrawal_does_not_create_a_row_for_an_unheld_identity() { + let settings = create_test_settings(); + // The cookie value is chosen by the client, so a withdrawal naming an + // identity this deployment never issued must not put a row in the + // identity graph. + let ec_id = sample_ec_id("zz9999"); + let consent = ConsentContext { + jurisdiction: Jurisdiction::UsState("CA".to_owned()), + gpc: true, + source: ConsentSource::Cookie, + ..Default::default() + }; + let ec_context = + make_context_with_consent(Some(&ec_id), Some(&ec_id), true, false, consent); + let kv = KvIdentityGraph::in_memory("test-store"); + let mut response = empty_response(); + let registry = PartnerRegistry::from_config(&[]).expect("should build registry"); + + ec_finalize_response( + &settings, + &ec_context, + Some(&kv), + ®istry, + None, + None, + &mut response, + ); + + assert!( + kv.get(&ec_id).expect("should read back").is_none(), + "should not write a tombstone for an identity that was never issued" + ); + let set_cookie = get_header_str(&response, "set-cookie").unwrap_or_default(); + assert!( + set_cookie.contains("Max-Age=0"), + "should still expire the browser cookie, which is the primary enforcement" + ); + } + + #[test] + fn finalize_withdrawal_tombstones_a_held_identity() { + let settings = create_test_settings(); + let ec_id = sample_ec_id("held01"); + let consent = ConsentContext { + jurisdiction: Jurisdiction::UsState("CA".to_owned()), + gpc: true, + source: ConsentSource::Cookie, + ..Default::default() + }; + let ec_context = + make_context_with_consent(Some(&ec_id), Some(&ec_id), true, false, consent); + let kv = KvIdentityGraph::in_memory("test-store"); + kv.create( + &ec_id, + &crate::ec::kv_types::KvEntry::minimal("p.example", "uid", 1), + ) + .expect("should seed the identity"); + let mut response = empty_response(); + let registry = PartnerRegistry::from_config(&[]).expect("should build registry"); + + ec_finalize_response( + &settings, + &ec_context, + Some(&kv), + ®istry, + None, + None, + &mut response, + ); + + let (entry, _) = kv + .get(&ec_id) + .expect("should read back") + .expect("should still hold the identity"); + assert!( + !entry.consent.ok, + "a genuine withdrawal must still tombstone the identity" + ); + } + + #[test] + fn withdrawal_still_expires_the_cookie_when_the_store_is_unavailable() { + // Cookie expiry is the primary enforcement, so it has to survive a + // store that cannot answer at all — the case where the identity-graph + // marker is exactly what goes missing. + let settings = create_test_settings(); + let ec_id = sample_ec_id("dead01"); + let consent = ConsentContext { + jurisdiction: Jurisdiction::UsState("CA".to_owned()), + gpc: true, + source: ConsentSource::Cookie, + ..Default::default() + }; + let ec_context = + make_context_with_consent(Some(&ec_id), Some(&ec_id), true, false, consent); + let kv = KvIdentityGraph::failing("test-store"); + let mut response = empty_response(); + set_header(&mut response, "x-ts-ec", "stale"); + let registry = PartnerRegistry::from_config(&[]).expect("should build registry"); + + ec_finalize_response( + &settings, + &ec_context, + Some(&kv), + ®istry, + None, + None, + &mut response, + ); + + let set_cookie = get_header_str(&response, "set-cookie").unwrap_or_default(); + assert!( + set_cookie.contains("Max-Age=0"), + "should expire the EC cookie even when the store is unavailable: {set_cookie}" + ); + assert!( + get_header(&response, "x-ts-ec").is_none(), + "should still strip EC response headers" + ); + } + #[test] fn finalize_withdrawal_clears_cookie_and_headers() { let settings = create_test_settings(); diff --git a/crates/trusted-server-core/src/ec/kv.rs b/crates/trusted-server-core/src/ec/kv.rs index 3572581ce..cefa29be2 100644 --- a/crates/trusted-server-core/src/ec/kv.rs +++ b/crates/trusted-server-core/src/ec/kv.rs @@ -123,6 +123,15 @@ impl fmt::Debug for KvIdentityGraph { } } +/// Result of [`KvIdentityGraph::write_withdrawal_tombstone`]. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum TombstoneOutcome { + /// The identity was found and is now tombstoned. + Written, + /// No such identity is held, so there was nothing to mark withdrawn. + UnknownIdentity, +} + impl KvIdentityGraph { /// Creates a new identity graph backed by the given store primitives. #[must_use] @@ -213,13 +222,16 @@ impl KvIdentityGraph { let entry: KvEntry = serde_json::from_slice(body_bytes).change_context(TrustedServerError::KvStore { store_name: store_name.to_owned(), - message: format!("Failed to deserialize entry for key '{ec_id}'"), + message: format!("Failed to deserialize entry for key '{}'", log_id(ec_id)), })?; entry.validate().map_err(|message| { Report::new(TrustedServerError::KvStore { store_name: store_name.to_owned(), - message: format!("Loaded invalid entry for key '{ec_id}': {message}"), + message: format!( + "Loaded invalid entry for key '{}': {message}", + log_id(ec_id) + ), }) })?; @@ -248,7 +260,7 @@ impl KvIdentityGraph { let meta: KvMetadata = serde_json::from_slice(&meta_bytes).change_context(TrustedServerError::KvStore { store_name: self.store_name().to_owned(), - message: format!("Failed to deserialize metadata for key '{ec_id}'"), + message: format!("Failed to deserialize metadata for key '{}'", log_id(ec_id)), })?; Ok(Some(meta)) @@ -268,7 +280,7 @@ impl KvIdentityGraph { match self.write_entry(ec_id, &body, &meta_str, ENTRY_TTL, EcKvWriteMode::Add)? { EcKvWriteOutcome::Written => Ok(()), EcKvWriteOutcome::PreconditionFailed => { - Err(self.kv_error(format!("Key '{ec_id}' already exists"))) + Err(self.kv_error(format!("Key '{}' already exists", log_id(ec_id)))) } } } @@ -379,7 +391,8 @@ impl KvIdentityGraph { } Err(self.kv_error(format!( - "CAS conflict after {MAX_CAS_RETRIES} retries reviving tombstone for '{ec_id}'" + "CAS conflict after {MAX_CAS_RETRIES} retries reviving tombstone for '{}'", + log_id(ec_id), ))) } @@ -413,8 +426,9 @@ impl KvIdentityGraph { updates.len(), ); return Err(self.kv_error(format!( - "Cannot upsert {} partner IDs for missing key '{ec_id}'", + "Cannot upsert {} partner IDs for missing key '{}'", updates.len(), + log_id(ec_id), ))); } }; @@ -428,8 +442,9 @@ impl KvIdentityGraph { updates.len(), ); return Err(self.kv_error(format!( - "Cannot upsert {} partner IDs for withdrawn key '{ec_id}'", + "Cannot upsert {} partner IDs for withdrawn key '{}'", updates.len(), + log_id(ec_id), ))); } @@ -459,8 +474,9 @@ impl KvIdentityGraph { } Err(self.kv_error(format!( - "CAS conflict after {MAX_CAS_RETRIES} retries upserting {} partner IDs for '{ec_id}'", + "CAS conflict after {MAX_CAS_RETRIES} retries upserting {} partner IDs for '{}'", updates.len(), + log_id(ec_id), ))) } @@ -491,7 +507,8 @@ impl KvIdentityGraph { log_id(ec_id) ); return Err(self.kv_error(format!( - "Cannot upsert partner '{partner_id}' for missing key '{ec_id}'" + "Cannot upsert partner '{partner_id}' for missing key '{}'", + log_id(ec_id), ))); } }; @@ -504,7 +521,8 @@ impl KvIdentityGraph { log_id(ec_id), ); return Err(self.kv_error(format!( - "Cannot upsert partner '{partner_id}' for withdrawn key '{ec_id}'" + "Cannot upsert partner '{partner_id}' for withdrawn key '{}'", + log_id(ec_id), ))); } @@ -547,7 +565,8 @@ impl KvIdentityGraph { } Err(self.kv_error(format!( - "CAS conflict after {MAX_CAS_RETRIES} retries upserting partner '{partner_id}' for '{ec_id}'" + "CAS conflict after {MAX_CAS_RETRIES} retries upserting partner '{partner_id}' for '{}'", + log_id(ec_id), ))) } @@ -617,10 +636,25 @@ impl KvIdentityGraph { } Err(self.kv_error(format!( - "CAS conflict after {MAX_CAS_RETRIES} retries upserting partner '{partner_id}' for '{ec_id}'" + "CAS conflict after {MAX_CAS_RETRIES} retries upserting partner '{partner_id}' for '{}'", + log_id(ec_id), ))) } + /// Whether `ec_id` names a key this store actually holds. + /// + /// Reads the key itself, so the answer is about that identity and no + /// other: counting keys by prefix would let a longer key carrying this one + /// as a prefix answer in its place. One round trip, and no bound on the + /// identifier is needed because nothing is scanned. + /// + /// # Errors + /// + /// Returns [`TrustedServerError::KvStore`] on store error. + fn key_exists_confirmed(&self, ec_id: &str) -> Result> { + Ok(self.lookup_raw(ec_id)?.is_some()) + } + /// Writes a withdrawal tombstone for consent enforcement. /// /// Overwrites the entry with `consent.ok = false`, empty partner IDs, @@ -630,15 +664,46 @@ impl KvIdentityGraph { /// The tombstone preserves consent enforcement for batch sync clients /// (`POST /_ts/api/v1/batch-sync`) during the 24-hour revocation window. /// + /// Only an identity this store already holds is tombstoned. The marker + /// exists to stop later reads of a real row, so writing one for an ID that + /// was never issued enforces nothing while still consuming a write and a + /// row; the identifier in a request is chosen by the client, so that write + /// would be the client's to trigger at will. Existence is confirmed with + /// [`Self::key_exists_confirmed`], which reads the key itself, so no + /// neighbouring key can answer for it. + /// + /// The check and the write are not one atomic operation: an entry that + /// expires between them is still tombstoned, briefly restoring a row that + /// had gone. That is deliberate — the write stays unconditional so a + /// withdrawal is not lost to a concurrent update — and it cannot be used to + /// create an identity, because the entry must have existed to pass the + /// check at all. + /// + /// The read may lag the write that issued the identity, so an identity + /// created and withdrawn inside that window is reported as unknown and no + /// tombstone is written. The browser cookie is expired either way, which is + /// the primary enforcement; the residual exposure is a batch-sync client + /// that already holds an identifier issued that recently. + /// /// # Errors /// - /// Returns [`TrustedServerError::KvStore`] on store error. Callers on - /// the browser path should log at `error` level and continue — cookie - /// deletion is the primary enforcement mechanism. + /// Returns [`TrustedServerError::KvStore`] when the tombstone write fails, + /// and when it cannot be determined whether the identity exists — in that + /// case nothing is written. Callers on the browser path should log at + /// `error` level and continue: cookie deletion is the primary enforcement + /// mechanism. pub fn write_withdrawal_tombstone( &self, ec_id: &str, - ) -> Result<(), Report> { + ) -> Result> { + // A store failure is an error, not a third outcome: writing blind + // would restore the unconditional write whenever the store can be made + // to fail, and an extra `Ok` variant would be discarded in silence by a + // caller that only inspects the error case. + if !self.key_exists_confirmed(ec_id)? { + return Ok(TombstoneOutcome::UnknownIdentity); + } + let entry = KvEntry::tombstone(current_timestamp()); let (body, meta_str) = Self::serialize_entry(&entry, self.store_name())?; @@ -649,10 +714,10 @@ impl KvIdentityGraph { TOMBSTONE_TTL, EcKvWriteMode::Overwrite, ) { - Ok(_) => Ok(()), + Ok(_) => Ok(TombstoneOutcome::Written), Err(report) => Err(report.change_context(TrustedServerError::KvStore { store_name: self.store_name().to_owned(), - message: format!("Failed to write tombstone for key '{ec_id}'"), + message: format!("Failed to write tombstone for key '{}'", log_id(ec_id)), })), } } @@ -929,6 +994,23 @@ mod tests { ) .expect("should seed tombstone"); } + + fn seed_live(&self, ec_id: &str) { + let (body, meta) = + KvIdentityGraph::serialize_entry(&live_entry(), self.inner.store_name()) + .expect("should serialize live entry"); + self.inner + .insert( + ec_id, + EcKvWrite { + body: &body, + metadata: &meta, + ttl: TOMBSTONE_TTL, + mode: EcKvWriteMode::Add, + }, + ) + .expect("should seed live entry"); + } } impl EcKvStore for ConflictInjectingEcKv { @@ -1258,14 +1340,119 @@ mod tests { assert_eq!(result, UpsertResult::ConsentWithdrawn); } + #[test] + fn a_store_error_never_carries_the_whole_identifier() { + // Every message in this module goes through `log_id`, so a report that + // reaches a log cannot disclose the identifier it is about. + let kv = KvIdentityGraph::failing("test_store"); + let ec_id = format!("{}.ABC123", "a".repeat(64)); + + let report = kv + .create(&ec_id, &live_entry()) + .expect_err("the failing store should error"); + + let rendered = format!("{report:?}"); + assert!( + !rendered.contains(&ec_id), + "a store error must not disclose the identifier: {rendered}" + ); + } + + #[test] + fn a_locally_built_error_never_carries_the_whole_identifier() { + // The injected-failure case above covers errors the backend produces. + // These are built in this module from the identifier itself, on every + // path a request can reach: a duplicate create, single and batched + // upserts naming a key the store does not hold or has withdrawn, and + // the CAS-exhaustion terminal errors. + let kv = KvIdentityGraph::in_memory("test_store"); + let ec_id = format!("{}.ABC123", "a".repeat(64)); + kv.create(&ec_id, &live_entry()).expect("should create"); + + let duplicate = kv + .create(&ec_id, &live_entry()) + .expect_err("a second create should be refused"); + let missing = kv + .upsert_partner_id(&format!("{}.ZZZ999", "b".repeat(64)), "partner", "uid") + .expect_err("an upsert on a missing key should be refused"); + let batched_missing = kv + .upsert_partner_ids( + &format!("{}.ZZZ999", "b".repeat(64)), + &[PartnerIdUpdate::new("partner", "uid")], + ) + .expect_err("a batched upsert on a missing key should be refused"); + let withdrawn = { + kv.write_withdrawal_tombstone(&ec_id) + .expect("should tombstone"); + kv.upsert_partner_id(&ec_id, "partner", "uid") + .expect_err("an upsert on a withdrawn key should be refused") + }; + let batched_withdrawn = kv + .upsert_partner_ids(&ec_id, &[PartnerIdUpdate::new("partner", "uid")]) + .expect_err("a batched upsert on a withdrawn key should be refused"); + + // The CAS-exhaustion paths build their message the same way, and a + // store that never lets a write land is the only way to reach them. + let cas_revive = { + let store = ConflictInjectingEcKv::new(MAX_CAS_RETRIES + 1, false); + store.seed_tombstone(&ec_id); + KvIdentityGraph::new(store) + .create_or_revive(&ec_id, &live_entry()) + .expect_err("should exhaust CAS retries") + }; + let cas_upsert = { + let store = ConflictInjectingEcKv::new(MAX_CAS_RETRIES + 1, false); + store.seed_live(&ec_id); + KvIdentityGraph::new(store) + .upsert_partner_id(&ec_id, "partner", "uid") + .expect_err("should exhaust CAS retries") + }; + let cas_batched = { + let store = ConflictInjectingEcKv::new(MAX_CAS_RETRIES + 1, false); + store.seed_live(&ec_id); + KvIdentityGraph::new(store) + .upsert_partner_ids(&ec_id, &[PartnerIdUpdate::new("partner", "uid")]) + .expect_err("should exhaust CAS retries") + }; + let cas_if_exists = { + let store = ConflictInjectingEcKv::new(MAX_CAS_RETRIES + 1, false); + store.seed_live(&ec_id); + KvIdentityGraph::new(store) + .upsert_partner_id_if_exists(&ec_id, "partner", "uid") + .expect_err("should exhaust CAS retries") + }; + + for (label, report) in [ + ("duplicate create", duplicate), + ("missing key", missing), + ("batched missing key", batched_missing), + ("withdrawn key", withdrawn), + ("batched withdrawn key", batched_withdrawn), + ("CAS exhaustion reviving", cas_revive), + ("CAS exhaustion upserting", cas_upsert), + ("CAS exhaustion batch upserting", cas_batched), + ("CAS exhaustion upserting if present", cas_if_exists), + ] { + let rendered = format!("{report:?}"); + assert!( + !rendered.contains(&ec_id) && !rendered.contains(&"b".repeat(64)), + "the {label} error must not disclose the identifier: {rendered}" + ); + } + } + #[test] fn write_withdrawal_tombstone_overwrites_live_entry() { let kv = KvIdentityGraph::in_memory("test_store"); let ec_id = format!("{}.ABC123", "a".repeat(64)); kv.create(&ec_id, &live_entry()).expect("should create"); - kv.write_withdrawal_tombstone(&ec_id) - .expect("should write tombstone"); + assert_eq!( + kv.write_withdrawal_tombstone(&ec_id) + .expect("should write tombstone"), + TombstoneOutcome::Written, + "should tombstone an identity the store holds" + ); let (loaded, _) = kv .get(&ec_id) @@ -1273,4 +1460,281 @@ mod tests { .expect("should find tombstone entry"); assert!(!loaded.consent.ok, "should be withdrawn after tombstone"); } + + #[test] + fn write_withdrawal_tombstone_ignores_an_identity_the_store_does_not_hold() { + let kv = KvIdentityGraph::in_memory("test_store"); + let ec_id = format!("{}.ABC123", "b".repeat(64)); + + assert_eq!( + kv.write_withdrawal_tombstone(&ec_id) + .expect("should resolve the withdrawal"), + TombstoneOutcome::UnknownIdentity, + "an identity that was never issued has nothing to withdraw" + ); + assert!( + kv.get(&ec_id).expect("should read back").is_none(), + "should not create a row for an identity the store never held" + ); + } + + #[test] + fn withdrawing_many_unheld_identities_creates_no_rows() { + let kv = KvIdentityGraph::in_memory("test_store"); + let hash = "c".repeat(64); + + // The suffix is caller-supplied, so a shared hash prefix must not be + // enough to have a row written under it. + for suffix in ["aaaaaa", "bbbbbb", "cccccc", "dddddd"] { + assert_eq!( + kv.write_withdrawal_tombstone(&format!("{hash}.{suffix}")) + .expect("should resolve the withdrawal"), + TombstoneOutcome::UnknownIdentity, + "suffix `{suffix}` was never issued" + ); + } + + assert_eq!( + kv.count_hash_prefix_keys(&hash) + .expect("should count the prefix"), + 0, + "should hold no rows under a hash nothing was issued for" + ); + } + + #[test] + fn a_withdrawal_costs_one_read_whether_or_not_the_identity_is_held() { + // The gate sits on the withdrawal response path, so it must not double + // the reads a withdrawal already pays for. + let (store, reads) = CountingEcKv::new(); + let kv = KvIdentityGraph::new(store); + let count = || *reads.lock().expect("should lock the read counter"); + + let absent = format!("{}.ABC123", "e".repeat(64)); + assert_eq!( + kv.write_withdrawal_tombstone(&absent) + .expect("should resolve the withdrawal"), + TombstoneOutcome::UnknownIdentity, + "an absent identity is not held" + ); + assert_eq!(count(), 1, "an unknown identity costs a single read"); + + let held = format!("{}.ABC123", "a".repeat(64)); + kv.create(&held, &live_entry()).expect("should create"); + let before = count(); + assert_eq!( + kv.write_withdrawal_tombstone(&held) + .expect("should resolve the withdrawal"), + TombstoneOutcome::Written, + "a held identity is tombstoned" + ); + assert_eq!( + count() - before, + 1, + "a held identity is checked once, then written" + ); + } + + #[test] + fn write_withdrawal_tombstone_refuses_an_empty_identifier() { + let kv = KvIdentityGraph::in_memory("test_store"); + kv.create(&format!("{}.ABC123", "f".repeat(64)), &live_entry()) + .expect("should create"); + + assert_eq!( + kv.write_withdrawal_tombstone("") + .expect("should resolve the withdrawal"), + TombstoneOutcome::UnknownIdentity, + "an empty identifier names no key and must not withdraw anything" + ); + let (held, _) = kv + .get(&format!("{}.ABC123", "f".repeat(64))) + .expect("should read back") + .expect("should still hold the identity"); + assert!( + held.consent.ok, + "should not have withdrawn an unrelated row" + ); + } + + /// Store double that records how many reads reach it. + struct CountingEcKv { + inner: super::super::kv_backend::test_support::InMemoryEcKv, + reads: std::sync::Arc>, + } + + impl CountingEcKv { + /// Returns the store and a handle to its counter, which stays readable + /// after the store moves into the graph. + fn new() -> (Self, std::sync::Arc>) { + let counter = std::sync::Arc::new(std::sync::Mutex::new(0)); + ( + Self { + inner: super::super::kv_backend::test_support::InMemoryEcKv::new("test_store"), + reads: std::sync::Arc::clone(&counter), + }, + counter, + ) + } + } + + impl EcKvStore for CountingEcKv { + fn store_name(&self) -> &str { + self.inner.store_name() + } + + fn lookup(&self, key: &str) -> Result, Report> { + *self.reads.lock().expect("should lock the read counter") += 1; + self.inner.lookup(key) + } + + fn insert( + &self, + key: &str, + write: EcKvWrite<'_>, + ) -> Result> { + self.inner.insert(key, write) + } + + fn count_keys_with_prefix( + &self, + prefix: &str, + limit: u32, + ) -> Result> { + self.inner.count_keys_with_prefix(prefix, limit) + } + + fn delete(&self, key: &str) -> Result<(), Report> { + self.inner.delete(key) + } + } + + /// Store double whose reads always fail while writes still work. + struct ReadFailingEcKv { + inner: super::super::kv_backend::test_support::InMemoryEcKv, + } + + impl ReadFailingEcKv { + fn new() -> Self { + Self { + inner: super::super::kv_backend::test_support::InMemoryEcKv::new("test_store"), + } + } + } + + impl EcKvStore for ReadFailingEcKv { + fn store_name(&self) -> &str { + self.inner.store_name() + } + + fn lookup(&self, _key: &str) -> Result, Report> { + Err(Report::new(TrustedServerError::KvStore { + store_name: "test_store".to_owned(), + message: "reads unavailable".to_owned(), + })) + } + + fn insert( + &self, + key: &str, + write: EcKvWrite<'_>, + ) -> Result> { + self.inner.insert(key, write) + } + + // Left working so a test can prove no row was created without going + // through the read path it just made fail. + fn count_keys_with_prefix( + &self, + prefix: &str, + limit: u32, + ) -> Result> { + self.inner.count_keys_with_prefix(prefix, limit) + } + + fn delete(&self, key: &str) -> Result<(), Report> { + self.inner.delete(key) + } + } + + #[test] + fn a_failing_check_is_not_a_way_to_write_for_an_identity_that_was_never_issued() { + // The caller controls the identifier and can drive load, so a store + // failure must not become a route to the write this gate exists to + // prevent. + let kv = KvIdentityGraph::new(ReadFailingEcKv::new()); + let hash = "8".repeat(64); + let ec_id = format!("{hash}.ABC123"); + + assert!( + kv.write_withdrawal_tombstone(&ec_id).is_err(), + "a check that cannot answer is a fault, so a caller inspecting only \ + the error case still reports it" + ); + assert_eq!( + kv.count_hash_prefix_keys(&hash) + .expect("should count the prefix"), + 0, + "should not create a row while the store is degraded" + ); + } + + #[test] + fn a_caller_that_only_inspects_the_error_case_still_sees_a_failed_check() { + // The withdrawal call site is edited by more than one branch. Reporting + // a failed check through `Err` means the common + // `if let Err(..) = ...` shape cannot discard it, where a third `Ok` + // variant would be dropped without a compiler complaint. + let kv = KvIdentityGraph::new(ReadFailingEcKv::new()); + let ec_id = format!("{}.ABC123", "6".repeat(64)); + + let mut reported = false; + if let Err(_err) = kv.write_withdrawal_tombstone(&ec_id) { + reported = true; + } + + assert!(reported, "a failed check must reach an error-only caller"); + } + + #[test] + fn a_longer_key_does_not_answer_for_the_identity_it_starts_with() { + let kv = KvIdentityGraph::in_memory("test_store"); + let ec_id = format!("{}.ABC123", "7".repeat(64)); + // Only a longer key exists. The identity itself was never issued, so a + // check that matched by prefix would report it as held and tombstone it. + kv.create(&format!("{ec_id}trailing"), &live_entry()) + .expect("should create"); + + assert!( + !kv.key_exists_confirmed(&ec_id).expect("should check"), + "a longer key is a different identity" + ); + assert_eq!( + kv.write_withdrawal_tombstone(&ec_id) + .expect("should resolve the withdrawal"), + TombstoneOutcome::UnknownIdentity, + "should not tombstone an identity the store never held" + ); + assert!( + kv.get(&ec_id).expect("should read back").is_none(), + "should not create a row via a prefix match" + ); + } + + #[test] + fn key_exists_confirmed_distinguishes_held_identities() { + let kv = KvIdentityGraph::in_memory("test_store"); + let held = format!("{}.ABC123", "d".repeat(64)); + let sibling = format!("{}.ZZZ999", "d".repeat(64)); + kv.create(&held, &live_entry()).expect("should create"); + + assert!( + kv.key_exists_confirmed(&held).expect("should check"), + "should confirm a held identity" + ); + assert!( + !kv.key_exists_confirmed(&sibling).expect("should check"), + "a different suffix under the same hash is a different identity" + ); + } } diff --git a/crates/trusted-server-core/src/ec/mod.rs b/crates/trusted-server-core/src/ec/mod.rs index 840ce90d3..953cef19b 100644 --- a/crates/trusted-server-core/src/ec/mod.rs +++ b/crates/trusted-server-core/src/ec/mod.rs @@ -49,14 +49,20 @@ pub mod pull_sync; pub mod rate_limiter; pub mod registry; +/// Characters of an identifier kept when redacting it for a log. +const LOG_ID_PREFIX_CHARS: usize = 8; + /// Truncates an EC ID for safe inclusion in log messages. /// -/// Returns the first 8 characters followed by `…` to aid debugging without -/// writing the full user identifier to logs (satisfies the `CodeQL` -/// "cleartext logging of sensitive information" rule). +/// Returns the first [`LOG_ID_PREFIX_CHARS`] characters followed by `…` to aid +/// debugging without writing the full user identifier to logs (satisfies the +/// `CodeQL` "cleartext logging of sensitive information" rule). #[must_use] pub fn log_id(ec_id: &str) -> String { - let prefix = ec_id.get(..8).unwrap_or(ec_id); + // Truncated by character, not by byte. A byte index that lands inside a + // multi-byte character makes `get` return `None`, and falling back to the + // whole value would print in full the identifier this exists to redact. + let prefix: String = ec_id.chars().take(LOG_ID_PREFIX_CHARS).collect(); format!("{prefix}\u{2026}") } @@ -497,6 +503,30 @@ mod tests { use crate::platform::test_support::noop_services; use crate::test_support::tests::create_test_settings; + #[test] + fn log_id_never_emits_more_than_the_redacted_prefix() { + // A byte index inside a multi-byte character used to make the + // truncation fall back to the whole value, printing in full the + // identifier this redacts. + let boundary_splitting = "abcdefg\u{e9}-tail-that-must-not-be-logged"; + let redacted = log_id(boundary_splitting); + + assert!( + !redacted.contains("must-not-be-logged"), + "should not disclose the rest of the identifier: {redacted}" + ); + assert_eq!( + redacted.chars().count(), + 9, + "should be eight characters plus the ellipsis: {redacted}" + ); + + // The ordinary case is unchanged. + assert_eq!(log_id("0123456789abcdef.ABC123"), "01234567\u{2026}"); + // A value shorter than the prefix is emitted whole, which is all there is. + assert_eq!(log_id("abc"), "abc\u{2026}"); + } + fn create_test_request(headers: &[(&str, &str)]) -> Request { let mut builder = Request::builder().method("GET").uri("http://example.com"); for &(key, value) in headers {