Repository navigation
fix(OMN-15918): node_gateway_attach_effect hardening — JWKS signature verification, identity binding, atomic transitions, outage/revocation split - #2727
Conversation
…e verification, identity binding, atomic transitions, outage/revocation split CodeRabbit-flagged hardening follow-ups on OMN-15750 (PR #2694), TDD RED-before/GREEN-after for each: - R1 (JWT signature verification): `decode_claims` (structural-only, never referenced the signature segment) replaced with `verify_and_decode_claims` — verifies against a resolved JWKS keyset via PyJWT before trusting any claim. `alg:none`, wrong-key-signed, and unknown-kid tokens are all rejected. JWKS fetch (network I/O) lives inline in each handler (`_fetch_jwks`), circuit-breaker guarded; verification itself stays I/O-free in the service module, matching the existing `_introspect` I/O-boundary pattern. - R2 (identity binding): heartbeat and detach now re-verify the presented token's signature and bind its tenant_id/principal_id/client_id to the STORED session's identity from attach time before acting. Detach previously took zero credential at all (session_id + free-text reason); ModelGatewayDetachRequest now requires access_token (contract minor bump 0.1.0 -> 0.2.0, wire-breaking on this new node with no external consumers yet). - R3 (atomic transitions): ProtocolGatewaySessionStore.put_if_present closes the heartbeat resurrection race — a concurrent detach landing in the read-introspect-write gap is no longer silently resurrected by the heartbeat's final write. - R4 (outage vs revocation): JWKS fetch and RFC 7662 introspection are now MixinAsyncCircuitBreaker-guarded; a transport error, non-200, or malformed body raises InfraUnavailableError and leaves the session untouched, instead of the previous fail-closed False that made every Keycloak outage read as mass revocation. Ticket item 1 (DI container for handler construction) is explicitly deferred — out of scope for this hardening slice, tracked as a known_gap in contract.yaml. Filed OMN-15952 (linked blocker to OMN-15877) for the unattended pairing/renewal contract gap the ground-truth verification surfaced separately. 35 tests (12 validator + 20 handler + 3 store), all new/changed assertions verified RED against the pre-hardening source via git stash before GREEN. ruff + mypy --strict + pre-commit (98 hooks, file-scoped) all clean.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
Comment |
✅ Hostile Reviewer — PASSEDBlocking findings (critical): 0 Gate semantics (pilot phase)
Powered by omniintelligence.review_pairing.cli_review — node-based adversarial review via HandlerLlmCliSubprocess (OMN-8468/OMN-8524) |
…ce/Evidence-Ticket PR-body fix
#6396) * evidence(OMN-15918): OCC companion for OmniNode-ai/omnibase_infra#2727 * evidence(OMN-15918): add falsifiable deploy probe for node_gateway_attach_effect (OMN-14505/OMN-8912) * fix(OMN-15918): harden deploy evidence probe (#6409) * fix(OMN-15918): bind OCC companion to PR (#6410)
1 participant
Summary
Hardening follow-ups on
node_gateway_attach_effect(OMN-15750, PR #2694) flagged by CodeRabbit review and confirmed open by a live ground-truth verification pass against mergedorigin/dev(commit52d8b601c). Implements 4 of the ticket's 5 DoD items — item 1 (DI container for handler construction) is explicitly deferred, see "Not in scope" below.decode_claimsonly base64-decoded the payload segment and never referenced the token's signature at all — a structurally-valid-but-forged token would attach and hold an ACTIVE session for up to one heartbeat interval. Replaced withverify_and_decode_claims, which verifies the signature against a resolved JWKS keyset (PyJWT) before trusting any claim.alg:none, wrong-key-signed, and unknown-kidtokens are all rejected.client_idvia introspection; detach took zero credential at all — any caller holding asession_idcould detach any tenant's session). Both now re-verify the presented token's signature and bindtenant_id/principal_id/client_idto the STORED session's identity from attach time before acting.HandlerGatewayHeartbeat.handleread the session, awaited an introspection round-trip, then wrote it back unconditionally — a concurrent detach landing in that gap was silently resurrected.ProtocolGatewaySessionStore.put_if_presentcloses this: the final write is a no-op if the session was removed during the await.MixinAsyncCircuitBreaker-guarded. A transport error, non-200, or malformed body raisesInfraUnavailableErrorand leaves the session untouched — previously, any Keycloak/JWKS blip fail-closed identically to genuine revocation and mass-revoked every active session on its next heartbeat.Seams
Boundary seam for every field the decoder now demands, matched against the minting side (
omninode_infra#873's tenant-scoped attach-only Keycloak client — a different client than the broker-scoped P0B client OMN-15923/omninode_infra#878 fixed separately):verify_and_decode_claims)isskeycloak_issuer_refsecret valueaudconfig.required_audience("gateway-attach")expoptions={"require": [...]}, defaultverify_expon), capped downstream atmax_session_ttl_secondssubtenant_idtenant_slugprincipal_idazp(client_id)gw-tenant-<hex>shape)kidkidin its JWKSalg"none"No naming or shape mismatch on this seam — everything the decoder requires is minted with matching name and type. This is the browser-exchanged attach-client seam (
omniweb#270 ⟷omninode_infra#873); it is the opposite of the now-resolved OMN-15923 situation (the separate broker-scoped P0B client, which had a genuine audience mismatch).Cross-boundary regression test driving the real seam end-to-end (no mocked far side of the decode path — only the JWKS HTTP transport is faked, matching the existing
_introspecttest convention already in this file):tests/unit/nodes/node_gateway_attach_effect/test_handlers.pybuilds a real RS256-signed JWT with a real RSA keypair, serializes the public half into a JWKS response body, and drives it through the actualHandlerGatewayAttach.handle→verify_and_decode_claims→ PyJWT signature-verification path — not a stub. Seetest_attach_registers_active_session(happy path) andtest_attach_rejects_forged_signature/test_attach_jwks_outage_raises_unavailable(the two failure-class proofs).Not in scope
Ticket item 1 (DI container: handlers resolve config/services via
ModelONEXContainer.get_service(...)instead of direct constructor args) is not built here — it needs the node's DI wiring to registerProtocolGatewaySessionStore/SecretResolver/ModelGatewayAttachConfigas container services first, which is an independent, heavier-lift change orthogonal to the security hardening in this PR. Tracked as aknown_gapsentry incontract.yaml; not silently dropped.Also filed and linked as a blocker to OMN-15877: OMN-15952 — unattended pairing/renewal contract for persistent runtimes across the 900-second attach-token expiry. The ground-truth verification pass surfaced this as a real, unticketed gap (the only drafted minting path,
omniweb#270 →omninode_infra#873, requires an active browser session per exchange with no renewal mechanism for a headless/persistent runtime) — not fixed here, just captured so it doesn't get lost.Evidence
test_handlers.py,test_service_keycloak_token_validator.py, and the newtest_store_gateway_session_memory.pywas verified to fail against the pre-hardening source (git stashof thesrc/changes only, tests kept in place) — 16 failed + 18 errors, 1 unrelated pass. Reverted to GREEN with the implementation restored.uv run pytest tests/unit/nodes/node_gateway_attach_effect/ -q.uv run ruff format/uv run ruff checkclean.uv run mypy src/omnibase_infra/nodes/node_gateway_attach_effect/ --strict— 0 issues, 22 source files.uv run pre-commit run --files <changed files>— 92/92 applicable hooks passed, 0 failed (98 configured hooks total; 6 are pre-push-stage-only and not invoked by this file-scoped run).OMN-13973) run live on this host.Contract version
node_gateway_attach_effectcontract/node version bumped 0.1.0 → 0.2.0 (minor):ModelGatewayDetachRequestgains a requiredaccess_tokenfield (wire-breaking on this brand-new node, which has no merged external consumers yet — the two draft PRs that will eventually call it,omniweb#270 andomninode_infra#873, are both still in draft) andModelGatewayAttachConfiggainskeycloak_jwks_ref+circuit_breaker_threshold/circuit_breaker_reset_timeout_seconds.Evidence-Source: OCC#6396
Evidence-Ticket: OMN-15918
OMN-15918