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

get the IP associated with the interface used to successfully discover the upnp setup ? #48

Closed
mh-cbon opened this issue Mar 5, 2022 · 26 comments

Comments

@mh-cbon
Copy link
Contributor

mh-cbon commented Mar 5, 2022

Hi,

In order to perform a PortMapping, the computer IP to route the traffic to, seems to be required (the remote does not appear capable of figuring that out by itself upon a query), otherwise a soap fault is encountered.

In this code,

func GetIPAndForwardPort(ctx context.Context, lPort, rPort int, proto string) error {
	client, err := PickRouterClient(ctx)
	if err != nil {
		return err
	}

...

	return client.AddPortMapping(
...
		// Internal address on the LAN we want to forward to.
		"", // <- how to fill this value
...
	)
}

I was hoping that the ssdp discovery result would contain the IP used to perform the successful query, but I was not able to locate it. ​

https://github.com/huin/goupnp/blob/master/network.go#L21

My setup is like that

$ ifconfig
enp58s0f1: flags=4099<UP,BROADCAST,MULTICAST>  mtu 1500
       ​ether 80:fa:5b:43:59:24  txqueuelen 1000  (Ethernet)

lo: flags=73<UP,LOOPBACK,RUNNING>  mtu 65536
       ​inet 127.0.0.1  netmask 255.0.0.0
       ​loop  txqueuelen 1000  (Boucle locale)

tun0: flags=4305<UP,POINTOPOINT,RUNNING,NOARP,MULTICAST>  mtu 1331
       ​inet 198.18.80.9  netmask 255.255.248.0  destination 198.18.80.9
       ​unspec 00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00  txqueuelen 100  (UNSPEC)

wlp59s0: flags=4163<UP,BROADCAST,RUNNING,MULTICAST>  mtu 1500
       ​inet 192.168.1.78  netmask 255.255.255.0  broadcast 192.168.1.255
       ​ether 00:28:f8:4b:d0:b1  txqueuelen 1000  (Ethernet)

I find it hard to guess but maybe i am missing something obvious ? ><

mh-cbon added a commit to mh-cbon/goupnp that referenced this issue Mar 5, 2022
@mh-cbon
Copy link
Contributor Author

mh-cbon commented Mar 5, 2022

I have sketched some changes, though, it smells the breakage !

mh-cbon added a commit to mh-cbon/goupnp that referenced this issue Mar 5, 2022
mh-cbon added a commit to mh-cbon/goupnp that referenced this issue Mar 5, 2022
mh-cbon added a commit to mh-cbon/goupnp that referenced this issue Mar 5, 2022
mh-cbon added a commit to mh-cbon/goupnp that referenced this issue Mar 6, 2022
@mh-cbon
Copy link
Contributor Author

mh-cbon commented Mar 6, 2022

Hello again,

It worth a shot to use response headers.

Though, when i was about to generate the files using the cli, I have ran into a small suprise.

UPNP website is down. Not sure what to think about that. I have recovered the files, best i could, and plug them back in the right place, for good measure I will attach them here...

$ pwd
/home/mh-cbon/gow/src/github.com/mh-cbon/goupnp
[mh-cbon@Host-001 goupnp]$ tree -P *.zip
.
├── cmd
│   ├── discoverall
│   ├── example_httpu_serving
│   ├── example_internetgateway1
│   ├── example_ssdp_registry
│   └── goupnpdcpgen
├── dcps
│   ├── av1
│   │   └── av1.zip
│   ├── internetgateway1
│   │   └── internetgateway1.zip
│   └── internetgateway2
│       └── internetgateway2.zip
├── example
├── httpu
├── scpd
├── soap
└── ssdp

15 directories, 3 files

av1.zip
internetgateway1.zip
internetgateway2.zip

Maybe it worth to put those into a gh-pages and host them through github cdn ? or just put them raw into the repo ?

$ go generate ./...
2022/03/06 00:20:00 could not acquire spec for av1: could not download "av1.zip.download" from "http://upnp.org/specs/av/UPnP-av-TestFiles-20070927.zip": 500 Internal Server Error
dcps/av1/gen.go:1: running "goupnpdcpgen": exit status 1
2022/03/06 00:20:01 could not acquire spec for internetgateway1: could not download "internetgateway1.zip.download" from "http://upnp.org/specs/gw/UPnP-gw-IGD-TestFiles-20010921.zip": 500 Internal Server Error
dcps/internetgateway1/gen.go:1: running "goupnpdcpgen": exit status 1
2022/03/06 00:20:01 could not acquire spec for internetgateway2: could not download "internetgateway2.zip.download" from "http://upnp.org/specs/gw/UPnP-gw-IGD-Testfiles-20110224.zip": 500 Internal Server Error
dcps/internetgateway2/gen.go:1: running "goupnpdcpgen": exit status 1

@huin
Copy link
Owner

huin commented Mar 6, 2022

I've avoided putting them into the repo previously because I've been worried about possible copyright violation by doing so (I am not a lawyer - so I've tried to be cautious). I figured that it was okay to post the library as a derived work of the standard, though.

It might be necessary to update the URL that they are downloaded from, though.

@mh-cbon
Copy link
Contributor Author

mh-cbon commented Mar 7, 2022

I wrote some stuff to overcome this situation. Some code to download the full archive from https://openconnectivity.org/upnp-specs/upnpresources.zip, then do some adjustements to plug that into the cli.

Though, I am concerned by this whole thing.

In that archive, the files are named differently, dates are different.
Contents are different, like av1 devices has only two media renderer, in the prevous archive, there was 3 addtionnal media servers.

More importantly, using the generator against those files add some more updates into igw1, and, av1 is now full empty for some reasons I did not understand so far.

The short diff is like

diff --git a/dcps/internetgateway1/internetgateway1.go b/dcps/internetgateway1/internetgateway1.go
index 49986d8..708d63c 100644
--- a/dcps/internetgateway1/internetgateway1.go
+++ b/dcps/internetgateway1/internetgateway1.go
@@ -25,7 +25,9 @@ var _ time.Time
 const (
        URN_LANDevice_1           = "urn:schemas-upnp-org:device:LANDevice:1"
        URN_WANConnectionDevice_1 = "urn:schemas-upnp-org:device:WANConnectionDevice:1"
+       URN_WANConnectionDevice_2 = "urn:schemas-upnp-org:device:WANConnectionDevice:2"
        URN_WANDevice_1           = "urn:schemas-upnp-org:device:WANDevice:1"
+       URN_WANDevice_2           = "urn:schemas-upnp-org:device:WANDevice:2"
 )
 
 // Service URNs:
@@ -37,6 +39,8 @@ const (
        URN_WANDSLLinkConfig_1         = "urn:schemas-upnp-org:service:WANDSLLinkConfig:1"
        URN_WANEthernetLinkConfig_1    = "urn:schemas-upnp-org:service:WANEthernetLinkConfig:1"
        URN_WANIPConnection_1          = "urn:schemas-upnp-org:service:WANIPConnection:1"
+       URN_WANIPConnection_2          = "urn:schemas-upnp-org:service:WANIPConnection:2"
+       URN_WANIPv6FirewallControl_1   = "urn:schemas-upnp-org:service:WANIPv6FirewallControl:1"
        URN_WANPOTSLinkConfig_1        = "urn:schemas-upnp-org:service:WANPOTSLinkConfig:1"
        URN_WANPPPConnection_1         = "urn:schemas-upnp-org:service:WANPPPConnection:1"
 )

av1

diff --git a/dcps/av1/av1.go b/dcps/av1/av1.go
index 8b6637e..fd94abf 100644
--- a/dcps/av1/av1.go
+++ b/dcps/av1/av1.go
@@ -25,10729 +25,4 @@ var _ time.Time
 const ()
 
 // Service URNs:
-const (
-       URN_AVTransport_1        = "urn:schemas-upnp-org:service:AVTransport:1"
-       URN_AVTransport_2        = "urn:schemas-upnp-org:service:AVTransport:2"
-       URN_ConnectionManager_1  = "urn:schemas-upnp-org:service:ConnectionManager:1"
-       URN_ConnectionManager_2  = "urn:schemas-upnp-org:service:ConnectionManager:2"
-       URN_ContentDirectory_1   = "urn:schemas-upnp-org:service:ContentDirectory:1"
-       URN_ContentDirectory_2   = "urn:schemas-upnp-org:service:ContentDirectory:2"
-       URN_ContentDirectory_3   = "urn:schemas-upnp-org:service:ContentDirectory:3"
-       URN_RenderingControl_1   = "urn:schemas-upnp-org:service:RenderingControl:1"
-       URN_RenderingControl_2   = "urn:schemas-upnp-org:service:RenderingControl:2"
-       URN_ScheduledRecording_1 = "urn:schemas-upnp-org:service:ScheduledRecording:1"
-       URN_ScheduledRecording_2 = "urn:schemas-upnp-org:service:ScheduledRecording:2"
-)
-

For now i stopped upon readng some of the contents of those av1 XML files. Something regarding spaces and tabs badly caught my attention. In 2022. Maybe will try harder later.

@mh-cbon
Copy link
Contributor Author

mh-cbon commented Mar 7, 2022

Note also that the files i recovered previously were taken from thewaybackmachine. It did not seem like a way to go. So i decided to try with that alternatives, from openconnectivity.org

@huin
Copy link
Owner

huin commented Mar 7, 2022

This is getting a bit beyond the scope of the original issue, but yes, I think this needs some work to switch to the newer resources URL and structure.

mh-cbon added a commit to mh-cbon/goupnp that referenced this issue Mar 12, 2022
@mh-cbon
Copy link
Contributor Author

mh-cbon commented Mar 12, 2022

that should do it with the least minimal change. While i was at it i gave a shot to openconnectivity integration https://github.com/huin/goupnp/compare/master...mh-cbon:oc?expand=1 but there are still some differences which i am not sure about. I dont think I will make any more changes, i dont have much more pretexts to keep digging those archives and specs, my interest, after all, is pretty limited.

@huin
Copy link
Owner

huin commented Mar 12, 2022

That's looking promising, thanks :)

I've added some comments, which are mostly nitpicks on details, but fundamentally it's looking good.

mh-cbon added a commit to mh-cbon/goupnp that referenced this issue Mar 13, 2022
mh-cbon added a commit to mh-cbon/goupnp that referenced this issue Mar 13, 2022
@mh-cbon
Copy link
Contributor Author

mh-cbon commented Mar 13, 2022

from here, the diff of the generated files is daunting, reading all the specs even more. Not sure how to proceed.

@huin
Copy link
Owner

huin commented Mar 13, 2022

Yep, the diff on the generated files is hard to visualize. I spot checked one case, but it's really hard to know offhand if the API changes.

I wonder if there's an ordering that changed, and if so if a base CL that fixes the ordering might help.

@huin
Copy link
Owner

huin commented Mar 13, 2022

I'm looking to see about making a deterministic order for the generated files (sorry to say that I didn't do this from the start if it has an effect). You might need to rebase on top of this.

