I did some editing on the verification options in openssl.pod, and I tried to put them into a more sensible ordering. Once this is reviewed and merged, I will change OPT_V_OPTIONS
in include/opt.h to match the ordering.

This is now based on #10159 and needs that to go in first.

Rebased since #10159 was merged.

re-pushing to restart hung CI jobs.

rebased and fixed conflicts.

Thanks for reading this, @vdukhovni I appreciate it. Fixup commit pushed.

I had to rebase because of a conflict. I fixed various failure descriptions and moved the verification error description all to the man3 file. That file now has the define name and the text. Should the text, which is basically a repetition of the text description, be removed? I think so.

Travis failure is a timeout.

I restarted the job.

Ping for a second review. Should be easy, it made it past one of the harder reviewers already :) And #10587 is kinda waiting on this.

There were conflicts, so I did a squash/rebase. The conflicts were simple, but this probably needs another review from @vdukhovni (as well as a committer).

I generated the pod files using 8062c027e9cdef644f1e63b210384e760c65a354 and its parent, see attached comparison.zip. For better comparison I sorted the options in the SYNOPSIS section.

The diff shows some duplicates (-CAfile, -CApath, -certfile) and some options which have disappeared (-no_check_time) or appeared (-verbose). I think the duplicates need to be removed and the other changes need a closer look and/or an explanation why they were changed.

See for example openssl-cms.pod:

msp@msppc:~/src/openssl$ diff before/openssl-cms.pod after/openssl-cms.pod
--- before/openssl-cms.pod	2020-01-06 16:25:32.978669176 +0100
+++ after/openssl-cms.pod	2020-01-06 16:26:16.407071393 +0100
@@ -15,6 +15,8 @@
 
 B<openssl> B<cms>
 [B<-CAfile> I<file>]
+[B<-CAfile> I<file>]
+[B<-CApath> I<dir>]
 [B<-CApath> I<dir>]
 [B<-CAstore> I<uri>]
 [B<-EncryptedData_encrypt>]
@@ -25,6 +27,7 @@
 [B<-binary>]
 [B<-cades>]
 [B<-certfile> I<file>]
+[B<-certfile> I<file>]
 [B<-certsout> I<file>]
 [B<-check_ss_sig>]
 [B<-cmsout>]
@@ -56,10 +59,11 @@
 [B<-keyopt> I<name>:I<parameter>]
 [B<-md> I<digest>]
 [B<-no-CAfile>]
+[B<-no-CAfile>]
+[B<-no-CApath>]
 [B<-no-CApath>]
 [B<-no-CAstore>]
 [B<-no_alt_chains>]
-[B<-no_check_time>]
 [B<-noattr>]
 [B<-nocerts>]
 [B<-nodetach>]
@@ -102,6 +106,7 @@
 [B<-trusted_first>]
 [B<-uncompress>]
 [B<-use_deltas>]
+[B<-verbose>]
 [B<-verify>]
 [B<-verify_depth> I<num>]
 [B<-verify_email> I<email>]
@@ -476,14 +481,16 @@
 
 =item B<-attime>, B<-check_ss_sig>, B<-crl_check>, B<-crl_check_all>,
 B<-explicit_policy>, B<-extended_crl>, B<-ignore_critical>, B<-inhibit_any>,
-B<-inhibit_map>, B<-no_alt_chains>, B<-no_check_time>, B<-partial_chain>, B<-policy>,
+B<-inhibit_map>, B<-no_alt_chains>, B<-partial_chain>, B<-policy>,
 B<-policy_check>, B<-policy_print>, B<-purpose>, B<-suiteB_128>,
 B<-suiteB_128_only>, B<-suiteB_192>, B<-trusted_first>, B<-use_deltas>,
 B<-auth_level>, B<-verify_depth>, B<-verify_email>, B<-verify_hostname>,
 B<-verify_ip>, B<-verify_name>, B<-x509_strict>
 
