From 7f6534a52d356a1def551ded22fe8805108dfe31 Mon Sep 17 00:00:00 2001 From: peusebiu Date: Tue, 17 Oct 2023 06:03:42 +0300 Subject: [PATCH] fix(sessions): periodically cleanup expired sessions (#1939) Signed-off-by: Petu Eusebiu --- pkg/api/authn.go | 35 ------ pkg/api/controller.go | 26 ++++- pkg/api/controller_test.go | 4 +- pkg/api/cookiestore.go | 159 +++++++++++++++++++++++++++ pkg/storage/imagestore/imagestore.go | 4 + pkg/storage/types/types.go | 1 + pkg/test/mocks/image_store_mock.go | 9 ++ 7 files changed, 198 insertions(+), 40 deletions(-) create mode 100644 pkg/api/cookiestore.go diff --git a/pkg/api/authn.go b/pkg/api/authn.go index 8a8b4284..bd7f03d7 100644 --- a/pkg/api/authn.go +++ b/pkg/api/authn.go @@ -6,13 +6,11 @@ import ( "crypto/sha256" "crypto/x509" "encoding/base64" - "encoding/gob" "errors" "fmt" "net" "net/http" "os" - "path" "strconv" "strings" "time" @@ -39,7 +37,6 @@ import ( zcommon "zotregistry.io/zot/pkg/common" "zotregistry.io/zot/pkg/log" reqCtx "zotregistry.io/zot/pkg/requestcontext" - storageConstants "zotregistry.io/zot/pkg/storage/constants" ) const ( @@ -260,38 +257,6 @@ func (amw *AuthnMiddleware) TryAuthnHandlers(ctlr *Controller) mux.MiddlewareFun delay := ctlr.Config.HTTP.Auth.FailDelay - // setup sessions cookie store used to preserve logged in user in web sessions - if ctlr.Config.IsBasicAuthnEnabled() { - // To store custom types in our cookies - // we must first register them using gob.Register - gob.Register(map[string]interface{}{}) - - cookieStoreHashKey := securecookie.GenerateRandomKey(64) - if cookieStoreHashKey == nil { - panic(zerr.ErrHashKeyNotCreated) - } - - // if storage is filesystem then use zot's rootDir to store sessions - if ctlr.Config.Storage.StorageDriver == nil { - sessionsDir := path.Join(ctlr.Config.Storage.RootDirectory, "_sessions") - if err := os.MkdirAll(sessionsDir, storageConstants.DefaultDirPerms); err != nil { - panic(err) - } - - cookieStore := sessions.NewFilesystemStore(sessionsDir, cookieStoreHashKey) - - cookieStore.MaxAge(cookiesMaxAge) - - ctlr.CookieStore = cookieStore - } else { - cookieStore := sessions.NewCookieStore(cookieStoreHashKey) - - cookieStore.MaxAge(cookiesMaxAge) - - ctlr.CookieStore = cookieStore - } - } - // ldap and htpasswd based authN if ctlr.Config.IsLdapAuthEnabled() { ldapConfig := ctlr.Config.HTTP.Auth.LDAP diff --git a/pkg/api/controller.go b/pkg/api/controller.go index 9df50547..aa362229 100644 --- a/pkg/api/controller.go +++ b/pkg/api/controller.go @@ -16,7 +16,6 @@ import ( "github.com/gorilla/handlers" "github.com/gorilla/mux" - "github.com/gorilla/sessions" "github.com/zitadel/oidc/pkg/client/rp" "zotregistry.io/zot/errors" @@ -35,7 +34,6 @@ import ( const ( idleTimeout = 120 * time.Second readHeaderTimeout = 5 * time.Second - cookiesMaxAge = 86400 // seconds ) type Controller struct { @@ -50,7 +48,7 @@ type Controller struct { CveScanner ext.CveScanner SyncOnDemand SyncOnDemand RelyingParties map[string]rp.RelyingParty - CookieStore sessions.Store + CookieStore *CookieStore // runtime params chosenPort int // kernel-chosen port } @@ -96,6 +94,10 @@ func (c *Controller) GetPort() int { } func (c *Controller) Run(reloadCtx context.Context) error { + if err := c.initCookieStore(); err != nil { + return err + } + c.StartBackgroundTasks(reloadCtx) // setup HTTP API router @@ -259,6 +261,20 @@ func (c *Controller) InitImageStore() error { return nil } +func (c *Controller) initCookieStore() error { + // setup sessions cookie store used to preserve logged in user in web sessions + if c.Config.IsBasicAuthnEnabled() { + cookieStore, err := NewCookieStore(c.StoreController) + if err != nil { + return err + } + + c.CookieStore = cookieStore + } + + return nil +} + func (c *Controller) InitMetaDB(reloadCtx context.Context) error { // init metaDB if search is enabled or we need to store user profiles, api keys or signatures if c.Config.IsSearchEnabled() || c.Config.IsBasicAuthnEnabled() || c.Config.IsImageTrustEnabled() { @@ -395,6 +411,10 @@ func (c *Controller) StartBackgroundTasks(reloadCtx context.Context) { c.SyncOnDemand = syncOnDemand } + if c.CookieStore != nil { + c.CookieStore.RunSessionCleaner(taskScheduler) + } + // we can later move enabling the other scheduled tasks inside the call below ext.EnableScheduledTasks(c.Config, taskScheduler, c.MetaDB, c.Log) //nolint: contextcheck } diff --git a/pkg/api/controller_test.go b/pkg/api/controller_test.go index 0f446eaa..0fdf0c90 100644 --- a/pkg/api/controller_test.go +++ b/pkg/api/controller_test.go @@ -3409,7 +3409,7 @@ func TestAuthnSessionErrors(t *testing.T) { session.IsNew = false session.Values["authStatus"] = true - cookieStore, ok := ctlr.CookieStore.(*sessions.FilesystemStore) + cookieStore, ok := ctlr.CookieStore.Store.(*sessions.FilesystemStore) So(ok, ShouldBeTrue) // first encode sessionID @@ -3450,7 +3450,7 @@ func TestAuthnSessionErrors(t *testing.T) { session.Values["authStatus"] = false session.Values["username"] = username - cookieStore, ok := ctlr.CookieStore.(*sessions.FilesystemStore) + cookieStore, ok := ctlr.CookieStore.Store.(*sessions.FilesystemStore) So(ok, ShouldBeTrue) // first encode sessionID diff --git a/pkg/api/cookiestore.go b/pkg/api/cookiestore.go new file mode 100644 index 00000000..d065399b --- /dev/null +++ b/pkg/api/cookiestore.go @@ -0,0 +1,159 @@ +package api + +import ( + "context" + "encoding/gob" + "io/fs" + "os" + "path" + "path/filepath" + "strings" + "time" + + "github.com/gorilla/securecookie" + "github.com/gorilla/sessions" + + zerr "zotregistry.io/zot/errors" + "zotregistry.io/zot/pkg/scheduler" + "zotregistry.io/zot/pkg/storage" + storageConstants "zotregistry.io/zot/pkg/storage/constants" +) + +const cookiesMaxAge = 7200 // 2h + +type CookieStore struct { + needsCleanup bool // if store should be periodically cleaned + rootDir string + sessions.Store +} + +func (c *CookieStore) RunSessionCleaner(sch *scheduler.Scheduler) { + if c.needsCleanup { + sch.SubmitGenerator(&SessionCleanup{rootDir: c.rootDir}, cookiesMaxAge, scheduler.LowPriority) + } +} + +func NewCookieStore(storeController storage.StoreController) (*CookieStore, error) { + // To store custom types in our cookies + // we must first register them using gob.Register + gob.Register(map[string]interface{}{}) + + hashKey, err := getHashKey() + if err != nil { + return &CookieStore{}, err + } + + var store sessions.Store + + var sessionsDir string + + var needsCleanup bool + + if storeController.DefaultStore.Name() == storageConstants.LocalStorageDriverName { + sessionsDir = path.Join(storeController.DefaultStore.RootDir(), "_sessions") + if err := os.MkdirAll(sessionsDir, storageConstants.DefaultDirPerms); err != nil { + return &CookieStore{}, err + } + + localStore := sessions.NewFilesystemStore(sessionsDir, hashKey) + + localStore.MaxAge(cookiesMaxAge) + + store = localStore + needsCleanup = true + } else { + memStore := sessions.NewCookieStore(hashKey) + + memStore.MaxAge(cookiesMaxAge) + + store = memStore + } + + return &CookieStore{ + Store: store, + rootDir: sessionsDir, + needsCleanup: needsCleanup, + }, nil +} + +func getHashKey() ([]byte, error) { + hashKey := securecookie.GenerateRandomKey(64) + if hashKey == nil { + return nil, zerr.ErrHashKeyNotCreated + } + + return hashKey, nil +} + +func getExpiredSessions(dir string) ([]string, error) { + sessions := make([]string, 0) + + err := filepath.WalkDir(dir, func(_ string, dirEntry fs.DirEntry, err error) error { + if err != nil { + return err + } + + fileInfo, err := dirEntry.Info() + if err != nil { // may have been deleted in the meantime + return nil //nolint: nilerr + } + + if !strings.HasPrefix(fileInfo.Name(), "session_") { + return nil + } + + if fileInfo.ModTime().Add(cookiesMaxAge * time.Second).Before(time.Now()) { + sessions = append(sessions, path.Join(dir, fileInfo.Name())) + } + + return nil + }) + + return sessions, err +} + +type SessionCleanup struct { + rootDir string + done bool +} + +func (gen *SessionCleanup) Next() (scheduler.Task, error) { + sessions, err := getExpiredSessions(gen.rootDir) + if err != nil { + return nil, err + } + + if len(sessions) == 0 { + gen.done = true + + return nil, nil + } + + return &CleanTask{sessions: sessions}, nil +} + +func (gen *SessionCleanup) IsDone() bool { + return gen.done +} + +func (gen *SessionCleanup) IsReady() bool { + return true +} + +func (gen *SessionCleanup) Reset() { + gen.done = false +} + +type CleanTask struct { + sessions []string +} + +func (cleanTask *CleanTask) DoWork(ctx context.Context) error { + for _, session := range cleanTask.sessions { + if err := os.Remove(session); err != nil { + return err + } + } + + return nil +} diff --git a/pkg/storage/imagestore/imagestore.go b/pkg/storage/imagestore/imagestore.go index a65762c1..3ef39c7d 100644 --- a/pkg/storage/imagestore/imagestore.go +++ b/pkg/storage/imagestore/imagestore.go @@ -52,6 +52,10 @@ type ImageStore struct { commit bool } +func (is *ImageStore) Name() string { + return is.storeDriver.Name() +} + func (is *ImageStore) RootDir() string { return is.rootDir } diff --git a/pkg/storage/types/types.go b/pkg/storage/types/types.go index 14e5569a..ab51c2ab 100644 --- a/pkg/storage/types/types.go +++ b/pkg/storage/types/types.go @@ -19,6 +19,7 @@ type StoreController interface { } type ImageStore interface { //nolint:interfacebloat + Name() string DirExists(d string) bool RootDir() string RLock(*time.Time) diff --git a/pkg/test/mocks/image_store_mock.go b/pkg/test/mocks/image_store_mock.go index 2252c07a..c691f013 100644 --- a/pkg/test/mocks/image_store_mock.go +++ b/pkg/test/mocks/image_store_mock.go @@ -12,6 +12,7 @@ import ( ) type MockedImageStore struct { + NameFn func() string DirExistsFn func(d string) bool RootDirFn func() string InitRepoFn func(name string) error @@ -68,6 +69,14 @@ func (is MockedImageStore) RUnlock(t *time.Time) { func (is MockedImageStore) RLock(t *time.Time) { } +func (is MockedImageStore) Name() string { + if is.NameFn != nil { + return is.NameFn() + } + + return "" +} + func (is MockedImageStore) DirExists(d string) bool { if is.DirExistsFn != nil { return is.DirExistsFn(d)