Major Bugfix

2026-08-24 22:19:20 +02:00
parent bb91e01c89
commit f369ea5f52
12 changed files with 332 additions and 38 deletions
--- 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)
--- 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) {
--- a/internal/auth/oidc_pkce_logout_test.go
+++ 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")
+	}
+}
--- 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
 }

--- 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)
+	}
+}
--- 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