gitlab-sshd: Respect the ssl_cert_dir config516-handle-ssl-cert-dir-correctly

Changelog: fixed

5 files changed, 97 insertions, 87 deletions

diff --git a/cmd/gitlab-sshd/main.go b/cmd/gitlab-sshd/main.go
index 04d6888..f777b9f 100644
--- a/cmd/gitlab-sshd/main.go
+++ b/cmd/gitlab-sshd/main.go
@@ -54,6 +54,9 @@ func main() {
 		}
 		log.Fatalf("configuration error: %v", err)
 	}
+
+	cfg.ApplyGlobalState()
+
 	logger.ConfigureStandalone(cfg)
 
 	// Startup monitoring endpoint.
diff --git a/internal/command/command.go b/internal/command/command.go
index bb430b7..038cd5d 100644
--- a/internal/command/command.go
+++ b/internal/command/command.go
@@ -2,7 +2,6 @@ package command
 
 import (
 	"context"
-	"os"
 
 	"gitlab.com/gitlab-org/gitlab-shell/internal/command/authorizedkeys"
 	"gitlab.com/gitlab-org/gitlab-shell/internal/command/authorizedprincipals"
@@ -37,10 +36,6 @@ func New(e *executable.Executable, arguments []string, env sshenv.Env, config *c
 	}
 
 	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 4b315aa..a70ea84 100644
--- a/internal/command/command_test.go
+++ b/internal/command/command_test.go
@@ -44,119 +44,97 @@ func buildEnv(command string) sshenv.Env {
 
 func TestNew(t *testing.T) {
 	testCases := []struct {
-		desc               string
-		executable         *executable.Executable
-		env                sshenv.Env
-		arguments          []string
-		config             *config.Config
-		expectedType       interface{}
-		expectedSslCertDir string
+		desc         string
+		executable   *executable.Executable
+		env          sshenv.Env
+		arguments    []string
+		config       *config.Config
+		expectedType interface{}
 	}{
 		{
-			desc:               "it returns a Discover command",
-			executable:         gitlabShellExec,
-			env:                buildEnv(""),
-			config:             basicConfig,
-			expectedType:       &discover.Command{},
-			expectedSslCertDir: "",
+			desc:         "it returns a Discover command",
+			executable:   gitlabShellExec,
+			env:          buildEnv(""),
+			config:       basicConfig,
+			expectedType: &discover.Command{},
 		},
 		{
-			desc:               "it returns a Discover command with SSL_CERT_DIR env var set",
-			executable:         gitlabShellExec,
-			env:                buildEnv(""),
-			config:             advancedConfig,
-			expectedType:       &discover.Command{},
-			expectedSslCertDir: "/tmp/certs",
+			desc:         "it returns a TwoFactorRecover command",
+			executable:   gitlabShellExec,
+			env:          buildEnv("2fa_recovery_codes"),
+			config:       basicConfig,
+			expectedType: &twofactorrecover.Command{},
 		},
 		{
-			desc:               "it returns a TwoFactorRecover command",
-			executable:         gitlabShellExec,
-			env:                buildEnv("2fa_recovery_codes"),
-			config:             basicConfig,
-			expectedType:       &twofactorrecover.Command{},
-			expectedSslCertDir: "",
+			desc:         "it returns a TwoFactorVerify command",
+			executable:   gitlabShellExec,
+			env:          buildEnv("2fa_verify"),
+			config:       basicConfig,
+			expectedType: &twofactorverify.Command{},
 		},
 		{
-			desc:               "it returns a TwoFactorVerify command",
-			executable:         gitlabShellExec,
-			env:                buildEnv("2fa_verify"),
-			config:             basicConfig,
-			expectedType:       &twofactorverify.Command{},
-			expectedSslCertDir: "",
+			desc:         "it returns an LfsAuthenticate command",
+			executable:   gitlabShellExec,
+			env:          buildEnv("git-lfs-authenticate"),
+			config:       basicConfig,
+			expectedType: &lfsauthenticate.Command{},
 		},
 		{
-			desc:               "it returns an LfsAuthenticate command",
-			executable:         gitlabShellExec,
-			env:                buildEnv("git-lfs-authenticate"),
-			config:             basicConfig,
-			expectedType:       &lfsauthenticate.Command{},
-			expectedSslCertDir: "",
+			desc:         "it returns a ReceivePack command",
+			executable:   gitlabShellExec,
+			env:          buildEnv("git-receive-pack"),
+			config:       basicConfig,
+			expectedType: &receivepack.Command{},
 		},
 		{
-			desc:               "it returns a ReceivePack command",
-			executable:         gitlabShellExec,
-			env:                buildEnv("git-receive-pack"),
-			config:             basicConfig,
-			expectedType:       &receivepack.Command{},
-			expectedSslCertDir: "",
+			desc:         "it returns an UploadPack command",
+			executable:   gitlabShellExec,
+			env:          buildEnv("git-upload-pack"),
+			config:       basicConfig,
+			expectedType: &uploadpack.Command{},
 		},
 		{
-			desc:               "it returns an UploadPack command",
-			executable:         gitlabShellExec,
-			env:                buildEnv("git-upload-pack"),
-			config:             basicConfig,
-			expectedType:       &uploadpack.Command{},
-			expectedSslCertDir: "",
+			desc:         "it returns an UploadArchive command",
+			executable:   gitlabShellExec,
+			env:          buildEnv("git-upload-archive"),
+			config:       basicConfig,
+			expectedType: &uploadarchive.Command{},
 		},
 		{
-			desc:               "it returns an UploadArchive command",
-			executable:         gitlabShellExec,
-			env:                buildEnv("git-upload-archive"),
-			config:             basicConfig,
-			expectedType:       &uploadarchive.Command{},
-			expectedSslCertDir: "",
+			desc:         "it returns a Healthcheck command",
+			executable:   checkExec,
+			config:       basicConfig,
+			expectedType: &healthcheck.Command{},
 		},
 		{
-			desc:               "it returns a Healthcheck command",
-			executable:         checkExec,
-			config:             basicConfig,
-			expectedType:       &healthcheck.Command{},
-			expectedSslCertDir: "",
+			desc:         "it returns a AuthorizedKeys command",
+			executable:   authorizedKeysExec,
+			arguments:    []string{"git", "git", "key"},
+			config:       basicConfig,
+			expectedType: &authorizedkeys.Command{},
 		},
 		{
-			desc:               "it returns a AuthorizedKeys command",
-			executable:         authorizedKeysExec,
-			arguments:          []string{"git", "git", "key"},
-			config:             basicConfig,
-			expectedType:       &authorizedkeys.Command{},
-			expectedSslCertDir: "",
+			desc:         "it returns a AuthorizedPrincipals command",
+			executable:   authorizedPrincipalsExec,
+			arguments:    []string{"key", "principal"},
+			config:       basicConfig,
+			expectedType: &authorizedprincipals.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,
-			env:                buildEnv("personal_access_token"),
-			config:             basicConfig,
-			expectedType:       &personalaccesstoken.Command{},
-			expectedSslCertDir: "",
+			desc:         "it returns a PersonalAccessToken command",
+			executable:   gitlabShellExec,
+			env:          buildEnv("personal_access_token"),
+			config:       basicConfig,
+			expectedType: &personalaccesstoken.Command{},
 		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
-			os.Unsetenv("SSL_CERT_DIR")
 			command, err := New(tc.executable, tc.arguments, tc.env, 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/config/config.go b/internal/config/config.go
index ed061a8..e3dd7c2 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -4,6 +4,7 @@ import (
 	"errors"
 	"io/ioutil"
 	"net/url"
+	"os"
 	"path"
 	"path/filepath"
 	"sync"
@@ -74,6 +75,12 @@ var (
 	}
 )
 
+func (c *Config) ApplyGlobalState() {
+	if c.SslCertDir != "" {
+		os.Setenv("SSL_CERT_DIR", c.SslCertDir)
+	}
+}
+
 func (c *Config) HttpClient() *client.HttpClient {
 	c.httpClientOnce.Do(func() {
 		c.httpClient = client.NewHTTPClient(
@@ -96,6 +103,9 @@ func NewFromDirExternal(dir string) (*Config, error) {
 	if err != nil {
 		return nil, err
 	}
+
+	cfg.ApplyGlobalState()
+
 	return cfg, nil
 }
 
diff --git a/internal/config/config_test.go b/internal/config/config_test.go
new file mode 100644
index 0000000..1a79320
--- /dev/null
+++ b/internal/config/config_test.go
@@ -0,0 +1,24 @@
+package config
+
+import (
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"gitlab.com/gitlab-org/gitlab-shell/internal/testhelper"
+)
+
+func TestConfigApplyGlobalState(t *testing.T) {
+	t.Cleanup(testhelper.TempEnv(map[string]string{"SSL_CERT_DIR": "unmodified"}))
+
+	config := &Config{SslCertDir: ""}
+	config.ApplyGlobalState()
+
+	require.Equal(t, "unmodified", os.Getenv("SSL_CERT_DIR"))
+
+	config.SslCertDir = "foo"
+	config.ApplyGlobalState()
+
+	require.Equal(t, "foo", os.Getenv("SSL_CERT_DIR"))
+}