AU: Detect and report public key verify failures, but continue updating.

AU: Detect and report public key verify failures, but continue updating.

Make a new OmahaEvent (Type: 14 (kTypeUpdateDownloadFinished), Result:
0 (kResultError), ErrorCode: 18
(kActionCodeDownloadPayloadPubKeyVerificationError)). This event is
sent if we have a successful download that fails public key
verification. Currently this is a non-fatal failure, but we plan to
make it fatal in future clients.

BUG=chromium-os:13341
TEST=unittests; 3 on device tests: w/o public key on device, w/ pub
key and matching download, w/ pub key and non-matching download

Change-Id: Ib6589c78449c2dfcbffa4c85ab679f7fe844584b

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=21816e1

[adlr, 2011-04-05 23:40:26]

Hey Gaurav, normally I'd have Petkov review this, but he's on vacation. Can you
do the review?

Thanks,
-andrew

[adlr, 2011-04-05 23:40:52]

[+cc: josafat]

[gauravsh, 2011-04-06 00:07:23]

I don't understand the unit test portion of it - are the test cases part of a
separate CL or housed somewhere else? Some nits below...

http://codereview.chromium.org/6792065/diff/1/download_action.cc
File download_action.cc (right):

http://codereview.chromium.org/6792065/diff/1/download_action.cc#newcode161
download_action.cc:161: bool sign_verify_failed = false;
nit: consider calling this signature_... to avoid confusion with the verb.

http://codereview.chromium.org/6792065/diff/1/download_action.h
File download_action.h (right):

http://codereview.chromium.org/6792065/diff/1/download_action.h#newcode141
download_action.h:141: // Called if the download fails OR (download success AND
signiture verifies)
nit: s/signiture/signature/

shouldn't the 2nd condition be (down succeeds and signtaure does *NOT* verify?)

http://codereview.chromium.org/6792065/diff/1/update_attempter.cc
File update_attempter.cc (right):

http://codereview.chromium.org/6792065/diff/1/update_attempter.cc#newcode192
update_attempter.cc:192: // This action is always initiall in place to warn of a
signature failure.
nit: "initiall"?

[gauravsh, 2011-04-06 01:52:05]

On Tue, Apr 5, 2011 at 5:45 PM, <adlr@chromium.org> wrote:

> Hm, I can't seem to reply on the site.
>

Yeah, riteveld issues. Could you upload the new patchset - seems to be
working now.


>
> I fixed the typos.
>
> The way this works is that we ALWAYS schedule an action that will report to
> the
> OS vendor's server that a signature verify has failed. If we err for some
> other
> reason, we can suppress the signature fail warning. If we succeed and the
> signature is verified, we also suppress the action.
>
> It's a bit backward, b/c you would normally think that we would get an
> error and
> then device to contact the server. But it was easier to code it to always
> contact the server and undo it in the cases it shouldn't. Plus, this is
> temporary anyway, since this will at some point be binding.
>
> The individual components are unit tested (in other CLs). In this CL, I
> updated
> the one unittest we have for this stuff, which is a very minor test.
>
> This is one part of the updater code that isn't as well unittested, b/c it
> joins
> many tested parts together. So, in this case, I did on-machine testing.


Ok, I wasn't sure if I there were unittests (since you mention in the TEST=
field) you had included but I could see on the review.


>
> http://codereview.chromium.org/6792065/
>



-- 
-g

[adlr, 2011-04-06 02:00:21]

PTAL. Thanks

[gauravsh, 2011-04-06 02:40:23]

http://codereview.chromium.org/6792065/diff/7001/download_action.h
File download_action.h (right):

http://codereview.chromium.org/6792065/diff/7001/download_action.h#newcode141
download_action.h:141: // Called if the download fails OR (download success AND
signature verifies)
I think the last condition in this comment is wrong - it should say (download
success AND signature *verification failure*). That would match your logic in
download_action.cc.

[gauravsh, 2011-04-07 02:13:56]

a belated lgtm