Closed Bug 1164168 Opened 4 years ago Closed 4 years ago

RFE: mozconfig option to disable unsigned addon warning at build time for non-Firefox applications

Categories

(Toolkit :: Add-ons Manager, defect, P1)

40 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla41
Iteration:
41.1 - May 25
Tracking Status
firefox40 + fixed
firefox41 --- fixed

People

(Reporter: kevink9876543, Assigned: mossop)

References

Details

(Whiteboard: [hijacking][fxsearch])

Attachments

(2 files, 1 obsolete file)

I've noticed that SeaMonkey nightlies addons manager is warning about unsigned addons: "[addon] could not be verified for use in SeaMonkey. Proceed with caution."
While this scary warning is very appropriate for unbranded Firefox builds, it makes no sense in an application that has no intention to enforce addon signing - it just needlessly spooks users into thinking most of our addons are suddenly dangerous or we'll be stuck to lose them.

Please make this warning, as well as all aspects of the signing enforcement, disable-able at build time with a mozconfig option, so that users of SeaMonkey and other non-Firefox Toolkit applications can rest assured that we can keep our custom addons.
Thanks.



Some comments from RattyAway from irc:
	RattyAway	The changes look pretty confined https://hg.mozilla.org/mozilla-central/rev/65afb97fc399
	RattyAway	should be eazy to add an extra condition to
	RattyAway	      } else if (this._addon.signedState <= AddonManager.SIGNEDSTATE_MISSING) {

Some discussion at mozillaZine: http://forums.mozillazine.org/viewtopic.php?p=14151755#p14151755


Ratty also suggest in bug 1149702 comment 20 that a #ifdef for MOZ_REQUIRE_SIGNING could be a possible solution.
See Also: → 1149702
Blocks: 1149702
I think the right solution here is an ifdef that makes verifyDirSignedState and verifyZipSignedState return undefined. That should flag everything as not needing signing and any messaging anywhere in the product should go away.
Blocks: 1163046
Assignee: nobody → dtownsend
Flags: firefox-backlog+
Whiteboard: [fxsearch][hijacking]
See Also: 1149702
Attached patch patch (obsolete) — Splinter Review
I think this is what we want, two build flags to control whether signatures are checked and required, set at the browser level only. Only trouble is it looks like MOZ_OFFICIAL_BRANDING isn't available in browser/confvars.sh so this doesn't work.

Should I move things in configure.in so MOZ_OFFICIAL_BRANDING is set sooner or should I do something else here?
Attachment #8604882 - Flags: feedback?(gps)
Comment on attachment 8604882 [details] [diff] [review]
patch

So with this patch, is there a way to *explicitly* disable building addon signing in .mozconfig for a non-Firefox build?
(In reply to kevink9876543 from comment #3)
> Comment on attachment 8604882 [details] [diff] [review]
> patch
> 
> So with this patch, is there a way to *explicitly* disable building addon
> signing in .mozconfig for a non-Firefox build?

No, with this patch add-on signing is already disabled for non-Firefox, there is no way to enable it
(In reply to Dave Townsend [:mossop] from comment #4)
> No, with this patch add-on signing is already disabled for non-Firefox,
> there is no way to enable it
Sounds good, thanks for the clarification.
Comment on attachment 8604882 [details] [diff] [review]
patch

Ted, maybe you can look at this sooner than gps?
Attachment #8604882 - Flags: feedback?(ted)
Rank: 10
Priority: -- → P1
Whiteboard: [fxsearch][hijacking] → [hijacking][fxsearch]
[Tracking Requested - why for this release]:

This causes problems for other applications
Blocks: 1149654
Based on the vote count (10 so far) and the impact (comment #1) adding a tracking flag for firefox40.
Comment on attachment 8604882 [details] [diff] [review]
patch

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

LGTM.

::: browser/confvars.sh
@@ +72,5 @@
> +MOZ_ADDON_SIGNING=1
> +if test "$MOZ_OFFICIAL_BRANDING"; then
> +  if test "$MOZ_UPDATE_CHANNEL" = "beta" -o \
> +          "$MOZ_UPDATE_CHANNEL" = "release" -o \
> +          "$MOZ_UPDATE_CHANNEL" = "esr"; then

I'm not a huge fan of this chained channel name check. IMO this should be encapsulated in its own variable defined in configure.in. We already have EARLY_BETA_OR_EARLIER. We may want to create BETA_OR_LATER. I /thought/ we already had such a mechanism. But on a quick glance, I can't find it, so maybe it doesn't exist.

Given the urgency of this request, I think a proper variable can be deferred to a follow-up bug.
Attachment #8604882 - Flags: feedback?(gps) → feedback+
Status: UNCONFIRMED → ASSIGNED
Iteration: --- → 41.1 - May 25
Ever confirmed: true
Flags: qe-verify?
(In reply to Dave Townsend [:mossop] from comment #2)
> Created attachment 8604882 [details] [diff] [review]
> patch
> 
> I think this is what we want, two build flags to control whether signatures
> are checked and required, set at the browser level only. Only trouble is it
> looks like MOZ_OFFICIAL_BRANDING isn't available in browser/confvars.sh so
> this doesn't work.
> 
> Should I move things in configure.in so MOZ_OFFICIAL_BRANDING is set sooner
> or should I do something else here?

I think you missed the question I asked here
Flags: needinfo?(gps)
I don't see any uses of MOZ_OFFICIAL_BRANDING in the app-specific confvars.sh or included files. It should be safe to move MOZ_OFFICIAL_BRANDING to before confvars.sh is called. However, MOZ_BRANDING_DIRECTORY is set by a few confvars.sh files. Furthermore, processing of --enable-official-branding validates that MOZ_BRANDING_DIRECTORY is set. So this isn't simply going to be copy and paste.
Flags: needinfo?(gps)
Attached patch patchSplinter Review
This seems to work. GPS to review the build config changes, Dan to review the XPIProvider.jsm changes.
Attachment #8604882 - Attachment is obsolete: true
Attachment #8604882 - Flags: feedback?(ted)
Attachment #8608358 - Flags: review?(gps)
Attachment #8608358 - Flags: review?(dveditz)
Comment on attachment 8608358 [details] [diff] [review]
patch

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

r=dveditz for the XPIProvider.jsm bits.
Attachment #8608358 - Flags: review?(dveditz) → review+
Comment on attachment 8608358 [details] [diff] [review]
patch

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

This is good enough to land.

If I had one nit, it would be to add configure flags for MOZ_ADDON_SIGNING and MOZ_REQUIRE_SIGNING. MOZ_REQUIRE_SIGNING is the more important one, as without it, developers will need to enable --enable-official-branding and pretend they are building on a release channel to activate this feature. That seems like a bit much. But presumably the audience who needs to do that is small. And you are part of that pool. So I'll leave this up to you to file a follow-up if necessary.
Attachment #8608358 - Flags: review?(gps) → review+
Note that it will take up to a day after a user updates to a version with this change in it before the signing warnings will disappear in other apps. I still need to do up an aurora version of this patch.
Flags: needinfo?(dtownsend)
Attached patch aurora patchSplinter Review
https://hg.mozilla.org/mozilla-central/rev/22761f1474f0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1167986
Comment on attachment 8609541 [details] [diff] [review]
aurora patch

This is essentially the same code uplifted to Aurora but without the bits to force signing on in beta/release since we don't want that in 40.

Approval Request Comment
[Feature/regressing bug #]: Various from add-on signing bugs
[User impact if declined]: Users of Thunderbird, Seamonkey etc. will see warnings about unsigned add-ons needlessly.
[Describe test coverage new/current, TreeHerder]: Running on m-c
[Risks and why]: This should have no impact on Firefox, only other applications are affected
[String/UUID change made/needed]: None
Flags: needinfo?(dtownsend)
Attachment #8609541 - Flags: approval-mozilla-aurora?
Attachment #8609541 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1196660
You need to log in before you can comment on or make changes to this bug.