-
Notifications
You must be signed in to change notification settings - Fork 156
Add header injection authentication support #2730
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2730 +/- ##
==========================================
+ Coverage 55.57% 55.96% +0.38%
==========================================
Files 314 318 +4
Lines 30460 30677 +217
==========================================
+ Hits 16928 17168 +240
+ Misses 12040 12022 -18
+ Partials 1492 1487 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds header injection authentication support to the MCP system, enabling backend MCP servers to receive custom authentication headers (like API keys). The implementation follows a clean registry pattern that allows for extensible auth type handling without modifying core authentication logic.
Key changes:
- Introduced
HeaderInjectionConfigCRD type with XOR validation between direct value and secret reference - Implemented converter registry pattern with
StrategyConverterinterface for pluggable auth types - Added
HeaderInjectionConverterand refactored existing token exchange logic to use the new registry
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/thv-operator/api/v1alpha1/mcpexternalauthconfig_types.go | Added HeaderInjectionConfig struct and ExternalAuthType enum with headerInjection support |
| cmd/thv-operator/api/v1alpha1/zz_generated.deepcopy.go | Auto-generated deepcopy methods for HeaderInjectionConfig |
| deploy/charts/operator-crds/crds/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml | Added headerInjection CRD schema with XOR validation rule |
| deploy/charts/operator-crds/Chart.yaml | Bumped CRD chart version to 0.0.63 |
| deploy/charts/operator-crds/README.md | Updated version badge to 0.0.63 |
| docs/operator/crd-api.md | Added documentation for ExternalAuthType and HeaderInjectionConfig |
| pkg/vmcp/auth/converters/interface.go | New converter registry pattern with StrategyConverter interface |
| pkg/vmcp/auth/converters/token_exchange.go | Refactored token exchange converter to implement StrategyConverter interface |
| pkg/vmcp/auth/converters/header_injection.go | New header injection converter with secret resolution support |
| pkg/vmcp/auth/converters/external_auth_config.go | Backward compatibility wrapper for existing external auth config usage |
| cmd/thv-operator/pkg/controllerutil/tokenexchange.go | Added header injection handling in AddExternalAuthConfigOptions |
| cmd/thv-operator/controllers/virtualmcpserver_deployment.go | Updated RBAC rules to grant vMCP access to mcpexternalauthconfigs |
| examples/operator/external-auth/header_injection_example.yaml | Example YAML demonstrating header injection usage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Implement header injection authentication type for MCPExternalAuthConfig, enabling backend MCP servers to receive custom authentication headers. Changes: - Add HeaderInjectionConfig to CRD with headerName and value/valueSecretRef - Implement converter registry pattern for extensible auth type handling - Create HeaderInjectionConverter for header_injection strategy - Refactor TokenExchangeConverter to use new registry pattern - Update RBAC rules to grant vMCP access to mcpexternalauthconfigs - Add controllerutil support for header injection validation - Bump operator CRD chart version to 0.0.63 - Include example YAML demonstrating header injection usage Header injection allows backends to authenticate using custom HTTP headers (e.g., X-API-Key) with values from either direct configuration or K8s secrets. The converter registry provides a clean abstraction for adding new auth types without modifying core authentication logic, following the Open/Closed Principle. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
d94e284 to
c28194a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
|
✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review. |
Large PR justification has been provided. Thank you!
7056f69 to
6bda036
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
test/e2e/thv-operator/virtualmcp/virtualmcp_header_injection_test.go
Outdated
Show resolved
Hide resolved
6bda036 to
800a1be
Compare
800a1be to
fe0da5a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fe0da5a to
a9ee52f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
a9ee52f to
8c2a397
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
test/e2e/thv-operator/virtualmcp/virtualmcp_discovered_mode_test.go
Outdated
Show resolved
Hide resolved
8c2a397 to
77cfe13
Compare
test/e2e/thv-operator/virtualmcp/virtualmcp_discovered_mode_test.go
Outdated
Show resolved
Hide resolved
77cfe13 to
10dc1f6
Compare
jhrozek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have two nits for later, but I'm fine cleaning them in a follow up
Implement header injection authentication type for MCPExternalAuthConfig, enabling backend MCP servers to receive custom authentication headers.
Changes:
Header injection allows backends to authenticate using custom HTTP headers (e.g., X-API-Key) with values from either direct configuration or K8s secrets.
The converter registry provides a clean abstraction for adding new auth types without modifying core authentication logic, following the Open/Closed Principle.
Large PR Justification
This PR implements header injection authentication for vMCP with two refactorings that must be atomic:
credentials in CRDs. Shipping the feature with this field would introduce a security vulnerability.
complexity (176→72 lines). Must ship together since both modify the same auth infrastructure - splitting would cause merge conflicts and require
multiple CRD regenerations.
Separating these changes would either compromise security or require maintaining deprecated code paths temporarily.
🤖 Generated with Claude Code