APPS: simplify `x509` cert generation; allow pubkey input options to read private key by DDvO · Pull Request #19076 · openssl/openssl

DDvO · 2022-08-27T09:25:12Z

When reviewing the addition to test/smime-certs/mksmime-certs.sh by #18567,
I noticed again that producing certs using openssl x509 is still needlessly cumbersome.
While there are already abbreviations for the case of self-issued (including self-signed) certs,
for other certs (produced with the -CA option) one is so far forced to generate a cert request first.
It turns out this was simply because is was not possible to combine the -key option with -CA.

Update: Due to the comments by @vdukhovni, changed the solution to allow reading a private key when a public key is expected. This enables conveniently using the -force_pubkey option with -CA.

This PR adds the missing feature and makes use of the simplification in mksmime-certs.sh.
It also adds a test case for the new feature in 25-test_x509.t.

On this occasion, also clean up the key input options of other apps such as pkey.

On this occasion it also cleans up mksmime-certs.sh, speeds up its cert generation by not freshly generating RSA and DSA keys, and fixes its generation of inconsistent key usages for non-RSA certs.

documentation is added or updated
tests are added or updated

DDvO · 2022-09-20T19:25:29Z

Ping @openssl/committers for 2nd review.

tmshort

Other than a wishy-washy word in the documentation, looks ok to me.

doc/man1/openssl-x509.pod.in

t8m · 2022-09-22T06:47:52Z

@tmshort please reconfirm

tmshort · 2022-09-22T13:37:47Z

reconfirm

vdukhovni · 2022-09-22T14:14:07Z

With -CA, it seems that the only part of the -key file that is used is the public component. If so, it seems that this should support (or require) a public key, rather than a private key. After all, if a CA is signing a certificate, why should the subject's private key be required???

DDvO · 2022-09-22T14:33:44Z

With -CA, it seems that the only part of the -key file that is used is the public component.

Right.

If so, it seems that this should support (or require) a public key, rather than a private key.
After all, if a CA is signing a certificate, why should the subject's private key be required???

It's still good to support a private key input because for many scenarios it is available anyway (and the public portion can be derived automatically from it) and it is typically easier to handle (at least at the openssl CLI level) than a file containing just a public key.

Yet good thought that we should offer also the possibility to directly provide a file with a public key.
In this case the existing -force_pubkey option could simply be reused.
If you like, I can provide a follow-up PR for this.

beldmit · 2022-09-22T14:37:08Z

So don't we get a mixed semantics for the -key option in case when your patch is applied?

vdukhovni · 2022-09-22T14:41:35Z

Ideally, the -key option would load either a private key or public key whichever is provided, and then extract the public part if a private key is provided. In other words, make the "user experience" as seamless as possible.

