1245956 - Drop sideloading cert requirement in the client

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Jorge Villalobos [:jorgev] (he/him)]

As part of the extension signing initiative, we initially established that there would be two different types of cert for add-ons that would be given on AMO depending on the type of review they passed. Full review would give the add-on sideloading privileges, and preliminary review would only give web install privileges. Firefox would enforce this.

Since then, we changed the signing system and all unlisted add-ons are getting signed without a review, and we will use different markers to determine if an unlisted add-on should be reviewed, sideloading being one of those markers.

Instead of blocking installation in Firefox depending on the type of install and cert, we have decided to drop this requirement and use telemetry data to detect which add-ons are being sideloaded. We can then use this data on AMO to prioritize reviews. This way, any bugs related to the foreign install flag will only affect the data we work with, rather than cause installation errors that affect developers and users directly.

In short, Firefox should allow both web installs and sideloading regardless of which of the two certs is used.

Comment 1

[Kev Needham [:kev]]

Before we take sideloading out, I'd like to ensure we have the telemetry up and running, and have a good process for identifying and confirming that sideloaded add-ons are being detected properly. Do we have the telemetry analysis process rolling? If not, we should get that defined and running first, then phase out the sideloaded certs.

Comment 2

[Andy McKay]

(In reply to Kev Needham [:kev] from comment #1)
> Before we take sideloading out, I'd like to ensure we have the telemetry up
> and running, and have a good process for identifying and confirming that
> sideloaded add-ons are being detected properly. Do we have the telemetry
> analysis process rolling? If not, we should get that defined and running
> first, then phase out the sideloaded certs.

Not that I'm aware of. But jorgev may know more.

Comment 3

[Jorge Villalobos [:jorgev] (he/him)]

This is what I got from Mossop when I asked about it:

> On the telemetry side we send the type of signing for every
> enabled add-on (full or preliminary). We also send the foreignInstall
> flag which is suspect in the usual ways.

To verify this data, we would probably need someone from Metrics to make a report for us so we can check how close it is to what we know. I don't know who to ask for this.

Comment 4

[Andy McKay]

I've filed bug 1248638 to track the telemetry report and https://github.com/mozilla/addons/issues/32 to cover the AMO issues.

If we change AMO to use the fully signed certificate and everything upgrades to use that cert, eventually we can remove the sideloaded certificate completely. But for the moment, let's accept both.

Comment 5

[Andrew Swan [:aswan]]

Attachment: Don't distinguish preliminary signing for add-ons

I think this is the most expedient change, but it leaves some holes in
the numbering of add-ons in browser_detail.js and browser_list.js.  I
can renumber the add-ons to make everything continiguous at the cost of
a much larger diff if that is desirable...

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 8720513 [details] [diff] [review]
Don't distinguish preliminary signing for add-ons

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

I'd like to be risk averse here and for the time being I'd like to keep all the logic that checks for preliminary and full certificates in place. Those constants are stored in the database so any change of them could potentially break existing users and the same is true if we were to revert this change for some reason. Instead lets just remove the code that refuses to load sideloaded add-ons if they aren't fully signed: https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#696

Given that I'd like to keep most of the tests in place and just update the assertions to check that add-ons signed by the preliminary cert can be loaded in any case.

Comment 7

[Andrew Swan [:aswan]]

Okay, that will be an even smaller change.  I'm not sure what to do about the existing browser tests though?  For instance this test: https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser_details.js#856
It tests that about:addons displays the expected warning for a sideloaded add-on signed with the preliminary cert, but it does so by manually assembling an the add-on state: https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser_details.js#159
Since that specific condition will no longer exist, should I remove that test?  (and a similar one in browser_list.js)

Comment 8

[Dave Townsend [:mossop]]

(In reply to Andrew Swan [:aswan] from comment #7)
> Okay, that will be an even smaller change.  I'm not sure what to do about
> the existing browser tests though?  For instance this test:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> test/browser/browser_details.js#856
> It tests that about:addons displays the expected warning for a sideloaded
> add-on signed with the preliminary cert, but it does so by manually
> assembling an the add-on state:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> test/browser/browser_details.js#159
> Since that specific condition will no longer exist, should I remove that
> test?  (and a similar one in browser_list.js)

I'd say to change its state so appDisabled is false and isActive is true and then verify the UI is correct (we're not showing a warning about needing a full signature unnecessarily).

Comment 9

[Andrew Swan [:aswan]]

Attachment: Don't distinguish preliminary signing for add-ons

Comment 10

[Dave Townsend [:mossop]]

Comment on attachment 8721372 [details] [diff] [review]
Don't distinguish preliminary signing for add-ons

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

Looks good other than the small test change

::: toolkit/mozapps/extensions/test/browser/browser_details.js
@@ -161,5 @@
>      signedState: AddonManager.SIGNEDSTATE_PRELIMINARY,
>      foreignInstall: true,
> -    isActive: false,
> -    appDisabled: true,
> -    isCompatible: false,

Oh I just remembered what this is testing. Leave this alone and then in both the cases below we should display the incompatible message rather than switching to the unsigned warning.

Comment 11

[Andrew Swan [:aswan]]

(In reply to Dave Townsend [:mossop] from comment #10)
> ::: toolkit/mozapps/extensions/test/browser/browser_details.js
> @@ -161,5 @@
> >      signedState: AddonManager.SIGNEDSTATE_PRELIMINARY,
> >      foreignInstall: true,
> > -    isActive: false,
> > -    appDisabled: true,
> > -    isCompatible: false,
> 
> Oh I just remembered what this is testing. Leave this alone and then in both
> the cases below we should display the incompatible message rather than
> switching to the unsigned warning.

Sorry, I'm not following.  I think this code is manually mimicking the logic in XPIProvider.jsm where (foreignInstall=true && signedState == SIGNEDSTATE_PRELIMINARY) causes appDisabled=true, and then appDisabled=true causes isActive=false.  With the changes in this patch, that combination of foreignInstall and signedState no longer causes appDisabled=true, so if we leave it as-is, I think we'll be testing a scenario that never actually occurs outside of the test?

The UI for showing a proper warning for an incompatible add-on is already tested here: https://dxr.mozilla.org/mozilla-central/rev/d5daf10d3b74b04e8fa63c4e5429de8a0adf79f8/toolkit/mozapps/extensions/test/browser/browser_details.js#419

Comment 12

[Dave Townsend [:mossop]]

(In reply to Andrew Swan [:aswan] from comment #11)
> (In reply to Dave Townsend [:mossop] from comment #10)
> > ::: toolkit/mozapps/extensions/test/browser/browser_details.js
> > @@ -161,5 @@
> > >      signedState: AddonManager.SIGNEDSTATE_PRELIMINARY,
> > >      foreignInstall: true,
> > > -    isActive: false,
> > > -    appDisabled: true,
> > > -    isCompatible: false,
> > 
> > Oh I just remembered what this is testing. Leave this alone and then in both
> > the cases below we should display the incompatible message rather than
> > switching to the unsigned warning.
> 
> Sorry, I'm not following.  I think this code is manually mimicking the logic
> in XPIProvider.jsm where (foreignInstall=true && signedState ==
> SIGNEDSTATE_PRELIMINARY) causes appDisabled=true, and then appDisabled=true
> causes isActive=false.  With the changes in this patch, that combination of
> foreignInstall and signedState no longer causes appDisabled=true, so if we
> leave it as-is, I think we'll be testing a scenario that never actually
> occurs outside of the test?

Not quite, this add-on is appDisabled for two reasons, the signing issue (which we're removing) and the incompatibility. The test verifies that when signing requirements are disabled we show it as disabled for being incompatible and when enabled we show it as disabled because it is unsigned. So the change you make to extensions.js changes the behaviour of the last bit so by changing the expectations there we verify that that change in extensions.js is correct.

Comment 13

[Andrew Swan [:aswan]]

(In reply to Dave Townsend [:mossop] from comment #12)
> (In reply to Andrew Swan [:aswan] from comment #11)
> > (In reply to Dave Townsend [:mossop] from comment #10)
> > > ::: toolkit/mozapps/extensions/test/browser/browser_details.js
> > > @@ -161,5 @@
> > > >      signedState: AddonManager.SIGNEDSTATE_PRELIMINARY,
> > > >      foreignInstall: true,
> > > > -    isActive: false,
> > > > -    appDisabled: true,
> > > > -    isCompatible: false,
> > > 
> > > Oh I just remembered what this is testing. Leave this alone and then in both
> > > the cases below we should display the incompatible message rather than
> > > switching to the unsigned warning.
> > 
> > Sorry, I'm not following.  I think this code is manually mimicking the logic
> > in XPIProvider.jsm where (foreignInstall=true && signedState ==
> > SIGNEDSTATE_PRELIMINARY) causes appDisabled=true, and then appDisabled=true
> > causes isActive=false.  With the changes in this patch, that combination of
> > foreignInstall and signedState no longer causes appDisabled=true, so if we
> > leave it as-is, I think we'll be testing a scenario that never actually
> > occurs outside of the test?
> 
> Not quite, this add-on is appDisabled for two reasons, the signing issue
> (which we're removing) and the incompatibility. The test verifies that when
> signing requirements are disabled we show it as disabled for being
> incompatible and when enabled we show it as disabled because it is unsigned.
> So the change you make to extensions.js changes the behaviour of the last
> bit so by changing the expectations there we verify that that change in
> extensions.js is correct.

I see, thanks.  Revised patch coming momentarily.

Comment 14

[Andrew Swan [:aswan]]

Attachment: Don't distinguish preliminary signing for add-ons

Comment 15

[Dave Townsend [:mossop]]

Comment on attachment 8721430 [details] [diff] [review]
Don't distinguish preliminary signing for add-ons

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

Thanks

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d86655d3c54c

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d86655d3c54c

Comment 18

[Andrew Halberstadt [:ahal]]

Even with this patch, I'm still hitting errors trying to install signed Talos addons (see bug 1248733 comment 17) because of this condition:
https://hg.mozilla.org/mozilla-central/file/d86655d3c54c/toolkit/mozapps/extensions/internal/XPIProvider.jsm#l3256

Was this left in accidentally?

Comment 19

[Andrew Halberstadt [:ahal]]

(sorry, that should be bug 1249733 comment 17)

Comment 20

[Andrew Swan [:aswan]]

Yep, sorry, patch coming shortly.

Comment 21

[Andrew Swan [:aswan]]

Attachment: MozReview Request: Bug 1245956 - Don't distinguish preliminary signing for add-ons part 2 r?mossop

Review commit: https://reviewboard.mozilla.org/r/36709/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36709/

Comment 22

[Dave Townsend [:mossop]]

Comment on attachment 8723760 [details]
MozReview Request: Bug 1245956 - Don't distinguish preliminary signing for add-ons part 2 r?mossop

https://reviewboard.mozilla.org/r/36709/#review33435

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/96077e578f26

Comment 24

[Andy McKay]

Krupa, we'd like to uplift this to 46 and since this will solve quite a few gnarly bugs, I'd like a bit of QA time on it.

Comment 25

[Andy McKay]

[Tracking Requested - why for this release]:
We are planning on removing the ability to turn off signing in Firefox 46. Some users are being caught by foreignInstall not being as reliable as we'd hoped. This stops using foreignInstall as a reliable install to disable add-ons.

If we don't do this, when we remove the pref, users caught by the foreignInstall issue will no longer be able to use those addons.

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/96077e578f26

Comment 27

[Andrew Halberstadt [:ahal]]

I can install talos addons now, thanks Andrew!

Comment 28

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking and marking affected for 46, as this is important for addon signing.

Comment 29

[Andrew Halberstadt [:ahal]]

I needed this on aurora to land Talos before the merge, so went ahead and landed:
https://hg.mozilla.org/releases/mozilla-aurora/rev/647147e1f80a
https://hg.mozilla.org/releases/mozilla-aurora/rev/6a84ad6d4f6d

Here's a try run I did (the red is infra issues in mozharness, unrelated to this patch):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=64d69835d7b0

Comment 30

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce this issue on Firefox 47.0a1 (2016-02-04) under Windows 10 64-bit.
Verified fixed on Firefox 47.0a1 (2016-03-07) and Firefox 46.0a2 (2016-03-07) under Windows 10 64-bit, Ubuntu 13.10 64-bit and Mac OS X 10.10.4. Both prelim and fully reviewed add-ons are successfully installed via web install and sideloading methods. 

Detailed results: https://goo.gl/LXqh00

Comment 31

[Mike Kaply [:mkaply]]

[Tracking Requested - why for this release]:

Enterprises have been having trouble with this concept as well (I've had to reports this week.).

Since we are getting rid of the concept completely, we should drop it in the ESR as well.

Comment 32

[Sylvestre Ledru [:Sylvestre]]

Andrew, could you fill an uplift request to esr45 too? Thanks

Comment 33

[Andrew Swan [:aswan]]

Comment on attachment 8721430 [details] [diff] [review]
Don't distinguish preliminary signing for add-ons

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
We tweaked the approach to signing in 47 and subsequently uplifted those changes to 46.  But users of add-ons that were signed prior to this change can experience installation errors, and complaints from ESR users have been reported, see eg https://bugzilla.mozilla.org/show_bug.cgi?id=1245956#c31.

User impact if declined: 
Users will experience installation errors with add-ons that were signed with the preliminary certificate.  Troubleshooting will be awkward since we have abandoned the concept of preliminary signing.

Fix Landed on Version:
47 originally, then uplifted to 46.

Risk to taking this patch (and alternatives if risky): 
Minimal, the change only removes code, resulting in relaxing the conditions required to load an add-on.  The new behavior brings ESR in sync with 46 and newer.

String or UUID changes made by this patch: 
none

Comment 34

[Andrew Swan [:aswan]]

Comment on attachment 8723760 [details]
MozReview Request: Bug 1245956 - Don't distinguish preliminary signing for add-ons part 2 r?mossop

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
We tweaked the approach to signing in 47 and subsequently uplifted those changes to 46.  But users of add-ons that were signed prior to this change can experience installation errors, and complaints from ESR users have been reported, see eg https://bugzilla.mozilla.org/show_bug.cgi?id=1245956#c31.

User impact if declined: 
Users will experience installation errors with add-ons that were signed with the preliminary certificate.  Troubleshooting will be awkward since we have abandoned the concept of preliminary signing.

Fix Landed on Version:
47 originally, then uplifted to 46.

Risk to taking this patch (and alternatives if risky): 
Minimal, the change only removes code, resulting in relaxing the conditions required to load an add-on.  The new behavior brings ESR in sync with 46 and newer.

String or UUID changes made by this patch: 
none

Comment 36

[Andy McKay]

It's been requested: that we back this out of 46 and instead land it in 47, but I'm concerned this is going to cause a problem for ahal in comment 18.

Comment 37

[Liz Henry (:lizzard) (relman/hg->git project)]

Wes or ahal, can you back this out this morning from mozilla-beta?

Comment 38

[Andy McKay]

If you could just hold off a little bit whilst we chat to security, that would be great.

Comment 39

[Sylvestre Ledru [:Sylvestre]]

Not sure what I should do wrt esr45 now?!

Comment 40

[Jorge Villalobos [:jorgev] (he/him)]

The change shouldn't be included on ESR 45 for now. Sorry for the last-minute change.

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

So am I backing this out of 46 or not?

Comment 42

[Liz Henry (:lizzard) (relman/hg->git project)]

Not knowing what the state of this is supposed to be now blocks the 46 release.

Comment 43

[Andy McKay]

After chatting with dveditz we are going to continue landing this. Kev is working with dveditz on the next steps and improvements.

Let's continue with landing this in 46 and ESR.

Apologies everyone for the noise and thank you for your patience.

Comment 44

[Liz Henry (:lizzard) (relman/hg->git project)]

Sylvestre, according to irc chat with andym we should land this in esr45. I will go ahead and approve that now so that you can start the esr build tomorrow.

Comment 45

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8721430 [details] [diff] [review]
Don't distinguish preliminary signing for add-ons

Approved for esr45 after talking with andym and dveditz.

Comment 46

[Sylvestre Ledru [:Sylvestre]]

OK, next time, please try to act sooner. A week before the release day is not enough...

Comment 47

[Kev Needham [:kev]]

Sorry Sylvestre, this was my fault. Wasn't part of the plan at all, and won't happen again.

(In reply to Sylvestre Ledru [:sylvestre] from comment #46)
> OK, next time, please try to act sooner. A week before the release day is
> not enough...

Comment 48

[Carsten Book [:Tomcat]]

remote:   https://hg.mozilla.org/releases/mozilla-esr45/rev/a488abb7ec3f
remote:   https://hg.mozilla.org/releases/mozilla-esr45/rev/9c9c4da067fe

landed on esr45 :)

Comment 49

[Liz Henry (:lizzard) (relman/hg->git project)]

Setting status back to verified for 46 as it sounds like this is still in the correct state on mozilla-release.

Comment 50

[Carsten Book [:Tomcat]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #49)
> Setting status back to verified for 46 as it sounds like this is still in
> the correct state on mozilla-release.

yeah confirmed its on beta with
https://hg.mozilla.org/releases/mozilla-beta/rev/6a84ad6d4f6d
and https://hg.mozilla.org/releases/mozilla-beta/rev/647147e1f80a

Comment 51

[Vasilica Tamas, QA [:vtamas]]

Confirm that this bug is fixed on Firefox 45.1.0 esr (20160420142932) under Windows 10 64-bit, Mac OS X 10.11.1 and Ubuntu 14.04 32-bit. All add-ons are successfully installed via both methods: web install and sideloading install.

Testing details are tracked in https://goo.gl/wMxcTj