From 2f15670094146ecb326610b2ff5b165b70410f3e Mon Sep 17 00:00:00 2001 From: Emil Lundberg Date: Thu, 21 Mar 2024 13:09:47 +0100 Subject: [PATCH] lib/api: Extract session store (#9425) This is an extract from PR #9175, which can be reviewed in isolation to reduce the volume of changes to review all at once in #9175. There are about to be several services and API handlers that read and set cookies and session state, so this abstraction will prove helpful. In particular a motivating cause for this is that with the current architecture in PR #9175, in `api.go` the [`webauthnService` needs to access the session](https://github.com/syncthing/syncthing/pull/9175/files#diff-e2e14f22d818b8e635572ef0ee7718dee875c365e07225d760a6faae8be7772dR309-R310) for authentication purposes but needs to be instantiated before the `configMuxBuilder` for config purposes, because the WebAuthn additions to config management need to perform WebAuthn registration ceremonies, but currently the session management is embedded in the `basicAuthAndSessionMiddleware` which is [instantiated much later](https://github.com/syncthing/syncthing/pull/9175/files#diff-e2e14f22d818b8e635572ef0ee7718dee875c365e07225d760a6faae8be7772dL371-R380) and only if authentication is enabled in `guiCfg`. This refactorization extracts the session management out from `basicAuthAndSessionMiddleware` so that `basicAuthAndSessionMiddleware` and `webauthnService` can both use the same shared session management service to perform session management logic. ### Testing This is a refactorization intended to not change any externally observable behaviour, so existing tests (e.g., `api_auth_test.go`) should cover this where appropriate. I have manually verified that: - Appending `+ "foo"` to the cookie name in `createSession` causes `TestHtmlFormLogin/invalid_URL_returns_403_before_auth_and_404_after_auth` and `TestHtmlFormLogin/UTF-8_auth_works` to fail - Inverting the return value of `hasValidSession` cases a whole bunch of tests in `TestHTTPLogin` and `TestHtmlFormLogin` to fail - (Fixed) Changing the cookie to `MaxAge: 1000` in `destroySession` does NOT cause any tests to fail! - Added tests `TestHtmlFormLogin/Logout_removes_the_session_cookie`, `TestHTTPLogin/*/Logout_removes_the_session_cookie`, `TestHtmlFormLogin/Session_cookie_is_invalid_after_logout` and `TestHTTPLogin/200_path#01/Session_cookie_is_invalid_after_logout` to cover this. - Manually verified that these tests pass both before and after the changes in this PR, and that changing the cookie to `MaxAge: 1000` or not calling `m.tokens.Delete(cookie.Value)` in `destroySession` makes the respective pair of tests fail. --- lib/api/api.go | 4 +- lib/api/api_auth.go | 96 ++++++++--------------------------------- lib/api/api_test.go | 87 +++++++++++++++++++++++++++++++++++-- lib/api/tokenmanager.go | 87 +++++++++++++++++++++++++++++++++++++ 4 files changed, 191 insertions(+), 83 deletions(-) diff --git a/lib/api/api.go b/lib/api/api.go index 5a4ee76a7..21f41f6f4 100644 --- a/lib/api/api.go +++ b/lib/api/api.go @@ -368,8 +368,8 @@ func (s *service) Serve(ctx context.Context) error { // Wrap everything in basic auth, if user/password is set. if guiCfg.IsAuthEnabled() { - sessionCookieName := "sessionid-" + s.id.Short().String() - authMW := newBasicAuthAndSessionMiddleware(sessionCookieName, s.id.Short().String(), guiCfg, s.cfg.LDAP(), handler, s.evLogger, s.miscDB) + tokenCookieManager := newTokenCookieManager(s.id.Short().String(), guiCfg, s.evLogger, s.miscDB) + authMW := newBasicAuthAndSessionMiddleware(tokenCookieManager, guiCfg, s.cfg.LDAP(), handler, s.evLogger) handler = authMW restMux.Handler(http.MethodPost, "/rest/noauth/auth/password", http.HandlerFunc(authMW.passwordAuthHandler)) diff --git a/lib/api/api_auth.go b/lib/api/api_auth.go index b24ad7950..3be4705f3 100644 --- a/lib/api/api_auth.go +++ b/lib/api/api_auth.go @@ -17,7 +17,6 @@ import ( ldap "github.com/go-ldap/ldap/v3" "github.com/syncthing/syncthing/lib/config" - "github.com/syncthing/syncthing/lib/db" "github.com/syncthing/syncthing/lib/events" "github.com/syncthing/syncthing/lib/rand" ) @@ -80,24 +79,20 @@ func isNoAuthPath(path string) bool { } type basicAuthAndSessionMiddleware struct { - cookieName string - shortID string - guiCfg config.GUIConfiguration - ldapCfg config.LDAPConfiguration - next http.Handler - evLogger events.Logger - tokens *tokenManager + tokenCookieManager *tokenCookieManager + guiCfg config.GUIConfiguration + ldapCfg config.LDAPConfiguration + next http.Handler + evLogger events.Logger } -func newBasicAuthAndSessionMiddleware(cookieName, shortID string, guiCfg config.GUIConfiguration, ldapCfg config.LDAPConfiguration, next http.Handler, evLogger events.Logger, miscDB *db.NamespacedKV) *basicAuthAndSessionMiddleware { +func newBasicAuthAndSessionMiddleware(tokenCookieManager *tokenCookieManager, guiCfg config.GUIConfiguration, ldapCfg config.LDAPConfiguration, next http.Handler, evLogger events.Logger) *basicAuthAndSessionMiddleware { return &basicAuthAndSessionMiddleware{ - cookieName: cookieName, - shortID: shortID, - guiCfg: guiCfg, - ldapCfg: ldapCfg, - next: next, - evLogger: evLogger, - tokens: newTokenManager("sessions", miscDB, maxSessionLifetime, maxActiveSessions), + tokenCookieManager: tokenCookieManager, + guiCfg: guiCfg, + ldapCfg: ldapCfg, + next: next, + evLogger: evLogger, } } @@ -107,22 +102,14 @@ func (m *basicAuthAndSessionMiddleware) ServeHTTP(w http.ResponseWriter, r *http return } - for _, cookie := range r.Cookies() { - // We iterate here since there may, historically, be multiple - // cookies with the same name but different path. Any "old" ones - // won't match an existing session and will be ignored, then - // later removed on logout or when timing out. - if cookie.Name == m.cookieName { - if m.tokens.Check(cookie.Value) { - m.next.ServeHTTP(w, r) - return - } - } + if m.tokenCookieManager.hasValidSession(r) { + m.next.ServeHTTP(w, r) + return } // Fall back to Basic auth if provided if username, ok := attemptBasicAuth(r, m.guiCfg, m.ldapCfg, m.evLogger); ok { - m.createSession(username, false, w, r) + m.tokenCookieManager.createSession(username, false, w, r) m.next.ServeHTTP(w, r) return } @@ -136,7 +123,7 @@ func (m *basicAuthAndSessionMiddleware) ServeHTTP(w http.ResponseWriter, r *http // Some browsers don't send the Authorization request header unless prompted by a 401 response. // This enables https://user:pass@localhost style URLs to keep working. if m.guiCfg.SendBasicAuthPrompt { - unauthorized(w, m.shortID) + unauthorized(w, m.tokenCookieManager.shortID) return } @@ -156,7 +143,7 @@ func (m *basicAuthAndSessionMiddleware) passwordAuthHandler(w http.ResponseWrite } if auth(req.Username, req.Password, m.guiCfg, m.ldapCfg) { - m.createSession(req.Username, req.StayLoggedIn, w, r) + m.tokenCookieManager.createSession(req.Username, req.StayLoggedIn, w, r) w.WriteHeader(http.StatusNoContent) return } @@ -189,55 +176,8 @@ func attemptBasicAuth(r *http.Request, guiCfg config.GUIConfiguration, ldapCfg c return "", false } -func (m *basicAuthAndSessionMiddleware) createSession(username string, persistent bool, w http.ResponseWriter, r *http.Request) { - sessionid := m.tokens.New() - - // Best effort detection of whether the connection is HTTPS -- - // either directly to us, or as used by the client towards a reverse - // proxy who sends us headers. - connectionIsHTTPS := r.TLS != nil || - strings.ToLower(r.Header.Get("x-forwarded-proto")) == "https" || - strings.Contains(strings.ToLower(r.Header.Get("forwarded")), "proto=https") - // If the connection is HTTPS, or *should* be HTTPS, set the Secure - // bit in cookies. - useSecureCookie := connectionIsHTTPS || m.guiCfg.UseTLS() - - maxAge := 0 - if persistent { - maxAge = int(maxSessionLifetime.Seconds()) - } - http.SetCookie(w, &http.Cookie{ - Name: m.cookieName, - Value: sessionid, - // In HTTP spec Max-Age <= 0 means delete immediately, - // but in http.Cookie MaxAge = 0 means unspecified (session) and MaxAge < 0 means delete immediately - MaxAge: maxAge, - Secure: useSecureCookie, - Path: "/", - }) - - emitLoginAttempt(true, username, r.RemoteAddr, m.evLogger) -} - func (m *basicAuthAndSessionMiddleware) handleLogout(w http.ResponseWriter, r *http.Request) { - for _, cookie := range r.Cookies() { - // We iterate here since there may, historically, be multiple - // cookies with the same name but different path. We drop them - // all. - if cookie.Name == m.cookieName { - m.tokens.Delete(cookie.Value) - - // Delete the cookie - http.SetCookie(w, &http.Cookie{ - Name: m.cookieName, - Value: "", - MaxAge: -1, - Secure: cookie.Secure, - Path: cookie.Path, - }) - } - } - + m.tokenCookieManager.destroySession(w, r) w.WriteHeader(http.StatusNoContent) } diff --git a/lib/api/api_test.go b/lib/api/api_test.go index cfdcb4234..7be473dc1 100644 --- a/lib/api/api_test.go +++ b/lib/api/api_test.go @@ -498,6 +498,15 @@ func hasSessionCookie(cookies []*http.Cookie) bool { return false } +func hasDeleteSessionCookie(cookies []*http.Cookie) bool { + for _, cookie := range cookies { + if cookie.MaxAge < 0 && strings.HasPrefix(cookie.Name, "sessionid") { + return true + } + } + return false +} + func httpGet(url string, basicAuthUsername string, basicAuthPassword string, xapikeyHeader string, authorizationBearer string, cookies []*http.Cookie, t *testing.T) *http.Response { req, err := http.NewRequest("GET", url, nil) for _, cookie := range cookies { @@ -527,7 +536,7 @@ func httpGet(url string, basicAuthUsername string, basicAuthPassword string, xap return resp } -func httpPost(url string, body map[string]string, t *testing.T) *http.Response { +func httpPost(url string, body map[string]string, cookies []*http.Cookie, t *testing.T) *http.Response { bodyBytes, err := json.Marshal(body) if err != nil { t.Fatal(err) @@ -538,6 +547,10 @@ func httpPost(url string, body map[string]string, t *testing.T) *http.Response { t.Fatal(err) } + for _, cookie := range cookies { + req.AddCookie(cookie) + } + resp, err := http.DefaultClient.Do(req) if err != nil { t.Fatal(err) @@ -622,6 +635,43 @@ func TestHTTPLogin(t *testing.T) { } }) + t.Run("Logout removes the session cookie", func(t *testing.T) { + t.Parallel() + resp := httpGetBasicAuth(url, "üser", "räksmörgås") // string literals in Go source code are in UTF-8 + if resp.StatusCode != expectedOkStatus { + t.Errorf("Unexpected non-%d return code %d for authed request (UTF-8)", expectedOkStatus, resp.StatusCode) + } + if !hasSessionCookie(resp.Cookies()) { + t.Errorf("Expected session cookie for authed request (UTF-8)") + } + logoutResp := httpPost(baseURL+"/rest/noauth/auth/logout", nil, resp.Cookies(), t) + if !hasDeleteSessionCookie(logoutResp.Cookies()) { + t.Errorf("Expected session cookie to be deleted for logout request") + } + }) + + t.Run("Session cookie is invalid after logout", func(t *testing.T) { + t.Parallel() + loginResp := httpGetBasicAuth(url, "üser", "räksmörgås") // string literals in Go source code are in UTF-8 + if loginResp.StatusCode != expectedOkStatus { + t.Errorf("Unexpected non-%d return code %d for authed request (UTF-8)", expectedOkStatus, loginResp.StatusCode) + } + if !hasSessionCookie(loginResp.Cookies()) { + t.Errorf("Expected session cookie for authed request (UTF-8)") + } + + resp := httpGet(url, "", "", "", "", loginResp.Cookies(), t) + if resp.StatusCode != expectedOkStatus { + t.Errorf("Unexpected non-%d return code %d for cookie-authed request (UTF-8)", expectedOkStatus, resp.StatusCode) + } + + httpPost(baseURL+"/rest/noauth/auth/logout", nil, loginResp.Cookies(), t) + resp = httpGet(url, "", "", "", "", loginResp.Cookies(), t) + if resp.StatusCode != expectedFailStatus { + t.Errorf("Expected session to be invalid (status %d) after logout, got status: %d", expectedFailStatus, resp.StatusCode) + } + }) + t.Run("ISO-8859-1 auth works", func(t *testing.T) { t.Parallel() resp := httpGetBasicAuth(url, "\xfcser", "r\xe4ksm\xf6rg\xe5s") // escaped ISO-8859-1 @@ -708,7 +758,7 @@ func TestHtmlFormLogin(t *testing.T) { resourceUrl404 := baseURL + "/any-path/that/does/nooooooot/match-any/noauth-pattern" performLogin := func(username string, password string) *http.Response { - return httpPost(loginUrl, map[string]string{"username": username, "password": password}, t) + return httpPost(loginUrl, map[string]string{"username": username, "password": password}, nil, t) } performResourceRequest := func(url string, cookies []*http.Cookie) *http.Response { @@ -773,9 +823,40 @@ func TestHtmlFormLogin(t *testing.T) { } }) + t.Run("Logout removes the session cookie", func(t *testing.T) { + t.Parallel() + // JSON is always UTF-8, so ISO-8859-1 case is not applicable + resp := performLogin("üser", "räksmörgås") // string literals in Go source code are in UTF-8 + if resp.StatusCode != http.StatusNoContent { + t.Errorf("Unexpected non-204 return code %d for authed request (UTF-8)", resp.StatusCode) + } + logoutResp := httpPost(baseURL+"/rest/noauth/auth/logout", nil, resp.Cookies(), t) + if !hasDeleteSessionCookie(logoutResp.Cookies()) { + t.Errorf("Expected session cookie to be deleted for logout request") + } + }) + + t.Run("Session cookie is invalid after logout", func(t *testing.T) { + t.Parallel() + // JSON is always UTF-8, so ISO-8859-1 case is not applicable + loginResp := performLogin("üser", "räksmörgås") // string literals in Go source code are in UTF-8 + if loginResp.StatusCode != http.StatusNoContent { + t.Errorf("Unexpected non-204 return code %d for authed request (UTF-8)", loginResp.StatusCode) + } + resp := performResourceRequest(resourceUrl, loginResp.Cookies()) + if resp.StatusCode != http.StatusOK { + t.Errorf("Unexpected non-200 return code %d for authed request (UTF-8)", resp.StatusCode) + } + httpPost(baseURL+"/rest/noauth/auth/logout", nil, loginResp.Cookies(), t) + resp = performResourceRequest(resourceUrl, loginResp.Cookies()) + if resp.StatusCode != http.StatusForbidden { + t.Errorf("Expected session to be invalid (status 403) after logout, got status: %d", resp.StatusCode) + } + }) + t.Run("form login is not applicable to other URLs", func(t *testing.T) { t.Parallel() - resp := httpPost(baseURL+"/meta.js", map[string]string{"username": "üser", "password": "räksmörgås"}, t) + resp := httpPost(baseURL+"/meta.js", map[string]string{"username": "üser", "password": "räksmörgås"}, nil, t) if resp.StatusCode != http.StatusForbidden { t.Errorf("Unexpected non-403 return code %d for incorrect form login URL", resp.StatusCode) } diff --git a/lib/api/tokenmanager.go b/lib/api/tokenmanager.go index 73ba8425e..adccda2e9 100644 --- a/lib/api/tokenmanager.go +++ b/lib/api/tokenmanager.go @@ -7,10 +7,14 @@ package api import ( + "net/http" "slices" + "strings" "time" + "github.com/syncthing/syncthing/lib/config" "github.com/syncthing/syncthing/lib/db" + "github.com/syncthing/syncthing/lib/events" "github.com/syncthing/syncthing/lib/rand" "github.com/syncthing/syncthing/lib/sync" ) @@ -135,3 +139,86 @@ func (m *tokenManager) scheduledSave() { bs, _ := m.tokens.Marshal() // can't fail _ = m.miscDB.PutBytes(m.key, bs) // can fail, but what are we going to do? } + +type tokenCookieManager struct { + cookieName string + shortID string + guiCfg config.GUIConfiguration + evLogger events.Logger + tokens *tokenManager +} + +func newTokenCookieManager(shortID string, guiCfg config.GUIConfiguration, evLogger events.Logger, miscDB *db.NamespacedKV) *tokenCookieManager { + return &tokenCookieManager{ + cookieName: "sessionid-" + shortID, + shortID: shortID, + guiCfg: guiCfg, + evLogger: evLogger, + tokens: newTokenManager("sessions", miscDB, maxSessionLifetime, maxActiveSessions), + } +} + +func (m *tokenCookieManager) createSession(username string, persistent bool, w http.ResponseWriter, r *http.Request) { + sessionid := m.tokens.New() + + // Best effort detection of whether the connection is HTTPS -- + // either directly to us, or as used by the client towards a reverse + // proxy who sends us headers. + connectionIsHTTPS := r.TLS != nil || + strings.ToLower(r.Header.Get("x-forwarded-proto")) == "https" || + strings.Contains(strings.ToLower(r.Header.Get("forwarded")), "proto=https") + // If the connection is HTTPS, or *should* be HTTPS, set the Secure + // bit in cookies. + useSecureCookie := connectionIsHTTPS || m.guiCfg.UseTLS() + + maxAge := 0 + if persistent { + maxAge = int(maxSessionLifetime.Seconds()) + } + http.SetCookie(w, &http.Cookie{ + Name: m.cookieName, + Value: sessionid, + // In HTTP spec Max-Age <= 0 means delete immediately, + // but in http.Cookie MaxAge = 0 means unspecified (session) and MaxAge < 0 means delete immediately + MaxAge: maxAge, + Secure: useSecureCookie, + Path: "/", + }) + + emitLoginAttempt(true, username, r.RemoteAddr, m.evLogger) +} + +func (m *tokenCookieManager) hasValidSession(r *http.Request) bool { + for _, cookie := range r.Cookies() { + // We iterate here since there may, historically, be multiple + // cookies with the same name but different path. Any "old" ones + // won't match an existing session and will be ignored, then + // later removed on logout or when timing out. + if cookie.Name == m.cookieName { + if m.tokens.Check(cookie.Value) { + return true + } + } + } + return false +} + +func (m *tokenCookieManager) destroySession(w http.ResponseWriter, r *http.Request) { + for _, cookie := range r.Cookies() { + // We iterate here since there may, historically, be multiple + // cookies with the same name but different path. We drop them + // all. + if cookie.Name == m.cookieName { + m.tokens.Delete(cookie.Value) + + // Create a cookie deletion command + http.SetCookie(w, &http.Cookie{ + Name: m.cookieName, + Value: "", + MaxAge: -1, + Secure: cookie.Secure, + Path: cookie.Path, + }) + } + } +}