Add ENGINE_init() calls before using OpenSSL engines. #1
Conversation
Jakuje
left a comment
Jakuje
left a comment
There was a problem hiding this comment.
I put several comments to the code. The changes look good to me according to OpenSSL documentation.
There might be some slight differences between OpenSSL 1.1.0 and 1.0.2. Did you test with both of the versions? Are both of them supported by nginx?
src/event/ngx_event_openssl.c
Outdated
| } | ||
|
|
||
| ENGINE_free(engine); | ||
| if (!ENGINE_set_default(engine, ENGINE_METHOD_ALL)) { |
There was a problem hiding this comment.
Do we need ENGINE_METHOD_ALL here? Would not be ENGINE_METHOD_PKEY_METHS enough? Or the it does not matter in this use case?
There was a problem hiding this comment.
Yes, probably setting ENGINE_METHOD_PKEY_METHS would be sufficient in this case. Unless libp11 include support to do other operations, like digests, in the hardware, what is unlikely. I'll test again and change if sufficient.
src/event/ngx_event_openssl.c
Outdated
| if (SSL_CTX_use_PrivateKey(ssl->ctx, pkey) == 0) { | ||
| ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0, | ||
| "SSL_CTX_use_PrivateKey(\"%s\") failed", last); | ||
| "SSL_CTX_use_PrivateKey(ssl->ctx, pkey) failed"); |
There was a problem hiding this comment.
I am not sure if this is unrelated fix of copy&paste error or attempt to show messages closer to the reality. But in case there are more engines/keys being loaded, you will not be able to distinguish between the errors so I would be for leaving some identification here, even if it would not have the same arguments as the function that failed.
There was a problem hiding this comment.
I did this intentionally. It is true that it does give less information, but is the explicit call made in the code. I kept the same style from the other error messages.
Putting this aside, I agree with you, the error messages should give more information, if possible.
src/event/ngx_event_openssl.c
Outdated
| if (ENGINE_set_default(engine, ENGINE_METHOD_ALL) == 0) { | ||
| ngx_ssl_error(NGX_LOG_EMERG, cf->log, 0, | ||
| "ENGINE_set_default(\"%V\", ENGINE_METHOD_ALL) failed", | ||
| &value[1]); |
There was a problem hiding this comment.
Here, the error is reported in different way than you added in the above case. Here we print &value[1] as &V, which says us which engine failed to initialize/set_default, which might be very helpful for debugging. I would consider using the same also in the other errors.
There was a problem hiding this comment.
Yes, I agree. As I commented before, the error messages should give as more information as possible. In this case the author of this part "made a mistake" not following the style of the error messages. I can follow this and "do the mistake" too and make the error messages more informative (but out of the coding style).
There was a problem hiding this comment.
Ok, I went through the code and it seems that the error messaging do not follow a strict rule. I think I can improve the messages in the parts I modified.
|
I forgot to mention, but I tested with OpenSSL 1.1.0 in fedora and 1.0.2 in ubuntu. I'll try the other combinations (1.0.2 in fedora and 1.1.0 in ubuntu). |
ansasaki
force-pushed
the
engine_init
branch
from
b648144 to
1ab4f82
Compare
It is necessary to call ENGINE_init() before using a OpenSSL engine to get the engine functional reference.
The previous error messages didn't specify the parameters used when the functions failed. This uses more descriptive error messages.
This replaces the registration of the engine to be the default engine using ENGINE_METHOD_PKEY_METHS instead of ENGINE_METHOD_ALL.
It is necessary to call ENGINE_init() before using a
OpenSSL engine to get the engine functional reference.