search

StevenLooman / async_upnp_client

Async UPnP Client for Python
Other
46 stars 37 forks source link

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

Closed Ben3094 closed 5 months ago

Ben3094 commented 6 months ago

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 commented 6 months ago

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., https://github.com/StevenLooman/async_upnp_client/blob/3ba37937afbc10fa764420d7ad4b5de327764fa6/async_upnp_client/profiles/igd.py#L213

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

Ben3094 commented 6 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

Ben3094 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

rytilahti commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 :-)