Do not panic on a whitespace-only SSH_CONNECTION
What does this MR do and why?
Closes #881 (closed)
remoteAddrFromEnv in internal/sshenv/sshenv.go checks that SSH_CONNECTION is not empty and then returns strings.Fields(address)[0]. A value that holds only whitespace passes that check, strings.Fields returns an empty slice, and the index panics with index out of range [0] with length 0. NewFromEnv runs on every start of gitlab-shell, so such an environment ends the process with a Go panic (exit code 2) instead of the Only SSH allowed error an unset variable gets.
This MR makes two changes in internal/sshenv/sshenv.go:
remoteAddrFromEnvsplits the value once and returns the first field only when there is one, so it can no longer panic.NewFromEnvtreats a whitespace-onlySSH_CONNECTIONas unset. sshd sets the variable to the client and server addresses and ports, so a value with no field in it does not come from sshd. Such an invocation now getsOnly SSH allowed, the same as an unset variable, rather than going on with an emptyRemoteAddr, which would leavecheck_ipout of the/api/v4/internal/allowedrequest and run the access check without a client IP.
A well-formed SSH_CONNECTION gives the same IsSSHConnection and RemoteAddr as before. Only an environment built outside sshd can produce a whitespace-only value: with OpenSSH 10.0p2, sshd replaced a value sent by the client (AcceptEnv *), an environment= option in authorized_keys, ~/.ssh/environment (PermitUserEnvironment yes) and a SetEnv line in sshd_config with its own. gitlab-sshd does not read the variable.
If you would rather only fix the panic and keep IsSSHConnection true for any non-empty value, I can set it from os.Getenv(SSHConnectionEnv) != "" again, as on main, and have the whitespace-only table case, renamed to match, expect Env{IsSSHConnection: true}; such an invocation then goes on to the access check without a client IP.
How to set up and validate locally
-
Run the package tests:
go test -count=1 -race ./internal/sshenv/...With these tests on
main'ssshenv.go, the whitespace-only case ofTestNewFromEnvandTestBlankRemoteAddrFromEnvboth panic atsshenv.go:47(run them one at a time: the first panic stops the package), and the other new cases pass, since they pin whatmainalready does. With only theremoteAddrFromEnvchange, the whitespace-only case fails onIsSSHConnection. With the whole change they all pass. -
To see it in the binary, build it into a temporary directory (it reads
config.ymlfrom the directory above its own, so nothing in the checkout changes) and run it withSSH_CONNECTIONset to a space, then unset:d=$(mktemp -d) go build -o "$d/bin/gitlab-shell" ./cmd/gitlab-shell cat > "$d/config.yml" <<'EOF' gitlab_url: "http://127.0.0.1:9" secret_file: .gitlab_shell_secret log_file: gitlab-shell.log EOF echo secret > "$d/.gitlab_shell_secret" SSH_CONNECTION=' ' SSH_ORIGINAL_COMMAND="git-upload-pack 'group/project.git'" "$d/bin/gitlab-shell" key-1; echo "exit $?" env -u SSH_CONNECTION SSH_ORIGINAL_COMMAND="git-upload-pack 'group/project.git'" "$d/bin/gitlab-shell" key-1; echo "exit $?"On
mainthe first run panics with exit code 2. With this change it printsOnly SSH allowedand exits with 1, as the second run does on both. No GitLab instance is needed.
What I ran
In registry.gitlab.com/gitlab-org/gitlab-build-images/debian-trixie-golang-1.25:latest (go1.25.14), the image the tests and lint jobs use, after the test job's make build and cp config.yml.example config.yml, without the Gitaly service (so the gitlab-sshd acceptance tests skip):
make verify,make lint(golangci-lint 2.12.2, no difference from the emptysupport/lint_last_known_acceptable.txt) andgo vet ./...: pass.make test,make test_raceandCGO_ENABLED=0 go test -count 1 ./...: pass, no failing package.nilawayat the version.gitlab-ci.ymlpins: no findings.
With Go 1.26.9 and Go 1.27.2: go test -race -count=1 on internal/sshenv and every package that imports it: pass.
Without the change, with Go 1.25.10 (the go.mod version):
--- FAIL: TestNewFromEnv/It_treats_a_whitespace-only_SSH_CONNECTION_as_unset (0.00s)
panic: runtime error: index out of range [0] with length 0 [recovered, repanicked]
gitlab.com/gitlab-org/gitlab-shell/v14/internal/sshenv.remoteAddrFromEnv()
internal/sshenv/sshenv.go:47 +0x53
gitlab.com/gitlab-org/gitlab-shell/v14/internal/sshenv.NewFromEnv()
internal/sshenv/sshenv.go:37 +0x6bWith only the remoteAddrFromEnv change:
--- FAIL: TestNewFromEnv/It_treats_a_whitespace-only_SSH_CONNECTION_as_unset (0.00s)
expected: sshenv.Env{GitProtocolVersion:"", IsSSHConnection:false, OriginalCommand:"", RemoteAddr:"", NamespacePath:""}
actual : sshenv.Env{GitProtocolVersion:"", IsSSHConnection:true, OriginalCommand:"", RemoteAddr:"", NamespacePath:""}With the change:
--- PASS: TestNewFromEnv (0.00s)
--- PASS: TestNewFromEnv/It_parses_GIT_PROTOCOL (0.00s)
--- PASS: TestNewFromEnv/It_parses_SSH_CONNECTION (0.00s)
--- PASS: TestNewFromEnv/It_parses_SSH_CONNECTION_with_leading_whitespace (0.00s)
--- PASS: TestNewFromEnv/It_parses_SSH_CONNECTION_with_a_single_field (0.00s)
--- PASS: TestNewFromEnv/It_treats_an_empty_SSH_CONNECTION_as_unset (0.00s)
--- PASS: TestNewFromEnv/It_treats_a_whitespace-only_SSH_CONNECTION_as_unset (0.00s)
--- PASS: TestNewFromEnv/It_parses_SSH_ORIGINAL_COMMAND (0.00s)
--- PASS: TestRemoteAddrFromEnv (0.00s)
--- PASS: TestEmptyRemoteAddrFromEnv (0.00s)
--- PASS: TestBlankRemoteAddrFromEnv (0.00s)
ok gitlab.com/gitlab-org/gitlab-shell/v14/internal/sshenv 1.015sThe binary, built from each tree and started the way sshd's forced command starts it (gitlab-shell key-1 with SSH_ORIGINAL_COMMAND set), against a GitLab URL nothing listens on:
SSH_CONNECTION |
main |
this MR |
|---|---|---|
| unset | Only SSH allowed, exit 1 |
Only SSH allowed, exit 1 |
| a space, or a tab | panic, exit 2 | Only SSH allowed, exit 1 |
192.0.2.1 50000 192.0.2.2 22 |
Internal API unreachable, exit 1 |
Internal API unreachable, exit 1 |
MR acceptance checklist
Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.