From d96b2dbe2f4f39e8e1248a2649786965a7e8a2de Mon Sep 17 00:00:00 2001 From: nilekh <1626598+nilekhc@users.noreply.github.com> Date: Wed, 29 Oct 2025 10:15:48 -0700 Subject: [PATCH 01/17] feat: implements ca-crl support in zTunnel Signed-off-by: nilekh <1626598+nilekhc@users.noreply.github.com> --- Cargo.lock | 148 +++++++++- Cargo.toml | 3 + fuzz/Cargo.lock | 216 +++++++++++++- src/config.rs | 18 ++ src/proxy.rs | 9 + src/proxy/connection_manager.rs | 33 +++ src/proxy/inbound.rs | 6 +- src/proxy/outbound.rs | 11 +- src/proxy/pool.rs | 54 +++- src/proxy/socks5.rs | 5 + src/proxyfactory.rs | 41 +++ src/socket.rs | 1 + src/test_helpers/app.rs | 8 + src/tls.rs | 1 + src/tls/certificate.rs | 36 ++- src/tls/crl.rs | 479 ++++++++++++++++++++++++++++++++ src/tls/workload.rs | 65 ++++- tests/namespaced.rs | 6 +- 18 files changed, 1112 insertions(+), 28 deletions(-) create mode 100644 src/tls/crl.rs diff --git a/Cargo.lock b/Cargo.lock index 7ce00e92d6..1768da4edf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -880,6 +880,27 @@ version = "2.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "37909eebbb50d72f9059c3b6d82c0463f2ff062c9e95845c43a6c9c0355411be" +[[package]] +name = "file-id" +version = "0.2.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e1fc6a637b6dc58414714eddd9170ff187ecb0933d4c7024d1abbd23a3cc26e9" +dependencies = [ + "windows-sys 0.60.2", +] + +[[package]] +name = "filetime" +version = "0.2.26" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bc0505cd1b6fa6580283f6bdf70a73fcf4aba1184038c90902b92b3dd0df63ed" +dependencies = [ + "cfg-if", + "libc", + "libredox", + "windows-sys 0.60.2", +] + [[package]] name = "find-msvc-tools" version = "0.1.5" @@ -995,6 +1016,15 @@ version = "1.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "42703706b716c37f96a77aea830392ad231f44c9e9a67872fa5548707e11b11c" +[[package]] +name = "fsevent-sys" +version = "4.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "76ee7a02da4d231650c7cea31349b889be2f45ddb3ef3032d2ec8185f6313fd2" +dependencies = [ + "libc", +] + [[package]] name = "fslock" version = "0.2.1" @@ -1567,6 +1597,26 @@ dependencies = [ "hashbrown 0.16.0", ] +[[package]] +name = "inotify" +version = "0.9.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f8069d3ec154eb856955c1c0fbffefbf5f3c40a104ec912d4797314c1801abff" +dependencies = [ + "bitflags 1.3.2", + "inotify-sys", + "libc", +] + +[[package]] +name = "inotify-sys" +version = "0.1.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e05c02b5e89bff3b946cedeca278abc628fe811e604f027c45a8aa3cf793d0eb" +dependencies = [ + "libc", +] + [[package]] name = "instant" version = "0.1.13" @@ -1696,6 +1746,26 @@ dependencies = [ "indexmap", ] +[[package]] +name = "kqueue" +version = "1.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "eac30106d7dce88daf4a3fcb4879ea939476d5074a9b7ddd0fb97fa4bed5596a" +dependencies = [ + "kqueue-sys", + "libc", +] + +[[package]] +name = "kqueue-sys" +version = "1.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ed9625ffda8729b85e45cf04090035ac368927b8cebc34898e7c120f52e4838b" +dependencies = [ + "bitflags 1.3.2", + "libc", +] + [[package]] name = "lazy_static" version = "1.5.0" @@ -1718,6 +1788,17 @@ dependencies = [ "windows-link", ] +[[package]] +name = "libredox" +version = "0.1.12" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3d0b95e02c851351f877147b7deea7b1afb1df71b63aa5f8270716e0c5720616" +dependencies = [ + "bitflags 2.10.0", + "libc", + "redox_syscall 0.7.0", +] + [[package]] name = "linux-raw-sys" version = "0.4.15" @@ -1849,6 +1930,18 @@ dependencies = [ "simd-adler32", ] +[[package]] +name = "mio" +version = "0.8.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a4a650543ca06a924e8b371db273b2756685faae30f8487da1b56505a8f78b0c" +dependencies = [ + "libc", + "log", + "wasi 0.11.1+wasi-snapshot-preview1", + "windows-sys 0.48.0", +] + [[package]] name = "mio" version = "1.1.0" @@ -1975,6 +2068,39 @@ dependencies = [ "minimal-lexical", ] +[[package]] +name = "notify" +version = "6.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6205bd8bb1e454ad2e27422015fb5e4f2bcc7e08fa8f27058670d208324a4d2d" +dependencies = [ + "bitflags 2.10.0", + "crossbeam-channel", + "filetime", + "fsevent-sys", + "inotify", + "kqueue", + "libc", + "log", + "mio 0.8.11", + "walkdir", + "windows-sys 0.48.0", +] + +[[package]] +name = "notify-debouncer-full" +version = "0.3.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fb7fd166739789c9ff169e654dc1501373db9d80a4c3f972817c8a4d7cf8f34e" +dependencies = [ + "crossbeam-channel", + "file-id", + "log", + "notify", + "parking_lot", + "walkdir", +] + [[package]] name = "nu-ansi-term" version = "0.50.3" @@ -2169,7 +2295,7 @@ checksum = "2621685985a2ebf1c516881c026032ac7deafcda1a2c9b7850dc81e3dfcb64c1" dependencies = [ "cfg-if", "libc", - "redox_syscall", + "redox_syscall 0.5.18", "smallvec", "windows-link", ] @@ -2468,7 +2594,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ac6c3320f9abac597dcbc668774ef006702672474aad53c6d596b62e487b40b1" dependencies = [ "heck", - "itertools 0.10.5", + "itertools 0.14.0", "log", "multimap", "once_cell", @@ -2488,7 +2614,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8a56d757972c98b346a9b766e3f02746cde6dd1cd1d1d563472929fdd74bec4d" dependencies = [ "anyhow", - "itertools 0.10.5", + "itertools 0.14.0", "proc-macro2", "quote", "syn 2.0.110", @@ -2501,7 +2627,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9120690fafc389a67ba3803df527d0ec9cbbc9cc45e4cc20b332996dfb672425" dependencies = [ "anyhow", - "itertools 0.10.5", + "itertools 0.14.0", "proc-macro2", "quote", "syn 2.0.110", @@ -2736,6 +2862,15 @@ dependencies = [ "bitflags 2.10.0", ] +[[package]] +name = "redox_syscall" +version = "0.7.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "49f3fe0889e69e2ae9e41f4d6c4c0181701d00e4697b356fb1f74173a5e0ee27" +dependencies = [ + "bitflags 2.10.0", +] + [[package]] name = "regex" version = "1.12.2" @@ -3468,7 +3603,7 @@ checksum = "ff360e02eab121e0bc37a2d3b4d4dc622e6eda3a8e5253d5435ecf5bd4c68408" dependencies = [ "bytes", "libc", - "mio", + "mio 1.1.0", "parking_lot", "pin-project-lite", "signal-hook-registry", @@ -4448,6 +4583,8 @@ dependencies = [ "matches", "netns-rs", "nix 0.30.1", + "notify", + "notify-debouncer-full", "num_cpus", "oid-registry", "once_cell", @@ -4473,6 +4610,7 @@ dependencies = [ "serde_yaml", "socket2 0.6.1", "split-iter", + "tempfile", "test-case", "textnonce", "thiserror 2.0.17", diff --git a/Cargo.toml b/Cargo.toml index ad64fd192b..8a25a00139 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -72,6 +72,8 @@ keyed_priority_queue = "0.4" libc = "0.2" log = "0.4" nix = { version = "0.30", features = ["socket", "sched", "uio", "fs", "ioctl", "user", "net", "mount", "resource" ] } +notify = "6.1" +notify-debouncer-full = "0.3" once_cell = "1.21" num_cpus = "1.16" ppp = "2.3" @@ -156,6 +158,7 @@ oid-registry = "0.8" rcgen = { version = "0.14", features = ["pem", "x509-parser"] } x509-parser = { version = "0.17", default-features = false, features = ["verify"] } ctor = "0.5" +tempfile = "3.21" [lints.clippy] # This rule makes code more confusing diff --git a/fuzz/Cargo.lock b/fuzz/Cargo.lock index ba6cc05983..d4fdf8682b 100644 --- a/fuzz/Cargo.lock +++ b/fuzz/Cargo.lock @@ -755,6 +755,27 @@ version = "2.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "37909eebbb50d72f9059c3b6d82c0463f2ff062c9e95845c43a6c9c0355411be" +[[package]] +name = "file-id" +version = "0.2.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e1fc6a637b6dc58414714eddd9170ff187ecb0933d4c7024d1abbd23a3cc26e9" +dependencies = [ + "windows-sys 0.60.2", +] + +[[package]] +name = "filetime" +version = "0.2.26" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bc0505cd1b6fa6580283f6bdf70a73fcf4aba1184038c90902b92b3dd0df63ed" +dependencies = [ + "cfg-if", + "libc", + "libredox", + "windows-sys 0.60.2", +] + [[package]] name = "findshlibs" version = "0.10.2" @@ -812,6 +833,15 @@ version = "1.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "42703706b716c37f96a77aea830392ad231f44c9e9a67872fa5548707e11b11c" +[[package]] +name = "fsevent-sys" +version = "4.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "76ee7a02da4d231650c7cea31349b889be2f45ddb3ef3032d2ec8185f6313fd2" +dependencies = [ + "libc", +] + [[package]] name = "futures" version = "0.3.31" @@ -1417,6 +1447,26 @@ dependencies = [ "hashbrown", ] +[[package]] +name = "inotify" +version = "0.9.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f8069d3ec154eb856955c1c0fbffefbf5f3c40a104ec912d4797314c1801abff" +dependencies = [ + "bitflags 1.3.2", + "inotify-sys", + "libc", +] + +[[package]] +name = "inotify-sys" +version = "0.1.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e05c02b5e89bff3b946cedeca278abc628fe811e604f027c45a8aa3cf793d0eb" +dependencies = [ + "libc", +] + [[package]] name = "instant" version = "0.1.13" @@ -1519,6 +1569,26 @@ dependencies = [ "indexmap", ] +[[package]] +name = "kqueue" +version = "1.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "eac30106d7dce88daf4a3fcb4879ea939476d5074a9b7ddd0fb97fa4bed5596a" +dependencies = [ + "kqueue-sys", + "libc", +] + +[[package]] +name = "kqueue-sys" +version = "1.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ed9625ffda8729b85e45cf04090035ac368927b8cebc34898e7c120f52e4838b" +dependencies = [ + "bitflags 1.3.2", + "libc", +] + [[package]] name = "lazy_static" version = "1.5.0" @@ -1557,6 +1627,17 @@ dependencies = [ "windows-targets 0.52.6", ] +[[package]] +name = "libredox" +version = "0.1.10" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "416f7e718bdb06000964960ffa43b4335ad4012ae8b99060261aa4a8088d5ccb" +dependencies = [ + "bitflags 2.8.0", + "libc", + "redox_syscall", +] + [[package]] name = "linux-raw-sys" version = "0.4.15" @@ -1670,6 +1751,18 @@ dependencies = [ "adler2", ] +[[package]] +name = "mio" +version = "0.8.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a4a650543ca06a924e8b371db273b2756685faae30f8487da1b56505a8f78b0c" +dependencies = [ + "libc", + "log", + "wasi 0.11.0+wasi-snapshot-preview1", + "windows-sys 0.48.0", +] + [[package]] name = "mio" version = "1.0.3" @@ -1772,6 +1865,39 @@ dependencies = [ "minimal-lexical", ] +[[package]] +name = "notify" +version = "6.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6205bd8bb1e454ad2e27422015fb5e4f2bcc7e08fa8f27058670d208324a4d2d" +dependencies = [ + "bitflags 2.8.0", + "crossbeam-channel", + "filetime", + "fsevent-sys", + "inotify", + "kqueue", + "libc", + "log", + "mio 0.8.11", + "walkdir", + "windows-sys 0.48.0", +] + +[[package]] +name = "notify-debouncer-full" +version = "0.3.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fb7fd166739789c9ff169e654dc1501373db9d80a4c3f972817c8a4d7cf8f34e" +dependencies = [ + "crossbeam-channel", + "file-id", + "log", + "notify", + "parking_lot", + "walkdir", +] + [[package]] name = "nu-ansi-term" version = "0.46.0" @@ -2399,9 +2525,9 @@ dependencies = [ [[package]] name = "redox_syscall" -version = "0.5.9" +version = "0.5.18" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "82b568323e98e49e2a0899dcee453dd679fae22d69adf9b11dd508d1549b7e2f" +checksum = "ed2bf2547551a7053d6fdfafda3f938979645c44812fbfcda098faae3f1a362d" dependencies = [ "bitflags 2.8.0", ] @@ -3016,7 +3142,7 @@ dependencies = [ "backtrace", "bytes", "libc", - "mio", + "mio 1.0.3", "parking_lot", "pin-project-lite", "signal-hook-registry", @@ -3528,6 +3654,12 @@ dependencies = [ "syn", ] +[[package]] +name = "windows-link" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f0805222e57f7521d6a62e36fa9163bc891acd422f971defe97d64e70d0a4fe5" + [[package]] name = "windows-result" version = "0.2.0" @@ -3574,6 +3706,15 @@ dependencies = [ "windows-targets 0.52.6", ] +[[package]] +name = "windows-sys" +version = "0.60.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f2f500e4d28234f72040990ec9d39e3a6b950f9f22d3dba18416c35882612bcb" +dependencies = [ + "windows-targets 0.53.5", +] + [[package]] name = "windows-targets" version = "0.48.5" @@ -3598,13 +3739,30 @@ dependencies = [ "windows_aarch64_gnullvm 0.52.6", "windows_aarch64_msvc 0.52.6", "windows_i686_gnu 0.52.6", - "windows_i686_gnullvm", + "windows_i686_gnullvm 0.52.6", "windows_i686_msvc 0.52.6", "windows_x86_64_gnu 0.52.6", "windows_x86_64_gnullvm 0.52.6", "windows_x86_64_msvc 0.52.6", ] +[[package]] +name = "windows-targets" +version = "0.53.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4945f9f551b88e0d65f3db0bc25c33b8acea4d9e41163edf90dcd0b19f9069f3" +dependencies = [ + "windows-link", + "windows_aarch64_gnullvm 0.53.1", + "windows_aarch64_msvc 0.53.1", + "windows_i686_gnu 0.53.1", + "windows_i686_gnullvm 0.53.1", + "windows_i686_msvc 0.53.1", + "windows_x86_64_gnu 0.53.1", + "windows_x86_64_gnullvm 0.53.1", + "windows_x86_64_msvc 0.53.1", +] + [[package]] name = "windows_aarch64_gnullvm" version = "0.48.5" @@ -3617,6 +3775,12 @@ version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "32a4622180e7a0ec044bb555404c800bc9fd9ec262ec147edd5989ccd0c02cd3" +[[package]] +name = "windows_aarch64_gnullvm" +version = "0.53.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a9d8416fa8b42f5c947f8482c43e7d89e73a173cead56d044f6a56104a6d1b53" + [[package]] name = "windows_aarch64_msvc" version = "0.48.5" @@ -3629,6 +3793,12 @@ version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "09ec2a7bb152e2252b53fa7803150007879548bc709c039df7627cabbd05d469" +[[package]] +name = "windows_aarch64_msvc" +version = "0.53.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b9d782e804c2f632e395708e99a94275910eb9100b2114651e04744e9b125006" + [[package]] name = "windows_i686_gnu" version = "0.48.5" @@ -3641,12 +3811,24 @@ version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8e9b5ad5ab802e97eb8e295ac6720e509ee4c243f69d781394014ebfe8bbfa0b" +[[package]] +name = "windows_i686_gnu" +version = "0.53.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "960e6da069d81e09becb0ca57a65220ddff016ff2d6af6a223cf372a506593a3" + [[package]] name = "windows_i686_gnullvm" version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0eee52d38c090b3caa76c563b86c3a4bd71ef1a819287c19d586d7334ae8ed66" +[[package]] +name = "windows_i686_gnullvm" +version = "0.53.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fa7359d10048f68ab8b09fa71c3daccfb0e9b559aed648a8f95469c27057180c" + [[package]] name = "windows_i686_msvc" version = "0.48.5" @@ -3659,6 +3841,12 @@ version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "240948bc05c5e7c6dabba28bf89d89ffce3e303022809e73deaefe4f6ec56c66" +[[package]] +name = "windows_i686_msvc" +version = "0.53.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1e7ac75179f18232fe9c285163565a57ef8d3c89254a30685b57d83a38d326c2" + [[package]] name = "windows_x86_64_gnu" version = "0.48.5" @@ -3671,6 +3859,12 @@ version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "147a5c80aabfbf0c7d901cb5895d1de30ef2907eb21fbbab29ca94c5b08b1a78" +[[package]] +name = "windows_x86_64_gnu" +version = "0.53.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9c3842cdd74a865a8066ab39c8a7a473c0778a3f29370b5fd6b4b9aa7df4a499" + [[package]] name = "windows_x86_64_gnullvm" version = "0.48.5" @@ -3683,6 +3877,12 @@ version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "24d5b23dc417412679681396f2b49f3de8c1473deb516bd34410872eff51ed0d" +[[package]] +name = "windows_x86_64_gnullvm" +version = "0.53.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0ffa179e2d07eee8ad8f57493436566c7cc30ac536a3379fdf008f47f6bb7ae1" + [[package]] name = "windows_x86_64_msvc" version = "0.48.5" @@ -3695,6 +3895,12 @@ version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "589f6da84c646204747d1270a2a5661ea66ed1cced2631d546fdfb155959f9ec" +[[package]] +name = "windows_x86_64_msvc" +version = "0.53.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d6bbff5f0aada427a1e5a6da5f1f98158182f26556f345ac9e04d36d0ebed650" + [[package]] name = "winnow" version = "0.7.6" @@ -3913,6 +4119,8 @@ dependencies = [ "log", "netns-rs", "nix 0.30.1", + "notify", + "notify-debouncer-full", "num_cpus", "once_cell", "pin-project-lite", diff --git a/src/config.rs b/src/config.rs index afdbd5c2ef..456356fcac 100644 --- a/src/config.rs +++ b/src/config.rs @@ -78,6 +78,10 @@ const HTTP2_FRAME_SIZE: &str = "HTTP2_FRAME_SIZE"; const UNSTABLE_ENABLE_SOCKS5: &str = "UNSTABLE_ENABLE_SOCKS5"; +const ENABLE_CRL: &str = "ENABLE_CRL"; +const CRL_PATH: &str = "CRL_PATH"; +const ALLOW_EXPIRED_CRL: &str = "ALLOW_EXPIRED_CRL"; + const DEFAULT_WORKER_THREADS: u16 = 2; const DEFAULT_ADMIN_PORT: u16 = 15000; const DEFAULT_READINESS_PORT: u16 = 15021; @@ -108,6 +112,7 @@ const DEFAULT_ROOT_CERT_PROVIDER: &str = "./var/run/secrets/istio/root-cert.pem" const TOKEN_PROVIDER_ENV: &str = "AUTH_TOKEN"; const DEFAULT_TOKEN_PROVIDER: &str = "./var/run/secrets/tokens/istio-token"; const CERT_SYSTEM: &str = "SYSTEM"; +const DEFAULT_CRL_PATH: &str = "./var/run/secrets/istio/crl/ca-crl.pem"; const PROXY_MODE_DEDICATED: &str = "dedicated"; const PROXY_MODE_SHARED: &str = "shared"; @@ -311,6 +316,15 @@ pub struct Config { pub ztunnel_workload: Option, pub ipv6_enabled: bool, + + /// Enable CRL (Certificate Revocation List) checking + pub enable_crl: bool, + + /// Path to CRL file + pub crl_path: PathBuf, + + /// Allow expired CRL (for testing/rollout scenarios) + pub allow_expired_crl: bool, } #[derive(serde::Serialize, Clone, Copy, Debug)] @@ -865,6 +879,10 @@ pub fn construct_config(pc: ProxyConfig) -> Result { ztunnel_identity, ztunnel_workload, ipv6_enabled, + + enable_crl: parse_default(ENABLE_CRL, false)?, + crl_path: parse_default(CRL_PATH, PathBuf::from(DEFAULT_CRL_PATH))?, + allow_expired_crl: parse_default(ALLOW_EXPIRED_CRL, false)?, }) } diff --git a/src/proxy.rs b/src/proxy.rs index a8b3e013e5..e87c533311 100644 --- a/src/proxy.rs +++ b/src/proxy.rs @@ -118,6 +118,7 @@ impl DefaultSocketFactory { let res = socket2::SockRef::from(&s).set_tcp_keepalive(&ka); tracing::trace!("set keepalive: {:?}", res); } + #[cfg(target_os = "linux")] if cfg.user_timeout_enabled { // https://blog.cloudflare.com/when-tcp-sockets-refuse-to-die/ // TCP_USER_TIMEOUT = TCP_KEEPIDLE + TCP_KEEPINTVL * TCP_KEEPCNT. @@ -258,6 +259,10 @@ pub(super) struct ProxyInputs { resolver: Option>, // If true, inbound connections created with these inputs will not attempt to preserve the original source IP. pub disable_inbound_freebind: bool, + // CRL manager for certificate revocation checking + pub(super) crl_manager: Option>, + // Pool registry for draining HTTP/2 connection pools on CRL reload + pub(super) pool_registry: pool::PoolRegistry, } #[allow(clippy::too_many_arguments)] @@ -271,6 +276,8 @@ impl ProxyInputs { resolver: Option>, local_workload_information: Arc, disable_inbound_freebind: bool, + crl_manager: Option>, + pool_registry: pool::PoolRegistry, ) -> Arc { Arc::new(Self { cfg, @@ -281,6 +288,8 @@ impl ProxyInputs { local_workload_information, resolver, disable_inbound_freebind, + crl_manager, + pool_registry, }) } } diff --git a/src/proxy/connection_manager.rs b/src/proxy/connection_manager.rs index 99368fe5a6..cdf4ad2795 100644 --- a/src/proxy/connection_manager.rs +++ b/src/proxy/connection_manager.rs @@ -260,6 +260,39 @@ impl ConnectionManager { // potentially large copy under read lock, could require optimization self.drains.read().expect("mutex").keys().cloned().collect() } + + /// Close all inbound and outbound connections + /// This is called when certificate revocations are detected + pub fn close_all_connections(&self) { + // Close all inbound connections + let inbound_conns = self.connections(); + let inbound_count = inbound_conns.len(); + + if inbound_count > 0 { + tracing::info!("Closing {} inbound connection(s)", inbound_count); + // Spawn async tasks to close connections without blocking + for conn in inbound_conns { + let cm = self.clone(); + tokio::spawn(async move { + cm.close(&conn).await; + }); + } + } + + // Clear outbound connections (they will be recreated on next use) + let mut outbound = self.outbound_connections.write().expect("mutex"); + let outbound_count = outbound.len(); + if outbound_count > 0 { + tracing::info!("Clearing {} outbound connection(s)", outbound_count); + outbound.clear(); + } + + tracing::info!( + "Connection closure initiated: {} inbound, {} outbound", + inbound_count, + outbound_count + ); + } } #[derive(serde::Serialize)] diff --git a/src/proxy/inbound.rs b/src/proxy/inbound.rs index 72c79a5270..e3052880a2 100644 --- a/src/proxy/inbound.rs +++ b/src/proxy/inbound.rs @@ -83,6 +83,7 @@ impl Inbound { let pi = self.pi.clone(); let acceptor = InboundCertProvider { local_workload: self.pi.local_workload_information.clone(), + crl_manager: self.pi.crl_manager.clone(), }; // Safety: we set nodelay directly in tls_server, so it is safe to convert to a normal listener. @@ -683,6 +684,7 @@ impl InboundFlagError { #[derive(Clone)] struct InboundCertProvider { local_workload: Arc, + crl_manager: Option>, } #[async_trait::async_trait] @@ -693,7 +695,7 @@ impl crate::tls::ServerCertProvider for InboundCertProvider { "fetching cert" ); let cert = self.local_workload.fetch_certificate().await?; - Ok(Arc::new(cert.server_config()?)) + Ok(Arc::new(cert.server_config(self.crl_manager.clone())?)) } } @@ -914,6 +916,8 @@ mod tests { None, local_workload, false, + None, + crate::proxy::pool::PoolRegistry::new(), )); let inbound_request = Inbound::build_inbound_request(&pi, conn, &request_parts).await; match want { diff --git a/src/proxy/outbound.rs b/src/proxy/outbound.rs index 6c23376522..de8c9d9d4d 100644 --- a/src/proxy/outbound.rs +++ b/src/proxy/outbound.rs @@ -80,7 +80,12 @@ impl Outbound { self.pi.cfg.clone(), self.pi.socket_factory.clone(), self.pi.local_workload_information.clone(), + self.pi.crl_manager.clone(), ); + + // Register pool with registry for CRL-triggered draining + self.pi.pool_registry.register(pool.clone()); + let pi = self.pi.clone(); let accept = async move |drain: DrainWatcher, force_shutdown: watch::Receiver<()>| { loop { @@ -248,7 +253,8 @@ impl OutboundConnection { .local_workload_information .fetch_certificate() .await?; - let connector = cert.outbound_connector(wl_key.dst_id.clone())?; + let connector = + cert.outbound_connector(wl_key.dst_id.clone(), self.pi.crl_manager.clone())?; let tls_stream = connector.connect(upgraded).await?; // Spawn inner CONNECT tunnel @@ -804,12 +810,15 @@ mod tests { connection_manager: ConnectionManager::default(), resolver: None, disable_inbound_freebind: false, + crl_manager: None, + pool_registry: crate::proxy::pool::PoolRegistry::default(), }), id: TraceParent::new(), pool: WorkloadHBONEPool::new( cfg.clone(), sock_fact, local_workload_information.clone(), + None, ), hbone_port: cfg.inbound_addr.port(), }; diff --git a/src/proxy/pool.rs b/src/proxy/pool.rs index d19ef0b419..3eb74a6315 100644 --- a/src/proxy/pool.rs +++ b/src/proxy/pool.rs @@ -22,6 +22,7 @@ use std::collections::hash_map::DefaultHasher; use std::hash::{Hash, Hasher}; use std::sync::Arc; +use std::sync::RwLock; use std::sync::atomic::{AtomicI32, Ordering}; use tokio::sync::watch; @@ -73,6 +74,7 @@ struct ConnSpawner { socket_factory: Arc, local_workload: Arc, timeout_rx: watch::Receiver, + crl_manager: Option>, } // Does nothing but spawn new conns when asked @@ -81,7 +83,7 @@ impl ConnSpawner { debug!("spawning new pool conn for {}", key); let cert = self.local_workload.fetch_certificate().await?; - let connector = cert.outbound_connector(key.dst_id.clone())?; + let connector = cert.outbound_connector(key.dst_id.clone(), self.crl_manager.clone())?; let tcp_stream = super::freebind_connect(None, key.dst, self.socket_factory.as_ref()) .await .map_err(|e: io::Error| match e.kind() { @@ -337,6 +339,7 @@ impl WorkloadHBONEPool { cfg: Arc, socket_factory: Arc, local_workload: Arc, + crl_manager: Option>, ) -> WorkloadHBONEPool { let (timeout_tx, timeout_rx) = watch::channel(false); let (timeout_send, timeout_recv) = watch::channel(false); @@ -347,6 +350,7 @@ impl WorkloadHBONEPool { socket_factory, local_workload, timeout_rx: timeout_recv.clone(), + crl_manager, }; Self { @@ -504,6 +508,52 @@ impl WorkloadHBONEPool { }; Ok(res) } + + /// Drain all connections in this pool + /// This triggers all connection drivers to shut down gracefully via timeout_tx + pub fn drain_all_connections(&self) { + tracing::debug!("Draining all connections in WorkloadHBONEPool"); + let _ = self.state.timeout_tx.send(true); + tracing::debug!("Drain signal sent to all connection drivers"); + } +} + +/// Global registry for managing all connection pools +/// Used to drain all pools when CRL is reloaded with new revocations +#[derive(Clone, Default)] +pub struct PoolRegistry { + pools: Arc>>, +} + +impl PoolRegistry { + pub fn new() -> Self { + Self { + pools: Arc::new(RwLock::new(Vec::new())), + } + } + + /// Register a pool with the registry + pub fn register(&self, pool: WorkloadHBONEPool) { + let mut pools = self.pools.write().unwrap(); + pools.push(pool); + tracing::debug!("Registered pool with registry (total: {})", pools.len()); + } + + /// Drain all registered pools + /// This triggers all connection drivers across all pools to shut down + pub fn drain_all(&self) { + let pools = self.pools.read().unwrap(); + if pools.is_empty() { + tracing::warn!("No pools registered in registry"); + return; + } + + tracing::info!("Draining {} pool(s)", pools.len()); + for pool in pools.iter() { + pool.drain_all_connections(); + } + tracing::debug!("All pools drained"); + } } #[cfg(test)] @@ -1027,7 +1077,7 @@ mod test { mock_proxy_state, identity::mock::new_secret_manager(Duration::from_secs(10)), )); - let pool = WorkloadHBONEPool::new(Arc::new(cfg), sock_fact, local_workload); + let pool = WorkloadHBONEPool::new(Arc::new(cfg), sock_fact, local_workload, None); let server = TestServer { conn_counter, drop_rx, diff --git a/src/proxy/socks5.rs b/src/proxy/socks5.rs index 9a9eeab3ca..8d9ee805b5 100644 --- a/src/proxy/socks5.rs +++ b/src/proxy/socks5.rs @@ -76,7 +76,12 @@ impl Socks5 { self.pi.cfg.clone(), self.pi.socket_factory.clone(), self.pi.local_workload_information.clone(), + self.pi.crl_manager.clone(), ); + + // Register pool with registry for CRL-triggered draining + self.pi.pool_registry.register(pool.clone()); + let accept = async move |drain: DrainWatcher, force_shutdown: watch::Receiver<()>| { loop { // Asynchronously wait for an inbound socket. diff --git a/src/proxyfactory.rs b/src/proxyfactory.rs index afedabf7c7..14a38c542a 100644 --- a/src/proxyfactory.rs +++ b/src/proxyfactory.rs @@ -15,6 +15,7 @@ use crate::config; use crate::identity::SecretManager; use crate::state::{DemandProxyState, WorkloadInfo}; +use crate::tls; use std::sync::Arc; use tracing::error; @@ -34,6 +35,8 @@ pub struct ProxyFactory { proxy_metrics: Arc, dns_metrics: Option>, drain: DrainWatcher, + crl_manager: Option>, + pool_registry: crate::proxy::pool::PoolRegistry, } impl ProxyFactory { @@ -55,6 +58,38 @@ impl ProxyFactory { } }; + // Initialize CRL manager ONCE if enabled + let pool_registry = crate::proxy::pool::PoolRegistry::new(); + + let crl_manager = if config.enable_crl { + tracing::info!("CRL support is ENABLED"); + match tls::crl::CrlManager::new(config.crl_path.clone(), config.allow_expired_crl) { + Ok(manager) => { + tracing::info!("CRL Manager initialized successfully"); + let manager_arc = Arc::new(manager); + + // Register pool registry with CRL manager for pool draining on revocation + manager_arc.register_pool_registry(pool_registry.clone()); + tracing::info!("pool registry registered with CRL manager"); + + if let Err(e) = manager_arc.start_file_watcher() { + tracing::error!("failed to start CRL file watcher: {}", e); + } else { + tracing::info!("CRL file watcher active"); + } + + Some(manager_arc) + } + Err(e) => { + tracing::error!("Failed to initialize CRL manager: {}", e); + None + } + } + } else { + tracing::info!("CRL support is DISABLED"); + None + }; + Ok(ProxyFactory { config, state, @@ -62,6 +97,8 @@ impl ProxyFactory { proxy_metrics, dns_metrics, drain, + crl_manager, + pool_registry, }) } @@ -132,6 +169,8 @@ impl ProxyFactory { resolver, local_workload_information, false, + self.crl_manager.clone(), + self.pool_registry.clone(), ); result.connection_manager = Some(cm); result.proxy = Some(Proxy::from_inputs(pi, drain).await?); @@ -177,6 +216,8 @@ impl ProxyFactory { None, local_workload_information, true, + self.crl_manager.clone(), + self.pool_registry.clone(), ); let inbound = Inbound::new(pi, self.drain.clone()).await?; diff --git a/src/socket.rs b/src/socket.rs index 1e4827b789..1b1d02b9a8 100644 --- a/src/socket.rs +++ b/src/socket.rs @@ -178,6 +178,7 @@ impl Listener { let res = SockRef::from(&stream).set_tcp_keepalive(&ka); tracing::trace!("set keepalive: {:?}", res); } + #[cfg(target_os = "linux")] if cfg.user_timeout_enabled { let ut = cfg.keepalive_time + cfg.keepalive_retries * cfg.keepalive_interval; let res = SockRef::from(&stream).set_tcp_user_timeout(Some(ut)); diff --git a/src/test_helpers/app.rs b/src/test_helpers/app.rs index ef3f625a5c..3572d45778 100644 --- a/src/test_helpers/app.rs +++ b/src/test_helpers/app.rs @@ -50,6 +50,7 @@ pub struct TestApp { pub udp_dns_proxy_address: Option, pub cert_manager: Arc, + #[cfg(target_os = "linux")] pub namespace: Option, pub shutdown: ShutdownTrigger, pub ztunnel_identity: Option, @@ -65,6 +66,7 @@ impl From<(&Bound, Arc)> for TestApp { tcp_dns_proxy_address: app.tcp_dns_proxy_address, udp_dns_proxy_address: app.udp_dns_proxy_address, cert_manager, + #[cfg(target_os = "linux")] namespace: None, shutdown: app.shutdown.trigger(), ztunnel_identity: None, @@ -113,6 +115,7 @@ impl TestApp { Ok(client.request(req).await?) }; + #[cfg(target_os = "linux")] match self.namespace { Some(ref _ns) => { // TODO: if this is needed, do something like admin_request_body. @@ -122,6 +125,8 @@ impl TestApp { } None => get_resp().await, } + #[cfg(not(target_os = "linux"))] + get_resp().await } pub async fn admin_request_body(&self, path: &str) -> anyhow::Result { let port = self.admin_address.port(); @@ -139,10 +144,13 @@ impl TestApp { Ok::<_, anyhow::Error>(res.collect().await?.to_bytes()) }; + #[cfg(target_os = "linux")] match self.namespace { Some(ref ns) => ns.clone().run(get_resp)?.join().unwrap(), None => get_resp().await, } + #[cfg(not(target_os = "linux"))] + get_resp().await } pub async fn metrics(&self) -> anyhow::Result { diff --git a/src/tls.rs b/src/tls.rs index 4228748e8b..1eebcadf01 100644 --- a/src/tls.rs +++ b/src/tls.rs @@ -14,6 +14,7 @@ mod certificate; mod control; +pub mod crl; pub mod csr; mod lib; #[cfg(any(test, feature = "testing"))] diff --git a/src/tls/certificate.rs b/src/tls/certificate.rs index 230676217a..89723ecbdc 100644 --- a/src/tls/certificate.rs +++ b/src/tls/certificate.rs @@ -298,7 +298,10 @@ impl WorkloadCertificate { .collect() } - pub fn server_config(&self) -> Result { + pub fn server_config( + &self, + crl_manager: Option>, + ) -> Result { let td = self.cert.identity().map(|i| match i { Identity::Spiffe { trust_domain, .. } => trust_domain, }); @@ -308,8 +311,11 @@ impl WorkloadCertificate { ) .build()?; - let client_cert_verifier = - crate::tls::workload::TrustDomainVerifier::new(raw_client_cert_verifier, td); + let client_cert_verifier = crate::tls::workload::TrustDomainVerifier::new( + raw_client_cert_verifier, + td, + crl_manager, + ); let mut sc = ServerConfig::builder_with_provider(crate::tls::lib::provider()) .with_protocol_versions(tls::TLS_VERSIONS) .expect("server config must be valid") @@ -322,9 +328,17 @@ impl WorkloadCertificate { Ok(sc) } - pub fn client_config(&self, identity: Vec) -> Result { + pub fn client_config( + &self, + identity: Vec, + crl_manager: Option>, + ) -> Result { let roots = self.root_store.clone(); - let verifier = IdentityVerifier { roots, identity }; + let verifier = IdentityVerifier { + roots, + identity, + crl_manager, + }; let mut cc = ClientConfig::builder_with_provider(crate::tls::lib::provider()) .with_protocol_versions(tls::TLS_VERSIONS) .expect("client config must be valid") @@ -340,8 +354,12 @@ impl WorkloadCertificate { Ok(cc) } - pub fn outbound_connector(&self, identity: Vec) -> Result { - let cc = self.client_config(identity)?; + pub fn outbound_connector( + &self, + identity: Vec, + crl_manager: Option>, + ) -> Result { + let cc = self.client_config(identity, crl_manager)?; Ok(OutboundConnector { client_config: Arc::new(cc), }) @@ -449,7 +467,7 @@ mod test { WorkloadCertificate::new(key.as_bytes(), cert.as_bytes(), vec![&joined]).unwrap(); // Do a simple handshake between them; we should be able to accept the trusted root - let server = cert1.server_config().unwrap(); + let server = cert1.server_config(None).unwrap(); let tls = TlsAcceptor::from(Arc::new(server)); let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); let addr = listener.local_addr().unwrap(); @@ -460,7 +478,7 @@ mod test { }); let stream = TcpStream::connect(addr).await.unwrap(); - let client = cert2.outbound_connector(vec![id]).unwrap(); + let client = cert2.outbound_connector(vec![id], None).unwrap(); let mut tls = client.connect(stream).await.unwrap(); let _ = tls.write(b"hi").await.unwrap(); diff --git a/src/tls/crl.rs b/src/tls/crl.rs new file mode 100644 index 0000000000..4ccc4388ad --- /dev/null +++ b/src/tls/crl.rs @@ -0,0 +1,479 @@ +// Copyright Istio Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use notify::RecommendedWatcher; +use notify_debouncer_full::{ + DebounceEventResult, Debouncer, FileIdMap, new_debouncer, + notify::{RecursiveMode, Watcher}, +}; +use rustls::pki_types::CertificateDer; +use std::collections::HashSet; +use std::path::PathBuf; +use std::sync::{Arc, RwLock}; +use std::time::{Duration, SystemTime}; +use tracing::{debug, error, info, warn}; + +#[derive(Debug, thiserror::Error)] +pub enum CrlError { + #[error("failed to read CRL file: {0}")] + IoError(#[from] std::io::Error), + + #[error("failed to parse CRL: {0}")] + ParseError(String), + + #[error("CRL is expired")] + ExpiredCrl, + + #[error("failed to parse certificate: {0}")] + CertificateParseError(String), +} + +#[derive(Clone)] +pub struct CrlManager { + inner: Arc>, +} + +impl std::fmt::Debug for CrlManager { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("CrlManager").finish_non_exhaustive() + } +} + +struct CrlManagerInner { + crl_data: Vec>, + crl_path: PathBuf, + allow_expired: bool, + last_load_time: Option, + _debouncer: Option>, + revoked_serials: HashSet>, + pool_registry: Option, +} + +impl CrlManager { + /// Create a new CRL manager + pub fn new(crl_path: PathBuf, allow_expired: bool) -> Result { + debug!( + "initializing CRL Manager: path={:?}, allow_expired={}", + crl_path, allow_expired + ); + + let manager = Self { + inner: Arc::new(RwLock::new(CrlManagerInner { + crl_data: Vec::new(), + crl_path: crl_path.clone(), + allow_expired, + last_load_time: None, + _debouncer: None, + revoked_serials: HashSet::new(), + pool_registry: None, + })), + }; + + // Try to load the CRL, but don't fail if the file doesn't exist yet + // (it might be mounted later via ConfigMap) + if let Err(e) = manager.load_crl() { + match e { + CrlError::IoError(ref io_err) if io_err.kind() == std::io::ErrorKind::NotFound => { + warn!( + "CRL file not found at {:?}, will retry on first validation", + crl_path + ); + } + _ => { + error!("failed to initialize CRL Manager: {}", e); + return Err(e); + } + } + } + + Ok(manager) + } + + /// Register pool registry for draining HTTP/2 pools when CRL is reloaded with new revocations + pub fn register_pool_registry(&self, registry: crate::proxy::pool::PoolRegistry) { + let mut inner = self.inner.write().unwrap(); + inner.pool_registry = Some(registry); + debug!("registered pool registry with CRL manager"); + } + + /// Load or reload the CRL from disk + pub fn load_crl(&self) -> Result { + let mut inner = self.inner.write().unwrap(); + + debug!("loading CRL from {:?}", inner.crl_path); + let data = std::fs::read(&inner.crl_path)?; + + if data.is_empty() { + warn!("CRL file is empty at {:?}", inner.crl_path); + return Err(CrlError::ParseError("CRL file is empty".to_string())); + } + + debug!("read CRL file: {} bytes", data.len()); + + // Parse all CRL blocks (handles concatenated CRLs) + let der_crls = if data.starts_with(b"-----BEGIN") { + debug!("CRL is in PEM format, extracting all CRL blocks"); + Self::parse_pem_crls(&data)? + } else { + debug!("CRL is in DER format"); + // Single DER-encoded CRL + vec![data] + }; + + debug!("found {} CRL block(s) in file", der_crls.len()); + + let mut total_revoked = 0; + let mut new_revoked_serials = HashSet::new(); + + for (idx, der_data) in der_crls.iter().enumerate() { + use x509_parser::prelude::*; + let (_, crl) = CertificateRevocationList::from_der(der_data).map_err(|e| { + CrlError::ParseError(format!("Failed to parse CRL {}: {}", idx + 1, e)) + })?; + + debug!("CRL {}:", idx + 1); + debug!(" Issuer: {}", crl.tbs_cert_list.issuer); + debug!(" this update: {:?}", crl.tbs_cert_list.this_update); + if let Some(next_update) = &crl.tbs_cert_list.next_update { + debug!(" next update: {:?}", next_update); + } + let revoked_count = crl.tbs_cert_list.revoked_certificates.len(); + debug!(" revoked certificates: {}", revoked_count); + total_revoked += revoked_count; + + for revoked in crl.tbs_cert_list.revoked_certificates.iter() { + let serial = revoked.serial().to_bytes_be(); + new_revoked_serials.insert(serial); + } + + // verify CRL validity + Self::verify_crl_validity(&crl, inner.allow_expired)?; + } + + // check if there are new revocations compared to previous load + let has_new_revocations = !new_revoked_serials.is_subset(&inner.revoked_serials); + + if has_new_revocations { + // calculate which serials are new + let newly_revoked: HashSet<_> = new_revoked_serials + .difference(&inner.revoked_serials) + .collect(); + warn!( + "detected {} NEW certificate revocation(s)", + newly_revoked.len() + ); + for serial in newly_revoked { + warn!(" newly revoked serial: {:?}", serial); + } + } + + // store all CRL DER data and update revoked serials + inner.crl_data = der_crls; + inner.revoked_serials = new_revoked_serials; + inner.last_load_time = Some(SystemTime::now()); + + debug!( + "CRL loaded successfully ({} CRL(s), {} total revoked certificate(s))", + inner.crl_data.len(), + total_revoked + ); + Ok(has_new_revocations) + } + + /// Parse PEM-encoded CRL data that may contain multiple CRL blocks + /// Returns a Vec of DER-encoded CRLs + fn parse_pem_crls(pem_data: &[u8]) -> Result>, CrlError> { + let data_str = std::str::from_utf8(pem_data) + .map_err(|e| CrlError::ParseError(format!("Invalid UTF-8: {}", e)))?; + + let mut crls = Vec::new(); + let mut in_pem = false; + let mut base64_data = String::new(); + + for line in data_str.lines() { + if line.starts_with("-----BEGIN") { + in_pem = true; + base64_data.clear(); // Start new CRL block + continue; + } + if line.starts_with("-----END") { + if in_pem && !base64_data.is_empty() { + use base64::Engine; + let der = base64::engine::general_purpose::STANDARD + .decode(&base64_data) + .map_err(|e| { + CrlError::ParseError(format!("failed to decode base64: {}", e)) + })?; + crls.push(der); + base64_data.clear(); + } + in_pem = false; + continue; + } + if in_pem { + base64_data.push_str(line.trim()); + } + } + + if crls.is_empty() { + return Err(CrlError::ParseError( + "no valid CRL blocks found in PEM data".to_string(), + )); + } + + Ok(crls) + } + + /// Verify CRL validity period + fn verify_crl_validity( + crl: &x509_parser::revocation_list::CertificateRevocationList, + allow_expired: bool, + ) -> Result<(), CrlError> { + let now = SystemTime::now(); + let unix_now = now + .duration_since(SystemTime::UNIX_EPOCH) + .expect("Time went backwards") + .as_secs() as i64; + + // check thisUpdate (CRL issue time) + if unix_now < crl.tbs_cert_list.this_update.timestamp() { + warn!("CRL is not yet valid"); + } + + // check nextUpdate (CRL expiry) + if let Some(next_update) = &crl.tbs_cert_list.next_update + && unix_now > next_update.timestamp() + { + if !allow_expired { + return Err(CrlError::ExpiredCrl); + } + warn!("CRL is expired but allow_expired_crl is enabled"); + } + + Ok(()) + } + + /// Check if any certificate in the chain is revoked + pub fn is_revoked_chain( + &self, + end_entity: &CertificateDer, + intermediates: &[CertificateDer], + ) -> Result { + debug!( + "checking certificate chain against CRL (chain length: {})", + 1 + intermediates.len() + ); + + debug!("checking leaf certificate"); + if self.is_cert_revoked(end_entity)? { + warn!("leaf certificate is REVOKED"); + return Ok(true); + } + + // check all intermediate certificates + for (idx, intermediate) in intermediates.iter().enumerate() { + debug!("checking intermediate certificate {} in chain", idx); + if self.is_cert_revoked(intermediate)? { + warn!("intermediate CA certificate at position {} is REVOKED", idx); + return Ok(true); + } + } + + debug!("certificate chain validation passed - no revoked certificates found"); + Ok(false) + } + + /// Internal method to check if a single certificate is revoked + /// Checks the certificate against ALL loaded CRLs + fn is_cert_revoked(&self, cert: &CertificateDer) -> Result { + let inner = self.inner.read().unwrap(); + + // if no CRLs are loaded, try to load them now + if inner.crl_data.is_empty() { + drop(inner); + debug!("CRL not loaded, attempting to load now"); + self.load_crl()?; + return self.is_cert_revoked(cert); + } + + // parse the certificate to get its serial number + use x509_parser::prelude::*; + let (_, parsed_cert) = X509Certificate::from_der(cert) + .map_err(|e| CrlError::CertificateParseError(e.to_string()))?; + + let cert_serial = &parsed_cert.serial; + debug!("certificate serial number: {:?}", cert_serial); + + // check the certificate against ALL CRLs + for (idx, crl_data) in inner.crl_data.iter().enumerate() { + // Parse CRL from stored data + let (_, crl) = CertificateRevocationList::from_der(crl_data).map_err(|e| { + CrlError::ParseError(format!("failed to parse stored CRL {}: {}", idx + 1, e)) + })?; + + debug!( + "checking against CRL {} (issuer: {})", + idx + 1, + crl.tbs_cert_list.issuer + ); + + // check if the certificate's serial number is in this CRL's revoked list + for revoked_cert in &crl.tbs_cert_list.revoked_certificates { + if revoked_cert.serial() == cert_serial { + warn!( + "certificate with serial {:?} is REVOKED in CRL {} (issuer: {})", + cert_serial, + idx + 1, + crl.tbs_cert_list.issuer + ); + warn!("revocation date: {:?}", revoked_cert.revocation_date); + return Ok(true); + } + } + } + + debug!( + "certificate serial {:?} is not in any of the {} CRL(s)", + cert_serial, + inner.crl_data.len() + ); + Ok(false) + } + + /// Refresh the CRL from disk if needed + pub fn refresh(&self) -> Result<(), CrlError> { + self.load_crl().map(|_| ()) + } + + /// Start watching the CRL file for changes + /// Uses debouncer to handle all file update patterns + pub fn start_file_watcher(self: &Arc) -> Result<(), CrlError> { + let crl_path = { + let inner = self.inner.read().unwrap(); + inner.crl_path.clone() + }; + + // watch the parent directory to catch ConfigMap updates via symlinks + let watch_path = crl_path + .parent() + .ok_or_else(|| CrlError::ParseError("CRL path has no parent directory".to_string()))?; + + debug!( + "starting CRL file watcher (debounced) for directory: {:?}", + watch_path + ); + debug!(" debounce timeout: 2 seconds"); + debug!(" watching for: Kubernetes ConfigMaps, direct writes, text editor saves"); + + let manager = Arc::clone(self); + + // create debouncer with 2-second timeout + // this collapses multiple events (CREATE/CHMOD/RENAME/REMOVE) into a single reload + let mut debouncer = new_debouncer( + Duration::from_secs(2), + None, + move |result: DebounceEventResult| { + match result { + Ok(events) => { + if !events.is_empty() { + // log all events for debugging + debug!("CRL directory events: {} event(s) detected", events.len()); + for event in events.iter() { + debug!( + " Event: kind={:?}, paths={:?}", + event.event.kind, event.event.paths + ); + } + + // reload CRL for any changes in the watched directory + // this handles Kubernetes ConfigMap updates (..data symlink changes) + // as well as direct file writes and text editor saves + debug!("CRL directory changed, reloading..."); + match manager.load_crl() { + Ok(has_new_revocations) => { + debug!("CRL reloaded successfully after file change"); + if has_new_revocations { + warn!("NEW REVOCATIONS DETECTED - Closing all connections to force re-validation"); + manager.close_all_connections(); + } + } + Err(e) => error!("failed to reload CRL: {}", e), + } + } + } + Err(errors) => { + for error in errors { + error!("CRL watcher error: {:?}", error); + } + } + } + }, + ) + .map_err(|e| CrlError::ParseError(format!("failed to create debouncer: {}", e)))?; + + // start watching the directory + debouncer + .watcher() + .watch(watch_path, RecursiveMode::NonRecursive) + .map_err(|e| CrlError::ParseError(format!("failed to watch directory: {}", e)))?; + + // Store debouncer to keep it alive + { + let mut inner = self.inner.write().unwrap(); + inner._debouncer = Some(debouncer); + } + + debug!("CRL file watcher started successfully"); + Ok(()) + } + + /// Close all connections when new certificate revocations are detected + /// This drains HTTP/2 connection pools, forcing clients to reconnect and re-validate certificates + fn close_all_connections(&self) { + let inner = self.inner.read().unwrap(); + + // drain HTTP/2 connection pools + if let Some(ref registry) = inner.pool_registry { + info!("draining HTTP/2 connection pools"); + registry.drain_all(); + } else { + warn!("no pool registry registered - cannot drain HTTP/2 pools"); + } + + debug!("connection pools drained - clients will reconnect and re-validate certificates"); + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::io::Write; + use tempfile::NamedTempFile; + + #[test] + fn test_crl_manager_missing_file() { + let result = CrlManager::new(PathBuf::from("/nonexistent/path/crl.pem"), false); + assert!(result.is_ok(), "should handle missing CRL file gracefully"); + } + + #[test] + fn test_crl_manager_invalid_file() { + let mut file = NamedTempFile::new().unwrap(); + file.write_all(b"not a valid CRL").unwrap(); + file.flush().unwrap(); + + let result = CrlManager::new(file.path().to_path_buf(), false); + assert!(result.is_err(), "should fail on invalid CRL data"); + } +} diff --git a/src/tls/workload.rs b/src/tls/workload.rs index 92e05d40c8..e2d41208c8 100644 --- a/src/tls/workload.rs +++ b/src/tls/workload.rs @@ -35,9 +35,10 @@ use tokio::io::{AsyncRead, AsyncWrite}; use crate::strng::Strng; use crate::tls; +use crate::tls::crl::CrlManager; use tokio::net::TcpStream; use tokio_rustls::client; -use tracing::{debug, trace}; +use tracing::{debug, error, trace}; #[derive(Clone, Debug)] pub struct InboundAcceptor { @@ -54,11 +55,20 @@ impl InboundAcceptor { pub(super) struct TrustDomainVerifier { base: Arc, trust_domain: Option, + crl_manager: Option>, } impl TrustDomainVerifier { - pub fn new(base: Arc, trust_domain: Option) -> Arc { - Arc::new(Self { base, trust_domain }) + pub fn new( + base: Arc, + trust_domain: Option, + crl_manager: Option>, + ) -> Arc { + Arc::new(Self { + base, + trust_domain, + crl_manager, + }) } fn verify_trust_domain(&self, client_cert: &CertificateDer<'_>) -> Result<(), rustls::Error> { @@ -108,10 +118,37 @@ impl ClientCertVerifier for TrustDomainVerifier { intermediates: &[CertificateDer<'_>], now: UnixTime, ) -> Result { + debug!( + "Verifying client certificate (chain length: {})", + 1 + intermediates.len() + ); + let res = self .base .verify_client_cert(end_entity, intermediates, now)?; self.verify_trust_domain(end_entity)?; + + // Check CRL if enabled + if let Some(crl_manager) = &self.crl_manager { + debug!("CRL checking enabled for client certificate"); + match crl_manager.is_revoked_chain(end_entity, intermediates) { + Ok(true) => { + error!("Client certificate or intermediate CA is REVOKED"); + return Err(rustls::Error::InvalidCertificate( + rustls::CertificateError::Revoked, + )); + } + Ok(false) => { + debug!("Client certificate chain is valid (not revoked)"); + } + Err(e) => { + error!("Failed to check CRL: {}, allowing certificate", e); + } + } + } else { + debug!("CRL checking disabled for client certificate"); + } + Ok(res) } @@ -182,6 +219,7 @@ impl OutboundConnector { pub struct IdentityVerifier { pub(super) roots: Arc, pub(super) identity: Vec, + pub(super) crl_manager: Option>, } impl IdentityVerifier { @@ -265,6 +303,27 @@ impl ServerCertVerifier for IdentityVerifier { self.verify_full_san(end_entity)?; + // Check CRL if enabled + if let Some(crl_manager) = &self.crl_manager { + debug!("CRL checking enabled for server certificate"); + match crl_manager.is_revoked_chain(end_entity, intermediates) { + Ok(true) => { + error!("Server certificate or intermediate CA is REVOKED"); + return Err(rustls::Error::InvalidCertificate( + rustls::CertificateError::Revoked, + )); + } + Ok(false) => { + debug!("Server certificate chain is valid (not revoked)"); + } + Err(e) => { + error!("Failed to check CRL: {}, allowing certificate", e); + } + } + } else { + debug!("CRL checking disabled for server certificate"); + } + Ok(ServerCertVerified::assertion()) } diff --git a/tests/namespaced.rs b/tests/namespaced.rs index c94badd612..eb7923989a 100644 --- a/tests/namespaced.rs +++ b/tests/namespaced.rs @@ -1028,7 +1028,7 @@ mod namespaced { identity::Identity::from_str("spiffe://cluster.local/ns/default/sa/server") .unwrap(); let cert = zt.cert_manager.fetch_certificate(id).await?; - let connector = cert.outbound_connector(vec![dst_id]).unwrap(); + let connector = cert.outbound_connector(vec![dst_id], None).unwrap(); let hbone = SocketAddr::new(srv.ip(), 15008); let tcp_stream = TcpStream::connect(hbone).await.unwrap(); let tls_stream = connector.connect(tcp_stream).await.unwrap(); @@ -1090,7 +1090,7 @@ mod namespaced { identity::Identity::from_str("spiffe://cluster.local/ns/default/sa/server") .unwrap(); let cert = zt.cert_manager.fetch_certificate(id).await?; - let connector = cert.outbound_connector(vec![dst_id]).unwrap(); + let connector = cert.outbound_connector(vec![dst_id], None).unwrap(); let tcp_stream = TcpStream::connect(SocketAddr::from((srv.ip(), 15008))) .await .unwrap(); @@ -1172,7 +1172,7 @@ mod namespaced { identity::Identity::from_str("spiffe://cluster.local/ns/default/sa/server") .unwrap(); let cert = zt.cert_manager.fetch_certificate(id).await?; - let connector = cert.outbound_connector(vec![dst_id]).unwrap(); + let connector = cert.outbound_connector(vec![dst_id], None).unwrap(); let tcp_stream = TcpStream::connect(SocketAddr::from((srv.ip(), 15008))) .await .unwrap(); From 4b6b8ebb84f65fbef1617a5e32b6a3b1d9e79351 Mon Sep 17 00:00:00 2001 From: nilekh <1626598+nilekhc@users.noreply.github.com> Date: Thu, 30 Oct 2025 12:23:55 -0700 Subject: [PATCH 02/17] chore: drains connection only for revoked cert Signed-off-by: nilekh <1626598+nilekhc@users.noreply.github.com> --- src/proxy.rs | 4 -- src/proxy/h2/client.rs | 4 ++ src/proxy/inbound.rs | 1 - src/proxy/outbound.rs | 17 ++++--- src/proxy/pool.rs | 103 ++++++++++++++++++++++------------------- src/proxy/socks5.rs | 3 -- src/proxyfactory.rs | 10 ---- src/tls/crl.rs | 36 ++++---------- 8 files changed, 79 insertions(+), 99 deletions(-) diff --git a/src/proxy.rs b/src/proxy.rs index e87c533311..dc6263ec8c 100644 --- a/src/proxy.rs +++ b/src/proxy.rs @@ -261,8 +261,6 @@ pub(super) struct ProxyInputs { pub disable_inbound_freebind: bool, // CRL manager for certificate revocation checking pub(super) crl_manager: Option>, - // Pool registry for draining HTTP/2 connection pools on CRL reload - pub(super) pool_registry: pool::PoolRegistry, } #[allow(clippy::too_many_arguments)] @@ -277,7 +275,6 @@ impl ProxyInputs { local_workload_information: Arc, disable_inbound_freebind: bool, crl_manager: Option>, - pool_registry: pool::PoolRegistry, ) -> Arc { Arc::new(Self { cfg, @@ -289,7 +286,6 @@ impl ProxyInputs { resolver, disable_inbound_freebind, crl_manager, - pool_registry, }) } } diff --git a/src/proxy/h2/client.rs b/src/proxy/h2/client.rs index 4c768d537e..c996d3c537 100644 --- a/src/proxy/h2/client.rs +++ b/src/proxy/h2/client.rs @@ -38,6 +38,8 @@ pub struct H2ConnectClient { pub max_allowed_streams: u16, stream_count: Arc, wl_key: WorkloadKey, + // Peer certificate serial number (used for selective connection closure on revocation) + pub peer_cert_serial: Option>, } #[derive(PartialEq, Eq, Hash, Clone, Debug)] @@ -148,6 +150,7 @@ pub async fn spawn_connection( s: impl AsyncRead + AsyncWrite + Unpin + Send + 'static, driver_drain: Receiver, wl_key: WorkloadKey, + peer_cert_serial: Option>, ) -> Result { let mut builder = h2::client::Builder::new(); builder @@ -188,6 +191,7 @@ pub async fn spawn_connection( stream_count: Arc::new(AtomicU16::new(0)), max_allowed_streams, wl_key, + peer_cert_serial, }; Ok(c) } diff --git a/src/proxy/inbound.rs b/src/proxy/inbound.rs index e3052880a2..bddf80180e 100644 --- a/src/proxy/inbound.rs +++ b/src/proxy/inbound.rs @@ -917,7 +917,6 @@ mod tests { local_workload, false, None, - crate::proxy::pool::PoolRegistry::new(), )); let inbound_request = Inbound::build_inbound_request(&pi, conn, &request_parts).await; match want { diff --git a/src/proxy/outbound.rs b/src/proxy/outbound.rs index de8c9d9d4d..5db4f0cbad 100644 --- a/src/proxy/outbound.rs +++ b/src/proxy/outbound.rs @@ -83,9 +83,6 @@ impl Outbound { self.pi.crl_manager.clone(), ); - // Register pool with registry for CRL-triggered draining - self.pi.pool_registry.register(pool.clone()); - let pi = self.pi.clone(); let accept = async move |drain: DrainWatcher, force_shutdown: watch::Receiver<()>| { loop { @@ -259,9 +256,16 @@ impl OutboundConnection { // Spawn inner CONNECT tunnel let (drain_tx, drain_rx) = tokio::sync::watch::channel(false); - let mut sender = - super::h2::client::spawn_connection(self.pi.cfg.clone(), tls_stream, drain_rx, wl_key) - .await?; + // peer_cert_serial is None here because SOCKS5 connections are not pooled, + // so we don't need to track certificate serial for revocation checking on next request + let mut sender = super::h2::client::spawn_connection( + self.pi.cfg.clone(), + tls_stream, + drain_rx, + wl_key, + None, + ) + .await?; let http_request = self.create_hbone_request(remote_addr, req); let inner_upgraded = sender.send_request(http_request).await?; @@ -811,7 +815,6 @@ mod tests { resolver: None, disable_inbound_freebind: false, crl_manager: None, - pool_registry: crate::proxy::pool::PoolRegistry::default(), }), id: TraceParent::new(), pool: WorkloadHBONEPool::new( diff --git a/src/proxy/pool.rs b/src/proxy/pool.rs index 3eb74a6315..fe5cb35600 100644 --- a/src/proxy/pool.rs +++ b/src/proxy/pool.rs @@ -22,7 +22,6 @@ use std::collections::hash_map::DefaultHasher; use std::hash::{Hash, Hasher}; use std::sync::Arc; -use std::sync::RwLock; use std::sync::atomic::{AtomicI32, Ordering}; use tokio::sync::watch; @@ -77,8 +76,43 @@ struct ConnSpawner { crl_manager: Option>, } +/// Extract peer certificate serial number from TLS stream +/// Returns None if extraction fails (connection will still work, just can't be selectively drained) +fn extract_peer_cert_serial( + tls_stream: &tokio_rustls::client::TlsStream, +) -> Option> { + use x509_parser::prelude::*; + + let (_, client_conn) = tls_stream.get_ref(); + client_conn + .peer_certificates() + .and_then(|certs| certs.first()) + .and_then(|cert| { + let (_, parsed) = X509Certificate::from_der(cert).ok()?; + Some(parsed.serial.to_bytes_be()) + }) +} + // Does nothing but spawn new conns when asked impl ConnSpawner { + /// Check if a certificate serial number is revoked + /// Returns false if no CRL manager, no serial, or not revoked + /// Returns true only if serial exists AND is in the revoked set + fn is_serial_revoked(&self, peer_cert_serial: &Option>) -> bool { + let Some(serial) = peer_cert_serial else { + // no serial available, can't check - assume not revoked + return false; + }; + + let Some(ref crl_mgr) = self.crl_manager else { + // no CRL manager, can't check - assume not revoked + return false; + }; + + // check if this serial is in the revoked set + crl_mgr.is_serial_revoked(serial) + } + async fn new_pool_conn(&self, key: WorkloadKey) -> Result { debug!("spawning new pool conn for {}", key); @@ -93,11 +127,21 @@ impl ConnSpawner { let tls_stream = connector.connect(tcp_stream).await?; trace!("connector connected, handshaking"); + + // extract peer certificate serial BEFORE H2 handshake (for selective connection closure) + let peer_cert_serial = extract_peer_cert_serial(&tls_stream); + if let Some(ref serial) = peer_cert_serial { + trace!("extracted peer certificate serial: {} bytes", serial.len()); + } else { + debug!("could not extract peer certificate serial"); + } + let sender = h2::client::spawn_connection( self.cfg.clone(), tls_stream, self.timeout_rx.clone(), key, + peer_cert_serial, ) .await?; Ok(sender) @@ -303,6 +347,17 @@ impl PoolState { ); continue; } + + // check if this connection's certificate has been revoked + if self.spawner.is_serial_revoked(&existing.peer_cert_serial) { + // certificate has been revoked - discard this connection and create a new one + tracing::warn!( + "connection for {} uses revoked certificate, discarding and creating new connection", + workload_key + ); + continue; + } + debug!("re-using connection for {}", workload_key); break existing; } @@ -508,52 +563,6 @@ impl WorkloadHBONEPool { }; Ok(res) } - - /// Drain all connections in this pool - /// This triggers all connection drivers to shut down gracefully via timeout_tx - pub fn drain_all_connections(&self) { - tracing::debug!("Draining all connections in WorkloadHBONEPool"); - let _ = self.state.timeout_tx.send(true); - tracing::debug!("Drain signal sent to all connection drivers"); - } -} - -/// Global registry for managing all connection pools -/// Used to drain all pools when CRL is reloaded with new revocations -#[derive(Clone, Default)] -pub struct PoolRegistry { - pools: Arc>>, -} - -impl PoolRegistry { - pub fn new() -> Self { - Self { - pools: Arc::new(RwLock::new(Vec::new())), - } - } - - /// Register a pool with the registry - pub fn register(&self, pool: WorkloadHBONEPool) { - let mut pools = self.pools.write().unwrap(); - pools.push(pool); - tracing::debug!("Registered pool with registry (total: {})", pools.len()); - } - - /// Drain all registered pools - /// This triggers all connection drivers across all pools to shut down - pub fn drain_all(&self) { - let pools = self.pools.read().unwrap(); - if pools.is_empty() { - tracing::warn!("No pools registered in registry"); - return; - } - - tracing::info!("Draining {} pool(s)", pools.len()); - for pool in pools.iter() { - pool.drain_all_connections(); - } - tracing::debug!("All pools drained"); - } } #[cfg(test)] diff --git a/src/proxy/socks5.rs b/src/proxy/socks5.rs index 8d9ee805b5..395a26f7ee 100644 --- a/src/proxy/socks5.rs +++ b/src/proxy/socks5.rs @@ -79,9 +79,6 @@ impl Socks5 { self.pi.crl_manager.clone(), ); - // Register pool with registry for CRL-triggered draining - self.pi.pool_registry.register(pool.clone()); - let accept = async move |drain: DrainWatcher, force_shutdown: watch::Receiver<()>| { loop { // Asynchronously wait for an inbound socket. diff --git a/src/proxyfactory.rs b/src/proxyfactory.rs index 14a38c542a..3c40200a1b 100644 --- a/src/proxyfactory.rs +++ b/src/proxyfactory.rs @@ -36,7 +36,6 @@ pub struct ProxyFactory { dns_metrics: Option>, drain: DrainWatcher, crl_manager: Option>, - pool_registry: crate::proxy::pool::PoolRegistry, } impl ProxyFactory { @@ -59,8 +58,6 @@ impl ProxyFactory { }; // Initialize CRL manager ONCE if enabled - let pool_registry = crate::proxy::pool::PoolRegistry::new(); - let crl_manager = if config.enable_crl { tracing::info!("CRL support is ENABLED"); match tls::crl::CrlManager::new(config.crl_path.clone(), config.allow_expired_crl) { @@ -68,10 +65,6 @@ impl ProxyFactory { tracing::info!("CRL Manager initialized successfully"); let manager_arc = Arc::new(manager); - // Register pool registry with CRL manager for pool draining on revocation - manager_arc.register_pool_registry(pool_registry.clone()); - tracing::info!("pool registry registered with CRL manager"); - if let Err(e) = manager_arc.start_file_watcher() { tracing::error!("failed to start CRL file watcher: {}", e); } else { @@ -98,7 +91,6 @@ impl ProxyFactory { dns_metrics, drain, crl_manager, - pool_registry, }) } @@ -170,7 +162,6 @@ impl ProxyFactory { local_workload_information, false, self.crl_manager.clone(), - self.pool_registry.clone(), ); result.connection_manager = Some(cm); result.proxy = Some(Proxy::from_inputs(pi, drain).await?); @@ -217,7 +208,6 @@ impl ProxyFactory { local_workload_information, true, self.crl_manager.clone(), - self.pool_registry.clone(), ); let inbound = Inbound::new(pi, self.drain.clone()).await?; diff --git a/src/tls/crl.rs b/src/tls/crl.rs index 4ccc4388ad..dc2530e14f 100644 --- a/src/tls/crl.rs +++ b/src/tls/crl.rs @@ -57,7 +57,6 @@ struct CrlManagerInner { last_load_time: Option, _debouncer: Option>, revoked_serials: HashSet>, - pool_registry: Option, } impl CrlManager { @@ -76,7 +75,6 @@ impl CrlManager { last_load_time: None, _debouncer: None, revoked_serials: HashSet::new(), - pool_registry: None, })), }; @@ -100,13 +98,6 @@ impl CrlManager { Ok(manager) } - /// Register pool registry for draining HTTP/2 pools when CRL is reloaded with new revocations - pub fn register_pool_registry(&self, registry: crate::proxy::pool::PoolRegistry) { - let mut inner = self.inner.write().unwrap(); - inner.pool_registry = Some(registry); - debug!("registered pool registry with CRL manager"); - } - /// Load or reload the CRL from disk pub fn load_crl(&self) -> Result { let mut inner = self.inner.write().unwrap(); @@ -264,6 +255,14 @@ impl CrlManager { Ok(()) } + /// Check if a certificate serial number is in the revoked set + /// This is used for revocation checking on pooled connections + /// when next request is received + pub fn is_serial_revoked(&self, serial: &[u8]) -> bool { + let inner = self.inner.read().unwrap(); + inner.revoked_serials.contains(serial) + } + /// Check if any certificate in the chain is revoked pub fn is_revoked_chain( &self, @@ -404,8 +403,7 @@ impl CrlManager { Ok(has_new_revocations) => { debug!("CRL reloaded successfully after file change"); if has_new_revocations { - warn!("NEW REVOCATIONS DETECTED - Closing all connections to force re-validation"); - manager.close_all_connections(); + info!("NEW REVOCATIONS DETECTED - revoked certificates will be rejected on next connection checkout"); } } Err(e) => error!("failed to reload CRL: {}", e), @@ -437,22 +435,6 @@ impl CrlManager { debug!("CRL file watcher started successfully"); Ok(()) } - - /// Close all connections when new certificate revocations are detected - /// This drains HTTP/2 connection pools, forcing clients to reconnect and re-validate certificates - fn close_all_connections(&self) { - let inner = self.inner.read().unwrap(); - - // drain HTTP/2 connection pools - if let Some(ref registry) = inner.pool_registry { - info!("draining HTTP/2 connection pools"); - registry.drain_all(); - } else { - warn!("no pool registry registered - cannot drain HTTP/2 pools"); - } - - debug!("connection pools drained - clients will reconnect and re-validate certificates"); - } } #[cfg(test)] From cd7e193be4c2f4c7c42376a4dcf6c5b8e5d0af22 Mon Sep 17 00:00:00 2001 From: nilekh <1626598+nilekhc@users.noreply.github.com> Date: Tue, 18 Nov 2025 10:39:31 -0800 Subject: [PATCH 03/17] feat: revokes affected inbound connections only Signed-off-by: nilekh <1626598+nilekhc@users.noreply.github.com> --- src/config.rs | 6 +- src/proxy.rs | 5 +- src/proxy/connection_manager.rs | 197 +++++++++++++++++++++++++++---- src/proxy/h2/client.rs | 4 - src/proxy/inbound.rs | 107 ++++++++++++++++- src/proxy/inbound_passthrough.rs | 2 +- src/proxy/outbound.rs | 19 +-- src/proxy/pool.rs | 63 +--------- src/proxy/socks5.rs | 2 - src/proxyfactory.rs | 17 +-- src/tls/crl.rs | 50 +++++++- src/tls/workload.rs | 50 +++----- tests/namespaced.rs | 6 +- 13 files changed, 366 insertions(+), 162 deletions(-) diff --git a/src/config.rs b/src/config.rs index 456356fcac..0c1b098f3c 100644 --- a/src/config.rs +++ b/src/config.rs @@ -317,13 +317,13 @@ pub struct Config { pub ipv6_enabled: bool, - /// Enable CRL (Certificate Revocation List) checking + // Enable CRL (Certificate Revocation List) checking pub enable_crl: bool, - /// Path to CRL file + // Path to CRL file pub crl_path: PathBuf, - /// Allow expired CRL (for testing/rollout scenarios) + // Allow expired CRL (for testing/rollout scenarios) pub allow_expired_crl: bool, } diff --git a/src/proxy.rs b/src/proxy.rs index dc6263ec8c..f2d17a77fe 100644 --- a/src/proxy.rs +++ b/src/proxy.rs @@ -259,7 +259,6 @@ pub(super) struct ProxyInputs { resolver: Option>, // If true, inbound connections created with these inputs will not attempt to preserve the original source IP. pub disable_inbound_freebind: bool, - // CRL manager for certificate revocation checking pub(super) crl_manager: Option>, } @@ -299,6 +298,10 @@ impl Proxy { // We setup all the listeners first so we can capture any errors that should block startup let inbound = Inbound::new(pi.clone(), drain.clone()).await?; + if let Some(ref crl_mgr) = pi.crl_manager { + crl_mgr.register_connection_manager(pi.connection_manager.clone()); + } + // This exists for `direct` integ tests, no other reason #[cfg(any(test, feature = "testing"))] if pi.cfg.fake_self_inbound { diff --git a/src/proxy/connection_manager.rs b/src/proxy/connection_manager.rs index cdf4ad2795..1067fdbc88 100644 --- a/src/proxy/connection_manager.rs +++ b/src/proxy/connection_manager.rs @@ -59,6 +59,7 @@ impl ConnectionDrain { pub struct ConnectionManager { drains: Arc>>, outbound_connections: Arc>>, + close_tx: Option>, } impl std::fmt::Debug for ConnectionManager { @@ -72,6 +73,7 @@ impl Default for ConnectionManager { ConnectionManager { drains: Arc::new(RwLock::new(HashMap::new())), outbound_connections: Arc::new(RwLock::new(HashSet::new())), + close_tx: None, } } } @@ -152,9 +154,43 @@ pub struct InboundConnection { #[serde(flatten)] pub ctx: ProxyRbacContext, pub dest_service: Option, + #[serde(skip)] + pub client_cert_serials: Option>>, // ALL serials in chain (leaf + intermediates) } impl ConnectionManager { + // Initialize the connection manager with CRL support + // This spawns an async task that handles connection closes from the CRL watcher + pub fn new_with_crl_support() -> Self { + let (close_tx, mut close_rx) = tokio::sync::mpsc::unbounded_channel::(); + + let cm = ConnectionManager { + drains: Arc::new(RwLock::new(HashMap::new())), + outbound_connections: Arc::new(RwLock::new(HashSet::new())), + close_tx: Some(close_tx), + }; + + // Spawn async task to handle close requests + let cm_clone = ConnectionManager { + drains: cm.drains.clone(), + outbound_connections: cm.outbound_connections.clone(), + close_tx: None, + }; + + tokio::spawn(async move { + while let Some(conn) = close_rx.recv().await { + tracing::debug!( + "Processing close request for connection from {}", + conn.ctx.conn.src + ); + cm_clone.close(&conn).await; + } + tracing::debug!("Connection close handler terminated"); + }); + + cm + } + pub fn track_outbound( &self, src: SocketAddr, @@ -185,12 +221,14 @@ impl ConnectionManager { state: &DemandProxyState, ctx: &ProxyRbacContext, dest_service: Option, + client_cert_serials: Option>>, ) -> Result { // Register before our initial assert. This prevents a race if policy changes between assert() and // track() let conn = InboundConnection { ctx: ctx.clone(), dest_service, + client_cert_serials, }; let Some(watch) = self.register(&conn) else { warn!("failed to track {conn:?}"); @@ -207,6 +245,29 @@ impl ConnectionManager { watch: Some(watch), }) } + + /// Register a TLS connection for tracking and potential termination on certificate revocation + pub fn register_tls_connection( + &self, + ctx: &ProxyRbacContext, + client_cert_serials: Option>>, + ) -> ConnectionGuard { + let conn = InboundConnection { + ctx: ctx.clone(), + dest_service: None, + client_cert_serials, + }; + let watch = self.register(&conn); + if watch.is_none() { + warn!("failed to track TLS connection {conn:?}"); + } + ConnectionGuard { + cm: self.clone(), + conn, + watch, + } + } + // register a connection with the connection manager // this must be done before a connection can be tracked // allows policy to be asserted against the connection @@ -261,37 +322,118 @@ impl ConnectionManager { self.drains.read().expect("mutex").keys().cloned().collect() } - /// Close all inbound and outbound connections - /// This is called when certificate revocations are detected - pub fn close_all_connections(&self) { - // Close all inbound connections - let inbound_conns = self.connections(); - let inbound_count = inbound_conns.len(); - - if inbound_count > 0 { - tracing::info!("Closing {} inbound connection(s)", inbound_count); - // Spawn async tasks to close connections without blocking - for conn in inbound_conns { - let cm = self.clone(); - tokio::spawn(async move { - cm.close(&conn).await; - }); - } + /// Close only inbound connections whose client certificates have been revoked + /// This is called when CRL updates with new revocations + pub fn close_revoked_connections(&self, revoked_serials: &std::collections::HashSet>) { + let conns = self.connections(); + + tracing::debug!( + "checking {} active inbound connections against {} revoked serial(s)", + conns.len(), + revoked_serials.len() + ); + + // Log all revoked serials we're checking against + for (idx, serial) in revoked_serials.iter().enumerate() { + tracing::debug!( + "revoked serial {}: {} bytes (hex: {})", + idx + 1, + serial.len(), + serial + .iter() + .map(|b| format!("{:02x}", b)) + .collect::() + ); } - // Clear outbound connections (they will be recreated on next use) - let mut outbound = self.outbound_connections.write().expect("mutex"); - let outbound_count = outbound.len(); - if outbound_count > 0 { - tracing::info!("Clearing {} outbound connection(s)", outbound_count); - outbound.clear(); + let mut closed_count = 0; + let mut no_serial_count = 0; + + for (idx, conn) in conns.iter().enumerate() { + tracing::debug!( + "connection {}: src={}, has_serials={}", + idx + 1, + conn.ctx.conn.src, + conn.client_cert_serials.is_some() + ); + + if let Some(ref serials) = conn.client_cert_serials { + tracing::debug!( + " connection {} has {} certificate(s) in chain", + idx + 1, + serials.len() + ); + + // Check if ANY certificate in the chain is revoked + let mut is_revoked = false; + for (cert_idx, serial) in serials.iter().enumerate() { + let serial_hex = serial + .iter() + .map(|b| format!("{:02x}", b)) + .collect::(); + tracing::debug!( + " cert {} serial: {} bytes (hex: {})", + cert_idx, + serial.len(), + serial_hex + ); + + if revoked_serials.contains(serial) { + is_revoked = true; + tracing::warn!(" cert {} serial MATCHES revoked serial!", cert_idx); + break; + } + } + + tracing::debug!( + " connection {} has revoked certificate: {}", + idx + 1, + is_revoked + ); + + if is_revoked { + tracing::warn!( + "closing inbound connection {} from {} due to revoked certificate in chain", + idx + 1, + conn.ctx.conn.src + ); + + // Send close request through channel (works from blocking context) + if let Some(ref tx) = self.close_tx { + if let Err(e) = tx.send(conn.clone()) { + tracing::error!("failed to send close request: {}", e); + } else { + closed_count += 1; + } + } else { + tracing::warn!("CRL support not initialized - cannot close connection"); + } + } + } else { + no_serial_count += 1; + tracing::debug!( + " connection {} has no client certificate serials (skipping)", + idx + 1 + ); + } } tracing::info!( - "Connection closure initiated: {} inbound, {} outbound", - inbound_count, - outbound_count + "connection closure summary: {} total connections checked, {} with serials, {} without serials, {} closed", + conns.len(), + conns.len() - no_serial_count, + no_serial_count, + closed_count ); + + if closed_count > 0 { + tracing::info!( + "closed {} inbound connection(s) with revoked certificates", + closed_count + ); + } else { + tracing::debug!("no active connections with newly revoked certificates"); + } } } @@ -431,6 +573,7 @@ mod tests { dest_workload: Arc::new(test_default_workload()), }, dest_service: None, + client_cert_serials: None, }; // ensure drains contains exactly 1 item @@ -465,6 +608,7 @@ mod tests { dest_workload: Arc::new(test_default_workload()), }, dest_service: None, + client_cert_serials: None, }; let mut close2 = register(&cm, &rbac_ctx2); @@ -533,6 +677,7 @@ mod tests { dest_workload: Arc::new(test_default_workload()), }, dest_service: None, + client_cert_serials: None, }; // create a second connection @@ -553,6 +698,7 @@ mod tests { dest_workload: Arc::new(test_default_workload()), }, dest_service: None, + client_cert_serials: None, }; let another_conn1 = conn1.clone(); @@ -649,6 +795,7 @@ mod tests { dest_workload: Arc::new(test_default_workload()), }, dest_service: None, + client_cert_serials: None, }; // watch the connection let close1 = connection_manager diff --git a/src/proxy/h2/client.rs b/src/proxy/h2/client.rs index c996d3c537..4c768d537e 100644 --- a/src/proxy/h2/client.rs +++ b/src/proxy/h2/client.rs @@ -38,8 +38,6 @@ pub struct H2ConnectClient { pub max_allowed_streams: u16, stream_count: Arc, wl_key: WorkloadKey, - // Peer certificate serial number (used for selective connection closure on revocation) - pub peer_cert_serial: Option>, } #[derive(PartialEq, Eq, Hash, Clone, Debug)] @@ -150,7 +148,6 @@ pub async fn spawn_connection( s: impl AsyncRead + AsyncWrite + Unpin + Send + 'static, driver_drain: Receiver, wl_key: WorkloadKey, - peer_cert_serial: Option>, ) -> Result { let mut builder = h2::client::Builder::new(); builder @@ -191,7 +188,6 @@ pub async fn spawn_connection( stream_count: Arc::new(AtomicU16::new(0)), max_allowed_streams, wl_key, - peer_cert_serial, }; Ok(c) } diff --git a/src/proxy/inbound.rs b/src/proxy/inbound.rs index bddf80180e..f229586c43 100644 --- a/src/proxy/inbound.rs +++ b/src/proxy/inbound.rs @@ -20,7 +20,7 @@ use std::time::Instant; use tls_listener::AsyncTls; use tokio::sync::watch; -use tracing::{Instrument, debug, error, info, info_span, trace_span}; +use tracing::{Instrument, debug, error, info, info_span, trace_span, warn}; use super::{ConnectionResult, Error, HboneAddress, LocalWorkloadInformation, ResponseFlags, util}; use crate::baggage::parse_baggage_header; @@ -121,6 +121,35 @@ impl Inbound { debug!(latency=?start.elapsed(), "accepted TLS connection"); let (_, ssl) = tls.get_ref(); let src_identity: Option = tls::identity_from_connection(ssl); + let client_cert_serials: Option>> = + tls::cert_serial_from_connection(ssl); + + // Debug log certificate serial extraction + match &client_cert_serials { + Some(serials) => { + debug!( + "extracted {} certificate serial(s) from chain", + serials.len() + ); + for (idx, serial) in serials.iter().enumerate() { + debug!( + " cert {} serial: {} bytes (hex: {})", + idx, + serial.len(), + serial + .iter() + .map(|b| format!("{:02x}", b)) + .collect::() + ); + } + } + None => { + debug!( + "no client certificate serials extracted (TLS connection without client cert or extraction failed)" + ); + } + } + let conn = Connection { src_identity, src, @@ -128,6 +157,56 @@ impl Inbound { dst, }; debug!(%conn, "accepted connection"); + + // Register TLS connection for tracking (for certificate revocation termination) + let destination_workload = + match pi.local_workload_information.get_workload().await { + Ok(wl) => wl, + Err(e) => { + warn!("failed to get workload for TLS connection tracking: {}", e); + // continue anyway, worst case we can't track this connection for termination + // but the connection can still proceed + let cfg = pi.cfg.clone(); + let request_handler = move |req| { + let id = Self::extract_traceparent(&req); + let peer = conn.src; + let req_handler = Self::serve_connect( + pi.clone(), + conn.clone(), + self.enable_orig_src, + req, + None, + ) + .instrument(info_span!("inbound", %id, %peer)); + assertions::size_between_ref(1500, 2500, &req_handler); + req_handler + }; + let serve_conn = h2::server::serve_connection( + cfg, + tls, + drain, + force_shutdown, + request_handler, + ); + let serve = + Box::pin(assertions::size_between(6000, 8000, serve_conn)); + let _ = serve.await; + return Ok(()); + } + }; + + 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(); + let cfg = pi.cfg.clone(); let request_handler = move |req| { let id = Self::extract_traceparent(&req); @@ -137,6 +216,7 @@ impl Inbound { conn.clone(), self.enable_orig_src, req, + None, ) .instrument(info_span!("inbound", %id, %peer)); // This is for each user connection, so most important to keep small @@ -154,11 +234,29 @@ impl Inbound { // This is per HBONE connection, so while would be nice to be small, at least it // is pooled so typically fewer of these. let serve = Box::pin(assertions::size_between(6000, 8000, serve_conn)); - serve.await + + // Use tokio::select! to race between serving and drain signal + // 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()) + } + } }; // This is small since it only handles the TLS layer -- the HTTP2 layer is boxed // and measured above. - assertions::size_between_ref(1000, 1600, &serve_client); + assertions::size_between_ref(1000, 2200, &serve_client); tokio::task::spawn(serve_client.in_current_span()); } }; @@ -187,6 +285,7 @@ impl Inbound { conn: Connection, enable_original_source: bool, req: H2Request, + client_cert_serials: Option>>, ) { let src = conn.src; let dst = conn.dst; @@ -216,7 +315,7 @@ impl Inbound { // Define a connection guard to ensure rbac conditions are maintained for the duration of the connection let conn_guard = pi .connection_manager - .assert_rbac(&pi.state, &ri.rbac_ctx, ri.for_host) + .assert_rbac(&pi.state, &ri.rbac_ctx, ri.for_host, client_cert_serials) .await .map_err(InboundFlagError::build( StatusCode::UNAUTHORIZED, diff --git a/src/proxy/inbound_passthrough.rs b/src/proxy/inbound_passthrough.rs index e2b8b41464..da38633489 100644 --- a/src/proxy/inbound_passthrough.rs +++ b/src/proxy/inbound_passthrough.rs @@ -195,7 +195,7 @@ impl InboundPassthrough { let mut conn_guard = match pi .connection_manager - .assert_rbac(&pi.state, &rbac_ctx, None) + .assert_rbac(&pi.state, &rbac_ctx, None, None) .await { Ok(cg) => cg, diff --git a/src/proxy/outbound.rs b/src/proxy/outbound.rs index 5db4f0cbad..40529fef37 100644 --- a/src/proxy/outbound.rs +++ b/src/proxy/outbound.rs @@ -80,9 +80,7 @@ impl Outbound { self.pi.cfg.clone(), self.pi.socket_factory.clone(), self.pi.local_workload_information.clone(), - self.pi.crl_manager.clone(), ); - let pi = self.pi.clone(); let accept = async move |drain: DrainWatcher, force_shutdown: watch::Receiver<()>| { loop { @@ -250,22 +248,14 @@ impl OutboundConnection { .local_workload_information .fetch_certificate() .await?; - let connector = - cert.outbound_connector(wl_key.dst_id.clone(), self.pi.crl_manager.clone())?; + let connector = cert.outbound_connector(wl_key.dst_id.clone())?; let tls_stream = connector.connect(upgraded).await?; // Spawn inner CONNECT tunnel let (drain_tx, drain_rx) = tokio::sync::watch::channel(false); - // peer_cert_serial is None here because SOCKS5 connections are not pooled, - // so we don't need to track certificate serial for revocation checking on next request - let mut sender = super::h2::client::spawn_connection( - self.pi.cfg.clone(), - tls_stream, - drain_rx, - wl_key, - None, - ) - .await?; + let mut sender = + super::h2::client::spawn_connection(self.pi.cfg.clone(), tls_stream, drain_rx, wl_key) + .await?; let http_request = self.create_hbone_request(remote_addr, req); let inner_upgraded = sender.send_request(http_request).await?; @@ -821,7 +811,6 @@ mod tests { cfg.clone(), sock_fact, local_workload_information.clone(), - None, ), hbone_port: cfg.inbound_addr.port(), }; diff --git a/src/proxy/pool.rs b/src/proxy/pool.rs index fe5cb35600..d19ef0b419 100644 --- a/src/proxy/pool.rs +++ b/src/proxy/pool.rs @@ -73,51 +73,15 @@ struct ConnSpawner { socket_factory: Arc, local_workload: Arc, timeout_rx: watch::Receiver, - crl_manager: Option>, -} - -/// Extract peer certificate serial number from TLS stream -/// Returns None if extraction fails (connection will still work, just can't be selectively drained) -fn extract_peer_cert_serial( - tls_stream: &tokio_rustls::client::TlsStream, -) -> Option> { - use x509_parser::prelude::*; - - let (_, client_conn) = tls_stream.get_ref(); - client_conn - .peer_certificates() - .and_then(|certs| certs.first()) - .and_then(|cert| { - let (_, parsed) = X509Certificate::from_der(cert).ok()?; - Some(parsed.serial.to_bytes_be()) - }) } // Does nothing but spawn new conns when asked impl ConnSpawner { - /// Check if a certificate serial number is revoked - /// Returns false if no CRL manager, no serial, or not revoked - /// Returns true only if serial exists AND is in the revoked set - fn is_serial_revoked(&self, peer_cert_serial: &Option>) -> bool { - let Some(serial) = peer_cert_serial else { - // no serial available, can't check - assume not revoked - return false; - }; - - let Some(ref crl_mgr) = self.crl_manager else { - // no CRL manager, can't check - assume not revoked - return false; - }; - - // check if this serial is in the revoked set - crl_mgr.is_serial_revoked(serial) - } - async fn new_pool_conn(&self, key: WorkloadKey) -> Result { debug!("spawning new pool conn for {}", key); let cert = self.local_workload.fetch_certificate().await?; - let connector = cert.outbound_connector(key.dst_id.clone(), self.crl_manager.clone())?; + let connector = cert.outbound_connector(key.dst_id.clone())?; let tcp_stream = super::freebind_connect(None, key.dst, self.socket_factory.as_ref()) .await .map_err(|e: io::Error| match e.kind() { @@ -127,21 +91,11 @@ impl ConnSpawner { let tls_stream = connector.connect(tcp_stream).await?; trace!("connector connected, handshaking"); - - // extract peer certificate serial BEFORE H2 handshake (for selective connection closure) - let peer_cert_serial = extract_peer_cert_serial(&tls_stream); - if let Some(ref serial) = peer_cert_serial { - trace!("extracted peer certificate serial: {} bytes", serial.len()); - } else { - debug!("could not extract peer certificate serial"); - } - let sender = h2::client::spawn_connection( self.cfg.clone(), tls_stream, self.timeout_rx.clone(), key, - peer_cert_serial, ) .await?; Ok(sender) @@ -347,17 +301,6 @@ impl PoolState { ); continue; } - - // check if this connection's certificate has been revoked - if self.spawner.is_serial_revoked(&existing.peer_cert_serial) { - // certificate has been revoked - discard this connection and create a new one - tracing::warn!( - "connection for {} uses revoked certificate, discarding and creating new connection", - workload_key - ); - continue; - } - debug!("re-using connection for {}", workload_key); break existing; } @@ -394,7 +337,6 @@ impl WorkloadHBONEPool { cfg: Arc, socket_factory: Arc, local_workload: Arc, - crl_manager: Option>, ) -> WorkloadHBONEPool { let (timeout_tx, timeout_rx) = watch::channel(false); let (timeout_send, timeout_recv) = watch::channel(false); @@ -405,7 +347,6 @@ impl WorkloadHBONEPool { socket_factory, local_workload, timeout_rx: timeout_recv.clone(), - crl_manager, }; Self { @@ -1086,7 +1027,7 @@ mod test { mock_proxy_state, identity::mock::new_secret_manager(Duration::from_secs(10)), )); - let pool = WorkloadHBONEPool::new(Arc::new(cfg), sock_fact, local_workload, None); + let pool = WorkloadHBONEPool::new(Arc::new(cfg), sock_fact, local_workload); let server = TestServer { conn_counter, drop_rx, diff --git a/src/proxy/socks5.rs b/src/proxy/socks5.rs index 395a26f7ee..9a9eeab3ca 100644 --- a/src/proxy/socks5.rs +++ b/src/proxy/socks5.rs @@ -76,9 +76,7 @@ impl Socks5 { self.pi.cfg.clone(), self.pi.socket_factory.clone(), self.pi.local_workload_information.clone(), - self.pi.crl_manager.clone(), ); - let accept = async move |drain: DrainWatcher, force_shutdown: watch::Receiver<()>| { loop { // Asynchronously wait for an inbound socket. diff --git a/src/proxyfactory.rs b/src/proxyfactory.rs index 3c40200a1b..9ff6961e6d 100644 --- a/src/proxyfactory.rs +++ b/src/proxyfactory.rs @@ -59,16 +59,12 @@ impl ProxyFactory { // Initialize CRL manager ONCE if enabled let crl_manager = if config.enable_crl { - tracing::info!("CRL support is ENABLED"); match tls::crl::CrlManager::new(config.crl_path.clone(), config.allow_expired_crl) { Ok(manager) => { - tracing::info!("CRL Manager initialized successfully"); let manager_arc = Arc::new(manager); if let Err(e) = manager_arc.start_file_watcher() { tracing::error!("failed to start CRL file watcher: {}", e); - } else { - tracing::info!("CRL file watcher active"); } Some(manager_arc) @@ -79,7 +75,6 @@ impl ProxyFactory { } } } else { - tracing::info!("CRL support is DISABLED"); None }; @@ -151,7 +146,11 @@ impl ProxyFactory { // Optionally create the HBONE proxy. if self.config.proxy { - let cm = ConnectionManager::default(); + let cm = if self.crl_manager.is_some() { + ConnectionManager::new_with_crl_support() + } else { + ConnectionManager::default() + }; let pi = crate::proxy::ProxyInputs::new( self.config.clone(), cm.clone(), @@ -196,7 +195,11 @@ impl ProxyFactory { let socket_factory = Arc::new(DefaultSocketFactory(self.config.socket_config)); - let cm = ConnectionManager::default(); + let cm = if self.crl_manager.is_some() { + ConnectionManager::new_with_crl_support() + } else { + ConnectionManager::default() + }; let pi = crate::proxy::ProxyInputs::new( self.config.clone(), diff --git a/src/tls/crl.rs b/src/tls/crl.rs index dc2530e14f..573e1389aa 100644 --- a/src/tls/crl.rs +++ b/src/tls/crl.rs @@ -57,6 +57,7 @@ struct CrlManagerInner { last_load_time: Option, _debouncer: Option>, revoked_serials: HashSet>, + connection_manager: Option, } impl CrlManager { @@ -75,6 +76,7 @@ impl CrlManager { last_load_time: None, _debouncer: None, revoked_serials: HashSet::new(), + connection_manager: None, })), }; @@ -263,17 +265,51 @@ impl CrlManager { inner.revoked_serials.contains(serial) } + /// Register connection manager for automatic connection closure on CRL updates + pub fn register_connection_manager( + &self, + conn_mgr: crate::proxy::connection_manager::ConnectionManager, + ) { + let mut inner = self.inner.write().unwrap(); + inner.connection_manager = Some(conn_mgr); + debug!("connection manager registered with CRL manager for inbound connection termination"); + } + + /// Get the current set of all revoked certificate serials + pub fn get_revoked_serials(&self) -> HashSet> { + let inner = self.inner.read().unwrap(); + inner.revoked_serials.clone() + } + /// Check if any certificate in the chain is revoked pub fn is_revoked_chain( &self, end_entity: &CertificateDer, intermediates: &[CertificateDer], ) -> Result { + use x509_parser::prelude::*; + debug!( "checking certificate chain against CRL (chain length: {})", 1 + intermediates.len() ); + // Log end-entity certificate serial + if let Ok((_, parsed)) = X509Certificate::from_der(end_entity) { + debug!(" end-entity serial: {:?}", parsed.serial.to_bytes_be()); + } + + // Log intermediate certificate serials + for (idx, intermediate) in intermediates.iter().enumerate() { + if let Ok((_, parsed)) = X509Certificate::from_der(intermediate) { + debug!( + " intermediate {} serial: {:?}", + idx, + parsed.serial.to_bytes_be() + ); + } + } + debug!("checking leaf certificate"); if self.is_cert_revoked(end_entity)? { warn!("leaf certificate is REVOKED"); @@ -403,7 +439,19 @@ impl CrlManager { Ok(has_new_revocations) => { debug!("CRL reloaded successfully after file change"); if has_new_revocations { - info!("NEW REVOCATIONS DETECTED - revoked certificates will be rejected on next connection checkout"); + info!("NEW REVOCATIONS DETECTED - closing affected inbound connections"); + + // close connections with revoked certificates + let (conn_mgr_opt, revoked_serials) = { + let inner = manager.inner.read().unwrap(); + (inner.connection_manager.clone(), inner.revoked_serials.clone()) + }; + + if let Some(conn_mgr) = conn_mgr_opt { + conn_mgr.close_revoked_connections(&revoked_serials); + } else { + warn!("connection manager not registered - cannot close revoked connections"); + } } } Err(e) => error!("failed to reload CRL: {}", e), diff --git a/src/tls/workload.rs b/src/tls/workload.rs index e2d41208c8..62c51b216a 100644 --- a/src/tls/workload.rs +++ b/src/tls/workload.rs @@ -131,20 +131,22 @@ impl ClientCertVerifier for TrustDomainVerifier { // Check CRL if enabled if let Some(crl_manager) = &self.crl_manager { debug!("CRL checking enabled for client certificate"); - match crl_manager.is_revoked_chain(end_entity, intermediates) { - Ok(true) => { - error!("Client certificate or intermediate CA is REVOKED"); - return Err(rustls::Error::InvalidCertificate( - rustls::CertificateError::Revoked, - )); - } - Ok(false) => { - debug!("Client certificate chain is valid (not revoked)"); - } - Err(e) => { - error!("Failed to check CRL: {}, allowing certificate", e); - } + // Fail-closed: reject connection if CRL check returns an error OR if cert is revoked + let is_revoked = crl_manager + .is_revoked_chain(end_entity, intermediates) + .map_err(|e| { + error!("CRL validation failed for client certificate: {}", e); + rustls::Error::General(format!("Certificate revocation check failed: {}", e)) + })?; + + if is_revoked { + error!("Client certificate is REVOKED - rejecting connection"); + return Err(rustls::Error::InvalidCertificate( + rustls::CertificateError::Revoked, + )); } + + debug!("Client certificate chain is valid (not revoked)"); } else { debug!("CRL checking disabled for client certificate"); } @@ -219,7 +221,6 @@ impl OutboundConnector { pub struct IdentityVerifier { pub(super) roots: Arc, pub(super) identity: Vec, - pub(super) crl_manager: Option>, } impl IdentityVerifier { @@ -303,27 +304,6 @@ impl ServerCertVerifier for IdentityVerifier { self.verify_full_san(end_entity)?; - // Check CRL if enabled - if let Some(crl_manager) = &self.crl_manager { - debug!("CRL checking enabled for server certificate"); - match crl_manager.is_revoked_chain(end_entity, intermediates) { - Ok(true) => { - error!("Server certificate or intermediate CA is REVOKED"); - return Err(rustls::Error::InvalidCertificate( - rustls::CertificateError::Revoked, - )); - } - Ok(false) => { - debug!("Server certificate chain is valid (not revoked)"); - } - Err(e) => { - error!("Failed to check CRL: {}, allowing certificate", e); - } - } - } else { - debug!("CRL checking disabled for server certificate"); - } - Ok(ServerCertVerified::assertion()) } diff --git a/tests/namespaced.rs b/tests/namespaced.rs index eb7923989a..c94badd612 100644 --- a/tests/namespaced.rs +++ b/tests/namespaced.rs @@ -1028,7 +1028,7 @@ mod namespaced { identity::Identity::from_str("spiffe://cluster.local/ns/default/sa/server") .unwrap(); let cert = zt.cert_manager.fetch_certificate(id).await?; - let connector = cert.outbound_connector(vec![dst_id], None).unwrap(); + let connector = cert.outbound_connector(vec![dst_id]).unwrap(); let hbone = SocketAddr::new(srv.ip(), 15008); let tcp_stream = TcpStream::connect(hbone).await.unwrap(); let tls_stream = connector.connect(tcp_stream).await.unwrap(); @@ -1090,7 +1090,7 @@ mod namespaced { identity::Identity::from_str("spiffe://cluster.local/ns/default/sa/server") .unwrap(); let cert = zt.cert_manager.fetch_certificate(id).await?; - let connector = cert.outbound_connector(vec![dst_id], None).unwrap(); + let connector = cert.outbound_connector(vec![dst_id]).unwrap(); let tcp_stream = TcpStream::connect(SocketAddr::from((srv.ip(), 15008))) .await .unwrap(); @@ -1172,7 +1172,7 @@ mod namespaced { identity::Identity::from_str("spiffe://cluster.local/ns/default/sa/server") .unwrap(); let cert = zt.cert_manager.fetch_certificate(id).await?; - let connector = cert.outbound_connector(vec![dst_id], None).unwrap(); + let connector = cert.outbound_connector(vec![dst_id]).unwrap(); let tcp_stream = TcpStream::connect(SocketAddr::from((srv.ip(), 15008))) .await .unwrap(); From 213d5e19fc148639f84ef7b82ca41f6d88a349c5 Mon Sep 17 00:00:00 2001 From: nilekh <1626598+nilekhc@users.noreply.github.com> Date: Mon, 1 Dec 2025 15:57:52 -0800 Subject: [PATCH 04/17] chore: refactors crl watcher Signed-off-by: nilekh <1626598+nilekhc@users.noreply.github.com> --- src/proxy.rs | 25 ++++-- src/proxy/connection_manager.rs | 150 +------------------------------- src/proxy/h2/server.rs | 13 ++- src/proxy/inbound.rs | 89 +++++-------------- src/proxyfactory.rs | 12 +-- src/tls.rs | 1 + src/tls/crl.rs | 48 +++++----- src/tls/crl_watcher.rs | 116 ++++++++++++++++++++++++ 8 files changed, 196 insertions(+), 258 deletions(-) create mode 100644 src/tls/crl_watcher.rs diff --git a/src/proxy.rs b/src/proxy.rs index f2d17a77fe..16b73b596e 100644 --- a/src/proxy.rs +++ b/src/proxy.rs @@ -169,6 +169,7 @@ pub struct Proxy { outbound: Outbound, socks5: Option, policy_watcher: PolicyWatcher, + crl_watcher: Option, } pub struct LocalWorkloadInformation { @@ -298,10 +299,6 @@ impl Proxy { // We setup all the listeners first so we can capture any errors that should block startup let inbound = Inbound::new(pi.clone(), drain.clone()).await?; - if let Some(ref crl_mgr) = pi.crl_manager { - crl_mgr.register_connection_manager(pi.connection_manager.clone()); - } - // This exists for `direct` integ tests, no other reason #[cfg(any(test, feature = "testing"))] if pi.cfg.fake_self_inbound { @@ -322,8 +319,19 @@ impl Proxy { } else { None }; - let policy_watcher = - PolicyWatcher::new(pi.state.clone(), drain, pi.connection_manager.clone()); + let policy_watcher = PolicyWatcher::new( + pi.state.clone(), + drain.clone(), + pi.connection_manager.clone(), + ); + + let crl_watcher = pi.crl_manager.as_ref().map(|crl_mgr| { + crate::tls::crl_watcher::CrlWatcher::new( + crl_mgr.clone(), + drain, + pi.connection_manager.clone(), + ) + }); Ok(Proxy { inbound, @@ -331,6 +339,7 @@ impl Proxy { outbound, socks5, policy_watcher, + crl_watcher, }) } @@ -346,6 +355,10 @@ impl Proxy { tasks.push(tokio::spawn(socks5.run().in_current_span())); }; + if let Some(crl_watcher) = self.crl_watcher { + tasks.push(tokio::spawn(crl_watcher.run().in_current_span())); + }; + futures::future::join_all(tasks).await; } diff --git a/src/proxy/connection_manager.rs b/src/proxy/connection_manager.rs index 1067fdbc88..8a4c5b441b 100644 --- a/src/proxy/connection_manager.rs +++ b/src/proxy/connection_manager.rs @@ -59,7 +59,6 @@ impl ConnectionDrain { pub struct ConnectionManager { drains: Arc>>, outbound_connections: Arc>>, - close_tx: Option>, } impl std::fmt::Debug for ConnectionManager { @@ -73,7 +72,6 @@ impl Default for ConnectionManager { ConnectionManager { drains: Arc::new(RwLock::new(HashMap::new())), outbound_connections: Arc::new(RwLock::new(HashSet::new())), - close_tx: None, } } } @@ -159,38 +157,6 @@ pub struct InboundConnection { } impl ConnectionManager { - // Initialize the connection manager with CRL support - // This spawns an async task that handles connection closes from the CRL watcher - pub fn new_with_crl_support() -> Self { - let (close_tx, mut close_rx) = tokio::sync::mpsc::unbounded_channel::(); - - let cm = ConnectionManager { - drains: Arc::new(RwLock::new(HashMap::new())), - outbound_connections: Arc::new(RwLock::new(HashSet::new())), - close_tx: Some(close_tx), - }; - - // Spawn async task to handle close requests - let cm_clone = ConnectionManager { - drains: cm.drains.clone(), - outbound_connections: cm.outbound_connections.clone(), - close_tx: None, - }; - - tokio::spawn(async move { - while let Some(conn) = close_rx.recv().await { - tracing::debug!( - "Processing close request for connection from {}", - conn.ctx.conn.src - ); - cm_clone.close(&conn).await; - } - tracing::debug!("Connection close handler terminated"); - }); - - cm - } - pub fn track_outbound( &self, src: SocketAddr, @@ -306,7 +272,7 @@ impl ConnectionManager { } // signal all connections listening to this channel to take action (typically terminate traffic) - async fn close(&self, c: &InboundConnection) { + pub async fn close(&self, c: &InboundConnection) { let drain = { self.drains.write().expect("mutex").remove(c) }; if let Some(cd) = drain { cd.drain().await; @@ -321,120 +287,6 @@ impl ConnectionManager { // potentially large copy under read lock, could require optimization self.drains.read().expect("mutex").keys().cloned().collect() } - - /// Close only inbound connections whose client certificates have been revoked - /// This is called when CRL updates with new revocations - pub fn close_revoked_connections(&self, revoked_serials: &std::collections::HashSet>) { - let conns = self.connections(); - - tracing::debug!( - "checking {} active inbound connections against {} revoked serial(s)", - conns.len(), - revoked_serials.len() - ); - - // Log all revoked serials we're checking against - for (idx, serial) in revoked_serials.iter().enumerate() { - tracing::debug!( - "revoked serial {}: {} bytes (hex: {})", - idx + 1, - serial.len(), - serial - .iter() - .map(|b| format!("{:02x}", b)) - .collect::() - ); - } - - let mut closed_count = 0; - let mut no_serial_count = 0; - - for (idx, conn) in conns.iter().enumerate() { - tracing::debug!( - "connection {}: src={}, has_serials={}", - idx + 1, - conn.ctx.conn.src, - conn.client_cert_serials.is_some() - ); - - if let Some(ref serials) = conn.client_cert_serials { - tracing::debug!( - " connection {} has {} certificate(s) in chain", - idx + 1, - serials.len() - ); - - // Check if ANY certificate in the chain is revoked - let mut is_revoked = false; - for (cert_idx, serial) in serials.iter().enumerate() { - let serial_hex = serial - .iter() - .map(|b| format!("{:02x}", b)) - .collect::(); - tracing::debug!( - " cert {} serial: {} bytes (hex: {})", - cert_idx, - serial.len(), - serial_hex - ); - - if revoked_serials.contains(serial) { - is_revoked = true; - tracing::warn!(" cert {} serial MATCHES revoked serial!", cert_idx); - break; - } - } - - tracing::debug!( - " connection {} has revoked certificate: {}", - idx + 1, - is_revoked - ); - - if is_revoked { - tracing::warn!( - "closing inbound connection {} from {} due to revoked certificate in chain", - idx + 1, - conn.ctx.conn.src - ); - - // Send close request through channel (works from blocking context) - if let Some(ref tx) = self.close_tx { - if let Err(e) = tx.send(conn.clone()) { - tracing::error!("failed to send close request: {}", e); - } else { - closed_count += 1; - } - } else { - tracing::warn!("CRL support not initialized - cannot close connection"); - } - } - } else { - no_serial_count += 1; - tracing::debug!( - " connection {} has no client certificate serials (skipping)", - idx + 1 - ); - } - } - - tracing::info!( - "connection closure summary: {} total connections checked, {} with serials, {} without serials, {} closed", - conns.len(), - conns.len() - no_serial_count, - no_serial_count, - closed_count - ); - - if closed_count > 0 { - tracing::info!( - "closed {} inbound connection(s) with revoked certificates", - closed_count - ); - } else { - tracing::debug!("no active connections with newly revoked certificates"); - } - } } #[derive(serde::Serialize)] diff --git a/src/proxy/h2/server.rs b/src/proxy/h2/server.rs index d16e75d983..f74e912323 100644 --- a/src/proxy/h2/server.rs +++ b/src/proxy/h2/server.rs @@ -25,7 +25,7 @@ use std::sync::Arc; use std::sync::atomic::{AtomicBool, Ordering}; use tokio::net::TcpStream; use tokio::sync::{oneshot, watch}; -use tracing::{Instrument, debug}; +use tracing::{Instrument, debug, info}; pub struct H2Request { request: Parts, @@ -99,6 +99,7 @@ pub async fn serve_connection( s: tokio_rustls::server::TlsStream, drain: DrainWatcher, mut force_shutdown: watch::Receiver<()>, + tls_drain: Option, handler: F, ) -> Result<(), Error> where @@ -169,6 +170,16 @@ where conn.graceful_shutdown(); break; } + _tls_shutdown = async { + match &tls_drain { + Some(d) => d.clone().wait_for_drain().await, + None => std::future::pending().await, + } + } => { + info!("starting graceful drain (TLS connection certificate revoked)"); + conn.graceful_shutdown(); + break; + } } } // Signal to the ping_pong it should also stop. diff --git a/src/proxy/inbound.rs b/src/proxy/inbound.rs index f229586c43..3c52892137 100644 --- a/src/proxy/inbound.rs +++ b/src/proxy/inbound.rs @@ -20,7 +20,7 @@ use std::time::Instant; use tls_listener::AsyncTls; use tokio::sync::watch; -use tracing::{Instrument, debug, error, info, info_span, trace_span, warn}; +use tracing::{Instrument, debug, error, info, info_span, trace_span}; use super::{ConnectionResult, Error, HboneAddress, LocalWorkloadInformation, ResponseFlags, util}; use crate::baggage::parse_baggage_header; @@ -133,13 +133,10 @@ impl Inbound { ); for (idx, serial) in serials.iter().enumerate() { debug!( - " cert {} serial: {} bytes (hex: {})", + " cert {} serial: {} bytes (hex: {:02x?})", idx, serial.len(), serial - .iter() - .map(|b| format!("{:02x}", b)) - .collect::() ); } } @@ -158,54 +155,28 @@ impl Inbound { }; debug!(%conn, "accepted connection"); - // Register TLS connection for tracking (for certificate revocation termination) - let destination_workload = - match pi.local_workload_information.get_workload().await { - Ok(wl) => wl, - Err(e) => { - warn!("failed to get workload for TLS connection tracking: {}", e); - // continue anyway, worst case we can't track this connection for termination - // but the connection can still proceed - let cfg = pi.cfg.clone(); - let request_handler = move |req| { - let id = Self::extract_traceparent(&req); - let peer = conn.src; - let req_handler = Self::serve_connect( - pi.clone(), - conn.clone(), - self.enable_orig_src, - req, - None, - ) - .instrument(info_span!("inbound", %id, %peer)); - assertions::size_between_ref(1500, 2500, &req_handler); - req_handler - }; - let serve_conn = h2::server::serve_connection( - cfg, - tls, - drain, - force_shutdown, - request_handler, - ); - let serve = - Box::pin(assertions::size_between(6000, 8000, serve_conn)); - let _ = serve.await; - return Ok(()); - } - }; + // Register TLS connection for CRL monitoring + let dest_workload = match pi.local_workload_information.get_workload().await { + Ok(wl) => wl, + Err(e) => { + error!( + "failed to fetch local workload for TLS connection tracking: {}", + e + ); + return Err(proxy::Error::SelfCall); + } + }; let rbac_ctx = ProxyRbacContext { conn: conn.clone(), - dest_workload: destination_workload, + dest_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(); + let tls_drain = tls_guard.watcher(); + debug!("registered TLS connection for CRL monitoring"); let cfg = pi.cfg.clone(); let request_handler = move |req| { @@ -216,7 +187,7 @@ impl Inbound { conn.clone(), self.enable_orig_src, req, - None, + client_cert_serials.clone(), ) .instrument(info_span!("inbound", %id, %peer)); // This is for each user connection, so most important to keep small @@ -229,30 +200,16 @@ impl Inbound { tls, drain, force_shutdown, + Some(tls_drain), request_handler, ); // This is per HBONE connection, so while would be nice to be small, at least it // is pooled so typically fewer of these. let serve = Box::pin(assertions::size_between(6000, 8000, serve_conn)); - - // Use tokio::select! to race between serving and drain signal - // 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()) - } - } + let _ = serve.await; + // Keep tls_guard alive for the entire connection + drop(tls_guard); + Ok(()) }; // This is small since it only handles the TLS layer -- the HTTP2 layer is boxed // and measured above. diff --git a/src/proxyfactory.rs b/src/proxyfactory.rs index 9ff6961e6d..a89ba30a06 100644 --- a/src/proxyfactory.rs +++ b/src/proxyfactory.rs @@ -146,11 +146,7 @@ impl ProxyFactory { // Optionally create the HBONE proxy. if self.config.proxy { - let cm = if self.crl_manager.is_some() { - ConnectionManager::new_with_crl_support() - } else { - ConnectionManager::default() - }; + let cm = ConnectionManager::default(); let pi = crate::proxy::ProxyInputs::new( self.config.clone(), cm.clone(), @@ -195,11 +191,7 @@ impl ProxyFactory { let socket_factory = Arc::new(DefaultSocketFactory(self.config.socket_config)); - let cm = if self.crl_manager.is_some() { - ConnectionManager::new_with_crl_support() - } else { - ConnectionManager::default() - }; + let cm = ConnectionManager::default(); let pi = crate::proxy::ProxyInputs::new( self.config.clone(), diff --git a/src/tls.rs b/src/tls.rs index 1eebcadf01..6c01ea98d9 100644 --- a/src/tls.rs +++ b/src/tls.rs @@ -15,6 +15,7 @@ mod certificate; mod control; pub mod crl; +pub mod crl_watcher; pub mod csr; mod lib; #[cfg(any(test, feature = "testing"))] diff --git a/src/tls/crl.rs b/src/tls/crl.rs index 573e1389aa..793b4843e7 100644 --- a/src/tls/crl.rs +++ b/src/tls/crl.rs @@ -57,7 +57,9 @@ struct CrlManagerInner { last_load_time: Option, _debouncer: Option>, revoked_serials: HashSet>, - connection_manager: Option, + // watch channel for notifying about CRL changes + change_tx: tokio::sync::watch::Sender, + change_rx: tokio::sync::watch::Receiver, } impl CrlManager { @@ -68,6 +70,9 @@ impl CrlManager { crl_path, allow_expired ); + // create watch channel for CRL change notifications + let (change_tx, change_rx) = tokio::sync::watch::channel(0u64); + let manager = Self { inner: Arc::new(RwLock::new(CrlManagerInner { crl_data: Vec::new(), @@ -76,7 +81,8 @@ impl CrlManager { last_load_time: None, _debouncer: None, revoked_serials: HashSet::new(), - connection_manager: None, + change_tx, + change_rx, })), }; @@ -176,6 +182,12 @@ impl CrlManager { inner.revoked_serials = new_revoked_serials; inner.last_load_time = Some(SystemTime::now()); + // notify subscribers if there are new revocations + if has_new_revocations { + inner.change_tx.send_modify(|v| *v += 1); + debug!("notified CRL change subscribers about new revocations"); + } + debug!( "CRL loaded successfully ({} CRL(s), {} total revoked certificate(s))", inner.crl_data.len(), @@ -265,22 +277,18 @@ impl CrlManager { inner.revoked_serials.contains(serial) } - /// Register connection manager for automatic connection closure on CRL updates - pub fn register_connection_manager( - &self, - conn_mgr: crate::proxy::connection_manager::ConnectionManager, - ) { - let mut inner = self.inner.write().unwrap(); - inner.connection_manager = Some(conn_mgr); - debug!("connection manager registered with CRL manager for inbound connection termination"); - } - - /// Get the current set of all revoked certificate serials + /// get the current set of all revoked certificate serials pub fn get_revoked_serials(&self) -> HashSet> { let inner = self.inner.read().unwrap(); inner.revoked_serials.clone() } + /// subscribe to CRL change notifications + pub fn subscribe_to_changes(&self) -> tokio::sync::watch::Receiver { + let inner = self.inner.read().unwrap(); + inner.change_rx.clone() + } + /// Check if any certificate in the chain is revoked pub fn is_revoked_chain( &self, @@ -439,19 +447,7 @@ impl CrlManager { Ok(has_new_revocations) => { debug!("CRL reloaded successfully after file change"); if has_new_revocations { - info!("NEW REVOCATIONS DETECTED - closing affected inbound connections"); - - // close connections with revoked certificates - let (conn_mgr_opt, revoked_serials) = { - let inner = manager.inner.read().unwrap(); - (inner.connection_manager.clone(), inner.revoked_serials.clone()) - }; - - if let Some(conn_mgr) = conn_mgr_opt { - conn_mgr.close_revoked_connections(&revoked_serials); - } else { - warn!("connection manager not registered - cannot close revoked connections"); - } + info!("NEW REVOCATIONS DETECTED - CRL watcher will handle connection closures"); } } Err(e) => error!("failed to reload CRL: {}", e), diff --git a/src/tls/crl_watcher.rs b/src/tls/crl_watcher.rs new file mode 100644 index 0000000000..c578e2a88b --- /dev/null +++ b/src/tls/crl_watcher.rs @@ -0,0 +1,116 @@ +// Copyright Istio Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use std::sync::Arc; +use tracing::{debug, info}; + +use crate::drain::DrainWatcher; +use crate::proxy::connection_manager::ConnectionManager; +use crate::tls::crl::CrlManager; + +/// CrlWatcher monitors CRL updates and terminates connections with revoked certificates +pub struct CrlWatcher { + crl_manager: Arc, + stop: DrainWatcher, + connection_manager: ConnectionManager, +} + +impl CrlWatcher { + pub fn new( + crl_manager: Arc, + stop: DrainWatcher, + connection_manager: ConnectionManager, + ) -> Self { + CrlWatcher { + crl_manager, + stop, + connection_manager, + } + } + + /// Run the CRL watcher loop + pub async fn run(self) { + // Subscribe to CRL changes via watch channel + let mut crl_changed = self.crl_manager.subscribe_to_changes(); + + info!("CRL watcher started"); + + debug!("performing initial connection check for revoked certificates"); + self.check_and_close_revoked_connections().await; + + loop { + tokio::select! { + _ = self.stop.clone().wait_for_drain() => { + info!("CRL watcher shutting down"); + break; + } + _ = crl_changed.changed() => { + debug!("CRL changed, checking connections for revoked certificates"); + self.check_and_close_revoked_connections().await; + } + } + } + } + + /// Check all active connections and close those with revoked certificates + async fn check_and_close_revoked_connections(&self) { + let revoked_serials = self.crl_manager.get_revoked_serials(); + let connections = self.connection_manager.connections(); + + debug!( + "checking {} connection(s) against {} revoked serial(s)", + connections.len(), + revoked_serials.len() + ); + + let mut closed_count = 0; + + for conn in connections { + if let Some(ref serials) = conn.client_cert_serials { + debug!( + "checking connection {} with {} certificate(s)", + conn.ctx, + serials.len() + ); + + // Check if any certificate in the chain is revoked + let is_revoked = serials.iter().any(|serial| { + let revoked = revoked_serials.contains(serial); + if revoked { + debug!(" certificate serial {:02x?} is REVOKED", serial); + } + revoked + }); + + if is_revoked { + self.connection_manager.close(&conn).await; + info!( + "closed connection {} due to certificate revocation", + conn.ctx + ); + closed_count += 1; + } + } + } + + if closed_count > 0 { + info!( + "closed {} connection(s) due to certificate revocation", + closed_count + ); + } else { + debug!("no connections needed to be closed"); + } + } +} From e783e832f63562893cad3e6d27d5e9e26cf46055 Mon Sep 17 00:00:00 2001 From: nilekh <1626598+nilekhc@users.noreply.github.com> Date: Wed, 3 Dec 2025 13:13:37 -0800 Subject: [PATCH 05/17] chore: rejects new connections only Signed-off-by: nilekh <1626598+nilekhc@users.noreply.github.com> --- src/proxy.rs | 21 +----- src/proxy/connection_manager.rs | 24 +------ src/proxy/h2/server.rs | 13 +--- src/proxy/inbound.rs | 52 +------------- src/tls.rs | 1 - src/tls/crl.rs | 36 +--------- src/tls/crl_watcher.rs | 116 -------------------------------- src/tls/workload.rs | 6 -- 8 files changed, 6 insertions(+), 263 deletions(-) delete mode 100644 src/tls/crl_watcher.rs diff --git a/src/proxy.rs b/src/proxy.rs index 16b73b596e..b780769858 100644 --- a/src/proxy.rs +++ b/src/proxy.rs @@ -169,7 +169,6 @@ pub struct Proxy { outbound: Outbound, socks5: Option, policy_watcher: PolicyWatcher, - crl_watcher: Option, } pub struct LocalWorkloadInformation { @@ -319,19 +318,8 @@ impl Proxy { } else { None }; - let policy_watcher = PolicyWatcher::new( - pi.state.clone(), - drain.clone(), - pi.connection_manager.clone(), - ); - - let crl_watcher = pi.crl_manager.as_ref().map(|crl_mgr| { - crate::tls::crl_watcher::CrlWatcher::new( - crl_mgr.clone(), - drain, - pi.connection_manager.clone(), - ) - }); + let policy_watcher = + PolicyWatcher::new(pi.state.clone(), drain, pi.connection_manager.clone()); Ok(Proxy { inbound, @@ -339,7 +327,6 @@ impl Proxy { outbound, socks5, policy_watcher, - crl_watcher, }) } @@ -355,10 +342,6 @@ impl Proxy { tasks.push(tokio::spawn(socks5.run().in_current_span())); }; - if let Some(crl_watcher) = self.crl_watcher { - tasks.push(tokio::spawn(crl_watcher.run().in_current_span())); - }; - futures::future::join_all(tasks).await; } diff --git a/src/proxy/connection_manager.rs b/src/proxy/connection_manager.rs index 8a4c5b441b..c71b334bb0 100644 --- a/src/proxy/connection_manager.rs +++ b/src/proxy/connection_manager.rs @@ -212,28 +212,6 @@ impl ConnectionManager { }) } - /// Register a TLS connection for tracking and potential termination on certificate revocation - pub fn register_tls_connection( - &self, - ctx: &ProxyRbacContext, - client_cert_serials: Option>>, - ) -> ConnectionGuard { - let conn = InboundConnection { - ctx: ctx.clone(), - dest_service: None, - client_cert_serials, - }; - let watch = self.register(&conn); - if watch.is_none() { - warn!("failed to track TLS connection {conn:?}"); - } - ConnectionGuard { - cm: self.clone(), - conn, - watch, - } - } - // register a connection with the connection manager // this must be done before a connection can be tracked // allows policy to be asserted against the connection @@ -272,7 +250,7 @@ impl ConnectionManager { } // signal all connections listening to this channel to take action (typically terminate traffic) - pub async fn close(&self, c: &InboundConnection) { + async fn close(&self, c: &InboundConnection) { let drain = { self.drains.write().expect("mutex").remove(c) }; if let Some(cd) = drain { cd.drain().await; diff --git a/src/proxy/h2/server.rs b/src/proxy/h2/server.rs index f74e912323..d16e75d983 100644 --- a/src/proxy/h2/server.rs +++ b/src/proxy/h2/server.rs @@ -25,7 +25,7 @@ use std::sync::Arc; use std::sync::atomic::{AtomicBool, Ordering}; use tokio::net::TcpStream; use tokio::sync::{oneshot, watch}; -use tracing::{Instrument, debug, info}; +use tracing::{Instrument, debug}; pub struct H2Request { request: Parts, @@ -99,7 +99,6 @@ pub async fn serve_connection( s: tokio_rustls::server::TlsStream, drain: DrainWatcher, mut force_shutdown: watch::Receiver<()>, - tls_drain: Option, handler: F, ) -> Result<(), Error> where @@ -170,16 +169,6 @@ where conn.graceful_shutdown(); break; } - _tls_shutdown = async { - match &tls_drain { - Some(d) => d.clone().wait_for_drain().await, - None => std::future::pending().await, - } - } => { - info!("starting graceful drain (TLS connection certificate revoked)"); - conn.graceful_shutdown(); - break; - } } } // Signal to the ping_pong it should also stop. diff --git a/src/proxy/inbound.rs b/src/proxy/inbound.rs index 3c52892137..7d9bad6ca7 100644 --- a/src/proxy/inbound.rs +++ b/src/proxy/inbound.rs @@ -124,29 +124,6 @@ impl Inbound { let client_cert_serials: Option>> = tls::cert_serial_from_connection(ssl); - // Debug log certificate serial extraction - match &client_cert_serials { - Some(serials) => { - debug!( - "extracted {} certificate serial(s) from chain", - serials.len() - ); - for (idx, serial) in serials.iter().enumerate() { - debug!( - " cert {} serial: {} bytes (hex: {:02x?})", - idx, - serial.len(), - serial - ); - } - } - None => { - debug!( - "no client certificate serials extracted (TLS connection without client cert or extraction failed)" - ); - } - } - let conn = Connection { src_identity, src, @@ -154,30 +131,6 @@ impl Inbound { dst, }; debug!(%conn, "accepted connection"); - - // Register TLS connection for CRL monitoring - let dest_workload = match pi.local_workload_information.get_workload().await { - Ok(wl) => wl, - Err(e) => { - error!( - "failed to fetch local workload for TLS connection tracking: {}", - e - ); - return Err(proxy::Error::SelfCall); - } - }; - - let rbac_ctx = ProxyRbacContext { - conn: conn.clone(), - dest_workload, - }; - - let mut tls_guard = pi - .connection_manager - .register_tls_connection(&rbac_ctx, client_cert_serials.clone()); - let tls_drain = tls_guard.watcher(); - debug!("registered TLS connection for CRL monitoring"); - let cfg = pi.cfg.clone(); let request_handler = move |req| { let id = Self::extract_traceparent(&req); @@ -200,20 +153,17 @@ impl Inbound { tls, drain, force_shutdown, - Some(tls_drain), request_handler, ); // This is per HBONE connection, so while would be nice to be small, at least it // is pooled so typically fewer of these. let serve = Box::pin(assertions::size_between(6000, 8000, serve_conn)); let _ = serve.await; - // Keep tls_guard alive for the entire connection - drop(tls_guard); Ok(()) }; // This is small since it only handles the TLS layer -- the HTTP2 layer is boxed // and measured above. - assertions::size_between_ref(1000, 2200, &serve_client); + assertions::size_between_ref(1000, 1600, &serve_client); tokio::task::spawn(serve_client.in_current_span()); } }; diff --git a/src/tls.rs b/src/tls.rs index 6c01ea98d9..1eebcadf01 100644 --- a/src/tls.rs +++ b/src/tls.rs @@ -15,7 +15,6 @@ mod certificate; mod control; pub mod crl; -pub mod crl_watcher; pub mod csr; mod lib; #[cfg(any(test, feature = "testing"))] diff --git a/src/tls/crl.rs b/src/tls/crl.rs index 793b4843e7..30c8d1cb64 100644 --- a/src/tls/crl.rs +++ b/src/tls/crl.rs @@ -57,9 +57,6 @@ struct CrlManagerInner { last_load_time: Option, _debouncer: Option>, revoked_serials: HashSet>, - // watch channel for notifying about CRL changes - change_tx: tokio::sync::watch::Sender, - change_rx: tokio::sync::watch::Receiver, } impl CrlManager { @@ -70,9 +67,6 @@ impl CrlManager { crl_path, allow_expired ); - // create watch channel for CRL change notifications - let (change_tx, change_rx) = tokio::sync::watch::channel(0u64); - let manager = Self { inner: Arc::new(RwLock::new(CrlManagerInner { crl_data: Vec::new(), @@ -81,8 +75,6 @@ impl CrlManager { last_load_time: None, _debouncer: None, revoked_serials: HashSet::new(), - change_tx, - change_rx, })), }; @@ -182,12 +174,6 @@ impl CrlManager { inner.revoked_serials = new_revoked_serials; inner.last_load_time = Some(SystemTime::now()); - // notify subscribers if there are new revocations - if has_new_revocations { - inner.change_tx.send_modify(|v| *v += 1); - debug!("notified CRL change subscribers about new revocations"); - } - debug!( "CRL loaded successfully ({} CRL(s), {} total revoked certificate(s))", inner.crl_data.len(), @@ -269,26 +255,6 @@ impl CrlManager { Ok(()) } - /// Check if a certificate serial number is in the revoked set - /// This is used for revocation checking on pooled connections - /// when next request is received - pub fn is_serial_revoked(&self, serial: &[u8]) -> bool { - let inner = self.inner.read().unwrap(); - inner.revoked_serials.contains(serial) - } - - /// get the current set of all revoked certificate serials - pub fn get_revoked_serials(&self) -> HashSet> { - let inner = self.inner.read().unwrap(); - inner.revoked_serials.clone() - } - - /// subscribe to CRL change notifications - pub fn subscribe_to_changes(&self) -> tokio::sync::watch::Receiver { - let inner = self.inner.read().unwrap(); - inner.change_rx.clone() - } - /// Check if any certificate in the chain is revoked pub fn is_revoked_chain( &self, @@ -447,7 +413,7 @@ impl CrlManager { Ok(has_new_revocations) => { debug!("CRL reloaded successfully after file change"); if has_new_revocations { - info!("NEW REVOCATIONS DETECTED - CRL watcher will handle connection closures"); + info!("New revocation detected"); } } Err(e) => error!("failed to reload CRL: {}", e), diff --git a/src/tls/crl_watcher.rs b/src/tls/crl_watcher.rs deleted file mode 100644 index c578e2a88b..0000000000 --- a/src/tls/crl_watcher.rs +++ /dev/null @@ -1,116 +0,0 @@ -// Copyright Istio Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -use std::sync::Arc; -use tracing::{debug, info}; - -use crate::drain::DrainWatcher; -use crate::proxy::connection_manager::ConnectionManager; -use crate::tls::crl::CrlManager; - -/// CrlWatcher monitors CRL updates and terminates connections with revoked certificates -pub struct CrlWatcher { - crl_manager: Arc, - stop: DrainWatcher, - connection_manager: ConnectionManager, -} - -impl CrlWatcher { - pub fn new( - crl_manager: Arc, - stop: DrainWatcher, - connection_manager: ConnectionManager, - ) -> Self { - CrlWatcher { - crl_manager, - stop, - connection_manager, - } - } - - /// Run the CRL watcher loop - pub async fn run(self) { - // Subscribe to CRL changes via watch channel - let mut crl_changed = self.crl_manager.subscribe_to_changes(); - - info!("CRL watcher started"); - - debug!("performing initial connection check for revoked certificates"); - self.check_and_close_revoked_connections().await; - - loop { - tokio::select! { - _ = self.stop.clone().wait_for_drain() => { - info!("CRL watcher shutting down"); - break; - } - _ = crl_changed.changed() => { - debug!("CRL changed, checking connections for revoked certificates"); - self.check_and_close_revoked_connections().await; - } - } - } - } - - /// Check all active connections and close those with revoked certificates - async fn check_and_close_revoked_connections(&self) { - let revoked_serials = self.crl_manager.get_revoked_serials(); - let connections = self.connection_manager.connections(); - - debug!( - "checking {} connection(s) against {} revoked serial(s)", - connections.len(), - revoked_serials.len() - ); - - let mut closed_count = 0; - - for conn in connections { - if let Some(ref serials) = conn.client_cert_serials { - debug!( - "checking connection {} with {} certificate(s)", - conn.ctx, - serials.len() - ); - - // Check if any certificate in the chain is revoked - let is_revoked = serials.iter().any(|serial| { - let revoked = revoked_serials.contains(serial); - if revoked { - debug!(" certificate serial {:02x?} is REVOKED", serial); - } - revoked - }); - - if is_revoked { - self.connection_manager.close(&conn).await; - info!( - "closed connection {} due to certificate revocation", - conn.ctx - ); - closed_count += 1; - } - } - } - - if closed_count > 0 { - info!( - "closed {} connection(s) due to certificate revocation", - closed_count - ); - } else { - debug!("no connections needed to be closed"); - } - } -} diff --git a/src/tls/workload.rs b/src/tls/workload.rs index 62c51b216a..1bb14c9355 100644 --- a/src/tls/workload.rs +++ b/src/tls/workload.rs @@ -118,11 +118,6 @@ impl ClientCertVerifier for TrustDomainVerifier { intermediates: &[CertificateDer<'_>], now: UnixTime, ) -> Result { - debug!( - "Verifying client certificate (chain length: {})", - 1 + intermediates.len() - ); - let res = self .base .verify_client_cert(end_entity, intermediates, now)?; @@ -131,7 +126,6 @@ impl ClientCertVerifier for TrustDomainVerifier { // Check CRL if enabled if let Some(crl_manager) = &self.crl_manager { debug!("CRL checking enabled for client certificate"); - // Fail-closed: reject connection if CRL check returns an error OR if cert is revoked let is_revoked = crl_manager .is_revoked_chain(end_entity, intermediates) .map_err(|e| { From edd3eb1a2c29bfd725d40e31882c6622b1e95426 Mon Sep 17 00:00:00 2001 From: nilekh <1626598+nilekhc@users.noreply.github.com> Date: Mon, 8 Dec 2025 13:41:40 -0800 Subject: [PATCH 06/17] chore: clr validation only at the HBONE layer Signed-off-by: nilekh <1626598+nilekhc@users.noreply.github.com> --- src/proxy/connection_manager.rs | 10 ---------- src/proxy/inbound.rs | 10 ++-------- src/proxy/inbound_passthrough.rs | 2 +- src/tls/crl.rs | 5 ----- 4 files changed, 3 insertions(+), 24 deletions(-) diff --git a/src/proxy/connection_manager.rs b/src/proxy/connection_manager.rs index c71b334bb0..99368fe5a6 100644 --- a/src/proxy/connection_manager.rs +++ b/src/proxy/connection_manager.rs @@ -152,8 +152,6 @@ pub struct InboundConnection { #[serde(flatten)] pub ctx: ProxyRbacContext, pub dest_service: Option, - #[serde(skip)] - pub client_cert_serials: Option>>, // ALL serials in chain (leaf + intermediates) } impl ConnectionManager { @@ -187,14 +185,12 @@ impl ConnectionManager { state: &DemandProxyState, ctx: &ProxyRbacContext, dest_service: Option, - client_cert_serials: Option>>, ) -> Result { // Register before our initial assert. This prevents a race if policy changes between assert() and // track() let conn = InboundConnection { ctx: ctx.clone(), dest_service, - client_cert_serials, }; let Some(watch) = self.register(&conn) else { warn!("failed to track {conn:?}"); @@ -211,7 +207,6 @@ impl ConnectionManager { watch: Some(watch), }) } - // register a connection with the connection manager // this must be done before a connection can be tracked // allows policy to be asserted against the connection @@ -403,7 +398,6 @@ mod tests { dest_workload: Arc::new(test_default_workload()), }, dest_service: None, - client_cert_serials: None, }; // ensure drains contains exactly 1 item @@ -438,7 +432,6 @@ mod tests { dest_workload: Arc::new(test_default_workload()), }, dest_service: None, - client_cert_serials: None, }; let mut close2 = register(&cm, &rbac_ctx2); @@ -507,7 +500,6 @@ mod tests { dest_workload: Arc::new(test_default_workload()), }, dest_service: None, - client_cert_serials: None, }; // create a second connection @@ -528,7 +520,6 @@ mod tests { dest_workload: Arc::new(test_default_workload()), }, dest_service: None, - client_cert_serials: None, }; let another_conn1 = conn1.clone(); @@ -625,7 +616,6 @@ mod tests { dest_workload: Arc::new(test_default_workload()), }, dest_service: None, - client_cert_serials: None, }; // watch the connection let close1 = connection_manager diff --git a/src/proxy/inbound.rs b/src/proxy/inbound.rs index 7d9bad6ca7..bddf80180e 100644 --- a/src/proxy/inbound.rs +++ b/src/proxy/inbound.rs @@ -121,9 +121,6 @@ impl Inbound { debug!(latency=?start.elapsed(), "accepted TLS connection"); let (_, ssl) = tls.get_ref(); let src_identity: Option = tls::identity_from_connection(ssl); - let client_cert_serials: Option>> = - tls::cert_serial_from_connection(ssl); - let conn = Connection { src_identity, src, @@ -140,7 +137,6 @@ impl Inbound { conn.clone(), self.enable_orig_src, req, - client_cert_serials.clone(), ) .instrument(info_span!("inbound", %id, %peer)); // This is for each user connection, so most important to keep small @@ -158,8 +154,7 @@ impl Inbound { // This is per HBONE connection, so while would be nice to be small, at least it // is pooled so typically fewer of these. let serve = Box::pin(assertions::size_between(6000, 8000, serve_conn)); - let _ = serve.await; - Ok(()) + serve.await }; // This is small since it only handles the TLS layer -- the HTTP2 layer is boxed // and measured above. @@ -192,7 +187,6 @@ impl Inbound { conn: Connection, enable_original_source: bool, req: H2Request, - client_cert_serials: Option>>, ) { let src = conn.src; let dst = conn.dst; @@ -222,7 +216,7 @@ impl Inbound { // Define a connection guard to ensure rbac conditions are maintained for the duration of the connection let conn_guard = pi .connection_manager - .assert_rbac(&pi.state, &ri.rbac_ctx, ri.for_host, client_cert_serials) + .assert_rbac(&pi.state, &ri.rbac_ctx, ri.for_host) .await .map_err(InboundFlagError::build( StatusCode::UNAUTHORIZED, diff --git a/src/proxy/inbound_passthrough.rs b/src/proxy/inbound_passthrough.rs index da38633489..e2b8b41464 100644 --- a/src/proxy/inbound_passthrough.rs +++ b/src/proxy/inbound_passthrough.rs @@ -195,7 +195,7 @@ impl InboundPassthrough { let mut conn_guard = match pi .connection_manager - .assert_rbac(&pi.state, &rbac_ctx, None, None) + .assert_rbac(&pi.state, &rbac_ctx, None) .await { Ok(cg) => cg, diff --git a/src/tls/crl.rs b/src/tls/crl.rs index 30c8d1cb64..35166582ff 100644 --- a/src/tls/crl.rs +++ b/src/tls/crl.rs @@ -360,11 +360,6 @@ impl CrlManager { Ok(false) } - /// Refresh the CRL from disk if needed - pub fn refresh(&self) -> Result<(), CrlError> { - self.load_crl().map(|_| ()) - } - /// Start watching the CRL file for changes /// Uses debouncer to handle all file update patterns pub fn start_file_watcher(self: &Arc) -> Result<(), CrlError> { From d46f0b1b7b08b10621b50323785dac46441c3192 Mon Sep 17 00:00:00 2001 From: nilekh <1626598+nilekhc@users.noreply.github.com> Date: Tue, 9 Dec 2025 14:58:05 -0800 Subject: [PATCH 07/17] chore: use rustls-webpki for CRL validation Signed-off-by: nilekh <1626598+nilekhc@users.noreply.github.com> --- Cargo.lock | 1 + Cargo.toml | 1 + fuzz/Cargo.lock | 14 +++- src/proxyfactory.rs | 7 +- src/tls/crl.rs | 171 ++++++++++++++++++++++++-------------------- 5 files changed, 115 insertions(+), 79 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1768da4edf..7177aea4c8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4605,6 +4605,7 @@ dependencies = [ "rustls-native-certs", "rustls-openssl", "rustls-pemfile", + "rustls-webpki", "serde", "serde_json", "serde_yaml", diff --git a/Cargo.toml b/Cargo.toml index 8a25a00139..3948035749 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -103,6 +103,7 @@ tracing = { version = "0.1"} tracing-subscriber = { version = "0.3", features = ["registry", "env-filter", "json"] } url = "2.5" x509-parser = { version = "0.17", default-features = false } +rustls-webpki = { version = "0.103", default-features = false, features = ["alloc"] } tracing-log = "0.2" backoff = "0.4" pin-project-lite = "0.2" diff --git a/fuzz/Cargo.lock b/fuzz/Cargo.lock index d4fdf8682b..11f6ebde6a 100644 --- a/fuzz/Cargo.lock +++ b/fuzz/Cargo.lock @@ -2663,7 +2663,7 @@ dependencies = [ "log", "once_cell", "rustls-pki-types", - "rustls-webpki", + "rustls-webpki 0.102.8", "subtle", "zeroize", ] @@ -2707,6 +2707,17 @@ dependencies = [ "untrusted", ] +[[package]] +name = "rustls-webpki" +version = "0.103.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e4a72fe2bcf7a6ac6fd7d0b9e5cb68aeb7d4c0a0271730218b3e92d43b4eb435" +dependencies = [ + "ring", + "rustls-pki-types", + "untrusted", +] + [[package]] name = "rustversion" version = "1.0.19" @@ -4138,6 +4149,7 @@ dependencies = [ "rustls", "rustls-native-certs", "rustls-pemfile", + "rustls-webpki 0.103.3", "serde", "serde_json", "serde_yaml", diff --git a/src/proxyfactory.rs b/src/proxyfactory.rs index a89ba30a06..abb1635bfa 100644 --- a/src/proxyfactory.rs +++ b/src/proxyfactory.rs @@ -64,7 +64,12 @@ impl ProxyFactory { let manager_arc = Arc::new(manager); if let Err(e) = manager_arc.start_file_watcher() { - tracing::error!("failed to start CRL file watcher: {}", e); + tracing::warn!( + "CRL file watcher could not be started: {}. \ + CRL validation will continue with current file, but \ + CRL updates will require restarting ztunnel.", + e + ); } Some(manager_arc) diff --git a/src/tls/crl.rs b/src/tls/crl.rs index 35166582ff..2c8d4eef37 100644 --- a/src/tls/crl.rs +++ b/src/tls/crl.rs @@ -18,11 +18,11 @@ use notify_debouncer_full::{ notify::{RecursiveMode, Watcher}, }; use rustls::pki_types::CertificateDer; -use std::collections::HashSet; use std::path::PathBuf; use std::sync::{Arc, RwLock}; use std::time::{Duration, SystemTime}; use tracing::{debug, error, info, warn}; +use webpki::CertRevocationList; #[derive(Debug, thiserror::Error)] pub enum CrlError { @@ -37,6 +37,12 @@ pub enum CrlError { #[error("failed to parse certificate: {0}")] CertificateParseError(String), + + #[error("lock error: {0}")] + LockError(String), + + #[error("CRL error: {0}")] + WebPkiError(String), } #[derive(Clone)] @@ -51,12 +57,11 @@ impl std::fmt::Debug for CrlManager { } struct CrlManagerInner { - crl_data: Vec>, + crl_list: Vec>, crl_path: PathBuf, allow_expired: bool, last_load_time: Option, _debouncer: Option>, - revoked_serials: HashSet>, } impl CrlManager { @@ -69,12 +74,11 @@ impl CrlManager { let manager = Self { inner: Arc::new(RwLock::new(CrlManagerInner { - crl_data: Vec::new(), + crl_list: Vec::new(), crl_path: crl_path.clone(), allow_expired, last_load_time: None, _debouncer: None, - revoked_serials: HashSet::new(), })), }; @@ -98,9 +102,11 @@ impl CrlManager { Ok(manager) } - /// Load or reload the CRL from disk pub fn load_crl(&self) -> Result { - let mut inner = self.inner.write().unwrap(); + let mut inner = self + .inner + .write() + .map_err(|e| CrlError::LockError(format!("failed to acquire write lock: {}", e)))?; debug!("loading CRL from {:?}", inner.crl_path); let data = std::fs::read(&inner.crl_path)?; @@ -124,59 +130,56 @@ impl CrlManager { debug!("found {} CRL block(s) in file", der_crls.len()); + let mut parsed_crls = Vec::new(); let mut total_revoked = 0; - let mut new_revoked_serials = HashSet::new(); for (idx, der_data) in der_crls.iter().enumerate() { - use x509_parser::prelude::*; - let (_, crl) = CertificateRevocationList::from_der(der_data).map_err(|e| { - CrlError::ParseError(format!("Failed to parse CRL {}: {}", idx + 1, e)) + let owned_crl = webpki::OwnedCertRevocationList::from_der(der_data).map_err(|e| { + CrlError::WebPkiError(format!("failed to parse CRL {}: {:?}", idx + 1, e)) })?; - debug!("CRL {}:", idx + 1); - debug!(" Issuer: {}", crl.tbs_cert_list.issuer); - debug!(" this update: {:?}", crl.tbs_cert_list.this_update); - if let Some(next_update) = &crl.tbs_cert_list.next_update { - debug!(" next update: {:?}", next_update); - } - let revoked_count = crl.tbs_cert_list.revoked_certificates.len(); - debug!(" revoked certificates: {}", revoked_count); - total_revoked += revoked_count; + let crl = CertRevocationList::from(owned_crl); - for revoked in crl.tbs_cert_list.revoked_certificates.iter() { - let serial = revoked.serial().to_bytes_be(); - new_revoked_serials.insert(serial); + // use x509-parser for detail logging + use x509_parser::prelude::*; + if let Ok((_, crl_info)) = CertificateRevocationList::from_der(der_data) { + debug!("CRL {}:", idx + 1); + debug!(" Issuer: {}", crl_info.tbs_cert_list.issuer); + debug!(" this update: {:?}", crl_info.tbs_cert_list.this_update); + if let Some(next_update) = &crl_info.tbs_cert_list.next_update { + debug!(" next update: {:?}", next_update); + } + let revoked_count = crl_info.tbs_cert_list.revoked_certificates.len(); + debug!(" revoked certificates: {}", revoked_count); + total_revoked += revoked_count; + + // validate CRL expiration using x509-parser + // webpki doesn't expose next_update easily + Self::validate_crl(der_data, inner.allow_expired)?; + } else { + // if x509-parser fails, we still have webpki parsed CRL, but with fewer log details + debug!("CRL {}: parsed successfully", idx + 1); } - // verify CRL validity - Self::verify_crl_validity(&crl, inner.allow_expired)?; + parsed_crls.push(crl); } - // check if there are new revocations compared to previous load - let has_new_revocations = !new_revoked_serials.is_subset(&inner.revoked_serials); + let has_new_revocations = parsed_crls.len() != inner.crl_list.len(); if has_new_revocations { - // calculate which serials are new - let newly_revoked: HashSet<_> = new_revoked_serials - .difference(&inner.revoked_serials) - .collect(); warn!( - "detected {} NEW certificate revocation(s)", - newly_revoked.len() + "CRL file changed - reloaded with {} CRL(s)", + parsed_crls.len() ); - for serial in newly_revoked { - warn!(" newly revoked serial: {:?}", serial); - } } - // store all CRL DER data and update revoked serials - inner.crl_data = der_crls; - inner.revoked_serials = new_revoked_serials; + // store parsed CRL objects + inner.crl_list = parsed_crls; inner.last_load_time = Some(SystemTime::now()); debug!( "CRL loaded successfully ({} CRL(s), {} total revoked certificate(s))", - inner.crl_data.len(), + inner.crl_list.len(), total_revoked ); Ok(has_new_revocations) @@ -226,16 +229,18 @@ impl CrlManager { Ok(crls) } - /// Verify CRL validity period - fn verify_crl_validity( - crl: &x509_parser::revocation_list::CertificateRevocationList, - allow_expired: bool, - ) -> Result<(), CrlError> { - let now = SystemTime::now(); - let unix_now = now + /// Validate CRL + fn validate_crl(der_data: &[u8], allow_expired: bool) -> Result<(), CrlError> { + use x509_parser::prelude::*; + + // parse with x509-parser only for expiration validation + let (_, crl) = CertificateRevocationList::from_der(der_data) + .map_err(|e| CrlError::ParseError(format!("validation parse failed: {}", e)))?; + + let now = SystemTime::now() .duration_since(SystemTime::UNIX_EPOCH) - .expect("Time went backwards") - .as_secs() as i64; + .map_err(|e| CrlError::ParseError(format!("system time error: {}", e)))?; + let unix_now = now.as_secs() as i64; // check thisUpdate (CRL issue time) if unix_now < crl.tbs_cert_list.this_update.timestamp() { @@ -304,58 +309,63 @@ impl CrlManager { } /// Internal method to check if a single certificate is revoked - /// Checks the certificate against ALL loaded CRLs + /// Checks the certificate against ALL loaded CRLs using rustls-webpki fn is_cert_revoked(&self, cert: &CertificateDer) -> Result { - let inner = self.inner.read().unwrap(); + let inner = self + .inner + .read() + .map_err(|e| CrlError::LockError(format!("failed to acquire read lock: {}", e)))?; // if no CRLs are loaded, try to load them now - if inner.crl_data.is_empty() { + if inner.crl_list.is_empty() { drop(inner); debug!("CRL not loaded, attempting to load now"); self.load_crl()?; return self.is_cert_revoked(cert); } - // parse the certificate to get its serial number + // extract certificate serial number using x509-parser + // webpki's Cert::from_der is not public, so we need x509-parser for this use x509_parser::prelude::*; let (_, parsed_cert) = X509Certificate::from_der(cert) .map_err(|e| CrlError::CertificateParseError(e.to_string()))?; - let cert_serial = &parsed_cert.serial; + let cert_serial = parsed_cert.serial.to_bytes_be(); debug!("certificate serial number: {:?}", cert_serial); // check the certificate against ALL CRLs - for (idx, crl_data) in inner.crl_data.iter().enumerate() { - // Parse CRL from stored data - let (_, crl) = CertificateRevocationList::from_der(crl_data).map_err(|e| { - CrlError::ParseError(format!("failed to parse stored CRL {}: {}", idx + 1, e)) - })?; - - debug!( - "checking against CRL {} (issuer: {})", - idx + 1, - crl.tbs_cert_list.issuer - ); + for (idx, crl) in inner.crl_list.iter().enumerate() { + debug!("checking against CRL {}", idx + 1); - // check if the certificate's serial number is in this CRL's revoked list - for revoked_cert in &crl.tbs_cert_list.revoked_certificates { - if revoked_cert.serial() == cert_serial { + match crl.find_serial(&cert_serial) { + Ok(Some(revoked_cert)) => { warn!( - "certificate with serial {:?} is REVOKED in CRL {} (issuer: {})", + "certificate with serial {:?} is REVOKED in CRL {}", cert_serial, - idx + 1, - crl.tbs_cert_list.issuer + idx + 1 ); warn!("revocation date: {:?}", revoked_cert.revocation_date); + if let Some(reason) = revoked_cert.reason_code { + warn!("revocation reason: {:?}", reason); + } return Ok(true); } + Ok(None) => { + // certificate isn't found in this CRL, continue checking others + continue; + } + Err(e) => { + // error parsing revoked certificates in this CRL + error!("error checking CRL {}: {:?}", idx + 1, e); + return Err(CrlError::WebPkiError(format!("CRL lookup failed: {:?}", e))); + } } } debug!( "certificate serial {:?} is not in any of the {} CRL(s)", cert_serial, - inner.crl_data.len() + inner.crl_list.len() ); Ok(false) } @@ -364,7 +374,10 @@ impl CrlManager { /// Uses debouncer to handle all file update patterns pub fn start_file_watcher(self: &Arc) -> Result<(), CrlError> { let crl_path = { - let inner = self.inner.read().unwrap(); + let inner = self + .inner + .read() + .map_err(|e| CrlError::LockError(format!("failed to acquire read lock: {}", e)))?; inner.crl_path.clone() }; @@ -433,7 +446,10 @@ impl CrlManager { // Store debouncer to keep it alive { - let mut inner = self.inner.write().unwrap(); + let mut inner = self + .inner + .write() + .map_err(|e| CrlError::LockError(format!("failed to acquire write lock: {}", e)))?; inner._debouncer = Some(debouncer); } @@ -456,9 +472,10 @@ mod tests { #[test] fn test_crl_manager_invalid_file() { - let mut file = NamedTempFile::new().unwrap(); - file.write_all(b"not a valid CRL").unwrap(); - file.flush().unwrap(); + let mut file = NamedTempFile::new().expect("failed to create temporary test file"); + file.write_all(b"not a valid CRL") + .expect("failed to write test data to temporary file"); + file.flush().expect("failed to flush temporary test file"); let result = CrlManager::new(file.path().to_path_buf(), false); assert!(result.is_err(), "should fail on invalid CRL data"); From f24f65c43be2cad29dea2905552dc689bf41927b Mon Sep 17 00:00:00 2001 From: nilekh <1626598+nilekhc@users.noreply.github.com> Date: Thu, 18 Dec 2025 10:38:54 -0800 Subject: [PATCH 08/17] chore: validates CRL using webpki instead of custom implementation. Signed-off-by: nilekh <1626598+nilekhc@users.noreply.github.com> --- src/config.rs | 5 -- src/proxyfactory.rs | 2 +- src/tls/crl.rs | 171 +++++++++++++------------------------------- src/tls/workload.rs | 14 ++-- 4 files changed, 58 insertions(+), 134 deletions(-) diff --git a/src/config.rs b/src/config.rs index 0c1b098f3c..bb8c0e7237 100644 --- a/src/config.rs +++ b/src/config.rs @@ -80,7 +80,6 @@ const UNSTABLE_ENABLE_SOCKS5: &str = "UNSTABLE_ENABLE_SOCKS5"; const ENABLE_CRL: &str = "ENABLE_CRL"; const CRL_PATH: &str = "CRL_PATH"; -const ALLOW_EXPIRED_CRL: &str = "ALLOW_EXPIRED_CRL"; const DEFAULT_WORKER_THREADS: u16 = 2; const DEFAULT_ADMIN_PORT: u16 = 15000; @@ -322,9 +321,6 @@ pub struct Config { // Path to CRL file pub crl_path: PathBuf, - - // Allow expired CRL (for testing/rollout scenarios) - pub allow_expired_crl: bool, } #[derive(serde::Serialize, Clone, Copy, Debug)] @@ -882,7 +878,6 @@ pub fn construct_config(pc: ProxyConfig) -> Result { enable_crl: parse_default(ENABLE_CRL, false)?, crl_path: parse_default(CRL_PATH, PathBuf::from(DEFAULT_CRL_PATH))?, - allow_expired_crl: parse_default(ALLOW_EXPIRED_CRL, false)?, }) } diff --git a/src/proxyfactory.rs b/src/proxyfactory.rs index abb1635bfa..f6633943ff 100644 --- a/src/proxyfactory.rs +++ b/src/proxyfactory.rs @@ -59,7 +59,7 @@ impl ProxyFactory { // Initialize CRL manager ONCE if enabled let crl_manager = if config.enable_crl { - match tls::crl::CrlManager::new(config.crl_path.clone(), config.allow_expired_crl) { + match tls::crl::CrlManager::new(config.crl_path.clone()) { Ok(manager) => { let manager_arc = Arc::new(manager); diff --git a/src/tls/crl.rs b/src/tls/crl.rs index 2c8d4eef37..bf0362ce09 100644 --- a/src/tls/crl.rs +++ b/src/tls/crl.rs @@ -20,7 +20,7 @@ use notify_debouncer_full::{ use rustls::pki_types::CertificateDer; use std::path::PathBuf; use std::sync::{Arc, RwLock}; -use std::time::{Duration, SystemTime}; +use std::time::Duration; use tracing::{debug, error, info, warn}; use webpki::CertRevocationList; @@ -32,9 +32,6 @@ pub enum CrlError { #[error("failed to parse CRL: {0}")] ParseError(String), - #[error("CRL is expired")] - ExpiredCrl, - #[error("failed to parse certificate: {0}")] CertificateParseError(String), @@ -59,25 +56,18 @@ impl std::fmt::Debug for CrlManager { struct CrlManagerInner { crl_list: Vec>, crl_path: PathBuf, - allow_expired: bool, - last_load_time: Option, _debouncer: Option>, } impl CrlManager { /// Create a new CRL manager - pub fn new(crl_path: PathBuf, allow_expired: bool) -> Result { - debug!( - "initializing CRL Manager: path={:?}, allow_expired={}", - crl_path, allow_expired - ); + pub fn new(crl_path: PathBuf) -> Result { + debug!("initializing crl manager: path={:?}", crl_path); let manager = Self { inner: Arc::new(RwLock::new(CrlManagerInner { crl_list: Vec::new(), crl_path: crl_path.clone(), - allow_expired, - last_load_time: None, _debouncer: None, })), }; @@ -131,7 +121,6 @@ impl CrlManager { debug!("found {} CRL block(s) in file", der_crls.len()); let mut parsed_crls = Vec::new(); - let mut total_revoked = 0; for (idx, der_data) in der_crls.iter().enumerate() { let owned_crl = webpki::OwnedCertRevocationList::from_der(der_data).map_err(|e| { @@ -144,21 +133,13 @@ impl CrlManager { use x509_parser::prelude::*; if let Ok((_, crl_info)) = CertificateRevocationList::from_der(der_data) { debug!("CRL {}:", idx + 1); - debug!(" Issuer: {}", crl_info.tbs_cert_list.issuer); + debug!(" issuer: {}", crl_info.tbs_cert_list.issuer); debug!(" this update: {:?}", crl_info.tbs_cert_list.this_update); if let Some(next_update) = &crl_info.tbs_cert_list.next_update { debug!(" next update: {:?}", next_update); } let revoked_count = crl_info.tbs_cert_list.revoked_certificates.len(); debug!(" revoked certificates: {}", revoked_count); - total_revoked += revoked_count; - - // validate CRL expiration using x509-parser - // webpki doesn't expose next_update easily - Self::validate_crl(der_data, inner.allow_expired)?; - } else { - // if x509-parser fails, we still have webpki parsed CRL, but with fewer log details - debug!("CRL {}: parsed successfully", idx + 1); } parsed_crls.push(crl); @@ -175,13 +156,8 @@ impl CrlManager { // store parsed CRL objects inner.crl_list = parsed_crls; - inner.last_load_time = Some(SystemTime::now()); - debug!( - "CRL loaded successfully ({} CRL(s), {} total revoked certificate(s))", - inner.crl_list.len(), - total_revoked - ); + debug!("CRL loaded successfully ({} CRL(s))", inner.crl_list.len()); Ok(has_new_revocations) } @@ -229,88 +205,10 @@ impl CrlManager { Ok(crls) } - /// Validate CRL - fn validate_crl(der_data: &[u8], allow_expired: bool) -> Result<(), CrlError> { - use x509_parser::prelude::*; - - // parse with x509-parser only for expiration validation - let (_, crl) = CertificateRevocationList::from_der(der_data) - .map_err(|e| CrlError::ParseError(format!("validation parse failed: {}", e)))?; - - let now = SystemTime::now() - .duration_since(SystemTime::UNIX_EPOCH) - .map_err(|e| CrlError::ParseError(format!("system time error: {}", e)))?; - let unix_now = now.as_secs() as i64; - - // check thisUpdate (CRL issue time) - if unix_now < crl.tbs_cert_list.this_update.timestamp() { - warn!("CRL is not yet valid"); - } - - // check nextUpdate (CRL expiry) - if let Some(next_update) = &crl.tbs_cert_list.next_update - && unix_now > next_update.timestamp() - { - if !allow_expired { - return Err(CrlError::ExpiredCrl); - } - warn!("CRL is expired but allow_expired_crl is enabled"); - } - - Ok(()) - } - - /// Check if any certificate in the chain is revoked - pub fn is_revoked_chain( - &self, - end_entity: &CertificateDer, - intermediates: &[CertificateDer], - ) -> Result { + /// Check if a certificate is revoked using webpki's native CRL API + pub fn is_cert_revoked(&self, cert: &CertificateDer) -> Result { use x509_parser::prelude::*; - debug!( - "checking certificate chain against CRL (chain length: {})", - 1 + intermediates.len() - ); - - // Log end-entity certificate serial - if let Ok((_, parsed)) = X509Certificate::from_der(end_entity) { - debug!(" end-entity serial: {:?}", parsed.serial.to_bytes_be()); - } - - // Log intermediate certificate serials - for (idx, intermediate) in intermediates.iter().enumerate() { - if let Ok((_, parsed)) = X509Certificate::from_der(intermediate) { - debug!( - " intermediate {} serial: {:?}", - idx, - parsed.serial.to_bytes_be() - ); - } - } - - debug!("checking leaf certificate"); - if self.is_cert_revoked(end_entity)? { - warn!("leaf certificate is REVOKED"); - return Ok(true); - } - - // check all intermediate certificates - for (idx, intermediate) in intermediates.iter().enumerate() { - debug!("checking intermediate certificate {} in chain", idx); - if self.is_cert_revoked(intermediate)? { - warn!("intermediate CA certificate at position {} is REVOKED", idx); - return Ok(true); - } - } - - debug!("certificate chain validation passed - no revoked certificates found"); - Ok(false) - } - - /// Internal method to check if a single certificate is revoked - /// Checks the certificate against ALL loaded CRLs using rustls-webpki - fn is_cert_revoked(&self, cert: &CertificateDer) -> Result { let inner = self .inner .read() @@ -319,43 +217,43 @@ impl CrlManager { // if no CRLs are loaded, try to load them now if inner.crl_list.is_empty() { drop(inner); - debug!("CRL not loaded, attempting to load now"); + debug!("crl not loaded, attempting to load now"); self.load_crl()?; return self.is_cert_revoked(cert); } // extract certificate serial number using x509-parser - // webpki's Cert::from_der is not public, so we need x509-parser for this - use x509_parser::prelude::*; + // webpki doesn't expose cert parsing publicly, so we use x509-parser for this let (_, parsed_cert) = X509Certificate::from_der(cert) .map_err(|e| CrlError::CertificateParseError(e.to_string()))?; let cert_serial = parsed_cert.serial.to_bytes_be(); - debug!("certificate serial number: {:?}", cert_serial); + debug!("checking certificate serial: {:?}", cert_serial); - // check the certificate against ALL CRLs + // check the certificate against ALL CRLs using webpki's native API for (idx, crl) in inner.crl_list.iter().enumerate() { debug!("checking against CRL {}", idx + 1); match crl.find_serial(&cert_serial) { Ok(Some(revoked_cert)) => { - warn!( + error!( "certificate with serial {:?} is REVOKED in CRL {}", cert_serial, idx + 1 ); - warn!("revocation date: {:?}", revoked_cert.revocation_date); + error!("revocation date: {:?}", revoked_cert.revocation_date); if let Some(reason) = revoked_cert.reason_code { - warn!("revocation reason: {:?}", reason); + error!("revocation reason: {:?}", reason); } return Ok(true); } Ok(None) => { // certificate isn't found in this CRL, continue checking others + debug!("certificate not found in CRL {}", idx + 1); continue; } Err(e) => { - // error parsing revoked certificates in this CRL + // error during CRL lookup error!("error checking CRL {}: {:?}", idx + 1, e); return Err(CrlError::WebPkiError(format!("CRL lookup failed: {:?}", e))); } @@ -363,13 +261,44 @@ impl CrlManager { } debug!( - "certificate serial {:?} is not in any of the {} CRL(s)", + "certificate serial {:?} is not revoked in any of the {} CRL(s)", cert_serial, inner.crl_list.len() ); Ok(false) } + /// Check if any certificate in the chain is revoked using webpki's native CRL API + pub fn is_revoked_chain( + &self, + end_entity: &CertificateDer, + intermediates: &[CertificateDer], + ) -> Result { + debug!( + "checking certificate chain against CRL (chain length: {})", + 1 + intermediates.len() + ); + + // check leaf certificate + debug!("checking leaf certificate"); + if self.is_cert_revoked(end_entity)? { + error!("leaf certificate is REVOKED"); + return Ok(true); + } + + // check all intermediate certificates + for (idx, intermediate) in intermediates.iter().enumerate() { + debug!("checking intermediate certificate {} in chain", idx); + if self.is_cert_revoked(intermediate)? { + error!("intermediate CA certificate at position {} is REVOKED", idx); + return Ok(true); + } + } + + debug!("certificate chain validation passed - no revoked certificates found"); + Ok(false) + } + /// Start watching the CRL file for changes /// Uses debouncer to handle all file update patterns pub fn start_file_watcher(self: &Arc) -> Result<(), CrlError> { @@ -466,7 +395,7 @@ mod tests { #[test] fn test_crl_manager_missing_file() { - let result = CrlManager::new(PathBuf::from("/nonexistent/path/crl.pem"), false); + let result = CrlManager::new(PathBuf::from("/nonexistent/path/crl.pem")); assert!(result.is_ok(), "should handle missing CRL file gracefully"); } @@ -477,7 +406,7 @@ mod tests { .expect("failed to write test data to temporary file"); file.flush().expect("failed to flush temporary test file"); - let result = CrlManager::new(file.path().to_path_buf(), false); + let result = CrlManager::new(file.path().to_path_buf()); assert!(result.is_err(), "should fail on invalid CRL data"); } } diff --git a/src/tls/workload.rs b/src/tls/workload.rs index 1bb14c9355..f7ffed98c7 100644 --- a/src/tls/workload.rs +++ b/src/tls/workload.rs @@ -123,26 +123,26 @@ impl ClientCertVerifier for TrustDomainVerifier { .verify_client_cert(end_entity, intermediates, now)?; self.verify_trust_domain(end_entity)?; - // Check CRL if enabled + // check CRL if enabled if let Some(crl_manager) = &self.crl_manager { - debug!("CRL checking enabled for client certificate"); + debug!("crl checking enabled for client certificate"); let is_revoked = crl_manager .is_revoked_chain(end_entity, intermediates) .map_err(|e| { - error!("CRL validation failed for client certificate: {}", e); - rustls::Error::General(format!("Certificate revocation check failed: {}", e)) + error!("crl validation failed for client certificate: {}", e); + rustls::Error::General(format!("certificate revocation check failed: {}", e)) })?; if is_revoked { - error!("Client certificate is REVOKED - rejecting connection"); + error!("client certificate is REVOKED - rejecting connection"); return Err(rustls::Error::InvalidCertificate( rustls::CertificateError::Revoked, )); } - debug!("Client certificate chain is valid (not revoked)"); + debug!("client certificate chain is valid (not revoked)"); } else { - debug!("CRL checking disabled for client certificate"); + debug!("crl checking disabled for client certificate"); } Ok(res) From ebceefc7f88d4787112f1cc4cd633fd13c48d6ea Mon Sep 17 00:00:00 2001 From: nilekh <1626598+nilekhc@users.noreply.github.com> Date: Wed, 7 Jan 2026 12:22:51 -0800 Subject: [PATCH 09/17] chore: addresses review comments Signed-off-by: nilekh <1626598+nilekhc@users.noreply.github.com> --- src/config.rs | 15 +- src/proxyfactory.rs | 14 +- src/tls/crl.rs | 338 +++++++++++++++++++++++++++----------------- src/tls/workload.rs | 14 +- 4 files changed, 227 insertions(+), 154 deletions(-) diff --git a/src/config.rs b/src/config.rs index bb8c0e7237..2918f143cf 100644 --- a/src/config.rs +++ b/src/config.rs @@ -78,7 +78,6 @@ const HTTP2_FRAME_SIZE: &str = "HTTP2_FRAME_SIZE"; const UNSTABLE_ENABLE_SOCKS5: &str = "UNSTABLE_ENABLE_SOCKS5"; -const ENABLE_CRL: &str = "ENABLE_CRL"; const CRL_PATH: &str = "CRL_PATH"; const DEFAULT_WORKER_THREADS: u16 = 2; @@ -111,7 +110,6 @@ const DEFAULT_ROOT_CERT_PROVIDER: &str = "./var/run/secrets/istio/root-cert.pem" const TOKEN_PROVIDER_ENV: &str = "AUTH_TOKEN"; const DEFAULT_TOKEN_PROVIDER: &str = "./var/run/secrets/tokens/istio-token"; const CERT_SYSTEM: &str = "SYSTEM"; -const DEFAULT_CRL_PATH: &str = "./var/run/secrets/istio/crl/ca-crl.pem"; const PROXY_MODE_DEDICATED: &str = "dedicated"; const PROXY_MODE_SHARED: &str = "shared"; @@ -316,11 +314,8 @@ pub struct Config { pub ipv6_enabled: bool, - // Enable CRL (Certificate Revocation List) checking - pub enable_crl: bool, - - // Path to CRL file - pub crl_path: PathBuf, + // path to CRL file; if set, enables CRL checking + pub crl_path: Option, } #[derive(serde::Serialize, Clone, Copy, Debug)] @@ -876,8 +871,10 @@ pub fn construct_config(pc: ProxyConfig) -> Result { ztunnel_workload, ipv6_enabled, - enable_crl: parse_default(ENABLE_CRL, false)?, - crl_path: parse_default(CRL_PATH, PathBuf::from(DEFAULT_CRL_PATH))?, + crl_path: env::var(CRL_PATH) + .ok() + .filter(|s| !s.is_empty()) + .map(PathBuf::from), }) } diff --git a/src/proxyfactory.rs b/src/proxyfactory.rs index f6633943ff..202117a960 100644 --- a/src/proxyfactory.rs +++ b/src/proxyfactory.rs @@ -57,17 +57,17 @@ impl ProxyFactory { } }; - // Initialize CRL manager ONCE if enabled - let crl_manager = if config.enable_crl { - match tls::crl::CrlManager::new(config.crl_path.clone()) { + // Initialize CRL manager if crl_path is set + let crl_manager = if let Some(crl_path) = &config.crl_path { + match tls::crl::CrlManager::new(crl_path.clone()) { Ok(manager) => { let manager_arc = Arc::new(manager); if let Err(e) = manager_arc.start_file_watcher() { tracing::warn!( - "CRL file watcher could not be started: {}. \ - CRL validation will continue with current file, but \ - CRL updates will require restarting ztunnel.", + "crl file watcher could not be started: {}. \ + crl validation will continue with current file, but \ + crl updates will require restarting ztunnel.", e ); } @@ -75,7 +75,7 @@ impl ProxyFactory { Some(manager_arc) } Err(e) => { - tracing::error!("Failed to initialize CRL manager: {}", e); + tracing::debug!("failed to initialize crl manager: {}", e); None } } diff --git a/src/tls/crl.rs b/src/tls/crl.rs index bf0362ce09..4969fb26b0 100644 --- a/src/tls/crl.rs +++ b/src/tls/crl.rs @@ -21,9 +21,20 @@ use rustls::pki_types::CertificateDer; use std::path::PathBuf; use std::sync::{Arc, RwLock}; use std::time::Duration; -use tracing::{debug, error, info, warn}; +use tracing::{debug, info, warn}; use webpki::CertRevocationList; +// CrlManager handles certificate revocation list (CRL) validation. +// +// validation notes: +// - webpki's from_der() validates ASN.1 structure, CRL version (v2), and rejects +// delta CRLs and indirect CRLs with unknown critical extensions +// - time bounds (thisUpdate/nextUpdate) are validated during load and at lookup time +// - signature verification is NOT performed during loading; this implementation +// relies on the CRL being from a trusted source (e.g., mounted ConfigMap) +// - per RFC 5280 section 3.3, entries may be removed from CRLs after the +// certificate's validity period expires + #[derive(Debug, thiserror::Error)] pub enum CrlError { #[error("failed to read CRL file: {0}")] @@ -35,9 +46,6 @@ pub enum CrlError { #[error("failed to parse certificate: {0}")] CertificateParseError(String), - #[error("lock error: {0}")] - LockError(String), - #[error("CRL error: {0}")] WebPkiError(String), } @@ -53,16 +61,22 @@ impl std::fmt::Debug for CrlManager { } } +// stores both the webpki CRL and the original DER for time validation at lookup +struct CrlEntry { + crl: CertRevocationList<'static>, + der: Vec, +} + struct CrlManagerInner { - crl_list: Vec>, + crl_list: Vec, crl_path: PathBuf, _debouncer: Option>, } impl CrlManager { - /// Create a new CRL manager + /// creates a new CRL manager pub fn new(crl_path: PathBuf) -> Result { - debug!("initializing crl manager: path={:?}", crl_path); + debug!(path = ?crl_path, "initializing crl manager"); let manager = Self { inner: Arc::new(RwLock::new(CrlManagerInner { @@ -72,18 +86,18 @@ impl CrlManager { })), }; - // Try to load the CRL, but don't fail if the file doesn't exist yet + // try to load the CRL, but don't fail if the file doesn't exist yet // (it might be mounted later via ConfigMap) if let Err(e) = manager.load_crl() { match e { CrlError::IoError(ref io_err) if io_err.kind() == std::io::ErrorKind::NotFound => { warn!( - "CRL file not found at {:?}, will retry on first validation", - crl_path + path = ?crl_path, + "crl file not found, will retry on first validation" ); } _ => { - error!("failed to initialize CRL Manager: {}", e); + debug!(error = %e, "failed to initialize crl manager"); return Err(e); } } @@ -92,80 +106,136 @@ impl CrlManager { Ok(manager) } + /// validates CRL time bounds: thisUpdate <= now <= nextUpdate + /// returns true if valid, false if expired or not yet valid + fn is_crl_time_valid( + crl_info: &x509_parser::revocation_list::CertificateRevocationList, + crl_index: usize, + ) -> bool { + use x509_parser::time::ASN1Time; + + let now = ASN1Time::now(); + let tbs = &crl_info.tbs_cert_list; + + // check thisUpdate <= now (CRL must be issued already) + if tbs.this_update > now { + warn!( + crl = crl_index + 1, + this_update = %tbs.this_update, + "crl is not yet valid, skipping" + ); + return false; + } + + // check now <= nextUpdate (CRL must not be expired) + if let Some(next_update) = tbs.next_update + && now > next_update + { + warn!( + crl = crl_index + 1, + next_update = %next_update, + "crl is expired, skipping" + ); + return false; + } + // note: if nextUpdate is absent, we accept the CRL (per RFC 5280, nextUpdate is optional) + + true + } + pub fn load_crl(&self) -> Result { - let mut inner = self - .inner - .write() - .map_err(|e| CrlError::LockError(format!("failed to acquire write lock: {}", e)))?; + let mut inner = self.inner.write().unwrap(); - debug!("loading CRL from {:?}", inner.crl_path); + debug!(path = ?inner.crl_path, "loading crl"); let data = std::fs::read(&inner.crl_path)?; + // empty file means no revocations - this is valid if data.is_empty() { - warn!("CRL file is empty at {:?}", inner.crl_path); - return Err(CrlError::ParseError("CRL file is empty".to_string())); + debug!(path = ?inner.crl_path, "crl file is empty, treating as no revocations"); + let crl_count_changed = !inner.crl_list.is_empty(); + inner.crl_list.clear(); + return Ok(crl_count_changed); } - debug!("read CRL file: {} bytes", data.len()); + debug!(bytes = data.len(), "read crl file"); - // Parse all CRL blocks (handles concatenated CRLs) + // parse all CRL blocks (handles concatenated CRLs) let der_crls = if data.starts_with(b"-----BEGIN") { - debug!("CRL is in PEM format, extracting all CRL blocks"); + debug!("crl is in PEM format, extracting all crl blocks"); Self::parse_pem_crls(&data)? } else { - debug!("CRL is in DER format"); - // Single DER-encoded CRL + debug!("crl is in DER format"); + // single DER-encoded CRL vec![data] }; - debug!("found {} CRL block(s) in file", der_crls.len()); + // empty PEM file (no CRL blocks) means no revocations + if der_crls.is_empty() { + debug!("no crl blocks found, treating as no revocations"); + let crl_count_changed = !inner.crl_list.is_empty(); + inner.crl_list.clear(); + return Ok(crl_count_changed); + } + + debug!(count = der_crls.len(), "found crl block(s) in file"); let mut parsed_crls = Vec::new(); for (idx, der_data) in der_crls.iter().enumerate() { + // parse with x509-parser for validation and logging + use x509_parser::prelude::*; + let (_, crl_info) = CertificateRevocationList::from_der(der_data).map_err(|e| { + CrlError::ParseError(format!("failed to parse crl {}: {}", idx + 1, e)) + })?; + + let tbs = &crl_info.tbs_cert_list; + + // validate time bounds before accepting the CRL + if !Self::is_crl_time_valid(&crl_info, idx) { + // skip this CRL but continue processing others + continue; + } + + // parse with webpki for revocation checking let owned_crl = webpki::OwnedCertRevocationList::from_der(der_data).map_err(|e| { - CrlError::WebPkiError(format!("failed to parse CRL {}: {:?}", idx + 1, e)) + CrlError::WebPkiError(format!("failed to parse crl {}: {:?}", idx + 1, e)) })?; let crl = CertRevocationList::from(owned_crl); - // use x509-parser for detail logging - use x509_parser::prelude::*; - if let Ok((_, crl_info)) = CertificateRevocationList::from_der(der_data) { - debug!("CRL {}:", idx + 1); - debug!(" issuer: {}", crl_info.tbs_cert_list.issuer); - debug!(" this update: {:?}", crl_info.tbs_cert_list.this_update); - if let Some(next_update) = &crl_info.tbs_cert_list.next_update { - debug!(" next update: {:?}", next_update); - } - let revoked_count = crl_info.tbs_cert_list.revoked_certificates.len(); - debug!(" revoked certificates: {}", revoked_count); - } + debug!( + crl = idx + 1, + issuer = %tbs.issuer, + this_update = %tbs.this_update, + next_update = ?tbs.next_update, + revoked = tbs.revoked_certificates.len(), + "loaded crl" + ); - parsed_crls.push(crl); + parsed_crls.push(CrlEntry { + crl, + der: der_data.clone(), + }); } - let has_new_revocations = parsed_crls.len() != inner.crl_list.len(); + let crl_count_changed = parsed_crls.len() != inner.crl_list.len(); - if has_new_revocations { - warn!( - "CRL file changed - reloaded with {} CRL(s)", - parsed_crls.len() - ); + if crl_count_changed { + debug!(count = parsed_crls.len(), "crl file changed, reloaded"); } // store parsed CRL objects inner.crl_list = parsed_crls; - debug!("CRL loaded successfully ({} CRL(s))", inner.crl_list.len()); - Ok(has_new_revocations) + debug!(count = inner.crl_list.len(), "crl loaded successfully"); + Ok(crl_count_changed) } - /// Parse PEM-encoded CRL data that may contain multiple CRL blocks - /// Returns a Vec of DER-encoded CRLs + /// parses PEM-encoded CRL data that may contain multiple CRL blocks + /// returns a Vec of DER-encoded CRLs (empty vec if no blocks found) fn parse_pem_crls(pem_data: &[u8]) -> Result>, CrlError> { let data_str = std::str::from_utf8(pem_data) - .map_err(|e| CrlError::ParseError(format!("Invalid UTF-8: {}", e)))?; + .map_err(|e| CrlError::ParseError(format!("invalid UTF-8: {}", e)))?; let mut crls = Vec::new(); let mut in_pem = false; @@ -174,7 +244,7 @@ impl CrlManager { for line in data_str.lines() { if line.starts_with("-----BEGIN") { in_pem = true; - base64_data.clear(); // Start new CRL block + base64_data.clear(); // start new CRL block continue; } if line.starts_with("-----END") { @@ -196,30 +266,40 @@ impl CrlManager { } } - if crls.is_empty() { - return Err(CrlError::ParseError( - "no valid CRL blocks found in PEM data".to_string(), - )); - } - Ok(crls) } - /// Check if a certificate is revoked using webpki's native CRL API - pub fn is_cert_revoked(&self, cert: &CertificateDer) -> Result { + /// checks if a certificate is revoked. + /// returns true if revoked, false otherwise (including on errors - fail-open policy) + pub fn is_cert_revoked(&self, cert: &CertificateDer) -> bool { + match self.check_cert_revocation(cert) { + Ok(is_revoked) => is_revoked, + Err(e) => { + debug!(error = %e, "crl check failed, allowing connection (fail-open)"); + false + } + } + } + + /// internal implementation that returns Result for error handling + fn check_cert_revocation(&self, cert: &CertificateDer) -> Result { use x509_parser::prelude::*; - let inner = self - .inner - .read() - .map_err(|e| CrlError::LockError(format!("failed to acquire read lock: {}", e)))?; + let inner = self.inner.read().unwrap(); // if no CRLs are loaded, try to load them now if inner.crl_list.is_empty() { drop(inner); debug!("crl not loaded, attempting to load now"); self.load_crl()?; - return self.is_cert_revoked(cert); + // re-check after loading + let inner = self.inner.read().unwrap(); + if inner.crl_list.is_empty() { + debug!("no crls loaded, treating certificate as not revoked"); + return Ok(false); + } + drop(inner); + return self.check_cert_revocation(cert); } // extract certificate serial number using x509-parser @@ -228,99 +308,104 @@ impl CrlManager { .map_err(|e| CrlError::CertificateParseError(e.to_string()))?; let cert_serial = parsed_cert.serial.to_bytes_be(); - debug!("checking certificate serial: {:?}", cert_serial); + debug!(serial = ?cert_serial, "checking certificate"); + + // check the certificate against all CRLs, skipping any that have expired since load + for (idx, entry) in inner.crl_list.iter().enumerate() { + // re-validate time bounds at lookup time (CRL may have expired since load) + if let Ok((_, crl_info)) = + x509_parser::revocation_list::CertificateRevocationList::from_der(&entry.der) + && !Self::is_crl_time_valid(&crl_info, idx) + { + // skip expired CRL + continue; + } - // check the certificate against ALL CRLs using webpki's native API - for (idx, crl) in inner.crl_list.iter().enumerate() { - debug!("checking against CRL {}", idx + 1); + debug!(crl = idx + 1, "checking against crl"); - match crl.find_serial(&cert_serial) { + match entry.crl.find_serial(&cert_serial) { Ok(Some(revoked_cert)) => { - error!( - "certificate with serial {:?} is REVOKED in CRL {}", - cert_serial, - idx + 1 + // note: RemoveFromCrl reason only appears in delta CRLs which webpki + // rejects, so we don't need to handle it specially here + debug!( + serial = ?cert_serial, + crl = idx + 1, + revocation_date = ?revoked_cert.revocation_date, + reason = ?revoked_cert.reason_code, + "certificate is revoked" ); - error!("revocation date: {:?}", revoked_cert.revocation_date); - if let Some(reason) = revoked_cert.reason_code { - error!("revocation reason: {:?}", reason); - } return Ok(true); } Ok(None) => { // certificate isn't found in this CRL, continue checking others - debug!("certificate not found in CRL {}", idx + 1); + debug!(crl = idx + 1, "certificate not found in crl"); continue; } Err(e) => { // error during CRL lookup - error!("error checking CRL {}: {:?}", idx + 1, e); - return Err(CrlError::WebPkiError(format!("CRL lookup failed: {:?}", e))); + debug!(crl = idx + 1, error = ?e, "error checking crl"); + return Err(CrlError::WebPkiError(format!("crl lookup failed: {:?}", e))); } } } debug!( - "certificate serial {:?} is not revoked in any of the {} CRL(s)", - cert_serial, - inner.crl_list.len() + serial = ?cert_serial, + crl_count = inner.crl_list.len(), + "certificate is not revoked" ); Ok(false) } - /// Check if any certificate in the chain is revoked using webpki's native CRL API + /// checks if any certificate in the chain is revoked. + /// returns true if any cert is revoked, false otherwise (including on errors - fail-open) pub fn is_revoked_chain( &self, end_entity: &CertificateDer, intermediates: &[CertificateDer], - ) -> Result { + ) -> bool { debug!( - "checking certificate chain against CRL (chain length: {})", - 1 + intermediates.len() + chain_length = 1 + intermediates.len(), + "checking certificate chain against crl" ); // check leaf certificate debug!("checking leaf certificate"); - if self.is_cert_revoked(end_entity)? { - error!("leaf certificate is REVOKED"); - return Ok(true); + if self.is_cert_revoked(end_entity) { + debug!("leaf certificate is revoked"); + return true; } // check all intermediate certificates for (idx, intermediate) in intermediates.iter().enumerate() { - debug!("checking intermediate certificate {} in chain", idx); - if self.is_cert_revoked(intermediate)? { - error!("intermediate CA certificate at position {} is REVOKED", idx); - return Ok(true); + debug!(position = idx, "checking intermediate certificate"); + if self.is_cert_revoked(intermediate) { + debug!(position = idx, "intermediate certificate is revoked"); + return true; } } - debug!("certificate chain validation passed - no revoked certificates found"); - Ok(false) + false } - /// Start watching the CRL file for changes - /// Uses debouncer to handle all file update patterns + /// starts watching the CRL file for changes. + /// uses debouncer to handle all file update patterns pub fn start_file_watcher(self: &Arc) -> Result<(), CrlError> { let crl_path = { - let inner = self - .inner - .read() - .map_err(|e| CrlError::LockError(format!("failed to acquire read lock: {}", e)))?; + let inner = self.inner.read().unwrap(); inner.crl_path.clone() }; // watch the parent directory to catch ConfigMap updates via symlinks let watch_path = crl_path .parent() - .ok_or_else(|| CrlError::ParseError("CRL path has no parent directory".to_string()))?; + .ok_or_else(|| CrlError::ParseError("crl path has no parent directory".to_string()))?; debug!( - "starting CRL file watcher (debounced) for directory: {:?}", - watch_path + path = ?watch_path, + debounce_secs = 2, + "starting crl file watcher" ); - debug!(" debounce timeout: 2 seconds"); - debug!(" watching for: Kubernetes ConfigMaps, direct writes, text editor saves"); let manager = Arc::clone(self); @@ -333,33 +418,26 @@ impl CrlManager { match result { Ok(events) => { if !events.is_empty() { - // log all events for debugging - debug!("CRL directory events: {} event(s) detected", events.len()); - for event in events.iter() { - debug!( - " Event: kind={:?}, paths={:?}", - event.event.kind, event.event.paths - ); - } + debug!(event_count = events.len(), "crl directory events detected"); // reload CRL for any changes in the watched directory // this handles Kubernetes ConfigMap updates (..data symlink changes) // as well as direct file writes and text editor saves - debug!("CRL directory changed, reloading..."); + debug!("crl directory changed, reloading"); match manager.load_crl() { - Ok(has_new_revocations) => { - debug!("CRL reloaded successfully after file change"); - if has_new_revocations { - info!("New revocation detected"); + Ok(crl_count_changed) => { + debug!("crl reloaded successfully after file change"); + if crl_count_changed { + info!("crl content changed"); } } - Err(e) => error!("failed to reload CRL: {}", e), + Err(e) => debug!(error = %e, "failed to reload crl"), } } } Err(errors) => { for error in errors { - error!("CRL watcher error: {:?}", error); + debug!(error = ?error, "crl watcher error"); } } } @@ -373,16 +451,13 @@ impl CrlManager { .watch(watch_path, RecursiveMode::NonRecursive) .map_err(|e| CrlError::ParseError(format!("failed to watch directory: {}", e)))?; - // Store debouncer to keep it alive + // store debouncer to keep it alive { - let mut inner = self - .inner - .write() - .map_err(|e| CrlError::LockError(format!("failed to acquire write lock: {}", e)))?; + let mut inner = self.inner.write().unwrap(); inner._debouncer = Some(debouncer); } - debug!("CRL file watcher started successfully"); + debug!("crl file watcher started successfully"); Ok(()) } } @@ -409,4 +484,13 @@ mod tests { let result = CrlManager::new(file.path().to_path_buf()); assert!(result.is_err(), "should fail on invalid CRL data"); } + + #[test] + fn test_crl_manager_empty_file() { + let file = NamedTempFile::new().expect("failed to create temporary test file"); + // file is empty by default + + let result = CrlManager::new(file.path().to_path_buf()); + assert!(result.is_ok(), "should handle empty CRL file gracefully"); + } } diff --git a/src/tls/workload.rs b/src/tls/workload.rs index f7ffed98c7..6fd7ef9486 100644 --- a/src/tls/workload.rs +++ b/src/tls/workload.rs @@ -38,7 +38,7 @@ use crate::tls; use crate::tls::crl::CrlManager; use tokio::net::TcpStream; use tokio_rustls::client; -use tracing::{debug, error, trace}; +use tracing::{debug, trace}; #[derive(Clone, Debug)] pub struct InboundAcceptor { @@ -125,16 +125,8 @@ impl ClientCertVerifier for TrustDomainVerifier { // check CRL if enabled if let Some(crl_manager) = &self.crl_manager { - debug!("crl checking enabled for client certificate"); - let is_revoked = crl_manager - .is_revoked_chain(end_entity, intermediates) - .map_err(|e| { - error!("crl validation failed for client certificate: {}", e); - rustls::Error::General(format!("certificate revocation check failed: {}", e)) - })?; - - if is_revoked { - error!("client certificate is REVOKED - rejecting connection"); + if crl_manager.is_revoked_chain(end_entity, intermediates) { + debug!("client certificate is revoked, rejecting connection"); return Err(rustls::Error::InvalidCertificate( rustls::CertificateError::Revoked, )); From 02cc467b9aa0c678ea4053a72e1ef38215425a61 Mon Sep 17 00:00:00 2001 From: nilekh <1626598+nilekhc@users.noreply.github.com> Date: Wed, 7 Jan 2026 15:57:08 -0800 Subject: [PATCH 10/17] chore: implements CRL validation entirely with webpki's verify_for_usage method. Signed-off-by: nilekh <1626598+nilekhc@users.noreply.github.com> --- src/tls/certificate.rs | 8 +- src/tls/crl.rs | 203 +++++---------------------------- src/tls/workload.rs | 248 +++++++++++++++++++++++++++++++++++------ 3 files changed, 242 insertions(+), 217 deletions(-) diff --git a/src/tls/certificate.rs b/src/tls/certificate.rs index 89723ecbdc..2f1032eebe 100644 --- a/src/tls/certificate.rs +++ b/src/tls/certificate.rs @@ -22,7 +22,6 @@ use std::{cmp, iter}; use rustls::client::Resumption; use rustls::pki_types::{CertificateDer, PrivateKeyDer}; -use rustls::server::WebPkiClientVerifier; use rustls::{ClientConfig, RootCertStore, ServerConfig, server}; use rustls_pemfile::Item; use std::io::Cursor; @@ -305,16 +304,11 @@ impl WorkloadCertificate { let td = self.cert.identity().map(|i| match i { Identity::Spiffe { trust_domain, .. } => trust_domain, }); - let raw_client_cert_verifier = WebPkiClientVerifier::builder_with_provider( - self.root_store.clone(), - crate::tls::lib::provider(), - ) - .build()?; let client_cert_verifier = crate::tls::workload::TrustDomainVerifier::new( - raw_client_cert_verifier, td, crl_manager, + self.root_store.clone(), ); let mut sc = ServerConfig::builder_with_provider(crate::tls::lib::provider()) .with_protocol_versions(tls::TLS_VERSIONS) diff --git a/src/tls/crl.rs b/src/tls/crl.rs index 4969fb26b0..931c1642c8 100644 --- a/src/tls/crl.rs +++ b/src/tls/crl.rs @@ -17,21 +17,22 @@ use notify_debouncer_full::{ DebounceEventResult, Debouncer, FileIdMap, new_debouncer, notify::{RecursiveMode, Watcher}, }; -use rustls::pki_types::CertificateDer; use std::path::PathBuf; use std::sync::{Arc, RwLock}; use std::time::Duration; use tracing::{debug, info, warn}; -use webpki::CertRevocationList; +use webpki::OwnedCertRevocationList; -// CrlManager handles certificate revocation list (CRL) validation. +// CrlManager handles certificate revocation list (CRL) loading. // // validation notes: // - webpki's from_der() validates ASN.1 structure, CRL version (v2), and rejects // delta CRLs and indirect CRLs with unknown critical extensions -// - time bounds (thisUpdate/nextUpdate) are validated during load and at lookup time -// - signature verification is NOT performed during loading; this implementation -// relies on the CRL being from a trusted source (e.g., mounted ConfigMap) +// - CRL signature verification is performed during verify_for_usage() against +// the issuing certificate's public key in the chain +// - time bounds (thisUpdate/nextUpdate) are enforced via ExpirationPolicy::Enforce +// in verify_for_usage() +// - webpki validates that the CRL issuer has the cRLSign KeyUsage bit // - per RFC 5280 section 3.3, entries may be removed from CRLs after the // certificate's validity period expires @@ -43,9 +44,6 @@ pub enum CrlError { #[error("failed to parse CRL: {0}")] ParseError(String), - #[error("failed to parse certificate: {0}")] - CertificateParseError(String), - #[error("CRL error: {0}")] WebPkiError(String), } @@ -61,10 +59,9 @@ impl std::fmt::Debug for CrlManager { } } -// stores both the webpki CRL and the original DER for time validation at lookup +// stores the webpki CRL for use with verify_for_usage struct CrlEntry { - crl: CertRevocationList<'static>, - der: Vec, + crl: OwnedCertRevocationList, } struct CrlManagerInner { @@ -106,43 +103,6 @@ impl CrlManager { Ok(manager) } - /// validates CRL time bounds: thisUpdate <= now <= nextUpdate - /// returns true if valid, false if expired or not yet valid - fn is_crl_time_valid( - crl_info: &x509_parser::revocation_list::CertificateRevocationList, - crl_index: usize, - ) -> bool { - use x509_parser::time::ASN1Time; - - let now = ASN1Time::now(); - let tbs = &crl_info.tbs_cert_list; - - // check thisUpdate <= now (CRL must be issued already) - if tbs.this_update > now { - warn!( - crl = crl_index + 1, - this_update = %tbs.this_update, - "crl is not yet valid, skipping" - ); - return false; - } - - // check now <= nextUpdate (CRL must not be expired) - if let Some(next_update) = tbs.next_update - && now > next_update - { - warn!( - crl = crl_index + 1, - next_update = %next_update, - "crl is expired, skipping" - ); - return false; - } - // note: if nextUpdate is absent, we accept the CRL (per RFC 5280, nextUpdate is optional) - - true - } - pub fn load_crl(&self) -> Result { let mut inner = self.inner.write().unwrap(); @@ -182,7 +142,7 @@ impl CrlManager { let mut parsed_crls = Vec::new(); for (idx, der_data) in der_crls.iter().enumerate() { - // parse with x509-parser for validation and logging + // parse with x509-parser for logging metadata use x509_parser::prelude::*; let (_, crl_info) = CertificateRevocationList::from_der(der_data).map_err(|e| { CrlError::ParseError(format!("failed to parse crl {}: {}", idx + 1, e)) @@ -190,19 +150,12 @@ impl CrlManager { let tbs = &crl_info.tbs_cert_list; - // validate time bounds before accepting the CRL - if !Self::is_crl_time_valid(&crl_info, idx) { - // skip this CRL but continue processing others - continue; - } - - // parse with webpki for revocation checking + // parse with webpki for revocation checking via verify_for_usage + // note: time bounds and signature validation are handled by verify_for_usage let owned_crl = webpki::OwnedCertRevocationList::from_der(der_data).map_err(|e| { CrlError::WebPkiError(format!("failed to parse crl {}: {:?}", idx + 1, e)) })?; - let crl = CertRevocationList::from(owned_crl); - debug!( crl = idx + 1, issuer = %tbs.issuer, @@ -212,18 +165,11 @@ impl CrlManager { "loaded crl" ); - parsed_crls.push(CrlEntry { - crl, - der: der_data.clone(), - }); + parsed_crls.push(CrlEntry { crl: owned_crl }); } let crl_count_changed = parsed_crls.len() != inner.crl_list.len(); - if crl_count_changed { - debug!(count = parsed_crls.len(), "crl file changed, reloaded"); - } - // store parsed CRL objects inner.crl_list = parsed_crls; @@ -269,123 +215,24 @@ impl CrlManager { Ok(crls) } - /// checks if a certificate is revoked. - /// returns true if revoked, false otherwise (including on errors - fail-open policy) - pub fn is_cert_revoked(&self, cert: &CertificateDer) -> bool { - match self.check_cert_revocation(cert) { - Ok(is_revoked) => is_revoked, - Err(e) => { - debug!(error = %e, "crl check failed, allowing connection (fail-open)"); - false - } - } - } - - /// internal implementation that returns Result for error handling - fn check_cert_revocation(&self, cert: &CertificateDer) -> Result { - use x509_parser::prelude::*; - - let inner = self.inner.read().unwrap(); - - // if no CRLs are loaded, try to load them now - if inner.crl_list.is_empty() { - drop(inner); - debug!("crl not loaded, attempting to load now"); - self.load_crl()?; - // re-check after loading + /// returns the loaded CRLs for use with verify_for_usage. + /// if no CRLs are loaded, attempts to load them first. + pub fn get_crls(&self) -> Vec { + // try to load if not already loaded + { let inner = self.inner.read().unwrap(); if inner.crl_list.is_empty() { - debug!("no crls loaded, treating certificate as not revoked"); - return Ok(false); - } - drop(inner); - return self.check_cert_revocation(cert); - } - - // extract certificate serial number using x509-parser - // webpki doesn't expose cert parsing publicly, so we use x509-parser for this - let (_, parsed_cert) = X509Certificate::from_der(cert) - .map_err(|e| CrlError::CertificateParseError(e.to_string()))?; - - let cert_serial = parsed_cert.serial.to_bytes_be(); - debug!(serial = ?cert_serial, "checking certificate"); - - // check the certificate against all CRLs, skipping any that have expired since load - for (idx, entry) in inner.crl_list.iter().enumerate() { - // re-validate time bounds at lookup time (CRL may have expired since load) - if let Ok((_, crl_info)) = - x509_parser::revocation_list::CertificateRevocationList::from_der(&entry.der) - && !Self::is_crl_time_valid(&crl_info, idx) - { - // skip expired CRL - continue; - } - - debug!(crl = idx + 1, "checking against crl"); - - match entry.crl.find_serial(&cert_serial) { - Ok(Some(revoked_cert)) => { - // note: RemoveFromCrl reason only appears in delta CRLs which webpki - // rejects, so we don't need to handle it specially here - debug!( - serial = ?cert_serial, - crl = idx + 1, - revocation_date = ?revoked_cert.revocation_date, - reason = ?revoked_cert.reason_code, - "certificate is revoked" - ); - return Ok(true); - } - Ok(None) => { - // certificate isn't found in this CRL, continue checking others - debug!(crl = idx + 1, "certificate not found in crl"); - continue; - } - Err(e) => { - // error during CRL lookup - debug!(crl = idx + 1, error = ?e, "error checking crl"); - return Err(CrlError::WebPkiError(format!("crl lookup failed: {:?}", e))); + drop(inner); + debug!("crl not loaded, attempting to load now"); + if let Err(e) = self.load_crl() { + debug!(error = %e, "failed to load crl"); + return Vec::new(); } } } - debug!( - serial = ?cert_serial, - crl_count = inner.crl_list.len(), - "certificate is not revoked" - ); - Ok(false) - } - - /// checks if any certificate in the chain is revoked. - /// returns true if any cert is revoked, false otherwise (including on errors - fail-open) - pub fn is_revoked_chain( - &self, - end_entity: &CertificateDer, - intermediates: &[CertificateDer], - ) -> bool { - debug!( - chain_length = 1 + intermediates.len(), - "checking certificate chain against crl" - ); - - // check leaf certificate - debug!("checking leaf certificate"); - if self.is_cert_revoked(end_entity) { - debug!("leaf certificate is revoked"); - return true; - } - - // check all intermediate certificates - for (idx, intermediate) in intermediates.iter().enumerate() { - debug!(position = idx, "checking intermediate certificate"); - if self.is_cert_revoked(intermediate) { - debug!(position = idx, "intermediate certificate is revoked"); - return true; - } - } - - false + let inner = self.inner.read().unwrap(); + inner.crl_list.iter().map(|e| e.crl.clone()).collect() } /// starts watching the CRL file for changes. diff --git a/src/tls/workload.rs b/src/tls/workload.rs index 6fd7ef9486..9f1cd104c4 100644 --- a/src/tls/workload.rs +++ b/src/tls/workload.rs @@ -22,7 +22,6 @@ use futures_util::TryFutureExt; use rustls::client::danger::{HandshakeSignatureValid, ServerCertVerified, ServerCertVerifier}; use rustls::pki_types::{CertificateDer, ServerName, UnixTime}; -use rustls::server::ParsedCertificate; use rustls::server::danger::{ClientCertVerified, ClientCertVerifier}; use rustls::{ ClientConfig, DigitallySignedStruct, DistinguishedName, RootCertStore, SignatureScheme, @@ -32,6 +31,7 @@ use std::io; use std::pin::Pin; use std::sync::Arc; use tokio::io::{AsyncRead, AsyncWrite}; +use webpki::{ExpirationPolicy, KeyUsage, OwnedCertRevocationList, RevocationOptionsBuilder}; use crate::strng::Strng; use crate::tls; @@ -53,21 +53,21 @@ impl InboundAcceptor { #[derive(Debug)] pub(super) struct TrustDomainVerifier { - base: Arc, trust_domain: Option, crl_manager: Option>, + root_store: Arc, } impl TrustDomainVerifier { pub fn new( - base: Arc, trust_domain: Option, crl_manager: Option>, + root_store: Arc, ) -> Arc { Arc::new(Self { - base, trust_domain, crl_manager, + root_store, }) } @@ -103,13 +103,131 @@ impl TrustDomainVerifier { }) .map(|_| ()) } + + /// verifies the certificate chain using webpki's verify_for_usage. + fn verify_cert_chain( + &self, + end_entity: &CertificateDer<'_>, + intermediates: &[CertificateDer<'_>], + crls: Option>, + now: UnixTime, + ) -> Result<(), rustls::Error> { + use rustls::pki_types::TrustAnchor; + use webpki::EndEntityCert; + + let cert = EndEntityCert::try_from(end_entity).map_err(|e| { + debug!(error = ?e, "failed to parse end entity certificate"); + rustls::Error::InvalidCertificate(rustls::CertificateError::BadEncoding) + })?; + + // convert root store to trust anchors + let trust_anchors: Vec> = self + .root_store + .roots + .iter() + .map(|ta| TrustAnchor { + subject: ta.subject.clone(), + subject_public_key_info: ta.subject_public_key_info.clone(), + name_constraints: ta.name_constraints.clone(), + }) + .collect(); + + if trust_anchors.is_empty() { + debug!("no trust anchors available for certificate verification"); + return Err(rustls::Error::InvalidCertificate( + rustls::CertificateError::UnknownIssuer, + )); + } + + let sig_algs = provider().signature_verification_algorithms.all; + + // convert CRLs if provided - keep in scope for lifetime + let crl_refs: Vec> = crls + .map(|c| { + c.into_iter() + .map(webpki::CertRevocationList::from) + .collect() + }) + .unwrap_or_default(); + let crl_ref_refs: Vec<&webpki::CertRevocationList<'_>> = crl_refs.iter().collect(); + + // build revocation options if CRLs are available + let revocation = if crl_ref_refs.is_empty() { + None + } else { + Some( + RevocationOptionsBuilder::new(&crl_ref_refs) + .map_err(|_| { + debug!("failed to build revocation options"); + rustls::Error::InvalidCertificate(rustls::CertificateError::BadEncoding) + })? + .with_expiration_policy(ExpirationPolicy::Enforce) + .build(), + ) + }; + + // verify_for_usage performs: + // - certificate chain validation + // - signature verification for certs (and CRLs if provided) + // - time bounds validation for certs (and CRLs if provided) + // - revocation status checking (if CRLs provided) + // - KeyUsage validation (cRLSign for CRL issuers if CRLs provided) + cert.verify_for_usage( + sig_algs, + &trust_anchors, + intermediates, + now, + KeyUsage::client_auth(), + revocation, + None, + ) + .map_err(|e| { + debug!(error = ?e, "certificate verification failed"); + match e { + webpki::Error::CertRevoked => { + rustls::Error::InvalidCertificate(rustls::CertificateError::Revoked) + } + webpki::Error::UnknownRevocationStatus => { + debug!("no authoritative crl found for certificate - issuer dn mismatch"); + rustls::Error::InvalidCertificate( + rustls::CertificateError::ApplicationVerificationFailure, + ) + } + webpki::Error::CrlExpired { .. } => { + rustls::Error::InvalidCertificate(rustls::CertificateError::Expired) + } + webpki::Error::InvalidCrlSignatureForPublicKey => { + rustls::Error::InvalidCertificate(rustls::CertificateError::BadSignature) + } + webpki::Error::UnknownIssuer => { + rustls::Error::InvalidCertificate(rustls::CertificateError::UnknownIssuer) + } + webpki::Error::CertExpired { .. } => { + rustls::Error::InvalidCertificate(rustls::CertificateError::Expired) + } + webpki::Error::CertNotValidYet { .. } => { + rustls::Error::InvalidCertificate(rustls::CertificateError::NotValidYet) + } + webpki::Error::InvalidSignatureForPublicKey => { + rustls::Error::InvalidCertificate(rustls::CertificateError::BadSignature) + } + _ => rustls::Error::InvalidCertificate( + rustls::CertificateError::ApplicationVerificationFailure, + ), + } + })?; + + Ok(()) + } } // Implement our custom ClientCertVerifier logic. We only want to add an extra check, but // need a decent amount of boilerplate to do so. impl ClientCertVerifier for TrustDomainVerifier { fn root_hint_subjects(&self) -> &[DistinguishedName] { - self.base.root_hint_subjects() + // return distinguished names from root store for client cert hints + static EMPTY: &[DistinguishedName] = &[]; + EMPTY } fn verify_client_cert( @@ -118,26 +236,45 @@ impl ClientCertVerifier for TrustDomainVerifier { intermediates: &[CertificateDer<'_>], now: UnixTime, ) -> Result { - let res = self - .base - .verify_client_cert(end_entity, intermediates, now)?; - self.verify_trust_domain(end_entity)?; - - // check CRL if enabled - if let Some(crl_manager) = &self.crl_manager { - if crl_manager.is_revoked_chain(end_entity, intermediates) { - debug!("client certificate is revoked, rejecting connection"); - return Err(rustls::Error::InvalidCertificate( - rustls::CertificateError::Revoked, - )); + // get CRLs if CRL manager is enabled + let crls = self.crl_manager.as_ref().map(|m| m.get_crls()); + + // use verify_for_usage for all certificate verification + // this validates certificate chain, signatures, time bounds, and CRL (if provided) + match self.verify_cert_chain(end_entity, intermediates, crls, now) { + Ok(()) => { + if self.crl_manager.is_some() { + debug!("client certificate chain is valid via verify_for_usage"); + } else { + debug!("client certificate chain is valid (crl checking disabled)"); + } + } + Err(e) => { + // fail-open for CRL-related errors only, fail-closed for cert errors + match &e { + rustls::Error::InvalidCertificate(rustls::CertificateError::Revoked) => { + debug!("client certificate is revoked, rejecting connection"); + return Err(e); + } + // fail-open for CRL expiration - CRL might be stale but cert could be valid + rustls::Error::InvalidCertificate(rustls::CertificateError::Expired) + if self.crl_manager.is_some() => + { + debug!(error = ?e, "crl expired, allowing connection (fail-open)"); + } + // all other errors should be propagated (unknown issuer, bad signature, etc.) + _ => { + debug!(error = ?e, "certificate verification failed"); + return Err(e); + } + } } - - debug!("client certificate chain is valid (not revoked)"); - } else { - debug!("crl checking disabled for client certificate"); } - Ok(res) + // trust domain verification + self.verify_trust_domain(end_entity)?; + + Ok(ClientCertVerified::assertion()) } fn verify_tls12_signature( @@ -146,7 +283,12 @@ impl ClientCertVerifier for TrustDomainVerifier { cert: &CertificateDer<'_>, dss: &DigitallySignedStruct, ) -> Result { - self.base.verify_tls12_signature(message, cert, dss) + rustls::crypto::verify_tls12_signature( + message, + cert, + dss, + &provider().signature_verification_algorithms, + ) } fn verify_tls13_signature( @@ -155,11 +297,18 @@ impl ClientCertVerifier for TrustDomainVerifier { cert: &CertificateDer<'_>, dss: &DigitallySignedStruct, ) -> Result { - self.base.verify_tls13_signature(message, cert, dss) + rustls::crypto::verify_tls13_signature( + message, + cert, + dss, + &provider().signature_verification_algorithms, + ) } fn supported_verify_schemes(&self) -> Vec { - self.base.supported_verify_schemes() + provider() + .signature_verification_algorithms + .supported_schemes() } } @@ -273,16 +422,51 @@ impl ServerCertVerifier for IdentityVerifier { ocsp_response: &[u8], now: UnixTime, ) -> Result { - let cert = ParsedCertificate::try_from(end_entity)?; + use rustls::pki_types::TrustAnchor; + use webpki::EndEntityCert; + + let cert = EndEntityCert::try_from(end_entity).map_err(|e| { + debug!(error = ?e, "failed to parse end entity certificate"); + rustls::Error::InvalidCertificate(rustls::CertificateError::BadEncoding) + })?; + + // convert root store to trust anchors + let trust_anchors: Vec> = self + .roots + .roots + .iter() + .map(|ta| TrustAnchor { + subject: ta.subject.clone(), + subject_public_key_info: ta.subject_public_key_info.clone(), + name_constraints: ta.name_constraints.clone(), + }) + .collect(); - let algs = provider().signature_verification_algorithms; - rustls::client::verify_server_cert_signed_by_trust_anchor( - &cert, - &self.roots, + if trust_anchors.is_empty() { + debug!("no trust anchors available for server cert verification"); + return Err(rustls::Error::InvalidCertificate( + rustls::CertificateError::UnknownIssuer, + )); + } + + let sig_algs = provider().signature_verification_algorithms.all; + + // verify_for_usage performs certificate chain validation and signature verification + cert.verify_for_usage( + sig_algs, + &trust_anchors, intermediates, now, - algs.all, - )?; + KeyUsage::server_auth(), + None, + None, + ) + .map_err(|e| { + debug!(error = ?e, "server certificate verification failed"); + rustls::Error::InvalidCertificate( + rustls::CertificateError::ApplicationVerificationFailure, + ) + })?; if !ocsp_response.is_empty() { trace!("Unvalidated OCSP response: {ocsp_response:?}"); From cedda4f999c2f61be4521345ba5296cc977c2924 Mon Sep 17 00:00:00 2001 From: nilekh <1626598+nilekhc@users.noreply.github.com> Date: Thu, 8 Jan 2026 10:15:38 -0800 Subject: [PATCH 11/17] chore: reverts verify_for_usage Signed-off-by: nilekh <1626598+nilekhc@users.noreply.github.com> --- src/tls/certificate.rs | 20 +++- src/tls/crl.rs | 76 +++++-------- src/tls/workload.rs | 247 ++++------------------------------------- 3 files changed, 65 insertions(+), 278 deletions(-) diff --git a/src/tls/certificate.rs b/src/tls/certificate.rs index 2f1032eebe..31262ae67b 100644 --- a/src/tls/certificate.rs +++ b/src/tls/certificate.rs @@ -22,6 +22,7 @@ use std::{cmp, iter}; use rustls::client::Resumption; use rustls::pki_types::{CertificateDer, PrivateKeyDer}; +use rustls::server::WebPkiClientVerifier; use rustls::{ClientConfig, RootCertStore, ServerConfig, server}; use rustls_pemfile::Item; use std::io::Cursor; @@ -305,11 +306,24 @@ impl WorkloadCertificate { Identity::Spiffe { trust_domain, .. } => trust_domain, }); - let client_cert_verifier = crate::tls::workload::TrustDomainVerifier::new( - td, - crl_manager, + // build the base client cert verifier with optional CRL support + let mut builder = WebPkiClientVerifier::builder_with_provider( self.root_store.clone(), + crate::tls::lib::provider(), ); + + // add CRLs if available + if let Some(ref mgr) = crl_manager { + let crls = mgr.get_crl_ders(); + if !crls.is_empty() { + builder = builder.with_crls(crls).allow_unknown_revocation_status(); // fail-open for unknown status + } + } + + let raw_client_cert_verifier = builder.build()?; + + let client_cert_verifier = + crate::tls::workload::TrustDomainVerifier::new(raw_client_cert_verifier, td); let mut sc = ServerConfig::builder_with_provider(crate::tls::lib::provider()) .with_protocol_versions(tls::TLS_VERSIONS) .expect("server config must be valid") diff --git a/src/tls/crl.rs b/src/tls/crl.rs index 931c1642c8..184c2cc52d 100644 --- a/src/tls/crl.rs +++ b/src/tls/crl.rs @@ -17,22 +17,20 @@ use notify_debouncer_full::{ DebounceEventResult, Debouncer, FileIdMap, new_debouncer, notify::{RecursiveMode, Watcher}, }; +use rustls::pki_types::CertificateRevocationListDer; use std::path::PathBuf; use std::sync::{Arc, RwLock}; use std::time::Duration; use tracing::{debug, info, warn}; -use webpki::OwnedCertRevocationList; // CrlManager handles certificate revocation list (CRL) loading. // // validation notes: // - webpki's from_der() validates ASN.1 structure, CRL version (v2), and rejects // delta CRLs and indirect CRLs with unknown critical extensions -// - CRL signature verification is performed during verify_for_usage() against -// the issuing certificate's public key in the chain -// - time bounds (thisUpdate/nextUpdate) are enforced via ExpirationPolicy::Enforce -// in verify_for_usage() -// - webpki validates that the CRL issuer has the cRLSign KeyUsage bit +// - CRL signature verification is performed by rustls's WebPkiClientVerifier +// - time bounds (thisUpdate/nextUpdate) handling depends on whether +// enforce_revocation_expiration() is called on the verifier builder // - per RFC 5280 section 3.3, entries may be removed from CRLs after the // certificate's validity period expires @@ -59,13 +57,8 @@ impl std::fmt::Debug for CrlManager { } } -// stores the webpki CRL for use with verify_for_usage -struct CrlEntry { - crl: OwnedCertRevocationList, -} - struct CrlManagerInner { - crl_list: Vec, + crl_ders: Vec>, crl_path: PathBuf, _debouncer: Option>, } @@ -77,7 +70,7 @@ impl CrlManager { let manager = Self { inner: Arc::new(RwLock::new(CrlManagerInner { - crl_list: Vec::new(), + crl_ders: Vec::new(), crl_path: crl_path.clone(), _debouncer: None, })), @@ -112,8 +105,8 @@ impl CrlManager { // empty file means no revocations - this is valid if data.is_empty() { debug!(path = ?inner.crl_path, "crl file is empty, treating as no revocations"); - let crl_count_changed = !inner.crl_list.is_empty(); - inner.crl_list.clear(); + let crl_count_changed = !inner.crl_ders.is_empty(); + inner.crl_ders.clear(); return Ok(crl_count_changed); } @@ -132,48 +125,33 @@ impl CrlManager { // empty PEM file (no CRL blocks) means no revocations if der_crls.is_empty() { debug!("no crl blocks found, treating as no revocations"); - let crl_count_changed = !inner.crl_list.is_empty(); - inner.crl_list.clear(); + let crl_count_changed = !inner.crl_ders.is_empty(); + inner.crl_ders.clear(); return Ok(crl_count_changed); } debug!(count = der_crls.len(), "found crl block(s) in file"); - let mut parsed_crls = Vec::new(); - - for (idx, der_data) in der_crls.iter().enumerate() { - // parse with x509-parser for logging metadata - use x509_parser::prelude::*; - let (_, crl_info) = CertificateRevocationList::from_der(der_data).map_err(|e| { - CrlError::ParseError(format!("failed to parse crl {}: {}", idx + 1, e)) - })?; - - let tbs = &crl_info.tbs_cert_list; + let mut validated_ders = Vec::new(); - // parse with webpki for revocation checking via verify_for_usage - // note: time bounds and signature validation are handled by verify_for_usage - let owned_crl = webpki::OwnedCertRevocationList::from_der(der_data).map_err(|e| { + for (idx, der_data) in der_crls.into_iter().enumerate() { + // validate with webpki to catch parse errors early + // rustls will use the raw DER bytes directly + webpki::OwnedCertRevocationList::from_der(&der_data).map_err(|e| { CrlError::WebPkiError(format!("failed to parse crl {}: {:?}", idx + 1, e)) })?; - debug!( - crl = idx + 1, - issuer = %tbs.issuer, - this_update = %tbs.this_update, - next_update = ?tbs.next_update, - revoked = tbs.revoked_certificates.len(), - "loaded crl" - ); + debug!(crl = idx + 1, "loaded crl"); - parsed_crls.push(CrlEntry { crl: owned_crl }); + validated_ders.push(der_data); } - let crl_count_changed = parsed_crls.len() != inner.crl_list.len(); + let crl_count_changed = validated_ders.len() != inner.crl_ders.len(); - // store parsed CRL objects - inner.crl_list = parsed_crls; + // store validated DER bytes + inner.crl_ders = validated_ders; - debug!(count = inner.crl_list.len(), "crl loaded successfully"); + debug!(count = inner.crl_ders.len(), "crl loaded successfully"); Ok(crl_count_changed) } @@ -215,13 +193,13 @@ impl CrlManager { Ok(crls) } - /// returns the loaded CRLs for use with verify_for_usage. + /// returns CRLs as DER bytes for rustls's with_crls(). /// if no CRLs are loaded, attempts to load them first. - pub fn get_crls(&self) -> Vec { + pub fn get_crl_ders(&self) -> Vec> { // try to load if not already loaded { let inner = self.inner.read().unwrap(); - if inner.crl_list.is_empty() { + if inner.crl_ders.is_empty() { drop(inner); debug!("crl not loaded, attempting to load now"); if let Err(e) = self.load_crl() { @@ -232,7 +210,11 @@ impl CrlManager { } let inner = self.inner.read().unwrap(); - inner.crl_list.iter().map(|e| e.crl.clone()).collect() + inner + .crl_ders + .iter() + .map(|der| CertificateRevocationListDer::from(der.clone())) + .collect() } /// starts watching the CRL file for changes. diff --git a/src/tls/workload.rs b/src/tls/workload.rs index 9f1cd104c4..92e05d40c8 100644 --- a/src/tls/workload.rs +++ b/src/tls/workload.rs @@ -22,6 +22,7 @@ use futures_util::TryFutureExt; use rustls::client::danger::{HandshakeSignatureValid, ServerCertVerified, ServerCertVerifier}; use rustls::pki_types::{CertificateDer, ServerName, UnixTime}; +use rustls::server::ParsedCertificate; use rustls::server::danger::{ClientCertVerified, ClientCertVerifier}; use rustls::{ ClientConfig, DigitallySignedStruct, DistinguishedName, RootCertStore, SignatureScheme, @@ -31,11 +32,9 @@ use std::io; use std::pin::Pin; use std::sync::Arc; use tokio::io::{AsyncRead, AsyncWrite}; -use webpki::{ExpirationPolicy, KeyUsage, OwnedCertRevocationList, RevocationOptionsBuilder}; use crate::strng::Strng; use crate::tls; -use crate::tls::crl::CrlManager; use tokio::net::TcpStream; use tokio_rustls::client; use tracing::{debug, trace}; @@ -53,22 +52,13 @@ impl InboundAcceptor { #[derive(Debug)] pub(super) struct TrustDomainVerifier { + base: Arc, trust_domain: Option, - crl_manager: Option>, - root_store: Arc, } impl TrustDomainVerifier { - pub fn new( - trust_domain: Option, - crl_manager: Option>, - root_store: Arc, - ) -> Arc { - Arc::new(Self { - trust_domain, - crl_manager, - root_store, - }) + pub fn new(base: Arc, trust_domain: Option) -> Arc { + Arc::new(Self { base, trust_domain }) } fn verify_trust_domain(&self, client_cert: &CertificateDer<'_>) -> Result<(), rustls::Error> { @@ -103,131 +93,13 @@ impl TrustDomainVerifier { }) .map(|_| ()) } - - /// verifies the certificate chain using webpki's verify_for_usage. - fn verify_cert_chain( - &self, - end_entity: &CertificateDer<'_>, - intermediates: &[CertificateDer<'_>], - crls: Option>, - now: UnixTime, - ) -> Result<(), rustls::Error> { - use rustls::pki_types::TrustAnchor; - use webpki::EndEntityCert; - - let cert = EndEntityCert::try_from(end_entity).map_err(|e| { - debug!(error = ?e, "failed to parse end entity certificate"); - rustls::Error::InvalidCertificate(rustls::CertificateError::BadEncoding) - })?; - - // convert root store to trust anchors - let trust_anchors: Vec> = self - .root_store - .roots - .iter() - .map(|ta| TrustAnchor { - subject: ta.subject.clone(), - subject_public_key_info: ta.subject_public_key_info.clone(), - name_constraints: ta.name_constraints.clone(), - }) - .collect(); - - if trust_anchors.is_empty() { - debug!("no trust anchors available for certificate verification"); - return Err(rustls::Error::InvalidCertificate( - rustls::CertificateError::UnknownIssuer, - )); - } - - let sig_algs = provider().signature_verification_algorithms.all; - - // convert CRLs if provided - keep in scope for lifetime - let crl_refs: Vec> = crls - .map(|c| { - c.into_iter() - .map(webpki::CertRevocationList::from) - .collect() - }) - .unwrap_or_default(); - let crl_ref_refs: Vec<&webpki::CertRevocationList<'_>> = crl_refs.iter().collect(); - - // build revocation options if CRLs are available - let revocation = if crl_ref_refs.is_empty() { - None - } else { - Some( - RevocationOptionsBuilder::new(&crl_ref_refs) - .map_err(|_| { - debug!("failed to build revocation options"); - rustls::Error::InvalidCertificate(rustls::CertificateError::BadEncoding) - })? - .with_expiration_policy(ExpirationPolicy::Enforce) - .build(), - ) - }; - - // verify_for_usage performs: - // - certificate chain validation - // - signature verification for certs (and CRLs if provided) - // - time bounds validation for certs (and CRLs if provided) - // - revocation status checking (if CRLs provided) - // - KeyUsage validation (cRLSign for CRL issuers if CRLs provided) - cert.verify_for_usage( - sig_algs, - &trust_anchors, - intermediates, - now, - KeyUsage::client_auth(), - revocation, - None, - ) - .map_err(|e| { - debug!(error = ?e, "certificate verification failed"); - match e { - webpki::Error::CertRevoked => { - rustls::Error::InvalidCertificate(rustls::CertificateError::Revoked) - } - webpki::Error::UnknownRevocationStatus => { - debug!("no authoritative crl found for certificate - issuer dn mismatch"); - rustls::Error::InvalidCertificate( - rustls::CertificateError::ApplicationVerificationFailure, - ) - } - webpki::Error::CrlExpired { .. } => { - rustls::Error::InvalidCertificate(rustls::CertificateError::Expired) - } - webpki::Error::InvalidCrlSignatureForPublicKey => { - rustls::Error::InvalidCertificate(rustls::CertificateError::BadSignature) - } - webpki::Error::UnknownIssuer => { - rustls::Error::InvalidCertificate(rustls::CertificateError::UnknownIssuer) - } - webpki::Error::CertExpired { .. } => { - rustls::Error::InvalidCertificate(rustls::CertificateError::Expired) - } - webpki::Error::CertNotValidYet { .. } => { - rustls::Error::InvalidCertificate(rustls::CertificateError::NotValidYet) - } - webpki::Error::InvalidSignatureForPublicKey => { - rustls::Error::InvalidCertificate(rustls::CertificateError::BadSignature) - } - _ => rustls::Error::InvalidCertificate( - rustls::CertificateError::ApplicationVerificationFailure, - ), - } - })?; - - Ok(()) - } } // Implement our custom ClientCertVerifier logic. We only want to add an extra check, but // need a decent amount of boilerplate to do so. impl ClientCertVerifier for TrustDomainVerifier { fn root_hint_subjects(&self) -> &[DistinguishedName] { - // return distinguished names from root store for client cert hints - static EMPTY: &[DistinguishedName] = &[]; - EMPTY + self.base.root_hint_subjects() } fn verify_client_cert( @@ -236,45 +108,11 @@ impl ClientCertVerifier for TrustDomainVerifier { intermediates: &[CertificateDer<'_>], now: UnixTime, ) -> Result { - // get CRLs if CRL manager is enabled - let crls = self.crl_manager.as_ref().map(|m| m.get_crls()); - - // use verify_for_usage for all certificate verification - // this validates certificate chain, signatures, time bounds, and CRL (if provided) - match self.verify_cert_chain(end_entity, intermediates, crls, now) { - Ok(()) => { - if self.crl_manager.is_some() { - debug!("client certificate chain is valid via verify_for_usage"); - } else { - debug!("client certificate chain is valid (crl checking disabled)"); - } - } - Err(e) => { - // fail-open for CRL-related errors only, fail-closed for cert errors - match &e { - rustls::Error::InvalidCertificate(rustls::CertificateError::Revoked) => { - debug!("client certificate is revoked, rejecting connection"); - return Err(e); - } - // fail-open for CRL expiration - CRL might be stale but cert could be valid - rustls::Error::InvalidCertificate(rustls::CertificateError::Expired) - if self.crl_manager.is_some() => - { - debug!(error = ?e, "crl expired, allowing connection (fail-open)"); - } - // all other errors should be propagated (unknown issuer, bad signature, etc.) - _ => { - debug!(error = ?e, "certificate verification failed"); - return Err(e); - } - } - } - } - - // trust domain verification + let res = self + .base + .verify_client_cert(end_entity, intermediates, now)?; self.verify_trust_domain(end_entity)?; - - Ok(ClientCertVerified::assertion()) + Ok(res) } fn verify_tls12_signature( @@ -283,12 +121,7 @@ impl ClientCertVerifier for TrustDomainVerifier { cert: &CertificateDer<'_>, dss: &DigitallySignedStruct, ) -> Result { - rustls::crypto::verify_tls12_signature( - message, - cert, - dss, - &provider().signature_verification_algorithms, - ) + self.base.verify_tls12_signature(message, cert, dss) } fn verify_tls13_signature( @@ -297,18 +130,11 @@ impl ClientCertVerifier for TrustDomainVerifier { cert: &CertificateDer<'_>, dss: &DigitallySignedStruct, ) -> Result { - rustls::crypto::verify_tls13_signature( - message, - cert, - dss, - &provider().signature_verification_algorithms, - ) + self.base.verify_tls13_signature(message, cert, dss) } fn supported_verify_schemes(&self) -> Vec { - provider() - .signature_verification_algorithms - .supported_schemes() + self.base.supported_verify_schemes() } } @@ -422,51 +248,16 @@ impl ServerCertVerifier for IdentityVerifier { ocsp_response: &[u8], now: UnixTime, ) -> Result { - use rustls::pki_types::TrustAnchor; - use webpki::EndEntityCert; - - let cert = EndEntityCert::try_from(end_entity).map_err(|e| { - debug!(error = ?e, "failed to parse end entity certificate"); - rustls::Error::InvalidCertificate(rustls::CertificateError::BadEncoding) - })?; - - // convert root store to trust anchors - let trust_anchors: Vec> = self - .roots - .roots - .iter() - .map(|ta| TrustAnchor { - subject: ta.subject.clone(), - subject_public_key_info: ta.subject_public_key_info.clone(), - name_constraints: ta.name_constraints.clone(), - }) - .collect(); - - if trust_anchors.is_empty() { - debug!("no trust anchors available for server cert verification"); - return Err(rustls::Error::InvalidCertificate( - rustls::CertificateError::UnknownIssuer, - )); - } - - let sig_algs = provider().signature_verification_algorithms.all; + let cert = ParsedCertificate::try_from(end_entity)?; - // verify_for_usage performs certificate chain validation and signature verification - cert.verify_for_usage( - sig_algs, - &trust_anchors, + let algs = provider().signature_verification_algorithms; + rustls::client::verify_server_cert_signed_by_trust_anchor( + &cert, + &self.roots, intermediates, now, - KeyUsage::server_auth(), - None, - None, - ) - .map_err(|e| { - debug!(error = ?e, "server certificate verification failed"); - rustls::Error::InvalidCertificate( - rustls::CertificateError::ApplicationVerificationFailure, - ) - })?; + algs.all, + )?; if !ocsp_response.is_empty() { trace!("Unvalidated OCSP response: {ocsp_response:?}"); From 71aa835f6fd97438314e4dfbf1ba1e5ce4130a06 Mon Sep 17 00:00:00 2001 From: nilekh <1626598+nilekhc@users.noreply.github.com> Date: Thu, 8 Jan 2026 12:01:55 -0800 Subject: [PATCH 12/17] chore: removes comment Signed-off-by: nilekh <1626598+nilekhc@users.noreply.github.com> --- src/tls/crl.rs | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/src/tls/crl.rs b/src/tls/crl.rs index 184c2cc52d..ffc1ab2090 100644 --- a/src/tls/crl.rs +++ b/src/tls/crl.rs @@ -23,17 +23,6 @@ use std::sync::{Arc, RwLock}; use std::time::Duration; use tracing::{debug, info, warn}; -// CrlManager handles certificate revocation list (CRL) loading. -// -// validation notes: -// - webpki's from_der() validates ASN.1 structure, CRL version (v2), and rejects -// delta CRLs and indirect CRLs with unknown critical extensions -// - CRL signature verification is performed by rustls's WebPkiClientVerifier -// - time bounds (thisUpdate/nextUpdate) handling depends on whether -// enforce_revocation_expiration() is called on the verifier builder -// - per RFC 5280 section 3.3, entries may be removed from CRLs after the -// certificate's validity period expires - #[derive(Debug, thiserror::Error)] pub enum CrlError { #[error("failed to read CRL file: {0}")] From 0a3fa209984b5de52a56d3e35ce21997b5cee754 Mon Sep 17 00:00:00 2001 From: nilekh <1626598+nilekhc@users.noreply.github.com> Date: Fri, 9 Jan 2026 12:06:54 -0800 Subject: [PATCH 13/17] chore: address review comments Signed-off-by: nilekh <1626598+nilekhc@users.noreply.github.com> --- src/tls/crl.rs | 80 +++++++++++++++++++++++++------------------------- 1 file changed, 40 insertions(+), 40 deletions(-) diff --git a/src/tls/crl.rs b/src/tls/crl.rs index ffc1ab2090..9238c66620 100644 --- a/src/tls/crl.rs +++ b/src/tls/crl.rs @@ -18,6 +18,9 @@ use notify_debouncer_full::{ notify::{RecursiveMode, Watcher}, }; use rustls::pki_types::CertificateRevocationListDer; +use rustls_pemfile::Item; +use std::hash::{Hash, Hasher}; +use std::io::Cursor; use std::path::PathBuf; use std::sync::{Arc, RwLock}; use std::time::Duration; @@ -36,6 +39,10 @@ pub enum CrlError { } #[derive(Clone)] +/// NOTE: CRL updates take effect when new ServerConfigs are created, which happens +/// on certificate refresh (~12hrs). For immediate CRL enforcement, a custom +/// ClientCertVerifier wrapper would be needed, but rustls doesn't provide a +/// built-in mechanism like `with_cert_resolver` for dynamic CRL updates. pub struct CrlManager { inner: Arc>, } @@ -49,6 +56,7 @@ impl std::fmt::Debug for CrlManager { struct CrlManagerInner { crl_ders: Vec>, crl_path: PathBuf, + loaded: bool, _debouncer: Option>, } @@ -61,6 +69,7 @@ impl CrlManager { inner: Arc::new(RwLock::new(CrlManagerInner { crl_ders: Vec::new(), crl_path: crl_path.clone(), + loaded: false, _debouncer: None, })), }; @@ -94,9 +103,10 @@ impl CrlManager { // empty file means no revocations - this is valid if data.is_empty() { debug!(path = ?inner.crl_path, "crl file is empty, treating as no revocations"); - let crl_count_changed = !inner.crl_ders.is_empty(); + let crl_changed = !inner.crl_ders.is_empty(); inner.crl_ders.clear(); - return Ok(crl_count_changed); + inner.loaded = true; + return Ok(crl_changed); } debug!(bytes = data.len(), "read crl file"); @@ -114,9 +124,10 @@ impl CrlManager { // empty PEM file (no CRL blocks) means no revocations if der_crls.is_empty() { debug!("no crl blocks found, treating as no revocations"); - let crl_count_changed = !inner.crl_ders.is_empty(); + let crl_changed = !inner.crl_ders.is_empty(); inner.crl_ders.clear(); - return Ok(crl_count_changed); + inner.loaded = true; + return Ok(crl_changed); } debug!(count = der_crls.len(), "found crl block(s) in file"); @@ -135,51 +146,40 @@ impl CrlManager { validated_ders.push(der_data); } - let crl_count_changed = validated_ders.len() != inner.crl_ders.len(); + // use hash comparison to detect actual content changes, not just count changes + let crl_changed = + Self::compute_crl_hash(&validated_ders) != Self::compute_crl_hash(&inner.crl_ders); // store validated DER bytes inner.crl_ders = validated_ders; + inner.loaded = true; debug!(count = inner.crl_ders.len(), "crl loaded successfully"); - Ok(crl_count_changed) + Ok(crl_changed) + } + + /// computes a hash of CRL DER bytes for change detection + fn compute_crl_hash(ders: &[Vec]) -> u64 { + let mut hasher = std::collections::hash_map::DefaultHasher::new(); + ders.hash(&mut hasher); + hasher.finish() } /// parses PEM-encoded CRL data that may contain multiple CRL blocks /// returns a Vec of DER-encoded CRLs (empty vec if no blocks found) fn parse_pem_crls(pem_data: &[u8]) -> Result>, CrlError> { - let data_str = std::str::from_utf8(pem_data) - .map_err(|e| CrlError::ParseError(format!("invalid UTF-8: {}", e)))?; - - let mut crls = Vec::new(); - let mut in_pem = false; - let mut base64_data = String::new(); - - for line in data_str.lines() { - if line.starts_with("-----BEGIN") { - in_pem = true; - base64_data.clear(); // start new CRL block - continue; - } - if line.starts_with("-----END") { - if in_pem && !base64_data.is_empty() { - use base64::Engine; - let der = base64::engine::general_purpose::STANDARD - .decode(&base64_data) - .map_err(|e| { - CrlError::ParseError(format!("failed to decode base64: {}", e)) - })?; - crls.push(der); - base64_data.clear(); - } - in_pem = false; - continue; - } - if in_pem { - base64_data.push_str(line.trim()); - } - } - - Ok(crls) + let mut reader = std::io::BufReader::new(Cursor::new(pem_data)); + + rustls_pemfile::read_all(&mut reader) + .filter_map(|result| match result { + Ok(Item::Crl(crl)) => Some(Ok(crl.to_vec())), + Ok(_) => None, // skip non-CRL items + Err(e) => Some(Err(CrlError::ParseError(format!( + "failed to parse PEM: {}", + e + )))), + }) + .collect() } /// returns CRLs as DER bytes for rustls's with_crls(). @@ -188,7 +188,7 @@ impl CrlManager { // try to load if not already loaded { let inner = self.inner.read().unwrap(); - if inner.crl_ders.is_empty() { + if !inner.loaded { drop(inner); debug!("crl not loaded, attempting to load now"); if let Err(e) = self.load_crl() { From dd51264910282121546e69623b156f51a12c0848 Mon Sep 17 00:00:00 2001 From: nilekh <1626598+nilekhc@users.noreply.github.com> Date: Fri, 9 Jan 2026 12:47:38 -0800 Subject: [PATCH 14/17] chore: fixes lock Signed-off-by: nilekh <1626598+nilekhc@users.noreply.github.com> --- src/tls/crl.rs | 39 ++++++++++++++++++++++----------------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/src/tls/crl.rs b/src/tls/crl.rs index 9238c66620..c95cdc209a 100644 --- a/src/tls/crl.rs +++ b/src/tls/crl.rs @@ -185,25 +185,30 @@ impl CrlManager { /// returns CRLs as DER bytes for rustls's with_crls(). /// if no CRLs are loaded, attempts to load them first. pub fn get_crl_ders(&self) -> Vec> { - // try to load if not already loaded - { - let inner = self.inner.read().unwrap(); - if !inner.loaded { - drop(inner); - debug!("crl not loaded, attempting to load now"); - if let Err(e) = self.load_crl() { - debug!(error = %e, "failed to load crl"); - return Vec::new(); - } + let inner = self.inner.read().unwrap(); + if inner.loaded { + // already loaded, use existing lock directly + inner + .crl_ders + .iter() + .map(|der| CertificateRevocationListDer::from(der.clone())) + .collect() + } else { + // not loaded yet, drop lock to call load_crl() + drop(inner); + debug!("crl not loaded, attempting to load now"); + if let Err(e) = self.load_crl() { + debug!(error = %e, "failed to load crl"); + return Vec::new(); } + // re-acquire after loading + let inner = self.inner.read().unwrap(); + inner + .crl_ders + .iter() + .map(|der| CertificateRevocationListDer::from(der.clone())) + .collect() } - - let inner = self.inner.read().unwrap(); - inner - .crl_ders - .iter() - .map(|der| CertificateRevocationListDer::from(der.clone())) - .collect() } /// starts watching the CRL file for changes. From 545d222a2b2177b03ed6f94c6777f2a03b388fc0 Mon Sep 17 00:00:00 2001 From: nilekh <1626598+nilekhc@users.noreply.github.com> Date: Fri, 9 Jan 2026 13:38:41 -0800 Subject: [PATCH 15/17] chore: uses Option rather than extra var Signed-off-by: nilekh <1626598+nilekhc@users.noreply.github.com> --- src/tls/crl.rs | 64 +++++++++++++++++++------------------------------- 1 file changed, 24 insertions(+), 40 deletions(-) diff --git a/src/tls/crl.rs b/src/tls/crl.rs index c95cdc209a..619575d509 100644 --- a/src/tls/crl.rs +++ b/src/tls/crl.rs @@ -19,12 +19,11 @@ use notify_debouncer_full::{ }; use rustls::pki_types::CertificateRevocationListDer; use rustls_pemfile::Item; -use std::hash::{Hash, Hasher}; use std::io::Cursor; use std::path::PathBuf; use std::sync::{Arc, RwLock}; use std::time::Duration; -use tracing::{debug, info, warn}; +use tracing::{debug, warn}; #[derive(Debug, thiserror::Error)] pub enum CrlError { @@ -54,9 +53,8 @@ impl std::fmt::Debug for CrlManager { } struct CrlManagerInner { - crl_ders: Vec>, + crl_ders: Option>>, // None = not loaded, Some = loaded (may be empty) crl_path: PathBuf, - loaded: bool, _debouncer: Option>, } @@ -67,9 +65,8 @@ impl CrlManager { let manager = Self { inner: Arc::new(RwLock::new(CrlManagerInner { - crl_ders: Vec::new(), + crl_ders: None, crl_path: crl_path.clone(), - loaded: false, _debouncer: None, })), }; @@ -94,7 +91,7 @@ impl CrlManager { Ok(manager) } - pub fn load_crl(&self) -> Result { + pub fn load_crl(&self) -> Result<(), CrlError> { let mut inner = self.inner.write().unwrap(); debug!(path = ?inner.crl_path, "loading crl"); @@ -103,10 +100,8 @@ impl CrlManager { // empty file means no revocations - this is valid if data.is_empty() { debug!(path = ?inner.crl_path, "crl file is empty, treating as no revocations"); - let crl_changed = !inner.crl_ders.is_empty(); - inner.crl_ders.clear(); - inner.loaded = true; - return Ok(crl_changed); + inner.crl_ders = Some(Vec::new()); + return Ok(()); } debug!(bytes = data.len(), "read crl file"); @@ -124,10 +119,8 @@ impl CrlManager { // empty PEM file (no CRL blocks) means no revocations if der_crls.is_empty() { debug!("no crl blocks found, treating as no revocations"); - let crl_changed = !inner.crl_ders.is_empty(); - inner.crl_ders.clear(); - inner.loaded = true; - return Ok(crl_changed); + inner.crl_ders = Some(Vec::new()); + return Ok(()); } debug!(count = der_crls.len(), "found crl block(s) in file"); @@ -146,23 +139,14 @@ impl CrlManager { validated_ders.push(der_data); } - // use hash comparison to detect actual content changes, not just count changes - let crl_changed = - Self::compute_crl_hash(&validated_ders) != Self::compute_crl_hash(&inner.crl_ders); - // store validated DER bytes - inner.crl_ders = validated_ders; - inner.loaded = true; - - debug!(count = inner.crl_ders.len(), "crl loaded successfully"); - Ok(crl_changed) - } + inner.crl_ders = Some(validated_ders); - /// computes a hash of CRL DER bytes for change detection - fn compute_crl_hash(ders: &[Vec]) -> u64 { - let mut hasher = std::collections::hash_map::DefaultHasher::new(); - ders.hash(&mut hasher); - hasher.finish() + debug!( + count = inner.crl_ders.as_ref().map(|v| v.len()).unwrap_or(0), + "crl loaded successfully" + ); + Ok(()) } /// parses PEM-encoded CRL data that may contain multiple CRL blocks @@ -186,10 +170,9 @@ impl CrlManager { /// if no CRLs are loaded, attempts to load them first. pub fn get_crl_ders(&self) -> Vec> { let inner = self.inner.read().unwrap(); - if inner.loaded { + if let Some(ref crl_ders) = inner.crl_ders { // already loaded, use existing lock directly - inner - .crl_ders + crl_ders .iter() .map(|der| CertificateRevocationListDer::from(der.clone())) .collect() @@ -205,9 +188,13 @@ impl CrlManager { let inner = self.inner.read().unwrap(); inner .crl_ders - .iter() - .map(|der| CertificateRevocationListDer::from(der.clone())) - .collect() + .as_ref() + .map(|ders| { + ders.iter() + .map(|der| CertificateRevocationListDer::from(der.clone())) + .collect() + }) + .unwrap_or_default() } } @@ -248,11 +235,8 @@ impl CrlManager { // as well as direct file writes and text editor saves debug!("crl directory changed, reloading"); match manager.load_crl() { - Ok(crl_count_changed) => { + Ok(()) => { debug!("crl reloaded successfully after file change"); - if crl_count_changed { - info!("crl content changed"); - } } Err(e) => debug!(error = %e, "failed to reload crl"), } From b92c08c9c752b9bdf4cea6a8fa1ab49bc1081185 Mon Sep 17 00:00:00 2001 From: nilekh <1626598+nilekhc@users.noreply.github.com> Date: Mon, 12 Jan 2026 07:22:23 -0800 Subject: [PATCH 16/17] chore: addresses review comments Signed-off-by: nilekh <1626598+nilekhc@users.noreply.github.com> --- Cargo.lock | 1 + Cargo.toml | 1 + src/proxyfactory.rs | 6 +++- src/tls/crl.rs | 71 +++++++++++++++++++++++++++++++++++++-------- 4 files changed, 66 insertions(+), 13 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7177aea4c8..d84d660b74 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4616,6 +4616,7 @@ dependencies = [ "textnonce", "thiserror 2.0.17", "tikv-jemallocator", + "time", "tls-listener", "tokio", "tokio-rustls", diff --git a/Cargo.toml b/Cargo.toml index 3948035749..ea79b1b140 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -158,6 +158,7 @@ test-case = "3.3" oid-registry = "0.8" rcgen = { version = "0.14", features = ["pem", "x509-parser"] } x509-parser = { version = "0.17", default-features = false, features = ["verify"] } +time = "0.3" ctor = "0.5" tempfile = "3.21" diff --git a/src/proxyfactory.rs b/src/proxyfactory.rs index 202117a960..0bdb883b11 100644 --- a/src/proxyfactory.rs +++ b/src/proxyfactory.rs @@ -75,7 +75,11 @@ impl ProxyFactory { Some(manager_arc) } Err(e) => { - tracing::debug!("failed to initialize crl manager: {}", e); + tracing::warn!( + path = ?crl_path, + error = %e, + "failed to initialize crl manager" + ); None } } diff --git a/src/tls/crl.rs b/src/tls/crl.rs index 619575d509..25c9dab55b 100644 --- a/src/tls/crl.rs +++ b/src/tls/crl.rs @@ -94,7 +94,6 @@ impl CrlManager { pub fn load_crl(&self) -> Result<(), CrlError> { let mut inner = self.inner.write().unwrap(); - debug!(path = ?inner.crl_path, "loading crl"); let data = std::fs::read(&inner.crl_path)?; // empty file means no revocations - this is valid @@ -104,27 +103,21 @@ impl CrlManager { return Ok(()); } - debug!(bytes = data.len(), "read crl file"); - // parse all CRL blocks (handles concatenated CRLs) - let der_crls = if data.starts_with(b"-----BEGIN") { - debug!("crl is in PEM format, extracting all crl blocks"); + let is_pem = data.starts_with(b"-----BEGIN"); + let der_crls = if is_pem { Self::parse_pem_crls(&data)? } else { - debug!("crl is in DER format"); - // single DER-encoded CRL vec![data] }; // empty PEM file (no CRL blocks) means no revocations if der_crls.is_empty() { - debug!("no crl blocks found, treating as no revocations"); + debug!(path = ?inner.crl_path, "no crl blocks found, treating as no revocations"); inner.crl_ders = Some(Vec::new()); return Ok(()); } - debug!(count = der_crls.len(), "found crl block(s) in file"); - let mut validated_ders = Vec::new(); for (idx, der_data) in der_crls.into_iter().enumerate() { @@ -134,8 +127,6 @@ impl CrlManager { CrlError::WebPkiError(format!("failed to parse crl {}: {:?}", idx + 1, e)) })?; - debug!(crl = idx + 1, "loaded crl"); - validated_ders.push(der_data); } @@ -143,6 +134,8 @@ impl CrlManager { inner.crl_ders = Some(validated_ders); debug!( + path = ?inner.crl_path, + format = if is_pem { "PEM" } else { "DER" }, count = inner.crl_ders.as_ref().map(|v| v.len()).unwrap_or(0), "crl loaded successfully" ); @@ -300,4 +293,58 @@ mod tests { let result = CrlManager::new(file.path().to_path_buf()); assert!(result.is_ok(), "should handle empty CRL file gracefully"); } + + #[test] + fn test_crl_manager_valid_crl() { + use rcgen::{ + CertificateParams, CertificateRevocationListParams, Issuer, KeyIdMethod, KeyPair, + RevocationReason, RevokedCertParams, SerialNumber, + }; + + // generate a CA key pair + let ca_key_pair = KeyPair::generate_for(&rcgen::PKCS_ECDSA_P256_SHA256) + .expect("failed to generate CA key pair"); + + // create CA certificate params + let mut ca_params = CertificateParams::default(); + ca_params.is_ca = rcgen::IsCa::Ca(rcgen::BasicConstraints::Unconstrained); + ca_params.key_usages = vec![ + rcgen::KeyUsagePurpose::KeyCertSign, + rcgen::KeyUsagePurpose::CrlSign, + ]; + + // create issuer from CA params and key + let issuer = Issuer::from_params(&ca_params, &ca_key_pair); + + // create CRL with one revoked certificate + let crl_params = CertificateRevocationListParams { + this_update: time::OffsetDateTime::now_utc(), + next_update: time::OffsetDateTime::now_utc() + time::Duration::days(30), + crl_number: SerialNumber::from(1u64), + issuing_distribution_point: None, + revoked_certs: vec![RevokedCertParams { + serial_number: SerialNumber::from(12345u64), + revocation_time: time::OffsetDateTime::now_utc(), + reason_code: Some(RevocationReason::KeyCompromise), + invalidity_date: None, + }], + key_identifier_method: KeyIdMethod::Sha256, + }; + + let crl = crl_params.signed_by(&issuer).expect("failed to sign CRL"); + let crl_pem = crl.pem().expect("failed to encode CRL as PEM"); + + // write CRL to temp file + let mut file = NamedTempFile::new().expect("failed to create temporary test file"); + file.write_all(crl_pem.as_bytes()) + .expect("failed to write CRL to temporary file"); + file.flush().expect("failed to flush temporary test file"); + + // test that CrlManager can load it + let manager = CrlManager::new(file.path().to_path_buf()) + .expect("should successfully parse valid CRL"); + + let ders = manager.get_crl_ders(); + assert_eq!(ders.len(), 1, "should have loaded one CRL"); + } } From 12afa0a1b15e75c0c59154b648d3860f2021c718 Mon Sep 17 00:00:00 2001 From: nilekh <1626598+nilekhc@users.noreply.github.com> Date: Mon, 12 Jan 2026 10:23:13 -0800 Subject: [PATCH 17/17] chore: fixes merge conflict Signed-off-by: nilekh <1626598+nilekhc@users.noreply.github.com> --- src/socket.rs | 1 - src/tls/certificate.rs | 25 ++++++++----------------- 2 files changed, 8 insertions(+), 18 deletions(-) diff --git a/src/socket.rs b/src/socket.rs index 1b1d02b9a8..1e4827b789 100644 --- a/src/socket.rs +++ b/src/socket.rs @@ -178,7 +178,6 @@ impl Listener { let res = SockRef::from(&stream).set_tcp_keepalive(&ka); tracing::trace!("set keepalive: {:?}", res); } - #[cfg(target_os = "linux")] if cfg.user_timeout_enabled { let ut = cfg.keepalive_time + cfg.keepalive_retries * cfg.keepalive_interval; let res = SockRef::from(&stream).set_tcp_user_timeout(Some(ut)); diff --git a/src/tls/certificate.rs b/src/tls/certificate.rs index 31262ae67b..78c5064605 100644 --- a/src/tls/certificate.rs +++ b/src/tls/certificate.rs @@ -320,6 +320,7 @@ impl WorkloadCertificate { } } + // TODO: check if our own certificate is revoked in the CRL and log warning let raw_client_cert_verifier = builder.build()?; let client_cert_verifier = @@ -336,17 +337,11 @@ impl WorkloadCertificate { Ok(sc) } - pub fn client_config( - &self, - identity: Vec, - crl_manager: Option>, - ) -> Result { + // TODO: add CRL support for outbound connections (client verifying server certs) + // this requires a separate design due to complexity - deferred for follow-up + pub fn client_config(&self, identity: Vec) -> Result { let roots = self.root_store.clone(); - let verifier = IdentityVerifier { - roots, - identity, - crl_manager, - }; + let verifier = IdentityVerifier { roots, identity }; let mut cc = ClientConfig::builder_with_provider(crate::tls::lib::provider()) .with_protocol_versions(tls::TLS_VERSIONS) .expect("client config must be valid") @@ -362,12 +357,8 @@ impl WorkloadCertificate { Ok(cc) } - pub fn outbound_connector( - &self, - identity: Vec, - crl_manager: Option>, - ) -> Result { - let cc = self.client_config(identity, crl_manager)?; + pub fn outbound_connector(&self, identity: Vec) -> Result { + let cc = self.client_config(identity)?; Ok(OutboundConnector { client_config: Arc::new(cc), }) @@ -486,7 +477,7 @@ mod test { }); let stream = TcpStream::connect(addr).await.unwrap(); - let client = cert2.outbound_connector(vec![id], None).unwrap(); + let client = cert2.outbound_connector(vec![id]).unwrap(); let mut tls = client.connect(stream).await.unwrap(); let _ = tls.write(b"hi").await.unwrap();