fix: the four self-contained findings left from the logic-bug audit

crates/core/src/runtime/tls/store.rs

@@ -335,7 +335,7 @@ pub fn list_certificates(db: &Db) -> rusqlite::Result<Vec<TlsCertificate>> {
     stmt.query_map([], row_to_certificate)?.collect()
 }
 
-/// Returns the most-recent active cert covering `hostname`, if any.
+/// Returns the most-recent unexpired active cert covering `hostname`, if any.
 ///
 /// Resolution rules:
 ///
@@ -347,11 +347,25 @@ pub fn list_certificates(db: &Db) -> rusqlite::Result<Vec<TlsCertificate>> {
 ///   (literal match or single-label wildcard). This auto-binds manual
 ///   uploads — including wildcard certs — without requiring the
 ///   operator to re-declare the binding per host.
+///
+/// Both paths skip rows whose `not_after` has passed. Without that the
+/// exact-hostname fast path returned an expired row in preference to a
+/// still-valid cert that covered the same hostname by SAN, so the proxy
+/// served the expired one and nothing on the serve path noticed. A row with
+/// no recorded `not_after` is kept: unparsed and expired are not the same
+/// thing, and treating them alike would stop serving a usable cert.
+///
+/// The sibling matcher in [`super::state`] deliberately does *not* filter on
+/// expiry — the renewal scheduler has to see an expiring cert in order to
+/// renew it — so the two are not interchangeable despite the shared rules.
 // r[impl tls.strategy.manual]
+// r[impl tls.cert.serve]
 pub fn find_active_for_hostname(
     db: &Db,
     hostname: &str,
 ) -> rusqlite::Result<Option<TlsCertificate>> {
+    let now = now_secs();
+
     if let Some(cert) = db
         .conn
         .query_row(
@@ -361,8 +375,9 @@ pub fn find_active_for_hostname(
                     ari_polled_at, created_at, updated_at
              FROM tls_certificates
              WHERE hostname = ?1 AND state = 'active'
+               AND (not_after IS NULL OR not_after > ?2)
              ORDER BY id DESC LIMIT 1",
-            [hostname],
+            rusqlite::params![hostname, now],
             row_to_certificate,
         )
         .optional()?
@@ -381,9 +396,10 @@ pub fn find_active_for_hostname(
                 ari_polled_at, created_at, updated_at
          FROM tls_certificates
          WHERE state = 'active'
+           AND (not_after IS NULL OR not_after > ?1)
          ORDER BY created_at DESC, id DESC",
     )?;
-    let mut rows = stmt.query([])?;
+    let mut rows = stmt.query([now])?;
     while let Some(row) = rows.next()? {
         let cert = row_to_certificate(row)?;
         let Some(pem) = cert.cert_pem.as_deref() else {
@@ -1075,6 +1091,31 @@ mod tests {
         assert!(!row.self_signed);
     }
 
+    /// Like [`insert_test_cert`] but with an explicit expiry, so a row can be
+    /// placed either side of "now".
+    fn insert_test_cert_expiring(db: &Db, hostname: &str, not_after: Option<i64>) -> i64 {
+        insert_certificate(
+            db,
+            hostname,
+            TlsCertState::Active,
+            TlsCertOrigin::Manual,
+            Some("-----BEGIN CERTIFICATE-----\nMIIBdummy\n-----END CERTIFICATE-----\n"),
+            None,
+            b"encrypted-key-bytes",
+            KeyType::EcdsaP256,
+            CertMetadata {
+                issuer: Some("CN=Test CA".to_string()),
+                not_before: Some(1_600_000_000),
+                not_after,
+                serial: Some("01".to_string()),
+                self_signed: false,
+            },
+            None,
+            None,
+        )
+        .unwrap()
+    }
+
     #[test]
     fn find_active_for_hostname_returns_latest() {
         let (db, _) = fresh_db();
@@ -1088,6 +1129,51 @@ mod tests {
         assert_eq!(found.id, id2);
     }
 
+    // r[verify tls.cert.serve]
+    #[test]
+    fn find_active_for_hostname_skips_an_expired_cert() {
+        let (db, _) = fresh_db();
+        insert_test_cert_expiring(&db, "a.example.com", Some(now_secs() - 1));
+
+        assert!(
+            find_active_for_hostname(&db, "a.example.com")
+                .unwrap()
+                .is_none(),
+            "an expired cert must not be served",
+        );
+    }
+
+    // r[verify tls.cert.serve]
+    // The exact-hostname path takes the highest id, so a newer expired row
+    // shadowed an older one that was still valid — and the proxy served the
+    // expired cert with nothing on the path noticing.
+    #[test]
+    fn an_expired_cert_does_not_shadow_a_valid_one_for_the_same_hostname() {
+        let (db, _) = fresh_db();
+        let valid = insert_test_cert_expiring(&db, "a.example.com", Some(now_secs() + 86_400));
+        let expired = insert_test_cert_expiring(&db, "a.example.com", Some(now_secs() - 1));
+        assert!(expired > valid, "the expired row must be the newer one");
+
+        let found = find_active_for_hostname(&db, "a.example.com")
+            .unwrap()
+            .expect("the valid cert is still servable");
+        assert_eq!(found.id, valid);
+    }
+
+    // r[verify tls.cert.serve]
+    // Unparsed and expired are different things; refusing to serve a row whose
+    // expiry was never recorded would withhold a usable cert.
+    #[test]
+    fn a_cert_with_no_recorded_expiry_is_still_served() {
+        let (db, _) = fresh_db();
+        let id = insert_test_cert_expiring(&db, "a.example.com", None);
+
+        let found = find_active_for_hostname(&db, "a.example.com")
+            .unwrap()
+            .expect("an unrecorded expiry is not an expiry");
+        assert_eq!(found.id, id);
+    }
+
     #[test]
     fn supersede_other_active_only_touches_target_hostname() {
         let (db, _) = fresh_db();

crates/daemon/src/main.rs

@@ -1639,26 +1639,138 @@ fn resolve_oi_addrs(
     explicit: &[std::net::SocketAddr],
     port: u16,
 ) -> Vec<std::net::SocketAddr> {
+    let available = if interfaces.is_empty() {
+        Vec::new()
+    } else {
+        if_addrs::get_if_addrs()
+            .unwrap_or_else(|e| fatal!("failed to list network interfaces: {e}"))
+    };
+    select_oi_addrs(interfaces, explicit, port, &available)
+        .unwrap_or_else(|e| fatal!("cannot resolve OI listen addresses: {e}"))
+}
+
+/// Decide which addresses the OI server binds, given the interfaces the host
+/// currently reports.
+///
+/// Split from [`resolve_oi_addrs`] so the decision is testable without
+/// touching the host's interface list or exiting the process.
+// i[impl transport.listen]
+fn select_oi_addrs(
+    interfaces: &[String],
+    explicit: &[std::net::SocketAddr],
+    port: u16,
+    available: &[if_addrs::Interface],
+) -> Result<Vec<std::net::SocketAddr>, String> {
     if interfaces.is_empty() && explicit.is_empty() {
-        return vec![format!("[::1]:{port}").parse().unwrap()];
+        return Ok(vec![format!("[::1]:{port}").parse().unwrap()]);
     }
 
     let mut addrs: Vec<std::net::SocketAddr> = explicit.to_vec();
 
     for iface_name in interfaces {
-        let all = if_addrs::get_if_addrs()
-            .unwrap_or_else(|e| fatal!("failed to list network interfaces: {e}"));
-        let iface_addrs: Vec<_> = all
-            .into_iter()
+        let iface_addrs: Vec<_> = available
+            .iter()
             .filter(|i| &i.name == iface_name)
             .map(|i| std::net::SocketAddr::new(i.ip(), port))
             .collect();
+        // An interface that is not up yet looks exactly like one that
+        // resolved to nothing. Continuing here bound the subset that did
+        // resolve — or nothing at all, when this was the only interface — and
+        // the daemon then logged "seedling ready" with no management plane.
         if iface_addrs.is_empty() {
-            tracing::warn!("interface {iface_name:?} not found or has no addresses");
-            continue;
+            return Err(format!(
+                "interface {iface_name:?} not found or has no addresses"
+            ));
         }
         addrs.extend(iface_addrs);
     }
 
-    addrs
+    Ok(addrs)
+}
+
+#[cfg(test)]
+mod tests {
+    use std::net::{Ipv4Addr, SocketAddr};
+
+    use if_addrs::{IfAddr, IfOperStatus, Ifv4Addr, Interface};
+
+    use super::select_oi_addrs;
+
+    fn iface(name: &str, ip: [u8; 4]) -> Interface {
+        Interface {
+            name: name.to_owned(),
+            addr: IfAddr::V4(Ifv4Addr {
+                ip: Ipv4Addr::from(ip),
+                netmask: Ipv4Addr::new(255, 255, 255, 0),
+                prefixlen: 24,
+                broadcast: None,
+            }),
+            index: None,
+            oper_status: IfOperStatus::Up,
+            is_p2p: false,
+            #[cfg(windows)]
+            adapter_name: String::new(),
+        }
+    }
+
+    // i[verify transport.listen]
+    #[test]
+    fn no_configuration_listens_on_loopback() {
+        let addrs = select_oi_addrs(&[], &[], 7891, &[]).expect("default");
+        assert_eq!(addrs, vec!["[::1]:7891".parse::<SocketAddr>().unwrap()]);
+    }
+
+    // i[verify transport.listen]
+    #[test]
+    fn a_named_interface_contributes_its_addresses() {
+        let available = [iface("eth0", [10, 0, 0, 5])];
+        let addrs =
+            select_oi_addrs(&["eth0".to_owned()], &[], 7891, &available).expect("resolve eth0");
+        assert_eq!(addrs, vec!["10.0.0.5:7891".parse::<SocketAddr>().unwrap()]);
+    }
+
+    // i[verify transport.listen]
+    // An interface that has not come up yet is indistinguishable from one that
+    // resolved to nothing, so it must not be silently skipped: doing so left
+    // the daemon reporting ready with no OI listener at all.
+    #[test]
+    fn an_unresolvable_interface_is_an_error_rather_than_no_listener() {
+        let err = select_oi_addrs(&["eth0".to_owned()], &[], 7891, &[])
+            .expect_err("must not resolve to an empty listen set");
+        assert!(
+            err.contains("eth0"),
+            "error should name the interface: {err}"
+        );
+    }
+
+    // i[verify transport.listen]
+    // The same applies when another interface did resolve: binding the subset
+    // silently drops whichever address the operator asked for.
+    #[test]
+    fn one_unresolvable_interface_fails_the_whole_set() {
+        let available = [iface("eth0", [10, 0, 0, 5])];
+        select_oi_addrs(
+            &["eth0".to_owned(), "eth1".to_owned()],
+            &[],
+            7891,
+            &available,
+        )
+        .expect_err("a partial resolution must not bind the subset");
+    }
+
+    // i[verify transport.listen]
+    #[test]
+    fn explicit_addresses_are_kept_alongside_interface_addresses() {
+        let available = [iface("eth0", [10, 0, 0, 5])];
+        let explicit: Vec<SocketAddr> = vec!["127.0.0.1:9000".parse().unwrap()];
+        let addrs = select_oi_addrs(&["eth0".to_owned()], &explicit, 7891, &available)
+            .expect("both sources");
+        assert_eq!(
+            addrs,
+            vec![
+                "127.0.0.1:9000".parse::<SocketAddr>().unwrap(),
+                "10.0.0.5:7891".parse::<SocketAddr>().unwrap(),
+            ]
+        );
+    }
 }

crates/web/src/main.rs

@@ -190,6 +190,31 @@ async fn connect_daemon_with_retry(
 /// interfaces/addresses when either is configured, otherwise the shared HTTP
 /// set. Lets a deployment bind WebTransport somewhere an HTTP reverse proxy
 /// can't reach (e.g. the tailnet) while HTTP stays on loopback.
+/// The port to advertise to clients, in `wt_url` and in the CSP's
+/// `connect-src` origin.
+///
+/// Taken from the addresses actually bound rather than from `--wt-port`:
+/// with explicit `--wt-listen` (or `--listen`) addresses that argument is not
+/// the port anything listens on, and substituting the default made every
+/// documented explicit-address combination advertise an unusable endpoint.
+///
+/// One port is advertised, so addresses that disagree on it are rejected
+/// rather than having one of them silently picked.
+// w[impl auth.connect]
+fn advertised_wt_port(addrs: &[SocketAddr]) -> Result<u16, String> {
+    let mut ports = addrs.iter().map(SocketAddr::port);
+    let first = ports
+        .next()
+        .ok_or_else(|| "no WebTransport listen addresses were resolved".to_owned())?;
+    if let Some(other) = ports.find(|p| *p != first) {
+        return Err(format!(
+            "WebTransport listen addresses disagree on the port ({first} and {other}); \
+             a single port is advertised to clients, so they must match"
+        ));
+    }
+    Ok(first)
+}
+
 fn wt_bind_sources<'a>(
     interface: &'a [String],
     listen: &'a [SocketAddr],
@@ -258,12 +283,18 @@ async fn main() {
         &args.wt_interface,
         &args.wt_listen,
     );
-    let wt_port = if wt_listen.is_empty() {
+    // Only interface-derived addresses take this port; explicit addresses
+    // carry their own.
+    let wt_bind_port = if wt_listen.is_empty() {
         args.wt_port
     } else {
         DEFAULT_WT_PORT
     };
-    let wt_addrs = resolve_bind_addrs(wt_interface, wt_listen, wt_port).unwrap_or_else(|e| {
+    let wt_addrs = resolve_bind_addrs(wt_interface, wt_listen, wt_bind_port).unwrap_or_else(|e| {
+        eprintln!("error: {e}");
+        std::process::exit(1);
+    });
+    let wt_port = advertised_wt_port(&wt_addrs).unwrap_or_else(|e| {
         eprintln!("error: {e}");
         std::process::exit(1);
     });
@@ -446,4 +477,43 @@ mod daemon_pin_tests {
         let pin = DaemonPin::Fixed(ClientAuth::TrustAny);
         assert!(matches!(pin.resolve(), Some(ClientAuth::TrustAny)));
     }
+
+    // w[verify auth.connect]
+    // With explicit listen addresses the `--wt-port` argument is not the port
+    // anything is bound to; advertising the default in its place made every
+    // documented explicit-address combination hand clients an unusable URL.
+    #[test]
+    fn the_advertised_port_comes_from_the_bound_address() {
+        let addrs: Vec<SocketAddr> = vec!["10.0.0.5:9999".parse().unwrap()];
+        assert_eq!(advertised_wt_port(&addrs), Ok(9999));
+    }
+
+    // w[verify auth.connect]
+    #[test]
+    fn matching_ports_across_several_addresses_are_advertised_once() {
+        let addrs: Vec<SocketAddr> = vec![
+            "10.0.0.5:9999".parse().unwrap(),
+            "[::1]:9999".parse().unwrap(),
+        ];
+        assert_eq!(advertised_wt_port(&addrs), Ok(9999));
+    }
+
+    // w[verify auth.connect]
+    // Only one port reaches the client, so picking one of several would leave
+    // the rest advertised as something they are not.
+    #[test]
+    fn addresses_disagreeing_on_the_port_are_rejected() {
+        let addrs: Vec<SocketAddr> = vec![
+            "10.0.0.5:9999".parse().unwrap(),
+            "[::1]:8888".parse().unwrap(),
+        ];
+        let err = advertised_wt_port(&addrs).expect_err("mixed ports must not resolve");
+        assert!(err.contains("9999") && err.contains("8888"), "{err}");
+    }
+
+    // w[verify auth.connect]
+    #[test]
+    fn an_empty_address_set_has_no_port_to_advertise() {
+        advertised_wt_port(&[]).expect_err("nothing bound means nothing to advertise");
+    }
 }

crates/web/src/wt.rs

@@ -375,7 +375,7 @@ pub async fn run_cert_rotation(
     rotation_tx: watch::Sender<()>,
 ) {
     loop {
-        tokio::time::sleep(std::time::Duration::from_secs(3600)).await;
+        tokio::time::sleep(crate::wt_cert::ROTATION_TICK).await;
         let rotated = cert_store.write().rotate_if_needed();
         if rotated {
             let _ = rotation_tx.send(());