Skip to content

Comments

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

Closed
DDvO wants to merge 11 commits intoopenssl:masterfrom
siemens:certs_passwd_pkcs12
Closed

Various fixes regarding PKCS#12 input and related cleanup of apps, doc, and tests#4930
DDvO wants to merge 11 commits intoopenssl:masterfrom
siemens:certs_passwd_pkcs12

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 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 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

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 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.

@DDvO DDvO force-pushed the certs_passwd_pkcs12 branch from 586885b to 925d966 Compare January 9, 2018 09:04
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
Copy link
Member

@levitte levitte left a comment

Choose a reason for hiding this comment

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

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

@DDvO DDvO force-pushed the certs_passwd_pkcs12 branch from 925d966 to 0711e0b Compare April 20, 2020 13:58
@DDvO
Copy link
Contributor Author

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 certs_passwd_pkcs12 branch 2 times, most recently from bb43921 to b0f9f20 Compare April 21, 2020 11:45
@DDvO
Copy link
Contributor Author

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 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 certs_passwd_pkcs12 branch 2 times, most recently from 183a86c to 707b0df Compare April 24, 2020 18:43
@DDvO
Copy link
Contributor Author

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

Ping @levitte

@DDvO DDvO modified the milestones: Post 1.1.1, 3.0.0 May 7, 2020
@DDvO DDvO force-pushed the certs_passwd_pkcs12 branch from 707b0df to 1d56e09 Compare May 8, 2020 19:54
@DDvO
Copy link
Contributor Author

DDvO commented May 8, 2020

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

@DDvO DDvO force-pushed the certs_passwd_pkcs12 branch from 1d56e09 to e8a14ef Compare May 9, 2020 15:24
openssl-machine pushed a commit that referenced this pull request Nov 19, 2020
openssl-machine pushed a commit that referenced this pull request Nov 19, 2020
openssl-machine pushed a commit that referenced this pull request Nov 19, 2020
Also do a minor extension on the documentation of the -passcerts option

Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #4930)
@DDvO
Copy link
Contributor Author

DDvO commented Nov 19, 2020

Merged 😅

I'm glad that this nearly three years old PR, after various adaptations and many long waiting times in between, finally has been brought to a good end, even in time for inclusion in 3.0.

Thanks again to @levitte and to all other reviewers for their comments and to @t8m for providing the final push.

@DDvO DDvO closed this Nov 19, 2020
tniessen added a commit to tniessen/openssl that referenced this pull request Mar 11, 2021
openssl-machine pushed a commit that referenced this pull request Mar 14, 2021
Refs: #4930

Reviewed-by: Tomas Mraz <[email protected]>
Reviewed-by: Paul Dale <[email protected]>
(Merged from #14520)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.