diff options
author | John Cai <jcai@gitlab.com> | 2019-09-18 00:50:25 +0300 |
---|---|---|
committer | John Cai <jcai@gitlab.com> | 2019-09-18 00:50:25 +0300 |
commit | 068f49ce8e6565fad32e2e3660946f126382e3fc (patch) | |
tree | 49cd3f1e0c90d8cd61358613ec9da4590da51e21 | |
parent | 8ab5bd595984678838f3f09a96798b149e68a939 (diff) | |
parent | a77752502e520d4e2c16f7b2a95068a4f1c2d9f3 (diff) |
Merge branch 'po-git-dsl' into 'master'
Git command DSL
Closes #1996, #1991, and #1847
See merge request gitlab-org/gitaly!1476
-rw-r--r-- | STYLE.md | 21 | ||||
-rw-r--r-- | changelogs/unreleased/po-git-dsl.yml | 5 | ||||
-rw-r--r-- | internal/command/command.go | 5 | ||||
-rw-r--r-- | internal/git/safecmd.go | 213 | ||||
-rw-r--r-- | internal/git/safecmd_test.go | 158 | ||||
-rw-r--r-- | internal/service/repository/gc.go | 11 | ||||
-rw-r--r-- | internal/service/repository/repack.go | 38 |
7 files changed, 432 insertions, 19 deletions
@@ -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 + diff --git a/changelogs/unreleased/po-git-dsl.yml b/changelogs/unreleased/po-git-dsl.yml new file mode 100644 index 000000000..7e8aefd74 --- /dev/null +++ b/changelogs/unreleased/po-git-dsl.yml @@ -0,0 +1,5 @@ +--- +title: Git Command DSL +merge_request: 1476 +author: +type: other diff --git a/internal/command/command.go b/internal/command/command.go index 3a7f3aebf..7e7fbdaa3 100644 --- a/internal/command/command.go +++ b/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 +} diff --git a/internal/git/safecmd.go b/internal/git/safecmd.go new file mode 100644 index 000000000..352ee2ffe --- /dev/null +++ b/internal/git/safecmd.go @@ -0,0 +1,213 @@ +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 +} diff --git a/internal/git/safecmd_test.go b/internal/git/safecmd_test.go new file mode 100644 index 000000000..e646c7d4d --- /dev/null +++ b/internal/git/safecmd_test.go @@ -0,0 +1,158 @@ +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:]) + } +} diff --git a/internal/service/repository/gc.go b/internal/service/repository/gc.go index ad1fc45aa..f1fd830c8 100644 --- a/internal/service/repository/gc.go +++ b/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 } diff --git a/internal/service/repository/repack.go b/internal/service/repository/repack.go index 4e55be085..6a72b1933 100644 --- a/internal/service/repository/repack.go +++ b/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() |