meshtls: Extract TLS id verification out of TLS backends (#2507)

To further prepare for the introduction of Spiffe IDs in the proxy, this change disables TLS SAN verification in both rustls and boring TLS implementations. The need to do that stems from the fact that these implementations only look at DNS SANs and cannot handle certificates that use URI SANs. To enable adding this functionality, TLS Id verification is not extracted as a separate step, after the TLS handshake completes. To do that in boring, we need to verify_hostname. For rustls, we need to pass a custom ServerCertVerifier that skips name validation. In our case, the verifier is nearly identical to ustls::client::WebPkiVerifier, except for the step where DNS SANs are validated based on the ServerName. This change is non functional, but it sets the stage to have more control over TLS Id verification. Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
linkerd · Nov 14, 2023 · 2ad49d6 · 2ad49d6
1 parent 8542ec7
commit 2ad49d6
Show file tree

Hide file tree

Showing 15 changed files with 443 additions and 71 deletions.
diff --git a/Cargo.lock b/Cargo.lock
@@ -141,6 +141,12 @@ version = "0.13.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "9e1b586273c5702936fe7b7d6896644d8be71e6314cfe09d3167c95f712589e8"
 
+[[package]]
+name = "base64"
+version = "0.21.5"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "35636a1494ede3b646cc98f74f8e62c773a38a659ebc777a2cf26b9b74171df9"
+
 [[package]]
 name = "bindgen"
 version = "0.66.1"
@@ -285,6 +291,15 @@ dependencies = [
  "gzip-header",
 ]
 
+[[package]]
+name = "deranged"
+version = "0.3.9"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "0f32d04922c60427da6f9fef14d042d9edddef64cb9d4ce0d64d0685fbeb1fd3"
+dependencies = [
+ "powerfmt",
+]
+
 [[package]]
 name = "derive_arbitrary"
 version = "1.3.0"
@@ -1391,6 +1406,8 @@ dependencies = [
  "linkerd-error",
  "linkerd-identity",
  "linkerd-io",
+ "linkerd-meshtls",
+ "linkerd-meshtls-test-util",
  "linkerd-stack",
  "linkerd-tls",
  "linkerd-tls-test-util",
@@ -1408,6 +1425,8 @@ dependencies = [
  "linkerd-error",
  "linkerd-identity",
  "linkerd-io",
+ "linkerd-meshtls",
+ "linkerd-meshtls-test-util",
  "linkerd-stack",
  "linkerd-tls",
  "linkerd-tls-test-util",
@@ -1420,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"
@@ -1850,7 +1877,7 @@ dependencies = [
 name = "linkerd-trace-context"
 version = "0.1.0"
 dependencies = [
- "base64",
+ "base64 0.13.1",
  "bytes",
  "futures",
  "hex",
@@ -2151,6 +2178,16 @@ version = "0.1.2"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "19b17cddbe7ec3f8bc800887bab5e717348c95ea2ca0b1bf0837fb964dc67099"
 
+[[package]]
+name = "pem"
+version = "3.0.2"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "3163d2912b7c3b52d651a055f2c7eec9ba5cd22d26ef75b8dd3a59980b185923"
+dependencies = [
+ "base64 0.21.5",
+ "serde",
+]
+
 [[package]]
 name = "percent-encoding"
 version = "2.2.0"
@@ -2199,6 +2236,12 @@ version = "0.1.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "8b870d8c151b6f2fb93e84a13146138f05d02ed11c7e7c54f8826aaaf7c9f184"
 
+[[package]]
+name = "powerfmt"
+version = "0.2.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "439ee305def115ba05938db6eb1644ff94165c5ab5e9420d1c1bcedbba909391"
+
 [[package]]
 name = "ppv-lite86"
 version = "0.2.17"
@@ -2351,6 +2394,18 @@ version = "1.3.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "8b9283c6b06096b47afc7109834fdedab891175bb5241ee5d4f7d2546549f263"
 
+[[package]]
+name = "rcgen"
+version = "0.11.3"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "52c4f3084aa3bc7dfbba4eff4fab2a54db4324965d8872ab933565e6fbd83bc6"
+dependencies = [
+ "pem",
+ "ring",
+ "time",
+ "yasna",
+]
+
 [[package]]
 name = "redox_syscall"
 version = "0.2.15"
@@ -2472,7 +2527,7 @@ version = "1.0.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "0864aeff53f8c05aa08d86e5ef839d3dfcf07aeba2db32f12db0ef716e87bd55"
 dependencies = [
- "base64",
+ "base64 0.13.1",
 ]
 
 [[package]]
@@ -2521,9 +2576,9 @@ checksum = "bebd363326d05ec3e2f532ab7660680f3b02130d780c299bca73469d521bc0ed"
 
 [[package]]
 name = "serde"
-version = "1.0.159"
+version = "1.0.185"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "3c04e8343c3daeec41f58990b9d77068df31209f2af111e059e9fe9646693065"
+checksum = "be9b6f69f1dfd54c3b568ffa45c310d6973a5e5148fd40cf515acaf38cf5bc31"
 
 [[package]]
 name = "serde_json"
@@ -2672,6 +2727,24 @@ dependencies = [
  "once_cell",
 ]
 
+[[package]]
+name = "time"
+version = "0.3.30"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "c4a34ab300f2dee6e562c10a046fc05e358b29f9bf92277f30c3c8d82275f6f5"
+dependencies = [
+ "deranged",
+ "powerfmt",
+ "serde",
+ "time-core",
+]
+
+[[package]]
+name = "time-core"
+version = "0.1.2"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "ef927ca75afb808a4d64dd374f00a2adf8d0fcff8e7b184af886c3c87ec4a3f3"
+
 [[package]]
 name = "tinyvec"
 version = "1.6.0"
@@ -2797,7 +2870,7 @@ dependencies = [
  "async-stream",
  "async-trait",
  "axum",
- "base64",
+ "base64 0.13.1",
  "bytes",
  "futures-core",
  "futures-util",
@@ -3349,6 +3422,15 @@ dependencies = [
  "winapi",
 ]
 
+[[package]]
+name = "yasna"
+version = "0.5.2"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "e17bb3549cc1321ae1296b9cdc2698e2b6cb1992adfa19a8c72e5b7a738f44cd"
+dependencies = [
+ "time",
+]
+
 [[package]]
 name = "zerocopy"
 version = "0.7.3"

diff --git a/Cargo.toml b/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",

diff --git a/deny.toml b/deny.toml
@@ -73,6 +73,9 @@ skip = [
     # `tonic` v0.6 depends on `bitflags` v1.x, while `boring-sys` depends on
     # `bitflags` v2.x. Allow both versions to coexist peacefully for now.
     { name = "bitflags", version = "1" },
+    # `linkerd-trace-context`, `rustls-pemfile` and `tonic` depend on `base64`
+    # v0.13.1 while `rcgen` depends on v0.21.5
+    { name = "base64" },
 ]
 skip-tree = [
     # right now we have a mix of versions of this crate in the ecosystem

diff --git a/linkerd/meshtls/boring/Cargo.toml b/linkerd/meshtls/boring/Cargo.toml
@@ -25,3 +25,6 @@ fips = ["boring/fips"]
 
 [dev-dependencies]
 linkerd-tls-test-util = { path = "../../tls/test-util" }
+linkerd-meshtls = { path = "../../meshtls" }
+linkerd-meshtls-test-util = { path = "../test-util" }
+
diff --git a/linkerd/meshtls/boring/src/client.rs b/linkerd/meshtls/boring/src/client.rs
@@ -1,9 +1,11 @@
+use crate::creds::verify;
 use crate::creds::CredsRx;
+use linkerd_identity as id;
 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);
@@ -12,6 +14,7 @@ pub struct NewClient(CredsRx);
 pub struct Connect {
     rx: CredsRx,
     alpn: Option<Arc<[Vec<u8>]>>,
+    id: id::Id,
     server: ServerName,
 }
 
@@ -50,6 +53,7 @@ impl Connect {
             rx,
             alpn: client_tls.alpn.map(|AlpnProtocols(ps)| ps.into()),
             server: client_tls.server_name,
+            id: client_tls.server_id.into(),
         }
     }
 }
@@ -68,32 +72,51 @@ where
 
     fn call(&mut self, io: I) -> Self::Future {
         let server_name = self.server.clone();
+        let server_id = self.id.clone();
         let connector = self
             .rx
             .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
+            let config = connector
+                .map_err(|e| io::Error::new(io::ErrorKind::Other, e))?
                 .configure()
                 .map_err(|e| io::Error::new(io::ErrorKind::Other, e))?;
-            let io = tokio_boring::connect(config, server_name.as_str(), io)
+
+            // 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"),
                 })?;
 
+            // 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))
         })
     }

