Merge branch 'sh-http-proxy-fix-13-1' into '13-1-stable'

Fix HTTP proxies not working in Gitaly hooks (13.1 stable)

See merge request gitlab-org/gitaly!2337

6 files changed, 77 insertions, 22 deletions

diff --git a/changelogs/unreleased/sh-fix-issue-2913.yml b/changelogs/unreleased/sh-fix-issue-2913.yml
new file mode 100644
index 000000000..9ffd75f3c
--- /dev/null
+++ b/changelogs/unreleased/sh-fix-issue-2913.yml
@@ -0,0 +1,5 @@
+---
+title: Fix HTTP proxies not working in Gitaly hooks
+merge_request: 2325
+author:
+type: fixed
diff --git a/cmd/gitaly-hooks/hooks.go b/cmd/gitaly-hooks/hooks.go
index 1e144c67a..0d59f334d 100644
--- a/cmd/gitaly-hooks/hooks.go
+++ b/cmd/gitaly-hooks/hooks.go
@@ -230,7 +230,8 @@ func gitalyFromEnv() (*grpc.ClientConn, error) {
 }
 
 func glValues() []string {
-	var glEnvVars []string
+	glEnvVars := command.AllowedEnvironment()
+
 	for _, kv := range os.Environ() {
 		if strings.HasPrefix(kv, "GL_") {
 			glEnvVars = append(glEnvVars, kv)
diff --git a/cmd/gitaly-hooks/hooks_test.go b/cmd/gitaly-hooks/hooks_test.go
index f4f1ed5a6..2a17ce886 100644
--- a/cmd/gitaly-hooks/hooks_test.go
+++ b/cmd/gitaly-hooks/hooks_test.go
@@ -72,6 +72,7 @@ func TestHooksPrePostReceive(t *testing.T) {
 	gitAlternateObjectDirs := []string{(filepath.Join(testRepoPath, "objects"))}
 
 	gitlabUser, gitlabPassword := "gitlab_user-1234", "gitlabsecret9887"
+	httpProxy, httpsProxy, noProxy := "http://test.example.com:8080", "https://test.example.com:8080", "*"
 
 	c := testhelper.GitlabTestServerOptions{
 		User:                        gitlabUser,
@@ -158,6 +159,11 @@ func TestHooksPrePostReceive(t *testing.T) {
 						GitObjectDir:           c.GitObjectDir,
 						GitAlternateObjectDirs: c.GitAlternateObjectDirs,
 					},
+					testhelper.ProxyValues{
+						HTTPProxy:  httpProxy,
+						HTTPSProxy: httpsProxy,
+						NoProxy:    noProxy,
+					},
 					gitPushOptions...,
 				)
 
@@ -172,9 +178,15 @@ func TestHooksPrePostReceive(t *testing.T) {
 				require.Empty(t, stdout.String())
 
 				output := string(testhelper.MustReadFile(t, customHookOutputPath))
-				require.Contains(t, output, "GL_USERNAME="+glUsername)
-				require.Contains(t, output, "GL_ID="+glID)
-				require.Contains(t, output, "GL_REPOSITORY="+glRepository)
+				requireContainsOnce(t, output, "GL_USERNAME="+glUsername)
+				requireContainsOnce(t, output, "GL_ID="+glID)
+				requireContainsOnce(t, output, "GL_REPOSITORY="+glRepository)
+				requireContainsOnce(t, output, "HTTP_PROXY="+httpProxy)
+				requireContainsOnce(t, output, "http_proxy="+httpProxy)
+				requireContainsOnce(t, output, "HTTPS_PROXY="+httpsProxy)
+				requireContainsOnce(t, output, "https_proxy="+httpsProxy)
+				requireContainsOnce(t, output, "no_proxy="+noProxy)
+				requireContainsOnce(t, output, "NO_PROXY="+noProxy)
 
 				if hookName == "pre-receive" {
 					gitObjectDirMatches := gitObjectDirRegex.FindStringSubmatch(output)
@@ -251,7 +263,7 @@ func testHooksUpdate(t *testing.T, gitlabShellDir, socket, token string, glValue
 	updateHookPath, err := filepath.Abs("../../ruby/git-hooks/update")
 	require.NoError(t, err)
 	cmd := exec.Command(updateHookPath, refval, oldval, newval)
-	cmd.Env = testhelper.EnvForHooks(t, gitlabShellDir, socket, token, testRepo, glValues)
+	cmd.Env = testhelper.EnvForHooks(t, gitlabShellDir, socket, token, testRepo, glValues, testhelper.ProxyValues{})
 	cmd.Dir = testRepoPath
 
 	tempDir, cleanup := testhelper.TempDir(t)
@@ -356,12 +368,14 @@ func TestHooksPostReceiveFailed(t *testing.T) {
 	postReceiveHookPath, err := filepath.Abs("../../ruby/git-hooks/post-receive")
 	require.NoError(t, err)
 	cmd := exec.Command(postReceiveHookPath)
-	cmd.Env = testhelper.EnvForHooks(t, tempGitlabShellDir, socket, token, testRepo, testhelper.GlHookValues{
-		GLID:       glID,
-		GLUsername: glUsername,
-		GLRepo:     glRepository,
-		GLProtocol: glProtocol,
-	})
+	cmd.Env = testhelper.EnvForHooks(t, tempGitlabShellDir, socket, token, testRepo,
+		testhelper.GlHookValues{
+			GLID:       glID,
+			GLUsername: glUsername,
+			GLRepo:     glRepository,
+			GLProtocol: glProtocol,
+		},
+		testhelper.ProxyValues{})
 	cmd.Stdout = &stdout
 	cmd.Stderr = &stderr
 	cmd.Stdin = bytes.NewBuffer([]byte(changes))
@@ -436,12 +450,14 @@ func TestHooksNotAllowed(t *testing.T) {
 	cmd.Stderr = &stderr
 	cmd.Stdout = &stdout
 	cmd.Stdin = strings.NewReader(changes)
-	cmd.Env = testhelper.EnvForHooks(t, tempGitlabShellDir, socket, token, testRepo, testhelper.GlHookValues{
-		GLID:       glID,
-		GLUsername: glUsername,
-		GLRepo:     glRepository,
-		GLProtocol: glProtocol,
-	})
+	cmd.Env = testhelper.EnvForHooks(t, tempGitlabShellDir, socket, token, testRepo,
+		testhelper.GlHookValues{
+			GLID:       glID,
+			GLUsername: glUsername,
+			GLRepo:     glRepository,
+			GLProtocol: glProtocol,
+		},
+		testhelper.ProxyValues{})
 	cmd.Dir = testRepoPath
 
 	require.Error(t, cmd.Run())
@@ -560,3 +576,9 @@ func runHookServiceServerWithAPI(t *testing.T, token string, gitlabAPI hook.Gitl
 
 	return server.Socket(), server.Stop
 }
+
+func requireContainsOnce(t *testing.T, s string, contains string) {
+	r := regexp.MustCompile(contains)
+	matches := r.FindAllStringIndex(s, -1)
+	require.Equal(t, 1, len(matches))
+}
diff --git a/internal/command/command.go b/internal/command/command.go
index 0baa29e42..f54b1694b 100644
--- a/internal/command/command.go
+++ b/internal/command/command.go
@@ -52,6 +52,7 @@ var exportedEnvVars = []string{
 	"HTTPS_PROXY",
 	// libcurl settings: https://curl.haxx.se/libcurl/c/CURLOPT_NOPROXY.html
 	"no_proxy",
+	"NO_PROXY",
 }
 
 const (
@@ -197,7 +198,7 @@ func New(ctx context.Context, cmd *exec.Cmd, stdin io.Reader, stdout, stderr io.
 	env = append(env, "GIT_TERMINAL_PROMPT=0")
 
 	// Export env vars
-	cmd.Env = exportEnvironment(env)
+	cmd.Env = append(env, AllowedEnvironment()...)
 
 	// Start the command in its own process group (nice for signalling)
 	cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
@@ -261,14 +262,19 @@ func New(ctx context.Context, cmd *exec.Cmd, stdin io.Reader, stdout, stderr io.
 	return command, nil
 }
 
-func exportEnvironment(env []string) []string {
+// AllowedEnvironment returns an environment based on the allowed
+// variables defined above. This is useful for constructing a base
+// environment in which a command can be run.
+func AllowedEnvironment() []string {
+	var filtered []string
+
 	for _, envVarName := range exportedEnvVars {
 		if val, ok := os.LookupEnv(envVarName); ok {
-			env = append(env, fmt.Sprintf("%s=%s", envVarName, val))
+			filtered = append(filtered, fmt.Sprintf("%s=%s", envVarName, val))
 		}
 	}
 
-	return env
+	return filtered
 }
 
 func escapeNewlineWriter(outbound io.WriteCloser, done chan struct{}, maxBytes int) io.WriteCloser {
diff --git a/internal/command/command_test.go b/internal/command/command_test.go
index e16c7a7cd..a457ae91b 100644
--- a/internal/command/command_test.go
+++ b/internal/command/command_test.go
@@ -83,6 +83,10 @@ func TestNewCommandProxyEnv(t *testing.T) {
 			key:   "no_proxy",
 			value: "https://excluded:5000",
 		},
+		{
+			key:   "NO_PROXY",
+			value: "https://excluded:5000",
+		},
 	}
 
 	for _, tc := range testCases {
diff --git a/internal/testhelper/testserver.go b/internal/testhelper/testserver.go
index 26f990aa7..390b7e6f4 100644
--- a/internal/testhelper/testserver.go
+++ b/internal/testhelper/testserver.go
@@ -726,10 +726,14 @@ type GlHookValues struct {
 	GitAlternateObjectDirs                             []string
 }
 
+type ProxyValues struct {
+	HTTPProxy, HTTPSProxy, NoProxy string
+}
+
 var jsonpbMarshaller jsonpb.Marshaler
 
 // EnvForHooks generates a set of environment variables for gitaly hooks
-func EnvForHooks(t testing.TB, gitlabShellDir, gitalySocket, gitalyToken string, repo *gitalypb.Repository, glHookValues GlHookValues, gitPushOptions ...string) []string {
+func EnvForHooks(t testing.TB, gitlabShellDir, gitalySocket, gitalyToken string, repo *gitalypb.Repository, glHookValues GlHookValues, proxyValues ProxyValues, gitPushOptions ...string) []string {
 	rubyDir, err := filepath.Abs("../../ruby")
 	require.NoError(t, err)
 
@@ -754,6 +758,19 @@ func EnvForHooks(t testing.TB, gitlabShellDir, gitalySocket, gitalyToken string,
 	}...)
 	env = append(env, hooks.GitPushOptions(gitPushOptions)...)
 
+	if proxyValues.HTTPProxy != "" {
+		env = append(env, fmt.Sprintf("HTTP_PROXY=%s", proxyValues.HTTPProxy))
+		env = append(env, fmt.Sprintf("http_proxy=%s", proxyValues.HTTPProxy))
+	}
+	if proxyValues.HTTPSProxy != "" {
+		env = append(env, fmt.Sprintf("HTTPS_PROXY=%s", proxyValues.HTTPSProxy))
+		env = append(env, fmt.Sprintf("https_proxy=%s", proxyValues.HTTPSProxy))
+	}
+	if proxyValues.NoProxy != "" {
+		env = append(env, fmt.Sprintf("NO_PROXY=%s", proxyValues.NoProxy))
+		env = append(env, fmt.Sprintf("no_proxy=%s", proxyValues.NoProxy))
+	}
+
 	if glHookValues.GitObjectDir != "" {
 		env = append(env, fmt.Sprintf("GIT_OBJECT_DIRECTORY=%s", glHookValues.GitObjectDir))
 	}