Skip to content

server: restrict container egress CONNECT listener to the managed container #6205

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dknecht wants to merge 1 commit into from
Closed

server: restrict container egress CONNECT listener to the managed container #6205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 70 additions & 4 deletions src/workerd/server/container-client.c++
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@
#include <kj/encoding.h>
#include <kj/exception.h>
#include <kj/string.h>
#include <arpa/inet.h>
#include <netinet/in.h>
#include <sys/socket.h>

namespace workerd::server {

Expand Down Expand Up @@ -225,7 +228,7 @@ class ContainerClient::DockerPort final: public rpc::Container::Port::Server {
// Port mappings might have outdated mappings, we can't know if a connect request
// fails because the app hasn't finished starting up or because the mapping is outdated.
// To be safe we should inspect the container to get up to date mappings.
const auto [_running, portMappings] = co_await containerClient.inspectContainer();
const auto [_running, portMappings, _ipAddress] = co_await containerClient.inspectContainer();
auto maybeMappedPort = portMappings.find(containerPort);
if (maybeMappedPort == kj::none) {
throw JSG_KJ_EXCEPTION(DISCONNECTED, Error,
Expand Down Expand Up @@ -322,6 +325,14 @@ class EgressHttpService final: public kj::HttpService {
kj::AsyncIoStream& connection,
ConnectResponse& response,
kj::HttpConnectSettings settings) override {
auto isAuthorized = co_await containerClient.isEgressPeerAuthorized(connection);
if (!isAuthorized) {
kj::HttpHeaders responseHeaders(headerTable);
response.accept(403, "Forbidden", responseHeaders);
connection.shutdownWrite();
co_return;
}
Comment on lines +328 to +334
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[High] accept() transitions the connection into tunnel mode. For rejecting a CONNECT, use reject() instead, which sends a proper error response without establishing a tunnel.

Suggested change
auto isAuthorized = co_await containerClient.isEgressPeerAuthorized(connection);
if (!isAuthorized) {
kj::HttpHeaders responseHeaders(headerTable);
response.accept(403, "Forbidden", responseHeaders);
connection.shutdownWrite();
co_return;
}
auto isAuthorized = co_await containerClient.isEgressPeerAuthorized(connection);
if (!isAuthorized) {
kj::HttpHeaders responseHeaders(headerTable);
response.reject(403, "Forbidden", responseHeaders);
co_return;
}


auto destAddr = kj::str(host);

kj::HttpHeaders responseHeaders(headerTable);
Expand Down Expand Up @@ -486,7 +497,7 @@ kj::Promise<ContainerClient::InspectResponse> ContainerClient::inspectContainer(
// We check if the container with the given name exist, and if it's not,
// we simply return false while avoiding an unnecessary error.
if (response.statusCode == 404) {
co_return InspectResponse{.isRunning = false, .ports = {}};
co_return InspectResponse{.isRunning = false, .ports = {}, .ipAddress = kj::none};
}

JSG_REQUIRE(response.statusCode == 200, Error, "Container inspect failed");
Expand Down Expand Up @@ -531,7 +542,22 @@ kj::Promise<ContainerClient::InspectResponse> ContainerClient::inspectContainer(
// perspective, a restarting container is still "alive" and should be treated as running
// so that start() correctly refuses to start a duplicate and destroy() can clean it up.
bool running = status == "running" || status == "restarting";
co_return InspectResponse{.isRunning = running, .ports = kj::mv(portMappings)};
auto ipAddress = kj::Maybe<kj::String>(kj::none);
if (jsonRoot.hasNetworkSettings()) {
auto networkSettings = jsonRoot.getNetworkSettings();
if (networkSettings.hasIpAddress()) {
auto ip = networkSettings.getIpAddress();
if (!ip.isEmpty()) {
ipAddress = kj::str(ip);
}
}
}

co_return InspectResponse{
.isRunning = running,
.ports = kj::mv(portMappings),
.ipAddress = kj::mv(ipAddress),
};
}

kj::Promise<void> ContainerClient::createContainer(
Expand Down Expand Up @@ -608,6 +634,46 @@ kj::Promise<void> ContainerClient::createContainer(
}
}


kj::Promise<bool> ContainerClient::isEgressPeerAuthorized(kj::AsyncIoStream& connection) {
auto [isRunning, _ports, maybeIpAddress] = co_await inspectContainer();
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Design note] This calls inspectContainer() (a full Docker API HTTP round-trip) on every CONNECT request. This creates a TOCTOU window: the container IP could change between the inspect response and the getpeername() call. Consider capturing the container IP once at container start time and caching it, rather than re-inspecting on every connection. This would also improve latency on the CONNECT path.

if (!isRunning || maybeIpAddress == kj::none) {
co_return false;
}

sockaddr_storage peerAddrStorage;
auto peerAddr = reinterpret_cast<sockaddr*>(&peerAddrStorage);
kj::uint peerAddrLen = sizeof(peerAddrStorage);
connection.getpeername(peerAddr, &peerAddrLen);

KJ_IF_SOME(containerIp, maybeIpAddress) {
switch (peerAddr->sa_family) {
case AF_INET: {
char addrBuf[INET_ADDRSTRLEN] = {0};
auto* sin = reinterpret_cast<sockaddr_in*>(peerAddr);
if (inet_ntop(AF_INET, &sin->sin_addr, addrBuf, INET_ADDRSTRLEN) != nullptr &&
containerIp == kj::str(addrBuf)) {
co_return true;
}
break;
}
case AF_INET6: {
char addrBuf[INET6_ADDRSTRLEN] = {0};
auto* sin6 = reinterpret_cast<sockaddr_in6*>(peerAddr);
if (inet_ntop(AF_INET6, &sin6->sin6_addr, addrBuf, INET6_ADDRSTRLEN) != nullptr &&
containerIp == kj::str(addrBuf)) {
co_return true;
}
break;
}
default:
break;
}
}

co_return false;
Comment on lines +638 to +674
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium/High] Multiple issues in isEgressPeerAuthorized():

  1. getpeername() can throw if the socket is disconnected. An exception here should be caught and treated as unauthorized.
  2. maybeIpAddress == kj::none is non-idiomatic — the subsequent KJ_IF_SOME already handles the none case, making line 640's check redundant.
  3. kj::str(addrBuf) allocates unnecessarily — containerIp (a kj::String) can be compared directly against a kj::StringPtr.
Suggested change
kj::Promise<bool> ContainerClient::isEgressPeerAuthorized(kj::AsyncIoStream& connection) {
auto [isRunning, _ports, maybeIpAddress] = co_await inspectContainer();
if (!isRunning || maybeIpAddress == kj::none) {
co_return false;
}
sockaddr_storage peerAddrStorage;
auto peerAddr = reinterpret_cast<sockaddr*>(&peerAddrStorage);
kj::uint peerAddrLen = sizeof(peerAddrStorage);
connection.getpeername(peerAddr, &peerAddrLen);
KJ_IF_SOME(containerIp, maybeIpAddress) {
switch (peerAddr->sa_family) {
case AF_INET: {
char addrBuf[INET_ADDRSTRLEN] = {0};
auto* sin = reinterpret_cast<sockaddr_in*>(peerAddr);
if (inet_ntop(AF_INET, &sin->sin_addr, addrBuf, INET_ADDRSTRLEN) != nullptr &&
containerIp == kj::str(addrBuf)) {
co_return true;
}
break;
}
case AF_INET6: {
char addrBuf[INET6_ADDRSTRLEN] = {0};
auto* sin6 = reinterpret_cast<sockaddr_in6*>(peerAddr);
if (inet_ntop(AF_INET6, &sin6->sin6_addr, addrBuf, INET6_ADDRSTRLEN) != nullptr &&
containerIp == kj::str(addrBuf)) {
co_return true;
}
break;
}
default:
break;
}
}
co_return false;
kj::Promise<bool> ContainerClient::isEgressPeerAuthorized(kj::AsyncIoStream& connection) {
auto [isRunning, _ports, maybeIpAddress] = co_await inspectContainer();
if (!isRunning) {
co_return false;
}
KJ_IF_SOME(containerIp, maybeIpAddress) {
sockaddr_storage peerAddrStorage;
auto peerAddr = reinterpret_cast<sockaddr*>(&peerAddrStorage);
kj::uint peerAddrLen = sizeof(peerAddrStorage);
KJ_IF_SOME(exception, kj::runCatchingExceptions([&]() {
connection.getpeername(peerAddr, &peerAddrLen);
})) {
// Socket disconnected or in bad state — treat as unauthorized.
co_return false;
}
switch (peerAddr->sa_family) {
case AF_INET: {
char addrBuf[INET_ADDRSTRLEN] = {0};
auto* sin = reinterpret_cast<sockaddr_in*>(peerAddr);
if (inet_ntop(AF_INET, &sin->sin_addr, addrBuf, INET_ADDRSTRLEN) != nullptr &&
containerIp == kj::StringPtr(addrBuf)) {
co_return true;
}
break;
}
case AF_INET6: {
char addrBuf[INET6_ADDRSTRLEN] = {0};
auto* sin6 = reinterpret_cast<sockaddr_in6*>(peerAddr);
if (inet_ntop(AF_INET6, &sin6->sin6_addr, addrBuf, INET6_ADDRSTRLEN) != nullptr &&
containerIp == kj::StringPtr(addrBuf)) {
co_return true;
}
break;
}
default:
break;
}
}
co_return false;
}

}

kj::Promise<void> ContainerClient::startContainer() {
auto endpoint = kj::str("/containers/", containerName, "/start");
// We have to send an empty body since docker API will throw an error if we don't.
Expand Down Expand Up @@ -755,7 +821,7 @@ ContainerClient::RpcTurn ContainerClient::getRpcTurn() {
}

kj::Promise<void> ContainerClient::status(StatusContext context) {
const auto [isRunning, _ports] = co_await inspectContainer();
const auto [isRunning, _ports, _ipAddress] = co_await inspectContainer();
containerStarted.store(isRunning, std::memory_order_release);
context.getResults().setRunning(isRunning);
}
Expand Down
4 changes: 4 additions & 0 deletions src/workerd/server/container-client.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ class ContainerClient final: public rpc::Container::Server, public kj::Refcounte
struct InspectResponse {
bool isRunning;
kj::HashMap<uint16_t, uint16_t> ports;
kj::Maybe<kj::String> ipAddress;
};

struct IPAMConfigResult {
Expand Down Expand Up @@ -141,6 +142,9 @@ class ContainerClient final: public rpc::Container::Server, public kj::Refcounte
kj::Maybe<workerd::IoChannelFactory::SubrequestChannel*> findEgressMapping(
kj::StringPtr destAddr, uint16_t defaultPort);

// Validates that an incoming egress proxy connection comes from the managed container.
kj::Promise<bool> isEgressPeerAuthorized(kj::AsyncIoStream& connection);

// Whether general internet access is enabled for this container
bool internetEnabled = false;

Expand Down
Loading