Repository navigation
feat(runner-server): configurable CA roots for outbound GitHub HTTPS - #354
Merged
Merged
Conversation
Pointing the engine at a GHES/emulator forge that serves its own CA (e.g. gh-simulate's generated CA) fails TLS verification on every server-side GitHub call — the clients were built ad-hoc with only the bundled roots. Add shared_http::ca_certificates() honoring PRELOOP_GITHUB_CA_FILE (forge- scoped, additive) and SSL_CERT_FILE (the OpenSSL/native convention the runner already honors in client::http::with_control). Apply via github_client_builder() to the shared CLIENT plus the ad-hoc builders in actions.rs, remote_workflows.rs, github_app.rs, and the snapshots LFS forge client, so custom roots reach every GitHub-facing call uniformly. Missing/unreadable/invalid bundles log a warning and add no roots — verification stays on; startup never aborts on a bad CA path. Refs #349
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 5 files
Confidence score: 3/5
crates/preloop-runner-server/src/actions.rs— The 30-second total timeout indownload_action_tarballcan interrupt slow or large GHES archive downloads, preventing those actions from completing; preserve a longer download timeout, as the LFS client does.crates/preloop-runner-server/src/shared_http.rs— This test can fail whereopensslis unavailable or pass using a stale fixed temp file, making test results unreliable; remove the unprovisioned CLI dependency and ensure the test uses a fresh, verified temporary file.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/preloop-runner-server/src/shared_http.rs">
<violation number="1" location="crates/preloop-runner-server/src/shared_http.rs:86">
P2: concern: This unit test depends on an unprovisioned `openssl` executable and ignores whether it succeeded, so the test fails on environments without that CLI (or can pass using a stale fixed temp file). Use a checked-in PEM fixture or an in-process certificate generator with unique temporary paths.</violation>
</file>
<file name="crates/preloop-runner-server/src/actions.rs">
<violation number="1" location="crates/preloop-runner-server/src/actions.rs:354">
P2: concern: `download_action_tarball` now applies a 30-second total timeout to streamed tarballs, so slow or large GHES archives fail mid-download. Preserve a longer download timeout here, as the LFS client does.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| fn write_self_signed_pem(path: &std::path::Path) { | ||
| let key = std::env::temp_dir().join("ghsim_ca_test_key.pem"); | ||
| let _ = std::process::Command::new("openssl").args([ |
There was a problem hiding this comment.
P2: concern: This unit test depends on an unprovisioned openssl executable and ignores whether it succeeded, so the test fails on environments without that CLI (or can pass using a stale fixed temp file). Use a checked-in PEM fixture or an in-process certificate generator with unique temporary paths.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At crates/preloop-runner-server/src/shared_http.rs, line 86:
<comment>concern: This unit test depends on an unprovisioned `openssl` executable and ignores whether it succeeded, so the test fails on environments without that CLI (or can pass using a stale fixed temp file). Use a checked-in PEM fixture or an in-process certificate generator with unique temporary paths.</comment>
<file context>
@@ -1,12 +1,113 @@
+
+ fn write_self_signed_pem(path: &std::path::Path) {
+ let key = std::env::temp_dir().join("ghsim_ca_test_key.pem");
+ let _ = std::process::Command::new("openssl").args([
+ "req","-x509","-newkey","rsa:2048","-keyout",
+ key.to_str().unwrap(),"-out",path.to_str().unwrap(),
</file context>
|
|
||
| let client = reqwest::Client::builder() | ||
| .user_agent("preloop-runner-server") | ||
| let client = crate::shared_http::github_client_builder() |
There was a problem hiding this comment.
P2: concern: download_action_tarball now applies a 30-second total timeout to streamed tarballs, so slow or large GHES archives fail mid-download. Preserve a longer download timeout here, as the LFS client does.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At crates/preloop-runner-server/src/actions.rs, line 354:
<comment>concern: `download_action_tarball` now applies a 30-second total timeout to streamed tarballs, so slow or large GHES archives fail mid-download. Preserve a longer download timeout here, as the LFS client does.</comment>
<file context>
@@ -351,8 +351,7 @@ pub async fn download_action_tarball(
- let client = reqwest::Client::builder()
- .user_agent("preloop-runner-server")
+ let client = crate::shared_http::github_client_builder()
.build()
.map_err(|e| ApiError::internal(format!("failed to build reqwest client: {e}")))?;
</file context>
Suggested change
| let client = crate::shared_http::github_client_builder() | |
| let client = crate::shared_http::github_client_builder() | |
| .timeout(std::time::Duration::from_secs(300)) |
The previous check run used the broken macstudio golden/provisioning state. No source files changed.
Bnjoroge1
added a commit
that referenced
this pull request
Oct 7, 2026
…354) * feat(runner-server): configurable CA roots for outbound GitHub HTTPS Pointing the engine at a GHES/emulator forge that serves its own CA (e.g. gh-simulate's generated CA) fails TLS verification on every server-side GitHub call — the clients were built ad-hoc with only the bundled roots. Add shared_http::ca_certificates() honoring PRELOOP_GITHUB_CA_FILE (forge- scoped, additive) and SSL_CERT_FILE (the OpenSSL/native convention the runner already honors in client::http::with_control). Apply via github_client_builder() to the shared CLIENT plus the ad-hoc builders in actions.rs, remote_workflows.rs, github_app.rs, and the snapshots LFS forge client, so custom roots reach every GitHub-facing call uniformly. Missing/unreadable/invalid bundles log a warning and add no roots — verification stays on; startup never aborts on a bad CA path. Refs #349 * style: format CA bundle helper * ci: rerun checks after shared engine recovery The previous check run used the broken macstudio golden/provisioning state. No source files changed. --------- Co-authored-by: Bnjoroge1 <Bnjoroge1@users.noreply.github.com>
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.
Part of #349 (gh-simulate prerequisites). Refs #349 — does not close it; sibling PRs cover PAT-over-http, remaining hardcoded hosts, and the probe off-switch.
Problem
Server-side GitHub calls are built with ad-hoc
reqwest::Client::builder()(and the sharedshared_http::CLIENT) that trust only the bundled webpki/native roots. Pointing the engine at a GHES / emulator forge that serves its own CA — gh-simulate generates one — makes every outbound call fail TLS verification. There was no way to extend trust for the forge.Change
crates/preloop-runner-server/src/shared_http.rs:ca_certificates()readsPRELOOP_GITHUB_CA_FILE(forge-scoped, additive) thenSSL_CERT_FILE(the OpenSSL/native convention the runner'sclient::http::with_controlalready honors), parses each as a PEM bundle, and returns the certs.github_client_builder()applies the shared timeouts/user-agent plus those roots — the single entry point for any outbound client that can target a forge.CLIENTbuilds on it, soPRELOOP_GITHUB_CA_FILE/SSL_CERT_FILEreach every existing caller with no per-site change.Routed the four ad-hoc builders through it:
actions.rs(action tarball fetch)remote_workflows.rs(remote workflow ref→SHA + content fetch)github_app.rs(preloop doctorrepo-access probe)snapshots.rsLFS_FORGE_CLIENT(LFS object fetch)Safety / failure mode
warn!and zero extra roots; TLS verification stays on, startup never aborts on a bad path.Tests
shared_http::tests— parses a real self-signed PEM, returns empty (not panic) on a missing path and on a non-PEM file.cargo test -p preloop-runner-server --lib shared_http: 3 passed.Summary by cubic
Makes server-side GitHub calls trust custom CA roots so the runner can talk to a GHES or emulator forge (like gh-simulate) without TLS verification failures. Previously every outbound client was built ad-hoc and trusted only bundled roots.
github_client_builder()now adds roots fromPRELOOP_GITHUB_CA_FILE(forge-scoped, additive) andSSL_CERT_FILE; the sharedCLIENTand all four ad-hoc builders (action tarballs, remote workflows,preloop doctorprobe, LFS forge client) route through it.Part of #349; sibling PRs cover PAT-over-http, remaining hardcoded hosts, and the probe off-switch.
Written for commit 978a3ac. Summary will update on new commits.