active mode data connection: fix bind: address already in use

[drakkan]

This patch set the socket option to reuse the same port, before the
patch, after connecting in active mode and setting ActiveTransferPortNon20
to false we had something like this:

ftp> ls
200 PORT command successful
425 could not establish active connection: dial tcp :20->127.0.0.1:36151: bind: address already in use
ftp> ls
200 PORT command successful
425 could not establish active connection: dial tcp :20->127.0.0.1:55911: bind: address already in use

More details here

@fclairamb please let me know what do you think about. There is no automated test for now, binding to port 20 requires root privileges, I'm not sure CI allows it. Also, adding a test that requires root access is annoying when running tests locally

[codecov]

Codecov Report

Merging #255 (10de64f) into main (77f91ac) will decrease coverage by 0.72%.
The diff coverage is 7.14%.

@@            Coverage Diff             @@
##             main     #255      +/-   ##
==========================================
- Coverage   85.92%   85.20%   -0.73%     
==========================================
  Files          10       11       +1     
  Lines        1528     1541      +13     
==========================================
  Hits         1313     1313              
- Misses        144      157      +13     
  Partials       71       71              

Impacted Files	Coverage Δ
control_unix.go	0.00% <0.00%> (ø)
transfer_active.go	86.20% <50.00%> (-1.01%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 77f91ac...10de64f. Read the comment docs.

[fclairamb]

Interesting.

I'm not sure I ever tested it like this. Does it even make sense to always make all active connections start from the 20 port? Is it defined anywhere in the RFCs?

In my opinion, adding a test wouldn't be an issue. We can skip it if the current user isn't root or if we detect we won't be able to open the port.

[stevyn81]

Hello, this patch fixes the problem for me, thank you!

[drakkan]

@stevyn81 thanks for confirming

@fclairamb The specs suggest/mandate that the data connection port must be adiacent to the control connection port

Both the user and the server-DTPs have a default
data port. The user-process default data port is the same as the
control connection port (i.e., U). The server-process default
data port is the port adjacent to the control connection port
(i.e., L-1).

and

The user-DTP must "listen" on the specified data port; this may be
the default user port (U) or a port specified in the PORT command.
The server shall initiate the data connection from his own default
data port (L-1) using the specified user data port. The direction
of the transfer and the port used will be determined by the FTP
service command.

From my tests:

• ProFTPD seems to always use a random port. This is the same behaviour as ftpserverlib if ActiveTransferPortNon20 is enabled
• PureFTPD seems to always use port 20, even if I set the control port to 2121. This is the same behaviour as ftpserverlib if ActiveTransferPortNon20 is disabled

It seems that both ProFTPD and PureFTPD don't allow to configure a specific data port.

The latest version of ProFTPD appears not to have the L-1 issue described here, see Multiple Daemons on Same Host:

http://www.proftpd.org/docs/howto/ConfigurationTricks.html

it always uses a random port (I tested the ArchLinux package).

I will try to add a test case in the next few days. I'm not sure if we also want an option to use L-1 for the active data port.

[drakkan]

Hi,

I added a test case, it fails if you revert the socket reuse patch. As expected the test does not run in CI:

=== RUN   TestActiveTransferFromPort20
    transfer_active_test.go:29: Binding on port 20 is not supported here: listen tcp :20: bind: permission denied

you can test locally executing the test as root. Please let me know if you also want an option to listen on L-1 for active data connection. I think this should be addresses in a separate PR, thanks

[drakkan]

Hello, this patch fixes the problem for me, thank you!

@stevyn81 this patch is included in SFTPGo v2.2.0 and above, please let me know if you find any issues using the latest SFTPGo version, thanks