From 86af38abfc04e7ccae61a9af026cb837e5dcd183 Mon Sep 17 00:00:00 2001 From: Vishwas Rajashekar Date: Sun, 5 Oct 2025 12:43:38 +0530 Subject: [PATCH] feat(sessions): add support for remote redis session store (#3345) Description ==================== zot currently stores session cookies in memory or in a local directory. For cases where the session cookies should be independent of the instance where they were created such as multiple instances of zot, or a fully stateless zot instance, there is a need to support a remote session storage. This change adds support for using Redis and Redis-compatible services as a remote session driver as well as introduces a new configuration option for it. What has changed ======================= - New config added under Auth config to specify configuration for the session driver. - Examples README updated with details of the new Auth config. - The config supports only 2 drivers in this change - local and redis - Using the local driver is backwards compatible and behaves the same way that zot currently works for local session storage. - Omitting this config does not result in an error. In this case, zot behaves as it normally does for local session storage. - When configured, zot can use redis for persisting cookie information for zot UI. - The cookie in the store is deleted on logout or after the max expiry time for the cookie. - Configuration for the redis session driver accepts the same configuration values as that of the remote meta cache. - A separate connection is established for the session driver. An existing connection for meta cache will not be re-used for the session driver. - A key prefix is configurable for the redis session driver. The value will be converted into a string for use. If no value is provided, a default prefix of "zotsession" will be used. - Redis sessions does not support hash key or encryption in this change. - New BATS test added to verify zot behavior with Redis session store. - Github workflow updated to install valkey-tools dependency for BATS. Signed-off-by: Vishwas Rajashekar --- .github/workflows/ecosystem-tools.yaml | 2 +- examples/README.md | 39 ++ .../config-remote-session-store-redis.json | 34 ++ go.mod | 1 + go.sum | 2 + pkg/api/authn_test.go | 188 ++++++-- pkg/api/config/config.go | 5 +- pkg/api/config/redis/redis.go | 16 +- pkg/api/controller.go | 3 +- pkg/api/controller_test.go | 411 ++++++++++++++++++ pkg/api/cookiestore.go | 118 ++++- pkg/cli/server/root.go | 56 +++ pkg/cli/server/root_test.go | 250 +++++++++++ test/blackbox/ci.sh | 2 +- test/blackbox/redis_session_store.bats | 253 +++++++++++ test/ports.json | 10 + 16 files changed, 1342 insertions(+), 48 deletions(-) create mode 100644 examples/config-remote-session-store-redis.json create mode 100644 test/blackbox/redis_session_store.bats diff --git a/.github/workflows/ecosystem-tools.yaml b/.github/workflows/ecosystem-tools.yaml index ed70ab4a..4053d769 100644 --- a/.github/workflows/ecosystem-tools.yaml +++ b/.github/workflows/ecosystem-tools.yaml @@ -27,7 +27,7 @@ jobs: go install github.com/swaggo/swag/cmd/swag@v1.16.2 go mod download sudo apt-get update - sudo apt-get install libgpgme-dev libassuan-dev libbtrfs-dev libdevmapper-dev pkg-config rpm uidmap haproxy jq + sudo apt-get install libgpgme-dev libassuan-dev libbtrfs-dev libdevmapper-dev pkg-config rpm uidmap haproxy jq valkey-tools # install skopeo git clone -b v1.12.0 https://github.com/containers/skopeo.git cd skopeo diff --git a/examples/README.md b/examples/README.md index 026eac55..e2c2963a 100644 --- a/examples/README.md +++ b/examples/README.md @@ -370,6 +370,45 @@ Using that cookie on subsequent calls will authenticate them, asumming the cooki In case of using filesystem storage sessions are saved in zot's root directory. In case of using cloud storage sessions are saved in memory. +Note: By default, the session driver config would be local for file system or in-memory. The session driver name for this is `local`. An example config is shown below, but the config can be omitted as it is a default. + +``` + "auth": { + "htpasswd": { + "path": "test/data/htpasswd" + }, + "sessionDriver": { + "name": "local" + } + } +``` + +Note: This `sessionDriver` config is optional if a local session storage is desired. + +#### Remote Session Storage Driver + +Redis and Redis-compatible storage drivers can also be used for cases where session storage is required to be kept separately from zot or multiple zot instances need to share the session information. + +This can be configured in the `auth` section of the configuration as shown below: + +`sessionDriver` + +``` + "auth": { + "htpasswd": { + "path": "test/data/htpasswd" + }, + "sessionDriver": { + "name": "redis", + "url": "redis://localhost:6379", + "keyprefix": "zotsession" + } + } +``` + +The `redis` driver configuration options are the same as those in the [Redis Cache Driver](#redis) section. If the `redis` session driver is being used along with a `redis` cache driver and both configurations point to the same Redis instance, there will be two independent connections used. + +Note: The `redis` session driver cannot be specified along with configuration for the SessionKeysFile. ### Securing session based login diff --git a/examples/config-remote-session-store-redis.json b/examples/config-remote-session-store-redis.json new file mode 100644 index 00000000..59c51e9b --- /dev/null +++ b/examples/config-remote-session-store-redis.json @@ -0,0 +1,34 @@ +{ + "distSpecVersion": "1.1.1", + "storage": { + "rootDirectory": "/tmp/zot" + }, + "http": { + "address": "127.0.0.1", + "port": "8080", + "realm": "zot", + "auth": { + "htpasswd": { + "path": "/tmp/zotpasswd" + }, + "sessionDriver": { + "name": "redis", + "url": "redis://localhost:6379", + "keyprefix": "zotsession" + } + } + }, + "log": { + "level": "debug" + }, + "extensions": { + "search": { + "cve": { + "updateInterval": "2h" + } + }, + "ui": { + "enable": true + } + } +} diff --git a/go.mod b/go.mod index 27df659a..d98fd1d5 100644 --- a/go.mod +++ b/go.mod @@ -56,6 +56,7 @@ require ( github.com/project-zot/mockoidc v0.0.0-20240610203808-d69d9e02020a github.com/prometheus/client_golang v1.23.2 github.com/prometheus/client_model v0.6.2 + github.com/rbcervilla/redisstore/v9 v9.0.0 github.com/redis/go-redis/v9 v9.14.0 github.com/regclient/regclient v0.9.2 github.com/santhosh-tekuri/jsonschema/v5 v5.3.1 diff --git a/go.sum b/go.sum index 4ea5dd55..fc510da4 100644 --- a/go.sum +++ b/go.sum @@ -1873,6 +1873,8 @@ github.com/prometheus/procfs v0.16.1 h1:hZ15bTNuirocR6u0JZ6BAHHmwS1p8B4P6MRqxtzM github.com/prometheus/procfs v0.16.1/go.mod h1:teAbpZRB1iIAJYREa1LsoWUXykVXA1KlTmWl8x/U+Is= github.com/protocolbuffers/txtpbfmt v0.0.0-20241112170944-20d2c9ebc01d h1:HWfigq7lB31IeJL8iy7jkUmU/PG1Sr8jVGhS749dbUA= github.com/protocolbuffers/txtpbfmt v0.0.0-20241112170944-20d2c9ebc01d/go.mod h1:jgxiZysxFPM+iWKwQwPR+y+Jvo54ARd4EisXxKYpB5c= +github.com/rbcervilla/redisstore/v9 v9.0.0 h1:wOPbBaydbdxzi1gTafDftCI/Z7vnsXw0QDPCuhiMG0g= +github.com/rbcervilla/redisstore/v9 v9.0.0/go.mod h1:q/acLpoKkTZzIsBYt0R4THDnf8W/BH6GjQYvxDSSfdI= github.com/rcrowley/go-metrics v0.0.0-20201227073835-cf1acfcdf475 h1:N/ElC8H3+5XpJzTSTfLsJV/mx9Q9g7kxmchpfZyxgzM= github.com/rcrowley/go-metrics v0.0.0-20201227073835-cf1acfcdf475/go.mod h1:bCqnVzQkZxMG4s8nGwiZ5l3QUCyqpo9Y+/ZMZ9VjZe4= github.com/redis/go-redis/extra/rediscmd/v9 v9.0.5 h1:EaDatTxkdHG+U3Bk4EUr+DZ7fOGwTfezUiUJMaIcaho= diff --git a/pkg/api/authn_test.go b/pkg/api/authn_test.go index 7838685d..376883f7 100644 --- a/pkg/api/authn_test.go +++ b/pkg/api/authn_test.go @@ -15,11 +15,14 @@ import ( "testing" "time" + "github.com/alicebob/miniredis/v2" guuid "github.com/gofrs/uuid" + godigest "github.com/opencontainers/go-digest" "github.com/project-zot/mockoidc" . "github.com/smartystreets/goconvey/convey" "gopkg.in/resty.v1" + zerr "zotregistry.dev/zot/errors" "zotregistry.dev/zot/pkg/api" "zotregistry.dev/zot/pkg/api/config" "zotregistry.dev/zot/pkg/api/constants" @@ -934,45 +937,65 @@ func TestCookiestoreCleanup(t *testing.T) { log := log.NewTestLogger() metrics := monitoring.NewMetricsServer(true, log) - Convey("Test cookiestore cleanup works", t, func() { - taskScheduler := scheduler.NewScheduler(config.New(), metrics, log) - taskScheduler.RateLimit = 50 * time.Millisecond - taskScheduler.RunScheduler() + authCfgTestCases := []struct { + name string + cfg config.AuthConfig + }{ + { + "empty Auth config", + config.AuthConfig{}, + }, + { + "local session driver", + config.AuthConfig{ + SessionDriver: map[string]any{ + "name": "local", + }, + }, + }, + } - rootDir := t.TempDir() + for _, testCase := range authCfgTestCases { + Convey("Test cookiestore cleanup works with "+testCase.name, t, func() { + taskScheduler := scheduler.NewScheduler(config.New(), metrics, log) + taskScheduler.RateLimit = 50 * time.Millisecond + taskScheduler.RunScheduler() - err := os.MkdirAll(path.Join(rootDir, "_sessions"), storageConstants.DefaultDirPerms) - So(err, ShouldBeNil) + rootDir := t.TempDir() - sessionPath := path.Join(rootDir, "_sessions", "session_1234") + err := os.MkdirAll(path.Join(rootDir, "_sessions"), storageConstants.DefaultDirPerms) + So(err, ShouldBeNil) - err = os.WriteFile(sessionPath, []byte("session"), storageConstants.DefaultFilePerms) - So(err, ShouldBeNil) + sessionPath := path.Join(rootDir, "_sessions", "session_1234") - changeTime := time.Now().Add(-4 * time.Hour) + err = os.WriteFile(sessionPath, []byte("session"), storageConstants.DefaultFilePerms) + So(err, ShouldBeNil) - err = os.Chtimes(sessionPath, changeTime, changeTime) - So(err, ShouldBeNil) + changeTime := time.Now().Add(-4 * time.Hour) - imgStore := local.NewImageStore(rootDir, false, false, log, metrics, nil, nil, nil, nil) + err = os.Chtimes(sessionPath, changeTime, changeTime) + So(err, ShouldBeNil) - storeController := storage.StoreController{ - DefaultStore: imgStore, - } + imgStore := local.NewImageStore(rootDir, false, false, log, metrics, nil, nil, nil, nil) - cookieStore, err := api.NewCookieStore(storeController, nil, nil) - So(err, ShouldBeNil) + storeController := storage.StoreController{ + DefaultStore: imgStore, + } - cookieStore.RunSessionCleaner(taskScheduler) + cookieStore, err := api.NewCookieStore(&testCase.cfg, storeController, log) + So(err, ShouldBeNil) - time.Sleep(2 * time.Second) + cookieStore.RunSessionCleaner(taskScheduler) - taskScheduler.Shutdown() + time.Sleep(2 * time.Second) - // make sure session is removed - _, err = os.Stat(sessionPath) - So(err, ShouldNotBeNil) - }) + taskScheduler.Shutdown() + + // make sure session is removed + _, err = os.Stat(sessionPath) + So(err, ShouldNotBeNil) + }) + } Convey("Test cookiestore cleanup without permissions on rootDir", t, func() { taskScheduler := scheduler.NewScheduler(config.New(), metrics, log) @@ -995,7 +1018,11 @@ func TestCookiestoreCleanup(t *testing.T) { DefaultStore: imgStore, } - cookieStore, err := api.NewCookieStore(storeController, []byte("secret"), nil) + authCfg := config.AuthConfig{ + SessionHashKey: []byte("secret"), + } + + cookieStore, err := api.NewCookieStore(&authCfg, storeController, log) So(err, ShouldBeNil) err = os.Chmod(rootDir, 0o000) @@ -1085,6 +1112,113 @@ func TestCookiestoreCleanup(t *testing.T) { }) } +func TestRedisCookieStore(t *testing.T) { + log := log.Logger{} + + testRedis := miniredis.RunT(t) + + storeController := storage.StoreController{ + DefaultStore: &mocks.MockedImageStore{ + GetImageManifestFn: func(repo string, reference string) ([]byte, godigest.Digest, string, error) { + return []byte{}, "", "", zerr.ErrRepoBadVersion + }, + }, + } + + testCases := []struct { + testName string + shouldErrBeNil bool + expectedErrStr string + inputCfg *config.AuthConfig + }{ + { + "Cookie store creation should not fail if the driver is local", + true, + "", + &config.AuthConfig{ + SessionDriver: map[string]any{ + "name": "local", + }, + }, + }, + { + "Cookie store creation should fail if the driver is unsupported", + false, + "invalid server config: sessiondriver unknowndriver not supported", + &config.AuthConfig{ + SessionDriver: map[string]any{ + "name": "unknowndriver", + }, + }, + }, + { + "Cookie store creation should not fail if the keyPrefix for Redis is not a string", + true, + "", + &config.AuthConfig{ + SessionDriver: map[string]any{ + "name": "redis", + "keyprefix": 8, + "url": "redis://" + testRedis.Addr(), + }, + }, + }, + { + "Cookie store creation should not fail if the SessionDriver Config is nil", + true, + "", + &config.AuthConfig{}, + }, + { + "Cookie store creation and use should succeed with valid configuration", + true, + "", + &config.AuthConfig{ + SessionDriver: map[string]any{ + "name": "redis", + "url": "redis://" + testRedis.Addr(), + }, + }, + }, + { + "Cookie store creation should fail if the url for Redis is incorrect", + false, + "dial tcp: lookup unknown on 127.0.0.53:53: server misbehaving", + &config.AuthConfig{ + SessionDriver: map[string]any{ + "name": "redis", + "url": "redis://unknown:1000", + }, + }, + }, + { + "Cookie store creation should fail if the url for Redis has an invalid value", + false, + "invalid server config: cachedriver map[name:redis url:%!s(int=100)] has invalid value for url", + &config.AuthConfig{ + SessionDriver: map[string]any{ + "name": "redis", + "url": 100, + }, + }, + }, + } + + for _, testCase := range testCases { + Convey(testCase.testName, t, func() { + cookieStore, err := api.NewCookieStore(testCase.inputCfg, storeController, log) + if testCase.shouldErrBeNil { + So(err, ShouldBeNil) + So(cookieStore, ShouldNotBeNil) + } else { + So(err, ShouldNotBeNil) + So(err.Error(), ShouldEqual, testCase.expectedErrStr) + So(cookieStore, ShouldBeNil) + } + }) + } +} + type mockUUIDGenerator struct { guuid.Generator succeedAttempts int diff --git a/pkg/api/config/config.go b/pkg/api/config/config.go index 003469ca..62f9f963 100644 --- a/pkg/api/config/config.go +++ b/pkg/api/config/config.go @@ -75,8 +75,9 @@ type AuthConfig struct { OpenID *OpenIDConfig APIKey bool SessionKeysFile string - SessionHashKey []byte `json:"-"` - SessionEncryptKey []byte `json:"-"` + SessionHashKey []byte `json:"-"` + SessionEncryptKey []byte `json:"-"` + SessionDriver map[string]any `mapstructure:",omitempty"` } type BearerConfig struct { diff --git a/pkg/api/config/redis/redis.go b/pkg/api/config/redis/redis.go index a5900393..47dd2142 100644 --- a/pkg/api/config/redis/redis.go +++ b/pkg/api/config/redis/redis.go @@ -81,7 +81,7 @@ func ParseRedisUniversalOptions(redisConfig map[string]interface{}, //nolint: go opts.Addrs = val } - if val, ok := getString(redisConfig, "client_name", false, log); ok { + if val, ok := GetString(redisConfig, "client_name", false, log); ok { opts.ClientName = val } @@ -93,19 +93,19 @@ func ParseRedisUniversalOptions(redisConfig map[string]interface{}, //nolint: go opts.Protocol = val } - if val, ok := getString(redisConfig, "username", false, log); ok { + if val, ok := GetString(redisConfig, "username", false, log); ok { opts.Username = val } - if val, ok := getString(redisConfig, "password", true, log); ok { + if val, ok := GetString(redisConfig, "password", true, log); ok { opts.Password = val } - if val, ok := getString(redisConfig, "sentinel_username", false, log); ok { + if val, ok := GetString(redisConfig, "sentinel_username", false, log); ok { opts.SentinelUsername = val } - if val, ok := getString(redisConfig, "sentinel_password", true, log); ok { + if val, ok := GetString(redisConfig, "sentinel_password", true, log); ok { opts.SentinelPassword = val } @@ -185,7 +185,7 @@ func ParseRedisUniversalOptions(redisConfig map[string]interface{}, //nolint: go opts.RouteRandomly = val } - if val, ok := getString(redisConfig, "master_name", false, log); ok { + if val, ok := GetString(redisConfig, "master_name", false, log); ok { opts.MasterName = val } @@ -193,7 +193,7 @@ func ParseRedisUniversalOptions(redisConfig map[string]interface{}, //nolint: go opts.DisableIdentity = val } - if val, ok := getString(redisConfig, "identity_suffix", false, log); ok { + if val, ok := GetString(redisConfig, "identity_suffix", false, log); ok { opts.IdentitySuffix = val } @@ -246,7 +246,7 @@ func getInt(dict map[string]interface{}, key string, log log.Logger) (int, bool) return ret, true } -func getString(dict map[string]interface{}, key string, hideValue bool, log log.Logger) (string, bool) { +func GetString(dict map[string]interface{}, key string, hideValue bool, log log.Logger) (string, bool) { value, ok := dict[key] if !ok { return "", false diff --git a/pkg/api/controller.go b/pkg/api/controller.go index 7b197a0a..b81bb557 100644 --- a/pkg/api/controller.go +++ b/pkg/api/controller.go @@ -324,8 +324,7 @@ func (c *Controller) initCookieStore() error { c.Config.HTTP.Auth.SessionHashKey = securecookie.GenerateRandomKey(64) //nolint: gomnd } - cookieStore, err := NewCookieStore(c.StoreController, c.Config.HTTP.Auth.SessionHashKey, - c.Config.HTTP.Auth.SessionEncryptKey) + cookieStore, err := NewCookieStore(c.Config.HTTP.Auth, c.StoreController, c.Log) if err != nil { return err } diff --git a/pkg/api/controller_test.go b/pkg/api/controller_test.go index 89066eb6..e5fedb99 100644 --- a/pkg/api/controller_test.go +++ b/pkg/api/controller_test.go @@ -4952,6 +4952,417 @@ func TestOpenIDMiddleware(t *testing.T) { } } +func TestOpenIDMiddlewareWithRedisSessionDriver(t *testing.T) { + port := test.GetFreePort() + baseURL := test.GetBaseURL(port) + defaultVal := true + + conf := config.New() + conf.HTTP.Port = port + + testCases := []struct { + testCaseName string + address string + externalURL string + }{ + { + address: "0.0.0.0", + externalURL: "http://" + net.JoinHostPort(conf.HTTP.Address, conf.HTTP.Port), + testCaseName: "with ExternalURL provided in config", + }, + { + address: "127.0.0.1", + externalURL: "", + testCaseName: "without ExternalURL provided in config", + }, + { + address: "127.0.0.1", + externalURL: "", + testCaseName: "without ExternalURL provided in config and session keys for cookies", + }, + } + + // need a username different than ldap one, to test both logic + htpasswdUsername, seedUser := test.GenerateRandomString() + htpasswdPassword, seedPass := test.GenerateRandomString() + htpasswdPath := test.MakeHtpasswdFileFromString(test.GetCredString(htpasswdUsername, htpasswdPassword)) + + defer os.Remove(htpasswdPath) + + ldapServer := newTestLDAPServer() + port = test.GetFreePort() + + ldapPort, err := strconv.Atoi(port) + if err != nil { + panic(err) + } + + ldapServer.Start(ldapPort) + defer ldapServer.Stop() + + mockOIDCServer, err := authutils.MockOIDCRun() + if err != nil { + panic(err) + } + + defer func() { + err := mockOIDCServer.Shutdown() + if err != nil { + panic(err) + } + }() + + mockOIDCConfig := mockOIDCServer.Config() + conf.HTTP.Auth = &config.AuthConfig{ + HTPasswd: config.AuthHTPasswd{ + Path: htpasswdPath, + }, + LDAP: (&config.LDAPConfig{ + Insecure: true, + Address: LDAPAddress, + + Port: ldapPort, + BaseDN: LDAPBaseDN, + UserAttribute: "uid", + }).SetBindDN(LDAPBindDN).SetBindPassword(LDAPBindPassword), + OpenID: &config.OpenIDConfig{ + Providers: map[string]config.OpenIDProviderConfig{ + "oidc": { + ClientID: mockOIDCConfig.ClientID, + ClientSecret: mockOIDCConfig.ClientSecret, + KeyPath: "", + Issuer: mockOIDCConfig.Issuer, + Scopes: []string{"openid", "email"}, + }, + // just for the constructor coverage + "github": { + ClientID: mockOIDCConfig.ClientID, + ClientSecret: mockOIDCConfig.ClientSecret, + KeyPath: "", + Issuer: mockOIDCConfig.Issuer, + Scopes: []string{"openid", "email"}, + }, + }, + }, + } + + searchConfig := &extconf.SearchConfig{ + BaseConfig: extconf.BaseConfig{Enable: &defaultVal}, + } + + // UI is enabled because we also want to test access on the mgmt route + uiConfig := &extconf.UIConfig{ + BaseConfig: extconf.BaseConfig{Enable: &defaultVal}, + } + + conf.Extensions = &extconf.ExtensionConfig{ + Search: searchConfig, + UI: uiConfig, + } + + ctlr := api.NewController(conf) + ctlr.Log.Info().Int64("seedUser", seedUser).Int64("seedPass", seedPass).Msg("random seed for username & password") + + for _, testcase := range testCases { + t.Run(testcase.testCaseName, func(t *testing.T) { + Convey("make controller", t, func() { + dir := t.TempDir() + + ctlr.Config.Storage.RootDirectory = dir + ctlr.Config.HTTP.ExternalURL = testcase.externalURL + ctlr.Config.HTTP.Address = testcase.address + + // Setup redis test server and config to use remote session store + testRedis := miniredis.RunT(t) + + conf.HTTP.Auth.SessionDriver = map[string]any{ + "name": "redis", + "url": "redis://" + testRedis.Addr(), + } + + cm := test.NewControllerManager(ctlr) + + cm.StartServer() + + defer cm.StopServer() + test.WaitTillServerReady(baseURL) + + Convey("browser client requests", func() { + Convey("login with no provider supplied", func() { + client := resty.New() + client.SetRedirectPolicy(test.CustomRedirectPolicy(20)) + // first login user + resp, err := client.R(). + SetHeader(constants.SessionClientHeaderName, constants.SessionClientHeaderValue). + SetQueryParam("provider", "unknown"). + Get(baseURL + constants.LoginPath) + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusBadRequest) + }) + + //nolint: dupl + Convey("make sure sessions are not used without UI header value", func() { + So(len(testRedis.Keys()), ShouldEqual, 0) + + client := resty.New() + + // without header should not create session + resp, err := client.R().SetBasicAuth(htpasswdUsername, htpasswdPassword).Get(baseURL + "/v2/") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + So(len(testRedis.Keys()), ShouldEqual, 0) + + client.SetHeader(constants.SessionClientHeaderName, constants.SessionClientHeaderValue) + + resp, err = client.R().SetBasicAuth(htpasswdUsername, htpasswdPassword).Get(baseURL + "/v2/") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + So(len(testRedis.Keys()), ShouldEqual, 1) + + // set cookies + client.SetCookies(resp.Cookies()) + + // should get same cookie + resp, err = client.R().SetBasicAuth(htpasswdUsername, htpasswdPassword).Get(baseURL + "/v2/") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + So(len(testRedis.Keys()), ShouldEqual, 1) + + resp, err = client.R().SetBasicAuth(htpasswdUsername, htpasswdPassword). + Get(baseURL + constants.FullMgmt) + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + client.SetCookies(resp.Cookies()) + + // call endpoint with session, without credentials, (added to client after previous request) + resp, err = client.R(). + Get(baseURL + "/v2/_catalog") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + resp, err = client.R().SetBasicAuth(htpasswdUsername, htpasswdPassword).Get(baseURL + "/v2/") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + So(len(testRedis.Keys()), ShouldEqual, 1) + }) + + Convey("login with openid and get catalog with session", func() { + client := resty.New() + client.SetRedirectPolicy(test.CustomRedirectPolicy(20)) + client.SetHeader(constants.SessionClientHeaderName, constants.SessionClientHeaderValue) + + Convey("with callback_ui value provided", func() { + // first login user + resp, err := client.R(). + SetQueryParam("provider", "oidc"). + SetQueryParam("callback_ui", baseURL+"/v2/"). + Get(baseURL + constants.LoginPath) + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + }) + + // first login user + resp, err := client.R(). + SetQueryParam("provider", "oidc"). + Get(baseURL + constants.LoginPath) + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusCreated) + + client.SetCookies(resp.Cookies()) + + // call endpoint with session (added to client after previous request) + resp, err = client.R(). + Get(baseURL + "/v2/_catalog") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + // logout with options method for coverage + resp, err = client.R(). + Options(baseURL + constants.LogoutPath) + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + + // logout user + resp, err = client.R(). + Post(baseURL + constants.LogoutPath) + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + // calling endpoint should fail with unauthorized access + resp, err = client.R(). + Get(baseURL + "/v2/_catalog") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusUnauthorized) + }) + + //nolint: dupl + Convey("login with basic auth(htpasswd) and get catalog with session", func() { + client := resty.New() + client.SetHeader(constants.SessionClientHeaderName, constants.SessionClientHeaderValue) + + // without creds, should get access error + resp, err := client.R().Get(baseURL + "/v2/") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusUnauthorized) + + var e apiErr.Error + + err = json.Unmarshal(resp.Body(), &e) + So(err, ShouldBeNil) + + // first login user + // with creds, should get expected status code + resp, err = client.R().SetBasicAuth(htpasswdUsername, htpasswdPassword).Get(baseURL) + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + resp, err = client.R().SetBasicAuth(htpasswdUsername, htpasswdPassword).Get(baseURL + "/v2/") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + resp, err = client.R().SetBasicAuth(htpasswdUsername, htpasswdPassword). + Get(baseURL + constants.FullMgmt) + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + client.SetCookies(resp.Cookies()) + + // call endpoint with session, without credentials, (added to client after previous request) + resp, err = client.R(). + Get(baseURL + "/v2/_catalog") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + resp, err = client.R(). + Get(baseURL + constants.FullMgmt) + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + // logout user + resp, err = client.R(). + Post(baseURL + constants.LogoutPath) + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + // calling endpoint should fail with unauthorized access + resp, err = client.R(). + Get(baseURL + "/v2/_catalog") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusUnauthorized) + }) + + //nolint: dupl + Convey("login with ldap and get catalog", func() { + client := resty.New() + client.SetHeader(constants.SessionClientHeaderName, constants.SessionClientHeaderValue) + + // without creds, should get access error + resp, err := client.R().Get(baseURL + "/v2/") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusUnauthorized) + + var e apiErr.Error + + err = json.Unmarshal(resp.Body(), &e) + So(err, ShouldBeNil) + + // first login user + // with creds, should get expected status code + resp, err = client.R().SetBasicAuth(username, password).Get(baseURL) + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + resp, err = client.R().SetBasicAuth(username, password).Get(baseURL + "/v2/") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + resp, err = client.R().SetBasicAuth(username, password). + Get(baseURL + constants.FullMgmt) + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + client.SetCookies(resp.Cookies()) + + // call endpoint with session, without credentials, (added to client after previous request) + resp, err = client.R(). + Get(baseURL + "/v2/_catalog") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + resp, err = client.R(). + Get(baseURL + constants.FullMgmt) + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + // logout user + resp, err = client.R(). + Post(baseURL + constants.LogoutPath) + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + // calling endpoint should fail with unauthorized access + resp, err = client.R(). + Get(baseURL + "/v2/_catalog") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusUnauthorized) + }) + + Convey("unauthenticated catalog request", func() { + client := resty.New() + + // mgmt should work both unauthenticated and authenticated + resp, err := client.R(). + Get(baseURL + constants.FullMgmt) + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + // call endpoint without session + resp, err = client.R(). + Get(baseURL + "/v2/_catalog") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusUnauthorized) + }) + }) + }) + }) + } +} + func TestIsOpenIDEnabled(t *testing.T) { Convey("make oidc server", t, func() { port := test.GetFreePort() diff --git a/pkg/api/cookiestore.go b/pkg/api/cookiestore.go index c584a562..e80345fb 100644 --- a/pkg/api/cookiestore.go +++ b/pkg/api/cookiestore.go @@ -12,7 +12,12 @@ import ( "time" "github.com/gorilla/sessions" + "github.com/rbcervilla/redisstore/v9" + "zotregistry.dev/zot/errors" + "zotregistry.dev/zot/pkg/api/config" + rediscfg "zotregistry.dev/zot/pkg/api/config/redis" + "zotregistry.dev/zot/pkg/log" "zotregistry.dev/zot/pkg/scheduler" "zotregistry.dev/zot/pkg/storage" storageConstants "zotregistry.dev/zot/pkg/storage/constants" @@ -36,7 +41,11 @@ func (c *CookieStore) RunSessionCleaner(sch *scheduler.Scheduler) { } } -func NewCookieStore(storeController storage.StoreController, hashKey, encryptKey []byte) (*CookieStore, error) { +func NewCookieStore( + authCfg *config.AuthConfig, + storeController storage.StoreController, + log log.Logger, +) (*CookieStore, error) { // To store custom types in our cookies // we must first register them using gob.Register gob.Register(map[string]interface{}{}) @@ -47,10 +56,109 @@ func NewCookieStore(storeController storage.StoreController, hashKey, encryptKey var needsCleanup bool + if authCfg.SessionDriver == nil { + // If the session driver is not configured, then + // behave in the usual way for file system cookie store and memory cookie store. + createdStore, returnedSessionsDir, doesStoreNeedCleanup, err := localSessionStoreInit( + storeController, + authCfg.SessionHashKey, + authCfg.SessionEncryptKey, + ) + if err != nil { + return &CookieStore{}, err + } + + store = createdStore + sessionsDir = returnedSessionsDir + needsCleanup = doesStoreNeedCleanup + } else { + switch authCfg.SessionDriver["name"] { + case storageConstants.RedisDriverName: + { + prefix, ok := rediscfg.GetString(authCfg.SessionDriver, "keyprefix", false, log) + if !ok { + prefix = "zotsession" + } + + // The redisstore library code uses a colon to separate the prefix + // and the actual key and is expected to be part of the prefix argument. + // ref: https://github.com/rbcervilla/redisstore/blob/v9.0.0/redisstore.go#L44 + // This adds a colon to the prefix only if it is not empty. + if prefix != "" { + prefix += ":" + } + + client, err := rediscfg.GetRedisClient(authCfg.SessionDriver, log) + if err != nil { + return nil, err + } + + redisStore, err := redisstore.NewRedisStore(context.Background(), client) + if err != nil { + return nil, err + } + + redisStore.KeyPrefix(prefix) + redisStore.Options(sessions.Options{ + MaxAge: cookiesMaxAge, + Path: "/", + }) + + store = redisStore + } + case storageConstants.LocalStorageDriverName: + { + // This behaves the same as if there was no sessionDriver config. + // It is also the same behaviour prior to supporting this config. + // This allows for a backwards compatible migration path for upgrades. + createdStore, sessDir, cleanupReq, err := localSessionStoreInit( + storeController, + authCfg.SessionHashKey, + authCfg.SessionEncryptKey, + ) + if err != nil { + return &CookieStore{}, err + } + + store = createdStore + sessionsDir = sessDir + needsCleanup = cleanupReq + } + default: + return nil, fmt.Errorf( + "%w: sessiondriver %s not supported", + errors.ErrBadConfig, + authCfg.SessionDriver["name"], + ) + } + } + + return &CookieStore{ + Store: store, + rootDir: sessionsDir, + needsCleanup: needsCleanup, + }, nil +} + +// Handles creation and init of a local session store. +// This can be either in memory or on the local file system. +// Returns a session Store, root directory for the sessions if applicable, +// a boolean indicating whether clean up is required, and an error. +func localSessionStoreInit( + storeController storage.StoreController, + hashKey, + encryptKey []byte, +) (sessions.Store, string, bool, error) { + 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 + return &CookieStore{}, "", false, err } localStore := sessions.NewFilesystemStore(sessionsDir, hashKey, encryptKey) @@ -67,11 +175,7 @@ func NewCookieStore(storeController storage.StoreController, hashKey, encryptKey store = memStore } - return &CookieStore{ - Store: store, - rootDir: sessionsDir, - needsCleanup: needsCleanup, - }, nil + return store, sessionsDir, needsCleanup, nil } func IsExpiredSession(dirEntry fs.DirEntry) bool { diff --git a/pkg/cli/server/root.go b/pkg/cli/server/root.go index 693df286..040a1dff 100644 --- a/pkg/cli/server/root.go +++ b/pkg/cli/server/root.go @@ -332,6 +332,58 @@ func validateCacheConfig(cfg *config.Config, logger zlog.Logger) error { return nil } +func validateRemoteSessionStoreConfig(cfg *config.Config, logger zlog.Logger) error { + // it is okay for the session driver config to be nil + // this is backwards compatible for older configs + if cfg.HTTP.Auth.SessionDriver == nil { + return nil + } + + sessionDriverName, ok := cfg.HTTP.Auth.SessionDriver["name"] + if !ok { + msg := "must provide session driver name!" + logger.Error().Err(zerr.ErrBadConfig).Msg(msg) + + return fmt.Errorf("%w: %s", zerr.ErrBadConfig, msg) + } + + allowedDriverNames := []string{ + storageConstants.RedisDriverName, + storageConstants.LocalStorageDriverName, + } + + isValidDriver := false + + for _, allowedDriverName := range allowedDriverNames { + if allowedDriverName == sessionDriverName { + isValidDriver = true + + break + } + } + + if !isValidDriver { + msg := fmt.Sprintf("session store driver %s is not allowed!", sessionDriverName) + logger.Error().Err(zerr.ErrBadConfig).Msg(msg) + + return fmt.Errorf("%w: %s", zerr.ErrBadConfig, msg) + } + + // If the redis driver is being used, then session keys must not be configured + // as redis session store does not support these yet. + + if sessionDriverName == storageConstants.RedisDriverName { + if cfg.HTTP.Auth.SessionKeysFile != "" { + msg := "session keys not supported when redis session driver is used!" + logger.Error().Err(zerr.ErrBadConfig).Msg(msg) + + return fmt.Errorf("%w: %s", zerr.ErrBadConfig, msg) + } + } + + return nil +} + func validateExtensionsConfig(cfg *config.Config, logger zlog.Logger) error { if cfg.Extensions != nil && cfg.Extensions.Mgmt != nil { logger.Warn().Msg("mgmt extensions configuration option has been made redundant and will be ignored.") @@ -405,6 +457,10 @@ func validateConfiguration(config *config.Config, logger zlog.Logger) error { return err } + if err := validateRemoteSessionStoreConfig(config, logger); err != nil { + return err + } + if err := validateExtensionsConfig(config, logger); err != nil { return err } diff --git a/pkg/cli/server/root_test.go b/pkg/cli/server/root_test.go index 1abe5765..308dbc44 100644 --- a/pkg/cli/server/root_test.go +++ b/pkg/cli/server/root_test.go @@ -11,6 +11,7 @@ import ( . "github.com/smartystreets/goconvey/convey" + zerr "zotregistry.dev/zot/errors" "zotregistry.dev/zot/pkg/api" "zotregistry.dev/zot/pkg/api/config" cli "zotregistry.dev/zot/pkg/cli/server" @@ -572,6 +573,255 @@ storage: So(err, ShouldBeNil) }) + Convey("Test session store config", t, func(c C) { + tmpfile, err := os.CreateTemp("", "zot-test*.json") + So(err, ShouldBeNil) + + defer os.Remove(tmpfile.Name()) + + tmpSessionKeysFile, err := os.CreateTemp("/tmp", "keys-*.json") + So(err, ShouldBeNil) + + defer os.Remove(tmpSessionKeysFile.Name()) + + _, err = tmpSessionKeysFile.WriteString(`{ + "hashKey": "my-very-secret", + "encryptKey": "another-secret" + }`, + ) + So(err, ShouldBeNil) + + testCases := []struct { + name string + config []byte + isValid bool + errMsg string + }{ + { + "Should fail verify if session driver is enabled, but invalid driver provided", + []byte(`{ + "storage":{ + "rootDirectory":"/tmp/zot" + }, + "http":{ + "address":"127.0.0.1", + "port":"8080", + "realm":"zot", + "auth":{ + "htpasswd":{ + "path":"test/data/htpasswd" + }, + "failDelay":1, + "sessionDriver":{ + "name": "badDriver" + } + } + }, + "extensions":{ + "search": { + "cve": { + "updateInterval": "2h" + } + }, + "ui": { + "enable": true + } + } + }`), + false, + zerr.ErrBadConfig.Error() + + ": session store driver badDriver is not allowed!", + }, + { + "Should fail verify if session driver is enabled, but driver name is not provided", + []byte(`{ + "storage":{ + "rootDirectory":"/tmp/zot" + }, + "http":{ + "address":"127.0.0.1", + "port":"8080", + "realm":"zot", + "auth":{ + "htpasswd":{ + "path":"test/data/htpasswd" + }, + "failDelay":1, + "sessionDriver":{ + "url": "redis://localhost" + } + } + }, + "extensions":{ + "search": { + "cve": { + "updateInterval": "2h" + } + }, + "ui": { + "enable": true + } + } + }`), + false, + zerr.ErrBadConfig.Error() + ": must provide session driver name!", + }, + { + "Should fail verify if session driver is enabled and sessionKeysFile present", + []byte(fmt.Sprintf(`{ + "storage":{ + "rootDirectory":"/tmp/zot" + }, + "http":{ + "address":"127.0.0.1", + "port":"8080", + "realm":"zot", + "auth":{ + "htpasswd":{ + "path":"test/data/htpasswd" + }, + "failDelay":1, + "sessionKeysFile": "%s", + "sessionDriver":{ + "name": "redis", + "url": "redis://localhost" + } + } + }, + "extensions":{ + "search": { + "cve": { + "updateInterval": "2h" + } + }, + "ui": { + "enable": true + } + } + }`, tmpSessionKeysFile.Name())), + false, + zerr.ErrBadConfig.Error() + ": session keys not supported when redis session driver is used!", + }, + { + "Should be successful if session driver config is valid for redis", + []byte(`{ + "storage":{ + "rootDirectory":"/tmp/zot" + }, + "http":{ + "address":"127.0.0.1", + "port":"8080", + "realm":"zot", + "auth":{ + "htpasswd":{ + "path":"test/data/htpasswd" + }, + "failDelay":1, + "sessionDriver":{ + "name": "redis", + "url": "redis://localhost" + } + } + }, + "extensions":{ + "search": { + "cve": { + "updateInterval": "2h" + } + }, + "ui": { + "enable": true + } + } + }`), + true, + "", + }, + { + "Should be successful if session driver config is valid for local", + []byte(`{ + "storage":{ + "rootDirectory":"/tmp/zot" + }, + "http":{ + "address":"127.0.0.1", + "port":"8080", + "realm":"zot", + "auth":{ + "htpasswd":{ + "path":"test/data/htpasswd" + }, + "failDelay":1, + "sessionDriver":{ + "name": "local" + } + } + }, + "extensions":{ + "search": { + "cve": { + "updateInterval": "2h" + } + }, + "ui": { + "enable": true + } + } + }`), + true, + "", + }, + { + "Should be successful if session driver config is missing", + []byte(`{ + "storage":{ + "rootDirectory":"/tmp/zot" + }, + "http":{ + "address":"127.0.0.1", + "port":"8080", + "realm":"zot", + "auth":{ + "htpasswd":{ + "path":"test/data/htpasswd" + }, + "failDelay":1 + } + }, + "extensions":{ + "search": { + "cve": { + "updateInterval": "2h" + } + }, + "ui": { + "enable": true + } + } + }`), + true, + "", + }, + } + + for _, testCase := range testCases { + Convey(testCase.name, func() { + err = os.WriteFile(tmpfile.Name(), testCase.config, 0o0600) + So(err, ShouldBeNil) + + os.Args = []string{"cli_test", "verify", tmpfile.Name()} + err = cli.NewServerRootCmd().Execute() + + if testCase.isValid { + So(err, ShouldBeNil) + } else { + So(err, ShouldNotBeNil) + So(err.Error(), ShouldEqual, testCase.errMsg) + } + }) + } + }) + Convey("Test verify with bad gc retention repo patterns", t, func(c C) { tmpfile, err := os.CreateTemp("", "zot-test*.json") So(err, ShouldBeNil) diff --git a/test/blackbox/ci.sh b/test/blackbox/ci.sh index 20efcb08..12aff31c 100755 --- a/test/blackbox/ci.sh +++ b/test/blackbox/ci.sh @@ -9,7 +9,7 @@ PATH=$PATH:${SCRIPTPATH}/../../hack/tools/bin tests=("pushpull" "pushpull_authn" "delete_images" "referrers" "metadata" "anonymous_policy" "annotations" "detect_manifest_collision" "cve" "sync" "sync_docker" "sync_replica_cluster" - "scrub" "garbage_collect" "metrics" "metrics_minimal" "multiarch_index" "docker_compat" "redis_local" + "scrub" "garbage_collect" "metrics" "metrics_minimal" "multiarch_index" "docker_compat" "redis_local" "redis_session_store" "events_nats" "events_http" "events_nats_lint_failure" "events_http_lint_failure" "events_sink_failure" "events_config_decoding") for test in ${tests[*]}; do diff --git a/test/blackbox/redis_session_store.bats b/test/blackbox/redis_session_store.bats new file mode 100644 index 00000000..0b9abf4f --- /dev/null +++ b/test/blackbox/redis_session_store.bats @@ -0,0 +1,253 @@ +# Note: Intended to be run as "make run-blackbox-tests" or "make run-blackbox-ci" +# Makefile target installs & checks all necessary tooling +# Extra tools that are not covered in Makefile target needs to be added in verify_prerequisites() + +load helpers_zot +load helpers_redis +load ../port_helper + +function verify_prerequisites() { + if [ ! $(command -v curl) ]; then + echo "you need to install curl as a prerequisite to running the tests" >&3 + return 1 + fi + + if [ ! $(command -v docker) ]; then + echo "you need to install docker as a prerequisite to running the tests" >&3 + return 1 + fi + + if [ ! $(command -v valkey-cli) ]; then + echo "you need to install valkey-cli as a prerequisite to running the tests" >&3 + return 1 + fi + + return 0 +} + +HTPASSWD_PATH=/tmp/zotpasswd +CURL_COOKIES_DIR=/tmp/zotcookies +REDIS_TEST_CONTAINER_NAME="redis_sessions_server_local" + +function setup_file() { + # Verify prerequisites are available + if ! $(verify_prerequisites); then + exit 1 + fi + + mkdir -p ${CURL_COOKIES_DIR} + + # Create htpasswd file for basic auth + htpasswd -bBn test test123 > ${HTPASSWD_PATH} + + # Setup redis server + redis_port=$(get_free_port_for_service "redis") + redis_start ${REDIS_TEST_CONTAINER_NAME} ${redis_port} + + # Setup zot server + local zot_root_dir=${BATS_FILE_TMPDIR}/zot + local zot_redis_session_config_file=${BATS_FILE_TMPDIR}/zot_redis_session_config.json + zot_port=$(get_free_port_for_service "zot") + echo ${zot_port} > ${BATS_FILE_TMPDIR}/zot.port + echo ${redis_port} > ${BATS_FILE_TMPDIR}/redis.port + + mkdir -p ${zot_root_dir} + + cat >${zot_redis_session_config_file} <