Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#7751 enhancement closed fixed (fixed)

Support IPv6 hostnames in SSHConnectForwardingChannel

Reported by: mkuron Owned by: Adi Roiban
Priority: normal Milestone:
Component: conch Keywords:
Cc: z3p Branch: branches/conch-server-ipv6-fwd-7751
branch-diff, diff-cov, branch-cov, buildbot
Author: adiroiban

Description

In twisted.conch.ssh.forwarding, SSHConnectForwardingChannel's channelOpen uses connectTCP which does not support IPv6 hostnames (and according to https://twistedmatrix.com/trac/wiki/IPv6, never will). Suggested solution: use twisted.internet.endpoints.HostnameEndpoint, which transparently handles IPv4 and v6.

Attached is a script based on docs/conch/examples/sshsimpleserver.py which uses SSHConnectForwardingChannel. Run it like this:

python fw.py 
ssh -L 8888:www.google.com:80 -p 5022 user@localhost # password: password
curl localhost:8888

This works.

Now try an IPv6-only forwarding:

python fw.py 
ssh -L 8888:ipv6.google.com:80 -p 5022 user@localhost # password: password
curl localhost:8888

This does not work because there is no DNA A record on ipv6.google.com. Error message received:

2015-01-18 10:11:35+0100 [SSHService ssh-connection on SSHServerTransport,4,::1] got channel direct-tcpip request
2015-01-18 10:11:35+0100 [SSHChannel None (2) on SSHService ssh-connection on SSHServerTransport,4,::1] connecting to ipv6.google.com:80
2015-01-18 10:11:35+0100 [-] failed to connect: [Failure instance: Traceback (failure with no frames): <class 'twisted.internet.error.DNSLookupError'>: DNS lookup failed: address 'ipv6.google.com' not found: [Errno 8] nodename nor servname provided, or not known.
	]
2015-01-18 10:11:35+0100 [-] sending close 2

The attached script also contains the solution (run the script with the command-line argument --ipv6 to enable the solution), which is quite simple:

from twisted.internet.endpoints import HostnameEndpoint
class SSHForwardingClientFactory(protocol.Factory):
	channel = None
	def __init__(self, channel):
		self.channel = channel
	def buildProtocol(self, addr):
		return SSHForwardingClient(self.channel)
class SSHConnectForwardingChannel(channel.SSHChannel):
	def channelOpen(self, specificData):
		log.msg("IPv6 enabled, connecting to %s:%i" % self.hostport)
		ep = HostnameEndpoint(reactor, self.hostport[0], self.hostport[1])
		d = ep.connect(SSHForwardingClientFactory(self))
		d.addCallbacks(self._setClient, self._close)

Attachments (8)

fw.py (4.9 KB) - added by mkuron 3 years ago.
Example conch SSH server using SSHConnectForwardingChannel. It also contains a small fix utilizing HostnameEndpoint in order to support IPv6.
7751.diff (1.3 KB) - added by mkuron 3 years ago.
7751-test.diff (1.4 KB) - added by mkuron 3 years ago.
7751-both.diff (3.4 KB) - added by mkuron 3 years ago.
7751-both-2.diff (3.6 KB) - added by mkuron 3 years ago.
7751-both-2.diff should be complete now. The previous diff was missing a line.
0001-SSH-forwarding-for-IPv6-addresses.diff (2.5 KB) - added by mkuron 3 years ago.
0002-SSH-forwarding-for-IPv6-addresses.diff (2.5 KB) - added by mkuron 3 years ago.
0003-SSH-forwarding-for-IPv6-addresses.diff (2.9 KB) - added by mkuron 3 years ago.

Download all attachments as: .zip

Change History (40)

comment:1 Changed 3 years ago by DefaultCC Plugin

Cc: z3p added

Changed 3 years ago by mkuron

Attachment: fw.py added

Example conch SSH server using SSHConnectForwardingChannel. It also contains a small fix utilizing HostnameEndpoint in order to support IPv6.

comment:2 Changed 3 years ago by Jean-Paul Calderone

Owner: set to mkuron

Thanks. Can you:

  1. Contribute the implementation fix as a diff
  2. Add unit tests for the new functionality
  3. Add documentation for the new feature

Thanks.

comment:3 Changed 3 years ago by Jean-Paul Calderone

Summary: SSHConnectForwardingChannel does not support IPv6Support IPv6 in SSHConnectForwardingChannel
Type: defectenhancement

Changed 3 years ago by mkuron

Attachment: 7751.diff added

Changed 3 years ago by mkuron

Attachment: 7751-test.diff added

comment:4 Changed 3 years ago by mkuron

Patch and test attached in diff format. The forwarding module is completely undocumented, so I have no idea how I should document my change.

Regarding the test: the conch command-line client does not support IPv6 forwarding, only the OpenSSH supports it. For this reason, test_localToRemoteForwardingV6 in my patch checks for OpenSSHClientMixin and returns True if it's not there. This is so the test doesn't get marked as failed when run with the conch client. Could you change that fix that to mark the test as skipped instead of passed/failed as I was not able to find out how to do that with the mixins?

Changed 3 years ago by mkuron

Attachment: 7751-both.diff added

comment:5 Changed 3 years ago by mkuron

Nevermind, figured out how to only run the v6 test with the OpenSSH client. Attached 7751-both.diff, which contains both the patch and the test.

Changed 3 years ago by mkuron

Attachment: 7751-both-2.diff added

7751-both-2.diff should be complete now. The previous diff was missing a line.

comment:6 Changed 3 years ago by mkuron

Is there anything else you need from me? I've been using 7751-both-2.diff in production for the past two weeks now, it appears to be working perfectly.

comment:7 Changed 3 years ago by Jean-Paul Calderone

Keywords: review added

Is there anything else you need from me? I've been using 7751-both-2.diff in production for the past two weeks now, it appears to be working perfectly.

Don't forget to add the "review" keyword to the ticket. It's not very likely anyone will think to look at the ticket otherwise.

comment:8 Changed 3 years ago by Jean-Paul Calderone

Owner: mkuron deleted

comment:9 Changed 3 years ago by Adi Roiban

Keywords: review removed
Owner: set to mkuron

Many thanks for the patch!


I will create a branch and send it to buildbot so you can see the "official" test results.

... but the latest patch requires a few minor changes.


It uses a strange indentation

self.echoServer = reactor.listenTCP(0, EchoFactory(), 
                                     interface="::") 

---

Just a minor comment. Starting a test doctstring with "Test that" is a bad practice as this does not add anything new. All test are written to "test that something does something".

For ForwardingMixinV6 you can document it by referring to ConchServerSetupMixin... otherwise each class inheriting ConchServerSetupMixin will have re-document what ConchServerSetupMixin does.

For the test doctrings you can be brief. "Can forward arbitrary over IPv6 protocol."


As you said, for ForwardingMixinV6 those arguments are specific to OpenSSH client so maybe is better to just extend OpenSSHClientMixin.

You can add a ticket to implement IPV6 forwarding in conch client and then update/refactor the test.


With current approach you will need to create a boilerplate factory for each protocol. Boilerplate code is bad and is a sign that things need to be improved.

I am not familiar with endpoints... but looking at https://twistedmatrix.com/documents/current/core/howto/endpoints.html there is a better way

Instead of

d = ep.connect(SSHForwardingClientFactory(self))

I was expecting something like

d = ep.connect(Factory.forProtocol(SSHForwardingClient, self))

This would also reduce the need to add docstrings.


For SSHForwardingClientFactory, does the "channel" instance member need to be public? I hope that we can get rid of SSHForwardingClientFactory, but just for future review. Try to keep the public interface as thin as possible


Thanks again for the patch!

comment:10 Changed 3 years ago by Adi Roiban

Author: adiroiban
Branch: branches/conch-server-ipv6-fwd-7751

(In [43809]) Branching to conch-server-ipv6-fwd-7751.

comment:11 Changed 3 years ago by Adi Roiban

The patch will also need a news file as described here https://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles

$ patch -p0 < ~/Downloads/7751-both-2.diff 
patching file twisted/conch/ssh/forwarding.py
patching file twisted/conch/test/test_conch.py
Hunk #1 succeeded at 323 (offset 12 lines).
Hunk #2 FAILED at 382.
Hunk #3 FAILED at 507.
2 out of 3 hunks FAILED -- saving rejects to file twisted/conch/test/test_conch.py.rej

