Closed Bug 1186522 Opened 4 years ago Closed 4 years ago

force per checkin and release desktop firefox builds to require signed add-ons on beta 48

Categories

(Release Engineering :: General, defect)

defect
Not set

Tracking

(firefox41 affected, firefox42 fixed, firefox43 fixed, firefox47 wontfix, firefox48+ fixed)

RESOLVED FIXED
Tracking Status
firefox41 --- affected
firefox42 --- fixed
firefox43 --- fixed
firefox47 --- wontfix
firefox48 + fixed

People

(Reporter: jlund, Assigned: kmoir)

References

Details

(Whiteboard: [releng])

Attachments

(10 files, 17 obsolete files)

1.42 KB, patch
rail
: review+
Details | Diff | Splinter Review
1.17 KB, patch
mossop
: review+
jlund
: checked-in+
Details | Diff | Splinter Review
644 bytes, patch
mshal
: review+
Details | Diff | Splinter Review
8.18 KB, patch
jlund
: review?
Details | Diff | Splinter Review
6.62 KB, patch
Details | Diff | Splinter Review
3.99 KB, patch
mshal
: review+
kmoir
: checked-in+
Details | Diff | Splinter Review
5.00 KB, patch
mshal
: review+
kmoir
: checked-in-
Details | Diff | Splinter Review
4.97 KB, patch
mshal
: review+
kmoir
: checked-in+
Details | Diff | Splinter Review
1.37 KB, patch
rail
: review+
kmoir
: checked-in+
Details | Diff | Splinter Review
3.87 KB, patch
kmoir
: checked-in+
Details | Diff | Splinter Review
Attached patch bug1186522.patch (obsolete) — Splinter Review
Jordan, is this is the right direction?  I didn't enable it for release builds until we got the checkin builds working
Attachment #8639437 - Flags: feedback?(jlund)
(In reply to Kim Moir [:kmoir] from comment #1)
> Created attachment 8639437 [details] [diff] [review]
> bug1186522.patch
> 
> Jordan, is this is the right direction?  I didn't enable it for release
> builds until we got the checkin builds working

looks pretty much like mine. I'd imagine we will end up taking a combination of our patches. I think we should enlist the 'pros' at this stage and see if we are on the right path.

I noticed that you have things like:
MOZ_REQUIRE_ADDON_SIGNING=

iiuc - the above, will ignore anything set from the mozconfigs and thus never get pref'd on our off. Or am I reading that wrong?

What we may need is something like:
MOZ_REQUIRE_ADDON_SIGNING=${MOZ_REQUIRE_ADDON_SIGNING-0}   # where we pass the env var and default to 0 if not defined.

in the patch I was working on I scrapped trying to find all the mozconfigs leaf nodes and instead I'm using the common mozconfig that is used by all mozconfigs (iiuc). I'm not using the common-override one though as I want to be able to flip MOZ_REQUIRE_ADDON_SIGNING off for things like Seamonkey or our newly upcoming disable-require-signing builds, etc: https://bugzilla.mozilla.org/attachment.cgi?id=8639624

finally, I also deleted the branch specific logic currently in confvars.sh. I think what we want to do is not rely on that but instead be explicit in the beta, release, and esr trees themselves.

So I was thinking of was:

1) landing something like https://bugzilla.mozilla.org/attachment.cgi?id=8639625 on m-c/m-a and then uplift it to esr.

2) then, landing something like the following on beta not on the next uplift but the one after that (Sept): https://bugzilla.mozilla.org/attachment.cgi?id=8639624

With the goal that (1) will be a no-op and we can land anywhere/anytime. and when (2) lands, will force all builds, unless specified in a leaf mozconfig, to require addon signing (MOZ_REQUIRE_ADDON_SIGNING) on m-b only. We will need to find out what seamonkey mozconfigs we need to patch to pref this off like we are doing in mozconfigs/linux64/nightly-no-add-on-sign

mossop: could you just let me know if we are still on the same page. I'd like to save you from having to do this work yourself but at least get your nod that this goal is correct

mshal: could you quickly poke the patches here and see if we are on the right track to my goal defined above.

rail: this will take some coordination with merges and also define some migration/release logic so that https://bugzilla.mozilla.org/attachment.cgi?id=8639624 stays on m-b after every migration no matter what m-a says and also gets uplifted to m-r/esr on every cycle. Will that require much work? I guess this is decided by 'whitelisting' and migration scripts?
Flags: needinfo?(rail)
Flags: needinfo?(mshal)
Flags: needinfo?(dtownsend)
Comment on attachment 8639437 [details] [diff] [review]
bug1186522.patch

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

commented in the bug
Attachment #8639437 - Flags: feedback?(jlund) → feedback+
That reminds me that the merge scripts live in mozharness. :)

Depending on what we want to do during merges, we'll need to change the configs, see http://hg.mozilla.org/build/mozharness/file/2982687f5fa8/configs/merge_day/aurora_to_beta.py#l4 as an example.

something like

config = {
    ...
    "replacements": [
        ...
        ("build/mozconfig.common",
         "export MOZ_REQUIRE_ADDON_SIGNING=1",
         "export MOZ_REQUIRE_ADDON_SIGNING=0"),
        ("build/mozconfig.common",
         "# force add-ons to be signed",
         "# disable add-ons signing"),
        ...
    ]
    ...
}


You may want to apply this to multiple files in http://hg.mozilla.org/build/mozharness/file/2982687f5fa8/configs/merge_day
Flags: needinfo?(rail)
This looks ok to me.
Flags: needinfo?(dtownsend)
Comment on attachment 8639625 [details] [diff] [review]
150727_1186522_force_addon_signing_per_checkin-disables-by-branch-check.patch

>diff --git a/configure.in b/configure.in
>--- a/configure.in
>+++ b/configure.in
>@@ -3913,19 +3913,19 @@ ACCESSIBILITY=1
> MOZ_TIME_MANAGER=
> MOZ_SIMPLEPUSH=
> MOZ_PAY=
> MOZ_AUDIO_CHANNEL_MANAGER=
> MOZ_CONTENT_SANDBOX=
> MOZ_GMP_SANDBOX=
> MOZ_SANDBOX=1
> MOZ_BINARY_EXTENSIONS=
> MOZ_ADDON_SIGNING=
>-MOZ_REQUIRE_SIGNING=
>+MOZ_REQUIRE_ADDON_SIGNING=${MOZ_REQUIRE_ADDON_SIGNING-0}

I don't think you need the ${MOZ_REQUIRE_ADDON_SIGNING-0} bit here. Since MOZ_REQUIRE_ADDON_SIGNING is already in the mozconfig, I think you can just AC_SUBST it (would need to test though). You could use a construct like 'MOZ_REQUIRE_ADDON_SIGNING=${MOZ_REQUIRE_ADDON_SIGNING-1}' in build/mozconfig.common if you wanted to have all builds default to MOZ_REQUIRE_ADDON_SIGNING=1, unless it was already explicitly set to 0 in something like the nightly-no-add-on-sign file. That's only necessary if nightly-no-add-on-sign includes mozconfig.common after MOZ_REQUIRE_ADDON_SIGNING is set, though - it may not be necessary here.
Flags: needinfo?(mshal)
Attachment #8639625 - Flags: feedback+
Comment on attachment 8639624 [details] [diff] [review]
150727_1186522_force_addon_signing_per_checkin-enables-on-all-mozconfigs.patch

>+export MOZ_REQUIRE_ADDON_SIGNING=0
>+export MOZ_REQUIRE_ADDON_SIGNING=1

Looks good, though I don't think you need the exports here.
Attachment #8639624 - Flags: feedback+
thanks mshal!


> I don't think you need the ${MOZ_REQUIRE_ADDON_SIGNING-0} bit here. Since
> MOZ_REQUIRE_ADDON_SIGNING is already in the mozconfig, I think you can just
> AC_SUBST it (would need to test though).

Ah so I was wrong about that and we should be doing what Kim has. 

> You could use a construct like
> 'MOZ_REQUIRE_ADDON_SIGNING=${MOZ_REQUIRE_ADDON_SIGNING-1}' in
> build/mozconfig.common if you wanted to have all builds default to
> MOZ_REQUIRE_ADDON_SIGNING=1, unless it was already explicitly set to 0 in
> something like the nightly-no-add-on-sign file. That's only necessary if
> nightly-no-add-on-sign includes mozconfig.common after
> MOZ_REQUIRE_ADDON_SIGNING is set, though - it may not be necessary here.

I had a look and it seemed like mozconfig.common was sourced early in all the leaf mozconfigs so it may not be necessary but to be sure to be sure, it is probably better to use the construct you suggest.

Kim, do you think it makes sense to use mozconfig.common and have it default REQUIRE_ADDON_SIGNING pref'd on. Then, pref'ing it off on the specific mozconfigs we choose? I argue this since it seems like we want this pref'd on for pretty much all ff/fennec builds on beta and there are a *lot* of leaf nodes to touch: http://gittup.org/blog/2015/01/12-combining-nodes-in-graphviz/mozconfigs.png
Jordan, yes that sounds like a good idea, I'll rework the patches

OMG that dependency graph!!!
this should be a no-op for existing ff and fennec builds. It also adds a linux64 mozconfig that will no matter what mozconfig.common says will override and disable requiring signed addons. If r+, I would land this on m-c and uplift across trees.

then, once beta hits 42, I can land on beta the following to actually flip on requiring all beta builds to have signed add-ons:
diff --git a/build/mozconfig.common b/build/mozconfig.common
--- a/build/mozconfig.common
+++ b/build/mozconfig.common
@@ -12,13 +12,13 @@
 
 mk_add_options AUTOCLOBBER=1
 
 ac_add_options --enable-crashreporter
 
 ac_add_options --enable-release
 
 # Enable checking that add-ons are signed by the trusted root
 MOZ_ADDON_SIGNING=1
-# Disable enforcing that add-ons are signed by the trusted root
-MOZ_REQUIRE_ADDON_SIGNING=0
+# Enable enforcing that add-ons are signed by the trusted root
+MOZ_REQUIRE_ADDON_SIGNING=1
 
 . "$topsrcdir/build/mozconfig.automation"


my staging master has this attachment and above paste on beta and behaves as I expect:

normal build:
[root@dev-linux64-ec2-012.dev.releng.use1.mozilla.com ~]# cd /builds/slave/m-beta-l64-0000000000000000000
[root@dev-linux64-ec2-012.dev.releng.use1.mozilla.com m-beta-l64-0000000000000000000]# grep MOZ_REQUIRE_ADDON_SIGNING build/obj-firefox/config.status
    (''' MOZ_REQUIRE_ADDON_SIGNING ''', ' 1 '),
[root@dev-linux64-ec2-012.dev.releng.use1.mozilla.com m-beta-l64-0000000000000000000]# grep MOZ_ADDON_SIGNING build/obj-firefox/config.status
    (''' MOZ_ADDON_SIGNING ''', ' 1 '),
[root@dev-linux64-ec2-012.dev.releng.use1.mozilla.com m-beta-l64-0000000000000000000]#


no-addon-sign build:
[root@dev-linux64-ec2-012.dev.releng.use1.mozilla.com ~]# cd /builds/slave/m-beta-l64-no-add-on-sign-0000
[root@dev-linux64-ec2-012.dev.releng.use1.mozilla.com m-beta-l64-no-add-on-sign-0000]# grep MOZ_ADDON_SIGNING build/obj-firefox/config.status
    (''' MOZ_ADDON_SIGNING ''', ' 1 '),
[root@dev-linux64-ec2-012.dev.releng.use1.mozilla.com m-beta-l64-no-add-on-sign-0000]# grep MOZ_REQUIRE_ADDON_SIGNING build/obj-firefox/config.status
[root@dev-linux64-ec2-012.dev.releng.use1.mozilla.com m-beta-l64-no-add-on-sign-0000]#

before this lands, we will also need to address seamonkey/tb and any build outside of ff that uses mozconfig.common. philip, kevin: would you be able to help me out with this. we are changing the logic from https://bugzil.la/1164168 and I believe you will need to either

1) add the following to all sm leaf mozconfigs like I've done in this patch's nightly-no-add-on-sign:
+MOZ_REQUIRE_ADDON_SIGNING=0
+MOZ_ADDON_SIGNING=0

2) override MOZ_REQUIRE_ADDON_SIGNING and MOZ_ADDON_SIGNING set in mozconfig.common in some common tb/sm mozconfig
Flags: needinfo?(philip.chee)
Flags: needinfo?(kevink9876543)
Attachment #8644137 - Flags: review?(mshal)
if r+, this won't be landed until beta hits 42

mshal: it looks like gps reviewed mossop's similar patches in https://bugzil.la/1163046 and https://bugzil.la/1164168 so feel free to bounce these if you like
Attachment #8644138 - Flags: review?(mshal)
rail: I believe this will be needed once my two previous patches land.
Attachment #8644139 - Flags: review?(rail)
@jlund: Thanks for adding me to the CC list, but I'm not one to ask about the official SeaMonkey builds because I know nothing about that system, sorry (and I mostly make my own builds anyway, so for me I'd just add those two lines to my own mozconfig and be done).
Flags: needinfo?(kevink9876543)
Attachment #8644139 - Flags: review?(rail) → review+
>+MOZ_REQUIRE_ADDON_SIGNING=0
>+MOZ_ADDON_SIGNING=0
Correct me if I am wrong but as long as we don't define either of these in our mozconfigs they are equivalent to:

MOZ_REQUIRE_ADDON_SIGNING=0
MOZ_ADDON_SIGNING=0
Flags: needinfo?(jlund)
Flags: needinfo?(philip.chee)
Is this work going to land on aurora before the merge? Otherwise we need to remove the code at https://dxr.mozilla.org/mozilla-central/source/browser/confvars.sh#69 or beta 41 will start failing
(In reply to Jordan Lund (:jlund) from comment #12)

> before this lands, we will also need to address seamonkey/tb and any build
> outside of ff that uses mozconfig.common. philip, kevin: would you be able
> to help me out with this. we are changing the logic from
> https://bugzil.la/1164168 and I believe you will need to either
> 
> 1) add the following to all sm leaf mozconfigs like I've done in this
> patch's nightly-no-add-on-sign:
> +MOZ_REQUIRE_ADDON_SIGNING=0
> +MOZ_ADDON_SIGNING=0
> 
> 2) override MOZ_REQUIRE_ADDON_SIGNING and MOZ_ADDON_SIGNING set in
> mozconfig.common in some common tb/sm mozconfig

FYI/HEADS-UP to SM and TB people too.
Flags: needinfo?(rkent)
Flags: needinfo?(iann_bugzilla)
Flags: needinfo?(bugspam.Callek)
Flags: needinfo?(Pidgeot18)
Comment on attachment 8644137 [details] [diff] [review]
150805_bug_1186522_use_mozconfigs_instead_of_confvars_and_branches.patch

Looks mostly good, though I think you've been bitrotted by a few patches to the mozconfigs since you started on this.

>diff --git a/browser/config/mozconfigs/linux64/nightly-no-add-on-sign b/browser/config/mozconfigs/linux64/nightly-no-add-on-sign
>+ac_add_options --enable-signmar

See bug 1158870 - I think this goes away and gets replaced by --enable-verify-mar

>+
>+

nit: Double newline

>+# Nightlies only since this has a cost in performance
>+ac_add_options --enable-js-diagnostics

See bug 1171904 - I think this line goes away.

>+# Use ccache
>+. "$topsrcdir/build/mozconfig.cache"
>+
>+. "$topsrcdir/build/mozconfig.common.override"

See bug 1181040 - mozconfig.cache needs to come after mozconfig.common.override

Basically, this mozconfig should be the same as 'nightly' but with those two variables set to 0, right? Up to you, but you could look into just including the nightly config and having build/mozconfig.common only setting the ADDON variables if they aren't already set. Eg:

nightly-no-addon-sign:
# addon signing is not required
MOZ_REQUIRE_ADDON_SIGNING=0
MOZ_ADDON_SIGNING=0
. $topsrcdir/browser/config/mozconfigs/linux64/nightly

build/mozconfig.common:
...
# Enable checking that add-ons are signed by the trusted root
MOZ_ADDON_SIGNING=${MOZ_ADDON_SIGNING-1}
# Enable enforcing that add-ons are signed by the trusted root
MOZ_REQUIRE_ADDON_SIGNING=${MOZ_REQUIRE_ADDON_SIGNING-1}

>diff --git a/toolkit/mozapps/extensions/internal/moz.build b/toolkit/mozapps/extensions/internal/moz.build
> if CONFIG['MOZ_ADDON_SIGNING']:
>     DEFINES['MOZ_ADDON_SIGNING'] = 1
> 
>-if CONFIG['MOZ_REQUIRE_SIGNING']:
>-    DEFINES['MOZ_REQUIRE_SIGNING'] = 1
>+if CONFIG['MOZ_REQUIRE_ADDON_SIGNING']:
>+    DEFINES['MOZ_REQUIRE_ADDON_SIGNING'] = 1

