Skip to content

ext/curl: Add CURLOPT_SSL_SIGNATURE_ALGORITHMS option #18692

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ayesh
Copy link
Member

@Ayesh Ayesh commented May 28, 2025

Adds support for CURLOPT_SSL_SIGNATURE_ALGORITHMS1, supported since Curl version 8.14.0.

Footnotes

  1. https://curl.se/libcurl/c/CURLOPT_SSL_SIGNATURE_ALGORITHMS.html

$curl_version = curl_version();
if ($curl_version['version_number'] < 0x080e00) die("skip: test works only with curl >= 8.14.0");

include 'skipif-nocaddy.inc';
Copy link
Member

Choose a reason for hiding this comment

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

hmm, this might change things in terms of setting up caddy for windows... I guess it's better than openssl s_server but even better solution would be to use openssl client server test like we use for some http stream tests: for example https://github.com/php/php-src/blob/master/ext/standard/tests/http/ghsa-hgf5-96fm-v528-001.phpt . Then it would work on windows too.

Copy link
Member Author

@Ayesh Ayesh May 28, 2025

Choose a reason for hiding this comment

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

I remember you also commented on #16093 that the whole Caddy server setup on Windows isn't worth it.

Right now, even on Linux builds, we barely test any of the recent Curl stuff because on GitHub Actions, the Curl version is Curl 8.5 or 8.6 while Windows has 8.12.0 or so at the moment. So having these tests run on Windows will be really nice. I'm not really that familiar with OpenSSL s_server/client features, so I could use any help, or take some time to test things around. I have a Windows machine to test things, I'll try what I can :)

That said, I wonder if we really need these tests. We can test if setting a sig-alg works, setting null clears it, check if all the constants exist, etc. But beyond that, we are merely testing Curl's functionality itself. Curl surely has tests for all the variations we can think about, including various TLS backends, so perhaps we can simplify the test to see if the constants exist and if curl_setopt works. We skip the curl_exec part because once we set the options, it's Curl's job to do the actual signature negotiations. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the late replay. I thought about this and I guess we don't really need to test the functionality in detail. But it makes sense to have tests that test whether the option is set correctly and it does something. It's also good to test error handling so we make sure that the error is set correctly. So I think this test is actually optimal.

The only question here is whether it's better to use Caddy more (and also try to make it available on Windows) or use the streams. Thinking about it again, I guess it might not be worth invest too much time to custom server setup that we would need to do for streams. The Caddy setup is pretty small and it's not a big issue if it's not yet available on Windows. Although as you said it would be good to add it because we usually run later versions there so we can test latest functionality. I will need to update my comment as I think it would be now worth it to invest time to do the Windows Caddy setup. Although it's not a a blocker for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you.

This test file only tries to see if the CURLOPT worked, and setting it to an invalid value actually fails the request with the expected error message. If we were to use openssl s_client, I think we can make a more meaningful test, but for a thin interface like our current CURLOPT support, I also think that a basic check is good enough. We recently added more tests for CURLOPTs that accept callable values, for which we can always be more thorough.

Do you think the rest of the PR is good to go ahead? I'd also love to help in ways I can, to add Caddy to Windows CI when we come across it.

@Ayesh Ayesh force-pushed the curl/CURLOPT_SSL_SIGNATURE_ALGORITHMS branch 4 times, most recently from 70596d6 to ce0ddf5 Compare June 4, 2025 06:24
Copy link
Member

@bukka bukka left a comment

Choose a reason for hiding this comment

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

I think it's good to go except the conflict. Please fix it (regenerate arginfo...).

$curl_version = curl_version();
if ($curl_version['version_number'] < 0x080e00) die("skip: test works only with curl >= 8.14.0");

include 'skipif-nocaddy.inc';
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the late replay. I thought about this and I guess we don't really need to test the functionality in detail. But it makes sense to have tests that test whether the option is set correctly and it does something. It's also good to test error handling so we make sure that the error is set correctly. So I think this test is actually optimal.

The only question here is whether it's better to use Caddy more (and also try to make it available on Windows) or use the streams. Thinking about it again, I guess it might not be worth invest too much time to custom server setup that we would need to do for streams. The Caddy setup is pretty small and it's not a big issue if it's not yet available on Windows. Although as you said it would be good to add it because we usually run later versions there so we can test latest functionality. I will need to update my comment as I think it would be now worth it to invest time to do the Windows Caddy setup. Although it's not a a blocker for this.

Adds support for `CURLOPT_SSL_SIGNATURE_ALGORITHMS`[^1], supported
since Curl version 8.14.0.

[^1]: https://curl.se/libcurl/c/CURLOPT_SSL_SIGNATURE_ALGORITHMS.html
@Ayesh Ayesh closed this Jul 16, 2025
@Ayesh Ayesh force-pushed the curl/CURLOPT_SSL_SIGNATURE_ALGORITHMS branch from ce0ddf5 to b9844b5 Compare July 16, 2025 16:52
@Ayesh Ayesh reopened this Jul 16, 2025
@Ayesh
Copy link
Member Author

Ayesh commented Jul 16, 2025

Rebased from latest master, arginfo rebuilt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants