Misc changes to cert_verify_tool for errors

Misc changes to cert_verify_tool for errors

 * Correct a bug that prevented the display of per-path errors.
 * Add some additional help text for CLI, and reformat it.

BUG=634443
Committed: https://crrev.com/554ace1bfac664fbf6603c938aeef98914d5d4b4
Cr-Commit-Position: refs/heads/master@{#416780}

[eroman, 2016-09-02 21:32:27]

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

[eroman, 2016-09-02 21:32:28]

https://codereview.chromium.org/2305083002/diff/1/net/tools/cert_verify_tool/...
File net/tools/cert_verify_tool/cert_verify_tool.cc (left):

https://codereview.chromium.org/2305083002/diff/1/net/tools/cert_verify_tool/...
net/tools/cert_verify_tool/cert_verify_tool.cc:66: std::cerr << "ERROR:
--hostname is required\n";
Mostly just a convenience, since I find myself mostly running the
CertPathBuilder flavor.

https://codereview.chromium.org/2305083002/diff/1/net/tools/cert_verify_tool/...
File net/tools/cert_verify_tool/cert_verify_tool.cc (right):

https://codereview.chromium.org/2305083002/diff/1/net/tools/cert_verify_tool/...
net/tools/cert_verify_tool/cert_verify_tool.cc:19: " [flags]
<target/intermediates>\n"
If you don't like these changes I can revert them.

But I think it is fairly readable in terminal output.

https://codereview.chromium.org/2305083002/diff/1/net/tools/cert_verify_tool/...
File net/tools/cert_verify_tool/cert_verify_tool_util.cc (right):

https://codereview.chromium.org/2305083002/diff/1/net/tools/cert_verify_tool/...
net/tools/cert_verify_tool/cert_verify_tool_util.cc:71: return true;
Considering this case success since the caller tests for |target| being empty.

https://codereview.chromium.org/2305083002/diff/1/net/tools/cert_verify_tool/...
File net/tools/cert_verify_tool/verify_using_path_builder.cc (right):

https://codereview.chromium.org/2305083002/diff/1/net/tools/cert_verify_tool/...
net/tools/cert_verify_tool/verify_using_path_builder.cc:152: if
(!result_path->errors.errors().empty()) {
Oops!

Clearly I didn't even test this when I wrote it earlier :(

[mattm, 2016-09-02 22:39:21]

https://codereview.chromium.org/2305083002/diff/1/net/tools/cert_verify_tool/...
File net/tools/cert_verify_tool/cert_verify_tool.cc (right):

https://codereview.chromium.org/2305083002/diff/1/net/tools/cert_verify_tool/...
net/tools/cert_verify_tool/cert_verify_tool.cc:19: " [flags]
<target/intermediates>\n"
"target/intermediates" seems a little misleading to me, since the "target" part
is required. It doesn't match the "target+chain" in the description below
either.

https://codereview.chromium.org/2305083002/diff/1/net/tools/cert_verify_tool/...
net/tools/cert_verify_tool/cert_verify_tool.cc:119: std::cerr << "ERROR:
Couldn't read certifcate chain\n";
certifcate

https://codereview.chromium.org/2305083002/diff/1/net/tools/cert_verify_tool/...
net/tools/cert_verify_tool/cert_verify_tool.cc:143: if (hostname.empty()) {
should be !empty  ?

https://codereview.chromium.org/2305083002/diff/1/net/tools/cert_verify_tool/...
net/tools/cert_verify_tool/cert_verify_tool.cc:154: }
I think it's better to leave the trust store warnings inside
VerifyUsingPathBuilder. Especially since it gets more complicated with the NSS
trust store CL.

[eroman, 2016-09-02 22:53:30]

Thanks for the feedback!

https://codereview.chromium.org/2305083002/diff/1/net/tools/cert_verify_tool/...
File net/tools/cert_verify_tool/cert_verify_tool.cc (right):

https://codereview.chromium.org/2305083002/diff/1/net/tools/cert_verify_tool/...
net/tools/cert_verify_tool/cert_verify_tool.cc:19: " [flags]
<target/intermediates>\n"
On 2016/09/02 22:39:21, mattm wrote:
> "target/intermediates" seems a little misleading to me, since the "target"
part
> is required. It doesn't match the "target+chain" in the description below
> either.

Did some rewording, what do you think now?

https://codereview.chromium.org/2305083002/diff/1/net/tools/cert_verify_tool/...
net/tools/cert_verify_tool/cert_verify_tool.cc:119: std::cerr << "ERROR:
Couldn't read certifcate chain\n";
On 2016/09/02 22:39:21, mattm wrote:
> certifcate

Done.

https://codereview.chromium.org/2305083002/diff/1/net/tools/cert_verify_tool/...
net/tools/cert_verify_tool/cert_verify_tool.cc:143: if (hostname.empty()) {
On 2016/09/02 22:39:21, mattm wrote:
> should be !empty  ?

Done.

https://codereview.chromium.org/2305083002/diff/1/net/tools/cert_verify_tool/...
net/tools/cert_verify_tool/cert_verify_tool.cc:154: }
On 2016/09/02 22:39:21, mattm wrote:
> I think it's better to leave the trust store warnings inside
> VerifyUsingPathBuilder. Especially since it gets more complicated with the NSS
> trust store CL.

Done.

[eroman, 2016-09-02 22:55:29]

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

[commit-bot: I haz the power, 2016-09-02 22:56:08]

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

[eroman, 2016-09-02 22:57:03]

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

[commit-bot: I haz the power, 2016-09-02 22:57:13]

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

[eroman, 2016-09-02 23:00:00]

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

[commit-bot: I haz the power, 2016-09-02 23:00:23]

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

[eroman, 2016-09-02 23:21:36]

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

[commit-bot: I haz the power, 2016-09-02 23:22:02]

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, 2016-09-03 00:11:10]

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

[commit-bot: I haz the power, 2016-09-03 00:11:11]

Dry run: This issue passed the CQ dry run.

[mattm, 2016-09-03 00:11:21]

(there will be a merge conflict once https://codereview.chromium.org/2272493002/
lands, too.)

https://codereview.chromium.org/2305083002/diff/80001/net/tools/cert_verify_t...
File net/tools/cert_verify_tool/cert_verify_tool.cc (right):

https://codereview.chromium.org/2305083002/diff/80001/net/tools/cert_verify_t...
net/tools/cert_verify_tool/cert_verify_tool.cc:133: } else if
(!hostname.empty()) {
this one shouldn't be !-ed though

[eroman, 2016-09-03 00:20:42]

> (there will be a merge conflict once
https://codereview.chromium.org/2272493002/
lands, too.)

Acknowledged. I will update once that lands.

https://codereview.chromium.org/2305083002/diff/80001/net/tools/cert_verify_t...
File net/tools/cert_verify_tool/cert_verify_tool.cc (right):

https://codereview.chromium.org/2305083002/diff/80001/net/tools/cert_verify_t...
net/tools/cert_verify_tool/cert_verify_tool.cc:133: } else if
(!hostname.empty()) {
On 2016/09/03 00:11:21, mattm wrote:
> this one shouldn't be !-ed though

oops i updated in a hurry, sorry about that.

[eroman, 2016-09-03 00:32:04]

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

[commit-bot: I haz the power, 2016-09-03 00:32:44]

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, 2016-09-03 01:12:36]

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

