feat(grpc): TLS channel credentials by arjan-bal · Pull Request #2508 · hyperium/tonic

arjan-bal · 2026-02-16T11:32:52Z

This change adds a rustls-based implementation of ChannelCredentials.

Notes:

The following features have been restricted on the recommendation form the gRPC security team: TLS cipher suites for TLS 1.2, session resumption.

sauravzg · 2026-02-19T06:57:14Z

+
+    // Test-only constructor that enables injecting a custom crypto provider.
+    #[cfg(test)]
+    pub fn new_for_test(


nit: test only code is usually considered an anti pattern(despite being widely used) , Can we manipulate the default provider in the unit test body instead of chaning the construction path?

This constructor was simply calling Self::new_impl. Since new_impl is a private method, it is not accessible outside this module.

Can we manipulate the default provider in the unit test body...

This would either require a test-only setter or make the tests rely on the internal structure of RustlsClientTlsCredentials, making them brittle.

The common way to mock dependencies is to have them injected into the constructor. In this case, however, the crypto provider is fetched from a process-level singleton, and we don't expect users to fetch and pass it to the constructor themselves. To solve this, I introduced an internal constructor that supports dependency injection. The public constructor is now a thin wrapper that delegates to the internal one. This pattern ensures maximum test coverage without compromising the public API.

sauravzg · 2026-02-19T06:57:18Z

+
+        let (_, connection) = tls_stream.get_ref();
+        if let Some(negotiated) = connection.alpn_protocol() {
+            if negotiated != b"h2" {


Maybe we should avoid magic strings at multiple places and use const strings?

Introduced a constant.

sauravzg · 2026-02-19T06:57:23Z

Quick question: Why do we need a separate tests file instead of keeping it in the module itself? Is this a standard practice?

The tests must be in a separate module so they stay out of the production binary. Usually, we keep the test module in the same file, but the test code was getting so long that it made scrolling through the actual logic difficult. I decided to move the tests to their own file to keep things organized, which is also a common practice.

sauravzg · 2026-02-19T06:57:50Z

+}
+
+impl KeyLogFileInner {
+    fn new(path: &PathBuf) -> Self {


I don't think new is appropriate here.
https://rust-lang.github.io/api-guidelines/predictability.html#constructors-are-static-inherent-methods-c-ctor mentions that IO like constructors should use domain spacific names . Aditionally, file::open retuns a Result, maybe we should consider doing the same.

Additionally, since I couldn't find any style guide around fallibility of constructors in rust . I'll be a heretic and point towards cpp style which requires constructors to be "small and infallible".

Another note, should we be supporting AsyncIo here , in case we need to support remote fs etc? I am guessing no, because the trait we want to implement is sync?

This file is copied from the rustls repository which uses the same constructor: https://github.com/rustls/rustls/blob/b07b8419b786f05a95d08868643193465449a136/rustls-util/src/key_log_file.rs#L20

I would prefer not to make too many modifications to this file. Please let me know if you think we need to apply readability fixes here.

sauravzg · 2026-02-19T06:57:59Z

+            return Ok(());
+        };
+
+        self.buf.truncate(0);


Is there a utility for it? This seems like a bunch of magic strings, how do we know this is correct?

This file is copied from the rustls repository, source here: https://github.com/rustls/rustls/blob/main/rustls-util/src/key_log_file.rs

The original implementation doesn't export the constructor to set an arbitrary file path, and relies on the SSLKEYLOGFILE env variable which is commonly used by openssl. Since gRPC C++ supports specifying the file path, I copied over the file and exposed the internal constructor.

sauravzg · 2026-02-19T06:58:03Z

+/// [`KeyLog`] implementation that opens a file whose name is
+/// given to the constructor, and writes keys into it.
+///
+/// If such a file cannot be opened, or cannot be written then


Is silently failing the correct option? What are the implications of silently failing?

Key logging is meant for debugging. This feature must not be used in production. This file is copied from the rsutls repo.

sauravzg · 2026-02-19T06:58:04Z

+    }
+}
+
+impl KeyLog for KeyLogFile {


I am guessing rustls depends on implementing this trait which happens to be infallible and hence requires us to also silently fail?

Yes, this file is copied from the rustls repo.

sauravzg · 2026-02-19T06:58:16Z

+    ///
+    /// The provided PEM should include at least one PEM encoded certificate.
+    pub fn from_pem(pem: impl AsRef<[u8]>) -> Self {
+        let pem = pem.as_ref().into();


Is this always a copy? Same for below.

This results in a single copy, during the creation of a gRPC channel. I don't think the overhead is significant. This is a similar API to the one in tonic:

tonic/tonic/src/transport/tls.rs

Lines 15 to 21 in e6318b7

/// Parse a PEM encoded X509 Certificate.

///

/// The provided PEM should include at least one PEM encoded certificate.

pub fn from_pem(pem: impl AsRef<[u8]>) -> Self {

let pem = pem.as_ref().into();

Self { pem }

}

sauravzg · 2026-02-19T06:58:18Z

+    }
+
+    /// Get a immutable reference to underlying certificate
+    fn get_ref(&self) -> &[u8] {


These are private, right? Why do we need all of them

Only get_ref is required. The other methods are an artifact from copying the tonic implementation, removed them now.

sauravzg · 2026-02-19T06:58:20Z

+    }
+}
+
+impl AsRef<[u8]> for RootCertificates {


I am assuming we'll also need AsMut for consistency?

Removed this for now. Using AsRead exposes the method publicly, while using the get_ref() method in the impl block above avoids this. We can add this in the future, when required.

sauravzg · 2026-02-19T06:58:25Z

+    }
+}
+
+mod provider {


nit: I think the provider stuff should probably be moved into its own file maybe?

This section is roughly 50 lines, consisting mostly of documentation comments. Even with this addition, the mod.rs file remains a manageable size. I’d prefer not to split the file prematurely, but I’m happy to do so if you feel strongly about it.

sauravzg · 2026-02-19T06:58:28Z

+    security_protocol: "tls",
+};
+
+fn sanitize_crypto_provider(mut crypto_provider: CryptoProvider) -> Result<CryptoProvider, String> {


The placement of the utility functions seem slightly odd . They are used in another file but are implemented as private here. Wouldn't it be better to move them closer to their point of use unless we want to share them ?

This will get used in the TLS server credentials implementation.

sauravzg · 2026-02-19T06:58:33Z

+use crate::rt::{endpoint, GrpcEndpoint};
+
+pub struct TlsStream<T> {
+    pub(crate) inner: RustlsStream<T>,


Wouldn't it be better to make it private and provide and inner and/or inner_ref method instead?

Changed. Added a a constructor and an inner method.

dfawley · 2026-02-24T21:36:33Z

+    /// This should be used **only for debugging purposes**. It should never be
+    /// used in a production environment due to security concerns.


Should we give it a scarier name, then? insecure_with_key_log_path e.g.?

Renamed to include an insecure_ prefix. Some rustls APIs use the word "dangerous" for this purpose.

dfawley · 2026-02-24T21:38:08Z

Should we put this in a rustls directory instead of tls since we know we'll eventually have a second TLS creds implementation?

Renamed the directory and module to rustls. I suspect the configuration objects could be reused for the future BoringSSL implementation, so having them in a separate tls module might make sense down the line. However, I've kept everything in the rustls module for now. We can always move the symbols to a new module and use aliases in the rustls module later.

Yeah, I think we'll want some things in tls that are common to the boring implementation, but we can wait until later.

dfawley · 2026-02-24T21:39:18Z

+            return Err(
+            "No crypto provider installed. Enable `tls-aws-lc` feature or install one manually."
+                .to_string(),
+        );


Formatting looks wrong here?

dfawley · 2026-02-24T21:41:14Z

+        let provider = if let Some(p) = CryptoProvider::get_default() {
+            p.as_ref().clone()
+        } else {
+            return Err(
+            "No crypto provider installed. Enable `tls-aws-lc` feature or install one manually."
+                .to_string(),
+        );
+        };


Does this work instead?

Suggested change

let provider = if let Some(p) = CryptoProvider::get_default() {

p.as_ref().clone()

} else {

return Err(

"No crypto provider installed. Enable `tls-aws-lc` feature or install one manually."

.to_string(),

);

};

let Some(provider) = CryptoProvider::get_default().cloned() else {

return Err(

"No crypto provider installed. Enable `tls-aws-lc` feature or install one manually."

.to_string(),

);

};

Applied the change manually. I was relying on rustfmt to auto-format it, but it doesn't seem to make any changes with the default configuration.

dfawley · 2026-02-24T21:56:34Z

+            .await
+            .map_err(|e| e.to_string())?;
+
+        let (_, connection) = tls_stream.get_ref();


I would like us to try to name the _ variables in situations like these so that we know what it is that we're ignoring, unless it's super obvious. _io? (What's the meaning of this API? There's two different ways to access the underlying stream?)

Renamed the ignored symbol to _io prefix.

What's the meaning of this API? There's two different ways to access the underlying stream?

The returned IO represents the plaintext IO stream that was passed to the connector.connect() method. The ClientConnection struct represents the TLS state machine. The TlsStream glues both these parts. It keeps reading data from the IO and passing it into to the ClientConnection.

dfawley · 2026-02-24T22:13:30Z

+    assert!(creds.is_err());
+    assert_eq!(
+        creds.err().unwrap(),
+        "Crypto provider has no cipher suites matching the security policy (TLS1.3 or TLS1.2+ECDHE)"


This might be brittle? Maybe contains("no cipher suites matching") or something would be less so?

Relaxed the assertion.

dfawley · 2026-02-24T22:16:41Z

+    let key_log_path = key_log_dir.join("grpc_rust_key_log.txt");
+
+    // Ensure file doesn't exist
+    if key_log_path.exists() {
+        std::fs::remove_file(&key_log_path).unwrap();
+    }


This pattern is problematic -- if you are running tests in different branches concurrently, e.g. We need something like mktemp where we don't have to see if the directory/file is already in use.

Nice catch! The rustls tests are running the keylog tests serially and using the CWD to create a temp file: permalink

The recommended way to create temporary directories seems to be using the tempfile crate, which tonic also seems to use:

tonic/tests/default_stubs/Cargo.toml

Lines 13 to 14 in fe0393c

[dev-dependencies]

tempfile = "3.20"

I've added a dev dependency and switched to using the tempfile crate.

dfawley · 2026-02-24T22:22:14Z

+// Note: This file has been modified by gRPC authors on 11th Feb 2026.
+// - Add ability to write logs to arbitrary path.


Is it a requirement to list all modifications here? Because I'd rather not since I'm sure we'll make a change on some future date and forget to update it. I would prefer something like "Note: this file contains modifications by the gRPC authors; see revision history for details" if that is acceptable.

Based on section 4.b of the liscence, the revision details are not required: https://www.apache.org/licenses/LICENSE-2.0

You must cause any modified files to carry prominent notices stating that You changed the files

I've updated the note as suggested.

dfawley · 2026-02-24T22:25:41Z

@@ -0,0 +1,117 @@
+// Copyright (c) 2016 Joseph Birr-Pixton <jpixton@gmail.com>
+//
+// Licensed under the Apache License, Version 2.0 (the "License");


I think we will want to put any third-party copyrighted files into a separate "vendored" directory, per guidance from legal. We can talk about the details later; this is probably OK for now, pre-1.0.

Tracking down all the files may be a problem later. I've moved this file to the src/vendored directory now.

After reading the CNCF guidelines for attribution, my understanding is that the keylog file qualifies as Incorporated Code instead of Vendored component since it's modified.

Incorporated code: portions of code that are taken from a third-party software component and incorporated directly into a CNCF project’s own source code

Vendored component: a component whose code is incorporated, unmodified, into a folder specifically intended for third-party dependencies, often with a name such as vendor/, third_party/, node_modules/ (for NPM dependencies), etc.

The guidelines for Incorporated Code only mention copying the license into source file for sources with an Apache 2 license.

Based on this, I've moved the keylog file back to the rustls directory.

dfawley · 2026-02-24T22:27:55Z

+        ///
+        /// This allows the consumer to observe the current value and await
+        /// future updates.
+        fn get_receiver(self) -> Receiver<T>;


With names so generic (Sender, Receiver) and reused so frequently, maybe we want to say they should always be prefixed by the module name? So, use tokio::sync::watch and watch::Receiver?

Changed to watch::Receiver.

tls client creds

bae4472

arjan-bal requested review from dfawley and sauravzg February 16, 2026 11:50

arjan-bal assigned dfawley and sauravzg Feb 16, 2026

Add session resumption test

ea358ee

sauravzg reviewed Feb 19, 2026

View reviewed changes

arjan-bal added 2 commits February 23, 2026 14:16

address review and clippy

62e5d4d

Merge remote-tracking branch 'source/master' into client-tls

a0c6591

arjan-bal requested a review from sauravzg February 23, 2026 08:50

dfawley reviewed Feb 24, 2026

View reviewed changes

dfawley assigned arjan-bal and unassigned dfawley Feb 24, 2026

arjan-bal added 2 commits February 25, 2026 16:29

address review

b269345

Merge remote-tracking branch 'source/master' into client-tls

2a1771e

arjan-bal force-pushed the client-tls branch from 61cbb74 to 2a1771e Compare February 25, 2026 11:07

Guard keylog with feature flag

2c6c4c2

arjan-bal assigned dfawley Feb 25, 2026

arjan-bal removed their assignment Feb 25, 2026

dfawley approved these changes Feb 26, 2026

View reviewed changes

arjan-bal added 2 commits February 27, 2026 14:39

Merge remote-tracking branch 'source/master' into client-tls

c3b0253

remove vendored dir and reformant imports

2aae498

arjan-bal merged commit 4195685 into hyperium:master Feb 27, 2026
21 checks passed

arjan-bal deleted the client-tls branch February 27, 2026 10:02

	/// Parse a PEM encoded X509 Certificate.
	///
	/// The provided PEM should include at least one PEM encoded certificate.
	pub fn from_pem(pem: impl AsRef<[u8]>) -> Self {
	let pem = pem.as_ref().into();
	Self { pem }
	}

		/// This should be used only for debugging purposes. It should never be
		/// used in a production environment due to security concerns.

		// Note: This file has been modified by gRPC authors on 11th Feb 2026.
		// - Add ability to write logs to arbitrary path.

Conversation

arjan-bal commented Feb 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arjan-bal Feb 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arjan-bal commented Feb 16, 2026 •

edited

Loading

arjan-bal Feb 23, 2026 •

edited

Loading

arjan-bal Feb 27, 2026 •

edited

Loading