Merge pull request #79 from dvdsk/warn_if_name_missing

adds diagnostic checking for missing domain names

CHANGELOG.md

@@ -5,6 +5,10 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## [0.4.0] - 2023-11-25
+
+### Changed
+ - Asks the use to confirm the request if the new certificate would miss domains that are valid with the current certificate. Rejects renewal if not running interactively (such as from a shell command).
 
 ## [0.3.2] - 2023-11-07
 

README.md

@@ -34,6 +34,9 @@ Currently *renewc* can investigate and advise these situations:
 *looks at HAProxy's configs and tells you what port to use instead*
  - Using a port below 1025 without sudo:  
 *advices to call *renewc* using sudo*
+ - When the new certificate would miss domains valid in a current certificate:  
+*warns user and blocks for input*
+
 
 We hope to expand this list in the near future, PRs are welcome.
 

main/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "renewc"
-version = "0.3.2"
+version = "0.4.0"
 authors = ["David Kleingeld <[email protected]>"]
 edition = "2021"
 rust-version = "1.70"

main/src/advise.rs

@@ -1,6 +1,8 @@
+use std::collections::HashSet;
 use std::io::Write;
 
 use cert::info::Info;
+use itertools::Itertools;
 
 use crate::{cert, Config};
 
@@ -38,6 +40,9 @@ pub enum CheckResult {
         status: String,
     },
     NoCert,
+    Warn {
+        warning: &'static str,
+    },
 }
 
 impl CheckResult {
@@ -71,20 +76,36 @@ pub fn given_existing(
         return CheckResult::NoCert;
     };
 
+    let new_domains: HashSet<_> = config.domains.iter().collect();
+    let prev_domains: HashSet<_> = cert.names.iter().collect();
+    let missing = prev_domains.difference(&new_domains).map(|s| s.as_str());
+    let n_missing = missing.clone().count();
+    let missing: String = Itertools::intersperse_with(missing, || "\n\t-").collect();
+
+    if !missing.is_empty() {
+        let question = if n_missing == 1 {
+            format!("Certificate will not be valid for (sub)domain that is currently valid, that (sub)domain is: {missing}")
+        } else {
+            format!("Certificate will not be valid for (sub)domains that are currently valid, these are:\n{missing}")
+        };
+        if exit_requested(stdout, config, &question) {
+            return CheckResult::refuse_without_status("Not renewing, while domains are missing");
+        }
+    }
+
     match (config.production, cert.staging, cert.should_renew()) {
         (false, true, _) => {
-            CheckResult::accept( "Requesting staging cert, certificates will not be valid")
+            CheckResult::accept("Requesting staging cert, certificates will not be valid")
         }
         (false, false, _) if cert.is_expired() => {
-            CheckResult::accept( "Requesting staging cert. Overwriting expired production certificate. Certificate will not be valid")
+            CheckResult::accept("Requesting staging cert. Overwriting expired production certificate. Certificate will not be valid")
         }
         (false, false, _) => {
             let question = "Found still valid production cert, continuing will overwrite it with a staging certificate";
             if !config.overwrite_production && exit_requested(stdout, config, question) {
                 return CheckResult::refuse_without_status("Not overwriting valid production cert");
             }
-            CheckResult::accept (                 "Requesting Staging cert, certificates will not be valid"
-                           )
+            CheckResult::accept ("Requesting Staging cert, certificates will not be valid")
         }
         (true, true, _) => {
             CheckResult::accept("Requesting production cert, existing certificate is staging")
@@ -94,11 +115,11 @@ pub fn given_existing(
                 CheckResult::Accept{ status: format!(
                     "Renewing production cert: existing certificate expired {} days, {} hours ago",
                     cert.since_expired().whole_days(),
-                    cert.since_expired().whole_hours())}
+                    cert.since_expired().whole_hours() % 24)}
             } else {
                 let status = format!("Renewing production cert: existing certificate expires soon: {} days, {} hours", 
                   cert.expires_in.whole_days(),
-                  cert.expires_in.whole_hours());
+                  cert.expires_in.whole_hours() % 24);
 
                 CheckResult::accept(status)
             }
