Skip to content

fix(ipc): bound the kernel IPC transport against abuse - #1379

Merged
fluck-boss merged 6 commits into
risa-labs-inc:devfrom
Mihir-Rabari:agent/flag-kernel-ipc-limits
Sep 24, 2026
Merged

fluck-boss merged 6 commits into
risa-labs-inc:devfrom
Mihir-Rabari:agent/flag-kernel-ipc-limits

Conversation

@Mihir-Rabari

Copy link
Copy Markdown
Contributor

Closes #1378

Cap maxInboundMessageSize on both ends of the kernel IPC wire from the
negotiated transport limits, so oversize frames die with a protocol-level
RESOURCE_EXHAUSTED at the deframer, before any handler runs or bytes are
buffered; refuse stream floods at the transport while streams are being
established, rather than masquerading the cap as an auth failure; and reap
connections idle past the deadline so their slots return to the pool.

The limits suite pins the contract against the real authenticated wire, both
halves of it: abuse is refused AND well-formed traffic keeps flowing before,
during and after each refusal - including across the transparent reconnect a
mid-write stream teardown forces.

@Mihir-Rabari
Mihir-Rabari force-pushed the agent/flag-kernel-ipc-limits branch 2 times, most recently from 5b682d0 to 6b0170a Compare September 20, 2026 13:05
@kshivang

Copy link
Copy Markdown
Contributor

Thanks @Mihir-Rabari for the focused IPC hardening and the authenticated-wire tests. The contributor credit should remain with this change when it is rebased or split.

Review result: unresolved — not ready to merge into current dev.

Before: the authenticated endpoint still inherited the transport defaults for message/metadata sizing, per-connection stream concurrency, and idle connection lifetime. A local process could therefore send oversized frames, open unbounded streams, or keep an idle connection around indefinitely. After: IpcTransportLimits wires a 4 MiB message cap and 8 KiB metadata cap into the server, caps concurrent calls, reaps idle connections, and puts the message cap on the client reader; the new tests exercise refusal and recovery over the real authenticated transport.

Required follow-up:

  1. Rebase/recreate the PR from current dev and retain only the six-file IPC change plus its focused tests. The branch is a stacked stale-base history: the PR diff is 135 files / about 10.9k additions, and most earlier commits are already represented in dev. Preserve Mihir-Rabari’s authorship and any existing co-author trailers.

  2. Make the configurable metadata limit symmetric. IpcTransportLimits.applyToServer() calls maxInboundMetadataSize(maxInboundMetadataBytes), but applyToChannel() only calls maxInboundMessageSize(maxInboundMessageBytes). The client is the reader of kernel response metadata, so a hostile kernel can exceed the configured metadata limit on that leg. Add .maxInboundMetadataSize(maxInboundMetadataBytes) to the channel builder and add an authenticated regression that sends oversized response metadata, alongside the existing server-side/request metadata coverage.

  3. After rebasing, rerun the focused ./gradlew :boss-ipc:test --tests 'ai.rever.boss.ipc.IpcTransportLimitsTest' --no-daemon, git diff --check origin/dev...HEAD, and the authenticated IPC matrix. My bounded local attempt on the submitted head did not reach the test: :boss-ipc:compileJava failed in generated PluginUIServiceGrpc.java with a UIUnregistration/Empty generic mismatch; hosted CI is green, so please confirm the rebased revision builds cleanly rather than treating that local result as a product failure.

Hosted CI is currently green on Ubuntu, macOS, and Windows, including the dedicated authenticated IPC job, code quality, compatibility, and build/test checks. No Claude review or correctness feedback is attached. Related searches found no true duplicate; #1378 is the source issue, and #1225/#575/#657/#363 are adjacent, distinct work.

@kshivang
kshivang marked this pull request as draft September 22, 2026 04:12
@kshivang
kshivang force-pushed the dev branch 2 times, most recently from bdcf620 to bc9aa3b Compare September 22, 2026 04:42
@Mihir-Rabari
Mihir-Rabari force-pushed the agent/flag-kernel-ipc-limits branch 2 times, most recently from 5165b66 to dca81c3 Compare September 22, 2026 04:55
@Mihir-Rabari

Copy link
Copy Markdown
Contributor Author

Rebased the six-file IPC change onto current dev. The client channel now applies maxInboundMetadataSize as well as maxInboundMessageSize, with an authenticated oversized-response-metadata regression and a normal-channel control. The first hosted run showed Netty reporting INTERNAL for the metadata refusal on Ubuntu/Windows; the test now accepts that transport refusal as well as RESOURCE_EXHAUSTED, and fresh authenticated IPC CI is running on the updated head.

@fluck-boss

Copy link
Copy Markdown
Collaborator

@claude review this PR - security-sensitive changes, part of the backlog sweep. Focus on auth, signing, path handling, secrets, and injection surfaces.

@claude

claude Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

Claude finished @fluck-boss's task in 3m 31s —— View job


Security review - kernel IPC transport limits

  • Gather PR diff and read changed files
  • Review IpcTransportLimits (caps, symmetry, defaults)
  • Review IpcAddressResolver rewrite (path handling, socket perms)
  • Review client/server wiring and test coverage
  • Post review

