Bind ACME revocation to quarantine

eldryoth · eldryoth · commit 08d4226eee4b · 2026-07-12T15:37:32.000+02:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -30,6 +30,9 @@ behavior when the change improves security or project direction.
 - Journal revocation quarantine phases durably so pre-remote crashes restore the
   complete pair, ambiguous remote outcomes stay fail-closed, and confirmed
   quarantine survives crashes while permitting replacement issuance.
+- Bind revocation to the exact certificate moved into quarantine while holding
+  the certificate mutation lock, and make the advisory ARI cache recover from
+  poisoned locks and timestamp overflow without aborting the process.
 - Validate online ACME directories structurally and require exact advertised ToS
   agreement, with an explicit private-directory override only for omitted terms.
 - Parse every advertised ACME endpoint as a bounded HTTPS URI with a real
diff --git a/crates/fluxheim-acme/src/acme_ari.rs b/crates/fluxheim-acme/src/acme_ari.rs
@@ -18,9 +18,9 @@ struct AriCacheEntry {
     refresh_after: SystemTime,
 }
 
-static CACHE: std::sync::OnceLock<
-    std::sync::Mutex<std::collections::HashMap<AriCacheKey, AriCacheEntry>>,
-> = std::sync::OnceLock::new();
+type AriCache = std::collections::HashMap<AriCacheKey, AriCacheEntry>;
+
+static CACHE: std::sync::OnceLock<std::sync::Mutex<AriCache>> = std::sync::OnceLock::new();
 
 pub(super) async fn execute_due_queue(
     config: &Config,
@@ -180,11 +180,7 @@ fn safe_ari_schedule(
 }
 
 fn cached_decision(key: &AriCacheKey, now: SystemTime, fallback: bool) -> Option<bool> {
-    let cache = CACHE.get_or_init(Default::default);
-    let mut cache = cache.lock().unwrap_or_else(|_| {
-        log::error!(target: "fluxheim::security", "ACME ARI cache lock poisoned");
-        std::process::abort();
-    });
+    let mut cache = lock_cache();
     cache.retain(|_, entry| entry.refresh_after > now);
     cache.get(key).map(|entry| {
         entry
@@ -194,11 +190,8 @@ fn cached_decision(key: &AriCacheKey, now: SystemTime, fallback: bool) -> Option
 }
 
 fn store_cache(key: &AriCacheKey, scheduled: Option<u64>, now: SystemTime, retry_after: Duration) {
-    let cache = CACHE.get_or_init(Default::default);
-    let mut cache = cache.lock().unwrap_or_else(|_| {
-        log::error!(target: "fluxheim::security", "ACME ARI cache lock poisoned");
-        std::process::abort();
-    });
+    let refresh_after = cache_refresh_after(now, retry_after);
+    let mut cache = lock_cache();
     cache.retain(|_, entry| entry.refresh_after > now);
     if cache.len() >= MAX_CACHE_ENTRIES && !cache.contains_key(key) {
         cache.clear();
@@ -207,12 +200,33 @@ fn store_cache(key: &AriCacheKey, scheduled: Option<u64>, now: SystemTime, retry
         key.clone(),
         AriCacheEntry {
             scheduled_unix_secs: scheduled,
-            refresh_after: now
-                + retry_after.clamp(Duration::from_secs(60), Duration::from_secs(86_400)),
+            refresh_after,
         },
     );
 }
 
+fn cache_refresh_after(now: SystemTime, retry_after: Duration) -> SystemTime {
+    now.checked_add(retry_after.clamp(Duration::from_secs(60), Duration::from_secs(86_400)))
+        .unwrap_or(now)
+}
+
+fn lock_cache() -> std::sync::MutexGuard<'static, AriCache> {
+    let cache = CACHE.get_or_init(Default::default);
+    match cache.lock() {
+        Ok(cache) => cache,
+        Err(poisoned) => {
+            log::error!(
+                target: "fluxheim::security",
+                "ACME ARI advisory cache lock poisoned; discarding cached decisions"
+            );
+            let mut recovered = poisoned.into_inner();
+            recovered.clear();
+            cache.clear_poison();
+            recovered
+        }
+    }
+}
+
 fn unix_secs(time: SystemTime) -> u64 {
     time.duration_since(UNIX_EPOCH)
         .map(|duration| duration.as_secs())
@@ -283,6 +297,39 @@ mod tests {
         assert_eq!(cached_decision(&second, now, false), Some(true));
     }
 
+    #[test]
+    fn cache_refresh_deadline_overflow_expires_immediately() {
+        let near_limit = UNIX_EPOCH
+            .checked_add(Duration::from_secs(i64::MAX as u64))
+            .unwrap();
+        assert_eq!(
+            cache_refresh_after(near_limit, Duration::from_secs(86_400)),
+            near_limit
+        );
+    }
+
+    #[test]
+    fn poisoned_advisory_cache_is_cleared_and_recovers() {
+        let _test_lock = CACHE_TEST_LOCK.lock().unwrap();
+        let cache = CACHE.get_or_init(Default::default);
+        let poisoned = std::panic::catch_unwind(|| {
+            let _cache = cache.lock().unwrap();
+            panic!("poison ARI cache for recovery test");
+        });
+        assert!(poisoned.is_err());
+        assert!(cache.is_poisoned());
+
+        let now = UNIX_EPOCH + Duration::from_secs(25_000);
+        let key = AriCacheKey {
+            issuer_directory: "https://issuer.example/directory".to_owned(),
+            certificate_identifier: "recovered-identifier".to_owned(),
+        };
+        store_cache(&key, Some(25_100), now, Duration::from_secs(300));
+
+        assert!(!cache.is_poisoned());
+        assert_eq!(cached_decision(&key, now, true), Some(false));
+    }
+
     #[test]
     fn ari_window_must_end_within_certificate_validity() {
         let not_after = UNIX_EPOCH + Duration::from_secs(30_000);
diff --git a/crates/fluxheim-acme/src/acme_certificate_revocation.rs b/crates/fluxheim-acme/src/acme_certificate_revocation.rs
@@ -4,6 +4,8 @@ use super::fs_ops::{CertificateDirectoryFd, rename_certificate_file, sync_direct
 use super::fs_ops::{
     ManagedCertificateOwner, certificate_directory, open_safe_certificate_directory, write_new_file,
 };
+#[cfg(feature = "acme-client")]
+use super::revocation_fs::read_bounded_regular_file;
 use super::revocation_fs::{
     certificate_file_exists_regular, ensure_certificate_slot_absent, ensure_existing_regular_file,
     revocation_file_names_valid,
@@ -105,6 +107,17 @@ pub(crate) fn begin_managed_certificate_quarantine(
 
 #[cfg(feature = "acme-client")]
 impl ManagedCertificateQuarantine {
+    pub(crate) fn read_quarantined_certificate(
+        &self,
+    ) -> Result<Vec<u8>, AcmeCertificateInstallError> {
+        read_bounded_regular_file(
+            &self.directory,
+            &self.directory.join(&self.journal.certificate_name),
+            self.directory_fd(),
+            MAX_CERTIFICATE_CHAIN_BYTES,
+        )
+    }
+
     pub(crate) fn mark_remote_pending(&mut self) -> Result<(), AcmeCertificateInstallError> {
         self.update_phase(RevocationPhase::RemotePending)
     }
diff --git a/crates/fluxheim-acme/src/acme_certificate_revocation_fs.rs b/crates/fluxheim-acme/src/acme_certificate_revocation_fs.rs
@@ -77,3 +77,86 @@ pub(crate) fn revocation_file_names_valid(
         && certificate_name == format!(".revoked-{transaction}-fullchain.pem")
         && private_key_name == format!(".revoked-{transaction}-privkey.pem")
 }
+
+#[cfg(feature = "acme-client")]
+pub(super) fn read_bounded_regular_file(
+    directory: &Path,
+    path: &Path,
+    directory_fd: Option<&CertificateDirectoryFd>,
+    max_bytes: usize,
+) -> Result<Vec<u8>, AcmeCertificateInstallError> {
+    #[cfg(unix)]
+    let mut file = {
+        let directory_fd = directory_fd.ok_or_else(|| AcmeCertificateInstallError::UnsafePath {
+            path: directory.to_path_buf(),
+            message: "managed certificate directory fd is unavailable".to_owned(),
+        })?;
+        let name = certificate_file_name_in_directory(directory, path)?;
+        let fd = rustix::fs::openat(
+            directory_fd,
+            name,
+            rustix::fs::OFlags::RDONLY | rustix::fs::OFlags::NOFOLLOW | rustix::fs::OFlags::CLOEXEC,
+            rustix::fs::Mode::empty(),
+        )
+        .map_err(|error| AcmeCertificateInstallError::Io {
+            path: path.to_path_buf(),
+            error: error.into(),
+        })?;
+        fs::File::from(fd)
+    };
+    #[cfg(not(unix))]
+    let mut file = {
+        ensure_existing_regular_file(directory, path, directory_fd)?;
+        fs::OpenOptions::new()
+            .read(true)
+            .open(path)
+            .map_err(|error| AcmeCertificateInstallError::Io {
+                path: path.to_path_buf(),
+                error,
+            })?
+    };
+    let metadata = file
+        .metadata()
+        .map_err(|error| AcmeCertificateInstallError::Io {
+            path: path.to_path_buf(),
+            error,
+        })?;
+    if !metadata.is_file() {
+        return Err(AcmeCertificateInstallError::UnsafePath {
+            path: path.to_path_buf(),
+            message: "managed certificate is not a regular file".to_owned(),
+        });
+    }
+    if metadata.len() > max_bytes as u64 {
+        return Err(AcmeCertificateInstallError::UnsafePath {
+            path: path.to_path_buf(),
+            message: format!("managed certificate exceeds {max_bytes} bytes"),
+        });
+    }
+    let admitted =
+        usize::try_from(metadata.len()).map_err(|_| AcmeCertificateInstallError::UnsafePath {
+            path: path.to_path_buf(),
+            message: format!("managed certificate exceeds {max_bytes} bytes"),
+        })?;
+    let mut bytes = vec![0_u8; admitted];
+    file.read_exact(&mut bytes)
+        .map_err(|error| AcmeCertificateInstallError::Io {
+            path: path.to_path_buf(),
+            error,
+        })?;
+    let mut growth_probe = [0_u8; 1];
+    if file
+        .read(&mut growth_probe)
+        .map_err(|error| AcmeCertificateInstallError::Io {
+            path: path.to_path_buf(),
+            error,
+        })?
+        != 0
+    {
+        return Err(AcmeCertificateInstallError::UnsafePath {
+            path: path.to_path_buf(),
+            message: format!("managed certificate exceeds {max_bytes} bytes"),
+        });
+    }
+    Ok(bytes)
+}
diff --git a/crates/fluxheim-acme/src/acme_instant_account.rs b/crates/fluxheim-acme/src/acme_instant_account.rs
@@ -141,36 +141,34 @@ pub async fn revoke_instant_acme_certificate(
                 format!("unknown ACME vhost target {vhost_name:?}"),
             )
         })?;
-    let certificate_pem = read_bounded_certificate_file(&target.certificate.cert_path)
-        .map_err(|error| {
+    let account = load_or_create_instant_acme_account(config, &target.issuer).await?;
+    let mut quarantine =
+        begin_managed_certificate_quarantine(&target.certificate).map_err(|error| {
             account_error(
                 &target.issuer,
-                format!("failed to read managed certificate: {error}"),
+                format!("failed to quarantine certificate before revocation: {error}"),
             )
-        })?
-        .ok_or_else(|| account_error(&target.issuer, "managed certificate is missing"))?;
+        })?;
+    let certificate_pem = quarantine.read_quarantined_certificate().map_err(|error| {
+        account_error(
+            &target.issuer,
+            format!("failed to read quarantined certificate: {error}"),
+        )
+    })?;
     use rustls::pki_types::pem::PemObject as _;
     let leaf = rustls::pki_types::CertificateDer::pem_slice_iter(&certificate_pem)
         .next()
         .transpose()
         .map_err(|error| {
             account_error(
                 &target.issuer,
-                format!("failed to parse managed certificate: {error}"),
+                format!("failed to parse quarantined certificate: {error}"),
             )
         })?
         .ok_or_else(|| {
             account_error(
                 &target.issuer,
-                "managed certificate has no leaf certificate",
-            )
-        })?;
-    let account = load_or_create_instant_acme_account(config, &target.issuer).await?;
-    let mut quarantine =
-        begin_managed_certificate_quarantine(&target.certificate).map_err(|error| {
-            account_error(
-                &target.issuer,
-                format!("failed to quarantine certificate before revocation: {error}"),
+                "quarantined certificate has no leaf certificate",
             )
         })?;
     quarantine.mark_remote_pending().map_err(|error| {
diff --git a/crates/fluxheim-acme/src/acme_tests_lifecycle.rs b/crates/fluxheim-acme/src/acme_tests_lifecycle.rs
@@ -92,6 +92,59 @@ fn dropped_revocation_quarantine_restores_the_complete_pair() {
     );
 }
 
+#[cfg(feature = "acme-client")]
+#[test]
+fn revocation_reads_quarantined_identity_while_renewal_waits() {
+    let storage = fluxheim_common::test_support::unique_temp_path("acme-revoke-renew-race");
+    let (certificate_a, key_a) = issued_material_for(&["example.test"]);
+    let paths = install_managed_certificate(
+        &storage,
+        "example",
+        certificate_a.as_bytes(),
+        key_a.as_bytes(),
+        &["example.test".to_owned()],
+    )
+    .unwrap();
+    let quarantine = crate::begin_managed_certificate_quarantine(&paths).unwrap();
+
+    assert_eq!(
+        quarantine.read_quarantined_certificate().unwrap(),
+        certificate_a.as_bytes()
+    );
+
+    let (certificate_b, key_b) = issued_material_for(&["example.test"]);
+    let (attempting_tx, attempting_rx) = std::sync::mpsc::channel();
+    let (completed_tx, completed_rx) = std::sync::mpsc::channel();
+    let renewal_storage = storage.clone();
+    let renewal = std::thread::spawn(move || {
+        attempting_tx.send(()).unwrap();
+        completed_tx
+            .send(install_managed_certificate(
+                &renewal_storage,
+                "example",
+                certificate_b.as_bytes(),
+                key_b.as_bytes(),
+                &["example.test".to_owned()],
+            ))
+            .unwrap();
+    });
+
+    attempting_rx
+        .recv_timeout(std::time::Duration::from_secs(2))
+        .unwrap();
+    assert!(
+        completed_rx
+            .recv_timeout(std::time::Duration::from_millis(100))
+            .is_err()
+    );
+    quarantine.rollback().unwrap();
+    completed_rx
+        .recv_timeout(std::time::Duration::from_secs(2))
+        .unwrap()
+        .unwrap();
+    renewal.join().unwrap();
+}
+
 fn installed_revocation_test_pair(label: &str) -> (std::path::PathBuf, AcmeCertificatePaths) {
     let storage = fluxheim_common::test_support::unique_temp_path(label);
     let paths = install_managed_certificate(
diff --git a/docs/certificate-renewal.md b/docs/certificate-renewal.md
@@ -281,7 +281,9 @@ operators can see this capability limit before an incident.
 
 Revocation remains available for managed targets even when automatic renewal is
 disabled. Fluxheim first quarantines the certificate and key atomically under
-the same mutation lock used by TLS readers. A durable fixed-name journal records
+the same mutation lock used by TLS readers, then reads and submits the exact
+quarantined certificate while retaining that lock so concurrent renewal cannot
+substitute another identity. A durable fixed-name journal records
 prepared, pair-quarantined, remote-pending, and remote-confirmed phases with
 validated transaction-derived filenames. A crash before remote contact restores
 the complete pair; an ambiguous remote-pending outcome remains quarantined for
@@ -682,6 +684,9 @@ ARI planning uses four concurrent lookups, a 10-second deadline per target, and
 a 30-second planning budget, executes due renewals progressively, and caches
 successful or unsupported responses until a bounded `Retry-After` deadline.
 Cache entries are namespaced by issuer directory plus certificate identifier.
+Refresh deadlines use checked arithmetic, and a poisoned advisory-cache lock
+causes cached decisions to be discarded and rebuilt instead of terminating the
+process.
 ARI can never postpone renewal inside the final seven days of certificate
 validity, and windows extending beyond `notAfter` are rejected. The cache is
 process-local: external timer invocations retain the concurrency and time
