feat: add mTLS support for model providers #7650

asm89 · 2025-12-05T23:13:13Z

Adds mutual TLS (mTLS) authentication support for model provider connections, enabling Codex to connect to endpoints requiring client certificates.

Ensure rustls-only TLS backend across the workspace
Add tls config block for CA and client certificates
Support reading cert paths from environment variables
Add just check-tls to verify no native-tls in dependency tree

github-actions · 2025-12-05T23:13:22Z

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

asm89 · 2025-12-05T23:13:42Z

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

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

chatgpt-codex-connector · 2025-12-05T23:18:09Z

codex-rs/core/src/default_client.rs

+            Err(e) => {
+                tracing::error!("Failed to apply TLS configuration: {}", e);
+                // Fall back to base builder without TLS
+                create_base_client_builder()


Fail mTLS client build instead of falling back silently

When apply_tls_config returns an error (e.g., a missing/ unreadable client certificate or key), build_configured_reqwest_client logs and reconstructs a default client instead of surfacing the failure. That means requests continue without the configured client identity or CA; on servers where client certificates are optional, mTLS is silently skipped rather than failing fast, undermining the security guarantee users expect when they supply certs. The code should propagate the error instead of returning a fallback client.

Useful? React with 👍 / 👎.

As noted in the commit description, the existing code already had a fallback on builder failures.

That said, agreed. Making create_default_client() fallible is a bigger change across the codebase however. So hopeful these fixes can be shipped separately?

Perhaps failing harder is okay for the time being?

I agree feeling harder seems like the right choice. Is there any commentary on the PR that introduced the original behavior to explain why it didn't do this before?

@bolinfest I traced it back to #3110 through a couple of refactors and other changes. Not much there.

It'd require updating the callsites to handle the error. Doesn't look too tricky to move that error one way up the stack. Not sure how the UI etc would deal with it.

etraut-openai · 2025-12-06T00:27:40Z

Thanks for the contribution. We've updated our contribution guidelines to clarify that we're currently accepting contributions for bugs and security fixes, but we're not generally accepting new features at this time. We need to make sure that all new features compose well with both existing and upcoming features and fit into our roadmap. If you would like to propose a new feature, please file or upvote an enhancement request in the issue tracker. We will generally prioritize new features based on community feedback.

bolinfest

Can you please also update https://github.com/openai/codex/blob/main/docs/config.md as part of this PR?

codex-rs/core/src/model_provider_info.rs

codex-rs/core/src/default_client.rs

bolinfest · 2025-12-08T17:15:13Z

codex-rs/core/src/default_client.rs

+            Err(e) => {
+                tracing::error!("Failed to apply TLS configuration: {}", e);
+                // Fall back to base builder without TLS
+                create_base_client_builder()


I agree feeling harder seems like the right choice. Is there any commentary on the PR that introduced the original behavior to explain why it didn't do this before?

codex-rs/core/src/default_client.rs

asm89 · 2025-12-08T19:58:20Z

@bolinfest Two thoughts/questions:

Would it be ok to follow-up with removing the infallible client construction? I can do it, or perhaps someone else.
The config loading in this PR follows what I could find in other parts of the codebase (e.g. OTEL). Instead of reading codex_home etc at this point, it might be worthwhile resolving the configuration further when initially loading it. E.g. placing resolved paths etc in the config early in startup. This would allow failing early.

codex-rs/core/src/default_client.rs

bolinfest · 2025-12-09T22:06:45Z

docs/config.md


+#### TLS and mTLS configuration
+
+Model providers accept an optional `tls` block for custom CA certificates or mutual TLS. Relative paths are resolved against `~/.codex/`:


Given my comments above, it would be more future-proof to say that they are resolved against the path to the folder containing the config.toml file that defines this config.

codex-rs/core/src/default_client_config.rs

asm89 · 2025-12-09T23:40:32Z

@bolinfest How about I remove the relative path loading from this PR and only support absolute paths? I'm a bit worried about getting stuck rebasing this for a while. FWIW for our enterprise deployment I really need commit 1 (making mTLS actually work!) and env var based loading.

As for the path resolving as implemented in this PR. I tried to mirror what OTEL does, but you're right that it incorrectly always resolves against codex home at the moment. Not something relative to the config file. To properly support the latter, I think something closer to my comment above would be needed.

bolinfest · 2025-12-10T01:11:35Z

@asm89 Admittedly, getting the config stuff correct is tricky in its current form, but #7796 should fix things?

bolinfest · 2025-12-10T01:39:34Z

@asm89 #7796 is merged, so can you please rebase this PR on top and take advantage of it?

Also, what's the motivation behind supporting environment variables? Honestly, that's not something we do a lot of in Codex CLI, so I'm not keen to open that up.

bolinfest · 2025-12-10T01:40:04Z

Would it be ok to follow-up with removing the infallible client construction? I can do it, or perhaps someone else.

Yes.

The mTLS implementation was failing with "tlsv13 alert certificate required" errors despite certificates loading correctly. The root cause was Cargo's feature unification - when multiple crates depend on reqwest, Cargo enables the union of all requested features. Since some crates were using default features (which includes native-tls) and others specified rustls, both TLS backends were being compiled and linked, causing unpredictable behavior during TLS handshakes. The fix ensures rustls-only is used across the entire workspace by setting `default-features = false` and explicitly specifying `features = ["rustls-tls"]` for all reqwest dependencies. This is important because the mTLS code uses `Identity::from_pem()` which only exists with rustls. Added a verification script (`just check-tls`) that fails if openssl-sys appears in the dependency tree.

asm89 · 2025-12-10T20:06:54Z

@bolinfest

@asm89 #7796 is merged, so can you please rebase this PR on top and take advantage of it?

Yes, on it. Thank you.

Also, what's the motivation behind supporting environment variables? Honestly, that's not something we do a lot of in Codex CLI, so I'm not keen to open that up.

In our enterprise environments certificates are per user, but the systems with the managed configs won't be. We can't hardcode the user specific certificate paths into the managed config. For environments with mTLS requirements I don't think it's uncommon to have a setup like this.

Adds mutual TLS (mTLS) authentication support for model provider connections, allowing Codex to connect to endpoints requiring client certificates. The implementation uses rustls exclusively - native-tls is not supported due to Cargo's feature unification causing both TLS backends to be linked when any dependency enables native-tls. Configuration is done via TOML for each model provider: ```toml [model-providers.my-provider] base_url = "https://example.com" tls.ca-certificate = "/path/to/ca.pem" tls.client-certificate = "/path/to/client.pem" tls.client-private-key = "/path/to/key.pem" ``` Relative paths are resolved against the config file's directory during config loading via AbsolutePathBuf. All three TLS fields are optional - omit CA certificate to use system roots, omit client cert/key for server-only TLS. The implementation gracefully falls back to a non-TLS client if certificate loading fails (with error logging), ensuring connectivity isn't completely broken by misconfigured certificates. This matches the previous behavior on reqwest builder failures.

Adds support for specifying TLS certificate paths via environment variables in model provider configuration. Each certificate/key path field now has a corresponding `-env` variant that names an environment variable containing the path. New config fields: - `ca-certificate-env`: env var for CA certificate path - `client-certificate-env`: env var for client certificate path - `client-private-key-env`: env var for client private key path If both a direct path and env var are specified, the env var takes precedence (if set and non-empty). This follows the same pattern used by `env_http_headers` for HTTP headers in the codebase. Example config: ```toml [model_providers.my-provider.tls] ca-certificate-env = "MY_CA_CERT_PATH" client-certificate-env = "MY_CLIENT_CERT_PATH" client-private-key-env = "MY_CLIENT_KEY_PATH" ``` The value of the environment variable has to be an absolute path.

bolinfest · 2025-12-11T00:50:01Z

codex-rs/core/src/default_client.rs


+/// Create an HTTP client with optional TLS configuration.
+/// Optionally configure TLS/mTLS settings via the `tls_config` parameter.
+pub fn create_configured_client(tls_config: Option<&TlsConfig>) -> CodexHttpClient {


Where is this function used?

bolinfest · 2025-12-11T00:52:24Z

codex-rs/core/src/default_client.rs

+}
+
+pub fn build_configured_reqwest_client(tls_config: Option<&TlsConfig>) -> reqwest::Client {
+    let builder = create_base_client_builder();


Why do this and then immediately reassign?

bolinfest · 2025-12-11T00:52:58Z

codex-rs/core/src/default_client.rs

+    let builder = create_base_client_builder();
+
+    // Apply TLS configuration if provided
+    let builder = if let Some(tls) = tls_config {


I would favor match and in the None case do create_base_client_builder()

bolinfest · 2025-12-11T00:53:12Z

codex-rs/core/src/default_client.rs

+        match apply_tls_config(builder, tls) {
+            Ok(configured_builder) => configured_builder,
+            Err(e) => {
+                tracing::error!("Failed to apply TLS configuration: {}", e);


Suggested change

tracing::error!("Failed to apply TLS configuration: {}", e);

tracing::error!("Failed to apply TLS configuration: {e}");

bolinfest · 2025-12-11T00:53:32Z

codex-rs/core/src/default_client.rs

    let ua = get_codex_user_agent();

    let mut builder = reqwest::Client::builder()
-        // Set UA via dedicated helper to avoid header validation pitfalls


bolinfest · 2025-12-11T01:00:14Z

codex-rs/core/src/model_provider_tls_config.rs

+/// Returns None if the value is empty/whitespace, relative, or cannot be converted.
+/// The `source` parameter is used for warning messages (e.g., "env var MY_VAR").
+fn parse_absolute_path(value: &str, source: &str) -> Option<AbsolutePathBuf> {
+    use std::path::Path;


this can go at the top

bolinfest · 2025-12-11T01:05:50Z

codex-rs/core/src/model_provider_tls_config.rs

+fn parse_absolute_path(value: &str, source: &str) -> Option<AbsolutePathBuf> {
+    use std::path::Path;
+
+    let trimmed = value.trim();


I would prefer to be more strict here: don't trim (paths can, in theory, end in whitespace...), but if value.is_empty(), then return None. Do not perform your own is_absolute() check: let AbsolutePathBuf::from_absolute_path() decide whether to reject the input.

bolinfest · 2025-12-11T01:06:28Z

codex-rs/core/src/model_provider_tls_config.rs

+    {
+        return Some(path);
+    }
+
+    // Fall back to direct path (already absolute from deserialization)
+    direct.cloned()


Suggested change

{

return Some(path);

}

// Fall back to direct path (already absolute from deserialization)

direct.cloned()

{

Some(path)

} else {

// Fall back to direct path (already absolute from deserialization)

direct.cloned()

}

bolinfest · 2025-12-11T01:09:29Z

codex-rs/core/src/model_provider_tls_config.rs

+impl ModelProviderTlsConfig {
+    /// Convert to the TlsConfig type used by HTTP clients.
+    /// Environment variable values take precedence over direct path values.
+    pub fn to_tls_config(&self) -> TlsConfig {


In general, prefer impl From<ModelProviderTlsConfig> for TlsConfig, though admittedly it takes ModelProviderTlsConfig by value, but just clone() at the callsite.

bolinfest · 2025-12-11T01:11:32Z

codex-rs/core/src/model_provider_info.rs

+    /// Build a reqwest client configured for this model provider.
+    /// This extracts TLS configuration from the provider if present.
+    pub fn build_reqwest_client(&self) -> reqwest::Client {
+        let tls_config = self.tls.as_ref().map(ModelProviderTlsConfig::to_tls_config);


Something like:

Suggested change

let tls_config = self.tls.as_ref().map(ModelProviderTlsConfig::to_tls_config);

let tls_config = self.tls.map(|m| m.clone().into());

github-actions bot added a commit that referenced this pull request Dec 5, 2025

@asm89 has signed the CLA in #7650

6742394

chatgpt-codex-connector bot reviewed Dec 5, 2025

View reviewed changes

etraut-openai closed this Dec 6, 2025

bolinfest reopened this Dec 6, 2025

asm89 force-pushed the model-providers-mtls branch 3 times, most recently from d81b29e to 6370c79 Compare December 8, 2025 11:31

bolinfest reviewed Dec 8, 2025

View reviewed changes

asm89 force-pushed the model-providers-mtls branch from 6370c79 to e0fb260 Compare December 8, 2025 19:52

bolinfest reviewed Dec 9, 2025

View reviewed changes

etraut-openai added the needs-response Additional information is requested label Dec 9, 2025

asm89 added 3 commits December 10, 2025 13:49

docs: document TLS/mTLS configuration for model providers

38c7cfe

asm89 force-pushed the model-providers-mtls branch from e0fb260 to 38c7cfe Compare December 10, 2025 21:56

bolinfest requested changes Dec 11, 2025

View reviewed changes


		#### TLS and mTLS configuration

		Model providers accept an optional `tls` block for custom CA certificates or mutual TLS. Relative paths are resolved against `~/.codex/`:

	tracing::error!("Failed to apply TLS configuration: {}", e);
	tracing::error!("Failed to apply TLS configuration: {e}");

	let tls_config = self.tls.as_ref().map(ModelProviderTlsConfig::to_tls_config);
	let tls_config = self.tls.map(\|m\| m.clone().into());

feat: add mTLS support for model providers #7650

Are you sure you want to change the base?

feat: add mTLS support for model providers #7650

Conversation

asm89 commented Dec 5, 2025

Uh oh!

github-actions bot commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asm89 commented Dec 5, 2025

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Dec 5, 2025

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!

etraut-openai commented Dec 6, 2025

Uh oh!

bolinfest left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

asm89 commented Dec 8, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

asm89 commented Dec 9, 2025

Uh oh!

bolinfest commented Dec 10, 2025

Uh oh!

bolinfest commented Dec 10, 2025

Uh oh!

bolinfest commented Dec 10, 2025

Uh oh!

asm89 commented Dec 10, 2025

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Dec 5, 2025 •

edited

Loading