Verdict: the change is sound and does not weaken auth. The metadata asymmetry from the previous review round is fixed (applyToChannel now sets both caps, IpcTransportLimits.kt:58), the caps sit at the deframer so a refusal happens before the token interceptor and before any handler, and the refusals are deliberately kept off UNAUTHENTICATED/PERMISSION_DENIED so abuse cannot be confused with an identity failure. I confirmed no production code path builds a Netty channel or server outside BossIpcClient/BossIpcServer, so the limits are not bypassable by another call site (the remaining direct NettyChannelBuilder uses are all tests, including the intentional channelWithToken negative-test escape hatch).

Findings below, ordered by what I'd want fixed before merge.


1. Both message-cap tests pass against a mutant that deletes the caps

IpcTransportLimitsTest.kt:61 and :109 size the abusive payload at DEFAULT_MAX_INBOUND_MESSAGE_BYTES + 1, and that constant is - by the KDoc's own admission at IpcTransportLimits.kt:65 - exactly io.grpc's own default. So delete .maxInboundMessageSize(...) from applyToServer and from applyToChannel and both tests still go green: the library default refuses the frame. The suite therefore pins the library's behaviour, not this class's.

The oversized-metadata test gets this right and is the model to copy: it configures IpcTransportLimits(maxInboundMetadataBytes = 1_024) at :160 and sends 2 KiB, so it fails if the channel line goes away. Do the same for messages - a small configured maxInboundMessageBytes with a payload over it, on both legs.

Given this repo's AGENTS.md standard ("mutation-verified: reversing each one fails a named test"), this is the one I'd hold the merge on.

Fix this →

2. The server-side (request) metadata cap has no test at all

The PR comment says the new client-side regression lands "alongside the existing server-side/request metadata coverage", but I could not find any. Grepping modules/boss-ipc/src/test for oversize-metadata coverage turns up only IpcTlsIdentityTest's oversized endpoint material test and IpcTransportTest's oversized version marker test - neither exercises maxInboundMetadataSize on the server. So the one leg a hostile local peer actually drives (request headers into the kernel) is the unpinned one, while the leg that only matters against a hostile kernel is pinned. Worth adding the mirror test: oversized request headers via a ClientInterceptor, against a server configured with a small maxInboundMetadataBytes.

3. The per-connection stream cap is bypassable by opening more connections, and the KDoc doesn't say so

maxConcurrentCallsPerConnection = 128 bounds streams per connection; nothing bounds the number of connections. The stated threat model is "the IPC endpoint is reachable by every process on the machine", and such a process can open N connections × 128 streams. maxConnectionIdle doesn't help - it only reaps connections with no RPCs in flight, and a flooder always has one.

That doesn't make the cap useless (it bounds a single accidental runaway, and it does advertise via SETTINGS), but it's a mitigation rather than a bound, and the "Deliberately not bounded here" paragraph at IpcTransportLimits.kt:25 lists connection age and keepalive rates while omitting connection count - the one a reader most needs to know is still open. Either bound it (a Netty connection-counting handler, or a bounded ServerBuilder.executor) or add it to that list explicitly. Documenting it is a legitimate outcome; leaving it unmentioned next to a list of considered-and-rejected bounds implies it was covered.

4. Exceeding the stream cap is a silent client-side queue, not an error - and nothing in production sets a deadline

gRPC-java's client queues streams above the peer's MAX_CONCURRENT_STREAMS rather than failing them. So a legitimate child that holds 128 long-lived streams (PluginUI + state watches + event subscriptions all multiplex on one channel) doesn't get RESOURCE_EXHAUSTED on the 129th call - it gets an indefinite hang, with no log on either side. I grepped for withDeadlineAfter and the only hits are in tests, so there's no deadline to convert that hang into a failure. The test at :180 confirms the behaviour (the flood's extra streams simply never start) but reads it as "refused", which is true server-side and not what the caller experiences.

Not a security hole, but a diagnosability trap worth a sentence in the KDoc, and an argument for either deadlines on kernel unary calls or a server-side log when the cap is hit.

5. Published-jar binary compatibility

AGENTS.md records that boss-ipc publishes as an upstream jar consumed by the standalone boss-microkernel-runtime repo. The new trailing default parameters change JVM descriptors: BossIpcClient(String, IpcClientCredentials, List), BossIpcServer(String, ProcessTokenRegistry, IpcTlsIdentity), and IpcAddressResolver.configureServerBuilder(String) / configureChannelBuilder(String) no longer exist as such - a consumer compiled against the published 1.2.0 jar hits NoSuchMethodError rather than a compile error. There's no .api dump in this module to catch it. If the runtime repo is built against a published jar rather than from source, @JvmOverloads on the two constructors (and keeping the one-arg resolver overloads) makes this free. Worth confirming which it is rather than assuming.


Checked and clear

  • Auth: caps are applied at the transport, before ProcessTokenServerInterceptor, so a header flood is refused pre-auth without consuming registry work. No token, credential or certificate material is logged or widened. The token still rides its own metadata key well under 8 KiB.
  • Idle reap side effects: BossIpcClient.isConnected returns state == READY, which a 2-minute idle GOAWAY will now flip to false - but it has no production consumers, so no health/supervision surface regresses. Reconnect on next call is proven by the :230 test.
  • Injection / secrets: nothing in the diff builds a command, path or query from peer input; the test-only oversized-response header is a fixed literal.
  • IpcAddressResolver rewrite: pure re-indentation to hang limits.applyTo* off the end - git diff -w shows no behavioural change to address parsing, the loopback requirement, or validateProcessIdentifier.

Pre-existing, adjacent - flagging, not attributing to this PR

configureServerBuilder still does an unconditional File(parsed.path()).delete() (IpcAddressResolver.kt:147) on a unix:// path that parseAddress does not validate at all - no confinement to ipcDir, unlike the careful PROCESS_ID_REGEX guard on the generated path. kernelAddress() takes BOSS_KERNEL_IPC_ADDR straight from the environment. Environment control largely implies process control, so the severity is low, but unix:// deserves the same confinement check tcp:// gets for loopback. Related: ipcDir is created with mkdirs() and no owner-only mode, where this repo's run-directory and policy writers pin 0600/owner-only. Both are worth their own issue rather than scope creep here.
· branch agent/flag-kernel-ipc-limits

@fluck-boss

Copy link
Copy Markdown
Collaborator

Verified the review claims against the real code:

  1. Message-cap tests do not discriminate pin removal: CONFIRMED, fixed (commit above). Both oversize tests used DEFAULT_MAX_INBOUND_MESSAGE_BYTES + 1 - equal to gRPC's built-in default, so removing the explicit pin changed nothing. They now pin 64KiB caps with payloads one byte over, so removal fails the test.
  2. No server-side request-metadata-cap test: CONFIRMED, needs you. Please add one: server with a small maxInboundMetadataBytes (e.g. 512), a request carrying an oversized header, asserting a transport-level refusal distinct from UNAUTHENTICATED - mirroring the response-metadata test you already have.
  3. Per-connection stream cap: not a defect - server-side refusal is test-proven and the client-side queuing is standard HTTP/2 SETTINGS behavior, as the class doc states.

@fluck-boss

Copy link
Copy Markdown
Collaborator

@claude please review this exact head (a411903) for merge into dev. Check that the change is correct and complete for its stated goal, that tests cover the risk being fixed (a test that passes without the fix is a finding), and flag any security issues (secrets, injection, auth or approval bypass, path handling). Keep the review focused on this diff.

@claude

claude Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Claude finished @fluck-boss's task in 5m 48s —— View job


Security + correctness review - a4119037 for merge into dev

  • Confirm the diff is the six-file IPC change (no stale-base sprawl)
  • Verify each builder line is mutation-discriminated by a named test
  • Check client/server wiring and bypass surfaces
  • Security pass: auth ordering, secrets, injection, path handling
  • Attempt the focused test run locally (blocked - see bottom)

Verdict: correct and nearly complete for its stated goal. One builder line is still unpinned, and it is the inbound leg. The scope is now the six files claimed (git diff --stat 2e16b8d~1 HEAD = 491/63 across exactly those files), the metadata asymmetry from the first round is fixed, and the three fixes flagged last round as non-discriminating have been repaired properly - both message tests now pin 64 KiB caps below gRPC's 4 MiB default, so deleting either builder line fails a named test.

I checked each of the six builder lines against the suite:

Line Pinned by Mutant (delete the line)
applyToServer message oversized request is refused... (:60, 64 KiB cap, 64 KiB+1 payload) publish succeeds, assertFailsWith fails ✅
applyToServer metadata nothing whole suite still green ❌
applyToServer concurrent calls a connection cannot exceed its concurrent stream cap... (:191, cap 2, flood 5) sampling loop at :205 observes 5 > 2 ✅
applyToServer idle an idle connection is reaped... (:241) channel stays READY, withTimeout fires ✅
applyToChannel message oversized response is refused at the child reader... (:111) client accepts 64 KiB+1, assertFailsWith fails ✅
applyToChannel metadata oversized response metadata... (:170, 1 KiB cap, 2 KiB header) default 8 KiB accepts it ✅

1. The server-side request-metadata cap has no test - hold on this

applyToServer sets .maxInboundMetadataSize(maxInboundMetadataBytes) (IpcTransportLimits.kt:53) and nothing exercises it. Grepping every reference to the four fields across modules/ and composeApp/ turns up maxInboundMetadataBytes exactly once outside the class itself, at IpcTransportLimitsTest.kt:170, and that is the client leg. Delete line 53 and the entire suite passes.

This is the same gap @fluck-boss confirmed and asked for in the comment above, and it is still absent at this head. It matters more than its size suggests, for two reasons:

  • It is the asymmetric one. The client metadata cap defends against a hostile kernel; the server metadata cap defends against a hostile local peer, which is the threat model the class KDoc actually states ("the IPC endpoint is reachable by every process on the machine"). The leg any process on the box can drive is the unpinned one.
  • The PR description says the client regression lands "alongside the existing server-side/request metadata coverage". There is no such coverage - the nearest neighbours are IpcTlsIdentityTest's oversized endpoint material and IpcTransportTest's oversized version marker, neither of which touches maxInboundMetadataSize. So the merge record currently claims a bound that no test holds.

The mirror of the test at :139 is the shape: IpcTestServer(service, limits = IpcTransportLimits(maxInboundMetadataBytes = 512)), a ClientInterceptor that puts an oversized header on the request, and an assertion that the refusal is transport-level rather than UNAUTHENTICATED - which also proves the useful security property, that a header flood is not laundered into an identity failure.

Fix this →

2. require(maxConnectionIdleMillis > 0) is weaker than the effective contract

IpcTransportLimits.kt:46 accepts any positive value, but NettyServerBuilder.maxConnectionIdle does not honour the whole range: values below one second are clamped up to one second, and values at or beyond its "as large as infinite" threshold (on the order of 1000 days) are converted to disabled. So IpcTransportLimits(maxConnectionIdleMillis = 5) silently becomes 1000, and a caller reaching for a very large sentinel silently turns off the one bound this class was added to introduce - with a passing require either way.

The shipped default (120_000) and the test's 1_000 are both inside the honoured range, so nothing is broken today; this is about the constructor's promise to a future caller, in a module whose whole job is pinning bounds that the library "happens to ship defaults for". Worth tightening the require to a documented range, or naming the floor in the KDoc. Worth confirming the exact thresholds against the pinned grpc = "1.84.0" rather than taking my reading of NettyServerBuilder on trust - I could not run a build here.


Notes - non-blocking, but worth not carrying into the merge record as-is

The KDoc promises RESOURCE_EXHAUSTED more firmly than the wire delivers. IpcTransportLimits.kt:19 says "Oversize requests and responses fail with RESOURCE_EXHAUSTED", while the test comment at :76-84 documents - honestly, and clearly from experience - that the request leg can surface as UNAVAILABLE, CANCELLED or INTERNAL depending on where the teardown catches the writer. The test comment is the accurate one. A consumer coding catch (RESOURCE_EXHAUSTED) to tell "my frame was too big" from "the connection dropped" will be wrong on some platforms, so the class doc should say what the test learned. Same applies to the response-metadata assertion, which already has to accept INTERNAL (:176).

Correcting the previous round on auth ordering. Last round I wrote that the caps sit "before ProcessTokenServerInterceptor, so a header flood is refused pre-auth". That is right for metadata - HTTP/2 header-size enforcement precedes stream creation - and not right for messages: maxInboundMessageSize fires in MessageDeframer when the length prefix is read, which is after interceptCall has run and therefore after the token has been validated against the registry. The class KDoc's own wording ("before any service handler runs", :20) is the correct and defensible claim, and the security property that matters still holds - the body is never buffered and no handler executes. But the stronger pre-auth claim should not be inherited by this merge; an oversize request does cost one token validation.

One assertion in the stream test cannot fail. :231 asserts refusals.none { UNAUTHENTICATED || PERMISSION_DENIED } over a list that, under correct behaviour, is empty or holds only the CANCELLEDs produced by flood.cancel() - because gRPC's client queues streams above the peer's advertised MAX_CONCURRENT_STREAMS rather than failing them, so the excess streams are never refused client-side at all. The load-bearing proof in that test is the sampling loop at :204-210 against bus.activeSubscribers, which is genuinely discriminating and does prove the server-side bound. I agree with @fluck-boss that the queuing is standard SETTINGS behaviour and not a defect; the narrower point is that the comment at :230 ("refused abuse at the transport and never masqueraded as an auth failure") describes an observation the test did not make.

Connection count is still missing from the "deliberately not bounded" list (:25-30). That paragraph names connection age and keepalive rates as considered-and-excluded, which reads as an exhaustive account, and omits the one dimension that bypasses the cap directly above it: nothing bounds how many connections a peer opens, and maxConnectionIdle does not help against a flooder that always has an RPC in flight. Documenting it is a perfectly good answer - the cap still bounds an accidental runaway and advertises via SETTINGS - but leaving it unmentioned next to a list of rejected bounds implies it was covered.


Verified clear

  • No bypass. Every production builder goes through IpcAddressResolver: KernelBootstrap.kt:490 (kernel server), ChildProcessBootstrap.kt:79/:87 (child server and kernel client), SpawnIpcSecurity.kt:46 (host client). All take the default IpcTransportLimits(), so all four get the caps. The remaining direct NettyChannelBuilder uses are the three composeApp/src/desktopTest bridge tests plus IpcTlsIdentityTest - tests only.
  • Tests run on the real authenticated wire, and the service-level interceptor does not sidestep auth. BossIpcServer.buildAndStart installs ProcessIdentityInterceptor through builder.intercept(...) (:68), which wraps outside anything ServerInterceptors.intercept attaches to a service - so the oversized-headers harness at :142 is layered inside a genuine token check, and channelFor issues a real token for every channel. The IpcTestServer change is a pure parameter addition with clientLimits defaulting to the server's, and the response-metadata test's use of a default-limits control channel on the same oversized-header server (:179) is a good touch: it proves the refusal came from the configured 1 KiB cap and not from the header's mere presence.
  • IpcAddressResolver rewrite is behaviour-preserving. git diff --ignore-all-space shows only the two signatures, two KDoc lines, and return limits.applyTo*(builder) appended - the 130-line churn is re-indentation to hoist the when into a local. Address parsing, the loopback requirement, validateProcessIdentifier and secureSocketFile's owner-only chmod are untouched.
  • Secrets, injection, path handling. Nothing in the diff logs token, certificate or credential material, and nothing widens what is logged. No command, path, or query is built from peer input; the only peer-influenced string added anywhere is the test-only fixed literal "x".repeat(2_048). No new file or socket path is constructed.
  • Idle reap has no health-surface fallout. BossIpcClient.isConnected (:87) is state == READY and a 2-minute GOAWAY will now flip it false, but grepping the tree shows no production consumer - the other isConnected hits are unrelated (NetworkMonitorService, CoreAuthService, PluginProcessMonitor). Reconnect-on-next-call is proven at :262.
  • Binary compatibility - correcting last round's point 5. I flagged the new trailing default parameters as a NoSuchMethodError risk for the boss-microkernel-runtime consumer. Checking rather than assuming: no module under modules/ applies libs.plugins.mavenPublish (only plugin-platform/* does), and publish-maven-central.yml lists no IPC package - so nothing in this repo publishes boss-ipc, there is no publish task to break, and the absence of an .api dump is consistent with that. The residual risk is only an out-of-band jar consumer; if one exists, @JvmOverloads on the two constructors makes it free, but this is not a merge blocker.

What I could not run

Gradle invocations are outside my allowed tools here (./gradlew :boss-ipc:compileKotlin was denied, and there is no populated Gradle cache in this checkout), so I could not execute :boss-ipc:test --tests 'ai.rever.boss.ipc.IpcTransportLimitsTest'. Every mutation verdict in the table above is read from the sources, not observed. The PluginUIServiceGrpc.java generic mismatch from the first round looks like a stale-generated-source artefact of that local tree rather than a product fault, consistent with hosted CI being green - the authenticated IPC job is the one to trust here.
· branch agent/flag-kernel-ipc-limits

@fluck-boss fluck-boss added the changes requested Review changes must be addressed before merge label Sep 24, 2026

@fluck-boss fluck-boss left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks @Mihir-Rabari - Claude calls this correct and nearly complete. One item still open on a411903, the same one I flagged earlier as needing you: the server-side request-metadata cap (maxInboundMetadataBytes on the inbound leg) has no test. Add one - a server with a small cap and a client sending metadata one byte over, failing if the pin is removed. Optional while you're there: require(maxConnectionIdleMillis > 0) is weaker than the effective contract.

Changes requested + draft until then. Push, get CI green, mark ready for review and it goes straight to merge.

@fluck-boss
fluck-boss marked this pull request as draft September 24, 2026 07:54
@Mihir-Rabari
Mihir-Rabari force-pushed the agent/flag-kernel-ipc-limits branch from a411903 to 601aa2d Compare September 24, 2026 12:13
Mihir-Rabari and others added 6 commits September 24, 2026 12:36
The microkernel IPC endpoint is reachable by every local process. Pinned
TLS and per-call process tokens prove the endpoint and the caller, but
nothing bounded what a peer could demand from the transport itself: one
connection could hold unlimited concurrent streams (gRPC defaults to
Integer.MAX_VALUE) and a connection with no RPCs in flight was never
closed (idle eviction ships disabled), so a stalled or hostile peer kept
its slot forever. The 4 MiB message and 8 KiB metadata caps existed only
as undocumented library defaults.

IpcTransportLimits now applies refuse-style bounds at both ends of the
wire. Requests and responses over the cap fail with RESOURCE_EXHAUSTED
before any handler runs. Excess concurrent streams on one connection are
refused, and well-behaved clients additionally see the cap advertised in
HTTP/2 SETTINGS. A connection idle for two minutes gets a graceful
GO_AWAY that clients survive by reconnecting on demand. Total connection
age is deliberately left unbounded because PluginUI and state watch
streams are designed to stay open indefinitely, and active RPCs already
exempt a connection from the idle reap.

Adversarial tests pin each contract on the real authenticated transport:
oversize request and response frames are refused while well-formed
traffic on the same channel still succeeds, a five-stream flood on one
connection never exceeds the configured cap server-side and recovers
once streams close, and a stalled connection is reaped by the server
with the next call transparently reconnecting.

Post first-CI corrections: the oversize request test pinned observable
surface that the transport's own teardown race owns, not the contract.
First, the exact client-observed status of that mid-write teardown is
platform timing - CI surfaced CANCELLED on ubuntu (write interrupted in
flight) and INTERNAL on windows (netty escalates the teardown into a
connection GOAWAY) while macos landed the server's RESOURCE_EXHAUSTED
trailers. The refusal held on every leg (the oversized frame never
reached a handler), so the pin is now the same transport-level guard
the stream flood test holds. Second, the follow-up well-formed call
raced the replacement transport that a GOAWAY teardown leaves behind -
the second windows run showed a single-shot call surfacing that race as
UNAVAILABLE without any lockout - so recovery is now proven across that
bounded window and still fails on a genuine lockout when the deadline
expires. Test-only changes.
… tests

Both oversize tests sized their payloads against DEFAULT_MAX_INBOUND_MESSAGE_BYTES,
which equals gRPC's built-in 4MiB cap: deleting the explicit pin would leave
the library refusing the same payload and the tests would pass with the bound
gone. Caps pinned at 64KiB with payloads one byte over make removal visible.
@Mihir-Rabari
Mihir-Rabari force-pushed the agent/flag-kernel-ipc-limits branch from 601aa2d to 3233d93 Compare September 24, 2026 12:36
@Mihir-Rabari
Mihir-Rabari marked this pull request as ready for review September 24, 2026 12:56
@Mihir-Rabari

Copy link
Copy Markdown
Contributor Author

Both review points are addressed:

  • The server-side maxInboundMetadataBytes cap now has its own test: oversized request metadata is refused at the server and later calls are unaffected in IpcTransportLimitsTest. The server is pinned at 1_024, the injected header value alone is one byte over the cap, the refusal is asserted to surface at the transport rather than as an auth-shaped code, and a well-formed call on the same capped server is still admitted.
  • require now enforces maxConnectionIdleMillis >= MIN_CONNECTION_IDLE_MILLIS (new companion const, 1_000 ms), because grpc-netty's NettyServerBuilder.maxConnectionIdle silently clamps any window shorter than one second up to one second, so a lower pin would pass construction but widen on the transport.

The branch was also rebased onto current dev (9e41738); range-diff confirms no other changes.

The first hosted run (601aa2d) failed :boss-ipc:detekt on MaxLineLength — the wrapped require message was 134 columns against detekt's 120 (ktlint's 140 let it through locally). The message is now split across two lines at 85ffc16 with the runtime message unchanged; the module's detekt + ktlint gates pass locally.

