Closed Bug 1562386 Opened 5 years ago Closed 5 years ago

Sort network attributes before hashing them to get a networkID

Categories

(Core :: Networking, task, P1)

Product:
Core
Component:
Networking
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla71
Tracking Flags:
Tracking Status
firefox70 --- fixed
firefox71 --- fixed

People

(Reporter: valentin, Assigned: o0ignition0o)

References

Details

(Whiteboard: [necko-triaged])

Attachments

(1 file)

Bug 1562386 - Sort network attributes before hashing them to get a networkID r?mayhemer,michal
47 bytes, text/x-phabricator-request
RyanVM
: approval-mozilla-beta+
Details | Review

https://phabricator.services.mozilla.com/D35684?id=124497#inline-218314

We should put the GUIDs into an array, sort them, then hash them, to get a consistent networkID regardless of the order they are returned in.
This might affect the Linux and Mac networkID implementations as well.

Not sure about the priority of network id bugs.
Nhi, could you set the priority or find an owner? Thanks.

Flags: needinfo?(nhnguyen)

This needs to go in Fx70.

Flags: needinfo?(nhnguyen)
Priority: -- → P2
Priority: P2 → P1

Leaving unassigned if someone can take this before Valentin is back.

Whiteboard: [necko-triaged]
Assignee: nobody → valentin.gosu

I just sent a WIP version of the task, trying to do what Valentin suggested.

It is still a pretty rough draft, since I would like to apply the same logic to the linux version of calculateNetworkId, and thoroughly test it before submitting it.

I'm submitting it as WIP to make sure any mistake I make can be caught early.

I'm not really sure, but it seems to me the mac os version of ipv4NetworkId does not require any sorting ?

Given the priority of the task, I would totally understand if someone told me it's too critical for me to handle.

Please let me know if there's any wrong assumption I've made so far, and if I can keep the ball rolling with the task :)

Assignee: valentin.gosu → jeremy.lempereur

It turns out ipv4NetworkId on Linux and Mac os gets the default Gateway (which is unique) and then finds one corresponding address (MAC on mac os / IPv4 on Linux).

IMHO there is no need for us to do any sorting before we hash the results.

In Linux, /proc/net/if_inet6 may contain several lines, so ordering the prefixes and prefixLengths seems to be the right way to go.

I'm about to push a draft with the required changes, and I would like to add unit / integration tests to make sure everything works as intended.

:mayhemer Can anyone give me pointers to how to run unit tests with fixtures? I have only had to work with ./mach gtest so far, and I can't find a way to run TEST_F() labeled tests .

The end goal would be to have the try server run them on linux / macos / windows. (I only have a macbook to test things out ATM)

Thank you for your time :)

Flags: needinfo?(honzab.moz)

You are asking for existing tests to run to catch regressions or create new tests for the feature?

For the former, just push to try: ./mach try -p all -b do -u all -t none

For the latter, I am not sure how exactly you want to test this. It's highly dependent on the host machine state and environment, and mainly on changes to those. I don't think we can setup boxes just to test this, this needs to be tested manually, I'm afraid.

Is this the info you needed?

Flags: needinfo?(honzab.moz)

You are asking for existing tests to run to catch regressions or create new tests for the feature?

I would say both, I don't want to risk having nightly users crash their browser on network status change because I overlooked something, and I'm only having a mac right now (I'm on holidays ^^), so I would like to see how the code behaves on windows / linux as well.

For the former, just push to try: ./mach try -p all -b do -u all -t none

I don't think I'm allowed to do try runs, I'm going to ask for a level 1 access
Here's the error I get:

ignition-macbook:mozilla-unified ignition$ ./mach try -p all -b do -u all -t none
Creating temporary commit for remote...
pushing to ssh://hg.mozilla.org/try
remote: Warning: Permanently added the ED25519 host key for IP address '63.245.208.203' to the list of known hosts.
remote: ignition@hg.mozilla.org: Permission denied (publickey).
temporary commit removed, repository restored
abort: no suitable response from remote hg!
Error running mach:

    ['try', '-p', 'all', '-b', 'do', '-u', 'all', '-t', 'none']

The error occurred in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.
You can invoke |./mach busted| to check if this issue is already on file. If it
isn't, please use |./mach busted file| to report it. If |./mach busted| is
misbehaving, you can also inspect the dependencies of bug 1543241.
Depends on: 1567967

An idea to check if the code using system APIs works is to simply wrap the system API to be able to replace it with a testing implementation of our own and under our control (a simulator) to exercise the code that is using this API. But having an automated test that really checks if the system API works is way harder and will likely need to be done manually (I would be fine with manual testing). We could also write a smoke test entry for our QA, if reasonable (I'll ask around how exactly).

It would be a pretty big endeavor indeed.

Since the issue I'm working on is focused on sorting the interfaces, I might write a -pretty simple- test asserting the hashes remain the same if the mocked interfaces order change, which shouldn't be too hard.

I can also create a follow-up bug to add tests with a test implementation following your idea. It would have a much lower priority and allow us to ship this one before Fx70 gets released :)

Does it sound reasonable to you ?

@mayhemer I have finally found a way to test things locally, and there's a couple of things I've done (horribly) wrong :/

I'll push something when it all works correctly, sorry for wasting your time, and thanks for the couple of hints you've provided me in your first round of review :)

