FtpServer
  1. FtpServer
  2. FTPSERVER-420

When picking a passive port, use "random port" from the pool instead of "lowest port"

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.0.6, 1.1.0
    • Component/s: Core
    • Labels:
      None

      Description

      As discussed on the mailing list < http://www.mail-archive.com/ftpserver-users@mina.apache.org/msg01635.html >, passive ports are allocated from the pool based on the lowest available port from the list. This may cause problems with some firewalls or clients that may not release the port as quickly as the server expects. It is also a minor security risk to provide an easily guessable port for passive connections.

      Discussion on the list centered around other options to allocate ports, focusing on a random port assignment from the available pool.

      1. PassivePorts.java.diff
        5 kB
        Allen Firstenberg
      2. DataConnectionConfigurationFactory.java.diff
        0.6 kB
        Allen Firstenberg
      3. PassivePorts.java
        8 kB
        Allen Firstenberg
      4. PassivePortsTest.java
        10 kB
        Allen Firstenberg

        Activity

        Hide
        Allen Firstenberg added a comment -

        Attached two files that change PassivePorts.java to use a random port, roughly using the algorithm proposed in the email discussion. Minor change to DataConnectionConfigurationFactory to create its default instance of PassivePorts.

        Show
        Allen Firstenberg added a comment - Attached two files that change PassivePorts.java to use a random port, roughly using the algorithm proposed in the email discussion. Minor change to DataConnectionConfigurationFactory to create its default instance of PassivePorts.
        Hide
        Allen Firstenberg added a comment -

        And in case it is easier for someone to review the code without the diff, here is the complete modified PassivePorts.java

        Show
        Allen Firstenberg added a comment - And in case it is easier for someone to review the code without the diff, here is the complete modified PassivePorts.java
        Hide
        Niklas Gustavsson added a comment -

        From an initial review, it looks good. However, I'm missing the check if the port is currently in use. This is required as ports can be used by other processes.

        Show
        Niklas Gustavsson added a comment - From an initial review, it looks good. However, I'm missing the check if the port is currently in use. This is required as ports can be used by other processes.
        Hide
        Allen Firstenberg added a comment -

        Line 214:
        } else if( checkPortUnbound(ret.intValue()) ){

        Show
        Allen Firstenberg added a comment - Line 214: } else if( checkPortUnbound(ret.intValue()) ){
        Hide
        Niklas Gustavsson added a comment -

        Sorry, completely missed it. Could you also update the test case as that's now broken since the random assignments?

        Show
        Niklas Gustavsson added a comment - Sorry, completely missed it. Could you also update the test case as that's now broken since the random assignments?
        Hide
        Allen Firstenberg added a comment -

        Whoops, knew I forgot something. (And then attached the wrong file.) This should be the correct one, however.

        Show
        Allen Firstenberg added a comment - Whoops, knew I forgot something. (And then attached the wrong file.) This should be the correct one, however.
        Hide
        Niklas Gustavsson added a comment -

        I've reviewed the patch and think it's very good. Still making some very minor changes before I will commit it.

        Show
        Niklas Gustavsson added a comment - I've reviewed the patch and think it's very good. Still making some very minor changes before I will commit it.
        Hide
        Niklas Gustavsson added a comment -

        Fixed in rev 1137251 and 1137252. Thanks for your work on this Allen!

        Show
        Niklas Gustavsson added a comment - Fixed in rev 1137251 and 1137252. Thanks for your work on this Allen!

          People

          • Assignee:
            Niklas Gustavsson
            Reporter:
            Allen Firstenberg
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development