Opened 7 years ago

Last modified 6 years ago

#7057 enhancement new

portforward.py: make the ProxyServer client connection behaviour overrideable in subclasses

Reported by: infinity0 Owned by: infinity0
Priority: normal Milestone:
Component: core Keywords:
Cc: infinity0 Branch:
Author:

Description

I want to write a simple bi-directional "dumb" proxy in Twisted. However, the server endpoint is a SOCKS proxy server rather than a TCP server.

If this patch were applied, it would make my life a lot easier because I could then just subclass ProxyServer instead of having to duplicate it.

The change is so trivial I am not sure what additional tests to write. Existing tests all pass.

Attachments (5)

portforward-overrideable.patch (740 bytes) - added by infinity0 7 years ago.
portforward-overrideable.2.patch (3.2 KB) - added by infinity0 7 years ago.
portforward-overrideable.3.patch (3.4 KB) - added by infinity0 7 years ago.
portforward-overrideable.4.patch (3.4 KB) - added by infinity0 7 years ago.
portforward-overrideable.5.patch (4.0 KB) - added by infinity0 7 years ago.

Download all attachments as: .zip

Change History (20)

Changed 7 years ago by infinity0

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

Cc: infinity0 added
Keywords: review removed

Thanks.

This patch adds a feature. The feature is "subclasses can override the connectProxyClient method to control how the outgoing connection is made". The tests should verify that this is actually so.

The new method should also have API documentation and ideally there should be an example demonstrating how this new feature can be used.

Please also include a news fragment describing the new feature.

Also perhaps worth noting is that you can already control this behavior without even subclassing ProxyServer. Just provide a reactor with a connectTCP implementation that does the SOCKS stuff that you want it to do.

(Can't re-assign the ticket to infinity0, the name doesn't appear in the "Assign to" dropdown.)

Thanks again.

Changed 7 years ago by infinity0

Changed 7 years ago by infinity0

comment:2 Changed 7 years ago by infinity0

Keywords: review added
Owner: set to Jean-Paul Calderone

I've added some API docs, a test, and a NEWS fragment.

I could add an example too, but the one I have is 20 lines long and uses an external library (txsocksx). Do you still want it? If so, how shall I format it within the docstring?

Changed 7 years ago by infinity0

comment:3 Changed 7 years ago by Tom Prince

It seems like a better way to implement this would be to pass a function to the constructor that controls how to connect, rather than forcing the user to subclass.

comment:4 Changed 7 years ago by infinity0

Why and how is that better? Even if so, the typical pattern in Twisted is to subclass, e.g. Protocol. What would justify a different approach?

comment:5 Changed 7 years ago by Tom Prince

comment:6 Changed 7 years ago by infinity0

Sure, in general I would agree with that. But here, what concrete steps would you like me to take, if you want to go down the functional road?

If there is an existing example of this pattern already in Twisted, I will be happy to match it. But it's a bit unfair to ask a new contributor to make up such a scheme by myself, or to use this as a guinea pig for experimental ideas not yet standard in Twisted.

comment:7 Changed 7 years ago by Itamar Turner-Trauring

Keywords: review removed
Owner: Jean-Paul Calderone deleted

