Skip to content

Conversation

@richsalz
Copy link
Contributor

Fixes: #9936

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

@richsalz
Copy link
Contributor Author

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

@richsalz
Copy link
Contributor Author

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 changed the title WIP: Add section heads to help commands. Add section heads to help commands. Sep 22, 2019
@richsalz
Copy link
Contributor Author

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

@richsalz
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.

Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few nits

@richsalz
Copy link
Contributor Author

richsalz commented Oct 8, 2019

fixup commit pushed.

Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work!

@t8m t8m added branch: master Applies to master branch approval: review pending This pull request needs review by a committer labels Oct 9, 2019
Copy link
Member

@levitte levitte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm getting blurry-eyed... I'll get back on this, but wanted to at least get a few findings out.

@richsalz
Copy link
Contributor Author

richsalz commented Oct 9, 2019

As for the Input, Output, Input/Output: if there were a bunch of options, or I thought it made sense, I divided them into sections, otherwise I didn't. I agree it might be better to go for consistency, but I also dislike a section that has only one option (yes sometimes that is unavoidable).

I hope that this topic can be addressed in a separate PR. (Probably by someone else :)

@t8m
Copy link
Member

t8m commented Oct 9, 2019

My approval still holds.

@richsalz richsalz mentioned this pull request Oct 9, 2019
2 tasks
@t8m
Copy link
Member

t8m commented Oct 16, 2019

ping @openssl/omc for 2nd review

@richsalz
Copy link
Contributor Author

Another sample, to entice reviewers:

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

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

Input options:
 -in infile        Input file
 -inform PEM|DER   Input format - DER or PEM

Output options:
 -outform PEM|DER  Output format - DER or PEM
 -out outfile      Output file
 -noout            Don't output encoded data
 -text             Print full details of certificates
 -print            Print out all fields of the PKCS7 structure
 -print_certs      Print_certs  print any certs or crl in the input
; 

@levitte
Copy link
Member

levitte commented Oct 22, 2019

Travis failures seem relevant, there seems to be some option duplication in some commands.

@t8m
Copy link
Member

t8m commented Oct 22, 2019

Yeah, the rebase broke the apps/enc.c. There are added duplicate options in, out, and pass.

@t8m
Copy link
Member

t8m commented Oct 23, 2019

OK, re-approved now.
ping @levitte ?

@mspncp mspncp added approval: omc review pending and removed approval: review pending This pull request needs review by a committer labels Oct 25, 2019
@richsalz
Copy link
Contributor Author

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
Copy link
Member

t8m commented Oct 30, 2019

For the record my approval still holds.

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
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
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 vdukhovni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"Print old-style (MD5) subject hash value"},
#endif

OPT_SECTION("Certificate"),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
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 paulidale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

richsalz commented Nov 6, 2019

I don't understand your question.

@richsalz
Copy link
Contributor Author

richsalz commented Nov 6, 2019

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

@paulidale
Copy link
Contributor

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.

@paulidale paulidale added approval: done This pull request has the required number of approvals and removed approval: omc review pending labels Nov 6, 2019
@richsalz
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
Copy link
Contributor

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

@paulidale paulidale closed this Nov 7, 2019
@richsalz richsalz deleted the app-help-categories branch November 7, 2019 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval: done This pull request has the required number of approvals branch: master Applies to master branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add group options to apps

6 participants