Conversation
825b6d8 to
580e5f8
Compare
|
|
3a5496b to
d2b1bc4
Compare
|
Those FreeBSD test failures look like they're not just regular flakiness... |
Yes, I agree and have looked at them carefully. However I don't see how they can be related to this PR: the failing tests deal with direct ftps and we have a proxy input and an http2 server logs. Unless I misunderstand something, it's like the test environment sets the target port for client where the wrong server listens. The failing tests are all tests using ftps server and performing a data transfer. In any case, I will change a commit a little bit, so consider this PR on hold. Thanks. |
6f7272d to
7b4833d
Compare
There was a problem hiding this comment.
Are you sure about this? I don't see it documented that way, I checked https://www.openssl.org/docs/man1.0.2/man3/SSL_CTX_clear_options.html
There was a problem hiding this comment.
Also, it seems like an unrelated change. It is not here to "suppress deprecation warnings" surely?
What's the reason for adding this function call? Everything that changes the setup of OpenSSL creates a risk and we need to be careful.
There was a problem hiding this comment.
Are you sure about this?
@jay : On the page you linked:
SSL_CTX_set_options() adds the options set via bitmask in options to ctx. Options already set before are not cleared!
and
SSL_OP_NO_SSLv2
Do not use the SSLv2 protocol. As of OpenSSL 1.0.2g the SSL_OP_NO_SSLv2 option is set by default.
@bagder :
Also, it seems like an unrelated change. It is not here to "suppress deprecation warnings" surely?
It is related because when conditionals enable TLS_client_method(), there's a risk that the target protocol version is disabled by default. The documentation does not specify if setting the min/max version in the context overrides the options or not.
OpenSSL changed the handling of protocol selection many times in the several last versions: they deprecated *_client_method(), set SSL_OP_NO_SSLv2 by default, then defined the later as 0. I think clearing the protocol options that are under our control is the simplest way of dealing with the problem for all versions up to now.
There was a problem hiding this comment.
BTW: there's nothing in our test environment that allows to verify the proper TLS protocol and cipher selection. I checked the code to see if this can be implemented easily: this answer is no :-( Server side uses stunnel and no backend-independent API allows it on the client side.
|
The freebsd test failures have disappeared miraculously. |
7aa5e13 to
2f8a947
Compare
01edbce to
d8120c5
Compare
d8120c5 to
684a29b
Compare
|
Drpped openssl commit as target code has disappeared. |
bagder
changed the title
|
thanks! |
…rn codepath Historically servers used the deprecated `siginterrupt()` function to configure restart behavior on specific signals. It accepts a flag, where 1 means to remove the `SA_RESTART` option, and 0 means to enable it. In year 2021 3fb6e5a introduced the modern alternative to the codebase, replacing `siginterrupt()` with `sigaction()`. After this patch, supporting, modern, systems reacted on the same flag, but, by accident, set the `SA_RESTART` bit when flag is 1, and did not set it when 0. This reversed the previous behavior, and the one still used on the `siginterrupt()` legacy codepath. Fix it by revesring the `SA_RESTART` logic for the `sigaction()` codepath, syncing it with the pre-existing behavior. I find it odd this did not cause any perceivable issue for 5 years, even though it's the active one in most Unix envs. Spotted by GitHub Code Quality, though suggesting to fix `siginterrupt()` calls. But looking into the history, those were correct all along. Refs: https://pubs.opengroup.org/onlinepubs/9699919799/functions/siginterrupt.html https://pubs.opengroup.org/onlinepubs/9699919799/functions/sigaction.html https://www.man7.org/linux/man-pages/man3/siginterrupt.3.html https://www.man7.org/linux/man-pages/man2/sigaction.2.html Follow-up to 3fb6e5a #6529 Closes #22037
Before this patch modern systems used `sigaction()` and `SA_RESTART` to install signal handlers, but the signal handler function itself still made a call to the legacy `signal()` function to re-register itself before returning. Re-registering the handler is not necessary with `sigaction()`. It's also undesired to use the legacy API when the modern one is available. Fix by guarding off this call in builds that support the modern API. Follow-up to 3fb6e5a #6529 Follow-up to 18cbb4d Closes #22497
3 participants
openssl >= 1.1.0 deprecates
*_client_method()functions other thanTLS_client_method(). Take care of it with conditionals.Test server uses deprecated
siginterrupt()aftersignal(). Depending on availability, merge both usingsigaction().