From 00bbef7eb038ddd6b189a08fceebbed781957f4d Mon Sep 17 00:00:00 2001 From: shankar0123 Date: Sun, 10 May 2026 23:25:54 +0000 Subject: [PATCH] feat(oidc): POST /api/v1/auth/oidc/test dry-run endpoint (MED-5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audit 2026-05-10 MED-5 closure (backend half). WHAT. New POST /api/v1/auth/oidc/test endpoint that validates an OIDC provider configuration without persisting anything. Mirrors the read-only legs of the production getOrLoad path so operators can catch typos / network reachability problems / IdP-advertises-weak- alg conditions BEFORE creating the provider row. Request body: {issuer_url, client_id, client_secret, scopes} — client_secret is accepted but unused (discovery + JWKS reachability do not require it). Response body: TestDiscoveryResult{ discovery_succeeded — gooidc.NewProvider returned without error jwks_reachable — explicit GET against jwks_uri succeeded supported_alg_values — verbatim id_token_signing_alg_values_supported iss_param_supported — RFC 9207 advertisement parsed off the disco doc issuer_echo — the iss URL we were called with authorization_url, token_url, jwks_uri, userinfo_endpoint — discovery doc fields for the GUI to preview errors[] — per-leg failure messages } HTTP status: - 200 even when individual checks fail (the per-leg errors[] carries detail so the GUI renders per-check status rows) - 400 only when the request body is malformed or issuer_url empty - 500 only when the service-layer call itself errors WHY. Pre-fix, operators configuring OIDC had to create a provider, then hit /refresh, then read the audit log to figure out whether the discovery doc was reachable / whether the IdP advertises HS256 (the alg-downgrade trap). The GUI rendered no per-check feedback. MED-5 closes the dry-run gap for the same reason every Issuer + Target connector has a 'Test connection' button — operator experience parity. HOW. internal/auth/oidc/test_discovery.go (NEW): - TestDiscoveryResult struct with the per-leg projection. - Service.TestDiscovery(ctx, issuerURL) drives the read-only subset of getOrLoad: gooidc.NewProvider, claims parse for alg-supported + iss-param-supported + jwks_uri + userinfo, alg-downgrade defense, jwksReachable HTTP GET. - jwksReachable is a package-level closure so tests can swap. internal/api/handler/auth_session_oidc.go: - TestProvider HTTP handler. Uses an inline discoveryTester interface to type-assert against the OIDCAuthHandshaker stub (the production Service satisfies; test stubs supply via explicit method). Audit row 'auth.oidc_provider_tested' carries the summary fields. internal/api/router/router.go: - Wired as POST /api/v1/auth/oidc/test under rbacGate('auth.oidc.create'). internal/api/handler/auth_session_oidc_test.go: - stubOIDCSvc gains testResult + testErr fields + TestDiscovery method so it satisfies the inline interface. - 3 regression tests: happy path, missing issuer_url -> 400, discovery-failure -> 200 with errors[] populated. VERIFY. - go vet ./internal/auth/oidc/... ./internal/api/handler/... ./internal/api/router/... PASS - go test -short -count=1 -run TestProvider ./internal/api/handler/... PASS (3/3) - go test -short -count=1 ./internal/auth/oidc/... PASS (3.7s) - go test -short -count=1 ./internal/api/handler/... PASS (4.7s) Out of scope for this commit: the GUI 'Test connection' button on OIDCProviderDetailPage — queued with the GUI batch (items 10-19 of HANDOFF.md). Refs: cowork/auth-bundles-audit-2026-05-10.md MED-5 cowork/auth-bundles-fixes-2026-05-10/HANDOFF.md item 2 --- CHANGELOG.md | 11 ++ internal/api/handler/auth_session_oidc.go | 65 +++++++++ .../api/handler/auth_session_oidc_test.go | 89 +++++++++++++ internal/api/router/router.go | 4 + internal/auth/oidc/test_discovery.go | 125 ++++++++++++++++++ 5 files changed, 294 insertions(+) create mode 100644 internal/auth/oidc/test_discovery.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e4d4c4..b5ff352 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,17 @@ RFC-9207 discovery. Providers that don't advertise support (the majority today) keep pre-fix behavior — back-compat is preserved. +- **OIDC provider test endpoint (Audit 2026-05-10 MED-5).** New + `POST /api/v1/auth/oidc/test` dry-runs an OIDC provider configuration + without persisting: fetches the discovery doc, runs the alg-downgrade + defense, detects RFC 9207 iss-parameter advertisement, and confirms + JWKS reachability. Returns `TestDiscoveryResult{discovery_succeeded, + jwks_reachable, supported_alg_values, iss_param_supported, errors[]}` + so the GUI (forthcoming) can render per-check status rows. Per-leg + failures ride in the response body's `errors` array; only a malformed + request body trips 400. Gate: `auth.oidc.create`. Audit row + `auth.oidc_provider_tested` carries the success/failure summary. + - **Pre-login UA / source-IP binding on OIDC callback (Audit 2026-05-10 MED-16).** RFC 9700 §4.7.1 defense against stolen-pre-login-cookie replay by a different browser / source. Migration `000044_prelogin_uaip` adds diff --git a/internal/api/handler/auth_session_oidc.go b/internal/api/handler/auth_session_oidc.go index 0a41013..3faf223 100644 --- a/internal/api/handler/auth_session_oidc.go +++ b/internal/api/handler/auth_session_oidc.go @@ -950,6 +950,71 @@ func (h *AuthSessionOIDCHandler) DeleteProvider(w http.ResponseWriter, r *http.R w.WriteHeader(http.StatusNoContent) } +// TestProvider handles POST /api/v1/auth/oidc/test. +// +// Audit 2026-05-10 MED-5 closure. Dry-run validator for an OIDC +// provider config: runs OIDC discovery, the alg-downgrade defense, +// the RFC 9207 iss-parameter detection, and a JWKS fetch — without +// persisting anything. Body: `{issuer_url, client_id, scopes}` +// (client_secret accepted but ignored — discovery + JWKS don't +// require it). Response: TestDiscoveryResult; HTTP 200 even when +// individual checks fail (the response Errors field carries them so +// the GUI can render per-check status rows). +// +// Permission gate: `auth.oidc.create` (the operator is dry-running a +// provider they're about to create; the lookup endpoints have their +// own .list gate so this can't be used as a roundabout reconnaissance +// vector beyond what those already permit). +func (h *AuthSessionOIDCHandler) TestProvider(w http.ResponseWriter, r *http.Request) { + caller, err := callerFromRequest(r) + if err != nil { + writeAuthError(w, err) + return + } + var req struct { + IssuerURL string `json:"issuer_url"` + ClientID string `json:"client_id"` + ClientSecret string `json:"client_secret"` + Scopes []string `json:"scopes"` + } + if derr := json.NewDecoder(r.Body).Decode(&req); derr != nil { + Error(w, http.StatusBadRequest, "invalid JSON body") + return + } + if strings.TrimSpace(req.IssuerURL) == "" { + Error(w, http.StatusBadRequest, "issuer_url is required") + return + } + // Type-assert to the concrete service so we can reach the + // TestDiscovery method. The OIDCAuthHandshaker interface is + // intentionally narrow; rather than widening it (which would force + // every test stub to implement TestDiscovery) we accept the + // concrete reference for this single endpoint. Production code + // always supplies *oidcsvc.Service. + type discoveryTester interface { + TestDiscovery(ctx context.Context, issuerURL string) (*oidcsvc.TestDiscoveryResult, error) + } + tester, ok := h.oidcSvc.(discoveryTester) + if !ok { + Error(w, http.StatusInternalServerError, "OIDC service does not support discovery test") + return + } + res, terr := tester.TestDiscovery(r.Context(), strings.TrimSpace(req.IssuerURL)) + if terr != nil { + Error(w, http.StatusInternalServerError, "discovery test execution failed") + return + } + h.recordAudit(r.Context(), "auth.oidc_provider_tested", caller.ActorID, caller.ActorType, "", + map[string]interface{}{ + "issuer_url": req.IssuerURL, + "discovery_succeeded": res.DiscoverySucceeded, + "jwks_reachable": res.JWKSReachable, + "iss_param_supported": res.IssParamSupported, + "error_count": len(res.Errors), + }) + writeJSON(w, http.StatusOK, res) +} + // RefreshProvider handles POST /api/v1/auth/oidc/providers/{id}/refresh. // Forces re-fetch of the IdP discovery doc + JWKS, re-runs the IdP // downgrade-attack defense. diff --git a/internal/api/handler/auth_session_oidc_test.go b/internal/api/handler/auth_session_oidc_test.go index 4e2c406..55fe134 100644 --- a/internal/api/handler/auth_session_oidc_test.go +++ b/internal/api/handler/auth_session_oidc_test.go @@ -41,6 +41,11 @@ type stubOIDCSvc struct { callbackRes *oidcsvc.CallbackResult callbackErr error refreshErr error + // Audit 2026-05-10 MED-5 — stub for the TestDiscovery dry-run. + // When testResult is non-nil, the handler-level type assertion + // resolves and the response carries this verbatim. + testResult *oidcsvc.TestDiscoveryResult + testErr error } func (s *stubOIDCSvc) HandleAuthRequest(_ context.Context, _, _, _ string) (string, string, string, error) { @@ -51,6 +56,12 @@ func (s *stubOIDCSvc) HandleCallback(_ context.Context, _, _, _, _, _, _ string) } func (s *stubOIDCSvc) RefreshKeys(_ context.Context, _ string) error { return s.refreshErr } +// TestDiscovery satisfies the inline discoveryTester interface used by +// the TestProvider HTTP handler. Audit 2026-05-10 MED-5. +func (s *stubOIDCSvc) TestDiscovery(_ context.Context, _ string) (*oidcsvc.TestDiscoveryResult, error) { + return s.testResult, s.testErr +} + type stubSession struct { createRes *sessionsvc.CreateResult createErr error @@ -1215,3 +1226,81 @@ func TestClassifyOIDCFailure(t *testing.T) { } } } + +// ============================================================================= +// MED-5 regression tests — TestProvider dry-run endpoint. +// ============================================================================= + +func TestTestProvider_HappyPath(t *testing.T) { + o := &stubOIDCSvc{ + testResult: &oidcsvc.TestDiscoveryResult{ + DiscoverySucceeded: true, + JWKSReachable: true, + SupportedAlgValues: []string{"RS256", "ES256"}, + IssParamSupported: true, + IssuerEcho: "https://idp.example.com", + }, + } + h, _, _, _, audit, _ := newPhase5Handler(t, o, &stubSession{}, &stubBCLVerifier{}) + + body := strings.NewReader(`{"issuer_url":"https://idp.example.com","client_id":"app","scopes":["openid"]}`) + req := httptest.NewRequest(http.MethodPost, "/api/v1/auth/oidc/test", body) + req = withActor(req, "u-admin", "User") + w := httptest.NewRecorder() + h.TestProvider(w, req) + if w.Code != http.StatusOK { + t.Fatalf("status = %d; want 200; body=%s", w.Code, w.Body.String()) + } + if !strings.Contains(w.Body.String(), `"discovery_succeeded":true`) { + t.Errorf("body missing discovery_succeeded:true; got %s", w.Body.String()) + } + if !strings.Contains(w.Body.String(), `"iss_param_supported":true`) { + t.Errorf("body missing iss_param_supported:true") + } + if !contains(audit.events, "auth.oidc_provider_tested") { + t.Errorf("expected auth.oidc_provider_tested audit event; got %v", audit.events) + } +} + +func TestTestProvider_MissingIssuerURL_Returns400(t *testing.T) { + h, _, _, _, _, _ := newPhase5Handler(t, &stubOIDCSvc{}, &stubSession{}, &stubBCLVerifier{}) + + body := strings.NewReader(`{"client_id":"app"}`) + req := httptest.NewRequest(http.MethodPost, "/api/v1/auth/oidc/test", body) + req = withActor(req, "u-admin", "User") + w := httptest.NewRecorder() + h.TestProvider(w, req) + if w.Code != http.StatusBadRequest { + t.Errorf("status = %d; want 400", w.Code) + } +} + +// TestTestProvider_DiscoveryFailureReturns200WithErrors pins the +// failure-shape contract: discovery failure is a per-leg failure +// surfaced in the response body's `errors` array, NOT a 5xx — the +// GUI renders the per-check status row from the response. +func TestTestProvider_DiscoveryFailureReturns200WithErrors(t *testing.T) { + o := &stubOIDCSvc{ + testResult: &oidcsvc.TestDiscoveryResult{ + DiscoverySucceeded: false, + JWKSReachable: false, + Errors: []string{"discovery fetch failed: connection refused"}, + }, + } + h, _, _, _, _, _ := newPhase5Handler(t, o, &stubSession{}, &stubBCLVerifier{}) + + body := strings.NewReader(`{"issuer_url":"https://broken.example.com"}`) + req := httptest.NewRequest(http.MethodPost, "/api/v1/auth/oidc/test", body) + req = withActor(req, "u-admin", "User") + w := httptest.NewRecorder() + h.TestProvider(w, req) + if w.Code != http.StatusOK { + t.Fatalf("status = %d; want 200 (per-leg failure rides in body); body=%s", w.Code, w.Body.String()) + } + if !strings.Contains(w.Body.String(), `"discovery_succeeded":false`) { + t.Errorf("expected discovery_succeeded:false in body; got %s", w.Body.String()) + } + if !strings.Contains(w.Body.String(), "connection refused") { + t.Errorf("expected error detail in body; got %s", w.Body.String()) + } +} diff --git a/internal/api/router/router.go b/internal/api/router/router.go index 71d045e..326fa82 100644 --- a/internal/api/router/router.go +++ b/internal/api/router/router.go @@ -459,6 +459,10 @@ func (r *Router) RegisterHandlers(reg HandlerRegistry) { r.Register("PUT /api/v1/auth/oidc/providers/{id}", rbacGate(reg.Checker, "auth.oidc.edit", reg.AuthSessionOIDC.UpdateProvider)) r.Register("DELETE /api/v1/auth/oidc/providers/{id}", rbacGate(reg.Checker, "auth.oidc.delete", reg.AuthSessionOIDC.DeleteProvider)) r.Register("POST /api/v1/auth/oidc/providers/{id}/refresh", rbacGate(reg.Checker, "auth.oidc.edit", reg.AuthSessionOIDC.RefreshProvider)) + // Audit 2026-05-10 MED-5 — dry-run validator for OIDC provider + // config. Returns discovery + JWKS + alg-downgrade + iss-param + // reachability without persisting. + r.Register("POST /api/v1/auth/oidc/test", rbacGate(reg.Checker, "auth.oidc.create", reg.AuthSessionOIDC.TestProvider)) // Group-mapping CRUD. r.Register("GET /api/v1/auth/oidc/group-mappings", rbacGate(reg.Checker, "auth.oidc.list", reg.AuthSessionOIDC.ListGroupMappings)) diff --git a/internal/auth/oidc/test_discovery.go b/internal/auth/oidc/test_discovery.go new file mode 100644 index 0000000..3bcb003 --- /dev/null +++ b/internal/auth/oidc/test_discovery.go @@ -0,0 +1,125 @@ +package oidc + +// Audit 2026-05-10 MED-5 closure — dry-run validator for OIDC provider +// configuration. Lets operators verify discovery + JWKS reachability + +// alg-downgrade defense BEFORE persisting a provider row. Mirrors the +// non-persistence-touching subset of getOrLoad. + +import ( + "context" + "fmt" + "net/http" + + gooidc "github.com/coreos/go-oidc/v3/oidc" +) + +// TestDiscoveryResult is the report TestDiscovery returns. The HTTP +// layer marshals this verbatim. Each field is independently observable +// so the GUI can render a per-check status row. +// +// `Errors` collects every leg that failed; a partial-success case +// (e.g. discovery OK but alg-downgrade tripped) returns +// DiscoverySucceeded=true + a non-empty Errors slice. +type TestDiscoveryResult struct { + DiscoverySucceeded bool `json:"discovery_succeeded"` + JWKSReachable bool `json:"jwks_reachable"` + SupportedAlgValues []string `json:"supported_alg_values"` + IssParamSupported bool `json:"iss_param_supported"` + IssuerEcho string `json:"issuer_echo,omitempty"` // the iss value the IdP advertised + AuthorizationURL string `json:"authorization_url,omitempty"` + TokenURL string `json:"token_url,omitempty"` + JWKSURI string `json:"jwks_uri,omitempty"` + UserInfoEndpoint string `json:"userinfo_endpoint,omitempty"` + Errors []string `json:"errors,omitempty"` +} + +// TestDiscovery runs the read-only subset of getOrLoad against a +// candidate issuer URL: fetches the discovery doc, runs the +// alg-downgrade defense, parses the RFC 9207 iss-parameter advert, +// then fetches the JWKS once to confirm reachability. +// +// The function NEVER persists anything; the caller is the +// /api/v1/auth/oidc/test endpoint that the GUI uses for dry-runs. +// +// Service-layer entry point so the handler stays HTTP-shaped only. +func (s *Service) TestDiscovery(ctx context.Context, issuerURL string) (*TestDiscoveryResult, error) { + res := &TestDiscoveryResult{} + + // Step 1 — discovery. gooidc.NewProvider fetches + // `/.well-known/openid-configuration` and runs the iss + // match check internally; on failure it returns a fmt-style + // wrapped error. + provider, err := gooidc.NewProvider(ctx, issuerURL) + if err != nil { + res.Errors = append(res.Errors, fmt.Sprintf("discovery fetch failed: %v", err)) + return res, nil // Non-fatal at this layer; the response carries the per-leg failure. + } + res.DiscoverySucceeded = true + res.IssuerEcho = issuerURL + endpoint := provider.Endpoint() + res.AuthorizationURL = endpoint.AuthURL + res.TokenURL = endpoint.TokenURL + + // Step 2 — parse the claims we care about from the discovery doc. + var advertised struct { + IDTokenSigningAlgValuesSupported []string `json:"id_token_signing_alg_values_supported"` + AuthorizationResponseIssParamSupported bool `json:"authorization_response_iss_parameter_supported"` + JWKSURI string `json:"jwks_uri"` + UserInfoEndpoint string `json:"userinfo_endpoint"` + } + if cerr := provider.Claims(&advertised); cerr != nil { + res.Errors = append(res.Errors, fmt.Sprintf("discovery claims: %v", cerr)) + return res, nil + } + res.SupportedAlgValues = advertised.IDTokenSigningAlgValuesSupported + res.IssParamSupported = advertised.AuthorizationResponseIssParamSupported + res.JWKSURI = advertised.JWKSURI + res.UserInfoEndpoint = advertised.UserInfoEndpoint + + // Step 3 — alg-downgrade defense. The IdP MUST NOT advertise HS* + // or none in the signing-alg list (operators that bind certctl to + // an IdP advertising these are at risk of a forged-token attack). + // Same check applied in getOrLoad's production path. + for _, a := range advertised.IDTokenSigningAlgValuesSupported { + if _, deny := disallowedAlgs[a]; deny { + res.Errors = append(res.Errors, fmt.Sprintf("alg-downgrade defense tripped: IdP advertises %s in id_token_signing_alg_values_supported", a)) + } + } + + // Step 4 — JWKS reachability. The go-oidc Verifier defers JWKS + // fetch until first token-verify; for the dry-run we explicitly + // HEAD/GET the JWKS endpoint to confirm network reachability. + if advertised.JWKSURI == "" { + res.Errors = append(res.Errors, "discovery doc omits jwks_uri") + } else if ok, herr := jwksReachable(ctx, advertised.JWKSURI); !ok { + if herr != nil { + res.Errors = append(res.Errors, fmt.Sprintf("JWKS fetch failed: %v", herr)) + } else { + res.Errors = append(res.Errors, "JWKS endpoint returned non-200") + } + } else { + res.JWKSReachable = true + } + + return res, nil +} + +// jwksReachable issues a GET against the JWKS URI and returns ok=true +// when the response status is 2xx. Used by TestDiscovery for the +// reachability leg of the dry-run. +// +// Kept distinct from go-oidc's internal JWKS fetcher because we want +// to surface the HTTP status to the operator without requiring a +// token-verify round-trip. +var jwksReachable = func(ctx context.Context, jwksURI string) (bool, error) { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, jwksURI, nil) + if err != nil { + return false, err + } + resp, err := http.DefaultClient.Do(req) + if err != nil { + return false, err + } + defer resp.Body.Close() + return resp.StatusCode >= 200 && resp.StatusCode < 300, nil +}