Prevent clobbering existing Git alternates

8 files changed, 148 insertions, 41 deletions

diff --git a/changelogs/unreleased/pool-link-no-clobber.yml b/changelogs/unreleased/pool-link-no-clobber.yml
new file mode 100644
index 000000000..5fefc327b
--- /dev/null
+++ b/changelogs/unreleased/pool-link-no-clobber.yml
@@ -0,0 +1,5 @@
+---
+title: Prevent clobbering existing Git alternates
+merge_request: 1132
+author:
+type: fixed
diff --git a/internal/git/objectpool/link.go b/internal/git/objectpool/link.go
index 45058ffdd..1b5614c1b 100644
--- a/internal/git/objectpool/link.go
+++ b/internal/git/objectpool/link.go
@@ -8,6 +8,7 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"strings"
 
 	"gitlab.com/gitlab-org/gitaly/internal/git/remote"
 
@@ -41,7 +42,36 @@ func (o *ObjectPool) Link(ctx context.Context, repo *gitalypb.Repository) error
 		}
 	}
 
-	return ioutil.WriteFile(altPath, []byte(filepath.Join(relPath, "objects")), 0644)
+	expectedContent := filepath.Join(relPath, "objects")
+
+	actualContent, err := ioutil.ReadFile(altPath)
+	if err == nil {
+		if strings.TrimSuffix(string(actualContent), "\n") == expectedContent {
+			return nil
+		}
+
+		return fmt.Errorf("unexpected alternates content: %q", actualContent)
+	}
+
+	if !os.IsNotExist(err) {
+		return err
+	}
+
+	tmp, err := ioutil.TempFile(filepath.Dir(altPath), "alternates")
+	if err != nil {
+		return err
+	}
+	defer os.Remove(tmp.Name())
+
+	if _, err := io.WriteString(tmp, expectedContent); err != nil {
+		return err
+	}
+
+	if err := tmp.Close(); err != nil {
+		return err
+	}
+
+	return os.Rename(tmp.Name(), altPath)
 }
 
 func (o *ObjectPool) getRelativeObjectPath(repo *gitalypb.Repository) (string, error) {
diff --git a/internal/git/objectpool/link_test.go b/internal/git/objectpool/link_test.go
index deaebf8d0..c3740f4f5 100644
--- a/internal/git/objectpool/link_test.go
+++ b/internal/git/objectpool/link_test.go
@@ -3,6 +3,7 @@ package objectpool
 import (
 	"io/ioutil"
 	"os"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -18,11 +19,11 @@ func TestLink(t *testing.T) {
 	testRepo, _, cleanupFn := testhelper.NewTestRepo(t)
 	defer cleanupFn()
 
-	pool, err := NewObjectPool(testRepo.GetStorageName(), t.Name())
+	pool, err := NewObjectPool(testRepo.GetStorageName(), testhelper.NewTestObjectPoolName(t))
 	require.NoError(t, err)
-	defer pool.Remove(ctx)
 
-	require.NoError(t, pool.Create(ctx, testRepo))
+	require.NoError(t, pool.Remove(ctx), "make sure pool does not exist prior to creation")
+	require.NoError(t, pool.Create(ctx, testRepo), "create pool")
 
 	altPath, err := git.AlternatesPath(testRepo)
 	require.NoError(t, err)
@@ -31,12 +32,13 @@ func TestLink(t *testing.T) {
 
 	require.NoError(t, pool.Link(ctx, testRepo))
 
-	_, err = os.Stat(altPath)
-	require.False(t, os.IsNotExist(err))
+	require.FileExists(t, altPath, "alternates file must exist after Link")
 
 	content, err := ioutil.ReadFile(altPath)
 	require.NoError(t, err)
 
+	require.True(t, strings.HasPrefix(string(content), "../"), "expected %q to be relative path", content)
+
 	require.NoError(t, pool.Link(ctx, testRepo))
 
 	newContent, err := ioutil.ReadFile(altPath)
@@ -68,8 +70,8 @@ func TestUnlink(t *testing.T) {
 
 	require.NoError(t, pool.Create(ctx, testRepo))
 	require.NoError(t, pool.Link(ctx, testRepo))
-	_, err = os.Stat(altPath)
-	require.False(t, os.IsNotExist(err))
+
+	require.FileExists(t, altPath, "alternates file must exist after Link")
 
 	require.NoError(t, pool.Unlink(ctx, testRepo))
 	_, err = os.Stat(altPath)
diff --git a/internal/git/objectpool/pool.go b/internal/git/objectpool/pool.go
index 828c74dd8..393a0bf65 100644
--- a/internal/git/objectpool/pool.go
+++ b/internal/git/objectpool/pool.go
@@ -2,6 +2,7 @@ package objectpool
 
 import (
 	"context"
+	"fmt"
 	"os"
 	"path"
 
@@ -71,22 +72,26 @@ func (o *ObjectPool) Create(ctx context.Context, repo *gitalypb.Repository) (err
 	}
 
 	if err := o.clone(ctx, repo); err != nil {
-		return err
+		return fmt.Errorf("clone: %v", err)
 	}
 
 	if err := o.removeHooksDir(); err != nil {
-		return err
+		return fmt.Errorf("remove hooks: %v", err)
 	}
 
 	if err := remote.Remove(ctx, o, "origin"); err != nil {
-		return err
+		return fmt.Errorf("remove origin: %v", err)
 	}
 
 	if err := o.removeRefs(ctx); err != nil {
-		return err
+		return fmt.Errorf("remove refs: %v", err)
+	}
+
+	if err := o.setConfig(ctx, "gc.auto", "0"); err != nil {
+		return fmt.Errorf("config gc.auto: %v", err)
 	}
 
-	return o.setConfig(ctx, "gc.auto", "0")
+	return nil
 }
 
 // Remove will remove the pool, and all its contents without preparing and/or
diff --git a/internal/service/objectpool/link_test.go b/internal/service/objectpool/link_test.go
index d34a20ad6..609b93155 100644
--- a/internal/service/objectpool/link_test.go
+++ b/internal/service/objectpool/link_test.go
@@ -1,6 +1,8 @@
 package objectpool
 
 import (
+	"io/ioutil"
+	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -27,8 +29,9 @@ func TestLink(t *testing.T) {
 
 	pool, err := objectpool.NewObjectPool(testRepo.GetStorageName(), t.Name())
 	require.NoError(t, err)
-	defer pool.Remove(ctx)
-	require.NoError(t, pool.Create(ctx, testRepo))
+
+	require.NoError(t, pool.Remove(ctx), "make sure pool does not exist at start of test")
+	require.NoError(t, pool.Create(ctx, testRepo), "create pool")
 
 	// Mock object in the pool, which should be available to the pool members
 	// after linking
@@ -68,14 +71,18 @@ func TestLink(t *testing.T) {
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
 			_, err := client.LinkRepositoryToObjectPool(ctx, tc.req)
-			require.Equal(t, tc.code, helper.GrpcCode(err))
 
-			if tc.code == codes.OK {
-				commit, err := log.GetCommit(ctx, testRepo, poolCommitID)
-				require.NoError(t, err)
-				require.NotNil(t, commit)
-				require.Equal(t, poolCommitID, commit.Id)
+			if tc.code != codes.OK {
+				testhelper.RequireGrpcError(t, err, tc.code)
+				return
 			}
+
+			require.NoError(t, err, "error from LinkRepositoryToObjectPool")
+
+			commit, err := log.GetCommit(ctx, testRepo, poolCommitID)
+			require.NoError(t, err)
+			require.NotNil(t, commit)
+			require.Equal(t, poolCommitID, commit.Id)
 		})
 	}
 }
@@ -110,7 +117,7 @@ func TestLinkIdempotent(t *testing.T) {
 	require.NoError(t, err)
 }
 
-func TestUnlink(t *testing.T) {
+func TestLinkNoClobber(t *testing.T) {
 	server, serverSocketPath := runObjectPoolServer(t)
 	defer server.Stop()
 
@@ -120,13 +127,54 @@ func TestUnlink(t *testing.T) {
 	ctx, cancel := testhelper.Context()
 	defer cancel()
 
-	testRepo, _, cleanupFn := testhelper.NewTestRepo(t)
+	testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
 	defer cleanupFn()
 
-	pool, err := objectpool.NewObjectPool(testRepo.GetStorageName(), t.Name())
+	pool, err := objectpool.NewObjectPool(testRepo.GetStorageName(), testhelper.NewTestObjectPoolName(t))
 	require.NoError(t, err)
 	defer pool.Remove(ctx)
+
 	require.NoError(t, pool.Create(ctx, testRepo))
+
+	alternatesFile := filepath.Join(testRepoPath, "objects/info/alternates")
+	testhelper.AssertFileNotExists(t, alternatesFile)
+
+	contentBefore := "mock/objects\n"
+	require.NoError(t, ioutil.WriteFile(alternatesFile, []byte(contentBefore), 0644))
+
+	request := &gitalypb.LinkRepositoryToObjectPoolRequest{
+		Repository: testRepo,
+		ObjectPool: pool.ToProto(),
+	}
+
+	_, err = client.LinkRepositoryToObjectPool(ctx, request)
+	require.Error(t, err)
+
+	contentAfter, err := ioutil.ReadFile(alternatesFile)
+	require.NoError(t, err)
+
+	require.Equal(t, contentBefore, string(contentAfter), "contents of existing alternates file should not have changed")
+}
+
+func TestUnlink(t *testing.T) {
+	server, serverSocketPath := runObjectPoolServer(t)
+	defer server.Stop()
+
+	client, conn := newObjectPoolClient(t, serverSocketPath)
+	defer conn.Close()
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	testRepo, _, cleanupFn := testhelper.NewTestRepo(t)
+	defer cleanupFn()
+
+	pool, err := objectpool.NewObjectPool(testRepo.GetStorageName(), testhelper.NewTestObjectPoolName(t))
+	require.NoError(t, err)
+
+	require.NoError(t, pool.Remove(ctx), "make sure pool does not exist prior to creation")
+	require.NoError(t, pool.Create(ctx, testRepo), "create pool")
+
 	require.NoError(t, pool.Link(ctx, testRepo))
 
 	poolCommitID := testhelper.CreateCommit(t, pool.FullPath(), "pool-test-branch", nil)
diff --git a/internal/service/repository/pre_fetch.go b/internal/service/repository/pre_fetch.go
index 0c62b2eb5..a0377f1f8 100644
--- a/internal/service/repository/pre_fetch.go
+++ b/internal/service/repository/pre_fetch.go
@@ -2,34 +2,33 @@ package repository
 
 import (
 	"context"
-	"errors"
-	"fmt"
-	"io/ioutil"
-	"os"
-	"path/filepath"
 
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
-	"gitlab.com/gitlab-org/gitaly/internal/git"
-	"gitlab.com/gitlab-org/gitaly/internal/git/objectpool"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 )
 
+// PreFetch is unsafe https://gitlab.com/gitlab-org/gitaly/issues/1552
 func (s *server) PreFetch(ctx context.Context, req *gitalypb.PreFetchRequest) (*gitalypb.PreFetchResponse, error) {
-	if err := validatePreFetchRequest(req); err != nil {
-		return nil, helper.ErrInvalidArgument(err)
-	}
+	return nil, helper.Unimplemented
 
-	if err := validatePreFetchPrecondition(req); err != nil {
-		return nil, helper.ErrPreconditionFailed(err)
-	}
+	/*
+		if err := validatePreFetchRequest(req); err != nil {
+			return nil, helper.ErrInvalidArgument(err)
+		}
 
-	if err := preFetch(ctx, req); err != nil {
-		return nil, helper.ErrInternal(err)
-	}
+		if err := validatePreFetchPrecondition(req); err != nil {
+			return nil, helper.ErrPreconditionFailed(err)
+		}
+
+		if err := preFetch(ctx, req); err != nil {
+			return nil, helper.ErrInternal(err)
+		}
 
-	return &gitalypb.PreFetchResponse{}, nil
+		return &gitalypb.PreFetchResponse{}, nil
+	*/
 }
 
+/*
 func validatePreFetchRequest(req *gitalypb.PreFetchRequest) error {
 	if req.GetTargetRepository() == nil {
 		return errors.New("repository is empty")
@@ -153,3 +152,4 @@ func preFetch(ctx context.Context, req *gitalypb.PreFetchRequest) error {
 
 	return os.Rename(tmpRepoDir, targetRepositoryFullPath)
 }
+*/
diff --git a/internal/service/repository/pre_fetch_test.go b/internal/service/repository/pre_fetch_test.go
index 9f23bcf80..d461b7b2a 100644
--- a/internal/service/repository/pre_fetch_test.go
+++ b/internal/service/repository/pre_fetch_test.go
@@ -43,6 +43,8 @@ func getGitObjectDirSize(t *testing.T, repoPath string) int64 {
 }
 
 func TestPreFetch(t *testing.T) {
+	t.Skip("PreFetch is unsafe https://gitlab.com/gitlab-org/gitaly/issues/1552")
+
 	server, serverSocketPath := runRepoServer(t)
 	defer server.Stop()
 
@@ -85,6 +87,8 @@ func TestPreFetch(t *testing.T) {
 }
 
 func TestPreFetchValidationError(t *testing.T) {
+	t.Skip("PreFetch is unsafe https://gitlab.com/gitlab-org/gitaly/issues/1552")
+
 	server, serverSocketPath := runRepoServer(t)
 	defer server.Stop()
 
@@ -167,6 +171,8 @@ func TestPreFetchValidationError(t *testing.T) {
 }
 
 func TestPreFetchDirectoryExists(t *testing.T) {
+	t.Skip("PreFetch is unsafe https://gitlab.com/gitlab-org/gitaly/issues/1552")
+
 	server, serverSocketPath := runRepoServer(t)
 	defer server.Stop()
 
diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go
index bd1863181..ea679e795 100644
--- a/internal/testhelper/testhelper.go
+++ b/internal/testhelper/testhelper.go
@@ -3,6 +3,7 @@ package testhelper
 import (
 	"bytes"
 	"context"
+	"crypto/rand"
 	"encoding/base64"
 	"encoding/json"
 	"fmt"
@@ -355,6 +356,7 @@ func CreateRepo(t *testing.T, storagePath string) (repo *gitalypb.Repository, re
 
 	repoPath, err := ioutil.TempDir(storagePath, normalizedPrefix)
 	require.NoError(t, err)
+
 	relativePath, err = filepath.Rel(storagePath, repoPath)
 	require.NoError(t, err)
 	repo = &gitalypb.Repository{StorageName: "default", RelativePath: relativePath, GlRepository: "project-1"}
@@ -455,3 +457,12 @@ func AssertFileNotExists(t *testing.T, path string) {
 	_, err := os.Stat(path)
 	assert.True(t, os.IsNotExist(err))
 }
+
+// NewTestObjectPoolName returns a random pool repository name.
+func NewTestObjectPoolName(t *testing.T) string {
+	b := make([]byte, 5)
+	_, err := io.ReadFull(rand.Reader, b)
+	require.NoError(t, err)
+
+	return fmt.Sprintf("pool-%x.git", b)
+}