diff --git a/crates/persistence/src/backends/elasticsearch/backend.rs b/crates/persistence/src/backends/elasticsearch/backend.rs index 99c8103c2..3096766ae 100644 --- a/crates/persistence/src/backends/elasticsearch/backend.rs +++ b/crates/persistence/src/backends/elasticsearch/backend.rs @@ -196,6 +196,7 @@ impl ElasticsearchBackend { /// Creates a new Elasticsearch backend with the given configuration. pub fn new(config: ElasticsearchConfig) -> StorageResult { + Self::validate_config(&config)?; let client = Self::build_client(&config)?; // Standalone ES has no store of its own, so tenants have no overlay: @@ -240,6 +241,7 @@ impl ElasticsearchBackend { config: ElasticsearchConfig, registries: Arc, ) -> StorageResult { + Self::validate_config(&config)?; let client = Self::build_client(&config)?; Ok(Self { @@ -325,14 +327,43 @@ impl ElasticsearchBackend { SearchParameterExtractor::new(self.registries.for_tenant(tenant_id)) } + /// Validates configuration that the index-name legality proof depends on. + /// + /// [`super::naming`] guarantees the *tenant segment* is Elasticsearch-legal, + /// but two of Elasticsearch's rules constrain the name as a whole: it must + /// not begin with `-`, `_` or `+`, and must not be `.` or `..`. Both are + /// discharged by the index prefix, since the prefix always comes first — so + /// the prefix itself has to be well-formed. Checked once here rather than on + /// every derivation. + fn validate_config(config: &ElasticsearchConfig) -> StorageResult<()> { + super::naming::validate_index_prefix(&config.index_prefix).map_err(|message| { + crate::error::StorageError::Backend(BackendError::Internal { + backend_name: "elasticsearch".to_string(), + message, + source: None, + }) + }) + } + /// Returns the index name for a tenant and resource type. + /// + /// Delegates to [`naming::index_name`](super::naming::index_name), the single + /// injective tenant → index derivation in this backend. It used to lowercase + /// the tenant id inline, which made tenants `ACME` and `acme` share an index + /// and every `_id`-addressed write cross the tenant boundary (issue #384). pub fn index_name(&self, tenant_id: &str, resource_type: &str) -> String { - format!( - "{}_{}_{}", - self.config.index_prefix, - tenant_id.to_lowercase(), - resource_type.to_lowercase() - ) + super::naming::index_name(&self.config.index_prefix, tenant_id, resource_type) + } + + /// Returns the glob matching every index belonging to `tenant_id`. + /// + /// Shares [`index_name`](Self::index_name)'s encoder by construction, so the + /// glob can never stop matching the indices that exist. Four call sites used + /// to hand-roll `format!("{prefix}_{tenant.to_lowercase()}_*")` instead; if + /// one of them had drifted from the exact name, `purge_tenant_data` would + /// have deleted nothing while reporting success. + pub(crate) fn tenant_index_pattern(&self, tenant_id: &str) -> String { + super::naming::tenant_index_pattern(&self.config.index_prefix, tenant_id) } /// Returns the ES document ID for a resource. @@ -626,25 +657,69 @@ mod tests { assert_eq!(config.nodes, vec!["http://localhost:9200"]); } + /// The method-level view of the injective derivation. The exhaustive + /// property tests (injectivity over an adversarial corpus, Elasticsearch + /// legality, round-tripping, glob/name agreement) live in + /// [`super::super::naming`]; this asserts only that the backend method is + /// actually wired to them. + /// + /// The test this replaces asserted `index_name("ACME", …) == "hfs_acme_…"` + /// — i.e. it pinned the #384 defect as intended behaviour. #[test] - fn test_index_name() { + fn index_name_delegates_to_the_injective_derivation() { let config = ElasticsearchConfig::default(); let backend = ElasticsearchBackend::new(config).unwrap(); + // Identity on an already-safe id: conforming deployments see no rename. assert_eq!(backend.index_name("acme", "Patient"), "hfs_acme_patient"); - assert_eq!( + // Case variants must NOT share an index (issue #384). + assert_ne!( backend.index_name("ACME", "Observation"), - "hfs_acme_observation" + backend.index_name("acme", "Observation") + ); + // The glob is derived from the same encoder as the exact name. + assert_eq!(backend.tenant_index_pattern("acme"), "hfs_acme_*"); + assert_ne!( + backend.tenant_index_pattern("ACME"), + backend.tenant_index_pattern("acme") ); } + /// `document_id` deliberately carries **no** tenant component, and this test + /// is the alarm if someone adds one. + /// + /// It is unnecessary: an Elasticsearch `_id` is unique only within its index, + /// and an injective `index_name` means every index belongs to exactly one + /// tenant — so no two tenants can ever contend for an `_id`. It would also be + /// actively harmful: changing `_id` changes the address of every document in + /// every deployment, including the conforming ones, and because `delete` + /// removes only the new `_id`, pre-upgrade documents would linger as + /// permanently-undeletable duplicate search hits. Issue #384 proposes this + /// change; it was considered and rejected for those reasons. #[test] - fn test_document_id() { + fn document_id_carries_no_tenant_component() { assert_eq!( ElasticsearchBackend::document_id("Patient", "123"), "Patient_123" ); } + /// The prefix is what keeps an index name from starting with a character + /// Elasticsearch reserves, so a bad one must fail at construction rather than + /// on the first write. + #[test] + fn construction_rejects_an_index_prefix_that_breaks_name_legality() { + for bad in ["", "_hfs", "-hfs", "+hfs", "HFS", "hfs/prod"] { + let config = ElasticsearchConfig { + index_prefix: bad.to_string(), + ..ElasticsearchConfig::default() + }; + assert!( + ElasticsearchBackend::new(config).is_err(), + "index prefix {bad:?} must be rejected" + ); + } + } + #[test] fn test_backend_capabilities() { let config = ElasticsearchConfig::default(); diff --git a/crates/persistence/src/backends/elasticsearch/mod.rs b/crates/persistence/src/backends/elasticsearch/mod.rs index dd6075613..5380f9481 100644 --- a/crates/persistence/src/backends/elasticsearch/mod.rs +++ b/crates/persistence/src/backends/elasticsearch/mod.rs @@ -16,7 +16,13 @@ //! # Index Structure //! //! Each tenant+resource type combination gets its own index: -//! `{prefix}_{tenant_id}_{resource_type_lowercase}` (e.g., `hfs_acme_patient`) +//! `{prefix}_{encoded_tenant_id}_{resource_type_lowercase}` (e.g. `hfs_acme_patient`). +//! +//! The tenant segment is produced by an **injective** encoding — see [`naming`], +//! which is the single derivation every index name and index glob in this backend +//! goes through. That injectivity is what confines each `_id`-addressed operation +//! (`create`, `update`, `delete`, and the `create_or_update` existence probe) to +//! exactly one tenant's index, since those operations cannot carry a query filter. //! //! Documents use nested objects for search parameters to ensure correct //! multi-value matching (e.g., system+code must co-occur in the same token). @@ -35,6 +41,7 @@ //! ``` mod backend; +mod naming; mod schema; pub mod search; mod search_impl; diff --git a/crates/persistence/src/backends/elasticsearch/naming.rs b/crates/persistence/src/backends/elasticsearch/naming.rs new file mode 100644 index 000000000..339e79f29 --- /dev/null +++ b/crates/persistence/src/backends/elasticsearch/naming.rs @@ -0,0 +1,467 @@ +//! The single tenant → Elasticsearch index-name derivation. +//! +//! # Why this module exists +//! +//! Every Elasticsearch operation addresses documents through an index name (and, +//! for the `_id`-addressed operations, a document id *within* that index). If two +//! distinct tenants can ever produce the same index name, then `create`, +//! `update`, `delete`, and the `create_or_update` existence probe — none of which +//! can carry a query filter, because `GET /{index}/_doc/{id}` admits none — will +//! read, overwrite, and delete each other's documents. +//! +//! The previous derivation was `format!("{prefix}_{tenant.to_lowercase()}_{type}")`. +//! `to_lowercase()` is not injective, so tenants `ACME` and `acme` shared an +//! index (issue #384). It was also *partial*: a tenant id containing `/` produced +//! a string Elasticsearch rejects outright, so every write for such a tenant +//! failed with a 500. +//! +//! [`encode_tenant_segment`] replaces it with a total, injective encoding. +//! +//! # The encoding +//! +//! Bytes of the UTF-8 encoding of the tenant id are mapped as follows: +//! +//! - a byte in the **safe set** `{a–z, 0–9, '-', '_', '.'}` is emitted verbatim; +//! - every other byte is emitted as `+` followed by two **lowercase** hex digits. +//! +//! `+` is not in the safe set, so a literal `+` is itself escaped (`+2b`) and can +//! never be confused with an escape introducer. +//! +//! ## Injectivity +//! +//! [`decode_tenant_segment`] is a total left inverse: scan left to right; on `+`, +//! consume exactly three bytes and emit the byte the two hex digits denote; +//! otherwise emit the byte. This is unambiguous because `+` never appears +//! un-escaped and escapes are fixed-width. A left inverse implies injectivity, so +//! distinct tenant ids always produce distinct segments. The escape is therefore +//! not forgeable either: `encode("+2f") == "+2b2f" != "+2f" == encode("/")`. +//! +//! This mirrors the pattern issue #271 established for the S3 tenant registry key +//! (`backends::s3::keyspace::registry_object_id`): an injective escape whose +//! introducer is escaped first, identity everywhere else. +//! +//! ## Legality as an Elasticsearch index name +//! +//! Elasticsearch requires index names to be lowercase; to exclude +//! `\ / * ? " < > | `, space, `,` and `#`; to not begin with `-`, `_` or `+`; to +//! not be `.` or `..`; and to be at most 255 bytes. +//! +//! The output alphabet is `{a–z, 0–9, '-', '_', '.', '+'}`, which contains none +//! of the excluded characters and no uppercase byte. The leading-character and +//! `.`/`..` rules are discharged by the *index prefix*, which always precedes the +//! tenant segment and is validated by [`validate_index_prefix`]. See +//! [`encode_tenant_segment`] for the length caveat. +//! +//! ## Why `+` and not `%` +//! +//! Both are legal in an Elasticsearch index name and both survive the client's +//! path encoder. `+` is preferred because (a) no HFS tenant-routing surface +//! accepts it, so it is only ever produced here and never carried in by a user, +//! and (b) `%` in a URL path is the classic double-decode hazard — an +//! intermediary that decodes once and re-forwards would turn `a%252Fb` into +//! `a%2Fb`, which Elasticsearch would then decode to a name containing `/`. A +//! spurious extra decode of `%2B` merely yields `+`, which is inert. +//! +//! # Identity on already-safe ids — a load-bearing property +//! +//! For any tenant id drawn from `[a-z0-9._-]*`, the encoding is the **identity**, +//! so the index name is byte-identical to the one the old derivation produced. +//! Deployments whose tenant ids are already lowercase therefore see no index +//! rename, no reindex, and no change of any kind on upgrade. +//! +//! The ids whose index names *do* change are exactly those that are already +//! broken today: mixed-case ids (colliding), ids containing `/` (500 on every +//! write), and exotic ids reachable only through the unvalidated JWT tenant claim +//! (see issue #385). The blast radius of the fix equals the blast radius of the +//! bug. This is asserted by +//! `already_safe_tenant_ids_are_unchanged_so_conforming_deployments_do_not_migrate`. + +/// Characters emitted verbatim by [`encode_tenant_segment`]. +/// +/// Chosen as the intersection of "legal in an Elasticsearch index name", +/// "lowercase", and "commonly present in a tenant id" — so that the encoding is +/// the identity on ids that are already safe. +/// +/// `_` is deliberately included even though it is also the field separator in +/// `{prefix}_{tenant}_{type}`. Excluding it would rename the index of every +/// deployment using a `my_tenant`-shaped id — the most common legitimate shape, +/// which has no bug — in exchange for making the tenant glob exact. That trade is +/// rejected: the glob's over-match (tenant `a`'s pattern `hfs_a_*` also matches +/// tenant `a_b`'s indices) is not a leak, because every glob-scoped query carries +/// a `{"term": {"tenant_id": …}}` filter on a `keyword` field, and that filter — +/// not the glob — is what isolates tenants. +fn is_safe_byte(b: u8) -> bool { + b.is_ascii_lowercase() || b.is_ascii_digit() || matches!(b, b'-' | b'_' | b'.') +} + +/// The escape introducer. Not a member of the safe set, so it is self-escaping. +const ESCAPE: u8 = b'+'; + +/// Encodes a tenant id into a single, injective, Elasticsearch-legal index-name +/// segment. +/// +/// See the module documentation for the encoding, its injectivity proof, and why +/// it is the identity on already-safe ids. +/// +/// # Length +/// +/// The encoding expands by at most 3× per byte. The full index name must be +/// ≤255 bytes, which holds comfortably for every id the validated routing +/// surfaces accept (`crates/rest/src/middleware/tenant_prefix.rs` caps at 64 +/// characters; `crates/rest/src/handlers/admin_tenants.rs` at 128). An id long +/// enough to overflow can only arrive through the unvalidated JWT tenant claim, +/// and Elasticsearch rejects the oversized name with a 400 that surfaces as a +/// storage error — loud and non-lossy. Truncating here to fit would reintroduce +/// exactly the non-injective derivation this module exists to remove, so it is +/// deliberately not done. Bounding the id globally is issue #385's job. +pub(crate) fn encode_tenant_segment(tenant_id: &str) -> String { + let bytes = tenant_id.as_bytes(); + // Fast path: an already-safe id is returned unchanged without allocating a + // byte at a time. This is the overwhelmingly common case on every request. + if bytes.iter().all(|b| is_safe_byte(*b)) { + return tenant_id.to_string(); + } + + // Lowercase hex: the whole index name must be lowercase, so the digits are + // written from this table rather than via `{:X}`. + const HEX: &[u8; 16] = b"0123456789abcdef"; + + let mut out = String::with_capacity(bytes.len() + 8); + for &b in bytes { + if is_safe_byte(b) { + out.push(b as char); + } else { + out.push(ESCAPE as char); + out.push(HEX[(b >> 4) as usize] as char); + out.push(HEX[(b & 0x0f) as usize] as char); + } + } + out +} + +/// The left inverse of [`encode_tenant_segment`]. +/// +/// Exists to make injectivity a *tested* property rather than an argued one — +/// `decode(encode(x)) == x` over an adversarial corpus is what the unit tests +/// assert. Returns `None` for input that is not well-formed encoder output. +#[cfg(test)] +pub(crate) fn decode_tenant_segment(segment: &str) -> Option { + let bytes = segment.as_bytes(); + let mut out: Vec = Vec::with_capacity(bytes.len()); + let mut i = 0; + while i < bytes.len() { + if bytes[i] == ESCAPE { + // Escapes are fixed-width, so there is no alignment ambiguity. + let hex = bytes.get(i + 1..i + 3)?; + let hex = std::str::from_utf8(hex).ok()?; + out.push(u8::from_str_radix(hex, 16).ok()?); + i += 3; + } else { + out.push(bytes[i]); + i += 1; + } + } + String::from_utf8(out).ok() +} + +/// Builds the index name for one tenant and resource type. +/// +/// `resource_type` is lowercased rather than escaped. That is safe — and, unlike +/// the tenant id, *forced*: Elasticsearch index names must be lowercase, and FHIR +/// resource types are a closed, fixed set whose members never differ only by +/// case, so lowercasing is injective over the actual domain. +pub(crate) fn index_name(index_prefix: &str, tenant_id: &str, resource_type: &str) -> String { + format!( + "{}_{}_{}", + index_prefix, + encode_tenant_segment(tenant_id), + resource_type.to_lowercase() + ) +} + +/// Builds the glob matching every index belonging to one tenant. +/// +/// **This must stay derived from the same encoder as [`index_name`].** If the two +/// ever diverge, the glob stops matching the indices that exist and the +/// glob-scoped operations fail silently in the worst possible direction: +/// `purge_tenant_data` deletes nothing and reports success, and `count` returns +/// zero. That is why all four formerly hand-rolled `{prefix}_{tenant}_*` literals +/// now route through this function. +/// +/// The glob narrows *which indices are scanned*; the `{"term": {"tenant_id": …}}` +/// filter that every caller supplies is what *isolates the tenant*. See +/// [`is_safe_byte`] for why the glob is deliberately allowed to over-match. +pub(crate) fn tenant_index_pattern(index_prefix: &str, tenant_id: &str) -> String { + format!("{}_{}_*", index_prefix, encode_tenant_segment(tenant_id)) +} + +/// Validates an operator-supplied index prefix. +/// +/// The legality proofs in this module lean on the prefix: it is what guarantees +/// an index name never begins with `-`, `_` or `+` and is never `.` or `..`, +/// since the prefix always comes first. Validating it once here discharges both +/// obligations for every name the module produces. +pub(crate) fn validate_index_prefix(prefix: &str) -> Result<(), String> { + let first = prefix.bytes().next().ok_or_else(|| { + "Elasticsearch index prefix must not be empty (it is what keeps every index name from \ + starting with a reserved character)" + .to_string() + })?; + if !(first.is_ascii_lowercase() || first.is_ascii_digit()) { + return Err(format!( + "Elasticsearch index prefix {prefix:?} must start with a lowercase letter or digit; \ + Elasticsearch rejects index names beginning with '-', '_' or '+'" + )); + } + if let Some(bad) = prefix.bytes().find(|b| !is_safe_byte(*b)) { + return Err(format!( + "Elasticsearch index prefix {prefix:?} contains the illegal byte {:?}; only \ + lowercase letters, digits, '-', '_' and '.' are allowed", + bad as char + )); + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + /// Tenant ids chosen so that each one kills a specific way a derivation can + /// lose information. A pair that a broken derivation would conflate must be + /// present for every such mechanism, or the injectivity test passes + /// vacuously. + const COLLIDING_TENANTS: &[&str] = &[ + // ASCII case — the actual #384 defect. + "acme", + "ACME", + "AcMe", + // Separator ambiguity in `{prefix}_{tenant}_{type}`. + "a", + "a_b", + "a_b_c", + // The classes a lossy sanitiser (`'/' | '\\' | ' ' => '_'`) collapses. + "a/b", + "a\\b", + "a b", + // Escape forgery: a literal spelling of an escape must not collide with + // the thing it would encode. + "a+2fb", + "+2f", + "/", + // Leading/trailing separators — kills any `trim_matches('/')`. + "/a", + "a/", + "//a", + // Unicode normalisation — kills a "fix" that NFKC-normalises rather than + // escapes. U+FB01 vs "fi", and U+00C5 vs U+212B. + "\u{fb01}le", + "file", + "\u{c5}", + "\u{212b}", + // Non-ASCII generally. + "café", + // Truncation — two ids sharing a long common prefix. + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-x", + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-y", + // Control-plane and hierarchy shapes that reach storage unvalidated. + "__system__", + "acme/research", + "tenant-123", + "my_tenant", + ]; + + const TYPES: &[&str] = &["Patient", "Observation", "DiagnosticReport"]; + + /// Every character Elasticsearch forbids in an index name. + const ES_FORBIDDEN: &[char] = &['\\', '/', '*', '?', '"', '<', '>', '|', ' ', ',', '#']; + + /// The core invariant. Distinct tenants must never address the same index — + /// this is what makes every `_id`-addressed operation tenant-confined, and it + /// is why `document_id` needs no tenant component. + #[test] + fn index_name_is_injective_in_the_tenant() { + for rt in TYPES { + for (i, a) in COLLIDING_TENANTS.iter().enumerate() { + for b in COLLIDING_TENANTS.iter().skip(i + 1) { + assert_ne!( + index_name("hfs", a, rt), + index_name("hfs", b, rt), + "tenants {a:?} and {b:?} must not share an index for {rt}" + ); + } + } + } + } + + /// Injectivity stated as its mechanism, so a future encoding change has to + /// keep the property rather than just keep the test green: a total left + /// inverse exists. + #[test] + fn encoding_round_trips_so_it_cannot_be_lossy() { + for t in COLLIDING_TENANTS { + let encoded = encode_tenant_segment(t); + assert_eq!( + decode_tenant_segment(&encoded).as_deref(), + Some(*t), + "encode/decode must round-trip for {t:?} (encoded as {encoded:?})" + ); + } + } + + /// The escape must not be forgeable: a tenant that literally spells an escape + /// sequence must not collide with the tenant that encoding produces it from. + #[test] + fn escape_sequences_cannot_be_forged() { + assert_ne!(encode_tenant_segment("/"), encode_tenant_segment("+2f")); + assert_ne!(encode_tenant_segment("a/b"), encode_tenant_segment("a+2fb")); + // The introducer is itself escaped, which is what makes the above hold. + assert_eq!(encode_tenant_segment("+"), "+2b"); + } + + /// Producing an injective name is not enough — Elasticsearch has to accept + /// it. This models the cluster's rules; the integration test is the oracle. + #[test] + fn every_index_name_is_legal_for_elasticsearch() { + for rt in TYPES { + for t in COLLIDING_TENANTS { + let name = index_name("hfs", t, rt); + assert_eq!(name, name.to_lowercase(), "{name:?} must be lowercase"); + for c in ES_FORBIDDEN { + assert!( + !name.contains(*c), + "{name:?} must not contain {c:?} (tenant {t:?})" + ); + } + assert!( + !name.starts_with('-') && !name.starts_with('_') && !name.starts_with('+'), + "{name:?} must not start with a reserved character" + ); + assert!(name != "." && name != "..", "{name:?} is a reserved name"); + assert!( + name.len() <= 255, + "{name:?} is {} bytes; Elasticsearch caps index names at 255", + name.len() + ); + } + } + } + + /// The property that confines the upgrade's blast radius to deployments that + /// are already broken. If this fails, every conforming deployment on earth + /// needs a reindex — see the module docs. + #[test] + fn already_safe_tenant_ids_are_unchanged_so_conforming_deployments_do_not_migrate() { + for t in ["acme", "default", "tenant-123", "my_tenant", "t.1", "a1"] { + assert_eq!(encode_tenant_segment(t), t, "{t:?} must encode to itself"); + // Byte-identical to what the pre-fix `to_lowercase()` derivation + // produced for these ids. + assert_eq!( + index_name("hfs", t, "Patient"), + format!("hfs_{}_patient", t.to_lowercase()) + ); + } + } + + /// One golden value, deliberately, so the encoding's readability is + /// reviewable. Safe because `acme` is already lowercase and slash-free, so + /// this constrains nothing about case or escape handling and cannot re-pin + /// the #384 defect the way the test it replaces did. + #[test] + fn plain_lowercase_tenant_index_name_documents_the_shape() { + assert_eq!(index_name("hfs", "acme", "Patient"), "hfs_acme_patient"); + } + + /// Regression for #384 as filed: the two ids named in the issue must land in + /// different indices. The test this replaces asserted the opposite. + #[test] + fn case_variant_tenants_do_not_share_an_index() { + assert_ne!( + index_name("hfs", "ACME", "Observation"), + index_name("hfs", "acme", "Observation") + ); + } + + /// The other half of #384: a hierarchical id used to produce a string + /// Elasticsearch rejects, so every write 500'd. + #[test] + fn hierarchical_tenant_ids_produce_a_legal_index_name() { + let name = index_name("hfs", "acme/research", "Patient"); + assert_eq!(name, "hfs_acme+2fresearch_patient"); + assert!(!name.contains('/')); + } + + /// The glob and the exact name must agree, or glob-scoped operations sweep + /// indices that do not exist — `purge_tenant_data` silently purging nothing. + #[test] + fn tenant_pattern_matches_every_index_name_for_that_tenant() { + for t in COLLIDING_TENANTS { + let pattern = tenant_index_pattern("hfs", t); + let stem = pattern + .strip_suffix('*') + .expect("pattern is a prefix glob by construction"); + for rt in TYPES { + let name = index_name("hfs", t, rt); + assert!( + name.starts_with(stem), + "pattern {pattern:?} must match index {name:?}" + ); + } + } + } + + /// The converse hazard: a pattern that matches a case-variant tenant's + /// indices would re-open #384 through the glob paths. + #[test] + fn tenant_pattern_does_not_match_a_case_variant_tenants_index() { + let stem = tenant_index_pattern("hfs", "acme"); + let stem = stem.strip_suffix('*').unwrap(); + assert!(!index_name("hfs", "ACME", "Patient").starts_with(stem)); + } + + /// The index template registers mappings against `{prefix}_*`. A name outside + /// that glob would be auto-created with *dynamic* mapping instead — no error, + /// no exception, search quality silently degraded. + #[test] + fn every_index_name_is_covered_by_the_index_template_glob() { + for t in COLLIDING_TENANTS { + for rt in TYPES { + assert!( + index_name("hfs", t, rt).starts_with("hfs_"), + "index for {t:?}/{rt} must fall under the template pattern" + ); + } + } + } + + /// The startup diagnostic in `schema.rs` decides "is this document in the + /// right index?" by re-encoding its `tenant_id` and comparing against the + /// index's own tenant segment. That only detects the #384 collision because + /// the encoder maps `ACME` somewhere other than `acme` — which the old + /// lowercasing derivation did not. + #[test] + fn re_encoding_a_tenant_id_identifies_a_misplaced_document() { + // A document written by tenant `ACME` under the pre-fix derivation sits + // in the index whose tenant segment is `acme`. Re-encoding must disagree. + assert_ne!(encode_tenant_segment("ACME"), "acme"); + // A document written by tenant `acme` sits where it belongs. + assert_eq!(encode_tenant_segment("acme"), "acme"); + } + + #[test] + fn index_prefix_validation_rejects_prefixes_that_break_the_legality_proof() { + assert!(validate_index_prefix("hfs").is_ok()); + assert!(validate_index_prefix("hfs-prod").is_ok()); + assert!(validate_index_prefix("h2").is_ok()); + // Empty would let the tenant segment lead the index name. + assert!(validate_index_prefix("").is_err()); + // Elasticsearch rejects names beginning with these. + assert!(validate_index_prefix("_hfs").is_err()); + assert!(validate_index_prefix("-hfs").is_err()); + assert!(validate_index_prefix("+hfs").is_err()); + // Uppercase and forbidden characters. + assert!(validate_index_prefix("HFS").is_err()); + assert!(validate_index_prefix("hfs/prod").is_err()); + assert!(validate_index_prefix("hfs prod").is_err()); + } +} diff --git a/crates/persistence/src/backends/elasticsearch/schema.rs b/crates/persistence/src/backends/elasticsearch/schema.rs index 7c43b8aa3..4eb407817 100644 --- a/crates/persistence/src/backends/elasticsearch/schema.rs +++ b/crates/persistence/src/backends/elasticsearch/schema.rs @@ -251,6 +251,10 @@ pub async fn create_index_template(backend: &ElasticsearchBackend) -> StorageRes pattern ); + // Startup is the one moment an operator is reading these logs, so it is where + // a pre-fix index layout gets surfaced. Best effort — never fails startup. + warn_on_misplaced_documents(backend).await; + Ok(()) } @@ -322,46 +326,133 @@ pub async fn ensure_index( Ok(()) } -/// Deletes an index for the given tenant and resource type. -#[allow(dead_code)] -pub async fn delete_index( - backend: &ElasticsearchBackend, - tenant_id: &str, - resource_type: &str, -) -> StorageResult<()> { - let index = backend.index_name(tenant_id, resource_type); +// `delete_index` — a whole-index `DELETE` addressed by index name alone — was +// removed here (issue #384). It was `#[allow(dead_code)]` with no callers, and it +// was safe only because `index_name` is injective. Keeping an unreachable, +// untested whole-index drop around is a latent footgun: the first caller to wire +// it up would not re-derive that argument. Its plausible use — tenant offboarding +// — is already served, document-level and tenant-term-filtered, by +// `ResourceStorage::purge_tenant_data` and `PurgableStorage::purge_all`. - let response = backend +/// Warns, once at startup, about documents sitting in an index that the current +/// tenant → index derivation would not put them in. +/// +/// # Why this exists +/// +/// The #384 fix makes the derivation injective. For a tenant id that was already +/// lowercase and Elasticsearch-safe the encoding is the identity, so nothing +/// moves and this reports nothing. A deployment that actually had a +/// non-conforming tenant id, however, now addresses a *different* index, and its +/// pre-upgrade documents stay where the old derivation put them. The symptom is +/// silent: that tenant's search results go empty (or, worse, stay partial) while +/// reads, writes, and history — all served by the primary — look perfectly +/// healthy. Nobody reindexes an index they do not know is wrong. +/// +/// # Why this compares documents, not index names +/// +/// The obvious check — "is this index name something the encoder could have +/// produced?" — would miss the very case the issue is about. The old derivation +/// *lowercased*, so tenant `ACME` wrote to `{prefix}_acme_patient`, which is a +/// perfectly well-formed name for tenant `acme`. There is no malformed name to +/// spot. What is actually wrong is the *contents*: that index holds documents +/// whose `tenant_id` is `ACME`, which this build would place in +/// `{prefix}_+41+43+4d+45_patient`. +/// +/// So this aggregates the distinct `tenant_id` values present in each index and +/// flags any whose encoded form does not match the index's own tenant segment. +/// That detects both the collision case and any stranded-index case, and it needs +/// no heuristic about name shape. +/// +/// # Deliberate limits +/// +/// - **Best effort; never fails startup.** Misplaced documents are inert — no +/// query path reaches them across a tenant boundary (`read` re-checks +/// `tenant_id`, every glob-scoped query carries a `term` filter), so refusing to +/// boot would turn a search-completeness problem into a total outage. An +/// unreachable cluster is silently ignored here; `health_check` reports that. +/// - **One aggregation, at startup only.** Not on the write path. +/// - It reports the condition; it does not repair it. Remediation is `$reindex` +/// for the affected tenant, then a delete-by-query filtered on that tenant's +/// exact `tenant_id` to remove the strays. +async fn warn_on_misplaced_documents(backend: &ElasticsearchBackend) { + let prefix = &backend.config().index_prefix; + let pattern = format!("{prefix}_*"); + + let response = match backend .client() - .indices() - .delete(elasticsearch::indices::IndicesDeleteParts::Index(&[&index])) + .search(elasticsearch::SearchParts::Index(&[&pattern])) + .body(json!({ + "size": 0, + "aggs": { + "per_index": { + "terms": { "field": "_index", "size": 1000 }, + "aggs": { + "tenants": { "terms": { "field": "tenant_id", "size": 100 } } + } + } + } + })) + .allow_no_indices(true) + .ignore_unavailable(true) .send() .await - .map_err(|e| { - crate::error::StorageError::Backend(BackendError::Internal { - backend_name: "elasticsearch".to_string(), - message: format!("Failed to delete index {}: {}", index, e), - source: None, - }) - })?; + { + Ok(r) if r.status_code().is_success() => r, + // Unreachable cluster, or a cluster with no indices yet. Not this + // function's job to report — stay silent rather than mislead. + _ => return, + }; - let status = response.status_code(); - if !status.is_success() { - let body = response.text().await.unwrap_or_default(); - // 404 is OK (index doesn't exist) - if !body.contains("index_not_found_exception") { - return Err(crate::error::StorageError::Backend( - BackendError::Internal { - backend_name: "elasticsearch".to_string(), - message: format!("Failed to delete index {}: {}", index, body), - source: None, - }, - )); + let Ok(body) = response.json::().await else { + return; + }; + let Some(index_buckets) = body + .pointer("/aggregations/per_index/buckets") + .and_then(|b| b.as_array()) + else { + return; + }; + + let index_prefix = format!("{prefix}_"); + for index_bucket in index_buckets { + let Some(index) = index_bucket.get("key").and_then(|k| k.as_str()) else { + continue; + }; + // `{prefix}_{tenant}_{type}`: the tenant segment is everything between + // the prefix and the final `_`. + let Some((tenant_segment, type_segment)) = index + .strip_prefix(&index_prefix) + .and_then(|rest| rest.rsplit_once('_')) + else { + continue; + }; + + let tenant_buckets = index_bucket + .pointer("/tenants/buckets") + .and_then(|b| b.as_array()) + .map(Vec::as_slice) + .unwrap_or_default(); + + for tenant_bucket in tenant_buckets { + let Some(tenant_id) = tenant_bucket.get("key").and_then(|k| k.as_str()) else { + continue; + }; + if super::naming::encode_tenant_segment(tenant_id) == tenant_segment { + continue; + } + tracing::warn!( + index = %index, + tenant_id = %tenant_id, + expected_index = %super::naming::index_name(prefix, tenant_id, type_segment), + doc_count = tenant_bucket.get("doc_count").and_then(|c| c.as_u64()).unwrap_or(0), + "Elasticsearch documents predate the injective tenant-index naming fix \ + (issue #384): they sit in an index this build would not write them to, so \ + they are invisible to that tenant's searches. Run `$reindex` for this \ + tenant, then remove the strays with a delete-by-query filtered on this \ + exact `tenant_id`." + ); } } - - tracing::debug!("Deleted Elasticsearch index '{}'", index); - Ok(()) } #[cfg(test)] diff --git a/crates/persistence/src/backends/elasticsearch/storage.rs b/crates/persistence/src/backends/elasticsearch/storage.rs index 45ef67b41..a2d7b64b8 100644 --- a/crates/persistence/src/backends/elasticsearch/storage.rs +++ b/crates/persistence/src/backends/elasticsearch/storage.rs @@ -381,11 +381,7 @@ impl ElasticsearchBackend { container_type: &str, container_id: &str, ) -> StorageResult<()> { - let pattern = format!( - "{}_{}_*", - self.config().index_prefix, - tenant_id.to_lowercase() - ); + let pattern = self.tenant_index_pattern(tenant_id); let body = json!({ "query": { "bool": { "filter": [ { "term": { "tenant_id": tenant_id } }, @@ -610,13 +606,49 @@ impl ResourceStorage for ElasticsearchBackend { let (version_id, is_new) = match existing { Ok(resp) if resp.status_code().is_success() => { let body = resp.json::().await.unwrap_or_default(); - let current_version: u64 = body - .get("_source") - .and_then(|s| s.get("version_id")) + let source = body.get("_source"); + // Belt-and-braces tenant guard, mirroring `read` (see below). + // + // After the #384 fix an injective `index_name` already guarantees + // this index belongs to exactly one tenant, so this check should + // never fire. It is kept because this is the one place the backend + // reads *foreign state* to make a *write* decision, and a future + // regression in the naming derivation would otherwise silently + // resume deriving one tenant's version from another's document. + // + // A mismatch is treated as **absent**, not as an error. An index + // upgraded from the pre-fix layout can still hold documents left + // behind by a colliding tenant; erroring would brick the rightful + // owner on exactly those ids, permanently, with no operator + // remedy. Treating them as absent is self-healing — the foreign + // document is overwritten and leaves an index it never belonged + // in — and is correct on the merits: from this tenant's + // perspective the resource genuinely does not exist. Resetting the + // version is harmless because Elasticsearch keeps no history and, + // in every supported composite mode, the primary is authoritative + // for version assignment (writes always land there first). + let doc_tenant = source + .and_then(|s| s.get("tenant_id")) .and_then(|v| v.as_str()) - .and_then(|v| v.parse().ok()) - .unwrap_or(0); - ((current_version + 1).to_string(), false) + .unwrap_or(""); + if doc_tenant != tenant_id { + tracing::warn!( + tenant = %tenant_id, + found_tenant = %doc_tenant, + resource_type, + id, + "Elasticsearch document at this address belongs to another tenant; \ + treating as absent and overwriting (see issue #384)" + ); + ("1".to_string(), true) + } else { + let current_version: u64 = source + .and_then(|s| s.get("version_id")) + .and_then(|v| v.as_str()) + .and_then(|v| v.parse().ok()) + .unwrap_or(0); + ((current_version + 1).to_string(), false) + } } Ok(resp) if resp.status_code().as_u16() == 404 => ("1".to_string(), true), Ok(resp) => { @@ -908,11 +940,7 @@ impl ResourceStorage for ElasticsearchBackend { let index_pattern = match resource_type { Some(rt) => self.index_name(tenant_id, rt), - None => format!( - "{}_{}_*", - self.config().index_prefix, - tenant_id.to_lowercase() - ), + None => self.tenant_index_pattern(tenant_id), }; let query = json!({ @@ -962,9 +990,10 @@ impl ResourceStorage for ElasticsearchBackend { // (in `*-elasticsearch` modes the primary's own search index is empty). async fn purge_tenant_data(&self, id: &str) -> StorageResult { // Documents are matched by an exact `tenant_id` term, not by the index - // pattern alone: index names lowercase the tenant id, so the pattern - // for tenant `acme` would also sweep `acme_corp`'s indices. - let pattern = format!("{}_{}_*", self.config().index_prefix, id.to_lowercase()); + // pattern alone: the pattern is a prefix glob, so tenant `a`'s pattern + // `{prefix}_a_*` also matches tenant `a_b`'s indices. The term filter, + // not the glob, is what bounds this to one tenant. + let pattern = self.tenant_index_pattern(id); let body = json!({ "query": { "bool": { "filter": [ { "term": { "tenant_id": id } } @@ -1199,9 +1228,10 @@ impl ReindexTarget for ElasticsearchBackend { // MUST be a delete-by-query with a `tenant_id` term filter, never a // delete of the indices matching the tenant's index pattern. The // pattern `{prefix}_{tenant}_*` is a prefix glob, so tenant "a" matches - // tenant "ab"'s indices — deleting by pattern would destroy a - // prefix-sharing tenant's data. The term filter is what actually bounds - // this to one tenant; the pattern only narrows which indices to scan. + // tenant "a_b"'s indices — deleting by pattern would destroy a + // separator-sharing tenant's data. The term filter is what actually + // bounds this to one tenant; the pattern only narrows which indices to + // scan. See `tenant_index_pattern` for why the over-match is deliberate. let pattern = tenant_index_pattern(self, tenant_id); delete_by_query_scoped( self, @@ -1348,16 +1378,19 @@ impl ReindexSource for ElasticsearchBackend { /// The index glob covering every one of a tenant's type indices. /// -/// This is a *prefix* glob: tenant "a" also matches tenant "ab"'s indices. -/// Every query built on it MUST also carry a `tenant_id` term filter — the -/// pattern narrows which indices are scanned, the filter is what enforces -/// tenant isolation. +/// This is a *prefix* glob, so it can over-match. The example previously given +/// here — tenant "a" matching tenant "ab" — was wrong: the pattern is +/// `{prefix}_a_*`, which requires the literal separator, so "ab" does not match. +/// The real case is the *underscore-bearing* one: tenant "a"'s `{prefix}_a_*` +/// does match tenant "a_b"'s `{prefix}_a_b_patient`. +/// +/// That over-match is deliberate — `_` is kept in the encoder's safe set so that +/// `my_tenant`-shaped ids need no escaping and conforming deployments never see +/// an index rename (see [`super::naming`]). Every query built on this glob MUST +/// therefore also carry a `tenant_id` term filter: the pattern narrows which +/// indices are scanned, the filter is what enforces tenant isolation. fn tenant_index_pattern(backend: &ElasticsearchBackend, tenant_id: &str) -> String { - format!( - "{}_{}_*", - backend.config().index_prefix, - tenant_id.to_lowercase() - ) + backend.tenant_index_pattern(tenant_id) } /// Runs a delete-by-query and returns how many documents it removed. diff --git a/crates/persistence/tests/elasticsearch_tests.rs b/crates/persistence/tests/elasticsearch_tests.rs index a1c2dd265..06a347a6c 100644 --- a/crates/persistence/tests/elasticsearch_tests.rs +++ b/crates/persistence/tests/elasticsearch_tests.rs @@ -71,19 +71,40 @@ fn test_backend_capabilities() { assert!(!backend.supports(BackendCapability::Versioning)); } +/// Index names must be injective in the tenant, because every `_id`-addressed +/// operation (`create`, `update`, `delete`, and the `create_or_update` existence +/// probe) is confined to a tenant *only* by the index it targets — those APIs +/// admit no query filter. +/// +/// This replaces a test that asserted `index_name("acme", …)` and +/// `index_name("tenant-1", …)` only. Neither input exercised the derivation's +/// lossy step, so the suite was green while `ACME` and `acme` shared an index +/// (issue #384). The exhaustive property tests live beside the encoder in +/// `backends/elasticsearch/naming.rs`. #[test] -fn test_index_name() { +fn test_index_name_is_injective_in_the_tenant() { let config = ElasticsearchConfig { index_prefix: "hfs".to_string(), ..Default::default() }; let backend = ElasticsearchBackend::new(config).unwrap(); + // Already-safe ids are unchanged, so conforming deployments do not migrate. assert_eq!(backend.index_name("acme", "Patient"), "hfs_acme_patient"); assert_eq!( backend.index_name("tenant-1", "Observation"), "hfs_tenant-1_observation" ); + + // Case variants must not collide. + assert_ne!( + backend.index_name("ACME", "Patient"), + backend.index_name("acme", "Patient") + ); + // A hierarchical id used to produce an illegal name and 500 on every write. + let hierarchical = backend.index_name("acme/research", "Patient"); + assert!(!hierarchical.contains('/')); + assert_ne!(hierarchical, backend.index_name("acmeresearch", "Patient")); } // ============================================================================ @@ -1068,6 +1089,162 @@ mod es_integration { assert_eq!(read_b.content()["name"][0]["family"], "B"); } + /// Regression for issue #384, against a real cluster. + /// + /// The two isolation tests above use `tenant-a`/`tenant-b`, which differ in a + /// way the old lossy derivation *preserved* — so they passed while tenants + /// differing only by case shared an index and a document `_id`, and could + /// read, overwrite, and delete each other's documents. + /// + /// **The lesson to keep: an isolation test must use the tenant pair its + /// identifier derivation is most likely to conflate.** + /// + /// This exercises every `_id`-addressed path the issue names — the + /// `create_or_update` existence probe, `update`, and `delete` — plus the + /// glob-scoped paths (`count`, `clear_search_index`), because those derive + /// their index pattern separately. A fix that escapes `index_name` but leaves + /// a glob lowercased passes every unit test and fails here. + #[tokio::test] + async fn es_integration_case_variant_tenants_are_not_the_same_tenant() { + let backend = create_backend().await; + let upper = create_tenant("Acme"); + let lower = create_tenant("acme"); + + // 1. `Acme` writes. + backend + .create_or_update( + &upper, + "Patient", + "shared-id", + json!({"resourceType": "Patient", "name": [{"family": "UPPER"}]}), + FhirVersion::default(), + ) + .await + .unwrap(); + + // 2. `acme` must not observe it — this is the existence probe, which + // reads a document's version to decide "is this new?". + assert!( + !backend + .exists(&lower, "Patient", "shared-id") + .await + .unwrap(), + "tenant `acme` must not see tenant `Acme`'s document" + ); + + // 3. `acme` writes to the same logical id. + backend + .create_or_update( + &lower, + "Patient", + "shared-id", + json!({"resourceType": "Patient", "name": [{"family": "lower"}]}), + FhirVersion::default(), + ) + .await + .unwrap(); + + // 4. `Acme`'s document must survive intact. On `main` this fails in an + // instructive way: the shared document's `_source.tenant_id` becomes + // "acme", so `read`'s tenant re-check returns None and the resource + // *vanishes* for its owner. Asserting the content catches both the + // vanish and the silent-overwrite variants. + let read_upper = backend + .read(&upper, "Patient", "shared-id") + .await + .unwrap() + .expect("tenant `Acme`'s document must still exist"); + assert_eq!(read_upper.content()["name"][0]["family"], "UPPER"); + + let read_lower = backend + .read(&lower, "Patient", "shared-id") + .await + .unwrap() + .expect("tenant `acme`'s document must exist"); + assert_eq!(read_lower.content()["name"][0]["family"], "lower"); + + // 5. `acme`'s update must not touch `Acme`'s document. Without this, a + // fix that hardens `create_or_update` but not `update` would pass. + backend + .update( + &lower, + &read_lower, + json!({"resourceType": "Patient", "name": [{"family": "lower-2"}]}), + ) + .await + .unwrap(); + assert_eq!( + backend + .read(&upper, "Patient", "shared-id") + .await + .unwrap() + .expect("still present after the other tenant's update") + .content()["name"][0]["family"], + "UPPER" + ); + + // 6. `acme`'s delete must not remove `Acme`'s document. + backend + .delete(&lower, "Patient", "shared-id") + .await + .unwrap(); + assert_eq!( + backend + .read(&upper, "Patient", "shared-id") + .await + .unwrap() + .expect("tenant `Acme`'s document must survive `acme`'s delete") + .content()["name"][0]["family"], + "UPPER" + ); + + // 7. Both directions of the glob-scoped count. Asserting only one would + // catch only one of the two ways the name/glob pair can drift apart. + backend.refresh_index("Acme", "Patient").await.ok(); + backend.refresh_index("acme", "Patient").await.ok(); + assert_eq!( + backend.count(&upper, Some("Patient")).await.unwrap(), + 1, + "tenant `Acme` must still count its own document" + ); + assert_eq!( + backend.count(&lower, Some("Patient")).await.unwrap(), + 0, + "tenant `acme` deleted its only document" + ); + } + + /// The unit tests model Elasticsearch's index-naming rules; this is the + /// oracle. An encoding can be provably injective and still be rejected by a + /// real cluster, which no pure test can catch. + #[tokio::test] + async fn es_integration_non_conforming_tenant_ids_are_accepted_by_elasticsearch() { + let backend = create_backend().await; + + // Uppercase (issue #384's collision) and hierarchical (which previously + // produced an illegal index name and 500'd on every write). + for tenant_id in ["ACME", "acme/research", "Acme.Corp"] { + let tenant = create_tenant(tenant_id); + backend + .create_or_update( + &tenant, + "Patient", + "p1", + json!({"resourceType": "Patient", "name": [{"family": tenant_id}]}), + FhirVersion::default(), + ) + .await + .unwrap_or_else(|e| panic!("write for tenant {tenant_id:?} must succeed: {e}")); + + let read = backend + .read(&tenant, "Patient", "p1") + .await + .unwrap() + .unwrap_or_else(|| panic!("read back for tenant {tenant_id:?}")); + assert_eq!(read.content()["name"][0]["family"], tenant_id); + } + } + #[tokio::test] async fn es_integration_tenant_isolation_search() { use helios_persistence::core::SearchProvider; diff --git a/crates/persistence/tests/sqlite_tests.rs b/crates/persistence/tests/sqlite_tests.rs index 83251749c..a5e333e46 100644 --- a/crates/persistence/tests/sqlite_tests.rs +++ b/crates/persistence/tests/sqlite_tests.rs @@ -416,6 +416,86 @@ async fn test_same_id_different_tenants() { assert_eq!(read_b.content()["name"][0]["family"], "B"); } +/// Tenants differing only by case must be distinct tenants here, as they are in +/// every other backend. +/// +/// SQLite is already correct: `tenant_id TEXT NOT NULL` under the default BINARY +/// collation compares byte-exactly. This test exists so it *stays* correct. The +/// risk is concrete rather than theoretical — `COLLATE NOCASE` is used liberally +/// in this backend's search SQL (`search/parameter_handlers/string.rs`, +/// `token.rs`, `reference.rs`, `search_impl.rs`), so the idiom is one copy-paste +/// away from a tenant predicate, and a `citext`-style migration would do the same +/// on PostgreSQL. +/// +/// Added alongside the Elasticsearch fix for issue #384, where the equivalent +/// property did *not* hold: the index name lowercased the tenant id, so `Acme` +/// and `acme` shared an index and a document `_id`. +#[tokio::test] +async fn test_case_variant_tenants_do_not_collide() { + let backend = create_backend(); + let upper = create_tenant("Acme"); + let lower = create_tenant("acme"); + + backend + .create_or_update( + &upper, + "Patient", + "shared-id", + json!({"resourceType": "Patient", "name": [{"family": "UPPER"}]}), + FhirVersion::default(), + ) + .await + .unwrap(); + + // The case variant must not observe it... + assert!( + backend + .read(&lower, "Patient", "shared-id") + .await + .unwrap() + .is_none(), + "tenant `acme` must not read tenant `Acme`'s resource" + ); + + // ...must not overwrite it... + backend + .create_or_update( + &lower, + "Patient", + "shared-id", + json!({"resourceType": "Patient", "name": [{"family": "lower"}]}), + FhirVersion::default(), + ) + .await + .unwrap(); + assert_eq!( + backend + .read(&upper, "Patient", "shared-id") + .await + .unwrap() + .expect("tenant `Acme`'s resource must survive") + .content()["name"][0]["family"], + "UPPER" + ); + + // ...and must not delete it. + backend + .delete(&lower, "Patient", "shared-id") + .await + .unwrap(); + assert_eq!( + backend + .read(&upper, "Patient", "shared-id") + .await + .unwrap() + .expect("tenant `Acme`'s resource must survive `acme`'s delete") + .content()["name"][0]["family"], + "UPPER" + ); + assert_eq!(backend.count(&upper, Some("Patient")).await.unwrap(), 1); + assert_eq!(backend.count(&lower, Some("Patient")).await.unwrap(), 0); +} + #[tokio::test] async fn test_tenant_isolation_delete() { let backend = create_backend();