Implement `dns::Name` using webpki's `DNSName`. (#1071)

webpki's DNSName type does full validation and normalization (lowercasing) of
DNS names, which is exactly what `dns::Name` does. webpki's DNSName type
considers a DNS name to be valid according to the rules for TLS certificates,
which is slightly stricter than what a DNS library might otherwise allow. In
anticipation of possible compatibility issues, introduce separate tls::DnsName
and dns::Name names for this type. In the future, if we find that tls::DnsName
is too strict for non-TLS cases, we can have these types diverge without
affecting TLS validation.

Signed-off-by: Brian Smith <brian@briansmith.org>

Cargo.lock

@@ -164,7 +164,7 @@ dependencies = [
  "tower-util 0.1.0 (git+https://github.com/tower-rs/tower)",
  "trust-dns-resolver 0.9.0 (git+https://github.com/bluejekyll/trust-dns)",
  "untrusted 0.6.1 (registry+https://github.com/rust-lang/crates.io-index)",
- "webpki 0.18.0-alpha3 (registry+https://github.com/rust-lang/crates.io-index)",
+ "webpki 0.18.0-alpha4 (registry+https://github.com/rust-lang/crates.io-index)",
 ]
 
 [[package]]
@@ -877,7 +877,7 @@ dependencies = [
  "ring 0.13.0-alpha4 (registry+https://github.com/rust-lang/crates.io-index)",
  "sct 0.3.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "untrusted 0.6.1 (registry+https://github.com/rust-lang/crates.io-index)",
- "webpki 0.18.0-alpha3 (registry+https://github.com/rust-lang/crates.io-index)",
+ "webpki 0.18.0-alpha4 (registry+https://github.com/rust-lang/crates.io-index)",
 ]
 
 [[package]]
@@ -1081,7 +1081,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 dependencies = [
  "rustls 0.12.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "tokio 0.1.6 (registry+https://github.com/rust-lang/crates.io-index)",
- "webpki 0.18.0-alpha3 (registry+https://github.com/rust-lang/crates.io-index)",
+ "webpki 0.18.0-alpha4 (registry+https://github.com/rust-lang/crates.io-index)",
 ]
 
 [[package]]
@@ -1378,7 +1378,7 @@ dependencies = [
 
 [[package]]
 name = "webpki"
-version = "0.18.0-alpha3"
+version = "0.18.0-alpha4"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 dependencies = [
  "ring 0.13.0-alpha4 (registry+https://github.com/rust-lang/crates.io-index)",
@@ -1601,7 +1601,7 @@ dependencies = [
 "checksum utf8-ranges 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)" = "662fab6525a98beff2921d7f61a39e7d59e0b425ebc7d0d9e66d316e55124122"
 "checksum void 1.0.2 (registry+https://github.com/rust-lang/crates.io-index)" = "6a02e4885ed3bc0f2de90ea6dd45ebcbb66dacffe03547fadbb0eeae2770887d"
 "checksum want 0.0.4 (registry+https://github.com/rust-lang/crates.io-index)" = "a05d9d966753fa4b5c8db73fcab5eed4549cfe0e1e4e66911e5564a0085c35d1"
-"checksum webpki 0.18.0-alpha3 (registry+https://github.com/rust-lang/crates.io-index)" = "30cf7434bea34e094993720093b0f0ef4117d3edd977e5bd234de72e6d4c354e"
+"checksum webpki 0.18.0-alpha4 (registry+https://github.com/rust-lang/crates.io-index)" = "724897af4bb44f3e0142b9cca300eb15f61b9b34fa559440bed8c43f2ff7afc0"
 "checksum widestring 0.2.2 (registry+https://github.com/rust-lang/crates.io-index)" = "7157704c2e12e3d2189c507b7482c52820a16dfa4465ba91add92f266667cadb"
 "checksum winapi 0.2.8 (registry+https://github.com/rust-lang/crates.io-index)" = "167dc9d6949a9b857f3451275e911c3f44255842c1f7a76f33c55103a909087a"
 "checksum winapi 0.3.4 (registry+https://github.com/rust-lang/crates.io-index)" = "04e3bd221fcbe8a271359c04f21a76db7d0c6028862d1bb5512d85e1e2eb5bb3"

proxy/Cargo.toml

@@ -52,7 +52,7 @@ tower-in-flight-limit = { git = "https://github.com/tower-rs/tower" }
 tower-util            = { git = "https://github.com/tower-rs/tower" }
 
 ring = "0.13.0-alpha4"
-webpki = "0.18.0-alpha3"
+webpki = "0.18.0-alpha4"
 rustls = "0.12.0"
 tokio-rustls = "0.6.0"
 untrusted = "0.6.1"

proxy/src/dns.rs

@@ -11,6 +11,7 @@ use trust_dns_resolver::{
     lookup_ip::LookupIp,
     AsyncResolver,
 };
+use tls;
 
 use config::Config;
 
@@ -37,30 +38,12 @@ pub enum Response {
 // `Box<Future>` implements `Future` so it doesn't need to be implemented manually.
 pub type IpAddrListFuture = Box<Future<Item=Response, Error=ResolveError> + Send>;
 
-/// A DNS name.
+/// A valid DNS name.
-#[derive(Clone, Debug, Eq, Hash, PartialEq)]
+///
-pub struct Name(String);
+/// This is an alias of the strictly-validated `tls::DnsName` based on the
-
+/// premise that we only need to support DNS names for which one could get a
-impl fmt::Display for Name {
+/// valid certificate.
-    fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
+pub type Name = tls::DnsName;
-        self.0.fmt(f)
-    }
-}
-
-impl<'a> From<&'a str> for Name {
-    fn from(s: &str) -> Self {
-        // TODO: Verify the name is a valid DNS name.
-        // TODO: Avoid this extra allocation.
-        Name(s.to_ascii_lowercase())
-    }
-}
-
-impl AsRef<str> for Name {
-    fn as_ref(&self) -> &str {
-        self.0.as_ref()
-    }
-}
-
 
 impl Resolver {
 
@@ -137,10 +120,10 @@ impl Resolver {
 
     // `ResolverFuture` can only be used for one lookup, so we have to clone all
     // the state during each resolution.
-    fn lookup_ip(self, &Name(ref name): &Name)
+    fn lookup_ip(self, name: &Name)
         -> impl Future<Item = LookupIp, Error = ResolveError>
     {
-        self.resolver.lookup_ip(name.as_str())
+        self.resolver.lookup_ip(name.as_ref())
     }
 }
 
@@ -187,6 +170,11 @@ mod tests {
 
     #[test]
     fn test_dns_name_parsing() {
+        // Make sure `dns::Name`'s validation isn't too strict. It is
+        // implemented in terms of `webpki::DNSName` which has many more tests
+        // at https://github.com/briansmith/webpki/blob/master/tests/dns_name_tests.rs.
+        use convert::TryFrom;
+
         struct Case {
             input: &'static str,
             output: &'static str,
@@ -197,19 +185,33 @@ mod tests {
             Case { input: "1.2.3.x", output: "1.2.3.x", },
             Case { input: "1.2.3.x", output: "1.2.3.x", },
             Case { input: "1.2.3.4A", output: "1.2.3.4a", },
-            Case { input: "a.1.2.3", output: "a.1.2.3", },
-            Case { input: "1.2.x.3", output: "1.2.x.3", },
             Case { input: "a.b.c.d", output: "a.b.c.d", },
 
             // Uppercase letters in labels
             Case { input: "A.b.c.d", output: "a.b.c.d", },
             Case { input: "a.mIddle.c", output: "a.middle.c", },
             Case { input: "a.b.c.D", output: "a.b.c.d", },
+
+            // Absolute
+            Case { input: "a.b.c.d.", output: "a.b.c.d.", },
         ];
 
         for case in VALID {
-            let name = Name::from(case.input);
+            let name = Name::try_from(case.input);
-            assert_eq!(name.as_ref(), case.output);
+            assert_eq!(name.as_ref().map(|x| x.as_ref()), Ok(case.output));
+        }
+
+        static INVALID: &[&str] = &[
+            // These are not in the "preferred name syntax" as defined by
+            // https://tools.ietf.org/html/rfc1123#section-2.1. In particular
+            // the last label only has digits.
+            "1.2.3.4",
+            "a.1.2.3",
+            "1.2.x.3",
+        ];
+
+        for case in INVALID {
+            assert!(Name::try_from(case).is_err());
         }
     }
 }

proxy/src/transport/connect.rs

@@ -9,6 +9,7 @@ use http;
 
 use connection;
 use control::destination;
+use convert::TryFrom;
 use dns;
 
 #[derive(Debug, Clone)]
@@ -58,7 +59,10 @@ impl HostAndPort {
     {
         let host = IpAddr::from_str(a.host())
             .map(Host::Ip)
-            .unwrap_or_else(|_| Host::DnsName(dns::Name::from(a.host())));
+            .or_else(|_|
+                dns::Name::try_from(a.host())
+                    .map(Host::DnsName)
+                    .map_err(|_| HostAndPortError::InvalidHost))?;
         let port = a.port()
             .or(default_port)
             .ok_or_else(|| HostAndPortError::MissingPort)?;

proxy/src/transport/tls/dns_name.rs

@@ -0,0 +1,33 @@
+use super::webpki;
+use std::fmt;
+use convert::TryFrom;
+
+/// A `DnsName` is guaranteed to be syntactically valid. The validity rules
+/// are specified in [RFC 5280 Section 7.2], except that underscores are also
+/// allowed.
+#[derive(Clone, Debug, Eq, PartialEq, Hash)]
+pub struct DnsName(webpki::DNSName);
+
+impl fmt::Display for DnsName {
+    fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
+        self.as_ref().fmt(f)
+    }
+}
+
+#[derive(Debug, Eq, PartialEq)]
+pub struct InvalidDnsName;
+
+impl<'a> TryFrom<&'a str> for DnsName {
+    type Err = InvalidDnsName;
+    fn try_from(s: &str) -> Result<Self, <Self as TryFrom<&str>>::Err> {
+        webpki::DNSNameRef::try_from_ascii_str(s)
+            .map(|r| DnsName(r.to_owned()))
+            .map_err(|()| InvalidDnsName)
+    }
+}
+
+impl AsRef<str> for DnsName {
+    fn as_ref(&self) -> &str {
+        <webpki::DNSName as AsRef<str>>::as_ref(&self.0)
+    }
+}

proxy/src/transport/tls/mod.rs

@@ -8,8 +8,10 @@ extern crate webpki;
 mod config;
 mod cert_resolver;
 mod connection;
+mod dns_name;
 
 pub use self::{
     config::{CommonSettings, CommonConfig, Error, ServerConfig, ServerConfigWatch},
     connection::Connection,
+    dns_name::DnsName,
 };