Repository navigation
feat: let embedders pass KeySelector and KeyPoolFilter through the HTTP server - #8577
jeremym-tanium wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Expose and pass key-selection hooks transports/bifrost-http/server/server.go, transports/changelog.md |
BifrostHTTPServer adds KeySelector and KeyPoolFilter fields. Bootstrap forwards them to the Bifrost client configuration. The changelog documents the forwarding and default behavior. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Severity of issue fixed: Medium
Possibly related issues
- Deterministic priority-based API key selection: this change adds hooks for key selection and filtering, but does not implement the requested priority policy.
Merge Risk: 
40d36
-
Embedders can configure the key hooks before startup, and configuration reloads retain them. No merge-blocking issue was found.
Security architecture risk: 
0a28a
-
The hooks are supplied by trusted embedding code before startup, not by HTTP callers, and unset hooks preserve existing behavior. The main limitation is inherited: pool filtering is best-effort and does not cover every selection path, so it should not be treated as a complete authorization boundary.
Retained concerns
No architecture-level concerns identified.Security review details
-
Security Blast Radius
- inferred — The callbacks are stored on the initialized core client, so their effects extend to credential selection for requests sharing that client, rather than being confined to Bootstrap. Their actual tenant and environment scope depends on the embedding application and its configured provider pools.
Trust Boundaries and Controls
- observed — Hook authority comes from trusted embedding code through pre-startup function fields, not HTTP request data. Existing account-key eligibility checks remain upstream of normal selection; the separate direct-key mechanism still requires its server-side enablement setting.
Resilience and Maintainability Implications
- observed — Filter failure prioritizes continued selection over strict veto enforcement: both initial and renewed rotating-pool filtering log callback errors and continue with eligible unfiltered keys. This is inherited core behavior, not a new fallback introduced by the transport change.
Hardening Proposals
- proposed — Clarify that pool filtering is currently best-effort and path-dependent. If embedders need an authorization boundary, define that guarantee separately, including fail-closed errors and consistent enforcement across fixed, rotating and realtime/WebSocket selection paths.
-
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: allowing HTTP server embedders to pass KeySelector and KeyPoolFilter to core. Description check The description covers the purpose, implementation, affected area, testing steps and result, breaking-change status, related issue, security considerations, and checklist. It also explains why no new … Linked Issues check Issue #8576requiresBifrostHTTPServerfields forKeySelectorandKeyPoolFilter, set beforeBootstrap, and passed tobifrost.Init. The reviewed changes add both exported fields and pass both…Out of Scope Changes check The reviewed changes only add the requested HTTP server fields, wire them through Bootstrap, and document the behavior in the changelog. These changes support issue#8576. No unrelated change is sho…Docstring Coverage No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Full details: Description check
-
Explanation
The description covers the purpose, implementation, affected area, testing steps and result, breaking-change status, related issue, security considerations, and checklist. It also explains why no new test was added and why ReloadConfig is unchanged.
Full details: Linked Issues check
-
Explanation
Issue
#8576requiresBifrostHTTPServerfields forKeySelectorandKeyPoolFilter, set beforeBootstrap, and passed tobifrost.Init. The reviewed changes add both exported fields and pass both values duringBootstrap. The changelog states that unset values use core defaults.SessionAffinityis optional context in the issue and is not required.
Full details: Out of Scope Changes check
-
Explanation
The reviewed changes only add the requested HTTP server fields, wire them through
Bootstrap, and document the behavior in the changelog. These changes support issue#8576. No unrelated change is shown.
Full details: Docstring Coverage
-
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 1 files.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
681fc4b to
0a28adc
Compare
0a28adc to
15dbd2e
Compare
…TP server Core accepts KeySelector and KeyPoolFilter on BifrostConfig, but the HTTP transport builds that config inside Bootstrap without them, so a program embedding the HTTP server cannot supply key-selection policy. Add both as fields on BifrostHTTPServer and pass them to bifrost.Init. Nil keeps core's defaults. Affected packages: transports
15dbd2e to
add2d48
Compare
 [High impact] Adds embedder hooks into core key-selection logic. Add the required regression tests before merging. Findings
Reviews (2) · Last reviewed commit: "chore: trailing one-line comments for th..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Per AGENTS.md, struct field comments stay short and trailing; the set-before-Bootstrap requirement moves to Bootstrap's doc comment. Affected packages: transports
| KeySelector: s.KeySelector, | ||
| KeyPoolFilter: s.KeyPoolFilter, |
There was a problem hiding this comment.
Bootstrap now passes KeySelector and KeyPoolFilter to core, but the PR adds no tests for either hook. The repository requires every behavior change to include regression tests. Add coverage showing that both supplied hooks reach core and that nil preserves the defaults. This repository requirement must be satisfied before merging.
File Used: AGENTS.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Summary
Core accepts
KeySelectorandKeyPoolFilteronschemas.BifrostConfig, butBifrostHTTPServer.Bootstrapbuilds that config without them, so a program embedding the HTTP server cannot supply key-selection policy to core. This passes both hooks through.Changes
transports/bifrost-http/server/server.go: addKeySelectorandKeyPoolFilterfields toBifrostHTTPServerand pass both tobifrost.InitinBootstrap. Nil keeps core's defaults.transports/changelog.md: entry under Features.ReloadConfigis unchanged, since core keeps the hooks set atInitacross reloads.Type of change
Affected areas
How to test
The package's tests pass apart from
TestAgentGatewayRoutesRegisteredAfterStartupInitialization, which fails the same way ondevat 6d3fbac without this change. There is no new test: the change passes two values throughBootstrap, which needs a full config to exercise.Screenshots/Recordings
N/A
Breaking changes
Related issues
Closes #8576
Security considerations
None. The hooks are set by the embedding program in code, not through config or the API.
Checklist
docs/contributing/README.mdand followed the guidelines