I tried to apply the patch on latest trunk, but I got conflicts.

Please send a new diff which does not produce conflicts when applied to trunk.

Here are some info about creating diff https://twistedmatrix.com/trac/wiki/VcsWorkflows Thanks!

Changed 3 years ago by mkuron

comment:12 Changed 3 years ago by mkuron

Here's a patch that addresses all your comments and applies to the current trunk.

comment:13 Changed 3 years ago by mkuron

Keywords: review added
Owner: mkuron deleted

comment:14 Changed 3 years ago by Adi Roiban

Keywords: review removed
Owner: set to mkuron

Thanks for the patch. I was able to apply it without problems :) Thanks!

for ep.connect(protocol.Factory.forProtocol(lambda: SSHForwardingClient(self)))

why do you need to lambda? You can use something similar to initial suggestion

ep.connect(protocol.Factory.forProtocol(SSHForwardingClient, self))


Your patch adds a line with only white spaces in test_conch.py:525


It looks like this is a change for all tests, including ipv4 ones self.echoServer = reactor.listenTCP(0, EchoFactory(), interface="::")

Maybe it needs a separate port for IPV4 tests.

I don't like the echoServer name as it is too generic and I had to dig deeper to understand where and how it is used. I think that forwardCollectorServer / forwardingReceiverIPv6 is a better name.


I know that it was my request to be concise in the test docstring but this Can forward arbitrary over IPv6 protocol. is to short... it forwards arbitrary what ?


Now the big progblem with the test.

Try to revert your changes in twisted/conch/ssh/forwarding.py and run your new test.

On my computer it still pass. This should not be the case, as the test is suppose to fail if support for ipv6 is there..

So either we have ipv6 support or the test is bad and still uses ipv4

This needs to be fixed.

Please write your test first using the trunk code and see that the test fails. Then you can write your patch and see that test pass. Then you can run all tests to see that you change has no regressions.

One hint is that instead of using IPV6_ANY :: address, try to use the IPV6_LOOPBACK ::1 address ... but as suggested above, you will need a dedicated echo server, listening only on IPV6 as all other IPV4 tests will fail.

Thanks!

Changed 3 years ago by mkuron

comment:15 Changed 3 years ago by mkuron

Keywords: review added
Owner: changed from mkuron to 107269088

I have no idea why I need the lambda; all I can say is that it doesn't work without it. Logically, the two should be the same, but in practice, if I do it the way you suggested, the test just never returns.

I tracked down why the test also succeeded on trunk: connectTCP supports IPv6 address literals, but not hostnames that resolve to an IPv6 address (which is why I switched to using the HostnameEndpoint). So to make the test work, instead of [::1], we need to use a hostname that resolves to ::1. I created such a hostname at a free DNS provider for testing, but we need to find a permanent solution.

I could split the echoServer into one for v4 and one for v6, though it doesn't seem that's necessary. By default, listenTCP listens on all IPv4 interfaces (not only loopback), and adding interface="::" makes it listen on all IPv6 interfaces as well.

comment:16 Changed 3 years ago by mkuron

Owner: 107269088 deleted

comment:17 Changed 3 years ago by Jean-Paul Calderone

why do you need to lambda? You can use something similar to initial suggestion

Nope, you can't. Factory.forProtocol(protocol, *args, **kwargs) is shorthand for:

f = Factory(*args, **kwargs)
f.protocol = protocol

comment:18 in reply to:  17 Changed 3 years ago by Adi Roiban

Keywords: ipv6 review removed
Owner: set to mkuron

Thanks for the update!

I could split the echoServer into one for v4 and one for v6, though it doesn't seem that's necessary. By default, listenTCP listens on all IPv4 interfaces (not only loopback), and adding interface="::" makes it listen on all IPv6 interfaces as well.

I find it necessary as it should help detect broken tests. From my part, it is important that tests are as strict as possible. But we can wait for feedback another developer.


Regarding HostnameEndpoint. I think that this is OK as from rfc 4254 section 7.1 I can see that SSH forwarding requests addresses can be IPv4/v6 or hostnames.

From what I can see, you tests are not using ::1, but ::, for the echo server part. I did a quick try on my local computer, and when I set the echo server to listen to ::1 I obsevers that ipv4 tests were failing. with :: ipv4 tests pass.


