more feedback

Signed-off-by: Zahari Dichev <[email protected]>

Cargo.lock

@@ -1391,7 +1391,6 @@ dependencies = [
  "linkerd-tls-test-util",
  "linkerd-tracing",
  "pin-project",
- "rcgen",
  "tokio",
  "tracing",
 ]
@@ -1408,6 +1407,7 @@ dependencies = [
  "linkerd-identity",
  "linkerd-io",
  "linkerd-meshtls",
+ "linkerd-meshtls-test-util",
  "linkerd-stack",
  "linkerd-tls",
  "linkerd-tls-test-util",
@@ -1426,6 +1426,7 @@ dependencies = [
  "linkerd-identity",
  "linkerd-io",
  "linkerd-meshtls",
+ "linkerd-meshtls-test-util",
  "linkerd-stack",
  "linkerd-tls",
  "linkerd-tls-test-util",
@@ -1438,6 +1439,14 @@ dependencies = [
  "tracing",
 ]
 
+[[package]]
+name = "linkerd-meshtls-test-util"
+version = "0.1.0"
+dependencies = [
+ "linkerd-identity",
+ "rcgen",
+]
+
 [[package]]
 name = "linkerd-metrics"
 version = "0.1.0"

Cargo.toml

@@ -36,6 +36,7 @@ members = [
     "linkerd/meshtls",
     "linkerd/meshtls/boring",
     "linkerd/meshtls/rustls",
+    "linkerd/meshtls/test-util",
     "linkerd/metrics",
     "linkerd/opencensus",
     "linkerd/proxy/api-resolve",

linkerd/meshtls/Cargo.toml

@@ -25,7 +25,6 @@ linkerd-meshtls-boring = { path = "boring", optional = true }
 linkerd-meshtls-rustls = { path = "rustls", optional = true }
 linkerd-stack = { path = "../stack" }
 linkerd-tls = { path = "../tls" }
-rcgen = "0.11.3"
 
 [dev-dependencies]
 tokio = { version = "1", features = ["macros", "net", "rt-multi-thread"] }

linkerd/meshtls/boring/Cargo.toml

@@ -26,4 +26,5 @@ fips = ["boring/fips"]
 [dev-dependencies]
 linkerd-tls-test-util = { path = "../../tls/test-util" }
 linkerd-meshtls = { path = "../../meshtls" }
+linkerd-meshtls-test-util = { path = "../test-util" }
 

linkerd/meshtls/boring/src/client.rs

@@ -5,7 +5,7 @@ use linkerd_io as io;
 use linkerd_stack::{NewService, Service};
 use linkerd_tls::{client::AlpnProtocols, ClientTls, NegotiatedProtocolRef, ServerName};
 use std::{future::Future, pin::Pin, sync::Arc, task::Context};
-use tracing::debug;
+use tracing::{debug, trace};
 
 #[derive(Clone)]
 pub struct NewClient(CredsRx);
@@ -78,39 +78,46 @@ where
             .borrow()
             .connector(self.alpn.as_deref().unwrap_or(&[]));
         Box::pin(async move {
-            let conn = connector.map_err(|e| io::Error::new(io::ErrorKind::Other, e))?;
-            let config = conn
-                .configure()
+            let config = connector
                 .map_err(|e| io::Error::new(io::ErrorKind::Other, e))?
-                // Switch off DNS SAN Verification based on the hostname
-                .verify_hostname(false);
-
-            tokio_boring::connect(config, server_name.as_str(), io)
+                .configure()
+                .map_err(|e| io::Error::new(io::ErrorKind::Other, e))?;
+
+            // Establish a TLS connection to the server using the provided
+            // `server_name` as an SNI value to the server.
+            //
+            // Hostname verification is DISABLED, as we do not require that the
+            // peer's certificate actually matches the `server_name`. Instead,
+            // the `server_id` is used to perform the appropriate form of
+            // verification after the session is established.
+            let io = tokio_boring::connect(config.verify_hostname(false), server_name.as_str(), io)
                 .await
                 .map_err(|e| match e.as_io_error() {
                     // TODO(ver) boring should let us take ownership of the error directly.
                     Some(ioe) => io::Error::new(ioe.kind(), ioe.to_string()),
-                    // XXX(ver) to use the boring error directly here we have to constraint the socket on Sync +
-                    // std::fmt::Debug, which is a pain.
+                    // XXX(ver) to use the boring error directly here we have to
+                    // constrain the socket on Sync + std::fmt::Debug, which is
+                    // a pain.
                     None => io::Error::new(io::ErrorKind::Other, "unexpected TLS handshake error"),
-                }).and_then(|io| {
-                    let cert = io.ssl()
-                        .peer_certificate()
-                        .ok_or_else(|| io::Error::new(
-                            io::ErrorKind::Other,
-                            "could not extract peer cert",
-                        ))?;
-                    verify::verify_id(&cert, &server_id)
-                        .map_err(|e| io::Error::new(io::ErrorKind::Other, e))?;
-                    debug!(
-                        tls = io.ssl().version_str(),
-                        client.cert = ?io.ssl().certificate().and_then(super::fingerprint),
-                        peer.cert = ?io.ssl().peer_certificate().as_deref().and_then(super::fingerprint),
-                        alpn = ?io.ssl().selected_alpn_protocol(),
-                        "Initiated TLS connection"
-                    );
-                    Ok(ClientIo(io))
-                })
+                })?;
+
+            // Servers must present a peer certificate. We extract the x509 cert
+            // and verify it manually against the `server_id`.
+            let cert = io.ssl().peer_certificate().ok_or_else(|| {
+                io::Error::new(io::ErrorKind::Other, "could not extract peer cert")
+            })?;
+            verify::verify_id(&cert, &server_id)
+                .map_err(|e| io::Error::new(io::ErrorKind::Other, e))?;
+
+            debug!(
+                tls = io.ssl().version_str(),
+                client.cert = ?io.ssl().certificate().and_then(super::fingerprint),
+                peer.cert = ?io.ssl().peer_certificate().as_deref().and_then(super::fingerprint),
+                alpn = ?io.ssl().selected_alpn_protocol(),
+                "Initiated TLS connection"
+            );
+            trace!(peer.id = %server_id, peer.name = %server_name);
+            Ok(ClientIo(io))
         })
     }
 }

linkerd/meshtls/boring/src/creds/verify.rs

@@ -24,39 +24,39 @@ pub(crate) fn verify_id(cert: &X509, id: &id::Id) -> io::Result<()> {
 mod tests {
     use super::verify_id;
     use boring::x509::X509;
-    use linkerd_meshtls::verify_tests;
+    use linkerd_meshtls_test_util as test_util;
 
     fn vec_to_cert(data: Vec<u8>) -> X509 {
         X509::from_der(data.as_slice()).expect("should parse")
     }
 
     #[test]
     fn cert_with_dns_san_matches_dns_id() {
-        verify_tests::cert_with_dns_san_matches_dns_id(verify_id, vec_to_cert);
+        test_util::cert_with_dns_san_matches_dns_id(verify_id, vec_to_cert);
     }
 
     #[test]
     fn cert_with_dns_san_does_not_match_dns_id() {
-        verify_tests::cert_with_dns_san_does_not_match_dns_id(verify_id, vec_to_cert);
+        test_util::cert_with_dns_san_does_not_match_dns_id(verify_id, vec_to_cert);
     }
 
     #[test]
     fn cert_with_uri_san_does_not_match_dns_id() {
-        verify_tests::cert_with_uri_san_does_not_match_dns_id(verify_id, vec_to_cert);
+        test_util::cert_with_uri_san_does_not_match_dns_id(verify_id, vec_to_cert);
     }
 
     #[test]
     fn cert_with_no_san_does_not_verify_for_dns_id() {
-        verify_tests::cert_with_no_san_does_not_verify_for_dns_id(verify_id, vec_to_cert);
+        test_util::cert_with_no_san_does_not_verify_for_dns_id(verify_id, vec_to_cert);
     }
 
     #[test]
     fn cert_with_dns_multiple_sans_one_matches_dns_id() {
-        verify_tests::cert_with_dns_multiple_sans_one_matches_dns_id(verify_id, vec_to_cert);
+        test_util::cert_with_dns_multiple_sans_one_matches_dns_id(verify_id, vec_to_cert);
     }
 
     #[test]
     fn cert_with_dns_multiple_sans_none_matches_dns_id() {
-        verify_tests::cert_with_dns_multiple_sans_none_matches_dns_id(verify_id, vec_to_cert);
+        test_util::cert_with_dns_multiple_sans_none_matches_dns_id(verify_id, vec_to_cert);
     }
 }

linkerd/meshtls/rustls/Cargo.toml

@@ -30,3 +30,5 @@ linkerd-tls-test-util = { path = "../../tls/test-util", optional = true }
 [dev-dependencies]
 linkerd-tls-test-util = { path = "../../tls/test-util" }
 linkerd-meshtls = { path = "../../meshtls" }
+linkerd-meshtls-test-util = { path = "../test-util" }
+

linkerd/meshtls/rustls/src/client.rs

@@ -101,6 +101,11 @@ where
     fn call(&mut self, io: I) -> Self::Future {
         let server_id = self.server_id.clone();
         Box::pin(
+            // Connect to the server, sending the `server_name` SNI in the
+            // client handshake. The provided config should use the
+            // `AnySanVerifier` to ignore the server certificate's DNS SANs.
+            // Instead, we extract the server's leaf certificate after the
+            // handshake and verify that it matches the provided `server_id``.
             tokio_rustls::TlsConnector::from(self.config.clone())
                 // XXX(eliza): it's a bummer that the server name has to be cloned here...
                 .connect(self.server_name.clone(), io)

linkerd/meshtls/rustls/src/creds/verify.rs

@@ -18,6 +18,13 @@ impl AnySanVerifier {
     }
 }
 
+// Note that this logic is almost identical to the implementation of
+// `rustls::client::WebPkiVerifier`` that cab be found here:
+// https://github.com/rustls/rustls/blob/ccb79947a4811412ee7dcddcd0f51ea56bccf101/rustls/src/webpki/server_verifier.rs#L239
+// The only difference is that we omit the step that performs
+// DNS SAN validation. The reason for that stems from the fact that
+// SAN validation in rustls is limited to DNS SANs only while we
+// want to support alternative SAN types (e.g. URI).
 impl ServerCertVerifier for AnySanVerifier {
     /// Will verify the certificate is valid in the following ways:
     /// - Signed by a  trusted `RootCertStore` CA
@@ -58,7 +65,7 @@ pub(crate) fn verify_id(end_entity: &Certificate, id: &id::Id) -> io::Result<()>
 #[cfg(test)]
 mod tests {
     use super::verify_id;
-    use linkerd_meshtls::verify_tests;
+    use linkerd_meshtls_test_util as test_util;
     use tokio_rustls::rustls::Certificate;
 
     fn vec_to_cert(data: Vec<u8>) -> Certificate {
@@ -67,31 +74,31 @@ mod tests {
 
     #[test]
     fn cert_with_dns_san_matches_dns_id() {
-        verify_tests::cert_with_dns_san_matches_dns_id(verify_id, vec_to_cert);
+        test_util::cert_with_dns_san_matches_dns_id(verify_id, vec_to_cert);
     }
 
     #[test]
     fn cert_with_dns_san_does_not_match_dns_id() {
-        verify_tests::cert_with_dns_san_does_not_match_dns_id(verify_id, vec_to_cert);
+        test_util::cert_with_dns_san_does_not_match_dns_id(verify_id, vec_to_cert);
     }
 
     #[test]
     fn cert_with_uri_san_does_not_match_dns_id() {
-        verify_tests::cert_with_uri_san_does_not_match_dns_id(verify_id, vec_to_cert);
+        test_util::cert_with_uri_san_does_not_match_dns_id(verify_id, vec_to_cert);
     }
 
     #[test]
     fn cert_with_no_san_does_not_verify_for_dns_id() {
-        verify_tests::cert_with_no_san_does_not_verify_for_dns_id(verify_id, vec_to_cert);
+        test_util::cert_with_no_san_does_not_verify_for_dns_id(verify_id, vec_to_cert);
     }
 
     #[test]
     fn cert_with_dns_multiple_sans_one_matches_dns_id() {
-        verify_tests::cert_with_dns_multiple_sans_one_matches_dns_id(verify_id, vec_to_cert);
+        test_util::cert_with_dns_multiple_sans_one_matches_dns_id(verify_id, vec_to_cert);
     }
 
     #[test]
     fn cert_with_dns_multiple_sans_none_matches_dns_id() {
-        verify_tests::cert_with_dns_multiple_sans_none_matches_dns_id(verify_id, vec_to_cert);
+        test_util::cert_with_dns_multiple_sans_none_matches_dns_id(verify_id, vec_to_cert);
     }
 }

linkerd/meshtls/src/lib.rs

@@ -15,7 +15,6 @@
 mod client;
 pub mod creds;
 mod server;
-pub mod verify_tests;
 
 pub use self::{
     client::{ClientIo, Connect, ConnectFuture, NewClient},

linkerd/meshtls/test-util/Cargo.toml

@@ -0,0 +1,11 @@
+[package]
+name = "linkerd-meshtls-test-util"
+version = "0.1.0"
+authors = ["Linkerd Developers <[email protected]>"]
+license = "Apache-2.0"
+edition = "2018"
+publish = false
+
+[dependencies]
+linkerd-identity = { path = "../../identity" }
+rcgen = "0.11.3"