AU: detect failure when using public key verification

AU: detect failure when using public key verification

This makes public key verification non-fatal. A future CL will use
this knowledge to report statistics.

Change-Id: I9440155dd71621662e5c0f4011b3001bbc65e6d7

BUG=chromium-os:13341
TEST=unittests

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

[petkov, 2011-03-30 21:04:42]

http://codereview.chromium.org/6778029/diff/1/delta_performer.cc
File delta_performer.cc (right):

http://codereview.chromium.org/6778029/diff/1/delta_performer.cc#newcode593
delta_performer.cc:593: if (!utils::FileExists(key_path.c_str())) {
I assume you have full confidence in the correctness of the
TEST_AND_RETURN_FALSE statements between this if and the new if? :-)

http://codereview.chromium.org/6778029/diff/1/delta_performer.cc#newcode599
delta_performer.cc:599:
TEST_AND_RETURN_FALSE(PayloadSigner::VerifySignature(signatures_message_data_,
If signatures get mixed up this will fail first. So you probably want to
conditionalize this code too.

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

http://codereview.chromium.org/6778029/diff/1/download_action.cc#newcode181
download_action.cc:181: // TODO(adlr): squash the http verify failure warning
what do you mean?

[adlr, 2011-03-30 21:52:19]

PTAL. Thanks!

http://codereview.chromium.org/6778029/diff/1/delta_performer.cc
File delta_performer.cc (right):

http://codereview.chromium.org/6778029/diff/1/delta_performer.cc#newcode593
delta_performer.cc:593: if (!utils::FileExists(key_path.c_str())) {
On 2011/03/30 21:04:42, petkov wrote:
> I assume you have full confidence in the correctness of the
> TEST_AND_RETURN_FALSE statements between this if and the new if? :-)

Good point! I was thinking we should really expect all these other things to
succeed, but I think you're right that we should allow them to fail. Changed.

http://codereview.chromium.org/6778029/diff/1/delta_performer.cc#newcode599
delta_performer.cc:599:
TEST_AND_RETURN_FALSE(PayloadSigner::VerifySignature(signatures_message_data_,
On 2011/03/30 21:04:42, petkov wrote:
> If signatures get mixed up this will fail first. So you probably want to
> conditionalize this code too.

Done.

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

http://codereview.chromium.org/6778029/diff/1/download_action.cc#newcode181
download_action.cc:181: // TODO(adlr): squash the http verify failure warning
On 2011/03/30 21:04:42, petkov wrote:
> what do you mean?

Removed this code and made a cleaner comment

[petkov, 2011-03-30 22:10:21]

http://codereview.chromium.org/6778029/diff/4001/delta_performer.cc
File delta_performer.cc (right):

http://codereview.chromium.org/6778029/diff/4001/delta_performer.cc#newcode606
delta_performer.cc:606: TEST_SET_TRUE_RET_TRUE(*signature_failed,
!signatures_message_data_.empty());
This doesn't check for signature_failed != NULL? Either fix the code or update
the docstring in the .h to account for the fact that signature_failed can't be
NULL.

http://codereview.chromium.org/6778029/diff/4001/delta_performer.h
File delta_performer.h (right):

http://codereview.chromium.org/6778029/diff/4001/delta_performer.h#newcode71
delta_performer.h:71: // it's non-NULL); this will not cause the method to fail.
the code may segfault on NULL and failure...

[petkov, 2011-03-30 22:12:39]

http://codereview.chromium.org/6778029/diff/4001/delta_performer.cc
File delta_performer.cc (right):

http://codereview.chromium.org/6778029/diff/4001/delta_performer.cc#newcode602
delta_performer.cc:602: if (!utils::FileExists(key_path.c_str())) {
you could probably just do a similar check for signature_failed == NULL and
return true without doing any signature checks...

[adlr, 2011-03-30 22:30:05]

Thank for the quick review. PTAL

http://codereview.chromium.org/6778029/diff/4001/delta_performer.cc
File delta_performer.cc (right):

http://codereview.chromium.org/6778029/diff/4001/delta_performer.cc#newcode602
delta_performer.cc:602: if (!utils::FileExists(key_path.c_str())) {
On 2011/03/30 22:12:39, petkov wrote:
> you could probably just do a similar check for signature_failed == NULL and
> return true without doing any signature checks...

Yeah, but I want to get a log of exactly what fails when it goes wrong.

http://codereview.chromium.org/6778029/diff/4001/delta_performer.cc#newcode606
delta_performer.cc:606: TEST_SET_TRUE_RET_TRUE(*signature_failed,
!signatures_message_data_.empty());
On 2011/03/30 22:10:21, petkov wrote:
> This doesn't check for signature_failed != NULL? Either fix the code or update
> the docstring in the .h to account for the fact that signature_failed can't be
> NULL.

Fixed to accept NULL

http://codereview.chromium.org/6778029/diff/4001/delta_performer.h
File delta_performer.h (right):

http://codereview.chromium.org/6778029/diff/4001/delta_performer.h#newcode71
delta_performer.h:71: // it's non-NULL); this will not cause the method to fail.
On 2011/03/30 22:10:21, petkov wrote:
> the code may segfault on NULL and failure...

Fixed

[petkov, 2011-03-30 22:36:23]

LGTM

http://codereview.chromium.org/6778029/diff/5006/download_action.cc
File download_action.cc (right):

http://codereview.chromium.org/6778029/diff/5006/download_action.cc#newcode167
download_action.cc:167: NULL)) {
Intentionally removed the TODO, I assume?

[adlr, 2011-04-04 23:27:13]

forgot to publish this note

http://codereview.chromium.org/6778029/diff/5006/download_action.cc
File download_action.cc (right):

http://codereview.chromium.org/6778029/diff/5006/download_action.cc#newcode167
download_action.cc:167: NULL)) {
On 2011/03/30 22:36:23, petkov wrote:
> Intentionally removed the TODO, I assume?

Yeah, I think it makes this CL cleaner