In any case, think about what is the least surprising (most natural) interface, and do that. Sure, this could be a followup PR, if you don't want to reopen reviews of the work so far. I'd be tempted to just amend this one, it is not a large PR. Your (and the other reviewers') call.

DDvO · 2022-09-22T15:07:28Z

So don't we get a mixed semantics for the -key option in case when your patch is applied?

With the current state of the PR, the -key argument must be a private key file.

Ideally, the -key option would load either a private key or public key whichever is provided, and then extract the public part if a private key is provided. In other words, make the "user experience" as seamless as possible.

Interesting idea.
Is there already any other openssl command option that supports reading a file that contains a public or private key?

In any case, think about what is the least surprising (most natural) interface, and do that. Sure, this could be a followup PR, if you don't want to reopen reviews of the work so far. I'd be tempted to just amend this one, it is not a large PR. Your (and the other reviewers') call.

I was concerned exactly with this point, since the approval is already done.
@t8m and @tmshort, what do you think?

vdukhovni · 2022-09-22T15:19:05Z

Interesting idea.
Is there already any other openssl command option that supports reading a file that contains a public or private key?

I can't think of one offhand, but in principle any command that expects a public key should be willing to read a private key instead, and extract the public key from that. That would be a general UI improvement.

vdukhovni · 2022-09-22T15:24:46Z

In this case, the option's expected data would depend on other options (-CA changes the semantics of -key). Is that natural or surprising? Should some other option actually be used, leaving -key as incompatible with -CA?

These questions perhaps mean that the PR should not immediately be merged. We should at least decide what the UI should look like long-term, and then perhaps merge this PR, with other PRs to follow, or amend it as seems best.

One way or another, when all we want is a public key, it should be possible to specify a file with just the public key. Whether that's a separate public-key-only option, or a combo option that sometimes wants a private key, and other times a public key.

Please think through what a user might expect in terms of behaviour, and what can be most clearly explained in the documentation, ...

openssl-machine · 2022-09-23T14:00:15Z

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

openssl-machine · 2023-01-26T00:10:02Z

This PR is in a state where it requires action by @openssl/committers but the last update was 123 days ago

DDvO · 2023-02-24T12:32:19Z

Rebased to fix merge conflicts.

@t8m can you reapprove?

DDvO · 2023-02-24T13:14:03Z

There is an unrelated CI failure for doc-nits:

/usr/bin/perl ./util/find-doc-nits -c -n -l -e
doc/man3/CMS_add0_cert.pod:1: Section missing in CMS_sign_ex()
doc/man3/CMS_add0_cert.pod:1: Section missing in CMS_sign()
doc/man3/CMS_add0_cert.pod:1: Section missing in CMS_verify()
doc/man3/CMS_add0_cert.pod:1: Section missing in CMS_verify()

is unrelated; fix for that is in #20369.

…t generation Also remove inconsistent key usages from non-RSA certs.

DDvO · 2023-02-27T07:50:22Z

Rebased again, just to get rid of the above unrelated CI failure message.

beldmit · 2023-03-06T19:19:02Z

LGTM

DDvO · 2023-03-09T14:17:31Z

LGTM

Thank you @beldmit for this statement.
Can you please make it formal and accordingly set approval:done ?

beldmit · 2023-03-09T14:20:12Z

Sorry, missed it

t8m · 2023-03-09T14:29:31Z

Sorry, missed it

It would be nice to also do the formal approval on review changes tab.

beldmit

LGTM

beldmit · 2023-03-09T14:33:24Z

@t8m done

openssl-machine · 2023-03-10T15:00:24Z

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Dmitry Belyavskiy <[email protected]> Reviewed-by: David von Oheimb <[email protected]> (Merged from #19076)

…t generation Also remove inconsistent key usages from non-RSA certs. Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Dmitry Belyavskiy <[email protected]> Reviewed-by: David von Oheimb <[email protected]> (Merged from #19076)

Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Dmitry Belyavskiy <[email protected]> Reviewed-by: David von Oheimb <[email protected]> (Merged from #19076)

DDvO · 2023-03-14T16:29:32Z

Merged - thanks @t8m, @tmshort, @vdukhovni, and @beldmit for your reviews and approvals.

DDvO added this to the 3.1.0 milestone Aug 27, 2022

t8m approved these changes Aug 29, 2022

View reviewed changes

t8m removed the approval: otc review pending label Aug 29, 2022

tmshort approved these changes Sep 21, 2022

View reviewed changes

doc/man1/openssl-x509.pod.in Outdated Show resolved Hide resolved

t8m approved these changes Sep 22, 2022

View reviewed changes

tmshort added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Sep 22, 2022

DDvO changed the title ~~APPS: simplify x509 cert generation~~ APPS: simplify x509 cert generation; allow pubkey input options to read private key Sep 24, 2022

t8m removed this from the 3.1.0 milestone Jan 26, 2023

DDvO added the tests: present The PR has suitable tests present label Feb 24, 2023

DDvO force-pushed the simplify_x509_cert_gen branch from dc5f448 to a1d1fc7 Compare February 24, 2023 12:31

DDvO added 3 commits February 24, 2023 16:42

apps/{dsa,ec,rsa,pkey}.c: harmonize coding w.r.t. 'pubin' variable

0998e83

test/smime-certs/{mksmime-certs.sh,ca.cnf}: simplify and speed up cer…

be501c3

…t generation Also remove inconsistent key usages from non-RSA certs.

apps/x509 etc.: allow private key input when public key is expected

b33d756

DDvO force-pushed the simplify_x509_cert_gen branch from a1d1fc7 to b33d756 Compare February 24, 2023 15:42

DDvO requested a review from vdukhovni February 24, 2023 15:43

t8m approved these changes Mar 6, 2023

View reviewed changes

t8m requested a review from beldmit March 6, 2023 19:08

beldmit added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Mar 9, 2023

beldmit approved these changes Mar 9, 2023

View reviewed changes

DDvO added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Mar 13, 2023

DDvO closed this Mar 14, 2023

Uh oh!

Comments

Conversation

DDvO commented Aug 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DDvO commented Sep 20, 2022

Uh oh!

tmshort left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

t8m commented Sep 22, 2022

Uh oh!

tmshort commented Sep 22, 2022

Uh oh!

vdukhovni commented Sep 22, 2022

Uh oh!

DDvO commented Sep 22, 2022

Uh oh!

beldmit commented Sep 22, 2022

Uh oh!

vdukhovni commented Sep 22, 2022

Uh oh!

DDvO commented Sep 22, 2022

Uh oh!

vdukhovni commented Sep 22, 2022

Uh oh!

vdukhovni commented Sep 22, 2022

Uh oh!

openssl-machine commented Sep 23, 2022

Uh oh!

openssl-machine commented Jan 26, 2023

Uh oh!

DDvO commented Feb 24, 2023

Uh oh!

DDvO commented Feb 24, 2023

Uh oh!

DDvO commented Feb 27, 2023

Uh oh!

beldmit commented Mar 6, 2023

Uh oh!

DDvO commented Mar 9, 2023

Uh oh!

beldmit commented Mar 9, 2023

Uh oh!

t8m commented Mar 9, 2023

Uh oh!

beldmit left a comment

Choose a reason for hiding this comment

Uh oh!

beldmit commented Mar 9, 2023

Uh oh!

openssl-machine commented Mar 10, 2023

Uh oh!

DDvO commented Mar 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DDvO commented Aug 27, 2022 •

edited

Loading