Closed Bug 1599816 Opened 4 years ago Closed 4 years ago

Requests are being handled via TRR while connected to a PPTP VPN server

Categories

(Core :: Networking: DNS, defect, P1)

72 Branch
x86_64
Windows 10
defect

Tracking

()

VERIFIED FIXED
mozilla73
Tracking Status
firefox72 --- verified
firefox73 --- verified

People

(Reporter: emilghitta, Assigned: valentin)

References

Details

(Whiteboard: [necko-triaged])

Attachments

(2 files)

Affected versions

  • Firefox 72.0a1 (BuildId:20191126212708)

Affected platforms

  • Windows 10 64bit.

Preconditions

Steps to reproduce

  1. Launch Firefox.
  2. Access the about:config page.
  3. Set the network.trr.mode pref to 2.
  4. Restart Firefox.
  5. Access a random domain.

Expected result

  • A VPN connection is detected and the request is not being handled via TRR.

Actual result

  • The request is being handled via TRR.

Regression Range

  • I don’t think that this is a regression.

Not sure about a priority, but there should be a code to detect VPN and turn off TRR already. Michal or Valentin may know more. This should be fixed in bug 1565004.

Flags: needinfo?(valentin.gosu)
Flags: needinfo?(michal.novotny)
Priority: -- → P1
Whiteboard: [necko-triage]

This is Windows only, using instructions provided by collaborators at Microsoft.
I'll try to diagnose why this doesn't work.

Assignee: nobody → valentin.gosu
Flags: needinfo?(valentin.gosu)
Flags: needinfo?(michal.novotny)
Whiteboard: [necko-triage] → [necko-triaged]

Hi Emil,

I just tested this on windows 10 - Built from https://hg.mozilla.org/mozilla-central/rev/329340b1608d79266217ae37b098fa270f444c89
When I turn on the PPTP VPN (de4.vpnbook.com from https://www.vpnbook.com/ ) new domains appear with TRR=false in about:networking
I also checked the logs and it says

2019-12-09 10:48:04.161000 UTC - [Parent 12708: Socket Thread]: D/nsHostResolver Resolving host [www.reddit.com] type 0. [this=000002CE1A575C10]
2019-12-09 10:48:04.161000 UTC - [Parent 12708: Socket Thread]: D/nsHostResolver www.reddit.com is excluded from TRR because of platform indications

So it seems to me like this works.
Could you give it another go? And if you can reproduce it, can you get some HTTP logs with MOZ_LOG=sync,timestamp,nsNotifyAddr:5,nsHostResolver:5

Thanks!

Flags: needinfo?(emil.ghitta)

I can confirm that this issue is not reproducible on the build provided in comment 3. The domains are no longer handled via TRR while a PPTP VPN connection is in place.

Flags: needinfo?(emil.ghitta)
Attached file log.txt.moz_log

Please Ignore my previous comment (comment 4). I am able to reproduce this issue using Firefox 73.0a1 (BuildId:20191209095039).

Attaching log file.

Hi Emil,

Great work tracking this down. The cause is that after computing the VPN/Suffix we send a NS_DNS_SUFFIX_LIST_UPDATED_TOPIC, but the TRRService only updates its VPN status when a network change topic is received. Sometimes those might come at close times, and it works, sometimes they might happen separately, and we get this bug.

I'll write up a patch ASAP.

This bug is caused by bug 1598676 (Adds NS_DNS_SUFFIX_LIST_UPDATED_TOPIC), and
bug 1598676 (makes CheckAdaptersAddresses be called at startup)

While at startup it's sometimes the case that the NS_NETWORK_LINK_TOPIC and
NS_DNS_SUFFIX_LIST_UPDATED_TOPIC are both called, due to coalescing and
other link service quirks (or bugs) that doesn't happen for every network
change.

This means that we end up calling RebuildSuffixList after every
CheckAdaptersAddresses call, but we don't do that for CheckPlatformDNSStatus.

This patch makes it so that we always call both methods regardless which of
the two observer notifications was received.

Attachment #9114675 - Attachment description: Bug 1599816 - TRR: Make sure we detect VPNs even at startup r=kershaw → Bug 1599816 - TRR: Make sure we recheck for VPNs on all relevant observer topics r=kershaw
Pushed by valentin.gosu@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/53c316fbea27
TRR: Make sure we recheck for VPNs on all relevant observer topics r=kershaw
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla73

Should we uplift this to beta?

Flags: qe-verify+
Flags: needinfo?(valentin.gosu)

This issue is verified fixed using Firefox 73.0a1 (BuildId:20191211094640) on Windows 10 64bit. Tried to reproduce this issue a couple of times but failed to do so. It looks that domain requests are no longer being handled via TRR if a PPTP VPN connection is used.

Pending for a possible beta uplift.

Flags: needinfo?(emil.ghitta)

Comment on attachment 9114675 [details]
Bug 1599816 - TRR: Make sure we recheck for VPNs on all relevant observer topics r=kershaw

Beta/Release Uplift Approval Request

  • User impact if declined: VPN detection will only happen on the first network change leading to the possibility that the user will be sending TRR requests even though there is an active VPN.
  • Is this code covered by automated tests?: Unknown
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: Configure and start a VPN connection (comment 0)
    Start firefox
    Browse around
    Check about:networking#dns that no requests used TRR
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Since bug 1598676 and bug 1598676 are both in fx 72 we should uplift this too, so users don't have an inconsistent experience.
  • String changes made/needed:
Flags: needinfo?(valentin.gosu)
Attachment #9114675 - Flags: approval-mozilla-beta?
QA Whiteboard: [qa-triaged]

Comment on attachment 9114675 [details]
Bug 1599816 - TRR: Make sure we recheck for VPNs on all relevant observer topics r=kershaw

DoH vs VPN fix, approved for 72.0b7

Attachment #9114675 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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

  • Why is the change risky/not risky? (and alternatives if risky): Since bug 1598676 and bug 1598676 are both in fx 72 we should uplift this too, so users don't have an inconsistent experience.

I assume one of these 1598676 should have been some other bug number :)

This issue is verified fixed using Firefox 72.0b7 on Windows 10 64bit.

Status: RESOLVED → VERIFIED
Flags: needinfo?(emil.ghitta)
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.