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

Any update on this PR? (thanks for the work put into this!!)

bruh

@aidan-gibson It's never worked, and demand is slim to none
main use case is games typically, which is not in priority for us

In T5835#187910, @syncer wrote:

@aidan-gibson main use case is games typically, which is not in priority for us

I don't think so, there are many other use cases beside games. Many peer to peer protocols rely on UPnP to open ports for NAT traversal (before falling back to some other hole punching techniques). Some examples we came across recently include libp2p or zerotier and there are many more.
One use case we had was on-demand opening UDP ports for wireguard tunnels, which is a nice way to grant some limited control of the firewall to the services behind it, without having to centralize everything on the firewall itself. With miniupnpd ability for fine grained permission rules (like allowed port range and allowed source ip ranges it accepts UPnP request from) this is a nice solution.

So vyos having proper UPnP support has definitely some relevancy outside of home use

No doubt that there are other use cases.
since 1.2 LTS, we received zero requests from customers about adding UPnP, hence, don't see any value in it

I have rarely seen UPnP in enterprise environments and rarely at home even if the main purpose is to use it at home and let applications backdoor your firewall (which often is a bad thing in enterprise evironments).

So I wont protest against having UPnP being removed but on the other hand it wouldnt matter if it remained in VyOS (those who dislike it will just not enable it - simple as that).

But if it will remain then somebody needs to dig through the UPnP code and have it properly fixed since it seems to be currently malfunctioning.

One reasons it is rarely seen is as most are not aware of it being used undercover and when not being present, nothing necessarily brakes (due to fallback to other mechanisms). For some home routers we saw this was an undocumented "feature" that you did not have any control over, more recent & reasonable implementation we have seen allow you to enable or disable it (but nothing much more like fine grained permissions)

From my understanding after our analysis of what is broken, @dylanneild has already done substantial work to fix it, so it is only a matter of wrapping it up. Would be quite a sad to let that effort go to waste (and allow contributers to work on it only to later propose to remove the feature all together instead of suggesting that in the first place when we opened the ticket)

Does it work now?

In T5835#187919, @syncer wrote:

Does it work now?

yes, I would think so:

In T5835#172718, @dylanneild wrote:

My tests so far seem to be working well and UPnP fully works [...]

but I think one product management related question that was raised is still kind of unanswered: Should / must IPv6 also be supported. @dylanneild later went ahead and implemented it regardless but had to do some extra work (patch of miniupnpd) to get it working with vyos.

I'd suggest to wait for @dylanneild to give a update before deciding on anything.

In summary, it works with custom scripts and patches, but it still does not work from CLI (not fully integrated)
The scripts that should be involved are in the repo https://github.com/miniupnp/miniupnp/tree/miniupnpd_2_3_3/miniupnpd/netfilter_nft/scripts
Until we do not have them and they do not communicate with the firewall, the feature does not work.
A patch is attached in several posts above https://vyos.dev/T5835#174066

So, the original implementation never worked, as firewall rules were not involved.
The current implementation starts miniupnpd with some configuration that, as I understand, is completely useless without a firewall.

That’s why the question arose: if it hasn’t worked since implementation and is especially unnecessary for anyone, what’s the point of keeping it in the code? No one is working on this yet.

Do I understand correctly that this feature opens/forwards required application ports for a host (like a home router) and won't be useful, for example, for 50-200 users?

I fail to comprehend how a firewall that autonomously opens ports via calls from internal networks is appropriate for an enterprise.
Indeed there are some use cases but this functionality can be used by malicious code and allow bypass security configuration that is enforced otherwise

I'm not sure if that summary from you @Viacheslav is fully reflecting the current state.
I'm also not sure if the original implementation never worked, might very well have been broken while refactoring some vyos internals how the firewall is structured, but I guess you should have a better understanding of (the history of) your product. Otherwise I would be very surprised if a broken feature got into your product without every working / being tested.

In T5835#187927, @syncer wrote:

I fail to comprehend how a firewall that autonomously opens ports via calls from internal networks is appropriate for an enterprise.

