1395914 - Network id: attempt to identify IPv6 network

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P2)

Description

[Daniel Stenberg [:bagder]]

The experiment to attempt to identify[*] the current IPv4 network (NETWORK_ID) on which Firefox is running has proven to have a fairly high degree of "unable to find an id" cases.

To lower the amount of ID failures, we should also extract an IPv6 network id (but make it a generic single output ID).

The IPv6 implementations need to be platform specific as these low level APIs are completely different between the different targets.

[*] = they're identified to the level that they get a hash value out. A value that *should* be the same the next time Firefox runs on that same network.

Comment 1

[Daniel Stenberg [:bagder]]

Attachment: 0001-bug-1395914-figure-out-network-id-for-IPv6-too-Linux.patch

First out, the Linux version.

(I've worked out and tested the logic with this command-line tool: https://github.com/bagder/gw-mac)

Comment 2

[Daniel Stenberg [:bagder]]

Forgot to mention the successful try-run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=769ded415f27714159eff77c9fa0a533039a835e&selectedJob=127730972

Comment 3

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8904168 [details] [diff] [review]
0001-bug-1395914-figure-out-network-id-for-IPv6-too-Linux.patch

Review of attachment 8904168 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

::: netwerk/system/linux/nsNotifyAddrListener_Linux.cpp
@@ +189,5 @@
> +        char ip6[40];
> +        int devnum;
> +        int preflen;
> +        int scope;
> +        int flags;

let's use fixed width types here - int32_t ?

@@ +219,5 @@
> +                            ip6, &devnum, &preflen, &scope, &flags, name)) {
> +                if (scope == GLOBAL) {
> +                    unsigned char id6[33];
> +                    int i;
> +                    int bits;

same. int32_t ?

@@ +226,5 @@
> +                    for (bits=preflen, i=0; bits>0; bits-=8, i++) {
> +                        char buf[3];
> +                        long val;
> +                        int mask;
> +                        int maskit[]={0x80, 0xc0, 0xe0, 0xf0, 0xf8, 0xfc, 0xfe, 0xff};

same. int32_t ?

@@ +231,5 @@
> +                        buf[0]=ip6[i*2];
> +                        buf[1]=ip6[i*2+1];
> +                        buf[2]=0;
> +                        /* convert from hex */
> +                        val = strtol(buf, NULL, 16);

nit: nullptr instead of NULL

@@ +233,5 @@
> +                        buf[2]=0;
> +                        /* convert from hex */
> +                        val = strtol(buf, NULL, 16);
> +                        mask = ( bits >= 8 ) ? 0xff : maskit[bits];
> +                        id6[i]=(unsigned char)val&mask;

nit: spacing

@@ +263,5 @@
> +        nsAutoCString output;
> +        sha1.update(addition.get(), addition.Length());
> +        uint8_t digest[SHA1Sum::kHashSize];
> +        sha1.finish(digest);
> +        nsCString newString(reinterpret_cast<char*>(digest),

can we use nsAutoCString here as well?

Comment 4

[Daniel Stenberg [:bagder]]

Attachment: v2-0001-bug-1395914-figure-out-network-id-for-IPv6-too-Li.patch

Thanks Valentin!

Some of the 'int' variables I use there are used as input arguments to sscanf() and that function is documented to take "pointers to int", so I think they should remain like that, but the rest of the remarks have been addressed in this version 2.

Comment 5

[Daniel Stenberg [:bagder]]

Attachment: 0001-bug-1395914-figure-out-network-id-for-IPv6-too-mac-r.patch

Here's the mac version of the same logic for ipv6 netid detection. The try build on mac is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=89a1673985e230ea69a4d29540710a3f7c496da3

Comment 6

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8906561 [details] [diff] [review]
0001-bug-1395914-figure-out-network-id-for-IPv6-too-mac-r.patch

Review of attachment 8906561 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/system/mac/nsNetworkLinkService.mm
@@ +299,5 @@
> +    return IPV6_SCOPE_GLOBAL;
> +}
> +
> +static int
> +ipv6_prefix(void *val, int size)

val doesn't really need to be void* - let's use the actual type to prevent calling it on something wrong.
Also, do we need the size argument, or can we just hardcode the size of the struct?

nit: aVal naming for argument

@@ +319,5 @@
> +        }
> +    }
> +    for (; bit != 0; bit--) {
> +        if (name[byte] & (1 << bit)) {
> +            return 0;

I assume this not normally happen, right? Consider adding some comments.

@@ +351,5 @@
> +                !(ifa->ifa_flags & (IFF_POINTOPOINT|IFF_LOOPBACK))) {
> +                // only IPv6 interfaces that aren't pointtopoint or loopback
> +                struct sockaddr_in6 *sin =
> +                    (struct sockaddr_in6 *)ifa->ifa_netmask;
> +                if (sin) {

nit: having variables named sin and sin6 is a bit confusing. consider more descriptive names

@@ +356,5 @@
> +                    char addr_buf[128];
> +                    int scope;
> +                    struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)ifa->ifa_addr;
> +                    scope = ipv6_scope(sin6);
> +                    if (scope == IPV6_SCOPE_GLOBAL) {

nit: can we reverse some of these if blocks so we get fewer levels of indentantion? if (!sin) continue; ?

@@ +358,5 @@
> +                    struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)ifa->ifa_addr;
> +                    scope = ipv6_scope(sin6);
> +                    if (scope == IPV6_SCOPE_GLOBAL) {
> +                        // only care about global scope
> +                        inet_ntop(AF_INET6, &sin6->sin6_addr, addr_buf,

nit: should we check the return value? also, you don't seem to use addr_buf for anything

@@ +365,5 @@
> +                                                 sizeof(struct in6_addr));
> +                        if (prefix && (prefix < 128)) {
> +                            unsigned char *p = (unsigned char *)&sin6->sin6_addr;
> +                            int match = 0;
> +                            /* check if prefix was already found */

nit: let's use c++ style comments everywhere.

@@ +367,5 @@
> +                            unsigned char *p = (unsigned char *)&sin6->sin6_addr;
> +                            int match = 0;
> +                            /* check if prefix was already found */
> +                            for (int i=0; i<prefixCount; i++) {
> +                                if (!memcmp(&prefixStore[i], p, prefix/8)) {

do we need to worry about comparing non-multiple-of-8 prefixes?

Comment 7

[Firefox Bug Husbandry Bot]

Bulk priority update: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:bagder, could you have a look please?

Comment 9

[Daniel Stenberg [:bagder]]

Sorry, I can't recall now why these particular ones were never merged. I have to defer to the existing necko people to decide how to deal with them (as I left the team months ago).

Network ID is still, the last time I discussed it, a dream that hasn't been fully realized and the patches here will not alone make the error rate low enough for the feature to become reliable. There needs to be more (semi-static) data input inserted into the algorithm for it to work decently, basing it on mac address and IPv6 prefix won't be enough. Possibly advertised DNS servers or other DHCP-related details can help.

I'm bouncing the NI over the valentin as a start.

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:bagder, could you have a look please?

Comment 11

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1395914 - Figure out network id for IPv6 too (Linux) r=michal

Comment 12

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1395914 - Figure out network id for IPv6 too (mac) r=michal

Depends on D34903

Comment 13

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1395914 - Update histogram with two new values for IPv6 network-id changes r=michal

Depends on D34904

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/47f2f3e8d5f9
Figure out network id for IPv6 too (Linux) r=michal
https://hg.mozilla.org/integration/autoland/rev/2b23587aa5ff
Figure out network id for IPv6 too (mac) r=michal
https://hg.mozilla.org/integration/autoland/rev/719b3b61b9a9
Update histogram with two new values for IPv6 network-id changes r=michal

Comment 15

[Alexandru Michis [:malexandru]]

Backed out 3 changesets for causing bustages in nsNetworkLinkService and nsNotifyAddrListener.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/8537d24d8aa8c0c2189c308d5835def2feec313c

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=c552aeab0fbfa729d61467f507b8a117da77e524&selectedJob=253898337

Failures:

• at nsNetworkLinkService: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=253898307&repo=autoland&lineNumber=13913
• at nsNotifyAddrListener.cpp: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=253898295&repo=autoland&lineNumber=54677

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dd05ca553d8e
Figure out network id for IPv6 too (Linux) r=michal
https://hg.mozilla.org/integration/autoland/rev/5ecb89d6b4b1
Figure out network id for IPv6 too (mac) r=michal
https://hg.mozilla.org/integration/autoland/rev/d3452fb68e51
Update histogram with two new values for IPv6 network-id changes r=michal

Comment 17

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/dd05ca553d8e
https://hg.mozilla.org/mozilla-central/rev/5ecb89d6b4b1
https://hg.mozilla.org/mozilla-central/rev/d3452fb68e51