You should be able to delete all of these lines (both the MOZ_ADDON_SIGNING and MOZ_REQUIRE_ADDON_SIGNING), since you are using AC_DEFINE. Is it not working if you remove these?
Attachment #8644137 - Flags: review?(mshal) → feedback+
Attachment #8644138 - Flags: review?(mshal) → review+
Fallen, we'll need to modify the build setup for this at some point.
Flags: needinfo?(rkent)
(In reply to Philip Chee from comment #16)
> >+MOZ_REQUIRE_ADDON_SIGNING=0
> >+MOZ_ADDON_SIGNING=0
> Correct me if I am wrong but as long as we don't define either of these in
> our mozconfigs they are equivalent to:
> 
> MOZ_REQUIRE_ADDON_SIGNING=0
> MOZ_ADDON_SIGNING=0

depends if you source build/mozconfig.common because I am doing the following:

diff --git a/build/mozconfig.common b/build/mozconfig.common
--- a/build/mozconfig.common
+++ b/build/mozconfig.common
@@ -10,10 +10,15 @@
 # another mozconfig file, you'll need to use mozconfig.common.override instead
 # of this file.
 
 mk_add_options AUTOCLOBBER=1
 
 ac_add_options --enable-crashreporter
 
 ac_add_options --enable-release
 
+# Enable checking that add-ons are signed by the trusted root
+MOZ_ADDON_SIGNING=1
+# Disable enforcing that add-ons are signed by the trusted root
+MOZ_REQUIRE_ADDON_SIGNING=0
+

and on beta, I'll soon also be:
+MOZ_REQUIRE_ADDON_SIGNING=1

so you would need to override those vars (if they == 1) like I am in nightly-no-add-on-sign
Flags: needinfo?(jlund)
(In reply to Dave Townsend [:mossop] from comment #17)
> Is this work going to land on aurora before the merge? Otherwise we need to
> remove the code at
> https://dxr.mozilla.org/mozilla-central/source/browser/confvars.sh#69 or
> beta 41 will start failing

yes that is the goal. If it doesn't, I'll back out the android and desktop confvars branch lines on Aug 9th
Blocks: 1178507
(In reply to Kent James (:rkent) from comment #20)
> Fallen, we'll need to modify the build setup for this at some point.

I've filed bug 1191941 for the Thunderbird part.
Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:

because tb and sm haven't had a chance to change their logic yet and the fact that we uplift + release beta1 on monday, I'm going to simply remove the branch logic that would break beta releases then follow up with the remaining bits next week.

this patch needs to land and get uplifted to m-a before monday's merge.
Attachment #8645217 - Flags: review?(dtownsend)
Attachment #8645217 - Flags: approval-mozilla-aurora?
addresses previous comments. tested in staging.

interdiff:
http://people.mozilla.org/~jlund/addon_signing_review_feedback-interdiff.patch
Attachment #8639437 - Attachment is obsolete: true
Attachment #8639624 - Attachment is obsolete: true
Attachment #8639625 - Attachment is obsolete: true
Attachment #8644137 - Attachment is obsolete: true
Attachment #8645219 - Flags: review?(mshal)
Attachment #8645217 - Flags: review?(dtownsend) → review+
Comment on attachment 8645217 [details] [diff] [review]
150807_bug_1186522_remove_beta_release_branch_logic_for_require_addon_signing.patch

thanks, https://hg.mozilla.org/integration/mozilla-inbound/rev/a6f105aba8d2
Attachment #8645217 - Flags: checked-in+
Keywords: leave-open
Comment on attachment 8645217 [details] [diff] [review]
150807_bug_1186522_remove_beta_release_branch_logic_for_require_addon_signing.patch

Approved.
Attachment #8645217 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Ritu Kothari (:ritu) from comment #29)
> Comment on attachment 8645217 [details] [diff] [review]
> 150807_bug_1186522_remove_beta_release_branch_logic_for_require_addon_signing
> .patch
> 
> Approved.

thanks https://hg.mozilla.org/releases/mozilla-aurora/rev/7a647fc4d0ba
No longer blocks: 1178507
Comment on attachment 8645219 [details] [diff] [review]
150807_bug_1186522_use_mozconfigs_instead_of_confvars_and_branches.patch

>diff --git a/browser/config/mozconfigs/linux64/nightly-no-add-on-sign b/browser/config/mozconfigs/linux64/nightly-no-add-on-sign
>new file mode 100644
>--- /dev/null
>+++ b/browser/config/mozconfigs/linux64/nightly-no-add-on-sign
>@@ -0,0 +1,4 @@
>+# addon signing is checked but not required
>+MOZ_REQUIRE_ADDON_SIGNING=0
>+MOZ_ADDON_SIGNING=1

These were both 0 in https://bug1186522.bmoattachments.org/attachment.cgi?id=8644137 - did you intend to change MOZ_ADDON_SIGNING to 1 here?

>diff --git a/build/mozconfig.common b/build/mozconfig.common
>+# Enable checking that add-ons are signed by the trusted root
>+MOZ_ADDON_SIGNING=${MOZ_ADDON_SIGNING-1}
>+# Disable enforcing that add-ons are signed by the trusted root
>+MOZ_REQUIRE_ADDON_SIGNING=${MOZ_REQUIRE_ADDON_SIGNING-1}

Similarly, in https://bug1186522.bmoattachments.org/attachment.cgi?id=8644137 you had MOZ_REQUIRE_ADDON_SIGNING default to 0 in mozconfig.common. I think you wanted to have it 0 here and then changed to 1 in https://bug1186522.bmoattachments.org/attachment.cgi?id=8645219 right? So you'd have: 

MOZ_REQUIRE_ADDON_SIGNING=${MOZ_REQUIRE_ADDON_SIGNING-0}

Everything else looks fine though, just wanted to call those out in case they are not intentional :)
Attachment #8645219 - Flags: review?(mshal) → review+
> >--- /dev/null
> >+++ b/browser/config/mozconfigs/linux64/nightly-no-add-on-sign
> >@@ -0,0 +1,4 @@
> >+# addon signing is checked but not required
> >+MOZ_REQUIRE_ADDON_SIGNING=0
> >+MOZ_ADDON_SIGNING=1
> 
> These were both 0 in
> https://bug1186522.bmoattachments.org/attachment.cgi?id=8644137 - did you
> intend to change MOZ_ADDON_SIGNING to 1 here?

I intended this. I realized that checking if addon's were signed (MOZ_ADDON_SIGNING) is not a big deal. as it stands, that should be 1 across all our platforms already. MOZ_REQUIRE_ADDON_SIGNING is the main one we care about and I'd like to explicitly set that to 0 for this mozconfig.

> 
> >diff --git a/build/mozconfig.common b/build/mozconfig.common
> >+# Enable checking that add-ons are signed by the trusted root
> >+MOZ_ADDON_SIGNING=${MOZ_ADDON_SIGNING-1}
> >+# Disable enforcing that add-ons are signed by the trusted root
> >+MOZ_REQUIRE_ADDON_SIGNING=${MOZ_REQUIRE_ADDON_SIGNING-1}
> 
> Similarly, in
> https://bug1186522.bmoattachments.org/attachment.cgi?id=8644137 you had
> MOZ_REQUIRE_ADDON_SIGNING default to 0 in mozconfig.common.

oops, I got confused about this one. for now, this patch should be:
+# Enable checking that add-ons are signed by the trusted root
+MOZ_ADDON_SIGNING=${MOZ_ADDON_SIGNING-1}
+# Disable enforcing that add-ons are signed by the trusted root
+MOZ_REQUIRE_ADDON_SIGNING=${MOZ_REQUIRE_ADDON_SIGNING-0}

then override that on beta with https://bug1186522.bmoattachments.org/attachment.cgi?id=8644138 <- and this patch needs updating too to reflect ${MOZ_REQUIRE_ADDON_SIGNING-1} style
carrying r+

interdiff:
diff --git a/build/mozconfig.common b/build/mozconfig.common
--- a/build/mozconfig.common
+++ b/build/mozconfig.common
@@ -13,12 +13,12 @@
 mk_add_options AUTOCLOBBER=1

 ac_add_options --enable-crashreporter

 ac_add_options --enable-release

 # Enable checking that add-ons are signed by the trusted root
 MOZ_ADDON_SIGNING=${MOZ_ADDON_SIGNING-1}
 # Disable enforcing that add-ons are signed by the trusted root
-MOZ_REQUIRE_ADDON_SIGNING=${MOZ_REQUIRE_ADDON_SIGNING-1}
+MOZ_REQUIRE_ADDON_SIGNING=${MOZ_REQUIRE_ADDON_SIGNING-0}

 . "$topsrcdir/build/mozconfig.automation"
Attachment #8645219 - Attachment is obsolete: true
Attachment #8646044 - Flags: review+
fighting bitrot
Attachment #8644138 - Attachment is obsolete: true
Attachment #8646045 - Flags: review?(mshal)
Attachment #8646045 - Flags: review?(mshal) → review+
fyi tb and sm folks - I'll be landing https://bugzilla.mozilla.org/attachment.cgi?id=8646044 tomorrow and looking to uplift that to beta by eow

seems like there is a patch in place or at least attached in https://bugzil.la/1191941 for tb case.
Comment on attachment 8646044 [details] [diff] [review]
150810_bug_1186522_use_mozconfigs_instead_of_confvars_and_branches.patch

Approval Request Comment
[Feature/regressing bug #]: this one: bug 1186522
[User impact if declined]: this lays the pipework for enforcing add-on signing in the future. this patch is required for when we actually change current pref values.
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: this patch should be a no-op. it simply rearranges where we set the enforcement and check for add-on signing. it does not change any mozconfig prefs
[String/UUID change made/needed]:

https://hg.mozilla.org/integration/mozilla-inbound/rev/292d13beeb7b
Attachment #8646044 - Flags: checked-in+
Attachment #8646044 - Flags: approval-mozilla-beta?
Attachment #8646044 - Flags: approval-mozilla-aurora?
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #38)
> Backed out for breaking Gaia unit tests.
> https://treeherder.mozilla.org/logviewer.html#?job_id=13433430&repo=mozilla-
> inbound
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f93f561fb199

hrm, I'll look into this today.
Comment on attachment 8646044 [details] [diff] [review]
150810_bug_1186522_use_mozconfigs_instead_of_confvars_and_branches.patch

Aurora-, Beta- due to the back outs. Please renominate once all is well.
Attachment #8646044 - Flags: approval-mozilla-beta?
Attachment #8646044 - Flags: approval-mozilla-beta-
Attachment #8646044 - Flags: approval-mozilla-aurora?
Attachment #8646044 - Flags: approval-mozilla-aurora-
2 parts in this:

1) fights some bitrot where we added/changed MOZ_REQUIRE_SIGNING/MOZ_ADDON_SIGNING bits.
   * the part I'm worried about here is changing browser/components/moz.build. But I think removing the code in there is safe like we did previously for toolkit/mozapps/extensions/internal/moz.build
   * interdiff: //people.mozilla.org/~jlund/disable_moz_addon_signing_for_all_b2g.patch

2) disable MOZ_ADDON_SIGNING for all of b2g/* as I guess the b2g desktop builds picked up the common mozconfig change I did and we don't want to enforce/check addon signing for those afaik
   * interdiff: http://people.mozilla.org/~jlund/disable_moz_addon_signing_for_all_b2g.patch

treeherder:
before: https://treeherder.mozilla.org/#/jobs?repo=try&revision=23304a6590ff
after: https://treeherder.mozilla.org/#/jobs?repo=try&revision=05dcebc63e3a
Attachment #8646044 - Attachment is obsolete: true
Attachment #8656163 - Flags: review?
Unfortunately we've decided to push forced signing out to 44 at the earliest. What needs to change here?
Flags: needinfo?(jlund)
Flags: needinfo?(Pidgeot18)
(In reply to Dave Townsend [:mossop] from comment #42)
> Unfortunately we've decided to push forced signing out to 44 at the
> earliest. What needs to change here?

afaik, nothing. we can proceed landing https://bugzilla.mozilla.org/attachment.cgi?id=8656163 which has been on hold due to unknown future.

we just basically won't be landing https://bugzilla.mozilla.org/attachment.cgi?id=8646045 on beta this merge but instead wait for 44
Flags: needinfo?(jlund)
> afaik, nothing. we can proceed landing
> https://bugzilla.mozilla.org/attachment.cgi?id=8656163 which has been on
> hold due to unknown future.

actually, we might not even need this at all if we end up going with a developer mode pref and a single builder/job for enforced-signing and not-forced in continuous integration.

I'll wait to see how the add-on mtg goes tomorrow.
Summary: force per checkin desktop and mobile firefox builds to require signed add-ons on beta 42 → force per checkin desktop and mobile firefox builds to require signed add-ons on beta 44
we don't need to land the patch yet so I am going to leave any cosmetic mozconfig changes till after merge/release week. we already took out the enforcement back in https://hg.mozilla.org/releases/mozilla-beta/rev/7a647fc4d0ba
Flags: needinfo?(iann_bugzilla) → needinfo?(ewong)
Blocks: 1219507
Signing being enabled, but allowed to be turned off has landed in 43. The plan was to force signing in 44, is there an update on this?
(In reply to Andy McKay [:andym] from comment #46)
> Signing being enabled, but allowed to be turned off has landed in 43. The
> plan was to force signing in 44, is there an update on this?
Flags: needinfo?(jlund)
keeping needinfo open on this bug. discussing this in a mtg today. will report back by EOD
after meeting today, we are going to be enforcing addon signing the per-checkin and release builds for beta 45. Getting back up to speed, this bug requires:

* a mozconfig change that flips MOZ_REQUIRE_SIGNING to 1 for desktop ff only on m-b.
** we should also change this to MOZ_REQUIRE_ADDON_SIGNING to not confuse with the actual ff signing itself
* a migration/uplift patch similar to: https://bug1186522.bmoattachments.org/attachment.cgi?id=8644139
Flags: needinfo?(jlund)
Summary: force per checkin desktop and mobile firefox builds to require signed add-ons on beta 44 → force per checkin and release desktop firefox builds to require signed add-ons on beta 45
Blocks: 1233199
Blocks: 1233200
Flags: needinfo?(bugspam.Callek)
For some reason Bugzilla made an unwarrented removal for me. Justin, see comment #18
Flags: needinfo?(bugspam.Callek)
(In reply to Philip Chee from comment #18)
> (In reply to Jordan Lund (:jlund) from comment #12)
> 
> > before this lands, we will also need to address seamonkey/tb and any build
> > outside of ff that uses mozconfig.common. philip, kevin: would you be able
> > to help me out with this. we are changing the logic from
> > https://bugzil.la/1164168 and I believe you will need to either
> > 
> > 1) add the following to all sm leaf mozconfigs like I've done in this
> > patch's nightly-no-add-on-sign:
> > +MOZ_REQUIRE_ADDON_SIGNING=0
> > +MOZ_ADDON_SIGNING=0
> > 
> > 2) override MOZ_REQUIRE_ADDON_SIGNING and MOZ_ADDON_SIGNING set in
> > mozconfig.common in some common tb/sm mozconfig
> 
> FYI/HEADS-UP to SM and TB people too.

I'm aware, thanks.
Flags: needinfo?(bugspam.Callek)
Seems like there's confusion in the community, and even among Firefox developers, what the actual plan is:

https://groups.google.com/d/msg/firefox-dev/wR5MvjIIPV0/yEP7MGT9DwAJ

Should we be removing the bits about "special unbranded builds" on beta/release from https://wiki.mozilla.org/Addons/Extension_Signing#FAQ ? I thought that was off the table now.
(In reply to Chris Cooper [:coop] from comment #52)
> Seems like there's confusion in the community, and even among Firefox
> developers, what the actual plan is:
> 
> https://groups.google.com/d/msg/firefox-dev/wR5MvjIIPV0/yEP7MGT9DwAJ
> 
> Should we be removing the bits about "special unbranded builds" on
> beta/release from https://wiki.mozilla.org/Addons/Extension_Signing#FAQ ? I
> thought that was off the table now.

The unbranded builds are still planned. Aiming for a clarifying blog post soon.
andym: Could you provide insight on the scope of the unbranded builds?  I seem to recall in a recent meeting that that the intent would be they would be on a periodic basis, but not on every commit. I looked for a blog post but didn't see one and catlee asked me about this today:-)
Flags: needinfo?(amckay)
301 over to kev who has been chatting to people about this
Flags: needinfo?(kev)
Scope of the unbranded builds would be installers based off what we test and release, but with official branding turned off, signing preference enabled, and a channel identified. No manual QA, and supplied on an as-is basis. No MAR generation for partial updates, and I don't know if anything's been discussed around whether we'd offer updates per release or expect users to update manually (this would be limited to update server config, we'd want to ensure the product checked for updates).
Flags: needinfo?(kev)
kev, thanks for the clarification. We read your comments in the addons meeting and have some further questions.  So we would envision these as similar to partner builds they are a one-off are generated each  point release on stable, with the two signing preference flipped to unsigned addons could be installed? And just for Windows, Windows 64-bit, Mac OS X, Linux and Linux 64-bit?
Flags: needinfo?(kev)
Some more questions after discussions with coop

Should the build be unbranded? i.e. is a build without MOZ_OFFICIAL_BRANDING sufficient

What parts of the release process they need to be generated for (aurora/beta/release)
Whiteboard: [releng]
(In reply to Kim Moir [:kmoir] from comment #58)
> Some more questions after discussions with coop
> 
> Should the build be unbranded? i.e. is a build without MOZ_OFFICIAL_BRANDING
> sufficient

The build should be unbranded, per the config you outline.

> What parts of the release process they need to be generated for
> (aurora/beta/release)

They'd need to be available for beta and release. Nightly and Aurora maintain the pref already, but when we switch to mandatory the xpinstall.signatures.required will be ignored (this is, I believe, a mozconfig pref). As a result, we wouldn't be creating a partner build, we'd use the build flags to remove official branding and to keep the signing pref something that a developer could still flip to test against release-based code.
Flags: needinfo?(kev)
Talked to coop about this last week, he suggested that we should just land the configs for this on a try run in tree and get addons team to validate that this is what they are looking for before adding all the automation to generate builds that won't be widely used.
Flags: needinfo?(amckay)
email from coop on this

dburns is interested in this build for other use cases


"As a starting point, we'll need to set the branding flag, something like:

ac_add_options --with-branding=browser/branding/unofficial

You can check one of the existing build logs on buildbot/treeherder
for a proper example.

Whether that actually sets the proper addon flag, I don't know. That's
what we need to figure out, and possibly change."
updated patch to require addon signing on beta
unbitrotten patch
Attachment #8726314 - Attachment is obsolete: true
Trying another try build here https://treeherder.mozilla.org/#/jobs?repo=try&revision=58e909082aa5 from beta
Attachment #8726264 - Attachment is obsolete: true
Trying another builder to fix branding and disable signing preference more thoroughly
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3dc48cc0686e
Attachment #8726317 - Attachment is obsolete: true
So this looks better, preference is disabled but branding is still nightly. Is this expected?  Lots of test failures, some related to addons.  Not sure if this is because I used mozilla-beta and pushed to try.
Here is the build from yesterday that I created on try from the latest mozilla-beta code
http://archive.mozilla.org/pub/firefox/try-builds/kmoir@mozilla.com-3dc48cc0686ee5f65836bb4aa6ed8e5d4145955b/try-macosx64/firefox-46.0.en-US.mac.dmg

If someone could validate that this reflects the content that we want to run on CI on a periodic basis, that would be helpful.  Then I'll add it to our configs.
As a note, you'll have to go into settings on your mac to be able to install it since it's not signed.  
System preferences -> Security & Privacy -> General 

temporarily set the preference to be 
Allow apps to be downloaded from Anywhere

to install the dmg

otherwise you'll get a message that the dmg is corrupt
Kev, I was talking to coop yesterday and he said you would have insight about the unbranded build should look like.  What is the expectation from a legal perspective?  Right now, I just have a build built from beta code that that has nightly branding but with the addon signing preferences disabled
Flags: needinfo?(kev)
That sounds perfect. Main thing is that we don't want to use official branding, lest there be confusion with release. 

(In reply to Kim Moir [:kmoir] from comment #72)
> Kev, I was talking to coop yesterday and he said you would have insight
> about the unbranded build should look like.  What is the expectation from a
> legal perspective?  Right now, I just have a build built from beta code that
> that has nightly branding but with the addon signing preferences disabled
Flags: needinfo?(kev)
So does this mean I can't use unsigned extensions anymore in Nightly?
(In reply to Martijn Wargers [:mwargers] (QA) from comment #74)
> So does this mean I can't use unsigned extensions anymore in Nightly?

No, this only applies to beta and release builds.
(In reply to Dave Townsend [:mossop] from comment #75)
> (In reply to Martijn Wargers [:mwargers] (QA) from comment #74)
> > So does this mean I can't use unsigned extensions anymore in Nightly?
> 
> No, this only applies to beta and release builds.

I'm trying to use the Special Powers extension in Nightly, which used to work for years, but recently stopped working without any error messages in the console.
You're saying that this isn't happening because of disabling unsigned extensions?
Flags: needinfo?(dtownsend)
(In reply to Martijn Wargers [:mwargers] (QA) from comment #76)
> (In reply to Dave Townsend [:mossop] from comment #75)
> > (In reply to Martijn Wargers [:mwargers] (QA) from comment #74)
> > > So does this mean I can't use unsigned extensions anymore in Nightly?
> > 
> > No, this only applies to beta and release builds.
> 
> I'm trying to use the Special Powers extension in Nightly, which used to
> work for years, but recently stopped working without any error messages in
> the console.
> You're saying that this isn't happening because of disabling unsigned
> extensions?

I'm saying you can still turn off signing requirements in nightly by setting xpinstall.signatures.required to false same as you have been able to since we turned this feature on. I don't know what the Special Powers extensions is so I can't really say more than that.
Flags: needinfo?(dtownsend)
(In reply to Dave Townsend [:mossop] from comment #77)
> I'm saying you can still turn off signing requirements in nightly by setting
> xpinstall.signatures.required to false same as you have been able to since
> we turned this feature on. I don't know what the Special Powers extensions
> is so I can't really say more than that.

It's used for mochitests and more, it works there by doing a temporary install from the Addon Manager or something.
Setting the xpinstall.signatures.required pref to false is the first thing I did, which doesn't help.
Bug 1186522 seems to indicate it should work, but it doesn't for me.
(In reply to Martijn Wargers [:mwargers] (QA) from comment #78)
> (In reply to Dave Townsend [:mossop] from comment #77)
> > I'm saying you can still turn off signing requirements in nightly by setting
> > xpinstall.signatures.required to false same as you have been able to since
> > we turned this feature on. I don't know what the Special Powers extensions
> > is so I can't really say more than that.
> 
> It's used for mochitests and more, it works there by doing a temporary
> install from the Addon Manager or something.
> Setting the xpinstall.signatures.required pref to false is the first thing I
> did, which doesn't help.
> Bug 1186522 seems to indicate it should work, but it doesn't for me.

Please either file another bug or bring this up in a newgroup where we can figure this out without cluttering up this bug.
Attached patch bug1186522m-b.patch (obsolete) — Splinter Review
patch for m-b to enable addon signing

Will have to update merge scripts so this is not overwritten upon uplift
Attachment #8737333 - Flags: review?(dtownsend)
Attachment #8726376 - Attachment is obsolete: true
Comment on attachment 8737333 [details] [diff] [review]
bug1186522m-b.patch

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

I'm not sure that this is right since MOZ_REQUIRE_ADDON_SIGNING doesn't appear anywhere in the tree. Should it be MOZ_REQUIRE_SIGNING or is one of the other patches in this bug that changes to MOZ_REQUIRE_ADDON_SIGNING landing too?

Either way I'd prefer a build config peer to sign off on this as I'm not totally sure on the syntax.
Attachment #8737333 - Flags: review?(dtownsend)
Attached patch bug1186522m-b-2.patch (obsolete) — Splinter Review
Attachment #8737333 - Attachment is obsolete: true
Assignee: jlund → kmoir
Attached patch bug1186522m-i.patch (obsolete) — Splinter Review
Patch for mozilla-inbound to redefine MOZ_REQUIRE_SIGNING in a common mozconfig file on mozilla-inbound.  Once this is uplifted, we can land the next patch on m-b to enable the requirement for signed addons on mozilla-beta desktop builds, but disabled for android.  There is also a patch I need to write for the merge scripts so the settings on beta aren't overwritten.
Attachment #8737356 - Attachment is obsolete: true
Attachment #8738001 - Flags: feedback?(mshal)
Attached patch bug1186522m-b.patch (obsolete) — Splinter Review
patch for m-b

as an aside, this is just to enable forcing addons to be signed

The unbranded builds with signing disabled will be in later patches

:mshal: jlund suggested you would be a good resource for feedback
Attachment #8738002 - Flags: feedback?(mshal)
Comment on attachment 8738001 [details] [diff] [review]
bug1186522m-i.patch

Overall it looks good to me.

>+if test "$MOZ_ADDON_SIGNING" = 1; then
>+  AC_DEFINE(MOZ_ADDON_SIGNING)
>+fi
>+if test "$MOZ_REQUIRE_SIGNING" = 1; then
>+  AC_DEFINE(MOZ_REQUIRE_SIGNING)
>+fi

Nowadays we would want this logic to go into moz.configure as a python script. However, since you need to uplift to beta, putting this in old-configure.in for now is fine. (For uplifting, you'll have to re-apply the change to configure.in). Just a heads up depending on when this lands.

Also, you can get rid of this moz.build line:

https://dxr.mozilla.org/mozilla-central/rev/d9f50aa0a1aaf90499b85c31e0f329b762e80fdd/toolkit/modules/moz.build#127

Since that is just defining MOZ_REQUIRE_SIGNING, but you're using AC_DEFINE which already does that. (The other three variables in the loop need to stay I'd guess).
Attachment #8738001 - Flags: feedback?(mshal) → feedback+
Comment on attachment 8738002 [details] [diff] [review]
bug1186522m-b.patch

Seems straightforward.
Attachment #8738002 - Flags: feedback?(mshal) → feedback+
Attached patch bug1186522m-i-2.patch (obsolete) — Splinter Review
Attachment #8738001 - Attachment is obsolete: true
Attachment #8738250 - Flags: review?(mshal)
Attachment #8738002 - Flags: review?(mshal)
Attachment #8738250 - Flags: review?(mshal) → review+
Attachment #8738002 - Flags: review?(mshal) → review+
This should land at leastfew hours before we intend to start the beta 10 build on Monday.
Attachment #8738250 - Flags: checked-in+
Backed it out harder in https://hg.mozilla.org/integration/mozilla-inbound/rev/4eb7a77cd764 because the original backout somehow managed to leave the signing requirement enabled, causing Mn tests to fail like https://treeherder.mozilla.org/logviewer.html#?job_id=25532084&repo=mozilla-inbound
(In reply to Wes Kocher (:KWierso) from comment #92)
> Backed it out harder in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/4eb7a77cd764 because
> the original backout somehow managed to leave the signing requirement
> enabled, causing Mn tests to fail like
> https://treeherder.mozilla.org/logviewer.html#?job_id=25532084&repo=mozilla-
> inbound

Er, the original backout ended up removing the signing requirement, breaking a test that expected unsigned addons to fail, I guess.
(In reply to Kev Needham [:kev] from comment #73)
> That sounds perfect. Main thing is that we don't want to use official
> branding, lest there be confusion with release. 
> 
> (In reply to Kim Moir [:kmoir] from comment #72)
> > Kev, I was talking to coop yesterday and he said you would have insight
> > about the unbranded build should look like.  What is the expectation from a
> > legal perspective?  Right now, I just have a build built from beta code that
> > that has nightly branding but with the addon signing preferences disabled

Does that mean that all the unbranded builds will be branded "Nightly"?
(In reply to Dave Townsend [:mossop] from comment #79)
> (In reply to Martijn Wargers [:mwargers] (QA) from comment #78)
> > (In reply to Dave Townsend [:mossop] from comment #77)
> > > I'm saying you can still turn off signing requirements in nightly by setting
> > > xpinstall.signatures.required to false same as you have been able to since
> > > we turned this feature on. I don't know what the Special Powers extensions
> > > is so I can't really say more than that.
> > 
> > It's used for mochitests and more, it works there by doing a temporary
> > install from the Addon Manager or something.
> > Setting the xpinstall.signatures.required pref to false is the first thing I
> > did, which doesn't help.
> > Bug 1186522 seems to indicate it should work, but it doesn't for me.
> 
> Please either file another bug or bring this up in a newgroup where we can
> figure this out without cluttering up this bug.

Filed bug 1263931.
(In reply to Mike Kaply [:mkaply] from comment #94)
> (In reply to Kev Needham [:kev] from comment #73)
> > That sounds perfect. Main thing is that we don't want to use official
> > branding, lest there be confusion with release. 
> > 
> > (In reply to Kim Moir [:kmoir] from comment #72)
> > > Kev, I was talking to coop yesterday and he said you would have insight
> > > about the unbranded build should look like.  What is the expectation from a
> > > legal perspective?  Right now, I just have a build built from beta code that
> > > that has nightly branding but with the addon signing preferences disabled
> 
> Does that mean that all the unbranded builds will be branded "Nightly"?

Mike raises a good point, and I made an assumption Kim was referring to the logo and visuals, which are the same as Nightly. For the avoidance of doubt, I want to make sure we're using the "unofficial" branding flag, which will incorporate logos that are the same as nightly, but use the "Mozilla Developer Preview" and associated branding in assets/info from http://mxr.mozilla.org/mozilla-central/source/browser/branding/unofficial/
Also in the interests of clarity, the plan of record is to allow a full beta cycle of testing before this hits release. Modifying the subject to make that clearer; signing enforcement will hit release in 47, so should be enabled for beta 47 at the latest, with unofficial builds available for testing.
Summary: force per checkin and release desktop firefox builds to require signed add-ons on beta 45 → force per checkin and release desktop firefox builds to require signed add-ons on beta 47
I tried these patches on try and the xpcshell tests failed, among others.  Ahal do you have any insight into why this might be happening
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4bd9b610af16202886ba9c8d73c18763db0ef798&selectedJob=19493604
Flags: needinfo?(ahalberstadt)
Sure, it's been awhile since the tests were greened up, and new bustages have likely slipped in. I'd say disable with extreme prejudice on beta-only and file a bug to fix it on all branches. I'm going to be on extended PTO starting this afternoon, so won't be able to help unfortunately. I'll be back in May.

Bug 1259528 is filed to stop setting the pref to disable signing on central so new dependencies on unsigned addons stop slipping in. But even that has proven tricky and is temporarily stalled out (I'll pick it back up when I get back).
Flags: needinfo?(ahalberstadt)
Depends on: 1264994
Any updates? Will we be landing this for 47 beta?
> Any updates? Will we be landing this for 47 beta?

After the mess around expired add-ons, I'm wondering if it should be delayed again...
:Kev I'm trying to get the tests to pass on trunk on beta/m-i with the preference enabled.  Right now, there are either problems with the mozconfigs and possibly all the test fixes did not land on beta.  So I can't land this until the failing tests are green again. See bug 1264994 for details

mshal:  Do you have any insight on to why the patches aren't enabling signing.  See https://bugzilla.mozilla.org/show_bug.cgi?id=1264994#c13 for details.  I'm really not that familiar with the build side of things and I don't know why the preference is not being enabled as expected.
Flags: needinfo?(mshal)
From mshal in #releng

3:24 PM <mshal> kmoir: looks like we need both the AC_SUBST and AC_DEFINE, so something like this: https://pastebin.mozilla.org/8868922
3:24 PM <mshal> I didn't realize mozinfo.py was using the substs, I thought it was just used for preprocessor defines
3:25 PM <kmoir> mshal: thanks - will try that
3:25 PM <mshal> okie, let me know how it goes!

diff --git a/old-configure.in b/old-configure.in
index ab08949..a279b4a 100644
--- a/old-configure.in
+++ b/old-configure.in
@@ -6762,9 +6762,11 @@ if test -n "$MOZ_BINARY_EXTENSIONS"; then
   AC_DEFINE(MOZ_BINARY_EXTENSIONS)
 fi
 
+AC_SUBST(MOZ_ADDON_SIGNING)
 if test "$MOZ_ADDON_SIGNING" = 1; then
   AC_DEFINE(MOZ_ADDON_SIGNING)
 fi
+AC_SUBST(MOZ_REQUIRE_SIGNING)
 if test "$MOZ_REQUIRE_SIGNING" = 1; then
   AC_DEFINE(MOZ_REQUIRE_SIGNING)
 fi
Flags: needinfo?(mshal)
Attachment #8738250 - Attachment is obsolete: true
new patch for m-b now that try run on m-c looks good
Attachment #8738002 - Attachment is obsolete: true
Comment on attachment 8745521 [details] [diff] [review]
bug1186522m-i-3.patch

Thanks Mike, your suggestion yesterday fixed the problem!
Attachment #8745521 - Flags: review?(mshal)
Running another try run because w-e10s 4 failed in previous try run on m-i
https://treeherder.mozilla.org/#/jobs?repo=try&revision=25b192dd5a5f650cc29d7a4b0b4c3f5d4ce7e3f2
Attachment #8745521 - Flags: review?(mshal) → review+
So the try run I ran last night on beta had several test failures which I will investigate and see if this is related to test fixes that were not uplifted.  

Another issue is for this build is that I can still toggle xpinstall.signatures.required in about:config in this build.  I thought this would be disabled so users can't change the preference

https://treeherder.mozilla.org/#/jobs?repo=try&revision=25b192dd5a5f650cc29d7a4b0b4c3f5d4ce7e3f2

This morning I ran another try run 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e175d0b7abab2464b11bdbf7791c880b59fac1ec

this patch
https://bugzilla.mozilla.org/show_bug.cgi?id=1264994#c20

to ensure the preferences are set correctly
From this log 
http://archive.mozilla.org/pub/firefox/try-builds/kmoir@mozilla.com-e175d0b7abab2464b11bdbf7791c880b59fac1ec/try-macosx64/try-macosx64-bm87-try1-build16315.txt.gz
it appears that 
 Exception: {'require_signing': '1', 'addon_signing': '1'}
are both set.  


Is there something that needs to be done besides this in to disable the preference?

diff --git a/build/mozconfig.common b/build/mozconfig.common
--- a/build/mozconfig.common
+++ b/build/mozconfig.common
@@ -11,9 +11,14 @@
 # of this file.
 
 mk_add_options AUTOCLOBBER=1
 
 ac_add_options --enable-crashreporter
 
 ac_add_options --enable-release
 
+# Enable checking that add-ons are signed by the trusted root
+MOZ_ADDON_SIGNING=${MOZ_ADDON_SIGNING-1}
+# Enable enforcing that add-ons are signed by the trusted root
+MOZ_REQUIRE_SIGNING=${MOZ_REQUIRE_SIGNING-1}
Flags: needinfo?(dtownsend)
The preference can be toggled if it's defined, but the change in the build should cause Firefox to ignore it (e.g. changing the pref will have no effect).
(In reply to Kim Moir [:kmoir] from comment #110)
> So the try run I ran last night on beta had several test failures which I
> will investigate and see if this is related to test fixes that were not
> uplifted.  
> 
> Another issue is for this build is that I can still toggle
> xpinstall.signatures.required in about:config in this build.  I thought this
> would be disabled so users can't change the preference

What Kev said. We can't stop users toggling preferences. With MOZ_REQUIRE_SIGNING set we should just ignore the pref entirely.
Flags: needinfo?(dtownsend)
Comment on attachment 8745521 [details] [diff] [review]
bug1186522m-i-3.patch

Try run showed w4 e10s tests passed
https://treeherder.mozilla.org/#/jobs?repo=try&revision=25b192dd5a5f650cc29d7a4b0b4c3f5d4ce7e3f2

landed this patch on m-i
Attachment #8745521 - Flags: checked-in+
Thanks Kev and mossop, for the clarification. I'll look at the test failures on beta and see what we can do to resolve them. I hope that most of them are just from test fixes from ahal that haven't been uplifted yet.
Attachment #8745809 - Flags: review?(mshal)
So my try run from yesterday on m-b was okay except for failures on M other

https://treeherder.mozilla.org/#/jobs?repo=try&author=kmoir@mozilla.com&selectedJob=20056985

opened bug 1268545 - for those issues
Depends on: 1268545
Attachment #8745809 - Flags: review?(mshal) → review+
rerunning a try run with the patches in bug 1256399 to ensure that the tests pass so once it lands we can ask for approval for the patches in this bug to enable the disabling of the pref

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a41428236474d805ebd7cd7ace5b4e8e587bfd21
Comment on attachment 8745809 [details] [diff] [review]
bug1186522m-b-2.patch

Approval Request Comment
[Feature/regressing bug #]:1186522
[User impact if declined]: preference to install unsign addons can continue to be toggled by users
[Describe test coverage new/current, TreeHerder]: N/A, this is already covered by existing tests
[Risks and why]: enabling addon signing by default in all automated testing is wanted for 47
[String/UUID change made/needed]: none
Attachment #8745809 - Flags: approval-mozilla-beta?
Depends on: 1256399
Comment on attachment 8745809 [details] [diff] [review]
bug1186522m-b-2.patch

Related to add-on signing required by default, Beta47+
Attachment #8745809 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8745809 - Flags: checked-in+
Depends on: 1269498
In addition to bug 1269498's failures, this also appears to have broken all android mochitests and reftests in similar ways. Backed out in https://hg.mozilla.org/releases/mozilla-beta/rev/6f82d30fe05e
Triggered android tests on your latest try run to see if they fail, too, but I'm pretty sure they will.
Flags: needinfo?(kmoir)
And indeed those manually triggered android tests are failing just like they were on beta.
I'm sorry, will run some more try runs to see the root cause of the problem. Thank you for backing it out.
Flags: needinfo?(kmoir)
Attachment #8745809 - Flags: checked-in+ → checked-in-
I ran another try run this morning to try to address the failing android tests.  And it still  is all orange.   
https://treeherder.mozilla.org/#/jobs?repo=try&revision=17225f8079db

I'm really stuck on this issue.  I think the root cause is probably that the android builds have that MOZ_REQUIRE_SIGNING preference on which is causing the tests to fail.  Since my understanding is that the work was not done to fix the android tests to work with signed addons.  Mike do you see anything that is missing in the mozconfigs patches that will allow the preference to continue to be set

looking at this
mobile/android/config/mozconfigs/common includes
. "$topsrcdir/build/mozconfig.common" which sets the value for 
MOZ_REQUIRE_SIGNING=${MOZ_REQUIRE_SIGNING-1}
however, we have defined the following in 
mobile/android/config/mozconfigs/common.override

# This Source Code Form is subject to the terms of the Mozilla Public
# License, v. 2.0. If a copy of the MPL was not distributed with this
# file, You can obtain one at http://mozilla.org/MPL/2.0/.

# This file is included at the bottom of all native android mozconfigs
#
#
# Disable enforcing that add-ons are signed by the trusted root
MOZ_REQUIRE_SIGNING=${MOZ_REQUIRE_SIGNING-0}

. "$topsrcdir/build/mozconfig.common.override"
. "$topsrcdir/build/mozconfig.cache"

which to my understanding would disable the preference for Android
Flags: needinfo?(mshal)
(In reply to Kim Moir [:kmoir] from comment #126)
> looking at this
> mobile/android/config/mozconfigs/common includes
> . "$topsrcdir/build/mozconfig.common" which sets the value for 
> MOZ_REQUIRE_SIGNING=${MOZ_REQUIRE_SIGNING-1}
> however, we have defined the following in 
> mobile/android/config/mozconfigs/common.override
> 
> # This Source Code Form is subject to the terms of the Mozilla Public
> # License, v. 2.0. If a copy of the MPL was not distributed with this
> # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> 
> # This file is included at the bottom of all native android mozconfigs
> #
> #
> # Disable enforcing that add-ons are signed by the trusted root
> MOZ_REQUIRE_SIGNING=${MOZ_REQUIRE_SIGNING-0}

This syntax only sets the variable if it isn't already set. From the web of includes, it looks like build/mozconfig.common gets parsed first, which sets MOZ_REQUIRE_SIGNING to 1. So the variable is already set by the time it gets to here, and it isn't subsequently disabled. You could either change mobile/android/config/mozconfigs/common.override to:

MOZ_REQUIRE_SIGNING=0

Or just move the MOZ_REQUIRE_SIGNING=${MOZ_REQUIRE_SIGNING-0} construct into mobile/android/config/mozconfigs/common before the '. "$topsrcdir/build/mozconfig.common"' line.

This seems to fix it locally, but I'm just running configure and looking at the value of MOZ_REQUIRE_SIGNING in config.status. Let me know if that doesn't fix the actual problem with the tests and I can look further.
Flags: needinfo?(mshal)
Thanks Mike!  I'm running a try run now with one of your suggestions.
This try run looks good so far.  The mda tests that are orange are not enabled on beta, just on try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2318cf7a8b7d&selectedJob=20312004
Attachment #8748367 - Flags: review?(mshal)
Attachment #8748367 - Flags: review?(mshal) → review+
Attachment #8748367 - Flags: checked-in+
patch for merge day script

looking at the script I think we just have to worry about the values that are different, the values that are added such as 
MOZ_REQUIRE_SIGNING=0
in
mobile/android/config/mozconfigs/common.override 
on m-b should be fine
Attachment #8749192 - Flags: review?(rail)
Attachment #8749192 - Flags: review?(rail) → review+
Attachment #8749192 - Flags: checked-in+
closing

there is still the work to generate unbranded builds with the pref turned off in bug 1135781, I'm having problems with the branding on that, still working on it.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Comment on attachment 8745809 [details] [diff] [review]
bug1186522m-b-2.patch

Reverting the approval as this was backed out. Please re-nom if it needs to land in m-b.
Attachment #8745809 - Flags: approval-mozilla-beta+ → approval-mozilla-beta-
ritu: actually the patch in comment 129 was landed on mozilla beta (the original patch was backed out). I verified that the preference cannot be toggled on a build subsequent to that. Do I need to request mozilla-beta approval again for that patch?
Flags: needinfo?(rkothari)
(In reply to Kim Moir [:kmoir] from comment #134)
> ritu: actually the patch in comment 129 was landed on mozilla beta (the
> original patch was backed out). I verified that the preference cannot be
> toggled on a build subsequent to that. Do I need to request mozilla-beta
> approval again for that patch?

No you don't need to. I think I was confused because the patch was A+'d but status-firefox47 flag was still showing affected. I would like update patch (8745809) as A+ and status 47 to fixed if this has landed on m-b. Is that the case? Please let me know.
Flags: needinfo?(rkothari)
Ritu: patch 8748367 landed on beta, not 8745809
Comment on attachment 8748367 [details] [diff] [review]
bug1186522m-b-3.patch

Approval Request Comment
[Feature/regressing bug #]:1186522
[User impact if declined]: preference to install unsign addons can continue to be toggled by users
[Describe test coverage new/current, TreeHerder]: N/A, this is already covered by existing tests
[Risks and why]: enabling addon signing by default in all automated testing is wanted for 47
[String/UUID change made/needed]: none
Attachment #8748367 - Flags: approval-mozilla-beta?
Comment on attachment 8748367 [details] [diff] [review]
bug1186522m-b-3.patch

Rubber stamping, this already landed on m-b.
Attachment #8748367 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Kim Moir [:kmoir] from comment #137)
> Ritu: patch 8748367 landed on beta, not 8745809

Got it. Thanks! Now the fields are set correctly, we are good!
Hi Kim, Kev (Product manager) & team have decided to disable add-on signing enforcement for Fx47. Can we backout this change so that pref for add-on signing enforcement is off by default? Thanks!
Flags: needinfo?(kmoir)
To be VERY clear, Kev has decided to back out the mandatory enforcement of signing (e.g. removing the pref that allows people to disable signing enforcement).

Because we don't have unbranded builds, we need to back out the change that made signing mandatory, with no option for disabling. The result of the change should be that Fx 47 will have singing enabled by default, and the xpinstall.signatures.required preference honored.
(In reply to Kev Needham [:kev] from comment #143)
> To be VERY clear, Kev has decided to back out the mandatory enforcement of
> signing (e.g. removing the pref that allows people to disable signing
> enforcement).
> 
> Because we don't have unbranded builds, we need to back out the change that
> made signing mandatory, with no option for disabling. The result of the
> change should be that Fx 47 will have singing enabled by default, and the
> xpinstall.signatures.required preference honored.

Ah! This is great. Thanks for the clarification.
Backed out of beta 47 in https://hg.mozilla.org/releases/mozilla-beta/rev/de10e990d861


Heads up to aleth, in case comm-beta needs a backout, too.
Flags: needinfo?(aleth)
This is now planned for Fx48.
(In reply to Wes Kocher (:KWierso) from comment #145)
> Heads up to aleth, in case comm-beta needs a backout, too.

Thanks!
Flags: needinfo?(aleth)
thanks for backing those out, yesterday was a holiday in Canada
Status: RESOLVED → REOPENED
Flags: needinfo?(kmoir)
Resolution: FIXED → ---
Attached patch bug1186522m-i-4.patch (obsolete) — Splinter Review
when patches were backed out last month, this part was removed accidentally
Pushed by kmoir@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbadac2844a9
force per checkin and release desktop firefox builds to require signed add-ons on beta 47 r=mshal DONTBUILD
Attachment #8760861 - Flags: checked-in+
kmoir, those settings were deliberately re-added in bug 1277965, we want ADDON_SIGNING to be enabled by default in local builds.
I'm not seeing the connection between removing those flags for local builds and the topic of this bug?
Flags: needinfo?(kmoir)
Pushed by kmoir@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/528945d5a054
revert Bug 1186522 - force per checkin and release desktop firefox builds to require signed add-ons on beta 47 r=mshal DONTBUILD
sorry, I didn't realize this, I have removed my patch.  I didn't realize it was related to another bug.
Flags: needinfo?(kmoir)
Attachment #8760861 - Attachment is obsolete: true
Attachment #8760861 - Flags: checked-in+
Comment on attachment 8762240 [details] [diff] [review]
bug1186522m-b-4.patch (unbitrotten)

Approval Request Comment
[Feature/regressing bug #]:1186522
[User impact if declined]: preference to install unsign addons can continue to be toggled by users
[Describe test coverage new/current, TreeHerder]: N/A, this is already covered by existing tests
[Risks and why]: enabling addon signing by default in all automated testing is desired on beta
[String/UUID change made/needed]: none
Attachment #8762240 - Flags: approval-mozilla-beta?
Kev, can you share your plan for this bug in 48? What changed since last month and 47? Thanks
Flags: needinfo?(kev)
I'm not kev but what has changed since last time is that we have builds for addon developers in bug 1135781 that allow addon developers to test their unsigned addons.  This is what was missing when we enabled the preference last time, and thus why it was backed out at Kev's direction.
Kev is on PTO this week, if I can help, please let me know. Everything Kim said is spot on though.
Flags: needinfo?(kev)
I would like to know if you are planning to ship addon signing in 48.
We had too many last minute changes and I would like to avoid that.
unbranded builds are the only known blocker
Attachment #8762240 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8762240 [details] [diff] [review]
bug1186522m-b-4.patch (unbitrotten)

https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=35e0d7dd7cd7bff301b75756361b36b275102404

will verify preference once the build are available
Attachment #8762240 - Flags: checked-in+
Depends on: 1282568
Comment on attachment 8762240 [details] [diff] [review]
bug1186522m-b-4.patch (unbitrotten)

I backed out enabling the preferences on beta until the tests are fixed (bug  	1282568)
Attachment #8762240 - Flags: checked-in+
I verified that the builds don't allow you to install unsigned addons in the revision we backed out so we should be good to go once the tests are fixed.

https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=35e0d7dd7cd7bff301b75756361b36b275102404
Depends on: 1282868
aswan is running a try run to test fixes for the test failures for bug 1282568 and bug 1282868

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0f5c530b95b
New try one since last one had issues with uploaded addons 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=11d1f8e6095c
Attachment #8762240 - Flags: checked-in+
The preference is now enabled on beta and the tests results seem to be good.  Thanks andym and aswan for fixing the tests.
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Summary: force per checkin and release desktop firefox builds to require signed add-ons on beta 47 → force per checkin and release desktop firefox builds to require signed add-ons on beta 48
Depends on: 1283622
Depends on: 1283638
Depends on: 1284179
On a side note, with [xpinstall.signatures.required=false], in about:addons there's still yellow-text warning msgs instead of red ones for unsigned addons.
 
Shouldn't they be red always to show that the preference is completely ignored already?
Firefox 48.0b6,  Build ID: 20160706215822

Regards!
andym: Do you have any comments about the previous question?  I'm not sure if the preference colour should change in this case?
Flags: needinfo?(amckay)
I don't follow comment 169, but if there is a problem there Javier, please file a new bug rather than add on to this one, its long enough already.

I installed an unsigned add-on in Firefox 47 and it shows up as yellow:

https://www.dropbox.com/s/mbwtpt0ji3tgtcs/Screenshot%202016-07-11%2013.10.01.png?dl=0

I then ran Firefox 48 beta 6 with the same profile and it shows up as red:

https://www.dropbox.com/s/i63a54lgbq8u9h1/Screenshot%202016-07-11%2013.07.41.png?dl=0
Flags: needinfo?(amckay) → needinfo?(redlibrepy)
(In reply to Andy McKay [:andym] from comment #171)
> I installed an unsigned add-on in Firefox 47 and it shows up as yellow:
> https://www.dropbox.com/s/mbwtpt0ji3tgtcs/Screenshot%202016-07-11%2013.10.01.png?dl=0
> 
> I then ran Firefox 48 beta 6 with the same profile and it shows up as red:
> https://www.dropbox.com/s/i63a54lgbq8u9h1/Screenshot%202016-07-11%2013.07.41.png?dl=0


If you set  [xpinstall.signatures.required] to false, the yellow text ("... Proceed with caution.") appears.
Fx 48.0b7,  Build ID: 20160711002726   (Windows_NT 6.1 x86)

I mentioned this just bc I think that colour/message can be misleading, now that [xpinstall.signatures.required] is already ignored.
Sorry to bother.
Flags: needinfo?(redlibrepy)
Now that the m-b -> m-r merge is complete, I've verified that this preference is set correctly on m-r builds and that unsigned addons cannot be installed.
Flags: needinfo?(ewong)
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
Component: General Automation → General
You need to log in before you can comment on or make changes to this bug.