From 34e1518038a8164c090ccfbfc30ebc7850e62cf0 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Thu, 8 Apr 2021 16:06:49 +1000 Subject: [PATCH 1/8] Allow serving zip from disk in chroot 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 after we roll-out zip architecture completely https://gitlab.com/gitlab-org/gitlab-pages/-/issues/561 Changelog: fixed --- internal/config/config.go | 12 ++++++++++++ internal/httpfs/http_fs.go | 7 +++++++ internal/vfs/zip/vfs.go | 20 +++++++++++++++++++- test/acceptance/zip_test.go | 22 ++++++++++++++++------ 4 files changed, 54 insertions(+), 7 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 0842a1951..6de66bcff 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 after we roll-out zip architecture completely https://gitlab.com/gitlab-org/gitlab-pages/-/issues/561 + ChrootPath string } func gitlabServerFromFlags() string { @@ -329,6 +333,14 @@ 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 after we roll-out zip architecture completely https://gitlab.com/gitlab-org/gitlab-pages/-/issues/561 + if config.Daemon.InplaceChroot { + config.Zip.ChrootPath = *pagesRoot + config.Zip.AllowedPaths = append(config.Zip.AllowedPaths, "/") + } + validateConfig(config) return config diff --git a/internal/httpfs/http_fs.go b/internal/httpfs/http_fs.go index cd2edb837..1ce900579 100644 --- a/internal/httpfs/http_fs.go +++ b/internal/httpfs/http_fs.go @@ -55,6 +55,13 @@ func (p *fileSystemPaths) Open(name string) (http.File, error) { return nil, err } for _, allowedPath := range p.allowedPaths { + // 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 after we roll-out zip architecture completely https://gitlab.com/gitlab-org/gitlab-pages/-/issues/561 + if allowedPath == "/" { + return os.Open(absPath) + } + if strings.HasPrefix(absPath, allowedPath+"/") { return os.Open(absPath) } diff --git a/internal/vfs/zip/vfs.go b/internal/vfs/zip/vfs.go index b69522f99..fc83fca27 100644 --- a/internal/vfs/zip/vfs.go +++ b/internal/vfs/zip/vfs.go @@ -5,6 +5,7 @@ import ( "errors" "net/http" "net/url" + "strings" "sync" "time" @@ -50,6 +51,11 @@ type zipVFS struct { archiveCount int64 httpClient *http.Client + + // 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 after we roll-out zip architecture completely https://gitlab.com/gitlab-org/gitlab-pages/-/issues/561 + chrootPath string } // New creates a zipVFS instance that can be used by a serving request @@ -93,6 +99,7 @@ func (fs *zipVFS) Reconfigure(cfg *config.Config) error { fs.cacheExpirationInterval = cfg.Zip.ExpirationInterval fs.cacheRefreshInterval = cfg.Zip.RefreshInterval fs.cacheCleanupInterval = cfg.Zip.CleanupInterval + fs.chrootPath = cfg.Zip.ChrootPath if err := fs.reconfigureTransport(cfg); err != nil { return err @@ -239,10 +246,21 @@ func (fs *zipVFS) findOrOpenArchive(ctx context.Context, key, path string) (*zip return nil, err } - err = zipArchive.openArchive(ctx, path) + err = zipArchive.openArchive(ctx, fs.removeChrootPath(path)) if err != nil { return nil, err } return zipArchive, nil } + +// 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 after we roll-out zip architecture completely https://gitlab.com/gitlab-org/gitlab-pages/-/issues/561 +func (fs *zipVFS) removeChrootPath(path string) string { + if fs.chrootPath == "" || strings.HasPrefix(path, "http") { + return path + } + + return strings.ReplaceAll(path, fs.chrootPath, "") +} diff --git a/test/acceptance/zip_test.go b/test/acceptance/zip_test.go index a7e82d27a..008ccc4b6 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)() @@ -185,10 +183,22 @@ func TestZipServingFromDisk(t *testing.T) { expectedContent: "The page you're looking for could not be found", }, "file_not_allowed_in_path": { - host: "zip-not-allowed-path.gitlab.io", - urlSuffix: "/", - expectedStatusCode: http.StatusInternalServerError, - expectedContent: "Whoops, something went wrong on our end.", + host: "zip-not-allowed-path.gitlab.io", + urlSuffix: "/", + expectedStatusCode: func() int { + if os.Getenv("TEST_DAEMONIZE") == "inplace" { + return http.StatusNotFound + } + + return http.StatusInternalServerError + }(), + expectedContent: func() string { + if os.Getenv("TEST_DAEMONIZE") == "inplace" { + return "The page you're looking for could not be found" + } + + return "Whoops, something went wrong on our end." + }(), }, } -- GitLab From c10549ed34d7a00ea087aa2076e47620fa980397 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Tue, 13 Apr 2021 09:21:44 +1000 Subject: [PATCH 2/8] Remove root from AllowedPaths --- internal/config/config.go | 1 - internal/httpfs/http_fs.go | 7 ------- internal/vfs/zip/vfs.go | 2 +- 3 files changed, 1 insertion(+), 9 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 6de66bcff..20a9ad8bc 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -338,7 +338,6 @@ func loadConfig() *Config { // To be removed after we roll-out zip architecture completely https://gitlab.com/gitlab-org/gitlab-pages/-/issues/561 if config.Daemon.InplaceChroot { config.Zip.ChrootPath = *pagesRoot - config.Zip.AllowedPaths = append(config.Zip.AllowedPaths, "/") } validateConfig(config) diff --git a/internal/httpfs/http_fs.go b/internal/httpfs/http_fs.go index 1ce900579..cd2edb837 100644 --- a/internal/httpfs/http_fs.go +++ b/internal/httpfs/http_fs.go @@ -55,13 +55,6 @@ func (p *fileSystemPaths) Open(name string) (http.File, error) { return nil, err } for _, allowedPath := range p.allowedPaths { - // 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 after we roll-out zip architecture completely https://gitlab.com/gitlab-org/gitlab-pages/-/issues/561 - if allowedPath == "/" { - return os.Open(absPath) - } - if strings.HasPrefix(absPath, allowedPath+"/") { return os.Open(absPath) } diff --git a/internal/vfs/zip/vfs.go b/internal/vfs/zip/vfs.go index fc83fca27..e8cae63c1 100644 --- a/internal/vfs/zip/vfs.go +++ b/internal/vfs/zip/vfs.go @@ -262,5 +262,5 @@ func (fs *zipVFS) removeChrootPath(path string) string { return path } - return strings.ReplaceAll(path, fs.chrootPath, "") + return strings.Replace(path, fs.chrootPath, "", 1) } -- GitLab From 1c91673a288e73d4127e441ecf6ccbab544f62c7 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Tue, 13 Apr 2021 09:57:43 +1000 Subject: [PATCH 3/8] Handle chrootPath in http_fs --- internal/httpfs/http_fs.go | 42 +++++++++++++++++++++++++++++++-- internal/httpfs/http_fs_test.go | 19 +++++++++++++-- internal/vfs/zip/vfs.go | 16 ++----------- test/acceptance/zip_test.go | 20 ++++------------ 4 files changed, 63 insertions(+), 34 deletions(-) diff --git a/internal/httpfs/http_fs.go b/internal/httpfs/http_fs.go index cd2edb837..1aa85f56a 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,14 +25,27 @@ var ( // fileSystemPaths implements the http.FileSystem interface type fileSystemPaths struct { 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 after we roll-out zip architecture completely 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) { +func NewFileSystemPath(allowedPaths []string, chrootPath string) (http.FileSystem, error) { for k, path := range allowedPaths { var err error + if chrootPath != "" { + if !strings.HasPrefix(path, chrootPath) { + return nil, fmt.Errorf("allowed path %q is not in chroot path %q", path, chrootPath) + } + + // strip chrootPath from each allowedPath + path = path[len(chrootPath):] + } + allowedPaths[k], err = filepath.Abs(path) 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,11 +65,34 @@ func (p *fileSystemPaths) Open(name string) (http.File, error) { return nil, errInvalidChar } - absPath, err := filepath.Abs(filepath.FromSlash(path.Clean("/" + name))) + absPath := 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: To be removed after we roll-out zip architecture completely https://gitlab.com/gitlab-org/gitlab-pages/-/issues/561 + if p.chrootPath != "" { + if !strings.HasPrefix(absPath, p.chrootPath) { + log.WithError(os.ErrPermission).Errorf( + "requested filepath %q not in chroot path: %q", absPath, p.chrootPath) + + return nil, os.ErrPermission + } + + // since we run in a chroot path, strip it + absPath = absPath[len(p.chrootPath):] + } + + absPath, err := filepath.Abs(absPath) if err != nil { return nil, err } for _, allowedPath := range p.allowedPaths { + // TODO: To be removed after we roll-out zip architecture completely https://gitlab.com/gitlab-org/gitlab-pages/-/issues/561 + // return early if in chroot and we have stripped p.chrootPath from absPath && allowedPath + if p.chrootPath != "" && allowedPath == "/" { + return os.Open(absPath) + } + if strings.HasPrefix(absPath, allowedPath+"/") { return os.Open(absPath) } diff --git a/internal/httpfs/http_fs_test.go b/internal/httpfs/http_fs_test.go index 3d1092b6c..1f7790001 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_allowed_when_not_in_real_chroot": { + allowedPaths: []string{wd + "/testdata"}, + fileName: wd + "/testdata/file1.txt", + expectedErrMsg: os.ErrPermission.Error(), + 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_forbidden_when_not_in_real_chroot": { + path: wd + "/testdata", + fileName: "file1.txt", + chrootPath: wd + "/testdata", + expectedStatusCode: http.StatusForbidden, + expectedContent: "403 Forbidden\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 e8cae63c1..1e71e07e7 100644 --- a/internal/vfs/zip/vfs.go +++ b/internal/vfs/zip/vfs.go @@ -5,7 +5,6 @@ import ( "errors" "net/http" "net/url" - "strings" "sync" "time" @@ -111,7 +110,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, fs.chrootPath) if err != nil { return err } @@ -246,21 +245,10 @@ func (fs *zipVFS) findOrOpenArchive(ctx context.Context, key, path string) (*zip return nil, err } - err = zipArchive.openArchive(ctx, fs.removeChrootPath(path)) + err = zipArchive.openArchive(ctx, path) if err != nil { return nil, err } return zipArchive, nil } - -// 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 after we roll-out zip architecture completely https://gitlab.com/gitlab-org/gitlab-pages/-/issues/561 -func (fs *zipVFS) removeChrootPath(path string) string { - if fs.chrootPath == "" || strings.HasPrefix(path, "http") { - return path - } - - return strings.Replace(path, fs.chrootPath, "", 1) -} diff --git a/test/acceptance/zip_test.go b/test/acceptance/zip_test.go index 008ccc4b6..91d5df998 100644 --- a/test/acceptance/zip_test.go +++ b/test/acceptance/zip_test.go @@ -183,22 +183,10 @@ func TestZipServingFromDisk(t *testing.T) { expectedContent: "The page you're looking for could not be found", }, "file_not_allowed_in_path": { - host: "zip-not-allowed-path.gitlab.io", - urlSuffix: "/", - expectedStatusCode: func() int { - if os.Getenv("TEST_DAEMONIZE") == "inplace" { - return http.StatusNotFound - } - - return http.StatusInternalServerError - }(), - expectedContent: func() string { - if os.Getenv("TEST_DAEMONIZE") == "inplace" { - return "The page you're looking for could not be found" - } - - return "Whoops, something went wrong on our end." - }(), + host: "zip-not-allowed-path.gitlab.io", + urlSuffix: "/", + expectedStatusCode: http.StatusInternalServerError, + expectedContent: "Whoops, something went wrong on our end.", }, } -- GitLab From 394eb6705edc7216f5565fec06df24b5b1dcbf74 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Tue, 13 Apr 2021 11:19:49 +1000 Subject: [PATCH 4/8] Use ChrootPath from config --- internal/vfs/zip/vfs.go | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/internal/vfs/zip/vfs.go b/internal/vfs/zip/vfs.go index 1e71e07e7..9c09384fa 100644 --- a/internal/vfs/zip/vfs.go +++ b/internal/vfs/zip/vfs.go @@ -50,11 +50,6 @@ type zipVFS struct { archiveCount int64 httpClient *http.Client - - // 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 after we roll-out zip architecture completely https://gitlab.com/gitlab-org/gitlab-pages/-/issues/561 - chrootPath string } // New creates a zipVFS instance that can be used by a serving request @@ -98,7 +93,6 @@ func (fs *zipVFS) Reconfigure(cfg *config.Config) error { fs.cacheExpirationInterval = cfg.Zip.ExpirationInterval fs.cacheRefreshInterval = cfg.Zip.RefreshInterval fs.cacheCleanupInterval = cfg.Zip.CleanupInterval - fs.chrootPath = cfg.Zip.ChrootPath if err := fs.reconfigureTransport(cfg); err != nil { return err @@ -110,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, fs.chrootPath) + fsTransport, err := httpfs.NewFileSystemPath(cfg.Zip.AllowedPaths, cfg.Zip.ChrootPath) if err != nil { return err } -- GitLab From 868dab812f2ef6e23edbfa2dc86862ccc909d893 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Wed, 14 Apr 2021 10:46:23 +1000 Subject: [PATCH 5/8] Strip chrootPath from allowedPaths --- internal/httpfs/http_fs.go | 79 ++++++++++++++++++++++---------------- 1 file changed, 46 insertions(+), 33 deletions(-) diff --git a/internal/httpfs/http_fs.go b/internal/httpfs/http_fs.go index 1aa85f56a..1e9dff1cc 100644 --- a/internal/httpfs/http_fs.go +++ b/internal/httpfs/http_fs.go @@ -25,28 +25,28 @@ var ( // fileSystemPaths implements the http.FileSystem interface type fileSystemPaths struct { 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 after we roll-out zip architecture completely https://gitlab.com/gitlab-org/gitlab-pages/-/issues/561 + // 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 +// 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, path := range allowedPaths { - var err error - - if chrootPath != "" { - if !strings.HasPrefix(path, chrootPath) { - return nil, fmt.Errorf("allowed path %q is not in chroot path %q", path, chrootPath) - } - - // strip chrootPath from each allowedPath - path = path[len(chrootPath):] + 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 } @@ -65,34 +65,24 @@ func (p *fileSystemPaths) Open(name string) (http.File, error) { return nil, errInvalidChar } - absPath := 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: To be removed after we roll-out zip architecture completely https://gitlab.com/gitlab-org/gitlab-pages/-/issues/561 - if p.chrootPath != "" { - if !strings.HasPrefix(absPath, p.chrootPath) { - log.WithError(os.ErrPermission).Errorf( - "requested filepath %q not in chroot path: %q", absPath, p.chrootPath) - - return nil, os.ErrPermission - } + // 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() - // since we run in a chroot path, strip it - absPath = absPath[len(p.chrootPath):] + return nil, os.ErrPermission } - absPath, err := filepath.Abs(absPath) + absPath, err := filepath.Abs(strippedPath) if err != nil { return nil, err } for _, allowedPath := range p.allowedPaths { - // TODO: To be removed after we roll-out zip architecture completely https://gitlab.com/gitlab-org/gitlab-pages/-/issues/561 - // return early if in chroot and we have stripped p.chrootPath from absPath && allowedPath - if p.chrootPath != "" && allowedPath == "/" { - return os.Open(absPath) - } - if strings.HasPrefix(absPath, allowedPath+"/") { return os.Open(absPath) } @@ -105,3 +95,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 +} -- GitLab From d7385218659e2eb3d3948fafab5c46782809bb17 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Wed, 14 Apr 2021 12:00:34 +1000 Subject: [PATCH 6/8] Return early if allowedPath is the root dir --- internal/httpfs/http_fs.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/internal/httpfs/http_fs.go b/internal/httpfs/http_fs.go index 1e9dff1cc..175183015 100644 --- a/internal/httpfs/http_fs.go +++ b/internal/httpfs/http_fs.go @@ -83,6 +83,13 @@ func (p *fileSystemPaths) Open(name string) (http.File, error) { return nil, err } for _, allowedPath := range p.allowedPaths { + // filepath.Abs(allowedPath) in NewFileSystemPath resolves an allowedPath to be `/` when + // chrootPath == allowedPath, so we need to return early if in chroot + //and we have stripped p.chrootPath from absPath && allowedPath + if p.chrootPath != "" && allowedPath == "/" { + return os.Open(absPath) + } + if strings.HasPrefix(absPath, allowedPath+"/") { return os.Open(absPath) } -- GitLab From a54b61e68c288d6eca5ce6e48cfc7658c817233b Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Wed, 14 Apr 2021 14:12:09 +1000 Subject: [PATCH 7/8] Update unit tests --- internal/httpfs/http_fs.go | 12 +++--------- internal/httpfs/http_fs_test.go | 10 +++++----- 2 files changed, 8 insertions(+), 14 deletions(-) diff --git a/internal/httpfs/http_fs.go b/internal/httpfs/http_fs.go index 175183015..3578352fa 100644 --- a/internal/httpfs/http_fs.go +++ b/internal/httpfs/http_fs.go @@ -73,7 +73,7 @@ func (p *fileSystemPaths) Open(name string) (http.File, error) { // https://gitlab.com/gitlab-org/gitlab-pages/-/issues/561 strippedPath, err := stripChrootPath(cleanedPath, p.chrootPath) if err != nil { - log.WithError(err).Error() + log.WithError(err).Error(os.ErrPermission) return nil, os.ErrPermission } @@ -83,14 +83,8 @@ func (p *fileSystemPaths) Open(name string) (http.File, error) { return nil, err } for _, allowedPath := range p.allowedPaths { - // filepath.Abs(allowedPath) in NewFileSystemPath resolves an allowedPath to be `/` when - // chrootPath == allowedPath, so we need to return early if in chroot - //and we have stripped p.chrootPath from absPath && allowedPath - if p.chrootPath != "" && allowedPath == "/" { - return os.Open(absPath) - } - - 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) } } diff --git a/internal/httpfs/http_fs_test.go b/internal/httpfs/http_fs_test.go index 1f7790001..7885dbf20 100644 --- a/internal/httpfs/http_fs_test.go +++ b/internal/httpfs/http_fs_test.go @@ -59,10 +59,10 @@ func TestFSOpen(t *testing.T) { fileName: wd + "/../httpfs/testdata/file1.txt", expectedErrMsg: os.ErrPermission.Error(), }, - "chroot_path_not_allowed_when_not_in_real_chroot": { + "chroot_path_not_found_when_not_in_real_chroot": { allowedPaths: []string{wd + "/testdata"}, fileName: wd + "/testdata/file1.txt", - expectedErrMsg: os.ErrPermission.Error(), + expectedErrMsg: "no such file or directory", chrootPath: wd + "/testdata", }, } @@ -144,12 +144,12 @@ func TestFileSystemPathCanServeHTTP(t *testing.T) { expectedStatusCode: http.StatusForbidden, expectedContent: "403 Forbidden\n", }, - "chroot_path_fails_in_unit_test_forbidden_when_not_in_real_chroot": { + "chroot_path_fails_in_unit_test_not_found_when_not_in_real_chroot": { path: wd + "/testdata", fileName: "file1.txt", chrootPath: wd + "/testdata", - expectedStatusCode: http.StatusForbidden, - expectedContent: "403 Forbidden\n", + expectedStatusCode: http.StatusNotFound, + expectedContent: "404 page not found\n", }, } -- GitLab From d52a7013df920b97937e48d619f4ad47d07fb10b Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Thu, 15 Apr 2021 11:12:42 +1000 Subject: [PATCH 8/8] Update comment about chroot removal --- internal/config/config.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 20a9ad8bc..528b586ab 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -148,7 +148,7 @@ type ZipServing struct { 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 after we roll-out zip architecture completely https://gitlab.com/gitlab-org/gitlab-pages/-/issues/561 + // To be removed along with chroot support https://gitlab.com/gitlab-org/gitlab-pages/-/issues/561 ChrootPath string } @@ -335,7 +335,7 @@ func loadConfig() *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 after we roll-out zip architecture completely https://gitlab.com/gitlab-org/gitlab-pages/-/issues/561 + // To be removed along with chroot support https://gitlab.com/gitlab-org/gitlab-pages/-/issues/561 if config.Daemon.InplaceChroot { config.Zip.ChrootPath = *pagesRoot } -- GitLab