Merge branch 'testing-chroot-changes' into 'master'

Allow serving zip from disk in chroot

See merge request gitlab-org/gitlab-pages!457

5 files changed, 88 insertions, 12 deletions

diff --git a/internal/config/config.go b/internal/config/config.go
index 0842a195..528b586a 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -146,6 +146,10 @@ type ZipServing struct {
 	RefreshInterval    time.Duration
 	OpenTimeout        time.Duration
 	AllowedPaths       []string
+	// TODO: this is a temporary workaround for https://gitlab.com/gitlab-org/gitlab/-/issues/326117#note_546346101
+	// where daemon-inplace-chroot=true fails to serve zip archives when pages_serve_with_zip_file_protocol is enabled
+	// To be removed along with chroot support https://gitlab.com/gitlab-org/gitlab-pages/-/issues/561
+	ChrootPath string
 }
 
 func gitlabServerFromFlags() string {
@@ -329,6 +333,13 @@ func loadConfig() *Config {
 		setGitLabAPISecretKey(*gitLabAPISecretKey, config)
 	}
 
+	// TODO: this is a temporary workaround for https://gitlab.com/gitlab-org/gitlab/-/issues/326117#note_546346101
+	// where daemon-inplace-chroot=true fails to serve zip archives when pages_serve_with_zip_file_protocol is enabled
+	// To be removed along with chroot support https://gitlab.com/gitlab-org/gitlab-pages/-/issues/561
+	if config.Daemon.InplaceChroot {
+		config.Zip.ChrootPath = *pagesRoot
+	}
+
 	validateConfig(config)
 
 	return config
diff --git a/internal/httpfs/http_fs.go b/internal/httpfs/http_fs.go
index cd2edb83..3578352f 100644
--- a/internal/httpfs/http_fs.go
+++ b/internal/httpfs/http_fs.go
@@ -8,6 +8,7 @@ package httpfs
 
 import (
 	"errors"
+	"fmt"
 	"net/http"
 	"os"
 	"path"
@@ -24,15 +25,28 @@ var (
 // fileSystemPaths implements the http.FileSystem interface
 type fileSystemPaths struct {
 	allowedPaths []string
+	// workaround for https://gitlab.com/gitlab-org/gitlab/-/issues/326117#note_546346101
+	// where daemon-inplace-chroot=true fails to serve zip archives when
+	// pages_serve_with_zip_file_protocol is enabled
+	// TODO: evaluate if we need to remove this field when we remove
+	// chroot https://gitlab.com/gitlab-org/gitlab-pages/-/issues/561
+	chrootPath string
 }
 
 // NewFileSystemPath creates a new fileSystemPaths that can be used to register
-// a file:// protocol with an http.Transport
-func NewFileSystemPath(allowedPaths []string) (http.FileSystem, error) {
-	for k, path := range allowedPaths {
-		var err error
+// a file:// protocol with an http.Transport.
+// When the daemon runs inside a chroot we need to strip chrootPath out of each
+// of the allowedPaths so that we are able to find the file correctly inside
+// the chroot. When Open is called, the same chrootPath will be stripped out of
+// the full filepath.
+func NewFileSystemPath(allowedPaths []string, chrootPath string) (http.FileSystem, error) {
+	for k, allowedPath := range allowedPaths {
+		strippedPath, err := stripChrootPath(ensureEndingSlash(allowedPath), chrootPath)
+		if err != nil {
+			return nil, err
+		}
 
-		allowedPaths[k], err = filepath.Abs(path)
+		allowedPaths[k], err = filepath.Abs(strippedPath)
 		if err != nil {
 			return nil, err
 		}
@@ -40,6 +54,7 @@ func NewFileSystemPath(allowedPaths []string) (http.FileSystem, error) {
 
 	return &fileSystemPaths{
 		allowedPaths: allowedPaths,
+		chrootPath:   chrootPath,
 	}, nil
 }
 
@@ -50,12 +65,26 @@ func (p *fileSystemPaths) Open(name string) (http.File, error) {
 		return nil, errInvalidChar
 	}
 
-	absPath, err := filepath.Abs(filepath.FromSlash(path.Clean("/" + name)))
+	cleanedPath := filepath.FromSlash(path.Clean("/" + name))
+
+	// since deamon can run in a chroot, we allow to define a chroot path that will be stripped from
+	// the FS location
+	// TODO: evaluate if we need to remove this check when we remove chroot
+	// https://gitlab.com/gitlab-org/gitlab-pages/-/issues/561
+	strippedPath, err := stripChrootPath(cleanedPath, p.chrootPath)
+	if err != nil {
+		log.WithError(err).Error(os.ErrPermission)
+
+		return nil, os.ErrPermission
+	}
+
+	absPath, err := filepath.Abs(strippedPath)
 	if err != nil {
 		return nil, err
 	}
 	for _, allowedPath := range p.allowedPaths {
-		if strings.HasPrefix(absPath, allowedPath+"/") {
+		// allowedPath may be a single / in chroot so we need to ensure it's not double slash
+		if strings.HasPrefix(absPath, ensureEndingSlash(allowedPath)) {
 			return os.Open(absPath)
 		}
 	}
@@ -67,3 +96,26 @@ func (p *fileSystemPaths) Open(name string) (http.File, error) {
 	// https://github.com/golang/go/blob/release-branch.go1.15/src/net/http/fs.go#L635
 	return nil, os.ErrPermission
 }
+
+func ensureEndingSlash(path string) string {
+	if strings.HasSuffix(path, "/") {
+		return path
+	}
+
+	return path + "/"
+}
+
+func stripChrootPath(path, chrootPath string) (string, error) {
+	if chrootPath == "" {
+		return path, nil
+	}
+
+	if !strings.HasPrefix(path, chrootPath+"/") {
+		return "", fmt.Errorf("allowed path %q is not in chroot path %q", path, chrootPath)
+	}
+
+	// path will contain a leading `/`
+	path = path[len(chrootPath):]
+
+	return path, nil
+}
diff --git a/internal/httpfs/http_fs_test.go b/internal/httpfs/http_fs_test.go
index 3d1092b6..7885dbf2 100644
--- a/internal/httpfs/http_fs_test.go
+++ b/internal/httpfs/http_fs_test.go
@@ -19,6 +19,7 @@ func TestFSOpen(t *testing.T) {
 
 	tests := map[string]struct {
 		allowedPaths    []string
+		chrootPath      string
 		fileName        string
 		expectedContent string
 		expectedErrMsg  string
@@ -58,11 +59,17 @@ func TestFSOpen(t *testing.T) {
 			fileName:       wd + "/../httpfs/testdata/file1.txt",
 			expectedErrMsg: os.ErrPermission.Error(),
 		},
+		"chroot_path_not_found_when_not_in_real_chroot": {
+			allowedPaths:   []string{wd + "/testdata"},
+			fileName:       wd + "/testdata/file1.txt",
+			expectedErrMsg: "no such file or directory",
+			chrootPath:     wd + "/testdata",
+		},
 	}
 
 	for name, test := range tests {
 		t.Run(name, func(t *testing.T) {
-			p, err := NewFileSystemPath(test.allowedPaths)
+			p, err := NewFileSystemPath(test.allowedPaths, test.chrootPath)
 			require.NoError(t, err)
 
 			got, err := p.Open(test.fileName)
@@ -88,6 +95,7 @@ func TestFileSystemPathCanServeHTTP(t *testing.T) {
 
 	tests := map[string]struct {
 		path               string
+		chrootPath         string
 		fileName           string
 		escapeURL          bool
 		expectedStatusCode int
@@ -136,12 +144,19 @@ func TestFileSystemPathCanServeHTTP(t *testing.T) {
 			expectedStatusCode: http.StatusForbidden,
 			expectedContent:    "403 Forbidden\n",
 		},
+		"chroot_path_fails_in_unit_test_not_found_when_not_in_real_chroot": {
+			path:               wd + "/testdata",
+			fileName:           "file1.txt",
+			chrootPath:         wd + "/testdata",
+			expectedStatusCode: http.StatusNotFound,
+			expectedContent:    "404 page not found\n",
+		},
 	}
 
 	for name, test := range tests {
 		t.Run(name, func(t *testing.T) {
 			transport := httptransport.NewTransport()
-			fs, err := NewFileSystemPath([]string{test.path})
+			fs, err := NewFileSystemPath([]string{test.path}, test.chrootPath)
 			require.NoError(t, err)
 
 			transport.RegisterProtocol("file", http.NewFileTransport(fs))
diff --git a/internal/vfs/zip/vfs.go b/internal/vfs/zip/vfs.go
index b69522f9..9c09384f 100644
--- a/internal/vfs/zip/vfs.go
+++ b/internal/vfs/zip/vfs.go
@@ -104,7 +104,7 @@ func (fs *zipVFS) Reconfigure(cfg *config.Config) error {
 }
 
 func (fs *zipVFS) reconfigureTransport(cfg *config.Config) error {
-	fsTransport, err := httpfs.NewFileSystemPath(cfg.Zip.AllowedPaths)
+	fsTransport, err := httpfs.NewFileSystemPath(cfg.Zip.AllowedPaths, cfg.Zip.ChrootPath)
 	if err != nil {
 		return err
 	}
diff --git a/test/acceptance/zip_test.go b/test/acceptance/zip_test.go
index a7e82d27..91d5df99 100644
--- a/test/acceptance/zip_test.go
+++ b/test/acceptance/zip_test.go
@@ -107,8 +107,6 @@ func TestZipServing(t *testing.T) {
 }
 
 func TestZipServingFromDisk(t *testing.T) {
-	skipUnlessEnabled(t, "not-inplace-chroot")
-
 	chdir := false
 	defer testhelpers.ChdirInPath(t, "../../shared/pages", &chdir)()