Optionally restrict the range of ephemeral ports

[sirf]

Optionally restrict the range of ephemeral ports that the Connection class can bind to. This is a proposed fix for https://github.com/aiortc/aioice/issues/47

[jlaine]

Hi! Thanks for putting this together. It feels as though this is only fixing part of the problem, there are many calls to create_datagram_endpoint throughout the code which all need to use a wrapper?

Also we're going to need an explicit test for this, as well as a test for resource exhaustion (no more ports available in the specific range).

[sirf]

asyncio.create_datagram_endpoint is used in ice.py, mdns.py, and turn.py, but ice.py is the only one that binds to an ephemeral port. mdns.py binds to the mDNS port 5353, and turn.py has a non-optional argument to set the port, so I believe ice.py is the only place where a change is needed unless I'm missing something.

I'll try to put some tests together.

[codecov[bot]]

Codecov Report

Base: 99.75% // Head: 99.91% // Increases project coverage by +0.16% :tada:

Coverage data is based on head (36ca8e8) compared to base (3cd4bcc). Patch coverage: 93.33% of modified lines in pull request are covered.

:exclamation: Current head 36ca8e8 differs from pull request most recent head 13a23e4. Consider uploading reports for the commit 13a23e4 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #63      +/-   ##
==========================================
+ Coverage   99.75%   99.91%   +0.16%     
==========================================
  Files           8        8              
  Lines        1205     1196       -9     
==========================================
- Hits         1202     1195       -7     
+ Misses          3        1       -2     

Impacted Files	Coverage Δ
src/aioice/ice.py	99.82% <93.33%> (-0.18%)	:arrow_down:
src/aioice/stun.py	100.00% <0.00%> (ø)
src/aioice/turn.py	100.00% <0.00%> (ø)
src/aioice/mdns.py	100.00% <0.00%> (+2.58%)	:arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[jlaine]

mdns.py binds to the mDNS port 5353,

You're right, we can forget about mDNS.

turn.py has a non-optional argument to set the port

I'm not sure what you mean here, we are not setting the local port:

https://github.com/aiortc/aioice/blob/2aed57f1fa0ffeedc3a5f257a8fbbace07ca2c1f/src/aioice/turn.py#L416-L425

Concerning the actual API, instead of port_lower and port_upper maybe have you considered ephemeral_ports: Optional[Iterable[int]] = None ? The docstring could also be more explicit about the fact these are local ports.

[sirf]

turn.py has a non-optional argument to set the port

I'm not sure what you mean here, we are not setting the local port:

https://github.com/aiortc/aioice/blob/2aed57f1fa0ffeedc3a5f257a8fbbace07ca2c1f/src/aioice/turn.py#L416-L425

turn.py creates a socket that is associated with a known remote address/port, and for this reason I'm not sure if it's important to be able to restrict which local port to use here, but I'll admit that I'm not 100% sure. I moved the critical code to utils.py however, so if it is indeed desirable to be able to restrict the local port for STUN as well it should take me about 5 minutes to do that.

Do you have any input on this, gregvish ?

Concerning the actual API, instead of port_lower and port_upper maybe have you considered ephemeral_ports: Optional[Iterable[int]] = None ? The docstring could also be more explicit about the fact these are local ports.

Good idea, that would allow a set of ports that is not restricted to a continuous sequence. I've updated the code, and also added unit tests.

[marshallmassengill]

I have a use for this with broadcasting data over a restricted port range. Any chance this could get merged?