chore: tunnel auth

[NathanFlurry]

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

[NathanFlurry]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• chore: fix driver test suite #4591
• chore: tunnel auth #4588 👈 (View in Graphite)
• chore: remove udb as a dependency of envoy-client #4587
• chore: revert actor v2 hack #4586
• fix: fix pool metadata protocol version refresh, rivetkit native ws #4585
• feat: US-001 - Define SqliteKv trait in rivetkit-sqlite-native #4584
• feat(rust/engine-runner): minimal rust engine runner #4289
• chore: delete kv channel #4583
• feat(engine): rust runner #4582
• chore: remove non-engine drivers #4574 : 1 other dependent PR (#4581 )
• feat: dynamic actors #4397
• fix: driver test suite & sqlite reliability #4559
• fix: remove serverless token #4572
• fix: misc token fixes #4569
• fix(pb): clean up actor stop decision handling #4566
• fix(envoy): use global instance, add signal handlers #4565
• chore: misc fixes, add pb snapshot test #4561
• fix(cache): make in memory cache global #4560
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[claude]

PR Review: chore: tunnel auth (#4588)

Good security addition. The core mechanism — whitelist authorized (gateway_id, request_id) pairs when the engine pushes tunnel messages outbound, then validate those pairs when the envoy/runner sends tunnel messages back — is correct and well-scoped.

Security

The auth model is sound: an envoy/runner can only send a tunnel response for a request the engine explicitly initiated. Using scc::HashMap for concurrent access (per CLAUDE.md guidance) avoids mutex contention and is the right call here.

message_index is intentionally excluded from the auth key. Once a (gateway_id, request_id) pair is authorized, all message_index values for that pair are accepted. This is correct for streaming responses, but deserves an inline comment to make the intent explicit so a future reader doesn't question it.

Memory Growth

authorized_tunnel_routes grows indefinitely for the lifetime of a connection — entries are inserted but never removed. For long-lived connections handling many requests this accumulates without bound. If completed routes are never reused, consider removing entries when the response stream terminates, or at minimum document that per-connection accumulation is acceptable and the map is bounded by request volume.

pegboard-envoy Tests Entirely Commented Out

The test file engine/packages/pegboard-envoy/tests/support/ws_to_tunnel_task.rs is 100% commented out with a // TODO: Use TestCtx note. This leaves the envoy auth enforcement side completely untested at the unit level. The pegboard-runner tests (all four) work and compile, but the two implementations have different test coverage. Per CLAUDE.md's Engine Runner Parity requirement, this gap should be tracked and resolved. Either unblock the envoy tests or file a follow-up task with context on what TestCtx dependency is needed.

Minor: Error Context Dropped in pegboard-envoy

In pegboard-envoy/src/ws_to_tunnel_task.rs, the error context string was removed:

// Before
ctx.ups().context("failed to get UPS instance for tunnel message")?

// After
ctx.ups()?

In pegboard-runner, the equivalent context was preserved at the call site. The envoy change loses diagnostic information for a UPS initialization failure. Easy fix: keep the .context(...) call.

Import Cleanup (Positive)

The merge of the two universalpubsub imports in pegboard-runner's ws_to_tunnel_task.rs is a clean improvement.

Blank Lines

Two unrelated blank lines removed in both conn.rs files (before enum Init and #[tracing::instrument]). Harmless but unrelated to the auth change.

Tests

Coverage in pegboard-runner is thorough: rejection and acceptance paths tested for both mk2 and mk1. The #[path] trick to test private functions is effective. One gap: the tunnel_to_ws_task insert path (the authorization registration side) has no unit tests in either package — only the enforcement side is tested. A test confirming that receiving a ToEnvoyTunnelMessage/ToClientTunnelMessage actually populates authorized_tunnel_routes would close that loop.

Summary

The implementation is correct. Key items before merging:

1. Unblock or track the envoy test gap (CLAUDE.md parity requirement)
2. Address the memory growth question (cleanup or explicit note)
3. Add a comment explaining message_index exclusion from the auth key
4. Restore .context() on ctx.ups()? in pegboard-envoy

[MasterPtato]

Merge activity

• Apr 8, 8:52 PM UTC: A user started a stack merge that includes this pull request via Graphite.