Closed Bug 1245956 Opened 4 years ago Closed 4 years ago

Drop sideloading cert requirement in the client

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla46
Iteration:
47.3 - Mar 7
Tracking Status
firefox46 + verified
firefox47 --- verified
firefox-esr45 46+ verified

People

(Reporter: jorgev, Assigned: aswan)

References

(Depends on 1 open bug)

Details

(Whiteboard: [good first bug][qa+])

Attachments

(2 files, 2 obsolete files)

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.
Blocks: 1240191
Whiteboard: [good first bug]
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.
(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.
Flags: needinfo?(jorge)
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.
Flags: needinfo?(jorge)
Depends on: 1248638
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.
Assignee: nobody → aswan
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...
Attachment #8720513 - Flags: review?(dtownsend)
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.
Attachment #8720513 - Flags: review?(dtownsend)
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)
Flags: needinfo?(dtownsend)
(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).
Flags: needinfo?(dtownsend)
Attachment #8721372 - Flags: review?(dtownsend)
Attachment #8720513 - Attachment is obsolete: true
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.
Attachment #8721372 - Flags: review?(dtownsend) → review+
(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
Flags: needinfo?(dtownsend)
(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.
Flags: needinfo?(dtownsend)
(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.
Attachment #8721372 - Attachment is obsolete: true
Comment on attachment 8721430 [details] [diff] [review]
Don't distinguish preliminary signing for add-ons

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

Thanks
Attachment #8721430 - Flags: review?(dtownsend) → review+
Status: NEW → ASSIGNED
Keywords: checkin-needed
Blocks: 1249733
Iteration: --- → 47.3 - Mar 7
https://hg.mozilla.org/mozilla-central/rev/d86655d3c54c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
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?
Flags: needinfo?(aswan)
(sorry, that should be bug 1249733 comment 17)
Yep, sorry, patch coming shortly.
Status: RESOLVED → REOPENED
Flags: needinfo?(aswan)
Resolution: FIXED → ---
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
Attachment #8723760 - Flags: review?(dtownsend) → review+
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.
Flags: needinfo?(krupa.mozbugs)
Whiteboard: [good first bug] → [good first bug][qa+]
[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.
https://hg.mozilla.org/mozilla-central/rev/96077e578f26
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
I can install talos addons now, thanks Andrew!
Tracking and marking affected for 46, as this is important for addon signing.
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
Target Milestone: mozilla47 → mozilla46
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
Status: RESOLVED → VERIFIED
Flags: needinfo?(krupa.mozbugs)
[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.
Andrew, could you fill an uplift request to esr45 too? Thanks
Flags: needinfo?(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
Attachment #8721430 - Flags: approval-mozilla-esr45?
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
Flags: needinfo?(aswan)
Attachment #8723760 - Flags: approval-mozilla-esr45?
Duplicate of this bug: 1264938
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.
Flags: needinfo?(ahalberstadt)
Wes or ahal, can you back this out this morning from mozilla-beta?
Flags: needinfo?(wkocher)
If you could just hold off a little bit whilst we chat to security, that would be great.
Not sure what I should do wrt esr45 now?!
The change shouldn't be included on ESR 45 for now. Sorry for the last-minute change.
So am I backing this out of 46 or not?
Flags: needinfo?(wkocher) → needinfo?(amckay)
Flags: needinfo?(lhenry)
Not knowing what the state of this is supposed to be now blocks the 46 release.
Flags: needinfo?(lhenry)
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.
Flags: needinfo?(amckay)
Flags: needinfo?(ahalberstadt)
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.
Flags: needinfo?(sledru)
Comment on attachment 8721430 [details] [diff] [review]
Don't distinguish preliminary signing for add-ons

Approved for esr45 after talking with andym and dveditz.
Attachment #8721430 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Attachment #8723760 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
OK, next time, please try to act sooner. A week before the release day is not enough...
Flags: needinfo?(sledru)
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...
Setting status back to verified for 46 as it sounds like this is still in the correct state on mozilla-release.
(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
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
You need to log in before you can comment on or make changes to this bug.