Skip to content

feat(networking): native mTLS with subject-name authorization for fabric inter-node communication#4681

Open
rushabhvaria wants to merge 27 commits into
restatedev:mainfrom
rushabhvaria:main
Open

feat(networking): native mTLS with subject-name authorization for fabric inter-node communication#4681
rushabhvaria wants to merge 27 commits into
restatedev:mainfrom
rushabhvaria:main

Conversation

@rushabhvaria

@rushabhvaria rushabhvaria commented Apr 30, 2026

Copy link
Copy Markdown

Closes #3306
Related: #3583

Summary

  • Add optional TLS/mTLS configuration for the fabric port (5122), securing inter-node communication at the application layer without requiring Kubernetes NetworkPolicy or external service meshes
  • Support strict mode (TLS only) and optional mode (accepts both plaintext and TLS) for zero-downtime rolling upgrades
  • Periodic certificate hot-reload from disk (configurable interval, default 1h)
  • Subject-name authorization (allowed-subject-names): after mTLS authentication, verify the peer's Subject CN and SANs match allowed patterns — prevents unauthorized services from connecting when using a shared CA
  • Fail-safe config validation: allowed-subject-names is required when require-client-auth is true — prevents accidental fail-open. Use ["*"] to explicitly opt into CA-only trust

Motivation

Restate's security docs state: "You are expected to secure access to [the fabric port] using the network and proxy layers available in your deployment environment." The recommended approach is Kubernetes NetworkPolicy — but many production environments don't support it (shared clusters, certain CNI plugins, platform constraints). Most distributed systems (etcd, CockroachDB, Consul) offer built-in inter-node TLS — this brings Restate to parity, especially for enterprise environments.

The authorization layer addresses feedback that mTLS alone is insufficient when using a shared CA (e.g., SPIFFE). Without identity checking, any service holding a cert from the same CA could connect to the fabric port.

Configuration

[networking.tls]
mode = "strict"                          # or "optional" for rolling upgrades
cert-file = "/certs/node.crt"
key-file = "/certs/node.key"
ca-files = ["/certs/ca.crt"]
require-client-auth = true               # default: mTLS enabled
refresh-interval = "1h"                  # hot-reload certs from disk

# Authorization: required when require-client-auth is true
# Use ["*"] for CA-only trust, or specify identity patterns
allowed-subject-names = [
    "spiffe://svc.example.com/restate/*",
]

# Optional: separate client certs for outbound (inherits from above if omitted)
[networking.tls.client]
cert-file = "/certs/client.crt"
key-file = "/certs/client.key"
root-ca-files = ["/certs/client-ca.crt"]

Without [networking.tls], behavior is identical to today (plaintext).

Authorization behavior

require-client-auth allowed-subject-names Result
true omitted/empty Startup error — must specify patterns
true ["*"] CA-only trust (explicit opt-in)
true ["spiffe://domain/*"] Identity-based authorization
false omitted/empty OK — no client auth, no subject check

Design

Encryption and Authentication (mTLS):

  • TLS termination at the tonic/hyper layer using rustls (already a workspace dep)
  • Inbound strict: tokio-rustls::TlsAcceptor wraps TcpStream before hyper
  • Inbound optional: peek first byte — 0x16 (TLS ClientHello) routes to TLS, else plaintext
  • Outbound: custom tower::service_fn connector using tokio_rustls::TlsConnector, reads latest certs from ArcSwap per-connection
  • Cert rotation: background tokio task reloads PEM files on interval, swaps via ArcSwap (lock-free)
  • Scheme signaling: TLS-enabled nodes advertise https:// — peers use the scheme to decide connection type

Authorization (subject-name verification):

  • SubjectNameVerifier wraps WebPkiClientVerifier — delegates chain validation, then checks identity
  • Checks Subject CN first, then SANs (DNS names and URIs) against glob patterns
  • Uses x509-parser for DER certificate parsing
  • ["*"] explicitly skips identity checking (CA-only trust, no SubjectNameVerifier overhead)
  • Config validation at startup prevents empty allowed-subject-names when client auth is enabled

