feat: trust extra CA bundles for corp gateways#84
Merged
Conversation
Promote the theme-loader's local `expand_tilde` into `util::path::expand_user` so `extra_ca_certs` can reuse it without duplicating tilde handling. Add `util::tls::load_extra_ca_certs` for appending a PEM bundle to a `reqwest` client; `rustls-tls` trusts only the baked-in Mozilla roots, so self-signed corporate endpoints need an explicit trust anchor.
Add `OX_EXTRA_CA_CERTS` env var plus `[client] extra_ca_certs = "..."` in `~/.config/ox/config.toml`. `~` and `~/` expand to `$HOME` at load time so Nix setups can write `~/.config/ox/...` paths without hard-coding a home. Treat the field as a trust-establishing setting and block it in project `ox.toml` alongside `api_key` and `base_url`. A checked-in corporate CA path there would let any repo widen TLS trust for the process.
…ilders Thread the resolved trust-anchor path into `Client::new` and the OAuth refresh client so both TLS surfaces honour it. `reqwest` with `rustls-tls` ships only the Mozilla webpki-roots bundle, so a self-signed corporate gateway would otherwise fail with `invalid peer certificate: UnknownIssuer` on every request. Resolve `extra_ca_certs` before the auth branch in `Config::load` so the OAuth refresh path can see it too. Under an SSL-inspecting corporate proxy the token endpoint is also terminated by the internal CA.
Add an "Extra CA Certs" row between "Base URL" and "Max Tokens" so `/config` shows the resolved trust-anchor path (tildified) or `(none)` when unset. Keeps trust state discoverable alongside the other TLS-relevant fields.
Document the new `client.extra_ca_certs` / `OX_EXTRA_CA_CERTS` knob in `docs/guide/configuration.md` with the corp-gateway use case and user-config-only caveat. Update CLAUDE.md so the util tree includes `tls.rs` and reflects the new `expand_user` entry on `path.rs`. Also normalise a stray U+2026 ellipsis in `slash::config` to the ASCII form per project punctuation rules.
Shorten the new `extra_ca_certs` row description to keep the `[client]` table aligned (markdownlint MD060). Whitelist `tildifies` and `cachain` so the /config test name and the corp-gateway PEM filename referenced in prose pass the spell gate.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Pins that a valid PEM bundle threads through add_root_certificate without error, complementing the missing-path error test. Promotes the hermetic test CA in util::tls to pub(crate) so both sites share one embedded cert.
Prior form read `dirs::home_dir()` and returned silently when HOME was unset, which green-lit CI runs that never exercised the tildify path. Pinning HOME via temp_env matches the pattern used in util/log tests and makes the assertion deterministic.
Collapses the duplicated "maybe load a PEM bundle, append each cert to the builder" loop from the Anthropic and OAuth-refresh clients into a single util::tls helper. Two copies hit the "extract after duplication appears" threshold. Call sites shrink from six lines to one and now read the trust-anchor concern as `apply_extra_ca_certs(builder, path)?`. Replaces inline crate::util::tls::... paths with a top-of-file use statement, matching the rest of each file.
Mirrors the anthropic client's \"failed to build HTTP client\" framing so a reqwest-level build error during OAuth refresh surfaces with an actionable label instead of a bare transport error.
Previously returned the raw `~`-prefixed string when `dirs::home_dir()` yielded None, leaving callers to fail downstream with misleading filesystem errors like "No such file or directory: ~/certs/corp.pem". Errors now cite the raw input so the user can tell that $HOME, not the path itself, is the problem. Tests pin HOME via temp_env so they stay deterministic across runners. The Linux unit-test body cannot exercise the None branch directly because dirs::home_dir() falls back to getpwuid_r; a comment records the limitation.
Pairs a happy-path test (valid PEM bundle threads through the builder and yields a parsed response) with an error test (unreadable bundle fails before any network call and cites the path). Closes the coverage gap where every previous refresh test passed None for extra_ca_certs, leaving the corp-proxy use case unexercised.
…ca_certs Relative paths in the TOML value resolve against \$PWD at the time ox is launched, not against the config file's directory. A user writing extra_ca_certs = \"./certs/corp.pem\" and launching ox from a repo root will get a \"failed to read extra CA bundle\" that looks like the file is missing. The note steers users toward absolute or tilde-rooted paths.
- Drop the X-not-Y antithesis ("must fail..., not silently fall back")
on new_surfaces_extra_ca_certs_error_with_path. Re-state the rule
directly.
- Replace the over-reaching "pins the happy-path wiring" comment with
an honest note about what the assertion actually covers and the
tradeoff that stopped us from asserting more.
- Rename new_appends_extra_ca_certs_to_the_trust_store to drop the
filler "the" so the name matches the terser sibling tests.
\"Not the config file's directory\" negated a strawman to imply the PWD rule; the direct form states the rule without the contrast.
- util/path.rs: em-dash in expand_user doc replaced with a comma (CLAUDE.md: em-dashes for true parentheticals only). - util/tls.rs: drop the third sentence of TEST_CA_PEM's doc; \`pub(crate)\` already communicates the sharing. - config.rs: \"Absolute path\" was inaccurate (expand_user can yield a relative path for a non-tilde input); \"Resolved path\" matches. Trim the ConfigSnapshot doc's redundant \"\`None\` when unset\" tail. - config/file.rs: semicolon in the trust-establishing-fields test comment split into two clauses; add a reminder on ClientConfig that new credential / endpoint / TLS-trust fields must also go through reject_project_secrets. - CLAUDE.md: path.rs tree entry separator aligned with neighbors (comma not semicolon).
- util/path.rs: pin that \`~//foo\` and \`~///foo/bar\` resolve under \$HOME.
PathBuf::join replaces the receiver on an absolute argument, so the
trim_start_matches('/') on the tail is load-bearing.
- config.rs: empty \`OX_EXTRA_CA_CERTS=\"\"\` falls through to the file
value (matches env::string's empty-is-absent semantics and mirrors
the existing load_file_api_key_used_when_env_is_empty test).
- util/tls.rs: split load_extra_ca_certs_reports_filename_on_read_and_parse_failures
into per-branch tests so a regression on either can surface
independently.
hakula139
force-pushed
the
feat/extra-ca-certs
branch
from
2df9e59 to
519d8db
Compare
…roduction CLAUDE.md: \"Section order must mirror the production function order in the same file.\" - config.rs: Config::load resolves extra_ca_certs (308) before auth (314), base_url (329), and effort (340). Tests now run in that order: extra_ca_certs -> base URL validation -> effort resolution -> prompt_cache_ttl. - config/file.rs: load_project_file (170) precedes load_file (205). Test sections swapped to match. Pure reorder, no test bodies change.
Old cert expired 2036-05-10. Suite would start failing in 10 years for a reason unrelated to the code under test. New cert is a P-256 self-signed throwaway with 2126-04-19 expiry. Also notes the validity in the docstring so a future reader does not have to re-derive it.
hakula139
force-pushed
the
feat/extra-ca-certs
branch
from
519d8db to
dbee26d
Compare
CLAUDE.md asks for test names phrased by scenario (`..._trusts_them`) rather than mechanism (`..._appends_to_trust_store`), since the latter describes internal reqwest builder wiring instead of observable behaviour. Also drop the semicolon in the doc comment — CLAUDE.md phrasing rules prefer a period or transition word, reserving semicolons for two independent clauses that genuinely belong as one thought.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the
invalid peer certificate: UnknownIssuererror whenANTHROPIC_BASE_URLpoints at a corporate gateway fronted by a private CA. reqwest withrustls-tlstrusts only the baked-in Mozilla webpki-roots bundle, so a self-signed chain never validates without an explicit trust-anchor append. The newclient.extra_ca_certsfield (mirrored byOX_EXTRA_CA_CERTS) takes a PEM bundle path and threads it into both the Anthropic streaming client and the OAuth refresh client, mirroring howNODE_EXTRA_CA_CERTSworks for the upstream Node-based Claude Code CLI.Design decisions
ox.toml. Trust anchors sit in the same bucket asapi_keyandbase_url: a checked-in path could widen TLS trust for the process or enable abase_urlredirect to MITM territory with the attacker's own CA. Project-levelclient.extra_ca_certsis rejected at load time alongside the other two.-----BEGIN CERTIFICATE-----blocks.NODE_EXTRA_CA_CERTS,SSL_CERT_FILE,REQUESTS_CA_BUNDLEall accept one path. Keeps TOML ergonomics simple and the env var shape obvious. If multi-path support is ever needed we can addserde(untagged)without breaking compat.add_root_certificatestacks on top of the default trust store, so public CAs likeapi.anthropic.comkeep working. Swapping inrustls-native-certsor switching tonative-tlswould change TLS semantics for every user and isn't justified by this use case.platform.claude.comtoken refresh too. ReorderingConfig::loadkeeps both TLS surfaces consistent without threading the field through more call sites than necessary.expand_tildeinto a sharedutil::path::expand_user. The helper now covers~,~/foo, non-tilde paths, and (deliberately) leaves~alice/…unexpanded to avoid a passwd-lookup dep.extra_ca_certsis the second call site, and future path-typed config fields can reuse it.util/tls.rsembeds a self-signed P-256 cert generated once withopenssl req -x509 -newkey ec:…so the suite never touches the network or filesystem for a fresh cert. Merged the single-block and multi-block happy-path tests into one parametric case, and merged the missing-path and malformed-PEM cases into a single "errors surface the filename" test.Changes
util.rs,util/tls.rsload_extra_ca_certs(path) -> Result<Vec<Certificate>>parses a PEM bundle, surfaces a clear error on missing / malformed / empty input, and pins the filename in the error message. Embedded test cert keeps the suite hermetic.util/path.rs,tui/theme/loader.rsexpand_tildetoutil::path::expand_user(handles~,~/foo, non-tilde passthrough,~aliceuntouched). Theme loader now calls the shared helper. Duplicateexpand_tilde_*tests dropped in favor of the broader coverage inutil::path.config.rs,config/file.rsextra_ca_certs: Option<PathBuf>onConfigandConfigSnapshot, populated fromclient.extra_ca_certsorOX_EXTRA_CA_CERTSand~-expanded at load time.reject_project_secretsextended to block the field inox.toml. Matchingload_extra_ca_certs_*test section and extendedsnapshot_*coverage.client/anthropic.rsClient::newappends certs fromconfig.extra_ca_certsto the reqwest builder viaadd_root_certificate. Newnew_surfaces_extra_ca_certs_error_with_pathpins that a bad path fails the build rather than silently falling back to native roots.config/oauth.rsload_token/load_token_from/refresh_oauth_tokentakeOption<&Path>; the refresh client's reqwest builder appends certs when supplied. Test call sites updated to passNone.client/anthropic/testing.rs,slash.rs,tui/app.rs,tui/components/welcome.rsConfig/ConfigSnapshotaddextra_ca_certs: None.slash/config.rs/configmodal gains an "Extra CA Certs" row between "Base URL" and "Max Tokens" that renders the tildified path or(none). Height anchor test updated; two new render tests pin the placeholder and tilde-rewrite branches.docs/guide/configuration.mdextra_ca_certsrow in the[client]table and env-var table, plus a short prose subsection covering the corp-gateway use case and user-config-only caveat. Precedence rule updated to include the field in the project-ox.tomlrejection list.CLAUDE.mdutil/tls.rsand notes theexpand_useraddition onpath.rs..cspell/words.txttildifies(new test name) andcachain(agenix secret convention for CA chain bundles).Test plan
cargo fmt --all --checkcargo buildcargo clippy --all-targets -- -D warnings: zero warningscargo test: 2016 tests passcargo llvm-cov --ignore-filename-regex 'main\.rs': 98.61% line coveragepnpm lintpnpm spellcheck