Could you point out some documentation/examples on which scripts are missing?
It seems it has never been tested since @jack9603301 implemented it in task T3420. It seems he also didn't test it.

The config script is https://github.com/vyos/vyos-1x/blob/current/src/conf_mode/service_upnp.py
The template config https://github.com/vyos/vyos-1x/blob/current/data/templates/firewall/upnpd.conf.j2

The mentioned file that missing is located upstream in https://github.com/miniupnp/miniupnp/tree/miniupnpd_2_3_1/miniupnpd/netfilter_nft/scripts
and the upstream configuration options that we think are missing to match vyos chains is https://github.com/miniupnp/miniupnp/blob/miniupnpd_2_3_1/miniupnpd/miniupnpd.conf#L77

(our experiment with changing the filter chain to vyos_filter did not succeed, so there might be additional things missing / wrong, but we did not continue experimenting due to lack of understanding of the desired integration approach of miniupnpd into vyos)

Also while at it, the smoketests regarding UPnP should probably be updated by this task aswell since they claim everything is OK:

30609 DEBUG - Running Testcase: /usr/libexec/vyos/tests/smoke/cli/test_service_upnp.py
30610 DEBUG - test_ipv4_base (__main__.TestServiceUPnP.test_ipv4_base) ... ok
30611 DEBUG - test_ipv6_base (__main__.TestServiceUPnP.test_ipv6_base) ... ok
30612 DEBUG -
30613 DEBUG - ----------------------------------------------------------------------
30614 DEBUG - Ran 2 tests in 7.593s
30615 DEBUG -
30616 DEBUG - OK

Can confirm this is exactly the same in 1.4 rolling (as of Jan 09). Same errors. The miniupnpd daemon receives the request (for either a UPnP, NAT-PMP, or PCP port mapping) and then reports the errors @simplysoft reports in the description.

Effectively, the daemon is functioning but it is not able to create the rules in nftables.

UPnP is therefore effectively broken in VyOS as things stand.

Will it work if you manually download the functions? https://github.com/miniupnp/miniupnp/blob/miniupnpd_2_3_1/miniupnpd/netfilter_nft/scripts/miniupnpd_functions.sh

Another bug it that /config/upnp.leases is hardcoded, but there is no script who creates it https://github.com/vyos/vyos-1x/blob/aebb458262072457c6a3840d1b17031fbd780eca/data/templates/firewall/upnpd.conf.j2#L128

It cause of

Jan 10 21:16:03 r4 miniupnpd[9869]: Reloading rules from lease file
Jan 10 21:16:03 r4 miniupnpd[9869]: could not open lease file: /config/upnp.leases

@Viacheslav

No, installing the miniupnpd_functions.sh file does not correct the problem.

The issue seems to be that the init script (nft_init.sh) doesn't look like it's getting executed as part of the initial service configuration / commit, so none of the chains are ever created in nftables.

Likewise, the chains that miniupnpd uses (created by the nft_init.sh script) need to be included via a jump call from the primary forward / prerouting / postrouting chains, per:

https://github.com/miniupnp/miniupnp/tree/miniupnpd_2_3_1/miniupnpd/netfilter_nft

These jumps would also need to be installed as part of the service setup.

So the key logging alert, the mnl_cb_run() in the netfilter API returning -1, makes sense. The miniupnpd daemon is incorrectly assuming the chains it needs are created, and they are not.

Additionally, miniupnpd is configured to use /config/upnp.leases, which it can't seem to create on daemon startup. I haven't looked into why. If I manually create it pre-daemon launch, the error complaining about the lease file being absent goes away/. At that point, miniupnpd does successfully destroy it on termination.

Likely a permissions issue here.

Sorry if there are details missing. I have to juggle between vyOS and my primary router instance during the work day so getting these details was a quick in/out while my wife was having her lunch break. :)

I've made some progress. I've been able to get miniupnpd to stop generating the mnl_cb_run() errors. It was as assumed; the lack of nft table / chains.

First, I copy:
https://github.com/miniupnp/miniupnp/tree/miniupnpd_2_3_1/miniupnpd/netfilter_nft

