opnsense
net/upnp: Service improvements
- Improve wording, move
Default denyto a better place and fix some help buttons - Rename status page to
Active Port Maps, improve layout and wording, and keep ports numeric by using pfctl with-P - Simplify the UI by creating
Advanced Settingsand move and rearrange multiple options - Reword
User specified permissionstoService Access Control Listalso used in other projects - Add
Allow third-party mappingUI option applied to UPnP IGD & PCP - Rename service (title) to
UPnP IGD & PCPto include PCP the UPnP IGD alternative, withoutNAT-PMP, otherwise it breaks into two lines in the collapsed menu as it currently does - Disable
Unused rules cleaning(upstream default), better described asCleaning port maps without traffic, as it does not follow standards and also deletes valid unexpired port maps, and does not work when tested. Remove configuration generation for upstream defaults
Screenshots:
Close #4608
This is the first part of the plugin revision. The second part is the migration to the MVC/API.
I'm not opposed but I don't have time in the next 7 days to finish this review.
@Self-Hosting-Group almost there now. Last bits are case changes in the translated strings and changing the clear playing the devils advocate. If you make sure it works we can go with the action rename. Sometimes people change it but don't test so we mention it.
For OpenWrt, I often manually adjusted the message IDs of the translations to avoid losing them when making minor changes. I thought it might be a good idea to do the same here.
For the mentioned strings, I was thinking of a slightly different configuration for the next MVC/API migration. As we can probably reuse the strings, I propose it now. Perhaps the UI options would be better if they were:
- Inverted, but keeping the same configuration options
- Use the word
Disableinstead ofDeny - Maybe a dropdown would make more sense since neither option can be disabled at the same time
What do you think?
In general non-inverted options are easier to understand, explain and operate. When you check a box it should enable something. In terms of MVC capabilities we can also introduce "on by default" options that provide opt-out of formerly default settings. There isn't a need to handle invert options, unless they already exist in which case they can also be flipped and migrated to non-inverted options. It's a case-by-case. Sometimes it also makes sense to follow the upstream manual on the wording options instead of making it simpler for users, because then it's only an illusion of it being simpler when you start looking at the configuration file. More mistakes happen in the code because of this.
Cheers, Franco
PS: if you want different build options for miniupnpd package just raise a PR in tools
Merged, thanks!
Wow! Nice, but...
PS: if you want different build options for miniupnpd package just raise a PR in tools
This must be done. Otherwise, the not supported daemon option will break. This was not intended to be taken on alone.
https://github.com/opnsense/plugins/blob/f69ae0ecd96ce164a58bfc927992b99bdbd2c508/net/upnp/src/etc/inc/plugins.inc.d/miniupnpd.inc#L252
Ok, in that case will you make a PR? :)
OK. And what about the prerequisite (actually a recommendation)? https://redirect.github.com/freebsd/freebsd-ports/pull/402
Ok, in that case will you make a PR? :)
OK. And what about the prerequisite (actually a recommendation)? https://redirect.github.com/freebsd/freebsd-ports/pull/402
Can't merge this but on opnsense/ports I could. :)
- Minor HTML validation fix in the commenting out of the uptime option in
services_upnp.php(for the string spitting):
@@ -361,7 +361,7 @@ include("head.inc");
<?=gettext("Report system instead of service uptime.");?>
</div>
</td>
- --> </tr>
+ </tr> -->
- And when we go later in OPNsense, with the future MVC/API plugin recreation with something more extended with the access control, with network presets, like what I propose to OpenWrt (screenshot, screenshot2) with the plugin, you could directly rename the heading
Access Control ListtoCustom Access Control List. PS: The UI/wording is now very similar to that of the current pfSense, and contains strings from OpenWrt. pfSense docs: https://docs.netgate.com/pfsense/en/latest/services/upnp.html#service-settings
@Self-Hosting-Group b6db17f7b0 then
I'm a little nagged by the imbalance of menu entries between "Settings" and "Active Port Maps". "Status" is pretty universal and well known for historic reasons, but a shorter entry like "Port Maps" or "Active Maps" looks a bit nicer. But I'm not saying it needs to change.
I'm a little nagged by the imbalance of menu entries between "Settings" and "Active Port Maps". "Status" is pretty universal and well known for historic reasons, but a shorter entry like "Port Maps" or "Active Maps" looks a bit nicer. But I'm not saying it needs to change.
Good point! None of the other UIs had this because there weren't multiple menu entries. I'll think about it. Maybe for the menu and ACL:
- Service Settings
- Active Port Maps
Or the shorter:
- Settings
- Port Maps
Both work but "service" is also redundant in the service menu so second one sounds good.
Then maybe:
diff --git a/net/upnp/src/opnsense/mvc/app/models/OPNsense/UPnP/ACL/ACL.xml b/net/upnp/src/opnsense/mvc/app/models/OPNsense/UPnP/ACL/ACL.xml
index 6d4d75598..98cadb648 100644
--- a/net/upnp/src/opnsense/mvc/app/models/OPNsense/UPnP/ACL/ACL.xml
+++ b/net/upnp/src/opnsense/mvc/app/models/OPNsense/UPnP/ACL/ACL.xml
@@ -6,7 +6,7 @@
</patterns>
</page-service-upnp>
<page-status-upnpstatus>
- <name>Services: UPnP IGD & PCP: Active Port Maps</name>
+ <name>Services: UPnP IGD & PCP: Port Maps</name>
<patterns>
<pattern>status_upnp.php*</pattern>
</patterns>
diff --git a/net/upnp/src/opnsense/mvc/app/models/OPNsense/UPnP/Menu/Menu.xml b/net/upnp/src/opnsense/mvc/app/models/OPNsense/UPnP/Menu/Menu.xml
index 27ffe1618..ba129ede3 100644
--- a/net/upnp/src/opnsense/mvc/app/models/OPNsense/UPnP/Menu/Menu.xml
+++ b/net/upnp/src/opnsense/mvc/app/models/OPNsense/UPnP/Menu/Menu.xml
@@ -4,7 +4,7 @@
<Settings order="10" url="/services_upnp.php">
<Edit url="/services_upnp.php?*" visibility="hidden"/>
</Settings>
- <ActivePortMaps VisibleName="Active Port Maps" order="20" url="/status_upnp.php"/>
+ <PortMaps VisibleName="Port Maps" order="20" url="/status_upnp.php"/>
</UPnP>
</Services>
</menu>
How about cherry-picking the first net/miniupnpd: Add build option commit to fix the current broken main tree?
Nothing’s broken yet. The target to sync tools, ports and plugins is 25.7.7 some time next week. I can add the one commit without issue though.
Ideally, the port gets updated first, then tools, then plugin for minimal friction 😊
Done via https://github.com/opnsense/plugins/commit/f658f82aa and https://github.com/opnsense/tools/commit/3b17ebf8e
Cheers, Franco
Found something cosmetic inconsistent:
Maybe the first (ACL) name tag could be changed to Services: UPnP IGD & PCP: Settings to be consistent with the menu.
https://github.com/opnsense/plugins/blob/f658f82aa46df30f64074c468b717aebb9ce712b/net/upnp/src/opnsense/mvc/app/models/OPNsense/UPnP/ACL/ACL.xml#L2-L9
Thank you for merging.
- In the plugin revision, I only added one new UI option, as the old plugin was not supposed to be expanded any further, but in the meantime we have reworded a lot more than I thought. Perhaps two additional options would be helpful to round things off (ipv6_disable/friendly_name), friendly_name depending on whether the new build defaults have already been implemented
- About the friendly_name option:
- Should this be customisable by the user?
- Should the default be changed from
FreeBSD routertoOPNsense UPnP IGD & PCP?
- Already mentioned an UPnP IGD daemon compatibility patch added to the FreeBSD ports PR which would be good to have in OPNsense miniupnp/miniupnp@c82bb2f freebsd/freebsd-ports#402
Here are the changes prepared, on hold for discussion, including minor optional text changes and adding a heading for UPnP IGD Adjustments options
Show
diff --git a/net/upnp/src/etc/inc/plugins.inc.d/miniupnpd.inc b/net/upnp/src/etc/inc/plugins.inc.d/miniupnpd.inc
index d27bcd751..3cb0d2526 100644
--- a/net/upnp/src/etc/inc/plugins.inc.d/miniupnpd.inc
+++ b/net/upnp/src/etc/inc/plugins.inc.d/miniupnpd.inc
@@ -206,6 +206,10 @@ function miniupnpd_configure_do($verbose = false)
$config_text .= "pcp_allow_thirdparty=no\n";
}
+ if (!empty($upnp_config['ipv6_disable'])) {
+ $config_text .= "ipv6_disable=yes\n";
+ }
+
/* enable logging of packets handled by miniupnpd rules */
if (!empty($upnp_config['logpackets'])) {
$config_text .= "packet_log=yes\n";
@@ -225,6 +229,11 @@ function miniupnpd_configure_do($verbose = false)
$config_text .= "/\n";
}
+ if (!empty($upnp_config['friendly_name'])) {
+ // Encode required XML entities of text UPnP IGD config options until the daemon does so
+ $config_text .= "friendly_name=" . htmlspecialchars($upnp_config['friendly_name'], ENT_NOQUOTES | ENT_XML1) . "\n";
+ }
+
/* set uuid and serial */
$config_text .= "uuid=" . miniupnpd_uuid() . "\n";
$config_text .= "serial=" . strtoupper(substr(miniupnpd_uuid(), 0, 8)) . "\n";
@@ -247,8 +256,8 @@ function miniupnpd_configure_do($verbose = false)
$config_text .= "enable_upnp=" . ( $upnp_config['enable_upnp'] ? "yes\n" : "no\n" );
$config_text .= "enable_pcp_pmp=" . ( $upnp_config['enable_natpmp'] ? "yes\n" : "no\n" );
- # When building with IGDv2, infinite (IGDv1 only) lease time port maps are reduced to 7d
- # following the IGDv2 standard. Disabling it at runtime allows IGDv2 incompatible clients
+ // When building the daemon with UPnP IGDv2, infinite (IGDv1 only) lease duration port maps are reduced
+ // to 7d, following the IGDv2 standard. Disabling it at runtime allows IGDv2-incompatible clients
$config_text .= "force_igd_desc_v1=yes\n";
/* write out the configuration */
diff --git a/net/upnp/src/www/services_upnp.php b/net/upnp/src/www/services_upnp.php
index 6a88aa9f7..8d77afdaa 100644
--- a/net/upnp/src/www/services_upnp.php
+++ b/net/upnp/src/www/services_upnp.php
@@ -75,7 +75,9 @@ if ($_SERVER['REQUEST_METHOD'] === 'GET') {
'enable_natpmp',
'enable_upnp',
'ext_iface',
+ 'friendly_name',
'iface_array',
+ 'ipv6_disable',
'logpackets',
'overridesubnet',
'overridewanip',
@@ -175,7 +177,7 @@ if ($_SERVER['REQUEST_METHOD'] === 'GET') {
// save form data
$upnp = [];
// boolean types
- foreach (['enable', 'enable_upnp', 'enable_natpmp', 'logpackets', 'sysuptime', 'permdefault', 'allow_third_party_mapping'] as $fieldname) {
+ foreach (['enable', 'enable_upnp', 'enable_natpmp', 'logpackets', 'sysuptime', 'permdefault', 'allow_third_party_mapping', 'ipv6_disable'] as $fieldname) {
$upnp[$fieldname] = !empty($pconfig[$fieldname]);
}
// numeric types
@@ -183,7 +185,7 @@ if ($_SERVER['REQUEST_METHOD'] === 'GET') {
$upnp['num_permuser'] = $pconfig['num_permuser'];
}
// text field types
- foreach (['ext_iface', 'download', 'upload', 'overridewanip', 'overridesubnet', 'stun_host', 'stun_port'] as $fieldname) {
+ foreach (['ext_iface', 'download', 'upload', 'overridewanip', 'overridesubnet', 'stun_host', 'stun_port', 'friendly_name'] as $fieldname) {
$upnp[$fieldname] = $pconfig[$fieldname];
}
foreach (miniupnpd_permuser_list() as $fieldname) {
@@ -257,7 +259,7 @@ include("head.inc");
</td>
</tr>
<tr>
- <td><a id="help_for_ext_iface" href="#" class="showhelp"><i class="fa fa-info-circle"></i></a> <?=gettext("External Interface");?></td>
+ <td><a id="help_for_ext_iface" href="#" class="showhelp"><i class="fa fa-info-circle"></i></a> <?=gettext("External interface");?></td>
<td>
<select class="selectpicker" name="ext_iface">
<?php
@@ -353,6 +355,12 @@ include("head.inc");
</div>
</td>
</tr>
+ <tr>
+ <td><i class="fa fa-info-circle text-muted"></i> <?= gettext('Disable IPv6 mapping') ?></td>
+ <td>
+ <input name="ipv6_disable" type="checkbox" value="yes" <?= !empty($pconfig['ipv6_disable']) ? "checked=\"checked\"" : ""; ?> />
+ </td>
+ </tr>
<!-- <tr>
<td><a id="help_for_sysuptime" href="#" class="showhelp"><i class="fa fa-info-circle"></i></a> <?=gettext("Report system uptime");?></td>
<td>
@@ -371,6 +379,22 @@ include("head.inc");
</div>
</td>
</tr>
+ </tbody>
+ </table>
+ </div>
+ </div>
+ </section>
+ <section class="col-xs-12">
+ <div class="content-box">
+ <div class="table-responsive">
+ <table class="table table-striped opnsense_standard_table_form">
+ <thead>
+ <tr>
+ <th style="width:22%"><?=gettext("UPnP IGD Adjustments")?></th>
+ <th style="width:78%"></th>
+ </tr>
+ </thead>
+ <tbody>
<tr>
<td><a id="help_for_download" href="#" class="showhelp"><i class="fa fa-info-circle"></i></a> <?=gettext("Download speed");?></td>
<td>
@@ -389,6 +413,12 @@ include("head.inc");
</div>
</td>
</tr>
+ <tr>
+ <td><i class="fa fa-info-circle text-muted"></i> <?= gettext('Router/friendly name') ?></td>
+ <td>
+ <input name="friendly_name" type="text" placeholder="FreeBSD router" value="<?= !empty($pconfig['friendly_name']) ? htmlspecialchars($pconfig['friendly_name']) : '' ?>" />
+ </td>
+ </tr>
</tbody>
</table>
</div>
@@ -433,8 +463,8 @@ include("head.inc");
<input name="<?= html_safe($permuser) ?>" type="text" value="<?= isset($pconfig[$permuser]) ? $pconfig[$permuser] : '' ?>" />
<?php if ($i == 1): ?>
<div class="hidden" data-for="help_for_permuser">
- <?=gettext("The ACL specifies which IP addresses and ports can be mapped. IPv6 is always accepted.");?><br/>
- <?=gettext("Format: (allow or deny) (ext port or range) (int IP or IP/netmask) (int port or range)");?><br/>
+ <?=gettext("The ACL specifies which IP addresses and ports can be mapped. IPv6 is always accepted unless disabled.");?><br/>
+ <?=gettext("Syntax: (allow or deny) (ext port or range) (int IP or IP/netmask) (int port or range)");?><br/>
<?=gettext("Example: allow 1024-65535 192.168.1.0/24 1024-65535");?>
</div>
<?php endif ?>
@Self-Hosting-Group ok for me but please create a PR for it :)
@fichtner PR created
@fichtner I just saw that you released version 25.7.7. It seems that you haven't included the plugin PR yet, which is fine, but you did include the UPNP_IGDV2 set in tools. This could cause problems for IGDv2-incompatible clients, unless it is deactivated in the configuration.
@Self-Hosting-Group we will have to see, worst case people can use
# opnsense-revert -r 25.7.6 miniupnpd
Either way this will break things sooner or later even with an on off in the config. Don't worry.
OK. However, IGDv2 is not currently enabled by default in any of the open-source router operating systems. I meant to remove the build set, or to configure this hard in the plugin for a short fix:
$config_text .= "force_igd_desc_v1=yes\n";
Let's wait for a report. I'll keep this in mind and report back if something comes up.
Less work would be better, could still be the case that nothing bad happens :)
So, let's look at the positive side: I finally have concrete reports of previously unknown IGDv2-incompatible clients ;-) I am particularly interested in whether the widely used and newer consoles (Xbox/PlayStation) also work. So, please forward me such reports. So, thanks for this situation. ;-)