diff --git a/platform/git/exec/BUILD.bazel b/platform/git/exec/BUILD.bazel index e506f08b9..f5c575674 100644 --- a/platform/git/exec/BUILD.bazel +++ b/platform/git/exec/BUILD.bazel @@ -1,4 +1,4 @@ -load("@rules_go//go:def.bzl", "go_library") +load("@rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "go_default_library", @@ -6,3 +6,13 @@ go_library( importpath = "github.com/uber/submitqueue/platform/git/exec", visibility = ["//visibility:public"], ) + +go_test( + name = "go_default_test", + srcs = ["gitexec_test.go"], + embed = [":go_default_library"], + deps = [ + "@com_github_stretchr_testify//assert:go_default_library", + "@com_github_stretchr_testify//require:go_default_library", + ], +) diff --git a/platform/git/exec/gitexec.go b/platform/git/exec/gitexec.go index 519e78cd9..0f7a6d8ad 100644 --- a/platform/git/exec/gitexec.go +++ b/platform/git/exec/gitexec.go @@ -12,19 +12,23 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Package gitexec locates a git binary and runs it with the ambient -// environment stripped out. +// Package gitexec locates a git binary and composes the environment git runs +// in. It is the single source of truth for that environment across every +// SubmitQueue caller — demo tooling, the change provider's repository, and the +// Runway merger. // -// Demo and development tooling drives git on a developer's own machine, where -// hooks, a signing key, or a commit template configured globally would each -// break a run in a way that has nothing to do with SubmitQueue. Every command -// built here therefore carries the same scrubbed environment the git merger -// uses (see runway/extension/merger/git), so tooling behaves the same on every -// machine. +// The environment has two halves. The scrub set denies git all ambient +// configuration that could change what a command produces — a global hooks +// path, a signing requirement, a commit template — which is what makes a +// scripted run behave the same on every machine. The transport set carries +// what a command needs to reach a remote — the SSH agent socket, git's ssh and +// credential helpers on PATH, TLS roots, proxy settings — none of which can +// change an answer. Every caller shares both halves; they differ only in the +// literal entries they add (a pinned exec path, an isolated HOME), which is why +// Env takes those as options rather than baking one caller's policy in. // -// This resolves only the executable, because tooling runs porcelain -// (init, clone, commit, push) rather than constructing a merger's GitRuntime, -// which additionally pins the exec path and template directory. +// HOME is deliberately not in the transport set: a caller that isolates HOME +// and a caller that inherits it disagree, so each supplies it itself. package gitexec import ( @@ -67,23 +71,75 @@ func Resolve(path string) (string, error) { return absolute, nil } +// scrubEnv denies git every ambient configuration input that could change what +// a command produces. Always applied, first, so a later entry can override it. +var scrubEnv = []string{ + "GIT_CONFIG_NOSYSTEM=1", + "GIT_CONFIG_GLOBAL=" + os.DevNull, + "GIT_ATTR_NOSYSTEM=1", + "GIT_TERMINAL_PROMPT=0", + "GIT_PAGER=cat", + "GIT_EDITOR=:", +} + +// transportEnvNames are inherited from the parent process when set. None can +// change what a command produces; each decides whether a remote is reachable. +// HOME is intentionally absent — see the package doc. +var transportEnvNames = []string{ + "PATH", + "SSH_AUTH_SOCK", "SSH_AGENT_PID", + "GIT_SSH", "GIT_SSH_COMMAND", "GIT_SSH_VARIANT", + "GIT_SSL_CAINFO", "GIT_SSL_CAPATH", + "SSL_CERT_DIR", "SSL_CERT_FILE", + "HTTP_PROXY", "HTTPS_PROXY", "NO_PROXY", + "http_proxy", "https_proxy", "no_proxy", +} + +// EnvOptions selects what, on top of the always-applied scrub set, a git +// command's environment carries. +type EnvOptions struct { + // Transport inherits the transport variables from the parent when set. + Transport bool + // Passthrough names further variables to inherit from the parent when set. + Passthrough []string + // Literal entries are appended last as "NAME=value", so they override any + // inherited value of the same name. + Literal []string +} + +// Env composes a git command environment: the scrub set, then the requested +// variables inherited from the parent (only those actually set, so an unset +// SSH_AUTH_SOCK stays absent rather than becoming empty), then the literals. +func Env(opts EnvOptions) []string { + env := make([]string, 0, len(scrubEnv)+len(transportEnvNames)+len(opts.Passthrough)+len(opts.Literal)) + env = append(env, scrubEnv...) + + names := make([]string, 0, len(transportEnvNames)+len(opts.Passthrough)) + if opts.Transport { + names = append(names, transportEnvNames...) + } + names = append(names, opts.Passthrough...) + + seen := make(map[string]bool, len(names)) + for _, name := range names { + if name == "" || seen[name] { + continue + } + seen[name] = true + if v, ok := os.LookupEnv(name); ok { + env = append(env, name+"="+v) + } + } + return append(env, opts.Literal...) +} + // Command builds a git invocation in dir with the ambient environment removed. -// An empty dir runs in the current working directory. +// An empty dir runs in the current working directory. PATH is passed so git can +// find its helpers; nothing else the host sets reaches the command. func Command(ctx context.Context, git, dir string, args ...string) *exec.Cmd { cmd := exec.CommandContext(ctx, git, args...) cmd.Dir = dir - // A developer's global config is the usual reason a scripted git run fails - // on one machine and not another: a hooks path, a signing requirement, or a - // commit template. None of it is relevant to seeding a sandbox. - cmd.Env = []string{ - "GIT_CONFIG_NOSYSTEM=1", - "GIT_CONFIG_GLOBAL=" + os.DevNull, - "GIT_ATTR_NOSYSTEM=1", - "GIT_TERMINAL_PROMPT=0", - "GIT_PAGER=cat", - "GIT_EDITOR=:", - "PATH=" + os.Getenv("PATH"), - } + cmd.Env = Env(EnvOptions{Literal: []string{"PATH=" + os.Getenv("PATH")}}) return cmd } diff --git a/platform/git/exec/gitexec_test.go b/platform/git/exec/gitexec_test.go new file mode 100644 index 000000000..1658b06d9 --- /dev/null +++ b/platform/git/exec/gitexec_test.go @@ -0,0 +1,97 @@ +// Copyright (c) 2025 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package gitexec + +import ( + "os" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// value returns the value of NAME=value entry for name, and whether it is +// present at all. +func value(env []string, name string) (string, bool) { + prefix := name + "=" + found := "" + ok := false + for _, e := range env { + if strings.HasPrefix(e, prefix) { + found = strings.TrimPrefix(e, prefix) + ok = true + } + } + return found, ok +} + +func TestEnv_AlwaysScrubs(t *testing.T) { + env := Env(EnvOptions{}) + for _, want := range scrubEnv { + assert.Contains(t, env, want) + } +} + +func TestEnv_TransportInheritedOnlyWhenSet(t *testing.T) { + t.Setenv("SSH_AUTH_SOCK", "/tmp/agent.sock") + + withTransport := Env(EnvOptions{Transport: true}) + got, ok := value(withTransport, "SSH_AUTH_SOCK") + assert.True(t, ok) + assert.Equal(t, "/tmp/agent.sock", got) + + withoutTransport := Env(EnvOptions{}) + _, ok = value(withoutTransport, "SSH_AUTH_SOCK") + assert.False(t, ok) +} + +func TestEnv_UnsetTransportVarStaysAbsent(t *testing.T) { + // An unset SSH_AUTH_SOCK means "there is no agent", so it must be absent + // rather than exported empty. t.Setenv records the original for restoration; + // Unsetenv then removes it for the duration of the test. + t.Setenv("SSH_AUTH_SOCK", "placeholder") + require.NoError(t, os.Unsetenv("SSH_AUTH_SOCK")) + + env := Env(EnvOptions{Transport: true}) + _, ok := value(env, "SSH_AUTH_SOCK") + assert.False(t, ok) +} + +func TestEnv_LiteralOverridesInherited(t *testing.T) { + t.Setenv("PATH", "/host/bin") + + env := Env(EnvOptions{Transport: true, Literal: []string{"PATH=/pinned/bin"}}) + got, ok := value(env, "PATH") + assert.True(t, ok) + assert.Equal(t, "/pinned/bin", got) +} + +func TestEnv_PassthroughDeduplicatesWithTransport(t *testing.T) { + t.Setenv("PATH", "/host/bin") + + env := Env(EnvOptions{Transport: true, Passthrough: []string{"PATH"}}) + count := 0 + for _, e := range env { + if strings.HasPrefix(e, "PATH=") { + count++ + } + } + assert.Equal(t, 1, count) +} + +func TestEnv_HomeNotInSharedTransportList(t *testing.T) { + assert.NotContains(t, transportEnvNames, "HOME") +}