Add option grouping support to openssl apps by jon-oracle · Pull Request #9920 · openssl/openssl

jon-oracle · 2019-09-17T07:36:35Z

Added a way to group options which is backward compatible with existing apps. The new function is opt_help_ex() which accepts a set of option groups.

As an example the output from 'openssl pkcs12 --help' could be changed to:

Usage: pkcs12 [options]
General options:
 -help                       Display this summary
 -keyex                      Set MS key exchange type
 -keysig                     Set MS key signature type
 -chain                      Add certificate chain
 -rand val                   Load the file(s) into the random number generator
 -writerand outfile          Write random data to the specified file
 -caname val                 Use name as CA friendly name (can be repeated)
 -engine val                 Use engine, possibly a hardware device
Password options:
 -twopass                    Separate MAC, encryption passwords
 -passin val                 Input file pass phrase source
 -passout val                Output file pass phrase source
 -password val               Set import/export password source
Encryption options:
 -descert                    Encrypt output with 3DES (default RC2-40)
 -certpbe val                Certificate PBE algorithm (default RC2-40)
 -noiter                     Don't use encryption iteration
 -nodes                      Don't encrypt private keys
 -keypbe val                 Private key PBE algorithm (default 3DES)
 -*                          Any supported cipher
MAC options:
 -nomacver                   Don't verify MAC
 -maciter                    Use MAC iteration
 -nomaciter                  Don't use MAC iteration
 -nomac                      Don't generate MAC
 -macalg val                 Digest algorithm used in MAC (default SHA1)
CA Certificate options:
 -CApath dir                 PEM-format directory of CA's
 -CAfile infile              PEM-format file of CA's
 -no-CAfile                  Do not load the default certificates file
 -no-CApath                  Do not load certificates from the default certificates directory
Attribute options:
 -LMK                        Add local machine keyset attribute to private key
 -name val                   Use name as friendly name
 -CSP val                    Microsoft CSP name
Input options:
 -inkey val                  Private key if not infile
 -certfile infile            Load certs from file
 -in infile                  Input filename
Output options:
 -nokeys                     Don't output private keys
 -nocerts                    Don't output certificates
 -clcerts                    Only output client certificates
 -cacerts                    Only output CA certificates
 -noout                      Don't output anything, just verify
 -info                       Print info about PKCS#12 structure
 -export                     Output PKCS12 file
 -out outfile                Output filename

levitte · 2019-09-17T08:06:58Z

apps/pkcs12.c

+
+const char* pkcs12_opt_groups[] = {
+    "General", "Password", "Encryption", "MAC", "CA Certificate", "Attribute", "Input", "Output"
+};


I would very much prefer if there was a struct { int groupnum; const char *groupname } for the elements of this array, so it could be given like this:

struct GROUPS pkcs12_groups[] = { { OPT_GRP_GENERAL, "General" }, { OPT_GRP_PASSWORD, "Password" }, { OPT_GRP_ENCRYPT, "Encryption" }, { OPT_GRP_MAC, "MAC" }, { OPT_GRP_CA, "CA Certificate" }, { OPT_GRP_ATTR, "Attribute" }, { OPT_GRP_INPUT, "Input" }, { OPT_GRP_OUTPUT, "Output" }, };

