Issue with UpnpSearcher.cs

BaronGreenback commented 4 years ago

I'm running this across my home network, but the code is never picking up my router, as it is also an access point - and the SSDP response is with "urn:schemas-wifialliance-org:device:WFADevice:1".

An ST to "urn:schemas-upnp-org:device:InternetGatewayDevice:1" actually returns the correct "view"of the device.

UpnpSearcher tries to initially match on "urn:schemas-upnp-org:service:WANPPPConnection" or "urn:schemas-upnp-org:service:WANIPConnection", neither of which fit, so its services are not parsed.

As a later function looks the services of the detected item - shouldn't UpnpSearcher be checking devices?

BaronGreenback commented 4 years ago

Also, the response string which i'm getting back when trying to program a port is the following, which doesn't parse.

"<?xml version=\"1.0\"?></s:Body></s:Envelope>"

alanmcgovern commented 3 years ago

@BaronGreenback Sorry it took so long to get back to you.

Can you let me know if this resolves your issue? https://github.com/alanmcgovern/Mono.Nat/pull/21

Also, with this patch in place, would it be possible to use the Mono.Nat NuGet package rather than copying/pasting the source code? I dropped a comment on the changes in your fork asking the same. It'd be great if unmodified source was sufficient!

BaronGreenback commented 3 years ago

Thanks for the response.

Unmodified source code would be the ideal solution, however, given the application in which we are using your code (Jellyfin), the fact that your singleton initialises once, at startup, causes a problem.

Initialising at startup means that if something happens to the router (ie. reboot), then as the device stays in the mono's device list, and won't be re-discovered, so there is no way of remapping ports. Likewise, if there are any other network changes after initialisation, the mono class will never detecte these and reflect them. (I live in a rural location where this happens repeatedly - so the problems are real to me),
Jellyfin has the capability of disabling upnp. If this is done, the port forwarding mappings should be ideally be removed from the device, but the RaiseDeviceLost event in isn't attached to anything, so doesn't work.

I understand that these are not bugs within your code, as it does exactly what you designed it to do, however, for the jellyfin implementation it isn't a perfect fit; hence the need to change.

The changes I've made to your code for jellyfin. basically instantiates the code, enabling it to be refreshed on network changes.

BaronGreenback commented 3 years ago

I had to include the following in the mono.nat.upnp.UpnpSearcher.cs as the response string didn't match.

else if (dataString.IndexOf("urn:schemas-upnp-org:device:InternetGatewayDevice:", StringComparison.OrdinalIgnoreCase) != -1) { Log(log, "urn:schemas-upnp-org:device:InternetGatewayDevice:"); }

Hope you don't mind, but I've noticed that you have left the :1 in line 126 of this file, even though it's not on the lines above and below. Shouldn't line 227 then also detect for the type without the version number?

alanmcgovern commented 3 years ago

Hrm, the line numbers you're quoting don't match what is in the PR. Can you double check against https://github.com/alanmcgovern/Mono.Nat/blob/9798da3be1ee9e8ba7393930d744b907a9397aca/Mono.Nat/Upnp/Searchers/UpnpSearcher.cs ?

I can additionally check for an unversioned IGD. At the moment the SSDP search request is sent for version 1 and 2, so i hope that the response received will be either v1 or v2. The original code did not accept any kind of 'urn:schemas-upnp-org:device:InternetGatewayDevice' as valid, but the new code will accept either 'urn:schemas-upnp-org:device:InternetGatewayDevice:1' or 'urn:schemas-upnp-org:device:InternetGatewayDevice:2' as we are now explicitly searching for those.

I can add an unversioned IGD as a fallback anyway as it's unlikely to do any harm :)

EDIT: Could you manually download a copy of your device's service list so I can manually review it? I want to see what's in there to make sure we will definitely do the right thing.

alanmcgovern commented 3 years ago

Initialising at startup means that if something happens to the router (ie. reboot), then as the device stays in the mono's device list, and won't be re-discovered, so there is no way of remapping ports. Likewise, if there are any other network changes after initialisation, the mono class will never detecte these and reflect them. (I live in a rural location where this happens repeatedly - so the problems are real to me)

