diff options
author | Nick Thomas <nick@gitlab.com> | 2018-04-25 00:42:56 +0300 |
---|---|---|
committer | Nick Thomas <nick@gitlab.com> | 2018-04-30 13:05:32 +0300 |
commit | b7bed720297e7bf37774c98b19e1a41a929cc9c8 (patch) | |
tree | 3d8e2a934f438d79490df13c876d9a1c1f5ad470 | |
parent | ca27f8df7263232c00f2cc0c476408feceb9c9f6 (diff) |
Restore the old in-place chroot behaviour as a command-line option
-rw-r--r-- | .gitlab-ci.yml | 6 | ||||
-rw-r--r-- | README.md | 40 | ||||
-rw-r--r-- | acceptance_test.go | 17 | ||||
-rw-r--r-- | daemon.go | 57 | ||||
-rw-r--r-- | helpers_test.go | 48 | ||||
-rw-r--r-- | internal/jail/jail.go | 55 | ||||
-rw-r--r-- | internal/jail/jail_test.go | 43 | ||||
-rw-r--r-- | main.go | 4 |
8 files changed, 221 insertions, 49 deletions
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 5cc17b88..771831a9 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -7,8 +7,10 @@ image: golang:1.8 - make cover - echo "Running all tests without daemonizing..." - make test - - echo "Running just the acceptance tests daemonized...." - - TEST_DAEMONIZE=1 make acceptance + - echo "Running just the acceptance tests daemonized (tmpdir)...." + - TEST_DAEMONIZE=tmpdir make acceptance + - echo "Running just the acceptance tests daemonized (inplace)...." + - TEST_DAEMONIZE=inplace make acceptance artifacts: paths: - bin/gitlab-pages @@ -106,7 +106,45 @@ $ make $ sudo ./gitlab-pages -listen-http ":80" -pages-root path/to/gitlab/shared/pages -pages-domain example.com -daemon-uid 1000 -daemon-gid 1000 ``` -Please note that changes to `/etc/resolv.conf` or `SSL_CERT_FILE` will be ignored by `gitlab-pages` until restarted. +#### Caveats + +The `/etc/resolv.conf` file, and any file pointed to by the `SSL_CERT_FILE` +environment variable, will be copied into the jail. As a result, changes to +these files will not be reflected in Pages until it's restarted. + +Bind mounts are unavailable on a range of non-Linux systems. Some of these +systems (e.g., BSD) offer native "jail" functionality. It is recommended to set +up an externally-managed jail and run the Pages daemon within it as an ordinary +user if available. + +A less-functional (but just as secure) operation mode is provided via the +`-daemon-inplace-chroot` command-line option. If passed, Pages will daemonize +as usual, but chroot directly to the `-pages-root` directory instead of building +a complete jail in the system temporary directory. This mode will break the +artifact server proxy and (on some systems) TLS operation, but was the default +mode prior to GitLab Pages v0.8.0 + +The default secure mode will also fail for certain Linux-based configurations. +Known cases include: + +* The Pages daemon is running inside an unprivileged container + * Bind mount functionality requires the `CAP_SYS_ADMIN` privilege + * This is only available to containers run in privileged mode +* The system temporary directory is mounted `noexec` or `nodev` + * The jail is created in `$TMPDIR`. + * Character device files are created within the jail + * A copy of the gitlab-pages executable is run from within the bind mount +* AppArmor/SELinux is enabled + * These systems disallow bind-mounting in certain configurations + +In these cases, workarounds are similar to those documented for non-Linux +systems - use an external jailing technology, or fall back to the pre-v0.8.0 +behaviour using `-daemon-inplace-chroot`. + +On Linux, Docker and other containerization systems can be used to build a jail +within which the Pages daemon can safely run with secure mode disabled. However, +this configuration **is not secure** if simply using the default +`gitlab/gitlab-ce` and `gitlab-gitlab-ee` Docker containers! ### Listen on multiple ports diff --git a/acceptance_test.go b/acceptance_test.go index 1b250050..8b3cca56 100644 --- a/acceptance_test.go +++ b/acceptance_test.go @@ -36,7 +36,7 @@ var ( httpsListener = listeners[2] ) -func skipUnlessEnabled(t *testing.T) { +func skipUnlessEnabled(t *testing.T, conditions ...string) { if testing.Short() { t.Log("Acceptance tests disabled") t.SkipNow() @@ -46,6 +46,19 @@ func skipUnlessEnabled(t *testing.T) { t.Errorf("Couldn't find gitlab-pages binary at %s", *pagesBinary) t.FailNow() } + + for _, condition := range conditions { + switch condition { + case "not-inplace-chroot": + if os.Getenv("TEST_DAEMONIZE") == "inplace" { + t.Log("Not supported with -daemon-inplace-chroot") + t.SkipNow() + } + default: + t.Error("Unknown condition:", condition) + t.FailNow() + } + } } func TestUnknownHostReturnsNotFound(t *testing.T) { @@ -341,7 +354,7 @@ func TestObscureMIMEType(t *testing.T) { } func TestArtifactProxyRequest(t *testing.T) { - skipUnlessEnabled(t) + skipUnlessEnabled(t, "not-inplace-chroot") transport := (TestHTTPSClient.Transport).(*http.Transport) defer func(t time.Duration) { @@ -1,7 +1,9 @@ package main import ( + "crypto/rand" "encoding/json" + "fmt" "os" "os/exec" "os/signal" @@ -102,8 +104,41 @@ func passSignals(cmd *exec.Cmd) { }() } -func daemonChroot(cmd *exec.Cmd) (*jail.Jail, error) { - cage := jail.TimestampedJail("gitlab-pages", 0755) +func chrootDaemon(cmd *exec.Cmd) (*jail.Jail, error) { + wd, err := os.Getwd() + if err != nil { + return nil, err + } + + chroot := jail.Into(wd) + + // Generate a probabilistically-unique suffix for the copy of the pages + // binary being placed into the chroot + suffix := make([]byte, 16) + if _, err := rand.Read(suffix); err != nil { + return nil, err + } + + tempExecutablePath := fmt.Sprintf("/.daemon.%x", suffix) + + if err := chroot.CopyTo(tempExecutablePath, cmd.Path); err != nil { + return nil, err + } + + // Update command to use chroot + cmd.SysProcAttr.Chroot = chroot.Path() + cmd.Path = tempExecutablePath + cmd.Dir = "/" + + if err := chroot.Build(); err != nil { + return nil, err + } + + return chroot, nil +} + +func jailDaemon(cmd *exec.Cmd) (*jail.Jail, error) { + cage := jail.CreateTimestamped("gitlab-pages", 0755) wd, err := os.Getwd() if err != nil { @@ -164,7 +199,7 @@ func daemonChroot(cmd *exec.Cmd) (*jail.Jail, error) { return cage, nil } -func daemonize(config appConfig, uid, gid uint) { +func daemonize(config appConfig, uid, gid uint, inPlace bool) error { var err error defer func() { if err != nil { @@ -173,8 +208,9 @@ func daemonize(config appConfig, uid, gid uint) { }() log.WithFields(log.Fields{ - "uid": uid, - "gid": gid, + "uid": uid, + "gid": gid, + "in-place": inPlace, }).Info("running the daemon as unprivileged user") cmd, err := daemonReexec(uid, gid, daemonRunProgram) @@ -184,12 +220,17 @@ func daemonize(config appConfig, uid, gid uint) { defer killProcess(cmd) // Run daemon in chroot environment - chroot, err := daemonChroot(cmd) + var wrapper *jail.Jail + if inPlace { + wrapper, err = chrootDaemon(cmd) + } else { + wrapper, err = jailDaemon(cmd) + } if err != nil { log.WithError(err).Print("chroot failed") return } - defer chroot.Dispose() + defer wrapper.Dispose() // Create a pipe to pass the configuration configReader, configWriter, err := os.Pipe() @@ -214,7 +255,7 @@ func daemonize(config appConfig, uid, gid uint) { } //detach binded mountpoints - if err = chroot.LazyUnbind(); err != nil { + if err = wrapper.LazyUnbind(); err != nil { log.WithError(err).Print("chroot lazy umount failed") return } diff --git a/helpers_test.go b/helpers_test.go index 656585c8..e853e1c4 100644 --- a/helpers_test.go +++ b/helpers_test.go @@ -10,7 +10,6 @@ import ( "net/http" "os" "os/exec" - "strconv" "strings" "testing" "time" @@ -206,23 +205,46 @@ func getPagesArgs(t *testing.T, listeners []ListenSpec, promPort string, extraAr args = append(args, "-metrics-address", promPort) } - // At least one of `-daemon-uid` and `-daemon-gid` must be non-zero - if daemon, _ := strconv.ParseBool(os.Getenv("TEST_DAEMONIZE")); daemon { - if os.Geteuid() == 0 { - t.Log("Running pages as a daemon") - args = append(args, "-daemon-uid", "0") - args = append(args, "-daemon-gid", "65534") // Root user can switch to "nobody" - } else { - t.Log("Privilege-dropping requested but not running as root!") - t.FailNow() - } - } - + args = append(args, getPagesDaemonArgs(t)...) args = append(args, extraArgs...) return } +func getPagesDaemonArgs(t *testing.T) []string { + mode := os.Getenv("TEST_DAEMONIZE") + if mode == "" { + return nil + } + + if os.Geteuid() != 0 { + t.Log("Privilege-dropping requested but not running as root!") + t.FailNow() + return nil + } + + out := []string{} + + switch mode { + case "tmpdir": + out = append(out, "-daemon-inplace-chroot=false") + case "inplace": + out = append(out, "-daemon-inplace-chroot=true") + default: + t.Log("Unknown daemonize mode", mode) + t.FailNow() + return nil + } + + t.Log("Running pages as a daemon") + + // This triggers the drop-privileges-and-chroot code in the pages daemon + out = append(out, "-daemon-uid", "0") + out = append(out, "-daemon-gid", "65534") + + return out +} + // Does a HTTP(S) GET against the listener specified, setting a fake // Host: and constructing the URL from the listener and the URL suffix. func GetPageFromListener(t *testing.T, spec ListenSpec, host, urlsuffix string) (*http.Response, error) { diff --git a/internal/jail/jail.go b/internal/jail/jail.go index d2b04be0..22f22eac 100644 --- a/internal/jail/jail.go +++ b/internal/jail/jail.go @@ -21,30 +21,45 @@ type pathAndMode struct { // Jail is a Chroot jail builder type Jail struct { + root string + deleteRoot bool directories []pathAndMode files map[string]pathAndMode bindMounts map[string]string } -// New returns a Jail for path -func New(path string, perm os.FileMode) *Jail { +// Into returns a Jail on path, assuming it already exists on disk. On disposal, +// the jail *will not* remove the path +func Into(path string) *Jail { return &Jail{ - directories: []pathAndMode{pathAndMode{path: path, mode: perm}}, - files: make(map[string]pathAndMode), - bindMounts: make(map[string]string), + root: path, + deleteRoot: false, + files: make(map[string]pathAndMode), + bindMounts: make(map[string]string), } } -// TimestampedJail return a Jail with Path composed by prefix and current timestamp -func TimestampedJail(prefix string, perm os.FileMode) *Jail { +// Create returns a Jail on path, creating the directory if needed. On disposal, +// the jail will remove the path +func Create(path string, perm os.FileMode) *Jail { + jail := Into(path) + jail.deleteRoot = true + jail.directories = append(jail.directories, pathAndMode{path: path, mode: perm}) + + return jail +} + +// CreateTimestamped returns a Jail on a path composed by prefix and current +// timestamp, creating the directory. On disposal, the jail will remove the path +func CreateTimestamped(prefix string, perm os.FileMode) *Jail { jailPath := path.Join(os.TempDir(), fmt.Sprintf("%s-%d", prefix, time.Now().UnixNano())) - return New(jailPath, perm) + return Create(jailPath, perm) } // Path returns the path of the jail func (j *Jail) Path() string { - return j.directories[0].path + return j.root } // Build creates the jail, making directories and copying files. If an error @@ -87,7 +102,27 @@ func (j *Jail) Build() error { } func (j *Jail) removeAll() error { - return os.RemoveAll(j.Path()) + // Deleting the root will remove all child directories, so there's no need + // to traverse files and directories + if j.deleteRoot { + if err := os.RemoveAll(j.Path()); err != nil { + return fmt.Errorf("Can't delete jail %q. %s", j.Path(), err) + } + } else { + for path := range j.files { + if err := os.Remove(path); err != nil { + return fmt.Errorf("Can't delete file in jail %q: %s", path, err) + } + } + + for _, dest := range j.directories { + if err := os.Remove(dest.path); err != nil { + return fmt.Errorf("Can't delete directory in jail %q: %s", dest.path, err) + } + } + } + + return nil } // Dispose erases everything inside the jail diff --git a/internal/jail/jail_test.go b/internal/jail/jail_test.go index 6de870df..04ceb0e5 100644 --- a/internal/jail/jail_test.go +++ b/internal/jail/jail_test.go @@ -26,8 +26,8 @@ func TestTimestampedJails(t *testing.T) { prefix := "jail" var mode os.FileMode = 0755 - j1 := jail.TimestampedJail(prefix, mode) - j2 := jail.TimestampedJail(prefix, mode) + j1 := jail.CreateTimestamped(prefix, mode) + j2 := jail.CreateTimestamped(prefix, mode) assert.NotEqual(j1.Path, j2.Path()) } @@ -36,7 +36,7 @@ func TestJailPath(t *testing.T) { assert := assert.New(t) jailPath := tmpJailPath() - cage := jail.New(jailPath, 0755) + cage := jail.Create(jailPath, 0755) assert.Equal(jailPath, cage.Path()) } @@ -45,7 +45,7 @@ func TestJailBuild(t *testing.T) { assert := assert.New(t) jailPath := tmpJailPath() - cage := jail.New(jailPath, 0755) + cage := jail.Create(jailPath, 0755) _, err := os.Stat(cage.Path()) assert.Error(err, "Jail path should not exist before Jail.Build()") @@ -60,7 +60,7 @@ func TestJailBuild(t *testing.T) { func TestJailOnlySupportsOneBindMount(t *testing.T) { jailPath := tmpJailPath() - cage := jail.New(jailPath, 0755) + cage := jail.Create(jailPath, 0755) cage.Bind("/bin", "/bin") cage.Bind("/lib", "/lib") @@ -75,7 +75,7 @@ func TestJailOnlySupportsOneBindMount(t *testing.T) { func TestJailBuildCleansUpWhenMountFails(t *testing.T) { jailPath := tmpJailPath() - cage := jail.New(jailPath, 0755) + cage := jail.Create(jailPath, 0755) cage.Bind("/foo", "/this/path/does/not/exist/so/mount/will/fail") err := cage.Build() @@ -89,7 +89,7 @@ func TestJailDispose(t *testing.T) { assert := assert.New(t) jailPath := tmpJailPath() - cage := jail.New(jailPath, 0755) + cage := jail.Create(jailPath, 0755) err := cage.Build() assert.NoError(err) @@ -105,7 +105,7 @@ func TestJailDisposeDoNotFailOnMissingPath(t *testing.T) { assert := assert.New(t) jailPath := tmpJailPath() - cage := jail.New(jailPath, 0755) + cage := jail.Create(jailPath, 0755) _, err := os.Stat(cage.Path()) assert.Error(err, "Jail path should not exist") @@ -132,7 +132,7 @@ func TestJailWithCharacterDevice(t *testing.T) { expectedRdev := sys.Rdev jailPath := tmpJailPath() - cage := jail.New(jailPath, 0755) + cage := jail.Create(jailPath, 0755) cage.MkDir("/dev", 0755) require.NoError(t, cage.CharDev("/dev/urandom")) @@ -182,7 +182,7 @@ func TestJailWithFiles(t *testing.T) { t.Run(test.name, func(t *testing.T) { assert := assert.New(t) - cage := jail.TimestampedJail("jail-mkdir", 0755) + cage := jail.CreateTimestamped("jail-mkdir", 0755) for _, dir := range test.directories { cage.MkDir(dir, 0755) } @@ -219,7 +219,7 @@ func TestJailCopyTo(t *testing.T) { content := "hello" - cage := jail.TimestampedJail("check-file-copy", 0755) + cage := jail.CreateTimestamped("check-file-copy", 0755) tmpFile, err := ioutil.TempFile("", "dummy-file") if err != nil { @@ -269,7 +269,7 @@ func TestJailLazyUnbind(t *testing.T) { tmpFile.Close() jailPath := tmpJailPath() - cage := jail.New(jailPath, 0755) + cage := jail.Create(jailPath, 0755) cage.MkDir("/my-bind", 0755) cage.Bind("/my-bind", toBind) @@ -298,3 +298,22 @@ func TestJailLazyUnbind(t *testing.T) { _, err = os.Stat(tmpFilePath) assert.NoError(err, "disposing a jail should not delete files under binded directories") } + +func TestJailIntoOnlyCleansSubpaths(t *testing.T) { + jailPath := tmpJailPath() + require.NoError(t, os.MkdirAll(jailPath, 0755)) + defer os.RemoveAll(jailPath) + + chroot := jail.Into(jailPath) + chroot.MkDir("/etc", 0755) + chroot.Copy("/etc/resolv.conf") + require.NoError(t, chroot.Build()) + require.NoError(t, chroot.Dispose()) + + _, err := os.Stat(path.Join(jailPath, "/etc/resolv.conf")) + require.True(t, os.IsNotExist(err), "/etc/resolv.conf in jail was not removed") + _, err = os.Stat(path.Join(jailPath, "/etc")) + require.True(t, os.IsNotExist(err), "/etc in jail was not removed") + _, err = os.Stat(jailPath) + require.NoError(t, err, "/ in jail (corresponding to external directory) was removed") +} @@ -31,6 +31,7 @@ var ( metricsAddress = flag.String("metrics-address", "", "The address to listen on for metrics requests") 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") logFormat = flag.String("log-format", "text", "The log output format: 'text' or 'json'") logVerbose = flag.Bool("log-verbose", false, "Verbose logging") @@ -115,6 +116,7 @@ func appMain() { "artifacts-server-timeout": *artifactsServerTimeout, "daemon-gid": *daemonGID, "daemon-uid": *daemonUID, + "daemon-inplace-chroot": *daemonInplaceChroot, "default-config-filename": flag.DefaultConfigFlagname, "disable-cross-origin-requests": *disableCrossOriginRequests, "domain": config.Domain, @@ -178,7 +180,7 @@ func appMain() { } if *daemonUID != 0 || *daemonGID != 0 { - daemonize(config, *daemonUID, *daemonGID) + daemonize(config, *daemonUID, *daemonGID, *daemonInplaceChroot) return } |