Closed
Bug 797477
Opened 12 years ago
Closed 9 years ago
Enable loading certificates and MAR verification in updater code for B2G
Categories
(Firefox OS Graveyard :: General, defect, P1)
Tracking
(blocking-b2g:-, blocking-basecamp:-, b2g18 affected, b2g18-v1.0.0 affected, b2g18-v1.0.1 affected)
RESOLVED
INVALID
People
(Reporter: bbondy, Unassigned)
References
Details
(Keywords: feature, Whiteboard: [c1-ex:comment14][Triaged:1/17][eta 2013-04-19][madrid])
Attachments
(8 files, 10 obsolete files)
3.01 KB,
patch
|
Details | Diff | Splinter Review | |
2.50 KB,
patch
|
Details | Diff | Splinter Review | |
1.45 KB,
text/plain
|
Details | |
1.34 KB,
text/plain
|
Details | |
6.33 KB,
patch
|
bbondy
:
feedback+
|
Details | Diff | Splinter Review |
2.50 KB,
patch
|
jgriffin
:
review-
|
Details | Diff | Splinter Review |
14.70 KB,
patch
|
Details | Diff | Splinter Review | |
4.46 KB,
patch
|
bbondy
:
review-
|
Details | Diff | Splinter Review |
- Add code to deploy the certificates for new device setups - Add code to load those certificates inside updater code - Add code to perform verifications when on B2G inside updater code - Do build related config
Reporter | ||
Comment 1•12 years ago
|
||
bsmith can you provide the bug number for the bug you're working on to get the test certs on the B2G NSS readonly config db? I'd like to add it to the depends on list of this bug.
Reporter | ||
Comment 2•12 years ago
|
||
Dependencies need to be implemented first, but here is most of the code needed for this bug.
Reporter | ||
Comment 3•12 years ago
|
||
Reporter | ||
Comment 4•12 years ago
|
||
Reporter | ||
Updated•12 years ago
|
Summary: Enable loading certificates and MAR verification code for those certificates in updater code for B2G → Enable loading certificates and MAR verification in updater code for B2G
Reporter | ||
Updated•12 years ago
|
Whiteboard: [WebAPI:P0] → [WebAPI:P0][Blocked by bug 793709 which is a P3]
Comment 5•12 years ago
|
||
We're marking this bug with the C1 milestone since it follows the criteria of "unfinished feature work" (see https://etherpad.mozilla.org/b2g-convergence-schedule). If this work is not finished by Nov19, this bug will need an exception and will be called out at the upcoming Exec Review.
Target Milestone: --- → B2G C1 (to 19nov)
Reporter | ||
Comment 6•12 years ago
|
||
What is the Exec Review? Please note that this bug has been on hold since Oct 11th due to it's dependency mentioned in the whiteboard flag which was a P3. catlee did start working on this dependency today though so it shouldn't be long. bsmith also has a dependency which must be completed first, but I'm not sure if there's a bug for that yet. That is to load the certs into the read only NSS db of the devices. Also I haven't heard any clarification on updater related work with B2G. In particular I *think* we are only doing FOTA updates but I'm not sure. And I *think* those updates will be packaged inside MARs as they are today, but I'm not sure if everyone is on board with that. If people are not on board with that then this shouldn't even be a blocker to begin with.
Comment 7•12 years ago
|
||
(In reply to Brian R. Bondy [:bbondy] from comment #6) > Also I haven't heard any clarification on updater related work with B2G. In > particular I *think* we are only doing FOTA updates but I'm not sure. And I > *think* those updates will be packaged inside MARs as they are today, but > I'm not sure if everyone is on board with that. If people are not on board > with that then this shouldn't even be a blocker to begin with. The answer to this greatly affects the timeline of bug 793709. If RelEng doesn't need to sign gecko update mars, then there's very little work to be done. FOTA updates will have to be created and signed manually given the current processes.
Reporter | ||
Comment 8•12 years ago
|
||
Update: I think catlee got us what we need in bug 793709. Status: The next steps is for bsmith to load those certs in a read only NSS db. To be clear: The November 19th deadline is completely dependent on bsmith's work being done. Without that this work cannot continue. Also I'd like clarification on what an Exec Review is.
Comment 9•12 years ago
|
||
Do we need separate mar signing keys for dep/try builds, nightly builds, and release builds, like we have for desktop updates?
Reporter | ||
Comment 10•12 years ago
|
||
Maybe eventually but since this is only used for testing internal builds I don't think it's needed. I'll defer to bsmith though.
Comment 11•12 years ago
|
||
(In reply to Brian R. Bondy [:bbondy] from comment #8) > Update: I think catlee got us what we need in bug 793709. > Status: The next steps is for bsmith to load those certs in a read only NSS > db. > > To be clear: The November 19th deadline is completely dependent on bsmith's > work being done. Without that this work cannot continue. Which bug # is bsmith's work that blocks this bug? It's not clear in the dependency tree. Also, are the patches here ready for review? Or need new version after bsmith and catlee's work lands? > Also I'd like clarification on what an Exec Review is. It means that we'll likely need broader in-Mozilla as well as multi-partner approval to land feature work, since the risk of new code at that point puts the whole release schedule at significant risk.
Reporter | ||
Comment 12•12 years ago
|
||
(In reply to Dietrich Ayala (:dietrich) from comment #11) > (In reply to Brian R. Bondy [:bbondy] from comment #8) > > Update: I think catlee got us what we need in bug 793709. > > Status: The next steps is for bsmith to load those certs in a read only NSS > > db. > > > > To be clear: The November 19th deadline is completely dependent on bsmith's > > work being done. Without that this work cannot continue. > > Which bug # is bsmith's work that blocks this bug? It's not clear in the > dependency tree. I'm not sure off hand. bsmith can you chime in? > Also, are the patches here ready for review? Or need new version after > bsmith and catlee's work lands? It will need extra fixes after catlee and bsmith's work lands, but not significant. I prepared most of the work without these dependencies to speed up any wait time on me.
Flags: needinfo?(bsmith)
Reporter | ||
Comment 14•12 years ago
|
||
Got an "End of C1 tomorrow" email, asking questions. Answers can be found below: > * A comment providing the following details will need to be included: > ** Why the bug should remain blocking-basecamp (layman's terms, > especially if the summary isn't human readable) I'm not sure it should be blocking-basecamp. This was only marked as blocking-basecamp originally based on a speculative need. The requirements are still not known for updates. If they are known, they are only recently known and have not been communicated yet. The need for such clarification has been known for over a month. > ** Level of effort and risk involved There is a real risk of breaking updates for b2g builds only. Additional work from RelEng and QA may be able to lower that risk. ** Lower risk or lower effort alternatives to resolving the bug What I think should be done, is to not sign MARs for now and to offer updates through HTTPS. We don't sign MARs today on neither of OSX nor Linux. We could then do this work for a future release. Updates are secure already as long as our update servers have not been compromised. > ** ETA for resolution I estimate a couple days of work from me once the dependencies are done. The last I heard in bug 793709 was that this should not be enabled yet, I asked for clarification on when it should be enabled in that bug. Also there is a task that bsmith mentioned he'd do in this bug (See Comment 13). My ETA does not include bsmith's work / estimation. > ** Whether the work is blocked Yes. By bsmith's work and by bug 793709 Comment 40. I think bsmith should probably add a different bug for his work instead of using this bug and it should have its C1-blocking comment like this. > * The whiteboard should then have the following added: [c1-ex:commentY] where > Y is the comment where the exception request can be found. Done.
Whiteboard: [WebAPI:P0][Blocked by bug 793709 which is a P3] → [WebAPI:P0][Blocked by bug 793709 which is a P3][c1-ex:comment14]
Comment 15•12 years ago
|
||
(In reply to Brian R. Bondy [:bbondy] from comment #14) > ** Lower risk or lower effort alternatives to resolving the bug > > What I think should be done, is to not sign MARs for now and to offer > updates through HTTPS. We don't sign MARs today on neither of OSX nor Linux. > We could then do this work for a future release. Updates are secure already > as long as our update servers have not been compromised. Setting the b-b flag to ? given this proposed alternative.
blocking-basecamp: + → ?
Target Milestone: B2G C1 (to 19nov) → ---
Comment 16•12 years ago
|
||
We discussed this at length in triage today and we decided we don't want to release without signed MARs. If there's information we don't have that this isn't as big of a security issue as we think it could be, please provide it here.
blocking-basecamp: ? → +
Reporter | ||
Comment 17•12 years ago
|
||
OK, to make it clear I cannot proceed with this work. See Comment 14 for details.
Reporter | ||
Comment 19•11 years ago
|
||
:) I'd prefer if you created a new bug as a dependency and added the same tracking flags, but I won't hold you to it.
Comment 20•11 years ago
|
||
Can we get a priority and target milestone for this>
Updated•11 years ago
|
Priority: -- → P1
Target Milestone: --- → B2G C3 (12dec-1jan)
Updated•11 years ago
|
Whiteboard: [WebAPI:P0][Blocked by bug 793709 which is a P3][c1-ex:comment14] → [c1-ex:comment14]
Reporter | ||
Comment 22•11 years ago
|
||
Same as on 2012-11-19, See Comment 14. Also see Comment 17. Deferring to bsmith for an update beyond that.
Flags: needinfo?
Comment 23•11 years ago
|
||
The lack of action and the ambiguity of ownership in this bug is a problem. Brian Smith, can you please file a separate bug for the blocking work you mention in comment #13 and add the patches there per comment #19? That would really help clear up the picture.
Updated•11 years ago
|
Target Milestone: B2G C3 (12dec-1jan) → B2G C4 (2jan on)
Comment 25•11 years ago
|
||
Do we have any update for this case? We don't have update for few days. Do we need someone to help?
Comment 26•11 years ago
|
||
I will be working on this starting tonight. I am not sure I need any help with it. We are confident that all other aspects of the client-side MAR updating (other than signature checking) work right?
Flags: needinfo?(bsmith)
Comment 27•11 years ago
|
||
Whats the status here?
Comment 28•11 years ago
|
||
I am still actively working on it now, still trying to get it done by the end of the afternoon. Will require review from people in the US though.
Comment 29•11 years ago
|
||
Let's not block basecamp on this but we'll do it for v1 for sure.
blocking-b2g: --- → tef+
blocking-basecamp: + → -
Updated•11 years ago
|
Whiteboard: [c1-ex:comment14] → [c1-ex:comment14] [NPOTB] [Triaged:1/17]
Comment 30•11 years ago
|
||
I don't think [NPOTB] is appropriate because this stuff does get built as part of Gecko, but also I don't know exactly how we're using [NPOTB], so I won't remove it.
Comment 31•11 years ago
|
||
Brian, what is the latest on this bug? This is still TEF+ so please let us know if this will not make the TEF+ deadline (this Friday Feb 1st).
Updated•11 years ago
|
Flags: needinfo?(bsmith)
Updated•11 years ago
|
status-b2g18:
--- → affected
status-b2g18-v1.0.0:
--- → affected
Comment 32•11 years ago
|
||
(In reply to Lucas Adamski from comment #31) > Brian, what is the latest on this bug? This is still TEF+ so please let us > know if this will not make the TEF+ deadline (this Friday Feb 1st). Our next handoff is 15feb... any progress on this?
blocking-b2g: tef+ → shira?
Comment 33•11 years ago
|
||
akeybl: bah - sorry, I changed TEF+ to shira? by accident. And i dont have privs to undo. Can you correct for me?
Updated•11 years ago
|
Whiteboard: [c1-ex:comment14] [NPOTB] [Triaged:1/17] → [c1-ex:comment14][Triaged:1/17]
Updated•11 years ago
|
status-b2g18-v1.0.1:
--- → affected
Comment 35•11 years ago
|
||
Brian, what is going on here?
Comment 36•11 years ago
|
||
Sorry for the lack of status update here. It is scarily close to being done but I've been getting distracted by other things. Will concentrate on getting this wrapped up.
Flags: needinfo?(bsmith)
Comment 37•11 years ago
|
||
Brian, 7 days without any changes. What's going on ?
Flags: needinfo?(bsmith)
Comment 38•11 years ago
|
||
From https://mail.mozilla.org/pipermail/packagedapps/2013-February/000137.html (Thursday 14 February): "Last night I wrote some tests for this and surprised myself when one of them failed this morning. It is likely that we will have to redo the certs. Consequently, I doubt we will be able to do the migration to the new certs before MWC, and so we should rely on the old test certs for any MWC demos."
Comment 39•11 years ago
|
||
(In reply to Andrew Overholt [:overholt] from comment #38) > From > https://mail.mozilla.org/pipermail/packagedapps/2013-February/000137.html > (Thursday 14 February): > > "Last night I wrote some tests for this and surprised myself when one of > them failed this morning. It is likely that we will have to redo the certs. > Consequently, I doubt we will be able to do the migration to the new certs > before MWC, and so we should rely on the old test certs for any MWC demos." I think this is related to the packaged app signing bugs, but not this bug. This bug talks about general OTA updates with certs I believe.
Comment 40•11 years ago
|
||
(In reply to Andrew Overholt [:overholt] from comment #38) > From > https://mail.mozilla.org/pipermail/packagedapps/2013-February/000137.html > (Thursday 14 February): > > "Last night I wrote some tests for this and surprised myself when one of > them failed this morning. It is likely that we will have to redo the certs. > Consequently, I doubt we will be able to do the migration to the new certs > before MWC, and so we should rely on the old test certs for any MWC demos." Jason is right, this is about other bugs regarding Marketplace app signing, not this bug.
Flags: needinfo?(bsmith)
Updated•11 years ago
|
Whiteboard: [c1-ex:comment14][Triaged:1/17] → [c1-ex:comment14][Triaged:1/17][target 28/2]
Comment 41•11 years ago
|
||
Attachment #670491 -
Attachment is obsolete: true
Comment 42•11 years ago
|
||
Comment 43•11 years ago
|
||
Attachment #670489 -
Attachment is obsolete: true
Comment 44•11 years ago
|
||
These four patches are what I have so far for getting the tests in toolkit/mozapps/update/test/unit to mostly function correctly. I say "mostly" because test_0010_general.js fails because it cannot write to /system/b2g/update_write_access_test, and because the change to runtestsb2g.py to add /system/b2g/updater_xpcshell_cert.der doesn't actually work. Oddly, the tset_0010_general.js test seemed to be working just fine for me until today, so I am not sure what I broke exactly. I will figure it out. That is a relatively minor problem. So far, I've been testing this by manually "adb remount && adb push $(GECKO_DIR)/toolkit/mozapps/update/updater/xpcshellCertificate.der /system/b2g/updater_xpcshell_cert.der && adb reboot". I will probably need some help from ATeam people to get the runtestsb2g.py changes to actually work. The good thing about the problem is that it makes it easier to check that the tests correctly fail with a certificate verification error when the updater_xpcshell_cert.der file is missing. I do not have the tests in modules/libmar/tests/unit working correctly yet. Also, there are some tests that are currently Windows-only that I need to enable for B2G too. Right now, I am punting on both of these issues until after I can get these four patches ready for review, because these two issues need a different kind of work. However, at least five of the already-enabled tests do exercise the signature verification functionality already. bbondy: I am going to clean these patches up tonight before I ask for review. I will explain in detail why I had to make so many changes to your original patches. In short, it wasn't because you did anything wrong, but rather I found that it was very problematic for various reasons to support the OEM and carrier customization of the keys in the build process when relying on the NSS database, so I switched to storing the files in the filesystem. Also, I rearranged the control flow for the B2G case to match as closely as possible the Windows control flow, in order to make it clearer that the code is doing the correct thing (since we already know your Windows code is doing the correct thing).
Updated•11 years ago
|
Attachment #727405 -
Attachment description: WIP 2: Have runtestsb2g.py push updater xpcshell cert before running xpcshell tests → WIP Part 2: Have runtestsb2g.py push updater xpcshell cert before running xpcshell tests
Comment 45•11 years ago
|
||
Updated•11 years ago
|
Attachment #727416 -
Attachment description: Script for running the updater xpcshell tests (only) on B2G, based on dhyland's work → Script for running the updater xpcshell tests (only) on a device, based on dhyland's work
Comment 46•11 years ago
|
||
Comment 47•11 years ago
|
||
Brian, in your original patch you tried to do the MOZ_WIDGET_TOOLKIT check within b2g/confvars.sh to differentiate between B2G device builds and B2G desktop builds. But, I couldn't get that to work, because MOZ_WIDGET_TOOLKIT is set after $PRODUCT/confvars.sh is sourced. So, I just put the logic directly in configure.in. Suggestions welcome. The way this works is that the person who runs B2G must put export MOZ_MAR_CERT_FILENAME_PREFIX=something in their .userconfig when building B2G. This will cause the build system to put $(MOZ_MAR_CERT_FILENAME_PREFIX)_primary.der and $(MOZ_MAR_CERT_FILENAME_PREFIX)_secondary.der into /system/b2g, and then the updater will use those certs as the primary and secondary certs for signature verification. For example, you can set MOZ_MAR_CERT_FILENAME_PREFIX=export MOZ_MAR_CERT_FILENAME_PREFIX=nightly_aurora_level3 to use the Nighty/Aurora key, export MOZ_MAR_CERT_FILENAME_PREFIX=release to use the release keys, or export MOZ_MAR_CERT_FILENAME_PREFIX=/full/path/to/my/own/keys to use your own keys.
Attachment #727403 -
Attachment is obsolete: true
Attachment #728006 -
Flags: review?(ted)
Attachment #728006 -
Flags: review?(netzen)
Comment 48•11 years ago
|
||
This modifies the xpcshell testing scripts for B2G so that they push the test certificate to the device so that the xpcshell tests can succeed. Unfortunately, it only works if you have done an "adb remount" before running the xpcshell tests. That is bad for two reasons: 1. There shouldn't be such a manual step required. 2. The xpcshell tests need to run in the *normal* configuration where /system is read-only. One option would be to do a "adb remount" before writing the cert and an "adb reboot" after writing the cert, but that will slow down the running of the xpcshell tests significantly. Presumably, the addition of the xpcshell cert could happen elsewhere, like in whatever script actually flashes the device, or we could add a B2G build system option to make this part of the image. I worry that making this a build system option will be really dangerous because it would be easy to accidentally set that option in a release build and ruin the security of this whole system. Suggestions?
Attachment #728008 -
Flags: feedback?(netzen)
Attachment #728008 -
Flags: feedback?(marshall)
Comment 49•11 years ago
|
||
Brian, these changes are different enough from your original patch that I think you can review them without worrying about reviewing too much of your own work. As you can see, I basically just tried to make the non-Windows code paths for verification be the same as the Windows code paths. In general, I tried to make sure that there are as few distinct code paths as possible. I think this is helpful since both you and I are primarily Windows developers, so any effort we spend that works for both Windows and non-Windows is a savings in the long run. In the course of doing this, I simplified the code. Perhaps I over-simplified it beyond the intended scope of this bug. It made sense at the time, though.
Attachment #727408 -
Attachment is obsolete: true
Attachment #728009 -
Flags: review?(netzen)
Comment 50•11 years ago
|
||
Attachment #728009 -
Attachment is obsolete: true
Attachment #728009 -
Flags: review?(netzen)
Updated•11 years ago
|
Attachment #728008 -
Attachment description: Have runtestsb2g.py push updater xpcshell cert before running xpcshell tests → Part 2: Have runtestsb2g.py push updater xpcshell cert before running xpcshell tests
Comment 51•11 years ago
|
||
First, this patch adds some logic to make debugging the updater easier. You can make the updater print its PID and sleep for few seconds so that you can attach GDB to it to step through the code. And, all the LOG() output will go to logcat in addition to the log file. This made debugging MUCH simpler. This patch generally tries to emulate the resource loading stuff that Windows does so that we can use the exact same code path that the Windows updater uses for signature verification. Instead of storing the certs in updater.exe, the certs are stored in /system/b2g/. This has the same effect since the updater itself lives in /system/b2g, so if you can write to that directory then you own the system.
Attachment #727414 -
Attachment is obsolete: true
Attachment #728017 -
Flags: review?(netzen)
Attachment #728017 -
Flags: review?(marshall)
Comment 52•11 years ago
|
||
I found a problem with part 3 of the patch, which seems to still be breaking Windows. I am going to try to minimize (undo) some my simplificatoins of the code which should make things more obviously non-breaky.
Reporter | ||
Updated•11 years ago
|
Attachment #728008 -
Attachment is patch: true
Reporter | ||
Comment 53•11 years ago
|
||
Comment on attachment 728017 [details] [diff] [review] Part 4: updater changes Review of attachment 728017 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/update/common/errors.h @@ +66,5 @@ > #define FOTA_UNKNOWN_ERROR 45 > #define WRITE_ERROR_SHARING_VIOLATION_SIGNALED 46 > #define WRITE_ERROR_SHARING_VIOLATION_NOPROCESSFORPID 47 > #define WRITE_ERROR_SHARING_VIOLATION_NOPID 48 > +#define NSS_INIT_CONFIG_ERROR 49 We should also add this error code to toolkit\mozapps\update\nsUpdateService.js. Check for the type of error handling you want to mirror when this happens. For example search that file for FOTA_GENERAL_ERROR if you want it to do the same as that. ::: toolkit/mozapps/update/common/updatelogging.cpp @@ +79,5 @@ > va_start(ap, fmt); > +#if defined(MOZ_WIDGET_GONK) > + __android_log_print (ANDROID_LOG_INFO, "updater", "*** Warning: "); > + __android_log_vprint(ANDROID_LOG_INFO, "updater", fmt, ap); > + __android_log_print (ANDROID_LOG_INFO, "updater", "***\n"); ANDROID_LOG_WARN? ::: toolkit/mozapps/update/updater/Makefile.in @@ +174,5 @@ > cp $< $@ > libs:: updater_xpcshell_cert.der > > endif > +ifeq (gonk,$(MOZ_WIDGET_TOOLKIT)) you should probably pull out this file for the buildconfig patch @@ +180,5 @@ > +ifneq (,$(MOZ_MAR_CERT_FILENAME_PREFIX)) > +updater_primary_cert.der: $(MOZ_MAR_CERT_FILENAME_PREFIX)_primary.der > + cp $< $@ > +updater_secondary_cert.der: $(MOZ_MAR_CERT_FILENAME_PREFIX)_secondary.der > + cp $< $@ Is there only one channel for B2G? Otherwise we need one (primary,secondary) for each channel. @@ +186,5 @@ > + $(NSINSTALL) $< $(dir $@) > +$(DIST)/bin/updater_secondary_cert.der: updater_secondary_cert.der > + $(NSINSTALL) $< $(dir $@) > +libs:: $(DIST)/bin/updater_primary_cert.der \ > + $(DIST)/bin/updater_secondary_cert.der I think these need to be added to some package manifest too. Are you sure they will also be installed to exiting devices? @@ +194,5 @@ > +# can find it, but we don't want to copy it to $(DIST)/bin because we > +# don't want it to be distributed to end users. > +updater_xpcshell_cert.der: xpcshellCertificate.der > + cp $< $@ > +libs:: updater_xpcshell_cert.der Has this passed try tests? As far as I understand the machine that builds is not the same machine that builds tests and I'm not sure if this will be included in the zip downloaded by the machine that does tests or not. ::: toolkit/mozapps/update/updater/archivereader.cpp @@ +60,5 @@ > + if (!f) { > + return false; > + } > + > + fclose(f); Use stat instead unless you want to be sure read access is granted. If that is the case add a comment here describing that. @@ +108,2 @@ > size = SizeofResource(handle, resourceInfoBlockHandle); > data = static_cast<const uint8_t*>(::LockResource(resourceHandle)); The SizeOfResource API can fail (returns 0 on failure). The LockResource API can fail (returns NULL on failure). While you're here please check for these values and return false. I think that mar verify signatures would just error out but it's best to check it here because the values get passed to CryptoX which is library dependent. @@ +113,5 @@ > + LOG(("Before GetResourceFilePath")); > + > + const char * src = GetResourceFilePath(name); > + if (!src) { > + LOG(("GetResourceFilePath failed: %d", (int) name)); name is already an int @@ +119,5 @@ > + } > + > + LOG(("After GetResourceFilePath: %s", src)); > + > + int rv = mar_read_entire_file(src, 16 * 1024, &data, &size); Will certs always be <16K? (I think so just checking) Maybe you can pull out this value to a header file. @@ +121,5 @@ > + LOG(("After GetResourceFilePath: %s", src)); > + > + int rv = mar_read_entire_file(src, 16 * 1024, &data, &size); > + > + return rv == 0; nit: rv == OK ::: toolkit/mozapps/update/updater/updater.cpp @@ +2195,5 @@ > gArchiveReader.Close(); > } > } > > + // TODO: is this the right thing to do for B2G? Check with rstrong or ehsan to be sure please @@ +2287,5 @@ > + if (NSS_NoDB_Init(NULL) != SECSuccess) { > + PRErrorCode error = PR_GetError(); > + LOG(("Could not initialize NSS: %s (%d)", PR_ErrorToName(error), (int) error)); > + _exit(1); > + } #if defined(MOZ_WIDGET_GONK) around this block.
Attachment #728017 -
Flags: review?(netzen) → review-
Reporter | ||
Comment 54•11 years ago
|
||
Comment on attachment 728017 [details] [diff] [review] Part 4: updater changes Review of attachment 728017 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/update/updater/Makefile.in @@ +180,5 @@ > +ifneq (,$(MOZ_MAR_CERT_FILENAME_PREFIX)) > +updater_primary_cert.der: $(MOZ_MAR_CERT_FILENAME_PREFIX)_primary.der > + cp $< $@ > +updater_secondary_cert.der: $(MOZ_MAR_CERT_FILENAME_PREFIX)_secondary.der > + cp $< $@ Ignore the comment about a (primary, secondary) for each channel I seen the comment explaining it after writing this.
Reporter | ||
Updated•11 years ago
|
Attachment #728006 -
Flags: review?(netzen) → feedback+
Reporter | ||
Comment 55•11 years ago
|
||
Comment on attachment 728008 [details] [diff] [review] Part 2: Have runtestsb2g.py push updater xpcshell cert before running xpcshell tests Review of attachment 728008 [details] [diff] [review]: ----------------------------------------------------------------- > One option would be to do a "adb remount" before writing the cert and an "adb reboot" after writing the cert, > but that will slow down the running of the xpcshell tests significantly. Maybe we can do it at most once for all xpcshell tests and not on each xpcshell test.
Attachment #728008 -
Flags: feedback?(netzen)
Comment 56•11 years ago
|
||
Comment on attachment 728017 [details] [diff] [review] Part 4: updater changes Review of attachment 728017 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/update/common/errors.h @@ +66,5 @@ > #define FOTA_UNKNOWN_ERROR 45 > #define WRITE_ERROR_SHARING_VIOLATION_SIGNALED 46 > #define WRITE_ERROR_SHARING_VIOLATION_NOPROCESSFORPID 47 > #define WRITE_ERROR_SHARING_VIOLATION_NOPID 48 > +#define NSS_INIT_CONFIG_ERROR 49 I removed this error code because now I am not using it. ::: toolkit/mozapps/update/common/updatelogging.cpp @@ +79,5 @@ > va_start(ap, fmt); > +#if defined(MOZ_WIDGET_GONK) > + __android_log_print (ANDROID_LOG_INFO, "updater", "*** Warning: "); > + __android_log_vprint(ANDROID_LOG_INFO, "updater", fmt, ap); > + __android_log_print (ANDROID_LOG_INFO, "updater", "***\n"); Thanks! Changed. ::: toolkit/mozapps/update/updater/Makefile.in @@ +174,5 @@ > cp $< $@ > libs:: updater_xpcshell_cert.der > > endif > +ifeq (gonk,$(MOZ_WIDGET_TOOLKIT)) Let's not worry about that. I am going to fold all of these patches into a single commit when landing it anyway. The important thing is that changes to configure.in and b2g/confvars.sh and similar should be in the first patch and that patch should change as little as possible to prevent needing to completely rebuild everything during development. @@ +186,5 @@ > + $(NSINSTALL) $< $(dir $@) > +$(DIST)/bin/updater_secondary_cert.der: updater_secondary_cert.der > + $(NSINSTALL) $< $(dir $@) > +libs:: $(DIST)/bin/updater_primary_cert.der \ > + $(DIST)/bin/updater_secondary_cert.der I did this in the buildconfig part of the patch (Part 1). @@ +194,5 @@ > +# can find it, but we don't want to copy it to $(DIST)/bin because we > +# don't want it to be distributed to end users. > +updater_xpcshell_cert.der: xpcshellCertificate.der > + cp $< $@ > +libs:: updater_xpcshell_cert.der Right. In general it isn't clear how to get the xpcshell cert onto the device for testing purposes. I will find a solution for that with ATeam on Monday. ::: toolkit/mozapps/update/updater/archivereader.cpp @@ +60,5 @@ > + if (!f) { > + return false; > + } > + > + fclose(f); I will add a comment explaining the reasoning. @@ +108,2 @@ > size = SizeofResource(handle, resourceInfoBlockHandle); > data = static_cast<const uint8_t*>(::LockResource(resourceHandle)); Done. @@ +119,5 @@ > + } > + > + LOG(("After GetResourceFilePath: %s", src)); > + > + int rv = mar_read_entire_file(src, 16 * 1024, &data, &size); Done. @@ +121,5 @@ > + LOG(("After GetResourceFilePath: %s", src)); > + > + int rv = mar_read_entire_file(src, 16 * 1024, &data, &size); > + > + return rv == 0; This function is defined to return 0 or -1, so I think comparison to zero is better. ::: toolkit/mozapps/update/updater/updater.cpp @@ +2287,5 @@ > + if (NSS_NoDB_Init(NULL) != SECSuccess) { > + PRErrorCode error = PR_GetError(); > + LOG(("Could not initialize NSS: %s (%d)", PR_ErrorToName(error), (int) error)); > + _exit(1); > + } I moved this to the top of the function and removed the logging. NSS_NoDB_Init should never fail because it doesn't need to open any files or anything except shared libraries. If we can't open the NSS shared libraries then we're really screwed probably didn't even get to main() due to dynamic linker failures. @@ +2416,5 @@ > + LOG(("updater process PID: %d", (int) getpid())); > + if (getenv("MOZ_UPDATER_WAIT_FOR_DEBUGGER")) { > + sleep(10); > + } > +#endif I found out after I wrote this that the reason the Windows tests were failing is that they are looking for exact matches of the LOG() output. So, we should avoid using LOG() for these types of things because it would make it harder to adapt the tests to B2G. Instead, I now call __android_log_print directly.
Reporter | ||
Comment 57•11 years ago
|
||
> I moved this to the top of the function and removed the logging.
>NSS_NoDB_Init should never fail because it doesn't need to open any
> files or anything except shared libraries
Even if it can't fail I don't see any point running it on other platforms. Doing extra things that may load in extra libraries isn't good in general because on Windows this code runs through the system account and is susceptible to DLL injection.
Comment 58•11 years ago
|
||
(In reply to Brian R. Bondy [:bbondy] from comment #57) > > I moved this to the top of the function and removed the logging. > >NSS_NoDB_Init should never fail because it doesn't need to open any > > files or anything except shared libraries > > Even if it can't fail I don't see any point running it on other platforms. > Doing extra things that may load in extra libraries isn't good in general > because on Windows this code runs through the system account and is > susceptible to DLL injection. Sorry, I forgot to point this out already: There isn't enough context in the patch to see it, but this is already within #ifdef MOZ_WIDGET_GONK, so there are no worries here. (Also, AFAICT the updater only links to NSS when building for gonk so this code wouldn't build if it weren't ifdef'd.) I found a memory leak in my code. I am going to finish cleaning this up today and submit it for re-review, including part 3.
Comment 59•11 years ago
|
||
Attachment #728015 -
Attachment is obsolete: true
Attachment #729123 -
Flags: review?(netzen)
Comment 60•11 years ago
|
||
Attachment #728017 -
Attachment is obsolete: true
Attachment #728017 -
Flags: review?(marshall)
Attachment #729124 -
Flags: review?(netzen)
Comment 61•11 years ago
|
||
LOG(...) -> __android_log_print(...) in the case where NSS initialization failed, removed // TODO comment.
Attachment #729124 -
Attachment is obsolete: true
Attachment #729124 -
Flags: review?(netzen)
Attachment #729140 -
Flags: review?(netzen)
Comment 62•11 years ago
|
||
There is a test that tries to verify that the directory containing the updater executable is writable. I do not know the purpose of that test, but that is not a valid test for B2G because the updater is in /system/b2g which is not writable until the updater remounts for filesystem to make it so. I split that test into two tests, one which tests for the writable updater directory, and another test that tests the other things, and I disabled the writable updater directory test for B2G. I also enabled the version downgrade and channel tests for B2G.
Attachment #729153 -
Flags: review?(netzen)
Updated•11 years ago
|
Severity: normal → critical
Reporter | ||
Comment 63•11 years ago
|
||
Comment on attachment 729140 [details] [diff] [review] Part 4, v3: updater changes Review of attachment 729140 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/update/updater/archivereader.cpp @@ +109,3 @@ > } > > + // XXX: can LockResource or SizeOfResource fail? This comment can be removed now. @@ +116,2 @@ > FreeLibrary(handle); > + return true; Since you went through all the trouble of computing `result` you should maybe return it here? ;)
Attachment #729140 -
Flags: review?(netzen) → review+
Reporter | ||
Comment 64•11 years ago
|
||
Comment on attachment 729123 [details] [diff] [review] Part 3, v2: libmar changes. Review of attachment 729123 [details] [diff] [review]: ----------------------------------------------------------------- ::: modules/libmar/moz.build @@ +12,1 @@ > # On Windows we don't verify with NSS and updater needs to link to it I think when the moz.build conversion happened this comment was misplaced, please remove. ::: modules/libmar/src/mar.h @@ +129,5 @@ > * @return A non-zero value if an error occurs. > */ > int mar_extract(const char *path); > > +#define MAR_MAX_CERT_SIZE (16*1024) // Way larger than necessary nit: spaces before and after multiplication #define MAR_MAX_CERT_SIZE (16 * 1024) // Way larger than necessary ::: modules/libmar/tool/mar.c @@ +336,5 @@ > + > + if (!rv) { > + MarFile *mar = mar_open(argv[2]); > + if (mar) { > + rv = mar_verify_signatures(mar, certBuffers, fileSizes, certCount); I like this change, thanks. ::: modules/libmar/verify/mar_verify.c @@ +33,5 @@ > + if (!f) { > + return -1; > + } > + > + result = -1; nit: just initialize result above to be -1.
Attachment #729123 -
Flags: review?(netzen) → review+
Reporter | ||
Comment 65•11 years ago
|
||
Comment on attachment 729153 [details] [diff] [review] Enable two additional tests for B2G, and disable part of another Review of attachment 729153 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/update/test/unit/test_0010_general.js @@ -15,5 @@ > - var testFile = getCurrentProcessDir(); > - testFile.append("update_write_access_test"); > - logTestInfo("trying to create file " + testFile.path); > - testFile.create(AUS_Ci.nsIFile.NORMAL_FILE_TYPE, 0644); > - do_check_true(testFile.exists()); I don't think removing this is OK but please check with rstrong. Perhaps just only skip it for b2g. @@ -18,5 @@ > - testFile.create(AUS_Ci.nsIFile.NORMAL_FILE_TYPE, 0644); > - do_check_true(testFile.exists()); > - testFile.remove(false); > - do_check_false(testFile.exists()); > - I don't think removing this is OK but please check with rstrong. Perhaps just only skip it for b2g. ::: toolkit/mozapps/update/test/unit/xpcshell.ini @@ +3,5 @@ > tail = > > [test_0010_general.js] > +[test_0011_updaterDirectoryWritable.js] > +skip-if = toolkit = "gonk" Please check with rstrong on this ::: toolkit/mozapps/update/test/unit/xpcshell_updater.ini @@ +4,5 @@ > [test_0113_general.js] > skip-if = os == "mac" || toolkit == "gonk" > reason = bug 820380 > +[test_0113_versionDowngradeCheck.js] > +[test_0114_productChannelCheck.js] I think we want these to be run-if = os == 'win' || toolkit == "gonk"
Attachment #729153 -
Flags: review?(netzen) → review-
Comment 66•11 years ago
|
||
Comment on attachment 728006 [details] [diff] [review] Part 1: build configuration changes. Review of attachment 728006 [details] [diff] [review]: ----------------------------------------------------------------- ::: configure.in @@ +6305,5 @@ > MOZ_VERIFY_MAR_SIGNATURE=1, > MOZ_VERIFY_MAR_SIGNATURE= ) > > if test -n "$MOZ_VERIFY_MAR_SIGNATURE"; then > + if test "$OS_ARCH" = "WINNT" -o -n "$gonkdir"; then Can you test "$MOZ_WIDGET_TOOLKIT" = "gonk" instead here? ::: toolkit/mozapps/update/updater/Makefile.in @@ +163,5 @@ > + $(NSINSTALL) $< $(dir $@) > +$(DIST)/bin/updater_secondary_cert.der: updater_secondary_cert.der > + $(NSINSTALL) $< $(dir $@) > +libs:: $(DIST)/bin/updater_primary_cert.der \ > + $(DIST)/bin/updater_secondary_cert.der I really don't want to r+ adding more random rules like this to Makefile.in when we're trying to get rid of them in the transition to moz.build. Can you rewrite these in terms of INSTALL_TARGETS instead? Example: http://mxr.mozilla.org/mozilla-central/source/browser/branding/aurora/Makefile.in#63
Attachment #728006 -
Flags: review?(ted)
Comment 67•11 years ago
|
||
wonder what's the latest on this bug? and if there is anything blocking this bug from moving along? thanks
Comment 68•11 years ago
|
||
Comment on attachment 728008 [details] [diff] [review] Part 2: Have runtestsb2g.py push updater xpcshell cert before running xpcshell tests Jonathan, does this look right to you? As I mentioned to Clint, we need to stick this file in /system/b2g which is normally not a writable location. It seems like this is the function that writes other stuff into that location. I guess at some point /system is remounted to be read+write so we can write the files there. But, when we run the xpcshell tests, we should be running them with /system mounted as read-only, because the updater code that is being tested here is actually responsible for ensuring that system/b2g is writable as part of its execution; if system/b2g is already writable then that part of the code isn't really being tested. I don't know if/how the B2G xpcshell test framework is handling this. Perhaps dealing with this should be a follow-up issue.
Attachment #728008 -
Flags: review?(jgriffin)
Comment 69•11 years ago
|
||
Comment on attachment 728008 [details] [diff] [review] Part 2: Have runtestsb2g.py push updater xpcshell cert before running xpcshell tests Review of attachment 728008 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/xpcshell/runtestsb2g.py @@ +46,5 @@ > > # Overridden > def pushLibs(self): > + updater_xpcshell_cert_dest = os.path.join(self.options.deviceRoot, > + '../../../system/b2g/updater_xpcshell_cert.der') We can probably just use '/system/b2g/updater_xpcshell_cert.der', no? That path is not related to deviceRoot, so no need to os.path.join(). @@ +50,5 @@ > + '../../../system/b2g/updater_xpcshell_cert.der') > + print 'pushing %s' % updater_xpcshell_cert_dest > + updater_xpcshell_cert_src = os.path.join( > + os.path.split(os.path.abspath(__file__))[0], > + '../../../toolkit/mozapps/update/updater/updater_xpcshell_cert.der') Where does this file come from? It's going to be in gecko, under /toolkit/mozapps/update/updater? This logic won't work for running the tests in many environments (including TBPL), since those tests only have access to the contents of tests.zip (i.e., the test-package-stage directory), and not the src or obj directory this was built from. We probably want to copy the .der file into xpcshell dir here, http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/Makefile.in#61, and then remove this os.path.join() logic. @@ +52,5 @@ > + updater_xpcshell_cert_src = os.path.join( > + os.path.split(os.path.abspath(__file__))[0], > + '../../../toolkit/mozapps/update/updater/updater_xpcshell_cert.der') > + print 'from %s' % updater_xpcshell_cert_src > + self.device.pushFile(updater_xpcshell_cert_src, updater_xpcshell_cert_dest, retryLimit=10) Does this work without an 'adb remount'?
Attachment #728008 -
Flags: review?(jgriffin) → review-
Comment 70•11 years ago
|
||
Comment on attachment 728008 [details] [diff] [review] Part 2: Have runtestsb2g.py push updater xpcshell cert before running xpcshell tests Review of attachment 728008 [details] [diff] [review]: ----------------------------------------------------------------- Deferring to jgriffin's comments.
Attachment #728008 -
Flags: feedback?(marshall)
Comment 71•11 years ago
|
||
Been over a week since last comment, so checking in - what needs to happen to move this forward?
Comment 72•11 years ago
|
||
Today I am finishing up the updated patches and testing them. I will king jgriffin if I have any more problems with the testing part. Also, when this lands, Gecko/Gaia updates created by releng will probably not apply on the device unless/until they reconfigure the build infrastructure. I will work on documenting what they need to do.
Whiteboard: [c1-ex:comment14][Triaged:1/17][target 28/2] → [c1-ex:comment14][Triaged:1/17][eta 2013-04-15]
Comment 73•11 years ago
|
||
Thanks Brian. What is the new ETA for this?
Whiteboard: [c1-ex:comment14][Triaged:1/17][eta 2013-04-15] → [c1-ex:comment14][Triaged:1/17][eta 2013-04-15][status: needs new ETA]
Comment 74•11 years ago
|
||
I got a clean run of this last night on Try on B2G: https://tbpl.mozilla.org/?tree=Try&rev=e54951568c44 I want to re-run it on try with all the platforms and all tests enabled to make sure I didn't break anything on non-B2G platforms. I will do so as soon as Try starts working again. (Currently I cannot push to try.) So, I am expecting to post all the patches for review this afternoon. Then, it will be up to the reviewers' availability.
Whiteboard: [c1-ex:comment14][Triaged:1/17][eta 2013-04-15][status: needs new ETA] → [c1-ex:comment14][Triaged:1/17][eta 2013-04-17]
Comment 75•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=67d198a01de2
Comment 76•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=67d198a01de2 It took a very long time to get these try results today, but the try run is all green. I will post the updated patches in the morning (Pacific time). New ETA is Pacific time too, to allow time for reviews.
Whiteboard: [c1-ex:comment14][Triaged:1/17][eta 2013-04-17] → [c1-ex:comment14][Triaged:1/17][eta 2013-04-19]
Comment 77•11 years ago
|
||
It looks like that try job didn't contain any commits.
Comment 78•11 years ago
|
||
(In reply to Jonathan Griffin (:jgriffin) from comment #77) > It looks like that try job didn't contain any commits. If you pull that revision, you will see that there are five patches. The first time I pushed this set of patches, I did so with only B2G Mochitests selected. Then, I re-pushed the same set of patches with all tests enabled on all platforms. TBPL only shows the changesets that are new since the last time you pushed, which is why it only shows the "new" try changeset.
Updated•11 years ago
|
Whiteboard: [c1-ex:comment14][Triaged:1/17][eta 2013-04-19] → [c1-ex:comment14][Triaged:1/17][eta 2013-04-19][madrid]
Comment 79•11 years ago
|
||
Per gal and clee, we are not going to implement OTA for 1.0.1 or 1.1 or in the near feature. FOTA works fine for our needs. Until there is a product need or free cycles, this work does need to continue.
blocking-b2g: tef+ → -
Comment 80•11 years ago
|
||
I told you so.
Comment 81•11 years ago
|
||
You also told us in comment 28 that this would be landed 3 months ago.
Updated•11 years ago
|
Target Milestone: B2G C4 (2jan on) → Madrid (19apr)
Reporter | ||
Comment 82•11 years ago
|
||
(In reply to Lucas Adamski [:ladamski] (plz needinfo) from comment #81) > You also told us in comment 28 that this would be landed 3 months ago. Six months ago in Comment 6, and again five months ago in Comment 14 I expressed that clarification on requirements was needed, and that I didn't think this should be done in the first place. That is not counting several emails. Based on things being unclear I'm not surprised that this was not Brian Smith's #1 priority. But who's counting.
Updated•11 years ago
|
Target Milestone: 1.0.1 Madrid (19apr) → ---
Reporter | ||
Comment 83•11 years ago
|
||
Comment on attachment 729123 [details] [diff] [review] Part 3, v2: libmar changes. I'll be taking bsmith's already reviewed patch for libmar changes into bug 903135. Marking it as obsolete here and uploading it there.
Attachment #729123 -
Attachment is obsolete: true
Reporter | ||
Comment 84•11 years ago
|
||
Comment on attachment 729140 [details] [diff] [review] Part 4, v3: updater changes Review of attachment 729140 [details] [diff] [review]: ----------------------------------------------------------------- Some of this patch is superseded by work being done in bug 903135, and some of it would need to be changed now as well if we ever revisit this bug.
Attachment #729140 -
Flags: review+
Updated•10 years ago
|
Assignee: brian → nobody
Reporter | ||
Comment 85•9 years ago
|
||
I don't think this is valid anymore, re-open if you disagree.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → INVALID
You need to log in
before you can comment on or make changes to this bug.
Description
•