identity: add spire identity client

[zaharidichev]

This PR adds SPIRE identity client that allows us to connect to the SPIRE workload API and stream new certificates through the identity::Credentials API. Notable changes include:

• added spire-proto crate that includes the workload API protobuf definitions and codegen utilities
• added spire-client crate that contains all the facilities to connect to the SPIRE UDS socket and consume updates
• moved the Identity metrics into a seprate crate so it can be reused across both the control plane and spire identity clients

TODO:

• add more tests
• validate and use trust roots received from SPIRE

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

[codecov]

Codecov Report

Merging #2580 (47f6887) into main (466f88c) will decrease coverage by 0.19%.
The diff coverage is 56.14%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2580      +/-   ##
==========================================
- Coverage   67.83%   67.64%   -0.19%     
==========================================
  Files         328      332       +4     
  Lines       14989    15125     +136     
==========================================
+ Hits        10168    10232      +64     
- Misses       4821     4893      +72     

Files	Coverage Δ
linkerd/app/core/src/lib.rs	87.50% <ø> (ø)
linkerd/app/src/env.rs	60.16% <100.00%> (ø)
linkerd/meshtls/tests/util.rs	97.19% <100.00%> (+0.35%)	⬆️
spiffe-proto/tests/bootstrap.rs	85.71% <85.71%> (ø)
linkerd/app/src/lib.rs	88.57% <50.00%> (ø)
linkerd/proxy/spire-client/src/lib.rs	73.33% <73.33%> (ø)
linkerd/app/src/identity.rs	75.00% <69.44%> (-15.48%)	⬇️
linkerd/app/src/spire.rs	0.00% <0.00%> (ø)
linkerd/proxy/spire-client/src/api.rs	40.29% <40.29%> (ø)

... and 4 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 466f88c...47f6887. Read the comment docs.

[mateiidavid]

I know this is still marked as draft but I had a look just to understand the changes. I won't be in for a while after today. Left some drive-by comments.

[zaharidichev]

@olix0r

I have:

• moved the GRPC client into app/src as we discussed
• added more tests
• wired the Spire task into the identity without externalizing these capabilities through the config

[olix0r]

Is it possible to move this into the Provider type to avoid having redundant enums?

[zaharidichev]

This is used in the App, here. It was returning a control::ControlAddr before.

[olix0r]

Here's an alternative approach to this (against main) #2617 -- updating these metrics to reflect best practices (e.g. _total vs _count); it also means neither clients need to be explicitly aware of these metrics.

[olix0r]

It looks like we're passing the server name as an ID into the identity watcher. That's wrong -- right? Is there a test we could run/write that would fail? Does this mean that the watch function doesn't actually need the identity?

https://github.com/linkerd/linkerd2-proxy/pull/2580/files#diff-cd5071685f69bb2bcf8101f58010eb26c5da18905f59fcdce85c7a1745d609c4R75-R80

In general, I think we my want to reorganize app::identity a little for consistency, modularization, etc.

It still feels icky (not a blocker) to me that we have an Addr enum, when the Config (neé Provider) already holds these addrs in effectively the same enum.

My gut tells me we're going against the grain here, and we need to restructure some of the app code for this. I did a quick and dirty attempt here.

[olix0r]

We tend not to use string errors, instead using typed errors via the thiserror crate.

#[derive(Debug, thiserror::Error)]
#[error("no matching SVID found")]
pub struct NoMatchingSVIDFound(());

Note that the unit field ensures the error can't be built outside of the module.

Suggested change

	Err("could not find an SVID".into())
	Err(NoMatchingSVIDFound(()).into())

[olix0r]

nit:

Suggested change

	if let Err(error) = process_svid(&mut credentials, svid_update, id) {
	tracing::error!("Error processing SVID update: {}", error);
	}
	if let Err(error) = updates.changed().await {
	tracing::debug!("SVID watch closed; terminating {}", error);
	return;
	}
	if let Err(error) = process_svid(&mut credentials, svid_update, id) {
	tracing::error!(%error, "Error processing SVID update");
	}
	if updates.changed().await.is_err() {
	tracing::debug!("SVID watch closed; terminating");
	return;
	}

We generally use field interpolation for errors (rather than direct string formatting).

Also, the error type from changed() is uninteresting -- it can only indicate that all receivers were dropped.

[olix0r]

This is a bit of yakshave that we can do as a followup, but I think we really ought to have a linkerd_identity::PrivateKey type so we're not passing around sensitive byte arrays everywhere.

Why does the private key need to be crate-visible?

[olix0r]

Ah, I see this is for process_svid -- what if we moved process_svid into this module and made that pub(super) -- this gives us a narrower module boundary where we can make all of the Svid stuff private

[olix0r]

LGTM. One last (I think ;) nit

[olix0r]

Ideally we'd use thiserror again here. Since is up at the app layer I don't care as much as I would in a lib.

[olix0r]

Suggested change

	let client = client.ready().await.expect("should be ready");
	match client.call(()).await {
	Ok(rsp) => consume_updates(&self.id, rsp.into_inner(), credentials).await,
	Err(error) => error!(%error, "could not establish SVID stream"),
	}
	let client = client.ready().await.expect("should be ready");
	let rsp = client.call(()).await.expect("spire client must gracefully handle errors");
	consume_updates(&self.id, rsp.into_inner(), credentials).await

Based on our earlier convo, I'm pretty sure that a failure here indicates something pathological (i.e., a bad request from the client). If so, it seems better to panic than to log and never satisfy an identity.

[olix0r]

🌮