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.

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.

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

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

codex-rs/core/src/default_client.rs

 use std::sync::Mutex;
 use std::sync::OnceLock;

+/// TLS configuration for HTTP clients


Should this also go in model_provider_tls_config.rs? Declaring it before this other stuff, (like the constant with the detailed docstring) doesn't feel like the right place.

@bolinfest I put this here because it's not strictly tied to model providers. It's possible to build and use a configured reqwest client that uses mTLS.

I'm guessing it'd be nice to make OTEL's http support use this client eventually too. It currently does its own thing.

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?

bolinfest · 2025-12-08T17:16:36Z

codex-rs/core/src/default_client.rs

+        path.clone()
+    } else {
+        // Resolve relative to $CODEX_HOME (defaults to ~/.codex)
+        let codex_home = std::env::var("CODEX_HOME")


Should never read env var inline: should read the codex_home field off of Config.

Checked how other parts in the codebase do this. Matched it.

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" ``` Paths can be absolute or relative to `~/.codex/`. 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" ```

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.

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 added 3 commits December 8, 2025 10:55

docs: document TLS/mTLS configuration for model providers

e0fb260

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

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!

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!

asm89 commented Dec 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Dec 5, 2025 •

edited

Loading