admin/felhom-controller
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

R-35: an ended session cannot return after a failed save (password fingerprint in the file; a failed revoking save removes it) — security review

Browse Source 
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0159rPz1ZhFKsS53msqPYxtS
This commit is contained in:
authentik Default Admin
2026-10-08 14:40:48 +02:00
parent 1040cfe225
commit 740339567a
3 changed files with 90 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files
controller/internal/web/auth.go
+2 -2
View File
@@ -240,7 +240,7 @@ func (s *Server) handleLogout(w http.ResponseWriter, r *http.Request) {
if cookie, err := r.Cookie(sessionCookieName); err == nil {
s.sessionsMu.Lock()
delete(s.sessions, sessionFingerprint(cookie.Value))
_ = s.saveSessionsLocked() // R-35: a signed-out cookie stays signed out across a restart
s.saveSessionsRevokingLocked() // R-35: a signed-out cookie stays signed out across a restart
s.sessionsMu.Unlock()
}
s.logger.Printf("[INFO] [web] User logged out from %s", r.RemoteAddr)
@@ -299,7 +299,7 @@ func (s *Server) invalidateAllSessions() {
s.sessionsMu.Lock()
count := len(s.sessions)
s.sessions = make(map[string]*session)
_ = s.saveSessionsLocked() // R-35: the password change ends every session on disk too
s.saveSessionsRevokingLocked() // R-35: the password change ends every session on disk too
s.sessionsMu.Unlock()
s.logger.Printf("[INFO] [web] All sessions invalidated (cleared %d)", count)
}
controller/internal/web/session_store.go
+37 -3
View File
@@ -26,14 +26,22 @@ import (
// invalidateAllSessions (password change, claim reset) write the file at once, so an ended session stays ended
// across a restart (TestR35_LogoutEndsSessionAcrossRestart, TestR35_InvalidateAllEndsSessionAcrossRestart).
//
// Two guards keep a REVOKED session from coming back if a write fails (security review 2026-10-08):
// - the file carries a fingerprint of the password hash in force when it was written (CredentialFP); at load, rows
// written under another password are dropped — a password change revokes on disk even if its own save failed
// (TestR35_PasswordChangeRevokesEvenIfSaveFailed);
// - a revoking save (logout, invalidateAllSessions) that fails removes the file instead, so the restart starts with
// no sessions rather than the stale ones (TestR35_FailedLogoutSaveRemovesFile).
//
// A missing file is normal; an unreadable or corrupt one is logged and the server starts with no sessions — never
// fatal, and the failure direction is "sign in again", never "signed in" (TestR35_CorruptFileStartsEmpty).
const sessionsFileName = "dashboard-sessions.json"
type sessionsFile struct {
Version int `json:"version"`
Sessions []sessionDisk `json:"sessions"`
Version int `json:"version"`
CredentialFP string `json:"credential_fp"`
Sessions []sessionDisk `json:"sessions"`
}
type sessionDisk struct {
@@ -48,6 +56,13 @@ func sessionFingerprint(token string) string {
return hex.EncodeToString(sum[:])
}
// credentialFingerprint is hex(sha256(the password hash in force)): it changes whenever the password does, from either
// source (settings.json or controller.yaml), and reveals nothing the bcrypt hash itself does not.
func (s *Server) credentialFingerprint() string {
sum := sha256.Sum256([]byte("felhom-dashboard-sessions\x00" + s.effectivePasswordHash()))
return hex.EncodeToString(sum[:])
}
func (s *Server) sessionsPath() string {
if s.cfg == nil || s.cfg.Paths.DataDir == "" {
return ""
@@ -75,6 +90,10 @@ func (s *Server) loadSessions() {
logx.Warnf(s.logger, "[web] sessions: %s is not valid JSON, starting with none: %v", path, err)
return
}
if f.CredentialFP != s.credentialFingerprint() {
logx.Infof(s.logger, "[web] sessions: %d session(s) on disk were written under another password — dropped, sign in again", len(f.Sessions))
return
}
now := time.Now()
loaded, expired, bad := 0, 0, 0
s.sessionsMu.Lock()
@@ -102,7 +121,7 @@ func (s *Server) saveSessionsLocked() error {
return nil
}
now := time.Now()
f := sessionsFile{Version: 1, Sessions: []sessionDisk{}}
f := sessionsFile{Version: 1, CredentialFP: s.credentialFingerprint(), Sessions: []sessionDisk{}}
for fp, sess := range s.sessions {
if !now.Before(sess.expiresAt) {
continue
@@ -122,6 +141,21 @@ func (s *Server) saveSessionsLocked() error {
return nil
}
// saveSessionsRevokingLocked is the save for a path that ENDS sessions (logout, invalidateAllSessions). If the write
// fails, the file is removed so a restart cannot bring an ended session back; a household then signs in again, which
// is the safe direction. Caller holds sessionsMu for writing.
func (s *Server) saveSessionsRevokingLocked() {
if err := s.saveSessionsLocked(); err == nil {
return
}
path := s.sessionsPath()
if err := os.Remove(path); err != nil && !errors.Is(err, os.ErrNotExist) {
logx.Errorf(s.logger, "[web] sessions: could not save NOR remove %s after ending a session — an ended session may return after a restart until the password changes: %v", path, err)
return
}
logx.Warnf(s.logger, "[web] sessions: save failed while ending a session — removed %s instead (every session ends at the next restart)", sessionsFileName)
}
// writeSessionsAtomic is tmp + fsync + rename at 0600 — the family.json shape (internal/family saveLocked).
func writeSessionsAtomic(path string, b []byte) error {
tmp := path + ".tmp"
controller/internal/web/session_store_test.go
+51
View File
@@ -195,3 +195,54 @@ func TestR35_CorruptFileStartsEmpty(t *testing.T) {
t.Fatal("after a corrupt file, a new session must persist again")
}
}
// Security review 2026-10-08: a password change revokes on disk even when its own save never happened. Here the
// password changes in settings.json WITHOUT invalidateAllSessions (the shape of a failed save that also could not
// remove the file); the restart drops every row written under the old password.
// RED-PROOF: skip the CredentialFP comparison in loadSessions → the old cookie signs in after the restart → FAILS.
func TestR35_PasswordChangeRevokesEvenIfSaveFailed(t *testing.T) {
dir := t.TempDir()
s1 := newR35Server(t, dir)
tok := s1.createSession()
s1.Close()
s2 := newR35Server(t, dir)
if !s2.isValidSession(tok) {
t.Fatal("setup: the session must survive one restart first (positive control)")
}
if err := s2.settings.SetPasswordHash("$2a$10$abcdefghijklmnopqrstuvABCDEFGHIJKLMNOPQRSTUVWXYZ01234"); err != nil {
t.Fatal(err)
}
s2.Close()
s3 := newR35Server(t, dir)
if s3.isValidSession(tok) {
t.Fatal("a session written under the old password signs in after the password changed")
}
}
// Security review 2026-10-08: a logout whose save fails removes the file, so the signed-out cookie cannot return.
// The save is made to fail by a DIRECTORY at the temp path (OpenFile on a directory fails).
// RED-PROOF: make saveSessionsRevokingLocked only call saveSessionsLocked → the stale file keeps the row → FAILS.
func TestR35_FailedLogoutSaveRemovesFile(t *testing.T) {
dir := t.TempDir()
s1 := newR35Server(t, dir)
tok := s1.createSession()
s1.Close()
s2 := newR35Server(t, dir)
if !s2.isValidSession(tok) {
t.Fatal("setup: the session must survive one restart first (positive control)")
}
if err := os.Mkdir(filepath.Join(dir, sessionsFileName+".tmp"), 0o700); err != nil {
t.Fatal(err)
}
req := httptest.NewRequest(http.MethodPost, "/logout", nil)
req.AddCookie(&http.Cookie{Name: sessionCookieName, Value: tok})
s2.handleLogout(httptest.NewRecorder(), req)
s2.Close()
if err := os.Remove(filepath.Join(dir, sessionsFileName+".tmp")); err != nil {
t.Fatal(err)
}
s3 := newR35Server(t, dir)
if s3.isValidSession(tok) {
t.Fatal("logout with a failed save: the signed-out cookie signs in again after a restart")
}
}