Restore the old in-place chroot behaviour as a command-line option

8 files changed, 223 insertions, 51 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
diff --git a/README.md b/README.md
index 079e757d..7e2c3cbf 100644
--- a/README.md
+++ b/README.md
@@ -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 78413205..e971588a 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) {
diff --git a/daemon.go b/daemon.go
index 5a456e8f..2b3cc8c6 100644
--- a/daemon.go
+++ b/daemon.go
@@ -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,10 +199,11 @@ func daemonChroot(cmd *exec.Cmd) (*jail.Jail, error) {
 	return cage, nil
 }
 
-func daemonize(config appConfig, uid, gid uint) error {
+func daemonize(config appConfig, uid, gid uint, inPlace bool) error {
 	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)
@@ -177,12 +213,17 @@ func daemonize(config appConfig, uid, gid uint) error {
 	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 err
 	}
-	defer chroot.Dispose()
+	defer wrapper.Dispose()
 
 	// Create a pipe to pass the configuration
 	configReader, configWriter, err := os.Pipe()
@@ -200,9 +241,9 @@ func daemonize(config appConfig, uid, gid uint) error {
 		return err
 	}
 
-	//detach binded mountpoints
-	if err := chroot.LazyUnbind(); err != nil {
-		log.WithError(err).Print("chroot lazy umount failed")
+	// Proactively detach any bind-mounts so they can't be left dangling
+	if err := wrapper.LazyUnbind(); err != nil {
+		log.WithError(err).Print("jail lazy umount failed")
 		return err
 	}
 
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 04a00a74..cb242b35 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()
 	require.NoError(t, 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)
 	require.NoError(t, 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")
+}
diff --git a/main.go b/main.go
index a4d315af..30f594ea 100644
--- a/main.go
+++ b/main.go
@@ -38,6 +38,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")
 	adminSecretPath        = flag.String("admin-secret-path", "", "Path to the file containing the admin secret token")
@@ -142,6 +143,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,
@@ -170,7 +172,7 @@ func appMain() {
 	}
 
 	if *daemonUID != 0 || *daemonGID != 0 {
-		if err := daemonize(config, *daemonUID, *daemonGID); err != nil {
+		if err := daemonize(config, *daemonUID, *daemonGID, *daemonInplaceChroot); err != nil {
 			fatal(err)
 		}