Skip to content

Auto-populate SubjectProviderName for token_exchange strategies when embedded auth server is active#4529

Merged
tgrunnagle merged 3 commits intomainfrom
issue_4528_default-token-exchange-subjectprovidername-vmcp
Apr 3, 2026
Merged

Auto-populate SubjectProviderName for token_exchange strategies when embedded auth server is active#4529
tgrunnagle merged 3 commits intomainfrom
issue_4528_default-token-exchange-subjectprovidername-vmcp

Conversation

@tgrunnagle
Copy link
Copy Markdown
Contributor

Summary

When a VirtualMCPServer uses the embedded authorization server alongside a token_exchange outgoing auth strategy, omitting subjectProviderName caused the strategy to silently fall back to identity.Token (the ToolHive-issued JWT) as the RFC 8693 subject token. The exchange endpoint rejects the ToolHive JWT, but the failure was opaque — nothing in the error indicated that subjectProviderName needed to be set. This mirrors the same footgun that was fixed for Cedar authorization policies in #4448 with injectUpstreamProviderIfNeeded.

  • Added injectSubjectProviderIfNeeded to the operator controller (virtualmcpserver_controller.go) to auto-populate SubjectProviderName on token_exchange strategies where it is empty, using the first upstream from vmcp.Spec.AuthServerConfig (resolved via authserver.ResolveUpstreamName, same logic as Cedar). Applied to both the default strategy and all inline per-backend strategies.
  • Added InjectSubjectProviderNames to pkg/vmcp/config/defaults.go for the YAML config path, so the same defaulting applies when the vMCP binary is run directly with an authserver-config.yaml sibling file.
  • Called config.InjectSubjectProviderNames in cmd/vmcp/app/commands.go immediately after loading the auth server config, before the embedded auth server is started.
  • Updated the SubjectProviderName field comments in MCPExternalAuthConfig (mcpexternalauthconfig_types.go) and TokenExchangeConfig (pkg/vmcp/auth/types/types.go) to document the auto-population behavior.
  • Regenerated CRD manifests and API docs to reflect the updated field comment.

Closes #4528

Type of change

  • Bug fix

Test plan

  • Unit tests (task test)
  • Linting (task lint-fix)

Unit tests were added for both paths:

  • pkg/vmcp/config/defaults_test.goTestInjectSubjectProviderNames covers nil cfg, nil RunConfig, nil OutgoingAuth, named upstream, unnamed upstream (falls back to DefaultUpstreamName), empty upstreams list, first-of-multiple upstreams selected, explicit provider not overridden, and non-token-exchange strategy left unchanged. Modelled on TestInjectUpstreamProviderIfNeeded in pkg/runner/middleware_test.go.
  • cmd/thv-operator/controllers/virtualmcpserver_externalauth_test.go — extended with cases covering inline-backends injection (default strategy and per-backend strategies both populated), explicit provider preserved, non-token-exchange strategy skipped, and nil embedded auth config skipped.

Changes

File Change
cmd/thv-operator/api/v1alpha1/mcpexternalauthconfig_types.go Updated SubjectProviderName field comment to document auto-population
cmd/thv-operator/controllers/virtualmcpserver_controller.go Added injectSubjectProviderIfNeeded helper; applied to default and per-backend strategies in buildOutgoingAuthConfig
cmd/thv-operator/controllers/virtualmcpserver_externalauth_test.go Added unit tests for operator-path injection
cmd/vmcp/app/commands.go Call config.InjectSubjectProviderNames after loading auth server config in runServe
deploy/charts/operator-crds/… (4 files) Regenerated CRD manifests
docs/operator/crd-api.md Regenerated API reference doc
pkg/vmcp/auth/types/types.go Updated SubjectProviderName field comment to document auto-population
pkg/vmcp/config/defaults.go Added InjectSubjectProviderNames and injectIntoStrategy helpers
pkg/vmcp/config/defaults_test.go Added TestInjectSubjectProviderNames unit tests

Does this introduce a user-facing change?

Users who configure a token_exchange outgoing auth strategy on a VirtualMCPServer with an embedded authorization server no longer need to explicitly set subjectProviderName — it is automatically populated from the first configured upstream provider, matching the behavior that Cedar's primaryUpstreamProvider already had. Explicit values are never overridden.

Special notes for reviewers

The operator-path helper (injectSubjectProviderIfNeeded) returns a copy of the strategy rather than mutating in place, because the original values come from the CRD spec and should not be modified. The YAML-path helper (injectIntoStrategy, called by InjectSubjectProviderNames) mutates the strategy in place because the OutgoingAuth maps in the loaded Config are already owned by that config object and safe to modify before first use.

Generated with Claude Code

tgrunnagle and others added 3 commits April 3, 2026 09:49
When a token_exchange outgoing auth strategy omits SubjectProviderName,
automatically derive it from the upstream provider name in AuthServerConfig.
Ensures the OIDC subject claim is correctly populated for both YAML-based
vMCP deployments and operator-managed VirtualMCPServer resources.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Address two medium-priority code review issues from #4528:
- Move InjectSubjectProviderNames and injectIntoStrategy from
  validator.go to defaults.go where defaulting/filling operations live
- Add nil guard on cfg at the top of InjectSubjectProviderNames to
  prevent a panic if called with a nil *Config pointer

Move TestInjectSubjectProviderNames from validator_test.go to
defaults_test.go accordingly, adding a nil_cfg_is_a_noop test case.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Address two medium-priority code review issues from #4528:
- Regenerate CRD YAML and API reference docs so the expanded
  SubjectProviderName auto-population comment is reflected in
  deploy/charts/operator-crds/ and docs/operator/crd-api.md
- Add TestBuildOutgoingAuthConfig_InlineBackendSubjectProviderInjection
  to cover the inline Spec.OutgoingAuth.Backends override path which
  was missing end-to-end injection test coverage

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions github-actions Bot added the size/L Large PR: 600-999 lines changed label Apr 3, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 3, 2026

Codecov Report

❌ Patch coverage is 97.36842% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 69.10%. Comparing base (f81841f) to head (2b205eb).
⚠️ Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
cmd/vmcp/app/commands.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4529      +/-   ##
==========================================
+ Coverage   69.02%   69.10%   +0.07%     
==========================================
  Files         505      505              
  Lines       52144    52178      +34     
==========================================
+ Hits        35994    36056      +62     
+ Misses      13360    13336      -24     
+ Partials     2790     2786       -4     

☔ 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.

@tgrunnagle tgrunnagle marked this pull request as ready for review April 3, 2026 17:54
Copy link
Copy Markdown
Contributor

@jerm-dro jerm-dro left a comment

Choose a reason for hiding this comment

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

suggestion: There are now four copies of the "resolve first upstream provider name" pattern across three files (pkg/runner/middleware.go x2, the new operator-path injectSubjectProviderIfNeeded, and the YAML-path InjectSubjectProviderNames). The core logic is identical each time — ResolveUpstreamName(upstreams[0].Name) with a DefaultUpstreamName fallback.

Not a concern for this PR, but the broader pattern of silently auto-populating fields across distinct configuration surfaces (Cedar's primaryUpstreamProvider, token exchange's subjectProviderName, upstream swap's providerName) suggests these concepts are more coupled than the config model makes apparent. Each one independently derives the same value from the same source, but there's no shared abstraction that makes that relationship visible. Over time this implicit coupling could become confusing — someone changing the upstream provider model wouldn't know they need to audit all these injection points. Worth thinking about whether there's a way to make this relationship explicit in the config model rather than papering over it with per-feature defaulting helpers.

@tgrunnagle tgrunnagle merged commit 51218a7 into main Apr 3, 2026
40 checks passed
@tgrunnagle tgrunnagle deleted the issue_4528_default-token-exchange-subjectprovidername-vmcp branch April 3, 2026 20:44
MatteoManzoni pushed a commit to DocPlanner/toolhive that referenced this pull request Apr 4, 2026
…embedded auth server is active (stacklok#4529)

When a `VirtualMCPServer` uses the embedded authorization server alongside a `token_exchange` outgoing auth strategy, omitting `subjectProviderName` caused the strategy to silently fall back to `identity.Token` (the ToolHive-issued JWT) as the RFC 8693 subject token. The exchange endpoint rejects the ToolHive JWT, but the failure was opaque — nothing in the error indicated that `subjectProviderName` needed to be set. This mirrors the same footgun that was fixed for Cedar authorization policies in stacklok#4448 with `injectUpstreamProviderIfNeeded`.

- Added `injectSubjectProviderIfNeeded` to the operator controller (`virtualmcpserver_controller.go`) to auto-populate `SubjectProviderName` on `token_exchange` strategies where it is empty, using the first upstream from `vmcp.Spec.AuthServerConfig` (resolved via `authserver.ResolveUpstreamName`, same logic as Cedar). Applied to both the default strategy and all inline per-backend strategies.
- Added `InjectSubjectProviderNames` to `pkg/vmcp/config/defaults.go` for the YAML config path, so the same defaulting applies when the vMCP binary is run directly with an `authserver-config.yaml` sibling file.
- Called `config.InjectSubjectProviderNames` in `cmd/vmcp/app/commands.go` immediately after loading the auth server config, before the embedded auth server is started.
- Updated the `SubjectProviderName` field comments in `MCPExternalAuthConfig` (`mcpexternalauthconfig_types.go`) and `TokenExchangeConfig` (`pkg/vmcp/auth/types/types.go`) to document the auto-population behavior.
- Regenerated CRD manifests and API docs to reflect the updated field comment.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L Large PR: 600-999 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Auto-populate SubjectProviderName in vMCP token_exchange strategy when embedded auth server is active

2 participants