Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add section heads to help commands. #9953

Closed
wants to merge 6 commits into from

Conversation

@richsalz
Copy link
Contributor

richsalz commented Sep 20, 2019

Fixes: #9936

This is a WIP. When done I'll remove the TODO file.

@richsalz richsalz force-pushed the richsalz:app-help-categories branch from bcd1968 to 8303d11 Sep 20, 2019
@richsalz

This comment has been minimized.

Copy link
Contributor Author

richsalz commented Sep 20, 2019

If someone can take a look at the output, I'd like an opinion of there should be blank line before each section.

@richsalz richsalz force-pushed the richsalz:app-help-categories branch 4 times, most recently from c1d6f96 to daaeb2f Sep 20, 2019
@richsalz

This comment has been minimized.

Copy link
Contributor Author

richsalz commented Sep 21, 2019

First pass done; all programs with >=20 help lines: asn1parse ca cms crl dgst ecparam enc ocsp pkcs12 pkcs8 pkeyutl req rsa rsautl s_client s_server smime speed ts verify x509

@richsalz richsalz force-pushed the richsalz:app-help-categories branch from 4186f59 to b7fc7d1 Sep 22, 2019
@richsalz richsalz changed the title WIP: Add section heads to help commands. Add section heads to help commands. Sep 22, 2019
@richsalz

This comment has been minimized.

Copy link
Contributor Author

richsalz commented Sep 22, 2019

All commands done, out of WIP, ready for review.

@richsalz richsalz force-pushed the richsalz:app-help-categories branch 2 times, most recently from 6dea9f0 to b276a99 Sep 24, 2019
@richsalz richsalz force-pushed the richsalz:app-help-categories branch 6 times, most recently from 15d5bbd to 4652017 Sep 28, 2019
@richsalz

This comment has been minimized.

Copy link
Contributor Author

richsalz commented Oct 5, 2019

in order to entice folks to look at and review this, here's some output:

; ./apps/openssl rsa --help
Usage: rsa [options]

General options:
 -help              Display this summary
 -check             Verify key consistency
 -*                 Any supported cipher
 -engine val        Use engine, possibly a hardware device

Input options:
 -in val            Input file
 -inform format     Input format, one of DER PEM
 -pubin             Expect a public key in input file
 -RSAPublicKey_in   Input is an RSAPublicKey
 -passin val        Input file pass phrase source

Output options:
 -out outfile       Output file
 -outform format    Output format, one of DER PEM PVK
 -pubout            Output a public key
 -RSAPublicKey_out  Output is an RSAPublicKey
 -passout val       Output file pass phrase source
 -noout             Don't print key out
 -text              Print the key in text
 -modulus           Print the RSA key modulus

PVK options:
 -pvk-strong        Enable 'Strong' PVK encoding level (default)
 -pvk-weak          Enable 'Weak' PVK encoding level
 -pvk-none          Don't enforce PVK encoding
; 
; ./apps/openssl rand --help
Usage: rand [flags] num

General options:
 -help               Display this summary
 -engine val         Use engine, possibly a hardware device

Output options:
 -out outfile        Output file
 -base64             Base64 encode output
 -hex                Hex encode output

Random state options:
 -rand val           Load the file(s) into the random number generator
 -writerand outfile  Write random data to the specified file

For common options (OPT_[VSXR]_OPTIONS in apps/lib/opt.c), as shown in the "Random state options" text above, the prolog is common text.

@richsalz richsalz force-pushed the richsalz:app-help-categories branch 2 times, most recently from 3c4f4d1 to de9caf5 Oct 5, 2019
Copy link
Member

t8m left a comment

Just a few nits

apps/fipsinstall.c Outdated Show resolved Hide resolved
apps/pkcs8.c Show resolved Hide resolved
@richsalz richsalz force-pushed the richsalz:app-help-categories branch from de9caf5 to 029b59e Oct 8, 2019
@richsalz

This comment has been minimized.

Copy link
Contributor Author

richsalz commented Oct 8, 2019

fixup commit pushed.

@t8m
t8m approved these changes Oct 9, 2019
Copy link
Member

t8m left a comment

Good work!

@richsalz

This comment has been minimized.

Copy link
Contributor Author

richsalz commented Oct 29, 2019

Another sample:

; ./apps/openssl ocsp --help
Usage: ocsp [options]

