Make empty_port to look for port on all interfaces by default

[redbaron]

When looking for a port, unless specified which IP to be looking on it should use 0.0.0.0 to be on the safe side.

[kazuho]

I believe that the default value should not be changed from 127.0.0.1 to 0.0.0.0.

The primary use case of empty_port function is to find an empty port suitable for testing. And for testing, we should better not open (even temporarily) a port that is accessible via network as a default behavior.

Requiring only the users who need to bind to 0.0.0.0 to explicitly specify the host argument is IMO a better (cautious) approach.

[redbaron]

@kazuho, this thing doesn't change what users of Test-TCP are binding to, therefore it fixes problems for numerous user of Test-TCP which use '0.0.0.0' to bind to in their server code and changes nothing for those who is binding to '127.0.0.1'

[miyagawa]

I'm not following what is the purpose of this patch, a few things though:

This thing doesn't change what users of Test-TCP are binding to

Correct, but inside empty_port() it calls can_bind() which will actually try to bind the port, so it does change the behavior (and is an insecure thing to do, as @kazuho pointed out), even if the server code might not.

therefore it fixes problems for numerous user of Test-TCP which use '0.0.0.0' to bind to in their server code

Test::TCP defaults the host to 127.0.0.1 in its own code and then pass that to empty_port(), which means this patch to change the default in empty_port doesn't fix a problem assuming the passed hostname is already 127.0.0.1. If the test server code does indeed bind to 0.0.0.0 they have to call Test::TCP with the host => "0.0.0.0" by themselves.

[redbaron]

Correct, but inside empty_port() it calls can_bind() which will actually try to bind the port, so it does change the behavior (and is an insecure thing to do, as @kazuho pointed out), even if the server code might no

Binding to a socket and not accepting any incoming connections is not a security risk at all, as long as you trust your kernel. can_bind doesn't accept connections on a successfully bound socket, therefore it doesn't introduce any security risk.

Test::TCP defaults the host to 127.0.0.1 in its own code and then pass that to empty_port(), which means this patch to change the default in empty_port doesn't fix a problem assuming the passed hostname is already 127.0.0.1. If the test server code does indeed bind to 0.0.0.0 they have to call Test::TCP with the host => "0.0.0.0" by themselves.

Test::TCP doesn't default to 127.0.0.1, it is empty_port and wait_port which defaults to it internally if there is no host => passed to test_tcp. Therefore patches DOES fix the case where users of test_tcp bind to 0.0.0.0 (probably unintentionally, but nevertheless), like here: https://github.com/plack/Plack/blob/master/t/HTTP-Server-PSGI/post.t and NOT passing host => 0.0.0.0 argument to test_tcp.

It is an easy to make error because the API Test::TCP provides is that only Port and not Host is passed explicitly to both server() and client() callbacks. Even though Test::TCP has very clear understanding where both server should be binding to and client should be connecting to for everything to work smoothly - it must be the pair of Host and Port which empty_port was using to identify empty port, Test::TCP doesn't pass it down to user supplied code, which forces your users to do guess work or carefully read fine print in the docs to understand where they should be binding to.

Fixing this API design flaw requires much more leg work and wait time to ensure wider adoption, than just making slight change in can_bind so that it continues to work even if your users didn't read docs carefully and more important to eliminate intermittent test failures which are nastiest, hardest to debug and waste peoples' time.

Intermittent test failures happen on our build servers where port found by empty_port is available on 127.0.0.1 but is taken on another IP address, then when server software binds to 0.0.0.0:$port and comes back with an error. On the next run found port maybe available on all interfaces and test passes.

Out options here is either to go and inspect all test in hundreds of CPAN modules and submit patches to their authors, or submit one line change to Test::TCP which fixes the error it arguably introduced itself with less than ideal API.

Patch is safe, secure, doesn't break any existing code and makes your library to be more robust when used by somebody without full understanding of it's internals.

[redbaron]

I see what you mean saying that '127.0.0.1' is passed to empty_port. I'll provide updated patch soon

[miyagawa]

Test::TCP doesn't default to 127.0.0.1, it is empty_port and wait_port which defaults to it internally if there is no host => passed to test_tcp.

(I see that you see it by now) It definitely defaults to 127.0.0.1 in new() and that value is passed around to both empty_port and wait_port.

Therefore patches DOES fix the case where users of test_tcp bind to 0.0.0.0 (probably unintentionally, but nevertheless), like here: https://github.com/plack/Plack/blob/master/t/HTTP-Server-PSGI/post.t and NOT passing host => 0.0.0.0 argument to test_tcp.

Right, so I fixed the Plack code instead.

It is an easy to make error because the API Test::TCP provides is that only Port and not Host is passed explicitly to both server() and client() callbacks.

That's a good point and I see no problem in enhancing Test::TCP to pass the host to both callbacks.

[nponeccop]

Note that the failed checks are due to TravisCI glitch. @tokuhirom can you go to https://travis-ci.org/tokuhirom/Test-TCP/builds/88768434 and click Restart Job (I don't have privileges)?

[nponeccop]

Ok, I have privileges now. Test passed.