resurrect ssh multiplexing

[ndeloof]

- What I did
resurrect #2132 which has been reverted due to CI flakyness

- How I did it
git revert + sme tweaks to adjust to current codebase

- How to verify it

- Description for the changelog
Add multiplexing to ssh connection helper so commands that do multiple requests share the connection. Environment variables for opting out and controlling the persist time for even better connection sharing.

- A picture of a cute animal (not mandatory but encouraged)

[codecov-commenter]

Codecov Report

❌ Patch coverage is 0% with 13 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
cli/connhelper/connhelper.go	0.00%	13 Missing ⚠️

📢 Thoughts on this report? Let us know!

[thaJeztah]

Some minor linting issues; one of them is probably to use 0o700 instead of 0700 (the new format with the o for octal)

59.95 cli/connhelper/connhelper.go:10: File is not `gofumpt`-ed (gofumpt)
59.95 	"strconv"
59.95 cli/connhelper/connhelper.go:86: File is not `gofumpt`-ed (gofumpt)
59.95 	if err := os.MkdirAll(config.Dir(), 0700); err != nil {
59.95 cli/connhelper/connhelper.go:89: File is not `gofumpt`-ed (gofumpt)
59.95 	sshFlags = append(sshFlags, "-o", "ControlMaster=auto", "-o", "ControlPath=" + config.Dir() + "/%r@%h:%p")
59.95 cli/connhelper/connhelper.go:103: File is not `gofumpt`-ed (gofumpt)
59.95 
59.95 cli/connhelper/connhelper.go:102: File is not `goimports`-ed (goimports)

[thaJeztah]

Oh! And looks like you missed adding DCO sign-offs 😅

[docker-agent]

Assessment: 🔴 CRITICAL

The SSH multiplexing implementation has two high-severity bugs where addMultiplexingArgs returns nil instead of the original sshFlags, silently dropping all previously-accumulated flags (including the connect timeout set by addSSHTimeout) in two paths: when DOCKER_SSH_NO_MUX is truthy, and when os.MkdirAll fails. Both should return sshFlags unchanged. Additionally, the DOCKER_SSH_NO_MUX env var silently ignores common Unix values like yes, and DOCKER_SSH_MUX_PERSIST is passed unsanitised to SSH.

[docker-agent]

[HIGH] addMultiplexingArgs returns nil (drops all existing sshFlags) when DOCKER_SSH_NO_MUX is truthy

When DOCKER_SSH_NO_MUX is set to a truthy value (e.g. "true", "1"), the function returns nil instead of the original sshFlags. The caller at line 56 does sshFlags = addMultiplexingArgs(sshFlags), so all flags previously appended by addSSHTimeout (e.g. -o ConnectTimeout=30) and any caller-supplied flags from GetConnectionHelperWithSSHOpts are silently discarded. The connection then proceeds without a connect timeout and without user-supplied flags.

Fix: Return sshFlags (unchanged) instead of nil:

if b, err := strconv.ParseBool(v); err == nil && b {
    return sshFlags  // not nil
}

[thaJeztah]

yeah, that doesn't look right; fixed

[docker-agent]

Thanks for the quick fix! Confirmed — line 93 now correctly returns sshFlags instead of nil, preserving the timeout and any user-supplied flags when multiplexing is disabled.

[docker-agent]

[HIGH] addMultiplexingArgs returns nil (drops all existing sshFlags) on MkdirAll error

When os.MkdirAll(config.Dir(), 0o700) fails (e.g. permissions error, read-only filesystem), the function returns nil, discarding all previously-accumulated sshFlags. This silently removes -o ConnectTimeout=30 (added by addSSHTimeout) and any caller-supplied flags on every connection attempt where the config directory is unavailable. The intent is clearly to fall back gracefully — but nil is not a graceful fallback.

Fix: Return the original sshFlags unchanged so the connection still benefits from the timeout and other flags:

if err := os.MkdirAll(config.Dir(), 0o700); err != nil {
    return sshFlags  // not nil — multiplexing skipped, but existing flags preserved
}

[docker-agent]

[MEDIUM] DOCKER_SSH_NO_MUX=yes silently fails to disable multiplexing — only Go ParseBool strings accepted

strconv.ParseBool only recognises "1", "t", "T", "TRUE", "true", "True", "0", "f", "F", "FALSE", "false", "False". A user setting DOCKER_SSH_NO_MUX=yes (the common Unix/shell convention used by many tools) will have ParseBool return an error, making the condition err == nil && b false, and multiplexing will not be disabled — the env var is silently ignored.

The documentation in docker.md only says "If set will turn off SSH multiplexing" without clarifying which values are accepted, so users will be surprised.

Fix: Use the simpler v != "" check (consistent with how Docker treats DOCKER_TLS, NO_COLOR, etc.) or at minimum document the accepted values:

if v := os.Getenv("DOCKER_SSH_NO_MUX"); v != "" {
    return sshFlags
}

[docker-agent]

[MEDIUM] DOCKER_SSH_MUX_PERSIST value passed unsanitised into SSH -o ControlPersist option

The value of DOCKER_SSH_MUX_PERSIST is appended directly to the SSH flag string without any validation. While the SSH client receives it as a separate argv element (not a shell string, so no shell injection), there is no check that the value is a valid SSH duration (e.g. 60, 60s, yes, no). A malformed value (e.g. an empty suffix, a value with spaces) will be passed to the SSH client as-is and may result in confusing SSH errors rather than a clear user-facing message.

Suggestion: Validate the value against the expected format (integer seconds or SSH duration string) before appending, or at minimum document what values are accepted in the env var table.