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.

Sign up for GitHub

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

Jump to bottom

ts(1): digest option is mandatory #8421

Closed
wants to merge 1 commit into from
Closed

ts(1): digest option is mandatory #8421

wants to merge 1 commit into from

Conversation

tomato42
Copy link
Contributor

@tomato42 tomato42 commented Mar 6, 2019

not specifying the digest both on command line and in the config file
will lead to response generation aborting with

140617514493760:error:2F098088:time stamp routines:ts_CONF_lookup_fail:cannot find config variable:crypto/ts/ts_conf.c:106:tsr_test::signer_digest

Checklist
  • documentation is added or updated

@mattcaswell
Copy link
Member

It kind of sounds like this option is somewhere between optional and mandatory! i.e. its optional on the command line if you've specified it in the config file and vice versa. So describing these options as mandatory doesn't quite sound right either.

@tomato42
Copy link
Contributor Author

tomato42 commented Mar 6, 2019 •
edited

well other options that need to be specified, but don't have to be specified in a specific place, are also listed as (Mandatory) (like digests), or am I missing something?

@mattcaswell
Copy link
Member

mattcaswell commented Mar 6, 2019 •
edited

Well, AFAICT, that man page only lists 3 options as mandatory at the moment:

-in: I don't think there is a config file equivalent of this - so this really is mandatory
serial: Seems to be config file only - so also really is mandatory
digests: Also seems to be config file only?

@tomato42
Copy link
Contributor Author

tomato42 commented Mar 7, 2019

hmm, true. Any suggestions?

@mattcaswell
Copy link
Member

I'd suggest marking it as "Optional" in the config file, and then mark the command line option as "Optional unless not specified in the configuration file" (or possibly the other way around "Mandatory unless specified in the configuration file").

@tomato42
Copy link
Contributor Author

tomato42 commented Mar 7, 2019

@mattcaswell updated

mattcaswell
mattcaswell reviewed Mar 7, 2019
View reviewed changes
doc/man1/ts.pod Outdated
@@ -460,7 +460,7 @@ command line option. (Optional)
=item B<signer_digest>

Signing digest to use. The same as the
B<-I<digest>> command line option. (Optional)
B<-I<digest>> command line option. (Mandatory unless specified on command line)
Copy link
Member

Choose a reason for hiding this comment

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

Why not just leave this as Optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because I feel like it better reflects the reality - it needs to be specified somewhere, so specifying it is mandatory

also, specifying it in both places will give less unexpected result than not specifying it at all

i.e. "Mandatory" better manages user expectations

Copy link
Contributor

Choose a reason for hiding this comment

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

grammar: specified on the command line

@mattcaswell mattcaswell added branch: master Merge to master branch branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch labels Mar 7, 2019
@mattcaswell mattcaswell added the approval: review pending This pull request needs review by a committer label Mar 7, 2019
mspncp
mspncp approved these changes Mar 24, 2019
View reviewed changes
Copy link
Contributor

@mspncp mspncp left a comment

Choose a reason for hiding this comment

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

"Mandatory" better manages user expectations

I agree with @tomato42 in this matter.

Approved, provided the two grammar nits are fixed.

doc/man1/ts.pod Outdated
@@ -460,7 +460,7 @@ command line option. (Optional)
=item B<signer_digest>

Signing digest to use. The same as the
B<-I<digest>> command line option. (Optional)
B<-I<digest>> command line option. (Mandatory unless specified on command line)
Copy link
Contributor

Choose a reason for hiding this comment

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

grammar: specified on the command line

doc/man1/ts.pod Outdated
@@ -262,7 +262,7 @@ specified, the argument is given to the engine as a key identifier.
=item B<-I<digest>>

Signing digest to use. Overrides the B<signer_digest> config file
option. (Optional)
option. (Mandatory unless specified in config file)
Copy link
Contributor

Choose a reason for hiding this comment

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

grammar: specified in the config file

@mspncp mspncp removed the approval: review pending This pull request needs review by a committer label Mar 24, 2019
@tomato42
ts(1): digest option is mandatory
b974c07 
not specifying the digest both on command line and in the config file
will lead to response generation aborting with

140617514493760:error:2F098088:time stamp routines:ts_CONF_lookup_fail:cannot find config variable:crypto/ts/ts_conf.c:106:tsr_test::signer_digest
@tomato42
Copy link
Contributor Author

"the" added in both places

mspncp
mspncp approved these changes Mar 25, 2019
View reviewed changes
Copy link
Contributor

@mspncp mspncp left a comment

Choose a reason for hiding this comment

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

reconfirmed

@mspncp mspncp added the approval: done This pull request has the required number of approvals label Mar 25, 2019
@mspncp mspncp self-assigned this Mar 25, 2019
levitte pushed a commit that referenced this pull request Mar 25, 2019
@tomato42 @mspncp
ts(1): digest option is mandatory
3d753b0 
not specifying the digest both on command line and in the config file
will lead to response generation aborting with

140617514493760:error:2F098088:time stamp routines:ts_CONF_lookup_fail: \
    cannot find config variable:crypto/ts/ts_conf.c:106:tsr_test::signer_digest

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com>
(Merged from #8421)

(cherry picked from commit 29716a0)
levitte pushed a commit that referenced this pull request Mar 25, 2019
@tomato42 @mspncp
ts(1): digest option is mandatory
29716a0 
not specifying the digest both on command line and in the config file
will lead to response generation aborting with

140617514493760:error:2F098088:time stamp routines:ts_CONF_lookup_fail: \
    cannot find config variable:crypto/ts/ts_conf.c:106:tsr_test::signer_digest

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com>
(Merged from #8421)
@mspncp
Copy link
Contributor

mspncp commented Mar 25, 2019

Merged to master and 1.1.1

29716a0 ts(1): digest option is mandatory (master)
3d753b0 ts(1): digest option is mandatory (1.1.1)

@mspncp mspncp closed this Mar 25, 2019
@tomato42 tomato42 deleted the ts-man-page branch March 25, 2019 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattcaswell mattcaswell mattcaswell approved these changes

@mspncp mspncp mspncp approved these changes

Assignees

@mspncp mspncp

Labels
approval: done This pull request has the required number of approvals branch: master Merge to master branch branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tomato42 @mattcaswell @mspncp