Hosted checks green on 3233d93. Marking ready for review.

@fluck-boss

Copy link
Copy Markdown
Collaborator

@claude please review this exact head (3233d93) for merge into dev. Check that the change is correct and complete for its stated goal, that tests cover the risk being fixed (a test that passes without the fix is a finding), that the earlier review findings on this PR are addressed, and flag any security issues (secrets, injection, auth or approval bypass, path handling). Keep the review focused on this diff.

@claude

claude Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Claude finished @fluck-boss's task in 3m 56s —— View job


Review of 3233d937 for merge into dev

  • Confirm the diff scope at this head
  • Verify the new server-side request-metadata test discriminates the pin
  • Re-check the require floor change
  • Re-check every other builder line is still mutation-discriminated
  • Security pass: auth ordering, secrets, injection, path handling
  • Post review

Verdict: both open items from the last round are properly addressed. I have no blocking finding at this head. Scope is still the six claimed files (git diff --stat 6ca338a~1 HEAD = 570/63, nothing else), and every one of the six builder lines is now pinned by a test that fails if the line is deleted.

Builder line Pinned by Mutant (delete the line)
applyToServer message oversized request is refused... (:62, cap 64 KiB, payload 64 KiB+1) 4 MiB library default admits it, assertFailsWith fails ✅
applyToServer metadata oversized request metadata is refused at the server... (:113, cap 1 KiB, header value 1 KiB+1) 8 KiB default admits it, assertFailsWith fails ✅
applyToServer concurrent calls a connection cannot exceed its concurrent stream cap... (:253, cap 2, flood 5) sampling loop at :271 observes 5 > 2 ✅
applyToServer idle an idle connection is reaped... (:306, 1 s window) channel stays READY, withTimeout fires ✅
applyToChannel message oversized response is refused at the child reader... (:173, reader cap 64 KiB, default-limits server) client accepts 64 KiB+1 ✅
applyToChannel metadata oversized response metadata... (:206, reader cap 1 KiB, 2 KiB header) 8 KiB default accepts it ✅

