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

[curl] Add http3 + remove ssl + openssl default ssl + remove other ssl/tls backend features. #37450

Closed

Conversation

talregev
Copy link
Contributor

@talregev talregev commented Mar 14, 2024

  • Changes comply with the maintainer guide.
  • SHA512s are updated for each updated download.
  • The "supports" clause reflects platforms that may be fixed by this new version.
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

@talregev talregev force-pushed the TalR/curl_http3_remove_ssl branch 2 times, most recently from 73ac5d1 to 433c82f Compare March 14, 2024 18:04

+option(CURL_USE_CA_NATIVE "Use standard certificate store of operating system" OFF)
+if(CURL_USE_CA_NATIVE)
+ set(USE_CA_NATIVE ON)
Copy link
Contributor

Choose a reason for hiding this comment

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

No. Use CURLSSLOPT_NATIVE_CA runtime option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. I will remove this feature.

Copy link
Contributor Author

@talregev talregev Mar 17, 2024

Choose a reason for hiding this comment

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

@BillyONeal I am thinking to bring the native certificate os by default. That even on windows, openssl can take native certificate by default.
What do you think?
curl/curl#13111

@talregev talregev changed the title [curl] Add http3 + remove ssl + add ca-native [curl] Add http3 + remove ssl Mar 14, 2024
@@ -142,35 +159,6 @@
"libssh2"
]
},
"ssl": {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see a reason why ssl should be removed and openssl made the default. Could you explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can read all comments in my other PR start with @BillyONeal
#37146 (comment)

@talregev talregev changed the title [curl] Add http3 + remove ssl [curl] Add http3 + remove ssl + openssl default ssl Mar 14, 2024
@talregev talregev marked this pull request as ready for review March 14, 2024 21:22
@talregev
Copy link
Contributor Author

talregev commented Mar 14, 2024

@BillyONeal Do you want me to remove schannel and other ssl backend features as well?

@dg0yt
Copy link
Contributor

dg0yt commented Mar 14, 2024

i.e. changing windows and osx backend to openssl.
Backend comparision: https://curl.se/docs/ssl-compared.html

  • no automatic CRL
  • no native cert check
  • extra deployment burden

alll for experimental http3 support.

@talregev
Copy link
Contributor Author

talregev commented Mar 14, 2024

i.e. changing windows and osx backend to openssl. Backend comparision: https://curl.se/docs/ssl-compared.html

  • no automatic CRL
  • no native cert check
  • extra deployment burden

alll for experimental http3 support.

You can enable native cert check, also for openssl. I wanted to enable it by default. I can make a patch for that.
This document is a little outdated.

Native cert check for openssl should be marked as manual.
https://github.com/curl/curl/blob/master/docs/cmdline-opts/ca-native.md
curl/curl#12155

@dg0yt Your link updated

@talregev
Copy link
Contributor Author

i.e. changing windows and osx backend to openssl. Backend comparision: https://curl.se/docs/ssl-compared.html

  • no automatic CRL
  • no native cert check
  • extra deployment burden

alll for experimental http3 support.

Is this PR, or http3 with restriction, or without http3.

@dg0yt
Copy link
Contributor

dg0yt commented Mar 14, 2024

Do you want me to remove schannel and other ssl backend features as well?

As long as other backends exist, you have multi-ssl. And cannot have http3.

@talregev
Copy link
Contributor Author

talregev commented Mar 14, 2024

Do you want me to remove schannel and other ssl backend features as well?

As long as other backends exist, you have multi-ssl. And cannot have http3.

As you wish.
mbedtls, schannel, sectransp, winssl, wolfssl features was removed.

@talregev talregev changed the title [curl] Add http3 + remove ssl + openssl default ssl [curl] Add http3 + remove ssl + openssl default ssl + remove other ssl/tls backend features. Mar 14, 2024
@Osyotr Osyotr mentioned this pull request Mar 14, 2024
@WangWeiLin-MV WangWeiLin-MV added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Mar 15, 2024
@BillyONeal
Copy link
Member

BillyONeal commented Mar 18, 2024

Correct, the expectation was that on osx | ios, the TLS backend selected is sectransp, on (windows & !uwp) | mingw the backend selected is schannel, and openssl selected otherwise.

Does http3 support only work with openssl?

@talregev
Copy link
Contributor Author

This document write down all the options for curl with http3.
https://curl.se/docs/http3.html

@BillyONeal
Copy link
Member

This document write down all the options for curl with http3. https://curl.se/docs/http3.html

Hmmm... that's unfortunate. Adding some maintainers of https://github.com/microsoft/msquic

@nibanks @rzikm @csujedihy Do we have guidance for how normal Windows folks should get to http3 that complies with machine policy?

@BillyONeal
Copy link
Member

:sigh: I'm sorry for making the suggestion to do this, I didn't realize that removing the TLS features would make the platform default TLS backends non functional. I'm no longer sure this is the correct tradeoff. I asked some Windows folks about their thoughts...

@talregev
Copy link
Contributor Author

@BillyONeal Don't be sorry, These PRs for that we check, test and learn. I am happy that we check all the options for http3.
My original offer to this PR is that openssl will check native certificate by default, include windows.
Then you can compile curl tool and test the new http3 connection.
I will make another commit later, with the command to test the curl http3 connection on windows (or any other os).

@talregev
Copy link
Contributor Author

@BillyONeal I added the feature ca-native. also add it by default.
On windows you can try:

vcpkg install curl[tool,http3]
cd installed\x64-windows\tools\curl
curl --http3 https://curl.se/ -I

Try and let me know what do you think.
We can also go back to my previous PR that is on draft: #37146
Or if there is other option.
Also we can wait for maintainers of msquic for their respond.

@talregev talregev marked this pull request as draft March 21, 2024 21:14
@BillyONeal
Copy link
Member

Closing this as per #37146 (comment) ; sorry our misunderstanding sent you on this path :/

@BillyONeal BillyONeal closed this Mar 29, 2024
@talregev talregev deleted the TalR/curl_http3_remove_ssl branch April 21, 2024 02:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants