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

Various fixes regarding PKCS#12 input and related cleanup of apps, doc, and tests #4930

Closed
wants to merge 11 commits into from

Conversation

@DDvO
Copy link
Contributor

@DDvO DDvO commented Dec 14, 2017

UPDATE: Originally this PR was about extending support for PKCS#12 input in apps.
I've meanwhile carved out the most interesting pieces of that and contributed them separately.
This is the leftovers fixing several corner cases in PKCS#12 input and its error handling.
There are also some rather unrelated fixes to several apps and their documentation, which I could separate if requested.

Checklist
  • documentation is added or updated
  • tests are added or updated
@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Dec 15, 2017

This PR also contains (in its first commit) a minor fix of a conditionally unused variable,
which caused a build failure when OPENSSL_SYS_UNIX is not defined and strict/pedantic compiler settings (-Werror) are used.

@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Dec 15, 2017

I also noticed that doc/man1/s_server.pod is somewhat out of sync.
Besides my adaptations that are related to the PKCS#12 extensions I made,
I removed the following references to options not available any more:

[B<-xkey>]
[B<-xcert>]
[B<-xchain>]
[B<-xchain_build>]
[B<-xcertform PEM|DER>]
[B<-xkeyform PEM|DER>]
@bernd-edlinger
Copy link
Member

@bernd-edlinger bernd-edlinger commented Dec 26, 2017

I think if you have a PKCS#12 file, then you have the certificate, private key and any chain
certificates all in one place, so you should only name one pkcs#12 file, and one password
and not have three different file paths, but only one password.

@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Dec 27, 2017

I agree that typically a PKCS#12 file contains these related types of key material and that it would be logical and most convenient to refer to such a file only once (including any password input).
Yet so far the OpenSSL CLI, except for the pkcs12 app, does not support this.
And I just noticed that, for some reason, even pkcs12.c does not make use of load_pkacs12() to load them jointly.

I fear that rectifying this would take some non-negligible effort, including a (backward compatible) extension of the CLI options design for all apps that should support joint PKCS#12 input. Shall we go for that, and who would be willing and have time to help doing this?

Nevertheless, the generaliizations I've proprosed here are already useful in themselves - with just the inconvenience that, as before, a PKCS#12 file used not only for key input but also for certificate input needs to be named (together with any password input) more than once.

apps/s_client.c Outdated Show resolved Hide resolved
doc/man1/ca.pod Outdated Show resolved Hide resolved
apps/s_server.c Outdated Show resolved Hide resolved
doc/man1/s_server.pod Outdated Show resolved Hide resolved
apps/apps.c Outdated Show resolved Hide resolved
apps/ca.c Outdated Show resolved Hide resolved
@DDvO DDvO force-pushed the siemens:certs_passwd_pkcs12 branch to 925d966 Jan 9, 2018
tpank pushed a commit to mpeylo/cmpossl that referenced this pull request Jan 15, 2018
…o_pkcs12()

see also openssl#4930

improved OpenSSL 1.0.2 compatibility of cmp.c
@mattcaswell mattcaswell added this to the Post 1.1.1 milestone Jan 24, 2018
@richsalz richsalz added after 1.1.1 and removed after 1.1.1 labels May 6, 2018
@mspncp mspncp removed the after 1.1.1 label Oct 25, 2019
Copy link
Member

@levitte levitte left a comment

I had these lying around... can't even remember when I wrote them.

apps/apps.c Outdated Show resolved Hide resolved
apps/apps.c Outdated Show resolved Hide resolved
@DDvO DDvO force-pushed the siemens:certs_passwd_pkcs12 branch from 925d966 Apr 20, 2020
@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Apr 20, 2020

I had these lying around... can't even remember when I wrote them.

Thanks for these comments. I've just handled them.

@DDvO DDvO force-pushed the siemens:certs_passwd_pkcs12 branch 2 times, most recently Apr 21, 2020
@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Apr 21, 2020

The Travis red cross is due to the (unrelated) issue handled in #11573.

@levitte, all comments have been handled.

@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Apr 22, 2020

The two CI failures currently reported here are unrelated.

Ready for further reviewing.
See in particular the fixes to PKCS12_parse() and its documentation.

@DDvO DDvO force-pushed the siemens:certs_passwd_pkcs12 branch 2 times, most recently Apr 22, 2020
@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Apr 24, 2020

I've rebased this and partly squashed the commits.
@levitte, is there anything else to improve in this PR?

Hopefully this can soon be merged for inclusion in #11470.

@mattcaswell
Copy link
Member

@mattcaswell mattcaswell commented Apr 27, 2020

Ping @levitte

@DDvO DDvO modified the milestones: Post 1.1.1, 3.0.0 May 7, 2020
apps/ca.c Outdated Show resolved Hide resolved
apps/lib/apps.c Outdated Show resolved Hide resolved
apps/lib/apps.c Outdated Show resolved Hide resolved
@DDvO DDvO force-pushed the siemens:certs_passwd_pkcs12 branch May 8, 2020
@DDvO
Copy link
Contributor Author

@DDvO DDvO commented May 8, 2020

Thanks @FdaSilvaYY for having a look.
Rebased this PR and fixed all reported nits.

@DDvO DDvO force-pushed the siemens:certs_passwd_pkcs12 branch May 9, 2020
clin1234 added a commit to clin1234/openssl that referenced this pull request Nov 21, 2020
…s in input files

Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from openssl#4930)
clin1234 added a commit to clin1234/openssl that referenced this pull request Nov 21, 2020
…decode_PKCS12()

Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from openssl#4930)
clin1234 added a commit to clin1234/openssl that referenced this pull request Nov 21, 2020
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from openssl#4930)
clin1234 added a commit to clin1234/openssl that referenced this pull request Nov 21, 2020
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from openssl#4930)
clin1234 added a commit to clin1234/openssl that referenced this pull request Nov 21, 2020
Also do a minor extension on the documentation of the -passcerts option

Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from openssl#4930)
tniessen added a commit to tniessen/openssl that referenced this pull request Mar 11, 2021
Refs: openssl#4930
openssl-machine pushed a commit that referenced this pull request Mar 14, 2021
Refs: #4930

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #14520)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet