feat: implements crl support in zTunnel #1660

nilekhc · 2025-10-30T02:55:26Z

This PR extends the work of CRL support by consuming the ca-crl config map in zTunnel and validates the request accordingly.

Fixes #1678

istio-testing · 2025-10-30T02:55:29Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

istio-policy-bot · 2025-10-30T02:55:30Z

😊 Welcome @nilekhc! This is either your first contribution to the Istio ztunnel repo, or it's been
a while since you've been here.

You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

nilekhc · 2025-10-30T02:56:42Z

/test all

howardjohn · 2025-10-30T03:04:16Z

I'm a bit confused. It seems there was a design doc that was approved for this (not sure by who, as I was out of office and there are no meeting notes or listed approvers) but the design is completely different than this

nilekhc · 2025-10-30T15:34:23Z

/test all

nilekhc · 2025-10-30T16:17:14Z

I'm a bit confused. It seems there was a design doc that was approved for this (not sure by who, as I was out of office and there are no meeting notes or listed approvers) but the design is completely different than this

Hi @howardjohn, Thanks for the comment. The original design I presented in one of the community meetings received some valuable feedback. Based on that, I pivoted the design to propagate CRL information through a ConfigMap, similar to how the ca-root-cert is propogated. I’ve captured these updates in the same design document, under the Implementation section.

This PR simply uses that CRL ConfigMap and validates the request accordingly.

cc @costinm @therealmitchconnors @keithmattix

nilekhc · 2025-10-30T16:20:55Z

/test all

nilekhc · 2025-10-30T16:35:20Z

/test all

nilekhc · 2025-10-30T16:44:50Z

src/proxy.rs

                socket2::SockRef::from(&s).set_tcp_keepalive(&ka)
            );
        }
+        #[cfg(target_os = "linux")]


I was getting an error on macos due to cross compilation. I have to add this to some places to fix it. Please let me know if there is a better way to handle this.

nilekhc · 2025-10-30T16:47:23Z

/retest

howardjohn · 2025-10-30T16:58:46Z

src/proxy/connection_manager.rs

+
+    /// Close all inbound and outbound connections
+    /// This is called when certificate revocations are detected
+    pub fn close_all_connections(&self) {


This seems really dangerous and bad behavior. How strongly do you feel about keeping this?

Does any other application do this?

+1 I don't think we should be closing every single connection just because of one revocation. What does Envoy do?

The only reason I addd this since http connections are long lived and does not check updated crl. Is there a better way to handle this? I know envoy has some timeout we can configure. Not comepletly sure how zTunnel handles it. Happy to tweak this.

My intuition is to do nothing. But at the very least, only close ones that are revoked

Doing nothing feels wrong IMO; the purpose of a CRL is to be able to break a particular trust chain, and you don't want existing connections to continue. Only closing the ones that are revoked is the right solution to me

Is restart your clients if you want the CRL to take effect quickly the UX we want then?

imo, zTunnel should terminate that connection selectively and avoid restarting the workload. Since the user provides/updates the CRL file, they’re already aware of potentially revoked certificates, so why ask them to take action twice?
(1) Create or update the CRL, and
(2) Restart the workload pod.

Playing advocate aside, I suspect that most folks would accept a limitation where existing connections are not re-evaluated on a CRL refresh. This seems to be a fairly common behavior. I think I would lean towards breaking support for re-evaluating existing connections into a separate PR (assuming we do want to explore that functionality).

Separate PR seems reasonable to me so we can evaluate the complexity on its own

I have reverted the outbound related changes and now it only selectively terminates inbound connections. PTAL. Thanks

/cc @howardjohn @keithmattix @ilrudie

therealmitchconnors · 2025-10-30T18:02:16Z

I'm a bit confused. It seems there was a design doc that was approved for this (not sure by who, as I was out of office and there are no meeting notes or listed approvers) but the design is completely different than this

If you'd like to see the conversation where this was reviewed, it's in the 4/14 wg meeting at https://www.youtube.com/watch?v=welIiuqUvn0&t=118s

Signed-off-by: nilekh <[email protected]>

nilekhc · 2025-11-20T22:23:25Z

@howardjohn @ilrudie @keithmattix Could you PTAL when you get a chance? Thanks.

Signed-off-by: nilekh <[email protected]>

howardjohn · 2025-11-24T22:01:51Z

src/proxy/connection_manager.rs

+                serial.len(),
+                serial
+                    .iter()
+                    .map(|b| format!("{:02x}", b))


why are we converting to string here and on L372? it will be quite expensive, we can just compare the bytes directly?

ilrudie

It seems to me this bundles functionality that belongs in something like PolicyWatcher into the ConnectionManager. Please take a look at that, specifically how run works and is invoked and works for an example of how similar functionality was implemented.

ilrudie · 2025-11-24T22:47:06Z

src/proxy/inbound.rs

+                    let rbac_ctx = ProxyRbacContext {
+                        conn: conn.clone(),
+                        dest_workload: destination_workload,
+                    };
+                    let mut tls_guard = pi
+                        .connection_manager
+                        .register_tls_connection(&rbac_ctx, client_cert_serials.clone());
+                    debug!("TLS connection registered for revocation tracking");
+
+                    // Get watcher BEFORE serving to listen for revocation-based termination
+                    let tls_drain_watch = tls_guard.watcher();


serv_connect will already create a connection guard which will end the connection when asked. I don't think we need a second connection guard

ilrudie · 2025-11-24T22:47:44Z

src/proxy/inbound.rs

+                    // This allows CRL revocation to terminate the connection immediately
+                    tokio::select! {
+                        result = serve => {
+                            tls_guard.release();
+                            result
+                        }
+                        _ = tls_drain_watch.wait_for_drain() => {
+                            debug!("TLS connection terminated due to certificate revocation");
+                            tls_guard.release();
+                            // Return an error to signal abnormal termination
+                            // This helps with proper cleanup and error reporting
+                            Err(std::io::Error::new(
+                                std::io::ErrorKind::ConnectionAborted,
+                                "connection terminated due to certificate revocation"
+                            ).into())
+                        }
+                    }


similar to above, we already take a guard that closes the connection, I don't think we need

istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Oct 30, 2025

istio-testing added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Oct 30, 2025

nilekhc changed the title ~~feat: implements ca-crl in zTunnel~~ feat: implements crl support in zTunnel Oct 30, 2025

nilekhc force-pushed the nilekh/f/crl-support branch from 6cfcad3 to c522f5f Compare October 30, 2025 15:33

nilekhc force-pushed the nilekh/f/crl-support branch from c522f5f to ce5886a Compare October 30, 2025 16:20

nilekhc force-pushed the nilekh/f/crl-support branch from ce5886a to 2402c4d Compare October 30, 2025 16:34

istio-testing added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 30, 2025

nilekhc marked this pull request as ready for review October 30, 2025 16:43

nilekhc requested a review from a team as a code owner October 30, 2025 16:43

istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Oct 30, 2025

nilekhc commented Oct 30, 2025

View reviewed changes

nilekhc mentioned this pull request Oct 30, 2025

feat: adds crl volume to the zTunnel chart istio/istio#58132

Merged

17 tasks

howardjohn reviewed Oct 30, 2025

View reviewed changes

nilekhc requested review from howardjohn, ilrudie and keithmattix October 30, 2025 20:32

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Nov 18, 2025

nilekhc added 2 commits November 18, 2025 14:45

feat: implements ca-crl support in zTunnel

9207907

Signed-off-by: nilekh <[email protected]>

chore: drains connection only for revoked cert

361dac3

Signed-off-by: nilekh <[email protected]>

nilekhc force-pushed the nilekh/f/crl-support branch from 5509cf6 to 37f0ca0 Compare November 19, 2025 01:11

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Nov 19, 2025

nilekhc force-pushed the nilekh/f/crl-support branch 2 times, most recently from 11a8b0b to 0f855bf Compare November 19, 2025 19:15

feat: revokes affected inbound connections only

01540f7

Signed-off-by: nilekh <[email protected]>

nilekhc force-pushed the nilekh/f/crl-support branch from 0f855bf to 01540f7 Compare November 20, 2025 23:27

nilekhc mentioned this pull request Nov 20, 2025

test: implements integration tests for CRL support in zTunnel istio/istio#58367

Draft

howardjohn reviewed Nov 24, 2025

View reviewed changes

ilrudie requested changes Nov 24, 2025

View reviewed changes

feat: implements crl support in zTunnel #1660

Are you sure you want to change the base?

feat: implements crl support in zTunnel #1660

Uh oh!

Conversation

nilekhc commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

istio-testing commented Oct 30, 2025

Uh oh!

istio-policy-bot commented Oct 30, 2025

Uh oh!

nilekhc commented Oct 30, 2025

Uh oh!

howardjohn commented Oct 30, 2025

Uh oh!

nilekhc commented Oct 30, 2025

Uh oh!

nilekhc commented Oct 30, 2025

Uh oh!

nilekhc commented Oct 30, 2025

Uh oh!

nilekhc commented Oct 30, 2025

Uh oh!

nilekhc Oct 30, 2025 • 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!

nilekhc commented Oct 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nilekhc Oct 30, 2025 • 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!

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!

therealmitchconnors commented Oct 30, 2025

Uh oh!

nilekhc commented Nov 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilrudie 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

nilekhc commented Oct 30, 2025 •

edited

Loading

nilekhc Oct 30, 2025 •

edited

Loading

nilekhc Oct 30, 2025 •

edited

Loading