diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index f07b69ed..ae2b7f27 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -27,23 +27,19 @@ jobs: # The -Pbenchmark profile DECLARES TWELVE k6 goals: the eight cross-gateway matrix aspects, the # two retained non-matrix health benchmarks (healthLiveCheck, gatewayHealth), and the two # API-Sheriff-only passthrough-relay executions (mapped relay throughput and the empty-mode - # no-regression run). ELEVEN of them execute: only uploadLarge still carries a bound to a - # property that defaults to true in benchmarks/pom.xml, because it remains blocked by an open - # gateway defect (see the "Deliberately skipped" table in the coverage step below). Maven - # therefore never runs it, so it consumes no wall time and cannot abort the suite. It stays - # ordered last in the profile as defense-in-depth, so that flipping its skip property to false - # cannot mask a goal that would otherwise produce a result. + # no-regression run). All TWELVE of them execute: uploadLarge and websocketEcho still carry a + # bound to their own property, but both properties are false in benchmarks/pom.xml, so + # both goals run. The two stay ordered last in the profile as defense-in-depth, so a fail-fast + # abort from either cannot discard a goal that would otherwise produce a result. # # Budget, from per-goal timings measured locally 2026-07-28 rather than estimated: - # * goal execution ~13 min -- sized on the ELEVEN goals that actually execute, which is the - # real basis: a skipped goal is not run at all, so it costs nothing but the - # Maven bookkeeping for a skipped execution. - # 11 goals x (60s k6 window + ~10s compose/k6 start and summary write). - # The websocketEcho term is the ~70s a re-enabled request-rate goal costs, - # now that its blocking permit leak is fixed. Re-enabling upload-50MB would - # cost more than that: it is transfer-bound at reduced concurrency, so it - # does not finish in the same wall time as a request-rate run. Re-derive - # this term when a skip property is flipped to false. + # * goal execution ~16 min -- sized on all TWELVE goals, which now all execute. + # 11 request-rate goals x (60s k6 window + ~10s compose/k6 start and + # summary write) = ~13 min, plus uploadLarge. uploadLarge is NOT a + # request-rate goal: it is transfer-bound at reduced concurrency + # (k6.vus.upload.large = 5), so it does not finish in the same wall time as + # a request-rate run — it is budgeted at ~3 min on its own. Re-derive this + # term when a skip property is flipped, or when a goal is added or removed. # * stack startup ~100 s -- the lane boots FIVE native gateway instances (api-sheriff, # api-sheriff-mtls, api-sheriff-cookie, api-sheriff-cookie-2 and # api-sheriff-ws-admission, the last added for the WebSocket relay-permit @@ -119,10 +115,8 @@ jobs: # twelve goals: the eight cross-gateway matrix aspects (unauth, bearer, http2, # graphql, upload-1MB, upload-50MB, ws, grpc), the two retained non-matrix health # benchmarks (healthLiveCheck, gatewayHealth), and the two API-Sheriff-only - # passthrough-relay executions (mapped and empty). Eleven of the twelve execute and are - # expected to produce a CI result; only upload-50MB (uploadLarge) is still skipped by - # property against an open gateway defect, so Maven does not run it and it cannot abort - # the suite. The on-demand APISIX comparison lane is deliberately NOT run + # passthrough-relay executions (mapped and empty). All twelve execute and are + # expected to produce a CI result. The on-demand APISIX comparison lane is deliberately NOT run # here. Maven still fails fast on the goals that do run, so the coverage step below # remains what proves which goals actually produced a result. echo "Running k6 integration benchmarks with native Quarkus..." @@ -134,61 +128,37 @@ jobs: ls -la benchmarks/target/benchmark-results/ # Maven fails fast, so a goal that errors aborts every goal after it and a goal that is never - # reached leaves no trace in the job output — a suite that ran 3 of the 11 executing goals - # renders exactly like one that ran all 11. This step diffs the goals that MUST produce a + # reached leaves no trace in the job output — a suite that ran a handful of the executing goals + # renders exactly like one that ran all of them. This step diffs the goals that MUST produce a # summary document against the documents k6 actually wrote, so a partial suite is visibly # partial and a goal that silently vanished fails the job. When Maven failed, it also states - # in words whether the suite was truncated by that failure. Skipped goals are named with their - # reason and their skip property rather than omitted, so "not run" is never - # indistinguishable from "forgotten". + # in words whether the suite was truncated by that failure. Goals that are not expected to run + # are named with their reason and their skip property rather than omitted, so "not run" is + # never indistinguishable from "forgotten". + # + # Both the expected set and the not-expected table are DERIVED from the build-generated + # execution manifest (benchmarks/pom.xml, generate-benchmark-manifest, bound to initialize), + # not maintained by hand here. That is deliberate: the previous hand-maintained list mirrored + # the POM with nothing enforcing agreement, and it drifted — the goal set moved from eleven to + # twelve while prose in this file still said eleven. Deriving both from the manifest makes that + # drift structurally impossible, and an absent manifest is itself a hard failure rather than an + # empty expected set that would vacuously report full coverage. + # + # The rendering lives in the generator's own `summarise` subcommand rather than being + # reimplemented in bash here, so the manifest has exactly ONE consumer. Splitting it across two + # implementations would recreate, one level up, the duplicated-contract defect this manifest was + # introduced to remove — and did: an earlier bash/jq version of this step diverged on the table + # heading and mislabelled an inline `` as "not wired". - name: Summarise benchmark coverage if: always() env: BENCHMARK_OUTCOME: ${{ steps.run-benchmarks.outcome }} run: | - set -uo pipefail - results_dir=benchmarks/target/benchmark-results/k6 - - # Goals expected to write a summary. A goal settled as deliberately skipped is NOT listed - # here — it belongs in the skip table below, which is what keeps the two states distinct. - expected="healthLiveCheck gatewayHealth proxiedStatic passthroughRelay passthroughRelayEmpty bearerProxied http2 graphql uploadSmall grpcUnary websocketEcho" - - missing=0 - missing_names="" - { - echo "## Benchmark coverage" - echo - echo "| Benchmark | Result |" - echo "| --- | --- |" - } >> "$GITHUB_STEP_SUMMARY" - - for name in ${expected}; do - if [ -f "${results_dir}/${name}-summary.json" ]; then - echo "| \`${name}\` | ran |" >> "$GITHUB_STEP_SUMMARY" - else - echo "| \`${name}\` | **DID NOT RUN** |" >> "$GITHUB_STEP_SUMMARY" - missing=$((missing + 1)) - missing_names="${missing_names:+${missing_names}, }${name}" - fi - done - - { - echo - echo "### Deliberately skipped" - echo - echo "| Benchmark | Skipped by | Reason |" - echo "| --- | --- | --- |" - echo "| \`uploadLarge\` | \`skip.benchmark.upload.large\` (defaults to \`true\`) | The gateway rejects the 50MB body with 413: the Quarkus HTTP body limit is not derived from the upload anchor's 64 MiB \`max_body_bytes\`, so the declared cap is unreachable. Needs a gateway-side fix; re-enable with \`-Dskip.benchmark.upload.large=false\`. |" - echo "| \`sessionMediated\` | not wired | Wired to no Maven goal on purpose — BFF session mediation belongs to PLAN-07A. |" - } >> "$GITHUB_STEP_SUMMARY" - - # Only fail on a missing summary when Maven itself reported success: when the benchmark - # step already failed, its own error is the actionable signal and this step must not - # mask it with a derived one. - if [ "${missing}" -gt 0 ] && [ "${BENCHMARK_OUTCOME}" = "success" ]; then - echo "::error::${missing} expected benchmark summary document(s) absent although the benchmark step succeeded." - exit 1 - fi + python3 benchmarks/scripts/benchmark-manifest.py summarise \ + --manifest benchmarks/target/benchmark-execution-manifest.json \ + --results-dir benchmarks/target/benchmark-results/k6 \ + --benchmark-outcome "${BENCHMARK_OUTCOME}" \ + --summary-file "$GITHUB_STEP_SUMMARY" # When Maven failed, the DID NOT RUN rows above and the Maven error are almost always the # same event, but nothing on the page says so — a reader is left to infer the causal link. diff --git a/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/DispatchStage.java b/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/DispatchStage.java index 8017e5ac..c236de60 100644 --- a/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/DispatchStage.java +++ b/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/DispatchStage.java @@ -53,7 +53,7 @@ * {@link ByteCappedBodyStream} that forwards each chunk to the upstream request as it arrives and * enforces the {@code max_body_bytes} ceiling with a running counter. A mid-stream breach ABORTS * the in-flight upstream call (Vert.x {@link HttpClientRequest#reset()}) and surfaces - * {@link EventType#PARAMETER_LIMIT_EXCEEDED} (400). The upstream body is never + * {@link EventType#CONTENT_TOO_LARGE} (413). The upstream body is never * materialized into an {@code HttpResult} (ADR-0006/0008): the returned * {@link HttpClientResponse} is a live {@link ReadStream} whose body {@link ResponseStage} streams * back with backpressure. @@ -283,7 +283,7 @@ private static String stripTrailingSlash(String value) { * A {@link ReadStream} decorator that forwards each request-body chunk to the upstream as it * arrives — never accumulating the body — while counting bytes against a ceiling. On breach it * aborts the in-flight upstream request and fails the stream with a - * {@link EventType#PARAMETER_LIMIT_EXCEEDED} {@link GatewayException}. + * {@link EventType#CONTENT_TOO_LARGE} {@link GatewayException}. */ static final class ByteCappedBodyStream implements ReadStream { @@ -320,7 +320,7 @@ private void onChunk(Buffer chunk) { aborted = true; delegate.pause(); abortAction.run(); - propagateFailure(new GatewayException(EventType.PARAMETER_LIMIT_EXCEEDED, + propagateFailure(new GatewayException(EventType.CONTENT_TOO_LARGE, "Request body exceeded max_body_bytes=" + maxBytes)); return; } diff --git a/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GrpcStatusMapper.java b/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GrpcStatusMapper.java index 2a9d82ac..7c109c83 100644 --- a/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GrpcStatusMapper.java +++ b/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GrpcStatusMapper.java @@ -40,6 +40,7 @@ *
  • {@code 403} → {@code PERMISSION_DENIED} (7)
    • *
  • {@code 404} → {@code NOT_FOUND} (5)
    • *
  • {@code 405} → {@code UNIMPLEMENTED} (12)
    • + *
  • {@code 413} → {@code RESOURCE_EXHAUSTED} (8)
    • *
  • {@code 502} / {@code 503} → {@code UNAVAILABLE} (14) — also an h2-negotiation failure
    • *
  • {@code 504} → {@code DEADLINE_EXCEEDED} (4)
    • *
  • anything else → {@code UNKNOWN} (2)
    • @@ -62,6 +63,8 @@ public final class GrpcStatusMapper { public static final int NOT_FOUND = 5; /** gRPC status code: the caller lacked permission (maps HTTP 403). */ public static final int PERMISSION_DENIED = 7; + /** gRPC status code: a per-request resource bound was exhausted (maps HTTP 413). */ + public static final int RESOURCE_EXHAUSTED = 8; /** gRPC status code: the operation is not implemented / not supported (maps HTTP 405). */ public static final int UNIMPLEMENTED = 12; /** gRPC status code: the service is unavailable (maps HTTP 502 / 503 and h2-negotiation failure). */ @@ -90,6 +93,7 @@ public int toGrpcStatus(EventType eventType) { case 403 -> PERMISSION_DENIED; case 404 -> NOT_FOUND; case 405 -> UNIMPLEMENTED; + case 413 -> RESOURCE_EXHAUSTED; case 502, 503 -> UNAVAILABLE; case 504 -> DEADLINE_EXCEEDED; default -> UNKNOWN; diff --git a/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/events/EventType.java b/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/events/EventType.java index 87cb9ef3..44860b17 100644 --- a/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/events/EventType.java +++ b/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/events/EventType.java @@ -75,8 +75,24 @@ public enum EventType { * {@code reserved_body_max_bytes} ceiling. These paths are read before the pipeline's per-route * body cap can apply, so the ceiling is enforced at the read itself and the request is rejected * {@code 413} without the oversized body ever being buffered. + *

    + * Contrast {@link #CONTENT_TOO_LARGE}, which is the per-route + * {@code security_filter.max_body_bytes} cap on the proxy path. This event is exclusively the + * edge-level {@code reserved_body_max_bytes} ceiling on gateway-terminated reserved paths. */ RESERVED_BODY_TOO_LARGE(EventCategory.INPUT_VALIDATION, 413), + /** + * A proxied request declared or streamed a body beyond its route's + * {@code security_filter.max_body_bytes} cap. The cap has two enforcement points: the + * {@code Content-Length} fast-reject in {@code ThoroughChecksStage}, which rejects before any + * body is read, and the streaming byte counter in {@code DispatchStage}, which aborts a chunked + * or under-declared body mid-transfer once the cap is crossed. + *

    + * Contrast {@link #RESERVED_BODY_TOO_LARGE}, which is the edge's {@code reserved_body_max_bytes} + * ceiling on gateway-terminated reserved BFF POST paths. This event is exclusively the per-route + * proxy-path cap. + */ + CONTENT_TOO_LARGE(EventCategory.INPUT_VALIDATION, 413), // --- Authentication (401) --- diff --git a/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/pipeline/ThoroughChecksStage.java b/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/pipeline/ThoroughChecksStage.java index bdc89db1..af2bf7ef 100644 --- a/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/pipeline/ThoroughChecksStage.java +++ b/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/pipeline/ThoroughChecksStage.java @@ -44,8 +44,8 @@ * whitelist, the canonical path must match one pattern, where a {@code {name}} segment matches * exactly one path segment; a miss is a 400 {@link EventType#PATH_NOT_ALLOWED}. *

  • {@code max_body_bytes} fast-reject. A declared {@code Content-Length} - * already exceeding the route config's {@code maxBodySize} is rejected 400 - * ({@link EventType#PARAMETER_LIMIT_EXCEEDED}) before the body is read.
    • + * already exceeding the route config's {@code maxBodySize} is rejected 413 + * ({@link EventType#CONTENT_TOO_LARGE}) before the body is read. * * * @author API Sheriff Team @@ -76,7 +76,7 @@ public ThoroughChecksStage(SecurityConfiguration defaultConfiguration, SecurityE * @param allowedPaths the selected route's {@code allowed_paths} whitelist, empty when unrestricted * @throws GatewayException on a divergent-pipeline violation ({@link EventType#SECURITY_FILTER_VIOLATION}), * a whitelist miss ({@link EventType#PATH_NOT_ALLOWED}), or a body-cap breach - * ({@link EventType#PARAMETER_LIMIT_EXCEEDED}) + * ({@link EventType#CONTENT_TOO_LARGE}) */ public void process(PipelineRequest request, List allowedPaths) { Objects.requireNonNull(request, "request"); @@ -119,7 +119,7 @@ private void reRunPipelines(PipelineRequest request, SecurityConfiguration route private static void enforceBodyCap(PipelineRequest request, SecurityConfiguration routeConfig) { long cap = routeConfig.maxBodySize(); if (request.declaredContentLength() > cap) { - throw new GatewayException(EventType.PARAMETER_LIMIT_EXCEEDED, + throw new GatewayException(EventType.CONTENT_TOO_LARGE, "Declared body %d exceeds route cap %d".formatted(request.declaredContentLength(), cap)); } } diff --git a/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducer.java b/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducer.java index 77be87cc..e3190379 100644 --- a/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducer.java +++ b/api-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducer.java @@ -16,10 +16,13 @@ package de.cuioss.sheriff.gateway.quarkus; import java.nio.file.Path; +import java.util.ArrayList; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.Set; +import java.util.stream.Stream; import de.cuioss.sheriff.gateway.config.ConfigLogMessages; @@ -28,6 +31,7 @@ import de.cuioss.sheriff.gateway.config.load.ConfigLoadException; import de.cuioss.sheriff.gateway.config.load.ConfigLoader; import de.cuioss.sheriff.gateway.config.load.EnvSecretResolver; +import de.cuioss.sheriff.gateway.config.model.AnchorConfig; import de.cuioss.sheriff.gateway.config.model.AssetConfig; import de.cuioss.sheriff.gateway.config.model.EdgeHardeningConfig; import de.cuioss.sheriff.gateway.config.model.EndpointConfig; @@ -36,6 +40,7 @@ import de.cuioss.sheriff.gateway.config.model.ResolvedTopology; import de.cuioss.sheriff.gateway.config.model.RouteConfig; import de.cuioss.sheriff.gateway.config.model.RouteTable; +import de.cuioss.sheriff.gateway.config.model.SecurityFilterConfig; import de.cuioss.sheriff.gateway.config.model.TlsConfig; import de.cuioss.sheriff.gateway.config.topology.TopologyResolver; import de.cuioss.sheriff.gateway.config.validation.ConfigValidator; @@ -43,6 +48,7 @@ import de.cuioss.tools.logging.CuiLogger; import io.quarkus.runtime.StartupEvent; +import io.quarkus.runtime.configuration.MemorySize; import jakarta.enterprise.context.ApplicationScoped; import jakarta.enterprise.event.Observes; import jakarta.enterprise.inject.Produces; @@ -56,7 +62,13 @@ * It drives the framework-agnostic boot pipeline — {@link ConfigLoader} (read, * schema-validate, secret-resolve, bind) → endpoint-enablement filter → * {@link TopologyResolver} → {@link ConfigValidator} → {@link RouteTableBuilder} — - * supplying every collaborator by construction. On the first collected violation + * supplying every collaborator by construction. It additionally applies one check the + * framework-agnostic validator cannot: the largest declared {@code max_body_bytes} + * across the anchors and the enabled endpoints' routes must not exceed the Vert.x + * {@code quarkus.http.limits.max-body-size} ceiling, because the framework rejects an + * over-ceiling request in a root handler in front of the gateway router and the declared + * cap would never be reached. That violation joins the validator's own collection and + * aborts through the same path. On the first collected violation * the producer logs every problem through structured ERROR * {@link ConfigLogMessages} records and throws, so Quarkus exits non-zero and never * serves on partial configuration. On success it emits the {@code CONFIG_LOADED} @@ -76,6 +88,14 @@ public class ConfigProducer { @ConfigProperty(name = "sheriff.config.dir", defaultValue = "config") String configDir; + /** + * The Vert.x request-body ceiling the framework enforces in a root handler in front of the + * gateway router. Injected here — the ADR-0005 framework-bound edge — rather than in the + * framework-agnostic {@code ConfigValidator}, so Quarkus-key knowledge stays at this seam. + */ + @ConfigProperty(name = "quarkus.http.limits.max-body-size") + MemorySize frameworkBodyLimit; + private GatewayConfig gateway; private RouteTable routeTable; private ResolvedTopology resolvedTopology; @@ -177,7 +197,9 @@ private synchronized void buildOnce() { List enabled = loaded.endpoints().stream().filter(EndpointConfig::enabled).toList(); ResolvedTopology topology = new TopologyResolver(resolver) .resolve(directory.resolve(TOPOLOGY_FILE), enabled, additionalTopologyAliases(loaded, enabled)); - List violations = new ConfigValidator().validate(loaded.gateway(), enabled, topology); + List violations = new ArrayList<>( + new ConfigValidator().validate(loaded.gateway(), enabled, topology)); + violations.addAll(frameworkBodyLimitViolations(loaded.gateway(), enabled)); if (!violations.isEmpty()) { abort(violations); } @@ -194,6 +216,50 @@ private synchronized void buildOnce() { } } + /** + * The fail-closed framework-limit check: a declared per-route body cap above the Vert.x ceiling + * is unreachable, because Quarkus rejects the request in a root handler installed in front of the + * gateway router. Rather than let that mismatch stay silent, the boot refuses to start and names + * the key the operator must raise. + * + * @param gateway the bound gateway document (source of the anchor-level caps) + * @param enabled the enabled endpoints whose routes carry the route-level caps + * @return the single violation when the largest declared cap exceeds the framework limit, + * otherwise an empty list + */ + private List frameworkBodyLimitViolations(GatewayConfig gateway, List enabled) { + long limit = frameworkBodyLimit.asLongValue(); + long declared = maxDeclaredBodyBytes(gateway, enabled); + if (declared <= limit) { + return List.of(); + } + return List.of(new ConfigError("application.properties", "quarkus.http.limits.max-body-size", + "%d exceeds framework limit %d; raise quarkus.http.limits.max-body-size to at least %d" + .formatted(declared, limit, declared))); + } + + /** + * The largest {@code max_body_bytes} declared anywhere in the configuration set — across the + * named policy anchors and every enabled endpoint's routes, the only two places a per-route cap + * can be declared. + * + * @param gateway the bound gateway document + * @param enabled the enabled endpoints + * @return the maximum declared cap in bytes, or {@code 0} when no cap is declared + */ + private static long maxDeclaredBodyBytes(GatewayConfig gateway, List enabled) { + return Stream.concat( + gateway.anchors().values().stream().map(AnchorConfig::securityFilter), + enabled.stream().flatMap(endpoint -> endpoint.routes().stream()) + .map(RouteConfig::securityFilter)) + .flatMap(Optional::stream) + .map(SecurityFilterConfig::maxBodyBytes) + .flatMap(Optional::stream) + .mapToLong(Integer::longValue) + .max() + .orElse(0L); + } + /** * The topology aliases that must resolve independently of any enabled endpoint's * {@code base_url}: the {@code tls.passthrough_sni} relay targets and every diff --git a/api-sheriff/src/main/resources/application.properties b/api-sheriff/src/main/resources/application.properties index e7020767..214c56bc 100644 --- a/api-sheriff/src/main/resources/application.properties +++ b/api-sheriff/src/main/resources/application.properties @@ -8,6 +8,22 @@ quarkus.http.port=8080 quarkus.http.ssl-port=8443 quarkus.http.insecure-requests=redirect +# Framework body-size floor under the gateway's own per-route max_body_bytes caps. +# Without this key Vert.x applies the Quarkus default of 10 MiB and silently clips any route +# declaring a larger security_filter.max_body_bytes, so the gateway's own cap never fires. +# (i) This is the FLOOR: the gateway still enforces the smaller per-route cap on top of it. +# (ii) A deployment declaring a larger max_body_bytes must raise this key — ConfigProducer's +# fail-closed boot check refuses to start when a declared cap exceeds this limit. +# (iii) At floor == largest declared cap, the Quarkus root handler's Content-Length pre-check +# answers a breach with a bare 413 (Connection: close, empty body) BEFORE the gateway +# pipeline runs, so the RFC 9457 problem+json envelope is rendered only for routes capped +# below the floor. An operator who wants the gateway's own envelope on the largest-capped +# route sets this floor ABOVE that cap rather than equal to it. +# (iv) Chunked requests carry no Content-Length, so they pass the pre-check untouched and are +# capped by the gateway's streaming byte counter instead. +# 64M == 67108864 bytes, matching the largest declared max_body_bytes in the shipped topology. +quarkus.http.limits.max-body-size=64M + # Management interface — health/metrics on a separate port (9000), served over HTTPS. # Follows the Keycloak pattern of splitting operations off the application port: application on # 8443, operations on 9000. Unlike quarkus.http, Quarkus' ManagementConfig declares NO ssl-port and diff --git a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/DispatchStageTest.java b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/DispatchStageTest.java index a4abde87..fd186ec7 100644 --- a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/DispatchStageTest.java +++ b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/DispatchStageTest.java @@ -134,12 +134,12 @@ void abortsOnBreach() { source.emit(Buffer.buffer("123456")); source.emit(Buffer.buffer("ABCDEF")); - // Assert — the breaching chunk is not forwarded, the call is aborted, and a 400 is raised + // Assert — the breaching chunk is not forwarded, the call is aborted, and a 413 is raised assertEquals(1, forwarded.size(), "the breaching chunk must never cross to the upstream"); assertTrue(aborted.get(), "a mid-stream breach must abort the in-flight upstream call"); Throwable raised = failure.get(); GatewayException gatewayException = assertInstanceOf(GatewayException.class, raised); - assertEquals(EventType.PARAMETER_LIMIT_EXCEEDED, gatewayException.getEventType()); + assertEquals(EventType.CONTENT_TOO_LARGE, gatewayException.getEventType()); } @Test diff --git a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GrpcDispatchStageTest.java b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GrpcDispatchStageTest.java index 829dec9d..72032379 100644 --- a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GrpcDispatchStageTest.java +++ b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GrpcDispatchStageTest.java @@ -319,7 +319,7 @@ void streamsMultiFrameUnderCapWithoutMisfire() { } @Test - @DisplayName("aborts the gRPC dispatch with PARAMETER_LIMIT_EXCEEDED when the body cap is breached") + @DisplayName("aborts the gRPC dispatch with CONTENT_TOO_LARGE when the body cap is breached") void abortsOnBodyCapBreach() { // Arrange — the shared body-abuse bound also applies to the opaque gRPC frame stream TestReadStream source = new TestReadStream(); @@ -335,13 +335,13 @@ void abortsOnBodyCapBreach() { source.emit(Buffer.buffer("frame")); source.emit(Buffer.buffer("flood")); - // Assert — the breaching frame is dropped, the dispatch is aborted, and a 400 is raised + // Assert — the breaching frame is dropped, the dispatch is aborted, and a 413 is raised assertEquals(1, forwarded.size(), "the breaching frame must never cross to the upstream"); assertTrue(aborted.get(), "a body-cap breach aborts the in-flight gRPC dispatch"); GatewayException raised = assertInstanceOf(GatewayException.class, failure.get(), "the breach raises a GatewayException"); - assertEquals(EventType.PARAMETER_LIMIT_EXCEEDED, raised.getEventType(), - "the shared body-abuse bound raises PARAMETER_LIMIT_EXCEEDED on the gRPC path"); + assertEquals(EventType.CONTENT_TOO_LARGE, raised.getEventType(), + "the shared body-abuse bound raises CONTENT_TOO_LARGE on the gRPC path"); } } diff --git a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GrpcStatusMapperTest.java b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GrpcStatusMapperTest.java index 15f3b9fd..858632f3 100644 --- a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GrpcStatusMapperTest.java +++ b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GrpcStatusMapperTest.java @@ -86,6 +86,13 @@ void mapsUnimplemented() { "an HTTP 405 rejection maps to gRPC UNIMPLEMENTED (12)"); } + @Test + @DisplayName("413 (body cap breached) maps to RESOURCE_EXHAUSTED") + void mapsContentTooLargeToResourceExhausted() { + assertEquals(GrpcStatusMapper.RESOURCE_EXHAUSTED, mapper.toGrpcStatus(EventType.CONTENT_TOO_LARGE), + "an HTTP 413 body-cap breach maps to gRPC RESOURCE_EXHAUSTED (8)"); + } + @Test @DisplayName("502 (upstream error / h2-negotiation failure) maps to UNAVAILABLE") void mapsBadGatewayToUnavailable() { diff --git a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/events/EventTypeTest.java b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/events/EventTypeTest.java index 285ef897..b2dcb833 100644 --- a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/events/EventTypeTest.java +++ b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/events/EventTypeTest.java @@ -20,31 +20,133 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.params.provider.Arguments.arguments; + +import java.util.Collection; +import java.util.EnumSet; +import java.util.List; +import java.util.Set; +import java.util.function.Predicate; +import java.util.stream.Stream; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.CsvSource; -import org.junit.jupiter.params.provider.EnumSource; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +/** + * Pins {@link EventType}'s category, HTTP-status and WebSocket-close mappings. + *

    + * Every constant is claimed by exactly one of four buckets — success / informational, boot-only + * configuration, WebSocket-close, and the request-time error contract. Bucket membership is + * DERIVED from {@link EventType}'s own accessors ({@code hasHttpMapping}, {@code wsCloseCode}, + * {@code category}), not hand-listed: a guard that merely detects an omission still lets the second + * registry drift first, whereas a derived set cannot drift at all. A constant added to the enum + * therefore lands in its bucket automatically, and {@link ContractCoverage} asserts the four + * predicates remain a true partition so a constant can never fall into none or two of them. + *

    + * What stays literal is the thing that carries independent information: {@link #ERROR_CONTRACT} + * states each mapped event's expected status and category as written-out values rather than + * re-reading them from the enum under test, so the mapping assertions cannot pass vacuously. The + * split is deliberate — derive the membership, state the mapping. + * + * @author API Sheriff Team + * @since 1.0 + */ @DisplayName("EventType — category and HTTP-status mapping") class EventTypeTest { + /** Success / informational events: no category, no HTTP mapping, no WebSocket-close mapping. */ + private static final Set SUCCESS_EVENTS = + classify(eventType -> !eventType.hasHttpMapping() + && eventType.wsCloseCode() == 0 + && eventType.category() == null); + + /** + * Selects the constants matching {@code predicate}. Membership is DERIVED from {@link EventType}'s + * own accessors rather than hand-listed, so a constant added to the enum lands in its bucket + * automatically and cannot be silently omitted from the suite. + */ + private static Set classify(Predicate predicate) { + return EnumSet.allOf(EventType.class).stream() + .filter(predicate) + .collect(() -> EnumSet.noneOf(EventType.class), Set::add, Set::addAll); + } + + /** Boot-only configuration failures: a category, but never an HTTP response. */ + private static final Set CONFIGURATION_EVENTS = + classify(eventType -> !eventType.hasHttpMapping() + && eventType.wsCloseCode() == 0 + && eventType.category() != null); + + /** + * Events that terminate an established WebSocket relay: no HTTP mapping (they occur + * after the {@code 101} upgrade) but a non-zero close code. + */ + private static final Set WEBSOCKET_CLOSE_EVENTS = + classify(eventType -> !eventType.hasHttpMapping() && eventType.wsCloseCode() > 0); + + /** + * The request-time error contract: one row per event the edge renders as an HTTP status, with the + * status and category the contract in {@code architecture.adoc} promises. + */ + private static final List ERROR_CONTRACT = List.of( + arguments(EventType.SECURITY_FILTER_VIOLATION, 400, EventCategory.INPUT_VALIDATION), + arguments(EventType.PATH_NOT_ALLOWED, 400, EventCategory.INPUT_VALIDATION), + arguments(EventType.PARAMETER_LIMIT_EXCEEDED, 400, EventCategory.INPUT_VALIDATION), + arguments(EventType.NO_ROUTE_MATCHED, 404, EventCategory.INPUT_VALIDATION), + arguments(EventType.PASSTHROUGH_HOST_SMUGGLED, 404, EventCategory.INPUT_VALIDATION), + arguments(EventType.METHOD_NOT_ALLOWED, 405, EventCategory.INPUT_VALIDATION), + arguments(EventType.RESERVED_BODY_TOO_LARGE, 413, EventCategory.INPUT_VALIDATION), + arguments(EventType.CONTENT_TOO_LARGE, 413, EventCategory.INPUT_VALIDATION), + arguments(EventType.TOKEN_MISSING, 401, EventCategory.AUTHENTICATION), + arguments(EventType.TOKEN_INVALID, 401, EventCategory.AUTHENTICATION), + arguments(EventType.LOGOUT_TOKEN_INVALID, 400, EventCategory.AUTHENTICATION), + arguments(EventType.SCOPE_MISSING, 403, EventCategory.AUTHORIZATION), + arguments(EventType.CSRF_REJECTED, 403, EventCategory.AUTHORIZATION), + arguments(EventType.WEBSOCKET_ORIGIN_REJECTED, 403, EventCategory.AUTHORIZATION), + arguments(EventType.UPSTREAM_ERROR, 502, EventCategory.UPSTREAM), + arguments(EventType.UPSTREAM_CIRCUIT_OPEN, 503, EventCategory.UPSTREAM), + arguments(EventType.UPSTREAM_TIMEOUT, 504, EventCategory.UPSTREAM)); + + static Stream successEvents() { + return SUCCESS_EVENTS.stream(); + } + + static Stream configurationEvents() { + return CONFIGURATION_EVENTS.stream(); + } + + static Stream websocketCloseEvents() { + return WEBSOCKET_CLOSE_EVENTS.stream(); + } + + static Stream errorContract() { + return ERROR_CONTRACT.stream(); + } + + private static Set errorContractKeys() { + return ERROR_CONTRACT.stream() + .map(row -> (EventType) row.get()[0]) + .collect(() -> EnumSet.noneOf(EventType.class), Set::add, Set::addAll); + } + @Nested @DisplayName("Success / informational events") class SuccessEvents { @ParameterizedTest - @EnumSource(names = {"REQUEST_FORWARDED", "TOKEN_REFRESHED", "CONFIG_LOADED", - "SESSION_CREATED", "SESSION_DESTROYED", "SESSION_REFRESH_FAILED", "BACKCHANNEL_LOGOUT"}) + @MethodSource("de.cuioss.sheriff.gateway.events.EventTypeTest#successEvents") @DisplayName("Should carry no category and no HTTP mapping") void shouldCarryNoCategoryAndNoHttpMapping(EventType eventType) { assertAll("success event " + eventType, () -> assertNull(eventType.category(), "Success events carry a null category"), () -> assertFalse(eventType.isFailure(), "Success events are not failures"), () -> assertEquals(0, eventType.httpStatus(), "Success events have no HTTP status"), - () -> assertFalse(eventType.hasHttpMapping(), "Success events have no HTTP mapping")); + () -> assertFalse(eventType.hasHttpMapping(), "Success events have no HTTP mapping"), + () -> assertEquals(0, eventType.wsCloseCode(), "Success events have no WebSocket-close mapping")); } } @@ -53,7 +155,7 @@ void shouldCarryNoCategoryAndNoHttpMapping(EventType eventType) { class ConfigurationEvents { @ParameterizedTest - @EnumSource(names = {"CONFIG_INVALID", "AUTH_WEAKENED"}) + @MethodSource("de.cuioss.sheriff.gateway.events.EventTypeTest#configurationEvents") @DisplayName("Should be failures in the CONFIGURATION category with no HTTP mapping") void shouldBeConfigurationFailuresWithoutHttpMapping(EventType eventType) { assertAll("configuration event " + eventType, @@ -70,23 +172,7 @@ void shouldBeConfigurationFailuresWithoutHttpMapping(EventType eventType) { class FailureEvents { @ParameterizedTest(name = "{0} -> {1} ({2})") - @CsvSource({ - "SECURITY_FILTER_VIOLATION, 400, INPUT_VALIDATION", - "PATH_NOT_ALLOWED, 400, INPUT_VALIDATION", - "PARAMETER_LIMIT_EXCEEDED, 400, INPUT_VALIDATION", - "NO_ROUTE_MATCHED, 404, INPUT_VALIDATION", - "PASSTHROUGH_HOST_SMUGGLED, 404, INPUT_VALIDATION", - "METHOD_NOT_ALLOWED, 405, INPUT_VALIDATION", - "RESERVED_BODY_TOO_LARGE, 413, INPUT_VALIDATION", - "TOKEN_MISSING, 401, AUTHENTICATION", - "TOKEN_INVALID, 401, AUTHENTICATION", - "SCOPE_MISSING, 403, AUTHORIZATION", - "CSRF_REJECTED, 403, AUTHORIZATION", - "LOGOUT_TOKEN_INVALID, 400, AUTHENTICATION", - "UPSTREAM_ERROR, 502, UPSTREAM", - "UPSTREAM_CIRCUIT_OPEN, 503, UPSTREAM", - "UPSTREAM_TIMEOUT, 504, UPSTREAM" - }) + @MethodSource("de.cuioss.sheriff.gateway.events.EventTypeTest#errorContract") @DisplayName("Should map each error-contract row to its status and category") void shouldMapEachErrorContractRow(EventType eventType, int expectedStatus, EventCategory expectedCategory) { assertAll("failure event " + eventType, @@ -97,6 +183,71 @@ void shouldMapEachErrorContractRow(EventType eventType, int expectedStatus, Even } } + @Nested + @DisplayName("Established-relay WebSocket-close events") + class WebSocketCloseEvents { + + @ParameterizedTest + @MethodSource("de.cuioss.sheriff.gateway.events.EventTypeTest#websocketCloseEvents") + @DisplayName("Should carry a close code and no HTTP mapping") + void shouldCarryCloseCodeWithoutHttpMapping(EventType eventType) { + assertAll("websocket-close event " + eventType, + () -> assertNull(eventType.category(), + "A reclaimed relay is not a request-time rejection, so it carries no category"), + () -> assertFalse(eventType.isFailure(), "WebSocket-close events are not failures"), + () -> assertEquals(0, eventType.httpStatus(), + "The close happens after the 101 upgrade, so there is no HTTP status to render"), + () -> assertFalse(eventType.hasHttpMapping(), "WebSocket-close events have no HTTP mapping"), + () -> assertTrue(eventType.wsCloseCode() > 0, "WebSocket-close events carry a close code")); + } + + @Test + @DisplayName("Should close an idle relay with 1001 Going Away") + void shouldCloseIdleRelayWithGoingAway() { + // The literal close code, stated independently of the enum: the parameterized test above + // derives its membership from wsCloseCode(), so it cannot also be what pins the value. + assertEquals(1001, EventType.WEBSOCKET_IDLE_TIMEOUT.wsCloseCode(), + "An idle relay is closed with 1001 Going Away"); + } + } + + @Nested + @DisplayName("Contract coverage (drift guards)") + class ContractCoverage { + + @Test + @DisplayName("Should cover exactly the HTTP-mapped constants in the error-contract table") + void shouldCoverExactlyTheHttpMappedConstants() { + Set mapped = EnumSet.allOf(EventType.class).stream() + .filter(EventType::hasHttpMapping) + .collect(() -> EnumSet.noneOf(EventType.class), Set::add, Set::addAll); + + assertEquals(mapped, errorContractKeys(), + "Every EventType with hasHttpMapping() must have an error-contract row, and no row may" + + " name an unmapped constant — add the row in the same change as the constant"); + } + + @Test + @DisplayName("Should claim every EventType constant in exactly one bucket") + void shouldClaimEveryConstantExactlyOnce() { + List claimed = Stream + .of(SUCCESS_EVENTS, CONFIGURATION_EVENTS, WEBSOCKET_CLOSE_EVENTS, errorContractKeys()) + .flatMap(Collection::stream) + .toList(); + + // The three membership sets are derived from the enum, so this is not guarding against a + // forgotten list entry — it guards the PREDICATES. A constant whose accessor combination + // no shape covers (or that two shapes both claim) means the four predicates have stopped + // being a partition, and the buckets would silently disagree about who owns it. + assertAll("bucket partition", + () -> assertEquals(EnumSet.allOf(EventType.class), EnumSet.copyOf(claimed), + "Every EventType constant must be claimed by one of the four bucket predicates —" + + " an unclaimed constant means the predicates no longer cover the enum"), + () -> assertEquals(claimed.size(), EnumSet.copyOf(claimed).size(), + "No EventType constant may be claimed by more than one bucket predicate")); + } + } + @Test @DisplayName("Should derive the RFC 9457 problem type from the category slug") void shouldDeriveProblemTypeFromCategorySlug() { diff --git a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/ThoroughChecksStageTest.java b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/ThoroughChecksStageTest.java index bdb2a1b3..fffd75c9 100644 --- a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/ThoroughChecksStageTest.java +++ b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/ThoroughChecksStageTest.java @@ -125,7 +125,7 @@ void rejectsBodyExceedingRouteCap() { () -> stage.process(request, List.of())); // Assert - assertEquals(EventType.PARAMETER_LIMIT_EXCEEDED, thrown.getEventType()); + assertEquals(EventType.CONTENT_TOO_LARGE, thrown.getEventType()); } @Test diff --git a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/ConfigFailFastTest.java b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/ConfigFailFastTest.java index 4dca1c93..ae7badf7 100644 --- a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/ConfigFailFastTest.java +++ b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/ConfigFailFastTest.java @@ -19,6 +19,7 @@ import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; +import java.math.BigInteger; import java.net.URISyntaxException; import java.nio.file.Path; @@ -27,6 +28,7 @@ import de.cuioss.test.juli.TestLogLevel; import de.cuioss.test.juli.junit5.EnableTestLogger; +import io.quarkus.runtime.configuration.MemorySize; import org.junit.jupiter.api.Test; /** @@ -40,11 +42,21 @@ @EnableTestLogger class ConfigFailFastTest { + /** + * The framework body ceiling wired onto every producer here, matching the shipped + * {@code quarkus.http.limits.max-body-size} floor. None of these fixtures declares a + * {@code max_body_bytes}, so the framework-limit check is inert for them — the field is set + * because the producer reads it on every boot, not because these tests exercise the check + * (that is {@code ConfigProducerTest}'s job). + */ + private static final long FRAMEWORK_LIMIT_BYTES = 67108864L; + private static ConfigProducer producerFor(String resourceDir) throws URISyntaxException { var resource = ConfigFailFastTest.class.getResource(resourceDir); assertNotNull(resource, resourceDir + " fixture must be on the test classpath"); ConfigProducer producer = new ConfigProducer(); producer.configDir = Path.of(resource.toURI()).toString(); + producer.frameworkBodyLimit = new MemorySize(BigInteger.valueOf(FRAMEWORK_LIMIT_BYTES)); return producer; } diff --git a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducerTest.java b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducerTest.java index 78e834d9..5b54e44b 100644 --- a/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducerTest.java +++ b/api-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducerTest.java @@ -23,6 +23,7 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import java.io.IOException; +import java.math.BigInteger; import java.nio.file.Files; import java.nio.file.Path; @@ -38,6 +39,7 @@ import de.cuioss.test.juli.TestLogLevel; import de.cuioss.test.juli.junit5.EnableTestLogger; +import io.quarkus.runtime.configuration.MemorySize; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; @@ -129,6 +131,35 @@ class ConfigProducerTest { websocket_relay_cap: 16 """; + /** + * The framework ceiling every producer in this class is wired with. Deliberately small and + * unrelated to the shipped {@code 64M} default: the check under test is the inequality between + * the declared cap and the injected limit, so a tiny limit exercises it exactly and keeps the + * fixture documents readable. + */ + private static final long FRAMEWORK_LIMIT_BYTES = 1024L; + + /** An anchor declaring a body cap ABOVE {@link #FRAMEWORK_LIMIT_BYTES} — unreachable, so boot must abort. */ + private static final String GATEWAY_WITH_CAP_ABOVE_FRAMEWORK_LIMIT = gatewayDeclaringBodyCap(2048); + + /** An anchor declaring a body cap exactly AT {@link #FRAMEWORK_LIMIT_BYTES} — reachable, so boot is clean. */ + private static final String GATEWAY_WITH_CAP_AT_FRAMEWORK_LIMIT = gatewayDeclaringBodyCap(1024); + + private static String gatewayDeclaringBodyCap(int maxBodyBytes) { + return """ + version: 1 + metadata: + config_version: "2026-07-13" + anchors: + uploads: + path_prefix: /uploads + type: proxy + access: public + security_filter: + max_body_bytes: %d + """.formatted(maxBodyBytes); + } + @TempDir Path configDir; @@ -145,6 +176,7 @@ private ConfigProducer producerForGateway(String gatewayYaml) throws IOException Files.writeString(configDir.resolve("gateway.yaml"), gatewayYaml); ConfigProducer producer = new ConfigProducer(); producer.configDir = configDir.toString(); + producer.frameworkBodyLimit = new MemorySize(BigInteger.valueOf(FRAMEWORK_LIMIT_BYTES)); return producer; } @@ -196,6 +228,42 @@ void shouldRefuseBootWhenTheRelayCapExceedsTheAdmissionCap() throws Exception { "a relay sub-budget larger than the admission pool aborts boot rather than serving"); } + /** + * The fail-closed framework-limit check: a declared {@code max_body_bytes} above the Vert.x + * ceiling is unreachable, because Quarkus rejects the oversize request in a root handler in + * front of the gateway router. Rather than let that mismatch stay silent, boot aborts through + * the same collected-violation path, and the violation names the Quarkus key the operator must + * raise — so the ERROR record is actionable without reading the source. + */ + @Test + void shouldRefuseBootWhenADeclaredBodyCapExceedsTheFrameworkLimit() throws Exception { + ConfigProducer producer = producerForGateway(GATEWAY_WITH_CAP_ABOVE_FRAMEWORK_LIMIT); + + IllegalStateException exception = assertThrows(IllegalStateException.class, + () -> producer.onStartup(null), + "a declared body cap above the framework limit must abort boot rather than be silently clipped"); + + assertTrue(exception.getMessage().contains("Refusing to start"), + "the abort should carry the refusing-to-start summary"); + LogAsserts.assertLogMessagePresentContaining(TestLogLevel.ERROR, + "quarkus.http.limits.max-body-size"); + } + + /** + * The boundary of the same inequality: a declared cap exactly AT the framework limit is + * reachable, so it must boot clean. This pins the comparison as {@code >} rather than + * {@code >=} — the shipped configuration deliberately sets the floor equal to the largest + * declared cap, so an off-by-one here would refuse every default deployment. + */ + @Test + void shouldBootWhenTheDeclaredBodyCapEqualsTheFrameworkLimit() throws Exception { + ConfigProducer producer = producerForGateway(GATEWAY_WITH_CAP_AT_FRAMEWORK_LIMIT); + + assertDoesNotThrow(() -> producer.onStartup(null), + "a declared cap equal to the framework limit is reachable and must boot clean"); + assertNotNull(producer.gatewayConfig(), "beans should be available after startup assembly"); + } + @Test void shouldLogConfigLoadedOnSuccess() throws Exception { ConfigProducer producer = producerForValidConfig(); diff --git a/benchmarks/README.adoc b/benchmarks/README.adoc index 6043c5b2..02ec729d 100644 --- a/benchmarks/README.adoc +++ b/benchmarks/README.adoc @@ -153,7 +153,13 @@ fail-fast discarded goals 5-12, which had therefore never run in CI even once. B `pom.xml` is not evidence that a goal produces a number. Current disposition — the CI end-of-job coverage step (see `.github/workflows/benchmark.yml`) -enforces this table, so a goal that silently stops running fails the job: +enforces this, so a goal that silently stops running fails the job. It does not read the table +below: it derives the expected set from the build-generated execution manifest +(`benchmarks/target/benchmark-execution-manifest.json`, emitted at `initialize` by the +`generate-benchmark-manifest` execution in `pom.xml`), so the gate cannot drift from the POM the way +a hand-maintained list can. The workflow calls `benchmark-manifest.py summarise` rather than +re-deriving the summary itself, so the manifest has exactly one consumer. The table here is +documentation of that resolved set, not its source: [cols="2,1,4", options="header"] |=== @@ -173,11 +179,14 @@ enforces this table, so a goal that silently stops running fails the job: |First execution in CI under PLAN-25. Their trend series start empty here. |`uploadLarge` -|*skipped* -|Skipped by `skip.benchmark.upload.large`, which defaults to `true`. The gateway rejects the 50MB - body with `413`: the `upload` anchor declares `max_body_bytes: 67108864`, but the Quarkus HTTP body - limit is not derived from it, so the declared 64 MiB cap is unreachable and the body is refused - before the security filter runs. Blocked on a gateway-side fix. +|runs +|Skipped until the framework body-size floor was declared: the `upload` anchor declares + `max_body_bytes: 67108864`, but `quarkus.http.limits.max-body-size` was left at the Quarkus 10 MiB + default, so the declared 64 MiB cap was unreachable and the 50MB body was refused with `413` before + the security filter ran. `application.properties` now declares that floor at `64M`, matching the + largest declared cap, so the goal measures upload throughput instead of the rejection path. Its + trend series starts empty. `skip.benchmark.upload.large` is retained (now `false`) so the goal stays + individually suppressible. |`websocketEcho` |runs @@ -193,20 +202,22 @@ enforces this table, so a goal that silently stops running fails the job: |Backs no Maven goal on purpose — see below. |=== -*Skipped means not executed.* A skipped goal carries a `` element in `benchmarks/pom.xml` bound -to its own property, declared there with the id of the finding that blocks it and defaulting to -`true` — so Maven does not run the goal, it consumes no wall time, and it cannot abort the suite. -The two states are kept distinct rather than collapsed: the CI coverage step gates on the eleven -goals that execute and names each skipped goal, its skip property and its reason in the job summary. -When Maven fails, that step also states in words whether the failure truncated the suite. Silence is -never the signal. +*Skipped means not executed.* A goal suppressed by a `` element in `benchmarks/pom.xml` — bound +to its own property — is not run at all: it consumes no wall time and cannot abort the suite. No k6 +goal is suppressed today; every declared goal executes. The two states are kept distinct rather than +collapsed: the CI coverage step gates on whichever goals the execution manifest reports as `enabled`, +and names every non-executing goal with its state, its skip property and its reason in the job +summary. Both sides of that split come from the manifest, so suppressing a goal automatically moves it +from the gated set into the reported table — there is no second list to remember to update. When Maven +fails, that step also states in words whether the failure truncated the suite. Silence is never the +signal. -Re-enable a goal once its gateway defect is fixed by flipping its property, which is also how you -reproduce the defect through the harness: +The per-goal properties are retained even when `false`, so a single goal can be suppressed for a +local run without editing its execution: [source,bash] ---- -./mvnw clean verify -pl benchmarks -Pbenchmark -Dskip.benchmark.upload.large=false +./mvnw clean verify -pl benchmarks -Pbenchmark -Dskip.benchmark.upload.large=true ---- === Deliberately unwired scripts @@ -216,10 +227,12 @@ reproduce the defect through the harness: server-session variant and explicitly deferred its per-variant benchmark additions because `benchmarks/**` is PLAN-25's surface. The script is therefore ready but unclaimed, not forgotten. -It is listed in the CI end-of-job coverage summary under its own skip section, so its absence from a -run is stated in every job rather than reading as a silent omission. Do not treat a missing -`sessionMediated-summary.json` as a regression, and do not wire the script to a Maven goal without -the owning plan. +The manifest generator discovers it by reading the `@unwiredReason` tag in the script's own header +comment and reports it as `unwired`, so the CI coverage summary lists it — with that reason — in the +"Not expected to run" table of every job rather than letting its absence read as a silent omission. +The tag in the script is the single place that reason is written; keep it there rather than +duplicating it into the workflow. Do not treat a missing `sessionMediated-summary.json` as a +regression, and do not wire the script to a Maven goal without the owning plan. == Running the CI baseline lane @@ -503,10 +516,10 @@ know: . *Post-processing*: `WrkBenchmarkConverter` / `WrkResultPostProcessor` are replaced by `K6BenchmarkConverter` / `K6ResultPostProcessor`. The downstream report pipeline (badges, 10-run history, trends, GitHub Pages) consumes an identically-shaped model and is unchanged. -. *Coverage*: the lane grew from three wired executions to twelve *declared* goals, eleven of which - execute and are expected to produce a CI result — only `uploadLarge` is declared but skipped by - property against an open gateway defect, so Maven does not run it at all. `websocketEcho` was - skipped alongside it until its blocking edge admission-permit leak was fixed. `bearer` +. *Coverage*: the lane grew from three wired executions to twelve *declared* goals, all of which + execute and are expected to produce a CI result. Two of them reached that state late: `websocketEcho` + once its blocking edge admission-permit leak was fixed, and `uploadLarge` once the framework + body-size floor was declared under the `upload` anchor's `max_body_bytes`. `bearer` is genuinely new CI coverage — the wrk-era bearer runner existed on disk but was registered in no Maven execution. Being *wired* is not the same as *executing*, and *executing* is not the same as *producing a result*: until PLAN-25 the job died at the fourth goal and goals 5-12 had never run @@ -525,11 +538,10 @@ incomparable, the k6 series starts fresh rather than continuing the wrk series: (see the `latency_ms.stdev` limit above). * Every benchmark that writes a summary document for the first time begins with *no history* and needs ten runs before its trend series is meaningful. Under PLAN-25 that is most of the lane: - eight of the twelve declared goals had never executed in CI, and seven of those eight are expected - to produce a result and so start their series empty — six at the PLAN-25 commit, and - `websocketEcho` once its blocking permit leak was fixed. `uploadLarge` starts *no* series at all — - only a goal that actually writes a summary document opens one, and it is skipped by property - rather than executed. The workflow's end-of-job coverage step + eight of the twelve declared goals had never executed in CI, and all eight are expected + to produce a result and so start their series empty — six at the PLAN-25 commit, + `websocketEcho` once its blocking permit leak was fixed, and `uploadLarge` once the framework + body-size floor was declared. The workflow's end-of-job coverage step (`.github/workflows/benchmark.yml`) is the record of which goals produced a result in a given run. A flat or short trend line on a name that *did* run is an absence of history, not a regression. diff --git a/benchmarks/pom.xml b/benchmarks/pom.xml index 5ed9b10b..b7986217 100644 --- a/benchmarks/pom.xml +++ b/benchmarks/pom.xml @@ -28,18 +28,15 @@ true - - - - true + + false ${k6.results.dir}/history + + ${project.build.directory}/benchmark-execution-manifest.json + true @@ -195,6 +200,44 @@ org.codehaus.mojo exec-maven-plugin + + + generate-benchmark-manifest + initialize + + exec + + + python3 + + ${project.basedir}/scripts/benchmark-manifest.py + generate + --pom + ${project.basedir}/pom.xml + --k6-script-dir + ${k6.script.dir} + --output + ${benchmark.manifest.file} + --skip-property + skip.benchmark.upload.large=${skip.benchmark.upload.large} + --skip-property + skip.benchmark.websocket.echo=${skip.benchmark.websocket.echo} + + ${project.basedir} + + + maven-build-integration-tests @@ -558,14 +601,14 @@ - @@ -578,7 +621,7 @@ exec - + ${skip.benchmark.upload.large} docker diff --git a/benchmarks/scripts/benchmark-manifest.py b/benchmarks/scripts/benchmark-manifest.py new file mode 100644 index 00000000..dce53771 --- /dev/null +++ b/benchmarks/scripts/benchmark-manifest.py @@ -0,0 +1,401 @@ +#!/usr/bin/env python3 +"""Benchmark execution manifest: generate it from the build, then consume it in CI. + +The CI coverage step used to carry a hand-written list of the benchmark goals that must +produce a summary document, plus a hand-written table of the goals deliberately not run. +Both mirrored `benchmarks/pom.xml` with nothing enforcing agreement, and both had already +drifted. This script removes the mirror: `generate` derives the goal set from the Maven +model itself, and `summarise` renders the CI job summary from what `generate` wrote. + +`generate` reads three authoritative sources and nothing else: + + * `benchmarks/pom.xml` -- which `exec-maven-plugin` executions run a k6 script, in which + order, and which property (if any) suppresses each one. + * the resolved value of every such skip property, handed over by Maven on the command + line so a `-Dskip.benchmark.upload.large=true` override is reflected. An execution + whose skip property was not handed over is a hard error, so a new suppressible goal + cannot be added without wiring its value through. + * the k6 scripts themselves -- each script names the summary document it writes through + its own `BENCHMARK_NAME`, so the manifest's goal names are the names k6 will actually + use rather than a third copy of them. + +A k6 script present in the script directory but referenced by no execution is reported as +`unwired` rather than dropped, so "deliberately not wired" never reads as "forgotten". + +Manifest schema (`benchmarks/target/benchmark-execution-manifest.json`): + + { + "schema_version": 1, + "profile": "benchmark", + "executions": [ + {"goal": "healthLiveCheck", "execution_id": "run-k6-health-live-benchmark", + "script": "health_live.js", "state": "enabled", + "skip_property": null, "reason": null}, + ... + ] + } + +`state` is one of `enabled`, `skipped` or `unwired`. +""" + +import argparse +import json +import os +import re +import sys +import xml.etree.ElementTree as ElementTree + +MAVEN_NS = {"m": "http://maven.apache.org/POM/4.0.0"} + +#: The k6 script argument shape an execution passes to the containerised k6 runner. +SCRIPT_ARGUMENT = re.compile(r"^/scripts/(?P[\w.\-]+\.js)$") + +#: A script that names its summary document with a single module-level constant. +STATIC_NAME = re.compile(r"const\s+BENCHMARK_NAME\s*=\s*'(?P[^']+)'") + +#: A script that selects its summary name from a mode switch (passthrough_relay.js). +MODE_NAME = re.compile(r"case\s+'(?P[^']+)'\s*:\s*return\s*\{\s*benchmarkName:\s*'(?P[^']+)'", + re.DOTALL) + +#: The reason marker an unwired script carries in its file header. +UNWIRED_REASON = re.compile(r"@unwiredReason\s+(?P.+?)\s*(?:\n\s*\*\s*\n|\n\s*\*/)", re.DOTALL) + +#: A `` element that defers to a property, e.g. `${skip.benchmark.upload.large}`. +PROPERTY_REFERENCE = re.compile(r"^\$\{(?P[^}]+)\}$") + +STATE_ENABLED = "enabled" +STATE_SKIPPED = "skipped" +STATE_UNWIRED = "unwired" + + +class ManifestError(RuntimeError): + """A defect in the build model the manifest cannot paper over.""" + + +def _text(element, path): + """Returns the stripped text of ``path`` under ``element``, or ``None``.""" + if element is None: + return None + found = element.find(path, MAVEN_NS) + if found is None or found.text is None: + return None + return found.text.strip() + + +def _local_name(tag): + return tag.rsplit("}", 1)[-1] + + +def _profile(root, profile_id): + for profile in root.findall("m:profiles/m:profile", MAVEN_NS): + if _text(profile, "m:id") == profile_id: + return profile + raise ManifestError(f"benchmarks/pom.xml declares no with id '{profile_id}'") + + +def _module_properties(root): + properties = {} + container = root.find("m:properties", MAVEN_NS) + if container is not None: + for child in container: + properties[_local_name(child.tag)] = (child.text or "").strip() + return properties + + +def _exec_executions(profile): + for plugin in profile.findall("m:build/m:plugins/m:plugin", MAVEN_NS): + if _text(plugin, "m:artifactId") != "exec-maven-plugin": + continue + for execution in plugin.findall("m:executions/m:execution", MAVEN_NS): + yield execution + + +def _script_argument(configuration): + if configuration is None: + return None + for argument in configuration.findall("m:arguments/m:argument", MAVEN_NS): + match = SCRIPT_ARGUMENT.match((argument.text or "").strip()) + if match: + return match.group("name") + return None + + +def _environment(configuration): + environment = {} + container = configuration.find("m:environmentVariables", MAVEN_NS) + if container is not None: + for child in container: + environment[_local_name(child.tag)] = (child.text or "").strip() + return environment + + +def _resolve_skip(configuration, execution_id, skip_properties): + """Returns ``(skipped, skip_property)`` for one execution. + + An execution with no ```` runs whenever the profile is active. An execution whose + ```` defers to a property is resolved against the values Maven handed over; a + property that was not handed over is fatal rather than assumed false, so a new + suppressible goal cannot silently report as enabled. + """ + raw = _text(configuration, "m:skip") + if raw is None: + return False, None + reference = PROPERTY_REFERENCE.match(raw) + if not reference: + return raw.lower() == "true", None + name = reference.group("name") + if name not in skip_properties: + raise ManifestError( + f"execution '{execution_id}' is suppressed by property '{name}', but that property's" + f" resolved value was not handed over. Add" + f" --skip-property {name}=${{{name}}} to the generate-benchmark-manifest execution" + f" in benchmarks/pom.xml.") + return skip_properties[name].lower() == "true", name + + +def _script_source(script_dir, script): + path = os.path.join(script_dir, script) + if not os.path.isfile(path): + raise ManifestError(f"k6 script '{script}' is referenced by an execution but is not on disk at {path}") + with open(path, encoding="utf-8") as handle: + return handle.read() + + +def _goal_name(source, script, environment): + """Resolves the summary-document name the script will write. + + A script naming itself with a single constant answers directly. A script selecting its + name from a mode switch is resolved through the mode its execution passes in the + environment; an unresolvable mode is fatal, because guessing would mislabel the goal. + """ + static = STATIC_NAME.search(source) + if static: + return static.group("name") + + modes = {mode.lower(): name for mode, name in MODE_NAME.findall(source)} + if not modes: + raise ManifestError( + f"k6 script '{script}' declares neither a BENCHMARK_NAME constant nor a mode switch," + f" so the summary document it writes cannot be derived") + for value in environment.values(): + if value.lower() in modes: + return modes[value.lower()] + raise ManifestError( + f"k6 script '{script}' selects its benchmark name from modes {sorted(modes)}, but its" + f" execution passes none of them in the environment") + + +def _unwired_names(source, script): + static = STATIC_NAME.search(source) + if static: + return [static.group("name")] + names = sorted({name for _, name in MODE_NAME.findall(source)}) + if not names: + raise ManifestError( + f"k6 script '{script}' declares no benchmark name, so it cannot be reported as unwired") + return names + + +def _unwired_reason(source): + match = UNWIRED_REASON.search(source) + if not match: + return None + return " ".join(part.strip().lstrip("*").strip() for part in match.group("reason").splitlines()).strip() + + +def generate(pom_path, script_dir, output_path, profile_id, skip_properties): + root = ElementTree.parse(pom_path).getroot() + profile = _profile(root, profile_id) + properties = _module_properties(root) + + executions = [] + wired_scripts = set() + + for execution in _exec_executions(profile): + configuration = execution.find("m:configuration", MAVEN_NS) + script = _script_argument(configuration) + if script is None: + continue + execution_id = _text(execution, "m:id") or "(unnamed)" + wired_scripts.add(script) + skipped, skip_property = _resolve_skip(configuration, execution_id, skip_properties) + source = _script_source(script_dir, script) + goal = _goal_name(source, script, _environment(configuration)) + reason = properties.get(f"{skip_property}.reason") if skip_property else None + executions.append({ + "goal": goal, + "execution_id": execution_id, + "script": script, + "state": STATE_SKIPPED if skipped else STATE_ENABLED, + "skip_property": skip_property, + "reason": reason if skipped else None, + }) + + for script in sorted(os.listdir(script_dir)): + if not script.endswith(".js") or script in wired_scripts: + continue + source = _script_source(script_dir, script) + reason = _unwired_reason(source) + for goal in _unwired_names(source, script): + executions.append({ + "goal": goal, + "execution_id": None, + "script": script, + "state": STATE_UNWIRED, + "skip_property": None, + "reason": reason, + }) + + if not any(entry["state"] == STATE_ENABLED for entry in executions): + raise ManifestError( + f"the '{profile_id}' profile resolved to no executing k6 goal — refusing to write a" + f" manifest that would make every goal's absence look expected") + + manifest = {"schema_version": 1, "profile": profile_id, "executions": executions} + os.makedirs(os.path.dirname(output_path) or ".", exist_ok=True) + with open(output_path, "w", encoding="utf-8") as handle: + json.dump(manifest, handle, indent=2) + handle.write("\n") + + executing = [entry["goal"] for entry in executions if entry["state"] == STATE_ENABLED] + print(f"Benchmark execution manifest written to {output_path}: " + f"{len(executing)} executing goal(s), " + f"{sum(1 for e in executions if e['state'] == STATE_SKIPPED)} skipped, " + f"{sum(1 for e in executions if e['state'] == STATE_UNWIRED)} unwired.") + return 0 + + +def _not_run_reason(entry): + if entry["state"] == STATE_UNWIRED: + return entry["reason"] or "Wired to no Maven goal; no reason declared — add an @unwiredReason tag to the script." + if entry["reason"]: + return entry["reason"] + return (f"No reason declared — add a `{entry['skip_property']}.reason` property to benchmarks/pom.xml." + if entry["skip_property"] else "No reason declared.") + + +def _skipped_by(entry): + if entry["state"] == STATE_UNWIRED: + return "not wired" + return f"`{entry['skip_property']}`" if entry["skip_property"] else "an inline ``" + + +def summarise(manifest_path, results_dir, benchmark_outcome, summary_file): + lines = [] + if not os.path.isfile(manifest_path): + lines.append("## Benchmark coverage") + lines.append("") + lines.append(f"> **The execution manifest at `{manifest_path}` was never generated**, so which " + "goals were expected cannot be stated. The build did not reach the manifest step.") + _emit(lines, summary_file) + print(f"::error::benchmark execution manifest absent at {manifest_path}.") + return 1 if benchmark_outcome == "success" else 0 + + with open(manifest_path, encoding="utf-8") as handle: + manifest = json.load(handle) + executions = manifest["executions"] + + expected = [entry for entry in executions if entry["state"] == STATE_ENABLED] + not_run = [entry for entry in executions if entry["state"] != STATE_ENABLED] + + missing = [] + lines.append("## Benchmark coverage") + lines.append("") + lines.append("| Benchmark | Result |") + lines.append("| --- | --- |") + for entry in expected: + goal = entry["goal"] + if os.path.isfile(os.path.join(results_dir, f"{goal}-summary.json")): + lines.append(f"| `{goal}` | ran |") + else: + lines.append(f"| `{goal}` | **DID NOT RUN** |") + missing.append(goal) + + lines.append("") + lines.append("### Deliberately not run") + lines.append("") + if not_run: + lines.append("| Benchmark | Skipped by | Reason |") + lines.append("| --- | --- | --- |") + for entry in not_run: + lines.append(f"| `{entry['goal']}` | {_skipped_by(entry)} | {_not_run_reason(entry)} |") + else: + lines.append(f"Every one of the {len(expected)} declared goals executes; none is suppressed or unwired.") + + if benchmark_outcome != "success": + lines.append("") + if missing: + lines.append(f"> **Maven failed and the suite was TRUNCATED**: goals {', '.join(missing)} produced " + f"no summary because the run aborted before reaching them. The DID NOT RUN rows above " + f"are a consequence of that failure, not {len(missing)} independent problems — fix the " + f"reported Maven error first, then re-read this table.") + else: + lines.append("> **Maven failed AFTER every expected goal produced a summary.** No goal was lost to " + "truncation, so the failure lies outside goal execution (post-processing, the baseline " + "comparison, or container teardown). The results above are complete and usable.") + + _emit(lines, summary_file) + + print(f"Benchmark coverage: {len(missing)} of {len(expected)} expected summary document(s) absent " + f"(benchmark step outcome: {benchmark_outcome}).") + + # Only fail on a missing summary when Maven itself reported success: when the benchmark step + # already failed, its own error is the actionable signal and this step must not mask it. + if missing and benchmark_outcome == "success": + print(f"::error::{len(missing)} expected benchmark summary document(s) absent although the " + f"benchmark step succeeded: {', '.join(missing)}.") + return 1 + return 0 + + +def _emit(lines, summary_file): + text = "\n".join(lines) + "\n" + if summary_file: + with open(summary_file, "a", encoding="utf-8") as handle: + handle.write(text) + else: + sys.stdout.write(text) + + +def _skip_property(raw): + if "=" not in raw: + raise argparse.ArgumentTypeError(f"--skip-property expects name=value, got '{raw}'") + name, value = raw.split("=", 1) + return name, value + + +def main(argv=None): + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + subparsers = parser.add_subparsers(dest="command", required=True) + + generate_parser = subparsers.add_parser( + "generate", help="derive the benchmark execution manifest from the Maven model") + generate_parser.add_argument("--pom", required=True, help="path to benchmarks/pom.xml") + generate_parser.add_argument("--k6-script-dir", required=True, help="directory holding the k6 scripts") + generate_parser.add_argument("--output", required=True, help="manifest output path") + generate_parser.add_argument("--profile", default="benchmark", help="the Maven profile to read") + generate_parser.add_argument("--skip-property", action="append", default=[], type=_skip_property, + metavar="NAME=VALUE", + help="a skip property's Maven-resolved value; repeatable") + + summarise_parser = subparsers.add_parser( + "summarise", help="render the CI coverage summary from the manifest and the written results") + summarise_parser.add_argument("--manifest", required=True, help="manifest path") + summarise_parser.add_argument("--results-dir", required=True, help="directory holding k6 summary documents") + summarise_parser.add_argument("--benchmark-outcome", required=True, + help="the benchmark step's outcome, e.g. success or failure") + summarise_parser.add_argument("--summary-file", default=None, + help="file to append the markdown to (defaults to stdout)") + + args = parser.parse_args(argv) + try: + if args.command == "generate": + return generate(args.pom, args.k6_script_dir, args.output, args.profile, dict(args.skip_property)) + return summarise(args.manifest, args.results_dir, args.benchmark_outcome, args.summary_file) + except ManifestError as error: + print(f"::error::{error}", file=sys.stderr) + return 1 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/benchmarks/src/main/resources/k6-scripts/session_mediated.js b/benchmarks/src/main/resources/k6-scripts/session_mediated.js index 24645594..148c4b78 100644 --- a/benchmarks/src/main/resources/k6-scripts/session_mediated.js +++ b/benchmarks/src/main/resources/k6-scripts/session_mediated.js @@ -20,6 +20,11 @@ * checks rate of 0 and fails the build instead of reporting the redirect/rejection path as an * excellent result. {@link setup} is fail-loud for the same reason: a run that could not establish * a session aborts here rather than measuring the unauthenticated fall-through. + * + * @unwiredReason Wired to no Maven goal on purpose -- BFF session mediation belongs to PLAN-07A, + * which deferred its per-variant benchmark additions. benchmark-manifest.py reads this tag and + * reports the script as `unwired` in the CI coverage summary, so its absence from a run is stated + * rather than silent. */ import http from 'k6/http'; import { check, fail } from 'k6'; diff --git a/doc/adr/0023-The_declared_max_body_bytes_cap_is_the_effective_one_and_a_breach_is_the_gateways_413.adoc b/doc/adr/0023-The_declared_max_body_bytes_cap_is_the_effective_one_and_a_breach_is_the_gateways_413.adoc new file mode 100644 index 00000000..27b6c6d6 --- /dev/null +++ b/doc/adr/0023-The_declared_max_body_bytes_cap_is_the_effective_one_and_a_breach_is_the_gateways_413.adoc @@ -0,0 +1,153 @@ += ADR-0023: The declared max_body_bytes cap is the effective one and a breach is the gateway's 413 +:toc: left +:toclevels: 2 +:sectnums: + +// adr-metadata +// Progressive-disclosure metadata block (see manage-adr SKILL.md → "ADR Template +// Structure"). Read by `manage-adr.py scan` so a caller can assess an ADR's +// relevance without reading the full file. List fields are comma-separated. +// summary: A route's declared security_filter.max_body_bytes is the effective ceiling; the framework floor is derived from it and validated fail-closed at boot, and a breach is rendered by the gateway as an RFC 9457 413. +// tags: body-limits, error-contract, fail-closed, configuration, edge +// affects: api-sheriff/src/main/resources/application.properties, api-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducer.java, api-sheriff/src/main/java/de/cuioss/sheriff/gateway/events/EventType.java, api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GrpcStatusMapper.java, doc/configuration.adoc, doc/architecture.adoc +// supersedes: +// end-adr-metadata + +== Status + +Accepted + +== Context + +A gateway that lets an operator declare a per-route request-body ceiling owns two +limits, not one: the ceiling the configuration declares, and the ceiling the HTTP +framework beneath it enforces. Whichever is lower is the one that actually applies. + +That creates a class of defect in which a documented configuration key is inert. The +declared value is parsed, modelled and enforced by real gateway code, yet every +request that would exercise it is terminated by the framework first, so the +enforcement path is never reached. Nothing fails: the framework's rejection is a +well-formed HTTP response, so the deployment looks correct from the outside while the +operator's stated policy is silently not the policy in force. Unit tests do not +detect it, because they exercise the filter directly — below the layer where the +truncation happens — and therefore pass on a path production never takes. + +The choice point is structural: either the framework floor is derived from the +declared configuration so the gateway is reliably the component that rejects, or the +two limits are allowed to disagree and the effective ceiling becomes an emergent +property of whichever happens to be smaller. + +A second question follows from the first. Once the gateway is the rejecting +component, its rejection must be distinguishable from the framework's. The framework +answers an over-limit request from a root handler in front of the router, with a bare +status line and no response body. If the gateway's own rejection is not +distinguishable from that, a test asserting "the request was rejected" cannot tell +whether the enforcement under test ran at all — which is precisely the blind spot +that let the inert key survive. + +== Decision + +**The declared cap is the effective cap.** The framework body limit is configuration +derived from the declared anchors rather than an independent knob, and the +relationship between them is validated fail-closed at boot: startup aborts when any +route declares a `max_body_bytes` above the framework floor, naming the offending +descriptor. A deployment whose stated policy cannot be honoured does not start, it +does not start and serve a quietly smaller limit. + +Enforcement predicates are unchanged by this decision. The `Content-Length` +fast-reject and the streaming running-counter remain where they were; what changes is +that they are now reachable. + +**A body-cap breach is the gateway's own 413.** The proxy-path breach renders as +`CONTENT_TOO_LARGE`, mapped to HTTP 413 in the `INPUT_VALIDATION` category and +carried in the standard RFC 9457 `application/problem+json` envelope. The gRPC path +maps the same event to `RESOURCE_EXHAUSTED` rather than falling through to `UNKNOWN`, +so a body-cap breach is the same semantic outcome on both protocols. + +The status alone is deliberately not the discriminator between the two rejecting +components, because the framework also answers 413. The envelope is: the gateway's +rejection carries `application/problem+json` with a problem type and title, and the +framework's carries no body at all. Any assertion that means to prove the gateway +enforced must therefore assert on the envelope, not only on the status. + +== Consequences + +=== Positive + +* A declared `max_body_bytes` is honoured or the deployment refuses to start. There + is no third state in which it is documented, parsed, and inert. +* The component that rejects an oversize body is the one whose configuration + described the limit, so the rejection is attributable and carries the gateway's + diagnostic envelope rather than a bare framework status line. +* The failure mode is legible at the layer that owns it: a misconfiguration fails at + boot with the offending descriptor named, rather than at request time as a smaller + ceiling than the operator asked for. +* Body-cap semantics are uniform across the proxy and gRPC paths. + +=== Negative + +* The framework floor and the declared caps are coupled: raising a declared cap above + the shipped floor is a two-part change, and doing only half of it is a boot + failure rather than a warning. +* The shipped floor is a product default that most deployments never touch but every + deployment inherits, so it must be chosen conservatively enough to be safe and + generous enough not to make the common case a boot failure. + +=== Risks + +* **Shadowing at equality.** When the framework floor is set exactly equal to the + largest declared cap, an over-cap request is rejected by the framework's + pre-router check before the gateway pipeline is entered, so the operator receives + the bare framework 413 rather than the RFC 9457 envelope. The boot check permits + this configuration because the declared cap is still honoured for every request at + or below it — only the *renderer* of an over-cap rejection changes. A deployment + that wants the gateway's envelope for over-cap requests must set the floor strictly + above its largest declared cap. This is documented in the configuration reference + rather than forbidden, because forbidding it would reject a configuration whose + stated policy is in fact honoured. +* **Chunked requests take a different path.** A request with no `Content-Length` + skips the framework's pre-check entirely, so the gateway's streaming counter is the + enforcing mechanism and the envelope is always rendered. The two transports + therefore differ in *which* component rejects, though not in the effective limit. +* Raising the framework floor does not widen any un-capped ingress path: every proxy + route falls back to a default body size when it declares none, and reserved + BFF paths carry their own independent ceiling. This is a property the + architecture must preserve — a future change that made the framework floor the only + ceiling for some route would silently re-open the risk this ADR closes. + +== Alternatives Considered + +**Leave the framework floor at its default and enforce only in the gateway.** This is +the state this ADR replaces. It requires no configuration derivation and no boot +validation, and it is why the declared cap was unreachable: the gateway's enforcement +sits behind the framework's, so the lower limit wins regardless of what the +configuration says. It fails not because it is complex but because it makes the +effective policy an accident of which limit is smaller. + +**Require the operator to set the framework key by hand, with no validation.** +Cheapest to implement, and it does make the declared cap reachable when the operator +gets it right. It fails on the same axis as the status quo: a deployment that omits +the key or sets it too low is indistinguishable from a correct one until a large +request arrives, so the defect class survives with a manual step added. + +**Raise the framework limit to a fixed safe ceiling unrelated to the declared caps.** +This makes the gateway the rejecting component without any derivation, and is +attractive for its simplicity. It was rejected because the constant is arbitrary: it +is either lower than some deployment's declared cap — reintroducing the original +defect for that deployment — or high enough to be meaningless as a limit, which +removes the framework's own backstop without putting anything in its place. + +The unifying failure is the same in all three: each leaves the effective ceiling +implicit. The decision above makes it explicit and refuses to boot when it cannot be +honoured, which is the only variant in which the documented configuration surface and +the enforced behaviour cannot diverge. + +== References + +* `doc/configuration.adoc` — the `max_body_bytes` reference and the operator-facing + statement of the framework floor, the shadowing consequence and the chunked-request + carve-out. +* `doc/architecture.adoc` — the error contract: the HTTP status and category tables + and the gRPC status mapping. +* ADR-0003 — the immediate-TCP-peer trust gate, for the edge trust boundary this + enforcement sits behind. diff --git a/doc/architecture.adoc b/doc/architecture.adoc index 666697c2..a6376ae4 100644 --- a/doc/architecture.adoc +++ b/doc/architecture.adoc @@ -349,7 +349,7 @@ Design (to be built as a low, framework-agnostic base package): category* (per CUI logging convention) and are metric counters only -- e.g. `REQUEST_FORWARDED`, `TOKEN_REFRESHED`, `CONFIG_LOADED`. * Failure event types carry a category -- e.g. `PATH_NOT_ALLOWED` / `SECURITY_FILTER_VIOLATION` - / `PARAMETER_LIMIT_EXCEEDED` (`INPUT_VALIDATION`), `TOKEN_MISSING` / `TOKEN_INVALID` + / `PARAMETER_LIMIT_EXCEEDED` / `CONTENT_TOO_LARGE` (`INPUT_VALIDATION`), `TOKEN_MISSING` / `TOKEN_INVALID` (`AUTHENTICATION`), `SCOPE_MISSING` / `CSRF_REJECTED` (`AUTHORIZATION`), `UPSTREAM_ERROR` / `UPSTREAM_TIMEOUT` (`UPSTREAM`). * Typed gateway exceptions carry their `EventType`, so the HTTP edge maps @@ -370,11 +370,12 @@ names the category but leaks no internal detail. The consolidated status mapping |=== | Status | Cause | EventCategory -| `400` | Security-filter violation (path/parameter/header pipeline, collection limits, body size) | `INPUT_VALIDATION` +| `400` | Security-filter violation (path/parameter/header pipeline, collection limits) | `INPUT_VALIDATION` | `401` | Missing/invalid bearer token; invalid or expired session on a non-navigation request. Unauthenticated *navigation* requests (those accepting `text/html`) on `session` routes are redirected into the login flow instead -- content negotiation, not configuration (see link:configuration.adoc#_auth[Configuration -- `auth`]). Gateway `401`s carry `WWW-Authenticate` (`Bearer` per RFC 6750 on bearer routes). | `AUTHENTICATION` | `403` | Valid credentials lacking a required scope; failed CSRF origin check | `AUTHORIZATION` | `404` | No route matched (deny-by-default routing); reserved-for-passthrough `Host` on a terminated connection; disabled endpoint (e.g. back-channel logout in cookie mode) | `INPUT_VALIDATION` | `405` | Request method not in the matched route's *effective* `allowed_methods` verb allowlist (global default, or anchor/endpoint replacement); the response carries an `Allow` header listing the permitted set | `INPUT_VALIDATION` +| `413` | Request body beyond a declared size limit -- the per-route `security_filter.max_body_bytes` cap on the proxy path (`CONTENT_TOO_LARGE`), or the edge's `reserved_body_max_bytes` ceiling on a gateway-terminated reserved BFF POST path (`RESERVED_BODY_TOO_LARGE`). Both reject on the declared `Content-Length` before any body is read, and both abort mid-transfer on a chunked or under-declared body once the cap is crossed | `INPUT_VALIDATION` | `429` | Rate limit exceeded -- *reserved*; the feature is out of scope | -- | `502` | Upstream connection failure / invalid upstream response | `UPSTREAM` | `503` | Circuit breaker open -- the upstream was not called (`Retry-After` hints at `reset_ms`) | `UPSTREAM` @@ -395,11 +396,12 @@ rejection it would render as an HTTP status for an HTTP route onto the canonical |=== | gRPC status | code | Gateway rejection (the HTTP status the same cause renders on an HTTP route) -| `INVALID_ARGUMENT` | 3 | `400` -- security-filter violation (path/parameter/header pipeline, collection limits, body size) +| `INVALID_ARGUMENT` | 3 | `400` -- security-filter violation (path/parameter/header pipeline, collection limits) | `UNAUTHENTICATED` | 16 | `401` -- missing/invalid bearer token; invalid or expired session on a non-navigation request | `PERMISSION_DENIED` | 7 | `403` -- valid credentials lacking a required scope; failed CSRF origin check | `NOT_FOUND` | 5 | `404` -- no route matched (deny-by-default routing); reserved-for-passthrough `Host`; disabled endpoint | `UNIMPLEMENTED` | 12 | `405` -- request method not in the matched route's effective `allowed_methods` allowlist +| `RESOURCE_EXHAUSTED` | 8 | `413` -- request body beyond the route's `security_filter.max_body_bytes` cap | `UNAVAILABLE` | 14 | `502` / `503` -- upstream connection failure / invalid upstream response, or circuit breaker open; *also* an HTTP/2 negotiation failure at dispatch (the forced-h2 upstream dial could not establish `h2`) | `DEADLINE_EXCEEDED` | 4 | `504` -- upstream timeout (`connect_timeout_ms` / `read_timeout_ms`) |=== diff --git a/doc/configuration.adoc b/doc/configuration.adoc index dceda153..745b7819 100644 --- a/doc/configuration.adoc +++ b/doc/configuration.adoc @@ -941,10 +941,14 @@ The explicit *not-a-WAF* boundary: allowlists and limits, no attack-signature ma | Baseline preset; the limits below override it. | `max_body_bytes` -| *(gateway-enforced)* -| DoS guard. `cui-http` removed its BODY validation pipeline ("body content validation should - be handled at application layer"), so the gateway enforces this cap itself while reading the - request body; `SecurityConfiguration.maxBodySize` is not wired. +| *(gateway-enforced, under a framework floor)* +| DoS guard, and the *effective* cap for the route: a request whose body exceeds it is rejected + *413*, carrying an `application/problem+json` envelope whenever the request reaches gateway + enforcement (see "Shadowing at the boundary" below -- a `Content-Length` breach of the framework + floor is answered by the framework with a bare `413` instead). `cui-http` removed its BODY validation + pipeline ("body content validation should be handled at application layer"), so the gateway + enforces the cap itself while reading the request body; `SecurityConfiguration.maxBodySize` is + not wired. The cap only takes effect *below the framework floor* described immediately below. | `max_header_count` / `max_query_params` | `maxHeaderCount` / `maxParameterCount` (`RequestCollectionValidator`) @@ -968,6 +972,30 @@ The explicit *not-a-WAF* boundary: allowlists and limits, no attack-signature ma | Optional `Content-Type` allowlist for request bodies. |=== +*The framework body-size floor.* `max_body_bytes` is a cap the gateway applies *on top of* an +underlying HTTP-framework limit, `quarkus.http.limits.max-body-size`. The framework limit is the +floor every declared cap sits under: a route cannot accept more than the floor allows, whatever it +declares. API Sheriff ships the floor at *`64M`* (67108864 bytes) in `application.properties`, +matching the largest `max_body_bytes` in the shipped topology. Without that declaration Vert.x +applies its own 10 MiB default and silently clips every larger declared cap, so the route's own +limit never fires. + +The relationship is *boot-enforced*: a deployment that declares a `max_body_bytes` larger than the +floor fails fast at startup rather than running with a cap that cannot be honoured. Raising a +declared cap therefore means raising `quarkus.http.limits.max-body-size` in the same change. + +*Shadowing at the boundary.* When the floor is set *equal to* the largest declared cap, the +framework's own `Content-Length` pre-check answers a breach of that largest cap with a bare `413` +(`Connection: close`, empty body) *before the gateway's request handling runs* — so the RFC 9457 +`application/problem+json` envelope is rendered only for routes capped strictly *below* the floor. +An operator who wants the gateway's own envelope on the largest-capped route sets the floor *above* +that cap rather than equal to it. The integration stack does exactly this: it overrides the floor to +`128M` so the gateway, not the framework, renders the rejection. + +*Chunked requests.* A chunked request carries no `Content-Length`, so it passes the framework's +pre-check untouched and is capped by the gateway's streaming byte counter instead — it always +receives the gateway's `application/problem+json` envelope, regardless of where the floor sits. + *Whitelisting.* `cui-http` has no path allow-list knob, so URL/parameter whitelisting is enforced at the gateway layer: `allowed_paths` (a request whose validated, normalized path matches no entry is rejected -- the manifest's "WAF via path white-listing"), plus `forward.query_allow` / diff --git a/integration-tests/docker-compose.benchmark.yml b/integration-tests/docker-compose.benchmark.yml index e4a9c69d..58222bd8 100644 --- a/integration-tests/docker-compose.benchmark.yml +++ b/integration-tests/docker-compose.benchmark.yml @@ -9,15 +9,19 @@ services: - "18081:8080" # published only for the host-side readiness probe volumes: - ./nginx/nginx-static.conf:/etc/nginx/conf.d/default.conf:ro + - ./src/main/docker/nginx/body-limits.conf:/etc/nginx/conf.d/00-body-limits.conf:ro networks: - api-sheriff restart: unless-stopped api-sheriff: environment: - # Override the UPSTREAM topology alias (go-httpbin) with the static backend - # for benchmarks. TOPOLOGY_ takes precedence over topology.properties, - # so the mounted config is reused unchanged and only the target is repointed. + # Repoint the UPSTREAM topology alias (go-httpbin) at the static backend for + # benchmarks. This binds to the explicit ${TOPOLOGY_UPSTREAM:-...} placeholder + # written in topology.properties — EnvSecretResolver substitutes only in-file + # placeholders, and there is no TOPOLOGY_ precedence mechanism. The + # mounted config is reused unchanged; unset, the placeholder's default keeps + # the IT stack on go-httpbin. - TOPOLOGY_UPSTREAM=http://nginx-static:8080 depends_on: - nginx-static diff --git a/integration-tests/docker-compose.yml b/integration-tests/docker-compose.yml index 1ef935b8..754de2a3 100644 --- a/integration-tests/docker-compose.yml +++ b/integration-tests/docker-compose.yml @@ -106,6 +106,12 @@ services: # readiness probe and local debugging. go-httpbin: image: ghcr.io/mccutchen/go-httpbin:2.23.1 + # go-httpbin caps request bodies at 1 MiB by default, well below the upload anchor's declared + # 64 MiB max_body_bytes. Without this raise the upstream — not the gateway — would reject + # LargeBodyIT's 11 MiB positive body, turning a test about the framework body-size floor into a + # test about go-httpbin's own default. The value matches the upload anchor's cap exactly, so a + # body the gateway forwards is a body this upstream accepts. + command: ["-max-body-size", "67108864"] ports: - "18080:8080" networks: @@ -188,6 +194,16 @@ services: # override moves it to sheriff.tls.internal-https-port (8444); the front L4-relays a matched # passthrough SNI to the resolved backend and hands every other connection to localhost:8444. - QUARKUS_HTTP_SSL_PORT=8444 + # Raise the framework body-size floor ABOVE the upload anchor's declared 64 MiB + # max_body_bytes for this instance only. application.properties ships the floor at exactly + # 64M, where the Quarkus root handler's Content-Length pre-check answers an oversize request + # with a bare 413 (Connection: close, empty body) before the gateway pipeline runs — so the + # RFC 9457 problem+json envelope would never be rendered for the largest-capped route. Lifting + # the floor to 128M puts LargeBodyIT's 68 MiB negative-case body under the framework limit and + # over the gateway's own cap, which is what makes the gateway (not the framework) the rejecting + # component. BodyLimitActivationWiringTest asserts this override stays strictly above that body + # size; dropping or lowering it silently turns LargeBodyIT into a test of the framework. + - QUARKUS_HTTP_LIMITS_MAX_BODY_SIZE=128M # Certificate paths (runtime override) - QUARKUS_HTTP_SSL_CERTIFICATE_FILES=/app/certificates/localhost.crt - QUARKUS_HTTP_SSL_CERTIFICATE_KEY_FILES=/app/certificates/localhost.key diff --git a/integration-tests/src/main/docker/nginx/body-limits.conf b/integration-tests/src/main/docker/nginx/body-limits.conf new file mode 100644 index 00000000..2e835de9 --- /dev/null +++ b/integration-tests/src/main/docker/nginx/body-limits.conf @@ -0,0 +1,22 @@ +# Body-size and lingering-close settings for the static benchmark backend +# (nginx-static), included into nginx's http context via /etc/nginx/conf.d/. +# +# Contract: this backend must NEVER be the component that rejects or severs a +# benchmark upload. The gateway's per-route max_body_bytes is the only cap under +# measurement — a backend-side rejection would measure a failure path (which is +# faster than a transfer path) and report flatteringly good numbers. +# +# client_max_body_size 0 removes any nginx-side ceiling. Note it is INERT for the +# current `return 200` location: nginx evaluates the directive inside its request-body +# reader, which a return-handled location never invokes. It is declared anyway so the +# contract stays correct if that location ever gains a body-reading handler. +# +# The lingering settings are the operative half. Because the backend answers before the +# transfer completes, nginx drains the remaining body under lingering_close; the defaults +# (5s timeout, 16k) are sized for small bodies and would reset a 50 MB uploadLarge aspect +# mid-transfer. The values below give that aspect room to drain cleanly. +client_max_body_size 0; + +lingering_close always; +lingering_time 120s; +lingering_timeout 30s; diff --git a/integration-tests/src/main/docker/sheriff-config/topology.properties b/integration-tests/src/main/docker/sheriff-config/topology.properties index a3d20640..9da36a99 100644 --- a/integration-tests/src/main/docker/sheriff-config/topology.properties +++ b/integration-tests/src/main/docker/sheriff-config/topology.properties @@ -2,9 +2,15 @@ # # UPSTREAM is the go-httpbin echo backend, reached by the gateway on the internal # compose network. Its /anything/* endpoint echoes the received request as JSON so -# the proxy ITs can assert exactly what was forwarded. The benchmark overlay -# overrides this at runtime with TOPOLOGY_UPSTREAM=http://nginx-static:8080. -UPSTREAM=http://go-httpbin:8080/anything +# the proxy ITs can assert exactly what was forwarded. +# +# The value uses the ADR-0004 Amendment A1 in-file placeholder form: EnvSecretResolver +# substitutes only explicit ${VAR} / ${VAR:-default} placeholders written here — there is +# NO convention-named TOPOLOGY_ environment precedence path, so an env variable +# binds if and only if this file names it. The default keeps the IT stack on go-httpbin +# (it sets no TOPOLOGY_UPSTREAM, and the proxy ITs assert go-httpbin's JSON echo); the +# benchmark overlay sets TOPOLOGY_UPSTREAM=http://nginx-static:8080 to repoint it. +UPSTREAM=${TOPOLOGY_UPSTREAM:-http://go-httpbin:8080/anything} # ASSET_ORIGIN is the secondary static server ('asset-origin', nginx) that backs the # source: upstream asset route (/assets/cdn). The gateway fetches confined assets from diff --git a/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffCookieSessionIT.java b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffCookieSessionIT.java index 99903372..c4c71c51 100644 --- a/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffCookieSessionIT.java +++ b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffCookieSessionIT.java @@ -21,8 +21,6 @@ import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; -import io.restassured.http.Cookie; -import io.restassured.response.Response; import java.util.Base64; import java.util.HashMap; import java.util.Map; @@ -31,6 +29,9 @@ import de.cuioss.sheriff.gateway.integration.BffKeycloakLoginFlow.Session; +import io.restassured.http.Cookie; +import io.restassured.response.Response; + import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; diff --git a/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffCookieStatelessnessIT.java b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffCookieStatelessnessIT.java index baa7c046..fc69d403 100644 --- a/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffCookieStatelessnessIT.java +++ b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffCookieStatelessnessIT.java @@ -20,13 +20,14 @@ import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; -import io.restassured.response.Response; import java.util.Base64; import java.util.HashMap; import java.util.Map; import de.cuioss.sheriff.gateway.integration.BffKeycloakLoginFlow.Session; +import io.restassured.response.Response; + import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; diff --git a/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffKeycloakLoginFlow.java b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffKeycloakLoginFlow.java index 52d61c8d..14d09f97 100644 --- a/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffKeycloakLoginFlow.java +++ b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffKeycloakLoginFlow.java @@ -17,14 +17,15 @@ import static io.restassured.RestAssured.given; -import io.restassured.http.Cookies; -import io.restassured.response.Response; -import io.restassured.specification.RequestSpecification; import java.util.HashMap; import java.util.Map; import java.util.regex.Matcher; import java.util.regex.Pattern; +import io.restassured.http.Cookies; +import io.restassured.response.Response; +import io.restassured.specification.RequestSpecification; + /** * Drives a scripted, browser-less OIDC authorization-code flow against the compose Keycloak * {@code integration} realm and the server-mode BFF gateway, following the {@code 302} chain with a diff --git a/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffSessionMediationIT.java b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffSessionMediationIT.java index 10263014..e760cb59 100644 --- a/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffSessionMediationIT.java +++ b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffSessionMediationIT.java @@ -21,10 +21,10 @@ import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; -import io.restassured.response.Response; - import de.cuioss.sheriff.gateway.integration.BffKeycloakLoginFlow.Session; +import io.restassured.response.Response; + import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; diff --git a/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BodyLimitActivationWiringTest.java b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BodyLimitActivationWiringTest.java new file mode 100644 index 00000000..747f72b6 --- /dev/null +++ b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BodyLimitActivationWiringTest.java @@ -0,0 +1,266 @@ +/* + * Copyright © 2026 CUI-OpenSource-Software (info@cuioss.de) + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package de.cuioss.sheriff.gateway.integration; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.IOException; +import java.io.InputStream; +import java.io.Reader; +import java.nio.file.DirectoryStream; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; +import java.util.Locale; +import java.util.Map; +import java.util.Properties; +import org.yaml.snakeyaml.Yaml; + +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +/** + * Fast, no-Docker surefire guard that the committed deployment descriptors actually + * activate the framework body-size floor the gateway's per-route + * {@code max_body_bytes} caps sit under. + *

    + * This closes a unit-green / integration-red blind spot. {@code ConfigProducer}'s fail-closed + * framework-limit check and both body-cap raise sites are unit-covered and correct in isolation, but + * the floor itself is a property: {@code quarkus.http.limits.max-body-size} in + * {@code application.properties}. Remove it and Vert.x silently reverts to its 10 MiB default, + * clipping every larger declared cap before the gateway pipeline ever runs; raise a declared cap + * past it and the boot check aborts that container. Neither failure is visible to a per-component + * unit test — only to a descriptor assertion like this one, or to the expensive container suite. + *

    + * The coverage is deliberately all committed descriptors, not just the base one: + * the compose stack boots five native gateway instances over four {@code sheriff-config*} gateway + * descriptors ({@code api-sheriff}, {@code api-sheriff-mtls}, {@code api-sheriff-cookie}, + * {@code api-sheriff-cookie-2} and {@code api-sheriff-ws-admission}), so a cap raised in any sibling + * descriptor pushes that instance into a boot abort. The descriptors are discovered by glob rather + * than hard-coded, so a new {@code sheriff-config-*} directory comes under the assertion + * automatically — and a glob that matches fewer than the four present today fails rather than + * passing vacuously. + *

    + * It parses the committed descriptors only (YAML / properties text) and asserts the activation is + * present — it starts no container and reaches no network. The containerised sibling that exercises + * the resulting 413 end-to-end is {@code LargeBodyIT}. + * + * @author API Sheriff Team + * @since 1.0 + */ +class BodyLimitActivationWiringTest { + + /** The module base directory (surefire runs with the module root as the working directory). */ + private static final Path MODULE = Path.of(System.getProperty("user.dir")); + private static final Path DOCKER = MODULE.resolve("src/main/docker"); + private static final Path APPLICATION_PROPERTIES = + MODULE.getParent().resolve("api-sheriff/src/main/resources/application.properties"); + + private static final String FRAMEWORK_LIMIT_KEY = "quarkus.http.limits.max-body-size"; + private static final String CONTAINER_OVERRIDE_KEY = "QUARKUS_HTTP_LIMITS_MAX_BODY_SIZE"; + private static final String DECLARED_CAP_KEY = "max_body_bytes"; + + /** + * The descriptor count committed today. The glob must match at least this many, so an empty or + * mis-rooted glob fails loudly instead of satisfying the per-descriptor loop vacuously. + */ + private static final int COMMITTED_DESCRIPTOR_COUNT = 4; + + /** {@code LargeBodyIT}'s negative-case body size — the container override must exceed it. */ + private static final long NEGATIVE_CASE_BODY_BYTES = 71303168L; + + @Test + @DisplayName("application.properties declares the framework body-size floor") + void applicationPropertiesDeclaresTheFrameworkLimit() throws Exception { + // Arrange + Properties properties = applicationProperties(); + + // Act + String declared = properties.getProperty(FRAMEWORK_LIMIT_KEY); + + // Assert — its absence is the exact defect this plan fixes: Vert.x then applies its 10 MiB + // default and silently clips every larger declared max_body_bytes. + assertNotNull(declared, FRAMEWORK_LIMIT_KEY + + " must be declared in application.properties, otherwise Vert.x applies its 10 MiB default" + + " and clips every larger declared max_body_bytes before the gateway pipeline runs"); + assertTrue(parseMemorySize(declared) > 0, + FRAMEWORK_LIMIT_KEY + " must parse to a positive byte count, was: " + declared); + } + + @Test + @DisplayName("the framework floor covers the largest declared cap in every committed descriptor") + void frameworkLimitCoversEveryCommittedDescriptor() throws Exception { + // Arrange + long floor = parseMemorySize(applicationProperties().getProperty(FRAMEWORK_LIMIT_KEY)); + List descriptors = committedGatewayDescriptors(); + + // Act + Assert — the glob must actually find the committed descriptors before the loop below + // can mean anything. + assertTrue(descriptors.size() >= COMMITTED_DESCRIPTOR_COUNT, + "expected at least " + COMMITTED_DESCRIPTOR_COUNT + + " committed sheriff-config*/gateway.yaml descriptors under " + DOCKER + + ", found " + descriptors.size() + ": " + descriptors); + + // The same strict inequality ConfigProducer's boot check enforces per booted instance — + // caught here, before a native image is ever built. + for (Path descriptor : descriptors) { + long declared = maxDeclaredBodyCap(loadYaml(descriptor)); + assertTrue(declared <= floor, + descriptor + " declares max_body_bytes " + declared + ", which exceeds the framework floor " + + floor + " (" + FRAMEWORK_LIMIT_KEY + + "). That instance would abort its boot on the fail-closed check —" + + " raise the floor or lower the declared cap."); + } + } + + @Test + @DisplayName("the IT container override exceeds LargeBodyIT's negative-case body size") + void containerOverrideExceedsTheNegativeCaseBody() throws Exception { + // Arrange + List environment = environment(composeServices(), "api-sheriff"); + + // Act + String override = environment.stream() + .filter(entry -> entry.startsWith(CONTAINER_OVERRIDE_KEY + "=")) + .map(entry -> entry.substring(CONTAINER_OVERRIDE_KEY.length() + 1)) + .findFirst() + .orElse(null); + + // Assert — this coupling is what keeps LargeBodyIT honest. Dropping or lowering the override + // would let the Quarkus root handler answer the oversize request with a bare 413 before the + // gateway pipeline runs, silently turning the IT into an assertion about the framework + // rather than about the gateway's RFC 9457 envelope. + assertNotNull(override, "the api-sheriff service must set " + CONTAINER_OVERRIDE_KEY + + " so the gateway — not the Quarkus root handler — rejects LargeBodyIT's oversize body"); + long overrideBytes = parseMemorySize(override); + assertTrue(overrideBytes > NEGATIVE_CASE_BODY_BYTES, + CONTAINER_OVERRIDE_KEY + " is " + overrideBytes + " but must be strictly greater than " + + NEGATIVE_CASE_BODY_BYTES + " (LargeBodyIT's negative-case body size), otherwise the" + + " framework pre-check shadows the gateway's own 413 envelope"); + } + + /** + * Every committed gateway descriptor under a {@code sheriff-config} directory, discovered by glob + * so a new instance directory is covered automatically. + * + * @return the descriptor paths, in directory-stream order + * @throws IOException when the docker directory cannot be listed + */ + private static List committedGatewayDescriptors() throws IOException { + List descriptors = new ArrayList<>(); + try (DirectoryStream directories = Files.newDirectoryStream(DOCKER, "sheriff-config*")) { + for (Path directory : directories) { + Path descriptor = directory.resolve("gateway.yaml"); + if (Files.isRegularFile(descriptor)) { + descriptors.add(descriptor); + } + } + } + return descriptors; + } + + /** + * The largest {@code max_body_bytes} declared anywhere in a descriptor. The whole document tree is + * walked rather than only the {@code anchors} block, so a cap declared at any nesting depth — + * including a future route-level one — is covered. + * + * @param node the parsed YAML node + * @return the maximum declared cap in bytes, or {@code 0} when none is declared + */ + private static long maxDeclaredBodyCap(Object node) { + long max = 0L; + if (node instanceof Map map) { + for (Map.Entry entry : map.entrySet()) { + if (DECLARED_CAP_KEY.equals(String.valueOf(entry.getKey())) + && entry.getValue() instanceof Number cap) { + max = Math.max(max, cap.longValue()); + } else { + max = Math.max(max, maxDeclaredBodyCap(entry.getValue())); + } + } + } else if (node instanceof Iterable items) { + for (Object item : items) { + max = Math.max(max, maxDeclaredBodyCap(item)); + } + } + return max; + } + + /** + * Parses a Quarkus {@code MemorySize} literal — a plain byte count, or a number with a binary + * {@code K} / {@code M} / {@code G} suffix (optionally {@code Ki} / {@code Mi} / {@code Gi}). + * + * @param value the declared literal + * @return the value in bytes + */ + private static long parseMemorySize(String value) { + String trimmed = value.trim().toUpperCase(Locale.ROOT); + long multiplier = 1L; + int end = trimmed.length(); + if (trimmed.endsWith("I")) { + end--; + } + if (end > 0) { + multiplier = switch (trimmed.charAt(end - 1)) { + case 'K' -> 1024L; + case 'M' -> 1024L * 1024L; + case 'G' -> 1024L * 1024L * 1024L; + default -> 1L; + }; + } + String digits = multiplier == 1L ? trimmed.substring(0, end) : trimmed.substring(0, end - 1); + return Long.parseLong(digits.trim()) * multiplier; + } + + private static Properties applicationProperties() throws IOException { + Properties properties = new Properties(); + try (Reader reader = Files.newBufferedReader(APPLICATION_PROPERTIES)) { + properties.load(reader); + } + return properties; + } + + @SuppressWarnings("unchecked") + private static Map composeServices() throws IOException { + Map doc = loadYaml(MODULE.resolve("docker-compose.yml")); + Object services = doc.get("services"); + assertInstanceOf(Map.class, services, "docker-compose.yml must declare services"); + return (Map) services; + } + + @SuppressWarnings("unchecked") + private static List environment(Map services, String service) { + Object node = services.get(service); + assertNotNull(node, "docker-compose.yml must declare the '" + service + "' service"); + Map serviceMap = (Map) node; + Object env = serviceMap.get("environment"); + assertInstanceOf(List.class, env, "the '" + service + "' service environment must be a list"); + List entries = ((List) env).stream().map(String::valueOf).toList(); + assertFalse(entries.isEmpty(), "the '" + service + "' service environment must not be empty"); + return entries; + } + + @SuppressWarnings("unchecked") + private static Map loadYaml(Path path) throws IOException { + try (InputStream in = Files.newInputStream(path)) { + return new Yaml().loadAs(in, Map.class); + } + } +} diff --git a/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/LargeBodyIT.java b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/LargeBodyIT.java new file mode 100644 index 00000000..3de9c9b9 --- /dev/null +++ b/integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/LargeBodyIT.java @@ -0,0 +1,272 @@ +/* + * Copyright © 2026 CUI-OpenSource-Software (info@cuioss.de) + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package de.cuioss.sheriff.gateway.integration; + +import static io.restassured.RestAssured.given; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.net.URI; +import java.net.http.HttpClient; +import java.net.http.HttpRequest; +import java.net.http.HttpResponse; +import java.nio.ByteBuffer; +import java.security.SecureRandom; +import java.time.Duration; +import java.util.Arrays; +import java.util.concurrent.Flow; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicLong; +import javax.net.ssl.SSLContext; +import javax.net.ssl.TrustManager; + +import de.cuioss.sheriff.gateway.integration.MtlsHandshakeIT.TrustAllManager; + +import io.restassured.path.json.JsonPath; + +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +/** + * Proves the body-size contract end-to-end across the real framework boundary: a body larger + * than the Vert.x default is accepted and forwarded, and a body larger than the route's declared + * {@code security_filter.max_body_bytes} is rejected by the gateway with its own RFC 9457 + * envelope rather than by the framework with a bare status line. + *

    + * Both cases run against the {@code /upload/small} route, which rides the {@code upload} anchor's + * declared cap of 64 MiB (67108864). Three descriptor facts have to line up for this suite to mean + * what it claims, and each is what a corresponding failure would expose: + *

      + *
    1. {@code quarkus.http.limits.max-body-size} in {@code application.properties} raises the + * framework floor above Vert.x' 10 MiB default. Revert it and the positive case's 11 MiB body + * is clipped by the framework before the gateway pipeline ever runs.
      2. + *
    2. The compose stack's {@code QUARKUS_HTTP_LIMITS_MAX_BODY_SIZE=128M} override on the + * {@code api-sheriff} service lifts that floor above the negative case's 68 MiB body. + * Drop or lower it and the Quarkus root handler answers the oversize request with a bare 413 + * before the gateway runs, silently turning the negative case into an assertion about the + * framework instead of about the gateway's envelope.
      3. + *
    3. go-httpbin's {@code -max-body-size 67108864} lifts the upstream's own 1 MiB default above the + * positive case's body. Drop it and the 11 MiB forward is rejected upstream, not at the edge.
      4. + *
    + * The fast, no-Docker sibling that asserts those descriptor facts directly is + * {@code BodyLimitActivationWiringTest}; this suite is the containerised proof of the behaviour they + * enable. + *

    + * Why the negative case is not driven with REST Assured. It must declare a + * {@code Content-Length} it never actually writes, and it must observe that no body bytes were + * produced. That needs {@code Expect: 100-continue} plus a body publisher whose produced bytes are + * counted — a shape REST Assured does not expose. The JDK {@link HttpClient} does: a publisher + * reporting a {@link HttpRequest.BodyPublisher#contentLength() contentLength} of 68 MiB makes the + * client emit that {@code Content-Length} header, and {@code expectContinue(true)} holds the body back + * until the server invites it. A gateway that rejects on the declared length never sends the invite, + * so the counter stays at {@code 0} — the observable proof the rejection happened before any + * payload crossed the wire, not after 68 MiB were uploaded and discarded. + *

    + * The negative case asserts both halves of the error contract, and neither replaces + * the other. The status assertion locks the mapping under test: a proxy-path body-cap breach renders + * {@code CONTENT_TOO_LARGE} as HTTP {@code 413}, so a regression that moved the event back to a + * 400-mapped constant fails here. The envelope assertions — the {@code application/problem+json} + * content type, the input-validation problem type and title, and the absence of the go-httpbin echo — + * are what discriminate the gateway's rejection from the framework's: the framework's + * own {@code Content-Length} pre-check answers with a bare {@code 413} carrying no envelope, so the + * status alone cannot tell the two apart. + * + * @author API Sheriff Team + * @since 1.0 + */ +class LargeBodyIT extends BaseIntegrationTest { + + /** + * 11 MiB — above Vert.x' 10 MiB default and below the {@code upload} anchor's 64 MiB cap, so it is + * accepted only when the framework floor has actually been raised. + */ + private static final int POSITIVE_BODY_BYTES = 11534336; + + /** + * 68 MiB — above the {@code upload} anchor's 64 MiB cap and below the compose stack's 128 MiB + * framework floor, so the rejection is the gateway's and not the framework's. + */ + private static final long NEGATIVE_BODY_BYTES = 71303168L; + + private static final String UPLOAD_PATH = "/upload/small"; + private static final String PROBLEM_JSON = "application/problem+json"; + private static final String INPUT_VALIDATION_TYPE = "urn:api-sheriff:problem:input-validation"; + + private static HttpClient httpClient; + + @BeforeAll + static void setUpLargeBodyClient() throws Exception { + // Trust-all TLS for the stack's self-signed localhost certificate — the JDK client's + // equivalent of BaseIntegrationTest's RestAssured.useRelaxedHTTPSValidation(). Scoped to this + // black-box IT against a throwaway local certificate; never a production trust decision. + SSLContext sslContext = SSLContext.getInstance("TLS"); + sslContext.init(null, new TrustManager[]{new TrustAllManager()}, new SecureRandom()); + httpClient = HttpClient.newBuilder() + .sslContext(sslContext) + .connectTimeout(Duration.ofSeconds(10)) + .build(); + } + + @Test + @DisplayName("an 11 MiB body under the route cap is accepted and forwarded to the upstream") + void bodyUnderTheRouteCapIsForwarded() { + byte[] payload = new byte[POSITIVE_BODY_BYTES]; + Arrays.fill(payload, (byte) 'a'); + + var response = given() + .contentType("text/plain") + .body(payload) + .when() + .post(UPLOAD_PATH) + .then() + .statusCode(200) + .extract(); + + assertNotNull(response.path("method"), + "an accepted body must reach the go-httpbin upstream — its echo carries the method"); + } + + @Test + @DisplayName("a 68 MiB declared body over the route cap is rejected by the gateway before any payload") + void bodyOverTheRouteCapIsRejectedByTheGateway() throws Exception { + CountingBodyPublisher body = new CountingBodyPublisher(NEGATIVE_BODY_BYTES); + HttpRequest request = HttpRequest.newBuilder(URI.create(uploadUri())) + .header("Content-Type", "text/plain") + .timeout(Duration.ofSeconds(30)) + .expectContinue(true) + .POST(body) + .build(); + + HttpResponse response = httpClient.send(request, HttpResponse.BodyHandlers.ofString()); + + assertEquals(413, response.statusCode(), + "an over-cap request must render CONTENT_TOO_LARGE as HTTP 413"); + + String contentType = response.headers().firstValue("Content-Type").orElse(""); + JsonPath problem = JsonPath.from(response.body()); + assertTrue(contentType.contains(PROBLEM_JSON), + "the gateway must render its own RFC 9457 envelope, Content-Type was: " + contentType); + // Use the typed getString accessor rather than the generic get(): inside String.valueOf(...) + // javac resolves JsonPath's T get(String) against the most specific overload, + // String.valueOf(char[]), inferring T = char[] and failing with a ClassCastException at runtime. + String problemType = problem.getString("type"); + assertTrue(String.valueOf(problemType).contains(INPUT_VALIDATION_TYPE), + "an oversize body is an input-validation rejection, type was: " + problemType); + assertEquals("Input Validation", problem.getString("title")); + assertNull(problem.get("method"), "a rejected request must not reach the go-httpbin upstream"); + assertEquals(0L, body.producedBytes(), + "the rejection must land on the declared Content-Length, before the 100-continue invite —" + + " a non-zero count means the gateway accepted the payload and discarded it afterwards"); + } + + private static String uploadUri() { + String testPort = System.getProperty("test.https.port", "10443"); + return "https://localhost:" + testPort + UPLOAD_PATH; + } + + /** + * A body publisher that declares {@code contentLength} bytes and counts how many it is + * actually asked to produce. Paired with {@code Expect: 100-continue}, the count is the observable + * proof of where the rejection landed: {@code 0} means the server refused on the declared length + * alone and never invited the payload. + */ + private static final class CountingBodyPublisher implements HttpRequest.BodyPublisher { + + private final long declaredLength; + private final AtomicLong produced = new AtomicLong(); + + CountingBodyPublisher(long declaredLength) { + this.declaredLength = declaredLength; + } + + @Override + public long contentLength() { + return declaredLength; + } + + long producedBytes() { + return produced.get(); + } + + @Override + public void subscribe(Flow.Subscriber subscriber) { + subscriber.onSubscribe(new ChunkedSubscription(subscriber, declaredLength, produced)); + } + } + + /** + * Emits zero-filled chunks on demand up to the declared length, tallying every byte handed out. + * The drain loop is guarded so a re-entrant {@code request(n)} from inside {@code onNext} does not + * recurse. + */ + private static final class ChunkedSubscription implements Flow.Subscription { + + private static final int CHUNK_BYTES = 64 * 1024; + + private final Flow.Subscriber subscriber; + private final AtomicLong produced; + private final AtomicLong demand = new AtomicLong(); + private final AtomicBoolean draining = new AtomicBoolean(); + private volatile boolean finished; + private volatile long remaining; + + ChunkedSubscription(Flow.Subscriber subscriber, long length, AtomicLong produced) { + this.subscriber = subscriber; + this.produced = produced; + this.remaining = length; + } + + @Override + public void request(long n) { + if (n <= 0) { + finished = true; + subscriber.onError(new IllegalArgumentException("non-positive subscription request: " + n)); + return; + } + demand.addAndGet(n); + drain(); + } + + @Override + public void cancel() { + finished = true; + } + + private void drain() { + if (!draining.compareAndSet(false, true)) { + return; + } + try { + while (!finished && demand.get() > 0 && remaining > 0) { + demand.decrementAndGet(); + int chunk = (int) Math.min(CHUNK_BYTES, remaining); + remaining -= chunk; + produced.addAndGet(chunk); + subscriber.onNext(ByteBuffer.allocate(chunk)); + } + if (!finished && remaining == 0) { + finished = true; + subscriber.onComplete(); + } + } finally { + draining.set(false); + } + } + } +}