Skip to content

Wire OAuth config to CLI flags / env vars (closes #96)#101

Merged
Slach merged 3 commits into
mainfrom
feature/oauth-flag-wiring
May 7, 2026
Merged

Wire OAuth config to CLI flags / env vars (closes #96)#101
Slach merged 3 commits into
mainfrom
feature/oauth-flag-wiring

Conversation

@BorisTyshkevich
Copy link
Copy Markdown
Collaborator

Summary

  • Drop 4 OAuth fields with no legitimate operator override (spec-fixed paths + an RFC-capped TTL): `ProtectedResourceMetadataPath`, `AuthorizationServerMetadataPath`, `OpenIDConfigurationPath`, `AuthCodeTTLSeconds`. Hardcoded defaults moved to `cmd/altinity-mcp/oauth_server.go`.
  • Add reflection-based flag/env wiring (`pkg/config/cli_reflect.go`). `BuildFlags(&Config{})` and `ApplyFlags(cfg, cmd)` walk the struct and emit / consume `cli.*Flag` based on `flag:`/`env:`/`desc:`/`default:` struct tags.
  • Add explicit `env:` tags on every `flag:`-tagged field across `Config`. Existing env-var names (`CLICKHOUSE_HOST`, `MCP_PORT`, `LOG_LEVEL`, …) preserved verbatim. New `MCP_OAUTH_*` env vars are now available for all OAuth fields — `MCP_OAUTH_GATING_SECRET_KEY` is the headline operator use case (Helm `valueFrom.secretKeyRef` injection without committing the secret to YAML).
  • Refactor `cmd/altinity-mcp/main.go`: the ~210-line explicit `Flags:` literal and ~200-line `overrideWithCLIFlags` body collapse into `BuildFlags` + `ApplyFlags` calls. `--config`, `--config-reload-time`, `--openapi` stay explicit (they don't fit the generic mechanism).
  • Future fields with a `flag:` tag are auto-wired — no `main.go` edits needed.
  • README env-vars section is now accurate.

Closes #96.

Net main.go shrinks by ~370 lines despite the OAuth flags being added; total diff is ~−374 lines across modified files (plus ~360 new lines for the helper and its tests).

Test plan

  • `go test ./... -count=1` — all green (cmd/altinity-mcp, pkg/config, pkg/server, etc.)
  • `pkg/config/cli_reflect_test.go` covers BuildFlags type dispatch, ApplyFlags CLI > YAML > default precedence, type-alias conversion (`MCPTransport` etc.), and skip semantics for missing/`flag:"-"` tags.
  • `altinity-mcp --help` lists every prior flag with its prior env-var name + new `oauth-` flags with `MCP_OAUTH_` env vars.
  • Smoke: `MCP_OAUTH_GATING_SECRET_KEY=… MCP_TRANSPORT=stdio go run ./cmd/altinity-mcp test-connection --clickhouse-host=… --clickhouse-port=…` honors both CLI flags and env vars.
  • Existing `TestOverrideWithCLIFlagsExtended` (incl. unrecognised-enum-defaults-to-info subtests) passes — defensive normalisation for `log-level`, `transport`, `clickhouse-protocol` retained post-apply.

🤖 Generated with Claude Code

BorisTyshkevich and others added 3 commits May 6, 2026 11:51
…ant fields

Closes #96.

The OAuthConfig struct carried 31 fields with `flag:` tags but none were
actually wired to CLI flags or env vars. Operators had to commit
gating_secret_key to rendered Helm values rather than inject from a
Kubernetes Secret via valueFrom.secretKeyRef.

Drop 4 fields with no legitimate operator use case:
  - ProtectedResourceMetadataPath  (RFC 9728 fixes /.well-known/oauth-protected-resource)
  - AuthorizationServerMetadataPath (RFC 8414 fixes /.well-known/oauth-authorization-server)
  - OpenIDConfigurationPath         (fixed /.well-known/openid-configuration)
  - AuthCodeTTLSeconds              (RFC 6749 caps; hardcoded 5 min default)

Add a struct-tag-driven flag/env wiring layer in pkg/config/cli_reflect.go.
BuildFlags walks the Config struct and emits cli.*Flag values from `flag:`,
`env:`, `desc:`, and `default:` tags. ApplyFlags walks again and copies CLI
values onto the struct, preserving CLI > YAML > hardcoded-default precedence.

In pkg/config/config.go: add explicit `env:` tags on every flag-tagged field
(matching existing env names verbatim so no operator-visible breakage), add
`default:` tags where defaults are non-zero, and add MCP_OAUTH_* env vars for
all OAuth fields.

In cmd/altinity-mcp/main.go: replace the ~210-line explicit Flags slice and
~200-line overrideWithCLIFlags body with config.BuildFlags + config.ApplyFlags
calls. Keep --config, --config-reload-time, --openapi explicit (they don't
fit the generic mechanism). Net main.go shrinks by ~370 lines.

Future fields with a `flag:` tag are now auto-wired — no main.go edits needed.

README env-vars section now points at struct tags as the source of truth and
calls out MCP_OAUTH_GATING_SECRET_KEY as the operator's headline use case.

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
The "gating" prefix was misleading: the secret is required in BOTH forward
and gating modes. It HMAC-signs every stateless artifact this server mints
— self-issued JWT access tokens (HS256), authorization codes, refresh
tokens, and RFC 7591 dynamic-client-registration client_secrets. Naming
should reflect that.

Lockstep rename across all four naming layers:
  Go field:  GatingSecretKey            -> SigningSecret
  YAML/JSON: gating_secret_key          -> signing_secret
  CLI flag:  --oauth-gating-secret-key  -> --oauth-signing-secret
  Env var:   MCP_OAUTH_GATING_SECRET_KEY -> MCP_OAUTH_SIGNING_SECRET

Plus error messages and docstrings updated for clarity.

This is a config-breaking rename. Safe to land because the env var only
reached main yesterday in PR #101 (no deployments yet) and the YAML key
ships pre-GA — no existing operator configs to migrate.

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
…er form on inbound aud

handleOAuthProtectedResource now returns "resource" in its canonical
RFC 9728 form (always trailing slash), produced by canonicalResourceURL.
This matches what most upstream IdPs (Auth0, Google) emit in `aud`
claims and what RFC 9728 §3.3 picks as the canonical resource
identifier. Strict OAuth clients (Claude.ai) compare the resource
metadata field literally, so the slash-less form caused mismatches.

audienceMatchesResource is a small helper that compares an audience
list against an expected resource URL with trailing-slash tolerance,
so tokens minted before this change (no slash) still validate, and
tokens minted after (with slash) also validate when admin config
keeps the no-slash form. Both validateOAuthClaims and the external-JWT
path use the helper.

Also adds:
- TestCanonicalResourceURL covering empty / whitespace / multi-slash
  inputs and path-suffixed bases.
- audience_trailing_slash_tolerant subtest in TestValidateOAuthClaims
  exercising both directions.
- Existing protected-resource tests updated to assert the canonical
  trailing-slash form.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@BorisTyshkevich BorisTyshkevich force-pushed the feature/oauth-flag-wiring branch from 5050de6 to 7ffdced Compare May 6, 2026 14:49
@Slach Slach merged commit d861116 into main May 7, 2026
13 checks passed
BorisTyshkevich added a commit that referenced this pull request May 7, 2026
Squashed rebase of feature/tools-strict-schema's substantive OAuth work
onto main (which already has the canonical-resource-URL-with-slash
direction from PR #101 / 7ffdced; this branch's earlier flip-flop
commits on that question were dropped during the rebase).

Spec-compliance hardening (MCP authorization 2025-11-25):
- RFC 8707 Resource Indicators: end-to-end. /authorize accepts and
  validates the `resource=` query parameter, propagates it through
  pendingAuth → auth code → mintGatingTokenResponse so the eventual
  `aud` claim byte-matches what the client sent. /token also accepts
  `resource=` for refresh-token narrowing.
- WWW-Authenticate Bearer challenge per RFC 6750: emit error=,
  error_description=, resource_metadata= on every 401/403; emit scope=
  on both initial 401 (SHOULD per MCP §Protected Resource Metadata
  Discovery Requirements) and insufficient_scope 403 (MUST per MCP
  §Runtime Insufficient Scope Errors). insufficient_scope returns 403
  with JSON body, not 401.
- Upstream-leg PKCE (OAuth 2.1 §7.5.2): generate fresh upstream verifier
  in /authorize, replay on upstream /token. Defends auth-code intercept
  even when we hold the upstream client_secret.
- UpstreamIssuerAllowlist enforcement: previously loaded into config but
  never consulted. Now constrains accepted upstream IdP issuers in
  parseAndVerifyExternalJWT.
- Stateless DCR with per-client client_secret: register clients as
  confidential by default (client_secret_post). Auth0/Anthropic
  artifact-proxy require this.
- Pending-auth TTL split: 10 min for /authorize → /callback (login
  window per RFC 6749 §3.1.2), 60 s for /callback → /token redemption
  (per OAuth 2.1 §4.1.2). Was a single 5-min value.
- signing_secret length gate: reject < 32 bytes at startup.
- Drop x-oauth-token / x-altinity-oauth-token fallback headers; MCP
  spec mandates Authorization: Bearer only.
- Startup warnings when public_resource_url / oauth_issuer /
  upstream_issuer_allowlist combinations leave host-spoof or
  unconstrained-issuer surfaces.

HKDF Step 2 (per-context keys + kid header):
- Each cryptographic use of the shared SigningSecret derives an
  independent 32-byte key via HKDF-SHA256 with a per-context info label
  (RFC 5869 §3.2 domain separation): client-id JWE, refresh-token JWE,
  self-issued access-token HS256 — three independent keys from one
  secret.
- Newly-issued artifacts carry kid="v1" in the protected JWE/JWS
  header. Decoders pick the derivation by inspecting kid: kid=="v1"
  → HKDF-derived key; kid absent → legacy SHA256(secret) fallback for
  artifacts minted before the cutover (refresh tokens up to 30 days
  old keep working).
- pkg/jwe_auth.DeriveKey + ValidateClaimsWhitelist + ValidateExpiration
  exposed for callers that decrypt via the HKDF path.

C-1 forward-mode local validation:
- Forward-mode bearers were previously accepted with no local check;
  the entire trust boundary collapsed onto ClickHouse token_processors.
  Now MCP validates JWT bearers locally (signature + iss + aud + exp)
  when issuer/jwks_url is configured. Opaque bearers and JWTs without
  a configured JWKS source soft-pass with debug log; ClickHouse remains
  the sole validator in those cases (compat with deployments that
  pre-date the fix).
- oauthRequiresLocalValidation now true in both gating and forward
  modes.
- createMCPAuthInjector calls ValidateOAuthToken regardless of mode.
- Startup warning when forward mode runs without issuer/jwks_url.

Tests: TestOAuthValidateToken, TestOAuthRequiresLocalValidation,
TestOAuthUpstreamIssuerAllowlist, TestOAuthMCPAuthInjector*,
TestOAuthJWEHKDFRoundtripAndLegacyFallback all pass.

Build: clean. go test ./pkg/server/ ./cmd/altinity-mcp/ green.

Backup tag: backup/feature-tools-strict-schema/pre-rebase-20260507-1503
preserves the 15-commit pre-rebase history.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
BorisTyshkevich added a commit that referenced this pull request May 7, 2026
Squashed rebase of feature/tools-strict-schema's substantive OAuth work
onto main (which already has the canonical-resource-URL-with-slash
direction from PR #101 / 7ffdced; this branch's earlier flip-flop
commits on that question were dropped during the rebase).

Spec-compliance hardening (MCP authorization 2025-11-25):
- RFC 8707 Resource Indicators: end-to-end. /authorize accepts and
  validates the `resource=` query parameter, propagates it through
  pendingAuth → auth code → mintGatingTokenResponse so the eventual
  `aud` claim byte-matches what the client sent. /token also accepts
  `resource=` for refresh-token narrowing.
- WWW-Authenticate Bearer challenge per RFC 6750: emit error=,
  error_description=, resource_metadata= on every 401/403; emit scope=
  on both initial 401 (SHOULD per MCP §Protected Resource Metadata
  Discovery Requirements) and insufficient_scope 403 (MUST per MCP
  §Runtime Insufficient Scope Errors). insufficient_scope returns 403
  with JSON body, not 401.
- Upstream-leg PKCE (OAuth 2.1 §7.5.2): generate fresh upstream verifier
  in /authorize, replay on upstream /token. Defends auth-code intercept
  even when we hold the upstream client_secret.
- UpstreamIssuerAllowlist enforcement: previously loaded into config but
  never consulted. Now constrains accepted upstream IdP issuers in
  parseAndVerifyExternalJWT.
- Stateless DCR with per-client client_secret: register clients as
  confidential by default (client_secret_post). Auth0/Anthropic
  artifact-proxy require this.
- Pending-auth TTL split: 10 min for /authorize → /callback (login
  window per RFC 6749 §3.1.2), 60 s for /callback → /token redemption
  (per OAuth 2.1 §4.1.2). Was a single 5-min value.
- signing_secret length gate: reject < 32 bytes at startup.
- Drop x-oauth-token / x-altinity-oauth-token fallback headers; MCP
  spec mandates Authorization: Bearer only.
- Startup warnings when public_resource_url / oauth_issuer /
  upstream_issuer_allowlist combinations leave host-spoof or
  unconstrained-issuer surfaces.

HKDF Step 2 (per-context keys + kid header):
- Each cryptographic use of the shared SigningSecret derives an
  independent 32-byte key via HKDF-SHA256 with a per-context info label
  (RFC 5869 §3.2 domain separation): client-id JWE, refresh-token JWE,
  self-issued access-token HS256 — three independent keys from one
  secret.
- Newly-issued artifacts carry kid="v1" in the protected JWE/JWS
  header. Decoders pick the derivation by inspecting kid: kid=="v1"
  → HKDF-derived key; kid absent → legacy SHA256(secret) fallback for
  artifacts minted before the cutover (refresh tokens up to 30 days
  old keep working).
- pkg/jwe_auth.DeriveKey + ValidateClaimsWhitelist + ValidateExpiration
  exposed for callers that decrypt via the HKDF path.

C-1 forward-mode local validation:
- Forward-mode bearers were previously accepted with no local check;
  the entire trust boundary collapsed onto ClickHouse token_processors.
  Now MCP validates JWT bearers locally (signature + iss + aud + exp)
  when issuer/jwks_url is configured. Opaque bearers and JWTs without
  a configured JWKS source soft-pass with debug log; ClickHouse remains
  the sole validator in those cases (compat with deployments that
  pre-date the fix).
- oauthRequiresLocalValidation now true in both gating and forward
  modes.
- createMCPAuthInjector calls ValidateOAuthToken regardless of mode.
- Startup warning when forward mode runs without issuer/jwks_url.

Tests: TestOAuthValidateToken, TestOAuthRequiresLocalValidation,
TestOAuthUpstreamIssuerAllowlist, TestOAuthMCPAuthInjector*,
TestOAuthJWEHKDFRoundtripAndLegacyFallback all pass.

Build: clean. go test ./pkg/server/ ./cmd/altinity-mcp/ green.

Backup tag: backup/feature-tools-strict-schema/pre-rebase-20260507-1503
preserves the 15-commit pre-rebase history.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.

OAuth config fields not wired to CLI flags / env vars (30 missing flags)

2 participants