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

Fix a bug in socks5 when rendering remote address #3110

Merged
merged 2 commits into from
Nov 5, 2019

Conversation

jieyu
Copy link
Contributor

@jieyu jieyu commented Oct 24, 2019

When the remote address is an IP address, we cannot directly convert
byte array to string. Otherwise, we will see hex strings like
"\xac\x11\x01\xc8:443".

See more details about the bug in:
https://community.gravitational.com/t/ssh-dynamic-port-forwarding-not-working/432

The above reported issue is resolved after applying the patch.

@gravitational-jenkins
Copy link

Can one of the admins verify this patch?

@webvictim
Copy link
Contributor

ok to test

@jieyu
Copy link
Contributor Author

jieyu commented Oct 24, 2019

hum, I cannot see test logs. Can someone check for me?

@webvictim
Copy link
Contributor

@jieyu

FAIL: socks_test.go:48: SOCKSSuite.TestHandshake

socks_test.go:69:
    c.Assert(string(buf), check.Equals, remoteAddr)
... obtained string = "?6578616d706c65"
... expected string = "example.com:443"

OOPS: 0 passed, 1 FAILED
--- FAIL: TestSocks (0.01s)

@jieyu
Copy link
Contributor Author

jieyu commented Oct 25, 2019

OK, i'll fix. I think we should distinguish between DNS name and IP addresses.

When the remote address is an IP address, we cannot directly convert
byte array to string. Otherwise, we will see hex strings like
"\xac\x11\x01\xc8:443".

See more details about the bug in:
https://community.gravitational.com/t/ssh-dynamic-port-forwarding-not-working/432

The above reported issue is resolved after applying the patch.
@jieyu
Copy link
Contributor Author

jieyu commented Oct 25, 2019

@webvictim can you take another look? I updated the code and the test.

@jieyu
Copy link
Contributor Author

jieyu commented Oct 25, 2019

ping @klizhentas @russjones

@webvictim
Copy link
Contributor

Looks like the test passes now @jieyu - the integration test is failing but that's probably unrelated.

@jieyu
Copy link
Contributor Author

jieyu commented Oct 28, 2019

@webvictim how can I get this bug fix reviewed and merged? This is blocking our progress

@klizhentas
Copy link
Contributor

Thanks for your patience. We are currently overloaded with requests. We will get to this issue in the context of 4.2 milestone in a couple of weeks from now.

@klizhentas klizhentas added this to the 4.2 "Alameda" milestone Oct 28, 2019
@webvictim
Copy link
Contributor

@jieyu You can always build Teleport yourself from source with your patch included if you like.

@jieyu
Copy link
Contributor Author

jieyu commented Oct 28, 2019

@webvictim we could, but that means our customers will have to use a forked version of teleport, which I am trying to avoid. This is clearly a bug, so I expect not too much trouble merging this.

Copy link
Contributor

@klizhentas klizhentas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@russjones can you please review sometime this week?

@klizhentas
Copy link
Contributor

@jieyu the fix looks quite straightforward to me, we will try to wrap up the review this week. What version are you using?

@jieyu
Copy link
Contributor Author

jieyu commented Oct 31, 2019

@klizhentas thanks for the review! we're using the latest. So if this can be included in the upcoming release, that'll be sufficient.

@klizhentas
Copy link
Contributor

sounds good to me, thank you for the patch.

@benarent benarent added the bug label Nov 1, 2019
@klizhentas klizhentas merged commit a1542f5 into gravitational:master Nov 5, 2019
@benarent benarent mentioned this pull request Nov 9, 2019
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants