730663 - Port |Bug 708690 - Signed MAR files do not protect against applying an update for the wrong product| to SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, defect, P3)

Comment 1

[Serge Gautherie (:sgautherie)]

Ftr,
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1330242947.1330245869.25819.gz&fulltext=1
OS X 10.6 comm-central-trunk debug test xpcshell on 2012/02/25 23:55:47
{
TEST-PASS | /builds/slave/test/build/xpcshell/tests/toolkit/mozapps/update/test/unit/test_0200_app_launch_apply_update.js | test passed (time: 4201.133ms)
}
still passes.
(Brian, that is expected, isn't it?)

Comment 2

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Fill and package update-settings.ini

Comment 3

[Brian R. Bondy [:bbondy]]

Comment on attachment 600754 [details] [diff] [review]
(Av1) Fill and package update-settings.ini

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

::: suite/confvars.sh
@@ +49,5 @@
>  MOZ_BRANDING_DIRECTORY=suite/branding/nightly
>  MOZ_OFFICIAL_BRANDING_DIRECTORY=suite/branding/nightly
>  MOZ_EXTENSIONS_DEFAULT=" venkman inspector irc gnomevfs"
>  MOZ_UPDATER=1
> +MAR_CHANNEL_ID=seamonkey-comm-central

This will encode your MAR files with that product/channel name.

So that the updater will accept that particular channel you'll have to also add this into confvars.sh:
ACCEPTED_MAR_CHANNEL_IDS=seamonkey-comm-central

also in toolkit/mozapps/update/updater/Makefile.in there is:


> ifneq (,$(filter beta release esr,$(MOZ_UPDATE_CHANNEL)))
> RCFLAGS += -DMAR_SIGNING_RELEASE_BETA=1
> else
> ifneq (,$(filter nightly aurora nightly-elm nightly-profiling nightly-oak,$(MOZ_UPDATE_CHANNEL)))
> RCFLAGS += -DMAR_SIGNING_AURORA_NIGHTLY=1
> endif
> endif

This determines what key will be used to verify your product signed MARs.  I checked with bhearsum and he said that looked good but just wanted to mention it here in case you want changes.

Comment 4

[Brian R. Bondy [:bbondy]]

About the last bit relating to the makefile, that is only for signed MAR files. I'm not sure if you want to work on that in the same bug.  If so in confvars.sh you also need MOZ_VERIFY_MAR_SIGNATURE=1 to enable MAR verifying inside updater code.

Comment 5

[Robert Kaiser]

Not that SeaMonkey even _can_ sign MARs right now, as the project has neither signing infrastructure nor a key for doing the signing.

Comment 6

[Brian R. Bondy [:bbondy]]

OK so basically these are the only important parts from my above messages:

::: suite/confvars.sh
@@ +49,5 @@
>  MOZ_BRANDING_DIRECTORY=suite/branding/nightly
>  MOZ_OFFICIAL_BRANDING_DIRECTORY=suite/branding/nightly
>  MOZ_EXTENSIONS_DEFAULT=" venkman inspector irc gnomevfs"
>  MOZ_UPDATER=1
> +MAR_CHANNEL_ID=seamonkey-comm-central

This will encode your MAR files with that product/channel name.

So that the updater will accept that particular channel you'll have to also add this into confvars.sh:
ACCEPTED_MAR_CHANNEL_IDS=seamonkey-comm-central

Comment 7

[Serge Gautherie (:sgautherie)]

(In reply to Brian R. Bondy [:bbondy] from comment #6)
> add this into confvars.sh:
> ACCEPTED_MAR_CHANNEL_IDS=seamonkey-comm-central

"You are not authorized to access bug #721758."
Can you CC me to it too?

Comment 8

[Brian R. Bondy [:bbondy]]

Yup, done.

Comment 9

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) Fill and package update-settings.ini

Av1, with comment 6 suggestion(s).

Comment 10

[Serge Gautherie (:sgautherie)]

Ping for review.

Comment 11

[Robert Kaiser]

(In reply to Serge Gautherie (:sgautherie) from comment #10)
> Ping for review.

At least in my case, this is a waste of time, I'm marking such things TODO in my mailbox, but I can only work on things like this in my free time, which is pretty sparse since I started working full-time for something other than SeaMonkey. I will get to it, no worries. Also, people are reminded weekly by Bugzilla nowadays about their outstanding review requests anyhow, no need to send bugmail in addition.

Comment 12

[Robert Kaiser]

Comment on attachment 600767 [details] [diff] [review]
(Av2) Fill and package update-settings.ini

OK, this looks good to me by code inspection. I'm not removing the request for Callek though as once this is deployed we need to make sure that updates still work, and he should be in the loop for that.

Comment 13

[Justin Wood (:Callek)]

(In reply to Robert Kaiser (:[e-mail]) from comment #12)
> for Callek though as once this is deployed we need to make sure that updates
> still work, and he should be in the loop for that.

Yes, please don't land this right now, I want to wait until after uplift and devote at least a few hours to researching what all these values specifically mean for us. Then once deployed I want to test!

Once I test and its baked for a few days, we can probably (without pain) deploy it to aurora.

Comment 14

[Serge Gautherie (:sgautherie)]

Attachment: (Av3) Fill and package update-settings.ini [Checked in: See comment 17]

Av2, plus bug 732480.

Comment 15

[Robert Kaiser]

Comment on attachment 609125 [details] [diff] [review]
(Av3) Fill and package update-settings.ini
[Checked in: See comment 17]

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

r=me but please let Callek OK the landing of this and the used "channel ID" as he and ewong will need to deal with any of this on the SeaMonkey releng side.

Comment 16

[Justin Wood (:Callek)]

Comment on attachment 609125 [details] [diff] [review]
(Av3) Fill and package update-settings.ini
[Checked in: See comment 17]

Ok, so I just had a brief meeting with bbondy to figure out this and what it would do/affect for us.

I'd love to do this, but *currently* it would be a noop for us, and it would be additional complexity for the train-day changes. So for the *time being* consider this r+ for Gecko 15 NOT Gecko 14 (as in, please land after tuesday)

This is currently no-op because its checked against the "is the mar signed" define. http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/updater/updater.cpp#1577 We can probably change the define for the part of the code that uses this, but I would say we need to do that in a separate bug.

If that change is accepted for any branches *or* if we start signing mars on our branches before Gecko 15 is out, we can uplift this change at that point.

Sorry for the delay in reviewing, but I am glad I could review correctly.

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 609125 [details] [diff] [review]
(Av3) Fill and package update-settings.ini
[Checked in: See comment 17]

http://hg.mozilla.org/comm-central/rev/5030dc66c861
Av3, unbitrotted.