Opened 14 years ago

Closed 13 years ago

#1904 enhancement closed fixed (fixed)

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, Jean-Paul Calderone Branch:
Author:

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 (1)

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

Download all attachments as: .zip

Change History (14)

Changed 14 years ago by krp

comment:1 Changed 13 years ago by therve

Owner: changed from spiv to therve

comment:2 Changed 13 years ago by therve

Cc: therve spiv added
Component: coreftp
Keywords: review added
Owner: therve deleted
Priority: normalhighest

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

spiv, willing to have a look?

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

Cc: Jean-Paul Calderone 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).

comment:4 in reply to:  3 Changed 13 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.

comment:5 Changed 13 years ago by Jean-Paul Calderone

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).

comment:6 Changed 13 years ago by therve

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

Refs #1904

comment:7 Changed 13 years ago by therve

This sounds good :).

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

Keywords: review removed
Owner: set to therve

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.

comment:9 Changed 13 years ago by therve

(In [21114])

  • Remove test using listenTCP
  • Rename pasvPortRange to passivePortRange

Refs #1904.

comment:10 Changed 13 years ago by therve

Keywords: review added
Owner: therve deleted

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

comment:11 Changed 13 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to therve

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.

comment:12 Changed 13 years ago by therve

(In [21189]) Apply review comments.

Refs #1904

comment:13 Changed 13 years ago by therve

Resolution: fixed
Status: newclosed

(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.