From 69125b325d7c303c37da3402f56f50d56db9a720 Mon Sep 17 00:00:00 2001 From: John Coffey Date: Thu, 5 Nov 2026 08:57:42 -0700 Subject: [PATCH] ACME: order a certificate when a domain's names change Editing Additional Hostnames on a domain already on ACME saved fine and ordered nothing, so the new names waited for the next renewal, about 60 days on a 90-day certificate. Saving now queues an order when the names, the provider or the domain name change. A certificate that already has those exact names just waits for its renewal date, so reordering the list orders nothing. Each renewal queues the next, so a second task would be a second chain of renewals for good, and on a cluster both could order at once. A new renewal now replaces the domain's queued one. CAA and the DNS-PERSIST-01 record name the provider, and the persist record says whether wildcards are allowed. When a save makes them out of date they're published before the order, or the CA refuses it. A DNS task that renews afterwards may now have an empty record list, which publishes just the persist record. Fixes #290 --- crates/jmap/src/registry/mapping/domain.rs | 169 ++++++++++++++++- crates/jmap/src/registry/set.rs | 24 ++- crates/services/src/task_manager/dns.rs | 4 +- .../services/src/task_manager/domain_check.rs | 59 +++++- tests/src/automation/acme.rs | 176 ++++++++++++++++++ 5 files changed, 410 insertions(+), 22 deletions(-) diff --git a/crates/jmap/src/registry/mapping/domain.rs b/crates/jmap/src/registry/mapping/domain.rs index 1d703881..471a46c1 100644 --- a/crates/jmap/src/registry/mapping/domain.rs +++ b/crates/jmap/src/registry/mapping/domain.rs @@ -2,6 +2,8 @@ * SPDX-FileCopyrightText: 2020 Stalwart Labs LLC * * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-SEL + * + * Modified by Coffey Labs in 2026 for INBUXA. */ use crate::registry::mapping::{ @@ -151,12 +153,7 @@ pub(crate) async fn validate_domain( // Schedule ACME renewal task if needed if !will_trigger_acme && let CertificateManagement::Automatic(details) = &domain.certificate_management - && old_domain.is_none_or(|old| { - !matches!( - old.certificate_management, - CertificateManagement::Automatic(_) - ) - }) + && needs_certificate_order(domain, old_domain) { let Some(provider) = set .server @@ -179,15 +176,82 @@ pub(crate) async fn validate_domain( ))); } - tasks.push(Task::AcmeRenewal(TaskDomainManagement { - domain_id: Id::default(), - status: TaskStatus::now(), - })); + // inbuxa: CAA and the DNS-PERSIST-01 record name the provider, and the + // persist record says whether wildcards are allowed. Publish them + // before ordering, or the CA refuses the order (#290). + let publishes_caa = matches!( + &domain.dns_management, + DnsManagement::Automatic(dns) if dns.publish_records.contains(&DnsRecordType::Caa) + ); + if matches!(domain.dns_management, DnsManagement::Automatic(_)) + && (publishes_caa || matches!(provider.challenge_type, AcmeChallengeType::DnsPersist01)) + && acme_records_outdated(domain, old_domain) + { + tasks.push(Task::DnsManagement(TaskDnsManagement { + domain_id: Id::default(), + update_records: Map::new(if publishes_caa { + vec![DnsRecordType::Caa] + } else { + vec![] + }), + on_success_renew_certificate: true, + status: TaskStatus::now(), + })); + } else { + tasks.push(Task::AcmeRenewal(TaskDomainManagement { + domain_id: Id::default(), + status: TaskStatus::now(), + })); + } } Ok(Ok(response)) } +/// inbuxa: whether a save changed what the domain's certificate is ordered +/// for. The renewal task decides what to do about it: a certificate that +/// already has these names just waits for its renewal date (#290). +fn needs_certificate_order(domain: &Domain, old_domain: Option<&Domain>) -> bool { + let CertificateManagement::Automatic(new) = &domain.certificate_management else { + return false; + }; + let Some(old_domain) = old_domain else { + return true; + }; + let CertificateManagement::Automatic(old) = &old_domain.certificate_management else { + return true; + }; + let names = |list: &'_ Map| { + let mut names = list.as_slice().to_vec(); + names.sort(); + names.dedup(); + names + }; + + old_domain.name != domain.name + || old.acme_provider_id != new.acme_provider_id + || names(&old.subject_alternative_names) != names(&new.subject_alternative_names) +} + +/// inbuxa: whether a save left the published CAA and DNS-PERSIST-01 records +/// out of date. Only an empty name list asks for a wildcard, so changing one +/// list of names for another doesn't touch them. +fn acme_records_outdated(domain: &Domain, old_domain: Option<&Domain>) -> bool { + let CertificateManagement::Automatic(new) = &domain.certificate_management else { + return false; + }; + let Some(old_domain) = old_domain else { + return true; + }; + let CertificateManagement::Automatic(old) = &old_domain.certificate_management else { + return true; + }; + + old_domain.name != domain.name + || old.acme_provider_id != new.acme_provider_id + || old.subject_alternative_names.is_empty() != new.subject_alternative_names.is_empty() +} + fn publishes_dkim(domain: &Domain) -> bool { matches!( &domain.dns_management, @@ -226,3 +290,88 @@ pub(crate) async fn validate_dns_server( Ok(Ok(response)) } + +#[cfg(test)] +mod tests { + use super::*; + use registry::schema::structs::CertificateManagementProperties; + + fn domain(name: &str, provider: u64, names: &[&str]) -> Domain { + Domain { + name: name.to_string(), + certificate_management: CertificateManagement::Automatic( + CertificateManagementProperties { + acme_provider_id: Id::from(provider), + subject_alternative_names: Map::new( + names.iter().map(|name| name.to_string()).collect(), + ), + }, + ), + ..Default::default() + } + } + + #[test] + fn saving_the_same_names_orders_nothing() { + let old = domain("example.org", 1, &["mail", "imap"]); + assert!(!needs_certificate_order( + &domain("example.org", 1, &["imap", "mail"]), + Some(&old) + )); + assert!(!acme_records_outdated( + &domain("example.org", 1, &["imap", "mail"]), + Some(&old) + )); + } + + #[test] + fn changing_the_names_orders_a_certificate() { + let old = domain("example.org", 1, &["mail"]); + for new in [ + domain("example.org", 1, &["mail", "imap"]), + domain("example.org", 1, &["imap"]), + domain("example.org", 1, &[]), + domain("example.org", 2, &["mail"]), + domain("example.net", 1, &["mail"]), + ] { + assert!(needs_certificate_order(&new, Some(&old))); + } + } + + #[test] + fn switching_to_automatic_orders_a_certificate_and_manual_doesnt() { + let manual = Domain { + certificate_management: CertificateManagement::Manual, + ..domain("example.org", 1, &[]) + }; + let automatic = domain("example.org", 1, &[]); + assert!(needs_certificate_order(&automatic, Some(&manual))); + assert!(needs_certificate_order(&automatic, None)); + assert!(!needs_certificate_order(&manual, Some(&automatic))); + assert!(!needs_certificate_order(&manual, Some(&manual))); + } + + #[test] + fn records_only_go_stale_when_what_they_say_changes() { + let old = domain("example.org", 1, &["mail"]); + // Still no wildcard, same provider + assert!(!acme_records_outdated( + &domain("example.org", 1, &["imap"]), + Some(&old) + )); + // Wildcard allowed from here on + assert!(acme_records_outdated( + &domain("example.org", 1, &[]), + Some(&old) + )); + // A different provider or owner name + assert!(acme_records_outdated( + &domain("example.org", 2, &["mail"]), + Some(&old) + )); + assert!(acme_records_outdated( + &domain("example.net", 1, &["mail"]), + Some(&old) + )); + } +} diff --git a/crates/jmap/src/registry/set.rs b/crates/jmap/src/registry/set.rs index 859cacf7..d081581f 100644 --- a/crates/jmap/src/registry/set.rs +++ b/crates/jmap/src/registry/set.rs @@ -55,11 +55,12 @@ use registry::{ }, structs::{ Certificate, DkimSignature, DnsServer, Domain, PublicKey, Role, SieveSystemScript, - SieveUserScript, Task, + SieveUserScript, Task, TaskDnsManagement, }, }, types::id::ObjectId, }; +use services::task_manager::{domain_check::clear_queued_renewals, lock::TaskLockManager}; use std::borrow::Cow; use store::{ registry::{ @@ -866,6 +867,24 @@ impl RegistrySet for Server { && let Some(object_id) = object_id { let mut batch = BatchBuilder::new(); + + // inbuxa: a new renewal replaces the domain's queued + // one, or both would keep renewing from here on (#290) + let replaced = if tasks.iter().any(|task| { + matches!( + task, + Task::AcmeRenewal(_) + | Task::DnsManagement(TaskDnsManagement { + on_success_renew_certificate: true, + .. + }) + ) + }) { + clear_queued_renewals(set.server, object_id, &mut batch).await? + } else { + Vec::new() + }; + for mut task in tasks.drain(..) { match &mut task { Task::AcmeRenewal(task) => task.domain_id = object_id, @@ -876,6 +895,9 @@ impl RegistrySet for Server { batch.schedule_task(task); } set.server.store().write(batch.build_all()).await?; + for id in replaced { + set.server.remove_index_lock(id).await; + } set.server.notify_task_queue(); } } diff --git a/crates/services/src/task_manager/dns.rs b/crates/services/src/task_manager/dns.rs index 1fb7cf1e..7675350e 100644 --- a/crates/services/src/task_manager/dns.rs +++ b/crates/services/src/task_manager/dns.rs @@ -103,7 +103,9 @@ impl DnsManagementTask for Server { } async fn dns_management(server: &Server, task: &TaskDnsManagement) -> trc::Result { - if task.update_records.is_empty() { + // inbuxa: a task that renews afterwards can have an empty list: the + // DNS-PERSIST-01 record is published whichever types are asked for (#290) + if task.update_records.is_empty() && !task.on_success_renew_certificate { return Ok(TaskResult::permanent( "No DNS records to update".to_string(), )); diff --git a/crates/services/src/task_manager/domain_check.rs b/crates/services/src/task_manager/domain_check.rs index 167dab55..369bfe1e 100644 --- a/crates/services/src/task_manager/domain_check.rs +++ b/crates/services/src/task_manager/domain_check.rs @@ -10,6 +10,7 @@ //! whose chain was broken) never got another, so its keys stopped rotating. //! This check runs on the task scheduler and queues the missing task. +use crate::task_manager::lock::TaskLockManager; use common::Server; use registry::{ schema::{ @@ -154,10 +155,45 @@ async fn has_dkim_keys(server: &Server, domain_id: Id) -> trc::Result { .caused_by(trc::location!()) } -async fn queued_domain_tasks(server: &Server) -> trc::Result { - // The due index carries each task's type, so only the few domain tasks - // are read in full. Failed tasks sit at the end of it (due u64::MAX). - let mut ids = Vec::new(); +/// Adds to `batch` the removal of a domain's queued renewal tasks, whatever +/// their state, because a new one is about to replace them: each run queues +/// the next, so every extra task would be another chain of renewals, for good +/// (#290). Should the new one never run, the scheduled check queues another. +/// +/// Returns the tasks it locked, to unlock once the batch is written. A task +/// that's running right now can't be removed, and it stays. +pub async fn clear_queued_renewals( + server: &Server, + domain_id: Id, + batch: &mut BatchBuilder, +) -> trc::Result> { + let mut locked = Vec::new(); + for (due, id) in queued_tasks(server, &[TaskType::AcmeRenewal]).await? { + let task = server + .store() + .get_value::(ValueKey::from(ValueClass::TaskQueue( + TaskQueueClass::Task { id }, + ))) + .await + .caused_by(trc::location!())?; + if matches!(task, Some(Task::AcmeRenewal(task)) if task.domain_id == domain_id) + && server.try_lock_task(id).await + { + batch + .clear(ValueClass::TaskQueue(TaskQueueClass::Task { id })) + .clear(ValueClass::TaskQueue(TaskQueueClass::Due { id, due })); + locked.push(id); + } + } + + Ok(locked) +} + +/// The due time and id of every queued task of the wanted types. The due +/// index carries each task's type, so nothing is read in full. Failed tasks +/// sit at the end of it (due u64::MAX). +async fn queued_tasks(server: &Server, wanted: &[TaskType]) -> trc::Result> { + let mut tasks = Vec::new(); server .store() .iterate( @@ -171,11 +207,8 @@ async fn queued_domain_tasks(server: &Server) -> trc::Result .ascending(), |key, value| { let task_type = value.deserialize_be_u16(0).ok().and_then(TaskType::from_id); - if matches!( - task_type, - Some(TaskType::DkimManagement | TaskType::AcmeRenewal) - ) { - ids.push(key.deserialize_be_u64(U64_LEN)?); + if task_type.is_some_and(|typ| wanted.contains(&typ)) { + tasks.push((key.deserialize_be_u64(0)?, key.deserialize_be_u64(U64_LEN)?)); } Ok(true) }, @@ -183,8 +216,14 @@ async fn queued_domain_tasks(server: &Server) -> trc::Result .await .caused_by(trc::location!())?; + Ok(tasks) +} + +async fn queued_domain_tasks(server: &Server) -> trc::Result { + let tasks = queued_tasks(server, &[TaskType::DkimManagement, TaskType::AcmeRenewal]).await?; + let mut queued = QueuedDomainTasks::default(); - for id in ids { + for (_, id) in tasks { match server .store() .get_value::(ValueKey::from(ValueClass::TaskQueue( diff --git a/tests/src/automation/acme.rs b/tests/src/automation/acme.rs index bf67ba07..1c0e93f2 100644 --- a/tests/src/automation/acme.rs +++ b/tests/src/automation/acme.rs @@ -445,6 +445,149 @@ pub async fn test(test: &TestServer) { vec!["*.persist.org".to_string(), "persist.org".to_string()] ); + account.registry_destroy_all(ObjectType::Certificate).await; + account.registry_destroy_all(ObjectType::Task).await; + + // Changing a domain's names orders a certificate for them straight away, + // and leaves one renewal queued (#290) + let names_acme_id = account + .registry_create_object(AcmeProvider { + directory: "https://localhost:14000/dir".to_string(), + contact: Map::new(vec!["mailto:hello@names.org".to_string()]), + challenge_type: AcmeChallengeType::TlsAlpn01, + ..Default::default() + }) + .await; + let names_domain = |names: &[&str]| { + CertificateManagement::Automatic(CertificateManagementProperties { + acme_provider_id: names_acme_id, + subject_alternative_names: Map::new(names.iter().map(|n| n.to_string()).collect()), + }) + }; + let names_domain_id = account + .registry_create_object(Domain { + name: "names.org".to_string(), + certificate_management: names_domain(&["a", "b"]), + dkim_management: DkimManagement::Manual, + dns_management: DnsManagement::Manual, + ..Default::default() + }) + .await; + test.wait_for_tasks_skip_not_due().await; + assert_certificates(account, &[&["a.names.org", "b.names.org"]]).await; + assert_one_renewal(account, names_domain_id).await; + + // Same names in a different order: nothing to do + account + .registry_update_object( + ObjectType::Domain, + names_domain_id, + json!({ Property::CertificateManagement: names_domain(&["b", "a"]) }), + ) + .await; + test.wait_for_tasks_skip_not_due().await; + assert_certificates(account, &[&["a.names.org", "b.names.org"]]).await; + assert_one_renewal(account, names_domain_id).await; + + // A new name + account + .registry_update_object( + ObjectType::Domain, + names_domain_id, + json!({ Property::CertificateManagement: names_domain(&["a", "b", "c"]) }), + ) + .await; + test.wait_for_tasks_skip_not_due().await; + assert_certificates( + account, + &[ + &["a.names.org", "b.names.org"], + &["a.names.org", "b.names.org", "c.names.org"], + ], + ) + .await; + assert_one_renewal(account, names_domain_id).await; + + // Off and on again, the way around this before it was fixed + account + .registry_update_object( + ObjectType::Domain, + names_domain_id, + json!({ Property::CertificateManagement: CertificateManagement::Manual }), + ) + .await; + account + .registry_update_object( + ObjectType::Domain, + names_domain_id, + json!({ Property::CertificateManagement: names_domain(&["a", "b", "c"]) }), + ) + .await; + test.wait_for_tasks_skip_not_due().await; + assert_one_renewal(account, names_domain_id).await; + + // DNS-PERSIST-01 only allows a wildcard if its record says so. Going from + // named hosts to the wildcard needs that record republished first. + let names_persist_id = account + .registry_create_object(Domain { + name: "names-persist.org".to_string(), + certificate_management: CertificateManagement::Automatic( + CertificateManagementProperties { + acme_provider_id: dns_acme_id, + subject_alternative_names: Map::new(vec!["www".to_string()]), + }, + ), + dkim_management: DkimManagement::Manual, + dns_management: DnsManagement::Automatic(DnsManagementProperties { + dns_server_id: pebble_dns_id, + publish_records: Map::new(vec![DnsRecordType::TlsRpt]), + ..Default::default() + }), + ..Default::default() + }) + .await; + test.wait_for_tasks_skip_not_due().await; + account + .registry_update_object( + ObjectType::Domain, + names_persist_id, + json!({ + Property::CertificateManagement: CertificateManagement::Automatic( + CertificateManagementProperties { + acme_provider_id: dns_acme_id, + subject_alternative_names: Default::default(), + }, + ), + }), + ) + .await; + test.wait_for_tasks_skip_not_due().await; + let persist_sans = account + .registry_get_all::() + .await + .into_iter() + .map(|(_, certificate)| { + let mut sans = certificate.subject_alternative_names.into_inner(); + sans.sort(); + sans + }) + .collect::>(); + assert!( + persist_sans.contains(&vec![ + "*.names-persist.org".to_string(), + "names-persist.org".to_string() + ]), + "no wildcard certificate: {persist_sans:?}" + ); + assert_one_renewal(account, names_persist_id).await; + + account + .registry_destroy(ObjectType::Domain, [names_domain_id, names_persist_id]) + .await + .assert_destroyed(&[names_domain_id, names_persist_id]); + account.registry_destroy_all(ObjectType::Certificate).await; + account.registry_destroy_all(ObjectType::Task).await; + // Test preferred chain selection against the alternate chains Pebble offers (RFC 8555 7.4.2) let pebble_roots = pebble_root_common_names().await; assert!( @@ -688,6 +831,39 @@ pub async fn test(test: &TestServer) { account.registry_destroy_all(ObjectType::Task).await; } +/// The stored certificates are exactly these sets of names. +async fn assert_certificates(account: &crate::utils::account::Account, expected: &[&[&str]]) { + let mut found = account + .registry_get_all::() + .await + .into_iter() + .map(|(_, certificate)| { + let mut sans = certificate.subject_alternative_names.into_inner(); + sans.sort(); + sans + }) + .collect::>(); + found.sort(); + let mut expected = expected + .iter() + .map(|names| names.iter().map(|n| n.to_string()).collect::>()) + .collect::>(); + expected.sort(); + assert_eq!(found, expected); +} + +async fn assert_one_renewal(account: &crate::utils::account::Account, domain: types::id::Id) { + let renewals = account + .registry_get_all::() + .await + .into_iter() + .filter(|(_, task)| { + matches!(task, Task::AcmeRenewal(TaskDomainManagement { domain_id, .. }) if *domain_id == domain) + }) + .count(); + assert_eq!(renewals, 1, "expected one queued renewal for the domain"); +} + const ISSUER_ABC_CERT: &str = r#"-----BEGIN CERTIFICATE----- MIIDLjCCAhagAwIBAgIURV4DMcpCSV95vODPEDWtzZ0XTjIwDQYJKoZIhvcNAQEL BQAwGDEWMBQGA1UEAwwNSXNzdWVyIEFCQyBDQTAeFw0yNjA0MDMxMjQ1MjVaFw0y -- 2.54.0