(In reply to Jeremy Lempereur [:o0ignition0o] from comment #12)

@mayhemer I have finally found a way to test things locally, and there's a couple of things I've done (horribly) wrong :/

I'll push something when it all works correctly, sorry for wasting your time, and thanks for the couple of hints you've provided me in your first round of review :)

Cool, no problem.

Attachment #9078890 - Attachment description: Bug 1562386 - WIP: Sort network attributes before hashing them to get a networkID r?mayhemer,michal → Bug 1562386 - Sort network attributes before hashing them to get a networkID r?mayhemer,michal

I've just pushed a couple of changes which seem to pass my manual tests.

The helper tests I have written can be found here

I tried to use memcmp for the linux version, but there's probably something I'm not groking wrt unsigned char arrays so I couldn't manage. I would love to figure it out though ^^.

I'm not experienced enough to know if there's any caveat I'm missing, there's for example probably something wrong with me working with std::max(prefixLengths[a], prefixLengths[b]) if both prefixes start the same, but one is shorter than the other (it seems to me though that both have been initialized with zeroes, so it could be fine in the end).

Thank you in advance for your time and your feedbacks :)

EDIT: After a bit of digging / reading I've just pushed a version using a string instead of an unsigned char array, which allows me to compare them easily (and which is probably less error prone). I hope it helps ^^

ni? :michal to review this once other network ID bugs are done

Flags: needinfo?(michal.novotny)

Forgot to press submit in phab.

Flags: needinfo?(michal.novotny)

Just saw your inline comment, I'm going to update the patch in an hour, so it will be ready to land :)

I just trust you that you have tested this sensitive code really well on all platforms. I didn't have time to do it myself on at least win/lnx.

I recall having tested it on linux and osx. I'll test it again on windows to make sure I don't break the build.

Rather make sure you are actually comparing thing you want to and that there are no out-of-bound access and you don't happen to crash or break stacks or so. Simply make sure to cover all the code paths you have added.

That's exactly what I'm doing. I've also found REFGUID which may be a wiser type than *GUID for my comparison function. I hope to come up with something correct and elegant today.

I have tested the sorting on windows in a separate app, which behaves fine.

I think we're good to go, but given Valentin comes back in 6 days I would really feel better if he could also have a look at it ?

I would like to land this in nightly this week if possible. Since Valentin won't be back till next week, Michal, could you give the patch another review please?

Flags: needinfo?(michal.novotny)
Flags: needinfo?(michal.novotny)

It turns out replying to a phabricator email sends the contents to /Dev/null...

Thanks a lot for letting me know.

It seems like I cannot view the bug/patch you mentioned (even after logging in to bugzilla) could you please let me know if it is the only affected platform ?

Also do you think just removing the prefixes would be enough ? Maybe I should work on the whole address instead?

Thanks a lot for your time and help

Flags: needinfo?(michal.novotny)

