Skip to content

Commit

Permalink
Fix invalid DNS SAN entries (#795) (#802)
Browse files Browse the repository at this point in the history 
* 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 a8148cc.

* 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.
  • Loading branch information
@myagley
myagley authored Feb 4, 2019
1 parent 8288bc9 commit 078bda7
Show file tree
Hide file tree
Showing 2 changed files with 99 additions and 22 deletions.
21 changes: 13 additions & 8 deletions edgelet/edgelet-http-workload/src/server/cert/server.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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()))))
Expand All @@ -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();
Expand Down
100 changes: 86 additions & 14 deletions edgelet/edgelet-utils/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ mod logging;
pub mod macros;
mod ser_de;

use std::cmp;
use std::collections::HashMap;

pub use error::{Error, ErrorKind};
Expand Down Expand Up @@ -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::<String>()
}

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::<Vec<String>>()
.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::<Vec<String>>()
.join(", ");
dns_sans.push_str(", ");
dns_sans.push_str(sans);
dns_sans
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down Expand Up @@ -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###$$$"])
Expand Down Expand Up @@ -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"])
);
}
}

0 comments on commit 078bda7

Please sign in to comment.