General options:
 -help                   Display this summary
 -ignore_err             Ignore error on OCSP request or response and continue running
 -CAfile infile          Trusted certificates file
 -CApath infile          Trusted certificates directory
 -no-CAfile              Do not load the default certificates file
 -no-CApath              Do not load certificates from the default certificates directory

Responder options:
 -timeout +int           Connection timeout (in seconds) to the OCSP responder
 -resp_no_certs          Don't include any certificates in response
 -multi +int             run multiple responder processes
 -no_certs               Don't include any certificates in signed request
 -badsig                 Corrupt last byte of loaded OSCP response signature (for test)
 -CA infile              CA certificate
 -nmin +int              Number of minutes before next update
 -nrequest +int          Number of requests to accept (default unlimited)
 -reqin val              File with the DER-encoded request
 -signer infile          Certificate to sign OCSP request with
 -sign_other infile      Additional certificates to include in signed request
 -index infile           Certificate status index file
 -ndays +int             Number of days before next update
 -rsigner infile         Responder certificate to sign responses with
 -rkey infile            Responder key to sign responses with
 -rother infile          Other certificates to include in response
 -rmd val                Digest Algorithm to use in signature of OCSP response
 -rsigopt val            OCSP response signature parameter in n:v form
 -header val             key=value header to add
 -rcid val               Use specified algorithm for cert id in response
 -*                      Any supported digest algorithm (sha1,sha256, ... )

Client options:
 -url val                Responder URL
 -host val               TCP/IP hostname:port to connect to
 -port +int              Port to run responder on
 -out outfile            Output filename
 -noverify               Don't verify response at all
 -nonce                  Add OCSP nonce to request
 -no_nonce               Don't add OCSP nonce to request
 -no_signature_verify    Don't check signature on response
 -resp_key_id            Identify response by signing certificate key ID
 -no_cert_verify         Don't check signing certificate
 -text                   Print text form of request and response
 -req_text               Print text form of request
 -resp_text              Print text form of response
 -no_chain               Don't chain verify response
 -no_cert_checks         Don't do additional checks on signing certificate
 -no_explicit            Do not explicitly check the chain, just verify the root
 -trust_other            Don't verify additional certificates
 -no_intern              Don't search certificates contained in response for signer
 -respin val             File with the DER-encoded response
 -VAfile infile          Validator certificates file
 -verify_other infile    Additional certificates to search for signer
 -path val               Path to use in OCSP request
 -cert infile            Certificate to check
 -serial val             Serial number to check
 -validity_period ulong  Maximum validity discrepancy in seconds
 -signkey val            Private key to sign OCSP request with
 -reqout val             Output file for the DER-encoded request
 -respout val            Output file for the DER-encoded response
 -issuer infile          Issuer certificate
 -status_age +int        Maximum status age in seconds

Validation options:
 -policy val             adds policy to the acceptable policy set
 -purpose val            certificate chain purpose
 -verify_name val        verification policy name
 -verify_depth int       chain depth limit
 -auth_level int         chain authentication security level
 -attime intmax          verification epoch time
 -verify_hostname val    expected peer hostname
 -verify_email val       expected peer email
 -verify_ip val          expected peer IP address
 -ignore_critical        permit unhandled critical extensions
 -issuer_checks          (deprecated)
 -crl_check              check leaf certificate revocation
 -crl_check_all          check full chain revocation
 -policy_check           perform rfc5280 policy checks
 -explicit_policy        set policy variable require-explicit-policy
 -inhibit_any            set policy variable inhibit-any-policy
 -inhibit_map            set policy variable inhibit-policy-mapping
 -x509_strict            disable certificate compatibility work-arounds
 -extended_crl           enable extended CRL features
 -use_deltas             use delta CRLs
 -policy_print           print policy processing diagnostics
 -check_ss_sig           check root CA self-signatures
 -trusted_first          search trust store first (default)
 -suiteB_128_only        Suite B 128-bit-only mode
 -suiteB_128             Suite B 128-bit mode allowing 192-bit algorithms
 -suiteB_192             Suite B 192-bit-only mode
 -partial_chain          accept chains anchored by intermediate trust-store CAs
 -no_alt_chains          (deprecated)
 -no_check_time          ignore certificate validity time
 -allow_proxy_certs      allow the use of proxy certificates
; 
@t8m

This comment has been minimized.

Copy link
Member

t8m commented Oct 30, 2019

For the record my approval still holds.

