Merge branch 'ashmckenzie/set-ssl-cert-dir-env-var' into 'master'

Set SSL_CERT_DIR env var when building command

See merge request gitlab-org/gitlab-shell!423

6 files changed, 94 insertions, 119 deletions

diff --git a/internal/command/authorizedkeys/authorized_keys_test.go b/internal/command/authorizedkeys/authorized_keys_test.go
index f15c34d..ab44580 100644
--- a/internal/command/authorizedkeys/authorized_keys_test.go
+++ b/internal/command/authorizedkeys/authorized_keys_test.go
@@ -47,11 +47,9 @@ func TestExecute(t *testing.T) {
 	defer cleanup()
 
 	defaultConfig := &config.Config{RootDir: "/tmp", GitlabUrl: url}
-	configWithSslCertDir := &config.Config{RootDir: "/tmp", GitlabUrl: url, SslCertDir: "/tmp/certs"}
 
 	testCases := []struct {
 		desc           string
-		config         *config.Config
 		arguments      *commandargs.AuthorizedKeys
 		expectedOutput string
 	}{
@@ -61,12 +59,6 @@ func TestExecute(t *testing.T) {
 			expectedOutput: "command=\"/tmp/bin/gitlab-shell key-1\",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty public-key\n",
 		},
 		{
-			desc:           "With SSL cert dir",
-			config:         configWithSslCertDir,
-			arguments:      &commandargs.AuthorizedKeys{ExpectedUser: "user", ActualUser: "user", Key: "key"},
-			expectedOutput: "command=\"SSL_CERT_DIR=/tmp/certs /tmp/bin/gitlab-shell key-1\",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty public-key\n",
-		},
-		{
 			desc:           "When key doesn't match any existing key",
 			arguments:      &commandargs.AuthorizedKeys{ExpectedUser: "user", ActualUser: "user", Key: "not-found"},
 			expectedOutput: "# No key was found for not-found\n",
@@ -87,13 +79,8 @@ func TestExecute(t *testing.T) {
 		t.Run(tc.desc, func(t *testing.T) {
 			buffer := &bytes.Buffer{}
 
-			config := defaultConfig
-			if tc.config != nil {
-				config = tc.config
-			}
-
 			cmd := &Command{
-				Config:     config,
+				Config:     defaultConfig,
 				Args:       tc.arguments,
 				ReadWriter: &readwriter.ReadWriter{Out: buffer},
 			}
diff --git a/internal/command/authorizedprincipals/authorized_principals_test.go b/internal/command/authorizedprincipals/authorized_principals_test.go
index ec97b65..2450a54 100644
--- a/internal/command/authorizedprincipals/authorized_principals_test.go
+++ b/internal/command/authorizedprincipals/authorized_principals_test.go
@@ -14,11 +14,9 @@ import (
 
 func TestExecute(t *testing.T) {
 	defaultConfig := &config.Config{RootDir: "/tmp"}
-	configWithSslCertDir := &config.Config{RootDir: "/tmp", SslCertDir: "/tmp/certs"}
 
 	testCases := []struct {
 		desc           string
-		config         *config.Config
 		arguments      *commandargs.AuthorizedPrincipals
 		expectedOutput string
 	}{
@@ -28,12 +26,6 @@ func TestExecute(t *testing.T) {
 			expectedOutput: "command=\"/tmp/bin/gitlab-shell username-key\",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty principal\n",
 		},
 		{
-			desc:           "With SSL cert dir",
-			config:         configWithSslCertDir,
-			arguments:      &commandargs.AuthorizedPrincipals{KeyId: "key", Principals: []string{"principal"}},
-			expectedOutput: "command=\"SSL_CERT_DIR=/tmp/certs /tmp/bin/gitlab-shell username-key\",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty principal\n",
-		},
-		{
 			desc:           "With multiple principals",
 			arguments:      &commandargs.AuthorizedPrincipals{KeyId: "key", Principals: []string{"principal-1", "principal-2"}},
 			expectedOutput: "command=\"/tmp/bin/gitlab-shell username-key\",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty principal-1\ncommand=\"/tmp/bin/gitlab-shell username-key\",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty principal-2\n",
@@ -44,13 +36,8 @@ func TestExecute(t *testing.T) {
 		t.Run(tc.desc, func(t *testing.T) {
 			buffer := &bytes.Buffer{}
 
-			config := defaultConfig
-			if tc.config != nil {
-				config = tc.config
-			}
-
 			cmd := &Command{
-				Config:     config,
+				Config:     defaultConfig,
 				Args:       tc.arguments,
 				ReadWriter: &readwriter.ReadWriter{Out: buffer},
 			}
diff --git a/internal/command/command.go b/internal/command/command.go
index 7e0617e..a2c5912 100644
--- a/internal/command/command.go
+++ b/internal/command/command.go
@@ -2,6 +2,7 @@ package command
 
 import (
 	"context"
+	"os"
 
 	"gitlab.com/gitlab-org/gitlab-shell/internal/command/authorizedkeys"
 	"gitlab.com/gitlab-org/gitlab-shell/internal/command/authorizedprincipals"
@@ -34,6 +35,10 @@ func New(e *executable.Executable, arguments []string, config *config.Config, re
 	}
 
 	if cmd := buildCommand(e, args, config, readWriter); cmd != nil {
+		if config.SslCertDir != "" {
+			os.Setenv("SSL_CERT_DIR", config.SslCertDir)
+		}
+
 		return cmd, nil
 	}
 
diff --git a/internal/command/command_test.go b/internal/command/command_test.go
index 9160abf..c2a7483 100644
--- a/internal/command/command_test.go
+++ b/internal/command/command_test.go
@@ -30,7 +30,8 @@ var (
 	checkExec                = &executable.Executable{Name: executable.Healthcheck}
 	gitlabShellExec          = &executable.Executable{Name: executable.GitlabShell}
 
-	basicConfig = &config.Config{GitlabUrl: "http+unix://gitlab.socket"}
+	basicConfig    = &config.Config{GitlabUrl: "http+unix://gitlab.socket"}
+	advancedConfig = &config.Config{GitlabUrl: "http+unix://gitlab.socket", SslCertDir: "/tmp/certs"}
 )
 
 func buildEnv(command string) map[string]string {
@@ -42,70 +43,100 @@ func buildEnv(command string) map[string]string {
 
 func TestNew(t *testing.T) {
 	testCases := []struct {
-		desc         string
-		executable   *executable.Executable
-		environment  map[string]string
-		arguments    []string
-		expectedType interface{}
+		desc               string
+		executable         *executable.Executable
+		environment        map[string]string
+		arguments          []string
+		config             *config.Config
+		expectedType       interface{}
+		expectedSslCertDir string
 	}{
 		{
-			desc:         "it returns a Discover command",
-			executable:   gitlabShellExec,
-			environment:  buildEnv(""),
-			expectedType: &discover.Command{},
+			desc:               "it returns a Discover command",
+			executable:         gitlabShellExec,
+			environment:        buildEnv(""),
+			config:             basicConfig,
+			expectedType:       &discover.Command{},
+			expectedSslCertDir: "",
 		},
 		{
-			desc:         "it returns a TwoFactorRecover command",
-			executable:   gitlabShellExec,
-			environment:  buildEnv("2fa_recovery_codes"),
-			expectedType: &twofactorrecover.Command{},
+			desc:               "it returns a Discover command with SSL_CERT_DIR env var set",
+			executable:         gitlabShellExec,
+			environment:        buildEnv(""),
+			config:             advancedConfig,
+			expectedType:       &discover.Command{},
+			expectedSslCertDir: "/tmp/certs",
 		},
 		{
-			desc:         "it returns an LfsAuthenticate command",
-			executable:   gitlabShellExec,
-			environment:  buildEnv("git-lfs-authenticate"),
-			expectedType: &lfsauthenticate.Command{},
+			desc:               "it returns a TwoFactorRecover command",
+			executable:         gitlabShellExec,
+			environment:        buildEnv("2fa_recovery_codes"),
+			config:             basicConfig,
+			expectedType:       &twofactorrecover.Command{},
+			expectedSslCertDir: "",
 		},
 		{
-			desc:         "it returns a ReceivePack command",
-			executable:   gitlabShellExec,
-			environment:  buildEnv("git-receive-pack"),
-			expectedType: &receivepack.Command{},
+			desc:               "it returns an LfsAuthenticate command",
+			executable:         gitlabShellExec,
+			environment:        buildEnv("git-lfs-authenticate"),
+			config:             basicConfig,
+			expectedType:       &lfsauthenticate.Command{},
+			expectedSslCertDir: "",
 		},
 		{
-			desc:         "it returns an UploadPack command",
-			executable:   gitlabShellExec,
-			environment:  buildEnv("git-upload-pack"),
-			expectedType: &uploadpack.Command{},
+			desc:               "it returns a ReceivePack command",
+			executable:         gitlabShellExec,
+			environment:        buildEnv("git-receive-pack"),
+			config:             basicConfig,
+			expectedType:       &receivepack.Command{},
+			expectedSslCertDir: "",
 		},
 		{
-			desc:         "it returns an UploadArchive command",
-			executable:   gitlabShellExec,
-			environment:  buildEnv("git-upload-archive"),
-			expectedType: &uploadarchive.Command{},
+			desc:               "it returns an UploadPack command",
+			executable:         gitlabShellExec,
+			environment:        buildEnv("git-upload-pack"),
+			config:             basicConfig,
+			expectedType:       &uploadpack.Command{},
+			expectedSslCertDir: "",
 		},
 		{
-			desc:         "it returns a Healthcheck command",
-			executable:   checkExec,
-			expectedType: &healthcheck.Command{},
+			desc:               "it returns an UploadArchive command",
+			executable:         gitlabShellExec,
+			environment:        buildEnv("git-upload-archive"),
+			config:             basicConfig,
+			expectedType:       &uploadarchive.Command{},
+			expectedSslCertDir: "",
 		},
 		{
-			desc:         "it returns a AuthorizedKeys command",
-			executable:   authorizedKeysExec,
-			arguments:    []string{"git", "git", "key"},
-			expectedType: &authorizedkeys.Command{},
+			desc:               "it returns a Healthcheck command",
+			executable:         checkExec,
+			config:             basicConfig,
+			expectedType:       &healthcheck.Command{},
+			expectedSslCertDir: "",
 		},
 		{
-			desc:         "it returns a AuthorizedPrincipals command",
-			executable:   authorizedPrincipalsExec,
-			arguments:    []string{"key", "principal"},
-			expectedType: &authorizedprincipals.Command{},
+			desc:               "it returns a AuthorizedKeys command",
+			executable:         authorizedKeysExec,
+			arguments:          []string{"git", "git", "key"},
+			config:             basicConfig,
+			expectedType:       &authorizedkeys.Command{},
+			expectedSslCertDir: "",
 		},
 		{
-			desc:         "it returns a PersonalAccessToken command",
-			executable:   gitlabShellExec,
-			environment:  buildEnv("personal_access_token"),
-			expectedType: &personalaccesstoken.Command{},
+			desc:               "it returns a AuthorizedPrincipals command",
+			executable:         authorizedPrincipalsExec,
+			arguments:          []string{"key", "principal"},
+			config:             basicConfig,
+			expectedType:       &authorizedprincipals.Command{},
+			expectedSslCertDir: "",
+		},
+		{
+			desc:               "it returns a PersonalAccessToken command",
+			executable:         gitlabShellExec,
+			environment:        buildEnv("personal_access_token"),
+			config:             basicConfig,
+			expectedType:       &personalaccesstoken.Command{},
+			expectedSslCertDir: "",
 		},
 	}
 
@@ -114,10 +145,12 @@ func TestNew(t *testing.T) {
 			restoreEnv := testhelper.TempEnv(tc.environment)
 			defer restoreEnv()
 
-			command, err := New(tc.executable, tc.arguments, basicConfig, nil)
+			os.Unsetenv("SSL_CERT_DIR")
+			command, err := New(tc.executable, tc.arguments, tc.config, nil)
 
 			require.NoError(t, err)
 			require.IsType(t, tc.expectedType, command)
+			require.Equal(t, tc.expectedSslCertDir, os.Getenv("SSL_CERT_DIR"))
 		})
 	}
 }
diff --git a/internal/keyline/key_line.go b/internal/keyline/key_line.go
index e2abb82..c6f2422 100644
--- a/internal/keyline/key_line.go
+++ b/internal/keyline/key_line.go
@@ -37,22 +37,9 @@ func NewPrincipalKeyLine(keyId, principal string, config *config.Config) (*KeyLi
 }
 
 func (k *KeyLine) ToString() string {
-	sslCertDirEnvVar := k.sslCertDirEnvVar()
 	command := fmt.Sprintf("%s %s-%s", path.Join(k.Config.RootDir, executable.BinDir, executable.GitlabShell), k.Prefix, k.Id)
 
-	if sslCertDirEnvVar != "" {
-		sslCertDirEnvVar = fmt.Sprintf(`%s `, sslCertDirEnvVar)
-	}
-
-	return fmt.Sprintf(`command="%s%s",%s %s`, sslCertDirEnvVar, command, SshOptions, k.Value)
-}
-
-func (k *KeyLine) sslCertDirEnvVar() string {
-	if k.Config.SslCertDir != "" {
-		return fmt.Sprintf(`SSL_CERT_DIR=%s`, k.Config.SslCertDir)
-	}
-
-	return ""
+	return fmt.Sprintf(`command="%s",%s %s`, command, SshOptions, k.Value)
 }
 
 func newKeyLine(id, value, prefix string, config *config.Config) (*KeyLine, error) {
diff --git a/internal/keyline/key_line_test.go b/internal/keyline/key_line_test.go
index 095de78..e652c23 100644
--- a/internal/keyline/key_line_test.go
+++ b/internal/keyline/key_line_test.go
@@ -70,37 +70,13 @@ func TestFailingNewPrincipalKeyLine(t *testing.T) {
 }
 
 func TestToString(t *testing.T) {
-	testCases := []struct {
-		desc           string
-		keyLine        *KeyLine
-		expectedOutput string
-	}{
-		{
-			desc: "Without SSL cert dir",
-			keyLine: &KeyLine{
-				Id:     "1",
-				Value:  "public-key",
-				Prefix: "key",
-				Config: &config.Config{RootDir: "/tmp"},
-			},
-			expectedOutput: `command="/tmp/bin/gitlab-shell key-1",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty public-key`,
-		},
-		{
-			desc: "With SSL cert dir",
-			keyLine: &KeyLine{
-				Id:     "1",
-				Value:  "public-key",
-				Prefix: "key",
-				Config: &config.Config{RootDir: "/tmp", SslCertDir: "/tmp/certs"},
-			},
-			expectedOutput: `command="SSL_CERT_DIR=/tmp/certs /tmp/bin/gitlab-shell key-1",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty public-key`,
-		},
+	keyLine := &KeyLine{
+		Id:     "1",
+		Value:  "public-key",
+		Prefix: "key",
+		Config: &config.Config{RootDir: "/tmp"},
 	}
 
-	for _, tc := range testCases {
-		t.Run(tc.desc, func(t *testing.T) {
-			result := tc.keyLine.ToString()
-			require.Equal(t, tc.expectedOutput, result)
-		})
-	}
+	result := keyLine.ToString()
+	require.Equal(t, `command="/tmp/bin/gitlab-shell key-1",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty public-key`, result)
 }