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

Improve help message #33

Merged
merged 4 commits into from
Oct 10, 2023
Merged

Improve help message #33

merged 4 commits into from
Oct 10, 2023

Conversation

RufusJWB
Copy link
Collaborator

@RufusJWB RufusJWB commented Sep 28, 2023

Motivation

Improve documentation. Took us ages to find the list of allowed algorithms

Proposed Changes

Link to openssl header file in which the algorithms are defined.

Test Plan

n/a

@DDvO
Copy link
Member

DDvO commented Oct 6, 2023

Thank you also for this PR.

Hints on which MAC algorithms are available would have been found in the man page or online documentation,
as also written in the README.md:

The CLI use with the available options are documented in cmpClient.pod.

I am not sure if the link https://raw.githubusercontent.com/openssl/openssl/master/include/openssl/obj_mac.h that you suggested to add would be sufficiently simple to use and provides better info than the text given there:

To get the names of supported MAC algorithms use B<openssl list -mac-algorithms>
and possibly combine such a name with the name of a supported digest algorithm,
e.g., hmacWithSHA256.
Defaults to C as per RFC 4210.

Moreover, the CLI help output needs to be very brief and should not have overlong lines.
So I suggest modifying the PR as follows:

  • add to cmpClient.c after the existing text for -mac just a line like
    OPT_MORE("See the man page or online doc for hints on available algorithms"),
  • as far as you still see value in the providing that above URL, please add it to doc/cmpClient.pod.

@RufusJWB
Copy link
Collaborator Author

RufusJWB commented Oct 7, 2023

@DDvO I updated my PR based on your proposal. Please double check. But honestly, I don't find the man-page helpful. Take for example

image

Why is one algorithm called hmacWithSHA256 but the other hmac-sha1? And why is hmac-sha256 not working but hmac-sha3-224?

@DDvO
Copy link
Member

DDvO commented Oct 10, 2023

@DDvO I updated my PR based on your proposal. Please double check.

Looks good - nice that you added it for both -mac and -digest.

But honestly, I don't find the man-page helpful. Take for example

image

Why is one algorithm called hmacWithSHA256 but the other hmac-sha1? And why is hmac-sha256 not working but hmac-sha3-224?

This is most likely due to the legacy and wonders of IETF & IANA standardization.
As finding a supported combination of names is not straightforward, I hinted at possibly also adding the URL you gave to cmpClient.pod.
I suggest adding there before the line

Defaults to C<hmac-sha1> as per RFC 4210.

something like

L<https://raw.githubusercontent.com/openssl/openssl/master/include/openssl/obj_mac.h>
contains the complete set of combinations of MAC and digest algorithm names.

@RufusJWB
Copy link
Collaborator Author

@DDvO please double check again.

Copy link
Member

@DDvO DDvO left a comment

Choose a reason for hiding this comment

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

LGTM - thanks for contributing.

@DDvO DDvO merged commit f976d9b into siemens:master Oct 10, 2023
@RufusJWB RufusJWB deleted the patch-3 branch October 10, 2023 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants