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

Resolve GetGenericPortMappingEntry error when router does not support WANIPConnection:1 but only 2 #226

Closed
Ben3094 opened this issue May 5, 2024 · 13 comments

Comments

@Ben3094
Copy link

Ben3094 commented May 5, 2024

Hi @StevenLooman,

My router does not support WANIPConnection:1. So, getting GetGenericPortMappingEntry is not supported too. In this specific case, you could do something like that :

igd_mapped_ports = -1
try:
    while True:
        igd_mapped_ports = igd_mapped_ports + 1
        await self._igd_device.async_get_generic_port_mapping_entry(igd_mapped_ports)
except Exception:
    pass

This improvement could be used to monitor UPnP/IGD router in Home Assistant to make sure that the local network is safe. ;)

Thank you in advance,
Best regards,

@StevenLooman
Copy link
Owner

StevenLooman commented May 7, 2024

Hi @Ben3094, thank you for this issue and the suggestion. I'm unsure where you are using this, can you provide some more context?

There are - currently - only two WANIPConnection specifications:

Trying both explicitly instead of a loop seems to be a better option, in my opinion. Note that there are also many broken devices which do not provide all services. In this case, if the WANIPConnection service would be missing/not provided by the router, this will become an endless loop (or at least a loop which will take a long time to complete).

Also note that many methods of the IgdDevice (or rather the base class UpnpProfileDevice) already ignore the version of the service, by use of the _action() method. E.g.,

action = self._action("WANCIC", "GetTotalBytesSent")

Of course, perhaps this doesn't work as expected. Therefore, can you give some more context so we can debug this?

@Ben3094
Copy link
Author

Ben3094 commented May 7, 2024

Hi @StevenLooman,

It is to answer this issue. To enable UPnP/IGD in router is risky. I think to add Home Assistant UPnP/IGD implementation the ability to monitor opened ports. (First, just count opened ports) This way, some Home Assistant enthusiasts can feel more safe while having UPnP/IGD impllementation running.

My router only supports WANIPConnection v2 but I do not know if v2 has a method to count opened ports. This way, I am searching for a way to get the number of opened ports.

For the moment, I only get SpecifiedArrayIndexInvalid exception... So, the loop ends indeed... ^^'

I will give you my router UPnP description when I can.

Thank you,
Best regards,

@StevenLooman
Copy link
Owner

If Home Assistant would show the number of (IPv4) port mapping entries (not the port mapping entries itself, as discussed before, due to the additional load), would this have any benefit to you? If so, how?

@Ben3094
Copy link
Author

Ben3094 commented May 29, 2024

Hello,

Your suggestion would be enough to diagnose if the router allow other undesired remote connections. So, for me, it is great this way.

Thank you 👍
Best regards,

@StevenLooman
Copy link
Owner

I am a bit hesitant as I expect users will then want to know which port mappings exist, and preferably via Home Assistant. In this case, all that can be done is to point these users to the web interface (if available) of their router.

Also note that this will only work for IPv4, not IPv6, as the WANIPv6FirewallControl does not give the number existing pinholes. Users might be confused about this as well.

@StevenLooman
Copy link
Owner

Perhaps if the sensor is disabled by default, this will cause less confusion.

@Ben3094
Copy link
Author

Ben3094 commented May 30, 2024

A good naming is always the solution 😉
To my point of view, "Number of IPv4 ports opened" would do the job.
I like the idea of giving users a link to the router web page. 👍

@StevenLooman
Copy link
Owner

StevenLooman commented Jun 2, 2024

I like the idea of giving users a link to the router web page. 👍

This used to be there from Home Assistant. Apparently this is no longer the case. I'm also unsure if this can be provided reliably. So lets leave it up to the user to handle this.

@StevenLooman
Copy link
Owner

Once the final PR is merged, I'll create a PR for Home Assistant. You'll get a new sensor with the number of port mappings. Closing this issue.

@Ben3094
Copy link
Author

Ben3094 commented Jun 3, 2024

Thank you for considering my request and answering it so quickly. 😉

@rytilahti
Copy link
Contributor

I like the idea of giving users a link to the router web page. 👍

This used to be there from Home Assistant. Apparently this is no longer the case. I'm also unsure if this can be provided reliably. So lets leave it up to the user to handle this.

Just a side note, the presentationURL in the device description file is meant for this purpose, but it's marked only as "recommended" so not all devices may implement it.

@StevenLooman
Copy link
Owner

Thank you for the clarification @rytilahti. (And impressive that you are watching this repo and commenting! You must having a lot of notifications etc. :) )

Actually, in my Home Assistant "production" environment a link to my router is given. The dummy router which I use for development does not seem to provide this, or I simply have missed it when I looked.

@rytilahti
Copy link
Contributor

Sure thing! Hehe, thankfully not that many notifications as I follow only a very few projects outside those I maintain. I'm following your project as I did some research in the past on UPnP security (https://github.com/RUB-SysSec/MiddleboxProtocolStudy/) and to keep up on changes just in case I need to adapt https://github.com/rytilahti/homeassistant-upnp-availability :-)

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

No branches or pull requests

3 participants