Add upstream_inject strategy type and auth server validation by jhrozek · Pull Request #4349 · stacklok/toolhive

jhrozek · 2026-03-24T16:50:30Z

Summary

Backend auth strategies need a way to inject upstream IDP tokens obtained by the embedded auth server into backend requests. This requires a new upstream_inject strategy type.
Add StrategyTypeUpstreamInject and UpstreamInjectConfig to backend auth strategy types.
Add ValidateAuthServerIntegration(cfg *Config, rc *authserver.RunConfig) with cross-cutting validation rules V-01 through V-07 (issuer consistency, audience matching, provider existence, etc.).
The validator takes *Config and *authserver.RunConfig as separate arguments — no RuntimeConfig wrapper type needed.

Fixes #4142
Fixes #4144

Type of change

New feature

Test plan

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

Special notes for reviewers

Draft — stacked on #4348. Review the incremental diff only.

The validation functions accept (*Config, *RunConfig) as separate parameters rather than a combined RuntimeConfig type. This keeps the config package free of auth server dependencies and makes the function signatures explicit about what they inspect.

Generated with Claude Code

codecov · 2026-03-24T16:55:59Z

Codecov Report

❌ Patch coverage is 85.58559% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.41%. Comparing base (074326e) to head (777eec5).

Files with missing lines	Patch %	Lines
pkg/vmcp/config/validator.go	84.76%	8 Missing and 8 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4349      +/-   ##
==========================================
+ Coverage   68.45%   69.41%   +0.96%     
==========================================
  Files         479      479              
  Lines       48642    48639       -3     
==========================================
+ Hits        33300    33765     +465     
+ Misses      12373    12277      -96     
+ Partials     2969     2597     -372

☔ 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 · 2026-03-24T18:52:54Z

pkg/vmcp/config/validator.go

+		if strategy.Type != authtypes.StrategyTypeTokenExchange {
+			continue
+		}
+		if rc.Issuer == cfg.IncomingAuth.OIDC.Issuer {


I'm a little confused about this check - rc.Issuer == cfg.IncomingAuth.OIDC.Issuer when vMCP is pointing to the embedded AS as its authn/z source, right? What's that have to do with token_exchange? Would you be able to add a comment about why upstream_inject is preferred here?

That function was in an earlier revision but got dropped during the rebase, sorry for the confusing patches. The idea behind it was: if the embedded AS is already managing the upstream OAuth flow, using token_exchange against the same issuer is redundant.

I did expand the doc comment on validateAuthServerIncomingAuthConsistency (the issuer/audience consistency check nearby) to clarify that it's strategy-agnostic.

(You might have reviewed the push before the rehoming from scaffolding-2 to main perhaps)

Add StrategyTypeUpstreamInject and UpstreamInjectConfig to backend auth strategy types, enabling vMCP backends to inject upstream IDP tokens obtained by the embedded authorization server. Add ValidateAuthServerIntegration with cross-cutting validation rules between the embedded auth server config and backend auth strategies: - V-01: upstream_inject requires auth server - V-02: upstream_inject providerName must exist in upstreams - V-04: auth server issuer must match incomingAuth OIDC issuer - V-05: auth server requires issuer and at least one upstream - V-07: incomingAuth audience must be in allowed audiences - V-09: auth server requires OIDC incoming auth - V-10: warn on duplicate upstream_inject provider names - V-11: warn when token_exchange targets the auth server - V-13: AllowedAudiences required for MCP compliance Refs: #4142

jhrozek · 2026-03-24T21:15:25Z

I apologize @tgrunnagle I shouldn't have opened the PR before I went through it carefully myself - that was unfiltered CC output that I just gave a cursory look to. I reviewed the patch manually this time and re-did it. I'm marking it ready for review.

Remove extra blank line and trailing newline that caused the gci linter to fail in CI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

tgrunnagle

Just a couple non-blocking comments

tgrunnagle · 2026-03-24T22:01:44Z

pkg/vmcp/config/validator.go

+		}
+		pn := authserver.ResolveUpstreamName(strategy.UpstreamInject.ProviderName)
+		if first, ok := seen[pn]; ok {
+			slog.Warn("multiple upstream_inject backends reference the same provider; likely a copy-paste error",


Question: why is this a warning case? Two backends requiring the same upstream token provider seems like a legit scenario.

hmm actually you're right, I was thinking it would more probably be copy-paste but we'd actually be confusing in a legitimate scenario

tgrunnagle · 2026-03-24T22:03:48Z

pkg/vmcp/config/validator.go

+// validateAuthServerRequiresOIDC checks that when the auth server is configured,
+// incomingAuth is OIDC. The AS issues tokens that the OIDC middleware
+// validates; without OIDC incoming auth the entire OAuth flow is pointless.
+func validateAuthServerRequiresOIDC(cfg *Config) error {


nit: this code is pretty similar to hasAuthServerWithOIDCIncoming, maybe could be refactored a bit

not a nit, good call, added

tgrunnagle · 2026-03-24T22:04:39Z

pkg/vmcp/config/validator.go

+		return result
+	}
+	if cfg.OutgoingAuth.Default != nil {
+		result["(default)"] = cfg.OutgoingAuth.Default


nit: does it matter if this is "default" vs with ()? I'm not sure name is used except for logging, but maybe worth being consistent.

The purpose was to avoid a key collision - this is a map key in a map that also stores user-defined backend names, so using just "default" could be problematic if someone named their backend default as well.. But "(default)" does look confusingly similar to authserver.DefaultUpstreamName.

I extracted the string to defaultStrategyKey = "<default-strategy>", hope this is clearer.

jerm-dro

Clean design — keeping (Config, RunConfig) as separate parameters is the right call. Validation rules are thorough and well-tested. The slices.Contains cleanup is a nice bonus.

@tgrunnagle

Expand the doc comment on validateAuthServerIncomingAuthConsistency to explain that it applies regardless of outgoing backend strategy (upstream_inject, token_exchange, etc.) and why the issuer/audience match matters: the OIDC middleware rejects every token whose iss claim differs from what it expects. Addresses PR review feedback from @tgrunnagle. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@tgrunnagle

Multiple backends referencing the same upstream provider is a valid and expected configuration (e.g., two GitHub-related backends both needing the "github" upstream token). The "likely a copy-paste error" warning was a false positive for legitimate configs. Addresses PR review feedback from @tgrunnagle. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@tgrunnagle

Extract the synthetic map key for the default outgoing auth strategy into a named constant. Use "<default-strategy>" instead of "(default)" to clearly distinguish it from authserver.DefaultUpstreamName and prevent key collisions with user-defined backend names. Addresses PR review feedback from @tgrunnagle. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@tgrunnagle

Both validateAuthServerRequiresOIDC and hasAuthServerWithOIDCIncoming checked the same three-field condition. Extract a shared hasOIDCIncoming predicate so the logic lives in one place. Addresses PR review feedback from @tgrunnagle. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

jhrozek · 2026-03-24T23:19:41Z

the additional commits address each of @tgrunnagle comments, patch per comment for easier review. the original patch is unchanged.

jhrozek mentioned this pull request Mar 24, 2026

Add AuthServerConfig validation condition to VirtualMCPServer reconciler #4350

Draft

4 tasks

github-actions bot added the size/M Medium PR: 300-599 lines changed label Mar 24, 2026

tgrunnagle reviewed Mar 24, 2026

View reviewed changes

jhrozek force-pushed the vmcp-add-as-scaffolding-2 branch from afc069c to 0bd1c80 Compare March 24, 2026 19:07

Base automatically changed from vmcp-add-as-scaffolding-2 to main March 24, 2026 19:59

jhrozek force-pushed the vmcp-add-as-scaffolding-3 branch from d447384 to 39a9a1e Compare March 24, 2026 21:14

github-actions bot added size/L Large PR: 600-999 lines changed and removed size/M Medium PR: 300-599 lines changed labels Mar 24, 2026

jhrozek marked this pull request as ready for review March 24, 2026 21:15

jhrozek requested review from ChrisJBurns, JAORMX, amirejaz, jerm-dro and yrobla as code owners March 24, 2026 21:15

Fix gci lint whitespace in validator.go

777eec5

Remove extra blank line and trailing newline that caused the gci linter to fail in CI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

github-actions bot added size/L Large PR: 600-999 lines changed and removed size/L Large PR: 600-999 lines changed labels Mar 24, 2026

tgrunnagle previously approved these changes Mar 24, 2026

View reviewed changes

jerm-dro previously approved these changes Mar 24, 2026

View reviewed changes

jhrozek and others added 4 commits March 24, 2026 22:53

jhrozek dismissed stale reviews from jerm-dro and tgrunnagle via 01d6fd3 March 24, 2026 23:18

github-actions bot added size/L Large PR: 600-999 lines changed and removed size/L Large PR: 600-999 lines changed labels Mar 24, 2026

Conversation

jhrozek commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Type of change

Test plan

Special notes for reviewers

Uh oh!

codecov bot commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhrozek commented Mar 24, 2026

Uh oh!

tgrunnagle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgrunnagle Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerm-dro left a comment

Choose a reason for hiding this comment

Uh oh!

jhrozek commented Mar 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jhrozek commented Mar 24, 2026 •

edited

Loading

codecov bot commented Mar 24, 2026 •

edited

Loading

tgrunnagle Mar 24, 2026 •

edited

Loading