Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net/upnp: enable STUN and allow LAN subnet override #3096

Merged
merged 14 commits into from Dec 13, 2022

Conversation

Tawmu
Copy link
Contributor

@Tawmu Tawmu commented Aug 28, 2022

@Tawmu
Copy link
Contributor Author

Tawmu commented Oct 31, 2022

@fichtner any way we can progress this?

@fichtner fichtner self-assigned this Nov 1, 2022
@fichtner fichtner linked an issue Nov 1, 2022 that may be closed by this pull request
3 tasks
@fichtner
Copy link
Member

fichtner commented Nov 1, 2022

Thanks so far! As a side node it would be better to split STUN and override IP for clearer PR audit and faster inclusion.

net/upnp/Makefile Outdated Show resolved Hide resolved
@Tawmu
Copy link
Contributor Author

Tawmu commented Nov 22, 2022

Hopefully this is better - have taken points above and amended as necessary with clarification on the listening_ip directive.

<option value="" <?=!empty($pconfig['overridesubnet']) ? "selected=\"selected\"" : "";?>>Interface default</option>
<?php
for ($i = 32; $i >= 1; $i--) { ?>
<option value="/<?=$i;?>" <?=!empty($pconfig['overridesubnet']) && $pconfig['overridesubnet'] == "/".$i ? "selected=\"selected\"" : "";?>>/<?=$i;?></option>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's probably safer to validate for subnet mask and add the "/" in the configuration. /32 is basically the default like when we omit the override?

Copy link
Contributor Author

@Tawmu Tawmu Dec 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default ends up being the subnet of the interface rather than simply /32 afaik - not sure how to "validate" the subnet mask in this case to be honest and there may be a better way of writing this entirely.

To give context for this option - in our case, OPNsense is 10.10.0.2 behind a L3 switch on 10.10.0.1, and we have VLANs 10.40.0.0/22 and 10.50.0.0/22 routed via the switch, we simply override the miniupnpd config in OPNsense to be igb0/8 so it responds to requests coming in from the entire 10.0.0.0/8 space. It's pretty dirty but I'm not sure what we can validate subnet masks against in this case? Or do you mean just run it through the is_subnet function?

@tiurs
Copy link

tiurs commented Nov 30, 2022

Is there a way I can apply this fix now or do I have to wait till it is released?

@fichtner fichtner merged commit 393fe11 into opnsense:master Dec 13, 2022
@fichtner
Copy link
Member

going to merge to work on this... thanks!

@fichtner
Copy link
Member

@Tawmu make sure to test 7144429

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

net/upnp: allow custom listen addresses to support non-directly connected networks
3 participants