Move the loading of the ssl_conf module to libcrypto#5818
Move the loading of the ssl_conf module to libcrypto#5818mattcaswell wants to merge 5 commits intoopenssl:masterfrom
Conversation
The GOST engine needs to be loaded before we initialise libssl. Otherwise the GOST ciphersuites are not enabled. However the SSL conf module must be loaded before we initialise libcrypto. Otherwise we will fail to read the SSL config from a config file properly. Another problem is that an application may make use of both libcrypto and libssl. If it performs libcrypto stuff first and OPENSSL_init_crypto() is called and loads a config file it will fail if that config file has any libssl stuff in it. This commit separates out the loading of the SSL conf module from the interpretation of its contents. The loading piece doesn't know anything about SSL so this can be moved to libcrypto. The interpretation of what it means remains in libssl. This means we can load the SSL conf data before libssl is there and interpret it when it later becomes available. Fixes openssl#5809
|
|
||
| if (!RUN_ONCE(&ssl_base, ossl_init_ssl_base)) | ||
| if (!OPENSSL_init_crypto(opts | ||
| | OPENSSL_INIT_LOAD_CONFIG |
There was a problem hiding this comment.
Loading the default config by default is at least not consistent with the documentation as far as I understand. So maybe the documentation should get updated?
There was a problem hiding this comment.
Done. I updated the pod documentation and also added a CHANGES entry.
util/libcrypto.num
Outdated
| SM2_plaintext_size 4468 1_1_1 EXIST::FUNCTION:SM2 | ||
| CONF_ssl_name_find 4469 1_1_1 EXIST::FUNCTION: | ||
| CONF_ssl_get_cmd 4470 1_1_1 EXIST::FUNCTION: | ||
| CONF_ssl_get 4471 1_1_1 EXIST::FUNCTION: |
There was a problem hiding this comment.
Exporting functions from libcrypto so libssl can use them just feels so wrong.
There was a problem hiding this comment.
It solves a probllem that's otherwise a catch 22, so I think it should be allowed.
However, since these are internal by design from the start, they should probably be all lowercase.
There was a problem hiding this comment.
There is quite a lot of precedent for this. I changed the names to all lowercase.
|
Please add some minimal documentation to the functions. |
|
The CONF_ssl functions should not be documented, and they should only be declared in an internal header file; they are not for general use, right? |
crypto/conf/conf_mall.c
Outdated
| #include <openssl/x509.h> | ||
| #include <openssl/asn1.h> | ||
| #include <openssl/engine.h> | ||
| #include "conflcl.h" |
There was a problem hiding this comment.
Hmmm, we usually have a _ before lcl...
util/libcrypto.num
Outdated
| SM2_plaintext_size 4468 1_1_1 EXIST::FUNCTION:SM2 | ||
| CONF_ssl_name_find 4469 1_1_1 EXIST::FUNCTION: | ||
| CONF_ssl_get_cmd 4470 1_1_1 EXIST::FUNCTION: | ||
| CONF_ssl_get 4471 1_1_1 EXIST::FUNCTION: |
There was a problem hiding this comment.
It solves a probllem that's otherwise a catch 22, so I think it should be allowed.
However, since these are internal by design from the start, they should probably be all lowercase.
I think like @kroeckx (even though I often forget it myself), that they should at least get documented intenally, for example as comments before the functions themselves. (@kroeckx keeps reminding me to do this a little now and then, and rightfully so) |
|
I'm all for this solution, pending fixups. |
When libssl is initialised it will attempt to load any config file. This ensures any system_default configuration (as per openssl#4848) is used.
|
I added some internal documentation (as comments before the functions). New commits pushed addressing all outstanding comments. |
|
Re 1.1.0, if I'm getting it right, #5813 has some corner case issue that this more radical way fixes, right? Are there any user visible drawbacks with this PR? If not, I see about zero problems going with the more radical change in 1.1.0 as well. It is, after all, not a new feature, but a bug fix, although a bit more involved one than what you'd usually expect. The way I understand it, this one is more robust than #5813, is that correct? That makes the choice even easier. However, make sure to correct the version numbers on the added functions in |
Right
No, there shouldn't be. Although it will need a slight tweak for 1.1.0 because this PR makes loading the config file a default for libssl. Although this is a behaviour change I think it was accepted for 1.1.1 and most people seemed to have been expecting it as a result of #4848 (but that PR didn't actually make that change). For 1.1.0 though we probably shouldn't be doing that.
Yes
Done. New fixup commit pushed. I'll go create a 1.1.0 version in a different PR. |
|
(let's see that the CIs agree as well before merging, yeah?) |
|
Pushed. Thanks. |
The GOST engine needs to be loaded before we initialise libssl. Otherwise the GOST ciphersuites are not enabled. However the SSL conf module must be loaded before we initialise libcrypto. Otherwise we will fail to read the SSL config from a config file properly. Another problem is that an application may make use of both libcrypto and libssl. If it performs libcrypto stuff first and OPENSSL_init_crypto() is called and loads a config file it will fail if that config file has any libssl stuff in it. This commit separates out the loading of the SSL conf module from the interpretation of its contents. The loading piece doesn't know anything about SSL so this can be moved to libcrypto. The interpretation of what it means remains in libssl. This means we can load the SSL conf data before libssl is there and interpret it when it later becomes available. Fixes #5809 Reviewed-by: Richard Levitte <[email protected]> (Merged from #5818)
When libssl is initialised it will attempt to load any config file. This ensures any system_default configuration (as per #4848) is used. Reviewed-by: Richard Levitte <[email protected]> (Merged from #5818)
4 participants
This is an alternative more radical approach to #5813. It fixes the same problems but additionally fixes the root issue about what happens if libcrypto gets initialised before libssl.
The GOST engine needs to be loaded before we initialise libssl. Otherwise
the GOST ciphersuites are not enabled. However the SSL conf module must
be loaded before we initialise libcrypto. Otherwise we will fail to read
the SSL config from a config file properly.
Another problem is that an application may make use of both libcrypto and
libssl. If it performs libcrypto stuff first and OPENSSL_init_crypto()
is called and loads a config file it will fail if that config file has
any libssl stuff in it.
This commit separates out the loading of the SSL conf module from the
interpretation of its contents. The loading piece doesn't know anything
about SSL so this can be moved to libcrypto. The interpretation of what it
means remains in libssl. This means we can load the SSL conf data before
libssl is there and interpret it when it later becomes available.
Fixes #5809