From e62df0027d76dec256986166f1f75242ffe01a41 Mon Sep 17 00:00:00 2001 From: Mathias Date: Wed, 3 Jun 2026 15:56:13 +0200 Subject: [PATCH] refactor(oidc): drop single-subject allowlist, authenticate-only (ADR-012) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ADR-011's single-user authz (ID-token subject must equal AllowedSubject, else 403) is replaced by ADR-012's model: Dex authentication is the only gate — any Dex-authenticated subject may establish a session. Whether that subject has a tapir user, and routing to registration if not, is decided downstream in internal/web (next commit). Removals (noted): oidc.Config.AllowedSubject + its required-field check + the callback 403 branch; config.Config.AllowedSubject + TAPIR_ALLOWED_SUBJECT env wiring; the AllowedSubject arg in cmdServe. ui-spec.md updated to reflect the supersession. Sessions, cookie signing, login/callback/logout unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) --- cmd/tapir/main.go | 17 ++++++------ docs/ui-spec.md | 12 ++++---- internal/config/config.go | 8 ++---- internal/web/oidc/oidc.go | 41 ++++++++++++--------------- internal/web/oidc/oidc_test.go | 51 +++++++++++++++++++--------------- 5 files changed, 64 insertions(+), 65 deletions(-) diff --git a/cmd/tapir/main.go b/cmd/tapir/main.go index 2a5af41..448e733 100644 --- a/cmd/tapir/main.go +++ b/cmd/tapir/main.go @@ -164,18 +164,17 @@ func cmdServe(ctx context.Context, log *slog.Logger) error { defer st.Close() // Auth seam (handlers depend on web.Auth only). With Dex configured - // (TAPIR_OIDC_ISSUER set) serve uses real OIDC login with single-user - // allowlist authz (ADR-011); otherwise it falls back to the allow-all - // StubAuth for local dev — never expose StubAuth publicly. + // (TAPIR_OIDC_ISSUER set) serve uses real OIDC login — any Dex subject may + // authenticate, then registers a tapir user (ADR-012); otherwise it falls + // back to the allow-all StubAuth for local dev — never expose StubAuth publicly. var authn web.Auth if cfg.DexConfigured() { authn, err = oidc.New(ctx, oidc.Config{ - Issuer: cfg.OIDCIssuer, - ClientID: cfg.DexClientID, - ClientSecret: cfg.DexClientSecret, - RedirectURL: cfg.OIDCRedirectURL, - SessionSecret: cfg.SessionSecret, - AllowedSubject: cfg.AllowedSubject, + Issuer: cfg.OIDCIssuer, + ClientID: cfg.DexClientID, + ClientSecret: cfg.DexClientSecret, + RedirectURL: cfg.OIDCRedirectURL, + SessionSecret: cfg.SessionSecret, }) if err != nil { return fmt.Errorf("dex oidc: %w", err) diff --git a/docs/ui-spec.md b/docs/ui-spec.md index fc31fc7..4c653b0 100644 --- a/docs/ui-spec.md +++ b/docs/ui-spec.md @@ -77,8 +77,9 @@ summary_actions - **Flow:** standard Authorization Code. Use `coreos/go-oidc` + `golang.org/x/oauth2` (justify the deps in the commit; both are the homelab-standard OIDC libs and small). - Discover issuer `https://auth.d-ma.be` (`TAPIR_OIDC_ISSUER`); scopes `openid profile email`. -- On callback: verify ID token, extract `sub` (and email); **allowlist check** against - `TAPIR_ALLOWED_SUBJECT` (the maintainer's Dex subject) — reject everyone else with 403. +- On callback: verify ID token, extract `sub` (and email). **ADR-012 superseded the + ADR-011 single-subject allowlist:** any Dex-authenticated subject may sign in; a subject + with no tapir user is routed to explicit registration (see `internal/web` registration gate). - **Session:** signed, httpOnly, Secure cookie (HS256 with `TAPIR_SESSION_SECRET`); short TTL + sliding refresh. Server-side session store can be in-memory at Stage 0 (single replica). - **Middleware** guards every route except `/healthz` and `/auth/*`. @@ -89,8 +90,9 @@ summary_actions `TAPIR_HTTP_ADDR` (`:8080`), `TAPIR_PUBLIC_URL` (`https://tapir.d-ma.be`), `TAPIR_OIDC_ISSUER` (`https://auth.d-ma.be`), `TAPIR_DEX_CLIENT_ID`, `TAPIR_DEX_CLIENT_SECRET`, -`TAPIR_OIDC_REDIRECT_URL` (`https://tapir.d-ma.be/auth/callback`), `TAPIR_SESSION_SECRET`, -`TAPIR_ALLOWED_SUBJECT`. Reuses existing `TAPIR_DB_DSN`, `TAPIR_USER_ID`. No secrets committed. +`TAPIR_OIDC_REDIRECT_URL` (`https://tapir.d-ma.be/auth/callback`), `TAPIR_SESSION_SECRET`. +Reuses existing `TAPIR_DB_DSN`, `TAPIR_USER_ID` (the StubAuth dev subject only). No secrets +committed. (`TAPIR_ALLOWED_SUBJECT` was removed by ADR-012.) ## 8. Deployment — k3s + Flux GitOps @@ -117,7 +119,7 @@ summary_actions 1. **Register a Dex static client** `tapir-web` in the Dex config (in `infra`) with redirect `https://tapir.d-ma.be/auth/callback`; client id/secret → 1P `TAPIR_DEX_CLIENT_ID` / - `TAPIR_DEX_CLIENT_SECRET`. Capture your Dex `sub` for `TAPIR_ALLOWED_SUBJECT`. + `TAPIR_DEX_CLIENT_SECRET`. (No allowlist subject to capture — ADR-012 dropped it.) 2. **DNS/edge** for `tapir.d-ma.be` → the k3s ingress (piguard NPM perimeter / existing `*.d-ma.be` pattern) + TLS cert. 3. Confirm the **registry** host/path the gitea CI pushes to and the Flux path diff --git a/internal/config/config.go b/internal/config/config.go index da0ca84..ef298e6 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -57,15 +57,14 @@ type Config struct { // HTTPAddr is the listen address for `tapir serve` (the Stage-0 web UI). HTTPAddr string - // Dex OIDC (web login, ADR-011). When OIDCIssuer is empty, `serve` falls back - // to the allow-all StubAuth (local dev). When set, serve uses Dex with - // single-user allowlist authz. + // Dex OIDC (web login, ADR-011/012). When OIDCIssuer is empty, `serve` falls + // back to the allow-all StubAuth (local dev). When set, serve uses Dex: any + // Dex-authenticated subject may sign in, then registers a tapir user (ADR-012). OIDCIssuer string DexClientID string DexClientSecret string OIDCRedirectURL string SessionSecret string - AllowedSubject string } // DexConfigured reports whether Dex OIDC login is wired (issuer present). When @@ -104,7 +103,6 @@ func Load() (Config, error) { DexClientSecret: os.Getenv("TAPIR_DEX_CLIENT_SECRET"), OIDCRedirectURL: os.Getenv("TAPIR_OIDC_REDIRECT_URL"), SessionSecret: os.Getenv("TAPIR_SESSION_SECRET"), - AllowedSubject: os.Getenv("TAPIR_ALLOWED_SUBJECT"), } timeout, err := durationOr("TAPIR_SUMMARIZER_TIMEOUT", defaultSummarizerTimeout) diff --git a/internal/web/oidc/oidc.go b/internal/web/oidc/oidc.go index ba11f2b..57da714 100644 --- a/internal/web/oidc/oidc.go +++ b/internal/web/oidc/oidc.go @@ -4,11 +4,13 @@ // interface, so swapping the stub for Dex is a wiring choice in cmd/tapir, not // a code change (ADR-003). // -// Authentication is real (Dex OIDC); authorization is single-user — the ID -// token's subject must equal Config.AllowedSubject or the request is refused -// with 403. Sessions are server-side (in-memory, fine for the single Stage-0 -// replica) addressed by an HMAC-signed (HS256) HttpOnly Secure SameSite=Lax -// cookie with a short TTL and sliding refresh. Tokens are never logged. +// Authentication is real (Dex OIDC) and is the only gate: any Dex-authenticated +// subject may sign in (ADR-012 dropped ADR-011's single-subject allowlist). +// Authorization/registration is layered on top in internal/web (an authenticated +// subject with no tapir user is routed to registration). Sessions are server-side +// (in-memory, fine for the single Stage-1 replica) addressed by an HMAC-signed +// (HS256) HttpOnly Secure SameSite=Lax cookie with a short TTL and sliding +// refresh. Tokens are never logged. // // This is mcp-chassis's cousin but NOT the same code: mcp-chassis validates // inbound Bearer JWTs for MCP APIs; this is a browser session login. @@ -28,8 +30,8 @@ import ( ) // Config is the OIDC + session configuration. cmd/tapir maps these from -// TAPIR_OIDC_*/TAPIR_DEX_*/TAPIR_SESSION_SECRET/TAPIR_ALLOWED_SUBJECT; this -// package takes the resolved struct. +// TAPIR_OIDC_*/TAPIR_DEX_*/TAPIR_SESSION_SECRET; this package takes the resolved +// struct. type Config struct { // Issuer is the Dex issuer URL, e.g. https://auth.d-ma.be. Discovery // (.well-known/openid-configuration) runs against it in New. @@ -42,9 +44,6 @@ type Config struct { RedirectURL string // SessionSecret keys the HS256 session-cookie signature. Never logged. SessionSecret string - // AllowedSubject is the single Dex subject permitted to sign in. Everyone - // else is refused 403 (single-user authz, ADR-011). - AllowedSubject string } const ( @@ -102,12 +101,11 @@ func WithInsecureCookies() Option { // discovery request only. func New(ctx context.Context, cfg Config, opts ...Option) (*DexAuth, error) { for name, val := range map[string]string{ - "issuer": cfg.Issuer, - "client id": cfg.ClientID, - "client secret": cfg.ClientSecret, - "redirect url": cfg.RedirectURL, - "session secret": cfg.SessionSecret, - "allowed subject": cfg.AllowedSubject, + "issuer": cfg.Issuer, + "client id": cfg.ClientID, + "client secret": cfg.ClientSecret, + "redirect url": cfg.RedirectURL, + "session secret": cfg.SessionSecret, } { if strings.TrimSpace(val) == "" { return nil, fmt.Errorf("oidc: missing %s", name) @@ -242,14 +240,9 @@ func (d *DexAuth) handleCallback(w http.ResponseWriter, r *http.Request) { return } - // Single-user authz: only the allowlisted subject may sign in. On mismatch - // we echo the caller's own subject (an opaque id, not a secret) so the - // maintainer can bootstrap TAPIR_ALLOWED_SUBJECT on first login. - if idToken.Subject != d.cfg.AllowedSubject { - http.Error(w, "forbidden — not the allowlisted subject. your subject is: "+idToken.Subject, http.StatusForbidden) - return - } - + // Authentication is the only gate (ADR-012): any Dex-authenticated subject may + // establish a session. Whether that subject has a tapir user — and routing to + // registration if not — is decided downstream in internal/web, not here. var claims struct { Email string `json:"email"` } diff --git a/internal/web/oidc/oidc_test.go b/internal/web/oidc/oidc_test.go index b8d4783..929cdff 100644 --- a/internal/web/oidc/oidc_test.go +++ b/internal/web/oidc/oidc_test.go @@ -20,7 +20,7 @@ import ( const ( testClientID = "tapir-web" - allowedSub = "allowed-subject-123" + testSubject = "dex-subject-123" ) // fakeIssuer is an httptest-backed OIDC provider: it serves a discovery @@ -115,12 +115,11 @@ func writeJSON(t *testing.T, w http.ResponseWriter, v any) { func newAuth(t *testing.T, f *fakeIssuer) *oidc.DexAuth { t.Helper() auth, err := oidc.New(context.Background(), oidc.Config{ - Issuer: f.server.URL, - ClientID: testClientID, - ClientSecret: "test-client-secret", - RedirectURL: "http://tapir.test/auth/callback", - SessionSecret: "test-session-secret-please-change", - AllowedSubject: allowedSub, + Issuer: f.server.URL, + ClientID: testClientID, + ClientSecret: "test-client-secret", + RedirectURL: "http://tapir.test/auth/callback", + SessionSecret: "test-session-secret-please-change", }, oidc.WithInsecureCookies()) require.NoError(t, err) return auth @@ -140,12 +139,12 @@ func login(t *testing.T, auth *oidc.DexAuth) (state, nonce string) { return q.Get("state"), q.Get("nonce") } -// authenticate completes a full login+callback for the allowlisted subject and -// returns the resulting session cookie. +// authenticate completes a full login+callback for the test subject and returns +// the resulting session cookie. func authenticate(t *testing.T, auth *oidc.DexAuth, f *fakeIssuer) *http.Cookie { t.Helper() state, nonce := login(t, auth) - f.sub, f.email, f.nonce = allowedSub, "maintainer@d-ma.be", nonce + f.sub, f.email, f.nonce = testSubject, "maintainer@d-ma.be", nonce rec := httptest.NewRecorder() auth.Routes().ServeHTTP(rec, httptest.NewRequest(http.MethodGet, @@ -189,7 +188,7 @@ func TestLoginRedirectsToAuthorize(t *testing.T) { require.Contains(t, q.Get("scope"), "openid") } -func TestCallbackAllowedSubjectSetsSession(t *testing.T) { +func TestCallbackSetsSession(t *testing.T) { f := newFakeIssuer(t) auth := newAuth(t, f) @@ -199,26 +198,35 @@ func TestCallbackAllowedSubjectSetsSession(t *testing.T) { req.AddCookie(cookie) user, ok := auth.CurrentUser(req) require.True(t, ok) - require.Equal(t, allowedSub, user.Subject) + require.Equal(t, testSubject, user.Subject) require.Equal(t, "maintainer@d-ma.be", user.Email) require.True(t, cookie.HttpOnly) require.Equal(t, http.SameSiteLaxMode, cookie.SameSite) } -func TestCallbackNonAllowedSubjectForbidden(t *testing.T) { +// TestCallbackAnySubjectAuthenticates proves the single-subject allowlist is gone +// (ADR-012): a subject other than any prior allowlist still gets a session. +func TestCallbackAnySubjectAuthenticates(t *testing.T) { f := newFakeIssuer(t) auth := newAuth(t, f) state, nonce := login(t, auth) - f.sub, f.email, f.nonce = "intruder-999", "intruder@elsewhere.test", nonce + f.sub, f.email, f.nonce = "some-other-subject-999", "other@elsewhere.test", nonce rec := httptest.NewRecorder() auth.Routes().ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/auth/callback?code=valid-code&state="+state, nil)) - require.Equal(t, http.StatusForbidden, rec.Code) - require.Empty(t, rec.Result().Cookies(), "no session for a rejected subject") + require.Equal(t, http.StatusFound, rec.Code) + require.Equal(t, "/", rec.Header().Get("Location")) + + cookie := sessionCookie(t, rec.Result()) + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.AddCookie(cookie) + user, ok := auth.CurrentUser(req) + require.True(t, ok) + require.Equal(t, "some-other-subject-999", user.Subject) } func TestCallbackUnknownStateRejected(t *testing.T) { @@ -304,12 +312,11 @@ func TestExpiredSessionRejected(t *testing.T) { f := newFakeIssuer(t) clock := time.Now() auth, err := oidc.New(context.Background(), oidc.Config{ - Issuer: f.server.URL, - ClientID: testClientID, - ClientSecret: "test-client-secret", - RedirectURL: "http://tapir.test/auth/callback", - SessionSecret: "test-session-secret-please-change", - AllowedSubject: allowedSub, + Issuer: f.server.URL, + ClientID: testClientID, + ClientSecret: "test-client-secret", + RedirectURL: "http://tapir.test/auth/callback", + SessionSecret: "test-session-secret-please-change", }, oidc.WithInsecureCookies(), oidc.WithSessionTTL(time.Minute), oidc.WithClock(func() time.Time { return clock }))