The new server-metadata test: checked properly, it does discriminate

This is the one I held the merge on last round, so I sized it rather than eyeballing it. The server pins 1024 and the injected header value alone is 1025 bytes, so on HPACK accounting (name + value + 32) that single header is 1074 - over the configured cap by itself, which is what makes the test independent of how the transport counts everything else. The rest of the header list is small and bounded: the process token is 64 hex chars (ProcessTokenRegistry.issue formats 32 random bytes, ProcessTokenRegistry.kt:32) plus the usual pseudo-headers, te, content-type, user-agent - call it ~1.5 KiB total. Comfortably under gRPC's 8 KiB default, so deleting IpcTransportLimits.kt:59 admits the request and assertFailsWith fails. That is the property the last two rounds were missing.

Two details worth recording because they are easy to get wrong and this got both right:

  • The refusal traces to the server's setting, not the client's. channelFor defaults clientLimits to the server's limits, so the writer channel also carries maxInboundMetadataSize = 1024 - but that bounds what the client will accept inbound, and gRPC does not bound outbound headers against it. What refuses the write is the server's SETTINGS_MAX_HEADER_LIST_SIZE, which is exactly the line under test. No accidental double-pin.
  • The control call (:166) opens a fresh BossIpcClient, so it survives the case where netty escalates the header refusal into a connection-level GOAWAY. The comment claims only what that proves ("a peer sending ordinary headers is admitted by the same capped server"), not same-connection recovery.

The require floor (:49) matches grpc-netty's actual behaviour: NettyServerBuilder.maxConnectionIdle clamps anything under MIN_MAX_CONNECTION_IDLE_NANO (1 s) up to 1 s, so the old > 0 really did accept values that widened silently. The test's 1 s window sits exactly on the new floor and still passes construction.


Residual notes - none blocking, ordered by how much I'd want them

1. The idle window is now guarded at the floor and still open at the ceiling. maxConnectionIdle has a second silent transform at the top of its range: at or beyond AS_LARGE_AS_INFINITE (1000 days) it converts the value to disabled, not to a very long window. So IpcTransportLimits(maxConnectionIdleMillis = Long.MAX_VALUE) passes the new require and turns off the one bound this class was added to introduce. Nothing does this today (LONG_IDLE_MILLIS is 60 s), but it is the same class of silent widening that was just closed on the other end, in a class whose entire job is honest pins - a matching require(maxConnectionIdleMillis < MAX_CONNECTION_IDLE_MILLIS) with a 1000-day constant would make the contract symmetric. Worth confirming the exact threshold against the pinned grpc = "1.84.0" rather than my reading.

Fix this →

2. The class KDoc still promises RESOURCE_EXHAUSTED more firmly than the wire delivers. IpcTransportLimits.kt:19 says "Oversize requests and responses fail with RESOURCE_EXHAUSTED before any service handler runs", while the test comments at :81-91 and :152-157 document - from experience, and accurately - that the request leg surfaces as UNAVAILABLE, CANCELLED or INTERNAL depending on where the teardown catches the writer, and the response-metadata assertion at :243 has to accept INTERNAL. The test comment is the true one. A consumer coding catch (RESOURCE_EXHAUSTED) to tell "my frame was too big" from "the connection dropped" will be wrong on some platforms. One sentence in the KDoc closes the gap between what the class promises and what its own suite learned. Carried over from last round, still open.

