Add support for SPIFFE TLS Ids #2520

zaharidichev · 2023-11-16T14:57:52Z

This PR adds support for TLS ids that are in SPIFFE format (URI). Notable changes:

san verification logic has been extracted into a separate crate (llinkerd-meshtls-util)
tests for ID extraction and validation logic are happening in this crate
the linkerd_identity::Id type has been turned into an enum that can be either a DNS URI type
the Metadata type now carries both ServerName and ServerId information

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

hawkw · 2023-11-16T20:50:04Z

linkerd/identity/src/lib.rs

-/// well.
+/// Practically speaking, this could be:
+/// - a DNS-like identity string that matches an x509 DNS SAN
+/// - a SPIFFE ID in URI form, that matches an x509


that matches an x509...what?

hawkw · 2023-11-16T20:51:27Z

linkerd/identity/src/lib.rs

+    Dns(linkerd_dns_name::Name),
+    Uri(http::Uri),


style nit, take it or leave it: consider maybe just moving the bulleted list to comments on the variants?

Suggested change

Dns(linkerd_dns_name::Name),

Uri(http::Uri),

/// A a DNS-like identity string that matches an x509 DNS SAN.

Dns(linkerd_dns_name::Name),

/// A SPIFFE ID in URI form, that matches an x509

Uri(http::Uri),

hawkw · 2023-11-16T20:53:14Z

linkerd/identity/src/lib.rs

+    Dns(linkerd_dns_name::Name),
+    Uri(http::Uri),


is the Id::Uri variant intended to be SPIFFE-specific? we might want to name the variant something like SpiffeUri or Spiffe, to make that clearer, especially if we later support other identity mechanisms that also use URIs?

I would argue that this type shouldn't make any SPIFFE requirements. That seems like more of an x509-handling concern than a restriction we care about in linkerd's core types.

Okay, that's kind of the question I was getting at --- I wasn't sure whether this variant represented a spiffe-specific thing or not.

hawkw · 2023-11-16T20:53:33Z

linkerd/identity/src/lib.rs

-        // TODO support spiffe:// URIs.
+        if s.starts_with(SPIFFE_URI_SCHEME) {
+            // TODO: we need to ensure the SPIFFE id is valid
+            // according to requirements otlined in:


typo:

Suggested change

// according to requirements otlined in:

// according to requirements outlined in:

hawkw · 2023-11-16T20:57:25Z

linkerd/identity/src/lib.rs

-        self.0.as_str().into()
+        match self {
+            Self::Dns(dns) => dns.as_str().into(),
+            Self::Uri(uri) => uri.to_string().into(),


it's quite unfortunate that this allocates (in to_string()) --- the DNS version just borrows the string. AFAICT there is no way to borrow an underlying string from an http::Uri, as the parts of the URI may have come from different string slices --- it's intended more as a builder for URIs to be sent in HTTP requests.

i wonder if representing the Uri variant using http::Uri is actually the best representation, since we use Id::to_str a lot, and it's a shame to have to allocate a new String to format the URI into every time we want to log it or whatever. we could, alternatively, change the representation to store both an http::Uri and an Arc<str> or something, so that the formatting of the URI into a string representation only happens once, and subsequent to_str calls just borrow that? I dunno...

hawkw · 2023-11-16T20:59:04Z

linkerd/meshtls/boring/src/server.rs

+                let der = cert
+                    .to_der()
+                    .map_err(
+                        |error| tracing::warn!(%error, "Failed to encode client end cert to der"),
+                    )
+                    .ok()?;
+
+                util::client_identity(&der).map(ClientId)


TIOLI: might be nicer as

Suggested change

let der = cert

.to_der()

.map_err(

|error| tracing::warn!(%error, "Failed to encode client end cert to der"),

)

.ok()?;

util::client_identity(&der).map(ClientId)

match cert.to_der() {

Ok(der) => util::client_identity(&der).map(ClientId),

Err(error) => {

tracing::warn!(%error, "Failed to encode client end cert to der");

None

}

}

hawkw · 2023-11-16T21:35:52Z

linkerd/meshtls/util/Cargo.toml

@@ -1,11 +1,19 @@
 [package]
-name = "linkerd-meshtls-test-util"
+name = "linkerd-meshtls-util"


nit, take it or leave it: i might have called this linkerd-meshtls-core, following the naming scheme we use elsewhere in the project?

olix0r · 2023-11-17T16:24:01Z

linkerd/proxy/api-resolve/src/metadata.rs

@@ -18,7 +19,7 @@ pub struct Metadata {
    tagged_transport_port: Option<u16>,

    /// How to verify TLS for the endpoint.
-    identity: Option<ServerId>,
+    identity: Option<(ServerId, ServerName)>,


Can we perhaps change this to store a tls::ClientTls? This also opens the option to pass through ALPN settings (later).

tls: Option<tls::ClientTls>,

I'd hope this makes the handling of names/ids less verbose throughout most of the app code.

This would also make sense as a discrete change to help minimize the diff we need to consider for URI handling.

olix0r · 2023-11-17T16:48:02Z

linkerd/meshtls/util/src/lib.rs

+
+fn extract_ids_from_cert(cert: &[u8]) -> Result<Vec<Id>> {
+    use x509_parser::prelude::*;
+    let (_, c) = X509Certificate::from_der(cert)?;


Since this changes our DNSName validation, I think we should make this change discretely from the rest of this PR. I.e. a targetted PR that adds a new linkerd-meshtls-verifier crate using x509_parser, without URI handling.

zaharidichev · 2023-11-20T07:51:29Z

@hawkw @olix0r Thanks for the thoughtful feedback, I am closing this PR now in order to split it into smaller, more focused changes, starting with #2534

add support for SPIFFE TLS Ids

2f8c4a0

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

zaharidichev requested a review from a team as a code owner November 16, 2023 14:57

hawkw reviewed Nov 16, 2023

View reviewed changes

olix0r self-assigned this Nov 17, 2023

olix0r reviewed Nov 17, 2023

View reviewed changes

zaharidichev closed this Nov 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for SPIFFE TLS Ids #2520

Add support for SPIFFE TLS Ids #2520

zaharidichev commented Nov 16, 2023

hawkw Nov 16, 2023

hawkw Nov 16, 2023

hawkw Nov 16, 2023

olix0r Nov 17, 2023

hawkw Nov 17, 2023

hawkw Nov 16, 2023

hawkw Nov 16, 2023

hawkw Nov 16, 2023

hawkw Nov 16, 2023

olix0r Nov 17, 2023 •

edited

Loading

olix0r Nov 17, 2023 •

edited

Loading

olix0r Nov 17, 2023 •

edited

Loading

zaharidichev commented Nov 20, 2023

	// according to requirements otlined in:
	// according to requirements outlined in:

Add support for SPIFFE TLS Ids #2520

Add support for SPIFFE TLS Ids #2520

Conversation

zaharidichev commented Nov 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olix0r Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

olix0r Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

olix0r Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

zaharidichev commented Nov 20, 2023

olix0r Nov 17, 2023 •

edited

Loading

olix0r Nov 17, 2023 •

edited

Loading

olix0r Nov 17, 2023 •

edited

Loading