Rolling upgrade path:

  1. Deploy all nodes with mode = "optional" and TLS certs — nodes advertise https://, accept both
  2. Verify all nodes communicate via TLS
  3. Switch to mode = "strict" — plaintext rejected

Note on restatectl compatibility (related to #3583):
Port 5122 currently serves both internal (CoreNodeSvc) and external (ClusterCtrlSvc, NodeCtlSvc) gRPC services. In optional mode, restatectl connects via plaintext while inter-node traffic uses TLS. Once #3583 splits these into separate ports, strict mode can be applied to the internal port without affecting restatectl.

Changes

File Change
crates/types/src/config/networking.rs FabricTlsOptions, TlsMode, allowed-subject-names, validate() + 11 unit tests
crates/types/src/net/address.rs PeerNetAddress::is_tls(), derive_from_bind_address_with_tls() + 2 tests
crates/core/src/network/tls.rs NEWTlsCertResolver, SubjectNameVerifier, cert loading, hot-reload, glob_match + 19 unit tests
crates/core/src/network/net_util.rs TLS accept (strict) + protocol sniff (optional)
crates/core/src/network/grpc/connector.rs Custom TLS connector for outbound https:// peers
crates/core/src/network/server_builder.rs Thread TlsCertResolver to listener
crates/core/src/network/networking.rs Accept TLS resolver in constructor
crates/node/src/lib.rs TLS init + config validation at startup, spawn reloader, wire to server + connector
crates/admin/src/service.rs Pass None for admin port (no TLS on admin)
server/tests/fabric_tls.rs NEW — 2 integration tests (strict cluster, optional mode)

Verification

  • cargo check — all modified crates compile
  • cargo clippy -D warnings — zero warnings
  • cargo fmt --check — clean
  • 32 unit tests pass (11 config + 2 address + 19 TLS)
  • 2 integration tests compile (strict cluster, optional mode)
  • No regressions in existing tests

Test plan

Config and validation (11 tests):

  • TOML parsing: defaults, modes, client inheritance, allowed-subject-names
  • Validation: empty + client auth = error, ["*"] = OK, no client auth = skip, specific patterns = OK

TLS core (19 tests):

  • PEM cert/key loading: valid, missing, empty, invalid
  • is_tls() detection: https, http, bare host, UDS
  • derive_from_bind_address_with_tls(): http:// vs https:// scheme
  • Glob matching: exact, trailing *, middle *, prefix, multiple wildcards
  • Subject-name verifier with real X.509 certs (via rcgen):
    • Accept matching SAN URI / SAN DNS / CN
    • Reject non-matching, reject no match anywhere
    • Multi-pattern authorization, CN fallback without SANs

Integration (2 tests):

  • fabric_tls_strict_cluster: 3-node cluster with strict mTLS
  • fabric_tls_optional_mode: 3-node cluster with optional TLS

…nication

Add optional TLS/mTLS configuration for Restate's fabric port (5122).
This enables securing inter-node communication at the application layer
without relying on Kubernetes NetworkPolicy or external service meshes.

Configuration lives under [networking.tls] with support for:
- Strict mode (TLS only) and optional mode (accepts both plaintext and TLS)
- Mutual TLS with configurable client certificate requirements
- Periodic certificate hot-reload from disk (default: 1h)
- Client config inheritance from server config when not specified separately
- Scheme-based signaling (https:// in advertised-address)

Key changes:
- Add FabricTlsOptions, FabricTlsClientOptions, TlsMode config structs
- Add TlsCertResolver with ArcSwap-based lock-free cert rotation
- Modify run_hyper_server to support TLS accept and protocol sniffing
- Modify GrpcConnector to use ClientTlsConfig for https:// peers
- Extend PeerNetAddress with is_tls() and derive_from_bind_address_with_tls()
- Add tokio-rustls, rustls-pemfile workspace dependencies

Without [networking.tls] configuration, behavior is identical to today.
- Config parsing tests: TOML deserialization, defaults, mode parsing,
  client inheritance fallback, client override
- TLS resolver tests: cert loading from PEM, missing file errors,
  empty cert file errors, invalid key handling, mismatched cert/key rejection
- Address tests: is_tls() for https/http/UDS, derive_from_bind_address_with_tls()

Also restores inline comments in derive_from_bind_address_with_tls that
were inadvertently dropped during refactoring.
feat(networking): native mTLS for fabric inter-node communication
@github-actions

github-actions Bot commented Apr 30, 2026

Copy link
Copy Markdown

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@rushabhvaria

Copy link
Copy Markdown
Author

I have read the CLA Document and I hereby sign the CLA

Add cluster-level integration tests that verify multi-node Restate
clusters form correctly with TLS-secured fabric communication.

Tests:
- fabric_tls_strict_cluster: 3-node cluster with strict mTLS, verifies
  all nodes connect and cluster becomes healthy
- fabric_tls_optional_mode: 3-node cluster with optional TLS mode,
  verifies nodes form cluster accepting both TLS and plaintext

Uses rcgen to generate test CA + per-node certificates at runtime.
Nodes use random TCP ports (not UDS) since TLS applies to TCP only.
@rushabhvaria
rushabhvaria marked this pull request as ready for review April 30, 2026 21:48

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

test(networking): add integration tests for fabric mTLS
mTLS authenticates the peer but doesn't authorize them. In environments
where a shared CA issues certs to many services (e.g., SPIFFE), any
service could connect to the fabric port. This adds an optional
`allowed-sans` config that checks the peer certificate's Subject
Alternative Names (DNS names and URIs) against glob patterns after
the TLS handshake succeeds.

Config example:
  [networking.tls]
  allowed-sans = ["spiffe://svc.pin220.com/restate-agents/*"]

Implementation:
- SanCheckingVerifier wraps WebPkiClientVerifier, adding SAN check
  after chain validation passes
- Uses x509-parser to extract SANs from DER certificates
- Supports * glob wildcards for flexible pattern matching
- When allowed-sans is empty (default), behavior is unchanged

Tests:
- glob_match: exact, trailing wildcard, middle wildcard, prefix, multi
- Config parsing with allowed-sans field
…d add CN matching

Rename `allowed-sans` to `allowed-subject-names` to better reflect that
both the Subject Common Name (CN) and Subject Alternative Names (DNS/URI)
are checked against the allowed patterns.

The verifier now checks CN first, then SANs. This handles certs that use
CN alone (without SANs) and provides a more complete authorization model.

Tests added:
- test_subject_verifier_accepts_matching_cn: CN-only cert accepted
- test_subject_verifier_cn_fallback_when_no_san: CN match when no SANs present
- test_subject_verifier_rejects_no_match_anywhere: neither CN nor SANs match
feat(networking): add SAN-based authorization for fabric mTLS
@rushabhvaria rushabhvaria changed the title feat(networking): native mTLS for fabric inter-node communication feat(networking): native mTLS with subject-name authorization for fabric inter-node communication May 1, 2026
… is enabled

Prevent accidental fail-open: when require-client-auth is true,
allowed-subject-names must be explicitly set. Operators who want
CA-only trust (no identity checking) set allowed-subject-names = ["*"]
to make the choice explicit. An empty list with client auth enabled
is now a configuration error that prevents node startup.

This addresses feedback that the previous default (empty = allow all)
could lead to unintended access when using a shared CA.

Changes:
- Add FabricTlsOptions::validate() with startup-time check
- Call validate() during node initialization before TLS setup
- Treat ["*"] as explicit CA-only trust (skip SubjectNameVerifier)
- Update integration tests to use allowed-subject-names = ["*"]
- 4 new validation unit tests

Config that now fails:
  [networking.tls]
  require-client-auth = true
  # missing allowed-subject-names → startup error

Config that works:
  [networking.tls]
  require-client-auth = true
  allowed-subject-names = ["*"]               # explicit CA-only trust
  # OR
  allowed-subject-names = ["spiffe://dom/*"]   # identity-based authz
…subject-names

feat(networking): require allowed-subject-names when mTLS client auth…
Comment thread crates/core/src/network/net_util.rs Outdated

@nickpan47 nickpan47 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall lgtm. Minor comment on duplicated code section.

@tillrohrmann

Copy link
Copy Markdown
Contributor

Thanks a lot for adding mTLS support to Restate @rushabhvaria. It looks like a great contribution.

Right now the team is a little bit busy with finalizing the 1.7 release and that's why we probably need a bit of time to give your PR the deserved attention. So please bear with us.

@tillrohrmann
tillrohrmann self-requested a review May 4, 2026 07:37
rushabhvaria added a commit to rushabhvaria/restate that referenced this pull request May 4, 2026
Extract serve_connection() helper to eliminate repeated connection
error-handling blocks across TLS, plaintext, and UDS code paths.
Also simplify the TLS/plaintext branching by resolving the TLS
acceptor first, then handling the connection in two clean branches
instead of five duplicated blocks.

Addresses review feedback from nickpan47 on PR restatedev#4681.
Extract serve_connection() helper to eliminate repeated connection
error-handling blocks across TLS, plaintext, and UDS code paths.
Also simplify the TLS/plaintext branching by resolving the TLS
acceptor first, then handling the connection in two clean branches
instead of five duplicated blocks.

Addresses review feedback from nickpan47 on PR restatedev#4681.
@AhmedSoliman
AhmedSoliman self-requested a review May 5, 2026 09:30
@rushabhvaria

Copy link
Copy Markdown
Author

@AhmedSoliman can you approve the workflow to have claude code review the pull request? also could you help provide an ETA for potentially merging this? Thank you

@AhmedSoliman

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0b449679a4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/types/src/net/address.rs Outdated
Comment thread crates/core/src/network/net_util.rs Outdated
Comment thread crates/core/src/network/grpc/connector.rs Outdated
Fix three issues identified in PR review:

1. Auto-advertise https:// when TLS is configured (P1)
   When [networking.tls] is set but no explicit advertised-address is
   configured, the auto-guessed address now uses https:// scheme.
   Previously it always used http://, causing peers to attempt plaintext
   connections that fail in strict mode.

2. Add TLS handshake timeout to prevent accept-loop blocking (P1)
   The TLS handshake was awaited inline in the listener loop, allowing
   a stalled client to block all new connections. Now bounded by a 5s
   timeout — stalled handshakes are dropped without blocking peers.

3. Strip IPv6 brackets before constructing ServerName (P2)
   uri.host() returns bracketed IPv6 (e.g., "[::1]") which is invalid
   for rustls ServerName. Now strips brackets before TLS connection.
@tillrohrmann

Copy link
Copy Markdown
Contributor

@rushabhvaria sorry for the slow response time. We are currently in the process of creating the v1.7 release which requires most of our attention. At the latest, we are going to review this PR once the release is created.

@rushabhvaria

Copy link
Copy Markdown
Author

@tillrohrmann following up on this PR's review

Resolve conflicts from upstream sync:
- Cargo.toml: keep upstream rand 0.10.1 bump; retain additive rcgen and
  rustls-pemfile deps for fabric mTLS
- crates/node/src/lib.rs: combine upstream `features: EnumSet<ClusterFeature>`
  param with our `fabric_tls: bool` in create_initial_nodes_configuration
- server/Cargo.toml, crates/core/src/network/tls.rs: upstream renamed
  restate-time-util -> restate-util-time (util/time crate)
- server/tests/fabric_tls.rs: provision_cluster gained a 4th
  `disabled_features` arg; pass EnumSet::empty()
- Drop `test_` prefix from mTLS test fns in tls.rs, networking.rs, address.rs
  to satisfy upstream's new `redundant_test_prefix = "deny"` lint

Verified: cargo check --workspace, test targets compile, clippy clean
(core/types/server), fmt clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@AhmedSoliman
AhmedSoliman requested review from MohamedBassem and removed request for AhmedSoliman July 9, 2026 10:19

@MohamedBassem MohamedBassem left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the PR and we're sorry it took to long to get back to it. I've left a bunch of comments.

The restatectl gap you mentioned in your PR is a big gap though. It'll prevent you from managing the cluster if you enable strict mode. Perhaps as a stop gap until the ports are separated, you might want to add some env variables to restatectl that allows it to load some certs to present to the server during connection?

Comment thread crates/core/src/network/grpc/connector.rs Outdated
Comment thread crates/core/src/network/net_util.rs Outdated
Comment thread crates/core/src/network/net_util.rs Outdated
Comment thread crates/types/src/net/address.rs Outdated
Comment on lines +367 to +369
pub fn derive_from_bind_address(address: SocketAddress, advertised_host: Option<&str>) -> Self {
Self::derive_from_bind_address_with_tls(address, advertised_host, false)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: It seems like this is only called from guess_advertised_address_with_tls, so might as well just remove it and make the TLS decision explicit?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed — derive_from_bind_address_with_tls is folded into derive_from_bind_address, which now takes the TLS flag explicitly. Same for the address-book variant.

Comment thread crates/types/src/net/listener.rs Outdated
Comment on lines +271 to +276
pub fn guess_advertised_address<P: ListenerPort + 'static>(
&self,
advertised_host: Option<&str>,
) -> AdvertisedAddress<P> {
self.guess_advertised_address_with_tls(advertised_host, false)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this implicit tls=false is hiding a lot of callsites that probably should have had TLS enabled. The join_cluster_inner I mentioned above is one example.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the implicit-default variants — derive_from_bind_address, guess_advertised_address, and the fabric advertised_address all take an explicit tls flag now, so every callsite has to make the decision consciously. join_cluster_inner and the metadata-server registration paths were indeed affected; details in the thread on node/lib.rs.

Comment thread crates/node/src/lib.rs
Comment on lines +857 to +859
let fabric_tls = Configuration::pinned().networking.tls.is_some();
let initial_nodes_configuration =
create_initial_nodes_configuration(common_opts, features, fabric_tls);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like when we're creating a new nodes configuration, we're advertising our address with TLS, but when nodes are joining the cluster in join_cluster_inner for example, we're not advertising self addresses with TLS. Which makes me wonder if this was needed in the first place? If it is, then we should also added to joining nodes?

@MohamedBassem MohamedBassem Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, coming back to this after reading the entire PR. I think the reason why this works now, is because server to server comms relies on the TLS in the config to decide whether they should initiate a TLS connection or not. Not on the scheme used in the advertised address.

This, however, will break non server clients that attempt to connect on the that port. Most notably is restatectl (which you've also noted in your PR description). If I'm to guess, this change was required to make the e2e test path because without it, the nodes[0].provision_cluster would have failed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, in the connector you're doing let use_tls = address.is_tls() && tls.is_some();. So not advertising the https address is a real bug. A bit surprised that the strict mode test is passing (unless it's not validating what it's supposed to validate).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a real bug — join_cluster_inner (and the metadata-server address registration paths) used the non-TLS advertised_address(), so any node joining an existing cluster advertised http:// and would be dialed in plaintext. Your read on the test was also right: the local-cluster-runner advertises UDS addresses (there's a todo: add tcp support in the runner), and TLS never applies to UDS, so the strict test wasn't exercising TLS over TCP at all.

Fixed by removing the implicit tls=false variants entirely — advertised_address() now takes an explicit tls flag and all fabric callsites pass networking.tls.is_some(). The integration tests now also assert that every node registered in the nodes configuration advertises an https:// fabric address, so a regression here fails the test rather than silently falling back to plaintext.

Comment thread crates/core/src/network/tls.rs Outdated
Comment thread crates/core/src/network/tls.rs Outdated
Comment thread crates/core/src/network/tls.rs Outdated
Comment thread crates/core/src/network/tls.rs Outdated
}
}

let builder = ClientConfig::builder().with_root_certificates(root_store);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we not want the client to verify the cert subject?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, and the question exposed a second issue: with SPIFFE-style certs (URI SANs only), rustls's default hostname verification would fail anyway, since the dialed host never matches a URI SAN. Added a SubjectNameServerVerifier on the client side: chain validation (trust, expiry, revocation) is delegated to WebPkiServerVerifier, and endpoint identity is established by matching the server cert's CN/SANs against allowed-subject-names — hostname mismatch is tolerated only when patterns are configured. This closes the shared-CA gap in the outbound direction too: a peer with a valid cert but foreign identity is now rejected by both sides. Covered by a test with real CA-signed SPIFFE certs (accept-on-match, reject-foreign-identity, reject-untrusted-chain).

rushabhvaria and others added 2 commits July 20, 2026 13:51
The custom glob_match was greedy without backtracking, so patterns like
"a*bc" failed to match "abcbc" — the wildcard could not give back
characters to let the literal tail match. This caused false rejections
(fail-closed) of otherwise-valid peer subject names.

Use the wildmatch crate with '?' disabled so only '*' is special.
Also replace the unmaintained rustls-pemfile with the PemObject API
from rustls-pki-types (fixes RUSTSEC-2025-0134 flagged by cargo deny).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
rushabhvaria and others added 2 commits July 20, 2026 14:56
fix(networking): replace hand-rolled glob matcher with wildmatch crate
…llsite

Joining nodes (join_cluster_inner) and the metadata-server address
registration paths used the non-TLS advertised_address(), so any node
that joined an existing cluster advertised http:// and was dialed in
plaintext by peers — since the connector gates TLS on address.is_tls().
Only freshly provisioned clusters got https:// addresses.

Remove the implicit tls=false wrappers entirely so this class of bug is
unrepresentable:
- AdvertisedAddress::derive_from_bind_address now takes an explicit tls flag
- AddressBook::guess_advertised_address now takes an explicit tls flag
- CommonOptions::advertised_address (fabric) now takes an explicit tls flag;
  advertised_address_with_tls is folded into it
- All fabric callsites pass networking.tls.is_some()

Test hardening: node certs now carry loopback SANs, and both integration
tests assert every registered node address uses the https:// scheme after
the cluster is healthy.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
rushabhvaria and others added 2 commits July 20, 2026 15:42
fix(networking): make fabric advertised address TLS-aware at every callsite
The optional-mode protocol sniff (peek) had no timeout and the TLS
handshake ran inline in the listener loop, so a single slow or stalled
client could block acceptance of all subsequent connections — a
head-of-line bottleneck and practical DoS vector.

The accept loop now only accepts and spawns. The protocol sniff and the
handshake both run inside the per-connection SocketHandler task, bounded
by a single 5s timeout. Connections register with the graceful shutdown
via an owned Watcher from within their own task, preserving drain
semantics. TLS and plaintext streams are served through one code path
via tokio_util's Either.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
rushabhvaria and others added 2 commits July 20, 2026 15:45
fix(networking): move TLS sniff and handshake out of the accept loop
…in TLS

Two changes that landed together because the second depends on the first:

1. The client now verifies the server certificate's subject. A new
   SubjectNameServerVerifier delegates chain validation (trust, expiry,
   revocation) to WebPkiServerVerifier and establishes endpoint identity
   by matching the server cert's CN/SANs against allowed-subject-names.
   Hostname mismatch is tolerated only when subject patterns are
   configured — required for SPIFFE-style certs (URI SANs only) where
   the dialed host can never match, and it closes the shared-CA gap in
   the outbound direction: a peer with a valid cert but foreign identity
   is now rejected by both sides.

2. The custom TLS dial closure in the connector is replaced with tonic's
   built-in TLS (tls-ring feature) via tls_config_with_verifier. The
   resolver now exposes ClientTlsMaterials (identity PEM + verifier),
   swapped atomically on cert reload; channels read it per connection
   attempt so hot-reload keeps working. Manual IPv6 bracket handling
   collapses to a domain_name override; port defaulting moves to tonic.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
rushabhvaria and others added 2 commits July 20, 2026 16:02
feat(networking): client-side subject verification via tonic's built-in TLS
…ly check

- The certificate reload loop now runs as a TaskCenter Background task
  with a cancellation_watcher, so it participates in graceful shutdown
  instead of being an orphaned tokio task.
- CA-only trust detection is now any(|s| s == "*"): a pattern list
  containing "*" matches every subject under glob semantics, so building
  a SubjectNameVerifier for it is pure overhead. Applied to the server
  side to match the client-side check.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
refactor(networking): TaskCenter-managed cert reloader, simpler CA-only check
@MohamedBassem

Copy link
Copy Markdown
Contributor

Thanks a lot for addressing the comments, I quickly went through the fixes and they overall look good to me. Will let CI run now, trigger another codex review, and tomorrow morning, will do some validation locally :)

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 65d2e2c567

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/node/src/init.rs
let my_advertised_address =
TaskCenter::with_current(|tc| common.advertised_address(tc.address_book()));
let my_advertised_address = TaskCenter::with_current(|tc| {
common.advertised_address(tc.address_book(), config.networking.tls.is_some())

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Route metadata/control gRPC through fabric TLS

When TCP fabric TLS is enabled, this registers joining nodes with an https:// fabric address (and initial provisioning does the same at crates/node/src/lib.rs:822), but several metadata/control paths still consume NodeConfig.address via create_tonic_channel rather than the new GrpcConnector (for example crates/metadata-server/src/raft/network/networking.rs:171 and crates/metadata-providers/src/replicated.rs:516). create_tonic_channel only builds a normal endpoint from the URI and applies common options (crates/core/src/network/net_util.rs:54-63), so in a TCP TLS deployment those metadata-store/raft/control channels do not get the fabric client cert, CA roots, or subject verifier and will fail against the TLS/mTLS-wrapped fabric port even though regular fabric messages can connect.

Useful? React with 👍 / 👎.

Comment on lines +359 to +362
let ca_only_trust = opts.allowed_subject_names.iter().any(|s| s == "*");
let verifier: Arc<dyn ServerCertVerifier> =
if ca_only_trust || opts.allowed_subject_names.is_empty() {
webpki_verifier

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Honor wildcard CA-only trust on outbound TLS

When allowed-subject-names = ["*"], this selects the raw WebPkiServerVerifier; that still enforces DNS/IP hostname matching against the URI host, while the custom SubjectNameServerVerifier above is the only outbound path that tolerates NotValidForName before applying the configured subject policy. For CA-only deployments using SPIFFE URI-only or CN-only node certificates, outbound peer connections will be rejected despite the documented wildcard meaning in crates/types/src/config/networking.rs:225-226; use a verifier that skips only endpoint-name checking while preserving chain/expiry validation.

Useful? React with 👍 / 👎.

)]
fabric_memory_limit: NonZeroByteCount,

/// # TLS Configuration

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Add release notes for fabric TLS

This introduces a user-facing networking.tls configuration section and new strict/optional fabric TLS behavior, but the commit does not add any release-notes/unreleased/ entry, so users will not get upgrade, rollout, or allowed-subject-name guidance for the new feature.

AGENTS.md reference: AGENTS.md:L51-L51

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for encrypting cross Restate node traffic

5 participants