A firewall is doing exactly this all the time when using NAT, autonomously opening ports via call from internal networks (aka internal originated traffic) to allow responses to reach the originator. Enterprises might have some strict outbound rules. For UPnP is exactly the same, an enterprise would have strict rules which services are allowed to open ports.
(all this is assuming that an "enterprise" stands for security aware entity with appropriate security measures in places, which not always hold true)

Security works with well defined boundaries.

Allowing certain services (from trusted internal network segment) to allow open certain range of ports (designed for particular service) when those services need that port, that is perfectly fine form a security point of view (and in some cases even better than static / permanent opening of ports). If you have the more "traditional" security approach in mind that is more centralized and static, you would rather start a internal (often manual) process to get certain ports opened for certain services. The issue there is often that those opened ports outlive the actual services behind them, and you end up with a firewall with many holes as well.

In T5835#187927, @syncer wrote:

[...] but this functionality can be used by malicious code and allow bypass security configuration that is enforced otherwise

As this feature is opt in, by definition it is no bypass of the security configuration. If you enable UPnP and allow requests from the whole internal network to open any ports, yes, maybe that is not the wisest security decision. But same is true for the firewall itself. Or would you suggest to remove the feature of the firewall to open ports, lets say port 80 or 443? This functionality could be used by malicious code to bypass security.

In T5835#187933, @simplysoft wrote:

A firewall is doing exactly this all the time when using NAT, autonomously opening ports via call from internal networks (aka internal originated traffic) to allow responses to reach the originator. Enterprises might have some strict outbound rules. For UPnP is exactly the same, an enterprise would have strict rules which services are allowed to open ports.

Not if it's not configured to do so.

In T5835#187933, @simplysoft wrote:

I'm not sure if that summary from you @Viacheslav is fully reflecting the current state.
I'm also not sure if the original implementation never worked, might very well have been broken while refactoring some vyos internals how the firewall is structured, but I guess you should have a better understanding of (the history of) your product. Otherwise I would be very surprised if a broken feature got into your product without every working / being tested.

I have zero knowledge of UPnP and never use it. It was implemented by a community member @jack9603301 and passed smoketests that he wrote. I see the code and examples of the config of the daemon and can check if the daemon starts at all without errors. But I have no clue how it works.
Based on the above discussion, I assume it never worked as expected, as the firewall part was never implemented.

If you know how to test it, it will be great. If no one needs it, even for tests, what are we talking about?

Just my IMHO

In T5835#187934, @syncer wrote:

In T5835#187933, @simplysoft wrote:

A firewall is doing exactly this all the time when using NAT, autonomously opening ports via call from internal networks (aka internal originated traffic) to allow responses to reach the originator. Enterprises might have some strict outbound rules. For UPnP is exactly the same, an enterprise would have strict rules which services are allowed to open ports.

Not if it's not configured to do so.

Yes, that is exactly the point. Glad you did not suggest to remove the NAT capability of vyos because it could be used to bypass security or is not appropriate for an "enterprise"

In T5835#187936, @simplysoft wrote:

Yes, that is exactly the point. Glad you did not suggest to remove the NAT capability of vyos because it could be used to bypass security or is not appropriate for an "enterprise"

You can be sarcastic in another place if you don´t like something
go learn how cheap cameras open firewalls via UPnP and make them available on the internet without people being aware of that

Created a poll for maintainers on this topic, and we will go with the decision made.

In T5835#187935, @Viacheslav wrote:

If you know how to test it will be great to test it. If no one needs it even for tests, what are we talking about?

We are happy to test when we have all necessary pieces (and there is a commitment to get this merged). Currently this ticket does not contain all patches / fixes required for a working solution, would be glad if @dylanneild could provide it

In T5835#187937, @syncer wrote:

go learn how cheap cameras open firewalls via UPnP and make them available on the internet without people being aware of that

or how malware exfiltrates data via port 443 because enterprises can't reliably block outbound traffic on that port.

Good (or should I say lucky?) for you that the initial contributor did not implement it with UPnP being enabled by default then.
No need to be educating. I get your product management related argument, but the technical and security arguments are pretty weak sorry.

In T5835#187938, @syncer wrote:

Created a poll for maintainers on this topic, and we will go with the decision made.