3. Connection count is still absent from the "Deliberately not bounded here" paragraph (:25-30). That paragraph names connection age and keepalive rates as considered-and-excluded, which reads as an exhaustive account of what was thought about, and omits the one dimension that walks straight around the cap directly above it: nothing bounds how many connections a peer opens, and maxConnectionIdle does not help against a flooder that always has an RPC in flight (N connections x 128 streams). Documenting it is a perfectly good answer - the cap still bounds an accidental runaway and advertises via SETTINGS - but leaving it unlisted next to a list of rejected bounds implies it was covered. Carried over from two rounds ago, still open.

4. One assertion in the stream test cannot fail (:298). refusals.none { UNAUTHENTICATED || PERMISSION_DENIED } runs over a list that, under correct behaviour, is empty: gRPC's client queues streams above the peer's advertised MAX_CONCURRENT_STREAMS rather than failing them, and flood.cancel() raises CancellationException, which the handler's as? StatusException drops. The load-bearing proof in that test is the sampling loop at :271-277 against bus.activeSubscribers, which genuinely discriminates. I agree with @fluck-boss that the queuing is standard SETTINGS behaviour and not a defect; the narrow point is that the comment at :297 describes an observation the test did not make.

5. Binary compatibility - re-opening my own correction from last round. I checked and confirmed again that no module under modules/ applies a publish plugin and publish-maven-central.yml lists no IPC package, so there is no in-repo publish task to break. But this repo's own AGENTS.md states that boss-ipc/boss-ui-sdk "publish as upstream jars consumed by the standalone boss-microkernel-runtime repo", and boss-ipc/build.gradle.kts does carry group/version = "1.2.0" - so an out-of-band consumer plausibly exists. The new trailing default parameters delete the JVM descriptors BossIpcClient(String, IpcClientCredentials, List), BossIpcServer(String, ProcessTokenRegistry, IpcTlsIdentity), IpcAddressResolver.configureServerBuilder(String) and configureChannelBuilder(String); a consumer compiled against the published 1.2.0 jar gets NoSuchMethodError, not a compile error, and there is no .api dump here to catch it. @JvmOverloads on the two constructors and the two resolver functions makes this free. Worth a yes/no from whoever owns the runtime repo (source build or jar?) rather than an assumption either way.


Verified clear

  • No bypass surface. Every production builder still goes through IpcAddressResolver, and all four production instantiations take the default IpcTransportLimits(): KernelBootstrap.kt:493, ChildProcessBootstrap.kt:79/:87, SpawnIpcSecurity.kt:46. The only remaining direct NettyChannelBuilder/NettyServerBuilder uses in the tree are the four composeApp/src/desktopTest bridge tests, IpcTlsIdentityTest, and IpcTestServer.channelWithToken - the intentional negative-test escape hatch.
  • Tests run on the real authenticated wire. IpcTestServer issues a genuine token per channel, and BossIpcServer.buildAndStart installs ProcessIdentityInterceptor via builder.intercept(...), which wraps outside anything ServerInterceptors.intercept attaches to a service - so both metadata harnesses are layered inside a real token check rather than around it.
  • Auth ordering. Metadata caps are enforced at HTTP/2 header decode, before stream creation and therefore before the token interceptor; message caps fire in MessageDeframer on the length prefix, which is after interceptCall but still before any handler and before the body is buffered. The KDoc's own wording ("before any service handler runs") is the defensible claim and is the one used. Refusals are deliberately kept off UNAUTHENTICATED/PERMISSION_DENIED, asserted in both request-side tests, so abuse cannot be laundered into an identity failure.
  • Secrets, injection, path handling. No token, certificate or credential material is logged, and the diff adds no logging at all. Nothing builds a command, path, file name or query from peer input; the only peer-influenced strings introduced anywhere are the two test-only fixed literals ("x".repeat(...)).
  • IpcAddressResolver rewrite is behaviour-preserving. git diff --ignore-all-space reduces the 130-line churn to exactly two signatures, two KDoc lines and two return limits.applyTo*(builder) statements. Address parsing, the loopback requirement, validateProcessIdentifier and secureSocketFile's owner-only chmod are untouched.
  • Idle reap has no health-surface fallout. BossIpcClient.isConnected is state == READY and a 2-minute GOAWAY flips it false, but it has no production consumer. Reconnect-on-next-call is proven at :329.
  • Style gates. No line over 120 columns anywhere under modules/boss-ipc/src, no trailing whitespace, no U+2014 in the new prose - consistent with the detekt MaxLineLength fix at 85ffc16.

