Skip to content

Commit

Permalink
fix(dgw): accept subject name even if it does not match the hostname
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
CBenoit committed Aug 19, 2024
1 parent 787027c commit 1f40b45
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 9 deletions.
6 changes: 1 addition & 5 deletions devolutions-gateway/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
Expand Down
15 changes: 15 additions & 0 deletions devolutions-gateway/src/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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:#}"),
Expand Down
25 changes: 21 additions & 4 deletions devolutions-gateway/src/tls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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))]
Expand Down Expand Up @@ -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,
Expand All @@ -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,
})
Expand All @@ -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"
)
}

Expand Down

0 comments on commit 1f40b45

Please sign in to comment.