M2: CA persistence + Keychain trust + system proxy toggle#73
Open
kalil0321 wants to merge 3 commits into
Open
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f2389c06c
ℹ️ 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".
Comment on lines
+35
to
+36
| "\(networksetup) -setwebproxystate \(q) off", | ||
| "\(networksetup) -setsecurewebproxystate \(q) off", |
There was a problem hiding this comment.
In environments where a network service already has an HTTP or HTTPS proxy configured (for example a corporate proxy), enable() overwrites the proxy host/port for every service and the signal cleanup later calls disable(), which only turns both proxy states off. Exiting rae-proxy --enable-system-proxy therefore leaves those users with their original proxy disabled instead of restoring their previous host/port/enabled values; snapshot the -getwebproxy and -getsecurewebproxy state before mutating and restore it here.
Useful? React with 👍 / 👎.
Comment on lines
+15
to
+17
| if args.contains("--reset-ca") { | ||
| try store.reset() | ||
| print("CA reset.") |
There was a problem hiding this comment.
When --reset-ca is used after the CA has previously been installed with --install-ca-trust, this deletes only the app's certificate file and private key, then immediately creates a new root. The old root certificate and its user trust settings remain in the Keychain because CertificateTrustInstaller.uninstall is never called before store.reset(), so repeated resets accumulate stale trusted MITM roots; load the existing DER and remove its trust settings before deleting the stored CA.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
5 issues found across 5 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
This is likely a transient issue. You can re-trigger a run from the dashboard. |
kalil0321
pushed a commit
that referenced
this pull request
May 19, 2026
Fixes for PR #73 review comments (greptile, cubic, codex): - CAStore.loadOrCreate: only fall through to creation when the cert truly does not exist (silent CA rotation on transient keychain errors was the worst-case symptom). New explicit error cases (missingCertificateOnDisk, missingPrivateKeyInKeychain, etc.) and reset() now propagates real failures. - CertificateTrustInstaller.isInstalled: actually inspect the trust settings array and only return true when a kSecTrustSettingsResult == trustRoot entry exists. Previously any trust settings (including "deny") were reported as installed. - SystemProxyController: * shell-quote the host argument and validate against shell metacharacters before composing the admin shell command — the host was previously interpolated raw into a command run with administrator privileges * validate the port against 1..65535 before doing anything * isEnabled now requires BOTH HTTP and HTTPS proxy to be on * snapshot() / restore() helpers + ProxyServiceSnapshot record so callers can preserve user-configured proxies * parse() helper for -getwebproxy output, exposed for tests - rae-proxy --reset-ca: uninstall the existing CA's user trust settings before deleting the on-disk material, so resets do not accumulate stale trusted MITM roots - rae-proxy --enable-system-proxy: roll back system proxy + engine state when enable() fails partway - rae-proxy SignalCleanup: call signal(sig, SIG_IGN) BEFORE source.resume() to close the race where the default handler can fire between resume() and the SIG_IGN call Tests (SystemProxyControllerTests): - shellQuote escapes single quotes and shell metacharacters - parse(getWebProxyOutput:) handles enabled / disabled / extra whitespace - enable rejects shell metacharacters, invalid ports, empty host
Owner
Author
|
@greptile review Generated by Claude Code |
Contributor
There was a problem hiding this comment.
4 issues found across 5 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
kalil0321
pushed a commit
that referenced
this pull request
May 19, 2026
Fixes for PR #73 review comments (greptile, cubic, codex): - CAStore.loadOrCreate: only fall through to creation when the cert truly does not exist (silent CA rotation on transient keychain errors was the worst-case symptom). New explicit error cases (missingCertificateOnDisk, missingPrivateKeyInKeychain, etc.) and reset() now propagates real failures. - CertificateTrustInstaller.isInstalled: actually inspect the trust settings array and only return true when a kSecTrustSettingsResult == trustRoot entry exists. Previously any trust settings (including "deny") were reported as installed. - SystemProxyController: * shell-quote the host argument and validate against shell metacharacters before composing the admin shell command — the host was previously interpolated raw into a command run with administrator privileges * validate the port against 1..65535 before doing anything * isEnabled now requires BOTH HTTP and HTTPS proxy to be on * snapshot() / restore() helpers + ProxyServiceSnapshot record so callers can preserve user-configured proxies * parse() helper for -getwebproxy output, exposed for tests - rae-proxy --reset-ca: uninstall the existing CA's user trust settings before deleting the on-disk material, so resets do not accumulate stale trusted MITM roots - rae-proxy --enable-system-proxy: roll back system proxy + engine state when enable() fails partway - rae-proxy SignalCleanup: call signal(sig, SIG_IGN) BEFORE source.resume() to close the race where the default handler can fire between resume() and the SIG_IGN call Tests (SystemProxyControllerTests): - shellQuote escapes single quotes and shell metacharacters - parse(getWebProxyOutput:) handles enabled / disabled / extra whitespace - enable rejects shell metacharacters, invalid ports, empty host
kalil0321
force-pushed
the
claude/proxy-monitor-m2-ca-keychain
branch
from
e78f8e9 to
fbf9bd7
Compare
Owner
Author
|
@greptile review Generated by Claude Code |
Comment on lines
+17
to
+26
| if args.contains("--reset-ca") { | ||
| if store.exists() { | ||
| if let existing = try? store.load() { | ||
| let existingDER = Data(try existing.derBytes()) | ||
| if installer.isInstalled(derBytes: existingDER) { | ||
| try installer.uninstall(derBytes: existingDER) | ||
| } | ||
| } | ||
| } | ||
| try store.reset() |
Contributor
There was a problem hiding this comment.
store.exists() returns false (it requires both to be present), so the entire trust-uninstall block is skipped. store.reset() then removes the cert file, leaving the old CA permanently installed in the user trust store with no backing files. Subsequent --install-ca-trust runs add a new cert alongside the orphaned trusted one. Uninstalling trust only needs the DER bytes from the cert file — not the private key — so reading certificateURL directly sidesteps this gap.
Suggested change
| if args.contains("--reset-ca") { | |
| if store.exists() { | |
| if let existing = try? store.load() { | |
| let existingDER = Data(try existing.derBytes()) | |
| if installer.isInstalled(derBytes: existingDER) { | |
| try installer.uninstall(derBytes: existingDER) | |
| } | |
| } | |
| } | |
| try store.reset() | |
| if args.contains("--reset-ca") { | |
| if FileManager.default.fileExists(atPath: store.certificateURL.path), | |
| let existingDER = try? Data(contentsOf: store.certificateURL) { | |
| if installer.isInstalled(derBytes: existingDER) { | |
| try installer.uninstall(derBytes: existingDER) | |
| } | |
| } | |
| try store.reset() |
Prompt To Fix With AI
This is a comment left during a code review.
Path: macos/Sources/rae-proxy/main.swift
Line: 17-26
Comment:
When the keychain item is missing but the cert file still exists, `store.exists()` returns `false` (it requires both to be present), so the entire trust-uninstall block is skipped. `store.reset()` then removes the cert file, leaving the old CA permanently installed in the user trust store with no backing files. Subsequent `--install-ca-trust` runs add a new cert alongside the orphaned trusted one. Uninstalling trust only needs the DER bytes from the cert file — not the private key — so reading `certificateURL` directly sidesteps this gap.
```suggestion
if args.contains("--reset-ca") {
if FileManager.default.fileExists(atPath: store.certificateURL.path),
let existingDER = try? Data(contentsOf: store.certificateURL) {
if installer.isInstalled(derBytes: existingDER) {
try installer.uninstall(derBytes: existingDER)
}
}
try store.reset()
```
How can I resolve this? If you propose a fix, please make it concise.
kalil0321
pushed a commit
that referenced
this pull request
May 19, 2026
…sertions Addresses additional review comments on PR #73: - main.swift --reset-ca: read the certificate DER directly from store.certificateURL instead of going through store.exists() + store.load(). The previous code skipped trust uninstall whenever the keychain private key was missing but the .cer file still existed, leaving an orphaned trusted CA in the user trust store after reset (r3266534877). - main.swift SignalCleanup: snapshot proxy state with systemProxy.snapshot() before calling enable(), and restore it in the cleanup closure instead of disable()ing unconditionally. Falls back to disable() only when no snapshot could be captured. Preserves any pre-existing user / corporate proxy (r3266475870, follow-up to chatgpt-codex-connector comment). - SystemProxyControllerTests: assert SystemProxyError.invalidHost / invalidPort on validation failures so the tests do not silently pass on unrelated shell errors (cubic comments r3266396504, r3266396510, r3266396519).
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
- CAStore: persists root cert to Application Support, private key to the user Keychain as a generic password (PEM-encoded) - CertificateTrustInstaller: SecItem + SecTrustSettings (user domain) for install / uninstall / status - SystemProxyController: networksetup wrapped via AppleScript 'with administrator privileges' for HTTP + HTTPS proxy on every enabled network service - ProxyEngine.bootstrap(applicationSupportURL:) helper for app and CLI - rae-proxy CLI: --install-ca-trust, --enable-system-proxy, --reset-ca flags + signal-driven cleanup that restores network state on Ctrl-C
Fixes for PR #73 review comments (greptile, cubic, codex): - CAStore.loadOrCreate: only fall through to creation when the cert truly does not exist (silent CA rotation on transient keychain errors was the worst-case symptom). New explicit error cases (missingCertificateOnDisk, missingPrivateKeyInKeychain, etc.) and reset() now propagates real failures. - CertificateTrustInstaller.isInstalled: actually inspect the trust settings array and only return true when a kSecTrustSettingsResult == trustRoot entry exists. Previously any trust settings (including "deny") were reported as installed. - SystemProxyController: * shell-quote the host argument and validate against shell metacharacters before composing the admin shell command — the host was previously interpolated raw into a command run with administrator privileges * validate the port against 1..65535 before doing anything * isEnabled now requires BOTH HTTP and HTTPS proxy to be on * snapshot() / restore() helpers + ProxyServiceSnapshot record so callers can preserve user-configured proxies * parse() helper for -getwebproxy output, exposed for tests - rae-proxy --reset-ca: uninstall the existing CA's user trust settings before deleting the on-disk material, so resets do not accumulate stale trusted MITM roots - rae-proxy --enable-system-proxy: roll back system proxy + engine state when enable() fails partway - rae-proxy SignalCleanup: call signal(sig, SIG_IGN) BEFORE source.resume() to close the race where the default handler can fire between resume() and the SIG_IGN call Tests (SystemProxyControllerTests): - shellQuote escapes single quotes and shell metacharacters - parse(getWebProxyOutput:) handles enabled / disabled / extra whitespace - enable rejects shell metacharacters, invalid ports, empty host
…sertions Addresses additional review comments on PR #73: - main.swift --reset-ca: read the certificate DER directly from store.certificateURL instead of going through store.exists() + store.load(). The previous code skipped trust uninstall whenever the keychain private key was missing but the .cer file still existed, leaving an orphaned trusted CA in the user trust store after reset (r3266534877). - main.swift SignalCleanup: snapshot proxy state with systemProxy.snapshot() before calling enable(), and restore it in the cleanup closure instead of disable()ing unconditionally. Falls back to disable() only when no snapshot could be captured. Preserves any pre-existing user / corporate proxy (r3266475870, follow-up to chatgpt-codex-connector comment). - SystemProxyControllerTests: assert SystemProxyError.invalidHost / invalidPort on validation failures so the tests do not silently pass on unrelated shell errors (cubic comments r3266396504, r3266396510, r3266396519).
kalil0321
force-pushed
the
claude/proxy-monitor-m2-ca-keychain
branch
from
6a29aaf to
1d3b348
Compare
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
2 participants
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.
Stacked on top of #72.
What's in
loadOrCreate/reset..user. First call triggers the system password prompt.networksetup -listallnetworkservices, then toggles HTTP + HTTPS proxy on every enabled service. Admin escalation viaNSAppleScriptdo shell script … with administrator privileges.--install-ca-trust,--enable-system-proxy,--reset-ca, signal-driven cleanup.How to try
Generated by Claude Code
Summary by cubic
Adds persistent root CA storage, user-level Keychain trust install, and a macOS system proxy toggle with admin escalation. Improves safety with snapshot-based rollback and reliable cleanup on exit.
New Features
CAStore: persists root cert (Application Support) and private key (Keychain); load/create/reset.CertificateTrustInstaller: install/uninstall user trust via Security;isInstalledchecks fortrustRoot.SystemProxyController: admin-escalated HTTP/HTTPS proxy for all services;snapshot()/restore();isEnabledrequires both.ProxyEngine.bootstrap(...): builds the engine and exposes root DER/PEM helpers.rae-proxyCLI:--install-ca-trust,--enable-system-proxy,--reset-ca,--exit-after-install; cleanup restores prior proxy state (falls back to disable).Bug Fixes
host; validateport.--reset-ca: uninstall user trust using DER from disk even if the Keychain item is missing; then delete on-disk material.Written for commit 1d3b348. Summary will update on new commits. Review in cubic
Greptile Summary
This PR introduces persistent CA storage (
CAStore), user-level certificate trust installation (CertificateTrustInstaller), and a system proxy toggle (SystemProxyController) with admin escalation via AppleScript, wired together throughProxyEngine.bootstrapand a newrae-proxyCLI.loadOrCreatenow throws on load failure rather than silently rotating the CA.SecTrustSettingsSetTrustSettingson the.userdomain, andisInstalledcorrectly inspects the returnedCFArrayfor atrustRootentry.networksetupcommands.--reset-catrust uninstall is gated onstore.exists(), which silently skips theuninstallcall whenever the cert file is present but the keychain item is missing.Confidence Score: 4/5
Mostly safe to merge; one defect in the --reset-ca path can leave an orphaned trusted CA in the user trust store when the keychain item has been independently removed.
The --reset-ca path gates the trust-uninstall call on store.exists(), which requires both the cert file and the keychain item to be present. If the keychain item is gone, the uninstall is skipped and the old CA remains trusted indefinitely after reset.
macos/Sources/rae-proxy/main.swift — the --reset-ca block at lines 17-28
Important Files Changed
Prompt To Fix All With AI
Reviews (3): Last reviewed commit: "M2 review fixes + tests" | Re-trigger Greptile