Pre-existing, adjacent - flagging, not attributing to this PR

configureServerBuilder still does an unconditional File(parsed.path()).delete() on a unix:// path that parseAddress does not confine to ipcDir, unlike the careful PROCESS_ID_REGEX guard on the generated path, and kernelAddress() reads BOSS_KERNEL_IPC_ADDR straight from the environment. Environment control largely implies process control so the severity is low, but unix:// deserves the confinement check tcp:// gets for loopback. Related: ipcDir is created with mkdirs() and no owner-only mode, where this repo's run-directory and policy writers pin 0600/owner-only. Both deserve their own issue rather than scope creep here.

What I could not run

Gradle invocations are outside my allowed tools in this environment, so :boss-ipc:test --tests 'ai.rever.boss.ipc.IpcTransportLimitsTest' was not executed - every mutation verdict in the table above is read from the sources and from grpc-netty's builder semantics, not observed. Hosted CI reported green on this head including the dedicated authenticated IPC job; that is the run to trust for execution.


· branch agent/flag-kernel-ipc-limits

@fluck-boss

Copy link
Copy Markdown
Collaborator

Merging: ubuntu and windows are green on 3233d93. Claude's re-review finds both open items fixed and no blocking finding: every builder limit is now pinned by a test that fails if the line is removed, including the server-side metadata cap. My diff read agrees: the limits go through IpcTransportLimits into both builders with defaults, and the address-resolver change is a re-indent plus the limits hook. Thanks @Mihir-Rabari!

@fluck-boss
fluck-boss merged commit a1959aa into risa-labs-inc:dev Sep 24, 2026
11 checks passed
kshivang pushed a commit that referenced this pull request Sep 24, 2026
* fix(ipc): bound the kernel IPC transport against abuse

The microkernel IPC endpoint is reachable by every local process. Pinned
TLS and per-call process tokens prove the endpoint and the caller, but
nothing bounded what a peer could demand from the transport itself: one
connection could hold unlimited concurrent streams (gRPC defaults to
Integer.MAX_VALUE) and a connection with no RPCs in flight was never
closed (idle eviction ships disabled), so a stalled or hostile peer kept
its slot forever. The 4 MiB message and 8 KiB metadata caps existed only
as undocumented library defaults.

IpcTransportLimits now applies refuse-style bounds at both ends of the
wire. Requests and responses over the cap fail with RESOURCE_EXHAUSTED
before any handler runs. Excess concurrent streams on one connection are
refused, and well-behaved clients additionally see the cap advertised in
HTTP/2 SETTINGS. A connection idle for two minutes gets a graceful
GO_AWAY that clients survive by reconnecting on demand. Total connection
age is deliberately left unbounded because PluginUI and state watch
streams are designed to stay open indefinitely, and active RPCs already
exempt a connection from the idle reap.

Adversarial tests pin each contract on the real authenticated transport:
oversize request and response frames are refused while well-formed
traffic on the same channel still succeeds, a five-stream flood on one
connection never exceeds the configured cap server-side and recovers
once streams close, and a stalled connection is reaped by the server
with the next call transparently reconnecting.

Post first-CI corrections: the oversize request test pinned observable
surface that the transport's own teardown race owns, not the contract.
First, the exact client-observed status of that mid-write teardown is
platform timing - CI surfaced CANCELLED on ubuntu (write interrupted in
flight) and INTERNAL on windows (netty escalates the teardown into a
connection GOAWAY) while macos landed the server's RESOURCE_EXHAUSTED
trailers. The refusal held on every leg (the oversized frame never
reached a handler), so the pin is now the same transport-level guard
the stream flood test holds. Second, the follow-up well-formed call
raced the replacement transport that a GOAWAY teardown leaves behind -
the second windows run showed a single-shot call surfacing that race as
UNAVAILABLE without any lockout - so recovery is now proven across that
bounded window and still fails on a genuine lockout when the deadline
expires. Test-only changes.

* fix(ipc): cap response metadata at the client reader

* test(ipc): accept transport-specific metadata refusal codes

* test(ipc): pin message caps below the library default in the oversize tests

Both oversize tests sized their payloads against DEFAULT_MAX_INBOUND_MESSAGE_BYTES,
which equals gRPC's built-in 4MiB cap: deleting the explicit pin would leave
the library refusing the same payload and the tests would pass with the bound
gone. Caps pinned at 64KiB with payloads one byte over make removal visible.

* fix(ipc): enforce grpc-netty's one-second idle floor in require (per review)

* test(ipc): cover the server-side inbound metadata cap (per review)

---------

Co-authored-by: Instinct Agent <agent@instinct.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changes requested Review changes must be addressed before merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants