Git command DSL

STYLE.md

@@ -132,3 +132,24 @@ production. When adding new Prometheus metrics, please follow the [best
 practices](https://prometheus.io/docs/practices/naming/) and be aware of
 the
 [gotchas](https://prometheus.io/docs/practices/instrumentation/#things-to-watch-out-for).
+
+## Git Commands
+
+Gitaly relies heavily on spawning git subprocesses to perform work. Any git
+commands spawned from Go code should use the constructs found in
+[`safecmd.go`](internal/git/safecmd.go). These constructs, all beginning with
+`Safe`, help prevent certain kinds of flag injection exploits. Proper usage is
+important to mitigate these injection risks:
+
+- When toggling an option, prefer a longer flag over a short flag for
+  readability.
+	- Desired: `git.Flag{"--long-flag"}` is easier to read and audit
+	- Undesired: `git.Flag{"-L"}`
+- When providing a variable to configure a flag, make sure to include the
+  variable after an equal sign
+	- Desired: `[]git.Flag{"-a="+foo}` prevents flag injection
+	- Undesired: `[]git.Flag("-a"+foo)` allows flag injection
+- Always define a flag's name via a constant, never use a variable:
+	- Desired: `[]git.Flag{"-a"}`
+	- Undesired: `[]git.Flag{foo}` is ambiguous and difficult to audit
+

changelogs/unreleased/po-git-dsl.yml

+---
+title: Git Command DSL
+merge_request: 1476
+author:
+type: other

internal/command/command.go

@@ -424,3 +424,8 @@ func checkNullArgv(cmd *exec.Cmd) error {
 
 	return nil
 }
+
+// Args is an accessor for the command arguments
+func (c *Command) Args() []string {
+	return c.cmd.Args
+}

internal/git/safecmd.go

+package git
+
+import (
+	"context"
+	"fmt"
+	"io"
+	"regexp"
+	"strings"
+
+	"github.com/prometheus/client_golang/prometheus"
+	"gitlab.com/gitlab-org/gitaly/internal/command"
+	"gitlab.com/gitlab-org/gitaly/internal/git/repository"
+)
+
+var invalidationTotal = prometheus.NewCounterVec(
+	prometheus.CounterOpts{
+		Name: "gitaly_invalid_commands_total",
+		Help: "Total number of invalid arguments tried to execute",
+	},
+	[]string{"command"},
+)
+
+func init() {
+	prometheus.MustRegister(invalidationTotal)
+}
+
+func incrInvalidArg(subcmdName string) {
+	invalidationTotal.WithLabelValues(subcmdName).Inc()
+}
+
+// SubCmd represents a specific git command
+type SubCmd struct {
+	Name        string   // e.g. "log", or "cat-file", or "worktree"
+	Flags       []Option // optional flags before the positional args
+	Args        []string // positional args after all flags
+	PostSepArgs []string // post separator (i.e. "--") positional args
+}
+
+var subCmdNameRegex = regexp.MustCompile(`^[[:alnum:]]+(-[[:alnum:]]+)*$`)
+
+// ValidateArgs checks all arguments in the sub command and validates them
+func (sc SubCmd) ValidateArgs() ([]string, error) {
+	var safeArgs []string
+
+	if !subCmdNameRegex.MatchString(sc.Name) {
+		return nil, &invalidArgErr{
+			msg: fmt.Sprintf("invalid sub command name %q", sc.Name),
+		}
+	}
+	safeArgs = append(safeArgs, sc.Name)
+
+	for _, o := range sc.Flags {
+		args, err := o.ValidateArgs()
+		if err != nil {
+			return nil, err
+		}
+		safeArgs = append(safeArgs, args...)
+	}
+
+	for _, a := range sc.Args {
+		if err := validatePositionalArg(a); err != nil {
+			return nil, err
+		}
+		safeArgs = append(safeArgs, a)
+	}
+
+	if len(sc.PostSepArgs) > 0 {
+		safeArgs = append(safeArgs, "--")
+	}
+
+	// post separator args do not need any validation
+	safeArgs = append(safeArgs, sc.PostSepArgs...)
+
+	return safeArgs, nil
+}
+
+// Option is a git command line flag with validation logic
+type Option interface {
+	IsOption()
+	ValidateArgs() ([]string, error)
+}
+
+// Flag is a single token optional command line argument that enables or
+// disables functionality (e.g. "-L")
+type Flag struct {
+	Name string
+}
+
+// IsOption is a method present on all Flag interface implementations
+func (Flag) IsOption() {}
+
+// ValidateArgs returns an error if the flag is not sanitary
+func (f Flag) ValidateArgs() ([]string, error) {
+	if !flagRegex.MatchString(f.Name) {
+		return nil, &invalidArgErr{
+			msg: fmt.Sprintf("flag %q failed regex validation", f.Name),
+		}
+	}
+	return []string{f.Name}, nil
+}
+
+// ValueFlag is an optional command line argument that is comprised of pair of
+// tokens (e.g. "-n 50")
+type ValueFlag struct {
+	Name  string
+	Value string
+}
+
+// IsOption is a method present on all Flag interface implementations
+func (ValueFlag) IsOption() {}
+
+// ValidateArgs returns an error if the flag is not sanitary
+func (vf ValueFlag) ValidateArgs() ([]string, error) {
+	if !flagRegex.MatchString(vf.Name) {
+		return nil, &invalidArgErr{
+			msg: fmt.Sprintf("value flag %q failed regex validation", vf.Name),
+		}
+	}
+	return []string{vf.Name, vf.Value}, nil
+}
+
+var flagRegex = regexp.MustCompile(`^(-|--)[[:alnum:]]`)
+
+type invalidArgErr struct {
+	msg string
+}
+
+func (iae *invalidArgErr) Error() string { return iae.msg }
+
+// IsInvalidArgErr relays if the error is due to an argument validation failure
+func IsInvalidArgErr(err error) bool {
+	_, ok := err.(*invalidArgErr)
+	return ok
+}
+
+func validatePositionalArg(arg string) error {
+	if strings.HasPrefix(arg, "-") {
+		return &invalidArgErr{
+			msg: fmt.Sprintf("positional arg %q cannot start with dash '-'", arg),
+		}
+	}
+	return nil
+}
+
+// SafeCmd creates a git.Command with the given args and Repository. It
+// validates the arguments in the command before executing.
+func SafeCmd(ctx context.Context, repo repository.GitRepo, globals []Option, sc SubCmd) (*command.Command, error) {
+	args, err := combineArgs(globals, sc)
+	if err != nil {
+		return nil, err
+	}
+
+	return Command(ctx, repo, args...)
+}
+
+// SafeBareCmd creates a git.Command with the given args, stdin/stdout/stderr,
+// and env. It validates the arguments in the command before executing.
+func SafeBareCmd(ctx context.Context, stdin io.Reader, stdout, stderr io.Writer, env []string, globals []Option, sc SubCmd) (*command.Command, error) {
+	args, err := combineArgs(globals, sc)
+	if err != nil {
+		return nil, err
+	}
+
+	return BareCommand(ctx, stdin, stdout, stderr, env, args...)
+}
+
+// SafeStdinCmd creates a git.Command with the given args and Repository that is
+// suitable for Write()ing to. It validates the arguments in the command before
+// executing.
+func SafeStdinCmd(ctx context.Context, repo repository.GitRepo, globals []Option, sc SubCmd) (*command.Command, error) {
+	args, err := combineArgs(globals, sc)
+	if err != nil {
+		return nil, err
+	}
+
+	return StdinCommand(ctx, repo, args...)
+}
+
+// SafeCmdWithoutRepo works like Command but without a git repository. It
+// validates the arugments in the command before executing.
+func SafeCmdWithoutRepo(ctx context.Context, globals []Option, sc SubCmd) (*command.Command, error) {
+	args, err := combineArgs(globals, sc)
+	if err != nil {
+		return nil, err
+	}
+
+	return CommandWithoutRepo(ctx, args...)
+}
+
+func combineArgs(globals []Option, sc SubCmd) (_ []string, err error) {
+	defer func() {
+		if err != nil && IsInvalidArgErr(err) {
+			incrInvalidArg(sc.Name)
+		}
+	}()
+
+	var args []string
+
+	for _, g := range globals {
+		gargs, err := g.ValidateArgs()
+		if err != nil {
+			return nil, err
+		}
+		args = append(args, gargs...)
+	}
+
+	scArgs, err := sc.ValidateArgs()
+	if err != nil {
+		return nil, err
+	}
+
+	return append(args, scArgs...), nil
+}

internal/git/safecmd_test.go

+package git_test
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/internal/git"
+	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
+	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
+)
+
+func TestFlagValidation(t *testing.T) {
+	for _, tt := range []struct {
+		option git.Option
+		valid  bool
+	}{
+		// valid Flag inputs
+		{option: git.Flag{"-k"}, valid: true},
+		{option: git.Flag{"-K"}, valid: true},
+		{option: git.Flag{"--asdf"}, valid: true},
+		{option: git.Flag{"--asdf-qwer"}, valid: true},
+		{option: git.Flag{"--asdf=qwerty"}, valid: true},
+		{option: git.Flag{"-D=A"}, valid: true},
+		{option: git.Flag{"-D="}, valid: true},
+
+		// valid ValueFlag inputs
+		{option: git.ValueFlag{"-k", "adsf"}, valid: true},
+		{option: git.ValueFlag{"-k", "--anything"}, valid: true},
+		{option: git.ValueFlag{"-k", ""}, valid: true},
+
+		// valid FlagCombo inputs
+
+		// invalid Flag inputs
+		{option: git.Flag{"-*"}},          // invalid character
+		{option: git.Flag{"a"}},           // missing dash
+		{option: git.Flag{"[["}},          // suspicious characters
+		{option: git.Flag{"||"}},          // suspicious characters
+		{option: git.Flag{"asdf=qwerty"}}, // missing dash
+
+		// invalid ValueFlag inputs
+		{option: git.ValueFlag{"k", "asdf"}}, // missing dash
+	} {
+		args, err := tt.option.ValidateArgs()
+		if tt.valid {
+			require.NoError(t, err)
+		} else {
+			require.Error(t, err,
+				"expected error, but args %v passed validation", args)
+			require.True(t, git.IsInvalidArgErr(err))
+		}
+	}
+}
+
+func TestSafeCmdInvalidArg(t *testing.T) {
+	for _, tt := range []struct {
+		globals []git.Option
+		subCmd  git.SubCmd
+		errMsg  string
+	}{
+		{
+			subCmd: git.SubCmd{Name: "--meow"},
+			errMsg: "invalid sub command name \"--meow\"",
+		},
+		{
+			subCmd: git.SubCmd{
+				Name:  "meow",
+				Flags: []git.Option{git.Flag{"woof"}},
+			},
+			errMsg: "flag \"woof\" failed regex validation",
+		},
+		{
+			subCmd: git.SubCmd{
+				Name: "meow",
+				Args: []string{"--tweet"},
+			},
+			errMsg: "positional arg \"--tweet\" cannot start with dash '-'",
+		},
+	} {
+		_, err := git.SafeCmd(
+			context.Background(),
+			&gitalypb.Repository{},
+			tt.globals,
+			tt.subCmd,
+		)
+		require.EqualError(t, err, tt.errMsg)
+		require.True(t, git.IsInvalidArgErr(err))
+	}
+}
+
+func TestSafeCmdValid(t *testing.T) {
+	testRepo, _, cleanup := testhelper.NewTestRepo(t)
+	defer cleanup()
+
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	for _, tt := range []struct {
+		globals    []git.Option
+		subCmd     git.SubCmd
+		expectArgs []string
+	}{
+		{
+			subCmd:     git.SubCmd{Name: "meow"},
+			expectArgs: []string{"meow"},
+		},
+		{
+			globals: []git.Option{
+				git.Flag{"--aaaa-bbbb"},
+			},
+			subCmd:     git.SubCmd{Name: "cccc"},
+			expectArgs: []string{"--aaaa-bbbb", "cccc"},
+		},
+		{
+			subCmd: git.SubCmd{
+				Name:        "meow",
+				Args:        []string{""},
+				PostSepArgs: []string{"-woof", ""},
+			},
+			expectArgs: []string{"meow", "", "--", "-woof", ""},
+		},
+		{
+			globals: []git.Option{
+				git.Flag{"-a"},
+				git.ValueFlag{"-b", "c"},
+			},
+			subCmd: git.SubCmd{
+				Name: "d",
+				Flags: []git.Option{
+					git.Flag{"-e"},
+					git.ValueFlag{"-f", "g"},
+					git.Flag{"-h=i"},
+				},
+				Args:        []string{"1", "2"},
+				PostSepArgs: []string{"3", "4", "5"},
+			},
+			expectArgs: []string{"-a", "-b", "c", "d", "-e", "-f", "g", "-h=i", "1", "2", "--", "3", "4", "5"},
+		},
+	} {
+		cmd, err := git.SafeCmd(ctx, testRepo, tt.globals, tt.subCmd)
+		require.NoError(t, err)
+		// ignore first 3 indeterministic args (executable path and repo args)
+		require.Equal(t, tt.expectArgs, cmd.Args()[3:])
+
+		cmd, err = git.SafeStdinCmd(ctx, testRepo, tt.globals, tt.subCmd)
+		require.NoError(t, err)
+		require.Equal(t, tt.expectArgs, cmd.Args()[3:])
+
+		cmd, err = git.SafeBareCmd(ctx, nil, nil, nil, nil, tt.globals, tt.subCmd)
+		require.NoError(t, err)
+		// ignore first indeterministic arg (executable path)
+		require.Equal(t, tt.expectArgs, cmd.Args()[1:])
+
+		cmd, err = git.SafeCmdWithoutRepo(ctx, tt.globals, tt.subCmd)
+		require.NoError(t, err)
+		require.Equal(t, tt.expectArgs, cmd.Args()[1:])
+	}
+}

internal/service/repository/gc.go

@@ -59,12 +59,17 @@ func gc(ctx context.Context, in *gitalypb.GarbageCollectRequest) error {
 
 	// run garbage collect and also write the commit graph
 	args = append(args,
-		"-c", "gc.writeCommitGraph=true",
-		"gc",
+		git.ValueFlag{"-c", "gc.writeCommitGraph=true"},
 	)
 
-	cmd, err := git.Command(ctx, in.GetRepository(), args...)
+	cmd, err := git.SafeCmd(ctx, in.GetRepository(), args,
+		git.SubCmd{Name: "gc"},
+	)
 	if err != nil {
+		if git.IsInvalidArgErr(err) {
+			return helper.ErrInvalidArgumentf("GarbageCollect: gitCommand: %v", err)
+		}
+
 		if _, ok := status.FromError(err); ok {
 			return err
 		}

internal/service/repository/repack.go

@@ -27,7 +27,12 @@ func init() {
 }
 
 func (server) RepackFull(ctx context.Context, in *gitalypb.RepackFullRequest) (*gitalypb.RepackFullResponse, error) {
-	if err := repackCommand(ctx, in.GetRepository(), in.GetCreateBitmap(), "-A", "--pack-kept-objects", "-l"); err != nil {
+	options := []git.Option{
+		git.Flag{"-A"},
+		git.Flag{"--pack-kept-objects"},
+		git.Flag{"-l"},
+	}
+	if err := repackCommand(ctx, in.GetRepository(), in.GetCreateBitmap(), options...); err != nil {
 		return nil, err
 	}
 	return &gitalypb.RepackFullResponse{}, nil
@@ -40,13 +45,14 @@ func (server) RepackIncremental(ctx context.Context, in *gitalypb.RepackIncremen
 	return &gitalypb.RepackIncrementalResponse{}, nil
 }
 
-func repackCommand(ctx context.Context, repo repository.GitRepo, bitmap bool, args ...string) error {
-	cmdArgs := repackConfig(ctx, bitmap)
-
-	cmdArgs = append(cmdArgs, "repack", "-d")
-	cmdArgs = append(cmdArgs, args...)
-
-	cmd, err := git.Command(ctx, repo, cmdArgs...)
+func repackCommand(ctx context.Context, repo repository.GitRepo, bitmap bool, args ...git.Option) error {
+	cmd, err := git.SafeCmd(ctx, repo,
+		repackConfig(ctx, bitmap), // global configs
+		git.SubCmd{
+			Name:  "repack",
+			Flags: append([]git.Option{git.Flag{"-d"}}, args...),
+		},
+	)
 	if err != nil {
 		if _, ok := status.FromError(err); ok {
 			return err
@@ -61,18 +67,18 @@ func repackCommand(ctx context.Context, repo repository.GitRepo, bitmap bool, ar
 	return nil
 }
 
-func repackConfig(ctx context.Context, bitmap bool) []string {
-	args := []string{
-		"-c", "pack.island=refs/heads",
-		"-c", "pack.island=refs/tags",
-		"-c", "repack.useDeltaIslands=true",
+func repackConfig(ctx context.Context, bitmap bool) []git.Option {
+	args := []git.Option{
+		git.ValueFlag{"-c", "pack.island=refs/heads"},
+		git.ValueFlag{"-c", "pack.island=refs/tags"},
+		git.ValueFlag{"-c", "repack.useDeltaIslands=true"},
 	}
 
 	if bitmap {
-		args = append(args, "-c", "repack.writeBitmaps=true")
-		args = append(args, "-c", "pack.writeBitmapHashCache=true")
+		args = append(args, git.ValueFlag{"-c", "repack.writeBitmaps=true"})
+		args = append(args, git.ValueFlag{"-c", "pack.writeBitmapHashCache=true"})
 	} else {
-		args = append(args, "-c", "repack.writeBitmaps=false")
+		args = append(args, git.ValueFlag{"-c", "repack.writeBitmaps=false"})
 	}
 
 	repackCounter.WithLabelValues(fmt.Sprint(bitmap)).Inc()