Fix a bug where packing an extension with bad private key causes crash.

Fix a bug where packing an extension with bad private key causes crash.
BUG=263968
TEST=PackExtensionTest.ExtensionWithInvalidKey

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=214622

[dhnishi (use Chromium), 2013-07-26 19:27:23]

@mpcomplete

Please review the extensions side of this when you get a chance. 

When finalizing the zip file fails due to the key being bad, it stops the
process and sets the error for an invalid key.

@rsleevi

Please review the signature creator side of this patch when you get a chance.

This NOTREACHED() was actually reachable when a private key that has the correct
structure for a private key (so it passes earlier validity tests), but has been
modified to not be correct. I am removing it so that Chrome doesn't get taken
down and so the return false gets sent.

[not at google - send to devlin, 2013-07-26 19:40:25]

https://codereview.chromium.org/20142007/ also does this but didn't add a test.
I was nervous about removing a NOTREACHED but I'll let rsleevi comment on that.

[Ryan Sleevi, 2013-07-26 19:56:48]

I just merged your bug into http://crbug.com/125997 , which was the first report
of the 'invalid/too large' key.

I'm fine with removing the NOTREACHED() - this is crypto code that has to deal
with 'hostile' input (eg: extension keys), so it shouldn't be triggering
NOTREACHED()'s like this. I tried to clean up most of these in crypto/.

So LGTM. The only concern I'd have with is "IDS_EXTENSION_PRIVATE_KEY_INVALID" -
signatures may fail for other reasons, better to just use a signature failed.
But that's a weak nit.

[dhnishi (use Chromium), 2013-07-26 20:03:00]

On 2013/07/26 19:56:48, Ryan Sleevi wrote:
> I just merged your bug into http://crbug.com/125997 , which was the first
report
> of the 'invalid/too large' key.
> 
> I'm fine with removing the NOTREACHED() - this is crypto code that has to deal
> with 'hostile' input (eg: extension keys), so it shouldn't be triggering
> NOTREACHED()'s like this. I tried to clean up most of these in crypto/.
> 
> So LGTM. The only concern I'd have with is "IDS_EXTENSION_PRIVATE_KEY_INVALID"
-
> signatures may fail for other reasons, better to just use a signature failed.
> But that's a weak nit.

Thanks for the quick review.

Alright, I switched over to the "IDS_EXTENSION_ERROR_WHILE_SIGNING" error to
address the nit.

[Matt Perry, 2013-07-26 20:17:37]

lgtm

[commit-bot: I haz the power, 2013-07-26 21:55:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/DHNishi@gmail.com/20794003/6001

[zhchbin, 2013-07-27 00:33:24]

https://codereview.chromium.org/20794003/diff/1/chrome/browser/extensions/pac...
File chrome/browser/extensions/pack_extension_unittest.cc (right):

https://codereview.chromium.org/20794003/diff/1/chrome/browser/extensions/pac...
chrome/browser/extensions/pack_extension_unittest.cc:77:
TEST_F(PackExtensionTest, ExtensionWithInvalidKey) {
Why don't you put this code to the ExtensionServiceTest.PackExtension like my
patch, it will be the right place I think. And the place here has some comment:
"Tests the environment for packing extensions from the command line via the
--pack-extension switch."

Sorry for If I make a mistake here.

[commit-bot: I haz the power, 2013-07-28 00:43:00]

Retried try job too often on win7_aura for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[dhnishi (use Chromium), 2013-07-29 16:13:01]

On 2013/07/27 00:33:24, zhchbin wrote:
>
https://codereview.chromium.org/20794003/diff/1/chrome/browser/extensions/pac...
> File chrome/browser/extensions/pack_extension_unittest.cc (right):
> 
>
https://codereview.chromium.org/20794003/diff/1/chrome/browser/extensions/pac...
> chrome/browser/extensions/pack_extension_unittest.cc:77:
> TEST_F(PackExtensionTest, ExtensionWithInvalidKey) {
> Why don't you put this code to the ExtensionServiceTest.PackExtension like my
> patch, it will be the right place I think. And the place here has some
comment:
> "Tests the environment for packing extensions from the command line via the
> --pack-extension switch."
> 
> Sorry for If I make a mistake here.

I think that testing it inside of the PackExtension unit tests is closer to the
source, rather than doing it from the ExtensionService.
The comment just means that it runs the test using the pack-extension command
line switch. It builds up a list of command line arguments, then passes it to
the |startup_helper_| which runs them to pack the extension for the test.

[commit-bot: I haz the power, 2013-07-29 16:16:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/DHNishi@gmail.com/20794003/6001

[Matt Perry, 2013-07-29 18:35:26]

On 2013/07/29 16:13:01, Daniel Nishi wrote:
> On 2013/07/27 00:33:24, zhchbin wrote:
> >
>
https://codereview.chromium.org/20794003/diff/1/chrome/browser/extensions/pac...
> > File chrome/browser/extensions/pack_extension_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/20794003/diff/1/chrome/browser/extensions/pac...
> > chrome/browser/extensions/pack_extension_unittest.cc:77:
> > TEST_F(PackExtensionTest, ExtensionWithInvalidKey) {
> > Why don't you put this code to the ExtensionServiceTest.PackExtension like
my
> > patch, it will be the right place I think. And the place here has some
> comment:
> > "Tests the environment for packing extensions from the command line via the
> > --pack-extension switch."
> > 
> > Sorry for If I make a mistake here.
> 
> I think that testing it inside of the PackExtension unit tests is closer to
the
> source, rather than doing it from the ExtensionService.
> The comment just means that it runs the test using the pack-extension command
> line switch. It builds up a list of command line arguments, then passes it to
> the |startup_helper_| which runs them to pack the extension for the test.

I think it makes sense as an ExtensionService test. The PackExtension test is
heavier-weight. It starts up a new instance of chrome and passes it a
commandline flag. The goal there is more to test the commandline flag than
packing functionality. The ExtensionService test is more self contained.

[dhnishi (use Chromium), 2013-07-29 19:15:06]

@mpcomplete

Good point. r192255 does specify that the unit test was added specifically to
test the command line flags.

I've remade the test with the extension service tests and slightly changed the
wording to make it more clear that the pack_extension_unittest is not a unit
test for just testing packing, but a unit test of the command line switch.

[Matt Perry, 2013-07-29 19:21:03]

Thanks, LGTM

[commit-bot: I haz the power, 2013-07-29 20:05:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/DHNishi@gmail.com/20794003/57001

[commit-bot: I haz the power, 2013-07-30 04:19:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/DHNishi@gmail.com/20794003/57001

[commit-bot: I haz the power, 2013-07-30 16:14:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/DHNishi@gmail.com/20794003/57001

[commit-bot: I haz the power, 2013-07-31 07:49:20]

Change committed as 214622