Skip to content

Conversation

JAORMX
Copy link
Collaborator

@JAORMX JAORMX commented Oct 10, 2025

Summary

Implement external authentication configuration for MCP servers via a new MCPExternalAuthConfig custom resource. This enables MCP servers to exchange incoming authentication tokens for tokens that can be used with external services via RFC-8693 OAuth 2.0 Token Exchange.

Implementation

  • MCPExternalAuthConfig CRD: Namespace-scoped configuration resource
  • Controller: Implements finalizer to prevent deletion while referenced, hash-based change detection to trigger MCPServer reconciliation
  • MCPServer Integration: Configuration injected into deployments via RunConfig ConfigMap
  • Security: OAuth client secret provided through TOOLHIVE_TOKEN_EXCHANGE_CLIENT_SECRET environment variable referencing a Kubernetes Secret

Testing

  • Unit tests: 83% average coverage
  • Integration tests for MCPServer external auth handling
  • E2E Chainsaw tests validating end-to-end flow
  • Example manifests included

Related

  • Proposal: docs/proposals/token-exchange-middleware.md

🤖 Generated with Claude Code

@JAORMX JAORMX force-pushed the external-auth-controller-impl branch 2 times, most recently from b185a87 to c61617d Compare October 10, 2025 14:06
Copy link

codecov bot commented Oct 10, 2025

Codecov Report

❌ Patch coverage is 63.73057% with 140 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.25%. Comparing base (6e18a3c) to head (0c8e399).

Files with missing lines Patch % Lines
...or/controllers/mcpexternalauthconfig_controller.go 58.86% 57 Missing and 8 partials ⚠️
...d/thv-operator/controllers/mcpserver_controller.go 46.78% 52 Missing and 6 partials ⚠️
cmd/thv-operator/main.go 0.00% 14 Missing ⚠️
...md/thv-operator/controllers/mcpserver_runconfig.go 95.77% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2150      +/-   ##
==========================================
+ Coverage   53.09%   53.25%   +0.15%     
==========================================
  Files         222      225       +3     
  Lines       28908    29270     +362     
==========================================
+ Hits        15348    15587     +239     
- Misses      12421    12525     +104     
- Partials     1139     1158      +19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@JAORMX JAORMX force-pushed the external-auth-controller-impl branch 4 times, most recently from ee7af78 to 0492738 Compare October 14, 2025 09:51
@JAORMX JAORMX marked this pull request as ready for review October 14, 2025 10:14
jhrozek and others added 2 commits October 14, 2025 17:48
Implement external authentication configuration for MCP servers via a new
MCPExternalAuthConfig custom resource. This enables MCP servers to exchange
incoming authentication tokens for tokens that can be used with external
services via RFC-8693 OAuth 2.0 Token Exchange.

The MCPExternalAuthConfig is namespace-scoped and can only be referenced by
MCPServers in the same namespace. The controller implements a finalizer to
prevent deletion while referenced, and uses hash-based change detection to
efficiently trigger MCPServer reconciliation when configuration changes.

Configuration is injected into MCPServer deployments via RunConfig ConfigMap
with the OAuth client secret provided through a TOOLHIVE_TOKEN_EXCHANGE_CLIENT_SECRET
environment variable that references a Kubernetes Secret, following security
best practices.

The controller follows the same pattern as MCPToolConfig, including:
- ReferencingServers status field for tracking which MCPServers reference the config
- Proper reconcile flow that updates status with referencing servers
- Correct SetupWithManager watch handler that reconciles only the specific
  MCPServers that reference a changed ExternalAuthConfig (not all configs in namespace)
- Status updates during deletion when config is still referenced

Includes comprehensive unit tests (83% coverage), integration tests, E2E
Chainsaw tests, and example manifests.

Co-Authored-By: Jakub Hrozek <jakub@stacklok.com>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Juan Antonio Osorio <ozz@stacklok.com>
Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com>
Extract duplicate code from MCPToolConfig and MCPExternalAuthConfig
controllers into reusable generic helper functions.

This change introduces two generic helper functions in config_helpers.go:
- CalculateConfigHash[T any](spec T): Generic hash calculation for any
  config spec using Kubernetes utilities
- FindReferencingMCPServers(): Generic function to find MCPServers that
  reference a config resource

Benefits:
- Eliminates ~50 lines of duplicate code
- Single source of truth for shared logic
- Type-safe with Go generics
- Makes future config-style CRDs easier to implement
- All existing tests pass with no behavioral changes

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@JAORMX JAORMX force-pushed the external-auth-controller-impl branch from 111dd67 to 8930036 Compare October 14, 2025 14:49
Copy link
Collaborator Author

@JAORMX JAORMX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re: the range loop pointer safety question on line 120 of mcpexternalauthconfig_controller.go:

Yes, it's safe to use &server in Go 1.22+ (we're using Go 1.25.2). Since Go 1.22, loop variables are scoped per iteration, so taking the address of the loop variable is now safe and idiomatic.

The same pattern is used in toolconfig_controller.go:118, confirming this is the established approach in this codebase.

For reference: https://go.dev/blog/loopvar-preview

@JAORMX JAORMX force-pushed the external-auth-controller-impl branch from 1a95ee8 to 26deb79 Compare October 14, 2025 15:08
JAORMX and others added 3 commits October 14, 2025 18:08
Adds test coverage for the external authentication configuration flow
in the operator's RunConfig generation. Tests cover:

- addExternalAuthConfigOptions: 5.7% → 97.1% coverage
- generateTokenExchangeEnvVars: 0% → 100% coverage
- createRunConfigFromMCPServer with external auth: improved to 70.6%

The new test suite includes 21 test cases covering:
- Token exchange middleware configuration generation
- Secret validation and reference handling
- Error paths for missing configs, secrets, and invalid types
- Edge cases like empty scopes and custom header strategies
- Environment variable generation for Kubernetes secrets

Overall operator controller coverage increased from 56.7% to 59.4%.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Change TokenExchangeConfig JSON field names from snake_case to
camelCase to match the convention used throughout the codebase:
- token_url → tokenUrl
- client_id → clientId
- client_secret_ref → clientSecretRef
- external_token_header_name → externalTokenHeaderName

Also change Scope from a space-separated string to Scopes as an
array of strings. This aligns with the existing middleware
implementation in pkg/auth/tokenexchange/middleware.go which
already expects Scopes as []string.

Update controller code to use the Scopes array directly instead
of converting from a string, and update all tests accordingly.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com>
Increment operator-crds chart version from 0.0.34 to 0.0.35 and operator
chart version from 0.2.22 to 0.2.23 to reflect the addition of the new
MCPExternalAuthConfig CRD and updates to the MCPServer CRD.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@JAORMX JAORMX force-pushed the external-auth-controller-impl branch from 26deb79 to 0c8e399 Compare October 14, 2025 15:08
}

// Calculate the hash of the current configuration
configHash := r.calculateConfigHash(externalAuthConfig.Spec)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this will not catch the case where we modify the secret contents right? I think it's acceptable, we just need to document that or at least know about the limitation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, secrets need to be dealt with separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants