diff options
author | Nick Thomas <nick@gitlab.com> | 2019-09-18 17:42:19 +0100 |
---|---|---|
committer | Nick Thomas <nick@gitlab.com> | 2019-09-20 11:41:38 +0100 |
commit | 9237ac094a060dbb31c1ee4d37ad7ef38e17e878 (patch) | |
tree | 78f94a4db3f94f95d17974845b37aca984227a20 | |
parent | 996b2e1d44cc671cc60fd4ddacd2c5750b72a025 (diff) | |
download | gitlab-shell-9237ac094a060dbb31c1ee4d37ad7ef38e17e878.tar.gz |
Remove feature flags and the fallback command
-rw-r--r-- | go/cmd/gitlab-shell-authorized-keys-check/main.go | 2 | ||||
-rw-r--r-- | go/cmd/gitlab-shell-authorized-principals-check/main.go | 2 | ||||
-rw-r--r-- | go/cmd/gitlab-shell/main.go | 2 | ||||
-rw-r--r-- | go/internal/command/command.go | 16 | ||||
-rw-r--r-- | go/internal/command/command_test.go | 188 | ||||
-rw-r--r-- | go/internal/command/fallback/fallback.go | 57 | ||||
-rw-r--r-- | go/internal/command/fallback/fallback_test.go | 84 | ||||
-rw-r--r-- | go/internal/config/config.go | 20 | ||||
-rw-r--r-- | go/internal/config/config_test.go | 71 |
9 files changed, 70 insertions, 372 deletions
diff --git a/go/cmd/gitlab-shell-authorized-keys-check/main.go b/go/cmd/gitlab-shell-authorized-keys-check/main.go index 2bde63d..d8bb524 100644 --- a/go/cmd/gitlab-shell-authorized-keys-check/main.go +++ b/go/cmd/gitlab-shell-authorized-keys-check/main.go @@ -37,8 +37,6 @@ func main() { os.Exit(1) } - // The command will write to STDOUT on execution or replace the current - // process in case of the `fallback.Command` if err = cmd.Execute(); err != nil { fmt.Fprintf(readWriter.ErrOut, "%v\n", err) os.Exit(1) diff --git a/go/cmd/gitlab-shell-authorized-principals-check/main.go b/go/cmd/gitlab-shell-authorized-principals-check/main.go index 121cc17..5c9caf8 100644 --- a/go/cmd/gitlab-shell-authorized-principals-check/main.go +++ b/go/cmd/gitlab-shell-authorized-principals-check/main.go @@ -37,8 +37,6 @@ func main() { os.Exit(1) } - // The command will write to STDOUT on execution or replace the current - // process in case of the `fallback.Command` if err = cmd.Execute(); err != nil { fmt.Fprintf(readWriter.ErrOut, "%v\n", err) os.Exit(1) diff --git a/go/cmd/gitlab-shell/main.go b/go/cmd/gitlab-shell/main.go index 9b0b6c5..adb5198 100644 --- a/go/cmd/gitlab-shell/main.go +++ b/go/cmd/gitlab-shell/main.go @@ -37,8 +37,6 @@ func main() { os.Exit(1) } - // The command will write to STDOUT on execution or replace the current - // process in case of the `fallback.Command` if err = cmd.Execute(); err != nil { fmt.Fprintf(readWriter.ErrOut, "%v\n", err) os.Exit(1) diff --git a/go/internal/command/command.go b/go/internal/command/command.go index 071cd1d..40d92e0 100644 --- a/go/internal/command/command.go +++ b/go/internal/command/command.go @@ -5,10 +5,10 @@ import ( "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/authorizedprincipals" "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/commandargs" "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/discover" - "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/fallback" "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/lfsauthenticate" "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/readwriter" "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/receivepack" + "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/shared/disallowedcommand" "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/twofactorrecover" "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/uploadarchive" "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/uploadpack" @@ -30,7 +30,7 @@ func New(e *executable.Executable, arguments []string, config *config.Config, re return cmd, nil } - return &fallback.Command{Executable: e, RootDir: config.RootDir, Args: args}, nil + return nil, disallowedcommand.Error } func buildCommand(e *executable.Executable, args commandargs.CommandArgs, config *config.Config, readWriter *readwriter.ReadWriter) Command { @@ -47,10 +47,6 @@ func buildCommand(e *executable.Executable, args commandargs.CommandArgs, config } func buildShellCommand(args *commandargs.Shell, config *config.Config, readWriter *readwriter.ReadWriter) Command { - if !config.FeatureEnabled(string(args.CommandType)) { - return nil - } - switch args.CommandType { case commandargs.Discover: return &discover.Command{Config: config, Args: args, ReadWriter: readWriter} @@ -70,17 +66,9 @@ func buildShellCommand(args *commandargs.Shell, config *config.Config, readWrite } func buildAuthorizedKeysCommand(args *commandargs.AuthorizedKeys, config *config.Config, readWriter *readwriter.ReadWriter) Command { - if !config.FeatureEnabled(executable.AuthorizedKeysCheck) { - return nil - } - return &authorizedkeys.Command{Config: config, Args: args, ReadWriter: readWriter} } func buildAuthorizedPrincipalsCommand(args *commandargs.AuthorizedPrincipals, config *config.Config, readWriter *readwriter.ReadWriter) Command { - if !config.FeatureEnabled(executable.AuthorizedPrincipalsCheck) { - return nil - } - return &authorizedprincipals.Command{Config: config, Args: args, ReadWriter: readWriter} } diff --git a/go/internal/command/command_test.go b/go/internal/command/command_test.go index e7e37d8..59f22e2 100644 --- a/go/internal/command/command_test.go +++ b/go/internal/command/command_test.go @@ -1,6 +1,7 @@ package command import ( + "errors" "testing" "github.com/stretchr/testify/require" @@ -8,9 +9,9 @@ import ( "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/authorizedkeys" "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/authorizedprincipals" "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/discover" - "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/fallback" "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/lfsauthenticate" "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/receivepack" + "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/shared/disallowedcommand" "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/twofactorrecover" "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/uploadarchive" "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/uploadpack" @@ -19,154 +20,77 @@ import ( "gitlab.com/gitlab-org/gitlab-shell/go/internal/testhelper" ) +var ( + authorizedKeysExec = &executable.Executable{Name: executable.AuthorizedKeysCheck} + authorizedPrincipalsExec = &executable.Executable{Name: executable.AuthorizedPrincipalsCheck} + gitlabShellExec = &executable.Executable{Name: executable.GitlabShell} + + basicConfig = &config.Config{GitlabUrl: "http+unix://gitlab.socket"} +) + +func buildEnv(command string) map[string]string { + return map[string]string{ + "SSH_CONNECTION": "1", + "SSH_ORIGINAL_COMMAND": command, + } +} + func TestNew(t *testing.T) { testCases := []struct { desc string executable *executable.Executable - config *config.Config environment map[string]string arguments []string expectedType interface{} }{ { - desc: "it returns a Discover command if the feature is enabled", - executable: &executable.Executable{Name: executable.GitlabShell}, - config: &config.Config{ - GitlabUrl: "http+unix://gitlab.socket", - Migration: config.MigrationConfig{Enabled: true, Features: []string{"discover"}}, - }, - environment: map[string]string{ - "SSH_CONNECTION": "1", - "SSH_ORIGINAL_COMMAND": "", - }, - arguments: []string{}, + desc: "it returns a Discover command", + executable: gitlabShellExec, + environment: buildEnv(""), expectedType: &discover.Command{}, }, { - desc: "it returns a Fallback command no feature is enabled", - executable: &executable.Executable{Name: executable.GitlabShell}, - config: &config.Config{ - GitlabUrl: "http+unix://gitlab.socket", - Migration: config.MigrationConfig{Enabled: false}, - }, - environment: map[string]string{ - "SSH_CONNECTION": "1", - "SSH_ORIGINAL_COMMAND": "", - }, - arguments: []string{}, - expectedType: &fallback.Command{}, - }, - { - desc: "it returns a TwoFactorRecover command if the feature is enabled", - executable: &executable.Executable{Name: executable.GitlabShell}, - config: &config.Config{ - GitlabUrl: "http+unix://gitlab.socket", - Migration: config.MigrationConfig{Enabled: true, Features: []string{"2fa_recovery_codes"}}, - }, - environment: map[string]string{ - "SSH_CONNECTION": "1", - "SSH_ORIGINAL_COMMAND": "2fa_recovery_codes", - }, - arguments: []string{}, + desc: "it returns a TwoFactorRecover command", + executable: gitlabShellExec, + environment: buildEnv("2fa_recovery_codes"), expectedType: &twofactorrecover.Command{}, }, { - desc: "it returns an LfsAuthenticate command if the feature is enabled", - executable: &executable.Executable{Name: executable.GitlabShell}, - config: &config.Config{ - GitlabUrl: "http+unix://gitlab.socket", - Migration: config.MigrationConfig{Enabled: true, Features: []string{"git-lfs-authenticate"}}, - }, - environment: map[string]string{ - "SSH_CONNECTION": "1", - "SSH_ORIGINAL_COMMAND": "git-lfs-authenticate", - }, - arguments: []string{}, + desc: "it returns an LfsAuthenticate command", + executable: gitlabShellExec, + environment: buildEnv("git-lfs-authenticate"), expectedType: &lfsauthenticate.Command{}, }, { - desc: "it returns a ReceivePack command if the feature is enabled", - executable: &executable.Executable{Name: executable.GitlabShell}, - config: &config.Config{ - GitlabUrl: "http+unix://gitlab.socket", - Migration: config.MigrationConfig{Enabled: true, Features: []string{"git-receive-pack"}}, - }, - environment: map[string]string{ - "SSH_CONNECTION": "1", - "SSH_ORIGINAL_COMMAND": "git-receive-pack", - }, - arguments: []string{}, + desc: "it returns a ReceivePack command", + executable: gitlabShellExec, + environment: buildEnv("git-receive-pack"), expectedType: &receivepack.Command{}, }, { - desc: "it returns an UploadPack command if the feature is enabled", - executable: &executable.Executable{Name: executable.GitlabShell}, - config: &config.Config{ - GitlabUrl: "http+unix://gitlab.socket", - Migration: config.MigrationConfig{Enabled: true, Features: []string{"git-upload-pack"}}, - }, - environment: map[string]string{ - "SSH_CONNECTION": "1", - "SSH_ORIGINAL_COMMAND": "git-upload-pack", - }, - arguments: []string{}, + desc: "it returns an UploadPack command", + executable: gitlabShellExec, + environment: buildEnv("git-upload-pack"), expectedType: &uploadpack.Command{}, }, { - desc: "it returns an UploadArchive command if the feature is enabled", - executable: &executable.Executable{Name: executable.GitlabShell}, - config: &config.Config{ - GitlabUrl: "http+unix://gitlab.socket", - Migration: config.MigrationConfig{Enabled: true, Features: []string{"git-upload-archive"}}, - }, - environment: map[string]string{ - "SSH_CONNECTION": "1", - "SSH_ORIGINAL_COMMAND": "git-upload-archive", - }, - arguments: []string{}, + desc: "it returns an UploadArchive command", + executable: gitlabShellExec, + environment: buildEnv("git-upload-archive"), expectedType: &uploadarchive.Command{}, }, { - desc: "it returns a AuthorizedKeys command if the feature is enabled", - executable: &executable.Executable{Name: executable.AuthorizedKeysCheck}, - config: &config.Config{ - Migration: config.MigrationConfig{Enabled: true, Features: []string{"gitlab-shell-authorized-keys-check"}}, - }, - environment: map[string]string{}, + desc: "it returns a AuthorizedKeys command", + executable: authorizedKeysExec, arguments: []string{"git", "git", "key"}, expectedType: &authorizedkeys.Command{}, }, { - desc: "it returns a AuthorizedPrincipals command if the feature is enabled", - executable: &executable.Executable{Name: executable.AuthorizedPrincipalsCheck}, - config: &config.Config{ - Migration: config.MigrationConfig{Enabled: true, Features: []string{"gitlab-shell-authorized-principals-check"}}, - }, - environment: map[string]string{}, + desc: "it returns a AuthorizedPrincipals command", + executable: authorizedPrincipalsExec, arguments: []string{"key", "principal"}, expectedType: &authorizedprincipals.Command{}, }, - { - desc: "it returns a Fallback command if the feature is unimplemented", - executable: &executable.Executable{Name: executable.GitlabShell}, - config: &config.Config{ - GitlabUrl: "http+unix://gitlab.socket", - Migration: config.MigrationConfig{Enabled: true, Features: []string{"git-unimplemented-feature"}}, - }, - environment: map[string]string{ - "SSH_CONNECTION": "1", - "SSH_ORIGINAL_COMMAND": "git-unimplemented-feature", - }, - arguments: []string{}, - expectedType: &fallback.Command{}, - }, - { - desc: "it returns a Fallback command if executable is unknown", - executable: &executable.Executable{Name: "unknown"}, - config: &config.Config{}, - arguments: []string{}, - expectedType: &fallback.Command{}, - }, } for _, tc := range testCases { @@ -174,7 +98,7 @@ func TestNew(t *testing.T) { restoreEnv := testhelper.TempEnv(tc.environment) defer restoreEnv() - command, err := New(tc.executable, tc.arguments, tc.config, nil) + command, err := New(tc.executable, tc.arguments, basicConfig, nil) require.NoError(t, err) require.IsType(t, tc.expectedType, command) @@ -183,9 +107,33 @@ func TestNew(t *testing.T) { } func TestFailingNew(t *testing.T) { - t.Run("It returns an error parsing arguments failed", func(t *testing.T) { - _, err := New(&executable.Executable{Name: executable.GitlabShell}, []string{}, &config.Config{}, nil) + testCases := []struct { + desc string + executable *executable.Executable + environment map[string]string + expectedError error + }{ + { + desc: "Parsing environment failed", + executable: gitlabShellExec, + expectedError: errors.New("Only SSH allowed"), + }, + { + desc: "Unknown command given", + executable: gitlabShellExec, + environment: buildEnv("unknown"), + expectedError: disallowedcommand.Error, + }, + } - require.Error(t, err) - }) + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + restoreEnv := testhelper.TempEnv(tc.environment) + defer restoreEnv() + + command, err := New(tc.executable, []string{}, basicConfig, nil) + require.Nil(t, command) + require.Equal(t, tc.expectedError, err) + }) + } } diff --git a/go/internal/command/fallback/fallback.go b/go/internal/command/fallback/fallback.go deleted file mode 100644 index 781eda1..0000000 --- a/go/internal/command/fallback/fallback.go +++ /dev/null @@ -1,57 +0,0 @@ -package fallback - -import ( - "errors" - "fmt" - "os" - "path/filepath" - "syscall" - - "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/commandargs" - "gitlab.com/gitlab-org/gitlab-shell/go/internal/executable" -) - -type Command struct { - Executable *executable.Executable - RootDir string - Args commandargs.CommandArgs -} - -var ( - // execFunc is overridden in tests - execFunc = syscall.Exec - whitelist = []string{ - executable.GitlabShell, - executable.AuthorizedKeysCheck, - executable.AuthorizedPrincipalsCheck, - } -) - -func (c *Command) Execute() error { - if !c.isWhitelisted() { - return errors.New("Failed to execute unknown executable") - } - - rubyCmd := c.fallbackProgram() - - // Ensure rubyArgs[0] is the full path to gitlab-shell-ruby - rubyArgs := append([]string{rubyCmd}, c.Args.GetArguments()...) - - return execFunc(rubyCmd, rubyArgs, os.Environ()) -} - -func (c *Command) isWhitelisted() bool { - for _, item := range whitelist { - if c.Executable.Name == item { - return true - } - } - - return false -} - -func (c *Command) fallbackProgram() string { - fileName := fmt.Sprintf("%s-ruby", c.Executable.Name) - - return filepath.Join(c.RootDir, "bin", fileName) -} diff --git a/go/internal/command/fallback/fallback_test.go b/go/internal/command/fallback/fallback_test.go deleted file mode 100644 index 7485084..0000000 --- a/go/internal/command/fallback/fallback_test.go +++ /dev/null @@ -1,84 +0,0 @@ -package fallback - -import ( - "errors" - "os" - "testing" - - "github.com/stretchr/testify/require" - - "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/commandargs" - "gitlab.com/gitlab-org/gitlab-shell/go/internal/executable" -) - -type fakeExec struct { - OldExec func(string, []string, []string) error - Error error - Called bool - - Filename string - Args []string - Env []string -} - -var ( - fakeArgs = &commandargs.GenericArgs{Arguments: []string{"foo", "bar"}} -) - -func (f *fakeExec) Exec(filename string, args []string, env []string) error { - f.Called = true - - f.Filename = filename - f.Args = args - f.Env = env - - return f.Error -} - -func (f *fakeExec) Setup() { - f.OldExec = execFunc - execFunc = f.Exec -} - -func (f *fakeExec) Cleanup() { - execFunc = f.OldExec -} - -func TestExecuteExecsCommandSuccesfully(t *testing.T) { - cmd := &Command{Executable: &executable.Executable{Name: executable.GitlabShell}, RootDir: "/tmp", Args: fakeArgs} - - // Override the exec func - fake := &fakeExec{} - fake.Setup() - defer fake.Cleanup() - - require.NoError(t, cmd.Execute()) - require.True(t, fake.Called) - require.Equal(t, fake.Filename, "/tmp/bin/gitlab-shell-ruby") - require.Equal(t, fake.Args, []string{"/tmp/bin/gitlab-shell-ruby", "foo", "bar"}) - require.Equal(t, fake.Env, os.Environ()) -} - -func TestExecuteExecsUnknownExecutable(t *testing.T) { - cmd := &Command{Executable: &executable.Executable{Name: "unknown"}, RootDir: "/test"} - - require.Error(t, cmd.Execute()) -} - -func TestExecuteExecsCommandOnError(t *testing.T) { - cmd := &Command{Executable: &executable.Executable{Name: executable.GitlabShell}, RootDir: "/test", Args: fakeArgs} - - // Override the exec func - fake := &fakeExec{Error: errors.New("Test error")} - fake.Setup() - defer fake.Cleanup() - - require.Error(t, cmd.Execute()) - require.True(t, fake.Called) -} - -func TestExecuteGivenNonexistentCommand(t *testing.T) { - cmd := &Command{Executable: &executable.Executable{Name: executable.GitlabShell}, RootDir: "/tmp/does/not/exist", Args: fakeArgs} - - require.Error(t, cmd.Execute()) -} diff --git a/go/internal/config/config.go b/go/internal/config/config.go index d651744..2231851 100644 --- a/go/internal/config/config.go +++ b/go/internal/config/config.go @@ -16,11 +16,6 @@ const ( defaultSecretFileName = ".gitlab_shell_secret" ) -type MigrationConfig struct { - Enabled bool `yaml:"enabled"` - Features []string `yaml:"features"` -} - type HttpSettingsConfig struct { User string `yaml:"user"` Password string `yaml:"password"` @@ -34,7 +29,6 @@ type Config struct { RootDir string LogFile string `yaml:"log_file"` LogFormat string `yaml:"log_format"` - Migration MigrationConfig `yaml:"migration"` GitlabUrl string `yaml:"gitlab_url"` GitlabTracing string `yaml:"gitlab_tracing"` SecretFilePath string `yaml:"secret_file"` @@ -56,20 +50,6 @@ func NewFromDir(dir string) (*Config, error) { return newFromFile(path.Join(dir, configFile)) } -func (c *Config) FeatureEnabled(featureName string) bool { - if !c.Migration.Enabled { - return false - } - - for _, enabledFeature := range c.Migration.Features { - if enabledFeature == featureName { - return true - } - } - - return false -} - func newFromFile(filename string) (*Config, error) { cfg := &Config{RootDir: path.Dir(filename)} diff --git a/go/internal/config/config_test.go b/go/internal/config/config_test.go index aefc145..e31ff70 100644 --- a/go/internal/config/config_test.go +++ b/go/internal/config/config_test.go @@ -28,7 +28,6 @@ func TestParseConfig(t *testing.T) { path string format string gitlabUrl string - migration MigrationConfig secret string httpSettings HttpSettingsConfig }{ @@ -56,13 +55,6 @@ func TestParseConfig(t *testing.T) { secret: "default-secret-content", }, { - yaml: "migration:\n enabled: true\n features:\n - foo\n - bar", - path: path.Join(testRoot, "gitlab-shell.log"), - format: "text", - migration: MigrationConfig{Enabled: true, Features: []string{"foo", "bar"}}, - secret: "default-secret-content", - }, - { yaml: "gitlab_url: http+unix://%2Fpath%2Fto%2Fgitlab%2Fgitlab.socket", path: path.Join(testRoot, "gitlab-shell.log"), format: "text", @@ -110,8 +102,6 @@ func TestParseConfig(t *testing.T) { err := parseConfig([]byte(tc.yaml), &cfg) require.NoError(t, err) - assert.Equal(t, tc.migration.Enabled, cfg.Migration.Enabled, "migration.enabled not equal") - assert.Equal(t, tc.migration.Features, cfg.Migration.Features, "migration.features not equal") assert.Equal(t, tc.path, cfg.LogFile) assert.Equal(t, tc.format, cfg.LogFormat) assert.Equal(t, tc.gitlabUrl, cfg.GitlabUrl) @@ -120,64 +110,3 @@ func TestParseConfig(t *testing.T) { }) } } - -func TestFeatureEnabled(t *testing.T) { - testCases := []struct { - desc string - config *Config - feature string - expectEnabled bool - }{ - { - desc: "When the protocol is supported and the feature enabled", - config: &Config{ - GitlabUrl: "http+unix://gitlab.socket", - Migration: MigrationConfig{Enabled: true, Features: []string{"discover"}}, - }, - feature: "discover", - expectEnabled: true, - }, - { - desc: "When the protocol is supported and the feature is not enabled", - config: &Config{ - GitlabUrl: "http+unix://gitlab.socket", - Migration: MigrationConfig{Enabled: true, Features: []string{}}, - }, - feature: "discover", - expectEnabled: false, - }, - { - desc: "When the protocol is supported and all features are disabled", - config: &Config{ - GitlabUrl: "http+unix://gitlab.socket", - Migration: MigrationConfig{Enabled: false, Features: []string{"discover"}}, - }, - feature: "discover", - expectEnabled: false, - }, - { - desc: "When the protocol is http and the feature enabled", - config: &Config{ - GitlabUrl: "http://localhost:3000", - Migration: MigrationConfig{Enabled: true, Features: []string{"discover"}}, - }, - feature: "discover", - expectEnabled: true, - }, - { - desc: "When the protocol is https and the feature enabled", - config: &Config{ - GitlabUrl: "https://localhost:3000", - Migration: MigrationConfig{Enabled: true, Features: []string{"discover"}}, - }, - feature: "discover", - expectEnabled: true, - }, - } - - for _, tc := range testCases { - t.Run(tc.desc, func(t *testing.T) { - assert.Equal(t, tc.expectEnabled, tc.config.FeatureEnabled(string(tc.feature))) - }) - } -} |