feat: Add token broker plugin

[davidhadas]

Summary

Add standalone token broker plugin for acquiring service-specific tokens:

• Self-contained plugin with own config, routing, and HTTP client
• Comprehensive unit tests (410 lines) covering all scenarios
• Complete documentation including API interface and usage examples

Related issue(s)

#390

[huang195]

Solid external-plugin contribution overall — the plugin is structurally correct, follows the open-registry side-effect-import pattern, uses the four-step Configure idiom, and owns its own config / HTTP client / error type / router rather than forcing framework changes. Tests are extensive. Good example of what a third-party plugin looks like.

I'd like three things addressed before approving, plus a few items worth discussing.

Blocking

B1. Zero Invocation recording

grep -n "Record\|Allow\|Skip\|Modify" authbridge/authlib/plugins/tokenbroker.go returns zero hits. The plugin:

• Returns pipeline.DenyStatus(...) without pctx.DenyAndRecord(...) → the denial lands on the HTTP response but nothing appears in the session timeline.
• Replaces the Authorization header silently → no Modify Invocation.
• Falls through on no-match → no Skip Invocation.

Every gate plugin is expected to emit one Invocation per OnRequest pass so abctl's per-plugin timeline works. Compare plugins/tokenexchange/plugin.go — it emits Modify / Skip / Record(ActionDeny, Details: {...}) at every branch. Without this, an operator running abctl against a pipeline containing token-broker sees that the plugin ran but can't tell what it did or why it denied.

Minimum:

// Passthrough
pctx.Skip("no_broker_route")

// Successful token replacement
pctx.Record(pipeline.Invocation{
    Action: pipeline.ActionModify,
    Reason: "token_replaced",
    Details: map[string]string{
        "broker_url":             p.cfg.BrokerURL,
        "server_url":             serverURL,
        "authorization_endpoint": authorizationEndpoint,
        "token_endpoint":         tokenEndpoint,
    },
})

// Deny paths
return pctx.DenyAndRecord("missing_subject_token", "auth.missing-token",
    "broker route requires authorization token")
// and similar for broker-error / broker-unavailable, with Details carrying
// the broker's OAuth error / description / status code

B2. Stray // Made with Bob trailer in authbridge/authlib/plugins/registry.go:120

Looks like a leaked marker. registry.go is framework code unrelated to this plugin — the diff should not touch it with this trailer. Please remove.

