Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test server: take care of siginterrupt() deprecation #6529

Closed
wants to merge 1 commit into from

Conversation

monnerat
Copy link
Contributor

openssl >= 1.1.0 deprecates *_client_method() functions other than TLS_client_method(). Take care of it with conditionals.

Test server uses deprecated siginterrupt() after signal(). Depending on availability, merge both using sigaction().

@jay
Copy link
Member

jay commented Jan 26, 2021

I'd split this into 2 commits Ah, I see you did that already...

@bagder
Copy link
Member

bagder commented Jan 27, 2021

Those FreeBSD test failures look like they're not just regular flakiness...

@monnerat
Copy link
Contributor Author

monnerat commented Jan 27, 2021

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.

@monnerat monnerat force-pushed the deprecations branch 2 times, most recently from 6f7272d to 7b4833d Compare January 28, 2021 02:45
@@ -2672,6 +2691,11 @@ static CURLcode ossl_connect_step1(struct Curl_easy *data,
ctx_options &= ~SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS;
#endif

/* As OpenSSL may disable protocol versions by default, clear these
options first.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@monnerat monnerat Jan 28, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@monnerat
Copy link
Contributor Author

monnerat commented Jan 28, 2021

The freebsd test failures have disappeared miraculously.
Current Travis CI job has not yet started. Stuck?
Else I do not plan any other modification to this PR, therefore I release the "hold".

@monnerat
Copy link
Contributor Author

Drpped openssl commit as target code has disappeared.

@bagder bagder changed the title Resolve deprecations test server: take care of siginterrupt() deprecation Apr 22, 2021
@bagder
Copy link
Member

bagder commented Apr 22, 2021

thanks!

@bagder bagder closed this in 3fb6e5a Apr 22, 2021
@monnerat monnerat deleted the deprecations branch April 22, 2021 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants