Update some documentation for CertVerifyResult::verified_cert

Update some documentation for CertVerifyResult::verified_cert

Review-Url: https://codereview.chromium.org/2616343005
Cr-Commit-Position: refs/heads/master@{#442486}
Committed: https://chromium.googlesource.com/chromium/src/+/4847079a1779a4f643dafca0c5da6425561a378e

[eroman, 2017-01-09 21:54:41]

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

[commit-bot: I haz the power, 2017-01-09 21:55:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eroman, 2017-01-09 23:04:22]

eroman@chromium.org changed reviewers:
+ rsleevi@chromium.org

[Ryan Sleevi, 2017-01-09 23:20:14]

LGTM w/ nits

https://codereview.chromium.org/2616343005/diff/20001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc.h (right):

https://codereview.chromium.org/2616343005/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc.h:99: // will be filled in by Verify().
Note quite true here: 

|verify_result| is explicitly initialized to set |verified_cert|

The contract might be better specified:

On entry, |verify_result| will be default-initialized as a successful
validation, with |verify_result->verified_cert| set to |cert|. Implementations
are expected to fill in all applicable fields, excluding |ocsp_result|, which
will be filled in by |Verify()|.  If an error code is returned,
|verify_result->cert_status| should be non-zero, indicating an error occurred.
On success, net::OK should be returned, with |verify_result| updated to reflect
the successfully verified chain.

IDK, WDYT?

https://codereview.chromium.org/2616343005/diff/20001/net/cert/cert_verify_re...
File net/cert/cert_verify_result.h (right):

https://codereview.chromium.org/2616343005/diff/20001/net/cert/cert_verify_re...
net/cert/cert_verify_result.h:29: // Comparing CertVerifyResult with a nullptr
|verified_cert| is invalid.
Why? Are you documenting implementation details or interface contract? Feels
like the former.

Alternatively, if the contract is "verified_cert should never be nullptr",
perhaps that's part of the contract for verified_cert, rather than being
documented here?

https://codereview.chromium.org/2616343005/diff/20001/net/cert/cert_verify_re...
net/cert/cert_verify_result.h:37: // partially constructed path, or the original
chain.
s/, / /

https://codereview.chromium.org/2616343005/diff/20001/net/cert/cert_verify_re...
net/cert/cert_verify_result.h:39: // intermediate, or the target certificate if
there are no intermediates.
Documentation wise, there's also some pre-existing subject/verb agreement issues

// The certificate chain that was constructed during verification.
// Note: Although |verified_cert| will match the originally supplied
// certificate to be validated, the results of
// GetIntermediateCertificates() may be substantially different,
// both in order and in content, then the originally supplied
// intermediates.
// In the event of validation failures, this may contain the
// originally supplied certificate chain or a partially
// constructed path, depending on the implementation.
// In the event of validation success, the trust anchor will be
// |verified_cert->GetIntermediateCertificates().back()| if
// there was a certificate chain to the trust anchor, and will
// be |verified_cert->os_cert_handle()| if the certificate was
// the trust anchor.

WDYT? Probably still botched the wording.

[eroman, 2017-01-09 23:33:08]

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

[eroman, 2017-01-09 23:33:12]

https://codereview.chromium.org/2616343005/diff/20001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc.h (right):

https://codereview.chromium.org/2616343005/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc.h:99: // will be filled in by Verify().
On 2017/01/09 23:20:14, Ryan Sleevi wrote:
> Note quite true here: 
> 
> |verify_result| is explicitly initialized to set |verified_cert|
> 
> The contract might be better specified:
> 
> On entry, |verify_result| will be default-initialized as a successful
> validation, with |verify_result->verified_cert| set to |cert|. Implementations
> are expected to fill in all applicable fields, excluding |ocsp_result|, which
> will be filled in by |Verify()|.  If an error code is returned,
> |verify_result->cert_status| should be non-zero, indicating an error occurred.
> On success, net::OK should be returned, with |verify_result| updated to
reflect
> the successfully verified chain.
> 
> IDK, WDYT?

Done.

https://codereview.chromium.org/2616343005/diff/20001/net/cert/cert_verify_re...
File net/cert/cert_verify_result.h (right):

https://codereview.chromium.org/2616343005/diff/20001/net/cert/cert_verify_re...
net/cert/cert_verify_result.h:29: // Comparing CertVerifyResult with a nullptr
|verified_cert| is invalid.
On 2017/01/09 23:20:14, Ryan Sleevi wrote:
> Why? Are you documenting implementation details or interface contract? Feels
> like the former.
> 
> Alternatively, if the contract is "verified_cert should never be nullptr",
> perhaps that's part of the contract for verified_cert, rather than being
> documented here?

I was documenting the current behavior, which relies on not being called with a
default-initialized (or Reset()) CertVerifyResult.

This looked a bit suspicious when reading through the code, since one generally
expects operator== to work on any instance, but in practice it doesn't get
called in that mode.

Instead of the comment I could modify the implementation to test for nullptr,
however I have not reviewed the callers to see if that policy is appropriate yet
(i.e. if hitting that situation is wrong, crashing may be preferable)

https://codereview.chromium.org/2616343005/diff/20001/net/cert/cert_verify_re...
net/cert/cert_verify_result.h:37: // partially constructed path, or the original
chain.
On 2017/01/09 23:20:14, Ryan Sleevi wrote:
> s/, / /

Done.

https://codereview.chromium.org/2616343005/diff/20001/net/cert/cert_verify_re...
net/cert/cert_verify_result.h:39: // intermediate, or the target certificate if
there are no intermediates.
On 2017/01/09 23:20:14, Ryan Sleevi wrote:
> Documentation wise, there's also some pre-existing subject/verb agreement
issues
> 
> // The certificate chain that was constructed during verification.
> // Note: Although |verified_cert| will match the originally supplied
> // certificate to be validated, the results of
> // GetIntermediateCertificates() may be substantially different,
> // both in order and in content, then the originally supplied
> // intermediates.
> // In the event of validation failures, this may contain the
> // originally supplied certificate chain or a partially
> // constructed path, depending on the implementation.
> // In the event of validation success, the trust anchor will be
> // |verified_cert->GetIntermediateCertificates().back()| if
> // there was a certificate chain to the trust anchor, and will
> // be |verified_cert->os_cert_handle()| if the certificate was
> // the trust anchor.
> 
> WDYT? Probably still botched the wording.

Done.

[commit-bot: I haz the power, 2017-01-09 23:33:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 01:52:21]

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

[commit-bot: I haz the power, 2017-01-10 01:52:22]

Dry run: This issue passed the CQ dry run.

[eroman, 2017-01-10 03:22:47]

The CQ bit was checked by eroman@chromium.org

[eroman, 2017-01-10 03:22:47]

The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/2616343005/#ps40001
(title: "use sleevi's text")

[commit-bot: I haz the power, 2017-01-10 03:23:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 04:56:58]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484018567349440,
"parent_rev": "a2407051a9a2c6e03125c8d959e1ef7d4e441e95", "commit_rev":
"4847079a1779a4f643dafca0c5da6425561a378e"}

[commit-bot: I haz the power, 2017-01-10 04:57:30]

Description was changed from

==========
Update some documentation for CertVerifyResult::verified_cert
==========

to

==========
Update some documentation for CertVerifyResult::verified_cert

Review-Url: https://codereview.chromium.org/2616343005
Cr-Commit-Position: refs/heads/master@{#442486}
Committed:
https://chromium.googlesource.com/chromium/src/+/4847079a1779a4f643dafca0c5da...
==========

[commit-bot: I haz the power, 2017-01-10 04:57:31]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/4847079a1779a4f643dafca0c5da...