From f369ea5f52b666c877aa66377ca514b5ef625b7e Mon Sep 17 00:00:00 2001 From: jbergner Date: Mon, 24 Aug 2026 22:19:20 +0200 Subject: [PATCH] Major Bugfix --- CHANGELOG.md | 9 +++ README.md | 2 +- configs/agent.example.json | 1 + configs/master.example.json | 1 + docs/ACCESS-AUTH.md | 10 ++- docs/POCKET-ID-HILDEN.md | 101 ++++++++++++++++++++++++ internal/auth/access.go | 17 ++-- internal/auth/oidc.go | 105 ++++++++++++++++++++----- internal/auth/oidc_pkce_logout_test.go | 85 ++++++++++++++++++++ internal/config/config.go | 12 +++ internal/config/config_test.go | 14 ++++ internal/model/types.go | 13 +-- 12 files changed, 332 insertions(+), 38 deletions(-) create mode 100644 docs/POCKET-ID-HILDEN.md create mode 100644 internal/auth/oidc_pkce_logout_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 6a32ad1..2f23baa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,14 @@ # Changelog +## v0.5.2 PKCE / logout hotfix + +- Added PKCE S256 (`code_challenge` / `code_verifier`) to both SessionGuard OIDC authorization-code flows. +- Fixed the Go 1.22+ ServeMux root route conflict by registering `GET /{$}`. +- Added RP-initiated logout for the primary Director/Admin OIDC flow using the discovered `end_session_endpoint`. +- Guacamole access logout now sends `client_id` on every end-session request and `id_token_hint` when available. +- Added `oidc.logout_redirect_url` and documented Pocket ID Logout Callback URL requirements. + + ## 0.5.2 — Public EdgeGuard security layer - Added `sessionguard-edgeguard`, a dependency-free Go edge pre-check for the public Caddy host. diff --git a/README.md b/README.md index 8eddf27..170dc6b 100644 --- a/README.md +++ b/README.md @@ -111,7 +111,7 @@ This keeps PocketID / header authentication and the browser gateway separate fro - Append-only audit and session-history tables with retention and indexes. - JSONB control-plane state for agents, farms, resources, leases, policies and alerts. - JSON-file persistence remains available as a small single-node fallback / migration path. -- PocketID / generic OIDC authentication. +- PocketID / generic OIDC authentication with Authorization Code + PKCE (S256). - Group-to-role RBAC. - Policy version history and rollback for global, farm and agent policies. - Agent-initiated HTTPS heartbeats and command delivery only; no inbound management port from master to RDS hosts. diff --git a/configs/agent.example.json b/configs/agent.example.json index 2a366ec..ed00959 100644 --- a/configs/agent.example.json +++ b/configs/agent.example.json @@ -10,6 +10,7 @@ "client_id": "POCKET-ID-AGENT-CLIENT-ID", "client_secret": "POCKET-ID-AGENT-CLIENT-SECRET", "redirect_url": "https://ts01-mgmt.example.org/oidc/callback", + "logout_redirect_url": "https://ts01-mgmt.example.org/", "admin_groups": [ "sessionguard-admins" ], diff --git a/configs/master.example.json b/configs/master.example.json index f9b1883..0ff43d4 100644 --- a/configs/master.example.json +++ b/configs/master.example.json @@ -11,6 +11,7 @@ "client_id": "SESSIONGUARD-POCKETID-CLIENT-ID", "client_secret": "SET-BY-SESSIONGUARD_OIDC_CLIENT_SECRET", "redirect_url": "https://sessionguard.example.org/oidc/callback", + "logout_redirect_url": "https://sessionguard.example.org/", "admin_groups": [], "secure_cookie": true }, diff --git a/docs/ACCESS-AUTH.md b/docs/ACCESS-AUTH.md index 3e0557d..bdb21f8 100644 --- a/docs/ACCESS-AUTH.md +++ b/docs/ACCESS-AUTH.md @@ -59,16 +59,18 @@ The browser cookie contains only a cryptographically-random opaque token. Sessio Configure the PocketID client with: ```text +Public client: OFF +PKCE: ON Callback URL: https://guacamole.example.org/_sessionguard/auth/oidc/callback -Logout Callback / Back-channel Logout URL: -https://guacamole.example.org/_sessionguard/auth/backchannel-logout +Logout Callback URL (post_logout_redirect_uri): +https://guacamole.example.org/ ``` -PocketID can send OIDC back-channel logout tokens to the latter URL. SessionGuard verifies signature, issuer, audience, event claim, `iat`, `jti`, and `sid`/`sub`, rejects replayed logout tokens, and revokes matching local access sessions. +The Pocket ID field **Logout Callback URLs** is the allow-list for `post_logout_redirect_uri`; do not put the SessionGuard back-channel endpoint into that field. If the provider exposes a dedicated Back-Channel Logout URI setting, SessionGuard's endpoint is `https://guacamole.example.org/_sessionguard/auth/backchannel-logout`. -For browser-initiated logout SessionGuard uses the `end_session_endpoint` discovered from PocketID, removes the local session first, then returns to `logout_redirect_url`. +For browser-initiated logout SessionGuard uses the `end_session_endpoint` discovered from PocketID, removes the local session first, sends `client_id` plus an `id_token_hint` when available, and then returns to `logout_redirect_url`. ## Traefik diff --git a/docs/POCKET-ID-HILDEN.md b/docs/POCKET-ID-HILDEN.md new file mode 100644 index 0000000..d77817d --- /dev/null +++ b/docs/POCKET-ID-HILDEN.md @@ -0,0 +1,101 @@ +# Pocket ID settings for the Hilden deployment + +There is one OIDC provider (`https://login.hilden.de`) and two relevant Pocket ID OIDC clients. SessionGuard v0.5.2 with this hotfix uses confidential Authorization Code clients with PKCE S256 and RP-initiated logout. + +## Client 1: Guacamole Director + +Use these settings in Pocket ID: + +- Name: `Guacamole Director` +- Public Client: **OFF** +- PKCE: **ON** +- Client secret: **keep the existing secret** +- Client start URL: `https://director.hilden.de` +- Require re-authentication: **OFF** unless you intentionally want a fresh Pocket ID authentication on every authorization +- Skip consent screen: optional; it does not affect PKCE or logout +- Federated Client Credentials: **not required** for SessionGuard's browser login; remove them unless another workload intentionally uses them + +Callback URLs: + +- `https://director.hilden.de/oidc/callback` +- `https://gts01.hilden.de:9091/oidc/callback` +- `https://gts02.hilden.de:9091/oidc/callback` +- `https://gts03.hilden.de:9091/oidc/callback` + +Logout Callback URLs (Pocket ID allow-list for `post_logout_redirect_uri`): + +- `https://director.hilden.de/` +- `https://gts01.hilden.de:9091/` +- `https://gts02.hilden.de:9091/` +- `https://gts03.hilden.de:9091/` + +The SessionGuard instance must use the callback/logout pair for the hostname through which that instance is opened. Example for the Director URL: + +```json +"oidc": { + "issuer": "https://login.hilden.de", + "client_id": "307fde28-7d87-4671-9c7a-201d2332a617", + "client_secret": "SET-BY-SESSIONGUARD_OIDC_CLIENT_SECRET", + "redirect_url": "https://director.hilden.de/oidc/callback", + "logout_redirect_url": "https://director.hilden.de/", + "admin_groups": [], + "secure_cookie": true +} +``` + +For a directly opened agent UI, use its exact pair instead, e.g. `https://gts01.hilden.de:9091/oidc/callback` and `https://gts01.hilden.de:9091/`. + +## Client 2: Guacamole (portal) + +Use these settings in Pocket ID: + +- Name: `Guacamole` +- Public Client: **OFF** +- PKCE: **ON** +- Client secret: **keep the existing secret** +- Client start URL: `https://portal.hilden.de` +- Require re-authentication: **OFF** unless explicitly desired +- Skip consent screen: optional +- Federated Client Credentials: **not required** for SessionGuard's browser login + +Callback URL used by SessionGuard: + +- `https://portal.hilden.de/_sessionguard/auth/oidc/callback` + +Logout Callback URL: + +- `https://portal.hilden.de/` + +SessionGuard configuration: + +```json +"access_auth": { + "enabled": true, + "issuer": "https://login.hilden.de", + "client_id": "170f9724-2d12-45c4-8e6f-e1e559be4f72", + "client_secret": "SET-BY-SESSIONGUARD_ACCESS_OIDC_CLIENT_SECRET", + "redirect_url": "https://portal.hilden.de/_sessionguard/auth/oidc/callback", + "logout_redirect_url": "https://portal.hilden.de/", + "cookie_name": "sg_access_session", + "cookie_domain": "", + "secure_cookie": true, + "session_hours": 8, + "username_claim": "preferred_username", + "allowed_groups": [], + "allowed_hosts": ["portal.hilden.de"] +} +``` + +If `https://guacauth.hilden.info/portals/guacamole/oauth2/callback` is no longer used by a separate native Guacamole OIDC integration, remove it from Pocket ID. Keep it only if that second integration is still actually in use. + +## Logout behavior in this hotfix + +Both SessionGuard flows discover Pocket ID's `end_session_endpoint`. On logout SessionGuard clears its local session and redirects the browser to Pocket ID with: + +- `client_id` +- `id_token_hint` when a successful SessionGuard login supplied one +- the configured and registered `post_logout_redirect_uri` + +For the Director/Admin/Agent flow the hotfix retains the ID token server-side for the lifetime of the local browser session so it can be supplied as `id_token_hint` without placing the raw ID token in a browser cookie. + +The existing SessionGuard `/auth/backchannel-logout` handler is separate from the browser post-logout redirect. Do not enter that endpoint as the normal browser Logout Callback URL unless your Pocket ID version/integration explicitly provides a dedicated back-channel logout setting and calls it as such. diff --git a/internal/auth/access.go b/internal/auth/access.go index b28402f..e99057f 100644 --- a/internal/auth/access.go +++ b/internal/auth/access.go @@ -30,9 +30,10 @@ type AccessSessionStore interface { } type accessPending struct { - Nonce string - ReturnURL string - Exp time.Time + Nonce string + CodeVerifier string + ReturnURL string + Exp time.Time } type AccessManager struct { @@ -100,9 +101,10 @@ func (m *AccessManager) Login(w http.ResponseWriter, r *http.Request) { _ = m.sessions.CleanupAuthSessions(time.Now().UTC()) target := m.validReturnURL(r.URL.Query().Get("return")) state, nonce := randomAccessToken(24), randomAccessToken(24) + verifier := oauth2.GenerateVerifier() m.mu.Lock() m.prunePendingLocked(time.Now()) - m.pending[state] = accessPending{Nonce: nonce, ReturnURL: target, Exp: time.Now().Add(5 * time.Minute)} + m.pending[state] = accessPending{Nonce: nonce, CodeVerifier: verifier, ReturnURL: target, Exp: time.Now().Add(5 * time.Minute)} m.mu.Unlock() // One state cookie per login flow avoids the common multi-tab race where a @@ -112,7 +114,7 @@ func (m *AccessManager) Login(w http.ResponseWriter, r *http.Request) { HttpOnly: true, Secure: m.cfg.SecureCookie, SameSite: http.SameSiteLaxMode, MaxAge: 300, }) - http.Redirect(w, r, m.oauth.AuthCodeURL(state, oidc.Nonce(nonce)), http.StatusFound) + http.Redirect(w, r, m.oauth.AuthCodeURL(state, oidc.Nonce(nonce), oauth2.S256ChallengeOption(verifier)), http.StatusFound) } func (m *AccessManager) Callback(w http.ResponseWriter, r *http.Request) { @@ -142,7 +144,7 @@ func (m *AccessManager) Callback(w http.ResponseWriter, r *http.Request) { return } - tok, err := m.oauth.Exchange(r.Context(), r.URL.Query().Get("code")) + tok, err := m.oauth.Exchange(r.Context(), r.URL.Query().Get("code"), oauth2.VerifierOption(p.CodeVerifier)) if err != nil { http.Error(w, "OIDC token exchange failed", http.StatusUnauthorized) return @@ -257,10 +259,9 @@ func (m *AccessManager) Logout(w http.ResponseWriter, r *http.Request) { return } q := u.Query() + q.Set("client_id", m.cfg.ClientID) if sess.IDToken != "" { q.Set("id_token_hint", sess.IDToken) - } else { - q.Set("client_id", m.cfg.ClientID) } if target != "" { q.Set("post_logout_redirect_uri", target) diff --git a/internal/auth/oidc.go b/internal/auth/oidc.go index 90271a1..df84144 100644 --- a/internal/auth/oidc.go +++ b/internal/auth/oidc.go @@ -10,6 +10,7 @@ import ( "errors" "fmt" "net/http" + "net/url" "strings" "sync" "time" @@ -28,18 +29,26 @@ type User struct { } type pending struct { - Nonce string - Exp time.Time + Nonce string + CodeVerifier string + Exp time.Time +} + +type logoutSession struct { + IDToken string + Exp time.Time } type Manager struct { - cfg model.OIDCConfig - provider *oidc.Provider - verifier *oidc.IDTokenVerifier - oauth oauth2.Config - key []byte - mu sync.Mutex - pending map[string]pending + cfg model.OIDCConfig + provider *oidc.Provider + verifier *oidc.IDTokenVerifier + oauth oauth2.Config + endSession string + key []byte + mu sync.Mutex + pending map[string]pending + logout map[string]logoutSession } func New(ctx context.Context, cfg model.OIDCConfig) (*Manager, error) { @@ -50,17 +59,23 @@ func New(ctx context.Context, cfg model.OIDCConfig) (*Manager, error) { if err != nil { return nil, err } + var discovery struct { + EndSessionEndpoint string `json:"end_session_endpoint"` + } + _ = p.Claims(&discovery) key := make([]byte, 32) if _, err := rand.Read(key); err != nil { return nil, err } return &Manager{ - cfg: cfg, - provider: p, - verifier: p.Verifier(&oidc.Config{ClientID: cfg.ClientID}), - oauth: oauth2.Config{ClientID: cfg.ClientID, ClientSecret: cfg.ClientSecret, Endpoint: p.Endpoint(), RedirectURL: cfg.RedirectURL, Scopes: []string{oidc.ScopeOpenID, "profile", "email", "groups"}}, - key: key, - pending: map[string]pending{}, + cfg: cfg, + provider: p, + verifier: p.Verifier(&oidc.Config{ClientID: cfg.ClientID}), + oauth: oauth2.Config{ClientID: cfg.ClientID, ClientSecret: cfg.ClientSecret, Endpoint: p.Endpoint(), RedirectURL: cfg.RedirectURL, Scopes: []string{oidc.ScopeOpenID, "profile", "email", "groups"}}, + endSession: discovery.EndSessionEndpoint, + key: key, + pending: map[string]pending{}, + logout: map[string]logoutSession{}, }, nil } @@ -72,11 +87,13 @@ func randomURLSafe(n int) string { func (m *Manager) Login(w http.ResponseWriter, r *http.Request) { state, nonce := randomURLSafe(24), randomURLSafe(24) + verifier := oauth2.GenerateVerifier() m.mu.Lock() - m.pending[state] = pending{Nonce: nonce, Exp: time.Now().Add(5 * time.Minute)} + m.pruneLocked(time.Now()) + m.pending[state] = pending{Nonce: nonce, CodeVerifier: verifier, Exp: time.Now().Add(5 * time.Minute)} m.mu.Unlock() http.SetCookie(w, &http.Cookie{Name: "sg_oidc_state", Value: state, Path: "/oidc/callback", HttpOnly: true, Secure: m.cfg.SecureCookie, SameSite: http.SameSiteLaxMode, MaxAge: 300}) - http.Redirect(w, r, m.oauth.AuthCodeURL(state, oidc.Nonce(nonce)), http.StatusFound) + http.Redirect(w, r, m.oauth.AuthCodeURL(state, oidc.Nonce(nonce), oauth2.S256ChallengeOption(verifier)), http.StatusFound) } func (m *Manager) Callback(w http.ResponseWriter, r *http.Request) error { @@ -96,7 +113,7 @@ func (m *Manager) Callback(w http.ResponseWriter, r *http.Request) error { if !ok || time.Now().After(p.Exp) { return errors.New("invalid or expired OIDC state") } - tok, err := m.oauth.Exchange(r.Context(), r.URL.Query().Get("code")) + tok, err := m.oauth.Exchange(r.Context(), r.URL.Query().Get("code"), oauth2.VerifierOption(p.CodeVerifier)) if err != nil { return err } @@ -126,6 +143,10 @@ func (m *Manager) Callback(w http.ResponseWriter, r *http.Request) error { if err != nil { return err } + m.mu.Lock() + m.pruneLocked(time.Now()) + m.logout[value] = logoutSession{IDToken: raw, Exp: time.Unix(u.Exp, 0)} + m.mu.Unlock() http.SetCookie(w, &http.Cookie{Name: "sg_session", Value: value, Path: "/", HttpOnly: true, Secure: m.cfg.SecureCookie, SameSite: http.SameSiteLaxMode, MaxAge: 8 * 3600}) return nil } @@ -147,8 +168,54 @@ func (m *Manager) allowed(u User) bool { } func (m *Manager) Logout(w http.ResponseWriter, r *http.Request) { + var idToken string + if c, err := r.Cookie("sg_session"); err == nil && c.Value != "" { + m.mu.Lock() + m.pruneLocked(time.Now()) + if sess, ok := m.logout[c.Value]; ok { + idToken = sess.IDToken + delete(m.logout, c.Value) + } + m.mu.Unlock() + } http.SetCookie(w, &http.Cookie{Name: "sg_session", Value: "", Path: "/", HttpOnly: true, Secure: m.cfg.SecureCookie, SameSite: http.SameSiteLaxMode, MaxAge: -1}) - http.Redirect(w, r, "/", http.StatusFound) + + target := strings.TrimSpace(m.cfg.LogoutRedirectURL) + if target == "" { + target = "/" + } + if m.endSession == "" { + http.Redirect(w, r, target, http.StatusFound) + return + } + u, err := url.Parse(m.endSession) + if err != nil { + http.Redirect(w, r, target, http.StatusFound) + return + } + q := u.Query() + q.Set("client_id", m.cfg.ClientID) + if idToken != "" { + q.Set("id_token_hint", idToken) + } + if strings.HasPrefix(target, "https://") || strings.HasPrefix(target, "http://") { + q.Set("post_logout_redirect_uri", target) + } + u.RawQuery = q.Encode() + http.Redirect(w, r, u.String(), http.StatusFound) +} + +func (m *Manager) pruneLocked(now time.Time) { + for state, p := range m.pending { + if !now.Before(p.Exp) { + delete(m.pending, state) + } + } + for session, p := range m.logout { + if !now.Before(p.Exp) { + delete(m.logout, session) + } + } } func (m *Manager) sign(u User) (string, error) { diff --git a/internal/auth/oidc_pkce_logout_test.go b/internal/auth/oidc_pkce_logout_test.go new file mode 100644 index 0000000..ee1a28f --- /dev/null +++ b/internal/auth/oidc_pkce_logout_test.go @@ -0,0 +1,85 @@ +package auth + +import ( + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + "time" + + "github.com/example/sessionguard/internal/model" + "golang.org/x/oauth2" +) + +func TestManagerLoginUsesPKCES256(t *testing.T) { + m := &Manager{ + cfg: model.OIDCConfig{SecureCookie: true}, + oauth: oauth2.Config{ + ClientID: "client-1", + RedirectURL: "https://director.example/oidc/callback", + Endpoint: oauth2.Endpoint{AuthURL: "https://login.example/authorize"}, + }, + pending: map[string]pending{}, + logout: map[string]logoutSession{}, + } + r := httptest.NewRequest("GET", "https://director.example/oidc/login", nil) + w := httptest.NewRecorder() + m.Login(w, r) + + loc, err := url.Parse(w.Header().Get("Location")) + if err != nil { + t.Fatal(err) + } + if got := loc.Query().Get("code_challenge_method"); got != "S256" { + t.Fatalf("code_challenge_method=%q, want S256", got) + } + if got := loc.Query().Get("code_challenge"); got == "" { + t.Fatal("missing code_challenge") + } + state := loc.Query().Get("state") + m.mu.Lock() + p, ok := m.pending[state] + m.mu.Unlock() + if !ok || p.CodeVerifier == "" { + t.Fatal("PKCE verifier was not retained for callback exchange") + } +} + +func TestManagerLogoutUsesIDTokenHintAndRegisteredRedirect(t *testing.T) { + const session = "signed-session-cookie" + m := &Manager{ + cfg: model.OIDCConfig{ + ClientID: "director-client", + LogoutRedirectURL: "https://director.example/", + SecureCookie: true, + }, + endSession: "https://login.example/api/oidc/end-session", + pending: map[string]pending{}, + logout: map[string]logoutSession{ + session: logoutSession{IDToken: "header.payload.signature", Exp: time.Now().Add(time.Hour)}, + }, + } + r := httptest.NewRequest("GET", "https://director.example/logout", nil) + r.AddCookie(&http.Cookie{Name: "sg_session", Value: session}) + w := httptest.NewRecorder() + m.Logout(w, r) + + loc, err := url.Parse(w.Header().Get("Location")) + if err != nil { + t.Fatal(err) + } + q := loc.Query() + if q.Get("client_id") != "director-client" { + t.Fatalf("client_id=%q", q.Get("client_id")) + } + if q.Get("id_token_hint") != "header.payload.signature" { + t.Fatalf("id_token_hint=%q", q.Get("id_token_hint")) + } + if q.Get("post_logout_redirect_uri") != "https://director.example/" { + t.Fatalf("post_logout_redirect_uri=%q", q.Get("post_logout_redirect_uri")) + } + if !strings.Contains(w.Header().Get("Set-Cookie"), "sg_session=") { + t.Fatal("local session cookie was not cleared") + } +} diff --git a/internal/config/config.go b/internal/config/config.go index 9f3ef98..103d202 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -222,6 +222,18 @@ func validateOIDC(c model.OIDCConfig) error { if c.Issuer == "" || c.ClientID == "" || c.RedirectURL == "" { return errors.New("oidc issuer, client_id and redirect_url are required") } + for label, raw := range map[string]string{"redirect_url": c.RedirectURL, "logout_redirect_url": c.LogoutRedirectURL} { + if strings.TrimSpace(raw) == "" { + continue + } + u, err := url.Parse(strings.TrimSpace(raw)) + if err != nil || u.Hostname() == "" || u.Scheme == "" { + return fmt.Errorf("oidc.%s must be an absolute URL", label) + } + if c.SecureCookie && !strings.EqualFold(u.Scheme, "https") { + return fmt.Errorf("oidc.%s must use https when secure_cookie is enabled", label) + } + } return nil } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 56509ff..b1036b7 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -74,3 +74,17 @@ func TestValidateAccessAuthRequiresClientSecret(t *testing.T) { t.Fatal("expected missing client secret validation error") } } + +func TestValidateOIDCLogoutRedirectRequiresHTTPSForSecureCookie(t *testing.T) { + c := model.OIDCConfig{ + Issuer: "https://id.example.org", ClientID: "client", ClientSecret: "secret", + RedirectURL: "https://director.example.org/oidc/callback", LogoutRedirectURL: "http://director.example.org/", SecureCookie: true, + } + if err := validateOIDC(c); err == nil { + t.Fatal("expected https validation error") + } + c.LogoutRedirectURL = "https://director.example.org/" + if err := validateOIDC(c); err != nil { + t.Fatal(err) + } +} diff --git a/internal/model/types.go b/internal/model/types.go index 381dbfa..2423518 100644 --- a/internal/model/types.go +++ b/internal/model/types.go @@ -5,12 +5,13 @@ import "time" const ProtocolVersion = 4 type OIDCConfig struct { - Issuer string `json:"issuer"` - ClientID string `json:"client_id"` - ClientSecret string `json:"client_secret"` - RedirectURL string `json:"redirect_url"` - AdminGroups []string `json:"admin_groups,omitempty"` - SecureCookie bool `json:"secure_cookie"` + Issuer string `json:"issuer"` + ClientID string `json:"client_id"` + ClientSecret string `json:"client_secret"` + RedirectURL string `json:"redirect_url"` + LogoutRedirectURL string `json:"logout_redirect_url,omitempty"` + AdminGroups []string `json:"admin_groups,omitempty"` + SecureCookie bool `json:"secure_cookie"` } // AccessAuthConfig configures the SessionGuard Master as a Traefik ForwardAuth