feat(tls-edge): SNI passthrough L4 relay + mTLS edge termination#100
Open
cuioss-oliver wants to merge 12 commits into
Open
feat(tls-edge): SNI passthrough L4 relay + mTLS edge termination#100cuioss-oliver wants to merge 12 commits into
cuioss-oliver wants to merge 12 commits into
Conversation
…y (D1) Add the Vert.x NetServer front listener that reassembles the full TLS ClientHello (RFC 6066), reads the SNI, and either opaquely L4-relays a passthrough_sni match to the topology-resolved backend (the gateway never handshakes) or hands the still-encrypted stream to the internal terminated Quarkus HTTPS listener. Fail-closed per GW-06: empty/unresolved/malformed SNI takes the terminated-strict path. Exposes the resolved topology as a CDI bean, adds relay-lifecycle INFO and fail-closed WARN LogRecords, and reserves the public TLS port. Unit tests cover ClientHello reassembly/SNI extraction, front-listener routing, and L4 relay byte-fidelity/backpressure/half-close/abort.
Add MtlsServerCustomizer, a Quarkus HttpServerOptionsCustomizer that maps tls.mtls.enabled + client_ca onto the terminated HTTPS listener's client-auth: require-and-verify a client certificate against the configured CA trust anchor, with no HTTP fallback and no optional/want mode. Client-auth stays off when tls.mtls is absent or disabled; enabled without client_ca fails the boot. Passthrough connections are split off at L4 and never mTLS-checked. Unit tests cover the enabled/disabled/absent/misconfigured mapping.
Add PassthroughHostGuardStage, a pipeline pre-check that rejects a terminated request whose Host header names a passthrough_sni hostname with 404 (PASSTHROUGH_HOST_SMUGGLED) before route selection, closing the host-confusion vector against the passthrough backend identity. Matching mirrors the front listener's case-insensitive, dot/port-normalized SNI handling; the stage is inert when passthrough_sni is empty. Cover the guard with a negative-test suite and extend the EventType catalogue test with the new PASSTHROUGH_HOST_SMUGGLED member. Co-Authored-By: Claude <noreply@anthropic.com>
…es (D4) Add a pinned Toxiproxy service and a TLS-enabled passthrough backend (serving its own CN=passthrough-backend certificate) to the integration-tests compose stack as purely additive services — existing services are untouched. Add four black-box integration suites exercising the TLS edge against the running stack under -Pintegration-tests: - TlsPassthroughIT: the passthrough relay surfaces the backend's certificate identity (opaque relay proof); terminated traffic on the same listener still served. - MtlsHandshakeIT: mTLS accept/reject proven at the handshake level (trusted cert, no cert, wrong-CA cert). - HostSmuggleGuardIT: a terminated Host naming a passthrough SNI is rejected 404. - PassthroughFaultIT: a mid-stream Toxiproxy reset propagates as a connection abort, not a hang, driven via the Toxiproxy REST control API. Co-Authored-By: Claude <noreply@anthropic.com>
Reference layer: add a TLS-active status section to doc/configuration.adoc documenting the now-enforced tls.passthrough_sni / tls.mtls behavior (accept-time SNI split, exact case-insensitive matching, no-base-path alias rule, the two guards, mTLS on terminated connections only, no wildcards) without touching the PLAN-06 reserved gateway-paths block. Operator layer: doc/user/tls-edge.adoc covers configuring passthrough SNI and mTLS plus the emergency trust-root-replacement blast radius (swapping client_ca invalidates the entire old trust root at once; no per-cert revocation, no CRL/OCSP). Developer layer: doc/development/tls-edge.adoc covers the Vert.x NetServer front, the accept-time L4/L7 split, the internal-handoff loopback relay and port ownership (8443 public / 8444 internal), the runtime Host-vs-SNI smuggle guard, and the fail-closed GW-06 rationale with the Traefik CVE-2026-32305 / CVE-2025-66491 and KrakenD CVE-2026-39821 references. Record the TLS edge in doc/architecture.adoc's component model and append-only index lines in doc/README.adoc, doc/user/README.adoc, and doc/development/README.adoc. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Pq3eQyg78SuTKrC8pLpLs
The Decision "Decompose-on-read" bullet described the forward URI as resolved(base_url) + route.upstream.path + <remainder>, implying concatenation of three terms. The shipped semantics (RouteTableBuilder.applyRouteUpstreamPath and DispatchStage.upstreamRequestUri) are REPLACE: a declared route.upstream.path replaces the alias-derived base path, and the forward URI is stripTrailingSlash(effective base path) + <request-path remainder after the matched prefix>. Doc-only; the decompose-on-read decision itself is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Pq3eQyg78SuTKrC8pLpLs
…gression comparator Add the k6 passthrough_relay.js aspect driving the opaque L4 TCP relay path (mapped SNI -> L4 relay -> TLS backend), plus an empty-passthrough_sni mode re-measuring the same proxied static route for a no-regression check against the PLAN-04 plain-proxy baseline. Add PassthroughBaselineComparator, a noise-band comparator asserting the empty-mode run does not regress beyond a percentile-band tolerance (n/a, never 0, for an unmeasured metric), with a JUnit 5 unit test using CUI Test Generator. Register the two -Pbenchmark k6 executions and the post-integration-test comparator, and document the new aspect and its two modes in the README aspect matrix and methodology. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Pq3eQyg78SuTKrC8pLpLs
The whole-tree pre-commit quality gate applied mechanical OpenRewrite normalizations across the plan's touched TLS-edge and benchmark sources: fully-qualified annotation/type references simplified to imports (@java.io.Serial, @org.jspecify.annotations.Nullable, StandardCharsets, Consumer), wildcard static-import collapse, import-group blank-line separators, and array-initializer whitespace. No semantic changes; the quality gate passes green with these applied. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Pq3eQyg78SuTKrC8pLpLs
…dge classes Resolves Q-Gate finding 765287 (jacoco-branch-min). The api-sheriff JaCoCo bundle branch ratio was 998/1262 = 0.7908, below the 0.80 minimum. Add branch-covering JUnit 5 tests (AAA, no Mockito) for the plan-touched TLS-edge classes, lifting the bundle to 1020/1262 = 0.8082: - ClientHelloSniParserTest: extension-walk / name-entry edge cases (seek past a leading non-SNI extension, non-host_name entry skip, truncated and overlong server_name list, name length past list end) and the Result null-normalization branch. - PassthroughHostGuardStageTest: normalize branches for non-numeric, empty, and multi-colon :port suffixes. - SniFrontListenerTest: normalizeSni trailing-dot / case / whitespace path. - TlsEdgeProducerTest (new): boot wiring — front listener start/stop with a resolved passthrough SNI, empty-passthrough short-circuit, and the unresolved-alias defensive skip (ADR-0009). api-sheriff jacoco check: "All coverage checks have been met." Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Pq3eQyg78SuTKrC8pLpLs
Consolidate the byte-identical TrustAllManager X509TrustManager defined in MtlsHandshakeIT, PassthroughFaultIT, and TlsPassthroughIT into a single package-visible class in MtlsHandshakeIT, imported by the other two, and drop the now-unused imports. Test-only, package-private; no API or behavior change. Co-Authored-By: Claude <noreply@anthropic.com>
The pre-commit OpenRewrite recipe normalizes import ordering in two grpc integration-test files touched incidentally by the whole-tree gate. Committing the gate-mandated formatting so the working tree is OpenRewrite-clean. No behavioral change. Co-Authored-By: Claude <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cuioss-oliver, your pull request is larger than the review limit of 150000 diff characters
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository: cuioss/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds an accept-time TLS SNI edge with opaque passthrough relaying, terminated mTLS support, Host-vs-SNI smuggle protection, integration coverage, and a new passthrough benchmark workflow with baseline regression comparison. ChangesTLS edge routing and security
Passthrough benchmark workflow
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api-sheriff/src/main/java/de/cuioss/sheriff/gateway/tls/package-info.java (1)
33-37: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd the Jspecify compile dependency.
api-sheriff/src/main/java/de/cuioss/sheriff/gateway/tls/package-info.javaimportsorg.jspecify.annotations.NullMarked, and this module uses@NullMarkedin many package-info classes. Addorg.jspecify:jspecifyas a compile dependency ifNullMarkedis intended to be available here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api-sheriff/src/main/java/de/cuioss/sheriff/gateway/tls/package-info.java` around lines 33 - 37, Add org.jspecify:jspecify as a compile dependency for the module containing the TLS package-info.java files, ensuring org.jspecify.annotations.NullMarked resolves consistently across all package-info classes.
🧹 Nitpick comments (3)
integration-tests/docker-compose.yml (2)
56-101: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
api-sheriff'sdepends_onisn't updated for the newpassthrough-backend/toxiproxyservices.
TlsPassthroughITandPassthroughFaultITrequirepassthrough-backend(and, for the latter,toxiproxy) to be reachable through the gateway's relay target, but theapi-sheriffservice'sdepends_onblock still only listskeycloak,go-httpbin,asset-origin, andgrpc-echo. Both new services define healthchecks, so gating onservice_healthyhere would reduce the chance of cold-start flakiness in the IT suite.♻️ Suggested depends_on addition (outside this range — in the api-sheriff service block)
depends_on: keycloak: condition: service_started go-httpbin: condition: service_started asset-origin: condition: service_started grpc-echo: condition: service_healthy passthrough-backend: condition: service_healthy toxiproxy: condition: service_healthy🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration-tests/docker-compose.yml` around lines 56 - 101, Update the api-sheriff service’s depends_on block to include passthrough-backend and toxiproxy, using service_healthy conditions for both alongside the existing dependencies. Preserve all current dependency entries and conditions.
56-101: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRuntime
apk add/cert-gen diverges from the repo's pre-provisioned cert convention.The comment says this "mirror[s] the self-signed convention the certificates/ scripts use," but keycloak/api-sheriff actually mount pre-generated certs (
./src/main/docker/certificates), whereas this service installsopenssland generates a cert on every container start. That adds a network dependency (package registry reachability) and startup latency to every test run/restart, and reintroduces the risk of theset -escript failing beforenginxeven execs.Consider generating the passthrough-backend cert once (e.g. via a build-time Dockerfile step or a script under
certificates/) and mounting it read-only, consistent with the existing convention.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration-tests/docker-compose.yml` around lines 56 - 101, The passthrough-backend service currently installs openssl and generates TLS material at runtime, unlike the repository’s pre-provisioned certificate convention. Remove the runtime apk add and openssl generation from its entrypoint, add or reuse pre-generated certificate and key assets under the existing certificates convention, and mount them read-only at the paths referenced by the nginx configuration while preserving the passthrough-backend service and healthcheck behavior.api-sheriff/src/main/java/de/cuioss/sheriff/gateway/tls/MtlsServerCustomizer.java (1)
73-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRoute mTLS-enabled logging through the catalog, at INFO not DEBUG.
This is a security-relevant boot decision — comparable to
SNI_FRONT_LISTENER_STARTEDandPASSTHROUGH_RELAY_ESTABLISHED, which are both added as structuredINFOLogRecords in this same PR. Here it's an ad-hocLOGGER.debug(...)string, invisible at the defaultINFOlevel and outside thedoc/LogMessages.adoccatalog.♻️ Proposed fix — add a catalog entry and log at INFO
- LOGGER.debug("mTLS enabled: terminated HTTPS listener requires a client certificate signed by '%s'", - clientCa); + LOGGER.info(ApiSheriffLogMessages.INFO.MTLS_ENABLED, clientCa);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api-sheriff/src/main/java/de/cuioss/sheriff/gateway/tls/MtlsServerCustomizer.java` around lines 73 - 85, Update customizeHttpsServer to replace the ad-hoc LOGGER.debug mTLS message with the catalog’s structured INFO LogRecord mechanism. Add the corresponding mTLS-enabled message to the documented log catalog, preserving clientCa as the logged value and matching the existing patterns used by SNI_FRONT_LISTENER_STARTED and PASSTHROUGH_RELAY_ESTABLISHED.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@api-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/ClientHelloSniParserTest.java`:
- Around line 169-178: Update ClientHelloFixture.withCorruptExtensionLength() to
modify the actual server_name extension-length field directly at its known
offset, rather than scanning for a 0x00 0x00 byte pattern. Ensure the fixture
makes that extension length exceed the remaining buffer while leaving session
and cipher-suite lengths intact, so malformedExtensionLength exercises the
extension-length overrun validation path.
In `@benchmarks/pom.xml`:
- Around line 616-632: Move the compare-passthrough-baseline execution for
PassthroughBaselineComparator after stop-integration-containers in the
post-integration-test execution order, while preserving its existing
configuration. Ensure dump-keycloak-logs and container shutdown run before the
comparator can fail the build.
In `@benchmarks/README.adoc`:
- Around line 212-217: Update the “Passthrough relay, two modes
(API-Sheriff-only)” description to state that only passthrough uses a
TLS-terminating backend with its own certificate. Retain the comparison to ws
and grpc only for including real protocol-appropriate backend cost, and remove
the claim that those aspects share a TLS-enabled backend.
In `@doc/configuration.adoc`:
- Around line 768-781: The operator-facing TLS documentation must describe
runtime normalization consistently: in doc/configuration.adoc lines 768-781,
update the active TLS summary near the passthrough and Host-vs-SNI rules; in
doc/user/tls-edge.adoc lines 29-40, update the passthrough operating rules.
State that runtime removes one trailing dot from SNI and strips one :port suffix
from Host before passthrough/smuggle decisions, while preserving the existing
exact-match and other TLS constraints.
---
Outside diff comments:
In `@api-sheriff/src/main/java/de/cuioss/sheriff/gateway/tls/package-info.java`:
- Around line 33-37: Add org.jspecify:jspecify as a compile dependency for the
module containing the TLS package-info.java files, ensuring
org.jspecify.annotations.NullMarked resolves consistently across all
package-info classes.
---
Nitpick comments:
In
`@api-sheriff/src/main/java/de/cuioss/sheriff/gateway/tls/MtlsServerCustomizer.java`:
- Around line 73-85: Update customizeHttpsServer to replace the ad-hoc
LOGGER.debug mTLS message with the catalog’s structured INFO LogRecord
mechanism. Add the corresponding mTLS-enabled message to the documented log
catalog, preserving clientCa as the logged value and matching the existing
patterns used by SNI_FRONT_LISTENER_STARTED and PASSTHROUGH_RELAY_ESTABLISHED.
In `@integration-tests/docker-compose.yml`:
- Around line 56-101: Update the api-sheriff service’s depends_on block to
include passthrough-backend and toxiproxy, using service_healthy conditions for
both alongside the existing dependencies. Preserve all current dependency
entries and conditions.
- Around line 56-101: The passthrough-backend service currently installs openssl
and generates TLS material at runtime, unlike the repository’s pre-provisioned
certificate convention. Remove the runtime apk add and openssl generation from
its entrypoint, add or reuse pre-generated certificate and key assets under the
existing certificates convention, and mount them read-only at the paths
referenced by the nginx configuration while preserving the passthrough-backend
service and healthcheck behavior.
🪄 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: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 87eea754-9255-4678-b467-49deb75868ab
📒 Files selected for processing (41)
api-sheriff/src/main/java/de/cuioss/sheriff/gateway/ApiSheriffLogMessages.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRoute.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/events/EventType.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/pipeline/PassthroughHostGuardStage.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducer.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/tls/ClientHelloSniParser.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/tls/MtlsServerCustomizer.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/tls/PassthroughRelay.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/tls/SniFrontListener.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/tls/TlsEdgeProducer.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/tls/package-info.javaapi-sheriff/src/main/resources/application.propertiesapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/events/EventTypeTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/PassthroughHostGuardStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/ClientHelloSniParserTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/MtlsServerCustomizerTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/PassthroughRelayTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/SniFrontListenerTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/TlsEdgeProducerTest.javabenchmarks/README.adocbenchmarks/pom.xmlbenchmarks/src/main/java/de/cuioss/sheriff/gateway/k6/benchmark/K6BenchmarkLogMessages.javabenchmarks/src/main/java/de/cuioss/sheriff/gateway/k6/benchmark/PassthroughBaselineComparator.javabenchmarks/src/main/resources/k6-scripts/passthrough_relay.jsbenchmarks/src/test/java/de/cuioss/sheriff/gateway/k6/benchmark/PassthroughBaselineComparatorTest.javadoc/LogMessages.adocdoc/README.adocdoc/adr/0004-topology-indirection.adocdoc/architecture.adocdoc/configuration.adocdoc/development/README.adocdoc/development/tls-edge.adocdoc/user/README.adocdoc/user/tls-edge.adocintegration-tests/docker-compose.ymlintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/GrpcProxyIT.javaintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/HostSmuggleGuardIT.javaintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/MtlsHandshakeIT.javaintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/PassthroughFaultIT.javaintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/TlsPassthroughIT.javaintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/grpc/GrpcEchoServiceTest.java
Comment on lines
+169
to
+178
| @Test | ||
| @DisplayName("fails closed on an extension length that runs past the buffer") | ||
| void malformedExtensionLength() { | ||
| byte[] hello = ClientHelloFixture.withCorruptExtensionLength(); | ||
|
|
||
| ClientHelloSniParser.Result result = parser.parse(hello); | ||
|
|
||
| assertTrue(result.complete()); | ||
| assertTrue(result.serverName().isEmpty(), "a structural inconsistency fails closed"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test fixture corrupts the wrong field — doesn't test what it claims.
withCorruptExtensionLength()'s scan starts at absolute offset 5+4+34=43, which is the session_id_length byte (0x00) immediately followed by the cipher_suites_length high byte (also 0x00) — a false "0x00 0x00" match that breaks the loop before it ever reaches the real server_name extension-length field (offset 45-46). It ends up inflating cipher_suites_length/corrupting the cipher-suite byte instead, so malformedExtensionLength (Line 169) passes via a different fail-closed path (a skip() past buffer bounds) rather than exercising the SNI extension-length overrun check it's named for.
🐛 Proposed fix — build the corrupt extension directly instead of scanning for a byte pattern
- static byte[] withCorruptExtensionLength() {
- byte[] hello = withSni("corrupt.example.com");
- // The extension length is the two bytes immediately after the server_name extension type
- // (0x00 0x00). Locate the first 0x00 0x00 pair inside the extensions area and inflate the
- // following length so it overruns the buffer.
- for (int i = 5 + 4 + 34; i < hello.length - 4; i++) {
- if (hello[i] == 0x00 && hello[i + 1] == 0x00) {
- hello[i + 2] = (byte) 0x7F;
- hello[i + 3] = (byte) 0xFF;
- break;
- }
- }
- return hello;
- }
+ static byte[] withCorruptExtensionLength() {
+ // Declare a server_name extension whose length field overruns the extensions block,
+ // exercising the extensionsEnd bound check directly instead of pattern-scanning bytes.
+ byte[] sni = serverNameExtension("corrupt.example.com");
+ sni[2] = 0x7F;
+ sni[3] = (byte) 0xFF;
+ return withRawExtensions(sni);
+ }Also applies to: 334-347
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@api-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/ClientHelloSniParserTest.java`
around lines 169 - 178, Update ClientHelloFixture.withCorruptExtensionLength()
to modify the actual server_name extension-length field directly at its known
offset, rather than scanning for a 0x00 0x00 byte pattern. Ensure the fixture
makes that extension length exceed the remaining buffer while leaving session
and cipher-suite lengths intact, so malformedExtensionLength exercises the
extension-length overrun validation path.
Comment on lines
+616
to
+632
| <!-- Assert the empty-passthrough_sni run did not regress beyond the noise | ||
| band against the PLAN-04 plain-proxy baseline. Additive, beside | ||
| process-k6-results; reads the two k6 summaries produced in this lane. --> | ||
| <execution> | ||
| <id>compare-passthrough-baseline</id> | ||
| <phase>post-integration-test</phase> | ||
| <goals> | ||
| <goal>java</goal> | ||
| </goals> | ||
| <configuration> | ||
| <mainClass>de.cuioss.sheriff.gateway.k6.benchmark.PassthroughBaselineComparator</mainClass> | ||
| <arguments> | ||
| <argument>${k6.output.dir}</argument> | ||
| </arguments> | ||
| </configuration> | ||
| </execution> | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Comparator failure will skip container cleanup — orphaned containers on every detected regression.
compare-passthrough-baseline is declared in post-integration-test before dump-keycloak-logs (Line 634) and stop-integration-containers (Line 650), all bound to the same plugin/phase. Maven runs same-phase executions of one plugin in declaration order, and PassthroughBaselineComparator.main() deliberately throws IllegalStateException when result.regressed() — that's the whole point of the gate. When that happens, the build aborts right there and the two cleanup executions after it never run, leaving the benchmark Docker containers running (and Keycloak logs undumped) on every real regression — exactly the failure mode CI is most likely to hit.
Move this execution after stop-integration-containers (it only reads local files under ${k6.output.dir}, so cleanup order doesn't affect it) so containers are always torn down regardless of the comparator's verdict.
♻️ Suggested reordering
- <!-- Assert the empty-passthrough_sni run did not regress beyond the noise
- band against the PLAN-04 plain-proxy baseline. Additive, beside
- process-k6-results; reads the two k6 summaries produced in this lane. -->
- <execution>
- <id>compare-passthrough-baseline</id>
- <phase>post-integration-test</phase>
- <goals>
- <goal>java</goal>
- </goals>
- <configuration>
- <mainClass>de.cuioss.sheriff.gateway.k6.benchmark.PassthroughBaselineComparator</mainClass>
- <arguments>
- <argument>${k6.output.dir}</argument>
- </arguments>
- </configuration>
- </execution>
-
<!-- Dump Keycloak container logs before stopping containers -->
<execution>
<id>dump-keycloak-logs</id>
...
</execution>
<!-- Stop integration test containers -->
<execution>
<id>stop-integration-containers</id>
...
</execution>
+
+ <!-- Assert the empty-passthrough_sni run did not regress beyond the noise
+ band against the PLAN-04 plain-proxy baseline. Placed after container
+ teardown so a detected regression never blocks cleanup. -->
+ <execution>
+ <id>compare-passthrough-baseline</id>
+ <phase>post-integration-test</phase>
+ <goals>
+ <goal>java</goal>
+ </goals>
+ <configuration>
+ <mainClass>de.cuioss.sheriff.gateway.k6.benchmark.PassthroughBaselineComparator</mainClass>
+ <arguments>
+ <argument>${k6.output.dir}</argument>
+ </arguments>
+ </configuration>
+ </execution>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmarks/pom.xml` around lines 616 - 632, Move the
compare-passthrough-baseline execution for PassthroughBaselineComparator after
stop-integration-containers in the post-integration-test execution order, while
preserving its existing configuration. Ensure dump-keycloak-logs and container
shutdown run before the comparator can fail the build.
Comment on lines
+212
to
+217
| Passthrough relay, two modes (API-Sheriff-only):: | ||
| The `passthrough` aspect drives the opaque L4 TCP relay and — unlike the six pure-overhead HTTP | ||
| aspects, and exactly like `ws` and `grpc` — rides a *TLS-enabled backend* that completes the | ||
| handshake and presents its own certificate. It therefore folds a real backend's cost into the | ||
| number and is a *protocol-relay measurement*, not a backend-free gateway-overhead figure: read it as | ||
| relay overhead versus the terminated baseline within one run, never as an absolute throughput claim. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Passthrough/ws-grpc analogy is inaccurate — mixes two different comparison axes.
The claim that passthrough "exactly like ws and grpc — rides a TLS-enabled backend that completes the handshake and presents its own certificate" conflates two distinct properties. Only passthrough's backend actually terminates its own TLS (the entire point of the opaque L4 relay). The doc's own "Protocol-appropriate backends" entry and the fairness-caveat NOTE describe ws/grpc backends only as "protocol-appropriate" (WebSocket echo, gRPC), never as TLS-terminating — for those aspects the gateway still owns TLS termination. The valid analogy to ws/grpc is "folds a real backend's cost into the number" (not backend-free), not "rides a TLS-enabled backend."
✏️ Suggested rewording
-The `passthrough` aspect drives the opaque L4 TCP relay and — unlike the six pure-overhead HTTP
-aspects, and exactly like `ws` and `grpc` — rides a *TLS-enabled backend* that completes the
-handshake and presents its own certificate. It therefore folds a real backend's cost into the
-number and is a *protocol-relay measurement*, not a backend-free gateway-overhead figure: read it as
-relay overhead versus the terminated baseline within one run, never as an absolute throughput claim.
+The `passthrough` aspect drives the opaque L4 TCP relay against a *TLS-enabled backend* that
+completes the handshake and presents its own certificate — unique among the matrix, since every
+other aspect's TLS is terminated at the gateway. Like `ws` and `grpc`, it is not backend-free: it
+folds a real backend's cost into the number and is a *protocol-relay measurement*, not a
+backend-free gateway-overhead figure: read it as relay overhead versus the terminated baseline
+within one run, never as an absolute throughput claim.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Passthrough relay, two modes (API-Sheriff-only):: | |
| The `passthrough` aspect drives the opaque L4 TCP relay and — unlike the six pure-overhead HTTP | |
| aspects, and exactly like `ws` and `grpc` — rides a *TLS-enabled backend* that completes the | |
| handshake and presents its own certificate. It therefore folds a real backend's cost into the | |
| number and is a *protocol-relay measurement*, not a backend-free gateway-overhead figure: read it as | |
| relay overhead versus the terminated baseline within one run, never as an absolute throughput claim. | |
| The `passthrough` aspect drives the opaque L4 TCP relay against a *TLS-enabled backend* that | |
| completes the handshake and presents its own certificate — unique among the matrix, since every | |
| other aspect's TLS is terminated at the gateway. Like `ws` and `grpc`, it is not backend-free: it | |
| folds a real backend's cost into the number and is a *protocol-relay measurement*, not a | |
| backend-free gateway-overhead figure: read it as relay overhead versus the terminated baseline | |
| within one run, never as an absolute throughput claim. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmarks/README.adoc` around lines 212 - 217, Update the “Passthrough
relay, two modes (API-Sheriff-only)” description to state that only passthrough
uses a TLS-terminating backend with its own certificate. Retain the comparison
to ws and grpc only for including real protocol-appropriate backend cost, and
remove the claim that those aspects share a TLS-enabled backend.
Comment on lines
+768
to
+781
| [[_tls_active]] | ||
| *Status -- active.* The `tls.passthrough_sni` and `tls.mtls` blocks are *enforced at runtime*, not | ||
| merely parsed: an accept-time front listener reads the SNI from the reassembled ClientHello and | ||
| splits each connection at L4 (passthrough) or L7 (terminated); terminated connections apply | ||
| `mtls` client-certificate verification; and the runtime Host-vs-SNI smuggle guard rejects a | ||
| terminated request whose `Host` names a passthrough SNI with `404` before route selection. Three | ||
| constraints an operator must keep in mind, all stated above and restated here because they are the | ||
| common configuration mistakes: SNI matching is *exact* -- *no wildcard* entries; a passthrough | ||
| alias URL carries *no base path*; and `mtls` applies to *terminated connections only* (the gateway | ||
| never participates in a passthrough handshake). Operating these keys -- including the emergency | ||
| trust-root-replacement blast radius -- is covered in the link:user/tls-edge.adoc[operator TLS-edge | ||
| topic]; the front-listener architecture and the fail-closed (GW-06) rationale are covered in the | ||
| link:development/tls-edge.adoc[developer TLS-edge topic]. | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Align the operator-facing TLS contract with runtime normalization.
The runtime removes one trailing FQDN dot from SNI and strips one :port suffix from Host before the passthrough/smuggle decision. Document those rules consistently in both operator-facing locations:
doc/configuration.adoc#L768-L781: include the normalization behavior in the active TLS summary.doc/user/tls-edge.adoc#L29-L40: include the same behavior in the passthrough operating rules.
📍 Affects 2 files
doc/configuration.adoc#L768-L781(this comment)doc/user/tls-edge.adoc#L29-L40
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@doc/configuration.adoc` around lines 768 - 781, The operator-facing TLS
documentation must describe runtime normalization consistently: in
doc/configuration.adoc lines 768-781, update the active TLS summary near the
passthrough and Host-vs-SNI rules; in doc/user/tls-edge.adoc lines 29-40, update
the passthrough operating rules. State that runtime removes one trailing dot
from SNI and strips one :port suffix from Host before passthrough/smuggle
decisions, while preserving the existing exact-match and other TLS constraints.
…(S2789) Sonar new_reliability_rating flagged java:S2789 at ClientHelloSniParser.Result: the canonical constructor null-normalized the serverName Optional, but every construction site (needMoreData, parsed, and extractServerName's returns) already supplies a non-null Optional, so the guard was dead defensive code that an Optional must never carry. Remove the compact constructor and the test that exercised the now-absent null branch. No behavioral change; coverage stays green. Co-Authored-By: Claude <noreply@anthropic.com>
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
1 participant
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.
Summary
Implements the TLS-edge design from
doc/plan/06-tls-edge.adoc: an accept-timeSNI split that routes
passthrough_sniconnections through an opaque L4 TCPrelay while everything else goes through mTLS-terminated handling, plus a
runtime Host-vs-SNI smuggle guard, Toxiproxy-backed TLS/fault integration
tests, three-layer documentation (reference/operator/developer), and an
ADR-0004 wording correction to match the shipped
upstream.pathREPLACEsemantics.
Compatibility is breaking (pre-1.0 clean-slate, no deprecation).
Changes
Accept-time SNI dispatch & L4 relay (
api-sheriff):tls/ClientHelloSniParser.java— parses the SNI extension from the rawClientHello to decide the routing path before TLS termination.
tls/SniFrontListener.java— accept-time listener that dispatchesconnections based on the parsed SNI.
tls/PassthroughRelay.java— opaque L4 TCP relay forpassthrough_sniconnections (no TLS termination at the edge).
tls/MtlsServerCustomizer.java— mTLS-terminated handling path fornon-passthrough connections.
tls/TlsEdgeProducer.java,tls/package-info.java— CDI wiring andpackage documentation for the new
tlspackage.quarkus/ConfigProducer.java,edge/GatewayEdgeRoute.java,events/EventType.java,ApiSheriffLogMessages.java— supportingproducer/routing/event/log-message plumbing for the new TLS edge path.
pipeline/PassthroughHostGuardStage.java— runtime Host-vs-SNI smuggle404 guard (distinct from
FramingGate's HTTP-framing control); theboot-time collision rule in
ConfigValidatorwas left untouched (ownedby a concurrently-running plan).
src/main/resources/application.properties— TLS-edge configurationdefaults.
(
ClientHelloSniParserTest,SniFrontListenerTest,PassthroughRelayTest,MtlsServerCustomizerTest,TlsEdgeProducerTest,PassthroughHostGuardStageTest,EventTypeTest).Toxiproxy-backed integration tests (
integration-tests):docker-compose.yml— adds the Toxiproxy service alongsidekeycloak.TlsPassthroughIT.java,MtlsHandshakeIT.java,PassthroughFaultIT.java,HostSmuggleGuardIT.java,GrpcProxyIT.java,integration/grpc/GrpcEchoServiceTest.java— new IT suites exercisingthe passthrough relay, mTLS termination, fault injection via Toxiproxy,
the Host-smuggle guard, and gRPC-over-passthrough.
Benchmarks (
benchmarks):PassthroughBaselineComparator.java(+ test),K6BenchmarkLogMessages.java,k6-scripts/passthrough_relay.js,pom.xml,README.adoc— baseline comparator and k6 script for the newpassthrough relay path.
Documentation (three-layer, per constraint):
doc/configuration.adoc— new, distinct TLS-keys section(does not touch the PLAN-06 reserved-path list at :521-527).
doc/user/tls-edge.adoc(+doc/user/README.adocindex).doc/development/tls-edge.adoc(+doc/development/README.adocindex).
doc/README.adoc— append-only top-level index update.doc/architecture.adoc,doc/LogMessages.adoc— updated for the newpackage/log-message surface.
doc/adr/0004-topology-indirection.adoc— wording correction: replacesthe concatenation phrasing with the actually-shipped
upstream.pathREPLACE semantics.
Test Plan
python3 .plan/execute-script.py plan-marshall:build-maven:maven run --command-args "verify -Ppre-commit")python3 .plan/execute-script.py plan-marshall:build-maven:maven run --command-args "verify")Generated by plan-finalize skill
Summary by CodeRabbit
New Features
Documentation
Tests