diff --git a/pkg/cli/server/root.go b/pkg/cli/server/root.go index f8215858..84fa8809 100644 --- a/pkg/cli/server/root.go +++ b/pkg/cli/server/root.go @@ -242,32 +242,138 @@ func NewServerRootCmd() *cobra.Command { return rootCmd } -func validateStorageConfig(cfg *config.Config, logger zlog.Logger) error { - expConfigMap := make(map[string]config.StorageConfig, 0) +// isPathInside checks if path1 is inside path2 (path1 is a subdirectory of path2) +// This function is platform-agnostic and handles Windows drive letters, UNC paths, and symlinks. +func isPathInside(path1, path2 string) bool { + // Normalize paths to absolute paths (handles platform-specific separators and symlinks) + abs1, err1 := filepath.Abs(path1) + abs2, err2 := filepath.Abs(path2) + if err1 != nil || err2 != nil { + return false + } + + // On Windows, if paths are on different drives, filepath.Rel returns an error + // which we handle by returning false (paths on different drives are not nested) + rel, err := filepath.Rel(abs2, abs1) + if err != nil { + return false + } + + // If the relative path doesn't start with "..", then path1 is inside path2 + // Also check that it's not "." (same directory) or empty (same path) + // On Windows, filepath.Rel uses backslashes, but strings.HasPrefix works with any separator + return rel != "." && rel != "" && !strings.HasPrefix(rel, "..") +} + +// pathsConflict checks if two paths conflict (identical or nested) and returns: +// - 0: no conflict +// - 1: paths are identical +// - 2: path1 is inside path2 +// - 3: path2 is inside path1. +func pathsConflict(path1, path2 string) int { + if strings.EqualFold(path1, path2) { + return 1 + } + + if isPathInside(path1, path2) { + return 2 + } + + if isPathInside(path2, path1) { + return 3 + } + + return 0 +} + +// getStorageType returns the storage driver type name. +// Returns "local" if StorageDriver is nil, otherwise extracts the name from StorageDriver["name"]. +func getStorageType(storageDriver map[string]any) string { + if storageDriver == nil { + return storageConstants.LocalStorageDriverName + } + + storeName := fmt.Sprintf("%v", storageDriver["name"]) + if storeName == storageConstants.S3StorageDriverName { + return storageConstants.S3StorageDriverName + } + + return storeName +} + +func validateStorageConfig(cfg *config.Config, logger zlog.Logger) error { storageConfig := cfg.CopyStorageConfig() defaultRootDir := storageConfig.RootDirectory + defaultStorageType := getStorageType(storageConfig.StorageDriver) - for _, subStorageConfig := range storageConfig.SubPaths { - if strings.EqualFold(defaultRootDir, subStorageConfig.RootDirectory) { - msg := "invalid storage config, storage subpaths cannot use default storage root directory" - logger.Error().Err(zerr.ErrBadConfig).Msg(msg) + // Collect all store root directories (default + substores) for nested path checking + type storeInfo struct { + route string // empty for default store + rootDir string + storageType string + } + allStores := make([]storeInfo, 0, 1+len(storageConfig.SubPaths)) + + allStores = append(allStores, storeInfo{route: "", rootDir: defaultRootDir, storageType: defaultStorageType}) + for route, subStorageConfig := range storageConfig.SubPaths { + allStores = append(allStores, storeInfo{ + route: route, + rootDir: subStorageConfig.RootDirectory, + storageType: getStorageType(subStorageConfig.StorageDriver), + }) + } + + // Validate each store + for _, store := range allStores { + route := store.route + rootDir := store.rootDir + storageType := store.storageType + + // Check if this store conflicts with any other store of the same type (identical or nested paths) + conflictingIdx := slices.IndexFunc(allStores, func(other storeInfo) bool { + return other.route != route && + other.storageType == storageType && + pathsConflict(rootDir, other.rootDir) != 0 + }) + + if conflictingIdx >= 0 { + other := allStores[conflictingIdx] + conflictType := pathsConflict(rootDir, other.rootDir) + + var storeName, otherStoreName string + if route == "" { + storeName = "default storage" + } else { + storeName = fmt.Sprintf("substore (route: %s)", route) + } + if other.route == "" { + otherStoreName = "default storage" + } else { + otherStoreName = fmt.Sprintf("substore (route: %s)", other.route) + } + + var msg string + switch conflictType { + case 1: // identical + msg = fmt.Sprintf("invalid storage config, %s and %s cannot use the same root directory", storeName, otherStoreName) + case 2: // rootDir is inside other.rootDir + msg = fmt.Sprintf("invalid storage config, %s root directory cannot be inside %s root directory", + storeName, otherStoreName) + case 3: // other.rootDir is inside rootDir + msg = fmt.Sprintf("invalid storage config, %s root directory cannot be inside %s root directory", + otherStoreName, storeName) + } + + logger.Error().Err(zerr.ErrBadConfig). + Str("rootDir", rootDir). + Str("otherRootDir", other.rootDir). + Str("route", route). + Str("otherRoute", other.route). + Msg(msg) return fmt.Errorf("%w: %s", zerr.ErrBadConfig, msg) } - - expConfig, ok := expConfigMap[subStorageConfig.RootDirectory] - if ok { - equal := expConfig.ParamsEqual(subStorageConfig) - if !equal { - msg := "invalid storage config, storage config with same root directory should have same parameters" - logger.Error().Err(zerr.ErrBadConfig).Msg(msg) - - return fmt.Errorf("%w: %s", zerr.ErrBadConfig, msg) - } - } else { - expConfigMap[subStorageConfig.RootDirectory] = subStorageConfig - } } return nil diff --git a/pkg/cli/server/root_test.go b/pkg/cli/server/root_test.go index ca4d4130..c03aa3c4 100644 --- a/pkg/cli/server/root_test.go +++ b/pkg/cli/server/root_test.go @@ -1076,7 +1076,11 @@ storage: os.Args = []string{"cli_test", "verify", tmpfile.Name()} err = cli.NewServerRootCmd().Execute() - So(err, ShouldBeNil) + // Two substores of the same type cannot use the same root directory + So(err, ShouldNotBeNil) + So(err.Error(), ShouldContainSubstring, "cannot use the same root directory") + So(err.Error(), ShouldContainSubstring, "substore (route: /a)") + So(err.Error(), ShouldContainSubstring, "substore (route: /b)") // sub paths that point to same directory should have same storage config. content = []byte(`{"storage":{"rootDirectory":"/tmp/zot", @@ -1090,6 +1094,10 @@ storage: os.Args = []string{"cli_test", "verify", tmpfile.Name()} err = cli.NewServerRootCmd().Execute() So(err, ShouldNotBeNil) + // Two substores of the same type cannot use the same root directory + So(err.Error(), ShouldContainSubstring, "cannot use the same root directory") + So(err.Error(), ShouldContainSubstring, "substore (route: /a)") + So(err.Error(), ShouldContainSubstring, "substore (route: /b)") // sub paths that point to default root directory should not be allowed. content = []byte(`{"storage":{"rootDirectory":"/tmp/zot", @@ -1152,6 +1160,189 @@ storage: So(err, ShouldNotBeNil) }) + Convey("Test verify storage config with different storage types", t, func(c C) { + tmpfile, err := os.CreateTemp("", "zot-test*.json") + So(err, ShouldBeNil) + + defer os.Remove(tmpfile.Name()) // clean up + + // Local and S3 stores with same rootDir should be allowed (different storage types) + content := []byte(`{"storage":{"rootDirectory":"/tmp/zot", + "subPaths": {"/a": {"rootDirectory": "/tmp/zot", + "storageDriver":{"name":"s3","rootdirectory":"/tmp/zot","region":"us-east-2", + "bucket":"zot-storage","secure":true,"skipverify":false}}}}, + "http":{"address":"127.0.0.1","port":"8080","realm":"zot", + "auth":{"htpasswd":{"path":"test/data/htpasswd"},"failDelay":1}}}`) + err = os.WriteFile(tmpfile.Name(), content, 0o0600) + So(err, ShouldBeNil) + + os.Args = []string{"cli_test", "verify", tmpfile.Name()} + err = cli.NewServerRootCmd().Execute() + So(err, ShouldBeNil) + + // Two local stores with same rootDir should be rejected (same storage type) + content = []byte(`{"storage":{"rootDirectory":"/tmp/zot", + "subPaths": {"/a": {"rootDirectory": "/tmp/zot"}}}, + "http":{"address":"127.0.0.1","port":"8080","realm":"zot", + "auth":{"htpasswd":{"path":"test/data/htpasswd"},"failDelay":1}}}`) + err = os.WriteFile(tmpfile.Name(), content, 0o0600) + So(err, ShouldBeNil) + + os.Args = []string{"cli_test", "verify", tmpfile.Name()} + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) + // Two stores of the same type cannot use the same root directory + So(err.Error(), ShouldContainSubstring, "cannot use the same root directory") + So(err.Error(), ShouldContainSubstring, "default storage") + So(err.Error(), ShouldContainSubstring, "substore (route: /a)") + + // Two S3 stores with same rootDir should be rejected (same storage type) + content = []byte(`{"storage":{"rootDirectory":"/zot", + "storageDriver":{"name":"s3","rootdirectory":"/zot","region":"us-east-2", + "bucket":"zot-storage","secure":true,"skipverify":false}, + "dedupe":false, + "subPaths": {"/a": {"rootDirectory": "/zot", + "storageDriver":{"name":"s3","rootdirectory":"/zot","region":"us-east-2", + "bucket":"zot-storage","secure":true,"skipverify":false}, + "dedupe":false}}}, + "http":{"address":"127.0.0.1","port":"8080","realm":"zot", + "auth":{"htpasswd":{"path":"test/data/htpasswd"},"failDelay":1}}}`) + err = os.WriteFile(tmpfile.Name(), content, 0o0600) + So(err, ShouldBeNil) + + os.Args = []string{"cli_test", "verify", tmpfile.Name()} + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) + // Two stores of the same type cannot use the same root directory + So(err.Error(), ShouldContainSubstring, "cannot use the same root directory") + So(err.Error(), ShouldContainSubstring, "default storage") + So(err.Error(), ShouldContainSubstring, "substore (route: /a)") + + // Local store with nested path inside default local store should be rejected + content = []byte(`{"storage":{"rootDirectory":"/tmp/zot", + "subPaths": {"/a": {"rootDirectory": "/tmp/zot/subdir"}}}, + "http":{"address":"127.0.0.1","port":"8080","realm":"zot", + "auth":{"htpasswd":{"path":"test/data/htpasswd"},"failDelay":1}}}`) + err = os.WriteFile(tmpfile.Name(), content, 0o0600) + So(err, ShouldBeNil) + + os.Args = []string{"cli_test", "verify", tmpfile.Name()} + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) + So(err.Error(), ShouldContainSubstring, + "invalid storage config, substore (route: /a) root directory cannot be inside default storage root directory") + + // S3 store with nested path inside default S3 store should be rejected + content = []byte(`{"storage":{"rootDirectory":"/zot", + "storageDriver":{"name":"s3","rootdirectory":"/zot","region":"us-east-2", + "bucket":"zot-storage","secure":true,"skipverify":false}, + "dedupe":false, + "subPaths": {"/a": {"rootDirectory": "/zot/subdir", + "storageDriver":{"name":"s3","rootdirectory":"/zot/subdir","region":"us-east-2", + "bucket":"zot-storage","secure":true,"skipverify":false}, + "dedupe":false}}}, + "http":{"address":"127.0.0.1","port":"8080","realm":"zot", + "auth":{"htpasswd":{"path":"test/data/htpasswd"},"failDelay":1}}}`) + err = os.WriteFile(tmpfile.Name(), content, 0o0600) + So(err, ShouldBeNil) + + os.Args = []string{"cli_test", "verify", tmpfile.Name()} + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) + So(err.Error(), ShouldContainSubstring, + "invalid storage config, substore (route: /a) root directory cannot be inside default storage root directory") + + // Local store with nested path inside S3 store should be allowed (different storage types) + content = []byte(`{"storage":{"rootDirectory":"/zot", + "storageDriver":{"name":"s3","rootdirectory":"/zot","region":"us-east-2", + "bucket":"zot-storage","secure":true,"skipverify":false}, + "dedupe":false, + "subPaths": {"/a": {"rootDirectory": "/zot/subdir"}}}, + "http":{"address":"127.0.0.1","port":"8080","realm":"zot", + "auth":{"htpasswd":{"path":"test/data/htpasswd"},"failDelay":1}}}`) + err = os.WriteFile(tmpfile.Name(), content, 0o0600) + So(err, ShouldBeNil) + + os.Args = []string{"cli_test", "verify", tmpfile.Name()} + err = cli.NewServerRootCmd().Execute() + So(err, ShouldBeNil) + + // S3 store with nested path inside local store should be allowed (different storage types) + content = []byte(`{"storage":{"rootDirectory":"/tmp/zot", + "subPaths": {"/a": {"rootDirectory": "/tmp/zot/subdir", + "storageDriver":{"name":"s3","rootdirectory":"/tmp/zot/subdir","region":"us-east-2", + "bucket":"zot-storage","secure":true,"skipverify":false}, + "dedupe":false}}}, + "http":{"address":"127.0.0.1","port":"8080","realm":"zot", + "auth":{"htpasswd":{"path":"test/data/htpasswd"},"failDelay":1}}}`) + err = os.WriteFile(tmpfile.Name(), content, 0o0600) + So(err, ShouldBeNil) + + os.Args = []string{"cli_test", "verify", tmpfile.Name()} + err = cli.NewServerRootCmd().Execute() + So(err, ShouldBeNil) + + // Two local substores with nested paths should be rejected + // /a is at /tmp/zot-a (not nested in default), /b is nested inside /a + content = []byte(`{"storage":{"rootDirectory":"/tmp/zot", + "subPaths": {"/a": {"rootDirectory": "/tmp/zot-a"}, + "/b": {"rootDirectory": "/tmp/zot-a/subdir"}}}, + "http":{"address":"127.0.0.1","port":"8080","realm":"zot", + "auth":{"htpasswd":{"path":"test/data/htpasswd"},"failDelay":1}}}`) + err = os.WriteFile(tmpfile.Name(), content, 0o0600) + So(err, ShouldBeNil) + + os.Args = []string{"cli_test", "verify", tmpfile.Name()} + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) + // /b is nested inside /a, validation reports this conflict + So(err.Error(), ShouldContainSubstring, + "invalid storage config, substore (route: /b) root directory cannot be inside substore (route: /a) root directory") + + // Two S3 substores with nested paths should be rejected + // /a is at /zot-a (not nested in default), /b is nested inside /a + content = []byte(`{"storage":{"rootDirectory":"/zot", + "storageDriver":{"name":"s3","rootdirectory":"/zot","region":"us-east-2", + "bucket":"zot-storage","secure":true,"skipverify":false}, + "dedupe":false, + "subPaths": {"/a": {"rootDirectory": "/zot-a", + "storageDriver":{"name":"s3","rootdirectory":"/zot-a","region":"us-east-2", + "bucket":"zot-storage","secure":true,"skipverify":false}, + "dedupe":false}, + "/b": {"rootDirectory": "/zot-a/subdir", + "storageDriver":{"name":"s3","rootdirectory":"/zot-a/subdir","region":"us-east-2", + "bucket":"zot-storage","secure":true,"skipverify":false}, + "dedupe":false}}}, + "http":{"address":"127.0.0.1","port":"8080","realm":"zot", + "auth":{"htpasswd":{"path":"test/data/htpasswd"},"failDelay":1}}}`) + err = os.WriteFile(tmpfile.Name(), content, 0o0600) + So(err, ShouldBeNil) + + os.Args = []string{"cli_test", "verify", tmpfile.Name()} + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) + // /b is nested inside /a, validation reports this conflict + So(err.Error(), ShouldContainSubstring, + "invalid storage config, substore (route: /b) root directory cannot be inside substore (route: /a) root directory") + + // Local and S3 substores with nested paths should be allowed (different storage types) + // /a is local at /tmp/zot-a (not nested in default), /b is S3 nested inside /a + content = []byte(`{"storage":{"rootDirectory":"/tmp/zot", + "subPaths": {"/a": {"rootDirectory": "/tmp/zot-a"}, + "/b": {"rootDirectory": "/tmp/zot-a/subdir", + "storageDriver":{"name":"s3","rootdirectory":"/tmp/zot-a/subdir","region":"us-east-2", + "bucket":"zot-storage","secure":true,"skipverify":false}, + "dedupe":false}}}, + "http":{"address":"127.0.0.1","port":"8080","realm":"zot", + "auth":{"htpasswd":{"path":"test/data/htpasswd"},"failDelay":1}}}`) + err = os.WriteFile(tmpfile.Name(), content, 0o0600) + So(err, ShouldBeNil) + + os.Args = []string{"cli_test", "verify", tmpfile.Name()} + err = cli.NewServerRootCmd().Execute() + So(err, ShouldBeNil) + }) + Convey("Test verify w/ authorization and w/o authentication", t, func(c C) { tmpfile, err := os.CreateTemp("", "zot-test*.json") So(err, ShouldBeNil) @@ -2202,7 +2393,11 @@ func TestLoadConfig(t *testing.T) { err = os.WriteFile(tmpfile.Name(), content, 0o0600) So(err, ShouldBeNil) err = cli.LoadConfiguration(config, tmpfile.Name()) - So(err, ShouldBeNil) + // Two substores of the same type cannot use the same root directory + So(err, ShouldNotBeNil) + So(err.Error(), ShouldContainSubstring, "cannot use the same root directory") + So(err.Error(), ShouldContainSubstring, "substore (route: /a)") + So(err.Error(), ShouldContainSubstring, "substore (route: /b)") }) Convey("Test HTTP port", t, func() { @@ -2214,7 +2409,7 @@ func TestLoadConfig(t *testing.T) { content := []byte(`{"storage":{"rootDirectory":"/tmp/zot", "subPaths": {"/a": {"rootDirectory": "/zot-a","dedupe":"true"}, - "/b": {"rootDirectory": "/zot-a","dedupe":"true"}}}, + "/b": {"rootDirectory": "/zot-b","dedupe":"true"}}}, "http":{"address":"127.0.0.1","port":"8080","realm":"zot", "auth":{"htpasswd":{"path":"test/data/htpasswd"},"failDelay":1}}}`) err = os.WriteFile(tmpfile.Name(), content, 0o0600) @@ -2224,7 +2419,7 @@ func TestLoadConfig(t *testing.T) { content = []byte(`{"storage":{"rootDirectory":"/tmp/zot", "subPaths": {"/a": {"rootDirectory": "/zot-a","dedupe":"true"}, - "/b": {"rootDirectory": "/zot-a","dedupe":"true"}}}, + "/b": {"rootDirectory": "/zot-b","dedupe":"true"}}}, "http":{"address":"127.0.0.1","port":"-1","realm":"zot", "auth":{"htpasswd":{"path":"test/data/htpasswd"},"failDelay":1}}}`) err = os.WriteFile(tmpfile.Name(), content, 0o0600)