agsh / onvif

ONVIF node.js implementation
https://onvif.pages.dev
MIT License
703 stars 240 forks source link

Discovery returns both ipV4 and ipv6 paths causing issues with connection #93

Closed hawkeye64 closed 4 years ago

hawkeye64 commented 6 years ago

This area of code:

var camAddr = data.probeMatches.probeMatch.endpointReference.address;
if (!cams[camAddr]) {
    var cam;
    if (options.resolve !== false) {
        var camUri = url.parse(data.probeMatches.probeMatch.XAddrs);
        cam = new Cam({
            hostname: camUri.hostname
            , port: camUri.port
            , path: camUri.path
        });

Is an issue. This line data.probeMatches.probeMatch.XAddrs looks like this:

http://10.10.1.60/onvif/device_service http://[fe80::4619:b6ff:fe3e:baa1]/onvif/device_service

When parse by URL(), the information is incorrect. Not all cameras return ipV6, so I would like to gather thoughts on handling this.

Suggestions are welcomed.

hawkeye64 commented 6 years ago

Here is a lit of my cameras and output from data.probeMatches.probeMatch.XAddrs

http://10.10.1.60/onvif/device_service http://[fe80::4619:b6ff:fe3e:baa1]/onvif/device_service
discovery.js:118
http://10.10.1.66/onvif/device_service
discovery.js:118
http://10.10.1.65/onvif/device_service http://[fe80::c256:e3ff:fef9:5568]/onvif/device_service
discovery.js:118
http://10.10.1.61/onvif/device_service http://[fe80::4619:b6ff:fe14:4db6]/onvif/device_service
discovery.js:118
http://10.10.1.67/onvif/device_service http://[fe80::3e8c:f8ff:fea1:32dc]/onvif/device_service
discovery.js:118
http://10.10.1.63/onvif/device_service http://[fe80::4619:b6ff:fe38:2bf0]/onvif/device_service
discovery.js:118
http://10.10.1.64/onvif/device_service http://[fe80::4619:b6ff:fe2f:7f8]/onvif/device_service
discovery.js:118
http://10.10.1.68/onvif/device_service http://[fe80::3e8c:f8ff:fea1:31b0]/onvif/device_service
discovery.js:118
http://10.10.1.62/onvif/device_service http://[fe80::4619:b6ff:fe12:7095]/onvif/device_service

The Hikvision and TrendNET cameras return both IPv4 and IPv6, but the Pelco is the only one returning IPv4. It's the only one the Discovery works with.

chriswiggins commented 6 years ago

Why don't we add an option to the discovery mechanism? That way, users can choose whether to use ipv4 or ipv6?

The other thing I'd be wary of is whether other manufacturers put ipv6 first in that list. We should really be checking the hostname.

Why not the following (modification of your patch on your branch):

var v4Obj;
var v6Obj;
var addresses = data.probeMatches.probeMatch.XAddrs.split(' ');
addresses.forEach((address) => {
    var urlObj = url.parse(address);
    if(net.isIPv4(urlObj.hostname)){
        v4Obj = urlObj;
    }else{
        v6Obj = urlObj
    }
});

var camUri = (options.useIPv6) ? v6Obj : v4Obj

That covers both use cases?

hawkeye64 commented 6 years ago

That was a temporary fix. I have a better fix I'll push later.

// Create cam with one of the XAddrs uri
var camUris = data.probeMatches.probeMatch.XAddrs.split(' ').map(url.parse)
    , camUri = matchXAddr(camUris, rinfo.address);
cam = new Cam({
    hostname: camUri.hostname
    , port: camUri.port
    , path: camUri.path
});
/**
 * All available XAddr fields from discovery
 * @name xaddrs
 * @memberOf Cam#
 * @type {Array.<Url>}
 */
cam.xaddrs = camUris;

and

/**
 * Try to find the most suitable record
 * Now it is simple ip match
 * @param {Array<Url>} xaddrs
 * @param {string} address
 */
function matchXAddr(xaddrs, address) {
    var ipMatch = xaddrs.filter(function(xaddr) {
        return xaddr.hostname === address;
    });
    return ipMatch[0] || xaddrs[0];
}

It will use the appropriate address and set cam.xaddrs so user can make decisions about both if needed.

hawkeye64 commented 6 years ago

Also, your case doesn't cover where IPv6 is not available (as in the Pelco camera). If you specify to use it and none is provided, you won't be bale to connect. It needs to fall back.

chriswiggins commented 6 years ago

Fair call - simple fix to that

var camUri = (options.useIPv6 && v6Obj ) ? v6Obj : v4Obj
hawkeye64 commented 6 years ago

:+1:

hawkeye64 commented 6 years ago

I'll implement your fix.

hawkeye64 commented 6 years ago

@chriswiggins We have a small issue with the code. I am at home now and testing with the Axis. It returns 4 addresses on the Discovery:

0:"http://169.254.138.132/onvif/device_service"
1:"http://192.168.0.23/onvif/device_service"
2:"http://[fd00:fc:8dc7:cbf2:240:8cff:fefa:b754]/onvif/device_service"
3:"http://[fe80::240:8cff:fefa:b754]/onvif/device_service"

I used the 169.254.138.132 address and oddly, it still worked.

Suggestions? The passed in rinfo has the 192.168.0.23 address.

chriswiggins commented 6 years ago

Hmmm good question - 169.254.0.0/16 is the link-local range which is why it works. Maybe add a check to exclude it? Is that the order that the addresses come in? I'd expect the 192.168.0.23 address to override the 169.254 one as its second?

hawkeye64 commented 6 years ago

Yes, that was from console.log on the addresses. Not quite what to do here. The code I had before might be a bit better as it'd pick out the proper address based on the incoming address (from rinfo).

On February 1, 2018 at 12:11 PM Chris Wiggins notifications@github.com wrote:

Hmmm good question - 169.254.0.0/16 is the link-local range which is why it works. Maybe add a check to exclude it? Is that the order that the addresses come in? I'd expect the 192.168.0.23 address to override the 169.254 one as its second? —You are receiving this because you authored the thread.Reply to this email directly, view it on GitHub, or mute the thread.

 

chriswiggins commented 4 years ago

Closing out old issue