@huin
Copy link
Owner

huin commented Mar 13, 2022

The order of SCPD.Actions wasn't sorted by its .Name in the upnp.org definitions, although the services were. I'm going to commit a change that only performs a sort on the order they come out in, and maybe that'll help the resulting diff from openconnectivity.org.

@huin
Copy link
Owner

huin commented Mar 13, 2022

ca81a64 changes the ordering to sort by URN or name. It'll create merge conflicts for you on the generated files, but those can be merged safety by accepting either side of the merge (or otherwise marking as resolved), and then regenerating.

@mh-cbon
Copy link
Contributor Author

mh-cbon commented Mar 16, 2022

the diff is a ton better now !

https://github.com/huin/goupnp/compare/master...mh-cbon:oc?expand=1

Do you know why igw2.DeviceProtection1 has gone away ?

Also, gw2.WANIPConnection2.GetStatusInfoCtx has som kind of looking alike breaking changes.

I am wondering now if it would not be better to publish both versions, into different packages. Make them available and selctable upon starting up using some sort of package level variable, for example. wdyt ?

@huin
Copy link
Owner

huin commented Mar 16, 2022

I did some debugging, and I've found the cause of the missing igw2.DeviceProtection1:

standardizeddcps/DeviceProtection_1/UPnP-gw-IGD-Testfiles-20110224.zip
standardizeddcps/Internet Gateway_2/UPnP-gw-IGD-TestFiles-20101210.zip

Both of those (and maybe some other files) need to match the pattern in

  XMLSpecZipPath: "*/*/UPnP-gw-IGD-TestFiles-*.zip",

It took me several minutes to realise the problem: Testfiles versus TestFiles. /facepalm

Given that there's going to be contributors to the standard using Windows, I think we'll have to allow for that. So I think we can fix it with this version of globFiles:

func globFiles(pattern string, archive []*zip.File) []*zip.File {
	var files []*zip.File
	pattern = strings.ToLower(pattern)
	for _, f := range archive {
		if matched, err := path.Match(pattern, strings.ToLower(f.Name)); err != nil {
			// This shouldn't happen - all patterns are hard-coded, errors in them
			// are a programming error.
			panic(err)
		} else if matched {
			files = append(files, f)
		}
	}
	return files
}