There are two approaches to resolve this problem.

1) If a NAT device has been discovered and then the network is disconnected/reconnected, you still have a reference to the INatDevice object and can refresh any port mappings.

2) We can clear the discovered devices when StopDiscovery is invoked, which gives people an easy way to re-discover devices.

Jellyfin has the capability of disabling upnp. If this is done, the port forwarding mappings should be ideally be removed from the device, but the RaiseDeviceLost event in isn't attached to anything, so doesn't work.

If jellyfin wants to disable port mappings then it should use the INatDevice object it has to delete/remove any port mappings it created. A DeviceLost event wouldn't help in this scenario, even if i were hooked up to anything :)

The semantics of what a 'DeviceLost' event actually means are pretty hard to define. I think removing the event would be best as it doesn't really add any value. The best sign a device is offline is when invocations to the INatDevice object begin to fail. At that point the state of the port mappings is unknown. They'll probably be gone when the device comes back online, but they might not be gone.

Additionally, if your device drops off the network and then reconnects and receives a different IP, you might be unable to delete the old port mappings as your IP will no longer match the one used to create the mapping. Most routers don't allow you to delete mappings created for a different local IP.

BaronGreenback commented 3 years ago

After an ssdp:all my router (an EE Smart Hub Manager) only responds with a urn:schemas-wifialliance-org:device:WFADevice:1

<scpd xmlns="urn:schemas-upnp-org:service-1-0">
<specVersion>
<major>1</major>
<minor>0</minor>
</specVersion>
<actionList>
<action>
<name>GetDeviceInfo</name>
<argumentList>
<argument>
<name>NewDeviceInfo</name>
<direction>out</direction>
<relatedStateVariable>DeviceInfo</relatedStateVariable>
</argument>
</argumentList>
</action>
<action>
<name>PutMessage</name>
<argumentList>
<argument>
<name>NewInMessage</name>
<direction>in</direction>
<relatedStateVariable>InMessage</relatedStateVariable>
</argument>
<argument>
<name>NewOutMessage</name>
<direction>out</direction>
<relatedStateVariable>OutMessage</relatedStateVariable>
</argument>
</argumentList>
</action>
<action>
<name>PutWLANResponse</name>
<argumentList>
<argument>
<name>NewMessage</name>
<direction>in</direction>
<relatedStateVariable>Message</relatedStateVariable>
</argument>
<argument>
<name>NewWLANEventType</name>
<direction>in</direction>
<relatedStateVariable>WLANEventType</relatedStateVariable>
</argument>
<argument>
<name>NewWLANEventMAC</name>
<direction>in</direction>
<relatedStateVariable>WLANEventMAC</relatedStateVariable>
</argument>
</argumentList>
</action>
<action>
<name>SetSelectedRegistrar</name>
<argumentList>
<argument>
<name>NewMessage</name>
<direction>in</direction>
<relatedStateVariable>Message</relatedStateVariable>
</argument>
</argumentList>
</action>
</actionList>
<serviceStateTable>
<stateVariable sendEvents="no">
<name>Message</name>
<dataType>bin.base64</dataType>
</stateVariable>
<stateVariable sendEvents="no">
<name>InMessage</name>
<dataType>bin.base64</dataType>
</stateVariable>
<stateVariable sendEvents="no">
<name>OutMessage</name>
<dataType>bin.base64</dataType>
</stateVariable>
<stateVariable sendEvents="no">
<name>DeviceInfo</name>
<dataType>bin.base64</dataType>
</stateVariable>
<stateVariable sendEvents="yes">
<name>APStatus</name>
<dataType>ui1</dataType>
</stateVariable>
<stateVariable sendEvents="yes">
<name>STAStatus</name>
<dataType>ui1</dataType>
</stateVariable>
<stateVariable sendEvents="yes">
<name>WLANEvent</name>
<dataType>bin.base64</dataType>
</stateVariable>
<stateVariable sendEvents="no">
<name>WLANEventType</name>
<dataType>ui1</dataType>
</stateVariable>
<stateVariable sendEvents="no">
<name>WLANEventMAC</name>
<dataType>string</dataType>
</stateVariable>
<stateVariable sendEvents="no">
<name>WLANResponse</name>
<dataType>bin.base64</dataType>
</stateVariable>
</serviceStateTable>
</scpd>

