feat(bff): add stateless cookie session variant with AES-GCM sealing - #118
Merged
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cuioss-oliver, your pull request is larger than the review limit of 150000 diff characters
📝 WalkthroughWalkthroughThe pull request adds stateless AES-GCM cookie sessions, key rotation, and runtime wiring through ChangesCookie-mode BFF session flow
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Reviewer Guide 🔍(Review updated until commit ba3edbc)
|
TokenRefreshCoordinator coalesces them via inFlight.putIfAbsent(sessionId, leader) because they share the same sessionId. The second session joins the leader's refresh and receives the leader's refreshed session record and Set-Cookie header, overwriting the second session's cookie with the first session's tokens.sessionId while remaining stable across re-seals, SealedSessionPayload should carry a randomly generated session identifier (minted at login) that is preserved across re-seals.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/runtime/SessionAuthenticationStage.java (1)
130-172: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRender
Map<String, String>Set-Cookie rows only once
PipelineRequest.responseHeaders()is documented as single-valued (Map<String, String>), and the reserved-route render path reads that map before appending special cookie handling. SinceemitSetCookiescurrently callsput(SET_COOKIE_HEADER, ...)for at most one cookie, on a failed-refresh + HTML navigation, both the clearing cookie and the login-challenge cookie land in the sameMapslot; whichever write is last is emitted, and the other is lost. Either support multi-value response-cookie storage/retention or renderSet-Cookiein the single pass where multiple values are already handled explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/runtime/SessionAuthenticationStage.java` around lines 130 - 172, Update emitSetCookies and the unauthenticated refresh-failure flow so multiple Set-Cookie values are retained instead of overwriting the single responseHeaders entry. Ensure the clearing cookie and the login-challenge cookie are both emitted for HTML navigation, using the existing response rendering path that supports multiple cookie values.api-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/BffRuntimeProducer.java (1)
300-304: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssembly diagnostic still says "Server-mode" on the cookie path.
build(...)is now reached for both modes, so a cookie-mode boot logs a message naming the wrong binding — the one line an operator greps to confirm which mode came up.🪵 Report the resolved mode
- LOGGER.debug("Server-mode BFF runtime assembled for origin %s (issuer %s)", gatewayOrigin, issuer); + LOGGER.debug("BFF runtime assembled in %s session mode for origin %s (issuer %s)", + isCookieMode(session) ? SESSION_MODE_COOKIE : SESSION_MODE_SERVER, gatewayOrigin, issuer);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/BffRuntimeProducer.java` around lines 300 - 304, Update the assembly diagnostic in build(...) to report the resolved runtime mode instead of hard-coding “Server-mode.” Use the existing mode value or symbol available in the method, while preserving the gateway origin and issuer details in the LOGGER.debug message.api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/reserved/CallbackEndpoint.java (1)
173-208: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
sessionBinding.bind()'s documentedIllegalStateExceptionis not caught during login completion.
SessionBinding.bind(...)is documented to throwIllegalStateExceptionwhen the binding cannot bind the session — including, for a stateless (cookie-mode) binding, when the sealed representation exceeds the cookie-size budget (a realistic outcome for large ID tokens / claim sets, enforced viaSealedSessionCookieCodec.seal()'sCookieSizeBudgetExceededException).completeLogin(...)is invoked inside thehandle()try block (line 175), but the catch only handlesTokenSheriffException(line 176). A budget-exceeded (or, in server mode, store-at-capacity) session therefore propagates as an unhandledIllegalStateExceptionout ofhandle()instead of a cleanCallbackOutcomeerror.Catch it alongside the exchange failure and map it to an explicit error status.
🛡️ Suggested fix
try { AuthorizationCodeFlow.AuthenticationResult result = codeExchange.exchange(pending.flowContext(), params); return completeLogin(result, pending, now); } catch (TokenSheriffException engineFailure) { LOGGER.debug(engineFailure, "OIDC callback code exchange / token validation failed"); return CallbackOutcome.error(BAD_REQUEST); + } catch (IllegalStateException bindingFailure) { + // A stateless binding whose sealed session exceeds the cookie-size budget, or a + // stateful binding at capacity — either way, the login cannot be completed. + LOGGER.warn(bindingFailure, "OIDC callback could not bind the new session"); + return CallbackOutcome.error(SOME_APPROPRIATE_STATUS); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/reserved/CallbackEndpoint.java` around lines 173 - 208, Update the handle() exception handling around codeExchange.exchange() and completeLogin() to also catch IllegalStateException from sessionBinding.bind(). Map it to an explicit CallbackOutcome.error status, preserving the existing TokenSheriffException handling and clean callback response behavior.api-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/validation/ConfigValidator.java (1)
737-762: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
oidc.session.modeis matched case-sensitively inConfigValidatorbut case-insensitively inGatewayEdgeRoute. Both read the same config field to decide "is this cookie mode?", but disagree on case handling — the shared root cause is that neither side normalizes the value before comparing.
api-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/validation/ConfigValidator.java#L737-L762:validateSessionModeuses"cookie".equals(mode)/"server".equals(mode); a value likeCookiematches neither branch, so it silently skips both the cookie-key companion rule and the server-store rule with no boot error.api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRoute.java#L1102-L1118:cookieHeaderConfigurationForusesSESSION_MODE_COOKIE::equalsIgnoreCase, so the sameCookievalue IS treated as active cookie-mode here, relaxing the pre-route Cookie header cap even though the runtime-selectedSessionBinding(built via case-sensitive matching elsewhere, perBffRuntimeProducer) is unlikely to actually be cookie-mode.Normalize the comparison consistently — either lower-case the value once at config-load time, or make both sites use the same (ideally
equalsIgnoreCase) comparison — so a case variation can't silently produce a validated-but-wrong runtime mode.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/validation/ConfigValidator.java` around lines 737 - 762, Normalize session mode comparisons consistently across ConfigValidator.validateSessionMode and GatewayEdgeRoute.cookieHeaderConfigurationFor, preferably by using the same case-insensitive comparison or lowercasing once during configuration loading. Update both affected sites—api-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/validation/ConfigValidator.java:737-762 and api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRoute.java:1102-1118—so case variations trigger the same validation and runtime behavior.api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRoute.java (1)
417-491: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClose or drain the keep-alive connection after oversized reserved-body 413s.
On both the pre-read
Content-Lengthreject and the mid-stream ceiling reject,rejectOversizedReservedBody()callsrenderProblem(...)→response.end(body)while the request body remains unconsumed. Vert.x does not automatically close the connection here, so reused HTTP/1.1 keep-alive connections can be left with unread bytes pending, which can desync subsequent requests or pin the connection until idle timeout. Addconnection.close()or drain the remaining request stream before returning the 413.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRoute.java` around lines 417 - 491, Update rejectOversizedReservedBody() to close the request connection or drain the remaining request stream before completing the 413 response, covering both declared-content-length and streamed-body rejection paths. Ensure the response still renders normally while preventing unread keep-alive bytes from being reused or left pending.
🧹 Nitpick comments (3)
api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/CookieKeyMaterial.java (1)
55-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale
@linktarget —codec()takes 3 args, not 2.
{@link#codec(String, Duration)}doesn't match the actualcodec(String, Duration, int)signature (Line 146), so this reference won't resolve.📝 Proposed fix
- * through {`@link` `#codec`(String, Duration)} and {`@link` `#identitySalt`()}, both of which consume it + * through {`@link` `#codec`(String, Duration, int)} and {`@link` `#identitySalt`()}, both of which consume it🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/CookieKeyMaterial.java` around lines 55 - 57, Correct the Javadoc link in CookieKeyMaterial to reference the three-argument codec(String, Duration, int) method signature, while leaving the surrounding documentation unchanged.api-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/cookie/CookieKeyMaterialTest.java (1)
213-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't actually assert a 256-bit key.
identitySalt()is always a 32-byte SHA-256 digest regardless of the underlying key length, soassertEquals(32, salt.length)would pass even ifgenerateKey()produced, say, a 128-bit key. The@DisplayNameclaims this verifies "a 256-bit key, so the codec it builds is AES-256-GCM," but nothing here actually checks the key size.Consider asserting on the generated key material directly (e.g., expose a package-private accessor, or assert via a round trip that specifically depends on 256-bit key length) rather than inferring it from the SHA-256 salt length.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/cookie/CookieKeyMaterialTest.java` around lines 213 - 221, Update shouldGenerateA256BitKey in CookieKeyMaterialTest so it verifies the generated encryption key’s actual 256-bit length rather than identitySalt().length, which is always the SHA-256 digest size. Expose or reuse a package-private accessor for the underlying key material, then assert its length is 32 bytes while preserving the separate per-boot salt uniqueness assertion.api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/CookieSessionBinding.java (1)
191-196: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRollover log fires on every resolve, not once per rollover.
markForReseallogsCOOKIE_RESEALED_WITH_CURRENT_KEYat INFO every timeresolve()unseals a still-previous-key cookie — i.e., on every request for that session until the nextbind/persistwrite actually re-seals it. That contradicts the class doc's "recorded once" framing and can flood logs at INFO level for the whole rotation window on a busy, long-TTL deployment; it also announces a completed "re-sealed" event before any write has actually re-sealed anything.Consider gating the log to the point where the re-seal write actually happens (inside
seal(), informed by whether the read that produced this payload came from the previous key), or downgrading repeat detections to DEBUG and reserving INFO for the completed write.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/CookieSessionBinding.java` around lines 191 - 196, Move the COOKIE_RESEALED_WITH_CURRENT_KEY INFO log out of markForReseal and gate it at the actual re-seal write in seal(), using the state that indicates the payload was read with the previous key. Ensure INFO is emitted only after a successful re-sealing write, while repeated resolve() detections before persistence do not produce the rollover log.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/SealedSessionPayload.java`:
- Around line 104-125: Update SealedSessionPayload.decode to catch
DateTimeException alongside IllegalArgumentException around payload
reconstruction, returning Optional.empty() for epoch seconds outside Instant’s
supported bounds while preserving the existing handling for other malformed
authenticated payloads.
In
`@api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/reserved/BackchannelLogoutEndpoint.java`:
- Around line 102-115: Change the capability-gate logging in receive so
unsupported IdP destruction no longer emits an attacker-triggerable WARN for
every request. Downgrade LOGGER.warn using
BffLogMessages.WARN.COOKIE_BACKCHANNEL_DISABLED to an appropriate lower-level
log, or replace the per-request warning with a suitable counter metric, while
preserving the existing NOT_FOUND response and fail-closed behavior.
In
`@api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/session/SessionRecord.java`:
- Around line 36-40: Update the SessionRecord session identity documentation
around sessionId() and the corresponding cookie-mode description near the later
referenced section: state that server mode uses the opaque store key as the
clear session-cookie value, while cookie mode uses the generated opaque ID from
newSessionId(), sealed inside the cookie rather than derived. Preserve the
existing identity and redaction semantics.
---
Outside diff comments:
In
`@api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/reserved/CallbackEndpoint.java`:
- Around line 173-208: Update the handle() exception handling around
codeExchange.exchange() and completeLogin() to also catch IllegalStateException
from sessionBinding.bind(). Map it to an explicit CallbackOutcome.error status,
preserving the existing TokenSheriffException handling and clean callback
response behavior.
In
`@api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/runtime/SessionAuthenticationStage.java`:
- Around line 130-172: Update emitSetCookies and the unauthenticated
refresh-failure flow so multiple Set-Cookie values are retained instead of
overwriting the single responseHeaders entry. Ensure the clearing cookie and the
login-challenge cookie are both emitted for HTML navigation, using the existing
response rendering path that supports multiple cookie values.
In
`@api-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/validation/ConfigValidator.java`:
- Around line 737-762: Normalize session mode comparisons consistently across
ConfigValidator.validateSessionMode and
GatewayEdgeRoute.cookieHeaderConfigurationFor, preferably by using the same
case-insensitive comparison or lowercasing once during configuration loading.
Update both affected
sites—api-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/validation/ConfigValidator.java:737-762
and
api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRoute.java:1102-1118—so
case variations trigger the same validation and runtime behavior.
In
`@api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRoute.java`:
- Around line 417-491: Update rejectOversizedReservedBody() to close the request
connection or drain the remaining request stream before completing the 413
response, covering both declared-content-length and streamed-body rejection
paths. Ensure the response still renders normally while preventing unread
keep-alive bytes from being reused or left pending.
In
`@api-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/BffRuntimeProducer.java`:
- Around line 300-304: Update the assembly diagnostic in build(...) to report
the resolved runtime mode instead of hard-coding “Server-mode.” Use the existing
mode value or symbol available in the method, while preserving the gateway
origin and issuer details in the LOGGER.debug message.
---
Nitpick comments:
In
`@api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/CookieKeyMaterial.java`:
- Around line 55-57: Correct the Javadoc link in CookieKeyMaterial to reference
the three-argument codec(String, Duration, int) method signature, while leaving
the surrounding documentation unchanged.
In
`@api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/CookieSessionBinding.java`:
- Around line 191-196: Move the COOKIE_RESEALED_WITH_CURRENT_KEY INFO log out of
markForReseal and gate it at the actual re-seal write in seal(), using the state
that indicates the payload was read with the previous key. Ensure INFO is
emitted only after a successful re-sealing write, while repeated resolve()
detections before persistence do not produce the rollover log.
In
`@api-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/cookie/CookieKeyMaterialTest.java`:
- Around line 213-221: Update shouldGenerateA256BitKey in CookieKeyMaterialTest
so it verifies the generated encryption key’s actual 256-bit length rather than
identitySalt().length, which is always the SHA-256 digest size. Expose or reuse
a package-private accessor for the underlying key material, then assert its
length is 32 bytes while preserving the separate per-boot salt uniqueness
assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 185f012d-219e-4993-a8ab-bce6c5389c6e
📒 Files selected for processing (65)
api-sheriff/pom.xmlapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/ApiSheriffLogMessages.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/BffLogMessages.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/CookieKeyMaterial.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/CookieSessionBinding.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/SealedSessionCookieCodec.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/SealedSessionPayload.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/package-info.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/logout/BackchannelLogoutReceiver.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/refresh/TokenRefreshCoordinator.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/reserved/BackchannelLogoutEndpoint.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/reserved/CallbackEndpoint.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/reserved/LoginInitiationEndpoint.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/reserved/LogoutEndpoint.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/reserved/UserInfoEndpoint.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/runtime/BffRuntime.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/runtime/SessionAuthenticationStage.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/runtime/package-info.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/session/ServerSessionBinding.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/session/SessionBinding.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/session/SessionRecord.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/session/SessionStore.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/session/package-info.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/model/OidcConfig.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/validation/ConfigValidator.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/EdgeHardeningOptions.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRoute.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/events/EventType.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/pipeline/BasicChecksStage.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/BffRuntimeProducer.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/auth/AuthenticationStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/cookie/CookieKeyMaterialTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/cookie/CookieSessionBindingTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/cookie/SealedSessionCookieCodecTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/cookie/SealedSessionPayloadTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/refresh/TokenRefreshCoordinatorTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/reserved/BackchannelLogoutEndpointTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/reserved/CallbackEndpointTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/reserved/LoginInitiationEndpointTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/reserved/LogoutEndpointTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/reserved/UserInfoEndpointTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/runtime/SessionAuthenticationStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/session/ServerSessionBindingTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/validation/ConfigValidatorTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/EdgeHardeningTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRouteBffWiringTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/ReservedBodyCeilingTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/events/EventTypeTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/BasicChecksStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/BffRuntimeProducerTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/CookieModeBootTest.javaapi-sheriff/src/test/resources/config/cookieboot/endpoints/bff-cookie.yamlapi-sheriff/src/test/resources/config/cookieboot/gateway.yamlapi-sheriff/src/test/resources/config/cookieboot/topology.propertiesdoc/LogMessages.adocdoc/configuration.adocintegration-tests/docker-compose.ymlintegration-tests/scripts/dump-keycloak-logs.shintegration-tests/scripts/start-integration-container.shintegration-tests/src/main/docker/sheriff-config-cookie/gateway.yamlintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffCookieActivationWiringTest.javaintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffCookieBackchannelDisabledIT.javaintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffCookieSessionIT.javaintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffCookieStatelessnessIT.javaintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffKeycloakLoginFlow.java
Collaborator
Author
Triage dispositionsIn reply to comment_id:
|
Spec D7. Replace every direct SessionStore binding in the BFF foundation with one mode-neutral seam a stateless variant can also implement, so login, CSRF, step-up, scope enforcement and logout orchestration stay single-sourced across both session.mode values. - SessionBinding: bind / resolve / persist / destroy plus the two IdP-driven destruction forms and their SUPPORTED/UNSUPPORTED capability flag. The contract names no store and no opaque id; BoundSession carries the session plus the Set-Cookie headers the caller emits. - ServerSessionBinding: the server-mode adapter over the unchanged SessionStore and SessionCookieCodec, behaviour-preserving by construction and reporting IdpDestruction.SUPPORTED. - Migrate all seven consumers (SessionAuthenticationStage, TokenRefreshCoordinator, CallbackEndpoint, LogoutEndpoint, UserInfoEndpoint, LoginInitiationEndpoint, BackchannelLogoutReceiver) plus the BffRuntimeProducer assembly site in one clean break — no shim, no compatibility layer, per the pre-1.0 rules and this plan's breaking compatibility setting. - Single identity model: every binding populates SessionRecord.sessionId(), so TokenRefreshCoordinator keeps single-flight keyed on that component and the seam gains no identity accessor. The coordinator now threads the request Cookie header so its re-resolve-and-re-check under single-flight exclusion stays mode-neutral, and RefreshOutcome carries the re-bind's Set-Cookie headers, which the stage emits on the response. - Javadoc corrected where it had become factually wrong: runtime/package-info no longer claims the stage resolves an opaque session id, SessionRecord states the seam's identity model, and SessionStore is narrowed to the server-mode implementation detail behind the seam. Tests: new ServerSessionBindingTest pins the adapter's preservation of every landed store semantic (upsert-on-persist with no destroy window, lazy-TTL eviction on resolve, O(1) sid/sub destruction). The nine compile-breaking consumer test classes are migrated to SessionBinding doubles with every landed assertion intact. Co-Authored-By: Claude <noreply@anthropic.com>
Spec D1. Seal the mediated token set into an authenticated encrypted cookie and read it back per request, so session.mode=cookie holds no server-side state. - SealedSessionPayload: the sealed plaintext — token material, identity/session claims, and the absolute login instant that anchors the server-enforced TTL. Encoding is compact, explicit and dependency-free; every credential is redacted from toString(). - SealedSessionCookieCodec: version(1B) || key-id(1B) || nonce(12B) || ciphertext || tag(16B), base64url without padding. A fresh 96-bit SecureRandom nonce per seal — never derived, never counter-based, because GCM nonce reuse under one key is catastrophic. Version, key id and cookie name are bound into the GCM associated data, so a value cannot be replayed under a different format version, key generation, or cookie name. unseal selects the key deterministically by key id (never a try-every-key decrypt) and returns "no session" for any tag failure, malformed length, unknown version, or unknown key id — never a 500. A value over the ~4 KB budget fails the seal with a checked outcome and a logged warning rather than truncating silently. Max-Age carries the remaining absolute lifetime, so a re-seal never extends the session. - CookieSessionBinding: the seam implementation over the codec. It enforces the absolute TTL server-side from the sealed login instant, so an expired cookie a browser still holds is refused; reports IdpDestruction.UNSUPPORTED because a stateless gateway has no index to destroy another browser's session through; and populates SessionRecord.sessionId() with a salted digest over the login instant and sub — stable for the session's life, used only as the in-instance single-flight key, and never emitted to the browser. - BffRuntimeProducer: isServerModeBff becomes the mode-aware isBffMode, so session.mode=cookie activates the runtime. Only the binding differs between modes; every other collaborator is identical. The stale comment claiming a cookie-mode gateway never touches the confidential-client engine is corrected. Cookie mode fails closed at boot on a missing or non-AES-256 sealing key. - Six LogRecord constants (INFO 15-17, WARN 113-115) carrying only bounded, non-sensitive dispositions, documented in doc/LogMessages.adoc as the project logging standard requires. No new Maven dependency: the sealing uses the JDK's own javax.crypto AES-GCM. Tests: 47 new cases across the payload, codec and binding — round-trip fidelity, a distinct value per seal, tamper rejection with a flipped byte in each of ciphertext / nonce / tag, cookie-name and version and key-id mismatch rejection, oversize refusal, server-side TTL expiry, remaining-lifetime Max-Age on re-seal, and the absence of key or token material from every emitted header. The BffRuntimeProducer inert-for-cookie-mode test is migrated to the new specification, where cookie mode is an active mode. Co-Authored-By: Claude <noreply@anthropic.com>
Drive the back-channel logout gate off the SessionBinding sid/sub destruction capability rather than a mode string: when idpDestruction() reports UNSUPPORTED the endpoint answers 404 before parsing the form body, so the logout_token is never read and the receiver is never reached. Each gated request records the catalogued WARN COOKIE_BACKCHANNEL_DISABLED with a bounded, non-sensitive reason and no token material. Server-mode behaviour (200 with the destroyed count, 400 on a missing or rejected token) is unchanged, and the reserved path stays registered in both modes so it returns a deliberate 404 instead of falling through to the proxy route table. The BffRuntime dispatch arm already renders every back-channel outcome uncacheable, so the 404 is served Cache-Control: no-store like the 200/400 outcomes.
…rtup guard
Add the config/cookieboot fixture (gateway.yaml, topology.properties, and one
require: session endpoint) mirroring the landed config/testboot layout, plus
CookieModeBootTest: a @QuarkusTest whose profile points sheriff.config.dir at
the fixture and asserts the gateway boots active in cookie mode with NO
encryption_key configured — the generate-on-startup key mode — and that the
resulting binding reports the UNSUPPORTED sid/sub capability, observed at the
runtime surface as the back-channel path answering an uncacheable 404.
The fixture carries a token_validation block because an active BFF mode
resolves the shared @GatewayValidator TokenValidator at boot, and an
oidc.client_secret as a bare ${VAR} because the D4 secrets rule refuses a
literal and the confidential-client engine refuses a blank secret; the surefire
environment supplies that throwaway value. The test starts no container and
reaches no network — OIDC discovery is lazy — so it runs in the ordinary
surefire phase, before the Docker suite.
…wiring guard
Add sheriff-config-cookie/gateway.yaml mirroring the sheriff-config-mtls overlay
pattern (session.mode: cookie, encryption_key as a bare ${SESSION_ENCRYPTION_KEY}
reference, no store) and exactly one additive api-sheriff-cookie compose service
following the api-sheriff-mtls precedent: same image, shared sheriff-config mount
plus the cookie gateway.yaml overlay, its own port 10445, and the session-key
environment variable. No existing service, network, or volume definition is
touched.
Add BffCookieActivationWiringTest, the fast no-Docker surefire guard that parses
the committed descriptors and asserts the variant is actually switched on: cookie
session mode with no store, the sealing key supplied by env reference rather than
generated per boot, the back-channel path still registered so its capability gate
answers 404 instead of falling through, and the compose service publishing 10445
with the overlay mounted.
Deliverable 8 is incomplete: the three Docker-backed cookie IT suites are blocked
on two descriptor-level conflicts raised for a planning decision.
…o end Add the Variant 3 cookie-mode integration suites and the deployment wiring they need. BffCookieSessionIT drives the login, steady-state mediation, logout and tamper round trips against the dedicated cookie-mode gateway, asserting the __Host- prefixed Secure/HttpOnly/SameSite=Lax cookie and that its value carries no readable segment of the live mediated token. BffCookieStatelessnessIT lands the statelessness proof literally rather than by inference: a session sealed by api-sheriff-cookie (10445) is replayed against a second, separate gateway process, api-sheriff-cookie-2 (10446), that shares nothing with the first but the AES sealing key -- no session store, no volume, no sticky routing. It also pins that the peer refuses a cookie it cannot unseal, so being served is evidence about the key rather than about laxness. BffCookieBackchannelDisabledIT pins the capability gate on both bindings side by side: 404 no-store in cookie mode, the unchanged 400 contract in server mode. Two operator-approved widenings of the plan's write boundary, both additive: - docker-compose.yml gains a SECOND cookie-mode service, api-sheriff-cookie-2, a literal clone of api-sheriff-cookie on 10446/19003 using the same image, the same sheriff-config-cookie overlay and a byte-identical SESSION_ENCRYPTION_KEY. No new network, no new volume, no edit to any existing service, no change to the compose networking. - scripts/start-integration-container.sh gains the missing readiness waits for both cookie instances' published management ports (19002 and 19003), mirroring the existing 19001 mTLS block. This makes the compose comment true rather than deleting its claim, and stops the ITs racing either instance. Additive only; the script is not restructured. BffCookieActivationWiringTest is the fast no-Docker guard that the committed descriptors actually activate the variant, extended here to assert the peer instance exists, publishes 10446, mounts the same overlay, and carries the identical sealing key -- the descriptor precondition the statelessness proof rests on.
The ReservedHttpRequest and ReservedHttpResponse record headers carry an unusual component layout -- a line break between each @nullable annotation and the component it annotates. That layout is what the formatter inherited from de.cuioss:cui-java-parent produces, and it reads as an accident that invites a well-meant hand-reflow, which the next format-check silently undoes. Name it instead: a short comment above each of the two record declarations records that the layout is formatter-produced, that it is accepted rather than fought because this project declares no formatter plugin of its own and adding one is out of scope, and that the header must therefore not be hand-reflowed. The two records are peers of the same structure, so both are annotated in the same change. Comments only -- no code, annotation, or component-ordering change, and no pom.xml or .editorconfig modification.
The pre-commit quality gate rewrites sources in place. Land what it produced across this plan's files so the tree matches the gate's own output and the next run is a no-op: - import-block whitespace normalized to the inherited convention - test methods that declared a single checked exception now declare throws Exception, per the inherited JUnit recipe - one constant-first equals() comparison No behavioural change; the gate passes clean on the result.
… config key
Cookie mode could not serve a single authenticated request: every request
carrying a live sealed session cookie was rejected 400 at the edge before any
BFF logic ran. Two independent constants encoded the same limit and had drifted
apart -- SecurityConfiguration.defaults().maxHeaderValueLength (2048), hard-coded
into the pre-route BasicChecksStage by GatewayEdgeRoute, versus
SealedSessionCookieCodec.MAX_COOKIE_VALUE_BYTES (4096), enforced at seal time.
CHOSEN DIRECTION (operator-selected, overriding the ranked options recorded on
TASK-016): remove the root cause rather than the symptom. The task recommended a
blanket Cookie-header exemption from the length check; that treats the symptom
and leaves the two contradicting constants in place. Instead ONE declared number
now drives BOTH ends of the round trip, so the contradiction is unrepresentable:
* New config key oidc.session.max_cookie_size (default 4096), validated in
ConfigValidator with the same rigour as its sibling session keys -- floor 40
(the encoded length of a structurally minimal sealed value, below which no
cookie could ever be emitted) and ceiling 8192 (kept well below the 16 KiB
inbound header-block limit, so a configured value can never exceed what the
transport carries).
* SealedSessionCookieCodec's seal-time budget is that configured value.
MAX_COOKIE_VALUE_BYTES is REMOVED, not deprecated or kept as a fallback --
leaving a second independent constant behind is exactly the defect.
* The pre-route cap derives from the same value (budget + 512 bytes headroom
for the cookie name and co-resident cookies).
THE LIFT IS SCOPED TWICE -- the narrowest relaxation that works:
* By MODE: GatewayEdgeRoute no longer hard-codes SecurityConfiguration
.defaults() for the pre-route stage; it derives the Cookie policy from
gatewayConfig + bffRuntime, following the pattern already used a few lines
above for forwarded()/oidc()/tls()/securityHeaders(). A bearer-only or
server-mode gateway keeps 2048 on every header, unchanged.
* By HEADER: within a cookie-mode gateway the raised cap applies to Cookie and
Set-Cookie values ONLY. BasicChecksStage.validateHeaders already had the
header name in hand at the loop head -- that is the seam. Every other header
keeps 2048, and every non-length validator (null-byte, control-character,
injection-pattern) still applies to the cookie value.
RECORDED CONSTRAINT: per-anchor scoping is unreachable for this stage. It runs
BEFORE route selection and GatewayEdgeRoute is one bean holding the whole route
table, so no anchor exists at that point. The key lives on the session block but
enforcement is necessarily gateway-wide. Stated in the code and in the reference
docs so the key's placement is not mistaken for per-anchor enforcement.
This remains a deliberate relaxation of an inbound hardening control on a
security gateway. It is justified in code, held as narrow as the two scopings
allow, and nothing beyond them is widened.
Also in this change:
* doc/configuration.adoc gains the reference-layer row for the new key
(default, bounds, the cookie-mode-only + Cookie-header-only scoping, and the
gateway-wide-enforcement caveat). A config key never merges undocumented.
* dump-keycloak-logs.sh's failure-path app-log dump now names both cookie
instances, so a CI-only recurrence is diagnosable from the uploaded artifact
instead of requiring a local repro.
* BffCookieSessionIT.sealedCookieValueCarriesNoReadableToken: a latent test
defect the 400 had masked. Passing response.path(...) straight into
String.valueOf() let the generic return type infer to char[], so the
compiler inserted a cast the JSON-path proxy cannot satisfy. Bound to Object
first, exactly as the sibling assertions in the same class already do.
COVERAGE pinning the new boundary at module-test level: a Cookie header at the
configured budget passes; one above the raised cap is rejected; an oversized
NON-Cookie header still yields 400 on a cookie-mode gateway; and a server /
bearer-only gateway still rejects an oversized Cookie header at 2048 (proving
the lift did not leak across modes). Plus the ConfigValidator floor/ceiling
bounds, on and outside the range.
Verified: quality gate green, full verify green, and the full Docker integration
profile green against a freshly rebuilt native image -- 75/75 ITs pass, with
BffCookieSessionIT (5), BffCookieStatelessnessIT (3) and
BffCookieBackchannelDisabledIT (2) all green and no pre-existing suite regressed.
…e-crypto-core Inline the SessionCookieCodec construction into the server-mode branch of BffRuntimeProducer. It was built unconditionally ahead of the mode-selection ternary but consumed only in the server-mode branch, so every cookie-mode boot constructed and discarded it. Co-Authored-By: Claude <noreply@anthropic.com>
Deliberate, operator-approved widening of this plan's scope onto a
PRE-EXISTING PLAN-06 defect surfaced by the finalize security audit.
GatewayEdgeRoute#readReservedBodyThenDispatch buffered the FULL request
body of the two reserved, PRE-AUTHENTICATION POST paths (the OIDC
response_mode=form_post callback and the back-channel logout receiver)
under a 20-second wall-clock deadline and NO byte-size cap. That read
happens before basicChecksStage / thoroughChecksStage run, so the
SecurityConfiguration.maxBodySize cap that bounds ordinary proxied routes
never applied to it, and EdgeHardeningOptions bounded only a single 16 KiB
chunk rather than the cumulative body. On a 512M container that was an
unauthenticated memory-exhaustion DoS vector.
The read is now bounded by EdgeHardeningOptions.reservedBodyMaxBytes(),
enforced twice (defence in depth):
a) a Content-Length pre-check that refuses before a single body byte is
buffered, and
b) a streaming cumulative byte counter that aborts mid-read, closing the
bypass a chunked / Content-Length-absent / lying-Content-Length body
would otherwise have.
The ceiling is DEFINED AS EdgeHardeningOptions.MAX_HEADER_SIZE_BYTES
(16 KiB) rather than restated as a second literal, so the gateway's single
inbound-unit bound cannot drift into two contradicting constants — the
exact defect class this plan already fixed once for the cookie budget. Both
payloads (a urlencoded code/state pair, a compact logout_token JWT) sit an
order of magnitude below it, and the worst case at the 2048 admission cap
is ~32 MiB.
Either breach is rejected 413 (EventType.RESERVED_BODY_TOO_LARGE, the
honest status for an over-large payload) through the surrounding RFC 9457
problem+json contract — never a 500, and never by silently truncating the
body into the handler. The new WARN ApiSheriff-109 records the ceiling and
a fixed disposition only, never the offending body, and is documented in
doc/LogMessages.adoc.
The 20-second deadline, the reserved-path dispatch shape, and the pipeline
stages are untouched.
Coverage: ReservedBodyCeilingTest drives a live Vert.x server and pins a
body at the ceiling (accepted), one byte over (413), an over-ceiling
declared Content-Length refused inside a bounded wait far below the body
deadline (proving no buffering), and an over-ceiling chunked body with no
declared length at all (proving the streaming counter). EdgeHardeningTest
pins the derivation itself; EventTypeTest pins the new error-contract row.
Also carries a whitespace-only reflow of BffRuntimeProducer emitted by the
pre-commit formatter.
Long.parseLong could succeed while Instant.ofEpochSecond threw DateTimeException, which is not an IllegalArgumentException and so escaped SealedSessionPayload.decode's catch and unseal() as an unhandled request error. Widen the catch and cover both the loginInstant and authTime fields with a regression test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XAAt7guDZEDA4S6nUTHcLW
The gate fired a catalogued WARN before any signature or body verification on a reserved, unauthenticated path, so any caller who could reach the gateway could drive an unbounded WARN flood with no logout_token, session, or credential. Every sibling malformed-input path already logs DEBUG; match them and drop the now-unreferenced ApiSheriff-115 LogRecord and its catalogue entry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XAAt7guDZEDA4S6nUTHcLW
emitSetCookies wrote into the single-valued response-header map and took only findFirst(), so on the failed-refresh HTML navigation path the clearing cookie was overwritten by the login-challenge cookie and a revoked session cookie was left in the browser. Set-Cookie is legitimately multi-valued (RFC 6265 3), so PipelineRequest now carries a dedicated Set-Cookie accumulator that the edge renders as distinct header lines on every terminal path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XAAt7guDZEDA4S6nUTHcLW
SessionBinding.bind is documented to throw IllegalStateException when it cannot hold the session -- for cookie mode whenever the sealed value exceeds the cookie-size budget, a realistic outcome for a large ID token. handle() caught only TokenSheriffException, so that escaped as an unhandled exception. Also widen the auth_time claim parse, which had the same DateTimeException gap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XAAt7guDZEDA4S6nUTHcLW
ConfigValidator compared the mode case-sensitively while the edge cap, the runtime binding selection and the readiness check each compared it case-insensitively against their own private constant. A value like 'Cookie' therefore skipped both boot companion rules while still relaxing the pre-route Cookie header-value cap. OidcConfig.Session now canonicalizes the value at construction and owns the single isCookieMode/isServerMode predicate all four sites read; the four duplicated constants are gone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XAAt7guDZEDA4S6nUTHcLW
Both 413 paths ended the response with the request body unconsumed, leaving pending bytes on a reused HTTP/1.1 keep-alive connection -- which desyncs the next request framed on it, or lets an attacker pin connections by repeatedly tripping the ceiling, undercutting the reserved-body DoS guard. The response now advertises Connection: close and the connection is closed once written. Also reattach the dangling Javadoc block to securityConfigurationFor (java:S8491). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XAAt7guDZEDA4S6nUTHcLW
Both catch parameters in SealedSessionCookieCodec.unseal were declared but never referenced (java:S7467). The rejection class each branch covers now lives in an adjacent comment instead of an unused binding. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XAAt7guDZEDA4S6nUTHcLW
Four statements were wrong about cookie mode: the session-identity docs implied one model for both bindings (cookie mode actually discards the minted id and derives its own), the assembly diagnostic hard-coded 'Server-mode' on a path now reached by both modes, a Javadoc link named a signature that no longer exists, and the rotation INFO fired on every resolve of an unrotated cookie -- flooding for the whole rotation window and announcing a re-seal no write had performed. The rotation record is now latched once per process, the repeats are DEBUG, and the LogRecord is retemplated to say what it observes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XAAt7guDZEDA4S6nUTHcLW
shouldGenerateA256BitKey asserted the derived identity salt's length, which is a fixed-width SHA-256 digest and so would pass for a 128-bit key too -- the crypto-strength claim in its name was never checked. It now asserts the key length through a package-private accessor that exposes no material, with the salt-uniqueness claim split into its own test. Also hoist assertThrows lambda arguments into locals so exactly one call can throw (java:S5778), and rename the locals named 'record' (java:S6213). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XAAt7guDZEDA4S6nUTHcLW
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XAAt7guDZEDA4S6nUTHcLW
cuioss-oliver
force-pushed
the
feature/plan-07a-bff-cookie-crypto-core
branch
from
July 28, 2026 05:54
66f3817 to
ba3edbc
Compare
Collaborator
Author
|
/review |
cuioss-oliver
enabled auto-merge
July 28, 2026 05:59
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/SealedSessionCookieCodec.java (1)
225-231: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winINFO-level seal-success logging fires on every refresh, not just login.
seal()logs INFO unconditionally on every successful seal, butCookieSessionBindingcalls it from bothbind()(login) andpersist()(every token refresh). On a busy gateway with many active sessions and short-lived tokens, this becomes one INFO line per refresh per session — the same "fires on every request" pattern the rollover detection inCookieSessionBindingwas explicitly throttled to avoid.♻️ Suggested downgrade
- LOGGER.info(BffLogMessages.INFO.COOKIE_SESSION_SEALED, encoded.length()); + LOGGER.debug("Sealed cookie-mode session (%s bytes)", encoded.length());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/SealedSessionCookieCodec.java` around lines 225 - 231, Update the successful-seal logging in SealedSessionCookieCodec.seal so routine refreshes do not emit an INFO-level COOKIE_SESSION_SEALED entry on every call. Preserve the size-budget validation and return behavior, and use an appropriate lower log level for this per-seal event while retaining the existing context.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/CookieSessionBinding.java`:
- Around line 118-124: Update the cookie-mode session identity derivation used
by CookieSessionBinding and SessionRecord so it incorporates a session-specific
value from the sealed refresh-token family or nonce, rather than only sub and
loginInstant epoch seconds. Ensure each sealed payload for the same user and
timestamp receives a distinct, stable sessionId that remains consistent across
token refreshes and prevents TokenRefreshCoordinator.refresh() from coalescing
unrelated sessions.
---
Nitpick comments:
In
`@api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/SealedSessionCookieCodec.java`:
- Around line 225-231: Update the successful-seal logging in
SealedSessionCookieCodec.seal so routine refreshes do not emit an INFO-level
COOKIE_SESSION_SEALED entry on every call. Preserve the size-budget validation
and return behavior, and use an appropriate lower log level for this per-seal
event while retaining the existing context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 953c9f8f-0052-4ba8-ab06-2ba699058d7d
📒 Files selected for processing (68)
api-sheriff/pom.xmlapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/ApiSheriffLogMessages.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/BffLogMessages.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/CookieKeyMaterial.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/CookieSessionBinding.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/SealedSessionCookieCodec.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/SealedSessionPayload.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/package-info.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/logout/BackchannelLogoutReceiver.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/refresh/TokenRefreshCoordinator.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/reserved/BackchannelLogoutEndpoint.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/reserved/CallbackEndpoint.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/reserved/LoginInitiationEndpoint.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/reserved/LogoutEndpoint.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/reserved/UserInfoEndpoint.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/runtime/BffRuntime.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/runtime/SessionAuthenticationStage.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/runtime/package-info.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/session/ServerSessionBinding.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/session/SessionBinding.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/session/SessionRecord.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/session/SessionStore.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/session/package-info.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/model/OidcConfig.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/validation/ConfigValidator.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/EdgeHardeningOptions.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRoute.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/events/EventType.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/pipeline/BasicChecksStage.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/pipeline/PipelineRequest.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/BffRuntimeProducer.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/GatewayReadinessCheck.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/auth/AuthenticationStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/cookie/CookieKeyMaterialTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/cookie/CookieSessionBindingTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/cookie/SealedSessionCookieCodecTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/cookie/SealedSessionPayloadTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/refresh/TokenRefreshCoordinatorTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/reserved/BackchannelLogoutEndpointTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/reserved/CallbackEndpointTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/reserved/LoginInitiationEndpointTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/reserved/LogoutEndpointTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/reserved/UserInfoEndpointTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/runtime/SessionAuthenticationStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/session/ServerSessionBindingTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/model/ConfigModelContractTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/validation/ConfigValidatorTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/EdgeHardeningTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRouteBffWiringTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/ReservedBodyCeilingTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/events/EventTypeTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/BasicChecksStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/BffRuntimeProducerTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/CookieModeBootTest.javaapi-sheriff/src/test/resources/config/cookieboot/endpoints/bff-cookie.yamlapi-sheriff/src/test/resources/config/cookieboot/gateway.yamlapi-sheriff/src/test/resources/config/cookieboot/topology.propertiesdoc/LogMessages.adocdoc/configuration.adocintegration-tests/docker-compose.ymlintegration-tests/scripts/dump-keycloak-logs.shintegration-tests/scripts/start-integration-container.shintegration-tests/src/main/docker/sheriff-config-cookie/gateway.yamlintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffCookieActivationWiringTest.javaintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffCookieBackchannelDisabledIT.javaintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffCookieSessionIT.javaintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffCookieStatelessnessIT.javaintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffKeycloakLoginFlow.java
🚧 Files skipped from review as they are similar to previous changes (56)
- api-sheriff/src/test/resources/config/cookieboot/topology.properties
- integration-tests/scripts/dump-keycloak-logs.sh
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/ApiSheriffLogMessages.java
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/GatewayReadinessCheck.java
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/session/package-info.java
- integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffCookieSessionIT.java
- api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/EdgeHardeningTest.java
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/pipeline/BasicChecksStage.java
- doc/configuration.adoc
- api-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/reserved/CallbackEndpointTest.java
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/reserved/LoginInitiationEndpoint.java
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/session/SessionBinding.java
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/reserved/LogoutEndpoint.java
- api-sheriff/src/test/resources/config/cookieboot/gateway.yaml
- integration-tests/src/main/docker/sheriff-config-cookie/gateway.yaml
- api-sheriff/pom.xml
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/events/EventType.java
- api-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/BasicChecksStageTest.java
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/session/ServerSessionBinding.java
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/session/SessionStore.java
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/runtime/BffRuntime.java
- api-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/reserved/LoginInitiationEndpointTest.java
- api-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/cookie/CookieSessionBindingTest.java
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/pipeline/PipelineRequest.java
- integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffCookieBackchannelDisabledIT.java
- api-sheriff/src/test/java/de/cuioss/sheriff/gateway/events/EventTypeTest.java
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/logout/BackchannelLogoutReceiver.java
- api-sheriff/src/test/resources/config/cookieboot/endpoints/bff-cookie.yaml
- api-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/reserved/UserInfoEndpointTest.java
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/session/SessionRecord.java
- api-sheriff/src/test/java/de/cuioss/sheriff/gateway/auth/AuthenticationStageTest.java
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/runtime/package-info.java
- api-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/reserved/LogoutEndpointTest.java
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/reserved/UserInfoEndpoint.java
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/EdgeHardeningOptions.java
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/reserved/BackchannelLogoutEndpoint.java
- api-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/validation/ConfigValidatorTest.java
- api-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/CookieModeBootTest.java
- integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffKeycloakLoginFlow.java
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/reserved/CallbackEndpoint.java
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/SealedSessionPayload.java
- api-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/cookie/CookieKeyMaterialTest.java
- api-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/BffRuntimeProducerTest.java
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/model/OidcConfig.java
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/BffLogMessages.java
- api-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/cookie/SealedSessionCookieCodecTest.java
- api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRouteBffWiringTest.java
- integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffCookieActivationWiringTest.java
- api-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/model/ConfigModelContractTest.java
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/refresh/TokenRefreshCoordinator.java
- api-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/refresh/TokenRefreshCoordinatorTest.java
- integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffCookieStatelessnessIT.java
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRoute.java
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/validation/ConfigValidator.java
- api-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/session/ServerSessionBindingTest.java
- api-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/BffRuntimeProducer.java
Collaborator
Author
|
/review |
1 similar comment
Collaborator
Author
|
/review |
This was referenced Jul 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the cryptographic half of the stateless
session.mode: cookieBFFvariant (
doc/plan/08-bff-cookie.adoc) as a delta on the landed PLAN-06server-session runtime, not as a greenfield build. AES-256-GCM sealed session
cookies replace server-side
InMemorySessionStorestate for this mode, withprevious_keyrotation, dual key-material sourcing (passed key orgenerate-on-startup), a mode-neutral session-binding seam so
SessionAuthenticationStage/TokenRefreshCoordinatorno longer hard-bindSessionStore, and a404mode gate on the existing back-channel-logoutendpoint in cookie mode.
Changes
bff/cookie/package:CookieKeyMaterial,SealedSessionCookieCodec,SealedSessionPayload,CookieSessionBindingseal/unseal the token material PLAN-06's servervariant kept in
InMemorySessionStore.previous_keyrotation. Decrypt-with-previous / encrypt-with-currentrollover, composing only with the passed-key mode
(
config/model/OidcConfig.java,CookieKeyMaterial).encryption_key/previous_key${ENV_VAR}refs, or generate-on-startup(auto-generated AES-256-GCM key).
config/validation/ConfigValidator.javarelaxed so cookie mode with no key configured defaults to
generate-on-startup instead of failing boot, with a companion rule that
keeps
previous_key-without-encryption_keyinvalid. Reference docs forthe new/changed keys, including the generate-on-startup caveats (session
loss on restart, not shareable across replicas), ship in
doc/configuration.adoc.bff/refresh/TokenRefreshCoordinator.javare-expressed against the mode-neutral
SessionBindingseam (D7) so itsexisting single-flight coalescing and reuse-as-failure/session-destruction
rules apply uniformly to cookie-sealed state.
bff/reserved/BackchannelLogoutEndpoint.javaand
bff/logout/BackchannelLogoutReceiver.javamode-gated; unreachablepre-login handshake state stays server-side (see Known residual below).
BffCookieActivationWiringTest,BffCookieSessionIT,BffCookieStatelessnessIT,BffCookieBackchannelDisabledITplusBffKeycloakLoginFlowsupport code.Per-variant benchmark additions are explicitly DEFERRED — see obligation 2
below.
SessionBindingseam. Newbff/session/SessionBinding.javainterface with
ServerSessionBinding(PLAN-06 store-backed) andCookieSessionBinding(new, stateless) implementations, replacing thedirect
SessionStorebinding inSessionAuthenticationStageandTokenRefreshCoordinator. Re-scoped into this plan at outline time per thespec's pre-authorization once the CENTRAL RISK resolved to the store-shaped
branch — see obligation 7 below.
Operator-approved decisions and deliberate omissions (auditable record)
cookie-mode services to
integration-tests/docker-compose.yml(
api-sheriff-cookieon 10445/19002 andapi-sheriff-cookie-2on10446/19003) plus a readiness wait in
integration-tests/scripts/start-integration-container.sh.docker-compose.ymlis PLAN-25's declared surface underparallelization_scope: 2. The crossing is additive only — no newnetwork, no new volume, no edit to any existing service, no change to
compose networking. Taken because mandatory lesson
2026-07-25-15-001requires at least one IT that actually activates
session.mode: cookieend to end, and the second instance is what makes the stateless
cross-process proof real rather than asserted.
benchmark additions. They are DEFERRED out of this plan because
benchmarks/**is PLAN-25's surface. Stating the deferral explicitly sothe omission is auditable rather than silent.
(
doc/configuration.adocanddoc/LogMessages.adoc). The operator guideand developer guide are PLAN-07B's deliverable 7 — a deliberate split,
not an omission. Also deferring the update of
doc/variants/03-bff-cookie.adoc:154,177(which still states thenow-removed "required
session.encryption_key") to PLAN-07B.PendingAuthorizationStore. Pre-login handshakestate is still server-side and in-memory, so in cookie mode a login
started on one instance cannot complete on another. This is an accepted,
documented residual of the variant, not a defect fixed here. The sealed
pre-login-state delta was deliberately not taken.
@Nullablerecord-componentlayout in
BffRuntime.java(ReservedHttpRequest,ReservedHttpResponse)is deliberately accepted and recorded in-code rather than fixed, because
no formatter plugin is declared in either POM (inherited from
de.cuioss:cui-java-parent) and adding one would be an unapprovedbuild-config change.
Finding
0ced30:GatewayEdgeRoute.readReservedBodyThenDispatchbufferedthe full request body of the two unauthenticated reserved POST paths with
no byte cap, before the pipeline stages that would bound it — an
unauthenticated memory-exhaustion vector. Pre-existing (PLAN-06's
reserved-path surface), not introduced by this plan, fixed here under the
Boy Scout rule because this plan already edits that file. The fix caps
reserved-path bodies at 16 KiB derived from
EdgeHardeningOptions,enforced twice (Content-Length pre-check + streaming byte counter),
rejecting with 413 and WARN
ApiSheriff-109.SessionBindingseam) was added to this plan at outline time after thespec's CENTRAL RISK resolved to the store-shaped branch: the landed
PLAN-06 session seam bound
SessionStoredirectly in seven consumers plusthe producer, so a stateless variant was not expressible without it. The
spec pre-authorised exactly this re-scope.
Test Plan
BffCookieActivationWiringTest,BffCookieSessionIT,BffCookieStatelessnessIT,BffCookieBackchannelDisabledIT) plus both cookie-mode composeinstances verified GREEN locally against a freshly built native image
(75/75 integration tests pass, no pre-existing suite regressed) before
opening this PR.
Related Issues
N/A
Generated by plan-finalize skill
Summary by CodeRabbit
Set-Cookieheaders when needed.413(including connection retirement).404; invalid/tampered/expired cookie sessions fail safely.encryption_key/previous_keybehavior andmax_cookie_sizelimits.