From 1f40b45baed4c4aca71344de30d826b354029086 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= <bcortier@proton.me>
Date: Mon, 19 Aug 2024 20:24:08 +0200
Subject: [PATCH] fix(dgw): accept subject name even if it does not match the
 hostname

Configurations where the certificate subject name does not match the
hostname are now accepted.
Instead, a few warning and debug log records are added to help
discover configuration issues in case of problem.
The problem with the strict approach we had previously is that we
may reject valid configurations where the hostname was actually
matched by one of the subject alternative names in the certificate.
---
 devolutions-gateway/src/config.rs  |  6 +-----
 devolutions-gateway/src/service.rs | 15 +++++++++++++++
 devolutions-gateway/src/tls.rs     | 25 +++++++++++++++++++++----
 3 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/devolutions-gateway/src/config.rs b/devolutions-gateway/src/config.rs
index d8ed2d76..7563a05e 100644
--- a/devolutions-gateway/src/config.rs
+++ b/devolutions-gateway/src/config.rs
@@ -190,11 +190,6 @@ impl Conf {
                     .clone()
                     .context("TLS usage implied, but TLS certificate subject name is missing")?;
 
-                anyhow::ensure!(
-                    crate::utils::wildcard_host_match(&cert_subject_name, &hostname),
-                    "hostname doesn’t match the TLS certificate subject name configured",
-                );
-
                 let store_location = conf_file.tls_certificate_store_location.unwrap_or_default();
 
                 let store_name = conf_file
@@ -203,6 +198,7 @@ impl Conf {
                     .unwrap_or_else(|| String::from("My"));
 
                 let cert_source = crate::tls::CertificateSource::SystemStore {
+                    machine_hostname: hostname.clone(),
                     cert_subject_name,
                     store_location,
                     store_name,
diff --git a/devolutions-gateway/src/service.rs b/devolutions-gateway/src/service.rs
index 738f37c3..50d1f16a 100644
--- a/devolutions-gateway/src/service.rs
+++ b/devolutions-gateway/src/service.rs
@@ -66,6 +66,21 @@ impl GatewayService {
             );
         }
 
+        if let Some((cert_subject_name, hostname)) = conf_file
+            .tls_certificate_subject_name
+            .as_deref()
+            .zip(conf_file.hostname.as_deref())
+        {
+            if !devolutions_gateway::utils::wildcard_host_match(cert_subject_name, hostname) {
+                warn!(
+                    %hostname,
+                    %cert_subject_name,
+                    "Hostname doesn’t match the TLS certificate subject name configured; \
+                    not necessarily a problem if it is instead matched by an alternative subject name"
+                )
+            }
+        }
+
         if let Err(error) = devolutions_gateway::tls::sanity::check_default_configuration() {
             warn!(
                 error = format!("{error:#}"),
diff --git a/devolutions-gateway/src/tls.rs b/devolutions-gateway/src/tls.rs
index 9686fa93..6ab4ca91 100644
--- a/devolutions-gateway/src/tls.rs
+++ b/devolutions-gateway/src/tls.rs
@@ -58,6 +58,8 @@ pub enum CertificateSource {
         private_key: pki_types::PrivateKeyDer<'static>,
     },
     SystemStore {
+        /// This field is only used to diagnostic potential configuration problems.
+        machine_hostname: String,
         cert_subject_name: String,
         store_location: crate::config::dto::CertStoreLocation,
         store_name: String,
@@ -77,12 +79,14 @@ pub fn build_server_config(cert_source: CertificateSource) -> anyhow::Result<rus
 
         #[cfg(windows)]
         CertificateSource::SystemStore {
+            machine_hostname,
             cert_subject_name,
             store_location,
             store_name,
         } => {
-            let resolver = windows::ServerCertResolver::new(cert_subject_name, store_location, &store_name)
-                .context("create ServerCertResolver")?;
+            let resolver =
+                windows::ServerCertResolver::new(machine_hostname, cert_subject_name, store_location, &store_name)
+                    .context("create ServerCertResolver")?;
             Ok(builder.with_cert_resolver(Arc::new(resolver)))
         }
         #[cfg(not(windows))]
@@ -114,12 +118,14 @@ pub mod windows {
 
     #[derive(Debug)]
     pub struct ServerCertResolver {
+        machine_hostname: String,
         subject_name: String,
         store: CertStore,
     }
 
     impl ServerCertResolver {
         pub fn new(
+            machine_hostname: String,
             cert_subject_name: String,
             store_type: dto::CertStoreLocation,
             store_name: &str,
@@ -133,6 +139,7 @@ pub mod windows {
             let store = CertStore::open(store_type, store_name).context("open Windows certificate store")?;
 
             Ok(Self {
+                machine_hostname,
                 subject_name: cert_subject_name,
                 store,
             })
@@ -145,11 +152,21 @@ pub mod windows {
                 .server_name()
                 .context("server name missing from ClientHello")?;
 
-            if !crate::utils::wildcard_host_match(&self.subject_name, request_server_name) {
+            // Sanity check.
+            if !request_server_name.eq_ignore_ascii_case(&self.machine_hostname) {
                 warn!(
+                    request_server_name,
+                    machine_hostname = self.machine_hostname,
+                    "Requested server name does not match the hostname"
+                );
+            }
+
+            // Sanity check.
+            if !crate::utils::wildcard_host_match(&self.subject_name, request_server_name) {
+                debug!(
                     request_server_name,
                     expected_server_name = self.subject_name,
-                    "Subject name mismatch"
+                    "Subject name mismatch; not necessarily a problem if it is instead matched by an alternative subject name"
                 )
             }