From c1564ce0830fcda1f7f0d28097d75c555f02dcad Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Thu, 16 Mar 2023 12:43:30 -0700 Subject: [PATCH 1/2] V3: prevent git's 'dubious ownership' error --- cmd/git-sync/main.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/cmd/git-sync/main.go b/cmd/git-sync/main.go index 01ce4541f..60550ed63 100644 --- a/cmd/git-sync/main.go +++ b/cmd/git-sync/main.go @@ -1285,6 +1285,10 @@ func setupDefaultGitConfigs(ctx context.Context) error { // How to manage credentials (for those modes that need it). key: "credential.helper", val: "cache --timeout 3600", + }, { + // Our working root is safe (avoid a "dubious ownership" error). + key: "safe.directory", + val: "*", }} for _, kv := range configs { if _, err := cmdRunner.Run(ctx, "", nil, *flGitCmd, "config", "--global", kv.key, kv.val); err != nil { From 9241b1061f73a7931f037feba90b67322bf83d00 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Thu, 16 Mar 2023 16:11:18 -0700 Subject: [PATCH 2/2] Exercise the git "dubious ownership" path To do this, we run the e2e test as a different user. To do that, we need git-sync to make sure that everything is group accessible. To clean up after the test, we need everything to be group writable. To do that, we add a new flag: `--group-write`. --- cmd/git-sync/main.go | 20 ++++++++++++++++---- pkg/logging/logging.go | 2 +- test_e2e.sh | 7 ++++++- 3 files changed, 23 insertions(+), 6 deletions(-) diff --git a/cmd/git-sync/main.go b/cmd/git-sync/main.go index 60550ed63..0b81de85a 100644 --- a/cmd/git-sync/main.go +++ b/cmd/git-sync/main.go @@ -36,6 +36,7 @@ import ( "strconv" "strings" "sync" + "syscall" "time" "github.com/prometheus/client_golang/prometheus" @@ -74,6 +75,8 @@ var flOneTime = flag.Bool("one-time", envBool("GIT_SYNC_ONE_TIME", false), "exit after the first sync") var flMaxSyncFailures = flag.Int("max-sync-failures", envInt("GIT_SYNC_MAX_SYNC_FAILURES", 0), "the number of consecutive failures allowed before aborting (the first sync must succeed, -1 will retry forever after the initial sync)") +var flGroupWrite = flag.Bool("group-write", envBool("GIT_SYNC_GROUP_WRITE", false), + "ensure that all data (repo, worktrees, etc.) is group writable") var flChmod = flag.Int("change-permissions", envInt("GIT_SYNC_PERMISSIONS", 0), "the file permissions to apply to the checked-out files (0 will not change permissions at all)") var flSparseCheckoutFile = flag.String("sparse-checkout-file", envString("GIT_SYNC_SPARSE_CHECKOUT_FILE", ""), @@ -178,6 +181,8 @@ const ( gcAlways = "always" gcAggressive = "aggressive" gcOff = "off" + + defaultDirMode = os.FileMode(0775) // subject to umask ) func init() { @@ -379,6 +384,13 @@ func main() { } } + // If the user asked for group-writable data, make sure the umask allows it. + if *flGroupWrite { + syscall.Umask(0002) + } else { + syscall.Umask(0022) + } + if *flAddUser { if err := addUser(); err != nil { handleError(false, "ERROR: can't write to /etc/passwd: %v", err) @@ -684,7 +696,7 @@ func updateSymlink(ctx context.Context, gitRoot, link, newDir string) (string, e // Make sure the link directory exists. We do this here, rather than at // startup because it might be under --root and that gets wiped in some // circumstances. - if err := os.MkdirAll(filepath.Dir(linkDir), os.FileMode(int(0755))); err != nil { + if err := os.MkdirAll(filepath.Dir(linkDir), defaultDirMode); err != nil { return "", fmt.Errorf("error making symlink dir: %v", err) } @@ -818,8 +830,7 @@ func addWorktreeAndSwap(ctx context.Context, repo, gitRoot, dest, branch, rev st defer source.Close() if _, err := os.Stat(gitInfoPath); os.IsNotExist(err) { - fileMode := os.FileMode(int(0755)) - err := os.Mkdir(gitInfoPath, fileMode) + err := os.Mkdir(gitInfoPath, defaultDirMode) if err != nil { return err } @@ -974,7 +985,7 @@ func cloneRepo(ctx context.Context, repo, branch, rev string, depth int, gitRoot defer source.Close() if _, err := os.Stat(gitInfoPath); os.IsNotExist(err) { - fileMode := os.FileMode(int(0755)) + fileMode := os.FileMode(defaultDirMode) err := os.Mkdir(gitInfoPath, fileMode) if err != nil { return err @@ -1290,6 +1301,7 @@ func setupDefaultGitConfigs(ctx context.Context) error { key: "safe.directory", val: "*", }} + for _, kv := range configs { if _, err := cmdRunner.Run(ctx, "", nil, *flGitCmd, "config", "--global", kv.key, kv.val); err != nil { return fmt.Errorf("error configuring git %q %q: %v", kv.key, kv.val, err) diff --git a/pkg/logging/logging.go b/pkg/logging/logging.go index 19e5f4f8f..738c20cd0 100644 --- a/pkg/logging/logging.go +++ b/pkg/logging/logging.go @@ -99,7 +99,7 @@ func (l *Logger) DeleteErrorFile() { // writeContent writes the error content to the error file. func (l *Logger) writeContent(content []byte) { if _, err := os.Stat(l.root); os.IsNotExist(err) { - fileMode := os.FileMode(0755) + fileMode := os.FileMode(0775) // umask applies if err := os.Mkdir(l.root, fileMode); err != nil { l.Logger.Error(err, "can't create the root directory", "root", l.root) return diff --git a/test_e2e.sh b/test_e2e.sh index b28f80863..f86a9123d 100755 --- a/test_e2e.sh +++ b/test_e2e.sh @@ -139,6 +139,7 @@ ROOT="$DIR/root" function clean_root() { rm -rf "$ROOT" mkdir -p "$ROOT" + chmod g+rwx "$ROOT" } # Init SSH for test cases. @@ -146,6 +147,7 @@ DOT_SSH="$DIR/dot_ssh" mkdir -p "$DOT_SSH" ssh-keygen -f "$DOT_SSH/id_test" -P "" >/dev/null cat "$DOT_SSH/id_test.pub" > "$DOT_SSH/authorized_keys" +chmod -R g+r "$DOT_SSH" SLOW_GIT_CLONE=/slow_git_clone.sh SLOW_GIT_FETCH=/slow_git_fetch.sh @@ -159,6 +161,7 @@ EXECHOOK_ENVVAL=envval RUNLOG="$DIR/runlog.exechook-fail-retry" rm -f $RUNLOG touch $RUNLOG +chmod g+rw $RUNLOG function GIT_SYNC() { #./bin/linux_amd64/git-sync "$@" @@ -171,7 +174,7 @@ function GIT_SYNC() { ${RM} \ --label git-sync-e2e="$RUNID" \ --network="host" \ - -u $(id -u):$(id -g) \ + -u git-sync:$(id -g) `# rely on GID, triggering "dubious ownership"` \ -v "$ROOT":"$ROOT":rw \ -v "$REPO":"$REPO":ro \ -v "$REPO2":"$REPO2":ro \ @@ -189,6 +192,7 @@ function GIT_SYNC() { e2e/git-sync:"${E2E_TAG}"__$(go env GOOS)_$(go env GOARCH) \ --v=6 \ --add-user \ + --group-write \ --git-config='protocol.file.allow:always' \ "$@" } @@ -392,6 +396,7 @@ function e2e::worktree_cleanup() { git -C "$REPO" commit -qam "$FUNCNAME new file" REV=$(git -C "$REPO" rev-list -n1 HEAD) git -C "$REPO" worktree add -q "$ROOT"/"$REV" -b e2e --no-checkout + chmod g+w "$ROOT"/"$REV" sleep 10 assert_file_exists "$ROOT"/link/file2 assert_file_eq "$ROOT"/link/file2 "$FUNCNAME-ok"