@@ -112,7 +133,7 @@ pub fn given_existing(
             if config.renew_early {
                 CheckResult::accept(status)
             } else {
-                CheckResult::refuse(status, "Quiting, you can force renewal using --renew-early")
+                CheckResult::refuse(status, "Quitting, you can force renewal using --renew-early")
             }
         }
     }
@@ -134,7 +155,7 @@ fn exit_requested(w: &mut impl Write, config: &Config, question: &str) -> bool {
     if let Some('y') = buf.chars().next() {
         false
     } else {
-        info!(w, "Quiting, user requested exit");
+        info!(w, "Quitting, user requested exit");
         true
     }
 }

main/src/cert/info.rs

@@ -8,12 +8,13 @@ use color_eyre::eyre;
 use rand::{self, Rng, SeedableRng};
 use time::Duration;
 use tracing::instrument;
-use x509_parser::prelude::Pem;
+use x509_parser::prelude::{GeneralName, Pem};
 
 #[derive(Debug, PartialEq, Eq)]
 pub struct Info {
     pub staging: bool,
     pub expires_in: Duration,
+    pub names: Vec<String>,
     // unix timestamp of expiration time
     // used to seed rng such that each randomness
     // only changes with a renewed certificate
@@ -80,10 +81,28 @@ pub fn analyze(signed: Signed<impl PemItem>) -> eyre::Result<Info> {
         .timestamp()
         .try_into()
         .expect("got negative timestamp from x509 certificate, this is a bug");
+    let names = cert
+        .subject_alternative_name()?
+        .map(|s| {
+            s.value
+                .general_names
+                .iter()
+                .filter_map(unwrap_dns)
+                .collect()
+        })
+        .unwrap_or_default();
 
     Ok(Info {
         staging,
         expires_in,
         seed: expires_at,
+        names,
     })
 }
+
+fn unwrap_dns(name: &GeneralName) -> Option<String> {
+    match name {
+        GeneralName::DNSName(s) => Some(s.to_string()),
+        _other => None,
+    }
+}

main/src/config.rs

@@ -115,7 +115,7 @@ impl OutputConfig {
 #[allow(clippy::struct_excessive_bools)]
 #[derive(Debug, Clone)]
 pub struct Config {
-    pub(crate) domains: Vec<String>,
+    pub domains: Vec<String>,
     pub(crate) email: Vec<String>,
     pub production: bool,
     pub port: u16,

main/src/lib.rs

@@ -51,7 +51,9 @@ pub async fn run<P: PemItem>(
         return Ok(Some(signed));
     }
 
-    match CertInfo::from_disk(config, stdout).map(|cert| advise::given_existing(config, &cert, stdout)) {
+    match CertInfo::from_disk(config, stdout)
+        .map(|cert| advise::given_existing(config, &cert, stdout))
+    {
         Ok(CheckResult::Refuse {
             status: Some(status),
             warning,
@@ -71,6 +73,9 @@ pub async fn run<P: PemItem>(
             info(stdout, &status);
         }
         Ok(CheckResult::NoCert) => (),
+        Ok(CheckResult::Warn { warning }) => {
+            warn(stdout, warning)
+        }
         Err(e) => {
             writeln!(stdout, "Warning: renew advise impossible").unwrap();
             for (i, err) in e.chain().enumerate() {

main/tests/behaviour.rs

@@ -158,3 +158,44 @@ async fn corrupt_existing_does_not_crash() {
         "stdout did not start with:\n\t{text:#?}\ninstead it was:\n\t{output:#?}"
     );
 }
+
+#[tokio::test]
+async fn warn_about_missing_name() {
+    shared::setup_color_eyre();
+    shared::setup_tracing();
+
+    let dir = tempfile::tempdir().unwrap();
+    let mut acme = TestAcme::new(gen_cert::expired());
+
+    let mut config = Config::test(42, &dir.path().join("test_cert"));
+    config.output_config.output = Output::Pem;
+    config.domains = ["example.org", "subdomain.example.org", "other.domain"]
+        .into_iter()
+        .map(str::to_string)
+        .collect();
+
+    // run to place expired cert
+    let certs = run::<Pem>(&mut acme, &mut TestPrinter, &config, true)
+        .await
+        .unwrap()
+        .unwrap();
+    cert::store::on_disk(&config, certs, &mut TestPrinter).unwrap();
+
+    let mut acme = TestAcme::new(gen_cert::valid());
+    config.domains = vec![String::from("example.org"), String::from("other.domain")];
+    let mut output = Vec::new();
+    let _cert = run::<Pem>(&mut acme, &mut output, &config, true)
+        .await
+        .unwrap();
+
+    let output = String::from_utf8(output).unwrap();
+    let text = format!(
+        "{}",
+        "Certificate will not be valid for (sub)domain that is currently valid, that (sub)domain is: subdomain.example.org".green()
+    );
+    let start = &text[..text.len() - 5]; // remove color end char
+    assert!(
+        dbg!(&output).starts_with(dbg!(start)),
+        "stdout did not start with:\n\t{start:#?}\ninstead it was:\n\t{output:#?}"
+    );
+}

main/tests/format.rs

@@ -16,7 +16,11 @@ async fn der_and_pem_equal() {
     let dir = tempfile::tempdir().unwrap();
 
     let valid_till = OffsetDateTime::now_utc();
-    let original: Signed<Pem> = gen_cert::generate_cert_with_chain(valid_till, false);
+    let original: Signed<Pem> = gen_cert::generate_cert_with_chain(
+        valid_till,
+        false,
+        &vec![String::from("testdomain.org")],
+    );
 
     let mut config = Config::test(42, &dir.path());
     config.production = false;
@@ -27,8 +31,7 @@ async fn der_and_pem_equal() {
         Output::PemSeperateChain,
         Output::PemAllSeperate,
         Output::Der,
-    ]
-    {
+    ] {
         config.output_config.output = dbg!(format);
         store::on_disk(&config, original.clone(), &mut TestPrinter).unwrap();
         let loaded = load::from_disk(&config, &mut TestPrinter).unwrap().unwrap();

main/tests/shared/gen_cert.rs

@@ -20,9 +20,8 @@ fn ca_cert(is_staging: bool) -> Certificate {
     Certificate::from_params(params).unwrap()
 }
 
-pub fn client_cert(valid_till: OffsetDateTime) -> Certificate {
-    let subject_alt_names = vec!["example.org".to_string()];
-    let mut params = CertificateParams::new(subject_alt_names);
+pub fn client_cert(valid_till: OffsetDateTime, domains: &[String]) -> Certificate {
+    let mut params = CertificateParams::new(domains);
     params.not_after = valid_till;
     Certificate::from_params(params).unwrap()
 }
@@ -32,6 +31,7 @@ pub fn client_cert(valid_till: OffsetDateTime) -> Certificate {
 pub fn generate_cert_with_chain<P: PemItem>(
     valid_till: OffsetDateTime,
     is_staging: bool,
+    domains: &[String],
 ) -> Signed<P> {
     let root_ca_cert = ca_cert(is_staging);
     let root_ca = root_ca_cert.serialize_pem().unwrap();
@@ -41,7 +41,7 @@ pub fn generate_cert_with_chain<P: PemItem>(
         .serialize_pem_with_signer(&root_ca_cert)
         .unwrap();
 
-    let client = client_cert(valid_till);
+    let client = client_cert(valid_till, domains);
     let client_cert = client
         .serialize_pem_with_signer(&intermediate_ca_cert)
         .unwrap();

main/tests/shared/mod.rs

@@ -16,7 +16,9 @@ pub struct TestAcme {
 
 impl TestAcme {
     pub fn new(cert_expires: OffsetDateTime) -> Self {
-        Self { cert_expires }
+        Self {
+            cert_expires,
+        }
     }
 }
 
@@ -28,7 +30,7 @@ impl renewc::ACME for TestAcme {
         _stdout: &mut W,
         _debug: bool,
     ) -> eyre::Result<Signed<P>> {
-        let combined = generate_cert_with_chain(self.cert_expires, !config.production);
+        let combined = generate_cert_with_chain(self.cert_expires, !config.production, &config.domains);
         Ok(combined)
     }
 }