kevink9876543 Reporter
	Description • 9 years ago

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.

	Dave Townsend [:mossop] Assignee
	Comment 1 • 9 years ago

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.

	Dave Townsend [:mossop] Assignee
	Comment 2 • 9 years ago

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?

	kevink9876543 Reporter
	Comment 3 • 9 years ago

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?

	Dave Townsend [:mossop] Assignee
	Comment 4 • 9 years ago

(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

	kevink9876543 Reporter
	Comment 5 • 9 years ago

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

	Dave Townsend [:mossop] Assignee
	Comment 6 • 9 years ago

Comment on attachment 8604882 [details] [diff] [review]
patch

Ted, maybe you can look at this sooner than gps?

	Dave Townsend [:mossop] Assignee
	Comment 7 • 9 years ago

[Tracking Requested - why for this release]:

This causes problems for other applications

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

Based on the vote count (10 so far) and the impact (comment #1) adding a tracking flag for firefox40.

	Gregory Szorc [:gps]
	Comment 9 • 9 years ago

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.

	Dave Townsend [:mossop] Assignee
	Comment 10 • 9 years ago

(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

	Gregory Szorc [:gps]
	Comment 11 • 9 years ago

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.

	Dave Townsend [:mossop] Assignee
	Comment 12 • 9 years ago

This seems to work. GPS to review the build config changes, Dan to review the XPIProvider.jsm changes.

	Daniel Veditz [:dveditz]
	Comment 13 • 9 years ago

Comment on attachment 8608358 [details] [diff] [review]
patch

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

r=dveditz for the XPIProvider.jsm bits.

	Gregory Szorc [:gps]
	Comment 14 • 9 years ago

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.

	Pulsebot
	Comment 15 • 9 years ago

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

	Dave Townsend [:mossop] Assignee
	Comment 16 • 9 years ago

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.

	Phil Ringnalda (:philor)
	Comment 18 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/22761f1474f0

	Dave Townsend [:mossop] Assignee
	Comment 19 • 9 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 9 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/0e18c5fc9056