Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

x/crypto/ssh: leaks connections? #28613

Closed
FabianGa opened this issue Nov 6, 2018 · 7 comments
Closed

x/crypto/ssh: leaks connections? #28613

FabianGa opened this issue Nov 6, 2018 · 7 comments
Labels
FrozenDueToAge
Milestone
Unreleased

Comments

@FabianGa
Copy link

FabianGa commented Nov 6, 2018

What version of Go are you using (go version)?

go version go1.11.2 darwin/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

i cross compile with:

 GOOS=linux GOARCH=amd64 go build

for SLES 12.3

go env for build env:

GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/d055627/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/d055627/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/Cellar/go/1.11.2/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.11.2/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/cx/rzjjsvf10z97t_pbf0669mpw0000gr/T/go-build829881747=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

i followed the Dial example on:
https://godoc.org/golang.org/x/crypto/ssh#Dial

exact code:

//SSHConnectAndRun runs a command per ssh on a host as a user and return the STDOUT from the command or an error
func SSHConnectAndRun(host, port, user, command string) (string, error) {
	pkey := parsekey(filepath.Join(os.Getenv("HOME"), ".ssh", "id_rsa"))
	hostKey := findHostKey(host)

config := &ssh.ClientConfig{
	User: user,
	Auth: []ssh.AuthMethod{
		ssh.PublicKeys(pkey),
	},
	HostKeyCallback: ssh.FixedHostKey(hostKey),
	Timeout:         1 * time.Minute,
}

//dial
client, err := ssh.Dial("tcp", host+":"+port, config)
if err != nil {
	return "", fmt.Errorf("ssh dial error: host: %s error: %s", host, err)
}

//session
session, err := client.NewSession()
if err != nil {
	return "", fmt.Errorf("ssh session error: host: %s error: %s", host, err)
}
defer session.Close()

//run command
var b bytes.Buffer
session.Stdout = &b
if err := session.Run(command); err != nil {
	return "", fmt.Errorf("ssh run error: host: %s command: %s error: %s", host, command, err)
}

return b.String(), nil



}

This leads newly (i updated recently the ssh repo) to the following error message of the ssh server:

2018/11/05 17:43:04 ssh dial error: host: xxx error: ssh: handshake failed: ssh: disconnect, reason 12: Too many concurrent connections (64) - max. allowed: 64

The ssh server restricts to max 64 concurrent connections per user.
It seems like some change in the library code misses internal cleanup or do i need additional cleanup on my side?

thanks
regards Fabian
@gopherbot gopherbot added this to the Unreleased milestone Nov 6, 2018
@bradfitz
Copy link
Contributor

bradfitz commented Nov 6, 2018

If your session setup fails, you still need to close the client.

@FabianGa
Copy link
Author

FabianGa commented Nov 7, 2018

thanks, I will cover this case.

but that does not explain the 64 open connections. I got only one error before the 64 connection limit was reached.

the error was:

2018/11/05 14:56:00 ssh run error: host: xxx command: xxx error: wait: remote command exited without exit status or exit signal

i will try with a

defer client.Close()

after the successful dial, to ensure that the connection is closed. But it seems this was done earlier somehow by the library itself.

@FabianGa
Copy link
Author

FabianGa commented Nov 7, 2018

i have checked, and the defer client.Close() does work, it closes the connection.
if I leave this line out, I can monitor via netstat or lsof that the connections are not closed, without happening of any errors previously.

So it seems there was a change recently that makes this call mandatory. (maybe the connection was previously closed during session.Close()?)

@bradfitz
Copy link
Contributor

bradfitz commented Nov 7, 2018

I'm not aware of any recent changes, and as long as I can remember it's been required to call close.

You might've just got lucky before.

/cc @hanwen in any case he remembers anything.

But otherwise I'm going to close this as nothing to do.

@bradfitz bradfitz closed this as completed Nov 7, 2018
@hanwen
Copy link
Contributor

hanwen commented Nov 7, 2018

as long as you don't close the client, you can start new sessions using same the client. This is useful because then you don't have to a new handshake, or do a new login for each new session.

@FabianGa
Copy link
Author

FabianGa commented Nov 7, 2018

my client was running nearly 2 month without reaching the 64 connection limit before.
But if you did not change a thing, than maybe the switch from go 1.11.1 to 1.11.2 did the thing...

thank for your answer and your time!

Could you please update the documentation about this, at least the example from Dial should contain imho the call to Close().

@bradfitz
Copy link
Contributor

bradfitz commented Nov 7, 2018

But if you did not change a thing, than maybe the switch from go 1.11.1 to 1.11.2 did the thing...

I promise that it did not.

@golang golang locked and limited conversation to collaborators Nov 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@bradfitz @hanwen @gopherbot @FabianGa