stacklok / stacklok/toolhive

authserver: export the RunConfig→Config construction so embedders can use authserver.New without reimplementing it

Open
#5,773 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Go
Stars
2.2k
Forks
300
Avg merge
1d 15h
Merged PRs (30d)
184

Description

Summary

pkg/authserver offers two ways to stand up the embedded auth server:

  • The runnerrunner.NewEmbeddedAuthServer(ctx, *RunConfig) / NewEmbeddedAuthServerWithStorage(ctx, *RunConfig, storage) — takes the serializable RunConfig, does all the RunConfig → authserver.Config translation internally, but does not expose authserver.Config.UpstreamFilter (or other Config-only fields).
  • authserver.New(ctx, Config, storage) — takes a fully-built authserver.Config, which is the only way to set Config.UpstreamFilter.

An embedder that needs Config.UpstreamFilter (or any Config-only field) must therefore call authserver.New directly — but there is no exported way to derive a Config from a RunConfig. That translation is entirely unexported in pkg/authserver/runner: buildUpstreamConfigsbuildOIDCConfig/buildPureOAuth2Config, resolveSecret, convertUserInfoConfig, the ephemeral key/HMAC defaulting (createKeyProvider/loadHMACSecrets), upstream.RegisterModifiers(), and the DCR body-size cap that EmbeddedAuthServer.Handler() applies.

Problem

So each embedder that wants the Config path re-implements that translation by hand. That is:

  • Error-prone. The translation quietly does several things that are easy to miss when copied: the DCR request-body cap (bodylimit.Middleware(handlers.MaxDCRBodySize) on Handler()), threading InsecureAllowHTTP onto the top-level Config, calling upstream.RegisterModifiers(), client-secret-from-file/env resolution, and scope defaults. Missing any of these silently weakens or breaks auth (e.g. serving authserver.Server.Handler() directly loses the body cap on the open DCR endpoint).
  • Drift-prone. A hand copy does not follow changes to the (unexported) source across releases.
  • Duplicated. At least two separate downstream consumers have independently re-implemented this same translation (plus a per-user UpstreamFilter on top). Same logic, multiple copies, each re-discovering the same gaps — a fix in one does not reach the others.

Recommended solution

Export the RunConfig → Config construction from pkg/authserver so embedders that need authserver.New reuse the same reviewed code instead of copying it. Any one of these closes the gap:

  1. A public constructor — func (rc *RunConfig) BuildConfig() (Config, keys.KeyProvider, error) (or a free func BuildConfig(rc *RunConfig) (...)) — that runs exactly what the runner does today (key/HMAC defaulting, upstream mapping incl. DCR, RegisterModifiers, etc.). The runner constructors then become thin wrappers over it, so there is a single source of truth.
  2. Export the sub-helpers (BuildUpstreamConfigs, …) if one coarse entry point isn't desirable.
  3. Export a handler-wrapping helper for the DCR body-size cap (e.g. WrapWithBodyLimit(http.Handler) http.Handler), since authserver.Server.Handler() returns the bare router while EmbeddedAuthServer.Handler() applies the cap — an easy footgun for anyone serving authserver.Server directly.

Alternative considered: add a WithUpstreamFilter(...) functional option to the runner constructors so consumers can keep using the runner. Smaller, but it only solves the UpstreamFilter case, not the general "I need a Config-only field" problem — exporting the construction is the more complete fix.

Why it matters

The Config path is (rightly) the intended integration point for UpstreamFilter and other advanced fields. But if building a Config from a RunConfig isn't a first-class exported operation, every embedder re-derives it: the construction gets copy-pasted across codebases, fixes in one don't reach the others, and security-relevant details (the DCR body-size cap, InsecureAllowHTTP, modifier registration) get dropped in the copies.

References (as of v0.34.0)

  • pkg/authserver/runner/embeddedauthserver.go — the unexported translation (buildUpstreamConfigs, buildOIDCConfig, buildPureOAuth2Config, resolveSecret, convertUserInfoConfig, createKeyProvider, loadHMACSecrets)
  • pkg/authserver/config.goConfig.UpstreamFilter
  • pkg/authserver/server_impl.goHandler() returns the bare router (no body cap)
  • pkg/authserver/server/handlers/handler.goUpstreamFilter, MaxDCRBodySize

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in pkg/authserver/runner/embeddedauthserver.go and trace the unexported RunConfig-to-Config helpers, then compare their behavior with pkg/authserver/config.go and pkg/authserver/server_impl.go. Check the handler and DCR references in pkg/authserver/server/handlers/handler.go. Done means embedders can reuse the runner's complete construction and handler safeguards without duplicating translation logic.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
authentication, backend-api-design
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.