Replying to exarkun:

why do you need to lambda? You can use something similar to initial suggestion

Nope, you can't. Factory.forProtocol(protocol, *args, **kwargs) is shorthand for:

f = Factory(*args, **kwargs)
f.protocol = protocol

True. Many thanks! I read the docs again and it seems that for client side endpoints we need:

# instead of
d = ep.connect(protocol.Factory.forProtocol(lambda: SSHForwardingClient(self)))
# we should have
d = connectProtocol(ep, SSHForwardingClient(self))

--

Please take a look at my comment and address or resumit for rewiew so that another person can take a look.

removing ipv6 tag as it is not documented. If we need ipv6 tags, than we should document them https://twistedmatrix.com/trac/wiki/BugKeywords

Thanks!

Changed 3 years ago by mkuron

comment:19 Changed 3 years ago by mkuron

Keywords: review added
Owner: mkuron deleted

I've split the echoServer into one for V4 and one for V6 and switched to connectProtocol.

The one issue that remains open and that I'm unsure how to fix is that the test case depends on the domain name ip6-localhost.us.to resolving to ::1. We definitely need to use a hostname there because the {{{connectTCP}}-based variant in trunk already works with IPv6 address literals; however I can't guarantee that this domain name will remain on the internet forever and it obviously breaks the test when running locally without an internet connection.

comment:20 Changed 3 years ago by Jean-Paul Calderone

You should write the test so that it doesn't do real network I/O. The test is for Conch-level functionality, not for aspects of the reactor implementation. If you want to test that Conch attempts an IPv6 address connection, make the test supply a fake resolver that turns some arbitrary hostname into an IPv6 address (by itself, without actually doing any DNS).

comment:21 Changed 3 years ago by mkuron

I've tried writing a fake resolver (inspired by FakeResolver in twisted.internet.test.test_tcp, but simply couldn't get it to actually be used by the reactor.

comment:22 Changed 3 years ago by Adi Roiban

Thanks for the patch. I have applied it on top of dedicated branch.

I have checked the current code, and as described in #5058, connectTCP already support ipv6 address... just that hostnames are sent to an IPv4 only resolver. I assume this is done for backward compatibility.

Now, if you rewrite to new test using sshArgs='-N -L%i:[::1]:%i' you can see that it will still pass with old implementation based on connectTCP. I have injected a debugger in SSHConnectForwardingChannel._setClient and I can see that the transport is connected to an IPV6 address.


So I guess that the ticket description should be updated to "Support IPv6 hostnames in SSHConnectForwardingChannel" and HostnameEndpoint is the way to go.


OpenSSHClientForwardingTests is functional / system / inter-operatibilty test. From my point of view writing a functional test for SSH forwarding using IPV6 hostname would be in fact a functional test for HostnameEndpoint. Also, functional/system test should not be moked/injected as by doing this they become unit/integration tests.

So, writing an unit/integration test for SSHConnectForwardingChannel.channelOpen should help cover the new changes. Current code uses the global reactor, so you might need/want to inject the reactor into the SSHConnectForwardingChannel instance.

I tried to do a quick search in current code and it looks like there are no unit/integration tests for SSHConnectForwardingChannel ... just the high level OpneSSH interoperability test. Let me know if you need help with the test and I will look at providing an example.

Thanks again for your work and feedback on this issue!

comment:23 Changed 3 years ago by mkuron

Summary: Support IPv6 in SSHConnectForwardingChannelSupport IPv6 hostnames in SSHConnectForwardingChannel

comment:24 Changed 3 years ago by mkuron

Ah, just noticed why a custom resolver didn't help here: the name resolution is performed by the OpenSSH client. That of course always uses the operating system's name resolution infrastructure.

I assume the internet.endpoints tests already test for HostnameEndpoint's IPv6 compatibility. So the existing IPv4 forwarding OpenSSH test is sufficient to ensure that Conch is indeed using HostnameEndpoint properly and therefore technically it shouldn't be necessary to even test IPv6 separately. Of course we need a test that fails on trunk and passes on the dedicated feature branch, and that criterion is met by my OpenSSH test, albeit only when running against a real DNS server.

