linkerd_identity: split linkerd_identity::Id into DNS and URI varia…

…nts (#2538) Our `linkerd_identity::Id` type was capable of representing only DNS-like SANs. In order to be able to perform SAN verification on SPIFFE identities, in this PR we turn this type into an enum that can represent either a DNS or URI ids. Additionally the logic in `meshtls::verifier`` has been updated to consider URI SANs as well. Signed-off-by: Zahari Dichev <[email protected]>
linkerd · Dec 8, 2023 · c5b1f63 · c5b1f63
1 parent 31b2aea
commit c5b1f63
Show file tree

Hide file tree

Showing 17 changed files with 512 additions and 114 deletions.
diff --git a/Cargo.lock b/Cargo.lock
@@ -582,9 +582,9 @@ checksum = "aa9a19cbb55df58761df49b23516a86d432839add4af60fc256da840f66ed35b"
 
 [[package]]
 name = "form_urlencoded"
-version = "1.1.0"
+version = "1.2.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "a9c384f161156f5260c24a097c56119f9be8c798586aecc13afbcbe7b7e26bf8"
+checksum = "e13624c2627564efccf4934284bdd98cbaa14e79b0b5a141218e507b3a823456"
 dependencies = [
  "percent-encoding",
 ]
@@ -899,6 +899,16 @@ dependencies = [
  "unicode-normalization",
 ]
 
+[[package]]
+name = "idna"
+version = "0.5.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "634d9b1461af396cad843f47fdba5597a4f9e6ddd4bfb6ff5d85028c25cb12f6"
+dependencies = [
+ "unicode-bidi",
+ "unicode-normalization",
+]
+
 [[package]]
 name = "indexmap"
 version = "1.9.3"
@@ -1514,6 +1524,8 @@ version = "0.1.0"
 dependencies = [
  "linkerd-dns-name",
  "linkerd-error",
+ "thiserror",
+ "url",
 ]
 
 [[package]]
@@ -2452,9 +2464,9 @@ dependencies = [
 
 [[package]]
 name = "percent-encoding"
-version = "2.2.0"
+version = "2.3.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "478c572c3d73181ff3c2539045f6eb99e5491218eae919370993b890cdbdd98e"
+checksum = "e3148f5046208a5d56bcfc03053e3ca6334e51da8dfb19b6cdc8b306fae3283e"
 
 [[package]]
 name = "petgraph"
@@ -3458,7 +3470,7 @@ dependencies = [
  "futures-channel",
  "futures-io",
  "futures-util",
- "idna",
+ "idna 0.2.3",
  "ipnet",
  "lazy_static",
  "rand",
@@ -3537,12 +3549,12 @@ checksum = "a156c684c91ea7d62626509bce3cb4e1d9ed5c4d978f7b4352658f96a4c26b4a"
 
 [[package]]
 name = "url"
-version = "2.3.0"
+version = "2.5.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "22fe195a4f217c25b25cb5058ced57059824a678474874038dc88d211bf508d3"
+checksum = "31e6302e3bb753d46e83516cae55ae196fc0c309407cf11ab35cc51a4c2a4633"
 dependencies = [
  "form_urlencoded",
- "idna",
+ "idna 0.5.0",
  "percent-encoding",
 ]
 

diff --git a/deny.toml b/deny.toml
@@ -79,6 +79,8 @@ skip = [
     # https://github.com/hawkw/matchers/pull/4
     { name = "regex-automata", version = "0.1" },
     { name = "regex-syntax", version = "0.6" },
+    # `trust-dns-proto`, depends on `idna` v0.2.3 while `url` depends on v0.5.0
+    { name = "idna" },
 ]
 skip-tree = [
     # right now we have a mix of versions of this crate in the ecosystem

diff --git a/linkerd/app/inbound/src/policy.rs b/linkerd/app/inbound/src/policy.rs
@@ -17,6 +17,7 @@ pub use self::{
 
 pub use linkerd_app_core::metrics::ServerLabel;
 use linkerd_app_core::{
+    identity as id,
     metrics::{RouteAuthzLabels, ServerAuthzLabels},
     tls,
     transport::{ClientAddr, OrigDstAddr, Remote},
@@ -150,15 +151,7 @@ impl AllowPolicy {
     }
 }
 
-fn is_authorized(
-    authz: &Authorization,
-    client_addr: Remote<ClientAddr>,
-    tls: &tls::ConditionalServerTls,
-) -> bool {
-    if !authz.networks.iter().any(|n| n.contains(&client_addr.ip())) {
-        return false;
-    }
-
+fn is_tls_authorized(tls: &tls::ConditionalServerTls, authz: &Authorization) -> bool {
     match authz.authentication {
         Authentication::Unauthenticated => true,
 
@@ -176,15 +169,30 @@ fn is_authorized(
             tls::ConditionalServerTls::Some(tls::ServerTls::Established {
                 client_id: Some(tls::server::ClientId(ref id)),
                 ..
-            }) => {
-                identities.contains(&*id.to_str())
-                    || suffixes.iter().any(|s| s.contains(&id.to_str()))
-            }
+            }) => match id {
+                id::Id::Uri(_) => identities.contains(&*id.to_str()),
+                id::Id::Dns(_) => {
+                    identities.contains(&*id.to_str())
+                        || suffixes.iter().any(|s| s.contains(&id.to_str()))
+                }
+            },
             _ => false,
         },
     }
 }
 
+fn is_authorized(
+    authz: &Authorization,
+    client_addr: Remote<ClientAddr>,
+    tls: &tls::ConditionalServerTls,
+) -> bool {
+    if !authz.networks.iter().any(|n| n.contains(&client_addr.ip())) {
+        return false;
+    }
+
+    is_tls_authorized(tls, authz)
+}
+
 // === impl Permit ===
 
 impl ServerPermit {
@@ -199,3 +207,119 @@ impl ServerPermit {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::is_tls_authorized;
+    use super::Meta;
+    use super::Suffix;
+    use super::{Authentication, Authorization};
+    use linkerd_app_core::tls;
+    use std::collections::BTreeSet;
+    use std::str::FromStr;
+    use std::sync::Arc;
+
+    fn authorization(identities: BTreeSet<String>, suffixes: Vec<Suffix>) -> Authorization {
+        Authorization {
+            networks: vec![],
+            meta: Arc::new(Meta::Default {
+                name: "name".into(),
+            }),
+            authentication: Authentication::TlsAuthenticated {
+                identities,
+                suffixes,
+            },
+        }
+    }
+
+    fn server_tls(identity: &str) -> tls::ConditionalServerTls {
+        let client_id = tls::ClientId::from_str(identity).expect("should parse id");
+        tls::ConditionalServerTls::Some(tls::ServerTls::Established {
+            client_id: Some(client_id),
+            negotiated_protocol: None,
+        })
+    }
+
+    #[test]
+    fn is_authorized_for_matching_spiffe_ids() {
+        let tls = server_tls("spiffe://some-root/some-workload");
+        let authz = authorization(
+            BTreeSet::from(["spiffe://some-root/some-workload".into()]),
+            vec![],
+        );
+        assert!(is_tls_authorized(&tls, &authz))
+    }
+
+    #[test]
+    fn is_not_authorized_for_non_matching_spiffe_ids() {
+        let tls = server_tls("spiffe://some-root/some-workload-1");
+        let authz = authorization(
+            BTreeSet::from(["spiffe://some-root/some-workload-2".into()]),
+            vec![],
+        );
+        assert!(!is_tls_authorized(&tls, &authz))
+    }
+
+    #[test]
+    fn is_authorized_for_matching_dns_ids() {
+        let tls = server_tls("some.id.local");
+        let authz = authorization(BTreeSet::from(["some.id.local".into()]), vec![]);
+        assert!(is_tls_authorized(&tls, &authz))
+    }
+
+    #[test]
+    fn is_not_authorized_for_non_matching_dns_ids() {
+        let tls = server_tls("some.id.local.one");
+        let authz = authorization(BTreeSet::from(["some.id.local.two".into()]), vec![]);
+        assert!(!is_tls_authorized(&tls, &authz))
+    }
+
+    #[test]
+    fn is_authorized_for_matching_dns_suffixes_ids() {
+        let tls = server_tls("some.id.local");
+        let authz = authorization(BTreeSet::new(), vec![Suffix::new("id.local")]);
+        assert!(is_tls_authorized(&tls, &authz))
+    }
+
+    #[test]
+    fn is_not_authorized_for_non_matching_suffixes_ids() {
+        let tls = server_tls("some.id.local");
+        let authz = authorization(BTreeSet::new(), vec![Suffix::new("another-id.local")]);
+        assert!(!is_tls_authorized(&tls, &authz))
+    }
+
+    #[test]
+    fn is_not_authorized_for_suffixes_and_spiffe_id() {
+        let tls = server_tls("spiffe://some-root/some-workload-1");
+        let authz = authorization(BTreeSet::new(), vec![Suffix::new("some-workload-1")]);
+        assert!(!is_tls_authorized(&tls, &authz))
+    }
+
+    #[test]
+    fn is_authorized_for_one_matching_spiffe_id() {
+        let tls = server_tls("spiffe://some-root/some-workload-1");
+        let authz = authorization(
+            BTreeSet::from([
+                "spiffe://some-root/some-workload-1".into(),
+                "some.workload.one".into(),
+                "some.workload.two".into(),
+            ]),
+            vec![],
+        );
+        assert!(is_tls_authorized(&tls, &authz))
+    }
+
+    #[test]
+    fn is_authorized_for_one_matching_dns_id() {
+        let tls = server_tls("some.workload.one");
+        let authz = authorization(
+            BTreeSet::from([
+                "spiffe://some-root/some-workload-1".into(),
+                "some.workload.one".into(),
+                "some.workload.two".into(),
+            ]),
+            vec![],
+        );
+        assert!(is_tls_authorized(&tls, &authz))
+    }
+}
diff --git a/linkerd/app/outbound/src/http/concrete.rs b/linkerd/app/outbound/src/http/concrete.rs
@@ -353,14 +353,15 @@ impl<T> svc::Param<tls::ConditionalClientTls> for Endpoint<T> {
         self.metadata
             .identity()
             .cloned()
-            .map(move |server_id| {
-                let alpn = if use_transport_header {
+            .map(move |mut client_tls| {
+                client_tls.alpn = if use_transport_header {
                     use linkerd_app_core::transport_header::PROTOCOL;
                     Some(tls::client::AlpnProtocols(vec![PROTOCOL.into()]))
                 } else {
                     None
                 };
-                tls::ConditionalClientTls::Some(tls::ClientTls::new(server_id, alpn))
+
+                tls::ConditionalClientTls::Some(client_tls)
             })
             .unwrap_or(tls::ConditionalClientTls::None(
                 tls::NoClientTls::NotProvidedByServiceDiscovery,

diff --git a/linkerd/app/outbound/src/http/require_id_header.rs b/linkerd/app/outbound/src/http/require_id_header.rs
@@ -1,5 +1,5 @@
 use futures::{future, TryFutureExt};
-use linkerd_app_core::{dns, identity, svc, tls, Conditional, Error};
+use linkerd_app_core::{identity, svc, tls, Conditional, Error};
 use std::task::{Context, Poll};
 use thiserror::Error;
 use tracing::{debug, trace};
@@ -59,8 +59,7 @@ impl<S> RequireIdentity<S> {
     #[inline]
     fn extract_id<B>(req: &mut http::Request<B>) -> Option<identity::Id> {
         let v = req.headers_mut().remove(HEADER_NAME)?;
-        let n = v.to_str().ok()?.parse::<dns::Name>().ok()?;
-        Some(n.into())
+        v.to_str().ok()?.parse().ok()
     }
 }
 

diff --git a/linkerd/app/outbound/src/opaq/concrete.rs b/linkerd/app/outbound/src/opaq/concrete.rs
@@ -283,14 +283,15 @@ impl<T> svc::Param<tls::ConditionalClientTls> for Endpoint<T> {
         self.metadata
             .identity()
             .cloned()
-            .map(move |server_id| {
-                let alpn = if use_transport_header {
+            .map(move |mut client_tls| {
+                client_tls.alpn = if use_transport_header {
                     use linkerd_app_core::transport_header::PROTOCOL;
                     Some(tls::client::AlpnProtocols(vec![PROTOCOL.into()]))
                 } else {
                     None
                 };
-                tls::ConditionalClientTls::Some(tls::ClientTls::new(server_id, alpn))
+
+                tls::ConditionalClientTls::Some(client_tls)
             })
             .unwrap_or(tls::ConditionalClientTls::None(
                 tls::NoClientTls::NotProvidedByServiceDiscovery,