-
Notifications
You must be signed in to change notification settings - Fork 7.1k
fix: add custom CA certificate support for login flows #6864
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
@codex review |
There was a problem hiding this comment.
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.
ℹ️ 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".
|
@codex review |
1 similar comment
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@3axap4eHko, thanks for the contribution! It looks like there are some minor code format issues. Please fix those when you have time. |
|
@3axap4eHko, the PR is looking good. We'll need to document the new environment variables. Ideally, this should be part of the same PR. Do you want to take a shot at adding the documentation? |
joshka-oai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brief review notes / questions to help work out whether to do a deeper review.
There was a problem hiding this comment.
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.
ℹ️ 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".
|
I'm assuming that your environment sets some sort of CA root authority that your general tools (browser etc.) properly check, so you don't tend to get errors in non-cli tooling. Is this something that we could generally load from the environment in the same way without having to manually configure this with environment variables? I.e. perhaps configuring the tooling to load from the OS Cert store where this is setup. There's probably a crate that sets this up automatically if that's the case. I'm not sure whether this is relevant to the use cases here, so please forgive the naive question. |
Totally fair question. Out of the box, reqwest already trusts the platform store (via either system OpenSSL/Schannel/Security‑Framework on the blocking TLS stack or rustls-native-certs when using the rustls backend). That’s the reason browsers and “normal” CLI calls don’t need extra configuration. The new code only kicks in when you explicitly point CODEX_CA_CERTIFICATE or SSL_CERT_FILE at a custom bundle, because corporate proxies often publish their own root chain that isn’t in the OS store. So, for users whose proxy installs its cert into the regular Windows/macOS/Linux trust store, nothing special is required—the default client still works. The env knob is just for the cases where IT hands you a standalone PEM bundle instead of touching the OS store. In that scenario you still need manual configuration somewhere, either by importing the PEM into the OS trust store (which is painful to automate across platforms) or by telling the app exactly which file to add. The helper we added is that second option. If we ever run into a platform where the OS store is available but reqwest/rustls can’t discover it, the fix would likely be as simple as enabling the rustls-native-certs crate (or the native-tls backend) for that target. But so far there hasn’t been a report that the default trust roots are being ignored; every bug we’ve seen came from proxy-issued certs that the OS store rightfully doesn’t trust. |
|
@3axap4eHko, I wanted to give you an update on this PR. Since it involves security-sensitive code, we have pulled in some security experts outside of the codex team to provide a more thorough code review. This is a holiday week in the U.S. (Thanksgiving), so many OpenAI employees are out of the office. That means we probably won't make any further progress on this until next week. |
|
After chatting about this with our internal security, we'd prefer to have the settings for this added to the config.toml file rather than as an environment variable. If you're concerned that you need single call setups (where you don't want to set this at the entire machine level) codex has a Let's use rustls-pki-types to open / parse the pem files rather thank hand coded. Happy to continue the implementation on this with codex, or let you complete it. The things I'd generally want to make sure are covered here:
|
@joshka-oai One thing I want to push back on in the "after chatting with internal security" part: environment-based CA configuration is a very common pattern on Unix. If the Codex policy is "all TLS-related overrides must live in config.toml and we intentionally ignore |
|
I'm in general agreement with you, but I'm not sure if there's things I don't know about this yet. |
|
I clarified with our Security team. We're fine with using environment variables on this. I misread a recommendation in a message. The main part of getting this over the line is testing this correctly in a way that doesn't cause other usages of this code path to be impacted. Leaving the ball with you for now to take a look, but feel free to ask more questions or kick it back to us if you're not sure of the right direction on this. |
Wraps env::set_var and env::remove_var calls in unsafe blocks as required by newer Rust versions. Also updates test certificate to valid RSA format and adds better error messages to test assertions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
caf76cc to
2eac899
Compare
|
@joshka-oai Oh thanks for helping, I didn't expect that. I was a bit off for holidays |
|
Rebased on main, and got codex to implement the necessary parts of this that
No problem - take a quick look to see if this change still solves your problem in real testing if you have a few moments. |
b5feeb4 to
16e15ce
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Quick clarification on what happened here, since this is a workflow not everyone runs into: As a maintainer, I have push access to contributor branches. I used that access to rebase your branch on current
After that, the branch was force‑pushed again and those maintainer changes were overwritten. So the PR currently doesn’t include the rewrite. To get us back to the version that includes our fixes, can you please reset your local branch back to the commit where that work landed, test it, and start any new changes from there: git fetch your-remote-name (e.g. origin)
git checkout ISSUE-6849
git reset --hard 2eac899ac7c43157c648c9cbb2b9825940daeab7
# run your usual tests
git push --forceMy main goal here is to ensure that you've tested the changes I made to this in your environment with a real TLS cert that is setup for this. Once you’ve done that, any additional changes should be made on top of that commit (2eac899) so we don’t lose the rewrite again. If anything in the force‑push after that point was intentional and should be preserved, let me know and I’ll help fold it back in. As a side note, I generally avoid merging from main except right before merging as it makes it difficult to follow the history of the commits. I rebased this PR to avoid that problem and start us off from a good point. We squash merge all our PRs so having the merge commits in there is not a big issue, but don't worry too much about getting the absolute latest code from main all the time. |
Enterprise TLS inspection proxies use custom roots, so OAuth token exchanges fail when we only trust system CAs. This change switches PEM parsing to rustls-pki-types (multi-cert bundles included) and surfaces clearer, user- facing errors that explain how to fix invalid or empty CA files via CODEX_CA_CERTIFICATE/SSL_CERT_FILE. To avoid cross-test races with process-wide env vars, CA path selection now uses a small EnvSource abstraction in unit tests, and environment-dependent behavior is verified via an assert_cmd-driven login_ca_probe helper binary. This keeps normal tests isolated while still validating env precedence and error messaging. Also updates login dev-deps (assert_cmd/pretty_assertions), removes serial_test, and re-exports build_login_http_client for the probe helper.
13ead7c to
a8831a7
Compare
|
Sorry for the churn here — this one ended up more confusing than it should have, and that’s on me. Very important workflow note for this PR (please read once): 🚫 Please do NOT merge I’m intentionally keeping this branch rebased as a maintainer so we can reason about the changes cleanly. When I’ve force-pushed the branch back to a canonical state that includes:
From here on, please treat the current tip of the branch as the source of truth. What I need from you (low effort, no Git surgery)If you have a few minutes, please sanity-check the current branch in your real environment (your actual CA / proxy setup) and confirm:
That confirmation is the main thing blocking this from landing. If you do need to make changes locallyTo make sure you’re working from the correct base and don’t accidentally overwrite things, the safest flow is: git fetch origin
git checkout ISSUE-6849
git reset --hard origin/ISSUE-6849Then:
If something from your earlier commits was intentional and got lost in the rewrite, just say so — I’m happy to help fold it back in. I think the main missing commit in your work on this is 181c08b Thanks for sticking with this, and sorry again for the workflow back-and-forth. Once we confirm it works for you, we should be in good shape to wrap this up. |
|
I added back in the docs commit, which got lost in the shuffle. |
|
@joshka-oai is it safe to pick up the current branch state, and work based on it? |
|
Yes. Go for it. |
|
Can you please add some extra justification for the new changes? E.g. references to standards / documentation of the specific firewalls that are adding the new parts of this added in 299047b and 77e87f0 If I'm reading between lines here, you tested this in your environment and found that you needed extra config to align. It would be helpful to make sure we capture the rationale here rather than just the changes. I see that rustls intentionally decided not to support pemfiles with "BEGIN TRUSTED CERTIFICATE" in them over in rustls/pemfile#52 I also see that rustls has pem file certs that have CRLs (but can't see any obvious parsing for this). I'd prefer all the parsing code for pem files to be in the library where possible. Doing anything with funky parsing of certs requires every developer that modifies that code to dig into the details of how the parsing should work, what specs it meets, etc. Pushing those decisions off to a library instead of manually implementing it allows the wisdom of developers more knowledgable about such things to be applied. Anything outside of the absolutely normal parsing of these files needs to either be well justified or use libraries in their normal form. |
|
@joannas-oai Of course, here’s the rationale behind the two parsing tweaks and a path to make them less bespoke: Why we normalized “TRUSTED CERTIFICATE”
Why we stripped CRL blocks
If we want to reduce ad-hoc parsing
Let me know if you’d prefer: |
|
Ok, it sounds like the TRUSTED prefix seems reasonable to think about. My concern is that several upstream things have decided to intentionally not support that. How about we split the PR at this on this part to a second PR? Same deal with CRL - is it possible to make the use of rustls-pki-types tolerant of the CRL without having to remove it upfront? In generaly, we shouldn't want to switch back to rustls-pemfile or generally do much ad-hoc parsing / modification of the certs.
|
Signed-off-by: Ivan Zakharchanka <[email protected]>
|
@codex review |
There was a problem hiding this comment.
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.
ℹ️ 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".
|
@joshka-oai is there something here that needs to be addressed? |
|
Hey there, I haven't forgotten about this. Thanks for hanging in there and putting up with the delay on this. It's just below my threshold for where I can allocate time to get this over the line, and the majority of the team has been taking a break over the holiday period. I'll talk with a few people early next week to get some extra eyes on this. Something to note is the changes coming in reqwest 0.13, which moves to using rustls by default and has some extra things around merging tls certs. I haven't looked into that change yet to see if there's overlap on this, but if there is, then we likely should upgrade reqwest first and then rebase this on top of those newer approaches. |
I'll review changes in reqwest |
|
@joshka-oai I confirm, upgrade will allow to get rid of some crates and reduce amount of code |

What
Adds support for custom CA certificates in OAuth login flows to fix authentication failures behind corporate
proxies with SSL/TLS inspection.
Why
The Codex CLI fails to authenticate when running behind corporate proxies that perform SSL/TLS inspection with
custom CA certificates. The OAuth token exchange fails because
reqwest::Client::new()only trusts systemdefault certificate roots, causing it to reject connections intercepted by corporate proxies presenting
certificates signed by custom CAs.
This affects all Codex CLI users in enterprise environments with:
Fixes #6849
How
Implementation:
build_login_http_client()function that creates an HTTP client with custom CA certificate supportCODEX_CA_CERTIFICATE(primary)SSL_CERT_FILE(fallback, standard across many tools)exchange_code_for_tokens()(OAuth code exchange)obtain_api_key()(API key exchange)run_device_code_login()(device code flow)otel_provider.rsimplementationTesting:
CODEX_CA_CERTIFICATEenv varSSL_CERT_FILEfallbackserial_testdependency to safely test env var manipulationUsage:
export CODEX_CA_CERTIFICATE=/path/to/corporate-ca.pem codex loginor using the standard env var:
export SSL_CERT_FILE=/path/to/corporate-ca.pem codex loginTesting: