Skip to content

Commit ad8d53d

Browse files
Lutherwavesclaude
andcommitted
fix(daemon,brokerclient): final review fix wave for remote transport
Blocking: - plans: replace the internal-service-name/deployment-name grep pattern in the pre-PR leak-audit step with a neutral IP-shaped pattern plus a manual scan instruction, and strip the absolute developer path from the same file - pkg/brokerclient: fix the package doc and Client doc to say the client reaches openbloxd over a Unix socket or a mutual-TLS network connection - specs: correct the design doc's callback name from VerifyPeerCertificate to VerifyConnection and explain why (VerifyPeerCertificate is skipped on TLS 1.3 PSK resumption) - CHANGELOG: add the Unreleased entries for the listen block, NewRemote/ TLSFiles, and caller identity; stop describing brokerclient as socket-only Also fixed: - assert MinVersion == tls.VersionTLS13 in TestTLSConfigWiresAllowlistIntoVerifyConnection - drop the dangling "fix report" reference in listener_tls_test.go - correct newPKI's ServerName comment: the test certificate's IPAddresses SAN already covers 127.0.0.1, so ServerName is set to exercise the documented override, not because verification would otherwise fail - main.go: log socket="off" (was socket="") for consistency with network="off" - rename serveOnce to serveHTTP (it serves until t.Cleanup, not once) - TestRemoteAcceptedRequestGetsExactlyTheProfilePolicy now also compares Lifetime, DefaultTimeout and MaxTimeout - config_test.go: add the "listen without tls" refusal case to the existing incomplete-listen-block table Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent df6d429 commit ad8d53d

10 files changed

Lines changed: 58 additions & 24 deletions

File tree

