GGist / ssdp-rs

Simple Service Discovery Protocol In Rust
Apache License 2.0
42 stars 13 forks source link

fix: ipv6 broke in commit 3f3c27e #31

Closed icorderi closed 7 years ago

icorderi commented 7 years ago

UPNP_MULTICAST_IPV6_LINK_LOCAL_ADDR is a link local address. On Ipv6, link-local requires a scope with it.

GGist commented 7 years ago

Thanks for looking into this. Apologies if I broke any code, however I was getting errors when trying to create a NotifyListener on Windows with these changes (but not with the current master).

In the code you are trying to bind the multicast address as the local address on the UdpSocket and Windows is saying that this isn't a valid address in this context, which to me makes sense (although I am not super experienced with IPv6 multicast). What OS do the changes in this pull request work on?

I do think my earlier changes were a bit screwy in that I threw the scope_id away by mistake.

icorderi commented 7 years ago

My bad, we don't run windows. I checked it on my laptop (OSX/macOS), the dev vagrant running Ubuntu 16.04 and our production systems which run a custom linux distro.

I just checked with my networking guy, the bind on windows IPV6 is slightly different than bsd and linux. Which is why we are stepping on each other on what to put as an address.

He recommends we bind to INADDR_ANY, that will work on windows, bsd and linux. We can also use #[cfg(not(windows))] / #[cfg(windows)] and use different code.

What do you prefer?

GGist commented 7 years ago

Ok, looks like there was a bit more going on here that was confusing me.

  1. With the above changes it seems like your code is running fine but have you been able to actually receive IPv6 multicast NOTIFY or M-SEARCH requests? I was trying to do some debugging but noticed that I wasn't getting them in the async_notify example for IPv6 requests. I traced it down to us throwing a MissingHeader(Host) when we do checks here. I was wondering if you were hitting the same code? Maybe a problem with hyper?
  2. I realized that I was testing your changes on Ubuntu via VirtualBox using a BridgedAdapter which seemed to exhibit similar behavior to what I was experiencing on my Windows host. Switching to a NAT adapter seemed to give correct Linux behavior with your code.
  3. I realized that there is hardly any information regarding IPv6 multicast on Windows. However, reading this gave me a bit more insight as to why Linux allows binding a socket to a non-local IP address.

I wasn't able to make much progress on a solution to the platform differences going on here because I spent so much time debugging only to realize that point 1 was occurring but I will do some more testing tomorrow.

icorderi commented 7 years ago

It's working on OSX and it's finding all the appliances in QA (custom distro) + the dev vms (Ubuntu) on the network.

I checked both NOTIFY and SEARCH.

Yea, maybe something changed with Hyper and Host recently.

My Cargo.toml is fixed to ssdp = "=0.3.0" for the time being. My Cargo.lock is showing hyper on version 0.9.10.

[[package]] name = "hyper" version = "0.9.10" source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ "cookie 0.2.5 (registry+https://github.com/rust-lang/crates.io-index)", "httparse 1.1.2 (registry+https://github.com/rust-lang/crates.io-index)", "language-tags 0.2.2 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.3.6 (registry+https://github.com/rust-lang/crates.io-index)", "mime 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)", "num_cpus 0.2.13 (registry+https://github.com/rust-lang/crates.io-index)", "openssl 0.7.14 (registry+https://github.com/rust-lang/crates.io-index)", "openssl-verify 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)", "rustc-serialize 0.3.19 (registry+https://github.com/rust-lang/crates.io-index)", "solicit 0.4.4 (registry+https://github.com/rust-lang/crates.io-index)", "time 0.1.35 (registry+https://github.com/rust-lang/crates.io-index)", "traitobject 0.0.1 (registry+https://github.com/rust-lang/crates.io-index)", "typeable 0.1.2 (registry+https://github.com/rust-lang/crates.io-index)", "unicase 1.4.0 (registry+https://github.com/rust-lang/crates.io-index)", "url 1.1.1 (registry+https://github.com/rust-lang/crates.io-index)", ]

GGist commented 7 years ago

So I tested binding to INADDR_ANY but, similar to what I learned in the link I mentioned from point 3, it causes...interesting behavior. Essentially if I have two IPv4 sockets (same applies to IPv6) and I bind each of them to 0.0.0.0 and sign up for multicast on each of the sockets using the desired interface, I will receive both messages on each interface.

So, what I did was take advantage of that behavior and create a single UdpSocket per all IPv4 and IPv6 addresses which also allows us to cut down on our thread usage significantly. Essentially we lazily create a UdpSocket when we see we don't have one already for either a IPv4 or IPv6 address. Then, we bind that to INADDR_ANY and proceed to sign up for multicast on each address we later see using the same UdpSocket.

I tested on both Ubuntu and Windows and it seems to work. One thing that I noticed on Windows however was that it would throw an error when trying to sign up for an IPv6 multicast address with a scope id of 0. I get an error "An invalid argument was supplied." so I had to add an if check so that I don't sign up with a scope id of 0 as I seem to be able to receive multicast from globally scoped IPv6 addresses either way. However, I don't have a globally scoped ip address on my Ubuntu virtual machine so I am not sure if it works there. I am kind of curious if IPv6 would still work if all of my IPv6 addresses were globally scoped causing the code to never get a chance to call join_multicast...

