diff options
-rw-r--r-- | .gitlab/ci/test.yml | 25 | ||||
-rw-r--r-- | README.md | 5 | ||||
-rw-r--r-- | daemon.go | 61 | ||||
-rw-r--r-- | internal/config/config.go | 3 | ||||
-rw-r--r-- | internal/config/flags.go | 1 | ||||
-rw-r--r-- | internal/httpfs/http_fs_test.go | 13 | ||||
-rw-r--r-- | internal/source/domains.go | 2 | ||||
-rw-r--r-- | internal/source/gitlab/cache/retriever.go | 6 | ||||
-rw-r--r-- | internal/vfs/zip/vfs.go | 7 | ||||
-rw-r--r-- | main.go | 6 | ||||
-rw-r--r-- | test/acceptance/artifacts_test.go | 4 | ||||
-rw-r--r-- | test/acceptance/auth_test.go | 74 | ||||
-rw-r--r-- | test/acceptance/config_test.go | 29 | ||||
-rw-r--r-- | test/acceptance/helpers_test.go | 16 |
14 files changed, 172 insertions, 80 deletions
diff --git a/.gitlab/ci/test.yml b/.gitlab/ci/test.yml index f0e484b9..7377d993 100644 --- a/.gitlab/ci/test.yml +++ b/.gitlab/ci/test.yml @@ -15,11 +15,16 @@ - make test ARGS='-short' - make junit-report -.tests-acceptance-deamon: +.tests-acceptance-tmpdir: extends: .tests-common script: - echo "Running just the acceptance tests daemonized (tmpdir)...." - TEST_DAEMONIZE=tmpdir make acceptance + - make junit-report + +.tests-acceptance-inplace: + extends: .tests-common + script: - echo "Running just the acceptance tests daemonized (inplace)...." - TEST_DAEMONIZE=inplace make acceptance - make junit-report @@ -29,7 +34,11 @@ test:1.14: image: golang:1.14 test-acceptance:1.14: - extends: .tests-acceptance-deamon + extends: .tests-acceptance-tmpdir + image: golang:1.14 + +test-acceptance-inplace:1.14: + extends: .tests-acceptance-inplace image: golang:1.14 test:1.15: @@ -37,7 +46,11 @@ test:1.15: image: golang:1.15 test-acceptance:1.15: - extends: .tests-acceptance-deamon + extends: .tests-acceptance-tmpdir + image: golang:1.15 + +test-acceptance-inplace:1.15: + extends: .tests-acceptance-inplace image: golang:1.15 test:1.16: @@ -45,7 +58,11 @@ test:1.16: image: golang:1.16 test-acceptance:1.16: - extends: .tests-acceptance-deamon + extends: .tests-acceptance-tmpdir + image: golang:1.16 + +test-acceptance-inplace:1.16: + extends: .tests-acceptance-inplace image: golang:1.16 race: @@ -83,6 +83,11 @@ See [doc/development.md](doc/development.md) ### Run daemon **in secure mode** +**Update**: + +Starting from GitLab 14.1 the +[jailing/chroot mechanism is disabled by default](https://docs.gitlab.com/ee/administration/pages/#jailing-mechanism-disabled-by-default-for-api-based-configuration). + When compiled with `CGO_ENABLED=0` (which is the default), `gitlab-pages` is a static binary and so can be run in chroot with dropped privileges. @@ -13,6 +13,7 @@ import ( "syscall" "github.com/sirupsen/logrus" + "gitlab.com/gitlab-org/labkit/log" "gitlab.com/gitlab-org/gitlab-pages/internal/config" "gitlab.com/gitlab-org/gitlab-pages/internal/jail" @@ -277,6 +278,7 @@ func daemonize(config *config.Config) error { uid := config.Daemon.UID gid := config.Daemon.GID inPlace := config.Daemon.InplaceChroot + enableJail := config.Daemon.EnableJail pagesRoot := config.General.RootDir // Ensure pagesRoot is an absolute path. This will produce a different path @@ -291,10 +293,11 @@ func daemonize(config *config.Config) error { } logrus.WithFields(logrus.Fields{ - "uid": uid, - "gid": gid, - "in-place": inPlace, - "pages-root": pagesRoot, + "uid": uid, + "gid": gid, + "in-place": inPlace, + "enable-jail": enableJail, + "pages-root": pagesRoot, }).Info("running the daemon as unprivileged user") cmd, err := daemonReexec(uid, gid, daemonRunProgram) @@ -303,28 +306,24 @@ func daemonize(config *config.Config) error { } defer killProcess(cmd) - // Run daemon in chroot environment - var wrapper *jail.Jail - if inPlace { - wrapper, err = chrootDaemon(cmd) - } else { - wrapper, err = jailDaemon(pagesRoot, cmd) - } - if err != nil { - logrus.WithError(err).Print("chroot failed") - return err - } - defer wrapper.Dispose() + if enableJail { + wrapper, err := createChroot(cmd, pagesRoot, inPlace) + if err != nil { + log.WithError(err).Error("create chroot failed") + return err + } + defer wrapper.Dispose() - // Unshare mount namespace - // 1. If this fails, in a worst case changes to mounts will propagate to other processes - // 2. Ensures that jail mount is not propagated to the parent mount namespace - // to avoid populating `tmp` directory with old mounts - _ = wrapper.Unshare() + // Unshare mount namespace + // 1. If this fails, in a worst case changes to mounts will propagate to other processes + // 2. Ensures that jail mount is not propagated to the parent mount namespace + // to avoid populating `tmp` directory with old mounts + _ = wrapper.Unshare() - if err := wrapper.Build(); err != nil { - logrus.WithError(err).Print("chroot build failed") - return err + if err := wrapper.Build(); err != nil { + log.WithError(err).Error("chroot build failed") + return err + } } // Create a pipe to pass the configuration @@ -356,6 +355,20 @@ func daemonize(config *config.Config) error { return cmd.Wait() } +func createChroot(cmd *exec.Cmd, pagesRoot string, inPlace bool) (*jail.Jail, error) { + // Run daemon in chroot environment + var wrapper *jail.Jail + var err error + + if inPlace { + wrapper, err = chrootDaemon(cmd) + } else { + wrapper, err = jailDaemon(pagesRoot, cmd) + } + + return wrapper, err +} + func updateFds(config *config.Config, cmd *exec.Cmd) { for _, fds := range [][]uintptr{ config.Listeners.HTTP, diff --git a/internal/config/config.go b/internal/config/config.go index 196b9685..b9deef3b 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -86,6 +86,7 @@ type Daemon struct { UID uint GID uint InplaceChroot bool + EnableJail bool } // Cache configuration for GitLab API @@ -226,6 +227,7 @@ func loadConfig() (*Config, error) { UID: *daemonUID, GID: *daemonGID, InplaceChroot: *daemonInplaceChroot, + EnableJail: *daemonEnableJail || *domainConfigSource == "disk", }, Log: Log{ Format: *logFormat, @@ -303,6 +305,7 @@ func LogConfig(config *Config) { "daemon-gid": *daemonGID, "daemon-uid": *daemonUID, "daemon-inplace-chroot": *daemonInplaceChroot, + "daemon-enable-jail": *daemonEnableJail, "default-config-filename": flag.DefaultConfigFlagname, "disable-cross-origin-requests": *disableCrossOriginRequests, "domain": config.General.Domain, diff --git a/internal/config/flags.go b/internal/config/flags.go index 5ebffcb9..250d3b60 100644 --- a/internal/config/flags.go +++ b/internal/config/flags.go @@ -24,6 +24,7 @@ var ( daemonUID = flag.Uint("daemon-uid", 0, "Drop privileges to this user") daemonGID = flag.Uint("daemon-gid", 0, "Drop privileges to this group") daemonInplaceChroot = flag.Bool("daemon-inplace-chroot", false, "Fall back to a non-bind-mount chroot of -pages-root when daemonizing") + daemonEnableJail = flag.Bool("daemon-enable-jail", false, "Enable legacy jailing mechanism (disabled by default for API-based configuration)") propagateCorrelationID = flag.Bool("propagate-correlation-id", false, "Reuse existing Correlation-ID from the incoming request header `X-Request-ID` if present") logFormat = flag.String("log-format", "json", "The log output format: 'text' or 'json'") logVerbose = flag.Bool("log-verbose", false, "Verbose logging") diff --git a/internal/httpfs/http_fs_test.go b/internal/httpfs/http_fs_test.go index 7885dbf2..f47e17ff 100644 --- a/internal/httpfs/http_fs_test.go +++ b/internal/httpfs/http_fs_test.go @@ -65,6 +65,12 @@ func TestFSOpen(t *testing.T) { expectedErrMsg: "no such file or directory", chrootPath: wd + "/testdata", }, + "chroot_path_empty_finds_file": { + allowedPaths: []string{wd + "/testdata"}, + fileName: wd + "/testdata/file1.txt", + expectedContent: "file1.txt\n", + chrootPath: "", + }, } for name, test := range tests { @@ -151,6 +157,13 @@ func TestFileSystemPathCanServeHTTP(t *testing.T) { expectedStatusCode: http.StatusNotFound, expectedContent: "404 page not found\n", }, + "chroot_path_empty_in_unit_test_file_found": { + path: wd + "/testdata", + fileName: "file1.txt", + chrootPath: "", + expectedStatusCode: http.StatusOK, + expectedContent: "file1.txt\n", + }, } for name, test := range tests { diff --git a/internal/source/domains.go b/internal/source/domains.go index 4c9028d3..8b7e2be9 100644 --- a/internal/source/domains.go +++ b/internal/source/domains.go @@ -16,7 +16,7 @@ type configSource int const ( sourceGitlab configSource = iota - // Disk source is deprecated and support will be removed in 14.0 + // Disk source is deprecated and support will be removed in 14.3 // https://gitlab.com/gitlab-org/gitlab-pages/-/issues/382 sourceDisk sourceAuto diff --git a/internal/source/gitlab/cache/retriever.go b/internal/source/gitlab/cache/retriever.go index 6c1d38d2..2f6c1f07 100644 --- a/internal/source/gitlab/cache/retriever.go +++ b/internal/source/gitlab/cache/retriever.go @@ -46,10 +46,8 @@ func (r *Retriever) Retrieve(originalCtx context.Context, domain string) (lookup select { case <-ctx.Done(): - logMsg = "retrieval context done" - - lookup = api.Lookup{Error: errors.New("retrieval context done")} + lookup = api.Lookup{Error: errors.New(logMsg)} case lookup = <-r.resolveWithBackoff(ctx, domain): logMsg = "retrieval response sent" } @@ -60,7 +58,7 @@ func (r *Retriever) Retrieve(originalCtx context.Context, domain string) (lookup "lookup_name": lookup.Name, "lookup_paths": lookup.Domain, "lookup_error": lookup.Error, - }).Debug(logMsg) + }).WithError(ctx.Err()).Debug(logMsg) return lookup } diff --git a/internal/vfs/zip/vfs.go b/internal/vfs/zip/vfs.go index 9c09384f..dc6eb5c4 100644 --- a/internal/vfs/zip/vfs.go +++ b/internal/vfs/zip/vfs.go @@ -104,7 +104,12 @@ func (fs *zipVFS) Reconfigure(cfg *config.Config) error { } func (fs *zipVFS) reconfigureTransport(cfg *config.Config) error { - fsTransport, err := httpfs.NewFileSystemPath(cfg.Zip.AllowedPaths, cfg.Zip.ChrootPath) + chrootPath := "" + if cfg.Daemon.EnableJail { + chrootPath = cfg.Zip.ChrootPath + } + + fsTransport, err := httpfs.NewFileSystemPath(cfg.Zip.AllowedPaths, chrootPath) if err != nil { return err } @@ -5,6 +5,7 @@ import ( "io" "math/rand" "os" + "strings" "time" "github.com/sirupsen/logrus" @@ -79,6 +80,11 @@ func appMain() { if config.Daemon.UID != 0 || config.Daemon.GID != 0 { if err := daemonize(config); err != nil { + if strings.Contains(err.Error(), "signal:") { + log.WithField("signal", err.Error()).Info("daemon received signal") + return + } + errortracking.Capture(err) fatal(err, "could not create pages daemon") } diff --git a/test/acceptance/artifacts_test.go b/test/acceptance/artifacts_test.go index 2f578a73..398b62a6 100644 --- a/test/acceptance/artifacts_test.go +++ b/test/acceptance/artifacts_test.go @@ -15,8 +15,6 @@ import ( ) func TestArtifactProxyRequest(t *testing.T) { - skipUnlessEnabled(t, "not-inplace-chroot") - transport := (TestHTTPSClient.Transport).(*http.Transport).Clone() transport.ResponseHeaderTimeout = 5 * time.Second @@ -161,8 +159,6 @@ func TestArtifactProxyRequest(t *testing.T) { } func TestPrivateArtifactProxyRequest(t *testing.T) { - skipUnlessEnabled(t, "not-inplace-chroot") - setupTransport(t) testServer := makeGitLabPagesAccessStub(t) diff --git a/test/acceptance/auth_test.go b/test/acceptance/auth_test.go index 980fe377..331bf7d6 100644 --- a/test/acceptance/auth_test.go +++ b/test/acceptance/auth_test.go @@ -483,8 +483,11 @@ func testAccessControl(t *testing.T, runPages runPagesFunc) { keyFile, certFile := CreateHTTPSFixtureFiles(t) cert, err := tls.LoadX509KeyPair(certFile, keyFile) require.NoError(t, err) - defer os.Remove(keyFile) - defer os.Remove(certFile) + + t.Cleanup(func() { + os.Remove(keyFile) + os.Remove(certFile) + }) testServer := makeGitLabPagesAccessStub(t) testServer.TLS = &tls.Config{Certificates: []tls.Certificate{cert}} @@ -572,67 +575,64 @@ func testAccessControl(t *testing.T, runPages runPagesFunc) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - teardown := runPages(t, *pagesBinary, supportedListeners(), "", certFile, testServer.URL) + teardown := runPages(t, *pagesBinary, []ListenSpec{httpsListener}, "", certFile, testServer.URL) defer teardown() - rsp, err := GetRedirectPage(t, httpsListener, tt.host, tt.path) + rsp1, err1 := GetRedirectPage(t, httpsListener, tt.host, tt.path) + require.NoError(t, err1) + defer rsp1.Body.Close() - require.NoError(t, err) - defer rsp.Body.Close() - - require.Equal(t, http.StatusFound, rsp.StatusCode) - cookie := rsp.Header.Get("Set-Cookie") + require.Equal(t, http.StatusFound, rsp1.StatusCode) + cookie := rsp1.Header.Get("Set-Cookie") // Redirects to the projects under gitlab pages domain for authentication flow - url, err := url.Parse(rsp.Header.Get("Location")) + loc1, err := url.Parse(rsp1.Header.Get("Location")) require.NoError(t, err) - require.Equal(t, "projects.gitlab-example.com", url.Host) - require.Equal(t, "/auth", url.Path) - state := url.Query().Get("state") + require.Equal(t, "projects.gitlab-example.com", loc1.Host) + require.Equal(t, "/auth", loc1.Path) + state := loc1.Query().Get("state") - rsp, err = GetRedirectPage(t, httpsListener, url.Host, url.Path+"?"+url.RawQuery) + rsp2, err2 := GetRedirectPage(t, httpsListener, loc1.Host, loc1.Path+"?"+loc1.RawQuery) + require.NoError(t, err2) + defer rsp2.Body.Close() - require.NoError(t, err) - defer rsp.Body.Close() - - require.Equal(t, http.StatusFound, rsp.StatusCode) - pagesDomainCookie := rsp.Header.Get("Set-Cookie") + require.Equal(t, http.StatusFound, rsp2.StatusCode) + pagesDomainCookie := rsp2.Header.Get("Set-Cookie") // Go to auth page with correct state will cause fetching the token - authrsp, err := GetRedirectPageWithCookie(t, httpsListener, "projects.gitlab-example.com", "/auth?code=1&state="+ + authrsp1, err := GetRedirectPageWithCookie(t, httpsListener, "projects.gitlab-example.com", "/auth?code=1&state="+ state, pagesDomainCookie) - require.NoError(t, err) - defer authrsp.Body.Close() + defer authrsp1.Body.Close() // Will redirect auth callback to correct host - url, err = url.Parse(authrsp.Header.Get("Location")) + authLoc, err := url.Parse(authrsp1.Header.Get("Location")) require.NoError(t, err) - require.Equal(t, tt.host, url.Host) - require.Equal(t, "/auth", url.Path) + require.Equal(t, tt.host, authLoc.Host) + require.Equal(t, "/auth", authLoc.Path) // Request auth callback in project domain - authrsp, err = GetRedirectPageWithCookie(t, httpsListener, url.Host, url.Path+"?"+url.RawQuery, cookie) + authrsp2, err := GetRedirectPageWithCookie(t, httpsListener, authLoc.Host, authLoc.Path+"?"+authLoc.RawQuery, cookie) require.NoError(t, err) // server returns the ticket, user will be redirected to the project page - require.Equal(t, http.StatusFound, authrsp.StatusCode) - cookie = authrsp.Header.Get("Set-Cookie") - rsp, err = GetRedirectPageWithCookie(t, httpsListener, tt.host, tt.path, cookie) + require.Equal(t, http.StatusFound, authrsp2.StatusCode) + cookie = authrsp2.Header.Get("Set-Cookie") - require.NoError(t, err) - defer rsp.Body.Close() + rsp3, err3 := GetRedirectPageWithCookie(t, httpsListener, tt.host, tt.path, cookie) + require.NoError(t, err3) + defer rsp3.Body.Close() - require.Equal(t, tt.status, rsp.StatusCode) - require.Equal(t, "", rsp.Header.Get("Cache-Control")) + require.Equal(t, tt.status, rsp3.StatusCode) + require.Equal(t, "", rsp3.Header.Get("Cache-Control")) if tt.redirectBack { - url, err = url.Parse(rsp.Header.Get("Location")) + loc3, err := url.Parse(rsp3.Header.Get("Location")) require.NoError(t, err) - require.Equal(t, "https", url.Scheme) - require.Equal(t, tt.host, url.Host) - require.Equal(t, tt.path, url.Path) + require.Equal(t, "https", loc3.Scheme) + require.Equal(t, tt.host, loc3.Host) + require.Equal(t, tt.path, loc3.Path) } }) } diff --git a/test/acceptance/config_test.go b/test/acceptance/config_test.go index aa568969..8e686277 100644 --- a/test/acceptance/config_test.go +++ b/test/acceptance/config_test.go @@ -5,6 +5,7 @@ import ( "net" "net/http" "testing" + "time" "github.com/stretchr/testify/require" ) @@ -21,7 +22,7 @@ func TestEnvironmentVariablesConfig(t *testing.T) { ) require.NoError(t, httpListener.WaitUntilRequestSucceeds(nil)) - rsp, err := GetPageFromListener(t, httpListener, "group.gitlab-example.com:", "project/") + rsp, err := GetPageFromListener(t, httpListener, "group.gitlab-example.com", "project/") require.NoError(t, err) rsp.Body.Close() @@ -69,3 +70,29 @@ func TestMultipleListenersFromEnvironmentVariables(t *testing.T) { require.Equal(t, http.StatusOK, rsp.StatusCode) } } + +// TODO: remove along chroot https://gitlab.com/gitlab-org/gitlab-pages/-/issues/561 +func TestEnableJailFromEnvironment(t *testing.T) { + out, teardown := runPagesProcess(t, + true, + *pagesBinary, + []ListenSpec{httpListener}, + "", + []string{ + "DAEMON_ENABLE_JAIL=true", + }, + "-domain-config-source", "disk", + ) + t.Cleanup(teardown) + + require.Eventually(t, func() bool { + require.Contains(t, out.String(), "\"daemon-enable-jail\":true") + return true + }, time.Second, 10*time.Millisecond) + + rsp, err := GetPageFromListener(t, httpListener, "group.gitlab-example.com", "project/") + + require.NoError(t, err) + rsp.Body.Close() + require.Equal(t, http.StatusOK, rsp.StatusCode) +} diff --git a/test/acceptance/helpers_test.go b/test/acceptance/helpers_test.go index b267f1a2..472f42d1 100644 --- a/test/acceptance/helpers_test.go +++ b/test/acceptance/helpers_test.go @@ -245,7 +245,13 @@ func RunPagesProcessWithStubGitLabServer(t *testing.T, opts ...processOption) *L source := NewGitlabDomainsSourceStub(t, processCfg.gitlabStubOpts) gitLabAPISecretKey := CreateGitLabAPISecretKeyFixtureFile(t) - processCfg.extraArgs = append(processCfg.extraArgs, "-pages-root", wd, "-internal-gitlab-server", source.URL, "-api-secret-key", gitLabAPISecretKey, "-domain-config-source", "gitlab") + processCfg.extraArgs = append( + processCfg.extraArgs, + "-pages-root", wd, + "-internal-gitlab-server", source.URL, + "-api-secret-key", gitLabAPISecretKey, + "-domain-config-source", "gitlab", + ) logBuf, cleanup := runPagesProcess(t, processCfg.wait, processCfg.pagesBinary, processCfg.listeners, "", processCfg.envs, processCfg.extraArgs...) @@ -386,11 +392,13 @@ func getPagesArgs(t *testing.T, listeners []ListenSpec, promPort string, extraAr } // most of our acceptance tests still work only with disk source - // TODO: remove this with -domain-config-source flag itself: - // https://gitlab.com/gitlab-org/gitlab-pages/-/issues/571 + // TODO: remove this with -domain-config-source flag itself along with daemon-enable-jail: // https://gitlab.com/gitlab-org/gitlab-pages/-/issues/382 + // https://gitlab.com/gitlab-org/gitlab-pages/-/issues/561 if !contains(extraArgs, "-domain-config-source") { - args = append(args, "-domain-config-source", "disk") + args = append(args, + "-domain-config-source", "disk", + ) } args = append(args, getPagesDaemonArgs(t)...) |