B3. Branch is 7 commits behind main, including the detype refactor (PR #392)

The plugin code itself doesn't reference pctx.Claims / pctx.Route, so the rebase should be small. But once you add Invocation recording per B1, the Details: map[string]string pattern is the new shape on main and the tests will assert against it — worth rebasing before adding the recording so both changes live on the same base.

Worth discussing

D1. No token caching

Every outbound request to a brokered host triggers a fresh POST to {broker}/sessions/token. For an LLM agent making 10+ tool calls per conversation, that's 10× broker traffic per conversation with no reuse. token-exchange caches by SHA-256 of (client_id, target_audience, scopes, subject_token) specifically to avoid this. Two options:

1. State in docs that the broker is expected to have its own cache, so the plugin intentionally doesn't.
2. Add a TTL cache (the now-moved plugins/tokenexchange/cache/ package is a natural fit, or a small local one).

Either is fine — picking one is the blocker.

D2. Hardcoded http:// scheme

serverURL := "http://" + host at tokenbroker.go:216 — no HTTPS support. In-cluster is usually fine, but a cross-cluster / federated broker setup would break silently. Either a server_scheme config field (default http) or at minimum a doc note that the plugin assumes cleartext upstream.

D3. Clarify relationship to token-exchange in the docs

token-broker and token-exchange both replace the outbound Authorization header. A pipeline with both would have them fighting. One sentence in docs/token-broker-plugin.md covers it: "This plugin is an alternative to token-exchange, not a complement — a given outbound chain should use one or the other." Optional follow-up: a startup check that fails if both appear in the same chain.

Nits (take or leave)

N1. errors.As instead of concrete type assertion

At tokenbroker.go:225:

if brokerErr, ok := err.(*tokenbroker.BrokerError); ok { ... }

Prefer errors.As for wrapping-safety:

var brokerErr *tokenbroker.BrokerError
if errors.As(err, &brokerErr) { ... }

N2. Test file size

tokenbroker_test.go at 1455 lines + tokenbroker/client_test.go at 1219 lines = ~6.5× the 411-line implementation. Coverage is thorough, which is good, but future maintenance would benefit from splitting into topical files (configure / routing / client / error-mapping). Optional.

N3. Route precedence — file then inline, first-match-wins — is undocumented

buildBrokerRouterFrom appends file routes before inline rules, and the router does first-match-wins. So file routes take priority. Worth one sentence in token-broker-plugin.md.

N4. The explicitRoutesFile dance in Configure is dead weight today

Configure tracks whether Routes.File was explicitly set so it can require-exists-when-specified but tolerate-missing-when-defaulted. Since Routes.File currently has no default path, "explicitly set" == Routes.File != "". Simplify to:

if c.Routes.File != "" {
    if _, err := os.Stat(c.Routes.File); err != nil {
        return fmt.Errorf("routes file %q: %w", c.Routes.File, err)
    }
}

If a default routes path gets added later, the dance becomes meaningful — but preemptively carrying it hides what the code is doing today.

Non-issues (correct as-is)

• Empty Capabilities() — right, the plugin reads only headers + host, not body.
• Own router rather than authlib/routing — acknowledged in PR #392's commit message; the shared routing package will be generalized when a second consumer lands.
• http.NewRequestWithContext wires cancellation correctly.
• 310s HTTP client timeout (broker's own 300s + 10s slack) is deliberate and documented inline.

Summary

Three blockers (B1–B3) and three discussion items (D1–D3) before approve. B1 is the big one — the plugin is invisible to operators without Invocation recording, which is the primary observability surface for the whole pipeline.

Happy to discuss any of the above.

[davidhadas]

Code changes following review:

B1. Zero Invocation recording

Done

B2. Stray // Made with Bob trailer in authbridge/authlib/plugins/registry.go:120

Removed

B3. Branch is 7 commits behind main, including the detype refactor (PR #392)

Rebased

D1. No token caching

Documented the expectation that Token Broker cache tokens

D2. Hardcoded http:// scheme

This is a limitation in Core since the scheme is not added to the Context
A followup PR will be used to update Core which will help resolve this limitation.

D3. Clarify relationship to token-exchange in the docs

Doc remark added as suggested - yet, this is a problem that core should resolve not the plugin

N1. errors.As instead of concrete type assertion

Changed as suggested

N2. Test file size

Tests were split as suggested

N3. Route precedence — file then inline, first-match-wins — is undocumented

Doc commend added

N4. The explicitRoutesFile dance in Configure is dead weight today

Changed as suggested

[huang195]

Thanks for the thorough rewrite — much improved. Reviewing against my earlier feedback:

All three blockers resolved:

• B1 (Invocation recording): All five branches of OnRequest now record. Skip on no-route, DenyAndRecord on missing token, Record(ActionDeny) on broker error / broker-unavailable with structured Details, Record(ActionModify) on success. Mirrors the token-exchange pattern cleanly. abctl will now show operators what this plugin did.
• B2 (stray comment): Gone — PR no longer touches registry.go.
• B3 (rebase): Rebased on current main; restructured as plugins/tokenbroker/ + plugins/tokenbroker/client/, matching the tokenexchange/ layout.

Discussion items:

• D1 (caching): Addressed architecturally — docs explicitly say the broker manages token caching. Legitimate design choice, clearly documented.
• D3 (doc note about token-exchange): Addressed — first paragraph of docs/token-broker-plugin.md states these are alternatives, not complements.

D2 (hardcoded http://) still open. serverURL := "http://" + host at plugin.go:228. This is fine for in-cluster outbound and probably what you want for the MVP. But a cross-cluster or internet-facing broker target would break silently. Small follow-up: either a server_scheme config field (default "http") or a doc note in token-broker-plugin.md that the plugin assumes cleartext upstream. Not a blocker.

All four nits resolved:

• N1: errors.As replaces concrete type assertion.
• N2: Tests split into topical files (configure / edge / request / routing for plugin; acquire / error / network / security / bench for client).
• N3: First-match-wins documented in both the code comment and the docs.
• N4: explicitRoutesFile dance simplified to the clean form.

Approving. Nice shape.