Conversation
…ination Add an SNI router on the shared HTTPS listener that inspects TLS ClientHello SNI, matches TCP routes by alias while honoring domain filters, and either forwards TLS end-to-end without termination or terminates with autocert before plaintext upstream relay when tls_termination is enabled. Wire TCPTCPStream and idlewatcher through ConnProxy.ProxyConn instead of inline handlers. Validate tls_termination for TCP schemes only and refresh README and config examples. Bump internal/go-oidc submodule.
…ls_termination Compare listen addresses with SplitHostPort so empty, 0.0.0.0, and :: wildcard hosts on the HTTPS port match the proxy’s shared HTTPS address for SNI route registration, keys, and passthrough eligibility. Require tls_termination TCP routes to listen on that shared HTTPS listener; add tests for address equality and validation.
Contributor
|
Caution Review failedPull request was closed or merged during review Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an SNI-aware TCP passthrough router with optional TLS termination on the shared HTTPS listener, introduces a ConnProxy API and implementations, extends route model/validation for Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SNIRouter as SNI Router
participant RouteRegistry as Route Registry
participant CertProvider as Cert Provider
participant Upstream as Upstream Server
Client->>SNIRouter: TCP conn (begins TLS handshake)
SNIRouter->>SNIRouter: Read ClientHello (extract & normalize SNI, buffer bytes)
SNIRouter->>RouteRegistry: Lookup route by SNI key
alt Route found & tls_termination = false
SNIRouter->>Upstream: Proxy raw TCP (replay ClientHello bytes)
else Route found & tls_termination = true
SNIRouter->>CertProvider: Obtain cert for SNI
SNIRouter->>SNIRouter: Perform TLS server handshake (terminate TLS)
SNIRouter->>Upstream: Proxy plaintext connection
else No route or extraction fails
SNIRouter->>SNIRouter: Enqueue connection for HTTPS passthrough (replay bytes)
end
Upstream->>Client: Response (proxied back)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/entrypoint/http_server.go (1)
87-124:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClose the temporary SNI listener on startup failure.
srv.ep.sni.Listen(addr)already starts the accept loop and caches the listener. Ifserver.StartServerfails on this path, the new listener and the HTTP subtask are left behind, which can leak the accept goroutine and make retries reuse a stale listener.Suggested cleanup
sniListener, err := srv.ep.sni.Listen(addr) if err != nil { return err } listener = sniListener @@ _, err := server.StartServer(task, opts) if err != nil { + _ = listener.Close() return err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/entrypoint/http_server.go` around lines 87 - 124, When you create sniListener via srv.ep.sni.Listen(addr) (inside the proto==HTTPProtoHTTPS && listener==nil && certProvider!=nil branch) ensure the listener is closed if server.StartServer fails: capture the created sniListener (and assign it to listener) and on any error returned by server.StartServer(task, opts) close listener (or sniListener) before returning the error (check/propagate Close() error if desired) to avoid leaking the accept goroutine and a stale cached listener.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/route/README.md`:
- Around line 96-100: Update the TLSTermination/tls_termination documentation to
explicitly state the autocert prerequisite: mention that TLSTermination
(tls_termination) is valid only for TCP routes on the shared HTTPS listener and
will be rejected at registration time unless an autocert provider is configured;
reference the registration-time enforcement and that enabling TLSTermination
causes Godoxy to match ClientHello SNI, terminate TLS via the autocert provider,
and proxy plaintext to the TCP target while absence of autocert falls back to
SNI passthrough.
In `@internal/route/route.go`:
- Around line 376-393: Extract the host/port wildcard matching logic into a
single helper and use it from both call sites: move the functions
isSharedHTTPSListenAddr and isWildcardListenHost into a shared package (e.g.,
internal/netutil or similar), export them (e.g., SharedHTTPSListenAddr or
IsSharedHTTPSListenAddr and IsWildcardListenHost), update usages in
internal/route/route.go and internal/entrypoint/sni_passthrough.go to call the
new shared helpers, and adjust imports accordingly so both files use the
identical matcher implementation.
In `@internal/route/stream/README.md`:
- Around line 191-200: Update the README examples so they don't hardcode port
443: either add a note that the shown port 443 assumes the default
HTTPS_ADDR=:443 or replace the literal ports with a placeholder tied to
HTTPS_ADDR (e.g., use HTTPS_ADDR in the example port comment) and document that
tls_termination is validated against the shared HTTPS listener; reference the
two example blocks named site2 and mqtt and ensure the mqtt tls_termination
explanation clarifies it checks the configured HTTPS_ADDR, not the literal 443
value.
---
Outside diff comments:
In `@internal/entrypoint/http_server.go`:
- Around line 87-124: When you create sniListener via srv.ep.sni.Listen(addr)
(inside the proto==HTTPProtoHTTPS && listener==nil && certProvider!=nil branch)
ensure the listener is closed if server.StartServer fails: capture the created
sniListener (and assign it to listener) and on any error returned by
server.StartServer(task, opts) close listener (or sniListener) before returning
the error (check/propagate Close() error if desired) to avoid leaking the accept
goroutine and a stale cached listener.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2e4c1908-013d-4573-bdf4-0e8793ff3bdb
📒 Files selected for processing (17)
config.example.ymlinternal/entrypoint/entrypoint.gointernal/entrypoint/entrypoint_test.gointernal/entrypoint/http_server.gointernal/entrypoint/routes.gointernal/entrypoint/sni_passthrough.gointernal/entrypoint/sni_passthrough_test.gointernal/go-oidcinternal/idlewatcher/handle_stream.gointernal/idlewatcher/handle_stream_test.gointernal/net/types/stream.gointernal/route/README.mdinternal/route/route.gointernal/route/route_test.gointernal/route/stream/README.mdinternal/route/stream/tcp_tcp.gointernal/route/stream/tcp_tcp_test.go
…hing helpers Move equivalence logic next to ProxyHTTPSAddr into internal/net/listen_addr.go and use it from SNI passthrough and tls_termination validation, dropping duplicated helpers. Close the SNI listener and finish the http_server task when StartServer fails after SNI Listen. Update README examples for autocert-gated tls_termination and HTTPS_ADDR-relative ports.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/entrypoint/http_server.go`:
- Around line 88-97: The HTTPS SNI listener should be installed regardless of
whether an autocert provider exists: in the branch handling proto ==
HTTPProtoHTTPS where listener == nil, call srv.ep.sni.Listen(addr)
unconditionally (remove the certProvider != nil gating) and assign sniListener
and listener from that call; preserve the subsequent assignments to aclCfg and
supportProxyProtocol as needed (adjust only the certProvider check, not
sni_passthrough.go registration), so passthrough-only SNI routes work when
certProvider == nil.
In `@internal/entrypoint/sni_passthrough_test.go`:
- Around line 70-75: TestSharedHTTPSListenAddrMatchesWildcardDefault is
hardcoding ":443" which breaks when HTTPS_ADDR changes; update the test to use
the configured common.ProxyHTTPSAddr instead of literal ports. Replace
occurrences like "0.0.0.0:443" and "[::]:443" with constructions that append
common.ProxyHTTPSAddr (or build the host:port with net.JoinHostPort using the
port portion of common.ProxyHTTPSAddr) and keep the negative case (e.g.,
"0.0.0.0:<other>") and positive loopback case using the same configured addr;
call out netutils.IsSharedHTTPSListenAddr and common.ProxyHTTPSAddr when making
the change.
In `@internal/net/listen_addr.go`:
- Around line 39-41: IsWildcardListenHost currently only checks a few literal
strings; update it to correctly detect all unspecified IP forms by first
stripping any port (and surrounding IPv6 brackets) from the input, then using
net.ParseIP on the host portion and returning true if the parsed IP is nil for
empty input or ip.IsUnspecified() is true; retain the empty-string check but
remove reliance on hard-coded "::"/"0.0.0.0" strings and handle bracketed IPv6
addresses like "[0:0:0:0:0:0:0:0]:443" and plain "0.0.0.0:443" via this
parse-and-IsUnspecified approach in IsWildcardListenHost.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cd6e18a2-0b2f-4d3b-abec-62463989aec4
📒 Files selected for processing (8)
internal/entrypoint/http_server.gointernal/entrypoint/sni_passthrough.gointernal/entrypoint/sni_passthrough_test.gointernal/net/README.mdinternal/net/listen_addr.gointernal/route/README.mdinternal/route/route.gointernal/route/stream/README.md
… wildcard host checks Remove the autocert guard so the shared HTTPS listener still starts the SNI multiplexer for passthrough when no certificate provider is configured. Extend IsWildcardListenHost for trimmed input, host:port and bracketed IPv6, using net.ParseIP and IsUnspecified for IPv4/IPv6. Add listen_addr tests, README detail, and align SNI passthrough tests with ProxyHTTPSAddr.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/entrypoint/sni_passthrough_test.go`:
- Around line 77-84: Wrap each address iteration in a subtest so failures are
scoped: in the loop that iterates the addresses and calls require.Equal/True,
call t.Run with a descriptive name (e.g., the address or host string), capture
the loop variable into a local (e.g., a := addr) to avoid closure capture
issues, and move the two assertions into the subtest function body using the
same checks netutils.IsSharedHTTPSListenAddr and netutils.IsWildcardListenHost
so behavior is unchanged but each case reports its own failure.
In `@internal/net/listen_addr_test.go`:
- Around line 11-29: The table-driven checks in listen_addr_test.go should be
scoped with t.Run so each host case is isolated on failure: replace the plain
for-loops that call require.True/False on IsWildcardListenHost(host) with
iterations that call t.Run(name, func(t *testing.T) { require.True/False(t,
IsWildcardListenHost(host), host) }) using the host (or an index) as the subtest
name; ensure you import testing.T in the closures and keep the same assertions
(require.True/False) and targets (IsWildcardListenHost) so behavior is unchanged
but failures are reported per-host.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3662bed9-6a37-450e-919d-0da8b51322d6
📒 Files selected for processing (5)
internal/entrypoint/http_server.gointernal/entrypoint/sni_passthrough_test.gointernal/net/README.mdinternal/net/listen_addr.gointernal/net/listen_addr_test.go
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 2 file(s) based on 2 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 2 file(s) based on 2 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/entrypoint/sni_passthrough_test.go`:
- Around line 48-70: The test TestSNIRouterListensPerHTTPSAddress can flake due
to TOCTOU when reserveSNIListenAddr closes the socket before ep.sni.Listen binds
the port; modify the test to retry obtaining+binding an address until
ep.sni.Listen succeeds (or a short retry timeout) instead of assuming the first
reserved port remains available: call reserveSNIListenAddr and then attempt
ep.sni.Listen(addr) in a small retry loop (backoff and limit retries) for both
addr1 and addr2, ensuring you still assert listener identity reuse by calling
ep.sni.Listen(addr1) again and comparing to listener1.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0ccc45fe-19f6-4dbd-9c3b-a7f2243f00c9
📒 Files selected for processing (2)
internal/entrypoint/sni_passthrough_test.gointernal/net/listen_addr_test.go
Comment on lines
+48
to
+70
| func TestSNIRouterListensPerHTTPSAddress(t *testing.T) { | ||
| ep := NewTestEntrypoint(t, nil) | ||
| t.Cleanup(func() { | ||
| require.NoError(t, ep.sni.Close()) | ||
| }) | ||
|
|
||
| addr1 := reserveSNIListenAddr(t) | ||
| addr2 := reserveSNIListenAddr(t) | ||
|
|
||
| listener1, err := ep.sni.Listen(addr1) | ||
| require.NoError(t, err) | ||
| listener2, err := ep.sni.Listen(addr2) | ||
| require.NoError(t, err) | ||
|
|
||
| require.NotSame(t, listener1, listener2) | ||
| require.Equal(t, addr1, listener1.Addr().String()) | ||
| require.Equal(t, addr2, listener2.Addr().String()) | ||
| require.Equal(t, 2, ep.sni.listeners.Size()) | ||
|
|
||
| again, err := ep.sni.Listen(addr1) | ||
| require.NoError(t, err) | ||
| require.Same(t, listener1, again) | ||
| } |
Contributor
There was a problem hiding this comment.
Fix potential test flakiness: TOCTOU in port reservation/re-bind.
reserveSNIListenAddr closes the socket and returns the port, and then ep.sni.Listen(addr) re-binds to that same port. Between close → re-bind, another process/test goroutine can take the port, causing intermittent “address already in use” failures.
Suggested fix (retry binding with a freshly reserved addr)
func TestSNIRouterListensPerHTTPSAddress(t *testing.T) {
ep := NewTestEntrypoint(t, nil)
t.Cleanup(func() {
require.NoError(t, ep.sni.Close())
})
- addr1 := reserveSNIListenAddr(t)
- addr2 := reserveSNIListenAddr(t)
+ listenOnFreshAddr := func(excludeAddr string) (net.Listener, string) {
+ t.Helper()
+ for i := 0; i < 20; i++ {
+ addr := reserveSNIListenAddr(t)
+ if addr == excludeAddr {
+ continue
+ }
+ ln, err := ep.sni.Listen(addr)
+ if err != nil {
+ continue
+ }
+ return ln, addr
+ }
+ require.FailNow(t, "failed to bind sni listener on a free port")
+ return nil, ""
+ }
- listener1, err := ep.sni.Listen(addr1)
- require.NoError(t, err)
- listener2, err := ep.sni.Listen(addr2)
- require.NoError(t, err)
+ listener1, addr1 := listenOnFreshAddr("")
+ listener2, addr2 := listenOnFreshAddr(addr1)
require.NotSame(t, listener1, listener2)
require.Equal(t, addr1, listener1.Addr().String())
require.Equal(t, addr2, listener2.Addr().String())
require.Equal(t, 2, ep.sni.listeners.Size())
again, err := ep.sni.Listen(addr1)
require.NoError(t, err)
require.Same(t, listener1, again)
}This keeps the test semantics the same (distinct listeners per addr, and reuse for the same addr) while removing the race.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/entrypoint/sni_passthrough_test.go` around lines 48 - 70, The test
TestSNIRouterListensPerHTTPSAddress can flake due to TOCTOU when
reserveSNIListenAddr closes the socket before ep.sni.Listen binds the port;
modify the test to retry obtaining+binding an address until ep.sni.Listen
succeeds (or a short retry timeout) instead of assuming the first reserved port
remains available: call reserveSNIListenAddr and then attempt
ep.sni.Listen(addr) in a small retry loop (backoff and limit retries) for both
addr1 and addr2, ensuring you still assert listener identity reuse by calling
ep.sni.Listen(addr1) again and comparing to listener1.
Wire `cmd/tcp_echo_server` with plaintext and self-signed TLS listeners, dev.compose labels for passthrough and `tls_termination` aliases, and `scripts/tcp_echo_test.ts` invoked by `make tcp-echo-test` via bun.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#218 #223
Summary by CodeRabbit
New Features
Documentation
Tests