Chain validation cleanup

crates/ct_worker/build.rs

@@ -58,14 +58,12 @@ fn main() {
         }
     }
 
-    // Get and validate roots from an embedded roots file, which must exist but
-    // can be empty unless 'enable_ccadb_roots' is set to false. If
-    // 'enable_ccadb_roots' is set to true, the log shard will combine trusted
-    // roots from the embedded roots file and from a roots file loaded from
-    // Workers KV.
+    // Get and validate roots from an embedded roots file, which must exist if
+    // 'enable_ccadb_roots' is false. If 'enable_ccadb_roots' is true, the log
+    // shard will combine trusted roots from the embedded roots file and from a
+    // roots file loaded from Workers KV.
     let roots_file: &str = &format!("roots.{env}.pem");
     let roots_file_exists = fs::exists(roots_file).expect("failed to check if file exists");
-    // If 'enable_ccadb_roots' is
     if roots_file_exists {
         let roots =
             Certificate::load_pem_chain(&fs::read(roots_file).expect("failed to read roots file"))

crates/mtc_api/src/lib.rs

@@ -498,6 +498,7 @@ pub fn tbs_cert_to_log_entry(
 ///
 /// Returns an error if either certificate is invalid, or if the bootstrap
 /// certificate doesn't cover the log entry.
+#[allow(clippy::too_many_lines)]
 pub fn validate_correspondence(
     log_entry: &TbsCertificateLogEntry,
     raw_chain: &[Vec<u8>],
@@ -506,8 +507,8 @@ pub fn validate_correspondence(
     // We will run ordinary chain validation on the given chain. After, we will do additional
     // validation, expressed in the below closure.
     let validator_hook = |leaf: Certificate,
-                          intermediates: Vec<Certificate>,
-                          _full_chain_fingerprints: Vec<[u8; 32]>,
+                          chain_certs: Vec<&Certificate>,
+                          _chain_fingerprints: Vec<[u8; 32]>,
                           _found_root_idx: Option<usize>|
      -> Result<(), MtcError> {
         let bootstrap = leaf.tbs_certificate.clone();
@@ -519,7 +520,7 @@ pub fn validate_correspondence(
         }
         // Make sure the validity is contained within the validity of every cert in
         // the chain.
-        for cert in core::iter::once(&leaf).chain(intermediates.iter()) {
+        for cert in core::iter::once(&leaf).chain(chain_certs) {
             if log_entry.validity.not_before.to_unix_duration().lt(&cert
                 .tbs_certificate
                 .validity
@@ -567,17 +568,18 @@ pub fn validate_correspondence(
             ));
         }
 
-        // If no extensions, we're done. If mismatched, that's an error
-        match (&log_entry.extensions, &bootstrap.extensions) {
-            (None, None) => return Ok(()),
-            (Some(_), None) | (None, Some(_)) => {
-                return Err(MtcError::Dynamic("mismatched extensions".into()))
-            }
-            _ => {}
-        };
-        // Otherwise both the log entry and bootstrap cert have extensions
-        let log_entry_extensions = log_entry.extensions.as_ref().unwrap();
-        let mut bootstrap_extensions = bootstrap.extensions.unwrap();
+        let (log_entry_extensions, mut bootstrap_extensions) =
+            match (&log_entry.extensions, bootstrap.extensions) {
+                // If no extensions in either entry or bootstrap, we're done.
+                (None, None) => return Ok(()),
+                // If mismatched, that's an error.
+                (Some(_), None) | (None, Some(_)) => {
+                    return Err(MtcError::Dynamic("mismatched extensions".into()))
+                }
+                // Otherwise both the log entry and bootstrap cert have
+                // extensions. Check them below.
+                (Some(log_ext), Some(boot_ext)) => (log_ext, boot_ext),
+            };
 
         // Check and filter bootstrap extensions.
         filter_extensions(&mut bootstrap_extensions)?;
@@ -587,7 +589,7 @@ pub fn validate_correspondence(
             return Err(MtcError::Dynamic(
                 "bootstrap extension lengths differ".into(),
             ));
-        };
+        }
 
         let bootstrap_extensions_map = bootstrap_extensions
             .into_iter()
@@ -625,7 +627,9 @@ pub fn validate_correspondence(
         Ok(())
     };
 
-    // Run the validation logic with the above validation hook
+    // Run the validation logic with the above validation hook. We do
+    // not give `validate_chain_lax` a window for the `not_after` validity,
+    // since validity is checked within the validator hook.
     validate_chain_lax(raw_chain, roots, None, None, validator_hook).map_err(|e| match e {
         x509_util::HookOrValidationError::Validation(ve) => ve.into(),
         x509_util::HookOrValidationError::Hook(he) => he,
@@ -634,6 +638,26 @@ pub fn validate_correspondence(
 
 /// Parse and validate a bootstrap chain, returning a pending log entry.
 ///
+/// # Arguments
+///
+/// * `raw_chain` - The 'bootstrap' chain of certificates submitted to the
+///   `add-entry` endpoint. Each entry must sign the previous entry, and the
+///   chain must start with a leaf certificate and end with a certificate that
+///   is a trusted root or is signed by a trusted root.
+/// * `roots` - A certificate pool containing the trusted roots.
+/// * `issuer` - The issuer name of the Merkle Tree CA, to replace the issuer in
+///   the bootstrap certificate.
+/// * `validity` - A bound on the maximum validity period for the returned
+///   Merkle Tree log entry, based on the Merkle Tree CA's parameters. This
+///   bound is further adjusted to ensure that it is covered by the bootstrap
+///   chain.
+///
+/// # Returns
+///
+/// Returns a pending Merkle Tree log entry derived from the bootstrap chain and
+/// other provided parameters and the inferred root, if a root had to be
+/// inferred.
+///
 /// # Errors
 ///
 /// Returns an error if the chain is invalid.
@@ -642,14 +666,17 @@ pub fn validate_chain(
     roots: &CertPool,
     issuer: RdnSequence,
     mut validity: Validity,
-) -> Result<BootstrapMtcPendingLogEntry, MtcError> {
+) -> Result<(BootstrapMtcPendingLogEntry, Option<usize>), MtcError> {
     // We will run the ordinary chain validation on our input, but we have some post-processing we
     // need to do too. Namely we need to adjust the validity period of the provided bootstrap cert,
     // and then construct a pending log entry. We do this in the validation hook.
-    let validation_hook = |leaf: Certificate, intermediates: Vec<Certificate>, _, _| {
+    let validator_hook = |leaf: Certificate,
+                          chain_certs: Vec<&Certificate>,
+                          chain_fingerprints: Vec<[u8; 32]>,
+                          found_root_idx: Option<usize>| {
         // Adjust the validity bound to the overlapping part of validity periods of
         // all certificates in the chain.
-        for cert in std::iter::once(&leaf).chain(intermediates.iter()) {
+        for cert in std::iter::once(&leaf).chain(chain_certs) {
             if validity.not_before.to_unix_duration().lt(&cert
                 .tbs_certificate
                 .validity
@@ -680,15 +707,11 @@ pub fn validate_chain(
         }
 
         let mut bootstrap = Vec::new();
+        // SAFETY: `validate_chain_lax` checks that `raw_chain` is non-empty. We
+        // use `raw_chain[0]` here instead of `leaf` to avoid having to
+        // re-encode it to DER format.
         bootstrap.write_length_prefixed(&raw_chain[0], 3)?;
-        bootstrap.write_length_prefixed(
-            &raw_chain[1..]
-                .iter()
-                .map(Sha256::digest)
-                .collect::<Vec<_>>()
-                .concat(),
-            2,
-        )?;
+        bootstrap.write_length_prefixed(&chain_fingerprints.concat(), 2)?;
 
         let mut bootstrap_tile_entry = Vec::new();
         bootstrap_tile_entry.write_length_prefixed(bootstrap.as_slice(), 3)?;
@@ -704,13 +727,13 @@ pub fn validate_chain(
                 .encode()?,
             },
         };
-        Ok(pending_entry)
+        Ok((pending_entry, found_root_idx))
     };
 
-    // Run the validation and return the hook-constructed pending entry. We do not give the
-    // validator a window for the `not_after` validity, since we already know the `not_after` we're
-    // giving it, and it's quite short.
-    let pending_entry = validate_chain_lax(raw_chain, roots, None, None, validation_hook);
+    // Run the validation and return the hook-constructed pending entry. We do
+    // not give `validate_chain_lax` a window for the `not_after` validity,
+    // since validity is checked within the validator hook.
+    let pending_entry = validate_chain_lax(raw_chain, roots, None, None, validator_hook);
     pending_entry.map_err(|e| match e {
         x509_util::HookOrValidationError::Validation(ve) => ve.into(),
         x509_util::HookOrValidationError::Hook(he) => he,
@@ -722,21 +745,10 @@ mod tests {
     use der::asn1::UtcTime;
     use std::time::Duration;
     use x509_cert::{time::Time, Certificate};
-    use x509_util::certs_to_bytes;
+    use x509_util::{build_chain, certs_to_bytes};
 
     use super::*;
 
-    /// Builds a certificate chain from the the given PEM files
-    macro_rules! build_chain {
-        ($($root_file:expr),+) => {{
-            let mut chain = Vec::new();
-            $(
-                chain.append(&mut Certificate::load_pem_chain(include_bytes!($root_file)).unwrap());
-            )*
-            chain
-        }}
-    }
-
     #[test]
     fn test_tbs_cert_to_log_entry() {
         let bootstrap_chain = build_chain!(

crates/mtc_worker/src/frontend_worker.rs

@@ -9,7 +9,7 @@ use crate::{
 };
 use der::{
     asn1::{SetOfVec, UtcTime, Utf8StringRef},
-    Any, Tag,
+    Any, Encode, Tag,
 };
 use generic_log_worker::{
     batcher_id_from_lookup_key, deserialize, get_cached_metadata, get_durable_object_stub,
@@ -306,6 +306,7 @@ async fn main(req: Request, env: Env, _ctx: Context) -> Result<Response> {
         })
 }
 
+#[allow(clippy::too_many_lines)]
 async fn add_entry(mut req: Request, env: &Env, name: &str) -> Result<Response> {
     let params = &CONFIG.logs[name];
     let req: AddEntryRequest = req.json().await?;
@@ -335,8 +336,9 @@ async fn add_entry(mut req: Request, env: &Env, name: &str) -> Result<Response>
         ),
     };
 
-    let pending_entry =
-        match mtc_api::validate_chain(&req.chain, load_roots(env, name).await?, issuer, validity) {
+    let roots = load_roots(env, name).await?;
+    let (pending_entry, found_root_idx) =
+        match mtc_api::validate_chain(&req.chain, roots, issuer, validity) {
             Ok(v) => v,
             Err(e) => {
                 log::debug!("{name}: Bad request: {e}");
@@ -362,17 +364,24 @@ async fn add_entry(mut req: Request, env: &Env, name: &str) -> Result<Response>
 
     // Entry is not cached, so we need to sequence it.
 
-    // First persist issuers.
-    let public_bucket = ObjectBucket::new(load_public_bucket(env, name)?);
-    generic_log_worker::upload_issuers(
-        &public_bucket,
-        &req.chain[1..]
+    // First persist issuers. Use a block so memory is deallocated sooner.
+    {
+        let public_bucket = ObjectBucket::new(load_public_bucket(env, name)?);
+        let mut issuers = req.chain[1..]
             .iter()
             .map(Vec::as_slice)
-            .collect::<Vec<&[u8]>>(),
-        name,
-    )
-    .await?;
+            .collect::<Vec<&[u8]>>();
+
+        // Make sure the found root is persisted as well, if the add-chain
+        // request did not include the root.
+        let root_bytes;
+        if let Some(idx) = found_root_idx {
+            root_bytes = roots.certs[idx].to_der().map_err(|e| e.to_string())?;
+            issuers.push(&root_bytes);
+        }
+
+        generic_log_worker::upload_issuers(&public_bucket, &issuers, name).await?;
+    }
 
     // Submit entry to be sequenced, either via a batcher or directly to the
     // sequencer.

crates/static_ct_api/src/lib.rs

@@ -37,12 +37,12 @@ pub enum StaticCTError {
     InvalidLeaf,
     #[error("CT poison extension is not critical or invalid")]
     InvalidCTPoison,
+    #[error("missing precertificate issuer")]
+    MissingPrecertIssuer,
     #[error("missing precertificate signing certificate issuer")]
     MissingPrecertSigningCertificateIssuer,
     #[error(
         "{}certificate submitted to add-{}chain", if *.is_precert { "pre-" } else { "final " }, if *.is_precert { "" } else { "pre-" }
     )]
     EndpointMismatch { is_precert: bool },
-    #[error("mismatching signature algorithm identifier")]
-    MismatchingSigAlg,
 }