Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
This review was generated by an AI code review assistant and may contain inaccuracies. Please verify any suggestions before applying them.
Adds interceptOutboundHttps for SNI-based TLS interception in containers, including CA cert injection via Docker archive API and sidecar MITM signaling.
Findings (highest severity first):
- [HIGH]
injectCACert()atomic guard is unsound with coroutines — concurrent callers skip injection while the first is still in-flight. - [MEDIUM]
findEgressHttpsMappingcalled at CONNECT time only to check existence, discarding theaddRef'd channel. - [MEDIUM]
readCACert()is called unconditionally instatus()recovery path even when no HTTPS mappings are configured (wastes a round-trip to the sidecar). - [LOW]
demuxDockerExecStreamreturns akj::Stringfrom raw bytes without validating that the content is valid UTF-8.
| kj::Promise<void> ContainerClient::injectCACert() { | ||
| if (caCertInjected.exchange(true, std::memory_order_acquire)) { | ||
| co_return; | ||
| } | ||
|
|
||
| bool succeeded = false; | ||
| KJ_DEFER(if (!succeeded) caCertInjected.store(false, std::memory_order_release)); | ||
|
|
||
| auto& bytes = KJ_REQUIRE_NONNULL(caBytes, "CA cert not read from sidecar yet"); | ||
| co_await writeFileToContainer(containerName, cloudflareCaDir, cloudflareCaFilename, bytes); | ||
|
|
||
| succeeded = true; | ||
| co_return; |
There was a problem hiding this comment.
[HIGH] injectCACert() uses atomic_bool::exchange as a once-guard, but co_await writeFileToContainer(...) suspends the coroutine. A second concurrent call (e.g. setEgressHttps racing with start) will see exchange(true) and return immediately, even though the first injection hasn't completed yet — the second caller has no way to wait for the first to finish.
The KJ_DEFER rollback on failure is also problematic: if the coroutine is destroyed (cancelled) while suspended at the co_await, KJ_DEFER fires, resetting caCertInjected to false, which can cause an infinite retry loop of partial injections.
Consider replacing the atomic bool with a kj::ForkedPromise<void> pattern (similar to mutationQueue), where subsequent callers co_await a branch of the in-flight injection promise instead of bailing out.
There was a problem hiding this comment.
this is OK. The pattern is widely used in the file. We fixed this concurrent RPC issues by making sure container-client RPCs don't overlap over each other.
| auto httpsMapping = containerClient.findEgressHttpsMapping(sni); | ||
|
|
||
| if (httpsMapping != kj::none) { |
There was a problem hiding this comment.
[MEDIUM] findEgressHttpsMapping(sni) returns a kj::Maybe<kj::Own<...>>. Here it's only used as a boolean existence check — the addRef'd channel is immediately discarded. This is a wasted refcount bump/drop on every CONNECT with TLS SNI.
Consider adding a hasEgressHttpsMapping(kj::StringPtr) method that just returns bool, or restructure with KJ_IF_SOME:
| auto httpsMapping = containerClient.findEgressHttpsMapping(sni); | |
| if (httpsMapping != kj::none) { | |
| KJ_IF_SOME(mapping, containerClient.findEgressHttpsMapping(sni)) { |
(Then mapping is available if needed later, and the intent is clearer.)
| this->sidecarIngressHostPort = sidecar.ingressHostPort; | ||
| co_await ensureEgressListenerStarted(); | ||
| co_await updateSidecarEgressPort(sidecar.ingressHostPort, egressListenerPort); | ||
| co_await readCACert(); |
There was a problem hiding this comment.
[MEDIUM] readCACert() is called unconditionally in the status() recovery path. If the user never calls interceptOutboundHttps, this is an unnecessary HTTP round-trip to the sidecar on every DO reconnection. Consider guarding this with a check like if (!egressHttpsMappings.empty()) — or lazily reading the cert on the first setEgressHttps call instead of eagerly here.
There was a problem hiding this comment.
I dont think it will be a performance concern in local dev but its true we could theoretically skip this call.
| if (!egressHttpsMappings.empty()) { | ||
| caCertInjected = false; | ||
| co_await injectCACert(); | ||
| } |
There was a problem hiding this comment.
Observation: caCertInjected = false (non-atomic store on line 1197) is followed by a coroutine co_await. Since std::atomic_bool::operator= performs a seq_cst store, this is technically safe, but mixing non-member-function assignment with the explicit store()/exchange() calls used everywhere else is inconsistent and could confuse future readers.
| if (!egressHttpsMappings.empty()) { | |
| caCertInjected = false; | |
| co_await injectCACert(); | |
| } | |
| if (!egressHttpsMappings.empty()) { | |
| caCertInjected.store(false, std::memory_order_release); | |
| co_await injectCACert(); | |
| } |
|
I'm Bonk, and I've done a quick review of your PR. This PR adds I posted a batch review with 5 inline comments on PR #6306. Key findings ranked by severity:
Things that looked good: capnp schema is wire-compatible (new field |
`interceptOutboundHttps` is a way for users to intercept their own TLS traffic. The way this works is different from interceptOutboundHttp. In the first one, we can decide which IP and port combinations should intercept HTTP traffic, but HTTPS is more idiomatic to handle at the SNI level. The reasoning behind this is that customers that want to intercept TLS might want to only be triggering on certain SNIs being intercepted. We do support currently other ports than 443 but that might change in the future by extending the method to accept a port with the SNI. It's just the use-case is clear to be SNI based only. The glob format of the SNI that we accept is really simple, only '*' and the domain (to support cases like *google.com and all its subdomains). No plans on supporting regex here whatsoever. The way local dev works is we generate the certificates in the networking sidecar, we read it via exec, and write them to the container to a known path (/etc/cloudflare/certs/cloudflare-containers-ca.crt). We could try to append the certificate to known distro paths, but that might be a more controversial move, we can discuss in the MR if it's worth doing. The flow of the connection is: ``` [container] --> [proxy-everything] (tls) --> [workerd container-client.c++] (processes configured egress policies) -> [workerd subrequest channel] ``` The only way to make files being written consistently across distros is by using the Docker /archive API. It can only accept a tar right now, so we had to add a method that creates a simple tar file that contains a single file that we want to add to the container (the CA).
| id @0 :Text $Json.name("Id"); | ||
| warning @1 :Text $Json.name("Warning"); | ||
| } | ||
|
|
There was a problem hiding this comment.
oops. will remove this.
| jsg::Lock& js, kj::String addr, jsg::Ref<Fetcher> binding); | ||
| jsg::Promise<void> interceptAllOutboundHttp(jsg::Lock& js, jsg::Ref<Fetcher> binding); | ||
| jsg::Promise<void> interceptOutboundHttps( | ||
| jsg::Lock& js, kj::String sniGlob, jsg::Ref<Fetcher> binding); |
There was a problem hiding this comment.
Kind of inconsistent that for HTTP we intercept an address, but for HTTPS we intercept a hostname.
Honestly I would like both to be specified as a hostname. Specifying a hostname is much nicer than specifying an IP address, if you're trying to intercept some known service.
Maybe both methods could actually accept both IP addresses and hostnames? Seems like you can always distinguish them, given a string?
For HTTPS, when intercepting by IP, we'd have to create certificates on-the-fly based on SNI, which is a bit weird but doable. This would be kind of cool, you could even have interceptAllOutboundHttps()...
There was a problem hiding this comment.
I agree that it's kind of weird right now. I am leaning towards accepting hostnames too on interceptOutboundHttp...
As for interceptAllOutboundHttps, user can already do that very easily with interceptOutboundHttps('*')!
There was a problem hiding this comment.
I think it's important that these methods don't end up with different and incompatible formats for their inputs -- that would be really confusing.
So I think we need to define the full format that we intend to support for both, now, to very it is really possible to distinguish everything we want to support as a single string. If it's not then I think we need to break out an options structure or something. And we need to plan to make the two support exactly the same format soon.
I don't want to introduce a different string format here and then discover after it's released that the formats are fundamentally incompatible and can't be unified.
|
/bigbonk roast this pr |
There was a problem hiding this comment.
This is an AI-generated follow-up review with additional findings from a deeper pass. Please verify suggestions before applying.
Follow-up from my initial batch review. Incorporating context from kentonv's API design feedback and gabivlj's responses.
Additional findings:
-
[MEDIUM]
setEgressHttpsdoes not callensureSidecarStarted()— unlikesetEgressHttpwhich calls it when the container is already running (line 1332). IfinterceptOutboundHttpsis called afterstart()but before any HTTP egress was configured, the sidecar may not have been started for this RPC turn. TheinjectCACert()call assumes the sidecar is already running (readscaBytes), butensureSidecarStarted()is what populatescaBytesviareadCACert(). -
[MEDIUM]
upsertEgressHttpsMappinguses exact string equality (m.sniGlob == mapping.sniGlob) for dedup, but the matching function (matchSniGlob) is case-insensitive. CallinginterceptOutboundHttps("*.Example.COM", ...)theninterceptOutboundHttps("*.example.com", ...)would create two separate mappings that match the same hostnames. The sniGlob should be lowercased before storage (or the comparison in upsert should be case-insensitive). -
[LOW]
pumpBidirectionalis declared at namespace scope but has nostaticlinkage. Since it's a free function in a.c++file this isn't a linker error today, but addingstaticor placing it in the anonymous namespace would be consistent with the other helpers (asciiCaseInsensitiveEquals,matchSniGlob, etc.). -
[LOW] In the test file,
testSetEgressHttpsandtestInterceptWebSocketHttpsuseabort: AbortSignal.timeout(...)instead ofsignal: AbortSignal.timeout(...)infetch()options. The correct property name issignal;abortis ignored by the Fetch API. This means those test requests have no timeout protection and could hang indefinitely if something goes wrong. (The HTTP tests correctly usesignal.)
| if (containerStarted.load(std::memory_order_acquire)) { | ||
| // Sidecar is always started before the container, so caBytes is already | ||
| // populated. We just need to push the cert into the user container. | ||
| co_await injectCACert(); |
There was a problem hiding this comment.
[MEDIUM] Unlike setEgressHttp (which calls ensureSidecarStarted() at line 1332 when the container is already running), setEgressHttps only calls ensureEgressListenerStarted() and then conditionally injectCACert(). But injectCACert() requires caBytes to be populated, which happens in readCACert(), which is called from ensureSidecarStarted().
If the user calls interceptOutboundHttps() after start() but no HTTP egress was ever configured (so ensureSidecarStarted() was only called from start() itself, which does populate caBytes), this works because start() calls ensureSidecarStarted(). But consider the edge case where status() recovered a running container and readCACert() failed or was skipped — the KJ_REQUIRE_NONNULL(caBytes, ...) in injectCACert would throw.
Also, not calling ensureSidecarStarted() here means the sidecar's egress port is not updated if it was somehow stale. For consistency with setEgressHttp, consider adding:
if (containerStarted.load(std::memory_order_acquire)) {
co_await ensureSidecarStarted();
co_await injectCACert();
}
|
|
||
| void ContainerClient::upsertEgressHttpsMapping(EgressHttpsMapping mapping) { | ||
| for (auto& m: egressHttpsMappings) { | ||
| if (m.sniGlob == mapping.sniGlob) { |
There was a problem hiding this comment.
[MEDIUM] upsertEgressHttpsMapping compares m.sniGlob == mapping.sniGlob using exact string equality, but matchSniGlob uses asciiCaseInsensitiveEquals for matching. This means:
await container.interceptOutboundHttps("*.Example.COM", binding1);
await container.interceptOutboundHttps("*.example.com", binding2);...would create two separate mappings that both match the same hostnames. The first one would win at match time.
Consider normalizing the glob to lowercase before storage (DNS hostnames are case-insensitive per RFC 4343), or making the comparison in upsertEgressHttpsMapping case-insensitive to match the matching semantics.
| bool isTls; | ||
| }; | ||
|
|
||
| kj::Promise<void> pumpBidirectional(kj::AsyncIoStream& a, kj::AsyncIoStream& b) { |
There was a problem hiding this comment.
[LOW] pumpBidirectional is declared at namespace scope without static linkage. Since the other helper functions (asciiCaseInsensitiveEquals, matchSniGlob, writeTarField, createTarWithFile) are all inside the anonymous namespace, this one should be too for consistency.
| kj::Promise<void> pumpBidirectional(kj::AsyncIoStream& a, kj::AsyncIoStream& b) { | |
| static kj::Promise<void> pumpBidirectional(kj::AsyncIoStream& a, kj::AsyncIoStream& b) { |
Or move it inside the anonymous namespace block above.
| .fetch('http://foo/intercept-https', { | ||
| headers: { 'x-host': 'example.com' }, | ||
| abort: AbortSignal.timeout(DEFAULT_TIMEOUT_DURATION), | ||
| }); |
There was a problem hiding this comment.
[LOW] Several fetch() calls in the HTTPS tests use abort: AbortSignal.timeout(...) instead of signal: AbortSignal.timeout(...). The Fetch API recognizes signal, not abort, so these requests have no timeout protection and could hang indefinitely if the sidecar or container misbehaves.
Compare with the HTTP tests above which correctly use signal:.
| }); | |
| abort: AbortSignal.timeout(DEFAULT_TIMEOUT_DURATION), |
should be:
signal: AbortSignal.timeout(DEFAULT_TIMEOUT_DURATION),This affects lines 541, 556, 571, 591, 606, and 735 in test.js.
|
I've posted a follow-up review on PR #6306 with 4 additional findings beyond my initial batch review:
These are posted as both a summary comment and individual inline comments at the relevant lines. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6306 +/- ##
==========================================
- Coverage 70.59% 70.47% -0.12%
==========================================
Files 420 420
Lines 111736 111923 +187
Branches 18403 18434 +31
==========================================
- Hits 78881 78880 -1
- Misses 21864 22051 +187
- Partials 10991 10992 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4 participants
interceptOutboundHttpsis a way for users to intercept their own TLS traffic.The way this works is different from interceptOutboundHttp. In the first one, we can decide which IP and port combinations should intercept HTTP traffic, but HTTPS is more idiomatic to handle at the SNI level.
The reasoning behind this is that customers that want to intercept TLS might want to only be triggering on certain SNIs being intercepted.
We do support currently other ports than 443 but that might change in the future by extending the method to accept a port with the SNI. It's just the use-case is clear to be SNI based only.
The glob format of the SNI that we accept is really simple, only '*' and the domain (to support cases like *google.com and all its subdomains). No plans on supporting regex here whatsoever.
The way local dev works is we generate the certificates in the networking sidecar, we read it via a HTTP request to the sidecar, and write them to the container to a known path (/etc/cloudflare/certs/cloudflare-containers-ca.crt).
We could try to append the certificate to
known distro paths, but that might be a more controversial move, we can discuss in the MR if it's worth doing.
The flow of the connection is:
The only way to make files being written consistently across distros is by using the Docker /archive API. It can only accept a tar right now, so we had to add a method that creates a simple tar file that contains a single file that we want to add to the container (the CA).