fix(tls): thread --tls-ca-file and --insecure-tls through the download engine - #550
Open
SuperCoolPencil wants to merge 6 commits into
Open
fix(tls): thread --tls-ca-file and --insecure-tls through the download engine#550SuperCoolPencil wants to merge 6 commits into
SuperCoolPencil wants to merge 6 commits into
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds global and per-download TLS CA file and insecure-TLS settings. The settings flow through runtime configuration, transport pooling, probing, downloading, persistence, and resume handling. CLI documentation now describes TLS flags and certificate troubleshooting. ChangesTLS configuration flow
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant DownloadRequest
participant Scheduler
participant ProbeServerWithProxy
participant NetworkPool
participant Downloader
CLI->>DownloadRequest: provide TLSCAFile and TLSInsecure
DownloadRequest->>Scheduler: retain TLS settings
Scheduler->>ProbeServerWithProxy: pass runtime TLS settings
ProbeServerWithProxy->>NetworkPool: acquire TLS-aware transport
NetworkPool-->>ProbeServerWithProxy: return transport or error
Scheduler->>Downloader: start download with TLS settings
Downloader->>NetworkPool: acquire matching transport
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/USAGE.md`:
- Around line 77-93: Update the “Missing root certificate” workaround in the
documentation to download and convert the Microsoft TLS RSA Root G2 certificate,
matching the certificate named in the explanation; keep the existing Surge
--tls-ca-file usage and command flow unchanged.
In `@internal/orchestrator/manager.go`:
- Line 218: Update enqueueResolved around the ProbeServerWithProxy call to
detect the typed transport-configuration error caused by invalid or unreadable
TLSCAFile and return it directly instead of classifying it as a non-terminal
probe failure. Ensure the existing enqueue behavior remains unchanged for other
probe errors, and expose or reuse the typed transport error from the probing
transport setup.
- Line 350: Update the synchronous persisted download record creation near the
existing state persistence to include req.TLSCAFile and req.TLSInsecure, then
restore those values into the resumed request or settings before RunDownload.
Keep the runtime config construction in WithTLS aligned with the restored
persisted values so cold resumes retain the original TLS configuration.
In `@internal/strategy/single/downloader_test.go`:
- Around line 317-320: Update TestNewSingleDownloader_TransportReuse to check
the error returned by each AcquireTransport call before using the transport;
fail the test immediately when acquisition fails, then defer ReleaseTransport
and compare the successfully acquired transports.
In `@internal/transport/network.go`:
- Around line 59-63: Normalize tlsCAFile using the same trimming behavior as
BuildTLSConfig before constructing poolKey or creating a transport, so
equivalent CA paths reuse one pool. Update the transport acquisition flow around
createNewTransport and add a regression test that acquires transports with
trimmed and whitespace-padded CA paths and verifies they share the transport
pool.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 459f5f25-9bca-4511-bb10-c75477129379
📒 Files selected for processing (20)
cmd/root.gocmd/root_downloads.godocs/USAGE.mdinternal/config/settings.gointernal/orchestrator/manager.gointernal/orchestrator/pause_resume.gointernal/probe/probe.gointernal/scheduler/manager.gointernal/service/http_client.gointernal/service/local_service_test.gointernal/strategy/concurrent/concurrent_test.gointernal/strategy/concurrent/downloader.gointernal/strategy/concurrent/prewarm_reuse_test.gointernal/strategy/single/downloader.gointernal/strategy/single/downloader_test.gointernal/transport/network.gointernal/transport/network_test.gointernal/types/config.gointernal/utils/http_test.gointernal/utils/tls.go
Binary Size Analysis
|
SuperCoolPencil
force-pushed
the
fix-tls-cert
branch
from
August 2, 2026 08:15
5e33bb7 to
3b14301
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/transport/network.go (1)
52-55: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPartition
ClientSessionCacheentries by effective TLS policy.
NetworkPoolkeys transports bytlsCAFileandtlsInsecure, but every TLS config is wired tosharedClientSessionCache. For Go modules using this package, a cached session established with a private CA can be offered to a later pool transport with a different, including system-default, trust policy. Partition the cache, or disable resumption for transported clients unless resumption policy is also a required test assertion. Add a regression test that establishes a session with one CA policy and attempts resumption with another policy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/transport/network.go` around lines 52 - 55, Update sharedClientSessionCache usage in NetworkPool TLS configuration to partition ClientSessionCache entries by effective TLS policy, including tlsCAFile and tlsInsecure, so sessions cannot cross trust-policy boundaries. Preserve session resumption within the same policy, and add a regression test that establishes a session under one CA policy then verifies it is not resumed under a different policy.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/transport/network_test.go`:
- Around line 18-19: Update all affected tests in network_test.go to capture and
assert errors from every AcquireTransport call with t.Fatalf before
dereferencing or releasing the returned transport. Ensure the cache, HTTP/2, and
reuse tests always use the successfully acquired pool transport, and in the
idle-timer test assign the reacquired transport to a variable and release it to
keep leases balanced.
In `@internal/types/models.go`:
- Around line 72-73: Update ConcurrentDownloader.saveStateSnapshot to copy
TLSCAFile and TLSInsecure from d.Runtime into every DownloadRecord literal used
for pause-event and direct state saves, preserving the configured TLS values
across cold resumes. Add a cold-resume test verifying both the CA path and
insecure mode survive snapshot restoration.
---
Outside diff comments:
In `@internal/transport/network.go`:
- Around line 52-55: Update sharedClientSessionCache usage in NetworkPool TLS
configuration to partition ClientSessionCache entries by effective TLS policy,
including tlsCAFile and tlsInsecure, so sessions cannot cross trust-policy
boundaries. Preserve session resumption within the same policy, and add a
regression test that establishes a session under one CA policy then verifies it
is not resumed under a different policy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fb403897-2bf3-4512-aad1-291a2ab8b6c6
📒 Files selected for processing (9)
cmd/root_downloads.godocs/USAGE.mdinternal/orchestrator/manager.gointernal/orchestrator/pause_resume.gointernal/strategy/concurrent/downloader.gointernal/strategy/single/downloader_test.gointernal/transport/network.gointernal/transport/network_test.gointernal/types/models.go
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/orchestrator/pause_resume.go
- docs/USAGE.md
- internal/strategy/single/downloader_test.go
- cmd/root_downloads.go
- internal/strategy/concurrent/downloader.go
- internal/orchestrator/manager.go
Comment on lines
+72
to
+73
| TLSCAFile string `json:"tls_ca_file,omitempty"` | ||
| TLSInsecure bool `json:"tls_insecure,omitempty"` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Persist TLS settings in every resume snapshot.
TLSCAFile and TLSInsecure are marked persistent here, but ConcurrentDownloader.saveStateSnapshot omits them from its DownloadRecord literal. Both the pause event state and direct state save can therefore contain zero TLS values. A state-only cold resume can lose the original CA path or insecure mode.
Copy the values from d.Runtime before saving and add a cold-resume test.
Suggested snapshot fields
Workers: d.Runtime.Workers,
MinChunkSize: d.Runtime.MinChunkSize,
+ TLSCAFile: d.Runtime.TLSCAFile,
+ TLSInsecure: d.Runtime.TLSInsecure,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/types/models.go` around lines 72 - 73, Update
ConcurrentDownloader.saveStateSnapshot to copy TLSCAFile and TLSInsecure from
d.Runtime into every DownloadRecord literal used for pause-event and direct
state saves, preserving the configured TLS values across cold resumes. Add a
cold-resume test verifying both the CA path and insecure mode survive snapshot
restoration.
SuperCoolPencil
force-pushed
the
fix-tls-cert
branch
2 times, most recently
from
August 2, 2026 08:55
f5da554 to
1076475
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/transport/network_test.go (1)
344-375: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winTest session-cache isolation for different CA files.
This test only separates the default and insecure TLS policies. A regression that keys the session cache by
tlsInsecurebut nottlsCAFilewould pass. Add two valid, distinct CA files and assert that their transports use different session caches.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/transport/network_test.go` around lines 344 - 375, Extend TestNetworkPool_SessionCachePartitioning to create two valid, distinct CA files and acquire transports using each CA path with the same TLS security settings. Assert their TLSClientConfig.ClientSessionCache values differ, while preserving the existing checks for insecure/default policy isolation and same-policy cache reuse.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/transport/network_test.go`:
- Around line 344-375: Extend TestNetworkPool_SessionCachePartitioning to create
two valid, distinct CA files and acquire transports using each CA path with the
same TLS security settings. Assert their TLSClientConfig.ClientSessionCache
values differ, while preserving the existing checks for insecure/default policy
isolation and same-policy cache reuse.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bc58b315-4692-4c64-bbe2-c474442babe5
📒 Files selected for processing (9)
docs/USAGE.mdinternal/orchestrator/manager.gointernal/orchestrator/pause_resume.gointernal/strategy/concurrent/downloader.gointernal/strategy/concurrent/downloader_helpers_test.gointernal/strategy/single/downloader_test.gointernal/transport/network.gointernal/transport/network_test.gointernal/types/models.go
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/strategy/concurrent/downloader_helpers_test.go
- internal/types/models.go
- internal/orchestrator/manager.go
- internal/orchestrator/pause_resume.go
- internal/strategy/single/downloader_test.go
- docs/USAGE.md
- internal/strategy/concurrent/downloader.go
SuperCoolPencil
force-pushed
the
fix-tls-cert
branch
from
August 2, 2026 09:17
1076475 to
02e69df
Compare
❌ Test Failures on
|
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.
Problem
Fixes #549.
--tls-ca-fileand--insecure-tlswere wired into the remote-API HTTP client (service.NewHTTPTransport) but not into theNetworkPoolused by the probe and download engines. The transport was created with a barehttp.Transportand noTLSClientConfig, silently ignoring both flags for every actual download.Alpine Linux's
ca-certificatespackage does not include theMicrosoft TLS RSA Root G2root, so downloading fromdownload.microsoft.com(Windows SDK, etc.) fails with:What changed
internal/utils/tls.go(new)BuildTLSConfig(caFile, insecure)— shared helper that appends to the system pool rather than replacing itinternal/service/http_client.gonewTLSConfigdelegates toutils.BuildTLSConfig— removes 30 lines of duplicationinternal/types/config.goTLSCAFile/TLSInsecureadded toRuntimeConfig;WithTLS()overlay helperinternal/transport/network.goAcquireTransportaccepts 2 new params;createNewTransportsetsTLSClientConfiginternal/probe/probe.gorunCfgtoAcquireTransportinternal/strategy/concurrent/downloader.gosetupNetwork()forwards TLS fromd.Runtimeinternal/strategy/single/downloader.goDownload()forwards TLS fromd.Runtimeinternal/orchestrator/manager.goDownloadRequestcarries the flags; overlaid viaWithTLS()at probe and download-record buildinternal/scheduler/manager.gocmd/root_downloads.goDownloadRequestconstructions inject CLI globalsdocs/USAGE.mdKey design choices
BuildTLSConfigcallsx509.SystemCertPool()and appends the user's PEM. Adding the Microsoft root doesn't break verification for any other host.utils.BuildTLSConfigis shared by both transport code paths; the old inline copy inservice/http_client.gois gone.globalTLSCAFile,globalInsecureTLS) already existed and are merely threaded through rather than re-declared.Testing
All existing tests pass. Added
TestNetworkPool_TLSKeyIsolationintransport/network_test.goto verify:insecure=truecorrectly setsInsecureSkipVerifyon the transportGreptile Summary
This PR propagates CLI TLS options through probing and download transports.
Confidence Score: 4/5
This PR is not safe to merge until per-download TLS state is persisted so cold resumes retain the original trust configuration.
The new resume logic reads TLS values exclusively from Runtime fields that persistence explicitly omits, leaving restarted custom-CA and insecure downloads unable to recover their required TLS settings.
internal/orchestrator/pause_resume.go, internal/types/models.go
What T-Rex did
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR CLI[CLI TLS flags] --> Req[DownloadRequest] Req --> Runtime[RuntimeConfig] Runtime --> Probe[Probe transport] Runtime --> Download[Download transport] Runtime -. excluded from persistence .-> Store[(Persistent state)] Store --> Resume[Cold resume] Resume --> Defaults[Current settings] Defaults --> DownloadPrompt To Fix All With AI
Reviews (3): Last reviewed commit: "fix(tls): address code review issues wit..." | Re-trigger Greptile
Summary by CodeRabbit
New Features
Bug Fixes