0.34.6_repo_cleanup

src/tls/certificates.rs

@@ -1,5 +1,3 @@
-/// Build the fixed administration TLS configuration. A named inventory
-/// certificate may be selected; otherwise the app-managed default pair is used.
 pub fn compile_admin(
     settings: &Settings,
     administration: &crate::settings::Administration,
@@ -132,15 +130,9 @@ pub fn import(directory: &Path, name: &str, cert: PathBuf, key: PathBuf) -> Resu
     )
 }
 
-/// Result of reconciling an externally supplied certificate/key pair with the
-/// immutable generation currently selected by a manual certificate id.
 #[derive(Debug, PartialEq, Eq)]
 pub enum ExternalPairDeployment {
-    /// The configured RustProxy-owned generation already contains the exact
-    /// certificate chain and private key supplied by the external issuer.
     Unchanged { covered_hosts: Vec<String> },
-    /// A new immutable generation was written and must be published through the
-    /// revisioned configuration path before the old generation can be removed.
     Staged {
         cert_file: String,
         key_file: String,
@@ -159,12 +151,6 @@ fn external_generation_pair(certificate_id: &str, cert_file: &str, key_file: &st
     cert_stem == key_stem && cert_stem.starts_with(&prefix)
 }
 
-/// Validate an externally supplied pair and reconcile it with the currently
-/// selected RustProxy-owned immutable generation. Repeating the same deploy is
-/// therefore a no-op, while a missing/different pair stages a fresh generation.
-///
-/// `current_pair` is advisory: unreadable/missing current files are treated as
-/// drift and repaired by staging a new generation instead of failing the deploy.
 pub fn reconcile_external_pair(
     directory: &Path,
     certificate_id: &str,
@@ -210,8 +196,6 @@ pub fn reconcile_external_pair(
     })
 }
 
-/// Stage a locally supplied certificate/key pair under generation-specific
-/// filenames. This compatibility wrapper always stages a new generation.
 pub fn stage_external_pair(
     directory: &Path,
     certificate_id: &str,
@@ -230,8 +214,6 @@ pub fn stage_external_pair(
     }
 }
 
-/// Best-effort rollback for a generation created by `stage_external_pair`.
-/// Only generation-specific external files may be removed through this helper.
 pub fn remove_external_pair(directory: &Path, cert_file: &str, key_file: &str) -> Result<()> {
     safe_name(cert_file)?;
     safe_name(key_file)?;
@@ -254,8 +236,6 @@ pub fn remove_external_pair(directory: &Path, cert_file: &str, key_file: &str) -
     Ok(())
 }
 
-/// Select named inventory certificates per vhost/redirect host. A single global
-/// SNI resolver cannot give the same name different certificates by destination IP.
 pub fn for_all_frontends(
     settings: &Settings,
     vhosts: &[crate::vhost::Vhost],
@@ -317,7 +297,6 @@ pub fn for_all_frontends(
     Ok(result)
 }
 
-/// Compatibility helper for callers without dedicated local-response hosts.
 pub fn for_frontends(
     settings: &Settings,
     vhosts: &[crate::vhost::Vhost],
@@ -326,12 +305,9 @@ pub fn for_frontends(
     for_all_frontends(settings, vhosts, redirects, &[])
 }
 
-/// Compatibility helper used by focused TLS tests and callers that only have vhosts.
 pub fn for_vhosts(settings: &Settings, vhosts: &[crate::vhost::Vhost]) -> Result<Settings> {
     for_frontends(settings, vhosts, &[])
 }
-/// Upload staging: files are private and immutable; configuration publication is
-/// a separate revisioned operation. Unbound files never alter active TLS state.
 pub fn upload(
     directory: &Path,
     certificate: &[u8],
@@ -358,8 +334,6 @@ pub fn upload(
     }
     crate::auth::prepare_directory(directory)?;
     let _lock = crate::auth::lock_directory(directory)?;
-    // Bound abandoned staging files. Admins may remove unreferenced upload-* pairs
-    // locally; never delete potentially active files automatically.
     let staged = fs::read_dir(directory)?
         .filter_map(|e| e.ok())
         .filter(|e| {
@@ -382,7 +356,6 @@ pub fn upload(
         hosts: Vec::new(),
     })
 }
-/// Validate immutable ACME outputs before they may be published to the library.
 pub fn validate_acme(
     directory: &Path,
     id: String,
@@ -402,8 +375,6 @@ pub fn validate_acme(
     let mut resolver = ResolvesServerCertUsingSni::new();
     for domain in domains {
         let domain = normalize_host_pattern(domain)?;
-        // A concrete name checks a wildcard SAN without relying on wildcard
-        // behavior in rustls' stock validation resolver.
         let name = domain
             .strip_prefix("*.")
             .map(|d| format!("rustproxy-acme-check.{d}"))

src/tls/external_paths.rs

@@ -52,9 +52,6 @@ fn validate_external_parent_chain(path: &Path) -> Result<()> {
             directory.display()
         );
         if mode & 0o022 != 0 {
-            // Root-owned sticky directories such as /tmp and /var/tmp cannot have
-            // entries renamed/unlinked by arbitrary users, so they are safe as an
-            // ancestor of a private service-owned test/custom directory.
             ensure!(
                 owner == 0 && mode & 0o1000 != 0,
                 "external certificate parent directory must not be group/world-writable unless it is a root-owned sticky directory: {}",
@@ -77,16 +74,9 @@ fn configured_certificate_path(directory: &Path, value: &str) -> Result<PathBuf>
             "external certificate path must be an absolute path up to 4096 bytes"
         );
         let configured = Path::new(value);
-        // The configured path itself may be Certbot's final live/*.pem symlink, but
-        // every parent directory must be a stable, non-writable directory. This
-        // prevents a writable parent from redirecting canonicalize() to attacker-
-        // controlled material between validation and open.
         validate_external_parent_chain(configured)?;
         let resolved = fs::canonicalize(configured)
             .with_context(|| format!("cannot resolve external certificate path {value}"))?;
-        // Validate the resolved archive/custom path as well. Certbot's final file
-        // symlink is intentionally followed on each reload; intermediate symlinks
-        // and group/world-writable parent directories are rejected.
         validate_external_parent_chain(&resolved)?;
         return Ok(resolved);
     }
@@ -136,12 +126,6 @@ fn read_pem_with_policy(
                 "external certificate/key must be owned by root or the RustProxy service user: {}",
                 path.display()
             );
-            // POSIX ACL masks are reflected in the traditional group mode bits.
-            // Therefore do not require an exact chmod such as 0600/0640 and do
-            // not reject harmless execute-mask bits. What matters here is that
-            // an untrusted group/other identity cannot modify the material and
-            // that the private key is never readable by "other". A named ACL
-            // such as `u:rustproxy:r--` is intentionally supported.
             ensure!(
                 mode & 0o022 == 0,
                 "external certificate/key must not be group/world-writable: {}",

src/tls/external_paths_tests.rs

@@ -15,10 +15,26 @@ fn external_absolute_paths_follow_certbot_style_symlinks_on_each_reload() {
 
     let first = rcgen::generate_simple_self_signed(vec!["example.test".into()]).unwrap();
     fs::write(archive.join("fullchain1.pem"), first.cert.pem()).unwrap();
-    fs::write(archive.join("privkey1.pem"), first.signing_key.serialize_pem()).unwrap();
-    fs::set_permissions(archive.join("privkey1.pem"), fs::Permissions::from_mode(0o650)).unwrap();
-    symlink("../../archive/example.test/fullchain1.pem", live.join("fullchain.pem")).unwrap();
-    symlink("../../archive/example.test/privkey1.pem", live.join("privkey.pem")).unwrap();
+    fs::write(
+        archive.join("privkey1.pem"),
+        first.signing_key.serialize_pem(),
+    )
+    .unwrap();
+    fs::set_permissions(
+        archive.join("privkey1.pem"),
+        fs::Permissions::from_mode(0o650),
+    )
+    .unwrap();
+    symlink(
+        "../../archive/example.test/fullchain1.pem",
+        live.join("fullchain.pem"),
+    )
+    .unwrap();
+    symlink(
+        "../../archive/example.test/privkey1.pem",
+        live.join("privkey.pem"),
+    )
+    .unwrap();
 
     let cert_path = live.join("fullchain.pem").display().to_string();
     let key_path = live.join("privkey.pem").display().to_string();
@@ -37,26 +53,44 @@ fn external_absolute_paths_follow_certbot_style_symlinks_on_each_reload() {
     };
     assert!(compile(&settings, Some(&cert_dir)).unwrap().is_some());
     assert_eq!(
-        certificate_metadata(&cert_dir, &cert_path).unwrap().covered_hosts,
+        certificate_metadata(&cert_dir, &cert_path)
+            .unwrap()
+            .covered_hosts,
         vec!["example.test"]
     );
 
-    let second = rcgen::generate_simple_self_signed(vec![
-        "example.test".into(),
-        "www.example.test".into(),
-    ])
-    .unwrap();
+    let second =
+        rcgen::generate_simple_self_signed(vec!["example.test".into(), "www.example.test".into()])
+            .unwrap();
     fs::write(archive.join("fullchain2.pem"), second.cert.pem()).unwrap();
-    fs::write(archive.join("privkey2.pem"), second.signing_key.serialize_pem()).unwrap();
-    fs::set_permissions(archive.join("privkey2.pem"), fs::Permissions::from_mode(0o640)).unwrap();
+    fs::write(
+        archive.join("privkey2.pem"),
+        second.signing_key.serialize_pem(),
+    )
+    .unwrap();
+    fs::set_permissions(
+        archive.join("privkey2.pem"),
+        fs::Permissions::from_mode(0o640),
+    )
+    .unwrap();
     fs::remove_file(live.join("fullchain.pem")).unwrap();
     fs::remove_file(live.join("privkey.pem")).unwrap();
-    symlink("../../archive/example.test/fullchain2.pem", live.join("fullchain.pem")).unwrap();
-    symlink("../../archive/example.test/privkey2.pem", live.join("privkey.pem")).unwrap();
+    symlink(
+        "../../archive/example.test/fullchain2.pem",
+        live.join("fullchain.pem"),
+    )
+    .unwrap();
+    symlink(
+        "../../archive/example.test/privkey2.pem",
+        live.join("privkey.pem"),
+    )
+    .unwrap();
 
     assert!(compile(&settings, Some(&cert_dir)).unwrap().is_some());
     assert_eq!(
-        certificate_metadata(&cert_dir, &cert_path).unwrap().covered_hosts,
+        certificate_metadata(&cert_dir, &cert_path)
+            .unwrap()
+            .covered_hosts,
         vec!["example.test", "www.example.test"]
     );
 }
@@ -70,9 +104,6 @@ fn external_key_permissions_accept_acl_style_group_mask_but_reject_unsafe_write_
     let key = root.path().join("external.key");
     fs::write(&key, b"not-a-real-key").unwrap();
 
-    // POSIX ACL masks can surface through the traditional group class bits.
-    // Group read/execute is not itself a disclosure when `other` has no read
-    // and neither group nor other can write the file.
     fs::set_permissions(&key, fs::Permissions::from_mode(0o650)).unwrap();
     assert!(read_pem_with_policy(&key, true, true).is_ok());
 

src/tls/server_config.rs

@@ -1,7 +1,3 @@
-/// Extract the DNS SubjectAltName set from the leaf certificate. This parser is
-/// deliberately narrow: it walks only the DER structures required to locate the
-/// SAN extension and ignores non-DNS GeneralName variants. Certificate validity
-/// and key matching are still delegated to rustls.
 fn certificate_dns_names(certificate: &[u8]) -> Result<Vec<String>> {
     let mut reader = certificate;
     let certs = rustls_pemfile::certs(&mut reader).collect::<io::Result<Vec<_>>>()?;
@@ -22,7 +18,6 @@ fn certificate_dns_names(certificate: &[u8]) -> Result<Vec<String>> {
     if tbs.first() == Some(&0xa0) {
         let _ = der_tlv(tbs, &mut tbs_offset)?;
     }
-    // serialNumber, signature, issuer, validity, subject, subjectPublicKeyInfo
     for _ in 0..6 {
         let _ = der_tlv(tbs, &mut tbs_offset)?;
     }
@@ -306,16 +301,11 @@ fn certificate_public_metadata(certificate: &[u8]) -> Result<CertificateMetadata
     })
 }
 
-/// Read DNS SubjectAltName coverage directly from an existing certificate file.
-/// Used by the authenticated certificate inventory to enrich older/manual entries
-/// that predate persisted `covered_hosts` metadata. Private key material is not read.
 pub fn certificate_covered_hosts(directory: &Path, cert_file: &str) -> Result<Vec<String>> {
     let certificate = read_configured_pem(directory, cert_file, false)?;
     certificate_dns_names(&certificate)
 }
 
-/// Read non-secret metadata directly from the configured public leaf certificate.
-/// Private keys are never opened by this path.
 pub fn certificate_metadata(directory: &Path, cert_file: &str) -> Result<CertificateMetadata> {
     let certificate = read_configured_pem(directory, cert_file, false)?;
     certificate_public_metadata(&certificate)
@@ -344,7 +334,6 @@ fn load_pair(directory: &Path, cert: &str, key: &str) -> Result<CertifiedKey> {
         &read_configured_pem(directory, key, true)?,
     )
 }
-/// Runs on a bounded control-plane blocking worker, never from a handshake.
 pub fn compile(settings: &Settings, directory: Option<&Path>) -> Result<Option<Arc<ServerConfig>>> {
     settings.policy.validate()?;
     let Some(directory) = directory else {
@@ -365,9 +354,6 @@ pub fn compile(settings: &Settings, directory: Option<&Path>) -> Result<Option<A
         for host in &cert.hosts {
             let host = normalize_host_pattern(host)?;
             ensure!(hosts.insert(host.clone()), "duplicate TLS hostname binding");
-            // Validate certificate/name compatibility with a concrete SNI name.
-            // rustls' stock resolver does not model routing wildcard precedence,
-            // so the immutable RustProxy resolver below owns exact/wildcard lookup.
             let validation_name = host
                 .strip_prefix("*.")
                 .map(|base| format!("rustproxy-wildcard-check.{base}"))
@@ -404,8 +390,6 @@ pub fn compile(settings: &Settings, directory: Option<&Path>) -> Result<Option<A
     )?))
 }
 
-/// Reuse the already validated immutable certificate resolver while narrowing
-/// protocols/ciphers for one frontend. No certificate I/O occurs here.
 pub fn variant(
     base: &Arc<ServerConfig>,
     policy: &ProtocolPolicy,

src/tls/tests.rs

@@ -162,7 +162,7 @@ mod tests {
         assert!(certificate_files_are_external("app.pem", "/etc/tls/app.key").is_err());
     }
 
-    include!("tests/external_paths.rs");
+    include!("external_paths_tests.rs");
 
     #[test]
     fn default_is_persistent_and_tls_advertises_both_protocols() {

src/tls/types.rs

@@ -25,7 +25,6 @@ pub struct Settings {
     #[serde(default)]
     pub certificates: Vec<Certificate>,
     pub policy: ProtocolPolicy,
-    /// Derived from explicitly confirmed Vhosts; never accepted from TLS JSON.
     #[serde(skip)]
     pub forced_mismatches: HashSet<(String, String)>,
 }
@@ -52,7 +51,6 @@ impl Settings {
 pub struct ProtocolPolicy {
     pub tls12: bool,
     pub tls13: bool,
-    /// Rustls cipher-suite names. Empty keeps the ring provider defaults.
     pub cipher_suites: Vec<String>,
 }
 impl Default for ProtocolPolicy {
@@ -119,9 +117,6 @@ impl ProtocolPolicy {
                     .iter()
                     .any(|suite| suite.starts_with("TLS13_")))
     }
-    /// Per-vhost policy may only remove protocols/suites from the global policy.
-    /// This keeps the global setting a real security ceiling rather than a default
-    /// that an individual vhost can silently widen.
     pub fn validate_narrower_than(&self, global: &Self) -> Result<()> {
         self.validate()?;
         global.validate()?;
@@ -223,11 +218,8 @@ pub struct Certificate {
     #[serde(default, skip_serializing_if = "Option::is_none")]
     pub issuer: Option<Issuer>,
     pub id: String,
-    /// DNS SubjectAltName values extracted from the leaf certificate during
-    /// upload or ACME import. This is certificate coverage, not active SNI binding.
     #[serde(default, skip_serializing_if = "Vec::is_empty")]
     pub covered_hosts: Vec<String>,
-    /// Active/configured SNI bindings. `for_vhosts` may rewrite this list.
     pub hosts: Vec<String>,
     pub cert_file: String,
     pub key_file: String,
@@ -330,10 +322,6 @@ pub fn safe_name(name: &str) -> Result<()> {
     Ok(())
 }
 
-/// Validate how a configured certificate pair is addressed. Legacy/simple names
-/// remain private to `<data-dir>/certs`; an explicitly selected external pair uses
-/// two absolute paths. Mixing the two modes is rejected so the source is never
-/// ambiguous.
 fn der_tlv<'a>(input: &'a [u8], offset: &mut usize) -> Result<(u8, &'a [u8])> {
     ensure!(*offset < input.len(), "truncated DER object");
     let tag = input[*offset];