fix: thread broker_token_issuer into the aggregator - #184
Merged
Conversation
The lifespan's populate_aggregator() call never passed the broker_token_issuer it had just loaded, so the aggregator's x509 client factories always saw None. In production every auth_type: x509 backend (ami-mcp) then connected with no Authorization header at list time -- the backend 401'd and was dropped as "unavailable", its tools never listed -- and an authorized tools/call raised a ToolError claiming no signing key was configured, even though the key had loaded fine onto app.state.broker_token_issuer and the /v1 redeem endpoint used it without issue. Existing tests constructed build_aggregator() directly with the issuer passed, so this app-level wiring gap was uncovered. The new regression test boots the real app with a signing key configured and asserts the x509 client factory the lifespan wired up mints a list-time identity header with the issuer the lifespan loaded. Assisted-by: Claude (Anthropic)
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.
Symptom (root-caused live on the UChicago AF)
Every
auth_type: x509backend (ami-mcp) was permanently dropped fromtools/listas "unavailable":even though
broker_token_issuer.loadedappeared at startup. An authorizedtools/callfared no better — it raised the ToolError claiming "no signing key is configured (chart: broker.identityToken.existingSigningKeySecret)", which was misleading: the key WAS configured, loaded ontoapplication.state.broker_token_issuer, and used successfully by the/v1redeem endpoint.Root cause
app.py's lifespan callspopulate_aggregator(...)to push the freshly loaded registry/policy/settings into the eagerly built aggregator — but it omitted thebroker_token_issuerkwarg.populate_aggregatorthreads that issuer into_make_client_factory, whose x509 branch mints the AF Broker Identity Token injected asAuthorization: Bearer. With the kwarg omitted, the deployed aggregator's_x509_factoryalways sawbroker_token_issuer is None:The fix is one line: pass
broker_token_issuer=broker_token_issuerin thepopulate_aggregator(...)call.Why tests missed it
Every existing x509-injection test constructed
build_aggregator(...)directly, passing the issuer itself — nothing exercised the app lifespan'spopulate_aggregatorwiring, so the gap between "issuer loaded onto app.state" and "issuer threaded into the aggregator's client factories" was uncovered.Regression test added
test_lifespan_threads_issuer_into_x509_client_factory(broker/tests/test_broker_issued_app.py): boots the real app viaapp_client_factorywith a signing key configured, fishes the shippedami(x509) backend's provider out of the app's aggregator, invokes its client factory in a list-time context for an entitled principal, and asserts the resulting connection carries anAuthorization: Bearerheader that verifies against the exact issuer the lifespan loaded (aud=ami). Written TDD: it failed on main with "x509 list-time connection carried no identity token" and passes with the fix.Full broker suite: 889 passed; ruff, mypy, and pre-commit clean.
🤖 Generated with Claude Code