richsalz added 5 commits Sep 20, 2019
Remove "Valid options" label, since all commands have sections (and
[almost] always the first one is "General options").
Have "list --options" ignore section headers
Reformat ts's additional help
@richsalz richsalz force-pushed the richsalz:app-help-categories branch from 5414f8b to 21ee462 Nov 4, 2019
@richsalz

This comment has been minimized.

Copy link
Contributor Author

richsalz commented Nov 4, 2019

I had to rebase this because #8442 added new flags to several commands. There were, of course, merge conflicts. No other changes were made.

Seeking reviews. First opened September 19. No feedback (except poor patient @t8m :) for nearly a month.

@t8m

This comment has been minimized.

Copy link
Member

t8m commented Nov 5, 2019

My approval still holds. It would be nice if @openssl/omc provided the second review soon to avoid further merge conflicts.

Copy link

vdukhovni left a comment

Looks well motivated overall, but I am requesting some changes.

apps/ciphers.c Outdated Show resolved Hide resolved
apps/ciphers.c Outdated Show resolved Hide resolved
apps/ciphers.c Outdated Show resolved Hide resolved
apps/ciphers.c Outdated Show resolved Hide resolved
apps/ciphers.c Outdated Show resolved Hide resolved
apps/s_time.c Show resolved Hide resolved
apps/smime.c Show resolved Hide resolved
apps/verify.c Show resolved Hide resolved
"Print old-style (MD5) subject hash value"},
#endif

OPT_SECTION("Certificate"),

This comment has been minimized.

Copy link
@vdukhovni

vdukhovni Nov 5, 2019

This mixes certificate creation options with certificate output options, probably should be separate.

This comment has been minimized.

Copy link
@richsalz

richsalz Nov 5, 2019

Author Contributor

The only output-related option I could see is "-nameopt" which I moved to output. Did you8 have others in mind?

{"force_pubkey", OPT_FORCE_PUBKEY, '<', "Force the key to put inside certificate"},
{"subj", OPT_SUBJ, 's', "Set or override certificate subject (and issuer)"},

OPT_SECTION("CA"),

This comment has been minimized.

Copy link
@vdukhovni

vdukhovni Nov 5, 2019

These are certificate creation options, should perhaps include some of the same from above.

This comment has been minimized.

Copy link
@richsalz

richsalz Nov 5, 2019

Author Contributor

Can you give me more details? Which options should move, and to which section?

Address some of Viktor's feedback.
@richsalz richsalz mentioned this pull request Nov 6, 2019
@t8m

This comment has been minimized.

Copy link
Member

t8m commented Nov 6, 2019

I believe we should focus on real errors/mistakes and not try to achieve perfection in this PR. IMO even in the state as it is now it would be major improvement over the current situation.

Copy link
Contributor

paulidale left a comment

Looks good.
It there any rsason for the OPT_MORE_STR lines are required? Thye'd be construable from the raw test I suspect.

@richsalz

This comment has been minimized.

Copy link
Contributor Author

richsalz commented Nov 6, 2019

I don't understand your question.

@richsalz

This comment has been minimized.

Copy link
Contributor Author

richsalz commented Nov 6, 2019

(PS: Want to tweak the labels so the 24hour hold timer works?)

@paulidale

This comment has been minimized.

Copy link
Contributor

paulidale commented Nov 6, 2019

I'm wondering why there are OPT_MORE_STR lines in the text. They seem to be manually putting line breaks in places -- this could be automated.

It's quite poissible I missing something egregious.

@richsalz

This comment has been minimized.

Copy link
Contributor Author

richsalz commented Nov 6, 2019

The number really didn't change. Auto-wrapping the text would be more trouble than it's worth for the roughly-a-dozen lines:

; g m
Switched to branch 'master'
Your branch is up-to-date with 'origin/master'.
; g grep OPT_MORE_STR | wc 
     14     105     859
; g co app-help-categories 
Switched to branch 'app-help-categories'
; g grep OPT_MORE_STR | wc 
     13      91     750
; 
openssl-machine pushed a commit that referenced this pull request Nov 7, 2019
Remove "Valid options" label, since all commands have sections (and
[almost] always the first one is "General options").
Have "list --options" ignore section headers
Reformat ts's additional help

Add output section

Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #9953)
@paulidale

This comment has been minimized.

Copy link
Contributor

paulidale commented Nov 7, 2019

Merged to master. Thanks for the effort and endurance on of this one.

@paulidale paulidale closed this Nov 7, 2019
@richsalz richsalz deleted the richsalz:app-help-categories branch Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.