I really don't know how I should go about writing a separate test as you described. I also don't know anything about the way Twisted's tests work; I only created mine by copying and modifying the existing IPv4 one. I don't think I'll be able to write a test from scratch that tests a completely different piece of code and would really appreciate help there.

comment:25 Changed 3 years ago by Adi Roiban

Keywords: review removed
Owner: set to Adi Roiban

Name resolution is performed by Twisted. OpenSSH will just send whatever name if ask it.

This is a feature as you can forward to private hostname which are known only by the forwarding server.

You can see that conch SSH server receives the request as hostname address.

$ ./bin/trial twisted.conch.test.test_conch.OpenSSHClientForwardingTests.test_localToRemoteForwardingV6
twisted.conch.test.test_conch
  OpenSSHClientForwardingTests
    test_localToRemoteForwardingV6 ... > /home/adi/chevah/twisted/twisted/conch/ssh/forwarding.py(79)channelOpen()
-> ep = HostnameEndpoint(reactor, self.hostport[0], self.hostport[1])
(Pdb) self.hostport
('ip6-localhost.us.to', 44997)

Your new test is good as a high level functional tests

I will try to work on top of your patch and write a unit/integration tests which checks that ssh forwarding will not only work with IP address but also use a resolver to resolve hostnames... then I will ask for a new review.

Thanks!

comment:26 Changed 3 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

Please check latest rev on the associated branch.

I am still learning about endpoints / reactor connectTCP so the patch/test might be wrong.

While writing the test I had to read the code for SSHConnectForwardingChannel and while trying to understand how it works, I have documented it.

I think that makeTCPConnection should be moved into MemoryReactor, but in a separate ticket and with a better method for associating factories to fake connectors.

Test is run with a fake HostnameEndpoint._nameResolution

There were no unit/integration tests for SSHConnectForwardingChannel so I have created a new test module. For now I wrote a single test which checks that HostnameEndpoint is used to resolve requested names.

I have changed the functional IPV6 OpenSSH test to use ::1 ipv6 address literal.

branch was sent to buildbot... waiting for results.

Thanks!

comment:27 Changed 3 years ago by Glyph

Keywords: review removed
Owner: set to Adi Roiban

Woohoo, replacing more usages of connectTCP by HostnameEndpoint is great :).

Thanks for doing this, adi!

Please fix the following minor issues and land:

  1. patchHostnameEndpointResolver's docstring looks like its docstring ends in the middle of a sentence; please complete it.
  2. one minor epytext issue on the builder

For future reference, it would be nice to have something like makeTCPConnection built in to MemoryReactor itself; having it capable of emulating a tiny Internet where multiple transports can listen and connect "realistically" would be a very nice testing addition, since partial versions of this are mocked throughout Twisted. Just something to keep an eye out for refactoring in the future.

comment:28 Changed 3 years ago by Adi Roiban

Resolution: fixed
Status: newclosed

(In [44329]) Merge conch-server-ipv6-fwd-7751: Support IPv6 hostnames in SSHConnectForwardingChannel.

Author: mkuron, adiroiban Reviewer: exarkun, glyph Fixes: #7751

comment:29 Changed 3 years ago by Jean-Paul Calderone

I didn't really review this ticket. On one hand, thanks for trying to spread credit as widely as reasonable (I did comment on the ticket, at least). On the other hand, this implies that I've consented to these changes - which I haven't.

comment:30 Changed 3 years ago by Adi Roiban

As described here https://github.com/twisted/twisted-dev-tools/issues/9 I always find it hard to know who are the reviewers for a branch.

For me any person who add a comment related to the patch is a reviewer.

You might have one reviewer who disagree with the changes and he/she does not approve the changes but other 3 reviewers who approve that change. For me all 4 are reviewers.

Are you saying that commit message should only include those persons who have approved the changes?

comment:31 Changed 3 years ago by Jean-Paul Calderone

Are you saying that commit message should only include those persons who have approved the changes?

Note quite - only that I don't consider myself a reviewer on this ticket. Perhaps a good guideline is that only people who remove the "review" keyword are listed as reviewers.

comment:32 Changed 3 years ago by Glyph

I've commented on the linked ticket.

Note: See TracTickets for help on using tickets.