From 0db96803a5d188fb9c7ffc190c3d327c351c95ea Mon Sep 17 00:00:00 2001 From: David Viejo Date: Mon, 13 Jul 2026 21:47:19 +0200 Subject: [PATCH 1/3] feat(dns): add ownership-guarded managed DNS records (ADR-031) Foundation slice for managed DNS record automation: - OwnershipMarker: typed JSON markers in _temps-owned. companion TXT records (external-dns registry pattern), versioned (v:1), strict parse so user TXT content can never read as ours - ManagedDnsRecordService: the only path for public A/AAAA/CNAME writes. Never overwrites records without a matching marker (typed RecordConflict / NotOwnedByInstance errors for the import-or-skip UI), marker-first write ordering, explicit import_record adoption flow - Cloudflare Universal SSL depth guardrail: proxied records >=2 subdomain levels are rejected at write time with a flat-hostname suggestion instead of failing at the edge with an opaque 526 - dns_instance_identity single-row table: install-scoped instance ID so two temps installs sharing a zone refuse to touch each other's records - dns_managed_domains.proxied_by_default column (default false) - ADR-031 documenting the design, alternatives, and non-goals --- crates/temps-dns/src/errors.rs | 26 + crates/temps-dns/src/lib.rs | 8 +- crates/temps-dns/src/ownership.rs | 250 +++++ crates/temps-dns/src/plugin.rs | 10 +- .../temps-dns/src/services/managed_records.rs | 900 ++++++++++++++++++ crates/temps-dns/src/services/mod.rs | 11 +- .../src/dns_instance_identity.rs | 46 + .../temps-entities/src/dns_managed_domains.rs | 6 + crates/temps-entities/src/lib.rs | 1 + .../m20260713_000002_add_dns_ownership.rs | 57 ++ crates/temps-migrations/src/migration/mod.rs | 2 + ...dns-records-and-cloudflare-proxied-mode.md | 101 ++ 12 files changed, 1411 insertions(+), 7 deletions(-) create mode 100644 crates/temps-dns/src/ownership.rs create mode 100644 crates/temps-dns/src/services/managed_records.rs create mode 100644 crates/temps-entities/src/dns_instance_identity.rs create mode 100644 crates/temps-migrations/src/migration/m20260713_000002_add_dns_ownership.rs create mode 100644 docs/adr/031-managed-dns-records-and-cloudflare-proxied-mode.md diff --git a/crates/temps-dns/src/errors.rs b/crates/temps-dns/src/errors.rs index fb57e621d..17d10b858 100644 --- a/crates/temps-dns/src/errors.rs +++ b/crates/temps-dns/src/errors.rs @@ -58,4 +58,30 @@ pub enum DnsError { #[error("Connection failed: {0}")] ConnectionFailed(String), + + #[error("DNS record conflict for {record_type} '{name}' in zone {domain}: {reason}. Temps never overwrites a record it does not manage — import the record into temps management from the domain's DNS settings, or remove it at the provider and retry")] + RecordConflict { + domain: String, + name: String, + record_type: String, + reason: String, + }, + + #[error("DNS record {record_type} '{name}' in zone {domain} is owned by temps instance '{owner_instance}', not this one; refusing to modify it")] + NotOwnedByInstance { + domain: String, + name: String, + record_type: String, + owner_instance: String, + }, + + #[error("Cannot create proxied record '{fqdn}': it sits {levels} subdomain levels below the zone apex, and Cloudflare Universal SSL only covers one level, so TLS would fail at the edge (error 526) without Advanced Certificate Manager. Use the flat public hostname strategy instead (e.g. '{flat_suggestion}'), or disable proxying for this record")] + ProxiedDepthUnsupported { + fqdn: String, + levels: usize, + flat_suggestion: String, + }, + + #[error("DNS provider '{provider}' does not support proxied records; disable proxying for this record or use a provider with proxy support (e.g. Cloudflare)")] + ProxyNotSupportedByProvider { provider: String }, } diff --git a/crates/temps-dns/src/lib.rs b/crates/temps-dns/src/lib.rs index a17cc3df1..2266b0b18 100644 --- a/crates/temps-dns/src/lib.rs +++ b/crates/temps-dns/src/lib.rs @@ -40,6 +40,7 @@ pub mod cp_resolver; pub mod errors; pub mod handlers; +pub mod ownership; pub mod plugin; pub mod providers; pub mod services; @@ -47,6 +48,7 @@ pub mod services; // Re-export main types pub use cp_resolver::{start_control_plane_resolver, OverlayDnsSlot}; pub use errors::DnsError; +pub use ownership::{registry_record_name, OwnershipMarker, OWNERSHIP_REGISTRY_PREFIX}; pub use plugin::DnsPlugin; pub use providers::{ CloudflareCredentials, CloudflareProvider, DnsProvider, DnsProviderCapabilities, @@ -56,7 +58,7 @@ pub use providers::{ }; pub use services::{ ChangeSet, DeploymentDnsPublisher, DnsOperationResult, DnsProviderService, DnsRecordService, - DnsRegistry, DnsRegistryError, EndpointDraft, ManualDnsInstructions, - OwnerKind as InternalOwnerKind, RecordType as InternalRecordType, ResolverHealth, - StaleResolver, ZoneSnapshot, + DnsRegistry, DnsRegistryError, EndpointDraft, ManagedDnsRecordService, ManualDnsInstructions, + OwnerKind as InternalOwnerKind, OwnershipScope, RecordOwnership, + RecordType as InternalRecordType, ResolverHealth, StaleResolver, ZoneSnapshot, }; diff --git a/crates/temps-dns/src/ownership.rs b/crates/temps-dns/src/ownership.rs new file mode 100644 index 000000000..5e56db046 --- /dev/null +++ b/crates/temps-dns/src/ownership.rs @@ -0,0 +1,250 @@ +//! DNS record ownership markers (ADR-031) +//! +//! Temps writes public A/AAAA/CNAME records into zones it does not own. +//! The one mistake this feature must never make is touching a record temps +//! did not create. Ownership is therefore recorded *at the provider*, next to +//! the record itself, as a companion TXT "registry" record +//! (`_temps-owned.`) whose content is a typed JSON marker. Before any +//! update or delete, the marker is fetched and must parse AND match this +//! install's instance ID; anything else refuses the write. +//! +//! The companion-TXT scheme works uniformly across every provider. Cloudflare +//! additionally has a per-record `comment` field, but the `cloudflare` crate's +//! DNS params don't expose it, so comment stamping is deferred (the TXT +//! registry is used there too). +//! +//! The marker JSON is a compatibility surface once it exists in user zones — +//! it carries a `v` field so the format can evolve. Unknown fields are +//! tolerated on parse so a `v: 2` writer doesn't brick a `v: 1` reader. + +use serde::{Deserialize, Serialize}; + +use crate::errors::DnsError; + +/// Current marker format version. +pub const OWNERSHIP_MARKER_VERSION: u32 = 1; + +/// Value of `managed_by` in every marker temps writes. +pub const OWNERSHIP_MANAGED_BY: &str = "temps"; + +/// Label prefix of the companion TXT registry record. +pub const OWNERSHIP_REGISTRY_PREFIX: &str = "_temps-owned"; + +/// Replacement for the `*` label when building a registry name for a +/// wildcard record (`*` is not a meaningful label to prefix). +const WILDCARD_REPLACEMENT: &str = "wildcard"; + +/// Ownership marker stored in the companion TXT record. +/// +/// `instance` is the install-scoped random ID from +/// [`crate::services::ManagedDnsRecordService`]; two temps installs managing +/// the same zone will refuse to touch each other's records. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct OwnershipMarker { + /// Always [`OWNERSHIP_MANAGED_BY`]. Anything else fails to parse as ours. + pub managed_by: String, + + /// Install-scoped random ID of the temps instance that created the record. + pub instance: String, + + /// Project the record was created for, when known. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub project_id: Option, + + /// Environment the record was created for, when known. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub environment_id: Option, + + /// Marker format version. + pub v: u32, +} + +impl OwnershipMarker { + pub fn new(instance: &str, project_id: Option, environment_id: Option) -> Self { + Self { + managed_by: OWNERSHIP_MANAGED_BY.to_string(), + instance: instance.to_string(), + project_id, + environment_id, + v: OWNERSHIP_MARKER_VERSION, + } + } + + /// Serialize to the TXT record content. + pub fn to_txt_content(&self) -> Result { + serde_json::to_string(self).map_err(DnsError::Serialization) + } + + /// Parse a TXT record content as an ownership marker. + /// + /// Returns `None` for anything that is not a well-formed temps marker — + /// unparsable JSON, wrong `managed_by`, missing fields. Callers treat + /// `None` as "not ours: hands off". + pub fn parse(content: &str) -> Option { + let marker: Self = serde_json::from_str(content.trim()).ok()?; + if marker.managed_by != OWNERSHIP_MANAGED_BY || marker.instance.is_empty() { + return None; + } + Some(marker) + } + + /// Whether this marker was written by the given temps instance. + pub fn is_owned_by(&self, instance: &str) -> bool { + self.instance == instance + } +} + +/// Name of the companion TXT registry record for a managed record name. +/// +/// - `@` / empty (zone apex) → `_temps-owned` +/// - `www` → `_temps-owned.www` +/// - `*-staging` → `_temps-owned.wildcard-staging` +/// - `*.staging` → `_temps-owned.wildcard.staging` +/// +/// The wildcard label is replaced because `_temps-owned.*` is not a queryable +/// name; the replacement is deterministic so lookups and writes agree. +pub fn registry_record_name(record_name: &str) -> String { + if record_name == "@" || record_name.is_empty() { + return OWNERSHIP_REGISTRY_PREFIX.to_string(); + } + let sanitized = record_name.replace('*', WILDCARD_REPLACEMENT); + format!("{}.{}", OWNERSHIP_REGISTRY_PREFIX, sanitized) +} + +/// Number of subdomain levels a record name adds below the zone apex. +/// +/// `@` → 0, `www` → 1, `*-staging` → 1, `*.staging` → 2, `a.b.c` → 3. +pub fn subdomain_depth(record_name: &str) -> usize { + if record_name == "@" || record_name.is_empty() { + return 0; + } + record_name.split('.').filter(|l| !l.is_empty()).count() +} + +/// Guardrail for Cloudflare's Universal SSL depth limit (ADR-031 §3). +/// +/// Cloudflare's free/pro certificates only cover ONE subdomain level below +/// the apex. A *proxied* record at depth ≥ 2 (`a.b.example.com`, +/// `*.foo.example.com`) passes DNS but fails TLS at the edge with an opaque +/// 526/525 unless the user pays for Advanced Certificate Manager. Detect it +/// at write time and refuse with an actionable message instead. +/// +/// Only applies to proxied records — unproxied deep records are fine. +pub fn check_proxied_depth(zone: &str, record_name: &str) -> Result<(), DnsError> { + let depth = subdomain_depth(record_name); + if depth < 2 { + return Ok(()); + } + let flat_suggestion = record_name.replace('.', "-"); + Err(DnsError::ProxiedDepthUnsupported { + fqdn: format!("{}.{}", record_name, zone), + levels: depth, + flat_suggestion: format!("{}.{}", flat_suggestion, zone), + }) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn marker_round_trips_through_txt_content() { + let marker = OwnershipMarker::new("inst-abc123", Some(7), Some(42)); + let content = marker.to_txt_content().unwrap(); + let parsed = OwnershipMarker::parse(&content).unwrap(); + assert_eq!(parsed, marker); + assert_eq!(parsed.v, OWNERSHIP_MARKER_VERSION); + } + + #[test] + fn marker_without_scope_omits_ids_in_json() { + let marker = OwnershipMarker::new("inst-abc123", None, None); + let content = marker.to_txt_content().unwrap(); + assert!(!content.contains("project_id")); + assert!(!content.contains("environment_id")); + assert_eq!(OwnershipMarker::parse(&content).unwrap(), marker); + } + + #[test] + fn parse_rejects_non_marker_content() { + // Existing user TXT records must never parse as ours. + assert!(OwnershipMarker::parse("v=spf1 -all").is_none()); + assert!(OwnershipMarker::parse("").is_none()); + assert!(OwnershipMarker::parse("{\"foo\": 1}").is_none()); + } + + #[test] + fn parse_rejects_wrong_managed_by() { + let content = r#"{"managed_by":"other-tool","instance":"x","v":1}"#; + assert!(OwnershipMarker::parse(content).is_none()); + } + + #[test] + fn parse_rejects_empty_instance() { + let content = r#"{"managed_by":"temps","instance":"","v":1}"#; + assert!(OwnershipMarker::parse(content).is_none()); + } + + #[test] + fn parse_tolerates_unknown_fields_from_future_versions() { + let content = r#"{"managed_by":"temps","instance":"x","v":2,"new_field":"y"}"#; + let marker = OwnershipMarker::parse(content).unwrap(); + assert_eq!(marker.v, 2); + } + + #[test] + fn ownership_is_instance_scoped() { + let marker = OwnershipMarker::new("inst-a", None, None); + assert!(marker.is_owned_by("inst-a")); + assert!(!marker.is_owned_by("inst-b")); + } + + #[test] + fn registry_name_for_apex_and_subdomains() { + assert_eq!(registry_record_name("@"), "_temps-owned"); + assert_eq!(registry_record_name(""), "_temps-owned"); + assert_eq!(registry_record_name("www"), "_temps-owned.www"); + assert_eq!( + registry_record_name("*-staging"), + "_temps-owned.wildcard-staging" + ); + assert_eq!( + registry_record_name("*.staging"), + "_temps-owned.wildcard.staging" + ); + } + + #[test] + fn subdomain_depth_counts_labels() { + assert_eq!(subdomain_depth("@"), 0); + assert_eq!(subdomain_depth(""), 0); + assert_eq!(subdomain_depth("www"), 1); + assert_eq!(subdomain_depth("*-staging"), 1); + assert_eq!(subdomain_depth("*.staging"), 2); + assert_eq!(subdomain_depth("a.b.c"), 3); + } + + #[test] + fn proxied_depth_guardrail_allows_single_level() { + assert!(check_proxied_depth("example.com", "@").is_ok()); + assert!(check_proxied_depth("example.com", "www").is_ok()); + assert!(check_proxied_depth("example.com", "*-staging").is_ok()); + } + + #[test] + fn proxied_depth_guardrail_rejects_two_levels_with_flat_suggestion() { + let err = check_proxied_depth("example.com", "*.staging").unwrap_err(); + match err { + DnsError::ProxiedDepthUnsupported { + fqdn, + levels, + flat_suggestion, + } => { + assert_eq!(fqdn, "*.staging.example.com"); + assert_eq!(levels, 2); + assert_eq!(flat_suggestion, "*-staging.example.com"); + } + other => panic!("expected ProxiedDepthUnsupported, got {:?}", other), + } + } +} diff --git a/crates/temps-dns/src/plugin.rs b/crates/temps-dns/src/plugin.rs index 91af31b16..5fb5b9251 100644 --- a/crates/temps-dns/src/plugin.rs +++ b/crates/temps-dns/src/plugin.rs @@ -17,7 +17,7 @@ use utoipa::openapi::OpenApi; use utoipa::OpenApi as OpenApiTrait; use crate::handlers::{self, dns_sync::DnsSyncAppState, DnsApiDoc, DnsAppState}; -use crate::services::{DnsProviderService, DnsRecordService, DnsRegistry}; +use crate::services::{DnsProviderService, DnsRecordService, DnsRegistry, ManagedDnsRecordService}; /// DNS Plugin for managing DNS providers and automatic DNS record configuration pub struct DnsPlugin; @@ -59,6 +59,14 @@ impl TempsPlugin for DnsPlugin { let record_service = Arc::new(DnsRecordService::new(provider_service.clone())); context.register_service(record_service.clone()); + // Ownership-guarded record management (ADR-031) — the only path + // for public A/AAAA/CNAME records in user zones. + let managed_record_service = Arc::new(ManagedDnsRecordService::new( + db.clone(), + provider_service.clone(), + )); + context.register_service(managed_record_service); + // Create DnsAppState for handlers let app_state = Arc::new(DnsAppState { provider_service, diff --git a/crates/temps-dns/src/services/managed_records.rs b/crates/temps-dns/src/services/managed_records.rs new file mode 100644 index 000000000..d448bbdc1 --- /dev/null +++ b/crates/temps-dns/src/services/managed_records.rs @@ -0,0 +1,900 @@ +//! Ownership-guarded DNS record management (ADR-031) +//! +//! [`ManagedDnsRecordService`] is the ONLY path other crates should use to +//! create public A/AAAA/CNAME records in user zones. Unlike the raw +//! [`crate::services::DnsRecordService`] (which upserts blindly and is kept +//! for ACME challenge TXT records that temps unambiguously owns), every write +//! here is guarded by the ownership scheme from [`crate::ownership`]: +//! +//! - **Create/update** refuses if a record with the target name/type exists +//! without a temps ownership marker, or with a marker from a different +//! temps install. Conflicts surface as typed [`DnsError::RecordConflict`] / +//! [`DnsError::NotOwnedByInstance`] so the UI can offer import-or-skip. +//! - **Delete** only removes records this install owns. +//! - **Import** is the explicit, user-confirmed adoption path that stamps a +//! marker onto a pre-existing record. +//! +//! Proxied (Cloudflare orange-cloud) writes additionally pass the Universal +//! SSL depth guardrail — see [`crate::ownership::check_proxied_depth`]. +//! +//! Write ordering: the ownership marker TXT is written BEFORE the target +//! record. A crash between the two leaves a harmless orphan marker, never a +//! live unmarked record that a later run would refuse to manage. + +use std::sync::Arc; + +use sea_orm::{ActiveModelTrait, ActiveValue::Set, DatabaseConnection, EntityTrait}; +use temps_entities::dns_instance_identity; +use tracing::{info, warn}; + +use crate::errors::DnsError; +use crate::ownership::{check_proxied_depth, registry_record_name, OwnershipMarker}; +use crate::providers::{DnsProvider, DnsRecord, DnsRecordContent, DnsRecordRequest, DnsRecordType}; +use crate::services::provider_service::DnsProviderService; + +/// What a managed record was created for; stamped into the ownership marker +/// so the provider-side registry shows which project/environment a record +/// belongs to. +#[derive(Debug, Clone, Copy, Default)] +pub struct OwnershipScope { + pub project_id: Option, + pub environment_id: Option, +} + +/// Ownership state of a record at the provider, for the domain UI's +/// per-record status (created / conflict / unmanaged). +#[derive(Debug, Clone)] +pub enum RecordOwnership { + /// No record with this name/type exists. + NotFound, + /// Record exists but carries no temps ownership marker — temps will not + /// touch it unless the user imports it. + Unmanaged(DnsRecord), + /// Record exists and is owned by this temps install. + Owned(DnsRecord, OwnershipMarker), + /// Record exists and is owned by a DIFFERENT temps install. + OwnedByOther(DnsRecord, OwnershipMarker), +} + +/// Ownership-guarded record management on top of [`DnsProviderService`]. +pub struct ManagedDnsRecordService { + db: Arc, + provider_service: Arc, + instance_id: tokio::sync::OnceCell, +} + +impl ManagedDnsRecordService { + pub fn new(db: Arc, provider_service: Arc) -> Self { + Self { + db, + provider_service, + instance_id: tokio::sync::OnceCell::new(), + } + } + + /// Get (or create on first use) this install's ownership instance ID. + /// + /// The ID never rotates once created — rotating would orphan every record + /// this install previously stamped. + pub async fn instance_id(&self) -> Result { + let id = self + .instance_id + .get_or_try_init(|| async { + if let Some(row) = dns_instance_identity::Entity::find() + .one(self.db.as_ref()) + .await? + { + return Ok::(row.instance_id); + } + + let fresh = uuid::Uuid::new_v4().to_string(); + let row = dns_instance_identity::ActiveModel { + id: Set(1), + instance_id: Set(fresh), + ..Default::default() + }; + // Two concurrent first writes can race on the single-row PK; + // whoever loses re-reads the winner's ID instead of failing. + match row.insert(self.db.as_ref()).await { + Ok(created) => Ok(created.instance_id), + Err(insert_err) => dns_instance_identity::Entity::find() + .one(self.db.as_ref()) + .await? + .map(|row| row.instance_id) + .ok_or(DnsError::Database(insert_err)), + } + }) + .await?; + Ok(id.clone()) + } + + /// Create or update a managed record, enforcing ownership and proxy + /// guardrails. `domain` may be any FQDN under a managed zone; the record + /// `request.name` is relative to that zone. + pub async fn set_managed_record( + &self, + domain: &str, + request: DnsRecordRequest, + scope: OwnershipScope, + ) -> Result { + let (provider_model, managed) = self + .provider_service + .find_provider_for_domain(domain) + .await? + .ok_or_else(|| DnsError::DomainNotManaged(domain.to_string()))?; + let provider = self + .provider_service + .create_provider_instance(&provider_model)?; + let zone = managed.domain.as_str(); + + if request.proxied { + if !provider.capabilities().proxy { + return Err(DnsError::ProxyNotSupportedByProvider { + provider: provider_model.name.clone(), + }); + } + check_proxied_depth(zone, &request.name)?; + } + + let instance = self.instance_id().await?; + let marker = OwnershipMarker::new(&instance, scope.project_id, scope.environment_id); + + let record = + Self::guarded_set(provider.as_ref(), zone, request, &marker, &instance).await?; + info!( + "Set managed {} record '{}' in zone {} via provider {} (proxied: {})", + record.content.record_type(), + record.name, + zone, + provider_model.name, + record.proxied + ); + Ok(record) + } + + /// Delete a managed record. Refuses unless this install owns it. + pub async fn remove_managed_record( + &self, + domain: &str, + name: &str, + record_type: DnsRecordType, + ) -> Result<(), DnsError> { + let (provider_model, managed) = self + .provider_service + .find_provider_for_domain(domain) + .await? + .ok_or_else(|| DnsError::DomainNotManaged(domain.to_string()))?; + let provider = self + .provider_service + .create_provider_instance(&provider_model)?; + let zone = managed.domain.as_str(); + + let instance = self.instance_id().await?; + Self::guarded_remove(provider.as_ref(), zone, name, record_type, &instance).await?; + info!( + "Removed managed {} record '{}' in zone {} via provider {}", + record_type, name, zone, provider_model.name + ); + Ok(()) + } + + /// Explicitly adopt a pre-existing record into temps management by + /// stamping an ownership marker onto it. This is the user-confirmed + /// "import" arm of the conflict flow — never called automatically. + pub async fn import_record( + &self, + domain: &str, + name: &str, + record_type: DnsRecordType, + scope: OwnershipScope, + ) -> Result { + let (provider_model, managed) = self + .provider_service + .find_provider_for_domain(domain) + .await? + .ok_or_else(|| DnsError::DomainNotManaged(domain.to_string()))?; + let provider = self + .provider_service + .create_provider_instance(&provider_model)?; + let zone = managed.domain.as_str(); + + let instance = self.instance_id().await?; + let marker = + Self::guarded_import(provider.as_ref(), zone, name, record_type, &instance, scope) + .await?; + info!( + "Imported {} record '{}' in zone {} into temps management", + record_type, name, zone + ); + Ok(marker) + } + + /// Ownership state of a record, for the domain UI. + pub async fn record_ownership( + &self, + domain: &str, + name: &str, + record_type: DnsRecordType, + ) -> Result { + let (provider_model, managed) = self + .provider_service + .find_provider_for_domain(domain) + .await? + .ok_or_else(|| DnsError::DomainNotManaged(domain.to_string()))?; + let provider = self + .provider_service + .create_provider_instance(&provider_model)?; + let instance = self.instance_id().await?; + + Self::ownership_of( + provider.as_ref(), + &managed.domain, + name, + record_type, + &instance, + ) + .await + } + + // ------------------------------------------------------------------ + // Guarded core — associated functions over `&dyn DnsProvider` so the + // safety logic is unit-testable with an in-memory provider, independent + // of the database and real provider APIs. + // ------------------------------------------------------------------ + + /// Fetch and parse the ownership marker TXT for a record name, if any. + async fn fetch_marker( + provider: &dyn DnsProvider, + zone: &str, + record_name: &str, + ) -> Result, DnsError> { + let registry_name = registry_record_name(record_name); + let txt = provider + .get_record(zone, ®istry_name, DnsRecordType::TXT) + .await?; + Ok(txt.and_then(|record| match &record.content { + DnsRecordContent::TXT { content } => OwnershipMarker::parse(content), + _ => None, + })) + } + + async fn ownership_of( + provider: &dyn DnsProvider, + zone: &str, + name: &str, + record_type: DnsRecordType, + instance: &str, + ) -> Result { + let existing = provider.get_record(zone, name, record_type).await?; + let Some(record) = existing else { + return Ok(RecordOwnership::NotFound); + }; + match Self::fetch_marker(provider, zone, name).await? { + None => Ok(RecordOwnership::Unmanaged(record)), + Some(marker) if marker.is_owned_by(instance) => { + Ok(RecordOwnership::Owned(record, marker)) + } + Some(marker) => Ok(RecordOwnership::OwnedByOther(record, marker)), + } + } + + async fn guarded_set( + provider: &dyn DnsProvider, + zone: &str, + request: DnsRecordRequest, + marker: &OwnershipMarker, + instance: &str, + ) -> Result { + let record_type = request.content.record_type(); + let existed = match Self::ownership_of(provider, zone, &request.name, record_type, instance) + .await? + { + RecordOwnership::NotFound => false, + RecordOwnership::Owned(_, _) => true, + RecordOwnership::Unmanaged(_) => { + return Err(DnsError::RecordConflict { + domain: zone.to_string(), + name: request.name.clone(), + record_type: record_type.to_string(), + reason: "an existing record with this name is not managed by temps".to_string(), + }); + } + RecordOwnership::OwnedByOther(_, marker) => { + return Err(DnsError::NotOwnedByInstance { + domain: zone.to_string(), + name: request.name.clone(), + record_type: record_type.to_string(), + owner_instance: marker.instance, + }); + } + }; + + // Marker first: a crash after this point leaves an orphan TXT (noise), + // never a live unmarked record (a permanent conflict against ourselves). + let registry_request = DnsRecordRequest { + name: registry_record_name(&request.name), + content: DnsRecordContent::TXT { + content: marker.to_txt_content()?, + }, + ttl: request.ttl, + proxied: false, + }; + provider.set_record(zone, registry_request).await?; + + match provider.set_record(zone, request.clone()).await { + Ok(record) => Ok(record), + Err(e) => { + // Creating the target failed. If nothing existed before, the + // fresh marker is pure junk — clean it up best-effort. + if !existed { + let registry_name = registry_record_name(&request.name); + if let Err(cleanup_err) = provider + .remove_record(zone, ®istry_name, DnsRecordType::TXT) + .await + { + warn!( + "Failed to clean up ownership marker '{}' in zone {} after record create failed: {}", + registry_name, zone, cleanup_err + ); + } + } + Err(e) + } + } + } + + async fn guarded_remove( + provider: &dyn DnsProvider, + zone: &str, + name: &str, + record_type: DnsRecordType, + instance: &str, + ) -> Result<(), DnsError> { + match Self::ownership_of(provider, zone, name, record_type, instance).await? { + RecordOwnership::NotFound => { + // Record already gone; clean up a stray marker of ours if the + // registry still has one so it doesn't accumulate. + if let Some(marker) = Self::fetch_marker(provider, zone, name).await? { + if marker.is_owned_by(instance) { + provider + .remove_record(zone, ®istry_record_name(name), DnsRecordType::TXT) + .await?; + } + } + Ok(()) + } + RecordOwnership::Owned(_, _) => { + provider.remove_record(zone, name, record_type).await?; + provider + .remove_record(zone, ®istry_record_name(name), DnsRecordType::TXT) + .await?; + Ok(()) + } + RecordOwnership::Unmanaged(_) => Err(DnsError::RecordConflict { + domain: zone.to_string(), + name: name.to_string(), + record_type: record_type.to_string(), + reason: "the record is not managed by temps, so temps will not delete it" + .to_string(), + }), + RecordOwnership::OwnedByOther(_, marker) => Err(DnsError::NotOwnedByInstance { + domain: zone.to_string(), + name: name.to_string(), + record_type: record_type.to_string(), + owner_instance: marker.instance, + }), + } + } + + async fn guarded_import( + provider: &dyn DnsProvider, + zone: &str, + name: &str, + record_type: DnsRecordType, + instance: &str, + scope: OwnershipScope, + ) -> Result { + match Self::ownership_of(provider, zone, name, record_type, instance).await? { + RecordOwnership::NotFound => Err(DnsError::RecordNotFound(format!( + "{} record '{}' in zone {} does not exist, so it cannot be imported", + record_type, name, zone + ))), + RecordOwnership::Owned(_, marker) => Ok(marker), // already ours — idempotent + RecordOwnership::OwnedByOther(_, marker) => Err(DnsError::NotOwnedByInstance { + domain: zone.to_string(), + name: name.to_string(), + record_type: record_type.to_string(), + owner_instance: marker.instance, + }), + RecordOwnership::Unmanaged(_) => { + let marker = OwnershipMarker::new(instance, scope.project_id, scope.environment_id); + let registry_request = DnsRecordRequest { + name: registry_record_name(name), + content: DnsRecordContent::TXT { + content: marker.to_txt_content()?, + }, + ttl: None, + proxied: false, + }; + provider.set_record(zone, registry_request).await?; + Ok(marker) + } + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::providers::{DnsProviderCapabilities, DnsProviderType, DnsZone}; + use async_trait::async_trait; + use std::collections::HashMap; + use std::sync::Mutex; + + /// In-memory provider: records keyed by (name, type). Panics are fine in + /// tests; production paths never touch this. + struct MockProvider { + records: Mutex>, + fail_target_writes: bool, + } + + impl MockProvider { + fn new() -> Self { + Self { + records: Mutex::new(HashMap::new()), + fail_target_writes: false, + } + } + + fn with_record(self, name: &str, content: DnsRecordContent) -> Self { + let record_type = content.record_type().to_string(); + self.records.lock().unwrap().insert( + (name.to_string(), record_type.clone()), + DnsRecord { + id: Some(format!("{}-{}", name, record_type)), + zone: "example.com".to_string(), + name: name.to_string(), + fqdn: format!("{}.example.com", name), + content, + ttl: 300, + proxied: false, + metadata: HashMap::new(), + }, + ); + self + } + + fn has_record(&self, name: &str, record_type: DnsRecordType) -> bool { + self.records + .lock() + .unwrap() + .contains_key(&(name.to_string(), record_type.to_string())) + } + } + + #[async_trait] + impl DnsProvider for MockProvider { + fn provider_type(&self) -> DnsProviderType { + DnsProviderType::Manual + } + + fn capabilities(&self) -> DnsProviderCapabilities { + DnsProviderCapabilities { + a_record: true, + cname_record: true, + txt_record: true, + proxy: true, + ..Default::default() + } + } + + async fn test_connection(&self) -> Result { + Ok(true) + } + + async fn list_zones(&self) -> Result, DnsError> { + Ok(vec![]) + } + + async fn get_zone(&self, _domain: &str) -> Result, DnsError> { + Ok(None) + } + + async fn list_records(&self, _domain: &str) -> Result, DnsError> { + Ok(self.records.lock().unwrap().values().cloned().collect()) + } + + async fn get_record( + &self, + _domain: &str, + name: &str, + record_type: DnsRecordType, + ) -> Result, DnsError> { + Ok(self + .records + .lock() + .unwrap() + .get(&(name.to_string(), record_type.to_string())) + .cloned()) + } + + async fn create_record( + &self, + domain: &str, + request: DnsRecordRequest, + ) -> Result { + let record_type = request.content.record_type(); + if self.fail_target_writes && record_type != DnsRecordType::TXT { + return Err(DnsError::ApiError("simulated write failure".to_string())); + } + let record = DnsRecord { + id: Some(format!("{}-{}", request.name, record_type)), + zone: domain.to_string(), + name: request.name.clone(), + fqdn: format!("{}.{}", request.name, domain), + content: request.content, + ttl: request.ttl.unwrap_or(300), + proxied: request.proxied, + metadata: HashMap::new(), + }; + self.records.lock().unwrap().insert( + (record.name.clone(), record_type.to_string()), + record.clone(), + ); + Ok(record) + } + + async fn update_record( + &self, + domain: &str, + _record_id: &str, + request: DnsRecordRequest, + ) -> Result { + self.create_record(domain, request).await + } + + async fn delete_record(&self, _domain: &str, record_id: &str) -> Result<(), DnsError> { + self.records + .lock() + .unwrap() + .retain(|_, r| r.id.as_deref() != Some(record_id)); + Ok(()) + } + } + + const INSTANCE: &str = "test-instance"; + + fn a_request(name: &str, proxied: bool) -> DnsRecordRequest { + DnsRecordRequest { + name: name.to_string(), + content: DnsRecordContent::A { + address: "192.0.2.10".to_string(), + }, + ttl: Some(300), + proxied, + } + } + + fn marker() -> OwnershipMarker { + OwnershipMarker::new(INSTANCE, Some(1), Some(2)) + } + + #[tokio::test] + async fn set_creates_record_and_ownership_marker() { + let provider = MockProvider::new(); + let record = ManagedDnsRecordService::guarded_set( + &provider, + "example.com", + a_request("app", false), + &marker(), + INSTANCE, + ) + .await + .unwrap(); + + assert_eq!(record.name, "app"); + assert!(provider.has_record("app", DnsRecordType::A)); + assert!(provider.has_record("_temps-owned.app", DnsRecordType::TXT)); + } + + #[tokio::test] + async fn set_refuses_to_overwrite_unmanaged_record() { + // The core ADR-031 invariant: an existing record without a marker is + // untouchable, whatever its content. + let provider = MockProvider::new().with_record( + "app", + DnsRecordContent::A { + address: "203.0.113.1".to_string(), + }, + ); + + let err = ManagedDnsRecordService::guarded_set( + &provider, + "example.com", + a_request("app", false), + &marker(), + INSTANCE, + ) + .await + .unwrap_err(); + + assert!(matches!(err, DnsError::RecordConflict { .. })); + // Original record untouched + let existing = provider + .get_record("example.com", "app", DnsRecordType::A) + .await + .unwrap() + .unwrap(); + assert_eq!(existing.content.to_value_string(), "203.0.113.1"); + } + + #[tokio::test] + async fn set_refuses_record_owned_by_other_instance() { + let foreign = OwnershipMarker::new("other-install", None, None); + let provider = MockProvider::new() + .with_record( + "app", + DnsRecordContent::A { + address: "203.0.113.1".to_string(), + }, + ) + .with_record( + "_temps-owned.app", + DnsRecordContent::TXT { + content: foreign.to_txt_content().unwrap(), + }, + ); + + let err = ManagedDnsRecordService::guarded_set( + &provider, + "example.com", + a_request("app", false), + &marker(), + INSTANCE, + ) + .await + .unwrap_err(); + + match err { + DnsError::NotOwnedByInstance { owner_instance, .. } => { + assert_eq!(owner_instance, "other-install"); + } + other => panic!("expected NotOwnedByInstance, got {:?}", other), + } + } + + #[tokio::test] + async fn set_updates_record_owned_by_this_instance() { + let provider = MockProvider::new() + .with_record( + "app", + DnsRecordContent::A { + address: "203.0.113.1".to_string(), + }, + ) + .with_record( + "_temps-owned.app", + DnsRecordContent::TXT { + content: marker().to_txt_content().unwrap(), + }, + ); + + let record = ManagedDnsRecordService::guarded_set( + &provider, + "example.com", + a_request("app", false), + &marker(), + INSTANCE, + ) + .await + .unwrap(); + + assert_eq!(record.content.to_value_string(), "192.0.2.10"); + } + + #[tokio::test] + async fn failed_create_cleans_up_fresh_marker() { + let mut provider = MockProvider::new(); + provider.fail_target_writes = true; + + let err = ManagedDnsRecordService::guarded_set( + &provider, + "example.com", + a_request("app", false), + &marker(), + INSTANCE, + ) + .await + .unwrap_err(); + + assert!(matches!(err, DnsError::ApiError(_))); + // No orphan marker left behind for a record that was never created. + assert!(!provider.has_record("_temps-owned.app", DnsRecordType::TXT)); + } + + #[tokio::test] + async fn remove_refuses_unmanaged_record() { + let provider = MockProvider::new().with_record( + "app", + DnsRecordContent::A { + address: "203.0.113.1".to_string(), + }, + ); + + let err = ManagedDnsRecordService::guarded_remove( + &provider, + "example.com", + "app", + DnsRecordType::A, + INSTANCE, + ) + .await + .unwrap_err(); + + assert!(matches!(err, DnsError::RecordConflict { .. })); + assert!(provider.has_record("app", DnsRecordType::A)); + } + + #[tokio::test] + async fn remove_deletes_owned_record_and_marker() { + let provider = MockProvider::new() + .with_record( + "app", + DnsRecordContent::A { + address: "192.0.2.10".to_string(), + }, + ) + .with_record( + "_temps-owned.app", + DnsRecordContent::TXT { + content: marker().to_txt_content().unwrap(), + }, + ); + + ManagedDnsRecordService::guarded_remove( + &provider, + "example.com", + "app", + DnsRecordType::A, + INSTANCE, + ) + .await + .unwrap(); + + assert!(!provider.has_record("app", DnsRecordType::A)); + assert!(!provider.has_record("_temps-owned.app", DnsRecordType::TXT)); + } + + #[tokio::test] + async fn remove_of_missing_record_is_ok_and_cleans_stray_marker() { + let provider = MockProvider::new().with_record( + "_temps-owned.app", + DnsRecordContent::TXT { + content: marker().to_txt_content().unwrap(), + }, + ); + + ManagedDnsRecordService::guarded_remove( + &provider, + "example.com", + "app", + DnsRecordType::A, + INSTANCE, + ) + .await + .unwrap(); + + assert!(!provider.has_record("_temps-owned.app", DnsRecordType::TXT)); + } + + #[tokio::test] + async fn import_stamps_marker_on_unmanaged_record() { + let provider = MockProvider::new().with_record( + "app", + DnsRecordContent::A { + address: "203.0.113.1".to_string(), + }, + ); + + let imported = ManagedDnsRecordService::guarded_import( + &provider, + "example.com", + "app", + DnsRecordType::A, + INSTANCE, + OwnershipScope { + project_id: Some(9), + environment_id: None, + }, + ) + .await + .unwrap(); + + assert_eq!(imported.project_id, Some(9)); + assert!(provider.has_record("_temps-owned.app", DnsRecordType::TXT)); + + // After import, set is allowed. + let record = ManagedDnsRecordService::guarded_set( + &provider, + "example.com", + a_request("app", false), + &marker(), + INSTANCE, + ) + .await + .unwrap(); + assert_eq!(record.content.to_value_string(), "192.0.2.10"); + } + + #[tokio::test] + async fn import_refuses_missing_and_foreign_records() { + let provider = MockProvider::new(); + let err = ManagedDnsRecordService::guarded_import( + &provider, + "example.com", + "ghost", + DnsRecordType::A, + INSTANCE, + OwnershipScope::default(), + ) + .await + .unwrap_err(); + assert!(matches!(err, DnsError::RecordNotFound(_))); + + let foreign = OwnershipMarker::new("other-install", None, None); + let provider = MockProvider::new() + .with_record( + "app", + DnsRecordContent::A { + address: "203.0.113.1".to_string(), + }, + ) + .with_record( + "_temps-owned.app", + DnsRecordContent::TXT { + content: foreign.to_txt_content().unwrap(), + }, + ); + let err = ManagedDnsRecordService::guarded_import( + &provider, + "example.com", + "app", + DnsRecordType::A, + INSTANCE, + OwnershipScope::default(), + ) + .await + .unwrap_err(); + assert!(matches!(err, DnsError::NotOwnedByInstance { .. })); + } + + #[tokio::test] + async fn user_txt_record_at_registry_name_does_not_grant_ownership() { + // A user TXT that happens to live at `_temps-owned.app` but isn't a + // valid marker must read as Unmanaged, not Owned. + let provider = MockProvider::new() + .with_record( + "app", + DnsRecordContent::A { + address: "203.0.113.1".to_string(), + }, + ) + .with_record( + "_temps-owned.app", + DnsRecordContent::TXT { + content: "v=spf1 -all".to_string(), + }, + ); + + let ownership = ManagedDnsRecordService::ownership_of( + &provider, + "example.com", + "app", + DnsRecordType::A, + INSTANCE, + ) + .await + .unwrap(); + assert!(matches!(ownership, RecordOwnership::Unmanaged(_))); + } +} diff --git a/crates/temps-dns/src/services/mod.rs b/crates/temps-dns/src/services/mod.rs index 774feadf3..4c3793014 100644 --- a/crates/temps-dns/src/services/mod.rs +++ b/crates/temps-dns/src/services/mod.rs @@ -2,9 +2,12 @@ //! //! This module contains two unrelated services: //! -//! - **External-facing** (`provider_service`, `record_service`): manages DNS -//! records at third-party providers (Cloudflare, Route53, …) for user -//! domains. +//! - **External-facing** (`provider_service`, `record_service`, +//! `managed_records`): manages DNS records at third-party providers +//! (Cloudflare, Route53, …) for user domains. `managed_records` is the +//! ownership-guarded path for public A/AAAA/CNAME records (ADR-031); +//! `record_service` is the raw upsert path kept for ACME challenge TXT +//! records temps unambiguously owns. //! - **Internal-facing** (`dns_registry`): authoritative store for the //! `*.temps.local` zone served by per-node Hickory resolvers (ADR-011). //! @@ -14,6 +17,7 @@ pub mod deployment_publisher; pub mod dns_registry; +pub mod managed_records; pub mod provider_service; pub mod record_service; @@ -22,6 +26,7 @@ pub use dns_registry::{ ChangeSet, DnsRegistry, DnsRegistryError, EndpointDraft, OwnerKind, RecordType, ResolverHealth, StaleResolver, ZoneSnapshot, }; +pub use managed_records::{ManagedDnsRecordService, OwnershipScope, RecordOwnership}; pub use provider_service::{ AddManagedDomainRequest, CreateProviderRequest, DnsProviderService, UpdateProviderRequest, }; diff --git a/crates/temps-entities/src/dns_instance_identity.rs b/crates/temps-entities/src/dns_instance_identity.rs new file mode 100644 index 000000000..cb3024673 --- /dev/null +++ b/crates/temps-entities/src/dns_instance_identity.rs @@ -0,0 +1,46 @@ +//! DNS ownership instance identity (ADR-031) +//! +//! Single-row table holding the random, install-scoped ID this temps +//! instance stamps into DNS ownership markers (`_temps-owned.*` TXT +//! records). Two temps installs managing the same zone use this to refuse +//! to touch each other's records. +//! +//! The ID is generated once on first managed-DNS write and never changes: +//! rotating it would orphan every record this install previously created. +//! It is intentionally NOT the telemetry `anonymous_id` — that one is +//! telemetry-scoped and must stay unlinkable to public DNS data. + +use async_trait::async_trait; +use sea_orm::entity::prelude::*; +use sea_orm::{ActiveValue::Set, ConnectionTrait, DbErr}; +use serde::{Deserialize, Serialize}; +use temps_core::DBDateTime; + +#[derive(Clone, Debug, PartialEq, DeriveEntityModel, Eq, Serialize, Deserialize)] +#[sea_orm(table_name = "dns_instance_identity")] +pub struct Model { + /// Always 1 — the table is constrained to a single row. + #[sea_orm(primary_key, auto_increment = false)] + pub id: i32, + + /// Random UUID identifying this temps install in ownership markers. + pub instance_id: String, + + pub created_at: DBDateTime, +} + +#[derive(Copy, Clone, Debug, EnumIter, DeriveRelation)] +pub enum Relation {} + +#[async_trait] +impl ActiveModelBehavior for ActiveModel { + async fn before_save(mut self, _db: &C, insert: bool) -> Result + where + C: ConnectionTrait, + { + if insert && self.created_at.is_not_set() { + self.created_at = Set(chrono::Utc::now()); + } + Ok(self) + } +} diff --git a/crates/temps-entities/src/dns_managed_domains.rs b/crates/temps-entities/src/dns_managed_domains.rs index 958bf2695..5efe9cf99 100644 --- a/crates/temps-entities/src/dns_managed_domains.rs +++ b/crates/temps-entities/src/dns_managed_domains.rs @@ -29,6 +29,12 @@ pub struct Model { /// Whether automatic DNS management is enabled for this domain pub auto_manage: bool, + /// Whether records created for this domain should be proxied through the + /// provider's CDN by default (Cloudflare orange-cloud). Only meaningful + /// when the provider's capabilities report proxy support; callers may + /// still override per record (ADR-031). + pub proxied_by_default: bool, + /// Whether this domain has been verified (provider can access it) pub verified: bool, diff --git a/crates/temps-entities/src/lib.rs b/crates/temps-entities/src/lib.rs index f49a1789c..cb9ebcced 100644 --- a/crates/temps-entities/src/lib.rs +++ b/crates/temps-entities/src/lib.rs @@ -38,6 +38,7 @@ pub mod deployment_domains; pub mod deployment_jobs; pub mod deployment_tokens; pub mod deployments; +pub mod dns_instance_identity; pub mod dns_managed_domains; pub mod dns_providers; pub mod domains; diff --git a/crates/temps-migrations/src/migration/m20260713_000002_add_dns_ownership.rs b/crates/temps-migrations/src/migration/m20260713_000002_add_dns_ownership.rs new file mode 100644 index 000000000..9a88620a7 --- /dev/null +++ b/crates/temps-migrations/src/migration/m20260713_000002_add_dns_ownership.rs @@ -0,0 +1,57 @@ +//! DNS record ownership foundation (ADR-031). +//! +//! Two pieces: +//! +//! - `dns_instance_identity`: single-row table holding the random install ID +//! stamped into `_temps-owned.*` ownership TXT markers, so two temps +//! installs managing the same zone refuse to touch each other's records. +//! The row is created lazily on first managed-DNS write, not here — a +//! migration must not generate per-install random state. +//! +//! - `dns_managed_domains.proxied_by_default`: per-domain default for +//! Cloudflare-style proxied (orange-cloud) records. Defaults to false so +//! existing managed domains keep today's unproxied behaviour. + +use sea_orm_migration::prelude::*; + +#[derive(DeriveMigrationName)] +pub struct Migration; + +#[async_trait::async_trait] +impl MigrationTrait for Migration { + async fn up(&self, manager: &SchemaManager) -> Result<(), DbErr> { + let db = manager.get_connection(); + + db.execute_unprepared( + r#" + CREATE TABLE IF NOT EXISTS dns_instance_identity ( + id integer PRIMARY KEY CHECK (id = 1), + instance_id text NOT NULL, + created_at timestamptz NOT NULL DEFAULT now() + ); + + ALTER TABLE dns_managed_domains + ADD COLUMN IF NOT EXISTS proxied_by_default boolean NOT NULL DEFAULT false; + "#, + ) + .await?; + + Ok(()) + } + + async fn down(&self, manager: &SchemaManager) -> Result<(), DbErr> { + let db = manager.get_connection(); + + db.execute_unprepared( + r#" + ALTER TABLE dns_managed_domains + DROP COLUMN IF EXISTS proxied_by_default; + + DROP TABLE IF EXISTS dns_instance_identity; + "#, + ) + .await?; + + Ok(()) + } +} diff --git a/crates/temps-migrations/src/migration/mod.rs b/crates/temps-migrations/src/migration/mod.rs index cea5f30f6..d73c2a682 100644 --- a/crates/temps-migrations/src/migration/mod.rs +++ b/crates/temps-migrations/src/migration/mod.rs @@ -148,6 +148,7 @@ mod m20260711_000001_add_proxy_logs_stats_cagg; mod m20260711_000002_add_ip_geolocations_hosting_provider; mod m20260711_000003_add_visitor_non_crawler_partial_index; mod m20260713_000001_add_mfa_pending_to_sessions; +mod m20260713_000002_add_dns_ownership; pub struct Migrator; @@ -301,6 +302,7 @@ impl MigratorTrait for Migrator { Box::new(m20260711_000002_add_ip_geolocations_hosting_provider::Migration), Box::new(m20260711_000003_add_visitor_non_crawler_partial_index::Migration), Box::new(m20260713_000001_add_mfa_pending_to_sessions::Migration), + Box::new(m20260713_000002_add_dns_ownership::Migration), ] } } diff --git a/docs/adr/031-managed-dns-records-and-cloudflare-proxied-mode.md b/docs/adr/031-managed-dns-records-and-cloudflare-proxied-mode.md new file mode 100644 index 000000000..cedc8fc0b --- /dev/null +++ b/docs/adr/031-managed-dns-records-and-cloudflare-proxied-mode.md @@ -0,0 +1,101 @@ +--- +title: "ADR-031: Managed DNS Records and Cloudflare Proxied Mode" +status: Proposed +date: 2026-07-12 +author: David Viejo +--- + +# ADR-031: Managed DNS Records and Cloudflare Proxied Mode + +**Status:** Proposed +**Date:** 2026-07-12 +**Author:** David Viejo +**Security review required:** Yes — this feature writes to users' public DNS zones and stores provider API tokens. A bug can take a production domain offline or hijack traffic. Requires security-auditor sign-off before implementation. +**Related:** Issue #139 (flattened public hostname templates for proxied wildcard TLS), PR #146 (flat public hostname strategy — hard dependency), PR #270 (DNS wired into TlsService), `temps-dns` crate (`DnsProvider` trait, Cloudflare/Route53/GCP/Azure/DigitalOcean/Namecheap providers) +**Demand signal:** Outside contributor (bherila) with a hard requirement: never expose the origin server IP; all public traffic must go through Cloudflare's proxy. He currently cannot use temps-managed domains without manual DNS work per hostname, and reports having written conflict-resolution logic in his fork. Discussed 2026-07-12 (WhatsApp). + +--- + +## Context + +### The problem + +Temps already has a multi-provider DNS abstraction (`temps-dns`): the `DnsProvider` trait exposes full record CRUD (`create_record`, `update_record`, `delete_record`, `set_record`, `remove_record`), zone listing, and a `DnsProviderCapabilities` struct that already models `proxy`, `auto_ssl`, and `wildcard`. Today this is used almost exclusively for ACME DNS-01 challenge TXT records. Nothing in temps creates the A/AAAA/CNAME records that actually point a domain at the server — users do that manually. + +For a user whose threat model forbids exposing the origin IP, every hostname must be a **Cloudflare-proxied** record. Two things break: + +1. **Manual toil per hostname.** Each custom domain, environment subdomain, and preview URL needs a proxied record created by hand in the Cloudflare dashboard. +2. **Cloudflare's Universal SSL depth limit.** Cloudflare's free/pro certificates only cover one subdomain level. A proxied `*.foo.example.com` fails TLS unless the user buys Advanced Certificate Manager (~$200/mo, per the contributor). The workaround is flattening: `*-foo.example.com` instead of `*.foo.example.com` — exactly what PR #146 implements as the "flat public hostname strategy." + +A secondary risk raised in the discussion: if temps auto-provisions a public hostname + Let's Encrypt certificate per deployment, 100 deployments in a day exhausts LE rate limits. Behind the Cloudflare proxy this is unnecessary anyway — Cloudflare terminates public TLS, and the origin can serve a self-signed or origin certificate. + +### Why the scary part is scary + +Temps would be writing to zones it does not own. Users have existing records — MX, SPF, apex A records, records managed by other tools. Overwriting an unmanaged record is the one mistake this feature cannot make: self-hosted users debug alone, and a clobbered production DNS record is an outage they may not trace back to temps for hours. + +## Decision + +Add **managed DNS record automation** as an opt-in, per-domain feature on top of the existing `DnsProvider` trait, with an ownership-marking scheme that makes "never touch a record temps didn't create" structurally enforced, plus first-class Cloudflare proxied mode that composes with PR #146's flat hostname strategy. + +### 1. Ownership marking (the core safety invariant) + +Every record temps creates carries a machine-readable ownership marker: a companion TXT record `_temps-owned.` holding typed JSON, e.g. `{"managed_by":"temps","instance":"","project_id":N,"environment_id":N,"v":1}` (the external-dns registry pattern). This works uniformly across all providers. + +*Implementation note (v1):* Cloudflare's per-record `comment` field was originally preferred there for dashboard visibility, but the `cloudflare` crate's DNS params don't expose it, so v1 uses the TXT registry on Cloudflare too. Comment stamping can be added later as a purely additive enhancement (the TXT registry stays authoritative). + +Rules, enforced in the service layer, not left to callers: + +- **Create:** if a record with the target name/type already exists and has no parseable temps marker → refuse, surface a conflict. +- **Update/Delete:** only permitted when the existing record's marker parses and matches this temps instance. Unparsable or foreign marker → refuse. +- **Conflict resolution UI:** on conflict, offer *import* (adopt the record: stamp it with a marker after explicit user confirmation) or *skip*. Default is always **never overwrite**. No bulk "overwrite all." + +### 2. Provider-agnostic surface, one provider per zone + +Record automation is configured per domain/zone, reusing the existing DNS provider credential records (encrypted via `EncryptionService`, per the no-env-var rule). One DNS provider per zone; the UI enforces this. Providers advertise support via the existing `DnsProviderCapabilities` — a zone on a provider without `a_record`/`cname_record` support falls back to today's manual instructions (`ManualDnsProvider` behavior). + +### 3. Cloudflare proxied mode + flat hostname coupling + +- `proxied: bool` on the managed-record config, only offered when `capabilities().proxy` is true. +- **Guardrail:** when a domain plan would create a proxied record at ≥2 subdomain levels (`*.foo.example.com`, `a.b.example.com`), temps must detect it, explain the Universal SSL depth limit in the error/warning, and recommend the flat hostname strategy (PR #146). Failing with Cloudflare's opaque 526/525 at request time is not acceptable. +- PR #146 is therefore a **merge prerequisite** for the proxied path. + +### 4. TLS strategy behind the proxy + +When a domain's records are proxied, the per-hostname Let's Encrypt flow is skipped by default. The origin serves a temps-generated self-signed certificate (Cloudflare Full mode) — this both removes the LE rate-limit exposure and matches how Cloudflare-fronted origins normally run. Authenticated Origin Pulls and Cloudflare Origin CA certificates are explicitly deferred (see Non-goals) but the config shape must not preclude them. + +### 5. Deployment / preview URL policy + +Per-project setting controlling what deployment and preview URLs get: + +- **(a)** no public DNS record (default — internal/testing use only, today's behavior), +- **(b)** flat-scheme records under the managed zone (requires #146), +- **(c)** *(deferred)* a separate TLD, potentially on a different DNS provider, for non-prod. + +v1 ships (a) and (b). (c) is a config-model consideration only: the setting is per-environment-class, not a single boolean, so adding (c) later is non-breaking. + +### 6. Defaults and observability + +- Record automation is **off** until a provider is explicitly connected and enabled per domain. +- Domain UI shows per-record state: `created` / `conflict` / `unmanaged` / `error`, each with the provider's actual error text. +- All record writes are audit-logged; reconciliation is O(changes) (triggered by domain/environment mutations), not a periodic full-zone rescan. + +## Alternatives considered + +- **Cloudflare-only integration (contributor's fork approach).** Fastest to his need, but temps already has six DNS providers behind one trait; a Cloudflare-specific path would fork the domain model and contradict the core-primitives philosophy. +- **No ownership marker, name-based matching only.** Simpler, but "temps deletes whatever matches the name" is exactly the clobbering failure mode. Rejected. +- **TXT-registry for all providers including Cloudflare.** Uniform, but the comment field is more visible in the Cloudflare dashboard (an operator sees *why* the record exists) and avoids doubling record count. Cloudflare uses comments; TXT is the generic fallback. +- **Keep LE per-hostname certs behind the proxy.** Works in Cloudflare Full (strict) only with valid origin certs and re-introduces rate-limit exposure per deployment. Default off behind proxy; still available for non-proxied records. + +## Non-goals (v1) + +- Cloudflare Origin CA certificate issuance and Authenticated Origin Pulls. +- Different TLD / different provider for non-prod deployments (5c). +- Managing records temps did not create, beyond the explicit one-at-a-time import flow. +- MX/SPF/any records unrelated to routing traffic to temps. + +## Consequences + +- The ownership scheme is the one-way door: once user zones contain temps-marked records, the marker format is a compatibility surface. It carries a `"v":1` field for that reason. +- PR #146 becomes load-bearing for the proxied path and must merge first. +- Providers gain no new trait methods for v1 — the work is a new orchestration service in `temps-dns`/`temps-domains` plus entities for per-domain automation config and record state. +- Security review must cover: zone-scoped token guidance in docs, marker spoofing (a foreign record with a forged temps marker — mitigated by `instance` install-id matching), and audit coverage of every write. From 361f2c1a5f9567a91241913196e695e17aa418c2 Mon Sep 17 00:00:00 2001 From: David Viejo Date: Tue, 14 Jul 2026 11:09:38 +0200 Subject: [PATCH 2/3] feat(dns): harden ownership guards, add managed-record API with audit logging Fixes from the ADR-031 security/code review of the foundation slice: - BLOCKING registry-TXT clobber: guarded_set/import now inspect the ownership registry name before writing; a non-marker TXT or another install's orphan marker at that name refuses with a typed conflict instead of being upserted over - BLOCKING type-scoping: registry names are per record type (_temps-owned-a. etc.) and markers carry record_type; owning a name's A record no longer reads as ownership of the user's AAAA/CNAME at the same name - BLOCKING wildcard collision: injective name escaping (underscore doubling then * -> _w) so *.staging and a literal wildcard.staging can never share a registry name - TOCTOU: per-(zone,name) keyed async locks serialize guarded ops in-process (self-cleaning map, bounded memory); remaining remote window documented as accepted - marker instance field validated on parse ([A-Za-z0-9-], <=64) to keep attacker-written TXT content out of logs/UI - proxied_by_default on the managed domain is now consumed by set_managed_record; proxy gate extracted to testable check_proxy_allowed - removal granularity (whole name+type unit) documented New HTTP API (all RequireAuth + permission_check + audit logged): - GET /dns-records/ownership per-record state for the conflict UI - POST /dns-records ownership-guarded create/update - DELETE /dns-records ownership-guarded removal - POST /dns-records/import explicit adoption of an existing record - new DnsError variants mapped to 409/400 Problem responses Tests: 271 passing in temps-dns (+12), covering registry clobber refusal, foreign orphan markers, our-orphan reuse, type-scoped ownership, injective escaping, proxy gate, keyed-lock serialization and cleanup, and instance_id get-or-create incl. insert-race recovery via MockDatabase --- .../temps-dns/src/handlers/managed_records.rs | 418 ++++++++++ crates/temps-dns/src/handlers/mod.rs | 42 + crates/temps-dns/src/ownership.rs | 226 +++++- crates/temps-dns/src/plugin.rs | 6 +- .../temps-dns/src/services/managed_records.rs | 759 +++++++++++++++--- ...dns-records-and-cloudflare-proxied-mode.md | 13 +- 6 files changed, 1299 insertions(+), 165 deletions(-) create mode 100644 crates/temps-dns/src/handlers/managed_records.rs diff --git a/crates/temps-dns/src/handlers/managed_records.rs b/crates/temps-dns/src/handlers/managed_records.rs new file mode 100644 index 000000000..d423be290 --- /dev/null +++ b/crates/temps-dns/src/handlers/managed_records.rs @@ -0,0 +1,418 @@ +//! HTTP handlers for ownership-guarded managed DNS records (ADR-031) +//! +//! These endpoints power the domain UI's per-record state and the +//! import-or-skip conflict flow. All writes go through +//! [`ManagedDnsRecordService`], so the never-overwrite invariant is enforced +//! in the service layer regardless of what the client sends; conflicts come +//! back as RFC 7807 responses with HTTP 409. + +use axum::{ + extract::{Query, State}, + http::StatusCode, + response::IntoResponse, + Extension, Json, +}; +use serde::{Deserialize, Serialize}; +use std::sync::Arc; +use temps_auth::{permission_check, Permission, RequireAuth}; +use temps_core::audit::{AuditContext, AuditOperation}; +use temps_core::problemdetails::Problem; +use temps_core::RequestMetadata; +use tracing::error; +use utoipa::ToSchema; + +use crate::providers::{DnsRecord, DnsRecordContent, DnsRecordRequest, DnsRecordType}; +use crate::services::{OwnershipScope, RecordOwnership}; + +use super::DnsAppState; + +// ======================================== +// Request/Response Types +// ======================================== + +/// Query selecting one record by zone-relative name and type +#[derive(Debug, Clone, Deserialize, ToSchema, utoipa::IntoParams)] +pub struct ManagedRecordQuery { + /// Domain (any FQDN under a managed zone) + #[schema(example = "example.com")] + pub domain: String, + /// Record name relative to the zone ("@" for apex) + #[schema(example = "app")] + pub name: String, + /// Record type + pub record_type: DnsRecordType, +} + +/// Request to create or update a managed DNS record +#[derive(Debug, Clone, Deserialize, ToSchema)] +pub struct SetManagedRecordRequest { + /// Domain (any FQDN under a managed zone) + #[schema(example = "example.com")] + pub domain: String, + /// Record name relative to the zone ("@" for apex) + #[schema(example = "app")] + pub name: String, + /// Record content (determines the record type) + pub content: DnsRecordContent, + /// TTL in seconds (None = provider default) + pub ttl: Option, + /// Proxy through the provider's CDN (Cloudflare orange-cloud). Also + /// enabled by the managed domain's `proxied_by_default`. + #[serde(default)] + pub proxied: bool, + /// Project this record belongs to (stamped into the ownership marker) + pub project_id: Option, + /// Environment this record belongs to (stamped into the ownership marker) + pub environment_id: Option, +} + +/// Request to import (adopt) an existing record into temps management +#[derive(Debug, Clone, Deserialize, ToSchema)] +pub struct ImportManagedRecordRequest { + /// Domain (any FQDN under a managed zone) + #[schema(example = "example.com")] + pub domain: String, + /// Record name relative to the zone ("@" for apex) + #[schema(example = "app")] + pub name: String, + /// Record type + pub record_type: DnsRecordType, + /// Project this record belongs to (stamped into the ownership marker) + pub project_id: Option, + /// Environment this record belongs to (stamped into the ownership marker) + pub environment_id: Option, +} + +/// Ownership state of one record, for the conflict/import UI +#[derive(Debug, Clone, Serialize, ToSchema)] +pub struct RecordOwnershipResponse { + /// One of: not_found | unmanaged | owned | owned_by_other + #[schema(example = "unmanaged")] + pub status: String, + /// The record at the provider, when one exists + pub record: Option, + /// Whether this temps install may modify the record + pub writable: bool, + /// Owning install's instance ID when owned by a different temps install + pub owner_instance: Option, + /// Project stamped in the ownership marker, when owned + pub project_id: Option, + /// Environment stamped in the ownership marker, when owned + pub environment_id: Option, +} + +impl From for RecordOwnershipResponse { + fn from(ownership: RecordOwnership) -> Self { + match ownership { + RecordOwnership::NotFound => Self { + status: "not_found".to_string(), + record: None, + writable: true, + owner_instance: None, + project_id: None, + environment_id: None, + }, + RecordOwnership::Unmanaged(record) => Self { + status: "unmanaged".to_string(), + record: Some(record), + writable: false, + owner_instance: None, + project_id: None, + environment_id: None, + }, + RecordOwnership::Owned(record, marker) => Self { + status: "owned".to_string(), + record: Some(record), + writable: true, + owner_instance: None, + project_id: marker.project_id, + environment_id: marker.environment_id, + }, + RecordOwnership::OwnedByOther(record, marker) => Self { + status: "owned_by_other".to_string(), + record: Some(record), + writable: false, + owner_instance: Some(marker.instance), + project_id: marker.project_id, + environment_id: marker.environment_id, + }, + } + } +} + +/// Result of importing a record into temps management +#[derive(Debug, Clone, Serialize, ToSchema)] +pub struct ImportManagedRecordResponse { + /// Record name that was imported + pub name: String, + /// Record type that was imported + pub record_type: String, + /// Project stamped in the ownership marker + pub project_id: Option, + /// Environment stamped in the ownership marker + pub environment_id: Option, +} + +// ======================================== +// Audit events +// ======================================== + +#[derive(Debug, Clone, Serialize)] +struct ManagedDnsRecordSetAudit { + context: AuditContext, + domain: String, + name: String, + record_type: String, + proxied: bool, + project_id: Option, + environment_id: Option, +} + +#[derive(Debug, Clone, Serialize)] +struct ManagedDnsRecordRemovedAudit { + context: AuditContext, + domain: String, + name: String, + record_type: String, +} + +#[derive(Debug, Clone, Serialize)] +struct ManagedDnsRecordImportedAudit { + context: AuditContext, + domain: String, + name: String, + record_type: String, + project_id: Option, + environment_id: Option, +} + +macro_rules! impl_audit_operation { + ($ty:ty, $op:literal) => { + impl AuditOperation for $ty { + fn operation_type(&self) -> String { + $op.to_string() + } + fn user_id(&self) -> i32 { + self.context.user_id + } + fn ip_address(&self) -> Option { + self.context.ip_address.clone() + } + fn user_agent(&self) -> &str { + &self.context.user_agent + } + fn serialize(&self) -> anyhow::Result { + serde_json::to_string(self) + .map_err(|e| anyhow::anyhow!("Failed to serialize audit operation: {}", e)) + } + } + }; +} + +impl_audit_operation!(ManagedDnsRecordSetAudit, "MANAGED_DNS_RECORD_SET"); +impl_audit_operation!(ManagedDnsRecordRemovedAudit, "MANAGED_DNS_RECORD_REMOVED"); +impl_audit_operation!(ManagedDnsRecordImportedAudit, "MANAGED_DNS_RECORD_IMPORTED"); + +fn audit_context(auth: &temps_auth::AuthContext, metadata: &RequestMetadata) -> AuditContext { + AuditContext { + user_id: auth.user_id(), + ip_address: Some(metadata.ip_address.clone()), + user_agent: metadata.user_agent.clone(), + } +} + +// ======================================== +// Handlers +// ======================================== + +/// Get the ownership state of a DNS record +#[utoipa::path( + tag = "DNS Records", + get, + path = "/dns-records/ownership", + params(ManagedRecordQuery), + responses( + (status = 200, description = "Ownership state", body = RecordOwnershipResponse), + (status = 401, description = "Unauthorized"), + (status = 403, description = "Insufficient permissions"), + (status = 404, description = "Domain not managed by any DNS provider"), + ), + security(("bearer_auth" = [])) +)] +pub(super) async fn get_record_ownership( + RequireAuth(auth): RequireAuth, + State(state): State>, + Query(query): Query, +) -> Result { + permission_check!(auth, Permission::SettingsRead); + + let ownership = state + .managed_record_service + .record_ownership(&query.domain, &query.name, query.record_type) + .await?; + + Ok(Json(RecordOwnershipResponse::from(ownership))) +} + +/// Create or update a managed DNS record (ownership-guarded) +#[utoipa::path( + tag = "DNS Records", + post, + path = "/dns-records", + request_body = SetManagedRecordRequest, + responses( + (status = 200, description = "Record set", body = DnsRecord), + (status = 400, description = "Validation error (e.g. proxied depth limit)"), + (status = 401, description = "Unauthorized"), + (status = 403, description = "Insufficient permissions"), + (status = 404, description = "Domain not managed by any DNS provider"), + (status = 409, description = "Record exists and is not managed by temps"), + ), + security(("bearer_auth" = [])) +)] +pub(super) async fn set_managed_record( + RequireAuth(auth): RequireAuth, + State(state): State>, + Extension(metadata): Extension, + Json(request): Json, +) -> Result { + permission_check!(auth, Permission::SettingsWrite); + + let record = state + .managed_record_service + .set_managed_record( + &request.domain, + DnsRecordRequest { + name: request.name.clone(), + content: request.content.clone(), + ttl: request.ttl, + proxied: request.proxied, + }, + OwnershipScope { + project_id: request.project_id, + environment_id: request.environment_id, + }, + ) + .await?; + + let audit = ManagedDnsRecordSetAudit { + context: audit_context(&auth, &metadata), + domain: request.domain.clone(), + name: request.name.clone(), + record_type: record.content.record_type().to_string(), + proxied: record.proxied, + project_id: request.project_id, + environment_id: request.environment_id, + }; + if let Err(e) = state.audit_service.create_audit_log(&audit).await { + error!( + "Failed to create audit log for managed DNS record set: {}", + e + ); + } + + Ok(Json(record)) +} + +/// Delete a managed DNS record (only records owned by this install) +#[utoipa::path( + tag = "DNS Records", + delete, + path = "/dns-records", + params(ManagedRecordQuery), + responses( + (status = 204, description = "Record removed (or already absent)"), + (status = 401, description = "Unauthorized"), + (status = 403, description = "Insufficient permissions"), + (status = 404, description = "Domain not managed by any DNS provider"), + (status = 409, description = "Record is not managed by temps"), + ), + security(("bearer_auth" = [])) +)] +pub(super) async fn remove_managed_record( + RequireAuth(auth): RequireAuth, + State(state): State>, + Extension(metadata): Extension, + Query(query): Query, +) -> Result { + permission_check!(auth, Permission::SettingsWrite); + + state + .managed_record_service + .remove_managed_record(&query.domain, &query.name, query.record_type) + .await?; + + let audit = ManagedDnsRecordRemovedAudit { + context: audit_context(&auth, &metadata), + domain: query.domain.clone(), + name: query.name.clone(), + record_type: query.record_type.to_string(), + }; + if let Err(e) = state.audit_service.create_audit_log(&audit).await { + error!( + "Failed to create audit log for managed DNS record removal: {}", + e + ); + } + + Ok(StatusCode::NO_CONTENT) +} + +/// Import an existing DNS record into temps management (explicit adoption) +#[utoipa::path( + tag = "DNS Records", + post, + path = "/dns-records/import", + request_body = ImportManagedRecordRequest, + responses( + (status = 200, description = "Record imported", body = ImportManagedRecordResponse), + (status = 401, description = "Unauthorized"), + (status = 403, description = "Insufficient permissions"), + (status = 404, description = "Record or managed domain not found"), + (status = 409, description = "Record is owned by another temps install"), + ), + security(("bearer_auth" = [])) +)] +pub(super) async fn import_managed_record( + RequireAuth(auth): RequireAuth, + State(state): State>, + Extension(metadata): Extension, + Json(request): Json, +) -> Result { + permission_check!(auth, Permission::SettingsWrite); + + let marker = state + .managed_record_service + .import_record( + &request.domain, + &request.name, + request.record_type, + OwnershipScope { + project_id: request.project_id, + environment_id: request.environment_id, + }, + ) + .await?; + + let audit = ManagedDnsRecordImportedAudit { + context: audit_context(&auth, &metadata), + domain: request.domain.clone(), + name: request.name.clone(), + record_type: request.record_type.to_string(), + project_id: marker.project_id, + environment_id: marker.environment_id, + }; + if let Err(e) = state.audit_service.create_audit_log(&audit).await { + error!( + "Failed to create audit log for managed DNS record import: {}", + e + ); + } + + Ok(Json(ImportManagedRecordResponse { + name: request.name, + record_type: request.record_type.to_string(), + project_id: marker.project_id, + environment_id: marker.environment_id, + })) +} diff --git a/crates/temps-dns/src/handlers/mod.rs b/crates/temps-dns/src/handlers/mod.rs index 635d409ce..cdb8f34d2 100644 --- a/crates/temps-dns/src/handlers/mod.rs +++ b/crates/temps-dns/src/handlers/mod.rs @@ -9,6 +9,7 @@ //! [`dns_sync::DnsSyncAppState`]. pub mod dns_sync; +pub mod managed_records; use axum::{ extract::{Path, State}, @@ -38,6 +39,8 @@ use crate::services::{ pub struct DnsAppState { pub provider_service: Arc, pub record_service: Arc, + pub managed_record_service: Arc, + pub audit_service: Arc, } // ======================================== @@ -296,6 +299,22 @@ impl From for Problem { DnsError::ApiError(msg) => problemdetails::new(StatusCode::BAD_GATEWAY) .with_title("API Error") .with_detail(msg), + DnsError::RecordConflict { .. } => problemdetails::new(StatusCode::CONFLICT) + .with_title("DNS Record Conflict") + .with_detail(error.to_string()), + DnsError::NotOwnedByInstance { .. } => problemdetails::new(StatusCode::CONFLICT) + .with_title("Record Owned By Another Temps Install") + .with_detail(error.to_string()), + DnsError::ProxiedDepthUnsupported { .. } => { + problemdetails::new(StatusCode::BAD_REQUEST) + .with_title("Proxied Subdomain Depth Not Supported") + .with_detail(error.to_string()) + } + DnsError::ProxyNotSupportedByProvider { .. } => { + problemdetails::new(StatusCode::BAD_REQUEST) + .with_title("Proxy Not Supported By Provider") + .with_detail(error.to_string()) + } _ => problemdetails::new(StatusCode::INTERNAL_SERVER_ERROR) .with_title("Internal Error") .with_detail(error.to_string()), @@ -823,6 +842,20 @@ pub fn configure_routes() -> Router> { "/dns-providers/{provider_id}/domains/{domain}/verify", post(verify_managed_domain), ) + // Ownership-guarded managed records (ADR-031) + .route( + "/dns-records", + post(managed_records::set_managed_record) + .delete(managed_records::remove_managed_record), + ) + .route( + "/dns-records/ownership", + get(managed_records::get_record_ownership), + ) + .route( + "/dns-records/import", + post(managed_records::import_managed_record), + ) } /// Configure internal DNS sync routes (ADR-011). @@ -860,6 +893,10 @@ pub fn configure_internal_routes() -> Router> { list_managed_domains, remove_managed_domain, verify_managed_domain, + managed_records::get_record_ownership, + managed_records::set_managed_record, + managed_records::remove_managed_record, + managed_records::import_managed_record, dns_sync::get_dns_changes, dns_sync::post_dns_ack, ), @@ -877,6 +914,10 @@ pub fn configure_internal_routes() -> Router> { DnsProviderType, DnsZone, DnsRecord, + managed_records::SetManagedRecordRequest, + managed_records::ImportManagedRecordRequest, + managed_records::RecordOwnershipResponse, + managed_records::ImportManagedRecordResponse, dns_sync::EndpointDto, dns_sync::DnsChangesResponse, dns_sync::DnsAckRequest, @@ -885,6 +926,7 @@ pub fn configure_internal_routes() -> Router> { ), tags( (name = "DNS Providers", description = "DNS provider management endpoints"), + (name = "DNS Records", description = "Ownership-guarded managed DNS records (ADR-031)"), (name = "Internal DNS", description = "Per-node DNS resolver sync (ADR-011)"), ) )] diff --git a/crates/temps-dns/src/ownership.rs b/crates/temps-dns/src/ownership.rs index 5e56db046..6612157ac 100644 --- a/crates/temps-dns/src/ownership.rs +++ b/crates/temps-dns/src/ownership.rs @@ -3,10 +3,16 @@ //! Temps writes public A/AAAA/CNAME records into zones it does not own. //! The one mistake this feature must never make is touching a record temps //! did not create. Ownership is therefore recorded *at the provider*, next to -//! the record itself, as a companion TXT "registry" record -//! (`_temps-owned.`) whose content is a typed JSON marker. Before any -//! update or delete, the marker is fetched and must parse AND match this -//! install's instance ID; anything else refuses the write. +//! the record itself, as a companion TXT "registry" record whose content is a +//! typed JSON marker. Before any update or delete, the marker is fetched and +//! must parse AND match this install's instance ID AND cover the record's +//! type; anything else refuses the write. +//! +//! The registry name is scoped by record type — `_temps-owned-a.`, +//! `_temps-owned-aaaa.`, … — so owning `app` A never grants ownership +//! of a user's `app` AAAA. The record name is escaped injectively (`_` → `__` +//! before `*` → `_w`) so no two distinct record names can share a registry +//! name (`*.staging` vs a literal `wildcard.staging`). //! //! The companion-TXT scheme works uniformly across every provider. Cloudflare //! additionally has a per-record `comment` field, but the `cloudflare` crate's @@ -20,6 +26,7 @@ use serde::{Deserialize, Serialize}; use crate::errors::DnsError; +use crate::providers::{DnsProviderCapabilities, DnsRecordType}; /// Current marker format version. pub const OWNERSHIP_MARKER_VERSION: u32 = 1; @@ -27,12 +34,14 @@ pub const OWNERSHIP_MARKER_VERSION: u32 = 1; /// Value of `managed_by` in every marker temps writes. pub const OWNERSHIP_MANAGED_BY: &str = "temps"; -/// Label prefix of the companion TXT registry record. +/// Label prefix of the companion TXT registry record. The record type is +/// appended (`_temps-owned-a`, `_temps-owned-cname`, …) so ownership is +/// scoped per (name, type), not per name. pub const OWNERSHIP_REGISTRY_PREFIX: &str = "_temps-owned"; -/// Replacement for the `*` label when building a registry name for a -/// wildcard record (`*` is not a meaningful label to prefix). -const WILDCARD_REPLACEMENT: &str = "wildcard"; +/// Maximum accepted length for the `instance` field when parsing markers. +/// Our own IDs are 36-char UUIDs; anything longer is not ours. +const MAX_INSTANCE_LEN: usize = 64; /// Ownership marker stored in the companion TXT record. /// @@ -47,6 +56,11 @@ pub struct OwnershipMarker { /// Install-scoped random ID of the temps instance that created the record. pub instance: String, + /// Record type this marker covers (e.g. "A"). Belt-and-braces on top of + /// the type-scoped registry name; a mismatch means "not ours". + #[serde(default, skip_serializing_if = "Option::is_none")] + pub record_type: Option, + /// Project the record was created for, when known. #[serde(default, skip_serializing_if = "Option::is_none")] pub project_id: Option, @@ -60,10 +74,16 @@ pub struct OwnershipMarker { } impl OwnershipMarker { - pub fn new(instance: &str, project_id: Option, environment_id: Option) -> Self { + pub fn new( + instance: &str, + record_type: DnsRecordType, + project_id: Option, + environment_id: Option, + ) -> Self { Self { managed_by: OWNERSHIP_MANAGED_BY.to_string(), instance: instance.to_string(), + record_type: Some(record_type.to_string()), project_id, environment_id, v: OWNERSHIP_MARKER_VERSION, @@ -78,37 +98,69 @@ impl OwnershipMarker { /// Parse a TXT record content as an ownership marker. /// /// Returns `None` for anything that is not a well-formed temps marker — - /// unparsable JSON, wrong `managed_by`, missing fields. Callers treat - /// `None` as "not ours: hands off". + /// unparsable JSON, wrong `managed_by`, missing fields, or an `instance` + /// outside the ID charset. Callers treat `None` as "not ours: hands off". + /// + /// The instance charset check ([A-Za-z0-9-], ≤ 64 chars) also keeps + /// attacker-written TXT content (newlines, ANSI, oversized strings) out of + /// temps' logs and error messages, where the field is interpolated. pub fn parse(content: &str) -> Option { let marker: Self = serde_json::from_str(content.trim()).ok()?; - if marker.managed_by != OWNERSHIP_MANAGED_BY || marker.instance.is_empty() { + if marker.managed_by != OWNERSHIP_MANAGED_BY { + return None; + } + if marker.instance.is_empty() + || marker.instance.len() > MAX_INSTANCE_LEN + || !marker + .instance + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '-') + { return None; } Some(marker) } + /// Whether this marker was written by the given temps instance AND covers + /// the given record type. A marker without a `record_type` (never written + /// by temps) does not cover anything. + pub fn covers(&self, instance: &str, record_type: DnsRecordType) -> bool { + self.instance == instance && self.record_type.as_deref() == Some(&record_type.to_string()) + } + /// Whether this marker was written by the given temps instance. pub fn is_owned_by(&self, instance: &str) -> bool { self.instance == instance } } -/// Name of the companion TXT registry record for a managed record name. +/// Injective escaping of a record name for use inside a registry name. /// -/// - `@` / empty (zone apex) → `_temps-owned` -/// - `www` → `_temps-owned.www` -/// - `*-staging` → `_temps-owned.wildcard-staging` -/// - `*.staging` → `_temps-owned.wildcard.staging` +/// `_` → `__` first, then `*` → `_w`; because every escape sequence starts +/// with `_` and literal underscores are doubled, no two distinct record names +/// map to the same escaped form (a literal `_w` becomes `__w`). +fn escape_record_name(record_name: &str) -> String { + record_name.replace('_', "__").replace('*', "_w") +} + +/// Name of the companion TXT registry record for a managed record. +/// +/// - (`@` / empty, A) → `_temps-owned-a` +/// - (`www`, A) → `_temps-owned-a.www` +/// - (`*-staging`, CNAME) → `_temps-owned-cname._w-staging` +/// - (`*.staging`, A) → `_temps-owned-a._w.staging` /// -/// The wildcard label is replaced because `_temps-owned.*` is not a queryable -/// name; the replacement is deterministic so lookups and writes agree. -pub fn registry_record_name(record_name: &str) -> String { +/// Type-scoped and injective — see the module docs for why both matter. +pub fn registry_record_name(record_name: &str, record_type: DnsRecordType) -> String { + let prefix = format!( + "{}-{}", + OWNERSHIP_REGISTRY_PREFIX, + record_type.to_string().to_lowercase() + ); if record_name == "@" || record_name.is_empty() { - return OWNERSHIP_REGISTRY_PREFIX.to_string(); + return prefix; } - let sanitized = record_name.replace('*', WILDCARD_REPLACEMENT); - format!("{}.{}", OWNERSHIP_REGISTRY_PREFIX, sanitized) + format!("{}.{}", prefix, escape_record_name(record_name)) } /// Number of subdomain levels a record name adds below the zone apex. @@ -143,22 +195,44 @@ pub fn check_proxied_depth(zone: &str, record_name: &str) -> Result<(), DnsError }) } +/// Full proxied-write gate: the provider must support proxying and the +/// record must pass the depth guardrail. Pure over the capabilities so it is +/// unit-testable without a database or provider API. +pub fn check_proxy_allowed( + capabilities: &DnsProviderCapabilities, + provider_name: &str, + zone: &str, + record_name: &str, +) -> Result<(), DnsError> { + if !capabilities.proxy { + return Err(DnsError::ProxyNotSupportedByProvider { + provider: provider_name.to_string(), + }); + } + check_proxied_depth(zone, record_name) +} + #[cfg(test)] mod tests { use super::*; + fn marker(record_type: DnsRecordType) -> OwnershipMarker { + OwnershipMarker::new("inst-abc123", record_type, Some(7), Some(42)) + } + #[test] fn marker_round_trips_through_txt_content() { - let marker = OwnershipMarker::new("inst-abc123", Some(7), Some(42)); + let marker = marker(DnsRecordType::A); let content = marker.to_txt_content().unwrap(); let parsed = OwnershipMarker::parse(&content).unwrap(); assert_eq!(parsed, marker); assert_eq!(parsed.v, OWNERSHIP_MARKER_VERSION); + assert_eq!(parsed.record_type.as_deref(), Some("A")); } #[test] fn marker_without_scope_omits_ids_in_json() { - let marker = OwnershipMarker::new("inst-abc123", None, None); + let marker = OwnershipMarker::new("inst-abc123", DnsRecordType::A, None, None); let content = marker.to_txt_content().unwrap(); assert!(!content.contains("project_id")); assert!(!content.contains("environment_id")); @@ -180,9 +254,25 @@ mod tests { } #[test] - fn parse_rejects_empty_instance() { - let content = r#"{"managed_by":"temps","instance":"","v":1}"#; - assert!(OwnershipMarker::parse(content).is_none()); + fn parse_rejects_invalid_instance() { + // Empty + assert!(OwnershipMarker::parse(r#"{"managed_by":"temps","instance":"","v":1}"#).is_none()); + // Charset: log/UI injection payloads must not survive parse + assert!(OwnershipMarker::parse( + r#"{"managed_by":"temps","instance":"evil\nFORGED LOG LINE","v":1}"# + ) + .is_none()); + assert!(OwnershipMarker::parse( + r#"{"managed_by":"temps","instance":"","v":1}"# + ) + .is_none()); + // Oversized + let long = "a".repeat(65); + assert!(OwnershipMarker::parse(&format!( + r#"{{"managed_by":"temps","instance":"{}","v":1}}"#, + long + )) + .is_none()); } #[test] @@ -193,24 +283,58 @@ mod tests { } #[test] - fn ownership_is_instance_scoped() { - let marker = OwnershipMarker::new("inst-a", None, None); - assert!(marker.is_owned_by("inst-a")); - assert!(!marker.is_owned_by("inst-b")); + fn covers_requires_instance_and_record_type() { + let m = marker(DnsRecordType::A); + assert!(m.covers("inst-abc123", DnsRecordType::A)); + assert!(!m.covers("inst-abc123", DnsRecordType::AAAA)); + assert!(!m.covers("other", DnsRecordType::A)); + + // A marker with no record_type (not something temps writes) covers nothing. + let untyped = + OwnershipMarker::parse(r#"{"managed_by":"temps","instance":"inst-abc123","v":1}"#) + .unwrap(); + assert!(!untyped.covers("inst-abc123", DnsRecordType::A)); } #[test] - fn registry_name_for_apex_and_subdomains() { - assert_eq!(registry_record_name("@"), "_temps-owned"); - assert_eq!(registry_record_name(""), "_temps-owned"); - assert_eq!(registry_record_name("www"), "_temps-owned.www"); + fn registry_name_is_type_scoped() { assert_eq!( - registry_record_name("*-staging"), - "_temps-owned.wildcard-staging" + registry_record_name("app", DnsRecordType::A), + "_temps-owned-a.app" ); assert_eq!( - registry_record_name("*.staging"), - "_temps-owned.wildcard.staging" + registry_record_name("app", DnsRecordType::AAAA), + "_temps-owned-aaaa.app" + ); + assert_ne!( + registry_record_name("app", DnsRecordType::A), + registry_record_name("app", DnsRecordType::CNAME) + ); + assert_eq!( + registry_record_name("@", DnsRecordType::A), + "_temps-owned-a" + ); + assert_eq!(registry_record_name("", DnsRecordType::A), "_temps-owned-a"); + } + + #[test] + fn registry_name_escaping_is_injective_for_wildcards() { + // The classic collision: a wildcard vs a literal name that the old + // '*' -> "wildcard" replacement would have merged. + let wildcard = registry_record_name("*.staging", DnsRecordType::A); + let literal = registry_record_name("wildcard.staging", DnsRecordType::A); + assert_ne!(wildcard, literal); + assert_eq!(wildcard, "_temps-owned-a._w.staging"); + + // A literal that looks like the escape sequence itself. + let escaped_literal = registry_record_name("_w.staging", DnsRecordType::A); + assert_ne!(wildcard, escaped_literal); + assert_eq!(escaped_literal, "_temps-owned-a.__w.staging"); + + // Underscore doubling round-trip distinctness. + assert_ne!( + registry_record_name("a_b", DnsRecordType::A), + registry_record_name("a__b", DnsRecordType::A) ); } @@ -247,4 +371,26 @@ mod tests { other => panic!("expected ProxiedDepthUnsupported, got {:?}", other), } } + + #[test] + fn proxy_gate_requires_capability_then_depth() { + let no_proxy = DnsProviderCapabilities::default(); + let err = check_proxy_allowed(&no_proxy, "route53-prod", "example.com", "www").unwrap_err(); + match err { + DnsError::ProxyNotSupportedByProvider { provider } => { + assert_eq!(provider, "route53-prod"); + } + other => panic!("expected ProxyNotSupportedByProvider, got {:?}", other), + } + + let with_proxy = DnsProviderCapabilities { + proxy: true, + ..Default::default() + }; + assert!(check_proxy_allowed(&with_proxy, "cf", "example.com", "www").is_ok()); + assert!(matches!( + check_proxy_allowed(&with_proxy, "cf", "example.com", "*.staging"), + Err(DnsError::ProxiedDepthUnsupported { .. }) + )); + } } diff --git a/crates/temps-dns/src/plugin.rs b/crates/temps-dns/src/plugin.rs index 5fb5b9251..cfbdb9eef 100644 --- a/crates/temps-dns/src/plugin.rs +++ b/crates/temps-dns/src/plugin.rs @@ -65,12 +65,16 @@ impl TempsPlugin for DnsPlugin { db.clone(), provider_service.clone(), )); - context.register_service(managed_record_service); + context.register_service(managed_record_service.clone()); + + let audit_service = context.require_service::(); // Create DnsAppState for handlers let app_state = Arc::new(DnsAppState { provider_service, record_service, + managed_record_service, + audit_service, }); context.register_service(app_state); diff --git a/crates/temps-dns/src/services/managed_records.rs b/crates/temps-dns/src/services/managed_records.rs index d448bbdc1..fdb6ebab3 100644 --- a/crates/temps-dns/src/services/managed_records.rs +++ b/crates/temps-dns/src/services/managed_records.rs @@ -8,19 +8,46 @@ //! //! - **Create/update** refuses if a record with the target name/type exists //! without a temps ownership marker, or with a marker from a different -//! temps install. Conflicts surface as typed [`DnsError::RecordConflict`] / -//! [`DnsError::NotOwnedByInstance`] so the UI can offer import-or-skip. +//! temps install — AND refuses if the ownership registry name itself is +//! occupied by a TXT record that is not our marker (so the marker write can +//! never clobber someone else's TXT). Conflicts surface as typed +//! [`DnsError::RecordConflict`] / [`DnsError::NotOwnedByInstance`] so the +//! UI can offer import-or-skip. //! - **Delete** only removes records this install owns. //! - **Import** is the explicit, user-confirmed adoption path that stamps a //! marker onto a pre-existing record. //! -//! Proxied (Cloudflare orange-cloud) writes additionally pass the Universal -//! SSL depth guardrail — see [`crate::ownership::check_proxied_depth`]. +//! Proxied (Cloudflare orange-cloud) writes additionally pass the proxy +//! capability + Universal SSL depth gate — see +//! [`crate::ownership::check_proxy_allowed`]. //! -//! Write ordering: the ownership marker TXT is written BEFORE the target -//! record. A crash between the two leaves a harmless orphan marker, never a -//! live unmarked record that a later run would refuse to manage. - +//! # Crash ordering +//! +//! The ownership marker TXT is written BEFORE the target record. A crash +//! between the two leaves a harmless orphan marker (which this install may +//! later reuse or clean up), never a live unmarked record that a later run +//! would refuse to manage. +//! +//! # Concurrency (TOCTOU) +//! +//! DNS provider APIs have no compare-and-swap, so a check-then-write window +//! against the remote zone is unavoidable: a record created by someone else +//! between our ownership check and our write can still be overwritten. That +//! residual window is accepted — closing it is impossible without provider +//! transactions. What IS controlled: all guarded operations on the same +//! (zone, record name) within this process are serialized through a keyed +//! async lock, so temps never races itself. +//! +//! # Removal granularity +//! +//! Ownership is per (name, type), and `remove_record` deletes every value at +//! that name+type. If a user manually adds a second A value to a +//! temps-managed name (DNS round-robin), temps removal deletes that value +//! too. Values under an owned name+type are treated as one owned unit; users +//! must not hand-edit temps-managed names (the marker makes them +//! discoverable). + +use std::collections::HashMap; use std::sync::Arc; use sea_orm::{ActiveModelTrait, ActiveValue::Set, DatabaseConnection, EntityTrait}; @@ -28,7 +55,7 @@ use temps_entities::dns_instance_identity; use tracing::{info, warn}; use crate::errors::DnsError; -use crate::ownership::{check_proxied_depth, registry_record_name, OwnershipMarker}; +use crate::ownership::{check_proxy_allowed, registry_record_name, OwnershipMarker}; use crate::providers::{DnsProvider, DnsRecord, DnsRecordContent, DnsRecordRequest, DnsRecordType}; use crate::services::provider_service::DnsProviderService; @@ -56,11 +83,64 @@ pub enum RecordOwnership { OwnedByOther(DnsRecord, OwnershipMarker), } +/// State of the ownership registry name itself (the `_temps-owned-.…` +/// TXT), independent of whether the target record exists. Distinguishing +/// "no TXT" from "a TXT that is not our marker" is what keeps the marker +/// write from ever clobbering foreign content. +#[derive(Debug, Clone)] +enum RegistryState { + /// No TXT record at the registry name. + Absent, + /// Our marker (this instance, covering this record type). + Owned(OwnershipMarker), + /// A valid temps marker from a different install. + Foreign(OwnershipMarker), + /// A TXT record exists but is not a marker that covers this + /// (instance, type) — user content or a tampered/mismatched marker. + /// Never overwrite it. + Occupied, +} + +/// Per-key async locks that self-clean when the last holder releases. +/// +/// Keys are unbounded user input (zone + record name), so entries are removed +/// as soon as no task holds or waits on them — memory stays proportional to +/// in-flight operations, not to history (CLAUDE.md bounded-memory rule). +type LockMap = HashMap<(String, String), Arc>>; + +struct KeyedLocks { + inner: std::sync::Mutex, +} + +impl KeyedLocks { + fn new() -> Self { + Self { + inner: std::sync::Mutex::new(HashMap::new()), + } + } + + fn get(&self, zone: &str, name: &str) -> Arc> { + let mut map = self.inner.lock().expect("keyed lock map poisoned"); + map.entry((zone.to_string(), name.to_string())) + .or_insert_with(|| Arc::new(tokio::sync::Mutex::new(()))) + .clone() + } + + /// Drop the map entry if no one else holds the Arc (map + caller = 2). + fn release(&self, zone: &str, name: &str, handle: Arc>) { + let mut map = self.inner.lock().expect("keyed lock map poisoned"); + if Arc::strong_count(&handle) == 2 { + map.remove(&(zone.to_string(), name.to_string())); + } + } +} + /// Ownership-guarded record management on top of [`DnsProviderService`]. pub struct ManagedDnsRecordService { db: Arc, provider_service: Arc, instance_id: tokio::sync::OnceCell, + locks: KeyedLocks, } impl ManagedDnsRecordService { @@ -69,6 +149,7 @@ impl ManagedDnsRecordService { db, provider_service, instance_id: tokio::sync::OnceCell::new(), + locks: KeyedLocks::new(), } } @@ -111,10 +192,14 @@ impl ManagedDnsRecordService { /// Create or update a managed record, enforcing ownership and proxy /// guardrails. `domain` may be any FQDN under a managed zone; the record /// `request.name` is relative to that zone. + /// + /// If the managed domain has `proxied_by_default` set, the record is + /// proxied even when the request doesn't ask for it (a per-record + /// `proxied: true` also always wins). pub async fn set_managed_record( &self, domain: &str, - request: DnsRecordRequest, + mut request: DnsRecordRequest, scope: OwnershipScope, ) -> Result { let (provider_model, managed) = self @@ -125,22 +210,35 @@ impl ManagedDnsRecordService { let provider = self .provider_service .create_provider_instance(&provider_model)?; - let zone = managed.domain.as_str(); + let zone = managed.domain.clone(); + request.proxied = request.proxied || managed.proxied_by_default; if request.proxied { - if !provider.capabilities().proxy { - return Err(DnsError::ProxyNotSupportedByProvider { - provider: provider_model.name.clone(), - }); - } - check_proxied_depth(zone, &request.name)?; + check_proxy_allowed( + &provider.capabilities(), + &provider_model.name, + &zone, + &request.name, + )?; } let instance = self.instance_id().await?; - let marker = OwnershipMarker::new(&instance, scope.project_id, scope.environment_id); + let marker = OwnershipMarker::new( + &instance, + request.content.record_type(), + scope.project_id, + scope.environment_id, + ); + + let name = request.name.clone(); + let lock = self.locks.get(&zone, &name); + let record = { + let _guard = lock.lock().await; + Self::guarded_set(provider.as_ref(), &zone, request, &marker, &instance).await + }; + self.locks.release(&zone, &name, lock); + let record = record?; - let record = - Self::guarded_set(provider.as_ref(), zone, request, &marker, &instance).await?; info!( "Set managed {} record '{}' in zone {} via provider {} (proxied: {})", record.content.record_type(), @@ -167,10 +265,17 @@ impl ManagedDnsRecordService { let provider = self .provider_service .create_provider_instance(&provider_model)?; - let zone = managed.domain.as_str(); + let zone = managed.domain.clone(); let instance = self.instance_id().await?; - Self::guarded_remove(provider.as_ref(), zone, name, record_type, &instance).await?; + let lock = self.locks.get(&zone, name); + let result = { + let _guard = lock.lock().await; + Self::guarded_remove(provider.as_ref(), &zone, name, record_type, &instance).await + }; + self.locks.release(&zone, name, lock); + result?; + info!( "Removed managed {} record '{}' in zone {} via provider {}", record_type, name, zone, provider_model.name @@ -196,12 +301,25 @@ impl ManagedDnsRecordService { let provider = self .provider_service .create_provider_instance(&provider_model)?; - let zone = managed.domain.as_str(); + let zone = managed.domain.clone(); let instance = self.instance_id().await?; - let marker = - Self::guarded_import(provider.as_ref(), zone, name, record_type, &instance, scope) - .await?; + let lock = self.locks.get(&zone, name); + let marker = { + let _guard = lock.lock().await; + Self::guarded_import( + provider.as_ref(), + &zone, + name, + record_type, + &instance, + scope, + ) + .await + }; + self.locks.release(&zone, name, lock); + let marker = marker?; + info!( "Imported {} record '{}' in zone {} into temps management", record_type, name, zone @@ -242,20 +360,32 @@ impl ManagedDnsRecordService { // of the database and real provider APIs. // ------------------------------------------------------------------ - /// Fetch and parse the ownership marker TXT for a record name, if any. - async fn fetch_marker( + /// State of the ownership registry TXT for (name, type). + async fn registry_state( provider: &dyn DnsProvider, zone: &str, record_name: &str, - ) -> Result, DnsError> { - let registry_name = registry_record_name(record_name); + record_type: DnsRecordType, + instance: &str, + ) -> Result { + let registry_name = registry_record_name(record_name, record_type); let txt = provider .get_record(zone, ®istry_name, DnsRecordType::TXT) .await?; - Ok(txt.and_then(|record| match &record.content { - DnsRecordContent::TXT { content } => OwnershipMarker::parse(content), - _ => None, - })) + let Some(record) = txt else { + return Ok(RegistryState::Absent); + }; + let DnsRecordContent::TXT { content } = &record.content else { + return Ok(RegistryState::Occupied); + }; + Ok(match OwnershipMarker::parse(content) { + None => RegistryState::Occupied, + Some(marker) if marker.covers(instance, record_type) => RegistryState::Owned(marker), + Some(marker) if !marker.is_owned_by(instance) => RegistryState::Foreign(marker), + // Parses, is ours, but doesn't cover this record type — temps + // never writes that at a type-scoped name; treat as untouchable. + Some(_) => RegistryState::Occupied, + }) } async fn ownership_of( @@ -269,12 +399,12 @@ impl ManagedDnsRecordService { let Some(record) = existing else { return Ok(RecordOwnership::NotFound); }; - match Self::fetch_marker(provider, zone, name).await? { - None => Ok(RecordOwnership::Unmanaged(record)), - Some(marker) if marker.is_owned_by(instance) => { - Ok(RecordOwnership::Owned(record, marker)) + match Self::registry_state(provider, zone, name, record_type, instance).await? { + RegistryState::Owned(marker) => Ok(RecordOwnership::Owned(record, marker)), + RegistryState::Foreign(marker) => Ok(RecordOwnership::OwnedByOther(record, marker)), + RegistryState::Absent | RegistryState::Occupied => { + Ok(RecordOwnership::Unmanaged(record)) } - Some(marker) => Ok(RecordOwnership::OwnedByOther(record, marker)), } } @@ -286,12 +416,20 @@ impl ManagedDnsRecordService { instance: &str, ) -> Result { let record_type = request.content.record_type(); - let existed = match Self::ownership_of(provider, zone, &request.name, record_type, instance) - .await? - { - RecordOwnership::NotFound => false, - RecordOwnership::Owned(_, _) => true, - RecordOwnership::Unmanaged(_) => { + let existing = provider + .get_record(zone, &request.name, record_type) + .await?; + let registry = + Self::registry_state(provider, zone, &request.name, record_type, instance).await?; + + // Both the target record AND the registry name must be free or ours. + match (&existing, ®istry) { + // Update of a record we own, or create where our (possibly + // orphaned) marker already sits. + (_, RegistryState::Owned(_)) => {} + // Fresh create: nothing at either name. + (None, RegistryState::Absent) => {} + (Some(_), RegistryState::Absent | RegistryState::Occupied) => { return Err(DnsError::RecordConflict { domain: zone.to_string(), name: request.name.clone(), @@ -299,20 +437,33 @@ impl ManagedDnsRecordService { reason: "an existing record with this name is not managed by temps".to_string(), }); } - RecordOwnership::OwnedByOther(_, marker) => { + (None, RegistryState::Occupied) => { + return Err(DnsError::RecordConflict { + domain: zone.to_string(), + name: request.name.clone(), + record_type: record_type.to_string(), + reason: format!( + "a TXT record already occupies the ownership registry name '{}' and is not a temps marker", + registry_record_name(&request.name, record_type) + ), + }); + } + (_, RegistryState::Foreign(marker)) => { return Err(DnsError::NotOwnedByInstance { domain: zone.to_string(), name: request.name.clone(), record_type: record_type.to_string(), - owner_instance: marker.instance, + owner_instance: marker.instance.clone(), }); } - }; + } - // Marker first: a crash after this point leaves an orphan TXT (noise), - // never a live unmarked record (a permanent conflict against ourselves). + // Marker first: a crash after this point leaves an orphan TXT (noise, + // reusable by us), never a live unmarked record (a permanent conflict + // against ourselves). + let registry_name = registry_record_name(&request.name, record_type); let registry_request = DnsRecordRequest { - name: registry_record_name(&request.name), + name: registry_name.clone(), content: DnsRecordContent::TXT { content: marker.to_txt_content()?, }, @@ -326,8 +477,7 @@ impl ManagedDnsRecordService { Err(e) => { // Creating the target failed. If nothing existed before, the // fresh marker is pure junk — clean it up best-effort. - if !existed { - let registry_name = registry_record_name(&request.name); + if existing.is_none() { if let Err(cleanup_err) = provider .remove_record(zone, ®istry_name, DnsRecordType::TXT) .await @@ -353,20 +503,29 @@ impl ManagedDnsRecordService { match Self::ownership_of(provider, zone, name, record_type, instance).await? { RecordOwnership::NotFound => { // Record already gone; clean up a stray marker of ours if the - // registry still has one so it doesn't accumulate. - if let Some(marker) = Self::fetch_marker(provider, zone, name).await? { - if marker.is_owned_by(instance) { - provider - .remove_record(zone, ®istry_record_name(name), DnsRecordType::TXT) - .await?; - } + // registry still has one so it doesn't accumulate. Foreign or + // occupied registry names are left untouched. + if let RegistryState::Owned(_) = + Self::registry_state(provider, zone, name, record_type, instance).await? + { + provider + .remove_record( + zone, + ®istry_record_name(name, record_type), + DnsRecordType::TXT, + ) + .await?; } Ok(()) } RecordOwnership::Owned(_, _) => { provider.remove_record(zone, name, record_type).await?; provider - .remove_record(zone, ®istry_record_name(name), DnsRecordType::TXT) + .remove_record( + zone, + ®istry_record_name(name, record_type), + DnsRecordType::TXT, + ) .await?; Ok(()) } @@ -394,22 +553,40 @@ impl ManagedDnsRecordService { instance: &str, scope: OwnershipScope, ) -> Result { - match Self::ownership_of(provider, zone, name, record_type, instance).await? { - RecordOwnership::NotFound => Err(DnsError::RecordNotFound(format!( + let existing = provider.get_record(zone, name, record_type).await?; + if existing.is_none() { + return Err(DnsError::RecordNotFound(format!( "{} record '{}' in zone {} does not exist, so it cannot be imported", record_type, name, zone - ))), - RecordOwnership::Owned(_, marker) => Ok(marker), // already ours — idempotent - RecordOwnership::OwnedByOther(_, marker) => Err(DnsError::NotOwnedByInstance { + ))); + } + + match Self::registry_state(provider, zone, name, record_type, instance).await? { + RegistryState::Owned(marker) => Ok(marker), // already ours — idempotent + RegistryState::Foreign(marker) => Err(DnsError::NotOwnedByInstance { domain: zone.to_string(), name: name.to_string(), record_type: record_type.to_string(), owner_instance: marker.instance, }), - RecordOwnership::Unmanaged(_) => { - let marker = OwnershipMarker::new(instance, scope.project_id, scope.environment_id); + RegistryState::Occupied => Err(DnsError::RecordConflict { + domain: zone.to_string(), + name: name.to_string(), + record_type: record_type.to_string(), + reason: format!( + "a TXT record already occupies the ownership registry name '{}' and is not a temps marker; remove it at the provider before importing", + registry_record_name(name, record_type) + ), + }), + RegistryState::Absent => { + let marker = OwnershipMarker::new( + instance, + record_type, + scope.project_id, + scope.environment_id, + ); let registry_request = DnsRecordRequest { - name: registry_record_name(name), + name: registry_record_name(name, record_type), content: DnsRecordContent::TXT { content: marker.to_txt_content()?, }, @@ -428,6 +605,7 @@ mod tests { use super::*; use crate::providers::{DnsProviderCapabilities, DnsProviderType, DnsZone}; use async_trait::async_trait; + use sea_orm::{DatabaseBackend, MockDatabase, MockExecResult}; use std::collections::HashMap; use std::sync::Mutex; @@ -470,6 +648,14 @@ mod tests { .unwrap() .contains_key(&(name.to_string(), record_type.to_string())) } + + fn record_value(&self, name: &str, record_type: DnsRecordType) -> Option { + self.records + .lock() + .unwrap() + .get(&(name.to_string(), record_type.to_string())) + .map(|r| r.content.to_value_string()) + } } #[async_trait] @@ -563,6 +749,7 @@ mod tests { } const INSTANCE: &str = "test-instance"; + const OTHER_INSTANCE: &str = "other-install"; fn a_request(name: &str, proxied: bool) -> DnsRecordRequest { DnsRecordRequest { @@ -575,10 +762,25 @@ mod tests { } } - fn marker() -> OwnershipMarker { - OwnershipMarker::new(INSTANCE, Some(1), Some(2)) + fn marker_for(record_type: DnsRecordType) -> OwnershipMarker { + OwnershipMarker::new(INSTANCE, record_type, Some(1), Some(2)) } + fn registry_txt( + name: &str, + record_type: DnsRecordType, + marker: &OwnershipMarker, + ) -> (String, DnsRecordContent) { + ( + registry_record_name(name, record_type), + DnsRecordContent::TXT { + content: marker.to_txt_content().unwrap(), + }, + ) + } + + // ==================== guarded_set ==================== + #[tokio::test] async fn set_creates_record_and_ownership_marker() { let provider = MockProvider::new(); @@ -586,7 +788,7 @@ mod tests { &provider, "example.com", a_request("app", false), - &marker(), + &marker_for(DnsRecordType::A), INSTANCE, ) .await @@ -594,7 +796,7 @@ mod tests { assert_eq!(record.name, "app"); assert!(provider.has_record("app", DnsRecordType::A)); - assert!(provider.has_record("_temps-owned.app", DnsRecordType::TXT)); + assert!(provider.has_record("_temps-owned-a.app", DnsRecordType::TXT)); } #[tokio::test] @@ -612,7 +814,7 @@ mod tests { &provider, "example.com", a_request("app", false), - &marker(), + &marker_for(DnsRecordType::A), INSTANCE, ) .await @@ -620,17 +822,95 @@ mod tests { assert!(matches!(err, DnsError::RecordConflict { .. })); // Original record untouched - let existing = provider - .get_record("example.com", "app", DnsRecordType::A) - .await - .unwrap() - .unwrap(); - assert_eq!(existing.content.to_value_string(), "203.0.113.1"); + assert_eq!( + provider.record_value("app", DnsRecordType::A).unwrap(), + "203.0.113.1" + ); + } + + #[tokio::test] + async fn set_refuses_to_clobber_user_txt_at_registry_name() { + // Target record absent, but a NON-marker TXT already lives at the + // registry name. The marker write must refuse, not upsert over it. + let provider = MockProvider::new().with_record( + "_temps-owned-a.app", + DnsRecordContent::TXT { + content: "v=spf1 -all".to_string(), + }, + ); + + let err = ManagedDnsRecordService::guarded_set( + &provider, + "example.com", + a_request("app", false), + &marker_for(DnsRecordType::A), + INSTANCE, + ) + .await + .unwrap_err(); + + assert!(matches!(err, DnsError::RecordConflict { .. })); + // The user's TXT is preserved verbatim and no A record was created. + assert_eq!( + provider + .record_value("_temps-owned-a.app", DnsRecordType::TXT) + .unwrap(), + "v=spf1 -all" + ); + assert!(!provider.has_record("app", DnsRecordType::A)); + } + + #[tokio::test] + async fn set_refuses_foreign_orphan_marker_at_registry_name() { + // Another install crashed between marker and record: its orphan + // marker must not be overwritten, or it gets locked out of the name. + let foreign = OwnershipMarker::new(OTHER_INSTANCE, DnsRecordType::A, None, None); + let (reg_name, reg_content) = registry_txt("app", DnsRecordType::A, &foreign); + let provider = MockProvider::new().with_record(®_name, reg_content); + + let err = ManagedDnsRecordService::guarded_set( + &provider, + "example.com", + a_request("app", false), + &marker_for(DnsRecordType::A), + INSTANCE, + ) + .await + .unwrap_err(); + + match err { + DnsError::NotOwnedByInstance { owner_instance, .. } => { + assert_eq!(owner_instance, OTHER_INSTANCE); + } + other => panic!("expected NotOwnedByInstance, got {:?}", other), + } + assert!(!provider.has_record("app", DnsRecordType::A)); + } + + #[tokio::test] + async fn set_reuses_our_orphan_marker() { + // WE crashed between marker and record last time: our own orphan + // marker must not block the retry. + let (reg_name, reg_content) = + registry_txt("app", DnsRecordType::A, &marker_for(DnsRecordType::A)); + let provider = MockProvider::new().with_record(®_name, reg_content); + + let record = ManagedDnsRecordService::guarded_set( + &provider, + "example.com", + a_request("app", false), + &marker_for(DnsRecordType::A), + INSTANCE, + ) + .await + .unwrap(); + assert_eq!(record.content.to_value_string(), "192.0.2.10"); } #[tokio::test] async fn set_refuses_record_owned_by_other_instance() { - let foreign = OwnershipMarker::new("other-install", None, None); + let foreign = OwnershipMarker::new(OTHER_INSTANCE, DnsRecordType::A, None, None); + let (reg_name, reg_content) = registry_txt("app", DnsRecordType::A, &foreign); let provider = MockProvider::new() .with_record( "app", @@ -638,18 +918,13 @@ mod tests { address: "203.0.113.1".to_string(), }, ) - .with_record( - "_temps-owned.app", - DnsRecordContent::TXT { - content: foreign.to_txt_content().unwrap(), - }, - ); + .with_record(®_name, reg_content); let err = ManagedDnsRecordService::guarded_set( &provider, "example.com", a_request("app", false), - &marker(), + &marker_for(DnsRecordType::A), INSTANCE, ) .await @@ -657,7 +932,7 @@ mod tests { match err { DnsError::NotOwnedByInstance { owner_instance, .. } => { - assert_eq!(owner_instance, "other-install"); + assert_eq!(owner_instance, OTHER_INSTANCE); } other => panic!("expected NotOwnedByInstance, got {:?}", other), } @@ -665,6 +940,8 @@ mod tests { #[tokio::test] async fn set_updates_record_owned_by_this_instance() { + let (reg_name, reg_content) = + registry_txt("app", DnsRecordType::A, &marker_for(DnsRecordType::A)); let provider = MockProvider::new() .with_record( "app", @@ -672,18 +949,13 @@ mod tests { address: "203.0.113.1".to_string(), }, ) - .with_record( - "_temps-owned.app", - DnsRecordContent::TXT { - content: marker().to_txt_content().unwrap(), - }, - ); + .with_record(®_name, reg_content); let record = ManagedDnsRecordService::guarded_set( &provider, "example.com", a_request("app", false), - &marker(), + &marker_for(DnsRecordType::A), INSTANCE, ) .await @@ -692,6 +964,74 @@ mod tests { assert_eq!(record.content.to_value_string(), "192.0.2.10"); } + #[tokio::test] + async fn ownership_is_type_scoped_a_marker_does_not_cover_aaaa() { + // Temps owns `app` A. A user manually maintains `app` AAAA. + // Writing or removing the AAAA must conflict, not ride on the A marker. + let (reg_name, reg_content) = + registry_txt("app", DnsRecordType::A, &marker_for(DnsRecordType::A)); + let provider = MockProvider::new() + .with_record( + "app", + DnsRecordContent::A { + address: "192.0.2.10".to_string(), + }, + ) + .with_record(®_name, reg_content) + .with_record( + "app", + DnsRecordContent::AAAA { + address: "2001:db8::1".to_string(), + }, + ); + + // Set AAAA → conflict (user's record, no AAAA-scoped marker) + let err = ManagedDnsRecordService::guarded_set( + &provider, + "example.com", + DnsRecordRequest { + name: "app".to_string(), + content: DnsRecordContent::AAAA { + address: "2001:db8::2".to_string(), + }, + ttl: None, + proxied: false, + }, + &marker_for(DnsRecordType::AAAA), + INSTANCE, + ) + .await + .unwrap_err(); + assert!(matches!(err, DnsError::RecordConflict { .. })); + + // Remove AAAA → conflict + let err = ManagedDnsRecordService::guarded_remove( + &provider, + "example.com", + "app", + DnsRecordType::AAAA, + INSTANCE, + ) + .await + .unwrap_err(); + assert!(matches!(err, DnsError::RecordConflict { .. })); + + // The user's AAAA is untouched; our A is still updatable. + assert_eq!( + provider.record_value("app", DnsRecordType::AAAA).unwrap(), + "2001:db8::1" + ); + assert!(ManagedDnsRecordService::guarded_set( + &provider, + "example.com", + a_request("app", false), + &marker_for(DnsRecordType::A), + INSTANCE, + ) + .await + .is_ok()); + } + #[tokio::test] async fn failed_create_cleans_up_fresh_marker() { let mut provider = MockProvider::new(); @@ -701,7 +1041,7 @@ mod tests { &provider, "example.com", a_request("app", false), - &marker(), + &marker_for(DnsRecordType::A), INSTANCE, ) .await @@ -709,9 +1049,11 @@ mod tests { assert!(matches!(err, DnsError::ApiError(_))); // No orphan marker left behind for a record that was never created. - assert!(!provider.has_record("_temps-owned.app", DnsRecordType::TXT)); + assert!(!provider.has_record("_temps-owned-a.app", DnsRecordType::TXT)); } + // ==================== guarded_remove ==================== + #[tokio::test] async fn remove_refuses_unmanaged_record() { let provider = MockProvider::new().with_record( @@ -737,6 +1079,8 @@ mod tests { #[tokio::test] async fn remove_deletes_owned_record_and_marker() { + let (reg_name, reg_content) = + registry_txt("app", DnsRecordType::A, &marker_for(DnsRecordType::A)); let provider = MockProvider::new() .with_record( "app", @@ -744,12 +1088,7 @@ mod tests { address: "192.0.2.10".to_string(), }, ) - .with_record( - "_temps-owned.app", - DnsRecordContent::TXT { - content: marker().to_txt_content().unwrap(), - }, - ); + .with_record(®_name, reg_content); ManagedDnsRecordService::guarded_remove( &provider, @@ -762,17 +1101,14 @@ mod tests { .unwrap(); assert!(!provider.has_record("app", DnsRecordType::A)); - assert!(!provider.has_record("_temps-owned.app", DnsRecordType::TXT)); + assert!(!provider.has_record("_temps-owned-a.app", DnsRecordType::TXT)); } #[tokio::test] async fn remove_of_missing_record_is_ok_and_cleans_stray_marker() { - let provider = MockProvider::new().with_record( - "_temps-owned.app", - DnsRecordContent::TXT { - content: marker().to_txt_content().unwrap(), - }, - ); + let (reg_name, reg_content) = + registry_txt("app", DnsRecordType::A, &marker_for(DnsRecordType::A)); + let provider = MockProvider::new().with_record(®_name, reg_content); ManagedDnsRecordService::guarded_remove( &provider, @@ -784,9 +1120,31 @@ mod tests { .await .unwrap(); - assert!(!provider.has_record("_temps-owned.app", DnsRecordType::TXT)); + assert!(!provider.has_record("_temps-owned-a.app", DnsRecordType::TXT)); } + #[tokio::test] + async fn remove_of_missing_record_leaves_foreign_marker_alone() { + let foreign = OwnershipMarker::new(OTHER_INSTANCE, DnsRecordType::A, None, None); + let (reg_name, reg_content) = registry_txt("app", DnsRecordType::A, &foreign); + let provider = MockProvider::new().with_record(®_name, reg_content); + + ManagedDnsRecordService::guarded_remove( + &provider, + "example.com", + "app", + DnsRecordType::A, + INSTANCE, + ) + .await + .unwrap(); + + // Their orphan marker survives. + assert!(provider.has_record(®_name, DnsRecordType::TXT)); + } + + // ==================== guarded_import ==================== + #[tokio::test] async fn import_stamps_marker_on_unmanaged_record() { let provider = MockProvider::new().with_record( @@ -811,14 +1169,15 @@ mod tests { .unwrap(); assert_eq!(imported.project_id, Some(9)); - assert!(provider.has_record("_temps-owned.app", DnsRecordType::TXT)); + assert_eq!(imported.record_type.as_deref(), Some("A")); + assert!(provider.has_record("_temps-owned-a.app", DnsRecordType::TXT)); // After import, set is allowed. let record = ManagedDnsRecordService::guarded_set( &provider, "example.com", a_request("app", false), - &marker(), + &marker_for(DnsRecordType::A), INSTANCE, ) .await @@ -827,7 +1186,35 @@ mod tests { } #[tokio::test] - async fn import_refuses_missing_and_foreign_records() { + async fn import_is_idempotent_when_already_owned() { + let (reg_name, reg_content) = + registry_txt("app", DnsRecordType::A, &marker_for(DnsRecordType::A)); + let provider = MockProvider::new() + .with_record( + "app", + DnsRecordContent::A { + address: "192.0.2.10".to_string(), + }, + ) + .with_record(®_name, reg_content); + + let marker = ManagedDnsRecordService::guarded_import( + &provider, + "example.com", + "app", + DnsRecordType::A, + INSTANCE, + OwnershipScope::default(), + ) + .await + .unwrap(); + // Existing marker returned as-is, including its original scope. + assert_eq!(marker.project_id, Some(1)); + } + + #[tokio::test] + async fn import_refuses_missing_foreign_and_occupied() { + // Missing target record let provider = MockProvider::new(); let err = ManagedDnsRecordService::guarded_import( &provider, @@ -841,7 +1228,9 @@ mod tests { .unwrap_err(); assert!(matches!(err, DnsError::RecordNotFound(_))); - let foreign = OwnershipMarker::new("other-install", None, None); + // Foreign marker at registry name + let foreign = OwnershipMarker::new(OTHER_INSTANCE, DnsRecordType::A, None, None); + let (reg_name, reg_content) = registry_txt("app", DnsRecordType::A, &foreign); let provider = MockProvider::new() .with_record( "app", @@ -849,10 +1238,32 @@ mod tests { address: "203.0.113.1".to_string(), }, ) + .with_record(®_name, reg_content); + let err = ManagedDnsRecordService::guarded_import( + &provider, + "example.com", + "app", + DnsRecordType::A, + INSTANCE, + OwnershipScope::default(), + ) + .await + .unwrap_err(); + assert!(matches!(err, DnsError::NotOwnedByInstance { .. })); + + // Non-marker TXT occupying the registry name: import must refuse + // rather than clobber the user's TXT. + let provider = MockProvider::new() .with_record( - "_temps-owned.app", + "app", + DnsRecordContent::A { + address: "203.0.113.1".to_string(), + }, + ) + .with_record( + "_temps-owned-a.app", DnsRecordContent::TXT { - content: foreign.to_txt_content().unwrap(), + content: "user-data".to_string(), }, ); let err = ManagedDnsRecordService::guarded_import( @@ -865,12 +1276,20 @@ mod tests { ) .await .unwrap_err(); - assert!(matches!(err, DnsError::NotOwnedByInstance { .. })); + assert!(matches!(err, DnsError::RecordConflict { .. })); + assert_eq!( + provider + .record_value("_temps-owned-a.app", DnsRecordType::TXT) + .unwrap(), + "user-data" + ); } + // ==================== ownership_of ==================== + #[tokio::test] async fn user_txt_record_at_registry_name_does_not_grant_ownership() { - // A user TXT that happens to live at `_temps-owned.app` but isn't a + // A user TXT that happens to live at the registry name but isn't a // valid marker must read as Unmanaged, not Owned. let provider = MockProvider::new() .with_record( @@ -880,7 +1299,7 @@ mod tests { }, ) .with_record( - "_temps-owned.app", + "_temps-owned-a.app", DnsRecordContent::TXT { content: "v=spf1 -all".to_string(), }, @@ -897,4 +1316,102 @@ mod tests { .unwrap(); assert!(matches!(ownership, RecordOwnership::Unmanaged(_))); } + + // ==================== KeyedLocks ==================== + + #[tokio::test] + async fn keyed_locks_serialize_same_key_and_clean_up() { + let locks = Arc::new(KeyedLocks::new()); + + // Same key returns the same lock; different keys don't contend. + let a1 = locks.get("example.com", "app"); + let a2 = locks.get("example.com", "app"); + let b = locks.get("example.com", "other"); + assert!(Arc::ptr_eq(&a1, &a2)); + assert!(!Arc::ptr_eq(&a1, &b)); + + // Serialization: hold a1, second locker must not acquire until drop. + let guard = a1.lock().await; + assert!(a2.try_lock().is_err()); + drop(guard); + assert!(a2.try_lock().is_ok()); + + // Cleanup: after all handles released, the map entry is gone. + locks.release("example.com", "other", b); + locks.release("example.com", "app", a1); + assert_eq!( + locks.inner.lock().unwrap().len(), + 1, + "app entry still held via a2" + ); + locks.release("example.com", "app", a2); + assert!(locks.inner.lock().unwrap().is_empty()); + } + + // ==================== instance_id (MockDatabase) ==================== + + fn identity_row(id: &str) -> dns_instance_identity::Model { + dns_instance_identity::Model { + id: 1, + instance_id: id.to_string(), + created_at: chrono::Utc::now(), + } + } + + fn service_with_db(db: sea_orm::DatabaseConnection) -> ManagedDnsRecordService { + let db = Arc::new(db); + let encryption = Arc::new( + temps_core::EncryptionService::new("0123456789abcdef0123456789abcdef") + .expect("32-byte test key"), + ); + let provider_service = Arc::new(DnsProviderService::new(db.clone(), encryption)); + ManagedDnsRecordService::new(db, provider_service) + } + + #[tokio::test] + async fn instance_id_returns_existing_row() { + let db = MockDatabase::new(DatabaseBackend::Postgres) + .append_query_results(vec![vec![identity_row("existing-id")]]) + .into_connection(); + let service = service_with_db(db); + + assert_eq!(service.instance_id().await.unwrap(), "existing-id"); + // Cached: a second call must not hit the DB again (mock has no more + // results queued and would error). + assert_eq!(service.instance_id().await.unwrap(), "existing-id"); + } + + #[tokio::test] + async fn instance_id_creates_row_on_first_use() { + let db = MockDatabase::new(DatabaseBackend::Postgres) + // find → empty + .append_query_results(vec![Vec::::new()]) + .append_exec_results(vec![MockExecResult { + last_insert_id: 1, + rows_affected: 1, + }]) + // insert RETURNING → the created row + .append_query_results(vec![vec![identity_row("fresh-id")]]) + .into_connection(); + let service = service_with_db(db); + + assert_eq!(service.instance_id().await.unwrap(), "fresh-id"); + } + + #[tokio::test] + async fn instance_id_recovers_when_losing_insert_race() { + let db = MockDatabase::new(DatabaseBackend::Postgres) + // find → empty + .append_query_results(vec![Vec::::new()]) + // insert → unique violation + .append_exec_errors(vec![sea_orm::DbErr::Custom( + "duplicate key value violates unique constraint".to_string(), + )]) + // re-find → winner's row + .append_query_results(vec![vec![identity_row("winner-id")]]) + .into_connection(); + let service = service_with_db(db); + + assert_eq!(service.instance_id().await.unwrap(), "winner-id"); + } } diff --git a/docs/adr/031-managed-dns-records-and-cloudflare-proxied-mode.md b/docs/adr/031-managed-dns-records-and-cloudflare-proxied-mode.md index cedc8fc0b..8a0cfc17f 100644 --- a/docs/adr/031-managed-dns-records-and-cloudflare-proxied-mode.md +++ b/docs/adr/031-managed-dns-records-and-cloudflare-proxied-mode.md @@ -39,15 +39,22 @@ Add **managed DNS record automation** as an opt-in, per-domain feature on top of ### 1. Ownership marking (the core safety invariant) -Every record temps creates carries a machine-readable ownership marker: a companion TXT record `_temps-owned.` holding typed JSON, e.g. `{"managed_by":"temps","instance":"","project_id":N,"environment_id":N,"v":1}` (the external-dns registry pattern). This works uniformly across all providers. +Every record temps creates carries a machine-readable ownership marker: a companion TXT record holding typed JSON, e.g. `{"managed_by":"temps","instance":"","record_type":"A","project_id":N,"environment_id":N,"v":1}` (the external-dns registry pattern). This works uniformly across all providers. + +The registry name is **type-scoped and injective** (both properties exist because their absence lets temps clobber a record it never created — found in security review): + +- Type-scoped: `_temps-owned-a.`, `_temps-owned-aaaa.`, `_temps-owned-cname.`, … — owning `app` A never grants ownership of a user's `app` AAAA. The marker JSON additionally carries `record_type` and both must match. +- Injective escaping of the record name: `_` → `__` before `*` → `_w`, so `*.staging` and a literal `wildcard.staging` (or `_w.staging`) can never share a registry name. *Implementation note (v1):* Cloudflare's per-record `comment` field was originally preferred there for dashboard visibility, but the `cloudflare` crate's DNS params don't expose it, so v1 uses the TXT registry on Cloudflare too. Comment stamping can be added later as a purely additive enhancement (the TXT registry stays authoritative). Rules, enforced in the service layer, not left to callers: -- **Create:** if a record with the target name/type already exists and has no parseable temps marker → refuse, surface a conflict. -- **Update/Delete:** only permitted when the existing record's marker parses and matches this temps instance. Unparsable or foreign marker → refuse. +- **Create:** refuse if a record with the target name/type already exists without a covering temps marker — AND refuse if the registry name itself is occupied by a TXT that is not our marker (the marker write must never upsert over foreign content, including another install's orphan marker). +- **Update/Delete:** only permitted when the marker parses, matches this temps instance, and covers the record type. Unparsable, foreign, or type-mismatched marker → refuse. - **Conflict resolution UI:** on conflict, offer *import* (adopt the record: stamp it with a marker after explicit user confirmation) or *skip*. Default is always **never overwrite**. No bulk "overwrite all." +- **Marker hygiene:** the `instance` field is validated on parse (`[A-Za-z0-9-]`, ≤ 64 chars) so attacker-written TXT content can't inject into temps logs/UI. +- **Concurrency:** all guarded operations on the same (zone, name) are serialized in-process through a keyed async lock. The remaining remote TOCTOU window (DNS APIs have no compare-and-swap) is an accepted residual risk. ### 2. Provider-agnostic surface, one provider per zone From 64734592673f1bf5205302d946d0ca8298ae8980 Mon Sep 17 00:00:00 2001 From: David Viejo Date: Tue, 14 Jul 2026 17:05:46 +0200 Subject: [PATCH 3/3] fix(dns): map DomainNotManaged to 404 and poison-proof keyed locks Review follow-ups on the managed-records API: - DomainNotManaged fell through the From catch-all to a 500, contradicting the 404 all four /dns-records endpoints document; a domain temps doesn't manage is client input, not a server fault. Map it explicitly to 404 with an actionable detail message - KeyedLocks now recovers a poisoned map mutex instead of expect()ing; a panic there would otherwise turn every later DNS write into a panic until restart --- crates/temps-dns/src/handlers/mod.rs | 6 ++++++ crates/temps-dns/src/services/managed_records.rs | 7 +++++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/crates/temps-dns/src/handlers/mod.rs b/crates/temps-dns/src/handlers/mod.rs index cdb8f34d2..f8b63ff3f 100644 --- a/crates/temps-dns/src/handlers/mod.rs +++ b/crates/temps-dns/src/handlers/mod.rs @@ -299,6 +299,12 @@ impl From for Problem { DnsError::ApiError(msg) => problemdetails::new(StatusCode::BAD_GATEWAY) .with_title("API Error") .with_detail(msg), + DnsError::DomainNotManaged(domain) => problemdetails::new(StatusCode::NOT_FOUND) + .with_title("Domain Not Managed") + .with_detail(format!( + "Domain {} is not managed by any DNS provider; connect a provider and add the domain under its managed domains first", + domain + )), DnsError::RecordConflict { .. } => problemdetails::new(StatusCode::CONFLICT) .with_title("DNS Record Conflict") .with_detail(error.to_string()), diff --git a/crates/temps-dns/src/services/managed_records.rs b/crates/temps-dns/src/services/managed_records.rs index fdb6ebab3..23b54571b 100644 --- a/crates/temps-dns/src/services/managed_records.rs +++ b/crates/temps-dns/src/services/managed_records.rs @@ -120,7 +120,10 @@ impl KeyedLocks { } fn get(&self, zone: &str, name: &str) -> Arc> { - let mut map = self.inner.lock().expect("keyed lock map poisoned"); + // Poison-proof: the critical section is a plain HashMap op that can't + // panic, but if it somehow did, recovering the map beats turning every + // future DNS write into a panic until restart. + let mut map = self.inner.lock().unwrap_or_else(|e| e.into_inner()); map.entry((zone.to_string(), name.to_string())) .or_insert_with(|| Arc::new(tokio::sync::Mutex::new(()))) .clone() @@ -128,7 +131,7 @@ impl KeyedLocks { /// Drop the map entry if no one else holds the Arc (map + caller = 2). fn release(&self, zone: &str, name: &str, handle: Arc>) { - let mut map = self.inner.lock().expect("keyed lock map poisoned"); + let mut map = self.inner.lock().unwrap_or_else(|e| e.into_inner()); if Arc::strong_count(&handle) == 2 { map.remove(&(zone.to_string(), name.to_string())); }