Skip to content
This repository has been archived by the owner on Jan 30, 2020. It is now read-only.

ssh: Do not leak ssh sessions on errors #1540

Merged
merged 1 commit into from
Apr 18, 2016

Conversation

xh3b4sd
Copy link

@xh3b4sd xh3b4sd commented Apr 8, 2016

Hello there. We recently ran into a problem where we were not able to do proper connection handling across ssh tunnels. It turned out that this was partially caused by some buggy code on our side in combination with the things being fixed here in this PR. In case ssh sessions are created, we need to make sure to close them properly even in error cases. The fix proposed here works for us. What do you guys think? Thanks to @zeisss helping out debugging and fixing this issue.

@zeisss
Copy link
Contributor

zeisss commented Apr 8, 2016

Some more background infos:

We are using the client/ package over ssh, similar to fleetctl. When our code tries to perform multiple operations in parallel, we ran into forward request denied errors for call 2-10 to ssh.DialCommand(). Afterwards we got administratively prohibited errors for call 11 onwards. This correlates with the default value MaxSessions in sshd. This PR closes the created session if any of the following steps fail.

@antrik
Copy link
Contributor

antrik commented Apr 8, 2016

This looks reasonable at a quick glance. Any chance you could create a functional test case for this?...

@kayrus
Copy link
Contributor

kayrus commented Apr 8, 2016

Excellent. This PR actually fixes #1425 issue in proper way. That is why fleet tried to send several agent requests... @antrik functional tests are already written (#1492). What we need to do it to revert my fix workaround (ea415ad) and apply this patch.

P.S. I had feeling that I fixed that wrong, but didn't have enough time to investigate.

@zeisss
Copy link
Contributor

zeisss commented Apr 8, 2016

Just rereading my comment I think I missed an info: this PR fixed the administratively prohibited errors for us, and we always now got forward request denied.

We fixed that then by adding a mutex to the RoundTrip() which unlocks when the body gets closed (see giantswarm/inago@7b0d822) - not sure sthg similar needs to be done in fleetctl.

@jonboulle jonboulle added this to the v0.13.0 milestone Apr 11, 2016
@kayrus
Copy link
Contributor

kayrus commented Apr 11, 2016

@zeisss false alarm. my tests succeed because my VM had not enough RAM (256). And functional tests returned positive answer for some reason. My fix should not be reverted. Your and my fixes supplement each other.

@tixxdz
Copy link
Contributor

tixxdz commented Apr 18, 2016

lgtm.

Thank you!

@tixxdz tixxdz merged commit 452a551 into coreos:master Apr 18, 2016
@xh3b4sd
Copy link
Author

xh3b4sd commented Apr 18, 2016

Wh00p wh00p.

@jonboulle
Copy link
Contributor

👍 nice

@zeisss zeisss deleted the do-not-leak-ssh-sessions branch May 3, 2016 10:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants