Extract TLS id verification out of TLS backends #2507

zaharidichev · 2023-11-08T09:17:14Z

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

zaharidichev · 2023-11-08T09:20:39Z

linkerd/meshtls/rustls/src/creds/store.rs

-        let server_name = match hello.server_name() {
-            Some(name) => {
-                let name = webpki::DnsNameRef::try_from_ascii_str(name)
-                    .expect("server name must be a valid server name");
-                webpki::SubjectNameRef::DnsName(name)
-            }
-            None => {
-                debug!("no SNI -> no certificate");
-                return None;
-            }
-        };
-
-        // Verify that our certificate is valid for the given SNI name.
-        let c = self.0.cert.first()?;
-        if let Err(error) = webpki::EndEntityCert::try_from(c.as_ref())
-            .and_then(|c| c.verify_is_valid_for_subject_name(server_name))
-        {
-            debug!(%error, "Local certificate is not valid for SNI");
-            return None;
-        };
-


This is removed, because if we assume that our certificate can holds its Id in either URI or DNS SAN, we cannot pose the restriction that the Server Name that is contained in the ClientHello matches the value of the SAN. This further begs the question of whether we should use a resolver at all. An alternative approach is to just set the certificates that will be used when we construct the tls config.

Is an analogous change necessary on the boring side?

No, not really. In boring we set the certificate that is going to be used, rather than relying on a resolver to pick it. There is the option to do that here as well. Not sure why the resolver has been picked as an approach. Maybe to add an extra check to ensure that the cert will have the SNI from the ClientHello.

I expect it's so that the TLS library may vary the certificates it presents based on the SNI value.

zaharidichev · 2023-11-08T09:21:38Z

linkerd/meshtls/rustls/src/creds/verify.rs

+};
+use tracing::trace;
+
+pub(crate) struct AnySANVerifier(Arc<RootCertStore>);


Not too sure about the naming.

olix0r

I have a few low-level suggestions I'll make in a followup, but my initial high-level reaction is (1) this is great and (2) verify_id needs tests :)

We need to ensure that both of our verify_id implementations have the same behavior for a set of Certificate & Id pairs. Note that the linkerd-tls-test-util crate provides some readymade certificates that can be used for testing.

The key point is: by taking this verification logic out of the library--even though we're just copying an implementation in the short term--it becomes our responsibility to ensure correctness going forward.

To that end, perhaps there are rustls tests we can incorporate?

zaharidichev · 2023-11-08T20:43:41Z

@olix0r That makes sense. I will add tests for these in my tomorrow.

hawkw · 2023-11-08T21:38:33Z

We need to ensure that both of our verify_id implementations have the same behavior for a set of Certificate & Id pairs. Note that the linkerd-tls-test-util crate provides some readymade certificates that can be used for testing.

If we need test certs that aren't those already checked in, we may also want to consider the rcgen crate to generate certificates for tests inside the test code, rather than checking in test key material to Git...

linkerd/meshtls/boring/src/client.rs

linkerd/meshtls/boring/src/creds/verify.rs

hawkw · 2023-11-08T21:48:19Z

linkerd/meshtls/rustls/src/client.rs

            server_name,
            config,
        }
    }
 }

+fn extract_cert(c: &rustls::ClientConnection) -> io::Result<&rustls::Certificate> {


i'm not sure if io::Error is really the right return type for this, since the error is not actually IO-related...should we maybe instead change the Service::Error associated type for Connect? Or, does that break some other thing?

linkerd/meshtls/rustls/src/client.rs

linkerd/meshtls/rustls/src/creds/verify.rs

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

linkerd/meshtls/boring/src/client.rs

linkerd/meshtls/boring/src/creds/verify.rs

linkerd/meshtls/rustls/src/creds/verify.rs

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

linkerd/meshtls/src/verify_tests.rs

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

olix0r · 2023-11-13T19:44:20Z

Note that we're going to have to allow duplicate versions of base64 in deny.toml

linkerd/meshtls/Cargo.toml

linkerd/meshtls/boring/src/client.rs

linkerd/meshtls/rustls/src/creds/verify.rs

linkerd/meshtls/rustls/src/client.rs

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

linkerd/meshtls/rustls/src/creds/verify.rs

olix0r

Excellent! Thank you.

hawkw

some last nits, but overall, this looks good to me!

hawkw · 2023-11-14T20:25:34Z

linkerd/meshtls/rustls/src/client.rs

+    match c.peer_certificates().and_then(|certs| certs.first()) {
+        Some(leaf_cert) => io::Result::Ok(leaf_cert),
+        None => Err(io::Error::new(io::ErrorKind::Other, "missing tls end cert")),
+    }


tiny nit: isn't this match just Option::ok_or_else?

hawkw · 2023-11-14T20:26:15Z

linkerd/meshtls/rustls/src/creds/store.rs

-        debug!("Certified");
-        Ok(())
+
+        // verify the id as the cert verifier does not do that (on purpose)


nit:

Suggested change

// verify the id as the cert verifier does not do that (on purpose)

// verify the identity, as the cert verifier does not do that (on purpose)

it might also be good if this comment explained why we don't do that --- right now this just says it's "on purpose", but it doesn't say why...

olix0r · 2023-11-14T20:27:58Z

I'm going to merge this through but feel free to address nits in followups!

* Include server address in server error logs (linkerd/linkerd2-proxy#2500) * dev: v42 (linkerd/linkerd2-proxy#2501) * Separate tls::ServerName and identity::Id types (linkerd/linkerd2-proxy#2506) * Use reference-counted strings in dns::Name (linkerd/linkerd2-proxy#2509) * build(deps): bump tj-actions/changed-files from 39.2.0 to 40.1.1 (linkerd/linkerd2-proxy#2508) * build(deps): bump actions/checkout from 4.1.0 to 4.1.1 (linkerd/linkerd2-proxy#2485) * ci: Fix check-each workflow (linkerd/linkerd2-proxy#2511) * ci: Turn off debuginfo in ci test builds (linkerd/linkerd2-proxy#2512) * ci: Fix fuzzer listing (linkerd/linkerd2-proxy#2513) * Use heap indirection to manage type signatures (linkerd/linkerd2-proxy#2510) * dev: optimize image build (linkerd/linkerd2-proxy#2452) * dev: Disable nightly install for now (linkerd/linkerd2-proxy#2515) * meshtls: Extract TLS id verification out of TLS backends (linkerd/linkerd2-proxy#2507) * Update DEFAULT_OUTBOUND_TCP_QUEUE_CAPACITY to 10K (linkerd/linkerd2-proxy#2521) * admin: Add optional /debug/pprof/profile endpoint (linkerd/linkerd2-proxy#2516) Signed-off-by: Oliver Gould <ver@buoyant.io>

* Include server address in server error logs (linkerd/linkerd2-proxy#2500) * dev: v42 (linkerd/linkerd2-proxy#2501) * Separate tls::ServerName and identity::Id types (linkerd/linkerd2-proxy#2506) * Use reference-counted strings in dns::Name (linkerd/linkerd2-proxy#2509) * build(deps): bump tj-actions/changed-files from 39.2.0 to 40.1.1 (linkerd/linkerd2-proxy#2508) * build(deps): bump actions/checkout from 4.1.0 to 4.1.1 (linkerd/linkerd2-proxy#2485) * ci: Fix check-each workflow (linkerd/linkerd2-proxy#2511) * ci: Turn off debuginfo in ci test builds (linkerd/linkerd2-proxy#2512) * ci: Fix fuzzer listing (linkerd/linkerd2-proxy#2513) * Use heap indirection to manage type signatures (linkerd/linkerd2-proxy#2510) * dev: optimize image build (linkerd/linkerd2-proxy#2452) * dev: Disable nightly install for now (linkerd/linkerd2-proxy#2515) * meshtls: Extract TLS id verification out of TLS backends (linkerd/linkerd2-proxy#2507) * Update DEFAULT_OUTBOUND_TCP_QUEUE_CAPACITY to 10K (linkerd/linkerd2-proxy#2521) * admin: Add optional /debug/pprof/profile endpoint (linkerd/linkerd2-proxy#2516) * proxy: Use debian12 distroless base image Signed-off-by: Oliver Gould <ver@buoyant.io>

zaharidichev requested a review from a team as a code owner November 8, 2023 09:17

zaharidichev commented Nov 8, 2023

View reviewed changes

olix0r reviewed Nov 8, 2023

View reviewed changes

hawkw reviewed Nov 8, 2023

View reviewed changes

zaharidichev added 2 commits November 9, 2023 08:30

extract TLS id verification out of tls backends

f1d702c

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

feedback

03ceeb5

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

zaharidichev force-pushed the zd/extract-tls-id-verification branch from 4de0156 to 03ceeb5 Compare November 9, 2023 10:21

zaharidichev added 3 commits November 9, 2023 10:57

add cert SAN verification tests

8515481

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

lint

48eedcf

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

move rcgen to dev deps

2e2a522

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

zaharidichev requested review from olix0r and hawkw November 9, 2023 16:18

hawkw reviewed Nov 9, 2023

View reviewed changes

zaharidichev requested a review from hawkw November 10, 2023 15:02

dry tests

e508425

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

zaharidichev force-pushed the zd/extract-tls-id-verification branch from 6dca7be to e508425 Compare November 10, 2023 15:09

hawkw reviewed Nov 10, 2023

View reviewed changes

linkerd/meshtls/src/verify_tests.rs Outdated Show resolved Hide resolved

unmacro tests

67ba81b

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

zaharidichev requested a review from hawkw November 13, 2023 16:51

olix0r reviewed Nov 13, 2023

View reviewed changes

linkerd/meshtls/Cargo.toml Outdated Show resolved Hide resolved

olix0r reviewed Nov 13, 2023

View reviewed changes

linkerd/meshtls/boring/src/client.rs Outdated Show resolved Hide resolved

olix0r reviewed Nov 13, 2023

View reviewed changes

linkerd/meshtls/rustls/src/creds/verify.rs Show resolved Hide resolved

olix0r reviewed Nov 13, 2023

View reviewed changes

linkerd/meshtls/rustls/src/client.rs Show resolved Hide resolved

zaharidichev added 2 commits November 14, 2023 12:05

more feedback

546bad6

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

add exception for different deps versions

fee54bb

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

zaharidichev requested a review from olix0r November 14, 2023 13:53

olix0r reviewed Nov 14, 2023

View reviewed changes

linkerd/meshtls/rustls/src/creds/verify.rs Outdated Show resolved Hide resolved

olix0r approved these changes Nov 14, 2023

View reviewed changes

Update linkerd/meshtls/rustls/src/creds/verify.rs

aeab64a

hawkw approved these changes Nov 14, 2023

View reviewed changes

olix0r merged commit 2ad49d6 into linkerd:main Nov 14, 2023
84 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract TLS id verification out of TLS backends #2507

Extract TLS id verification out of TLS backends #2507

zaharidichev commented Nov 8, 2023

zaharidichev Nov 8, 2023

olix0r Nov 13, 2023 •

edited

Loading

zaharidichev Nov 14, 2023

olix0r Nov 14, 2023

zaharidichev Nov 8, 2023

olix0r left a comment •

edited

Loading

zaharidichev commented Nov 8, 2023

hawkw commented Nov 8, 2023

hawkw Nov 8, 2023

olix0r commented Nov 13, 2023

olix0r left a comment

hawkw left a comment

hawkw Nov 14, 2023

hawkw Nov 14, 2023

olix0r commented Nov 14, 2023

	// verify the id as the cert verifier does not do that (on purpose)
	// verify the identity, as the cert verifier does not do that (on purpose)

Extract TLS id verification out of TLS backends #2507

Extract TLS id verification out of TLS backends #2507

Conversation

zaharidichev commented Nov 8, 2023

zaharidichev Nov 8, 2023

Choose a reason for hiding this comment

olix0r Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

zaharidichev Nov 14, 2023

Choose a reason for hiding this comment

olix0r Nov 14, 2023

Choose a reason for hiding this comment

zaharidichev Nov 8, 2023

Choose a reason for hiding this comment

olix0r left a comment • edited Loading

Choose a reason for hiding this comment

zaharidichev commented Nov 8, 2023

hawkw commented Nov 8, 2023

hawkw Nov 8, 2023

Choose a reason for hiding this comment

olix0r commented Nov 13, 2023

olix0r left a comment

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

hawkw Nov 14, 2023

Choose a reason for hiding this comment

hawkw Nov 14, 2023

Choose a reason for hiding this comment

olix0r commented Nov 14, 2023

olix0r Nov 13, 2023 •

edited

Loading

olix0r left a comment •

edited

Loading