Robert Helmer [:rhelmer] Assignee
	Description • 7 years ago

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.

	Dave Townsend [:mossop]
	Comment 2 • 7 years ago
mozreview-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/#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.

	Robert Helmer [:rhelmer] Assignee
	Comment 6 • 7 years ago

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.

	Georg Fritzsche [:gfritzsche]
	Comment 7 • 7 years ago
mozreview-review

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?

	Eric Rescorla (:ekr)
	Comment 8 • 7 years ago

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

	Robert Helmer [:rhelmer] Assignee
	Comment 9 • 7 years ago

(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.

	Kris Maglione [:kmag]
	Comment 10 • 7 years ago
mozreview-review

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?

	Kris Maglione [:kmag]
	Comment 11 • 7 years ago
mozreview-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?

	Robert Helmer [:rhelmer] Assignee
	Comment 12 • 7 years ago
mozreview-review-reply

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.

	Pulsebot
	Comment 21 • 7 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 22 • 7 years ago

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

	Robert Helmer [:rhelmer] Assignee
	Comment 23 • 7 years ago

(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.

	Pulsebot
	Comment 26 • 7 years ago

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

	Kris Maglione [:kmag]
	Comment 27 • 7 years ago

(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.

	Phil Ringnalda (:philor)
	Comment 28 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/5d35c4556536
https://hg.mozilla.org/mozilla-central/rev/1972c0d34e2a
https://hg.mozilla.org/mozilla-central/rev/24cb98d079e5

	Robert Helmer [:rhelmer] Assignee
	Comment 29 • 7 years ago

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

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 30 • 7 years ago

Matt, is this something you could test and verify once it lands in beta? If it should go to Softvision instead, let me know.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 31 • 7 years ago

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?

	Matt Wobensmith [:mwobensmith][:matt:]
	Comment 32 • 7 years ago

(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.

	Ryan VanderMeulen [:RyanVM]
	Comment 33 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/e99d27f978b4
https://hg.mozilla.org/releases/mozilla-aurora/rev/a4c896337223
https://hg.mozilla.org/releases/mozilla-aurora/rev/dd173da99760

	Ryan VanderMeulen [:RyanVM]
	Comment 34 • 7 years ago

Needs rebasing for Beta.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 35 • 7 years ago

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?

	Eric Rescorla (:ekr)
	Comment 36 • 7 years ago

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.

	Robert Helmer [:rhelmer] Assignee
	Comment 37 • 7 years ago

I've rebased this onto beta, here's an `hg export` patch.

	Julien Cristau [:jcristau]
	Comment 38 • 7 years ago

Comment on attachment 8821453 [details]
Bug 1325501 - move addons manager from XHR to ServiceRequest

this already landed in aurora

	Julien Cristau [:jcristau]
	Comment 39 • 7 years ago

Comment on attachment 8821454 [details]
Bug 1325501 - move Telemetry from XHR to ServiceRequest

this already landed in aurora

	David Durst [:ddurst]
	Comment 40 • 7 years ago

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?

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 41 • 7 years ago

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

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 42 • 7 years ago

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.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 43 • 7 years ago

(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).

	Robert Helmer [:rhelmer] Assignee
	Comment 44 • 7 years ago

Oops! Sorry Wes, I accidentally attached the wrong patch. Here is one that is an hg export, so it should have all three commits.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 45 • 7 years ago

That works much better! :)
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/e3127efa2f570ce95935c310d0919efd5453d272
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/b2636a0a25011e60e704701fddf3de03945c561a
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/10eb6b8c37f12d0d970774e3b5fae360fe57d856

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 46 • 7 years ago

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

	Phil Ringnalda (:philor)
	Comment 47 • 7 years ago

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).

	Robert Helmer [:rhelmer] Assignee
	Comment 48 • 7 years ago

Thanks Phil. I am going to spend some more quality time with my local beta build and see what's going on here.

	Robert Helmer [:rhelmer] Assignee
	Comment 49 • 7 years ago

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.

	Georg Fritzsche [:gfritzsche]
	Comment 50 • 7 years ago

(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?

	Eric Rescorla (:ekr)
	Comment 51 • 7 years ago

One server is fine. The test is just that it offers the old version no matter whether 1.3 is enabled.

	Patrick McManus [:mcmanus]
	Comment 52 • 7 years ago

(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.

	Patrick McManus [:mcmanus]
	Comment 53 • 7 years ago

> 
> 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

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 54 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/8b0392813468

	Robert Helmer [:rhelmer] Assignee
	Comment 55 • 7 years ago

We also need this import patch from comment 46 - only on Beta.

	Robert Helmer [:rhelmer] Assignee
	Comment 56 • 7 years ago

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.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 57 • 7 years ago

That landed in https://hg.mozilla.org/releases/mozilla-beta/rev/78de6c8098e439e5dedbd8fb677121008ee8646b

	Robert Helmer [:rhelmer] Assignee
	Comment 58 • 7 years ago

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.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 59 • 7 years ago

Comment on attachment 8824255 [details] [diff] [review]
bug1325501-xhr-import.diff

One more try!

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 60 • 7 years ago

https://hg.mozilla.org/releases/mozilla-beta/rev/9ddd4fee07842e72ba49f1583ec5f596f6e60e72

	[SV Manager] Florin Mezei, QA (:FlorinMezei)
	Comment 61 • 7 years ago

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.