diff --git a/linkerd/meshtls/boring/src/creds.rs b/linkerd/meshtls/boring/src/creds.rs
@@ -1,5 +1,6 @@
 mod receiver;
 mod store;
+pub(crate) mod verify;
 
 pub use self::{receiver::Receiver, store::Store};
 use boring::{
@@ -27,8 +28,8 @@ pub fn watch(
     };
 
     let (tx, rx) = watch::channel(Creds::from(creds.clone()));
-    let rx = Receiver::new(local_id, server_name.clone(), rx);
-    let store = Store::new(creds, csr, server_name, tx);
+    let rx = Receiver::new(local_id.clone(), server_name, rx);
+    let store = Store::new(creds, csr, local_id, tx);
 
     Ok((store, rx))
 }

diff --git a/linkerd/meshtls/boring/src/creds/store.rs b/linkerd/meshtls/boring/src/creds/store.rs
@@ -1,42 +1,27 @@
-use super::{BaseCreds, Certs, Creds, CredsTx};
+use super::{verify, BaseCreds, Certs, Creds, CredsTx};
 use boring::x509::{X509StoreContext, X509};
-use linkerd_dns_name as dns;
 use linkerd_error::Result;
 use linkerd_identity as id;
 use std::sync::Arc;
 
 pub struct Store {
     creds: Arc<BaseCreds>,
     csr: Vec<u8>,
-    name: dns::Name,
+    id: id::Id,
     tx: CredsTx,
 }
 
 // === impl Store ===
 
 impl Store {
-    pub(super) fn new(creds: Arc<BaseCreds>, csr: &[u8], name: dns::Name, tx: CredsTx) -> Self {
+    pub(super) fn new(creds: Arc<BaseCreds>, csr: &[u8], id: id::Id, tx: CredsTx) -> Self {
         Self {
             creds,
             csr: csr.into(),
-            name,
+            id,
             tx,
         }
     }
-
-    fn cert_matches_name(&self, cert: &X509) -> bool {
-        for san in cert.subject_alt_names().into_iter().flatten() {
-            if let Some(n) = san.dnsname() {
-                if let Ok(name) = n.parse::<dns::Name>() {
-                    if name == self.name {
-                        return true;
-                    }
-                }
-            }
-        }
-
-        false
-    }
 }
 
 impl id::Credentials for Store {
@@ -53,9 +38,8 @@ impl id::Credentials for Store {
         _expiry: std::time::SystemTime,
     ) -> Result<()> {
         let leaf = X509::from_der(&leaf)?;
-        if !self.cert_matches_name(&leaf) {
-            return Err("certificate does not have a DNS name SAN for the local identity".into());
-        }
+
+        verify::verify_id(&leaf, &self.id)?;
 
         let intermediates = intermediates
             .into_iter()