[commit-bot: I haz the power, 2016-09-03 01:12:37]

Dry run: This issue passed the CQ dry run.

[eroman, 2016-09-06 18:33:23]

Rebased

[eroman, 2016-09-06 18:37:57]

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

[commit-bot: I haz the power, 2016-09-06 18:38:30]

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, 2016-09-06 19:38:22]

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

[commit-bot: I haz the power, 2016-09-06 19:38:24]

Dry run: This issue passed the CQ dry run.

[mattm, 2016-09-06 22:10:50]

lgtm

https://codereview.chromium.org/2305083002/diff/140001/net/tools/cert_verify_...
File net/tools/cert_verify_tool/cert_verify_tool_util.h (right):

https://codereview.chromium.org/2305083002/diff/140001/net/tools/cert_verify_...
net/tools/cert_verify_tool/cert_verify_tool_util.h:37: // nothing is written to
|target| or |intermediates.
missing |

[eroman, 2016-09-06 22:32:50]

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

[eroman, 2016-09-06 22:32:57]

https://codereview.chromium.org/2305083002/diff/140001/net/tools/cert_verify_...
File net/tools/cert_verify_tool/cert_verify_tool_util.h (right):

https://codereview.chromium.org/2305083002/diff/140001/net/tools/cert_verify_...
net/tools/cert_verify_tool/cert_verify_tool_util.h:37: // nothing is written to
|target| or |intermediates.
On 2016/09/06 22:10:50, mattm wrote:
> missing |

Done.

[commit-bot: I haz the power, 2016-09-06 22:33:18]

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, 2016-09-06 23:12:30]

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

[commit-bot: I haz the power, 2016-09-06 23:12:31]

Dry run: This issue passed the CQ dry run.

[eroman, 2016-09-06 23:50:18]

The CQ bit was checked by eroman@chromium.org

[eroman, 2016-09-06 23:50:18]

The patchset sent to the CQ was uploaded after l-g-t-m from mattm@chromium.org
Link to the patchset: https://codereview.chromium.org/2305083002/#ps160001
(title: "correct comment")

[commit-bot: I haz the power, 2016-09-06 23:50:51]

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, 2016-09-06 23:54:34]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-09-06 23:57:48]

Description was changed from

==========
Misc changes to cert_verify_tool for errors

 * Correct a bug that prevented the display of per-path errors.
 * Add some additional help text for CLI, and reformat it.

BUG=634443
==========

to

==========
Misc changes to cert_verify_tool for errors

 * Correct a bug that prevented the display of per-path errors.
 * Add some additional help text for CLI, and reformat it.

BUG=634443

Committed: https://crrev.com/554ace1bfac664fbf6603c938aeef98914d5d4b4
Cr-Commit-Position: refs/heads/master@{#416780}
==========

[commit-bot: I haz the power, 2016-09-06 23:57:49]

Patchset 9 (id:??) landed as
https://crrev.com/554ace1bfac664fbf6603c938aeef98914d5d4b4
Cr-Commit-Position: refs/heads/master@{#416780}