From 078bda7b86b55e8017077b8e2490dede1f8703dc Mon Sep 17 00:00:00 2001 From: Mike Yagley Date: Mon, 4 Feb 2019 13:30:32 -0800 Subject: [PATCH] Fix invalid DNS SAN entries (#795) (#802) * Fix invalid DNS SAN entries (#795) Changes here fix a situation where a edge device's host name that begins with number(s) [0-9] gets sanitized. For example host name "2019edgehost" is consumed as "edgehost". This has caused problems was observed when using VMs that begin with numbers since it appears to be permitted configuration contrary to RFC 1035. The changes involve passing the configured host name as is into the SAN entry without any modifications. The module id DNS entry continues to be sanitized. * Revert "Fix invalid DNS SAN entries (#795)" This reverts commit a8148ccc46749252769ef85c488d9788599968a4. * prepare_dns_san_entries was allowing characters that are not A-Za-z0-9 (#736) * Fix invalid DNS SAN entries (#795) Changes here fix a situation where a edge device's host name that begins with number(s) [0-9] gets sanitized. For example host name "2019edgehost" is consumed as "edgehost". This has caused problems was observed when using VMs that begin with numbers since it appears to be permitted configuration contrary to RFC 1035. The changes involve passing the configured host name as is into the SAN entry without any modifications. The module id DNS entry continues to be sanitized. --- .../src/server/cert/server.rs | 21 ++-- edgelet/edgelet-utils/src/lib.rs | 100 +++++++++++++++--- 2 files changed, 99 insertions(+), 22 deletions(-) diff --git a/edgelet/edgelet-http-workload/src/server/cert/server.rs b/edgelet/edgelet-http-workload/src/server/cert/server.rs index 11f67aedb9e..03fade379e1 100755 --- a/edgelet/edgelet-http-workload/src/server/cert/server.rs +++ b/edgelet/edgelet-http-workload/src/server/cert/server.rs @@ -11,7 +11,9 @@ use edgelet_core::{ }; use edgelet_http::route::{Handler, Parameters}; use edgelet_http::Error as HttpError; -use edgelet_utils::{ensure_not_empty_with_context, prepare_dns_san_entries}; +use edgelet_utils::{ + append_dns_san_entries, ensure_not_empty_with_context, prepare_dns_san_entries, +}; use workload::models::ServerCertificateRequest; use error::{CertOperation, Error, ErrorKind}; @@ -87,7 +89,10 @@ where // an alternative DNS name; we also need to add the common_name that we are using // as a DNS name since the presence of a DNS name SAN will take precedence over // the common name - let sans = vec![prepare_dns_san_entries(&[&module_id, common_name])]; + let sans = vec![append_dns_san_entries( + &prepare_dns_san_entries(&[&module_id]), + &[common_name], + )]; #[cfg_attr(feature = "cargo-clippy", allow(cast_sign_loss))] let props = CertificateProperties::new( @@ -546,12 +551,12 @@ mod tests { fn succeeds_key() { let handler = ServerCertHandler::new( TestHsm::default().with_on_create(|props| { - assert_eq!("marvin", props.common_name()); - assert_eq!("beeblebroxIserver", props.alias()); + assert_eq!("2020marvin", props.common_name()); + assert_eq!("$beeblebroxIserver", props.alias()); assert_eq!(CertificateType::Server, *props.certificate_type()); let san_entries = props.san_entries().unwrap(); assert_eq!(1, san_entries.len()); - assert_eq!("DNS:beeblebrox, DNS:marvin", san_entries[0]); + assert_eq!("DNS:2020marvin, DNS:beeblebrox", san_entries[0]); assert!(MAX_DURATION_SEC >= *props.validity_in_secs()); Ok(TestCert::default() .with_private_key(PrivateKey::Key(KeyBytes::Pem("Betelgeuse".to_string())))) @@ -560,17 +565,17 @@ mod tests { ); let cert_req = ServerCertificateRequest::new( - "marvin".to_string(), + "2020marvin".to_string(), (Utc::now() + Duration::hours(1)).to_rfc3339(), ); let request = - Request::get("http://localhost/modules/beeblebrox/genid/I/certificate/server") + Request::get("http://localhost/modules/$beeblebrox/genid/I/certificate/server") .body(serde_json::to_string(&cert_req).unwrap().into()) .unwrap(); let params = Parameters::with_captures(vec![ - (Some("name".to_string()), "beeblebrox".to_string()), + (Some("name".to_string()), "$beeblebrox".to_string()), (Some("genid".to_string()), "I".to_string()), ]); let response = handler.handle(request, params).wait().unwrap(); diff --git a/edgelet/edgelet-utils/src/lib.rs b/edgelet/edgelet-utils/src/lib.rs index 7801e713638..9359ba6c395 100755 --- a/edgelet/edgelet-utils/src/lib.rs +++ b/edgelet/edgelet-utils/src/lib.rs @@ -35,7 +35,6 @@ mod logging; pub mod macros; mod ser_de; -use std::cmp; use std::collections::HashMap; pub use error::{Error, ErrorKind}; @@ -70,27 +69,56 @@ pub fn prepare_cert_uri_module(hub_name: &str, device_id: &str, module_id: &str) ) } +const ALLOWED_CHAR_DNS: char = '-'; +const DNS_MAX_SIZE: usize = 63; + +/// The name returned from here must conform to following rules (as per RFC 1035): +/// - length must be <= 63 characters +/// - must be all lower case alphanumeric characters or '-' +/// - must start with an alphabet +/// - must end with an alphanumeric character +pub fn sanitize_dns_label(name: &str) -> String { + name.trim_start_matches(|c: char| !c.is_ascii_alphabetic()) + .trim_end_matches(|c: char| !c.is_ascii_alphanumeric()) + .to_lowercase() + .chars() + .filter(|c| c.is_ascii_alphanumeric() || c == &ALLOWED_CHAR_DNS) + .take(DNS_MAX_SIZE) + .collect::() +} + pub fn prepare_dns_san_entries(names: &[&str]) -> String { names .iter() - .map(|name| { - // The name returned from here must conform to following rules (as per RFC 1035): - // - length must be <= 63 characters - // - must be all lower case alphanumeric characters or '-' - // - must start with an alphabet - // - must end with an alphanumeric character - let name = name - .to_lowercase() - .trim_start_matches(|c| !char::is_ascii_lowercase(&c)) - .trim_end_matches(|c| !char::is_alphanumeric(c)) - .replace(|c| !(char::is_alphanumeric(c) || c == '-'), ""); - - format!("DNS:{}", &name[0..cmp::min(name.len(), 63)]) + .filter_map(|name| { + let dns = sanitize_dns_label(name); + if dns.is_empty() { + None + } else { + Some(format!("DNS:{}", dns)) + } }) .collect::>() .join(", ") } +pub fn append_dns_san_entries(sans: &str, names: &[&str]) -> String { + let mut dns_sans = names + .iter() + .filter_map(|name| { + if name.trim().is_empty() { + None + } else { + Some(format!("DNS:{}", name.to_lowercase())) + } + }) + .collect::>() + .join(", "); + dns_sans.push_str(", "); + dns_sans.push_str(sans); + dns_sans +} + #[cfg(test)] mod tests { use super::*; @@ -151,11 +179,43 @@ mod tests { ); } + #[test] + fn dns_label() { + assert_eq!( + "abcdefg-hijklmnop-qrs-tuv-wxyz", + sanitize_dns_label(" -abcdefg-hijklmnop-qrs-tuv-wxyz- ") + ); + assert!('\u{4eac}'.is_alphanumeric()); + assert_eq!( + "abcdefg-hijklmnop-qrs-tuv-wxyz", + sanitize_dns_label("\u{4eac}ABCDEFG-\u{4eac}HIJKLMNOP-QRS-TUV-WXYZ\u{4eac}") + ); + assert_eq!(String::default(), sanitize_dns_label("--------------")); + assert_eq!("a", sanitize_dns_label("a")); + assert_eq!("a-1", sanitize_dns_label("a - 1")); + assert_eq!("edgehub", sanitize_dns_label("$edgeHub")); + let expected_name = "a23456789-123456789-123456789-123456789-123456789-123456789-123"; + assert_eq!(expected_name.len(), DNS_MAX_SIZE); + assert_eq!( + expected_name, + sanitize_dns_label("a23456789-123456789-123456789-123456789-123456789-123456789-1234") + ); + + assert_eq!( + expected_name, + sanitize_dns_label("$a23456789-123456789-123456789-123456789-123456789-123456789-1234") + ); + } + #[test] fn dns_san() { assert_eq!("DNS:edgehub", prepare_dns_san_entries(&["edgehub"])); assert_eq!("DNS:edgehub", prepare_dns_san_entries(&["EDGEhub"])); assert_eq!("DNS:edgehub", prepare_dns_san_entries(&["$$$Edgehub"])); + assert_eq!( + "DNS:edgehub", + prepare_dns_san_entries(&["\u{4eac}Edge\u{4eac}hub\u{4eac}"]) + ); assert_eq!( "DNS:edgehub", prepare_dns_san_entries(&["$$$Edgehub###$$$"]) @@ -187,5 +247,17 @@ mod tests { "DNS:edgehub, DNS:edgy, DNS:moo", prepare_dns_san_entries(&["edgehub", "edgy", "moo"]) ); + // test skipping invalid entries + assert_eq!( + "DNS:edgehub, DNS:moo", + prepare_dns_san_entries(&[" -edgehub -", "-----", "- moo- "]) + ); + + // test appending host name to sanitized label + let sanitized_labels = prepare_dns_san_entries(&["1edgehub", "2edgy"]); + assert_eq!( + "DNS:2019host, DNS:2020host, DNS:edgehub, DNS:edgy", + append_dns_san_entries(&sanitized_labels, &["2019host", " ", "2020host"]) + ); } }