There already exists a new, documented and relevant API: endpoints (https://twistedmatrix.com/documents/current/core/howto/endpoints.html). For example there's a 3rd party SOCKS library that provides endpoints for that.

The obvious API is therefore to have an optional argument to the constructor, a callable that takes the reactor, host and port as arguments and returns an endpoint instance. By default it would be TCP4ClientEndpoint, or better yet HostnameEndpoint (in which case you've added IPv6 support to the proxy as a side-effect).

comment:8 Changed 7 years ago by Itamar Turner-Trauring

Changed 7 years ago by infinity0

comment:9 Changed 7 years ago by infinity0

Keywords: review added

All done, I think.

I skipped an explicit test, since the new code is using HostnameEndpoint by default so it's being tested already, and also I had to change another test to pass in a different parameter (TCP4ClientEndpoint). Adding a further test for this parameter specifically seems overkill.

comment:10 Changed 7 years ago by Adi Roiban

Keywords: review removed
Owner: set to infinity0

The patch needs a news file as described here https://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles and not manual edit for news file.


Just a simple comment from a new guy to Twisted:

Not sure what to say about the callable. It takes a (reactor, host, port) and returns a twisted.internet.interfaces.IStreamClientEndpoint.

While TCP endpoint are initialized with reactor,host,port there are also TLS/SSL endpoints or UnixEndpoints which have extra arguments.

host and port are passed in init together with callable.... so maybe you can just pass the endpoint instance.

To avoid breaking the interface, I think that you can add a new ProxyFactoryWithEndpoint class which inherit ProxyFactory ...and ProxyFactory would have a class member as endpointFactory = endpoints.HostnameEndpoint.

Thanks!

comment:11 Changed 7 years ago by infinity0

One cannot simply pass in an endpoint instance, because the design of the Proxy means the reactor is only passed in per-client.

I could have a reactor->Endpoint function, but this makes the interface awkward for the usual Endpoin classess, and also makes it awkward for people that just want a straight-up HostnameEndpoint. If your Endpoint class takes extra args, perhaps functools.partial will help you.

This change does not break any interfaces. If you don't need the endpoint argument, you can just ignore it. What would you achieve with ProxyFactoryWithEndpoint that cannot already be achieved?

comment:12 Changed 7 years ago by Tom Prince

I don't think there is anything that can't be achieved by having a function that returns an endpoint. It simply isn't the interface that I would implement, if starting from scratch. Having a function that takes the other arguments that are passed in alongside it feels awkward.

Also, if I was writing a proxy, and had to write a function that took a host and port and returned an endpoint, my first thought would be that the host and port would be of the client that is connecting.

comment:13 Changed 7 years ago by Itamar Turner-Trauring

For backwards compat we need to preserve current structure; not much we can do about that. Once tubes is implemented we'd probably want a newer better portforward implementation anyway. Even if not, we can always add a new API that just takes an endpoint instance (in a different ticket).

There is not reason to stop an improvement to the current API, even if it's not ideal. So within the limits of "let's make things slightly better without preventing better implementations" we should just accept this patch as is (after review of course) and implement more generic/superior features separately.

comment:14 Changed 7 years ago by Itamar Turner-Trauring

Keywords: review added

comment:15 Changed 6 years ago by Glyph

Keywords: review removed

Hi infinity0,

Thanks again for your patience and sorry for the long delays on reviews. We nevertheless appreciate your contribution.

Here are some points that need to be addressed with portforward-overrideable.5.patch:

  1. The "feature" entry you've added there is to the wrong version - the (already released) 13.2.0; you're editing a generated file, rather than providing an input to that file. Please see the documentation for NEWS files in our review process explaining how this should be done.
  2. The tests no longer cover the default constructor. You can replace HostnameEndpoint's hostname resolution with something synchronous that returns a succeed in order to make this test behave as expected.
  3. The new feature of pluggable endpoint types isn't really tested. The existing functionality is made to work within the tests, but this is new functionality which should have new test coverage ensuring that it behaves as expected.
  4. We are generally cautious about introducing new public API, and I don't think there is any particular reason that endpointFactory should be a mutable, public attribute in addition to a constructor argument. Consider prefixing that with a _ - or, if you feel it should be public, just go ahead and explain why, I'm not saying it necessarily can't be, just that we should consider the cost of that extra API surface first.
  5. Minor stylistic nitpick: docstrings in Twisted should not have any text on the same line as the opening triple quote.

Thanks again for your contribution. I hope that you'll pick this up again despite the long review cycle - we'll try to make the next one shorter!

-glyph

Note: See TracTickets for help on using tickets.