Skip to content

Replace OnceCell::get_or_try_init with deadlock-safe non-blocking load_roots #205

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lukevalenta merged 1 commit into from
Apr 13, 2026
Merged

Replace OnceCell::get_or_try_init with deadlock-safe non-blocking load_roots #205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion crates/bootstrap_mtc_worker/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ serde_json.workspace = true
serde_with.workspace = true
signed_note.workspace = true
tlog_tiles.workspace = true
tokio.workspace = true
worker.workspace = true
x509-cert.workspace = true
x509_util.workspace = true
Expand Down
113 changes: 60 additions & 53 deletions crates/bootstrap_mtc_worker/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ use std::collections::HashMap;
use std::str::FromStr;
use std::sync::{LazyLock, OnceLock};
use tlog_tiles::SequenceMetadata;
use tokio::sync::OnceCell;
#[allow(clippy::wildcard_imports)]
use worker::*;
use x509_util::CertPool;
Expand All @@ -32,7 +31,7 @@ static CONFIG: LazyLock<AppConfig> = LazyLock::new(|| {
});

static SIGNING_KEY_MAP: OnceLock<HashMap<String, OnceLock<Ed25519SigningKey>>> = OnceLock::new();
static ROOTS: OnceCell<CertPool> = OnceCell::const_new();
static ROOTS: OnceLock<CertPool> = OnceLock::new();

pub(crate) fn load_signing_key(env: &Env, name: &str) -> Result<&'static Ed25519SigningKey> {
load_ed25519_key(env, name, &SIGNING_KEY_MAP, &format!("SIGNING_KEY_{name}"))
Expand Down Expand Up @@ -85,58 +84,66 @@ pub(crate) fn load_origin(name: &str) -> KeyName {
}

async fn load_roots(env: &Env, name: &str) -> Result<&'static CertPool> {
// Load embedded roots.
ROOTS
.get_or_try_init(|| async {
let mut pool = CertPool::default();
// Load additional roots from the CCADB roots file in Workers KV.
let kv = env.kv(CCADB_ROOTS_NAMESPACE)?;
let pem = if let Some(pem) = kv.get(CCADB_ROOTS_FILENAME).text().await? {
pem
} else {
// The roots file might not exist if the CCADB roots cron job hasn't
// run yet. Try to create it once before failing.
update_ccadb_roots(&kv).await?;
kv.get(CCADB_ROOTS_FILENAME)
.text()
.await?
.ok_or(format!("{name}: '{CCADB_ROOTS_FILENAME}' not found in KV"))?
};
// Fast path: already initialized.
if let Some(pool) = ROOTS.get() {
return Ok(pool);
}

// Build the pool for this request. If another request concurrently built
// and stored one first, we discard ours and return the stored value.
// This avoids awaiting an OnceLock initialized by another request context,
// which the Workers runtime would cancel as a cross-request deadlock.
let mut pool = CertPool::default();

pool.append_certs_from_pem(pem.as_bytes())
.map_err(|e| format!("failed to add CCADB certs to pool: {e}"))?;
// Load additional roots from the CCADB roots file in Workers KV.
let kv = env.kv(CCADB_ROOTS_NAMESPACE)?;
let pem = if let Some(pem) = kv.get(CCADB_ROOTS_FILENAME).text().await? {
pem
} else {
// The roots file might not exist if the CCADB roots cron job hasn't
// run yet. Try to create it once before failing.
update_ccadb_roots(&kv).await?;
kv.get(CCADB_ROOTS_FILENAME)
.text()
.await?
.ok_or(format!("{name}: '{CCADB_ROOTS_FILENAME}' not found in KV"))?
};

// Add additional roots when the 'dev-bootstrap-roots' feature is
// enabled.
//
// A note on the differences between how roots are handled for the
// MTC vs CT applications:
//
// The purpose of CT is to observe certificates but not police them.
// As long as it's not a spam vector, we're generally willing to
// accept any root certificates that have been trusted by at least
// one major root program during the log shard's lifetime. Roots
// aren't removed from the list once they're added in order to keep
// a better record. We have the ability to add in custom roots from
// a per-environment roots file too, in order to support test CAs.
//
// For bootstrap MTC, the roots are meant to ensure that the log
// only accepts bootstrap MTC chains that will be trusted by Chrome,
// since Chrome might reject an entire batch of MTCs if there's a
// single untrusted entry. Thus, we want to keep the trusted roots
// as a subset of Chrome's trust store. We're using Mozilla's CRLite
// filters to check for revocation, so we need to be a subset of
// Mozilla's trust store too. When either root program stops
// trusting a root, we also need to remove it from our trust store.
// Given that, we gate the ability to add in custom roots behind the
// 'dev-bootstrap-roots' feature flag.
#[cfg(feature = "dev-bootstrap-roots")]
{
pool.append_certs_from_pem(include_bytes!("../dev-bootstrap-roots.pem"))
.map_err(|e| format!("failed to add dev certs to pool: {e}"))?;
}
pool.append_certs_from_pem(pem.as_bytes())
.map_err(|e| format!("failed to add CCADB certs to pool: {e}"))?;

// Add additional roots when the 'dev-bootstrap-roots' feature is
// enabled.
//
// A note on the differences between how roots are handled for the
// MTC vs CT applications:
//
// The purpose of CT is to observe certificates but not police them.
// As long as it's not a spam vector, we're generally willing to
// accept any root certificates that have been trusted by at least
// one major root program during the log shard's lifetime. Roots
// aren't removed from the list once they're added in order to keep
// a better record. We have the ability to add in custom roots from
// a per-environment roots file too, in order to support test CAs.
//
// For bootstrap MTC, the roots are meant to ensure that the log
// only accepts bootstrap MTC chains that will be trusted by Chrome,
// since Chrome might reject an entire batch of MTCs if there's a
// single untrusted entry. Thus, we want to keep the trusted roots
// as a subset of Chrome's trust store. We're using Mozilla's CRLite
// filters to check for revocation, so we need to be a subset of
// Mozilla's trust store too. When either root program stops
// trusting a root, we also need to remove it from our trust store.
// Given that, we gate the ability to add in custom roots behind the
// 'dev-bootstrap-roots' feature flag.
#[cfg(feature = "dev-bootstrap-roots")]
{
pool.append_certs_from_pem(include_bytes!("../dev-bootstrap-roots.pem"))
.map_err(|e| format!("failed to add dev certs to pool: {e}"))?;
}

Ok(pool)
})
.await
// Store the pool if no other request got there first; either way return
// the value now in the cell.
let _ = ROOTS.set(pool);
Ok(ROOTS.get().expect("just set"))
}
1 change: 0 additions & 1 deletion crates/ct_worker/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ serde_with.workspace = true
static_ct_api.workspace = true
signed_note.workspace = true
tlog_tiles.workspace = true
tokio.workspace = true
worker.workspace = true
x509-cert.workspace = true
x509_util.workspace = true
Expand Down
73 changes: 40 additions & 33 deletions crates/ct_worker/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ use static_ct_api::StaticCTCheckpointSigner;
use std::collections::HashMap;
use std::sync::{LazyLock, OnceLock};
use tlog_tiles::{CheckpointSigner, Ed25519CheckpointSigner, SequenceMetadata};
use tokio::sync::OnceCell;
use worker::{Env, Result};
use x509_util::CertPool;

Expand All @@ -30,7 +29,7 @@ static CONFIG: LazyLock<AppConfig> = LazyLock::new(|| {

static SIGNING_KEY_MAP: OnceLock<HashMap<String, OnceLock<EcdsaSigningKey>>> = OnceLock::new();
static WITNESS_KEY_MAP: OnceLock<HashMap<String, OnceLock<Ed25519SigningKey>>> = OnceLock::new();
static ROOTS: OnceCell<CertPool> = OnceCell::const_new();
static ROOTS: OnceLock<CertPool> = OnceLock::new();

pub(crate) fn load_signing_key(env: &Env, name: &str) -> Result<&'static EcdsaSigningKey> {
let once = &SIGNING_KEY_MAP.get_or_init(|| {
Expand Down Expand Up @@ -101,36 +100,44 @@ pub(crate) fn load_origin(name: &str) -> KeyName {
}

async fn load_roots(env: &Env, name: &str) -> Result<&'static CertPool> {
// Load embedded roots.
ROOTS
.get_or_try_init(|| async {
let pem = include_bytes!(concat!(env!("OUT_DIR"), "/roots.pem"));
let mut pool = CertPool::default();
// load_pem_chain fails on empty input: https://github.com/RustCrypto/formats/pull/1965
if !pem.is_empty() {
pool.append_certs_from_pem(pem)
.map_err(|e| format!("failed to load PEM chain: {e}"))?;
}
// Fast path: already initialized.
if let Some(pool) = ROOTS.get() {
return Ok(pool);
}

// Build the pool for this request. If another request concurrently built
// and stored one first, we discard ours and return the stored value.
// This avoids awaiting an OnceLock initialized by another request context,
// which the Workers runtime would cancel as a cross-request deadlock.
let pem = include_bytes!(concat!(env!("OUT_DIR"), "/roots.pem"));
let mut pool = CertPool::default();
// load_pem_chain fails on empty input: https://github.com/RustCrypto/formats/pull/1965
if !pem.is_empty() {
pool.append_certs_from_pem(pem)
.map_err(|e| format!("failed to load PEM chain: {e}"))?;
}

// Load additional roots from the CCADB roots file in Workers KV.
if CONFIG.logs[name].enable_ccadb_roots {
let key = ccadb_roots_filename(name);
let kv = env.kv(CCADB_ROOTS_NAMESPACE)?;
let pem = if let Some(pem) = kv.get(&key).text().await? {
pem
} else {
// The roots file might not exist if the CCADB roots cron job hasn't
// run yet. Try to create it once before failing.
update_ccadb_roots(&[&key], &kv).await?;
kv.get(&key)
.text()
.await?
.ok_or(format!("{name}: '{key}' not found in KV"))?
};
pool.append_certs_from_pem(pem.as_bytes())
.map_err(|e| format!("failed to add CCADB certs to pool: {e}"))?;
}

// Load additional roots from the CCADB roots file in Workers KV.
if CONFIG.logs[name].enable_ccadb_roots {
let key = ccadb_roots_filename(name);
let kv = env.kv(CCADB_ROOTS_NAMESPACE)?;
let pem = if let Some(pem) = kv.get(&key).text().await? {
pem
} else {
// The roots file might not exist if the CCADB roots cron job hasn't
// run yet. Try to create it once before failing.
update_ccadb_roots(&[&key], &kv).await?;
kv.get(&key)
.text()
.await?
.ok_or(format!("{name}: '{key}' not found in KV"))?
};
pool.append_certs_from_pem(pem.as_bytes())
.map_err(|e| format!("failed to add CCADB certs to pool: {e}"))?;
}
Ok(pool)
})
.await
// Store the pool if no other request got there first; either way return
// the value now in the cell.
let _ = ROOTS.set(pool);
Ok(ROOTS.get().expect("just set"))
}
1 change: 0 additions & 1 deletion crates/integration_tests/tests/bootstrap_mtc_api.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,6 @@ async fn ensure_initialized() {
/// DER-encoded X.509 certificates.
#[tokio::test]
async fn get_roots_returns_valid_certs() {
ensure_initialized().await;
let client = BootstrapMtcClient::default_log();
let roots = client.get_roots().await.expect("get-roots failed");

Expand Down
31 changes: 8 additions & 23 deletions crates/integration_tests/tests/static_ct_api.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,8 @@ fn now_millis() -> u64 {
/// has sequenced at least one entry before any test that depends on sequencer
/// state runs.
///
/// `ct_worker` initializes its root pool, Durable Objects, and sequencer lazily
/// on the first request. Tests that run before initialization completes see
/// `ct_worker` initializes its Durable Objects and sequencer lazily on the
/// first request. Tests that run before initialization completes may see
/// 503 (sequencer busy) or missing checkpoints. Calling `ensure_initialized`
/// at the start of any such test avoids these races without requiring a
/// specific test ordering.
Expand All @@ -75,25 +75,14 @@ async fn ensure_initialized() {
let client = CtClient::default_log();
let chains = make_chains(&client.log).expect("make_chains for warmup");

// Wait until get-roots succeeds before attempting add-chain.
// get-roots triggers the CCADB fetch that populates the ROOTS
// OnceCell. If add-chain races with that fetch in-flight from
// another request, the Workers runtime cancels it with a 500
// (cross-request promise resolution is not permitted). Waiting
// here ensures ROOTS is fully populated before add-chain is called.
let mut roots_ready = false;
for _ in 0..MAX_ATTEMPTS {
match client.get_roots().await {
Ok(_) => { roots_ready = true; break; }
// Fetch log metadata (needed for checkpoint verification).
// Retry until the frontend is reachable.
let meta = loop {
match client.get_log_v3_json().await {
Ok(m) => break m,
Err(_) => tokio::time::sleep(RETRY_DELAY).await,
}
}
if !roots_ready {
panic!("ct_worker get-roots never succeeded after {MAX_ATTEMPTS}s");
}

// Fetch log metadata (needed for checkpoint verification).
let meta = client.get_log_v3_json().await.expect("log.v3.json in warmup");
};

for attempt in 0..MAX_ATTEMPTS {
// Submit a chain to trigger full initialization (root pool load,
Expand Down Expand Up @@ -148,10 +137,6 @@ async fn ensure_initialized() {
/// valid DER-encoded X.509 certificates.
#[tokio::test]
async fn get_roots_returns_valid_certs() {
// get-roots triggers the CCADB fetch that populates the ROOTS OnceCell.
// ensure_initialized uses get-roots as its readiness probe, so whichever
// test runs first will serialize the fetch before add-chain is attempted.
ensure_initialized().await;
let client = CtClient::default_log();
let roots = client.get_roots().await.expect("get-roots failed");

Expand Down
Loading