If you can test these changes to verify that they also work for you that would be great. I have them up in a pull request.

Edit: Also, the problem with hyper incorrectly parsing Host headers still persists, so if you do happen to run into that issue, test these changes with line 218 in message/ssdp.rs commented out.

icorderi commented 7 years ago

@GGist, that's a great idea. I added it to commit 59cd309 + some code cleanup, since we weren't mapping anything.

As far as PR #33, that's not ok on unixes. You are effectively getting rid of a network interface doing that. On windows, scopeids are slightly different, I believe they actually start on 1.

I just had a meeting with my networking guru. The difference probably comes from how we are getting the interface information here.

That windows code is probably returning the wrong scope information, or no scope info at all. He thinks it's probably happening for the link-local/site-local addresses.

Can you add a trace on your system and show me the addresses? Modify this debug!("Joining ipv6 multicast {} at iface: {}", mcast_ip, addr); to print scope information. RUST_LOG=ssdp::message::notify=trace should narrow the output.

This is from my laptop, corporate (wired, wifi) and host network with my VMs, all dual ipv4/ipv6

DEBUG:ssdp::message::notify: Joining ipv6 multicast ff02::c at iface: [fe80::1]:0 DEBUG:ssdp::message::notify: Joining ipv6 multicast ff02::c at iface: [fe80::aebc:32ff:feb3:f773]:0 DEBUG:ssdp::message::notify: Joining ipv4 multicast 239.255.255.250 at iface: 192.168.200.90:0 DEBUG:ssdp::message::notify: Joining ipv6 multicast ff02::c at iface: [fe80::a84b:a2ff:fe58:4a58]:0 DEBUG:ssdp::message::notify: Joining ipv4 multicast 239.255.255.250 at iface: 172.31.100.1:0 DEBUG:ssdp::message::notify: Joining ipv6 multicast ff02::c at iface: [fe80::12dd:b1ff:fedd:b5d7]:0 DEBUG:ssdp::message::notify: Joining ipv4 multicast 239.255.255.250 at iface: 192.168.169.36:0

We believe that [fe80::1] is blowing up on your windows. That's your link-local "localhost" in ipv6. We can't turn site-local addresses off, that's how you do auto-conf in ipv6 land. There might be another windows api call we need to do to fetch the scope information from the returned addresses in net::lookup_host("").

GGist commented 7 years ago

Here is the output:

     Running `target\debug\examples\async_notify.exe`
Joining ipv6 multicast ff02::c at iface: [fe80::57e:6316:39c6:a017]:0 with scope id 14
Joining ipv6 multicast ff02::c at iface: [fe80::b4fd:3d6f:97a7:af91]:0 with scope id 13
Joining ipv6 multicast ff02::c at iface: [fe80::4524:ab89:f81e:3587]:0 with scope id 15
Joining ipv6 multicast ff02::c at iface: [2601:601:201:3fa0:b4fd:3d6f:97a7:af91]:0 with scope id 0
thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: Other(Error { repr: Os { code: 10022, message: "An invalid argument was supplied." } })', ../src/libcore\result.rs:788
note: Run with `RUST_BACKTRACE=1` for a backtrace.
Press Enter When You Wish To Exit...

On windows, scopeids are slightly different, I believe they actually start on 1.

Yes you are exactly right. I have been looking into alternate APIs so we can get rid of lookup_host as it seems a bit hacky. Looks like GetAdaptersAddresses is what I am looking for. If you see on this page at Ipv6IfIndex it says

Ipv6IfIndex
Type: DWORD
The interface index for the IPv6 IP address. This member is zero if IPv6 is not available on the interface.

So in this case, lookup_host isn't really sufficient for our needs; as I understand it, globally unique IPv6 addresses don't require scope ids so it isn't giving us them? However, GetAdaptersAddresses should be able to give them to us.

Right now I am working on extending rust-ifaces and will make a pull request soon adding Windows support as well as a cross platform function we can call to from this crate to get all addresses.

GGist commented 7 years ago

Just an update.

I have changes for rust-ifaces mostly done. For Windows I initially went ahead and backfilled the SocketAddrV6 scope id information for all sockets. However, I realized that this has the side effect of not being able to bind to the address. Essentially, even if some global address is associated with a specific interface on the actual system (I verified this), setting the scope id to that interface index causes a failure. So for that, we have to retain the scope id of 0 when using a SocketAddr for binding (such as when we send multicast messages on all interfaces).

With that in mind, I am updating both the Unix code as well as the Windows code to provide interface indices separate from the SocketAddr scope ids (in the case of IPv6).

@icorderi Also, I noticed that for duplicate interface indices, join_multicast will fail on Windows. I imagine we can just call join_multicast on only unique interface indices in the case of IPv6? Just wanted to make sure that wont be a problem for you.

icorderi commented 7 years ago

@ggist, the unix code is working as is. I think we are gonna be much better of with a windows specific filter for scopeid 0 at this point.

We should also filter out globals from both platform code paths. Doing SSDP discovery on a global iface is very... odd.

GGist commented 7 years ago

Sorry for the delay.

@icorderi If we are filtering global ipv6 interfaces on both platforms, then I don't have to add code on Windows for filtering out scope ids of 0.

This is the code I have for filtering. Let me know if it looks fine.