Skip to content

Comments

apps: make use of OSSL_STORE for generalized cert and CRLs loading#12647

Merged
openssl-machine merged 2 commits intoopenssl:masterfrom
siemens:generalize_app_certs_crls_input
Aug 20, 2020
Merged

apps: make use of OSSL_STORE for generalized cert and CRLs loading#12647
openssl-machine merged 2 commits intoopenssl:masterfrom
siemens:generalize_app_certs_crls_input

Conversation

@DDvO
Copy link
Contributor

@DDvO DDvO commented Aug 14, 2020 •
edited
Loading

This generalizes the load_certs(), load_cert(), and load_crls() functions used by the CLI apps ca, cmp, cms, ocsp,
pkcs12, s_client, s_server, smime, verify, and x509 taking advantage of the OSSL_STORE mechanism.
This allows loading not only in PEM format but also in DER and PKCS#12 format and loading also from URIs.
It also enables using passwords for certificate input files, using a new -passcerts option of pkcs12
and, as far as it makes sense, any existing password input options for other apps.

This supersedes the core of my old PR #4930.
This is also strongly related to #12643.

Checklist
  • documentation is added or updated
  • tests are added or updated

@DDvO DDvO force-pushed the generalize_app_certs_crls_input branch 3 times, most recently from aaa493a to 9058846 Compare August 16, 2020 12:56
@DDvO DDvO changed the title apps: make use of OSSL_STORE for generalized certs and CRLs loading apps: make use of OSSL_STORE for generalized cert and CRLs loading Aug 16, 2020
levitte
levitte reviewed Aug 17, 2020
View reviewed changes
@levitte levitte added approval: done This pull request has the required number of approvals branch: master Applies to master branch and removed approval: otc review pending labels Aug 19, 2020
@levitte
Copy link
Member

levitte commented Aug 19, 2020

I think this finalizes a lot of what I did in #7390. When this is merged, I'll revisit that one to see what remains.

@DDvO
Copy link
Contributor Author

DDvO commented Aug 19, 2020

I think this finalizes a lot of what I did in #7390. When this is merged, I'll revisit that one to see what remains.

Ah, I was not aware of that PR!

At times I think it would be good to have a mechanism that points out overlaps between existing PRs,
or even better, that could be used to detect if a planned change could overlap with existing PRs.

@levitte
Copy link
Member

levitte commented Aug 19, 2020

Ah, I was not aware of that PR!

No matter, I'm actually glad to see someone else doing this, that indicates the desire for this change.

@openssl-machine
Copy link
Collaborator

openssl-machine commented Aug 20, 2020

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.

@levitte levitte 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 Aug 20, 2020
@levitte
Copy link
Member

levitte commented Aug 20, 2020

This is ready to be merged

DDvO added 2 commits August 20, 2020 14:55
@DDvO
apps: make use of OSSL_STORE for generalized certs and CRLs loading
b3c5aad 
Reviewed-by: Richard Levitte <[email protected]>
(Merged from openssl#12647)
@DDvO
Make better use of new load_cert_pass() variant of load_cert() in apps/
2a33470 
allows loading password-protected PKCS#12 files in x509, ca, s_client, s_server

Reviewed-by: Richard Levitte <[email protected]>
(Merged from openssl#12647)
@DDvO DDvO force-pushed the generalize_app_certs_crls_input branch from d757fc1 to 2a33470 Compare August 20, 2020 13:01
@openssl-machine openssl-machine merged commit 2a33470 into openssl:master Aug 20, 2020
@DDvO
Copy link
Contributor Author

DDvO commented Aug 20, 2020

This is ready to be merged

Done - thanks @levitte

@kaduk
Copy link
Contributor

kaduk commented Aug 20, 2020

At times I think it would be good to have a mechanism that points out overlaps between existing PRs,
or even better, that could be used to detect if a planned change could overlap with existing PRs.

gerrit code review will link to changes that conflict or are "related to" the change in question. (I am not 100% sure of the details of what determines those relationships, and it might be configurable, but I think that the conflicts are actual 'git merge' failures and the "related"ness is parent/child relationships.

@DDvO
Copy link
Contributor Author

DDvO commented Aug 21, 2020

At times I think it would be good to have a mechanism that points out overlaps between existing PRs,
or even better, that could be used to detect if a planned change could overlap with existing PRs.

gerrit code review will link to changes that conflict or are "related to" the change in question.

This sounds interesting!
Are there plans to integrate this with GitHub or OpenSSL?

@kaduk
Copy link
Contributor

kaduk commented Aug 21, 2020

Gerrit is in some sense a competitor to github (it's a java application maintained by google, IIUC). I don't know of any particular efforts by github to include similar features.
That said, there is a service called "gerrithub" (gerrithub.io) that, IIUC, provides the gerrit UI over repositories hosted at github. But my understanding is that you have to opt in a whole project at once, and I assume that entails no longer using the "regular" github UI for the project in question. (I have not looked at it very hard.)

swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
@DDvO
apps: make use of OSSL_STORE for generalized certs and CRLs loading
4154b72 
Reviewed-by: Richard Levitte <[email protected]>
(Merged from openssl#12647)
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
@DDvO
Make better use of new load_cert_pass() variant of load_cert() in apps/
62bdfcd 
allows loading password-protected PKCS#12 files in x509, ca, s_client, s_server

Reviewed-by: Richard Levitte <[email protected]>
(Merged from openssl#12647)
@DDvO DDvO mentioned this pull request Oct 9, 2020
Closed
@rainerjung rainerjung mentioned this pull request Oct 24, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@t8m t8m t8m left review comments

@levitte levitte levitte approved these changes

Assignees

No one assigned

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@DDvO @levitte @openssl-machine @kaduk @t8m