Ticket #1904 (closed enhancement: fixed)

Opened 4 years ago

Last modified 3 years ago

Passive port ranges are not settable in t.p.ftp

Reported by: krp Owned by: therve
Priority: highest Milestone:
Component: ftp Keywords:
Cc: therve, spiv, exarkun Branch:
Author: Launchpad Bug:

Description

As discussed on #twisted, FTP passive ports are chosed randomly by the OS (since we use reactor.listenTCP(0, f) for the dtp factory).

I propose this patch that offers the user the possibility to tune the passive port ranges.

To be reviewed, it may not fit twisted's style.

Attachments

twisted-ftp-port-ranges.patch Download (2.0 KB) - added by krp 4 years ago.

Change History

Changed 4 years ago by krp

  Changed 3 years ago by therve

  • owner changed from spiv to therve

  Changed 3 years ago by therve

  • priority changed from normal to highest
  • owner therve deleted
  • component changed from core to ftp
  • keywords review added
  • cc therve, spiv added

This is ready to review in ftp-pasv-port-range-1904.

spiv, willing to have a look?

follow-up: ↓ 4   Changed 3 years ago by exarkun

  • cc exarkun added

So, here is a crazy thought

Port ranges should be determined by some method on the avatar (perhaps not a method which is part of the primary interface, though, since that seems to be oriented towards filesystem interaction, although maybe it'd be okay to add it there anyway).

in reply to: ↑ 3   Changed 3 years ago by therve

Replying to exarkun:

So, here is a crazy thought Port ranges should be determined by some method on the avatar (perhaps not a method which is part of the primary interface, though, since that seems to be oriented towards filesystem interaction, although maybe it'd be okay to add it there anyway).

It seems it's totally possible, but what does it bring exactly? We'll move the information of the ports from FTPFactory/FTP to FTPRealm/FTPShell. It looks relatively equivalent to me. If your point is for customisation, I've never seen a case where you want port settings customizable by client, but that could be useful for someone.

  Changed 3 years ago by exarkun

Hmm, you're right, it's a pretty rare use-case. Okay, let's forget about that for now.

What I would like to see, then, is some simpler semantics for the range. How about this, instead of having a min and a max (with 0 for min having a special meaning), there is a single attribute which gives a port range. It can just be an iterator (eg, xrange(10, 20)). getDtpPort can just iterate over it. This should be less code and offer more flexibility.

How does this sound?

Incidentally, getDtpPort should capitalize DTP, since it's an acronym (Data Transfer Protocol).

  Changed 3 years ago by therve

(In [20940]) Use a range for passive port instead of 2 variables.

Refs #1904

  Changed 3 years ago by therve

This sounds good :).

  Changed 3 years ago by exarkun

  • owner set to therve
  • keywords review removed

Looking good, thanks!

I'd suggest some minor adjustments to the tests - I don't think any of them need to bind actual tcp ports. Two of them are careful not to, but test_PASVportRange seems to want to, and I'm not sure why. It could easily provide a listenFactory that asserts the beginning of the range is passed in first, and so on. Also, it has a weird name :) It might be okay to expand "pasv" to "passive" (maybe in the attribute name on FTP, too).

If test_PASVportRange stops using the real reactor.listenTCP, then it probably also won't have to take care to return the deferred returned by port.stopListening to avoid a cleanup error.

  Changed 3 years ago by therve

(In [21114])

  • Remove test using listenTCP
  • Rename pasvPortRange to passivePortRange

Refs #1904.

  Changed 3 years ago by therve

  • owner therve deleted
  • keywords review added

This test actually acts as a duplicate of test_portRange, so I removed it, and I rename the variable.

  Changed 3 years ago by exarkun

  • owner set to therve
  • keywords review removed

Using list() on passivePortRange when raising CannotListenError from getDTPPort worries me a little. I might not want to see a list of 5 thousand port numbers if that exception gets logged. Maybe just having passivePortRange there would be okay? xrange objects str kind of nicely on their own, at least.

test_portRangeForwardError still sets pasvMinPort which probably doesn't do anything anymore.

Some suggestions for docstrings:

  • test_portRangeForwardError - Exceptions other than L{error.CannotListenError} which are raised by C{listenFactory} should be raised to the caller of L{FTP.getDTPPort}.
  • test_portRange - L{FTP.passivePortRange} should determine the ports which L{FTP.getDTPPort} attempts to bind. If no port from that iterator can be bound, L{error.CannotListenError} should be raised, otherwise the first successful result from L{FTP.listenFactory} should be returned.
  • FTP.listenFactory - @ivar listenFactory: A callable with the signature of L{twisted.internet.interfaces.IReactorTCP.listenTCP} which will be used to create Ports for passive connections (mainly for testing).

These are all quite minor points. I think you should be able to merge without another review.

  Changed 3 years ago by therve

(In [21189]) Apply review comments.

Refs #1904

  Changed 3 years ago by therve

  • status changed from new to closed
  • resolution set to fixed

(In [21190]) Merge ftp-pasv-port-range-1904

Author: therve Reviewer: exarkun Fixes #1904

Throught the passivePortRange attribute of twisted.protocols.ftp.FTPFactory, allow a FTP server to restrict the passive ports used instead of using a random port.

Note: See TracTickets for help on using tickets.