From ddc25139f7432a13bd0cb899232891c9304c4a98 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Fri, 23 Oct 2026 22:01:42 +1100 Subject: [PATCH] Support OIDC form_post, require loopback listener, redact request URLs in errors --- docs/vcm.md | 3 ++- shared/oidc.go | 29 +++++++++++++++++++++------- shared/oidc_test.go | 46 ++++++++++++++++++++++++++++++++++++++++++--- shared/vault.go | 14 +++++++++++--- 4 files changed, 78 insertions(+), 14 deletions(-) diff --git a/docs/vcm.md b/docs/vcm.md index d64cfc7..c392275 100644 --- a/docs/vcm.md +++ b/docs/vcm.md @@ -77,7 +77,8 @@ For each context, every field is resolved with the chain - `namespace` — no built-in default (unset means the root namespace). - `path` — the auth mount path; defaults to the resolved `method`. - `role` — OIDC role; unset uses the mount's `default_role`. -- `oidc_listen` — OIDC callback listener; defaults to `localhost:8250`. +- `oidc_listen` — OIDC callback listener on a loopback host (`localhost`, + `127.0.0.1` or `[::1]`); defaults to `localhost:8250`. `--method` / `--user` on the command line override the resolved values. When `--method` changes the method and the context did not pin an explicit `path`, diff --git a/shared/oidc.go b/shared/oidc.go index 1fc81a3..cc1a73a 100644 --- a/shared/oidc.go +++ b/shared/oidc.go @@ -32,6 +32,13 @@ type oidcResult struct { // fetch auth_url, open the browser, receive code+state on a local callback, // and exchange them via auth//oidc/callback. func LoginOIDC(rc ResolvedContext) (*Token, error) { + host, _, err := net.SplitHostPort(rc.OIDCListen) + if err != nil { + return nil, fmt.Errorf("oidc listen address %q: %w", rc.OIDCListen, err) + } + if host != "localhost" && host != "127.0.0.1" && host != "::1" { + return nil, fmt.Errorf("oidc listen address %q must be on localhost, 127.0.0.1 or [::1]", rc.OIDCListen) + } ln, err := net.Listen("tcp", rc.OIDCListen) if err != nil { return nil, fmt.Errorf("oidc callback listener: %w", err) @@ -90,17 +97,25 @@ func LoginOIDC(rc ResolvedContext) (*Token, error) { } } -// oidcCallback exchanges the provider's state+code for a Vault token. +// oidcCallback exchanges the provider's state+code for a Vault token. A POST +// is the form_post response mode: forward the form to Vault, then GET as usual. func oidcCallback(rc ResolvedContext, r *http.Request, nonce string) oidcResult { - q := r.URL.Query() - if e := q.Get("error"); e != "" { - return oidcResult{err: fmt.Errorf("provider returned %s: %s", e, q.Get("error_description"))} + if e := r.FormValue("error"); e != "" { + return oidcResult{err: fmt.Errorf("provider returned %s: %s", e, r.FormValue("error_description"))} } - if q.Get("code") == "" || q.Get("state") == "" { + state, code := r.FormValue("state"), r.FormValue("code") + if state == "" || (code == "" && r.Method != http.MethodPost) { return oidcResult{err: errors.New("callback missing code or state")} } - v := url.Values{"state": {q.Get("state")}, "code": {q.Get("code")}, "client_nonce": {nonce}} - data, err := doRequest(http.MethodGet, rc.Address, "auth/"+rc.Path+"/oidc/callback?"+v.Encode(), rc.Namespace, "", nil) + cb := "auth/" + rc.Path + "/oidc/callback" + if r.Method == http.MethodPost { + if _, err := doRequest(http.MethodPost, rc.Address, cb, rc.Namespace, "", + map[string]string{"state": state, "code": code, "id_token": r.FormValue("id_token"), "client_nonce": nonce}); err != nil { + return oidcResult{err: err} + } + } + v := url.Values{"state": {state}, "code": {code}, "client_nonce": {nonce}} + data, err := doRequest(http.MethodGet, rc.Address, cb+"?"+v.Encode(), rc.Namespace, "", nil) if err != nil { return oidcResult{err: err} } diff --git a/shared/oidc_test.go b/shared/oidc_test.go index d7e118f..8446625 100644 --- a/shared/oidc_test.go +++ b/shared/oidc_test.go @@ -27,7 +27,7 @@ func freeListen(t *testing.T) string { // nonce/redirect round-trip, and records the redirect_uri it was given. func fakeOIDCVault(t *testing.T, wantRole string) (*httptest.Server, *string) { t.Helper() - var redirect, nonce string + var redirect, nonce, idToken string srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Header.Get("X-Vault-Namespace") != "team-a" { t.Errorf("namespace header = %q", r.Header.Get("X-Vault-Namespace")) @@ -41,9 +41,16 @@ func fakeOIDCVault(t *testing.T, wantRole string) (*httptest.Server, *string) { } redirect, nonce = body["redirect_uri"], body["client_nonce"] _, _ = io.WriteString(w, `{"data":{"auth_url":"https://idp.example/authorize?state=st1"}}`) + case r.Method == http.MethodPost && r.URL.Path == "/v1/auth/sso/oidc/callback": + var body map[string]string + _ = json.NewDecoder(r.Body).Decode(&body) + if body["state"] != "st1" || body["client_nonce"] != nonce { + t.Errorf("form_post body = %v", body) + } + idToken = body["id_token"] case r.Method == http.MethodGet && r.URL.Path == "/v1/auth/sso/oidc/callback": q := r.URL.Query() - if q.Get("state") != "st1" || q.Get("code") != "c0de" || q.Get("client_nonce") != nonce { + if q.Get("state") != "st1" || (q.Get("code") != "c0de" && idToken != "idt") || q.Get("client_nonce") != nonce { w.WriteHeader(http.StatusBadRequest) _, _ = io.WriteString(w, `{"errors":["bad callback"]}`) return @@ -60,6 +67,11 @@ func fakeOIDCVault(t *testing.T, wantRole string) (*httptest.Server, *string) { // fakeBrowser stands in for the IdP: it hits the local callback with query. func fakeBrowser(t *testing.T, redirect *string, query string) { + t.Helper() + fakeBrowserDo(t, func() (*http.Response, error) { return http.Get(*redirect + "?" + query) }) +} + +func fakeBrowserDo(t *testing.T, do func() (*http.Response, error)) { t.Helper() prev := OpenBrowser OpenBrowser = func(u string) error { @@ -67,7 +79,7 @@ func fakeBrowser(t *testing.T, redirect *string, query string) { t.Errorf("browser url = %q", u) } go func() { - resp, err := http.Get(*redirect + "?" + query) + resp, err := do() if err == nil { _ = resp.Body.Close() } @@ -136,3 +148,31 @@ func TestLoginOIDCNoAuthURL(t *testing.T) { t.Errorf("want no auth_url error, got %v", err) } } + +func TestLoginOIDCFormPost(t *testing.T) { + srv, redirect := fakeOIDCVault(t, "") + fakeBrowserDo(t, func() (*http.Response, error) { + return http.PostForm(*redirect, url.Values{"state": {"st1"}, "id_token": {"idt"}}) + }) + rc := ResolvedContext{Name: "sso", Address: srv.URL, Method: "oidc", Path: "sso", Namespace: "team-a", OIDCListen: freeListen(t)} + tok, err := LoginOIDC(rc) + if err != nil || tok.Token != "s.oidc" { + t.Fatalf("form_post login: tok=%+v err=%v", tok, err) + } +} + +func TestLoginOIDCRejectsNonLoopbackListen(t *testing.T) { + for _, l := range []string{":8250", "0.0.0.0:8250", "192.0.2.1:8250", "localhost"} { + rc := ResolvedContext{Name: "sso", Address: "http://unused", Method: "oidc", Path: "sso", OIDCListen: l} + if _, err := LoginOIDC(rc); err == nil || !strings.Contains(err.Error(), "oidc listen address") { + t.Errorf("%q: want listen error, got %v", l, err) + } + } +} + +func TestDoRequestErrorOmitsQuery(t *testing.T) { + _, err := doRequest(http.MethodGet, "http://127.0.0.1:1", "auth/sso/oidc/callback?code=secretcode&state=st", "", "", nil) + if err == nil || strings.Contains(err.Error(), "secretcode") || !strings.Contains(err.Error(), "/v1/auth/sso/oidc/callback") { + t.Errorf("error = %v", err) + } +} diff --git a/shared/vault.go b/shared/vault.go index 23abcca..5539cbb 100644 --- a/shared/vault.go +++ b/shared/vault.go @@ -3,9 +3,11 @@ package shared import ( "bytes" "encoding/json" + "errors" "fmt" "io" "net/http" + "net/url" "strings" "time" ) @@ -79,8 +81,8 @@ func doRequest(method, address, path, namespace, token string, payload any) ([]b body = bytes.NewReader(b) } - url := strings.TrimRight(address, "/") + "/v1/" + strings.TrimLeft(path, "/") - req, err := http.NewRequest(method, url, body) + reqURL := strings.TrimRight(address, "/") + "/v1/" + strings.TrimLeft(path, "/") + req, err := http.NewRequest(method, reqURL, body) if err != nil { return nil, fmt.Errorf("building request: %w", err) } @@ -96,7 +98,13 @@ func doRequest(method, address, path, namespace, token string, payload any) ([]b resp, err := httpClient.Do(req) if err != nil { - return nil, fmt.Errorf("request to %s failed: %w", url, err) + // Query strings can carry OIDC code/state/nonce; keep them out of errors. + bare, _, _ := strings.Cut(reqURL, "?") + var ue *url.Error + if errors.As(err, &ue) { + ue.URL = bare + } + return nil, fmt.Errorf("request to %s failed: %w", bare, err) } defer func() { _ = resp.Body.Close() }()