To:
/etc/miniupnpd/miniupnpd_functions.sh

Then manually run:

core-router:/etc/miniupnpd$ sudo ./nft_init.sh

This creates a new nft table: table inet filter (note that it's using inet to handle IPv4/IPv6 in a single table). The script puts both the forward chain and the nat chains in this single table. It then creates the standard filter, prerouting, and postrouting chains and adds a hook and a jump call to the miniupnpd, prerouting_miniupnpd, or postrouting_miniupnpd chains as appropriate.

So far, so good.

At that point, I can create port forwards using miniupnpc on my client machine and the daemon creates them as expected. I see the rules appearing in the table inet filter chains and the associated rules are added to /config/upnp.leases (which the daemon creates once the first pinhole is created by an external client).

However, forwarding still fails to work. I suspect that the standard vyOS filter / nat chains are grabbing priority and my default-action drop rules are being triggered before the miniupnpd chains are being run, but that's as far as I've gotten debugging this so far.

If anyone else has any ideas I'm all ears. Otherwise I'll try to play with it a bit more this weekend when I get some time. I'm assuming this is a priority issue, but someone with nftables experience that could weigh in hopefully.

Did some more work on this.

When you have firewall ipv4 forward filter default-action drop set (to have a closed firewall by default), the following line gets added by vyOS to table ip vyos_filter in the VYOS_FORWARD_filter chain:

counter packets 21 bytes 1260 drop comment "FWD-filter default-action drop"

Per the nft documentation, this drop becomes absolute, regardless of priority or the existence of the accept directives issued by miniupnpd.

https://wiki.nftables.org/wiki-nftables/index.php/Configuring_chains

NOTE: If a packet is accepted and there is another chain, bearing the same hook type and with a later priority, then the packet will subsequently traverse this other chain. Hence, an accept verdict - be it by way of a rule or the default chain policy - isn't necessarily final. However, the same is not true of packets that are subjected to a drop verdict. Instead, drops take immediate effect, with no further rules or chains being evaluated.

This all means that the rules which miniupnpd adds to table inet filter table are never processed as the VyOS firewall rules assert a drop on them first; nftables carries that VyoS drop between chains and priority levels.

Additionally, the nft_init.sh shipped with VyOS has policy drop set by default on the forward chain. This breaks the entire firewall the minute nft_init.sh is run as the miniupnpd rules are installed with default drop on the chain, which will also override any VyOS installed rules.

So the general show-stopper in VyOS is the default-action drop use of the counter drop rule, plus the default policy drop in miniupnpd. The first breaks miniupnpd, the second breaks the entire firewall when nft_init.sh is run unmodified.

The ideal workaround here is to remove the miniupnpd forward chain entirely from nft_init.sh, but leave the miniupnpd chains (filter, prerouting, postrouting) intact. Then, have VyOS create and maintain a jump to the individual chains within the standard VyOS chains themselves (when miniupnpd is enabled, of course).

That would allow the existing firewall setup to remain largely unchanged, avoid the miniupnpd 'drop' default rule created by the forward chain miniupnpd creates by default, and then allow the addition any UPnP/NAT-PMP forward rules as they are added to the miniupnpd chains without breaking anything else.

I suspect this change can happen cleanly in:

https://github.com/vyos/vyos-1x/blob/9753fafbfed02a3b6ebe7b6ddf51783c5dcbcf62/data/templates/firewall/nftables.j2#L48

By inserting jump miniupnpd at line 48 ONLY if the miniupnpd service is active (and of course it's chains have been pre-created). I'm not fully sure how to check for that within the template, though. I've just gotten started with VyOS. :)

I hope this helps. If anything I'm writing doesn't make sense I can clarify and/or do any additional testing. Likewise if I'm missing something key here, I'm anxious to learn.

In T5835#171322, @Viacheslav wrote:

Another bug it that /config/upnp.leases is hardcoded, but there is no script who creates it https://github.com/vyos/vyos-1x/blob/aebb458262072457c6a3840d1b17031fbd780eca/data/templates/firewall/upnpd.conf.j2#L128

It cause of

Jan 10 21:16:03 r4 miniupnpd[9869]: Reloading rules from lease file
Jan 10 21:16:03 r4 miniupnpd[9869]: could not open lease file: /config/upnp.leases

FYI; the miniupnpd daemon will create the lease file when it handles the first lease request from a UPnP/NAT-PMP client; it doesn't need to be created by any install script other than to address the aesthetics of the log notice.

I've found some time to do some work on a fork of vyos-1x. I have a working patch to 1.5-rolling that does the following:

1. Removes the need for any /etc/miniupnpd/*.sh scripts, which could be removed.
2. Moves all miniupnpd firewall rule management to vyos-configd so that the table/chain management happens 'natively'. If service upnp is enabled, the appropriate firewall rules and jumps are created on commit. If it's disabled, the rules are removed cleanly.
3. Updates miniupnpd.conf to point to the correct tables and chains (that are now maintained by VyOS directly). The chains use VyOS style naming conventions, e.g. VYOS_UPNP_filter.
4. The patch works even if no firewall rules are actively being maintained. firewall.py simply makes the standard, blank firewall configuration with the UPnP-relevant prerouting and postrouting chains installed so as to apply the port forwards.

My tests so far seem to be working well and UPnP fully works. I can clean the patch up and present it as a commit if people are interested in checking it out.

If this is something people are interested in perhaps getting a PR made for, there are a couple of issues I'd like to discuss. Specifically:

1. Is IPv6 support for UPnP truly necessary? If so, I'd have to rework my patch a bit to move miniupnpd rules back into an inet filter table to apply to both protocols.
2. For IPv4, UPnP fundamentally requires that a nat be configured. There is currently no check in the system to ensure that when service upnp is configured with an IPv4 listen address that nat is also configured. Furthermore, to do this properly, I think it's necessary to make sure that a nat exists that covers the listen range of the UPnP service. Or am I overthinking this?

Personally I would prefer that the "automagic" firewall ruleset would be done optionally through method described in:

https://vyos.dev/T5509

I think there are actually two aspects here.

The first is that as it stands, UPnP is broken in VyOS. The necessary firewall tables and chains aren't created by default, requiring the user to manually run nft_init.sh. Furthermore, nft_init.sh depends on the nft variant of miniupnpd_functions.sh, which isn't even included in VyOS by default and has to be sourced and installed manually.

Making matters worse, as I describe in https://vyos.dev/T5835#172157 the way the firewall is configured makes UPnP functionality remain essentially broken if your firewall forwards rules use default-action drop. The tables and chains created by the sh scripts can never be reached as the primary VyOS filters drop the packets explicitly.

The patch I've built addresses these aspects; it correctly creates the miniupnpd chains and ensures they are properly called via jump in the firewall rules before the default-action drop is handled.

There isn't a lot 'automagic' here as it just enables the base functions of the UPnP daemon (though UPnP is itself a kind of 'automagic port opener' I suppose).

I agree that the second aspect would be more accurately addressed by https://vyos.dev/T5509 as it is proposed, and I've made no effort to deal with it via my patch.

Specifically, users with restrictive policies applied to an interface that they enable UPnP listening on would need add rules to allow the various UPnP service ports to run (1900 for UPnP IGD, 5351 for NAT-PMP, etc). In 5509 this would be more like set firewall auto-ruleset upnp-server enable to open the necessary ports per the UPnP configuration.

Sounds like a great solution to me, happy to review your PR.

In T5835#172718, @dylanneild wrote:

1. Is IPv6 support for UPnP truly necessary? If so, I'd have to rework my patch a bit to move miniupnpd rules back into an inet filter table to apply to both protocols.

If IPv6 is supported (or meant to be, even if a weird configuration) then we probably shoudn't change it. If using separate tables that won't conflict (or 'catch-all' with an earlier hook) with the standard firewall tables, you might as well use inet tables here.

In T5835#172718, @dylanneild wrote:

2. For IPv4, UPnP fundamentally requires that a nat be configured. There is currently no check in the system to ensure that when service upnp is configured with an IPv4 listen address that nat is also configured. Furthermore, to do this properly, I think it's necessary to make sure that a nat exists that covers the listen range of the UPnP service. Or am I overthinking this?

We could add a block to the UPNP verify() to check if nat is configured, but should only throw a Warning ideally. I don't think it needs to be any stricter or check NAT rules for validity - these potential configuration issues could be addressed in documentation.

Sounds good. I'll do some updates and testing to see if I can move the chains to a dedicated inet table for upnp. The IPv6 use case is probably just to use UPnP/NAT-PMP/PCP to open firewall ports but for completion reasons I'll implement it.

The approach of using a verify() warning when UPnP is enabled without NAT rules seems like a good one. It's also, happily, the least amount of work. I think 99%+ of anyone enabling UPnP is doing so with IPv4/NAT enabled anyways so this is probably an edge case that most will never see.

@sdev Quick question on this issue.

In order to make miniupnpd work with the VyOS firewall as it is presently configured (dedicated ip and ip6 tables), I've had to make a fork of miniupnpd. This is due to the miniupnpd folks effectively declaring that inet tables are "the way it's all going" and effectively removing any ip and ip6 table use in the daemon.

I've tested with the stock miniupnpd to try to implement all rules in a single inet chain, but this creates a problem where, per my comment above, the default-action drop steps on the miniupnpd table, and I'm back to square one.

Using my fork of miniupnpd, I've tested extensively and can see everything working with VyOS IPv4. Rules can be added, checked, and removed without issue. Secure mode works, etc. The fork itself is a fairly small patch; it just reverts some calls in the nftables code to use NFTPROTO_IPV4 rather than NFPROTO_INET for filter and NAT rule changes so as to target the correct table variants.

I can test IPv6 pinhole, which I think can still work using the ip6 tables as-is (my patch doesn't change this at all), but I'd have to setup some tunnelling to validate it as I don't have native IPv6 at my house.

So, my question is: how open is VyOS to using forks of packages as opposed to just pulling them in from Debian?

If not at all, this patch may be at a dead-end unless someone has a clean way to avoid the default-action drop using only priorities (the nftables documentation maintains this can't work)? I considered using meta marking but that would also involve patching miniupnpd to apply the marks to rules matched by it's firewall additions and then modifying VyOS to not apply the default drop when the mark is matched; but that seems a lot less clean.

In T5835#172989, @dylanneild wrote:

@sdev Quick question on this issue.

In order to make miniupnpd work with the VyOS firewall as it is presently configured (dedicated ip and ip6 tables), I've had to make a fork of miniupnpd. This is due to the miniupnpd folks effectively declaring that inet tables are "the way it's all going" and effectively removing any ip and ip6 table use in the daemon.

I've tested with the stock miniupnpd to try to implement all rules in a single inet chain, but this creates a problem where, per my comment above, the default-action drop steps on the miniupnpd table, and I'm back to square one.

Using my fork of miniupnpd, I've tested extensively and can see everything working with VyOS IPv4. Rules can be added, checked, and removed without issue. Secure mode works, etc. The fork itself is a fairly small patch; it just reverts some calls in the nftables code to use NFTPROTO_IPV4 rather than NFPROTO_INET for filter and NAT rule changes so as to target the correct table variants.

I can test IPv6 pinhole, which I think can still work using the ip6 tables as-is (my patch doesn't change this at all), but I'd have to setup some tunnelling to validate it as I don't have native IPv6 at my house.

So, my question is: how open is VyOS to using forks of packages as opposed to just pulling them in from Debian?

If not at all, this patch may be at a dead-end unless someone has a clean way to avoid the default-action drop using only priorities (the nftables documentation maintains this can't work)? I considered using meta marking but that would also involve patching miniupnpd to apply the marks to rules matched by it's firewall additions and then modifying VyOS to not apply the default drop when the mark is matched; but that seems a lot less clean.

We've done custom packages based on the debian source repo when necessary. Do you have a link to the patch diff?

Yep.

Just putting together a PR for vyos-build to integrate it.

In the meantime, this is the actual patch (against the 2.3.3 tag, which is the latest miniupnp that's passing tests and generally deployed).

https://github.com/miniupnp/miniupnp/tree/miniupnpd_2_3_3