search

cloudfoundry / socks5-proxy

This is a go library for starting a socks5 proxy server via SSH
https://cloudfoundry.org
Apache License 2.0
52 stars 21 forks source link

Avoid deadlock in HostKey.Get with wrong private key #13

Closed jamesjoshuahill closed 5 years ago

jamesjoshuahill commented 5 years ago

This caused an issue in the BBR CLI, see https://github.com/cloudfoundry-incubator/bosh-backup-and-restore/issues/25. When the wrong private key is used in BOSH_ALL_PROXY the CLI hangs indefinitely.

This is a non-breaking change.

Josh and @MModhaPivotal

cfdreddbot commented 5 years ago

:white_check_mark: Hey jamesjoshuahill! The commit authors and yourself have already signed the CLA.

ciphercules commented 5 years ago

Hey @jamesjoshuahill and @MModhaPivotal, really appreciate the PR and nice work on finding this bug (its tricky) 👯‍♂️

After looking at this a bit, we're think we found a hybrid solution (that we pushed today) that maintains the goroutine while solving the issue of more writes than reads happening to the publicKeyChannel. We used your tests too, they were very helpful!

We wanted to keep the goroutine because switching from a non-blocking call to a blocking call could be a big change, and we wanted to keep the changes to a minimum while still solving the bug. We're open to discussion.

https://github.com/cloudfoundry/socks5-proxy/commit/7ade90b228c8472a45c189e77f78497b93bbaeaa

cc: @zachgersh

jamesjoshuahill commented 5 years ago

Hi @Polar-Beard and @zachgersh

Thank you for reviewing our PR 😄

We didn't consider the impact of removing the go routine on consumers. Thank you for addressing this in the hybrid solution.

Also, thank you very much for:

💯Maximum community points!

Many thanks Josh

cc: @cloudfoundry/bosh-backup-and-restore-team