diff --git a/crates/trusted-server-adapter-fastly/src/ec_kv.rs b/crates/trusted-server-adapter-fastly/src/ec_kv.rs index 229c6c2d3..87ecf119c 100644 --- a/crates/trusted-server-adapter-fastly/src/ec_kv.rs +++ b/crates/trusted-server-adapter-fastly/src/ec_kv.rs @@ -7,8 +7,10 @@ use error_stack::{Report, ResultExt}; use fastly::kv_store::{InsertMode, KVStore}; use trusted_server_core::ec::kv_backend::{ - EcKvLookup, EcKvStore, EcKvWrite, EcKvWriteMode, EcKvWriteOutcome, + EcKvLookup, EcKvStore, EcKvWrite, EcKvWriteMode, EcKvWriteOutcome, ExactKeyMatch, + contains_exact_key, }; +use trusted_server_core::ec::log_id; use trusted_server_core::error::TrustedServerError; /// Fastly KV Store backend for the EC identity graph. @@ -42,6 +44,18 @@ impl FastlyEcKvStore { } } +/// Keys requested per page when checking for one exact key. +const EXACT_MATCH_PAGE_SIZE: u32 = 100; +/// Pages walked before giving up on an exact-key check. +/// +/// A well-formed identifier is the whole key, so at most a handful of keys can +/// carry it as a prefix and one page is the normal case. The cap stops a short +/// prefix from walking the keyspace on the response path. Exceeding it is +/// reported as an error rather than as absence, so the caller re-checks by +/// lookup instead of discarding a withdrawal for an identity that may be held +/// on a page this never read. +const EXACT_MATCH_MAX_PAGES: usize = 4; + impl EcKvStore for FastlyEcKvStore { fn store_name(&self) -> &str { &self.store_name @@ -56,7 +70,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 +112,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 +131,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)] @@ -128,13 +139,55 @@ impl EcKvStore for FastlyEcKvStore { Ok(count) } + fn key_exists(&self, key: &str) -> Result> { + let store = self.open_store()?; + // The list is strongly consistent, unlike a lookup, but it matches by + // prefix, so the decision of what counts as a match belongs to + // `contains_exact_key` — which is where that behaviour is tested. + let pages = store + .build_list() + .prefix(key) + .limit(EXACT_MATCH_PAGE_SIZE) + .iter() + // Unbounded here on purpose: `contains_exact_key` stops reading at + // its budget, so the page count lives with the code that is tested + // against it rather than being recomputed at the call site. + .map(|page| match page { + // A store that reports nothing under the prefix is telling us + // the key is absent, which is what `lookup` already does for a + // missing key. Treating it as a failure would send every + // withdrawal naming an unissued identity down the error path. + Err(fastly::kv_store::KVStoreError::ItemNotFound) => Ok(Vec::new()), + other => other + .map(fastly::kv_store::ListPage::into_keys) + .change_context(TrustedServerError::KvStore { + store_name: self.store_name.clone(), + message: format!("Failed to check key '{}'", log_id(key)), + }), + }); + + match contains_exact_key(pages, key, EXACT_MATCH_MAX_PAGES)? { + ExactKeyMatch::Found => Ok(true), + ExactKeyMatch::Absent => Ok(false), + // Not an answer, so it must not be returned as one. The caller + // falls back to a lookup on an error. + ExactKeyMatch::Undetermined => Err(Report::new(TrustedServerError::KvStore { + store_name: self.store_name.clone(), + message: format!( + "Listing for key '{}' exceeded {EXACT_MATCH_MAX_PAGES} pages, so existence is undetermined", + log_id(key) + ), + })), + } + } + fn delete(&self, key: &str) -> Result<(), Report> { let store = self.open_store()?; store .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)), }) } } 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..c4aa9647c 100644 --- a/crates/trusted-server-core/src/ec/kv.rs +++ b/crates/trusted-server-core/src/ec/kv.rs @@ -123,6 +123,22 @@ impl fmt::Debug for KvIdentityGraph { } } +/// Longest identifier this store will look for. +/// +/// Bounds the work a caller-supplied cookie can ask for. It is not a format +/// check: the existence check is exact, so it does not depend on the shape of +/// an identifier. +const MAX_EC_ID_LEN: usize = 256; + +/// 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 +229,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 +267,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 +287,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 +398,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 +433,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 +449,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 +481,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 +514,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 +528,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 +572,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 +643,33 @@ 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. + /// + /// Delegates to an exact, strongly consistent check. Counting keys by + /// prefix would not do: another key may carry this one as a prefix and + /// would answer for it, and a read that may lag would report a freshly + /// written identity as absent, discarding a genuine withdrawal. + /// + /// A value longer than any identifier this deployment could hold is + /// refused without a store round trip. That bound is about cost, not + /// correctness — the check is exact either way, so it does not care what + /// shape an identifier takes. + /// + /// # Errors + /// + /// Returns [`TrustedServerError::KvStore`] on store error. + fn key_exists_confirmed(&self, ec_id: &str) -> Result> { + if ec_id.is_empty() || ec_id.len() > MAX_EC_ID_LEN { + return Ok(false); + } + self.store.key_exists(ec_id) + } + /// Writes a withdrawal tombstone for consent enforcement. /// /// Overwrites the entry with `consent.ok = false`, empty partner IDs, @@ -630,15 +679,79 @@ 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`] so a freshly issued identity is never + /// mistaken for an unknown one. + /// + /// 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. + /// + /// When the exact check cannot answer, existence is re-checked with a + /// lookup. A lookup may lag, so it can miss a very recent write, and it can + /// return a row already deleted or expired at the primary — but it cannot + /// report an identifier this deployment never issued, which is what the + /// gate is for. If that cannot answer either, nothing is written and an + /// error is returned; the caller still expires the browser cookie, which is + /// the primary enforcement. + /// /// # 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> { + match self.key_exists_confirmed(ec_id) { + Ok(true) => {} + Ok(false) => return Ok(TombstoneOutcome::UnknownIdentity), + Err(list_error) => { + // The check could not answer. Fall back to a raw lookup rather + // than writing blind: a lookup may lag, so it can miss a very + // recent write, but no identifier this deployment never issued + // can appear in it. Writing blind would instead restore the + // unconditional write whenever the store can be made to fail. + // + // The raw form is deliberate: a row whose body no longer + // deserializes is still a row, and presence is all that matters. + // + // Failing to determine existence is an error, not a third + // outcome. A caller that only inspects the error case still + // reports it, where an extra `Ok` variant would be discarded in + // silence. + match self.lookup_raw(ec_id) { + Ok(Some(_)) => { + log::warn!( + "Confirmed EC ID '{}' by lookup after a list failure", + log_id(ec_id), + ); + } + Ok(None) => { + return Err(list_error.attach( + "a lookup found nothing, but it may lag behind a recent write", + )); + } + Err(lookup_error) => { + return Err(lookup_error.attach(format!( + "the exact check also failed: {}", + list_error.current_context() + ))); + } + } + } + } + let entry = KvEntry::tombstone(current_timestamp()); let (body, meta_str) = Self::serialize_entry(&entry, self.store_name())?; @@ -649,10 +762,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 +1042,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 { @@ -986,6 +1116,10 @@ mod tests { self.inner.count_keys_with_prefix(prefix, limit) } + fn key_exists(&self, key: &str) -> Result> { + self.inner.key_exists(key) + } + fn delete(&self, key: &str) -> Result<(), Report> { self.inner.delete(key) } @@ -1258,14 +1392,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 +1512,310 @@ 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 an_out_of_bounds_value_is_refused_without_reaching_the_store() { + let (store, checks) = CountingEcKv::new(); + let kv = KvIdentityGraph::new(store); + let count = || *checks.lock().expect("should lock the check counter"); + + for value in ["", &"z".repeat(257)] { + assert!( + !kv.key_exists_confirmed(value) + .expect("should resolve the check"), + "should not report `{value}` as held" + ); + } + assert_eq!( + count(), + 0, + "a value no key could be must not cost a store round trip" + ); + + // A value that is within bounds but simply absent does reach the store. + let absent = format!("{}.ABC123", "e".repeat(64)); + assert!( + !kv.key_exists_confirmed(&absent).expect("should check"), + "an absent identity is not held" + ); + assert_eq!(count(), 1, "an in-bounds value is checked exactly once"); + } + + #[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 existence checks reach it. + struct CountingEcKv { + inner: super::super::kv_backend::test_support::InMemoryEcKv, + existence_checks: 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"), + existence_checks: 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.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 key_exists(&self, key: &str) -> Result> { + *self + .existence_checks + .lock() + .expect("should lock the check counter") += 1; + self.inner.key_exists(key) + } + + fn delete(&self, key: &str) -> Result<(), Report> { + self.inner.delete(key) + } + } + + /// Store double whose list API always fails while writes still work. + struct ListFailingEcKv { + inner: super::super::kv_backend::test_support::InMemoryEcKv, + } + + impl ListFailingEcKv { + fn new() -> Self { + Self { + inner: super::super::kv_backend::test_support::InMemoryEcKv::new("test_store"), + } + } + } + + impl EcKvStore for ListFailingEcKv { + fn store_name(&self) -> &str { + self.inner.store_name() + } + + fn lookup(&self, key: &str) -> Result, Report> { + 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> { + Err(Report::new(TrustedServerError::KvStore { + store_name: "test_store".to_owned(), + message: "list unavailable".to_owned(), + })) + } + + fn key_exists(&self, _key: &str) -> Result> { + Err(Report::new(TrustedServerError::KvStore { + store_name: "test_store".to_owned(), + message: "list unavailable".to_owned(), + })) + } + + fn delete(&self, key: &str) -> Result<(), Report> { + self.inner.delete(key) + } + } + + #[test] + fn write_withdrawal_tombstone_falls_back_to_lookup_when_the_list_fails() { + // A store outage must not discard a genuine withdrawal, so a failed + // list is re-checked with a lookup, which can still see the row. + let kv = KvIdentityGraph::new(ListFailingEcKv::new()); + let ec_id = format!("{}.ABC123", "9".repeat(64)); + kv.create(&ec_id, &live_entry()).expect("should create"); + + assert_eq!( + kv.write_withdrawal_tombstone(&ec_id) + .expect("should still resolve the withdrawal"), + TombstoneOutcome::Written, + "a held identity should still be tombstoned when only the list fails" + ); + let (loaded, _) = kv + .get(&ec_id) + .expect("should read entry back") + .expect("should find the tombstone"); + assert!(!loaded.consent.ok, "should be withdrawn"); + } + + #[test] + fn a_failing_list_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. A lagging lookup may return a row already deleted, but it + // cannot return one for an identifier this deployment never issued. + let kv = KvIdentityGraph::new(ListFailingEcKv::new()); + let ec_id = format!("{}.ABC123", "8".repeat(64)); + + assert!( + kv.write_withdrawal_tombstone(&ec_id).is_err(), + "an undetermined check is a fault, so a caller inspecting only the \ + error case still reports it" + ); + assert!( + kv.get(&ec_id).expect("should read back").is_none(), + "should not create a row while the store is degraded" + ); + } + + #[test] + fn a_caller_that_only_inspects_the_error_case_still_sees_an_undetermined_check() { + // The withdrawal call site is edited by more than one branch. Reporting + // an undetermined 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(ListFailingEcKv::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, + "an undetermined 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/kv_backend.rs b/crates/trusted-server-core/src/ec/kv_backend.rs index 60f938291..881c615bf 100644 --- a/crates/trusted-server-core/src/ec/kv_backend.rs +++ b/crates/trusted-server-core/src/ec/kv_backend.rs @@ -64,6 +64,56 @@ pub enum EcKvWriteOutcome { PreconditionFailed, } +/// Whether `key` appears exactly in a paged listing. +/// +/// Backends that can only match by prefix use this to decide existence: a +/// longer key carrying `key` as a prefix is a different identity and must not +/// answer for it, so the listed keys are compared for equality. +/// +/// Every page is visited until a match is found. Nothing guarantees the exact +/// key lands in the first page when other keys share its prefix, and stopping +/// early would report a held identity as missing. +/// +/// `max_pages` bounds how far the listing is followed. Running out of budget +/// is reported as [`ExactKeyMatch::Undetermined`] rather than as absence: the +/// key may sit on a page that was never read, and answering "absent" would +/// discard a withdrawal for an identity the store actually holds. The listing +/// is read lazily and at most one page beyond `max_pages` — just far enough to +/// learn that it continues — so callers pass their listing untruncated rather +/// than pre-trimming it to a count that has to agree with this one. +/// +/// # Errors +/// +/// Returns the first page error, so a listing that cannot be read is never +/// mistaken for a listing that does not contain the key. +pub fn contains_exact_key( + pages: impl IntoIterator, E>>, + key: &str, + max_pages: usize, +) -> Result { + for (index, page) in pages.into_iter().enumerate() { + if index >= max_pages { + return Ok(ExactKeyMatch::Undetermined); + } + if page?.iter().any(|listed| listed == key) { + return Ok(ExactKeyMatch::Found); + } + } + Ok(ExactKeyMatch::Absent) +} + +/// What a bounded prefix listing established about one exact key. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ExactKeyMatch { + /// The listing contained the key. + Found, + /// The listing was read to its end and did not contain the key. + Absent, + /// The listing was longer than the budget allowed, so the key's absence + /// was never established. + Undetermined, +} + /// Raw KV store primitives backing the EC identity graph. /// /// Implementations map these operations onto the platform KV API. @@ -107,6 +157,17 @@ pub trait EcKvStore { limit: u32, ) -> Result>; + /// Whether the store holds exactly `key`. + /// + /// Must be an exact match and strongly consistent. A prefix scan is not a + /// substitute: another key may carry this one as a prefix, and a read that + /// may lag would report a freshly written key as absent. + /// + /// # Errors + /// + /// Returns [`TrustedServerError::KvStore`] on store open or list failure. + fn key_exists(&self, key: &str) -> Result>; + /// Hard-deletes a key. /// /// # Errors @@ -205,6 +266,11 @@ pub(crate) mod test_support { Ok(count as u32) } + fn key_exists(&self, key: &str) -> Result> { + let entries = self.entries.lock().expect("should lock in-memory store"); + Ok(entries.contains_key(key)) + } + fn delete(&self, key: &str) -> Result<(), Report> { let mut entries = self.entries.lock().expect("should lock in-memory store"); entries.remove(key); @@ -256,8 +322,160 @@ pub(crate) mod test_support { Err(self.error("list")) } + fn key_exists(&self, _key: &str) -> Result> { + Err(self.error("key_exists")) + } + fn delete(&self, _key: &str) -> Result<(), Report> { Err(self.error("delete")) } } } + +#[cfg(test)] +mod tests { + use super::{ExactKeyMatch, contains_exact_key}; + + /// Page iterator that records how many pages were consumed. + struct CountingPages { + pages: std::vec::IntoIter, &'static str>>, + consumed: std::rc::Rc>, + } + + impl Iterator for CountingPages { + type Item = Result, &'static str>; + + fn next(&mut self) -> Option { + let page = self.pages.next(); + if page.is_some() { + self.consumed.set(self.consumed.get() + 1); + } + page + } + } + + fn counting( + pages: Vec, &'static str>>, + ) -> (CountingPages, std::rc::Rc>) { + let consumed = std::rc::Rc::new(std::cell::Cell::new(0)); + ( + CountingPages { + pages: pages.into_iter(), + consumed: std::rc::Rc::clone(&consumed), + }, + consumed, + ) + } + + fn page(keys: &[&str]) -> Result, &'static str> { + Ok(keys.iter().map(|key| (*key).to_owned()).collect()) + } + + #[test] + fn finds_a_match_on_a_later_page() { + let (pages, _) = counting(vec![ + page(&["wanted-suffix-a", "wanted-suffix-b"]), + page(&["wanted-suffix-c"]), + page(&["wanted"]), + ]); + + assert_eq!( + contains_exact_key(pages, "wanted", 8).expect("should scan the pages"), + ExactKeyMatch::Found, + "a match on the last page must still be found" + ); + } + + #[test] + fn ignores_keys_that_only_start_with_the_one_asked_for() { + let (pages, _) = counting(vec![page(&["wanted-suffix", "wantedx", "wanted2"])]); + + assert_eq!( + contains_exact_key(pages, "wanted", 8).expect("should scan the pages"), + ExactKeyMatch::Absent, + "a longer key is a different identity" + ); + } + + #[test] + fn stops_at_the_first_match() { + let (pages, consumed) = counting(vec![ + page(&["wanted"]), + page(&["never-read"]), + page(&["never-read-either"]), + ]); + + assert_eq!( + contains_exact_key(pages, "wanted", 8).expect("should scan the pages"), + ExactKeyMatch::Found, + "should find the match" + ); + assert_eq!(consumed.get(), 1, "should not read past the match"); + } + + #[test] + fn propagates_a_page_error_rather_than_reporting_absent() { + let (pages, _) = counting(vec![page(&["wanted-suffix"]), Err("list unavailable")]); + + assert_eq!( + contains_exact_key(pages, "wanted", 8), + Err("list unavailable"), + "an unreadable listing must not read as absent" + ); + } + + #[test] + fn reports_undetermined_when_the_listing_outruns_the_budget() { + // The key sits past the budget, which is exactly the case that must + // not read as absent: answering "absent" discards the withdrawal. + let (pages, consumed) = counting(vec![ + page(&["wanted-suffix-a"]), + page(&["wanted-suffix-b"]), + page(&["wanted"]), + ]); + + assert_eq!( + contains_exact_key(pages, "wanted", 2).expect("should scan the pages"), + ExactKeyMatch::Undetermined, + "a key beyond the budget must not read as absent" + ); + assert_eq!( + consumed.get(), + 3, + "should read one page past the budget to learn the listing continues" + ); + } + + #[test] + fn reports_absent_for_a_listing_that_ends_exactly_at_the_budget() { + let (pages, _) = counting(vec![page(&["wanted-suffix-a"]), page(&["wanted-suffix-b"])]); + + assert_eq!( + contains_exact_key(pages, "wanted", 2).expect("should scan the pages"), + ExactKeyMatch::Absent, + "a listing that fits the budget is answered definitively" + ); + } + + #[test] + fn finds_a_match_on_the_last_page_within_the_budget() { + let (pages, _) = counting(vec![page(&["wanted-suffix-a"]), page(&["wanted"])]); + + assert_eq!( + contains_exact_key(pages, "wanted", 2).expect("should scan the pages"), + ExactKeyMatch::Found, + "the final permitted page still counts" + ); + } + + #[test] + fn reports_absent_for_an_empty_listing() { + let (pages, _) = counting(Vec::new()); + + assert_eq!( + contains_exact_key(pages, "wanted", 8).expect("should scan the pages"), + ExactKeyMatch::Absent, + "nothing listed means nothing held" + ); + } +} diff --git a/crates/trusted-server-core/src/ec/mod.rs b/crates/trusted-server-core/src/ec/mod.rs index 840ce90d3..1e7396390 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}") } @@ -493,6 +499,30 @@ pub(crate) fn current_timestamp() -> u64 { #[cfg(test)] mod tests { + + #[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}"); + } use super::*; use crate::platform::test_support::noop_services; use crate::test_support::tests::create_test_settings;