Repository navigation
fix(pce): PCE lock, failed-onboarding quarantine, and clientId-only PCE - #227
Conversation
…fect The HTTP onboarding route read AIAC_DEFAULT_EFFECT, the NATS path always used DENY, and nothing stored the value. So an unrelated run that re-rendered an agent could silently change its default. The default is now fixed: a request that no rule allows is denied. - Remove AgentPolicyModel.default_effect (a legacy payload that carries it is ignored), the ALLOW branch of the Rego decision block, and the default_effect parameter of onboard_service, the consumer, and compute_and_apply/_run/_derive/_fresh_apm. - Remove AIAC_DEFAULT_EFFECT from the Controller and its manifest, and the harness hook _set_controller_default_effect. - Move the denyworld (Policy B) oracle to test/system/disabled/; its tests stay skipped, as they need the removed ALLOW default. Handoff 11 B3 (D25). Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Oleg Blinder <oblinder@gmail.com>
_run reads an SPM, changes it, and writes it back, and the store has no versions. Two onboardings that route rules into one shared SPM (for example two agents granted on one tool's scope) lost one run's rules with no error. A module-level threading.Lock now covers the whole body of compute_and_apply and decommission. The PRB (LLM) work runs before compute_and_apply, outside the lock. Known limits (documented in the engine): one Controller replica only; a pair between two overlapping new services can stay unjudged (fail closed). Handoff 11 B1 (D22). Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Oleg Blinder <oblinder@gmail.com>
A failed first onboarding of an agent stayed fail-open (the combiner allows a pod with no CR), a failed re-onboarding kept its old SPM and CR, and a build that started before the rollback could write its rules back afterwards. The type clear was the only protection, and the user decided that it is a bug. - _rollback keeps the client type (unset_service_type has no caller). - The failure path is: rollback (delete this run's created roles and scopes, disable the client last), then the new PCE quarantine, then re-raise. The orchestrator reaches the PDP only through the PCE. - quarantine(service_uuid) holds the PCE lock: it deletes SPM(X), removes X's roles from the other SPMs, replaces an agent's CR with a no-rules CR (not a delete: today's combiner allows a missing CR), and re-derives the affected agents in one call. It shares the store teardown with decommission (_remove_footprint, _apply_derived). - The focal resolver skips the roles and scopes of disabled services, except the focus service. - The routing guard in _run drops every rule that touches a disabled service, except the focus service. compute_and_apply gets an optional focus_service (Keycloak UUID), which the onboarding route and the NATS consumer pass. - The lift is a successful re-onboarding: its apply writes the real CR with the same SSA field manager, then reenable_service runs. Handoff 11 B2 (D21, D22). Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Oleg Blinder <oblinder@gmail.com>
The outbound keyed only on input.mcp.params.name, so initialize, notifications/initialized, ping and tools/list (no tool name) were denied, and a normal MCP client stopped at its first message. - The four session messages are allowed to a target iff at least one tool of that target passes the full per-tool check for this request (user gate allows it, target gate allows it, no deny vetoes it). - tools/call stays a per-tool check on params.name and now also requires input.mcp.method == "tools/call"; every other MCP method is denied. - The four outbound gates are one set of Rego functions over a tool name, so tools/call and the session share one per-tool check. - opa eval tests: the no-rules CR of a quarantined agent denies every inbound (also the rossoctl platform client) and outbound request. Handoff 11 B4 (rule A, confirmed by the user, plus the tools/call method check). Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Oleg Blinder <oblinder@gmail.com>
… main - Operator 5e4c991 folded the bundle service into the operator image and removed operator/hack/bundle-service-kind.sh; no released operator chart carries it yet. Step 1 now builds the operator image from $OPERATOR_DIR, loads it into Kind, applies the AuthorizationPolicy CRD, and renders only the bundle-service templates from the local operator chart (bundleService.enabled=true, local image). A legacy bundle-service Deployment (selector app=bundle-service only) is deleted first, because the chart selector differs and a selector is immutable. The chart NetworkPolicy is not applied: it admits only pods labelled rossoctl.dev/authbridge=true, which nothing sets today. - Cortex afb49e9f moved authbridge/ to the repo root: Step 2 builds $CORTEX_DIR/cmd/authbridge-proxy/Dockerfile from the cortex root. Handoff 11 B5. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Oleg Blinder <oblinder@gmail.com>
- policy-computation-engine: the PCE lock and its known limits, the quarantine operation, the routing guard and focus_service; no default effect. - uc1-service-onboarding, aiac-agent, policy-rules-builder: the rollback keeps the client type; failure path is rollback -> quarantine -> raise; the focal resolver skips disabled services except the focus service; the lift is a successful re-onboarding. - library-idp, idp-configuration-service: the rollback no longer clears the type (the primitive stays, with no caller). - pdp-policy-writer-opa, policy-model: no default effect; the no-rules CR of a quarantined agent; the outbound MCP session rule. - uc1-onboarding-pipeline: the rung-5 failure-rollback expectations. - opa-kind-runbook: the new enable Step 1 and the moved cortex paths. Handoff 11. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Oleg Blinder <oblinder@gmail.com>
The cortex authbridge Dockerfile now refuses an untagged build (plugins are opt-in). Step 2 passes GO_BUILD_TAGS with the cortex "full" profile, as the cortex CI does, derived with a local go or in a golang container (GOWORK=off). GO_BUILD_TAGS overrides it. Checked on the rossoctl Kind cluster: the script runs to the end, the bundle service runs from the operator-main image, and OPA is wired into both legs. Handoff 11 B5. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Oleg Blinder <oblinder@gmail.com>
The library sends /policy/agents/<agent_id> with the id quoted as one
segment (%2F), but the server decodes %2F back to / before routing. A
SPIFFE agent id therefore never matched {agent_id}, and the writer
returned 404. Found on the Kind cluster: the PCE quarantine
(apply_agent_policy) could not write the no-rules CR. decommission
(delete_agent_policy) had the same latent bug.
Both routes now use {agent_id:path}. New tests drive each route with
the exact URL the library builds.
Handoff 11 B2 (found by the Group 8 system test).
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Oleg Blinder <oblinder@gmail.com>
Checked on the Kind cluster: from the github-agent container, an A2A message/send and an LLM /v1/chat/completions request through the outbound proxy (HTTP_PROXY=127.0.0.1:8081) both got HTTP 403 from OPA (policy.forbidden). The outbound package allows only granted tools/call and the MCP session messages. Recorded as a known limit; no behavior change. Handoff 11 (Check). Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Oleg Blinder <oblinder@gmail.com>
New test/system/test_uc1_onboard_failure_rollback.py (@pytest.mark.system, skips cleanly without the cluster or env). One shared stack, four phases: 1. failed agent: client disabled, type kept, created roles/scopes gone, no-rules CR present, a real inbound request denied, no SPM, failure logged and dead-lettered; 2. lift: a re-onboarding writes the real CR and re-enables the client; 3. tool onboarded: an MCP session (initialize, tools/list, tools/call) through the agent's outbound works for a granted user and is denied for an ungranted one; 4. failed tool: no CR, no SPM, the agent's outbound loses the grant. Failure injection: the Controller's LLM_BASE_URL points at a 404 path (a permanent UnparseableLLMResponseError, dead-lettered on the first delivery), restored in finally. The re-onboarding trigger publishes aiac.apply.service.<uuid> from the Controller pod. Harness helpers are additions only (pristine_stack, controller_llm_unusable, publish_service_event, CR/SPM readers, MCP session probes). On the Kind cluster, phases 1 and 2 pass. Phases 3 and 4 did not run to the end: the tool's first onboarding delivery hits a race with the operator's tool rollout (tools/list: connection refused), and the NATS redelivery comes after ACK_WAIT (600 s), past the harness window. Docs: a quarantined tool cannot be lifted today (tool Provision mints its discovery token with the tool client's own secret), recorded as a known limit; the test proves the lift on the agent. Handoff 11 Group 8. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Oleg Blinder <oblinder@gmail.com>
The operator registers a tool's Keycloak client (which fires the onboarding event) while it still rolls the tool pod onto the AuthBridge-injected template. tools/list then got "connection refused", and the retryable 502 was redelivered only after ACK_WAIT (600 s). Found on the Kind cluster: it broke the agent+tool system rungs. _mcp_tools_list now tries again every 3 s while the endpoint is not ready (connection error, or 502/503/504 from the sidecar), until AIAC_MCP_DISCOVERY_READY_TIMEOUT ends (default 120 s). A 4xx or a read timeout still fails at once. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Oleg Blinder <oblinder@gmail.com>
The example held two CR documents with the same name. The second was an older copy (single-effect maps), so `kubectl apply -f` overwrote the first with stale Rego. It also still described the removed default effect and had no MCP session rule. It is now one CR whose two packages are the exact generate_inbound_rego / generate_outbound_rego output for the demo policy (opa check --strict passes; server dry run accepts it). The runbook and the writer spec no longer say that the example predates the session rule. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Oleg Blinder <oblinder@gmail.com>
…agent The store returns a placeholder SPM typed AGENT on a 404, and the PCE fell back to that type for a service absent from the catalog. A late onboarding of a tool whose client was already deleted therefore wrote a CR (and an SPM) for the tool; a run that touched the stored SPM of a deleted agent wrote that agent's CR again. Found on the Kind cluster (a stray github-tool CR after a test teardown). - The routing guard in _run now also drops every rule whose scope owner, or the owner of whose agent role, is absent from the catalog (no focus exception: an absent service is deleted). - _run derives only agents that are in the catalog; decommission keeps its persisted-SPM fallback for the service it removes. - The store library's placeholder comment and the PCE spec say so. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Oleg Blinder <oblinder@gmail.com>
…fixes Apply the code-review findings on fix/pce-concurrency-quarantine: - onboard_service runs quarantine in a finally, so a failed rollback no longer skips it and leaves a first onboarding fail-open. A rollback or quarantine error still replaces the build error (the build error stays on __context__), so the consumer redelivers instead of term()ing. - Remove the orphan IdP primitive unset_service_type and its tests. - Delete the denyworld oracle and its tests: they need the ALLOW default effect that handoff 11 B3 removes. Drop the default-effect asserts from the policy-agnostic harness test. - Catch requests.RequestException, not Exception, in the MCP readiness wait. - Make outbound_probe a one-frame outbound_session_probe. - Stop using the avoided term "deny-overrides" in rego.py and the example. - Add Quarantine and Focus service to the CONTEXT.md glossary. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Oleg Blinder <oblinder@gmail.com>
… roles The PCE took two kinds of service id, both bare str: quarantine() and compute_and_apply(focus_service=...) took the Keycloak UUID, decommission() the clientId. A mix-up type-checked silently. Now every PCE service id is the clientId (the SPM key); the UUID is only for finding the service in the IdP (handoff 15). - quarantine(service_id) and focus_service take the clientId. _quarantine no longer searches the catalog by UUID; _disabled_services compares the catalog key. - onboard_service reads the Service once, before Provision (a failed read leaves nothing to compensate), passes it to _rollback, and returns (rules, False, client_id). The route and the consumer pass that clientId as focus_service, with no second IdP read; reenable_service keeps the UUID. - The consumer's _handle is its only subject switch: it returns (rules, override, focus), and _dispatch re-enables only when focus is set. - Add the ServiceUuid / ClientId NewTypes and use them where the two meet. - quarantine(client_id, deleted_roles): the orchestrator passes the created roles that the rollback deleted from the IdP, so their grants on other SPMs are purged too, not left until a later reconcile. - Tests fail if a UUID reaches a clientId parameter. Update the PCE, UC1 and agent specs and the CONTEXT.md Quarantine / Focus service entries. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Oleg Blinder <oblinder@gmail.com>
…EXT.md Fix the lint (3.12) and Markdown Lint PR checks. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Oleg Blinder <oblinder@gmail.com>
oblinder
left a comment
There was a problem hiding this comment.
Code review: 4 findings. Findings 2 and 4 are high-confidence. Findings 1 and 3 are plausible, but they depend on edge-case state.
| return all(owner in catalog and owner not in disabled for owner in owners) | ||
|
|
||
|
|
||
| def quarantine(service_id: ClientId, deleted_roles: Iterable[Role] = ()) -> None: |
There was a problem hiding this comment.
(Plausible) quarantine removes every role that X still has from the policy models of all other services. This includes a role that X reused by name and that another live service Z also holds. Z then loses its own grants on each failed onboarding of X. Before this change, this occurred only on an explicit offboard.
There was a problem hiding this comment.
Fixed in 7409457. _remove_footprint (shared by quarantine and decommission) now skips a role that another service in the catalog also holds. Thus Z keeps its grants. X stays denied: a quarantined agent has the no-rules CR, and an offboarded client cannot authenticate. Tests: test_quarantine_keeps_the_grants_of_a_role_another_service_also_holds, test_decommission_keeps_the_grants_of_a_role_another_service_also_holds.
| # (quarantined, except the focus service) or absent from the catalog (deleted). See | ||
| # ``compute_and_apply``. | ||
| disabled = _disabled_services(catalog, focus_service) | ||
| kept = [rule for rule in rules if _routable(rule, catalog, disabled)] |
There was a problem hiding this comment.
(High confidence) The new routing guard filters rules before the code builds the set of roles to purge. In override mode, if every new rule for a role touches a disabled service, the purge for that role does not run, and its old grants stay. At this time this is latent, because update_role and rebuild_policy are stubs that return []. Fix: build the purge set from the unfiltered input rules.
There was a problem hiding this comment.
Fixed in 7409457. _run now builds distinct_roles (the override purge set and the affected seed) from the input rules before the routing guard. Test: test_override_purges_a_role_whose_every_new_rule_the_guard_drops.
| apply_agent_policy(service_id, _fresh_apm(service_id)) | ||
|
|
||
| # (8) Re-derive every affected agent (X excluded) in one partial upsert. | ||
| _apply_derived(affected, spm, is_agent) |
There was a problem hiding this comment.
(Plausible) This PR stops the normal apply path from re-deriving an agent that is not in the catalog (affected & catalog.keys()). _quarantine and _decommission (also see line 500) do not apply that filter. Thus, an agent whose client was deleted without an offboard, and whose stored policy model remains, can get its policy CR back.
There was a problem hiding this comment.
Fixed in 7409457. All three paths now derive only live agents through _live_services() (in the catalog and not disabled; the focus service counts as live). This also stops a re-derive from writing over the no-rules CR of a disabled agent. Tests: test_quarantine_never_derives_an_agent_that_is_absent_from_the_catalog, test_quarantine_never_derives_a_disabled_agent, test_decommission_never_derives_an_agent_that_is_absent_from_the_catalog.
| lock (one agent replica only); cross-replica serialization is out of scope.""" | ||
| with _service_lock(service_id): | ||
| config = _config() | ||
| service = config.get_service(service_id) |
There was a problem hiding this comment.
(High confidence) The new config.get_service() call runs before Provision, outside the error handling of Provision. When the IdP is down or the UUID is unknown, the caller now gets a raw RuntimeError (a generic 500). Before this change, the caller got the sanitized HTTPException(502).
There was a problem hiding this comment.
Fixed in 7409457. onboard_service now maps a failed get_service() to HTTPException(502), the same boundary as Provision's classify_service. The test now raises a raw RuntimeError and asserts the 502.
- Override: build the purge set from the input before the routing guard, so a role whose every new rule the guard drops still loses its old grants. - Quarantine and decommission: derive only live agents (in the catalog and not disabled), as _run does. A stored SPM of a deleted agent no longer brings its CR back, and a quarantined agent keeps its no-rules CR. - Quarantine and decommission: keep the grants of a role that another service also holds (a realm role reused by name); the edges are keyed by role id, so the purge removed the other service's grants too. - UC1: map a failed pre-Provision get_service() to HTTPException(502), as Provision does, instead of a raw RuntimeError (a generic 500). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Oleg Blinder <oblinder@gmail.com>
03f752c to
7409457
Compare
|
All 4 review findings are fixed in 7409457. Each inline thread has a reply with the details and the test names.
The PCE and UC1 specs are updated to match. Verification: 6 new engine tests fail on the old code and pass with the fix. The 960 offline unit tests pass, and ruff reports no problems. I did not run the system suite ( |
|
Re-review after 7409457: all 4 findings are fixed. I found no new problems. At the new head, the unit suite passes (960 passed, 305 deselected).
Note (does not block the merge): the new |
Summary
This branch fixes the PCE concurrency bugs and the cleanup of a failed onboarding (handoff 11). It also makes the PCE take only the clientId (handoff 15).
Always DENY by default
(role, scope)pair that no rule mentions.PCE lock
compute_and_apply,decommissionandquarantine. Before this, two runs that wrote to one shared SPM could lose the rules of the first run. The PRB (LLM) work stays outside the lock.Quarantine a failed onboarding (UC1)
quarantinedeletes the SPM of the service, removes its roles from the other SPMs (also the roles that the rollback deleted), and replaces the CR of an agent with a no-rules CR that denies every request.quarantineruns in afinally, so a failed rollback cannot leave a first onboarding fail-open.The PCE takes only the clientId
quarantine,decommissionandcompute_and_apply(focus_service=…)take the clientId (Service.serviceId, the SPM key). The Keycloak UUID is only for IdP reads.onboard_servicereads theServiceonce, before Provision, and returns(rules, False, client_id). The route and the NATS consumer pass that clientId asfocus_service.ServiceUuid/ClientIdNewTypes mark the two kinds of id._handleis now its only subject switch.PDP and cluster
k8s/opa-kind-enable.sh(current operator, plugin build tags).Tests and docs
test_uc1_onboard_failure_rollback.py(quarantine and lift).CONTEXT.md(Quarantine, Focus service).Test plan
.venv/bin/pytest(unit lane): 954 passed.venv/bin/pytest -m system --collect-only -q: 85 collected, no errors.venv/bin/pytest -m system -k failure_rollbackon the Kind cluster, after a redeploy of the Controller (running now)🤖 Generated with Claude Code