linkerd_identity: split `linkerd_identity::Id` into DNS and URI variants by zaharidichev · Pull Request #2538 · linkerd/linkerd2-proxy

zaharidichev · 2023-11-28T16:51:00Z

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/identity/src/lib.rs

zaharidichev · 2023-11-28T16:58:04Z

linkerd/app/inbound/src/policy.rs

-                    || suffixes.iter().any(|s| s.contains(&id.to_str()))
-            }
+            }) => match id {
+                id::Id::Uri(_) => identities.contains(&*id.to_str()),


does that make sense for now ?

linkerd/app/outbound/src/opaq/concrete.rs

codecov · 2023-11-28T17:01:02Z

Codecov Report

Merging #2538 (2defa19) into main (31b2aea) will increase coverage by 0.05%.
The diff coverage is 84.84%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2538      +/-   ##
==========================================
+ Coverage   67.61%   67.66%   +0.05%     
==========================================
  Files         330      330              
  Lines       14781    14803      +22     
==========================================
+ Hits         9994    10017      +23     
+ Misses       4787     4786       -1

Files	Coverage Δ
linkerd/app/inbound/src/policy.rs	`90.00% <100.00%> (-1.49%)`	⬇️
linkerd/app/outbound/src/http/require_id_header.rs	`50.00% <100.00%> (+1.72%)`	⬆️
linkerd/app/outbound/src/tcp/tagged_transport.rs	`86.95% <ø> (ø)`
linkerd/identity/src/lib.rs	`100.00% <100.00%> (+9.09%)`	⬆️
linkerd/meshtls/tests/util.rs	`96.81% <100.00%> (+0.63%)`	⬆️
linkerd/meshtls/verifier/src/lib.rs	`92.85% <100.00%> (+5.90%)`	⬆️
linkerd/proxy/api-resolve/src/metadata.rs	`100.00% <100.00%> (ø)`
linkerd/proxy/server-policy/src/authz.rs	`71.11% <100.00%> (+1.34%)`	⬆️
linkerd/tls/src/client.rs	`67.21% <100.00%> (+3.72%)`	⬆️
linkerd/app/outbound/src/http/concrete.rs	`66.87% <33.33%> (ø)`
... and 3 more

... and 2 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 31b2aea...2defa19. Read the comment docs.

linkerd/app/outbound/src/opaq/concrete.rs

linkerd/identity/src/lib.rs

linkerd/meshtls/verifier/src/lib.rs

olix0r · 2023-11-30T14:40:33Z

linkerd/identity/src/lib.rs

-pub struct Id(pub linkerd_dns_name::Name);
+pub enum Id {
+    Dns(linkerd_dns_name::Name),
+    Uri(http::Uri),


I'm not sure about the suitability of http::Uri to represent RFC 3986 URIs. The crate documentation makes no claims about satisfying the spec, and is designed to represent the HTTP-specific URI quirks.

I believe our primary options are:

iref

url

We already have a dependency on the url crate:

:; cargo tree -i url --depth 1 url v2.3.0 ├── linkerd-http-route v0.1.0 (/workspaces/linkerd2-proxy/linkerd/http-route) └── trust-dns-proto v0.22.0

so this is probably a more appealing target.

Also, it provides an Url::as_str, so we can avoid allocating strings on each comparison.

olix0r · 2023-11-30T14:42:42Z

linkerd/meshtls/verifier/src/lib.rs

+                            // Wildcards can perhaps be handled in a future path...
+                            return None;
+                        }


I'm curious where this arises from. Is there a bug in the current name parsing? Is this not handled properly by the ID parser?

You mentioned that this was already present but I do not see any other wildcard handling in the diff.

Yes, this was present in the rustls client id extraction code: https://github.com/linkerd/linkerd2-proxy/blob/f4f606555e9c671694f4f21009ba7affe0aaacad/linkerd/meshtls/rustls/src/server.rs#L143C21-L143C21

Seems this has been there for a while. Do you remember the precise reason?

This comes from cd04d7a#diff-6eab3f24cc45f109bf10a46bd6be573f28ebd28d826dd38ffb5a7cb1cca46bc7L135-L143 -- there used to be an explicit variant.

It made sense to document this case when it was an explicit enum case we had to handle, but I don't think it makes any sense to explicitly handle a "*" DNS name: A "*" dns name must not be parsed by Id::parse_dns_name(dns).

olix0r

Is it true that we don't actually enforce any requirements around spiffe:// IDs? This seems fine I just want to confirm that's the case.
Given that, it seems beneficial to have a test that documents the behavior with, for instance, a http:// uri.

olix0r · 2023-11-30T19:56:11Z

linkerd/identity/src/lib.rs

-        self.0.without_trailing_dot().fmt(f)
+        match self {
+            Self::Dns(dns) => dns.without_trailing_dot().fmt(f),
+            Self::Uri(uri) => uri.fmt(f),


Should we test round-tripping an Id through a parser? like:

something like

let id: Id = "uri://host:1234/path".parse().unwrap(); assert_eq!(id, id.to_string().parse().unwrap());

olix0r · 2023-11-30T19:56:43Z

linkerd/identity/src/lib.rs

+    pub fn parse_uri(s: &str) -> Result<Self, InvalidId> {
+        http::Uri::try_from(s)
+            .map(Self::Uri)
+            .map_err(|e| InvalidId(e.into()))


codecov says we never hit InvalidId. is that true? should we test passing a DNS name to parse_uri?

I have added a test for that

zaharidichev · 2023-12-07T08:53:40Z

Is it true that we don't actually enforce any requirements around spiffe:// IDs? This seems fine I just want to confirm that's the case.

Given that, it seems beneficial to have a test that documents the behavior with, for instance, a http:// uri.

It is true. I have added a test for that.

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

olix0r · 2023-12-08T14:33:57Z

deny.toml

+    # `trust-dns-proto`, depends on `idna` v0.2.3 while `url` depends on v0.5.0
+    { name = "idna" },


I looked into this and (1) it appears that trust-dns has rebranded as hickory-dns, so we're going to have to be aware of that when we upgrade trust-dns; (2) and the latest version is on idna v0.4.

Normally, we'd follow up this PR with an upstream PR to update hickory-dns to idna 0.5 so that we can be sure to resolve this when we take the next hickory udpate.

These exceptions are fine temporarily, but each skip here means we're double-building dependencies. We try to keep this overhead manageable over time.

olix0r · 2023-12-08T14:36:00Z

linkerd/app/inbound/Cargo.toml

 linkerd-proxy-client-policy = { path = "../../proxy/client-policy" }
 linkerd-tonic-watch = { path = "../../tonic-watch" }
 linkerd2-proxy-api = { version = "0.12", features = ["inbound"] }
+linkerd-identity = { path = "../../identity" }


This shouldn't be necessary: app-inbound and app-outbound pick up the vast majority of their dependencies via reexport from app-core:

linkerd2-proxy/linkerd/app/core/src/lib.rs

Lines 50 to 54 in 31b2aea

pub mod identity {

pub use linkerd_identity::*;

pub use linkerd_meshtls::*;

pub use linkerd_proxy_identity_client as client;

}

olix0r · 2023-12-08T14:40:03Z

linkerd/app/outbound/src/http/concrete.rs

+                tls::ConditionalClientTls::Some(tls::ClientTls::new(
+                    client_tls.server_id,
+                    client_tls.server_name,
+                    alpn,
+                ))


tioli

.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(client_tls)

alternately -- we could add a ClientTls::with_alpn(self, alpn: Option<AlpnProtocols>) -> ClientTls if you wanted to avoid the mut, so this would end up

.map(move |client_tls| { tls::ConditionalClientTls::Some(client_tls.with_alpn(if use_transport_header { use linkerd_app_core::transport_header::PROTOCOL; Some(tls::client::AlpnProtocols(vec![PROTOCOL.into()])) } else { None }))

olix0r · 2023-12-08T14:40:55Z

linkerd/app/outbound/src/http/require_id_header.rs

        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::<identity::Id>().ok()


tioli -- the explicit annotation is no longer needed because we're not using an intermediate type.

Suggested change

v.to_str().ok()?.parse::<identity::Id>().ok()

v.to_str().ok()?.parse().ok()

olix0r · 2023-12-08T14:41:16Z

linkerd/app/outbound/src/opaq/concrete.rs

                    None
                };
-                tls::ConditionalClientTls::Some(tls::ClientTls::new(server_id, alpn))
+                tls::ConditionalClientTls::Some(tls::ClientTls::new(


same suggestion as above could apply here

olix0r · 2023-12-08T14:42:03Z

linkerd/app/outbound/src/tcp/tagged_transport.rs

+                    tls::ConditionalClientTls::Some(tls::ClientTls::new(
+                        client_tls.server_id,
+                        client_tls.server_name,
+                        alpn,
+                    ))


olix0r · 2023-12-08T14:42:54Z

linkerd/tls/src/client.rs

    // XXX(ver) We'll have to change this when ServerIds are not necessarily DNS names.
-    pub fn new(server_id: ServerId, alpn: Option<AlpnProtocols>) -> Self {
-        let ServerId(linkerd_identity::Id(name)) = server_id.clone();
+    pub fn new(server_id: ServerId, server_name: ServerName, alpn: Option<AlpnProtocols>) -> Self {


I suspect you could leave this unchanged -- avoiding having to specifiy None in many cases -- and explicitly set the alpn field only when an override is necessary.

olix0r

There's nothing blocking merging this, though there are a few small cleanups/followups we should include.

zaharidichev · 2023-12-08T14:59:16Z

@olix0r I will address the followups, and we can merge after

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

This change culminates recent work to restructure the balancer to use a PoolQueue so that balancer changes may occur independently of request processing. This replaces independent discovery buffering so that the balancer task is responsible for polling discovery streams without independent buffering. Requests are buffered and processed as soon as the pool has available backends. Fail-fast circuit breaking is enforced on the balancer's queue so that requests can't get stuck in a queue indefinitely. In general, the new balancer is instrumented directly with metrics, and the relevant metric name prefix and labelset is provided by the stack. In addition to detailed queue metrics including request (in-queue) latency histograms, but also failfast states, discovery updates counts, and balancer endpoint pool sizes. --- * outbound: Move queues into the concrete stack (linkerd/linkerd2-proxy#2539) * metrics: Remove unused features (linkerd/linkerd2-proxy#2542) * Add the PoolQueue middleware (linkerd/linkerd2-proxy#2540) * ci: Fixup codecov config (linkerd/linkerd2-proxy#2545) * ci: Cancel prior runs (linkerd/linkerd2-proxy#2546) * ci: Skip ARM builds during non-release CI (linkerd/linkerd2-proxy#2547) * deps: Update tokio, tonic, and prost (linkerd/linkerd2-proxy#2544) * build(deps): bump tj-actions/changed-files from 40.2.0 to 40.2.1 (linkerd/linkerd2-proxy#2549) * metrics: Use prometheus-client for proxy_build_info (linkerd/linkerd2-proxy#2551) * balance: Add a p2c Pool implementation (linkerd/linkerd2-proxy#2541) * metrics: Export process metrics using prometheus-client (linkerd/linkerd2-proxy#2552) * linkerd_identity: split `linkerd_identity::Id` into DNS and URI variants (linkerd/linkerd2-proxy#2538) * outbound: Move HTTP balancer into its own module (linkerd/linkerd2-proxy#2554) * app: Setup prom registry for use in balancers (linkerd/linkerd2-proxy#2555) * vscode: Move workspace settings to devcontainer (linkerd/linkerd2-proxy#2557) * build(deps): bump tj-actions/changed-files from 40.2.1 to 40.2.2 (linkerd/linkerd2-proxy#2556) * balance: Instrument metrics in pool balancer (linkerd/linkerd2-proxy#2558) * Enable PoolQueue balancer (linkerd/linkerd2-proxy#2559) Signed-off-by: Oliver Gould <[email protected]>

zaharidichev requested a review from a team as a code owner November 28, 2023 16:51

zaharidichev commented Nov 28, 2023

View reviewed changes

hawkw self-assigned this Nov 28, 2023

hawkw reviewed Nov 28, 2023

View reviewed changes

olix0r reviewed Nov 30, 2023

View reviewed changes

zaharidichev added 4 commits December 7, 2023 09:04

split tls id into DNS and URI variants

ca41400

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

feedback

d2242f6

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

make authz fns private again

3e31ee8

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

feedback

0359465

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

zaharidichev force-pushed the zd/add-uri-tls-id branch from 692c9d6 to 0359465 Compare December 7, 2023 09:12

allow two versions of idna

a187e19

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

zaharidichev requested a review from olix0r December 7, 2023 09:37

olix0r reviewed Dec 8, 2023

View reviewed changes

olix0r approved these changes Dec 8, 2023

View reviewed changes

feedback

2defa19

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

zaharidichev force-pushed the zd/add-uri-tls-id branch from eec7bb9 to 2defa19 Compare December 8, 2023 17:03

zaharidichev merged commit c5b1f63 into linkerd:main Dec 8, 2023

olix0r mentioned this pull request Dec 13, 2023

proxy: v2.215.0 linkerd/linkerd2#11750

Merged

		# `trust-dns-proto`, depends on `idna` v0.2.3 while `url` depends on v0.5.0
		{ name = "idna" },

	pub mod identity {
	pub use linkerd_identity::*;
	pub use linkerd_meshtls::*;
	pub use linkerd_proxy_identity_client as client;
	}

	v.to_str().ok()?.parse::<identity::Id>().ok()
	v.to_str().ok()?.parse().ok()

Conversation

zaharidichev commented Nov 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Nov 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

olix0r Nov 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olix0r Nov 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olix0r Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olix0r left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zaharidichev commented Dec 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olix0r Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olix0r left a comment

Choose a reason for hiding this comment

Uh oh!

zaharidichev commented Dec 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zaharidichev commented Nov 28, 2023 •

edited

Loading

codecov bot commented Nov 28, 2023 •

edited

Loading

olix0r Nov 30, 2023 •

edited

Loading

olix0r Nov 30, 2023 •

edited

Loading

olix0r Dec 8, 2023 •

edited

Loading

olix0r Dec 8, 2023 •

edited

Loading