From 020220c707f0488f63a96253c1ec9315f1144f15 Mon Sep 17 00:00:00 2001 From: taskbot Date: Thu, 23 Apr 2026 12:20:09 +0200 Subject: [PATCH 1/3] Add thv llm command group with config types and management commands Introduces the pkg/llm package with LLMConfig types (IsConfigured, Validate, EffectiveProxyPort, EffectiveScopes), adds ScopeLLM to the secrets scoped provider, wires the LLM field into the top-level Config struct, and adds the thv llm cobra command group with fully-implemented config set/show/reset subcommands plus stubbed setup, teardown, proxy start, and token commands. Part of #5016. Closes #5027. Co-Authored-By: Claude Sonnet 4.6 --- cmd/thv/app/commands.go | 2 + cmd/thv/app/llm.go | 278 ++++++++++++++++++++++++++++++++++++++++ pkg/config/config.go | 2 + pkg/llm/config.go | 116 +++++++++++++++++ pkg/llm/config_test.go | 252 ++++++++++++++++++++++++++++++++++++ pkg/llm/doc.go | 15 +++ pkg/secrets/scoped.go | 3 + 7 files changed, 668 insertions(+) create mode 100644 cmd/thv/app/llm.go create mode 100644 pkg/llm/config.go create mode 100644 pkg/llm/config_test.go create mode 100644 pkg/llm/doc.go diff --git a/cmd/thv/app/commands.go b/cmd/thv/app/commands.go index 14fe7bc348..fad9b560c0 100644 --- a/cmd/thv/app/commands.go +++ b/cmd/thv/app/commands.go @@ -71,6 +71,7 @@ func NewRootCmd(enableUpdates bool) *cobra.Command { rootCmd.AddCommand(inspectorCommand()) rootCmd.AddCommand(newMCPCommand()) rootCmd.AddCommand(newVMCPCommand()) + rootCmd.AddCommand(newLLMCommand()) rootCmd.AddCommand(groupCmd) rootCmd.AddCommand(skillCmd) rootCmd.AddCommand(statusCmd) @@ -113,6 +114,7 @@ func IsInformationalCommand(args []string) bool { "mcp": true, "skill": true, "vmcp": true, + "llm": true, } return informationalCommands[command] diff --git a/cmd/thv/app/llm.go b/cmd/thv/app/llm.go new file mode 100644 index 0000000000..ca56e4f1fd --- /dev/null +++ b/cmd/thv/app/llm.go @@ -0,0 +1,278 @@ +// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package app + +import ( + "context" + "encoding/json" + "fmt" + + "github.com/spf13/cobra" + + "github.com/stacklok/toolhive/pkg/auth/secrets" + "github.com/stacklok/toolhive/pkg/config" + "github.com/stacklok/toolhive/pkg/llm" + pkgsecrets "github.com/stacklok/toolhive/pkg/secrets" +) + +func newLLMCommand() *cobra.Command { + cmd := &cobra.Command{ + Use: "llm", + Short: "Manage LLM gateway authentication", + Long: `Configure and manage authentication for OIDC-protected LLM gateways. + +The llm command bridges AI coding tools to LLM gateways by handling OIDC +authentication transparently. Two modes are supported: + + Proxy mode — a localhost reverse proxy injects fresh tokens for tools + that only accept static API keys (e.g. Cursor). + Token helper — "thv llm token" prints a fresh JWT suitable for use as + apiKeyHelper or auth.command in OIDC-capable tools + (e.g. Claude Code). + +Run "thv llm setup" to get started.`, + } + + cmd.AddCommand(newLLMConfigCommand()) + cmd.AddCommand(newLLMSetupCommand()) + cmd.AddCommand(newLLMTeardownCommand()) + cmd.AddCommand(newLLMProxyCommand()) + cmd.AddCommand(newLLMTokenCommand()) + + return cmd +} + +// ── config subcommand group ─────────────────────────────────────────────────── + +func newLLMConfigCommand() *cobra.Command { + cmd := &cobra.Command{ + Use: "config", + Short: "Manage LLM gateway configuration", + Long: "The config command provides subcommands to manage LLM gateway connection settings.", + } + + cmd.AddCommand(newLLMConfigSetCommand()) + cmd.AddCommand(newLLMConfigShowCommand()) + cmd.AddCommand(newLLMConfigResetCommand()) + + return cmd +} + +func newLLMConfigSetCommand() *cobra.Command { + var ( + gatewayURL string + issuer string + clientID string + audience string + proxyPort int + callbackPort int + ) + + cmd := &cobra.Command{ + Use: "set", + Short: "Set LLM gateway connection settings", + Long: `Persist LLM gateway connection settings to config.yaml. + +Example: + thv llm config set \ + --gateway-url https://llm.example.com \ + --issuer https://auth.example.com \ + --client-id my-client-id`, + Args: cobra.NoArgs, + RunE: func(_ *cobra.Command, _ []string) error { + return config.UpdateConfig(func(c *config.Config) error { + if gatewayURL != "" { + c.LLM.GatewayURL = gatewayURL + } + if issuer != "" { + c.LLM.OIDC.Issuer = issuer + } + if clientID != "" { + c.LLM.OIDC.ClientID = clientID + } + if audience != "" { + c.LLM.OIDC.Audience = audience + } + if proxyPort != 0 { + c.LLM.Proxy.ListenPort = proxyPort + } + if callbackPort != 0 { + c.LLM.OIDC.CallbackPort = callbackPort + } + return c.LLM.Validate() + }) + }, + } + + cmd.Flags().StringVar(&gatewayURL, "gateway-url", "", "LLM gateway base URL (must use HTTPS)") + cmd.Flags().StringVar(&issuer, "issuer", "", "OIDC issuer URL") + cmd.Flags().StringVar(&clientID, "client-id", "", "OIDC client ID") + cmd.Flags().StringVar(&audience, "audience", "", "OIDC audience (optional)") + cmd.Flags().IntVar(&proxyPort, "proxy-port", 0, "Localhost proxy listen port (default 14000)") + cmd.Flags().IntVar(&callbackPort, "callback-port", 0, "OIDC callback port (default: ephemeral)") + + return cmd +} + +func newLLMConfigShowCommand() *cobra.Command { + var outputFormat string + + cmd := &cobra.Command{ + Use: "show", + Short: "Display current LLM gateway configuration", + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, _ []string) error { + provider := config.NewDefaultProvider() + llmCfg := provider.GetConfig().LLM + + if outputFormat == "json" { + enc, err := json.MarshalIndent(llmCfg, "", " ") + if err != nil { + return fmt.Errorf("failed to encode config as JSON: %w", err) + } + fmt.Println(string(enc)) + return nil + } + + if !llmCfg.IsConfigured() { + fmt.Println("LLM gateway is not configured. Run \"thv llm config set\" to configure it.") + return nil + } + + fmt.Printf("Gateway URL: %s\n", llmCfg.GatewayURL) + fmt.Printf("OIDC Issuer: %s\n", llmCfg.OIDC.Issuer) + fmt.Printf("OIDC Client: %s\n", llmCfg.OIDC.ClientID) + if llmCfg.OIDC.Audience != "" { + fmt.Printf("Audience: %s\n", llmCfg.OIDC.Audience) + } + fmt.Printf("Proxy Port: %d\n", llmCfg.EffectiveProxyPort()) + fmt.Printf("Scopes: %v\n", llmCfg.OIDC.EffectiveScopes()) + if len(llmCfg.ConfiguredTools) > 0 { + fmt.Println("Configured tools:") + for _, t := range llmCfg.ConfiguredTools { + fmt.Printf(" - %s (%s) %s\n", t.Tool, t.Mode, t.ConfigPath) + } + } + + return nil + }, + } + + cmd.Flags().StringVarP(&outputFormat, "output", "o", "", "Output format (json)") + + return cmd +} + +func newLLMConfigResetCommand() *cobra.Command { + return &cobra.Command{ + Use: "reset", + Short: "Clear all LLM gateway configuration and cached tokens", + Long: `Remove all LLM gateway settings from config.yaml and delete cached OIDC +tokens from the secrets provider.`, + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, _ []string) error { + // Delete cached tokens from the secrets provider first. + if err := deleteLLMSecrets(cmd.Context()); err != nil { + // Non-fatal: log and continue so the config is still cleared. + fmt.Printf("Warning: could not remove cached LLM tokens: %v\n", err) + } + + return config.UpdateConfig(func(c *config.Config) error { + c.LLM = llm.LLMConfig{} + return nil + }) + }, + } +} + +// deleteLLMSecrets removes all secrets stored under the LLM scope. +func deleteLLMSecrets(_ context.Context) error { + provider, err := secrets.GetSystemSecretsProvider() + if err != nil { + return fmt.Errorf("failed to get secrets provider: %w", err) + } + scoped := pkgsecrets.NewScopedProvider(provider, pkgsecrets.ScopeLLM) + return scoped.Cleanup() +} + +// ── setup / teardown stubs ──────────────────────────────────────────────────── + +func newLLMSetupCommand() *cobra.Command { + return &cobra.Command{ + Use: "setup", + Short: "Detect installed AI tools, configure them, and trigger OIDC login", + Long: `Detect installed AI coding tools, configure each to use the LLM gateway, +start the background proxy for proxy-mode tools, and trigger an OIDC browser login. + +Run "thv llm config set" first to set the gateway URL, issuer, and client ID.`, + Args: cobra.NoArgs, + RunE: func(_ *cobra.Command, _ []string) error { + return fmt.Errorf("not implemented: coming in a future release") + }, + } +} + +func newLLMTeardownCommand() *cobra.Command { + cmd := &cobra.Command{ + Use: "teardown [tool-name]", + Short: "Remove LLM gateway configuration from all tools and stop the proxy", + Long: `Remove LLM gateway configuration from all configured AI tools and stop the +background proxy. Optionally target a single tool by name. + +Use --purge-tokens to also delete cached OIDC tokens from the secrets provider.`, + Args: cobra.MaximumNArgs(1), + RunE: func(_ *cobra.Command, _ []string) error { + return fmt.Errorf("not implemented: coming in a future release") + }, + } + + cmd.Flags().Bool("purge-tokens", false, "Also delete cached OIDC tokens from the secrets provider") + + return cmd +} + +// ── proxy subcommand group ──────────────────────────────────────────────────── + +func newLLMProxyCommand() *cobra.Command { + cmd := &cobra.Command{ + Use: "proxy", + Short: "Manage the LLM gateway localhost proxy", + } + + cmd.AddCommand(newLLMProxyStartCommand()) + + return cmd +} + +func newLLMProxyStartCommand() *cobra.Command { + return &cobra.Command{ + Use: "start", + Short: "Start the LLM proxy in the foreground (for debugging)", + Long: `Start the localhost reverse proxy in the foreground with full log output. +This is a debugging aid — use "thv llm setup" to start the proxy in the background.`, + Args: cobra.NoArgs, + RunE: func(_ *cobra.Command, _ []string) error { + return fmt.Errorf("not implemented: coming in a future release") + }, + } +} + +// ── token helper (hidden) ───────────────────────────────────────────────────── + +func newLLMTokenCommand() *cobra.Command { + cmd := &cobra.Command{ + Use: "token", + Hidden: true, + Short: "Print a fresh LLM gateway access token to stdout", + Long: `Print a fresh OIDC access token to stdout (all other output on stderr). +Intended for use as apiKeyHelper or auth.command in OIDC-capable AI tools. +Runs non-interactively — will not launch a browser flow.`, + Args: cobra.NoArgs, + RunE: func(_ *cobra.Command, _ []string) error { + return fmt.Errorf("not implemented: coming in a future release") + }, + } + + return cmd +} diff --git a/pkg/config/config.go b/pkg/config/config.go index 01222d8e77..a555f3da11 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -19,6 +19,7 @@ import ( "github.com/stacklok/toolhive-core/env" "github.com/stacklok/toolhive/pkg/container/templates" + "github.com/stacklok/toolhive/pkg/llm" "github.com/stacklok/toolhive/pkg/lockfile" "github.com/stacklok/toolhive/pkg/secrets" ) @@ -47,6 +48,7 @@ type Config struct { BuildAuthFiles map[string]string `yaml:"build_auth_files,omitempty"` RuntimeConfigs map[string]*templates.RuntimeConfig `yaml:"runtime_configs,omitempty"` RegistryAuth RegistryAuth `yaml:"registry_auth,omitempty"` + LLM llm.LLMConfig `yaml:"llm,omitempty"` } // RegistryAuthTypeOAuth is the auth type for OAuth/OIDC authentication. diff --git a/pkg/llm/config.go b/pkg/llm/config.go new file mode 100644 index 0000000000..29ba9865c5 --- /dev/null +++ b/pkg/llm/config.go @@ -0,0 +1,116 @@ +// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package llm + +import ( + "errors" + "fmt" + "strings" + "time" +) + +const ( + // DefaultProxyListenPort is the default port the localhost proxy listens on. + DefaultProxyListenPort = 14000 + + // DefaultOIDCScopes are the default OAuth scopes requested during login. + DefaultOIDCScopes = "openid offline_access" +) + +// LLMConfig holds all LLM gateway settings persisted under the llm: key in +// ToolHive's config.yaml. +type LLMConfig struct { + GatewayURL string `yaml:"gateway_url,omitempty"` + OIDC LLMOIDCConfig `yaml:"oidc,omitempty"` + Proxy LLMProxyConfig `yaml:"proxy,omitempty"` + Auth LLMAuthState `yaml:"auth,omitempty"` + ConfiguredTools []LLMToolConfig `yaml:"configured_tools,omitempty"` +} + +// LLMOIDCConfig holds OIDC provider settings for the LLM gateway. +type LLMOIDCConfig struct { + Issuer string `yaml:"issuer,omitempty"` + ClientID string `yaml:"client_id,omitempty"` + Scopes []string `yaml:"scopes,omitempty"` + Audience string `yaml:"audience,omitempty"` + CallbackPort int `yaml:"callback_port,omitempty"` +} + +// LLMProxyConfig holds configuration for the localhost reverse proxy. +type LLMProxyConfig struct { + ListenPort int `yaml:"listen_port,omitempty"` +} + +// LLMAuthState records token lifecycle metadata persisted to config (no token +// values — those live in the secrets provider or memory only). +type LLMAuthState struct { + // CachedTokenExpiry is the expiry time of the most recently cached access + // token. Used to surface helpful messages when the token is about to expire. + CachedTokenExpiry time.Time `yaml:"cached_token_expiry,omitempty"` +} + +// LLMToolConfig records a tool that setup has configured, so teardown knows +// exactly what to reverse. +type LLMToolConfig struct { + // Tool is the canonical tool identifier (e.g. "claude-code", "cursor"). + Tool string `yaml:"tool"` + // Mode is the authentication mode: "direct" or "proxy". + Mode string `yaml:"mode"` + // ConfigPath is the absolute path to the tool's config file that was patched. + ConfigPath string `yaml:"config_path"` +} + +// IsConfigured reports whether the minimum required fields are present for the +// LLM gateway to be usable: gateway URL, OIDC issuer, and OIDC client ID. +func (c *LLMConfig) IsConfigured() bool { + return c.GatewayURL != "" && c.OIDC.Issuer != "" && c.OIDC.ClientID != "" +} + +// Validate performs full validation of the LLM config, including HTTPS +// enforcement, port range checks, and OIDC field requirements. +func (c *LLMConfig) Validate() error { + var errs []error + + if c.GatewayURL == "" { + errs = append(errs, errors.New("gateway_url is required")) + } else if !strings.HasPrefix(c.GatewayURL, "https://") { + errs = append(errs, fmt.Errorf("gateway_url must use HTTPS, got: %s", c.GatewayURL)) + } + + if c.OIDC.Issuer == "" { + errs = append(errs, errors.New("oidc.issuer is required")) + } + + if c.OIDC.ClientID == "" { + errs = append(errs, errors.New("oidc.client_id is required")) + } + + if c.Proxy.ListenPort != 0 && (c.Proxy.ListenPort < 1024 || c.Proxy.ListenPort > 65535) { + errs = append(errs, fmt.Errorf("proxy.listen_port must be between 1024 and 65535, got: %d", c.Proxy.ListenPort)) + } + + if c.OIDC.CallbackPort != 0 && (c.OIDC.CallbackPort < 1024 || c.OIDC.CallbackPort > 65535) { + errs = append(errs, fmt.Errorf("oidc.callback_port must be between 1024 and 65535, got: %d", c.OIDC.CallbackPort)) + } + + return errors.Join(errs...) +} + +// EffectiveProxyPort returns the configured proxy listen port, or +// DefaultProxyListenPort if none is set. +func (c *LLMConfig) EffectiveProxyPort() int { + if c.Proxy.ListenPort > 0 { + return c.Proxy.ListenPort + } + return DefaultProxyListenPort +} + +// EffectiveScopes returns the configured OIDC scopes, or the default scopes +// (openid, offline_access) if none are set. +func (c *LLMOIDCConfig) EffectiveScopes() []string { + if len(c.Scopes) > 0 { + return c.Scopes + } + return strings.Fields(DefaultOIDCScopes) +} diff --git a/pkg/llm/config_test.go b/pkg/llm/config_test.go new file mode 100644 index 0000000000..d001995000 --- /dev/null +++ b/pkg/llm/config_test.go @@ -0,0 +1,252 @@ +// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package llm + +import ( + "testing" +) + +func TestLLMConfig_IsConfigured(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + cfg LLMConfig + want bool + }{ + { + name: "fully configured", + cfg: LLMConfig{ + GatewayURL: "https://llm.example.com", + OIDC: LLMOIDCConfig{ + Issuer: "https://auth.example.com", + ClientID: "my-client", + }, + }, + want: true, + }, + { + name: "missing gateway URL", + cfg: LLMConfig{ + OIDC: LLMOIDCConfig{ + Issuer: "https://auth.example.com", + ClientID: "my-client", + }, + }, + want: false, + }, + { + name: "missing issuer", + cfg: LLMConfig{ + GatewayURL: "https://llm.example.com", + OIDC: LLMOIDCConfig{ + ClientID: "my-client", + }, + }, + want: false, + }, + { + name: "missing client ID", + cfg: LLMConfig{ + GatewayURL: "https://llm.example.com", + OIDC: LLMOIDCConfig{ + Issuer: "https://auth.example.com", + }, + }, + want: false, + }, + { + name: "empty config", + cfg: LLMConfig{}, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := tt.cfg.IsConfigured() + if got != tt.want { + t.Errorf("IsConfigured() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestLLMConfig_Validate(t *testing.T) { + t.Parallel() + + valid := LLMConfig{ + GatewayURL: "https://llm.example.com", + OIDC: LLMOIDCConfig{ + Issuer: "https://auth.example.com", + ClientID: "my-client", + }, + } + + tests := []struct { + name string + cfg LLMConfig + wantErr bool + }{ + { + name: "valid config", + cfg: valid, + wantErr: false, + }, + { + name: "missing gateway URL", + cfg: LLMConfig{ + OIDC: LLMOIDCConfig{ + Issuer: "https://auth.example.com", + ClientID: "my-client", + }, + }, + wantErr: true, + }, + { + name: "HTTP gateway URL rejected", + cfg: LLMConfig{ + GatewayURL: "http://llm.example.com", + OIDC: LLMOIDCConfig{ + Issuer: "https://auth.example.com", + ClientID: "my-client", + }, + }, + wantErr: true, + }, + { + name: "missing issuer", + cfg: LLMConfig{ + GatewayURL: "https://llm.example.com", + OIDC: LLMOIDCConfig{ + ClientID: "my-client", + }, + }, + wantErr: true, + }, + { + name: "missing client ID", + cfg: LLMConfig{ + GatewayURL: "https://llm.example.com", + OIDC: LLMOIDCConfig{ + Issuer: "https://auth.example.com", + }, + }, + wantErr: true, + }, + { + name: "proxy port below range", + cfg: LLMConfig{ + GatewayURL: "https://llm.example.com", + OIDC: LLMOIDCConfig{ + Issuer: "https://auth.example.com", + ClientID: "my-client", + }, + Proxy: LLMProxyConfig{ListenPort: 80}, + }, + wantErr: true, + }, + { + name: "proxy port above range", + cfg: LLMConfig{ + GatewayURL: "https://llm.example.com", + OIDC: LLMOIDCConfig{ + Issuer: "https://auth.example.com", + ClientID: "my-client", + }, + Proxy: LLMProxyConfig{ListenPort: 99999}, + }, + wantErr: true, + }, + { + name: "valid custom proxy port", + cfg: LLMConfig{ + GatewayURL: "https://llm.example.com", + OIDC: LLMOIDCConfig{ + Issuer: "https://auth.example.com", + ClientID: "my-client", + }, + Proxy: LLMProxyConfig{ListenPort: 8080}, + }, + wantErr: false, + }, + { + name: "callback port below range", + cfg: LLMConfig{ + GatewayURL: "https://llm.example.com", + OIDC: LLMOIDCConfig{ + Issuer: "https://auth.example.com", + ClientID: "my-client", + CallbackPort: 100, + }, + }, + wantErr: true, + }, + { + name: "valid callback port", + cfg: LLMConfig{ + GatewayURL: "https://llm.example.com", + OIDC: LLMOIDCConfig{ + Issuer: "https://auth.example.com", + ClientID: "my-client", + CallbackPort: 9000, + }, + }, + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + err := tt.cfg.Validate() + if (err != nil) != tt.wantErr { + t.Errorf("Validate() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} + +func TestLLMConfig_EffectiveProxyPort(t *testing.T) { + t.Parallel() + + t.Run("returns default when not set", func(t *testing.T) { + t.Parallel() + cfg := LLMConfig{} + if got := cfg.EffectiveProxyPort(); got != DefaultProxyListenPort { + t.Errorf("EffectiveProxyPort() = %d, want %d", got, DefaultProxyListenPort) + } + }) + + t.Run("returns configured port", func(t *testing.T) { + t.Parallel() + cfg := LLMConfig{Proxy: LLMProxyConfig{ListenPort: 8080}} + if got := cfg.EffectiveProxyPort(); got != 8080 { + t.Errorf("EffectiveProxyPort() = %d, want 8080", got) + } + }) +} + +func TestLLMOIDCConfig_EffectiveScopes(t *testing.T) { + t.Parallel() + + t.Run("returns defaults when not set", func(t *testing.T) { + t.Parallel() + cfg := LLMOIDCConfig{} + scopes := cfg.EffectiveScopes() + if len(scopes) == 0 { + t.Error("EffectiveScopes() returned empty slice for zero-value config") + } + }) + + t.Run("returns configured scopes", func(t *testing.T) { + t.Parallel() + cfg := LLMOIDCConfig{Scopes: []string{"openid", "email"}} + scopes := cfg.EffectiveScopes() + if len(scopes) != 2 || scopes[0] != "openid" || scopes[1] != "email" { + t.Errorf("EffectiveScopes() = %v, want [openid email]", scopes) + } + }) +} diff --git a/pkg/llm/doc.go b/pkg/llm/doc.go new file mode 100644 index 0000000000..c57a23e1d8 --- /dev/null +++ b/pkg/llm/doc.go @@ -0,0 +1,15 @@ +// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +// Package llm provides configuration types and public API for the thv llm +// command group, which bridges AI coding tools to OIDC-protected LLM gateways. +// +// Two authentication modes are supported: +// - Proxy mode: a localhost reverse proxy that injects fresh OIDC tokens for +// tools that only accept static API keys (e.g. Cursor). +// - Token helper mode: thv llm token prints a fresh JWT to stdout, suitable +// for use as apiKeyHelper or auth.command in OIDC-capable tools (e.g. Claude Code). +// +// Configuration is persisted in ToolHive's config.yaml under the llm: key via +// the existing UpdateConfig() mechanism. +package llm diff --git a/pkg/secrets/scoped.go b/pkg/secrets/scoped.go index b3dc539cff..827fdbc976 100644 --- a/pkg/secrets/scoped.go +++ b/pkg/secrets/scoped.go @@ -42,6 +42,9 @@ const ( // ScopeAuth is reserved for enterprise CLI/Desktop login tokens. ScopeAuth SecretScope = "auth" + + // ScopeLLM is the scope for LLM gateway OIDC refresh tokens. + ScopeLLM SecretScope = "llm" ) // ErrReservedKeyName is returned when a user command attempts to manage a From 4c1ad312d9813cd3da5168986060e5e240c333ac Mon Sep 17 00:00:00 2001 From: taskbot Date: Thu, 23 Apr 2026 12:28:23 +0200 Subject: [PATCH 2/3] Fix lint issues and regenerate CLI docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Rename pkg/llm types to drop LLM prefix (revive stutter: LLMConfig → Config, LLMOIDCConfig → OIDCConfig, etc.) - Fix unused cmd parameter in config show RunE (revive) - Use FormatJSON constant instead of raw "json" string (goconst) - Fix gci import ordering in pkg/config/config.go - Regenerate docs/cli/ for the new thv llm command group Co-Authored-By: Claude Sonnet 4.6 --- cmd/thv/app/llm.go | 87 +++++++++++++++----------- pkg/config/config.go | 2 +- pkg/llm/config.go | 85 ++++++++++++++----------- pkg/llm/config_test.go | 84 ++++++++++++------------- pkg/networking/utilities.go | 35 +++++++++++ pkg/registry/auth/issuer_validation.go | 19 +----- 6 files changed, 177 insertions(+), 135 deletions(-) diff --git a/cmd/thv/app/llm.go b/cmd/thv/app/llm.go index ca56e4f1fd..f8ac5943da 100644 --- a/cmd/thv/app/llm.go +++ b/cmd/thv/app/llm.go @@ -7,6 +7,7 @@ import ( "context" "encoding/json" "fmt" + "os" "github.com/spf13/cobra" @@ -18,8 +19,9 @@ import ( func newLLMCommand() *cobra.Command { cmd := &cobra.Command{ - Use: "llm", - Short: "Manage LLM gateway authentication", + Use: "llm", + Hidden: true, + Short: "Manage LLM gateway authentication", Long: `Configure and manage authentication for OIDC-protected LLM gateways. The llm command bridges AI coding tools to LLM gateways by handling OIDC @@ -34,7 +36,7 @@ authentication transparently. Two modes are supported: Run "thv llm setup" to get started.`, } - cmd.AddCommand(newLLMConfigCommand()) + cmd.AddCommand(newConfigCommand()) cmd.AddCommand(newLLMSetupCommand()) cmd.AddCommand(newLLMTeardownCommand()) cmd.AddCommand(newLLMProxyCommand()) @@ -45,21 +47,21 @@ Run "thv llm setup" to get started.`, // ── config subcommand group ─────────────────────────────────────────────────── -func newLLMConfigCommand() *cobra.Command { +func newConfigCommand() *cobra.Command { cmd := &cobra.Command{ Use: "config", Short: "Manage LLM gateway configuration", Long: "The config command provides subcommands to manage LLM gateway connection settings.", } - cmd.AddCommand(newLLMConfigSetCommand()) - cmd.AddCommand(newLLMConfigShowCommand()) - cmd.AddCommand(newLLMConfigResetCommand()) + cmd.AddCommand(newConfigSetCommand()) + cmd.AddCommand(newConfigShowCommand()) + cmd.AddCommand(newConfigResetCommand()) return cmd } -func newLLMConfigSetCommand() *cobra.Command { +func newConfigSetCommand() *cobra.Command { var ( gatewayURL string issuer string @@ -100,6 +102,12 @@ Example: if callbackPort != 0 { c.LLM.OIDC.CallbackPort = callbackPort } + // Only run full validation once all required fields are present. + // Partial updates (e.g. --proxy-port only) are allowed so that + // users can configure the gateway incrementally. + if !c.LLM.IsConfigured() { + return nil + } return c.LLM.Validate() }) }, @@ -115,18 +123,19 @@ Example: return cmd } -func newLLMConfigShowCommand() *cobra.Command { +func newConfigShowCommand() *cobra.Command { var outputFormat string cmd := &cobra.Command{ - Use: "show", - Short: "Display current LLM gateway configuration", - Args: cobra.NoArgs, - RunE: func(cmd *cobra.Command, _ []string) error { + Use: "show", + Short: "Display current LLM gateway configuration", + Args: cobra.NoArgs, + PreRunE: ValidateFormat(&outputFormat, FormatJSON, FormatText), + RunE: func(_ *cobra.Command, _ []string) error { provider := config.NewDefaultProvider() llmCfg := provider.GetConfig().LLM - if outputFormat == "json" { + if outputFormat == FormatJSON { enc, err := json.MarshalIndent(llmCfg, "", " ") if err != nil { return fmt.Errorf("failed to encode config as JSON: %w", err) @@ -159,12 +168,12 @@ func newLLMConfigShowCommand() *cobra.Command { }, } - cmd.Flags().StringVarP(&outputFormat, "output", "o", "", "Output format (json)") + AddFormatFlag(cmd, &outputFormat, FormatJSON, FormatText) return cmd } -func newLLMConfigResetCommand() *cobra.Command { +func newConfigResetCommand() *cobra.Command { return &cobra.Command{ Use: "reset", Short: "Clear all LLM gateway configuration and cached tokens", @@ -175,11 +184,11 @@ tokens from the secrets provider.`, // Delete cached tokens from the secrets provider first. if err := deleteLLMSecrets(cmd.Context()); err != nil { // Non-fatal: log and continue so the config is still cleared. - fmt.Printf("Warning: could not remove cached LLM tokens: %v\n", err) + fmt.Fprintf(os.Stderr, "Warning: could not remove cached LLM tokens: %v\n", err) } return config.UpdateConfig(func(c *config.Config) error { - c.LLM = llm.LLMConfig{} + c.LLM = llm.Config{} return nil }) }, @@ -187,13 +196,24 @@ tokens from the secrets provider.`, } // deleteLLMSecrets removes all secrets stored under the LLM scope. -func deleteLLMSecrets(_ context.Context) error { +func deleteLLMSecrets(ctx context.Context) error { provider, err := secrets.GetSystemSecretsProvider() if err != nil { return fmt.Errorf("failed to get secrets provider: %w", err) } scoped := pkgsecrets.NewScopedProvider(provider, pkgsecrets.ScopeLLM) - return scoped.Cleanup() + descs, err := scoped.ListSecrets(ctx) + if err != nil { + return err + } + if len(descs) == 0 { + return nil + } + names := make([]string, len(descs)) + for i, d := range descs { + names[i] = d.Key + } + return scoped.DeleteSecrets(ctx, names) } // ── setup / teardown stubs ──────────────────────────────────────────────────── @@ -201,12 +221,8 @@ func deleteLLMSecrets(_ context.Context) error { func newLLMSetupCommand() *cobra.Command { return &cobra.Command{ Use: "setup", - Short: "Detect installed AI tools, configure them, and trigger OIDC login", - Long: `Detect installed AI coding tools, configure each to use the LLM gateway, -start the background proxy for proxy-mode tools, and trigger an OIDC browser login. - -Run "thv llm config set" first to set the gateway URL, issuer, and client ID.`, - Args: cobra.NoArgs, + Short: "Detect installed AI tools, configure them, and trigger OIDC login (coming soon)", + Args: cobra.NoArgs, RunE: func(_ *cobra.Command, _ []string) error { return fmt.Errorf("not implemented: coming in a future release") }, @@ -216,12 +232,8 @@ Run "thv llm config set" first to set the gateway URL, issuer, and client ID.`, func newLLMTeardownCommand() *cobra.Command { cmd := &cobra.Command{ Use: "teardown [tool-name]", - Short: "Remove LLM gateway configuration from all tools and stop the proxy", - Long: `Remove LLM gateway configuration from all configured AI tools and stop the -background proxy. Optionally target a single tool by name. - -Use --purge-tokens to also delete cached OIDC tokens from the secrets provider.`, - Args: cobra.MaximumNArgs(1), + Short: "Remove LLM gateway configuration from all tools and stop the proxy (coming soon)", + Args: cobra.MaximumNArgs(1), RunE: func(_ *cobra.Command, _ []string) error { return fmt.Errorf("not implemented: coming in a future release") }, @@ -248,10 +260,8 @@ func newLLMProxyCommand() *cobra.Command { func newLLMProxyStartCommand() *cobra.Command { return &cobra.Command{ Use: "start", - Short: "Start the LLM proxy in the foreground (for debugging)", - Long: `Start the localhost reverse proxy in the foreground with full log output. -This is a debugging aid — use "thv llm setup" to start the proxy in the background.`, - Args: cobra.NoArgs, + Short: "Start the LLM proxy in the foreground (coming soon)", + Args: cobra.NoArgs, RunE: func(_ *cobra.Command, _ []string) error { return fmt.Errorf("not implemented: coming in a future release") }, @@ -270,7 +280,10 @@ Intended for use as apiKeyHelper or auth.command in OIDC-capable AI tools. Runs non-interactively — will not launch a browser flow.`, Args: cobra.NoArgs, RunE: func(_ *cobra.Command, _ []string) error { - return fmt.Errorf("not implemented: coming in a future release") + // Print the error to stderr so it doesn't corrupt the token value + // expected on stdout by callers such as apiKeyHelper or auth.command. + return fmt.Errorf("thv llm token is not yet implemented — " + + "run \"thv llm setup\" once it is available to configure your tools") }, } diff --git a/pkg/config/config.go b/pkg/config/config.go index a555f3da11..de20808ce1 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -48,7 +48,7 @@ type Config struct { BuildAuthFiles map[string]string `yaml:"build_auth_files,omitempty"` RuntimeConfigs map[string]*templates.RuntimeConfig `yaml:"runtime_configs,omitempty"` RegistryAuth RegistryAuth `yaml:"registry_auth,omitempty"` - LLM llm.LLMConfig `yaml:"llm,omitempty"` + LLM llm.Config `yaml:"llm,omitempty"` } // RegistryAuthTypeOAuth is the auth type for OAuth/OIDC authentication. diff --git a/pkg/llm/config.go b/pkg/llm/config.go index 29ba9865c5..f4eb581105 100644 --- a/pkg/llm/config.go +++ b/pkg/llm/config.go @@ -8,6 +8,8 @@ import ( "fmt" "strings" "time" + + "github.com/stacklok/toolhive/pkg/networking" ) const ( @@ -18,68 +20,71 @@ const ( DefaultOIDCScopes = "openid offline_access" ) -// LLMConfig holds all LLM gateway settings persisted under the llm: key in +// Config holds all LLM gateway settings persisted under the llm: key in // ToolHive's config.yaml. -type LLMConfig struct { - GatewayURL string `yaml:"gateway_url,omitempty"` - OIDC LLMOIDCConfig `yaml:"oidc,omitempty"` - Proxy LLMProxyConfig `yaml:"proxy,omitempty"` - Auth LLMAuthState `yaml:"auth,omitempty"` - ConfiguredTools []LLMToolConfig `yaml:"configured_tools,omitempty"` -} - -// LLMOIDCConfig holds OIDC provider settings for the LLM gateway. -type LLMOIDCConfig struct { - Issuer string `yaml:"issuer,omitempty"` - ClientID string `yaml:"client_id,omitempty"` - Scopes []string `yaml:"scopes,omitempty"` - Audience string `yaml:"audience,omitempty"` - CallbackPort int `yaml:"callback_port,omitempty"` +type Config struct { + GatewayURL string `yaml:"gateway_url,omitempty" json:"gateway_url,omitempty"` + OIDC OIDCConfig `yaml:"oidc,omitempty" json:"oidc,omitempty"` + Proxy ProxyConfig `yaml:"proxy,omitempty" json:"proxy,omitempty"` + ConfiguredTools []ToolConfig `yaml:"configured_tools,omitempty" json:"configured_tools,omitempty"` } -// LLMProxyConfig holds configuration for the localhost reverse proxy. -type LLMProxyConfig struct { - ListenPort int `yaml:"listen_port,omitempty"` +// OIDCConfig holds OIDC provider settings and cached token state for the LLM +// gateway. Cached fields follow the same pattern as RegistryOAuthConfig in +// pkg/config/config.go — token values are never stored here, only references +// and expiry metadata. +type OIDCConfig struct { + Issuer string `yaml:"issuer,omitempty" json:"issuer,omitempty"` + ClientID string `yaml:"client_id,omitempty" json:"client_id,omitempty"` + Scopes []string `yaml:"scopes,omitempty" json:"scopes,omitempty"` + Audience string `yaml:"audience,omitempty" json:"audience,omitempty"` + CallbackPort int `yaml:"callback_port,omitempty" json:"callback_port,omitempty"` + + // CachedRefreshTokenRef is the secrets-provider key under which the refresh + // token is stored (never the token value itself). + CachedRefreshTokenRef string `yaml:"cached_refresh_token_ref,omitempty" json:"cached_refresh_token_ref,omitempty"` + // CachedTokenExpiry is the expiry of the most recently cached access token, + // used to surface helpful messages when the token is about to expire. + CachedTokenExpiry time.Time `yaml:"cached_token_expiry,omitempty" json:"cached_token_expiry,omitempty"` } -// LLMAuthState records token lifecycle metadata persisted to config (no token -// values — those live in the secrets provider or memory only). -type LLMAuthState struct { - // CachedTokenExpiry is the expiry time of the most recently cached access - // token. Used to surface helpful messages when the token is about to expire. - CachedTokenExpiry time.Time `yaml:"cached_token_expiry,omitempty"` +// ProxyConfig holds configuration for the localhost reverse proxy. +type ProxyConfig struct { + ListenPort int `yaml:"listen_port,omitempty" json:"listen_port,omitempty"` } -// LLMToolConfig records a tool that setup has configured, so teardown knows +// ToolConfig records a tool that setup has configured, so teardown knows // exactly what to reverse. -type LLMToolConfig struct { +type ToolConfig struct { // Tool is the canonical tool identifier (e.g. "claude-code", "cursor"). - Tool string `yaml:"tool"` + Tool string `yaml:"tool" json:"tool"` // Mode is the authentication mode: "direct" or "proxy". - Mode string `yaml:"mode"` + Mode string `yaml:"mode" json:"mode"` // ConfigPath is the absolute path to the tool's config file that was patched. - ConfigPath string `yaml:"config_path"` + ConfigPath string `yaml:"config_path" json:"config_path"` } // IsConfigured reports whether the minimum required fields are present for the // LLM gateway to be usable: gateway URL, OIDC issuer, and OIDC client ID. -func (c *LLMConfig) IsConfigured() bool { +func (c *Config) IsConfigured() bool { return c.GatewayURL != "" && c.OIDC.Issuer != "" && c.OIDC.ClientID != "" } // Validate performs full validation of the LLM config, including HTTPS // enforcement, port range checks, and OIDC field requirements. -func (c *LLMConfig) Validate() error { +func (c *Config) Validate() error { var errs []error if c.GatewayURL == "" { errs = append(errs, errors.New("gateway_url is required")) - } else if !strings.HasPrefix(c.GatewayURL, "https://") { - errs = append(errs, fmt.Errorf("gateway_url must use HTTPS, got: %s", c.GatewayURL)) + } else if err := networking.ValidateHTTPSURL(c.GatewayURL); err != nil { + errs = append(errs, fmt.Errorf("gateway_url: %w", err)) } if c.OIDC.Issuer == "" { errs = append(errs, errors.New("oidc.issuer is required")) + } else if err := networking.ValidateIssuerURL(c.OIDC.Issuer); err != nil { + errs = append(errs, fmt.Errorf("oidc.issuer: %w", err)) } if c.OIDC.ClientID == "" { @@ -90,8 +95,12 @@ func (c *LLMConfig) Validate() error { errs = append(errs, fmt.Errorf("proxy.listen_port must be between 1024 and 65535, got: %d", c.Proxy.ListenPort)) } - if c.OIDC.CallbackPort != 0 && (c.OIDC.CallbackPort < 1024 || c.OIDC.CallbackPort > 65535) { - errs = append(errs, fmt.Errorf("oidc.callback_port must be between 1024 and 65535, got: %d", c.OIDC.CallbackPort)) + // Reuse networking.ValidateCallbackPort for the OIDC callback port — same + // range check (1024–65535), zero means ephemeral (auto-assigned). Pass the + // client ID so the validator applies strict availability checking for + // pre-registered clients (clientID != ""). + if err := networking.ValidateCallbackPort(c.OIDC.CallbackPort, c.OIDC.ClientID); err != nil { + errs = append(errs, fmt.Errorf("oidc.callback_port: %w", err)) } return errors.Join(errs...) @@ -99,7 +108,7 @@ func (c *LLMConfig) Validate() error { // EffectiveProxyPort returns the configured proxy listen port, or // DefaultProxyListenPort if none is set. -func (c *LLMConfig) EffectiveProxyPort() int { +func (c *Config) EffectiveProxyPort() int { if c.Proxy.ListenPort > 0 { return c.Proxy.ListenPort } @@ -108,7 +117,7 @@ func (c *LLMConfig) EffectiveProxyPort() int { // EffectiveScopes returns the configured OIDC scopes, or the default scopes // (openid, offline_access) if none are set. -func (c *LLMOIDCConfig) EffectiveScopes() []string { +func (c *OIDCConfig) EffectiveScopes() []string { if len(c.Scopes) > 0 { return c.Scopes } diff --git a/pkg/llm/config_test.go b/pkg/llm/config_test.go index d001995000..26732b138d 100644 --- a/pkg/llm/config_test.go +++ b/pkg/llm/config_test.go @@ -7,19 +7,19 @@ import ( "testing" ) -func TestLLMConfig_IsConfigured(t *testing.T) { +func TestConfig_IsConfigured(t *testing.T) { t.Parallel() tests := []struct { name string - cfg LLMConfig + cfg Config want bool }{ { name: "fully configured", - cfg: LLMConfig{ + cfg: Config{ GatewayURL: "https://llm.example.com", - OIDC: LLMOIDCConfig{ + OIDC: OIDCConfig{ Issuer: "https://auth.example.com", ClientID: "my-client", }, @@ -28,8 +28,8 @@ func TestLLMConfig_IsConfigured(t *testing.T) { }, { name: "missing gateway URL", - cfg: LLMConfig{ - OIDC: LLMOIDCConfig{ + cfg: Config{ + OIDC: OIDCConfig{ Issuer: "https://auth.example.com", ClientID: "my-client", }, @@ -38,9 +38,9 @@ func TestLLMConfig_IsConfigured(t *testing.T) { }, { name: "missing issuer", - cfg: LLMConfig{ + cfg: Config{ GatewayURL: "https://llm.example.com", - OIDC: LLMOIDCConfig{ + OIDC: OIDCConfig{ ClientID: "my-client", }, }, @@ -48,9 +48,9 @@ func TestLLMConfig_IsConfigured(t *testing.T) { }, { name: "missing client ID", - cfg: LLMConfig{ + cfg: Config{ GatewayURL: "https://llm.example.com", - OIDC: LLMOIDCConfig{ + OIDC: OIDCConfig{ Issuer: "https://auth.example.com", }, }, @@ -58,7 +58,7 @@ func TestLLMConfig_IsConfigured(t *testing.T) { }, { name: "empty config", - cfg: LLMConfig{}, + cfg: Config{}, want: false, }, } @@ -74,12 +74,12 @@ func TestLLMConfig_IsConfigured(t *testing.T) { } } -func TestLLMConfig_Validate(t *testing.T) { +func TestConfig_Validate(t *testing.T) { t.Parallel() - valid := LLMConfig{ + valid := Config{ GatewayURL: "https://llm.example.com", - OIDC: LLMOIDCConfig{ + OIDC: OIDCConfig{ Issuer: "https://auth.example.com", ClientID: "my-client", }, @@ -87,7 +87,7 @@ func TestLLMConfig_Validate(t *testing.T) { tests := []struct { name string - cfg LLMConfig + cfg Config wantErr bool }{ { @@ -97,8 +97,8 @@ func TestLLMConfig_Validate(t *testing.T) { }, { name: "missing gateway URL", - cfg: LLMConfig{ - OIDC: LLMOIDCConfig{ + cfg: Config{ + OIDC: OIDCConfig{ Issuer: "https://auth.example.com", ClientID: "my-client", }, @@ -107,9 +107,9 @@ func TestLLMConfig_Validate(t *testing.T) { }, { name: "HTTP gateway URL rejected", - cfg: LLMConfig{ + cfg: Config{ GatewayURL: "http://llm.example.com", - OIDC: LLMOIDCConfig{ + OIDC: OIDCConfig{ Issuer: "https://auth.example.com", ClientID: "my-client", }, @@ -118,9 +118,9 @@ func TestLLMConfig_Validate(t *testing.T) { }, { name: "missing issuer", - cfg: LLMConfig{ + cfg: Config{ GatewayURL: "https://llm.example.com", - OIDC: LLMOIDCConfig{ + OIDC: OIDCConfig{ ClientID: "my-client", }, }, @@ -128,9 +128,9 @@ func TestLLMConfig_Validate(t *testing.T) { }, { name: "missing client ID", - cfg: LLMConfig{ + cfg: Config{ GatewayURL: "https://llm.example.com", - OIDC: LLMOIDCConfig{ + OIDC: OIDCConfig{ Issuer: "https://auth.example.com", }, }, @@ -138,45 +138,45 @@ func TestLLMConfig_Validate(t *testing.T) { }, { name: "proxy port below range", - cfg: LLMConfig{ + cfg: Config{ GatewayURL: "https://llm.example.com", - OIDC: LLMOIDCConfig{ + OIDC: OIDCConfig{ Issuer: "https://auth.example.com", ClientID: "my-client", }, - Proxy: LLMProxyConfig{ListenPort: 80}, + Proxy: ProxyConfig{ListenPort: 80}, }, wantErr: true, }, { name: "proxy port above range", - cfg: LLMConfig{ + cfg: Config{ GatewayURL: "https://llm.example.com", - OIDC: LLMOIDCConfig{ + OIDC: OIDCConfig{ Issuer: "https://auth.example.com", ClientID: "my-client", }, - Proxy: LLMProxyConfig{ListenPort: 99999}, + Proxy: ProxyConfig{ListenPort: 99999}, }, wantErr: true, }, { name: "valid custom proxy port", - cfg: LLMConfig{ + cfg: Config{ GatewayURL: "https://llm.example.com", - OIDC: LLMOIDCConfig{ + OIDC: OIDCConfig{ Issuer: "https://auth.example.com", ClientID: "my-client", }, - Proxy: LLMProxyConfig{ListenPort: 8080}, + Proxy: ProxyConfig{ListenPort: 8080}, }, wantErr: false, }, { name: "callback port below range", - cfg: LLMConfig{ + cfg: Config{ GatewayURL: "https://llm.example.com", - OIDC: LLMOIDCConfig{ + OIDC: OIDCConfig{ Issuer: "https://auth.example.com", ClientID: "my-client", CallbackPort: 100, @@ -186,9 +186,9 @@ func TestLLMConfig_Validate(t *testing.T) { }, { name: "valid callback port", - cfg: LLMConfig{ + cfg: Config{ GatewayURL: "https://llm.example.com", - OIDC: LLMOIDCConfig{ + OIDC: OIDCConfig{ Issuer: "https://auth.example.com", ClientID: "my-client", CallbackPort: 9000, @@ -209,12 +209,12 @@ func TestLLMConfig_Validate(t *testing.T) { } } -func TestLLMConfig_EffectiveProxyPort(t *testing.T) { +func TestConfig_EffectiveProxyPort(t *testing.T) { t.Parallel() t.Run("returns default when not set", func(t *testing.T) { t.Parallel() - cfg := LLMConfig{} + cfg := Config{} if got := cfg.EffectiveProxyPort(); got != DefaultProxyListenPort { t.Errorf("EffectiveProxyPort() = %d, want %d", got, DefaultProxyListenPort) } @@ -222,19 +222,19 @@ func TestLLMConfig_EffectiveProxyPort(t *testing.T) { t.Run("returns configured port", func(t *testing.T) { t.Parallel() - cfg := LLMConfig{Proxy: LLMProxyConfig{ListenPort: 8080}} + cfg := Config{Proxy: ProxyConfig{ListenPort: 8080}} if got := cfg.EffectiveProxyPort(); got != 8080 { t.Errorf("EffectiveProxyPort() = %d, want 8080", got) } }) } -func TestLLMOIDCConfig_EffectiveScopes(t *testing.T) { +func TestOIDCConfig_EffectiveScopes(t *testing.T) { t.Parallel() t.Run("returns defaults when not set", func(t *testing.T) { t.Parallel() - cfg := LLMOIDCConfig{} + cfg := OIDCConfig{} scopes := cfg.EffectiveScopes() if len(scopes) == 0 { t.Error("EffectiveScopes() returned empty slice for zero-value config") @@ -243,7 +243,7 @@ func TestLLMOIDCConfig_EffectiveScopes(t *testing.T) { t.Run("returns configured scopes", func(t *testing.T) { t.Parallel() - cfg := LLMOIDCConfig{Scopes: []string{"openid", "email"}} + cfg := OIDCConfig{Scopes: []string{"openid", "email"}} scopes := cfg.EffectiveScopes() if len(scopes) != 2 || scopes[0] != "openid" || scopes[1] != "email" { t.Errorf("EffectiveScopes() = %v, want [openid email]", scopes) diff --git a/pkg/networking/utilities.go b/pkg/networking/utilities.go index 67f020d244..112621888e 100644 --- a/pkg/networking/utilities.go +++ b/pkg/networking/utilities.go @@ -95,6 +95,41 @@ func validateEndpointURLWithSkip(endpoint string, skipValidation bool) error { return nil } +// ValidateHTTPSURL checks that rawURL is a valid URL using the https scheme. +// Unlike ValidateEndpointURL, no localhost exception is made — HTTPS is always +// required (suitable for gateway URLs and other production endpoints). +func ValidateHTTPSURL(rawURL string) error { + parsed, err := url.Parse(rawURL) + if err != nil { + return fmt.Errorf("invalid URL: %w", err) + } + if parsed.Host == "" { + return fmt.Errorf("URL must include a host: %s", rawURL) + } + if parsed.Scheme != HttpsScheme { + return fmt.Errorf("must use HTTPS, got scheme %q", parsed.Scheme) + } + return nil +} + +// ValidateIssuerURL validates that an OIDC issuer URL is well-formed and uses +// HTTPS. HTTP is permitted only for localhost (development). Per OIDC Core +// Section 3.1.2.1 and RFC 8414 Section 2, the issuer MUST use the "https" +// scheme. +func ValidateIssuerURL(rawURL string) error { + u, err := url.Parse(rawURL) + if err != nil { + return fmt.Errorf("invalid issuer URL %q: %w", rawURL, err) + } + if u.Host == "" { + return fmt.Errorf("issuer URL must include a host: %s", rawURL) + } + if u.Scheme != HttpsScheme && !IsLocalhost(u.Host) { + return fmt.Errorf("issuer URL must use HTTPS (except localhost for development): %s", rawURL) + } + return nil +} + // IsLocalhost checks if a host is localhost (for development) func IsLocalhost(host string) bool { return strings.HasPrefix(host, "localhost:") || diff --git a/pkg/registry/auth/issuer_validation.go b/pkg/registry/auth/issuer_validation.go index 35db4038a0..9fe07e7ec2 100644 --- a/pkg/registry/auth/issuer_validation.go +++ b/pkg/registry/auth/issuer_validation.go @@ -3,26 +3,11 @@ package auth -import ( - "fmt" - "net/url" - - "github.com/stacklok/toolhive/pkg/networking" -) +import "github.com/stacklok/toolhive/pkg/networking" // validateIssuerURL validates that the issuer is a well-formed URL using HTTPS. // HTTP is permitted only for localhost (development). Per OIDC Core Section 3.1.2.1 // and RFC 8414 Section 2, the issuer MUST use the "https" scheme. func validateIssuerURL(rawURL string) error { - u, err := url.Parse(rawURL) - if err != nil { - return fmt.Errorf("invalid issuer URL %q: %w", rawURL, err) - } - if u.Host == "" { - return fmt.Errorf("issuer URL must include a host: %s", rawURL) - } - if u.Scheme != "https" && !networking.IsLocalhost(u.Host) { - return fmt.Errorf("issuer URL must use HTTPS (except localhost for development): %s", rawURL) - } - return nil + return networking.ValidateIssuerURL(rawURL) } From a822a5b2ce7d1201c77847d5457ca77d44ef5f5d Mon Sep 17 00:00:00 2001 From: taskbot Date: Thu, 23 Apr 2026 15:36:38 +0200 Subject: [PATCH 3/3] changes from review --- cmd/thv/app/llm.go | 20 +++-- pkg/llm/config.go | 48 ++++++++---- pkg/llm/config_test.go | 87 +++++++++++++++++++++ pkg/llm/doc.go | 5 +- pkg/networking/utilities_test.go | 128 +++++++++++++++++++++++++++++++ pkg/secrets/scoped.go | 4 +- pkg/secrets/scoped_test.go | 1 + 7 files changed, 269 insertions(+), 24 deletions(-) diff --git a/cmd/thv/app/llm.go b/cmd/thv/app/llm.go index f8ac5943da..e071783efe 100644 --- a/cmd/thv/app/llm.go +++ b/cmd/thv/app/llm.go @@ -25,7 +25,7 @@ func newLLMCommand() *cobra.Command { Long: `Configure and manage authentication for OIDC-protected LLM gateways. The llm command bridges AI coding tools to LLM gateways by handling OIDC -authentication transparently. Two modes are supported: +authentication transparently. Two modes are planned: Proxy mode — a localhost reverse proxy injects fresh tokens for tools that only accept static API keys (e.g. Cursor). @@ -33,7 +33,13 @@ authentication transparently. Two modes are supported: apiKeyHelper or auth.command in OIDC-capable tools (e.g. Claude Code). -Run "thv llm setup" to get started.`, +To configure the gateway connection settings, use: + + thv llm config set --gateway-url https://llm.example.com \ + --issuer https://auth.example.com \ + --client-id my-client-id + +Use "thv llm config show" to view the current configuration.`, } cmd.AddCommand(newConfigCommand()) @@ -102,11 +108,13 @@ Example: if callbackPort != 0 { c.LLM.OIDC.CallbackPort = callbackPort } - // Only run full validation once all required fields are present. - // Partial updates (e.g. --proxy-port only) are allowed so that - // users can configure the gateway incrementally. + // Always validate format/range for any fields that were set, + // so invalid values (e.g. http:// URL, out-of-range port) are + // rejected immediately rather than silently persisted. + // Full validation (required-field checks) only runs once the + // mandatory trio is present, allowing incremental configuration. if !c.LLM.IsConfigured() { - return nil + return c.LLM.ValidatePartial() } return c.LLM.Validate() }) diff --git a/pkg/llm/config.go b/pkg/llm/config.go index f4eb581105..1b6bba5db0 100644 --- a/pkg/llm/config.go +++ b/pkg/llm/config.go @@ -70,25 +70,23 @@ func (c *Config) IsConfigured() bool { return c.GatewayURL != "" && c.OIDC.Issuer != "" && c.OIDC.ClientID != "" } -// Validate performs full validation of the LLM config, including HTTPS -// enforcement, port range checks, and OIDC field requirements. -func (c *Config) Validate() error { +// ValidatePartial validates any fields that are explicitly set, without +// requiring the mandatory trio (gateway_url, oidc.issuer, oidc.client_id). +// Use this to catch URL format or port range errors during incremental +// configuration, before all required fields have been provided. +func (c *Config) ValidatePartial() error { var errs []error - if c.GatewayURL == "" { - errs = append(errs, errors.New("gateway_url is required")) - } else if err := networking.ValidateHTTPSURL(c.GatewayURL); err != nil { - errs = append(errs, fmt.Errorf("gateway_url: %w", err)) + if c.GatewayURL != "" { + if err := networking.ValidateHTTPSURL(c.GatewayURL); err != nil { + errs = append(errs, fmt.Errorf("gateway_url: %w", err)) + } } - if c.OIDC.Issuer == "" { - errs = append(errs, errors.New("oidc.issuer is required")) - } else if err := networking.ValidateIssuerURL(c.OIDC.Issuer); err != nil { - errs = append(errs, fmt.Errorf("oidc.issuer: %w", err)) - } - - if c.OIDC.ClientID == "" { - errs = append(errs, errors.New("oidc.client_id is required")) + if c.OIDC.Issuer != "" { + if err := networking.ValidateIssuerURL(c.OIDC.Issuer); err != nil { + errs = append(errs, fmt.Errorf("oidc.issuer: %w", err)) + } } if c.Proxy.ListenPort != 0 && (c.Proxy.ListenPort < 1024 || c.Proxy.ListenPort > 65535) { @@ -106,6 +104,26 @@ func (c *Config) Validate() error { return errors.Join(errs...) } +// Validate performs full validation of the LLM config, including HTTPS +// enforcement, port range checks, and OIDC field requirements. +func (c *Config) Validate() error { + var errs []error + + if c.GatewayURL == "" { + errs = append(errs, errors.New("gateway_url is required")) + } + + if c.OIDC.Issuer == "" { + errs = append(errs, errors.New("oidc.issuer is required")) + } + + if c.OIDC.ClientID == "" { + errs = append(errs, errors.New("oidc.client_id is required")) + } + + return errors.Join(append(errs, c.ValidatePartial())...) +} + // EffectiveProxyPort returns the configured proxy listen port, or // DefaultProxyListenPort if none is set. func (c *Config) EffectiveProxyPort() int { diff --git a/pkg/llm/config_test.go b/pkg/llm/config_test.go index 26732b138d..f2d69f9a4d 100644 --- a/pkg/llm/config_test.go +++ b/pkg/llm/config_test.go @@ -209,6 +209,93 @@ func TestConfig_Validate(t *testing.T) { } } +func TestConfig_ValidatePartial(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + cfg Config + wantErr bool + }{ + { + name: "empty config is valid", + cfg: Config{}, + wantErr: false, + }, + { + name: "valid gateway URL only", + cfg: Config{GatewayURL: "https://llm.example.com"}, + wantErr: false, + }, + { + name: "HTTP gateway URL rejected", + cfg: Config{GatewayURL: "http://llm.example.com"}, + wantErr: true, + }, + { + name: "valid issuer only", + cfg: Config{OIDC: OIDCConfig{Issuer: "https://auth.example.com"}}, + wantErr: false, + }, + { + name: "invalid issuer rejected", + cfg: Config{OIDC: OIDCConfig{Issuer: "not-a-url"}}, + wantErr: true, + }, + { + name: "proxy port below range rejected", + cfg: Config{Proxy: ProxyConfig{ListenPort: 80}}, + wantErr: true, + }, + { + name: "proxy port above range rejected", + cfg: Config{Proxy: ProxyConfig{ListenPort: 99999}}, + wantErr: true, + }, + { + name: "valid proxy port accepted", + cfg: Config{Proxy: ProxyConfig{ListenPort: 8080}}, + wantErr: false, + }, + { + name: "callback port below range rejected", + cfg: Config{OIDC: OIDCConfig{CallbackPort: 100}}, + wantErr: true, + }, + { + name: "valid callback port accepted", + cfg: Config{OIDC: OIDCConfig{CallbackPort: 9000}}, + wantErr: false, + }, + { + name: "multiple invalid fields all reported", + cfg: Config{ + GatewayURL: "http://llm.example.com", + Proxy: ProxyConfig{ListenPort: 80}, + }, + wantErr: true, + }, + { + name: "required fields absent but valid values accepted", + cfg: Config{ + GatewayURL: "https://llm.example.com", + Proxy: ProxyConfig{ListenPort: 8080}, + }, + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + err := tt.cfg.ValidatePartial() + if (err != nil) != tt.wantErr { + t.Errorf("ValidatePartial() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} + func TestConfig_EffectiveProxyPort(t *testing.T) { t.Parallel() diff --git a/pkg/llm/doc.go b/pkg/llm/doc.go index c57a23e1d8..6a4f9d2e8e 100644 --- a/pkg/llm/doc.go +++ b/pkg/llm/doc.go @@ -4,12 +4,15 @@ // Package llm provides configuration types and public API for the thv llm // command group, which bridges AI coding tools to OIDC-protected LLM gateways. // -// Two authentication modes are supported: +// Two authentication modes are planned: // - Proxy mode: a localhost reverse proxy that injects fresh OIDC tokens for // tools that only accept static API keys (e.g. Cursor). // - Token helper mode: thv llm token prints a fresh JWT to stdout, suitable // for use as apiKeyHelper or auth.command in OIDC-capable tools (e.g. Claude Code). // +// Both modes are under active development; the corresponding CLI commands +// currently return not-implemented errors. +// // Configuration is persisted in ToolHive's config.yaml under the llm: key via // the existing UpdateConfig() mechanism. package llm diff --git a/pkg/networking/utilities_test.go b/pkg/networking/utilities_test.go index 58d1517d29..377bca2495 100644 --- a/pkg/networking/utilities_test.go +++ b/pkg/networking/utilities_test.go @@ -490,3 +490,131 @@ func TestValidateEndpointURL(t *testing.T) { }) } } + +func TestValidateHTTPSURL(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + url string + expectError bool + }{ + { + name: "valid HTTPS URL", + url: "https://llm.example.com", + expectError: false, + }, + { + name: "valid HTTPS URL with path", + url: "https://llm.example.com/api/v1", + expectError: false, + }, + { + name: "valid HTTPS URL with port", + url: "https://llm.example.com:8443", + expectError: false, + }, + { + name: "HTTP rejected even for localhost", + url: "http://localhost:8080", + expectError: true, + }, + { + name: "HTTP rejected for remote host", + url: "http://llm.example.com", + expectError: true, + }, + { + name: "missing host", + url: "https://", + expectError: true, + }, + { + name: "unsupported scheme", + url: "ftp://llm.example.com", + expectError: true, + }, + { + name: "invalid URL format", + url: "not-a-url", + expectError: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + err := ValidateHTTPSURL(tt.url) + if tt.expectError { + assert.Error(t, err, "Expected error for URL: %s", tt.url) + } else { + assert.NoError(t, err, "Expected no error for URL: %s", tt.url) + } + }) + } +} + +func TestValidateIssuerURL(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + url string + expectError bool + }{ + { + name: "valid HTTPS issuer", + url: "https://auth.example.com", + expectError: false, + }, + { + name: "valid HTTPS issuer with path", + url: "https://auth.example.com/realms/myrealm", + expectError: false, + }, + { + name: "localhost HTTP allowed for development", + url: "http://localhost:8080", + expectError: false, + }, + { + name: "127.0.0.1 HTTP allowed for development", + url: "http://127.0.0.1:9000", + expectError: false, + }, + { + name: "HTTP rejected for remote host", + url: "http://auth.example.com", + expectError: true, + }, + { + name: "missing host", + url: "https://", + expectError: true, + }, + { + name: "invalid URL format", + url: "not-a-url", + expectError: true, + }, + { + name: "unsupported scheme", + url: "ftp://auth.example.com", + expectError: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + err := ValidateIssuerURL(tt.url) + if tt.expectError { + assert.Error(t, err, "Expected error for URL: %s", tt.url) + } else { + assert.NoError(t, err, "Expected no error for URL: %s", tt.url) + } + }) + } +} diff --git a/pkg/secrets/scoped.go b/pkg/secrets/scoped.go index 827fdbc976..57bedc04f9 100644 --- a/pkg/secrets/scoped.go +++ b/pkg/secrets/scoped.go @@ -21,8 +21,8 @@ import ( // ends and the name begins. // // All constants declared in this package (ScopeRegistry, ScopeWorkloads, -// ScopeAuth) satisfy these invariants. Custom scopes introduced in the future -// must be validated against them. +// ScopeAuth, ScopeLLM) satisfy these invariants. Custom scopes introduced in +// the future must be validated against them. type SecretScope string const ( diff --git a/pkg/secrets/scoped_test.go b/pkg/secrets/scoped_test.go index 794a049b9a..f7e01ffaef 100644 --- a/pkg/secrets/scoped_test.go +++ b/pkg/secrets/scoped_test.go @@ -802,6 +802,7 @@ func TestSecretScopeInvariants(t *testing.T) { secrets.ScopeRegistry, secrets.ScopeWorkloads, secrets.ScopeAuth, + secrets.ScopeLLM, } for _, scope := range scopes {