(In reply to Jeremy Lempereur [:o0ignition0o] from comment #25)

It turns out replying to a phabricator email sends the contents to /Dev/null...

Thanks a lot for letting me know.

It seems like I cannot view the bug/patch you mentioned (even after logging in to bugzilla) could you please let me know if it is the only affected platform ?

Yes, only Linux part has changed. You can see the full patch here https://hg.mozilla.org/mozilla-central/rev/c8660505bc7e65f20c5959fb4e940df17a1c3d9e

Also do you think just removing the prefixes would be enough ? Maybe I should work on the whole address instead?

Just strip the Linux part from your patch. The new code doesn't use IPv6 prefixes and the information which is used is sorted before it is used.

Flags: needinfo?(michal.novotny)

I think this can be marked as ready to merge, but I have never done that before.
Michal could you please provide me with a direction to push it through the finish line ?
Thanks a lot for your time.
PS: Nhi I'm sorry we haven't pushed it last week :(

Flags: needinfo?(michal.novotny)
Pushed by valentin.gosu@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/566627a48370
Sort network attributes before hashing them to get a networkID r=mayhemer,michal

Normally after the patch is accepted you can use Lando to land it - see the View Stack in Lando button in phabricator. I'm not sure if you actually need landing permissions to click it or not - but in any case I did it for you.
Alternatively, you can set the checkin-needed flag on the bugzilla issue, and someone will do it for you.
Thanks!

Flags: needinfo?(michal.novotny)

Ok I know where it comes from, a REFGUID is already a reference.
I wonder why I haven't caught it though.
I'll push something in a couple of hours, sorry for overseeing it

Flags: needinfo?(jeremy.lempereur)

I pushed the last revision on a macbook, which is the reason why it compiled fine on my machine (the error is located in the windows file).
I just pushed the fix and I'm about to run a try to make sure it builds fine.
Sorry about that (and welcome back Valentin!)
Edit: the try can be followed here

Just tried to have a look at Lando and I don't have the required commit access:

You have insufficient permissions to land. Level 3 Commit Access is required. See the FAQ for help.

Valentin: Should the fix go through an other round of reviews ?

Thank you for your time.

Flags: needinfo?(valentin.gosu)

Thanks for updating the patch. I gave the changes a quick look, and I think I noticed a bug. I hope you don't mind giving it another look.

Flags: needinfo?(valentin.gosu)

(In reply to Valentin Gosu [:valentin] (he/him) from comment #34)

Thanks for updating the patch. I gave the changes a quick look, and I think I noticed a bug. I hope you don't mind giving it another look.

Of course not, I hope we can figure it out and work it through together :)

Pushed by valentin.gosu@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8a98e2a91bdc
Sort network attributes before hashing them to get a networkID r=mayhemer,michal,valentin

(In reply to Pulsebot from comment #36)

Pushed by valentin.gosu@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8a98e2a91bdc
Sort network attributes before hashing them to get a networkID
r=mayhemer,michal,valentin

So we just landed a mistake in the code? I think the tests are a must now. Jeremy, can you please jump on it? I can provide guidance if needed.

(In reply to Honza Bambas (:mayhemer) from comment #37)

So we just landed a mistake in the code? I think the tests are a must now. Jeremy, can you please jump on it? I can provide guidance if needed.

No, I think I was just looking at the wrong definition. But Jeremy fixed it, so we should be fine - see phabricator.

Either way adding tests would be great, I am more used to TDD and I should have probably started there. I will start working on tests because "I think" and "we should" is not the quality standard to be expected in my work and in Firefox. I really want to improve and deliver patches I'm confident with.

https://hg.mozilla.org/mozilla-central/rev/8a98e2a91bdc

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox71: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

Valentin, should this be uplifted to 70 (together with the test patch)?

If it improves the accuracy of network ID greatly, I think it should be uplifted, as we need network ID to work correctly in 70. Can you weigh in on the technical necessity and risks of this patch?

Flags: needinfo?(valentin.gosu)

(In reply to Nhi Nguyen (:nhi) from comment #41)

Valentin, should this be uplifted to 70 (together with the test patch)?

If it improves the accuracy of network ID greatly, I think it should be uplifted, as we need network ID to work correctly in 70. Can you weigh in on the technical necessity and risks of this patch?

I don't think it's easy to write a test for this code. What Jeremy mentioned in phabricator was a standalone test just for testing of sorting the GUIDs. I've tested manually the code change in MacOS. If any bode can do the same for the Windows part, I think we could uplift it without a test.

I think we should uplift it. The risk isn't too high.
Leaving ni? so I check how well it behaves on Windows.

Depends on: 1580924
No longer depends on: 1580924

(In reply to Valentin Gosu [:valentin] (he/him) from comment #43)

I think we should uplift it. The risk isn't too high.
Leaving ni? so I check how well it behaves on Windows.

I just checked the windows code, and it works correctly regardless of the order in which networks are turned on.
So I'll file a request to uplift.

Flags: needinfo?(valentin.gosu)

Fwiw I factored out the sorting and hashing in bug 1580157 and wrote tests making sure it works as expected on Windows and osx

Comment on attachment 9078890 [details]
Bug 1562386 - Sort network attributes before hashing them to get a networkID r?mayhemer,michal

Beta/Release Uplift Approval Request

  • User impact if declined: The networkID may be incorrect for users who have multiple active network adapters.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The changes just involve sorting the network identifiers before hashing.
    Should be low risk as we've manually verified correctness.
  • String changes made/needed:
Attachment #9078890 - Flags: approval-mozilla-beta?

Comment on attachment 9078890 [details]
Bug 1562386 - Sort network attributes before hashing them to get a networkID r?mayhemer,michal

Network ID improvement for Windows users. Approved for 70.0b8.

Attachment #9078890 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: