RFC: Simple built-in SSH server

cmd/check/main.go

@@ -24,7 +24,7 @@ func main() {
 		os.Exit(1)
 	}
 
-	config, err := config.NewFromDir(executable.RootDir)
+	config, err := config.NewFromDirExternal(executable.RootDir)
 	if err != nil {
 		fmt.Fprintln(readWriter.ErrOut, "Failed to read config, exiting")
 		os.Exit(1)

cmd/gitlab-shell-authorized-keys-check/main.go

@@ -25,7 +25,7 @@ func main() {
 		os.Exit(1)
 	}
 
-	config, err := config.NewFromDir(executable.RootDir)
+	config, err := config.NewFromDirExternal(executable.RootDir)
 	if err != nil {
 		fmt.Fprintln(readWriter.ErrOut, "Failed to read config, exiting")
 		os.Exit(1)

cmd/gitlab-shell-authorized-principals-check/main.go

@@ -25,7 +25,7 @@ func main() {
 		os.Exit(1)
 	}
 
-	config, err := config.NewFromDir(executable.RootDir)
+	config, err := config.NewFromDirExternal(executable.RootDir)
 	if err != nil {
 		fmt.Fprintln(readWriter.ErrOut, "Failed to read config, exiting")
 		os.Exit(1)

cmd/gitlab-shell/main.go

@@ -39,7 +39,7 @@ func main() {
 		os.Exit(1)
 	}
 
-	config, err := config.NewFromDir(executable.RootDir)
+	config, err := config.NewFromDirExternal(executable.RootDir)
 	if err != nil {
 		fmt.Fprintln(readWriter.ErrOut, "Failed to read config, exiting")
 		os.Exit(1)

cmd/gitlab-sshd/Dockerfile

+FROM gcr.io/distroless/static-debian10
+COPY gitlab-sshd /gitlab-sshd
+CMD ["/gitlab-sshd"]
\ No newline at end of file

cmd/gitlab-sshd/main.go

+package main
+
+import (
+	"flag"
+	"os"
+
+	log "github.com/sirupsen/logrus"
+
+	"gitlab.com/gitlab-org/gitlab-shell/internal/config"
+	"gitlab.com/gitlab-org/gitlab-shell/internal/logger"
+	"gitlab.com/gitlab-org/gitlab-shell/internal/sshd"
+)
+
+var (
+	configDir = flag.String("config-dir", "", "The directory the config is in")
+)
+
+func overrideConfigFromEnvironment(cfg *config.Config) {
+	if gitlabUrl := os.Getenv("GITLAB_URL"); gitlabUrl != "" {
+		cfg.GitlabUrl = gitlabUrl
+	}
+	if gitlabTracing := os.Getenv("GITLAB_TRACING"); gitlabTracing != "" {
+		cfg.GitlabTracing = gitlabTracing
+	}
+	if gitlabShellSecret := os.Getenv("GITLAB_SHELL_SECRET"); gitlabShellSecret != "" {
+		cfg.Secret = gitlabShellSecret
+	}
+	if gitlabLogFormat := os.Getenv("GITLAB_LOG_FORMAT"); gitlabLogFormat != "" {
+		cfg.LogFormat = gitlabLogFormat
+	}
+	return
+}
+
+func main() {
+	flag.Parse()
+	cfg := new(config.Config)
+	if *configDir != "" {
+		var err error
+		cfg, err = config.NewFromDir(*configDir)
+		if err != nil {
+			log.Fatalf("failed to load configuration from specified directory: %v", err)
+		}
+	}
+	overrideConfigFromEnvironment(cfg)
+	cfg.ApplyServerDefaults()
+	if err := cfg.IsSane(); err != nil {
+		if *configDir == "" {
+			log.Warn("note: no config-dir provided, using only environment variables")
+		}
+		log.Fatalf("configuration error: %v", err)
+	}
+	logger.ConfigureStandalone(cfg)
+
+	if err := sshd.Run(cfg); err != nil {
+		log.Fatalf("Failed to start GitLab built-in sshd: %v", err)
+	}
+}

config.yml.example

@@ -61,3 +61,15 @@ audit_usernames: false
 # Distributed Tracing. GitLab-Shell has distributed tracing instrumentation.
 # For more details, visit https://docs.gitlab.com/ee/development/distributed_tracing.html
 # gitlab_tracing: opentracing://driver
+
+# This section configures the built-in SSH server. Ignored when running on OpenSSH.
+sshd:
+  # Address which the SSH server listens on. Defaults to [::]:22.
+  listen: "[::]:22"
+  # Maximum number of concurrent sessions allowed on a single SSH connection. Defaults to 10.
+  concurrent_sessions_limit: 10
+  # SSH host key files. 
+  host_key_files:
+    - /run/secrets/ssh-hostkeys/ssh_host_rsa_key
+    - /run/secrets/ssh-hostkeys/ssh_host_ecdsa_key
+    - /run/secrets/ssh-hostkeys/ssh_host_ed25519_key
\ No newline at end of file

go.mod

@@ -9,6 +9,8 @@ require (
 	github.com/stretchr/testify v1.4.0
 	gitlab.com/gitlab-org/gitaly v1.68.0
 	gitlab.com/gitlab-org/labkit v0.0.0-20200908084045-45895e129029
+	golang.org/x/crypto v0.0.0-20201216223049-8b5274cf687f
+	golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e
 	google.golang.org/grpc v1.24.0
 	gopkg.in/yaml.v2 v2.2.8
 )

go.sum

@@ -337,6 +337,10 @@ golang.org/x/crypto v0.0.0-20190621222207-cc06ce4a13d4/go.mod h1:yigFU9vqHzYiE8U
 golang.org/x/crypto v0.0.0-20190701094942-4def268fd1a4/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI=
 golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550 h1:ObdrDkeb4kJdCP557AjRjq69pTHfNouLtWZG7j9rPN8=
 golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI=
+golang.org/x/crypto v0.0.0-20200323165209-0ec3e9974c59 h1:3zb4D3T4G8jdExgVU/95+vQXfpEPiMdCaZgmGVxjNHM=
+golang.org/x/crypto v0.0.0-20200323165209-0ec3e9974c59/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto=
+golang.org/x/crypto v0.0.0-20201216223049-8b5274cf687f h1:aZp0e2vLN4MToVqnjNEYEtrEA8RH8U8FN1CU7JgqsPU=
+golang.org/x/crypto v0.0.0-20201216223049-8b5274cf687f/go.mod h1:jdWPYTVW3xRLrWPugEBEK3UY2ZEsg3UU495nc5E+M+I=
 golang.org/x/exp v0.0.0-20190121172915-509febef88a4/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA=
 golang.org/x/exp v0.0.0-20190306152737-a1d7652674e8/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA=
 golang.org/x/exp v0.0.0-20190510132918-efd6b22b2522/go.mod h1:ZjyILWgesfNpC6sMxTJOJm9Kp84zZh5NQWvqDGG3Qr8=
@@ -410,9 +414,11 @@ golang.org/x/sys v0.0.0-20190624142023-c5567b49c5d0/go.mod h1:h1NjWce9XRLGQEsW7w
 golang.org/x/sys v0.0.0-20190626221950-04f50cda93cb/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20190726091711-fc99dfbffb4e/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20190813064441-fde4db37ae7a/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
+golang.org/x/sys v0.0.0-20191026070338-33540a1f6037/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20191204072324-ce4227a45e2e/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20200113162924-86b910548bc1 h1:gZpLHxUX5BdYLA08Lj4YCJNN/jk7KtquiArPoeX0WvA=
 golang.org/x/sys v0.0.0-20200113162924-86b910548bc1/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
+golang.org/x/term v0.0.0-20201117132131-f5c789dd3221/go.mod h1:Nr5EML6q2oocZ2LXRh80K7BxOlk5/8JxuGnuhpl+muw=
 golang.org/x/text v0.3.0 h1:g61tztE5qeGQ89tm6NTjjM9VPIm088od1l6aSorWRWg=
 golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
 golang.org/x/text v0.3.1-0.20180807135948-17ff2d5776d2/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=

internal/command/command.go

@@ -70,7 +70,7 @@ func ContextWithCorrelationID() (context.Context, func()) {
 func buildCommand(e *executable.Executable, args commandargs.CommandArgs, config *config.Config, readWriter *readwriter.ReadWriter) Command {
 	switch e.Name {
 	case executable.GitlabShell:
-		return buildShellCommand(args.(*commandargs.Shell), config, readWriter)
+		return BuildShellCommand(args.(*commandargs.Shell), config, readWriter)
 	case executable.AuthorizedKeysCheck:
 		return buildAuthorizedKeysCommand(args.(*commandargs.AuthorizedKeys), config, readWriter)
 	case executable.AuthorizedPrincipalsCheck:
@@ -82,7 +82,7 @@ func buildCommand(e *executable.Executable, args commandargs.CommandArgs, config
 	return nil
 }
 
-func buildShellCommand(args *commandargs.Shell, config *config.Config, readWriter *readwriter.ReadWriter) Command {
+func BuildShellCommand(args *commandargs.Shell, config *config.Config, readWriter *readwriter.ReadWriter) Command {
 	switch args.CommandType {
 	case commandargs.Discover:
 		return &discover.Command{Config: config, Args: args, ReadWriter: readWriter}

internal/command/commandargs/shell.go

@@ -2,6 +2,7 @@ package commandargs
 
 import (
 	"errors"
+	"net"
 	"os"
 	"regexp"
 
@@ -32,6 +33,10 @@ type Shell struct {
 	GitlabKeyId    string
 	SshArgs        []string
 	CommandType    CommandType
+
+	// Only set when running standalone
+	RemoteAddr         *net.TCPAddr
+	GitProtocolVersion string
 }
 
 func (s *Shell) Parse() error {
@@ -40,7 +45,6 @@ func (s *Shell) Parse() error {
 	}
 
 	s.parseWho()
-	s.defineCommandType()
 
 	return nil
 }
@@ -67,7 +71,7 @@ func (s *Shell) isSshConnection() bool {
 }
 
 func (s *Shell) isValidSshCommand() bool {
-	err := s.parseCommand(os.Getenv("SSH_ORIGINAL_COMMAND"))
+	err := s.ParseCommand(os.Getenv("SSH_ORIGINAL_COMMAND"))
 	return err == nil
 }
 
@@ -107,7 +111,7 @@ func tryParseUsername(argument string) string {
 	return ""
 }
 
-func (s *Shell) parseCommand(commandString string) error {
+func (s *Shell) ParseCommand(commandString string) error {
 	args, err := shellwords.Parse(commandString)
 	if err != nil {
 		return err
@@ -123,6 +127,8 @@ func (s *Shell) parseCommand(commandString string) error {
 
 	s.SshArgs = args
 
+	s.defineCommandType()
+
 	return nil
 }
 

internal/command/receivepack/gitalycall.go

@@ -2,7 +2,6 @@ package receivepack
 
 import (
 	"context"
-	"os"
 
 	"google.golang.org/grpc"
 
@@ -13,7 +12,7 @@ import (
 	"gitlab.com/gitlab-org/gitlab-shell/internal/handler"
 )
 
-func (c *Command) performGitalyCall(response *accessverifier.Response) error {
+func (c *Command) performGitalyCall(response *accessverifier.Response, gitProtocolVersion string) error {
 	gc := &handler.GitalyCommand{
 		Config:      c.Config,
 		ServiceName: string(commandargs.ReceivePack),
@@ -27,7 +26,7 @@ func (c *Command) performGitalyCall(response *accessverifier.Response) error {
 		GlId:             response.Who,
 		GlRepository:     response.Repo,
 		GlUsername:       response.Username,
-		GitProtocol:      os.Getenv(commandargs.GitProtocolEnv),
+		GitProtocol:      gitProtocolVersion,
 		GitConfigOptions: response.GitConfigOptions,
 	}
 

internal/command/receivepack/receivepack.go

@@ -2,6 +2,7 @@ package receivepack
 
 import (
 	"context"
+	"os"
 
 	"gitlab.com/gitlab-org/gitlab-shell/internal/command/commandargs"
 	"gitlab.com/gitlab-org/gitlab-shell/internal/command/readwriter"
@@ -38,7 +39,14 @@ func (c *Command) Execute(ctx context.Context) error {
 		return customAction.Execute(ctx, response)
 	}
 
-	return c.performGitalyCall(response)
+	var gitProtocolVersion string
+	if c.Args.RemoteAddr != nil {
+		gitProtocolVersion = c.Args.GitProtocolVersion
+	} else {
+		gitProtocolVersion = os.Getenv(commandargs.GitProtocolEnv)
+	}
+
+	return c.performGitalyCall(response, gitProtocolVersion)
 }
 
 func (c *Command) verifyAccess(ctx context.Context, repo string) (*accessverifier.Response, error) {

internal/command/uploadpack/gitalycall.go

@@ -2,7 +2,6 @@ package uploadpack
 
 import (
 	"context"
-	"os"
 
 	"google.golang.org/grpc"
 
@@ -13,7 +12,7 @@ import (
 	"gitlab.com/gitlab-org/gitlab-shell/internal/handler"
 )
 
-func (c *Command) performGitalyCall(response *accessverifier.Response) error {
+func (c *Command) performGitalyCall(response *accessverifier.Response, gitProtocolVersion string) error {
 	gc := &handler.GitalyCommand{
 		Config:      c.Config,
 		ServiceName: string(commandargs.UploadPack),
@@ -24,7 +23,7 @@ func (c *Command) performGitalyCall(response *accessverifier.Response) error {
 
 	request := &pb.SSHUploadPackRequest{
 		Repository:       &response.Gitaly.Repo,
-		GitProtocol:      os.Getenv(commandargs.GitProtocolEnv),
+		GitProtocol:      gitProtocolVersion,
 		GitConfigOptions: response.GitConfigOptions,
 	}
 

internal/command/uploadpack/uploadpack.go

@@ -2,6 +2,7 @@ package uploadpack
 
 import (
 	"context"
+	"os"
 
 	"gitlab.com/gitlab-org/gitlab-shell/internal/command/commandargs"
 	"gitlab.com/gitlab-org/gitlab-shell/internal/command/readwriter"
@@ -38,7 +39,14 @@ func (c *Command) Execute(ctx context.Context) error {
 		return customAction.Execute(ctx, response)
 	}
 
-	return c.performGitalyCall(response)
+	var gitProtocolVersion string
+	if c.Args.RemoteAddr != nil {
+		gitProtocolVersion = c.Args.GitProtocolVersion
+	} else {
+		gitProtocolVersion = os.Getenv(commandargs.GitProtocolEnv)
+	}
+
+	return c.performGitalyCall(response, gitProtocolVersion)
 }
 
 func (c *Command) verifyAccess(ctx context.Context, repo string) (*accessverifier.Response, error) {

internal/config/config.go

 package config
 
 import (
+	"errors"
 	"io/ioutil"
 	"net/url"
-	"os"
 	"path"
 	"path/filepath"
 
@@ -17,6 +17,12 @@ const (
 	defaultSecretFileName = ".gitlab_shell_secret"
 )
 
+type ServerConfig struct {
+	Listen                  string   `yaml:"listen"`
+	ConcurrentSessionsLimit int64    `yaml:"concurrent_sessions_limit"`
+	HostKeyFiles            []string `yaml:"host_key_files"`
+}
+
 type HttpSettingsConfig struct {
 	User               string `yaml:"user"`
 	Password           string `yaml:"password"`
@@ -27,17 +33,20 @@ type HttpSettingsConfig struct {
 }
 
 type Config struct {
+	User                  string `yaml:"user"`
 	RootDir               string
-	LogFile               string             `yaml:"log_file"`
-	LogFormat             string             `yaml:"log_format"`
-	GitlabUrl             string             `yaml:"gitlab_url"`
-	GitlabRelativeURLRoot string             `yaml:"gitlab_relative_url_root"`
-	GitlabTracing         string             `yaml:"gitlab_tracing"`
-	SecretFilePath        string             `yaml:"secret_file"`
-	Secret                string             `yaml:"secret"`
-	SslCertDir            string             `yaml:"ssl_cert_dir"`
-	HttpSettings          HttpSettingsConfig `yaml:"http_settings"`
-	HttpClient            *client.HttpClient `-`
+	LogFile               string `yaml:"log_file"`
+	LogFormat             string `yaml:"log_format"`
+	GitlabUrl             string `yaml:"gitlab_url"`
+	GitlabRelativeURLRoot string `yaml:"gitlab_relative_url_root"`
+	GitlabTracing         string `yaml:"gitlab_tracing"`
+	// SecretFilePath is only for parsing. Application code should always use Secret.
+	SecretFilePath string             `yaml:"secret_file"`
+	Secret         string             `yaml:"secret"`
+	SslCertDir     string             `yaml:"ssl_cert_dir"`
+	HttpSettings   HttpSettingsConfig `yaml:"http_settings"`
+	Server         ServerConfig       `yaml:"sshd"`
+	HttpClient     *client.HttpClient `-`
 }
 
 func (c *Config) GetHttpClient() *client.HttpClient {
@@ -58,66 +67,52 @@ func (c *Config) GetHttpClient() *client.HttpClient {
 	return client
 }
 
-func New() (*Config, error) {
-	dir, err := os.Getwd()
+// NewFromDirExternal returns a new config from a given root dir. It also applies defaults appropriate for
+// gitlab-shell running in an external SSH server.
+func NewFromDirExternal(dir string) (*Config, error) {
+	cfg, err := newFromFile(filepath.Join(dir, configFile))
 	if err != nil {
 		return nil, err
 	}
-
-	return NewFromDir(dir)
+	cfg.ApplyExternalDefaults()
+	return cfg, nil
 }
 
+// NewFromDir returns a new config given a root directory. It looks for the config file name in the
+// given directory and reads the config from it. It doesn't apply any defaults. New code should prefer
+// this over NewFromDirIntegrated and apply the right default via one of the Apply... functions.
 func NewFromDir(dir string) (*Config, error) {
-	return newFromFile(path.Join(dir, configFile))
+	return newFromFile(filepath.Join(dir, configFile))
 }
 
-func newFromFile(filename string) (*Config, error) {
-	cfg := &Config{RootDir: path.Dir(filename)}
+// newFromFile reads a new Config instance from the given file path. It doesn't apply any defaults.
+func newFromFile(path string) (*Config, error) {
+	cfg := &Config{RootDir: filepath.Dir(path)}
 
-	configBytes, err := ioutil.ReadFile(filename)
+	configBytes, err := ioutil.ReadFile(path)
 	if err != nil {
 		return nil, err
 	}
 
-	if err := parseConfig(configBytes, cfg); err != nil {
-		return nil, err
-	}
-
-	return cfg, nil
-}
-
-// parseConfig expects YAML data in configBytes and a Config instance with RootDir set.
-func parseConfig(configBytes []byte, cfg *Config) error {
 	if err := yaml.Unmarshal(configBytes, cfg); err != nil {
-		return err
-	}
-
-	if cfg.LogFile == "" {
-		cfg.LogFile = logFile
-	}
-
-	if len(cfg.LogFile) > 0 && cfg.LogFile[0] != '/' {
-		cfg.LogFile = path.Join(cfg.RootDir, cfg.LogFile)
-	}
-
-	if cfg.LogFormat == "" {
-		cfg.LogFormat = "text"
+		return nil, err
 	}
 
 	if cfg.GitlabUrl != "" {
+		// This is only done for historic reasons, don't implement it for new config sources.
 		unescapedUrl, err := url.PathUnescape(cfg.GitlabUrl)
 		if err != nil {
-			return err
+			return nil, err
 		}
 
 		cfg.GitlabUrl = unescapedUrl
 	}
 
 	if err := parseSecret(cfg); err != nil {
-		return err
+		return nil, err
 	}
 
-	return nil
+	return cfg, nil
 }
 
 func parseSecret(cfg *Config) error {
@@ -142,3 +137,58 @@ func parseSecret(cfg *Config) error {
 
 	return nil
 }
+
+// ApplyServerDefaults applies defaults running inside an external SSH server.
+func (cfg *Config) ApplyExternalDefaults() {
+	// Set default LogFile to a file since with an external SSH server stdout is not a possibility.
+	if cfg.LogFile == "" {
+		cfg.LogFile = logFile
+	}
+	cfg.applyGenericDefaults()
+}
+
+// applyGenericDefaults applies defaults common to all operating modes.
+func (cfg *Config) applyGenericDefaults() {
+	if cfg.LogFormat == "" {
+		cfg.LogFormat = "text"
+	}
+	// Currently only used by the built-in SSH server, but not specific to it, so let's to it here.
+	if cfg.User == "" {
+		cfg.User = "git"
+	}
+	if len(cfg.LogFile) > 0 && cfg.LogFile[0] != '/' && cfg.RootDir != "" {
+		cfg.LogFile = filepath.Join(cfg.RootDir, cfg.LogFile)
+	}
+}
+
+// ApplyServerDefaults applies defaults for the built-in SSH server.
+func (cfg *Config) ApplyServerDefaults() {
+	if cfg.Server.ConcurrentSessionsLimit == 0 {
+		cfg.Server.ConcurrentSessionsLimit = 10
+	}
+	if cfg.Server.Listen == "" {
+		cfg.Server.Listen = "[::]:22"
+	}
+	if len(cfg.Server.HostKeyFiles) == 0 {
+		cfg.Server.HostKeyFiles = []string{
+			"/run/secrets/ssh-hostkeys/ssh_host_rsa_key",
+			"/run/secrets/ssh-hostkeys/ssh_host_ecdsa_key",
+			"/run/secrets/ssh-hostkeys/ssh_host_ed25519_key",
+		}
+	}
+	cfg.applyGenericDefaults()
+}
+
+// IsSane checks if the given config fulfills the minimum requirements to be able to run.
+// Any error returned by this function should be a startup error. On the other hand
+// if this function returns nil, this doesn't guarantee the config will work, but it's
+// at least worth a try.
+func (cfg *Config) IsSane() error {
+	if cfg.GitlabUrl == "" {
+		return errors.New("gitlab_url is required")
+	}
+	if cfg.Secret == "" {
+		return errors.New("secret or secret_file_path is required")
+	}
+	return nil
+}

internal/config/config_test.go

-package config
-
-import (
-	"fmt"
-	"path"
-	"testing"
-
-	"github.com/stretchr/testify/require"
-	"gitlab.com/gitlab-org/gitlab-shell/internal/testhelper"
-)
-
-const (
-	customSecret = "custom/my-contents-is-secret"
-)
-
-var (
-	testRoot = testhelper.TestRoot
-)
-
-func TestParseConfig(t *testing.T) {
-	cleanup, err := testhelper.PrepareTestRootDir()
-	require.NoError(t, err)
-	defer cleanup()
-
-	testCases := []struct {
-		yaml         string
-		path         string
-		format       string
-		gitlabUrl    string
-		secret       string
-		sslCertDir   string
-		httpSettings HttpSettingsConfig
-	}{
-		{
-			path:   path.Join(testRoot, "gitlab-shell.log"),
-			format: "text",
-			secret: "default-secret-content",
-		},
-		{
-			yaml:   "log_file: my-log.log",
-			path:   path.Join(testRoot, "my-log.log"),
-			format: "text",
-			secret: "default-secret-content",
-		},
-		{
-			yaml:   "log_file: /qux/my-log.log",
-			path:   "/qux/my-log.log",
-			format: "text",
-			secret: "default-secret-content",
-		},
-		{
-			yaml:   "log_format: json",
-			path:   path.Join(testRoot, "gitlab-shell.log"),
-			format: "json",
-			secret: "default-secret-content",
-		},
-		{
-			yaml:      "gitlab_url: http+unix://%2Fpath%2Fto%2Fgitlab%2Fgitlab.socket",
-			path:      path.Join(testRoot, "gitlab-shell.log"),
-			format:    "text",
-			gitlabUrl: "http+unix:///path/to/gitlab/gitlab.socket",
-			secret:    "default-secret-content",
-		},
-		{
-			yaml:   fmt.Sprintf("secret_file: %s", customSecret),
-			path:   path.Join(testRoot, "gitlab-shell.log"),
-			format: "text",
-			secret: "custom-secret-content",
-		},
-		{
-			yaml:   fmt.Sprintf("secret_file: %s", path.Join(testRoot, customSecret)),
-			path:   path.Join(testRoot, "gitlab-shell.log"),
-			format: "text",
-			secret: "custom-secret-content",
-		},
-		{
-			yaml:   "secret: an inline secret",
-			path:   path.Join(testRoot, "gitlab-shell.log"),
-			format: "text",
-			secret: "an inline secret",
-		},
-		{
-			yaml:       "ssl_cert_dir: /tmp/certs",
-			path:       path.Join(testRoot, "gitlab-shell.log"),
-			format:     "text",
-			secret:     "default-secret-content",
-			sslCertDir: "/tmp/certs",
-		},
-		{
-			yaml:         "http_settings:\n  user: user_basic_auth\n  password: password_basic_auth\n  read_timeout: 500",
-			path:         path.Join(testRoot, "gitlab-shell.log"),
-			format:       "text",
-			secret:       "default-secret-content",
-			httpSettings: HttpSettingsConfig{User: "user_basic_auth", Password: "password_basic_auth", ReadTimeoutSeconds: 500},
-		},
-		{
-			yaml:         "http_settings:\n  ca_file: /etc/ssl/cert.pem\n  ca_path: /etc/pki/tls/certs\n  self_signed_cert: true",
-			path:         path.Join(testRoot, "gitlab-shell.log"),
-			format:       "text",
-			secret:       "default-secret-content",
-			httpSettings: HttpSettingsConfig{CaFile: "/etc/ssl/cert.pem", CaPath: "/etc/pki/tls/certs", SelfSignedCert: true},
-		},
-	}
-
-	for _, tc := range testCases {
-		t.Run(fmt.Sprintf("yaml input: %q", tc.yaml), func(t *testing.T) {
-			cfg := Config{RootDir: testRoot}
-
-			err := parseConfig([]byte(tc.yaml), &cfg)
-			require.NoError(t, err)
-
-			require.Equal(t, tc.path, cfg.LogFile)
-			require.Equal(t, tc.format, cfg.LogFormat)
-			require.Equal(t, tc.gitlabUrl, cfg.GitlabUrl)
-			require.Equal(t, tc.secret, cfg.Secret)
-			require.Equal(t, tc.sslCertDir, cfg.SslCertDir)
-			require.Equal(t, tc.httpSettings, cfg.HttpSettings)
-		})
-	}
-}

internal/gitlabnet/accessverifier/client.go

@@ -87,7 +87,11 @@ func (c *Client) Verify(ctx context.Context, args *commandargs.Shell, action com
 		request.KeyId = args.GitlabKeyId
 	}
 
-	request.CheckIp = sshenv.LocalAddr()
+	if args.RemoteAddr != nil {
+		request.CheckIp = args.RemoteAddr.IP.String()
+	} else {
+		request.CheckIp = sshenv.LocalAddr()
+	}
 
 	response, err := c.client.Post(ctx, "/allowed", request)
 	if err != nil {

internal/logger/logger.go

 package logger
 
 import (
-	"fmt"
-	"io"
 	"io/ioutil"
-	golog "log"
 	"log/syslog"
-	"math"
 	"os"
-	"sync"
-	"time"
-
-	"gitlab.com/gitlab-org/gitlab-shell/internal/config"
 
 	log "github.com/sirupsen/logrus"
+	"gitlab.com/gitlab-org/gitlab-shell/internal/config"
 )
 
-var (
-	logWriter       io.Writer
-	bootstrapLogger *golog.Logger
-	pid             int
-	mutex           sync.Mutex
-	ProgName        string
-)
-
-func Configure(cfg *config.Config) error {
-	mutex.Lock()
-	defer mutex.Unlock()
-
-	pid = os.Getpid()
-	ProgName, _ = os.Executable()
-
-	// Avoid leaking output if we can't set up the logging output
-	log.SetOutput(ioutil.Discard)
-
-	output, err := os.OpenFile(cfg.LogFile, os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0644)
-	if err != nil {
-		setupBootstrapLogger()
-		logPrint("Unable to configure logging", err)
-		return err
-	}
-
-	logWriter = output
-	log.SetOutput(logWriter)
+func configureLogFormat(cfg *config.Config) {
 	if cfg.LogFormat == "json" {
 		log.SetFormatter(&log.JSONFormatter{})
 	}
-
-	return nil
 }
 
-// If our log file is not available we want to log somewhere else, but
-// not to standard error because that leaks information to the user. This
-// function attempts to log to syslog.
-func logPrint(msg string, err error) {
-	if logWriter == nil {
-		if bootstrapLogger != nil {
-			bootstrapLogger.Print(ProgName+":", msg+":", err)
-		}
-		return
-	}
+// Configure configures the logging singleton for operation inside a remote TTY (like SSH). In this
+// mode an empty LogFile is not accepted and syslog is used as a fallback when LogFile could not be
+// opened for writing.
+func Configure(cfg *config.Config) {
+	logFile, err := os.OpenFile(cfg.LogFile, os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0644)
+	if err != nil {
+		progName, _ := os.Executable()
+		syslogLogger, err := syslog.NewLogger(syslog.LOG_ERR|syslog.LOG_USER, 0)
+		syslogLogger.Print(progName + ": Unable to configure logging: " + err.Error())
 
-	log.WithError(err).WithFields(log.Fields{
-		"pid": pid,
-	}).Error(msg)
-}
+		// Discard logs since a log file was specified but couldn't be opened
+		log.SetOutput(ioutil.Discard)
+	}
 
-func Fatal(msg string, err error) {
-	mutex.Lock()
-	defer mutex.Unlock()
-	setupBootstrapLogger()
+	log.SetOutput(logFile)
 
-	logPrint(msg, err)
-	// We don't show the error to the end user because it can leak
-	// information that is private to the GitLab server.
-	fmt.Fprintf(os.Stderr, "%s: fatal: %s\n", ProgName, msg)
-	os.Exit(1)
+	configureLogFormat(cfg)
 }
 
-// We assume the logging mutex is already locked.
-func setupBootstrapLogger() {
-	if bootstrapLogger == nil {
-		bootstrapLogger, _ = syslog.NewLogger(syslog.LOG_ERR|syslog.LOG_USER, 0)
+// ConfigureStandalone configures the logging singleton for standalone operation. In this mode an
+// empty LogFile is treated as logging to standard output and standard output is used as a fallback
+// when LogFile could not be opened for writing.
+func ConfigureStandalone(cfg *config.Config) {
+	if cfg.LogFile == "" {
+		return
 	}
-}
 
-func ElapsedTimeMs(start time.Time, end time.Time) float64 {
-	// Later versions of Go support Milliseconds directly:
-	// https://go-review.googlesource.com/c/go/+/167387/
-	return roundFloat(end.Sub(start).Seconds() * 1e3)
-}
-
-func roundFloat(x float64) float64 {
-	return round(x, 1000)
-}
+	logFile, err := os.OpenFile(cfg.LogFile, os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0644)
+	if err != nil {
+		log.Printf("Unable to configure logging, falling back to stdout: %v", err)
+		return
+	}
+	log.SetOutput(logFile)
 
-func round(x, unit float64) float64 {
-	return math.Round(x*unit) / unit
+	configureLogFormat(cfg)
 }

internal/logger/logger_test.go

 package logger
 
 import (
-	"fmt"
 	"io/ioutil"
 	"os"
 	"strings"
 	"testing"
-	"time"
 
 	log "github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/require"
@@ -23,10 +21,7 @@ func TestConfigure(t *testing.T) {
 		LogFormat: "json",
 	}
 
-	err = Configure(&config)
-
-	require.NoError(t, err)
-
+	Configure(&config)
 	log.Info("this is a test")
 
 	tmpFile.Close()
@@ -35,48 +30,3 @@ func TestConfigure(t *testing.T) {
 	require.NoError(t, err)
 	require.True(t, strings.Contains(string(data), `msg":"this is a test"`))
 }
-
-func TestElapsedTimeMs(t *testing.T) {
-	testCases := []struct {
-		delta    float64
-		expected float64
-	}{
-		{
-			delta:    123.0,
-			expected: 123.0,
-		},
-		{
-			delta:    123.4,
-			expected: 123.4,
-		},
-		{
-			delta:    123.45,
-			expected: 123.45,
-		},
-		{
-			delta:    123.456,
-			expected: 123.456,
-		},
-
-		{
-			delta:    123.4567,
-			expected: 123.457,
-		},
-		{
-			delta:    123.4564,
-			expected: 123.456,
-		},
-	}
-
-	for _, tc := range testCases {
-		duration := fmt.Sprintf("%fms", tc.delta)
-
-		t.Run(duration, func(t *testing.T) {
-			delta, _ := time.ParseDuration(duration)
-			start := time.Now()
-			end := start.Add(delta)
-			require.Equal(t, tc.expected, ElapsedTimeMs(start, end))
-			require.InDelta(t, tc.expected, ElapsedTimeMs(start, end), 0.001)
-		})
-	}
-}