CHANGELOG.md

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,13 +29,22 @@ Releases are cut automatically from [Conventional Commits](https://www.conventio
2929
need `/var/run/docker.sock`, with per-profile isolation policy resolved
3030
server-side from configuration alone.
3131
- `pkg/brokerclient`: a drop-in `sandbox.Backend` that talks to `openbloxd`
32-
over its Unix socket, satisfying the same contract the Docker backend does.
32+
over a Unix socket, satisfying the same contract the Docker backend does.
3333
- `openbloxd`: `max_sandboxes` per profile, bounding how many sandboxes exist
3434
at once — the one resource dimension a profile did not otherwise cover.
3535
Exceeding it returns `429` with the new `at_capacity` error kind
3636
(`brokerapi.ErrAtCapacity`), distinct from a malformed request because the
3737
request is valid and may succeed once the reaper frees a slot. Unset means
3838
unlimited, so existing deployments are unchanged.
39+
- `openbloxd`: an optional `listen` block for a mutual-TLS network listener,
40+
alongside (or instead of) the Unix socket — `socket` is now optional once
41+
`listen` is set. Every caller presents a client certificate; only Common
42+
Names on the configured allowlist are accepted, so a shared or mis-issued
43+
CA cannot silently grant access. The caller's verified CN is recorded on
44+
every request the daemon handles.
45+
- `pkg/brokerclient`: `NewRemote` and `TLSFiles`, so a caller can reach
46+
`openbloxd` over the network with the same `sandbox.Backend` contract the
47+
Unix-socket client satisfies.
3948

4049
### Changed
4150

cmd/openbloxd/main.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,12 +130,16 @@ func run(configPath string) error {
130130
// connection, even though that peer is local.
131131
httpSrv := &http.Server{Handler: daemon.WithCaller(srv.Handler()), ReadHeaderTimeout: 10 * time.Second}
132132

133+
socket := "off"
134+
if cfg.Socket != "" {
135+
socket = cfg.Socket
136+
}
133137
network := "off"
134138
if cfg.Listen != nil {
135139
network = cfg.Listen.Address
136140
}
137141
slog.Info("openbloxd listening",
138-
slog.String("socket", cfg.Socket),
142+
slog.String("socket", socket),
139143
slog.String("network", network),
140144
slog.Int("profiles", len(cfg.Profiles)))
141145
return serve(ctx, httpSrv, lns...)

internal/daemon/config_test.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -285,6 +285,10 @@ listen:
285285
client_ca_file: /ca
286286
allowed_client_cns: ["sandbox-caller"]
287287
`, "address"},
288+
"no tls block": {`
289+
listen:
290+
address: "127.0.0.1:9443"
291+
`, "cert_file"},
288292
"address is not host:port": {`
289293
listen:
290294
address: "127.0.0.1"

internal/daemon/listener_tls_test.go

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -80,9 +80,9 @@ func clientTLS(t *testing.T, p *testpki.PKI, cn string) *tls.Config {
8080
return cfg
8181
}
8282

83-
// serveOnce accepts on ln and answers every request with 200 "ok", so a test
83+
// serveHTTP accepts on ln and answers every request with 200 "ok", so a test
8484
// can assert whether a client got through the handshake at all.
85-
func serveOnce(t *testing.T, ln net.Listener) {
85+
func serveHTTP(t *testing.T, ln net.Listener) {
8686
t.Helper()
8787
srv := &http.Server{Handler: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
8888
_, _ = io.WriteString(w, "ok")
@@ -116,7 +116,7 @@ func TestListenTLSAcceptsAnAllowedCommonName(t *testing.T) {
116116
t.Fatalf("ListenTLS: %v", err)
117117
}
118118
defer func() { _ = ln.Close() }()
119-
serveOnce(t, ln)
119+
serveHTTP(t, ln)
120120

121121
got, err := get(t, ln.Addr().String(), clientTLS(t, pki, "sandbox-caller"))
122122
if err != nil {
@@ -137,7 +137,7 @@ func TestListenTLSRejectsUnlistedCommonName(t *testing.T) {
137137
t.Fatalf("ListenTLS: %v", err)
138138
}
139139
defer func() { _ = ln.Close() }()
140-
serveOnce(t, ln)
140+
serveHTTP(t, ln)
141141

142142
if _, err := get(t, ln.Addr().String(), clientTLS(t, pki, "someone-else")); err == nil {
143143
t.Fatal("a certificate with an unlisted common name was accepted")
@@ -152,7 +152,7 @@ func TestListenTLSRejectsForeignCA(t *testing.T) {
152152
t.Fatalf("ListenTLS: %v", err)
153153
}
154154
defer func() { _ = ln.Close() }()
155-
serveOnce(t, ln)
155+
serveHTTP(t, ln)
156156

157157
// Right name, wrong CA. Trust our real CA for the server leg so the only
158158
// thing under test is the client certificate.
@@ -170,7 +170,7 @@ func TestListenTLSRejectsNoClientCertificate(t *testing.T) {
170170
t.Fatalf("ListenTLS: %v", err)
171171
}
172172
defer func() { _ = ln.Close() }()
173-
serveOnce(t, ln)
173+
serveHTTP(t, ln)
174174

175175
cfg := &tls.Config{MinVersion: tls.VersionTLS13, RootCAs: pki.Pool(), ServerName: "openbloxd"}
176176
if _, err := get(t, ln.Addr().String(), cfg); err == nil {
@@ -252,8 +252,9 @@ func TestCheckAllowedClientCN(t *testing.T) {
252252
//
253253
// Confirmed by reverting to VerifyPeerCertificate in a scratch copy of
254254
// tlsConfigFor and re-running: this test goes red (VerifyConnection == nil),
255-
// while every other test in the package still passes. See the fix report
256-
// for the transcript.
255+
// while every other test in the package still passes — because
256+
// VerifyPeerCertificate is simply never invoked on a resumed session, so a
257+
// resumed connection sailed through with no allowlist check running at all.
257258
func TestTLSConfigWiresAllowlistIntoVerifyConnection(t *testing.T) {
258259
pki := newPKI(t)
259260
tlsCfg, err := tlsConfigFor(listenConfigFor(pki, "127.0.0.1:0", "sandbox-caller"))
@@ -266,6 +267,9 @@ func TestTLSConfigWiresAllowlistIntoVerifyConnection(t *testing.T) {
266267
if tlsCfg.VerifyPeerCertificate != nil {
267268
t.Error("VerifyPeerCertificate is set — Go skips this callback on a resumed session, so the allowlist must not depend on it")
268269
}
270+
if tlsCfg.MinVersion != tls.VersionTLS13 {
271+
t.Errorf("MinVersion = %v, want tls.VersionTLS13", tlsCfg.MinVersion)
272+
}
269273
}
270274

271275
// TestListenTLSResumedConnectionStillReachesTheGate proves the wiring is
@@ -286,7 +290,7 @@ func TestListenTLSResumedConnectionStillReachesTheGate(t *testing.T) {
286290
t.Fatalf("ListenTLS: %v", err)
287291
}
288292
defer func() { _ = ln.Close() }()
289-
serveOnce(t, ln)
293+
serveHTTP(t, ln)
290294

291295
clientCfg := clientTLS(t, pki, "sandbox-caller")
292296
clientCfg.ClientSessionCache = tls.NewLRUClientSessionCache(4)
@@ -371,7 +375,7 @@ func TestVerifyConnectionRejectsAResumedSessionOnceItsCNIsRevoked(t *testing.T)
371375
}
372376
tlsLn := tls.NewListener(ln, serverCfg)
373377
defer func() { _ = tlsLn.Close() }()
374-
serveOnce(t, tlsLn)
378+
serveHTTP(t, tlsLn)
375379

376380
clientCfg := clientTLS(t, pki, "sandbox-caller")
377381
clientCfg.ClientSessionCache = tls.NewLRUClientSessionCache(4)

internal/daemon/policy_test.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -180,7 +180,9 @@ func TestRemoteAcceptedRequestGetsExactlyTheProfilePolicy(t *testing.T) {
180180
got := fake.created["a"]
181181
want := sandbox.NewSpec(srv.cfg.Profiles["code-exec"].Options()...)
182182
if got.Runtime != want.Runtime || got.Egress != want.Egress ||
183-
got.User != want.User || got.Resources != want.Resources || got.Image != want.Image {
183+
got.User != want.User || got.Resources != want.Resources || got.Image != want.Image ||
184+
got.Lifetime != want.Lifetime || got.DefaultTimeout != want.DefaultTimeout ||
185+
got.MaxTimeout != want.MaxTimeout {
184186
t.Errorf("spec = %+v, want the profile's %+v", got, want)
185187
}
186188
}

pkg/brokerclient/client.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,9 @@ import (
2222
// a well-formed URL to build a request from.
2323
const baseURL = "http://openbloxd"
2424

25-
// Client reaches openbloxd over its Unix socket. It satisfies sandbox.Backend
26-
// so it can stand in for a Docker-backed one without the caller changing any
27-
// other code.
25+
// Client reaches openbloxd over a Unix socket or a mutual-TLS network
26+
// connection. It satisfies sandbox.Backend so it can stand in for a
27+
// Docker-backed one without the caller changing any other code.
2828
type Client struct {
2929
http *http.Client
3030

pkg/brokerclient/options.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
1-
// Package brokerclient reaches openbloxd over its Unix socket.
1+
// Package brokerclient reaches openbloxd over a Unix socket or a mutual-TLS
2+
// network connection.
23
//
34
// Its Client satisfies sandbox.Backend and its handle satisfies
45
// sandbox.Sandbox, so a caller swaps one constructor and stops needing

pkg/brokerclient/remote_test.go

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -81,9 +81,11 @@ func newPKI(t *testing.T) (*testpki.PKI, TLSFiles) {
8181
CertFile: certFile,
8282
KeyFile: keyFile,
8383
CAFile: p.CAFile,
84-
// The daemon's certificate names "openbloxd", but the test dials
85-
// 127.0.0.1 — so the name to verify has to be given explicitly. This
86-
// is exactly the case ServerName exists for.
84+
// Set explicitly, not because it's required here — the test
85+
// certificate's IPAddresses SAN covers 127.0.0.1, so an empty
86+
// ServerName would verify fine too. This exercises the override
87+
// documented on TLSFiles.ServerName, for deployments that dial the
88+
// daemon by an address its certificate doesn't name.
8789
ServerName: "openbloxd",
8890
}
8991
}

plans/2026-08-18-openbloxd-remote-transport.md

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -196,7 +196,7 @@ func TestIsWildcardHost(t *testing.T) {
196196

197197
- [ ] **Step 2: Run the tests to verify they fail**
198198

199-
Run: `cd /home/blox-master/dev/openblox-32-remote-transport && go test ./internal/daemon/ -run 'TestLoadAcceptsListen|TestLoadRejects|TestIsWildcardHost' -v`
199+
Run: `go test ./internal/daemon/ -run 'TestLoadAcceptsListen|TestLoadRejects|TestIsWildcardHost' -v`
200200
Expected: FAIL — compile error, `cfg.Listen` undefined and `ListenConfig` undefined.
201201

202202
- [ ] **Step 3: Add the types**
@@ -1849,8 +1849,8 @@ Expected: no diff. This work is stdlib-only.
18491849

18501850
- [ ] **Step 4: Audit the diff for anything deployment-specific**
18511851

1852-
Run: `git diff origin/main -- . | grep -inE 'mcpblox|tekom|prod|tailscale|tailnet|[0-9]{1,3}(\.[0-9]{1,3}){3}'`
1853-
Expected: only `127.0.0.1` from the neutral examples. Anything else is a leak of a specific deployment into a public repository and must be replaced with a neutral placeholder.
1852+
Run: `git diff origin/main -- . | grep -inE '[0-9]{1,3}(\.[0-9]{1,3}){3}'`
1853+
Expected: only `127.0.0.1` (and a documented `0.0.0.0` example). Also manually scan the diff against your own organisation's internal service names, deployment names, hostnames, and VPN/overlay-network products — none belong in this public repository. Anything found is a leak of deployment-specific detail and must be replaced with a neutral placeholder.
18541854

18551855
- [ ] **Step 5: Open the PR**
18561856

specs/2026-08-18-openbloxd-remote-transport-design.md

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,10 +94,18 @@ issued. That is the common way an mTLS deployment ends up weaker than the token
9494
it replaced. The allowlist makes a CA mis-issuance survivable, and it makes the
9595
set of permitted callers something an operator can read in one place.
9696
97-
Both gates run during the TLS handshake, via `VerifyPeerCertificate`, so a
97+
Both gates run during the TLS handshake, via `VerifyConnection`, so a
9898
rejected caller never reaches the request router at all. The daemon logs the
9999
rejected CN; the client sees a TLS alert.
100100
101+
`VerifyConnection` is used rather than `VerifyPeerCertificate` deliberately:
102+
Go does not invoke `VerifyPeerCertificate` on a resumed TLS session — the
103+
peer's certificates come back from cached session state and that callback is
104+
skipped — so an allowlist check living there would stop being enforced for a
105+
resumed caller even after its CN was removed from the allowlist.
106+
`VerifyConnection` runs on every connection, fresh or resumed, so the check
107+
applies without exception.
108+
101109
## Configuration
102110
103111
`listen` is a new optional block. Absent, nothing about an existing deployment

0 commit comments

Comments
 (0)