Sad that you did not wanted to wait to hear the current status from the one who invested the most of time in this ticket. I would strongly suggest to first understand the status & context, the feature and its implications before making decisions.

And this hold true for both including or removing a feature into your product.

Either way how this turns out in the end, I can't see much good light in all of this, lots of time wasted.
Luckily we don't depend on UPnP and/or vyos

A bunch to unpack here.

I'm pretty dismissive of the general notion that modern UPnP / NAT-PMP is an attack vector in and of itself. If you enable UPnP on a VLAN with 'cheap cameras', that's probably more a configuration error on your part than an issue with UPnP. More importantly, if you're worried about UPnP with this type of device, just wait until you find out about hole punching.

Well configured / deployed UPnP/NAT-PMP allows devices to receive external ports to themselves only. It's not going to compromise another device on the network directly.

It's almost like the solution is to just not put 'cheap cameras' on a network you need to keep secure, but I digress.

As for VyOS and UPnP:

(I'm writing all of this from memory and the technicals may have changed in current VyOS builds, etc)

• The integrated miniupnpd "works", but it doesn't work with VyOS or the associated firewall configuration mechanism.
• The root cause is that VyOS manages the underlying netfilter firewall by translating it's firewall configuration into the ip and ip6 tables. The integrated minupnpd daemon expects all rules to go into the inet table as, generally, using only the inet table is what the netfilter folks seem to think is the more 'correct' way of managing the firewall going forward.
• As a result, any rules the miniupnpd creates when it attempts to open ports go into a firewall table that conflicts with other firewall rules. Because of the way jumping between tables works in netfilter this is impossible to correct/address directly with the integrated binary.
• The smoke tests that exist/existed for UPnP were basically valueless as I recall. They were passing without actually testing anything was in fact working. This is self-evident, as UPnP doesn't work in VyOS. I'm not sure how UPnP ever got merged in the first place, to be honest.
• My fix was to fork miniupnpd and patch it so as to force it to add it's IPv4 rules to the ip tables and it's IPv6 rules to the ip6 tables, matching the pattern that VyOS uses. This had to be coupled with patches to the VyOS firewall scripts to generate the appropriate 'jumps' in the filtering and forwarding rulesets so as to apply the miniupnpd rule additions.
• I had all of this working without issue on a 1.4 / current build of VyOS and it appeared to be stable.

As for getting my patches merged in, I hate to say it but I gave up on the effort.

In general I found the response to the entire idea to be exactly what has happened in this thread today. Mostly just arguing about if it was necessary, what the point was, etc. On top of that, I found the path to getting the patch integrated involved too many parts of VyOS and too many people with opinions about that particular part and/or the suitability of patching that part to accommodate UPnP.

1. You need to remove the stock miniupnpd Debian install, which doesn't work.
2. You need to merge in a patched version of miniupnpd that does work, outside of the standard Debian package dependencies.
3. You need to patch the various Python scripts that deal with the firewall, as you need to make patches to the way the nftables configuration file is generated when the firewall is updated and/or when the UPnP daemon configuration changes.
4. The configuration scripts for UPnP have some missing conventions and/or incorrect configuration elements, as I recall.
5. There is no status mechanism script to read the UPnP lease file and show what ports have actually been opened, which makes the system less useful.

Broadly, it just seemed like a lot more trouble than it was worth to get this working.

If I had to recommend anything, it'd be that VyOS just remove UPnP completely. What is included doesn't work (or didn't when I last checked) and the attitude from the VyOS community seems to be that it's not wanted/needed anyways.

In T5835#187938, @syncer wrote:

Created a poll for maintainers on this topic, and we will go with the decision made.

Out of curiosity, will the details of the poll be public or the results being shared transparently?
I would have expected the poll to be linked to this ticket in the meantime, reading your marketing material about "VyOS development process is open and transparent"

In T5835#187963, @dylanneild wrote:

A bunch to unpack here.
[...]

Thanks @dylanneild, IMHO the best contribution to this matter today

Out of curiosity, will the details of the poll be public or the results being shared transparently?

If you are really that curious, I can attach a screenshot.

But if we had a working implementation from the community, there wouldn't be a poll. Just saying...

If someone wants, I can probably unearth my patches to 1.4 and miniupnpd to make it all work. It was technically functional and worked as expected. I just don't have the time or patience to deal with getting it merged/integrated back into the project.

The only thing I didn't add was a status script to get lease data, but that would be easy enough to create.

I'm AFK for a few days and would have to spin up some cold VMware instances so it'd take me a bit to grab / package up the relevant components.

In T5835#187967, @dmbaturin wrote:

If you are really that curious, I can attach a screenshot.

I leave that up to you guys and gals if you live up to your claims

But if we had a working implementation from the community, there wouldn't be a poll. Just saying...

There are many other but ifs:

• If maintainers would understand the technical details...
• If maintainers would test features before merging...
• If maintainers would discuss product management related questions before community is working on a fix...
• If product management would answer open product management related questions...
• If product management would make an attempt to understand the context / ticket...

Just saying...

In T5835#187927, @syncer wrote:

I fail to comprehend how a firewall that autonomously opens ports via calls from internal networks is appropriate for an enterprise.
Indeed there are some use cases but this functionality can be used by malicious code and allow bypass security configuration that is enforced otherwise

The thing is that VyOS can be also used by others who does not have unlimited wallet sizes aka multibillion corporations.

Plenty of users of Ubiquiti Edgerouter series exists both among enterprises AND home users - and EdgeOS is a Vyatta fork, which VyOS also is...

My €0.05 is that a feature which currently malfunctions should be removed until its properly fixed and then be included as a PR commit to the current rolling-series.

When it comes to UPnP that should be fully configurable through CLI (and CLI only) to be functional and that it should support both IPv4 and IPv6 (since we nowadays have NAT64 and NAT66 in the IPv6 world) before let back to the rolling-releases.

Having something that is semi-working and needs additional behind the scenes hacking incl custom non official scripts is IMHO just wrong.

In T5835#187927, @syncer wrote:

I fail to comprehend how a firewall that autonomously opens ports via calls from internal networks is appropriate for an enterprise.
Indeed there are some use cases but this functionality can be used by malicious code and allow bypass security configuration that is enforced otherwise

Because the other option which these admins would revert to is "any, any, accept" which is far worser.

I could argue with additional upnp use cases, but that doesn't seem useful here. It seems the only solution that isn't cringe is outlined here:

The root cause is that VyOS manages the underlying netfilter firewall by translating it's firewall configuration into the ip and ip6 tables. The integrated minupnpd daemon expects all rules to go into the inet table as, generally, using only the inet table is what the netfilter folks seem to think is the more 'correct' way of managing the firewall going forward.

Arguing about adding in unsustainable bodges or deleting all upnp entirely is silly when the real issue is the way VyOS manages the firewall config. upnp breakage is just a byproduct of that.

The question is, is there any interest in fixing it, / how hard would that be?

I'm tempted to put in the time and fix it myself, but having only begun my VyOS adventure the past week or so, my initial excitement has tapered down. VyOS is "open source" in name, but the attitude of the project of late has been a huge turn-off for contributors & potential contributors, the dev documentation is completely dead, and the build process is extremely convoluted and there's a clear financial incentive to keep it that way. This is pretty disappointing. Given all this and the technical debt in the project, it's tempting to start anew with NixOS rather than put in work with a project that forgot it's GPLv2.

Not dismissing the frustrations that must've led the main devs to this point, maybe this perspective is naïve (and I still love VyOS: total breath of fresh air coming from opnsense), but the priorities of the project seem disjoint from when it was founded, and it seems its only a matter of time before disruption.

I guess the fate of upnp has already been decided https://vyos.dev/rVYOSONEX7c438caa2c21101cbefc2eec21935ab55af19c46
RIP

In T5835#188048, @aidan-gibson wrote:

I guess the fate of upnp has already been decided https://vyos.dev/rVYOSONEX7c438caa2c21101cbefc2eec21935ab55af19c46
RIP

yeah, turned out as kind of foreseeable / feared.
Decision was probably already made way before an alleged "poll" was carried out. If there was a poll it was probably "Pollice verso" style without much background, details and informed discussions around it.
Original ticket T3420 was marked as invalid yesterday at at 4:58 PM with only "Implementation never worked". So far no intention visible to share the poll nor the result, nor the final reasoning. Removal of the feature not referencing discussion in this ticket and the PR still containing questionable reasoning that was discussed in this ticket:

Security:
UPnP has been historically associated with security risks due to its automatic
and potentially unauthenticated nature.
UPnP devices might be vulnerable to unauthorized access or exploitation.

With all that, everyone is still in the dark if a working implementation should ever be attempted & is wanted at all.

So we are left with our own personal conclusion: UPnP is gone because CEO does not like it and they did not make any money with it as clearly stated:

In T5835#187914, @syncer wrote:

we received zero requests from customers about adding UPnP, hence, don't see any value in it

Sad that they could not even be at honest but instead hide behind dubious security reasons.

And never even a word of gratitude for the work of the "community" but rather subliminal blame of "community" to provide such a feature.
And kind of worst: Not a word of apology for rather late "change of mind" that should have taken place in the very beginning.

Failures & oversight can happen, falling for questionable arguments / reasoning happens to all of us, we can all have late insights, but what makes the difference is the attitude towards it.
The culture & project we experienced as part of this ticket is not something worth supporting for us.

Sorry @dylanneild for triggering your wasted work, let us know if we can buy you some coffee or otherwise partly make up for it.

you are not from the community @simplysoft
it's time to depart from here

Wow...honestly no words

yes, @aidan-gibson, there are no plans to entertain shit-talkers with a single task and no contributions

Sorry, but seems like he was right. If the build process or devs with power weren't toxic I'd jump on preparing a PR for fixing the firewall config issue, but both being the case, I'd rather not waste my time contributing to a burning ship.

Best of luck.

Awesome. Good luck.

In T5835#187963, @dylanneild wrote:

If I had to recommend anything, it'd be that VyOS just remove UPnP completely. What is included doesn't work (or didn't when I last checked) and the attitude from the VyOS community seems to be that it's not wanted/needed anyways.

Just because some of the community or a majority of the community dislikes a specific feature doesnt mean that EVERYBODY dislikes it.

Im one of those who wont use UPnP myself but I have nothing against if its included as a feature, aka disabled by default but when needed can be enabled. Same with Dynamic DNS and a couple of other features.

But as I also previously wrote in this task this means that there needs to be somebody who is actively maintaining this feature specially if it depends on a custom binary build of miniupnpd.

dylanneild added a comment.Tue, May 14, 8:59 AM
If someone wants, I can probably unearth my patches to 1.4 and miniupnpd to make it all work. It was technically functional and worked as expected. I just don't have the time or patience to deal with getting it merged/integrated back into the project.

The only thing I didn't add was a status script to get lease data, but that would be easy enough to create.

I'm AFK for a few days and would have to spin up some cold VMware instances so it'd take me a bit to grab / package up the relevant components.

Well, personally (not a contributor) I'd love to have it, it's way better than opening a whole swath of ports for IPv6 and pretty much the only way to have full hosting capability when using NAT.

You can still have it in a container easily; as I mentioned, it has never worked since 2021
You do not lose anything.

I'd prefer to integrate the Port Control Protocol (PCP) instead.

In T5835#188605, @Viacheslav wrote:

You can still have it in a container easily; as I mentioned, it has never worked since 2021
You do not lose anything.

I understand it never worked, but if it could be made to work it would allow me to host some games I can't now. PCP may be preferable in abstract, but I have no software (that I know of) that uses it, currently most games and consoles support UPNP. Both would be ideal.

I don't understand how it could be easily run in a container, even if the guest could modify the hosts firewall tables it would have the same integration issues that dylanneild resolved, would it not?

In T5835#188611, @Viacheslav wrote:

I'd prefer to integrate the Port Control Protocol (PCP) instead.

Regardless of whether you prefer upnp, nat-pmp, or pcp, the solution is still successful integration with miniupnp, which supports em all. (In theory if you found a program that just supported pcp and wanted to implement that, sure, but irl everyone just uses miniupnp)