From b9111a8e46a91efd5174b8b8912cb4c75722d53c Mon Sep 17 00:00:00 2001 From: dvdsk Date: Sat, 25 Nov 2023 12:56:44 +0100 Subject: [PATCH 1/2] now warns if a domain name is missing that was in the old certificate --- Cargo.lock | 2 +- main/src/advise.rs | 37 ++++++++++++++++++++++++------- main/src/cert/info.rs | 21 +++++++++++++++++- main/src/config.rs | 2 +- main/src/lib.rs | 7 +++++- main/tests/behaviour.rs | 41 +++++++++++++++++++++++++++++++++++ main/tests/format.rs | 9 +++++--- main/tests/shared/gen_cert.rs | 8 +++---- main/tests/shared/mod.rs | 6 +++-- 9 files changed, 112 insertions(+), 21 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 45c4453..a173304 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1488,7 +1488,7 @@ checksum = "436b050e76ed2903236f032a59761c1eb99e1b0aead2c257922771dab1fc8c78" [[package]] name = "renewc" -version = "0.3.1" +version = "0.3.2" dependencies = [ "async-trait", "axum", diff --git a/main/src/advise.rs b/main/src/advise.rs index d2eadeb..47c203a 100644 --- a/main/src/advise.rs +++ b/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 } } diff --git a/main/src/cert/info.rs b/main/src/cert/info.rs index 44e29b8..815eebd 100644 --- a/main/src/cert/info.rs +++ b/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, // 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) -> eyre::Result { .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 { + match name { + GeneralName::DNSName(s) => Some(s.to_string()), + _other => None, + } +} diff --git a/main/src/config.rs b/main/src/config.rs index 8f9e239..aa37f22 100644 --- a/main/src/config.rs +++ b/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, + pub domains: Vec, pub(crate) email: Vec, pub production: bool, pub port: u16, diff --git a/main/src/lib.rs b/main/src/lib.rs index f1561c9..77e6f9f 100644 --- a/main/src/lib.rs +++ b/main/src/lib.rs @@ -51,7 +51,9 @@ pub async fn run( 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( 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() { diff --git a/main/tests/behaviour.rs b/main/tests/behaviour.rs index 65c15ea..9dda6aa 100644 --- a/main/tests/behaviour.rs +++ b/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::(&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::(&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:#?}" + ); +} diff --git a/main/tests/format.rs b/main/tests/format.rs index e0566e7..3e4aa93 100644 --- a/main/tests/format.rs +++ b/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 = gen_cert::generate_cert_with_chain(valid_till, false); + let original: Signed = 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(); diff --git a/main/tests/shared/gen_cert.rs b/main/tests/shared/gen_cert.rs index e11ab83..3afcbc3 100644 --- a/main/tests/shared/gen_cert.rs +++ b/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( valid_till: OffsetDateTime, is_staging: bool, + domains: &[String], ) -> Signed

{ 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( .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(); diff --git a/main/tests/shared/mod.rs b/main/tests/shared/mod.rs index 8dc58e8..17213b5 100644 --- a/main/tests/shared/mod.rs +++ b/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> { - 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) } } From 6b05df52e3f47b916f9f05649080c255c2ba80c1 Mon Sep 17 00:00:00 2001 From: dvdsk Date: Sat, 25 Nov 2023 16:13:13 +0100 Subject: [PATCH 2/2] updates version, changelog and readme --- CHANGELOG.md | 4 ++++ README.md | 3 +++ main/Cargo.toml | 2 +- 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 469d9d2..5827c78 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/README.md b/README.md index 34cad49..f265ef8 100644 --- a/README.md +++ b/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. diff --git a/main/Cargo.toml b/main/Cargo.toml index eb4ce2b..5d270d9 100644 --- a/main/Cargo.toml +++ b/main/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "renewc" -version = "0.3.2" +version = "0.4.0" authors = ["David Kleingeld "] edition = "2021" rust-version = "1.70"