BaronGreenback commented 3 years ago

Specifically querying for InternetGatewayDevice:1 returns

<?xml version="1.0"?>
<root xmlns="urn:schemas-upnp-org:device-1-0" configId="1337">
  <specVersion>
    <major>1</major>
    <minor>0</minor>
  </specVersion>
  <device>
    <deviceType>urn:schemas-upnp-org:device:InternetGatewayDevice:1</deviceType>
    <friendlyName>EE Smart Hub</friendlyName>
    <manufacturer>EE</manufacturer>
    <manufacturerURL>http://www.ee.co.uk/</manufacturerURL>
    <modelDescription>EE Smart Hub 6.0B</modelDescription>
    <modelName>EE Smart Hub 6.0B</modelName>
    <modelNumber>1</modelNumber>
    <modelURL>http://eehub.home</modelURL>
    <serialNumber>+EEH001+1903011269</serialNumber>
    <UDN>uuid:5e9ceedd-3868-4d16-bd65-5756f2ce3013</UDN>
    <deviceList>
      <device>
        <deviceType>urn:schemas-upnp-org:device:WANDevice:1</deviceType>
        <friendlyName>WANDevice</friendlyName>
        <manufacturer>EE</manufacturer>
        <manufacturerURL>http://www.ee.co.uk/</manufacturerURL>
        <modelDescription>EE Smart Hub 6.0B</modelDescription>
        <modelName>EE Smart Hub 6.0B</modelName>
        <modelNumber>1</modelNumber>
        <modelURL>http://eehub.home</modelURL>
        <serialNumber>+EEH001+1903011269</serialNumber>
        <UDN>uuid:5e9ceedd-3868-4d16-bd65-5756f2ce3014</UDN>
        <UPC>000000000000</UPC>
        <serviceList>
          <service>
            <serviceType>urn:schemas-upnp-org:service:WANCommonInterfaceConfig:1</serviceType>
            <serviceId>urn:upnp-org:serviceId:WANCommonIFC1</serviceId>
            <controlURL>/5e9ceedd/ctl/CmnIfCfg</controlURL>
            <eventSubURL>/5e9ceedd/evt/CmnIfCfg</eventSubURL>
            <SCPDURL>/5e9ceedd/WANCfg.xml</SCPDURL>
          </service>
        </serviceList>
        <deviceList>
          <device>
            <deviceType>urn:schemas-upnp-org:device:WANConnectionDevice:1</deviceType>
            <friendlyName>WANConnectionDevice</friendlyName>
            <manufacturer>EE</manufacturer>
            <manufacturerURL>http://www.ee.co.uk/</manufacturerURL>
            <modelDescription>EE Smart Hub 6.0B</modelDescription>
            <modelName>EE Smart Hub 6.0B</modelName>
            <modelNumber>1</modelNumber>
            <modelURL>http://eehub.home</modelURL>
            <serialNumber>+EEH001+1903011269</serialNumber>
            <UDN>uuid:5e9ceedd-3868-4d16-bd65-5756f2ce3015</UDN>
            <UPC>000000000000</UPC>
            <serviceList>
              <service>
                <serviceType>urn:schemas-upnp-org:service:WANPPPConnection:1</serviceType>
                <serviceId>urn:upnp-org:serviceId:WANPPPConn1</serviceId>
                <controlURL>/5e9ceedd/ctl/PPPConn</controlURL>
                <eventSubURL>/5e9ceedd/evt/PPPConn</eventSubURL>
                <SCPDURL>/5e9ceedd/WANPPPCn.xml</SCPDURL>
              </service>
            </serviceList>
          </device>
        </deviceList>
      </device>
    </deviceList>
    <serviceList>
      <service>
        <serviceType>urn:schemas-upnp-org:service:Layer3Forwarding:1</serviceType>
        <serviceId>urn:upnp-org:serviceId:L3Forwarding1</serviceId>
        <SCPDURL>/5e9ceedd/L3F.xml</SCPDURL>
        <controlURL>/5e9ceedd/ctl/L3F</controlURL>
        <eventSubURL>/5e9ceedd/evt/L3F</eventSubURL>
      </service>
    </serviceList>
    <presentationURL>http://eehub.home</presentationURL>
  </device>