(incidently, there's already a type you can use rather than defining GROUP, called OSSL_ITEM, found in include/openssl/core.h)

Having to synchronise the positions in an array with a separate enum is a maintenance problem waiting to happen. We've had that in the past, which is why I'm pushing for something that makes the connection more explicit.

t8m · 2019-09-17T10:14:57Z

Do you have a plan to apply this to all the commands - at least all of them which have more than a few options?

richsalz · 2019-09-17T12:32:44Z

This really isn't needed.

; ./apps/openssl pkcs12 -help
General commands:
 -help               Display this summary
 -nokeys             Don't output private keys
 -keyex              Set MS key exchange type
Other commands:
 -keysig             Set MS key signature type
 -nocerts            Don't output certificates
...

Diff:

; g diff
diff --git a/apps/pkcs12.c b/apps/pkcs12.c
index a893553..f188502 100644
--- a/apps/pkcs12.c
+++ b/apps/pkcs12.c
@@ -61,9 +61,11 @@ typedef enum OPTION_choice {
 } OPTION_CHOICE;
 
 const OPTIONS pkcs12_options[] = {
+    {OPT_HELP_STR, 1, '-', "General commands:\n"},
     {"help", OPT_HELP, '-', "Display this summary"},
     {"nokeys", OPT_NOKEYS, '-', "Don't output private keys"},
     {"keyex", OPT_KEYEX, '-', "Set MS key exchange type"},
+    {OPT_HELP_STR, 1, '-', "Other commands:\n"},
     {"keysig", OPT_KEYSIG, '-', "Set MS key signature type"},
     {"nocerts", OPT_NOCERTS, '-', "Don't output certificates"},
     {"clcerts", OPT_CLCERTS, '-', "Only output client certificates"},
;

richsalz · 2019-09-17T12:50:47Z

And, of course you can make it more explicit by doing something like this:

#define OPT_STR(x) #x
#define OPT_SECTION(what) { +    {OPT_HELP_STR, 1, '-', OPT_STR(x) " commands:\n"}

But beware Perlis's quote :)

levitte · 2019-09-17T13:16:26Z

I actually like @richsalz's solution. It's simple, it makes the groups immediately visible, ...

levitte · 2019-09-17T13:19:14Z

But beware Perlis's quote :)

I had to look that up...

richsalz · 2019-09-17T13:34:04Z

It’s #3 in the epigrams :) Also, Andy was strongly opposed to this kind of thing.

jon-oracle · 2019-09-18T04:09:36Z

And, of course you can make it more explicit by doing something like this:
#define OPT_STR(x) #x
#define OPT_SECTION(what) { +    {OPT_HELP_STR, 1, '-', OPT_STR(x) " commands:\n"}
But beware Perlis's quote :)

Yeah this does look clean :) I've added something similar to this instead.

levitte · 2019-09-18T04:33:52Z

apps/lib/opt.c

+    char start[80 + 1];
+
+    /* Starts with its own help message? */
+    standard_prolog = list[0].name != OPT_HELP_STR;


Soooo... this is a bit of a probem. What if someone, somewhere, has this:

const OPTIONS foo[] = { OPT_SECTION("General"), ...

How to decide it the resulting OPTIONS item is a section start or a usage?

One quick and fairly easy solution is to have another magic string called OPT_USAGE_STR and replace all things looking like this:

{OPT_HELP_STR, 1, '-'. "Usage: ..."},

with

{OPT_USAGE_STR, 1, '-'. "Usage: ..."},

The other option is to have a magic OPT_SECTION_STR instead of re-using OPT_HELP_STR. That's probably going to be less work.

Yeah using OPT_SECTION_STR seems like a smaller change that won't break what's already there.

slontis · 2019-09-18T05:14:59Z

Do you have a plan to apply this to all the commands - at least all of them which have more than a few options?

That would maybe take quite a bit of time and should be outside the scope of this PR.
Think of something like s_client with pages of options..

I can imagine there are going to be items that belong to multiple subgroups somewhere..

levitte

So, I like this! Assuming the CIs do to, I'm all for it.

slontis · 2019-09-18T07:50:10Z

Would this need to be in 1_1_1 ?

levitte · 2019-09-18T08:30:43Z

Would this need to be in 1_1_1 ?

... nah. Or well, I dunno, but don't see that as necessary

t8m · 2019-09-18T10:31:37Z

I do not think this belongs to 1_1_1.
As for the other commands - I am not saying fixing them should be part of this PR, but for consistency for the 3.0 release I think the same approach should be applied across all the commands (or at least the more complicated ones). So that's why I am asking the PR author if he has an intention to work on them.

slontis · 2019-09-18T10:43:50Z

So that's why I am asking the PR author if he has an intention to work on them.

Jon will be busy on other projects - so the answer is no.
I don't think it is reasonable to ask him to fix them...
Someone else will need to take up the task if this is required.

levitte · 2019-09-18T10:54:36Z

I think it's fine for this PR to serve as inspiration to others.

richsalz · 2019-09-18T12:17:11Z

I think it's fine for this PR to serve as inspiration to others.

I understand that, but it's too bad. One of the nice things about the commands is that the flags parsing and behavior for common options is that, starting with 1.1, they all became consistent -- and had consistent help. I did most of that work, so I'm biased, but I think that was a big step forward for OpenSSL's usability. It is a shame to step back from that.

Perhaps this PR can just do any infrastructure changes and wait until someone steps up to do the grouping of command flags.

levitte · 2019-09-18T12:19:41Z

Perhaps this PR can just do any infrastructure changes and wait until someone steps up to do the grouping of command flags.

Considering what @slontis said about @jon-oracle, I would refrain from making many more demands. I'm thinking of taking this up myself, should no one else take up the gauntlet, but not immediately... I've already quite a bunch of things in my queue.

richsalz · 2019-09-18T12:38:02Z

I am asking him to do less. Remove the pkcs12.c changes.

slontis · 2019-09-18T12:46:23Z

I am asking him to do less. Remove the pkcs12.c changes.

And put them in a second PR? that will be on hold..

richsalz · 2019-09-18T12:52:20Z

And put them in a second PR? that will be on hold..

yes.

levitte · 2019-09-18T15:49:13Z

I think you and I mean different things, @richsalz. Undoing is yet another thing to do.

richsalz · 2019-09-18T16:33:34Z

Undoing is yet another thing to do.

It's less work than following this thread, ain't it:

; git co pks12-options
; git show master:apps/pkcs12.c >apps/pkcs12.c
; git commit --amend --no-edit apps/pkcs12.c
; git push -f github HEAD

jon-oracle · 2019-09-19T00:36:56Z

Undoing is yet another thing to do.

It's less work than following this thread, ain't it:
; git co pks12-options
; git show master:apps/pkcs12.c >apps/pkcs12.c
; git commit --amend --no-edit apps/pkcs12.c
; git push -f github HEAD

Your git-fu is better than mine ;)

I think the last force push did the trick, let me know if it didn't.

slontis · 2019-09-19T05:44:33Z

Can you change the commit message and title now that this does something different from its original purpose..

Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Shane Lontis <[email protected]> (Merged from #9920)

slontis · 2019-09-19T11:17:53Z

Thanks for reviewing. Merged to master.

Add option grouping to pkcs12 app

2bdeaee

levitte requested changes Sep 17, 2019

View reviewed changes

Add option grouping to pkcs12 app

1b263a6

Add option grouping to pkcs12 app

ad3e8f5

levitte requested changes Sep 18, 2019

View reviewed changes

levitte approved these changes Sep 18, 2019

View reviewed changes

slontis added the branch: master Applies to master branch label Sep 18, 2019

slontis mentioned this pull request Sep 18, 2019

Add group options to apps #9936

Closed

jon-oracle force-pushed the pkcs12-options branch from b25eaa0 to c9488fd Compare September 19, 2019 00:35

levitte approved these changes Sep 19, 2019

View reviewed changes

Add option grouping capability to apps

b9ea711

jon-oracle force-pushed the pkcs12-options branch from c9488fd to b9ea711 Compare September 19, 2019 06:21

slontis approved these changes Sep 19, 2019

View reviewed changes

slontis changed the title ~~Add option grouping to pkcs12 app~~ Add option grouping support to openssl apps Sep 19, 2019

slontis added the approval: done This pull request has the required number of approvals label Sep 19, 2019

levitte pushed a commit that referenced this pull request Sep 19, 2019

Add option grouping capability to apps

4ed8389

Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Shane Lontis <[email protected]> (Merged from #9920)

slontis closed this Sep 19, 2019

jon-oracle deleted the pkcs12-options branch February 12, 2021 01:45

Uh oh!

Conversation

jon-oracle commented Sep 17, 2019 • edited by slontis Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

levitte Sep 17, 2019

Choose a reason for hiding this comment

Uh oh!

t8m commented Sep 17, 2019

Uh oh!

richsalz commented Sep 17, 2019

Uh oh!

richsalz commented Sep 17, 2019

Uh oh!

levitte commented Sep 17, 2019

Uh oh!

levitte commented Sep 17, 2019

Uh oh!

richsalz commented Sep 17, 2019 via email

Uh oh!

jon-oracle commented Sep 18, 2019

Uh oh!

levitte Sep 18, 2019

Choose a reason for hiding this comment

Uh oh!

levitte Sep 18, 2019

Choose a reason for hiding this comment

Uh oh!

jon-oracle Sep 18, 2019

Choose a reason for hiding this comment

Uh oh!

slontis commented Sep 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

levitte left a comment

Choose a reason for hiding this comment

Uh oh!

slontis commented Sep 18, 2019

Uh oh!

levitte commented Sep 18, 2019

Uh oh!

t8m commented Sep 18, 2019

Uh oh!

slontis commented Sep 18, 2019

Uh oh!

levitte commented Sep 18, 2019

Uh oh!

richsalz commented Sep 18, 2019

Uh oh!

levitte commented Sep 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richsalz commented Sep 18, 2019

Uh oh!

slontis commented Sep 18, 2019

Uh oh!

richsalz commented Sep 18, 2019

Uh oh!

levitte commented Sep 18, 2019

Uh oh!

richsalz commented Sep 18, 2019

Uh oh!

jon-oracle commented Sep 19, 2019

Uh oh!

slontis commented Sep 19, 2019

Uh oh!

slontis commented Sep 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jon-oracle commented Sep 17, 2019 •

edited by slontis

Loading

slontis commented Sep 18, 2019 •

edited

Loading

levitte commented Sep 18, 2019 •

edited

Loading