AES get error message#65
Conversation
This addition fixes the following alert in nginx logs: [alert] ignoring stale global SSL error See: openresty/lua-resty-string#65
|
This patch not only improves informativity but also eliminates the following alert in the nginx log: The alert is emitted during the successful subsequent method call. |
|
@un-def could you help to add more test to cover the more error case? |
| end | ||
|
|
||
| local msg = ffi_new("char[?]", 256) | ||
| C.ERR_error_string_n(errno, msg, 256) |
There was a problem hiding this comment.
One OpenSSL call may cause several error messages. Therefore, to handle the error message properly, we need to draw the whole error messages out like:
https://github.com/spacewander/lua-resty-rsa/blob/6a6073f2dcfc319fdf760c65a26382f2a8cf67df/lib/resty/rsa.lua#L158
There was a problem hiding this comment.
One OpenSSL call may cause several error messages
I suspected it but couldn't find confirmation in the OpenSSL documentation. Does the documentation say it clearly or you know it from source code (or another source)?
There was a problem hiding this comment.
attention with https://github.com/spacewander/lua-resty-rsa/blob/6a6073f2dcfc319fdf760c65a26382f2a8cf67df/lib/resty/rsa.lua#L158.
C.ERR_reason_error_string function may return a null value when the code not match a human readable message.
so null value checking for err variable
local err = C.ERR_reason_error_string(code)
if err ~= ffi.null then
err_queue[i] = ffi_str(err)
...
end
There was a problem hiding this comment.
@toruneko
Yes, you are right. We need to check the returned err before using it.
There was a problem hiding this comment.
There was a problem hiding this comment.
BTW, sometimes OpenSSL don't even return an error message when the call is failed. (Perhaps it is a bug): openssl/openssl#11962
So strictly speaking, we need to provide a default one if no error is found:
https://github.com/spacewander/lua-resty-rsa/blob/6a6073f2dcfc319fdf760c65a26382f2a8cf67df/lib/resty/rsa.lua#L180
There was a problem hiding this comment.
@spacewander I've seen it, but this is not the official OpenSSL wiki and has not been updated for a long time. I meant the official docs hosted at https://www.openssl.org/ or official wiki https://wiki.openssl.org/ Anyway, I agree that it's worth popping all entries from the error stack, it's safe and future-compatible.
|
@syzh what error cases did you mean? Should we add test cases for each OpenSSL FFI call that can produce an error (in other words, cover each place where BTW, I am not the author of this PR :) I just came across it when discovered the reason of |
| local msg = ffi_new("char[?]", 256) | ||
| C.ERR_error_string_n(errno, msg, 256) |
|
Hello Team, I hope this PR has not been frozen in development, as the Decrypt Failures can lead to OpenSSL SSL Handshake errors, as the NGINX SSL Handshake Flow checks for errors in the process. (and this decrypt will show up and prevent successful SSL Handshake, even with a valid certificate) I have for now bypassed this issue by calling the ERR_get_error in my own code if the decrypt returns nil, but this PR proposes a much cleaner and more unified solution. 👍 to this PR, please seriously review this as even unrelated decrypt failures cause SSL handshake errors and prevent successful requests. Thanks! |
7 participants
No description provided.