Explicitly set properties of net::CertVerifyResult

Explicitly set properties of net::CertVerifyResult

CL explains why most fields of net::CertVerifyResult are being set
to hard-coded values.
PrivetV3ContextGetter::CertVerifier checks only Sha256 fingerprint
of the certificate. So calculation of the most of properties is
unnecessary.

BUG=524788
Committed: https://crrev.com/a9e739330a94ac9a9ac3372bea9fe577e4f662d1
Cr-Commit-Position: refs/heads/master@{#360435}

[Vitaly Buka (NO REVIEWS), 2015-11-16 03:50:03]

The CQ bit was checked by vitalybuka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-16 03:50:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1451573002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1451573002/1

[commit-bot: I haz the power, 2015-11-16 04:28:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-16 04:28:37]

Dry run: This issue passed the CQ dry run.

[Vitaly Buka (NO REVIEWS), 2015-11-16 19:35:06]

vitalybuka@chromium.org changed reviewers:
+ alekseys@chromium.org

[Aleksey Shlyapnikov, 2015-11-16 20:39:46]

lgtm

[Vitaly Buka (NO REVIEWS), 2015-11-16 22:41:01]

vitalybuka@chromium.org changed reviewers:
+ davidben@chromium.org, mmenke@chromium.org, rsleevi@chromium.org

[Vitaly Buka (NO REVIEWS), 2015-11-16 22:41:02]

+rsleevi
+davidben
+mmenke

[Ryan Sleevi, 2015-11-16 22:44:20]

Not LGTM in present form.

You should explain in this CL description, and the bug, and possibly even the
code, the motivation for this change.

Doing normal certificate verification after you've done the fingerprint doesn't
seem to make any sense. My best guess is that you're trying to fill in the
fields of the CertVerifyResult, but if so, that should be clearer.
Realistically, however, you should just be filling those in directly, as
appropriate - and not deferring to another implementation whose results you
ignore/override, as that simply adds runtime overhead unnecessarily.

[Vitaly Buka (NO REVIEWS), 2015-11-16 22:56:23]

On 2015/11/16 22:44:20, Ryan Sleevi (OOO. no rvws plz) wrote:
> Not LGTM in present form.
> 
> You should explain in this CL description, and the bug, and possibly even the
> code, the motivation for this change.
> 
> Doing normal certificate verification after you've done the fingerprint
doesn't
> seem to make any sense. My best guess is that you're trying to fill in the
> fields of the CertVerifyResult, but if so, that should be clearer.
> Realistically, however, you should just be filling those in directly, as
> appropriate - and not deferring to another implementation whose results you
> ignore/override, as that simply adds runtime overhead unnecessarily.

Thanks, that was quick.

Seems I do not understand your request about full semantics. Calling Default
implementation also
seems is not efficient to me too.

CertVerifyResults after Reset has
  verified_cert = NULL;
  cert_status = 0;
  has_md2 = false;
  has_md4 = false;
  has_md5 = false;
  has_sha1 = false;
  is_issued_by_known_root = false;
  is_issued_by_additional_trust_anchor = false;
  common_name_fallback_used = false;

Without looking into cert seems only verified_cert and cert_status  can be
efficiently set by my
implementation. Would be that enough?

[Ryan Sleevi, 2015-11-16 23:11:55]

On 2015/11/16 22:56:23, Vitaly Buka wrote:
> On 2015/11/16 22:44:20, Ryan Sleevi (OOO. no rvws plz) wrote:
> > Not LGTM in present form.
> > 
> > You should explain in this CL description, and the bug, and possibly even
the
> > code, the motivation for this change.
> > 
> > Doing normal certificate verification after you've done the fingerprint
> doesn't
> > seem to make any sense. My best guess is that you're trying to fill in the
> > fields of the CertVerifyResult, but if so, that should be clearer.
> > Realistically, however, you should just be filling those in directly, as
> > appropriate - and not deferring to another implementation whose results you
> > ignore/override, as that simply adds runtime overhead unnecessarily.
> 
> Thanks, that was quick.
> 
> Seems I do not understand your request about full semantics. Calling Default
> implementation also
> seems is not efficient to me too.
> 
> CertVerifyResults after Reset has
>   verified_cert = NULL;
>   cert_status = 0;
>   has_md2 = false;
>   has_md4 = false;
>   has_md5 = false;
>   has_sha1 = false;
>   is_issued_by_known_root = false;
>   is_issued_by_additional_trust_anchor = false;
>   common_name_fallback_used = false;
> 
> Without looking into cert seems only verified_cert and cert_status  can be
> efficiently set by my
> implementation. Would be that enough?

Probably :)

I suggested Reset() + the description/assignment within the verifier so that
you're explicit about your intent - that is, you're "setting" the fields to
explicitly intentional behaviour, rather than just presuming Reset() will set
them to some default value that might be the same.

[Vitaly Buka (NO REVIEWS), 2015-11-16 23:59:30]

Description was changed from

==========
Use default net::CertVerifier to set net::CertVerifyResult

BUG=524788
==========

to

==========
Explicitly set properties of net::CertVerifyResult

CL explains why most fields of net::CertVerifyResult are being set
to hard-coded values.
PrivetV3ContextGetter::CertVerifier checks only Sha256 fingerprint
of the certificate. So calculation of the most of properties is
unnecessary.

BUG=524788
==========

[Vitaly Buka (NO REVIEWS), 2015-11-17 00:08:14]

https://codereview.chromium.org/1451573002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/gcd_private/privet_v3_context_getter.cc
(right):

https://codereview.chromium.org/1451573002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/gcd_private/privet_v3_context_getter.cc:56:
verify_result->public_key_hashes.clear();
Default implementation calculates hashes for certs in the chain.
Also code is hidden in platform specific implementations of VerifyProc.
So if we need this, we need expose that code.

I can put whatever I can get from X509Certificate interfaces, e.g. result
of CalculateFingerprint256 and CalculateFingerprint.
But I don't know what is more preferable, empty or partial array.

[Vitaly Buka (NO REVIEWS), 2015-11-17 22:06:38]

Ryan, can you please advice regarding public_key_hashes?

[Ryan Sleevi, 2015-11-18 20:36:01]

lgtm

[Vitaly Buka (NO REVIEWS), 2015-11-18 21:25:42]

The CQ bit was checked by vitalybuka@chromium.org

[Vitaly Buka (NO REVIEWS), 2015-11-18 21:25:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
alekseys@chromium.org
Link to the patchset: https://codereview.chromium.org/1451573002/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-11-18 21:25:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1451573002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1451573002/60001

[commit-bot: I haz the power, 2015-11-18 22:57:53]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-18 22:59:01]

Patchset 4 (id:??) landed as
https://crrev.com/a9e739330a94ac9a9ac3372bea9fe577e4f662d1
Cr-Commit-Position: refs/heads/master@{#360435}