From ca066a55b5240bbcce7e916d7d6acfbbe7afb097 Mon Sep 17 00:00:00 2001 From: Aaron L Date: Mon, 17 Dec 2018 23:03:55 -0800 Subject: [PATCH] Add DelAllSession method In order to prevent leaking of session values (and to avoid the mess of deleting the entire session cookie which could still have values we want in it) this nuclear method is now called by expire/logout with a whitelist of keys to keep (passed in from Config.Storage.SessionWhitelistKeys). --- CHANGELOG.md | 17 +++++++++++++++++ README.md | 4 +++- client_state.go | 32 ++++++++++++++++++++++++++++++-- client_state_test.go | 17 +++++++++++++++++ config.go | 6 ++++++ expire/expire.go | 12 +++++++++--- logout/logout.go | 1 + 7 files changed, 83 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a6e676..091b98e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,23 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Added + +- DelAllSession is a new method called both by Expire and Logout (in addition + to still calling DelKnownSession etc. as they do now) to ensure that + conforming implementations of ClientStateReadWriter's delete all keys + in the session. +- Config.Storage.SessionWhitelistKeys has been added in order to allow users + to persist session variables past logout/expire. + +### Deprecated + +- Deprecated DelKnownSession for DelAllSession. DelAllSession should be + implemented by existing ClientStateReadWriters in order to prevent session + values from leaking to a different user post-logout/expire. + ## [2.2.0] - 2018-12-16 ### Added diff --git a/README.md b/README.md index e53d335..70dcc3b 100644 --- a/README.md +++ b/README.md @@ -382,9 +382,11 @@ method. This ensures the above facets are taken care of which the exception of t If it's also desirable to have the user logged out, please use the following methods to erase all known sessions and cookies from the user. -* [authboss.DelKnownSession](https://godoc.org//github.com/volatiletech/authboss/#DelKnownSession) +* [authboss.DelAllSession](https://godoc.org//github.com/volatiletech/authboss/#DelAllSession) * [authboss.DelKnownCookie](https://godoc.org//github.com/volatiletech/authboss/#DelKnownCookie) +*Note: DelKnownSession has been deprecated for security reasons* + ## User Auth via Password | Info and Requirements | | diff --git a/client_state.go b/client_state.go index 32ce44e..1247cc5 100644 --- a/client_state.go +++ b/client_state.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "net/http" + "strings" ) const ( @@ -45,11 +46,21 @@ type ClientStateEventKind int // ClientStateEvent kinds const ( + // ClientStateEventPut means you should put the key-value pair into the + // client state. ClientStateEventPut ClientStateEventKind = iota + // ClientStateEventPut means you should delete the key-value pair from the + // client state. ClientStateEventDel + // ClientStateEventDelAll means you should delete EVERY key-value pair from + // the client state - though a whitelist of keys that should not be deleted + // may be passed through as a comma separated list of keys in + // the ClientStateEvent.Key field. + ClientStateEventDelAll ) // ClientStateEvent are the different events that can be recorded during +// a request. type ClientStateEvent struct { Kind ClientStateEventKind Key string @@ -247,8 +258,21 @@ func IsTwoFactored(r *http.Request) bool { return has2fa } -// DelKnownSession deletes all known session variables, effectively -// logging a user out. +// DelAllSession deletes all variables in the session except for those on +// the whitelist. +// +// The whitelist is typically provided directly from the authboss config. +// +// This is the best way to ensure the session is cleaned up after use for +// a given user. An example is when a user is expired or logged out this method +// is called. +func DelAllSession(w http.ResponseWriter, whitelist []string) { + delAllState(w, CTXKeySessionState, whitelist) +} + +// DelKnownSession is deprecated. See DelAllSession for an alternative. +// DelKnownSession deletes all known session variables, +// effectively logging a user out. func DelKnownSession(w http.ResponseWriter) { DelSession(w, SessionKey) DelSession(w, SessionHalfAuthKey) @@ -299,6 +323,10 @@ func delState(w http.ResponseWriter, CTXKey contextKey, key string) { setState(w, CTXKey, ClientStateEventDel, key, "") } +func delAllState(w http.ResponseWriter, CTXKey contextKey, whitelist []string) { + setState(w, CTXKey, ClientStateEventDelAll, strings.Join(whitelist, ","), "") +} + func setState(w http.ResponseWriter, ctxKey contextKey, op ClientStateEventKind, key, val string) { csrw := MustClientStateResponseWriter(w) ev := ClientStateEvent{ diff --git a/client_state_test.go b/client_state_test.go index 5aed014..36ca41d 100644 --- a/client_state_test.go +++ b/client_state_test.go @@ -160,6 +160,23 @@ func TestFlashClearer(t *testing.T) { } } +func TestDelAllSession(t *testing.T) { + t.Parallel() + + csrw := &ClientStateResponseWriter{} + + DelAllSession(csrw, []string{"notthisone", "orthis"}) + + if len(csrw.sessionStateEvents) != 1 { + t.Error("should have one delete all") + } + if ev := csrw.sessionStateEvents[0]; ev.Kind != ClientStateEventDelAll { + t.Error("it should be a delete all event:", ev.Kind) + } else if ev.Key != "notthisone,orthis" { + t.Error("the whitelist should be passed through as CSV:", ev.Key) + } +} + func TestDelKnown(t *testing.T) { t.Parallel() diff --git a/config.go b/config.go index 4581573..dd00660 100644 --- a/config.go +++ b/config.go @@ -189,6 +189,12 @@ type Config struct { // storing session-only values for the given response, and reading them // from the request. SessionState ClientStateReadWriter + + // SessionStateWhitelistKeys are set to preserve keys in the session + // when authboss.DelAllSession is called. A correct implementation + // of ClientStateReadWriter will delete ALL session key-value pairs + // unless that key is whitelisted here. + SessionStateWhitelistKeys []string } Core struct { diff --git a/expire/expire.go b/expire/expire.go index 92413cd..3553d0b 100644 --- a/expire/expire.go +++ b/expire/expire.go @@ -47,8 +47,9 @@ func refreshExpiry(w http.ResponseWriter) { } type expireMiddleware struct { - expireAfter time.Duration - next http.Handler + expireAfter time.Duration + next http.Handler + sessionWhitelist []string } // Middleware ensures that the user's expiry information is kept up-to-date @@ -58,7 +59,11 @@ type expireMiddleware struct { // at the same time. func Middleware(ab *authboss.Authboss) func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { - return expireMiddleware{ab.Config.Modules.ExpireAfter, next} + return expireMiddleware{ + expireAfter: ab.Config.Modules.ExpireAfter, + next: next, + sessionWhitelist: ab.Config.Storage.SessionStateWhitelistKeys, + } } } @@ -68,6 +73,7 @@ func (m expireMiddleware) ServeHTTP(w http.ResponseWriter, r *http.Request) { if _, ok := authboss.GetSession(r, authboss.SessionKey); ok { ttl := timeToExpiry(r, m.expireAfter) if ttl == 0 { + authboss.DelAllSession(w, m.sessionWhitelist) authboss.DelSession(w, authboss.SessionKey) authboss.DelSession(w, authboss.SessionLastAction) ctx := context.WithValue(r.Context(), authboss.CTXKeyPID, nil) diff --git a/logout/logout.go b/logout/logout.go index bb626ee..b7dd301 100644 --- a/logout/logout.go +++ b/logout/logout.go @@ -49,6 +49,7 @@ func (l *Logout) Logout(w http.ResponseWriter, r *http.Request) error { logger.Info("user (unknown) logged out") } + authboss.DelAllSession(w, l.Config.Storage.SessionStateWhitelistKeys) authboss.DelKnownSession(w) authboss.DelKnownCookie(w)