-Set various certificate chain validation options. See the
-L<openssl-verify(1)> manual page for details.
+Set various options of certificate chain verification.
+See L<openssl(1)/Verification Options> for details.
+
+Any verification errors cause the command to exit.
 
 =item B<-CAfile> I<file>, B<-no-CAfile>, B<-CApath> I<dir>, B<-no-CApath>,
 B<-CAstore> I<uri>, B<-no-CAstore>

Note: I did not go through all the diffs yet.

The duplicates (CApath,no-CApath,CAfile,no-CAfile,certfile) where rebase mistakes that slipped through; fixup commit pushed. The no_check_time isn't implemented so I removed it. As you go through the review, I'll fix each one in turn. Thanks!

The no_check_time isn't implemented so I removed it.

Are you sure it's not implemented?

$ git grep -in no_check_time apps
apps/include/opt.h:30: OPT_V_PARTIAL_CHAIN, OPT_V_NO_ALT_CHAINS, OPT_V_NO_CHECK_TIME,
apps/include/opt.h:82: { "no_check_time", OPT_V_NO_CHECK_TIME, '-', "ignore certificate validity time" },
apps/include/opt.h:115: case OPT_V_NO_CHECK_TIME:
apps/lib/opt.c:608: case OPT_V_NO_CHECK_TIME:
apps/lib/opt.c:609: X509_VERIFY_PARAM_set_flags(vpm, X509_V_FLAG_NO_CHECK_TIME);

Thanks @mspncp you're right. Lucky a simple fix in only one file fixes it everywhere :)

-no_check_time

Did you restore -no_check_time already?

More duplicates

I see more duplicates of the following options:

-CAfile, -CApath, -no-CAfile, -no-CApath

I'm sure you'll find them all using grep ;-)

Verbosity

The -verbose option was somehow 'inverted': it is now documented where it previously wasn't and vice versa.

msp@msppc:~/src/openssl$ grep -- -verbose before/*
before/openssl-verify.pod:[B<-verbose>]
before/openssl-verify.pod:=item B<-verbose>

msp@msppc:~/src/openssl$ grep -- -verbose after/*
after/openssl-cms.pod:[B<-verbose>]
after/openssl-ocsp.pod:[B<-verbose>]
after/openssl-s_client.pod:[B<-verbose>]
after/openssl-s_server.pod:[B<-verbose>]
after/openssl-smime.pod:[B<-verbose>]
after/openssl-ts.pod:[B<-verbose>]

According to my quick test, none of the latter commands supports this flag:

msp@msppc:~/src/openssl$ for cmd in cms ocsp s_client smime ts; do openssl $cmd -verbose ; done

cms: Unrecognized flag verbose
ocsp: Unrecognized flag verbose
ocsp: Use -help for summary.
s_client: Option unknown option -verbose
s_client: Use -help for summary.
smime: Unrecognized flag verbose
smime: Use -help for summary.
ts: Unrecognized flag verbose
ts: Use -help for summary.

Apropos: the missing-flag error messages are also very higgledy-piggledy:

cms: Unrecognized flag verbose
ocsp: Unrecognized flag verbose
ocsp: Use -help for summary.
s_client: Option unknown option -verbose
s_client: Use -help for summary.
smime: Unrecognized flag verbose
smime: Use -help for summary.
ts: Unrecognized flag verbose
ts: Use -help for summary.

Could you open a separate issue for #10132 (comment) ? Thanks.

Could you open a separate issue for #10132 (comment) ? Thanks.

Done, see #10773.

@vdukhovni any chance you'll be able to take another (quick) look, or should I try to find someone else?

Rebased to address .gitignore conflict. Also squashed most fixup commits. Still needs review.

@vdukhovni any chance you'll be able to take another (quick) look, or should I try to find someone else?

Cycles a bit scarce lately, I hope someone else will take you across the finish line. At least I got the ball rolling. Thanks for hanging in to the bitter end!

Travis red cross not relevant. See #10932

Rebased to pick up the fix in #10932; no other changes made.

Merged to master in 21d08b9, thanks for your patience :-)

Thanks for the grammar correction, folks. Sheesh. 3:1 you were right and i was wrong.