@huin
Copy link
Owner

huin commented Mar 16, 2022

As for internetgateway1, I think this is newly including the WANIPConnection2 service by mistake. Is there a reason to include "*/service/WANIPConnection2.xml" in XMLServicePath for internetgateway1?

Likewise, should "*/device/WANConnectionDevice2.xml" and "*/device/WANDevice2.xml" be included in XMLDevicePath for internetgateway1?

In short, I think that for internetgateway1, these should be the values (unless I'm missing something):

				XMLServicePath: []string{"*/service/*1.xml"},
				XMLDevicePath:  []string{"*/device/*1.xml"},

@mh-cbon
Copy link
Contributor Author

mh-cbon commented Mar 16, 2022

About igw2.DeviceProtection1, I suggest to change XMLSpecZipPath type def to a slice of string. That would require an extra loop and avoid magic over unstable grounds.

About igw1, I am confused. I must have mixed up the two folders at somepoint and defined those in additions.

@mh-cbon
Copy link
Contributor Author

mh-cbon commented Mar 16, 2022

I just realized a stupid thing. In the zip published by openconnectivity, i cant find AV1 anymore.

I can only find av3/4.

@huin
Copy link
Owner

huin commented Mar 16, 2022

With regards XMLSpecZipPath being a []string - is that to support the casing on TestFiles and Testfiles? If so - we could do that, although I think we could expect filename casing to change so I'm more favouring the case-insensitive glob. It shouldn't be too bad to accidentally catch something with a different case as the generation happens manually and the result can be inspected.

Random thought: I think when it comes to doing a version 2 of this package (with breaking API changes), I'd end up having a separate package for each service. But that's for future.

With regards to av1 - that's a bit of a pain. I guess we'll have to stick with the upnpdotorg definition for now, even if the file isn't available. In the meantime we can add in av3 and av4 to the metadata.

@mh-cbon
Copy link
Contributor Author

mh-cbon commented Mar 16, 2022

I also note that it exists another gw thing.

But no sure if that should be considered at all.

WLAN Access Point Device_1UPnP-gw-WLAN-TestFiles-20031017.zip

Back on igw2.DeviceProtection1, in fact standardizeddcps/DeviceProtection_1/ does not declare such device. It declares a service, but there is no related device file under the device directory.

Also, applying the magic resulted in the dcp generator loading both zip archives, which resulted in duplicated declarations of device and services. That makes me think both 'things'/folders should not get mixed up ike this.

fy, i have pushed as-is the current code at https://github.com/huin/goupnp/compare/master...mh-cbon:oc?expand=1

@huin
Copy link
Owner

huin commented Mar 16, 2022

Ah, so some services are defined in multiple files? Which service(s) and files are those? (mostly curious to understand the scope of the problem)

Going to have to sign off for the evening, getting late for me. There seem to be some challenges here, but this looks like some solid forward progress. :)

@mh-cbon
Copy link
Contributor Author

mh-cbon commented Mar 16, 2022

I have just read that upnp consortium was replaced by openconntivity. So there is no chance those urls ever come back. Which is a sad situation.

It reinforces the idea to generate those new dcps into a different package path and let the user modify its preferred source of device definition at runtime. with a default on the pre existing implementation.

@huin
Copy link
Owner

huin commented Mar 17, 2022

The new DCPs should be in a new package, yes. We should try to keep the old packages supporting the same DCPs that they used to.

@mh-cbon
Copy link
Contributor Author

mh-cbon commented Mar 21, 2022

All good now. In this diff (https://github.com/huin/goupnp/compare/master...mh-cbon:oc?expand=1) upnp cod gen is almost identical, only the doc mention is changing.

The lastest specific changes are here (mh-cbon@7204c5d) it was mostly about being careful at using filepath.Base on the dcp metadata name, otherwise various broke (package name, using wrong directory, dir not existing etc).

got ride of av3.

Also updated the hacks to be less hardcoded.

mh-cbon added a commit to mh-cbon/goupnp that referenced this issue Mar 21, 2022
@mh-cbon
Copy link
Contributor Author

mh-cbon commented Mar 21, 2022

I have created two PR at your convenience.

Thanks for all.

@mh-cbon mh-cbon closed this as completed Mar 21, 2022
@huin
Copy link
Owner

huin commented Mar 21, 2022

Thanks for your patience :D

mh-cbon added a commit to mh-cbon/goupnp that referenced this issue Mar 22, 2022
huin pushed a commit that referenced this issue Mar 22, 2022
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

2 participants