Last Comment Bug 721758 - Beta builds should be able to apply release mars
: Beta builds should be able to apply release mars
Status: RESOLVED FIXED
[sg:nse][qa-]
:
Product: Toolkit
Classification: Components
Component: Application Update (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Brian R. Bondy [:bbondy]
:
Mentors:
Depends on: 708697 728935
Blocks: 730663
  Show dependency treegraph
 
Reported: 2012-01-27 08:16 PST by Chris AtLee [:catlee]
Modified: 2012-04-26 14:33 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
unaffected
fixed
fixed
unaffected


Attachments
Patc h v1. Add ability to configure updater to accept multiple channels (7.97 KB, patch)
2012-02-02 09:21 PST, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Add ability to configure updater to accept multiple channels. Patch v2. (8.03 KB, patch)
2012-02-02 16:36 PST, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Add ability to configure updater to accept multiple channels. Patch v3. (8.03 KB, patch)
2012-02-07 07:38 PST, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Add ability to configure updater to accept multiple channels. Patch v4. (8.06 KB, patch)
2012-02-07 07:44 PST, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Add ability to configure updater to accept multiple channels. Patch v5. (7.93 KB, patch)
2012-02-07 17:39 PST, Brian R. Bondy [:bbondy]
robert.strong.bugs: review-
Details | Diff | Splinter Review
dd ability to configure updater to accept multiple channels. Patch v6. (8.14 KB, patch)
2012-02-09 06:52 PST, Brian R. Bondy [:bbondy]
robert.strong.bugs: review+
Details | Diff | Splinter Review

Description Chris AtLee [:catlee] 2012-01-27 08:16:33 PST
In bug 708697 we're adding a channel value to signed mars, and restricting the updater service and updater to apply only mars that match its channel. This works for most use cases, but one place it fails is being able to serve beta users our final release mars. Up until recently this was the regular process for releases. e.g. the 8.0 beta users would receive 8.0 final ('release' builds) first, and then be moved onto 9.0b1 when that was ready. This scenario is no longer possible with signed mars as specified in bug 708697.

I propose we allow the updater to be configured with a list of allowable channels. beta builds would be configured to be allowed to update to 'beta,release', and release builds only 'release'
Comment 1 Brian R. Bondy [:bbondy] 2012-01-27 08:31:21 PST
bsmith and imelven, before I begin work on this, please confirm that you are OK with a beta build being able to consume release MARs.  This task is to avoid having to do the work of repackaging MAR files for a common use case.
Comment 2 Ian Melven :imelven 2012-01-27 09:41:49 PST
(In reply to Brian R. Bondy [:bbondy] from comment #1)
> bsmith and imelven, before I begin work on this, please confirm that you are
> OK with a beta build being able to consume release MARs.  This task is to
> avoid having to do the work of repackaging MAR files for a common use case.

Please see https://bugzilla.mozilla.org/show_bug.cgi?id=708697#c39

Can we mark (in the example in comment #0 above) the 8.0 final build as still being on 'beta' channel in the MAR ? Then the user is always on the 'beta' channel (as they are in fact) even though they have applied the same MAR that was sent to the release channel as well. From the changes bbondy has made it seems like with the mar tool, the signature can be stripped, the product info block in the MAR updated to 'beta' (or 'release' depending on which was built first) and then re-signed - this can all be automated using the tool.
Comment 3 Chris AtLee [:catlee] 2012-01-27 10:23:28 PST
(In reply to Ian Melven :imelven from comment #2)
> (In reply to Brian R. Bondy [:bbondy] from comment #1)
> > bsmith and imelven, before I begin work on this, please confirm that you are
> > OK with a beta build being able to consume release MARs.  This task is to
> > avoid having to do the work of repackaging MAR files for a common use case.
> 
> Please see https://bugzilla.mozilla.org/show_bug.cgi?id=708697#c39
> 
> Can we mark (in the example in comment #0 above) the 8.0 final build as
> still being on 'beta' channel in the MAR ? Then the user is always on the
> 'beta' channel (as they are in fact) even though they have applied the same
> MAR that was sent to the release channel as well. From the changes bbondy
> has made it seems like with the mar tool, the signature can be stripped, the
> product info block in the MAR updated to 'beta' (or 'release' depending on
> which was built first) and then re-signed - this can all be automated using
> the tool.

RelEng's concern is that automating this is non-trivial for the several hundred mar files that are involved in a release.
Comment 4 Ian Melven :imelven 2012-01-27 12:56:01 PST
after discussing with catlee, this seems ok to me. dveditz also is on board.
Comment 5 Brian R. Bondy [:bbondy] 2012-01-27 13:03:43 PST
As per catlee's suggestion I'll implement this via 2 settings in the confvars.sh.  One will be the default to use for MAR creation.  The other will be a comma separated list of acceptable channel names that the updater can consume.

By the way. this implies that MAR channel names cannot contain ",".  Such strings will be interpreted as multiple different MAR channel values.  

The mar-channel.ini file (which will be renamed to update-settings.ini) will only list the comma separated list define.  There is no need for it to have the value for the MAR default since that value is only used by the mar command line utility.

All configs will have both values in the .ini file the same except for the beta channel which will have 2 values in the comma separated list.
Comment 6 Daniel Veditz [:dveditz] 2012-02-01 15:51:08 PST
Rating this "not a security exploit" since it's just an improvement to bug 708697 and not a vulnerability in itself. Still hidden while that bug is hidden.
Comment 7 Brian R. Bondy [:bbondy] 2012-02-02 09:21:35 PST
Created attachment 593884 [details] [diff] [review]
Patc h v1. Add ability to configure updater to accept multiple channels

Note for Rob: 
This is probably the lowest priority of the remaining silent update reviews.
Comment 8 Brian R. Bondy [:bbondy] 2012-02-02 09:23:34 PST
Also wanted to mention that in the patch, the actual value of MOZ_ACCEPTED_MAR_CHANNELS in confvars.sh will be normalized before landing.
Comment 9 Brian R. Bondy [:bbondy] 2012-02-02 16:36:20 PST
Created attachment 594009 [details] [diff] [review]
Add ability to configure updater to accept multiple channels. Patch v2.

Rebased after moving code out of readstrings from another patch.
Comment 10 Brian R. Bondy [:bbondy] 2012-02-07 07:38:24 PST
Created attachment 595032 [details] [diff] [review]
Add ability to configure updater to accept multiple channels. Patch v3.

Rebased for new mar define name.
Comment 11 Brian R. Bondy [:bbondy] 2012-02-07 07:44:47 PST
Created attachment 595033 [details] [diff] [review]
Add ability to configure updater to accept multiple channels. Patch v4.

Missed renaming the accepted MAR ID to have "firefox-" prefix
Comment 12 Brian R. Bondy [:bbondy] 2012-02-07 17:39:43 PST
Created attachment 595264 [details] [diff] [review]
Add ability to configure updater to accept multiple channels. Patch v5.

Rebased for ini change
Comment 13 Robert Strong [:rstrong] (use needinfo to contact me) 2012-02-08 23:50:41 PST
Comment on attachment 595264 [details] [diff] [review]
Add ability to configure updater to accept multiple channels. Patch v5.

>diff --git a/build/update-settings.ini.in b/build/update-settings.ini.in
>--- a/build/update-settings.ini.in
>+++ b/build/update-settings.ini.in
>@@ -36,9 +36,9 @@
> ;
> ; ***** END LICENSE BLOCK *****
> #endif
> ; If you modify this file updates may fail.
> ; Do not modify this file.
> 
> #filter substitution
> [Channel]
>-MAR_CHANNEL=@MAR_CHANNEL_ID@
>+ACCEPTED_MAR_CHANNELS=@MOZ_ACCEPTED_MAR_CHANNELS@
ACCEPTED_MAR_CHANNEL_IDS

>diff --git a/toolkit/mozapps/update/updater/archivereader.cpp b/toolkit/mozapps/update/updater/archivereader.cpp
>--- a/toolkit/mozapps/update/updater/archivereader.cpp
>+++ b/toolkit/mozapps/update/updater/archivereader.cpp
>@@ -164,16 +164,18 @@ ArchiveReader::VerifySignature()
> 
> /**
>  * Verifies that the MAR file matches the current product, channel, and version
>  * 
>  * @param MARChannelName The MAR channel name to use, only updates from MARs
>  *                       with a matching MAR channel name will succeed.
>  *                       If an empty string is passed, no check will be done
>  *                       for the channel name in the product information block.
>+ *                       If a comma separated list of values is passed then
>+ *                       only one value must match.
s/only //

>  * @param appVersion     The application version to use, only MARs with an
>  *                       application version >= to appVersion will be applied.
>  * @return OK on success
>  *         COULD_NOT_READ_PRODUCT_INFO_BLOCK if the product info block 
>  *                                           could not be read.
>  *         MARCHANNEL_MISMATCH_ERROR         if the channel name for this 
>  *                                           updater doesn't match the MAR
>  *                                           file's channel name.
>@@ -194,18 +196,30 @@ ArchiveReader::VerifyProductInformation(
>                                        &productInfoBlock);
>   if (rv != OK) {
>     return COULD_NOT_READ_PRODUCT_INFO_BLOCK_ERROR;
>   }
> 
>   // Only check the MAR channel name if specified, it should be passed in from
>   // the update-settings.ini file.
>   if (MARChannelName && strlen(MARChannelName)) {
>-    if (OK == rv && strcmp(MARChannelName, productInfoBlock.MARChannelName)) {
>-      rv = MAR_CHANNEL_MISMATCH_ERROR;
>+    // Check for at least one match in the comma separated list of values.
>+    const char *delimiter = " ,\t\r\n";
It doesn't seem like this should delimit on space, tab, return, and newline

Should be a quick second pass.
Comment 14 Brian R. Bondy [:bbondy] 2012-02-09 06:52:22 PST
Created attachment 595719 [details] [diff] [review]
dd ability to configure updater to accept multiple channels. Patch v6.

> It doesn't seem like this should delimit on space, tab, return, and newline

Having the whitespace there has the nice side effect that strtok will automatically trim your tokens of whitespace.  I removed the \r and \n and I added a a couple comments into confvars.sh:

> + # This should usually be the same as the value MAR_CHANNEL_ID.
> + # If more than one ID is needed, then you should use a comma separated list
> + # of values.
>   ACCEPTED_MAR_CHANNEL_IDS=firefox-mozilla-central
> + # The MAR_CHANNEL_ID must not contain the following 3 characters: ",\t "  
>   MAR_CHANNEL_ID=firefox-mozilla-central
Comment 15 Brian R. Bondy [:bbondy] 2012-02-24 13:41:43 PST
http://hg.mozilla.org/mozilla-central/rev/d74612c58245
Comment 16 Brian R. Bondy [:bbondy] 2012-02-29 18:23:46 PST
http://hg.mozilla.org/releases/mozilla-aurora/rev/18840d884c97
Comment 17 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-29 12:31:56 PDT
Is this something QA can test?
Comment 18 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-04-26 14:33:57 PDT
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #17)
> Is this something QA can test?

Taking silence as a 'no'.

Note You need to log in before you can comment on or make changes to this bug.