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 #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]

Codecov Report

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

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

❗ 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%)	⬇️
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%)	⬆️

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

☔ View full report at Codecov.
📢 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:

aioice/src/aioice/turn.py

Lines 416 to 425 in 2aed57f

	inner_transport, inner_protocol = await loop.create_datagram_endpoint(
	lambda: TurnClientUdpProtocol(
	server_addr,
	username=username,
	password=password,
	lifetime=lifetime,
	channel_refresh_time=channel_refresh_time,
	),
	remote_addr=server_addr,
	)

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:

aioice/src/aioice/turn.py

Lines 416 to 425 in 2aed57f

	inner_transport, inner_protocol = await loop.create_datagram_endpoint(
	lambda: TurnClientUdpProtocol(
	server_addr,
	username=username,
	password=password,
	lifetime=lifetime,
	channel_refresh_time=channel_refresh_time,
	),
	remote_addr=server_addr,
	)

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?

[dawgster]

Merge would be very appreciated