</root>

alanmcgovern commented 3 years ago

Perfect, thanks for sharing that! The patch in master should cover this case correctly now. Do let me know if that's not the case!

alanmcgovern commented 3 years ago

Mono.Nat now clears it's cache of detected devices when you call NatUtility.StartDiscovery/StopDiscovery. This should address your issue about re-discovering devices after stopping/starting.

In addition, if you want to clear port mapping when jellyfin disables port forwarding, the appropriate thing to do would be to call DeletePortMapAsync for each mapping you want to remove.

If you think this puts mono.nat in a place where you can rely on the nuget package without modifying it - great! Otherwise I'd love to know what else needs to be done so you don't have to modify it.

BaronGreenback commented 3 years ago

Jellyfin is intended to be a single launch application - so needs to be able to deal with anything thrown at it, without having to restart it. It also needs to be able to trace issues if they arise. I really apprecate what you have done so far, but can still see one issue, and one nice to have. The issue, is the static implementation of the sockets at creation. If the network changes and interface state changes (I'm from a corporate IT background so see this quite regularly), the isn't a way of re-initialising, or re-scanning for interfaces for changes. (I'm trying to think of every eventuality)

I know this is a design change from how the system is currently implemented - but it would add a level of stability.

The second is a nice to have - and could be very easy to implement - custom logging. If you could modify the NatUtility log function to use an interface for the NAT Log function (not a TextWriter) then the user could employ their own custom logging, enabling user apps to use centralised logging. Another possibilty would be callback functionality.

eg.

interface INATLogger 
{
    void Log (string format, params object [] args);
}

internal static void Log (string format, params object [] args)
{
    INATLogger logger = Logger;
        if (logger != null)
       lock(LoggerLocker)
          logger.Log(format, args);
}

alanmcgovern commented 3 years ago

The issue, is the static implementation of the sockets at creation. If the network changes and interface state changes (I'm from a corporate IT background so see this quite regularly), the isn't a way of re-initialising, or re-scanning for interfaces for changes. (I'm trying to think of every eventuality)

This is supported now by calling NatUtility.StopDiscovery/NatUtility.StartDiscovery. When you stop the searcher it cleans up any sockets.

The second is a nice to have - and could be very easy to implement - custom logging. If you could modify the NatUtility log function to use an interface for the NAT Log function (not a TextWriter) then the user could employ their own custom logging, enabling user apps to use centralised logging. Another possibilty would be callback functionality.

I copied an implementation of this which I used in another project. Users of the library can use Mono.Nat.Logging.Logger.Factory to pass in a function which let's you create an ILogger. The parameter passed to you is the name of the class. You can discard this if you don't want to use it.

Let me know if there's anything else!

BaronGreenback commented 3 years ago

That's great! Thankyou for the hard work.

I'll close my PR and compile the latest into jellyfin and do some testing.

Thanks again.

BaronGreenback commented 3 years ago

M$ SSDPSRV intercepts the Search responses under windows.

I've updated the PR to implement a multicast port that listens on 0.0.0.0:1900.

Ideally this should only need to be used if it's running on windows.

alanmcgovern commented 3 years ago

I think all of the points raised in this issue have been addressed and the required changes are in master now.

I'll go ahead and close this, but please do re-open this issue, or open new ones, if there are further changes which would improve the library for your usecase!

alanmcgovern / Mono.Nat

Issue with UpnpSearcher.cs #17