Closed Bug 1325501 Opened 3 years ago Closed 3 years ago

use conservative TLS settings for XHR for Firefox code/data updates and telemetry

Categories

(Toolkit :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox51 + verified
firefox52 + fixed
firefox53 --- fixed

People

(Reporter: rhelmer, Assigned: rhelmer)

References

(Blocks 1 open bug)

Details

(Whiteboard: [measurement:client:tracking])

Attachments

(6 files, 1 obsolete file)

As discussed in bug 1321783, we need to use conservative TLS settings for code that does updates, as we know there are problems with (at least) TLS 1.3 and users behind some middleboxes.

There are many places in the tree that fetch important updates (blocklist, addons, GMP, etc) using XHR directly, and Telemetry uses it to send data.

We investigated setting this for all chrome code directly in the XHR implementation (bug 1323538) so we wouldn't have to touch all the existing XHR callers, but we came to the conclusion that we'd need to make it optional so we'd need to touch them anyway.

We've been talking about consolidating these sorts of requests into a helper module that wraps XHR anyway, we could do this in such a way that it's possible to use as a drop-in replacement.

Later we'll move more callers over to it, provide a nicer API etc. but we could start by just setting the single TLS "beConservative" flag.
See Also: → 1323538
Comment on attachment 8821360 [details]
Bug 1325501 - Adds ServiceRequest as a drop-in replacement for XHR, which uses conservative TLS settings

https://reviewboard.mozilla.org/r/100666/#review101150

Does this have to be in its own new directory and not just toolkit/modules?

::: toolkit/components/servicerequests/ServiceRequests.jsm:22
(Diff revision 1)
> +
> +/**
> +  * ServiceRequest is intended to be a drop-in replacement for current users
> +  * of XMLHttpRequest.
> +  */
> +class ServiceRequest extends XMLHttpRequest {

I love that we can do this!

::: toolkit/components/servicerequests/ServiceRequests.jsm:31
(Diff revision 1)
> +    * @param {String} method - HTTP method to use, e.g. "GET".
> +    * @param {String} url - URL to open.
> +    * @param {String} user - Optional username to provide.
> +    * @param {String} password - Optional password to provide.
> +    */
> +  open(method, url, isAsync, user, password) {

I am of the opinion that we should make this signature be:

open(method, url, options)

It's a slight departure from XHR, but isAsync should always be true for our cases and I don't think anything ever passes auth details that we care about. Then we can extend options later.

::: toolkit/components/servicerequests/ServiceRequests.jsm:35
(Diff revision 1)
> +    */
> +  open(method, url, isAsync, user, password) {
> +    super.open(method, url, isAsync, user, password);
> +
> +    // Disable cutting edge features, like TLS 1.3, where middleboxes might brick us
> +    super.channel.QueryInterface(Ci.nsIHttpChannelInternal).beConservative = true;

This can throw in some cases for example when for testing we set an update url to a local file, be sure and catch it.
Attachment #8821360 - Flags: review?(dtownsend) → review+
These patches should cover at least:

* Telemetry
* Addons
* System Add-ons
* GMP
* Blocklist

No doubt there are others, but this seems like a good start.
Status: NEW → ASSIGNED
Comment on attachment 8821454 [details]
Bug 1325501 - move Telemetry from XHR to ServiceRequest

https://reviewboard.mozilla.org/r/100740/#review101232

Are there plans to test this to make sure we don't end up breaking Telemetry?

In bug 1321783 i see this fixes an issue present on Firefox 52+.
Are you planning to uplift this change to Firefox 52 or do we need to act on this?
Attachment #8821454 - Flags: review?(gfritzsche) → review+
Whiteboard: [measurement:client:tracking]
Ideally we would uplift this or the other version to 51. I have approval to uplift TLS 1.3 for beta 11 and we do want to make sure that updates telemetry and add-on updates work
(In reply to Georg Fritzsche [:gfritzsche] [away Dec 24 - Jan 2] from comment #7)
> Comment on attachment 8821454 [details]
> Bug 1325501 - move Telemetry from XHR to ServiceRequest
> 
> https://reviewboard.mozilla.org/r/100740/#review101232
> 
> Are there plans to test this to make sure we don't end up breaking Telemetry?
> 
> In bug 1321783 i see this fixes an issue present on Firefox 52+.
> Are you planning to uplift this change to Firefox 52 or do we need to act on
> this?

I've done manual testing, and this just wraps XHR so I am pretty confident it doesn't break anything - however Telemetry is important enough that we should ask QA to take a look too.
Comment on attachment 8821453 [details]
Bug 1325501 - move addons manager from XHR to ServiceRequest

https://reviewboard.mozilla.org/r/100738/#review101380

::: toolkit/mozapps/extensions/LightweightThemeManager.jsm:15
(Diff revision 1)
>  const Ci = Components.interfaces;
>  
>  Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
>  Components.utils.import("resource://gre/modules/AddonManager.jsm");
>  /* globals AddonManagerPrivate*/
> +Components.utils.import("resource://gre/modules/ServiceRequest.jsm");

Can we use defineLazyModule getter for this? Also in the other modules?

::: toolkit/mozapps/extensions/content/extensions.js:3472
(Diff revision 1)
>          });
>  
>          if (aCallback)
>            aCallback();
>        } else {
> -        var xhr = new XMLHttpRequest();
> +        var xhr = new ServiceRequest();

Let's stick to XHR for this. It's only ever used for local `chrome:`/`resource:`/`file:`/`jar:` URLs.

::: toolkit/mozapps/extensions/internal/AddonRepository.jsm:105
(Diff revision 1)
>    weekly_downloads: "weeklyDownloads",
>    daily_users:      "dailyUsers"
>  };
>  
> -// Wrap the XHR factory so that tests can override with a mock
> -var XHRequest = Components.Constructor("@mozilla.org/xmlextras/xmlhttprequest;1",
> +// tests override XHRequest with a mock.
> +var XHRequest = ServiceRequest;

Can we just get rid of this and override the `ServiceRequest` global instead?

::: toolkit/mozapps/extensions/internal/ProductAddonChecker.jsm:24
(Diff revision 1)
>    "src": "chrome://global/content/gmp-sources/widevinecdm.json"
>  }];
>  
>  this.EXPORTED_SYMBOLS = [ "ProductAddonChecker" ];
>  
>  Cu.importGlobalProperties(["XMLHttpRequest"]);

I think we can get rid of this now.

::: toolkit/mozapps/extensions/internal/ProductAddonChecker.jsm:48
(Diff revision 1)
>  // This exists so that tests can override the XHR behaviour for downloading
>  // the addon update XML file.
>  var CreateXHR = function() {
> -  return Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].
> +  return new ServiceRequest();
> -    createInstance(Ci.nsISupports);
>  }

Same here. Can we just get rid of this and use the `ServiceRequest` global directly?
Attachment #8821453 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8821360 [details]
Bug 1325501 - Adds ServiceRequest as a drop-in replacement for XHR, which uses conservative TLS settings

https://reviewboard.mozilla.org/r/100666/#review101390

::: toolkit/modules/ServiceRequest.jsm:46
(Diff revision 2)
> +  open(method, url, options) {
> +    super.open(method, url, true);
> +
> +    // Disable cutting edge features, like TLS 1.3, where middleboxes might brick us
> +    try {
> +      super.channel.QueryInterface(Ci.nsIHttpChannelInternal).beConservative = true;

Can we do `if (channel instanceof Ci.nsIHttpChannelInternal)` here instead, for cases where this isn't an HTTP request?
Comment on attachment 8821453 [details]
Bug 1325501 - move addons manager from XHR to ServiceRequest

https://reviewboard.mozilla.org/r/100738/#review101380

> Same here. Can we just get rid of this and use the `ServiceRequest` global directly?

I can't actually find any place this was used anyway.
Keywords: qawanted
Pushed by rhelmer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/880decff07b3
Adds ServiceRequest as a drop-in replacement for XHR, which uses conservative TLS settings r=mossop
https://hg.mozilla.org/integration/autoland/rev/c0493757d21e
move addons manager from XHR to ServiceRequest r=kmag
https://hg.mozilla.org/integration/autoland/rev/b6e50911ef79
move Telemetry from XHR to ServiceRequest r=gfritzsche
(In reply to Wes Kocher (:KWierso) from comment #22)
> I had to back these out for xpcshell failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=8400689&repo=autoland
> 
> https://hg.mozilla.org/integration/autoland/rev/
> 57af1c590bf9dab0e1b0d8dd6dc6000a3bf446e1

Thanks Wes, I'll do a full try run before autolanding again. I didn't realize there was code outside of toolkit/mozapps/extensions that was using.

There's a GMP module in a different part of the tree, which is what the `createXHR` function in comment 12 is for - there's a lot of code to change in there, so I am going to back out just these changes in ProductAddonChecker, and set beConservative in the two places it wants. We can clean it up in a followup bug.
Flags: needinfo?(rhelmer)
Pushed by rhelmer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5d35c4556536
Adds ServiceRequest as a drop-in replacement for XHR, which uses conservative TLS settings r=mossop
https://hg.mozilla.org/integration/autoland/rev/1972c0d34e2a
move addons manager from XHR to ServiceRequest r=kmag
https://hg.mozilla.org/integration/autoland/rev/24cb98d079e5
move Telemetry from XHR to ServiceRequest r=gfritzsche
(In reply to Robert Helmer [:rhelmer] from comment #23)
> There's a GMP module in a different part of the tree, which is what the
> `createXHR` function in comment 12 is for - there's a lot of code to change
> in there, so I am going to back out just these changes in
> ProductAddonChecker, and set beConservative in the two places it wants. We
> can clean it up in a followup bug.

Sorry, I actually noticed that the other day when I was working on bug 1325149. I should have double checked when you said that it didn't appear to be used.
https://hg.mozilla.org/mozilla-central/rev/5d35c4556536
https://hg.mozilla.org/mozilla-central/rev/1972c0d34e2a
https://hg.mozilla.org/mozilla-central/rev/24cb98d079e5
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8821360 [details]
Bug 1325501 - Adds ServiceRequest as a drop-in replacement for XHR, which uses conservative TLS settings

Approval Request Comment
[Feature/Bug causing the regression]: TLS 1.3, see comment 8 and also bug 1321783.
[User impact if declined]: users behind some middleboxes will not get add-on and blocklist updates, or be able to send Telemetry.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: yes - ekr might have examples of middleboxes which are known to break this and/or how to simulate them for testing purposes.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: this extends the existing XMLHttpRequest object and adds a single setting to the channel.
[String changes made/needed]: none
Flags: needinfo?(ekr)
Attachment #8821360 - Flags: approval-mozilla-beta?
Attachment #8821360 - Flags: approval-mozilla-aurora?
Attachment #8821453 - Flags: approval-mozilla-beta?
Attachment #8821453 - Flags: approval-mozilla-aurora?
Attachment #8821454 - Flags: approval-mozilla-beta?
Attachment #8821454 - Flags: approval-mozilla-aurora?
Matt, is this something you could test and verify once it lands in beta? If it should go to Softvision instead, let me know.
Flags: needinfo?(mwobensmith)
Comment on attachment 8821360 [details]
Bug 1325501 - Adds ServiceRequest as a drop-in replacement for XHR, which uses conservative TLS settings

Part of the TLS 1.3 update, let's uplift this to aurora and beta. 
If we test this and it looks decent, but we release it in 51 and we break updates or telemetry, how would we react/mitigate the problem?
Attachment #8821360 - Flags: approval-mozilla-beta?
Attachment #8821360 - Flags: approval-mozilla-beta+
Attachment #8821360 - Flags: approval-mozilla-aurora?
Attachment #8821360 - Flags: approval-mozilla-aurora+
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #30)
> Matt, is this something you could test and verify once it lands in beta? If
> it should go to Softvision instead, let me know.

Actually, if the concern is that telemetry functions as expected, I don't think I'm the right person, as I've never tested that. I believe SV does, if I'm not mistaken.
Flags: needinfo?(mwobensmith)
Needs rebasing for Beta.
Flags: needinfo?(rhelmer)
Andrei, can your team help to test this once it lands in beta? 
David, is there anything you can think of to make sure that we aren't breaking telemetry with this, either now in aurora or once we land the patches for beta?
Flags: needinfo?(ddurst)
Flags: qe-verify+
Sorry, I just caught this needinfo. We don't have a working middlebox that demonstrates the failure here (one of the frustrations of working in networking). We could presumably hack one up, but I think just demonstrating that we offer TLS 1.2 even if 1.3 is on (which it is in Aurora and can be flipped with a pref in Beta) is sufficient. I do think it's important to verify that this doesn't *break* telemetry as-is (as any change in this area could) but that can be done with the usual testing.
Flags: needinfo?(ekr)
Attached patch bug1325501-beta.diff (obsolete) — Splinter Review
I've rebased this onto beta, here's an `hg export` patch.
Flags: needinfo?(rhelmer) → needinfo?(ryanvm)
Comment on attachment 8821453 [details]
Bug 1325501 - move addons manager from XHR to ServiceRequest

this already landed in aurora
Attachment #8821453 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8821454 [details]
Bug 1325501 - move Telemetry from XHR to ServiceRequest

this already landed in aurora
Attachment #8821454 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
If we're worried that there's the possibility of breaking telemetry transmission in general, I would think that would be Georg's bailiwick, no?
Flags: needinfo?(ddurst) → needinfo?(gfritzsche)
Comment on attachment 8821453 [details]
Bug 1325501 - move addons manager from XHR to ServiceRequest

Ekr mentioned that we need this patch for the TLS 1.3 experiment for Beta51. This should be in 51.0b12 which we will gtb 1/5/2016
Attachment #8821453 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8821454 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Hi Rhelmer, is this the only patch that needs to land on m-b? https://bugzilla.mozilla.org/attachment.cgi?id=8823396. KWierso will help land the right one today as we'd like this to go out in 51.0b12 on Friday.
Flags: needinfo?(wkocher)
Flags: needinfo?(rhelmer)
(In reply to Ritu Kothari (:ritu) from comment #42)
> Hi Rhelmer, is this the only patch that needs to land on m-b?
> https://bugzilla.mozilla.org/attachment.cgi?id=8823396. KWierso will help
> land the right one today as we'd like this to go out in 51.0b12 on Friday.

To expand on this, it looks like the rebased patch is supposed to replace part 1 of the three original patches, but after swapping it in, I'm hitting conflicts in part 2 (looks like we're missing bug 1267495 on firefox51).
Flags: needinfo?(wkocher)
Flags: needinfo?(ryanvm)
Oops! Sorry Wes, I accidentally attached the wrong patch. Here is one that is an hg export, so it should have all three commits.
Attachment #8823396 - Attachment is obsolete: true
Flags: needinfo?(rhelmer) → needinfo?(wkocher)
Well, except for the mass bustage of things saying XPCOMUtils.jsm is not defined.

Hopefully this fixes those: https://hg.mozilla.org/releases/mozilla-beta/rev/4f14552f2b1acd5e93752b8d2fee02497392bece


This import was originally added in bug 1267495, which didn't get uplifted to 51: https://hg.mozilla.org/mozilla-central/rev/7c1929f35c5d
Backed out in https://hg.mozilla.org/releases/mozilla-beta/rev/f06afa54549bff3f2f03a8510334046c1050465a for what will eventually be best illustrated by https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=4f14552f2b1acd5e93752b8d2fee02497392bece&filter-failure_classification_id=2 (browser-chrome and chrome complaining that you "Cannot modify properties of a WrappedNative" and a large handful of xpcshell timeouts).
Thanks Phil. I am going to spend some more quality time with my local beta build and see what's going on here.
It looks like the problem now is that the beConservative setting is not available on beta.

Patrick, ekr suggested I ask you about this - is that safe to uplift to beta? relman has said that they'd like to go to beta (b12) tomorrow afternoon.
Flags: needinfo?(mcmanus)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #35)
> David, is there anything you can think of to make sure that we aren't
> breaking telemetry with this, either now in aurora or once we land the
> patches for beta?

We should make sure that Telemetry still keeps submitting successfully against our servers.
It would be sufficient to test that one "main" ping type is submitted successfully.
I can work with QA on the details and set them up to see confirmation of their submission success real-time.

What i can't judge here is whether this needs testing through different network setups or if just any standard setup is sufficient.
Maybe rhelmer, Patrick, ekr can comment to that?
Flags: needinfo?(gfritzsche)
One server is fine. The test is just that it offers the old version no matter whether 1.3 is enabled.
(In reply to Robert Helmer [:rhelmer] from comment #49)
> It looks like the problem now is that the beConservative setting is not
> available on beta.
> 
> Patrick, ekr suggested I ask you about this - is that safe to uplift to
> beta? relman has said that they'd like to go to beta (b12) tomorrow
> afternoon.

1321783 would be needed on beta. That should be ok if we really want tls 1.3 on beta - I'll rebase the patch in that bug and nom for uplift asap.
Flags: needinfo?(mcmanus)
> 
> 1321783 would be needed on beta. That should be ok if we really want tls 1.3
> on beta - I'll rebase the patch in that bug and nom for uplift asap.

done
https://bugzilla.mozilla.org/show_bug.cgi?id=1321783#c35
Depends on: 1321783
We also need this import patch from comment 46 - only on Beta.
Comment on attachment 8824212 [details] [diff] [review]
bug1325501-xpcomutils-import.diff

This is just a JSM import that hasn't landed on Beta yet, needed for this bug.
Attachment #8824212 - Flags: approval-mozilla-beta?
Attachment #8824212 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
There's another minor import issue on beta-only with XHR - I've just reverted it to the way it was on beta before.

I've confirmed locally that it fixes the test_GMPInstallManager.js failure we're seeing on beta.
Attachment #8824255 - Flags: approval-mozilla-beta?
Comment on attachment 8824255 [details] [diff] [review]
bug1325501-xhr-import.diff

One more try!
Attachment #8824255 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
We've performed manual testing using Firefox 51 beta 12, and everything looks fine.

Operating Systems tested: Win 7 x64, Mac OS X 10.11.6, Ubuntu 14.04 x64

Areas tested:
- Sending Telemetry
   * Verified sending of sync ping, Environment change (add-on disabling), and FHR disabling (deletion ping)
- Add-on updates
   * Regular add-ons (update from an older add-on version to the latest version)
   * System add-ons
- Blocklist update

Detailed test results can be found here: https://public.etherpad-mozilla.org/p/1325501-51b12.

Let us know if you think there's something else that would need testing here.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.