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:

  • remoteAddrFromEnv splits the value once and returns the first field only when there is one, so it can no longer panic.
  • NewFromEnv treats a whitespace-only SSH_CONNECTION as 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 gets Only SSH allowed, the same as an unset variable, rather than going on with an empty RemoteAddr, which would leave check_ip out of the /api/v4/internal/allowed request 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

  1. Run the package tests:

    go test -count=1 -race ./internal/sshenv/...

    With these tests on main's sshenv.go, the whitespace-only case of TestNewFromEnv and TestBlankRemoteAddrFromEnv both panic at sshenv.go:47 (run them one at a time: the first panic stops the package), and the other new cases pass, since they pin what main already does. With only the remoteAddrFromEnv change, the whitespace-only case fails on IsSSHConnection. With the whole change they all pass.

  2. To see it in the binary, build it into a temporary directory (it reads config.yml from the directory above its own, so nothing in the checkout changes) and run it with SSH_CONNECTION set 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 main the first run panics with exit code 2. With this change it prints Only SSH allowed and 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 empty support/lint_last_known_acceptable.txt) and go vet ./...: pass.
  • make test, make test_race and CGO_ENABLED=0 go test -count 1 ./...: pass, no failing package.
  • nilaway at the version .gitlab-ci.yml pins: 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 +0x6b

With 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.015s

The 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.

Edited by José M. Requena Plens

Merge request reports

Loading
Loading