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

Support sslversionandmax argument #22

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

scorriere
Copy link
Contributor

I was looking to specifying a TLS version (basically a minimum TLS version in newer versions of curl) with a max TLS version (-tls-max option in curl). tclcurl supports using 1 or the other inside of the -sslversion option. It is not possible to set both at once with this option. This option sets CURLOPT_SSLVERSION. Unfortunately, libcurl does not support setting the max TLS version in its own option, so it isn't easy to just add another tclcurl option to set this.

https://curl.se/libcurl/c/CURLOPT_SSLVERSION.html

Curl allows us to set both at the same time with a bitwise or operation. I have tested some local changes with adding an option -sslversionandmax to take a 2 element list.

@scorriere
Copy link
Contributor Author

I'm not really certain how to fix the Mac CI tests because they are failing when installing dependencies. I think the copying of libtcl is the real error here? Should .github./workflows/mac-ci.yml have that step changed to include a -f?

@bovine bovine linked an issue Aug 15, 2024 that may be closed by this pull request
@bovine
Copy link
Member

bovine commented Aug 16, 2024

The Mac CI issue has been resolved. I've updated your PR to avoid conflicts from other changes in our master branch.

Do we also need to add another element to optionTable[] for parity with the configTable[]? CURLOPT_SSLVERSION is already present once. https://github.com/flightaware/tclcurl-fa/pull/22/files#diff-a6144812bd1cfbfc5f8bf9742b646862cf6c60f6405d8aec3452eb79de96e1a7L140-L205

@scorriere
Copy link
Contributor Author

Yeah, it probably should go there? To be honest, it has been 2 years since I have looked at this code and I don't think I fully understood it then.

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

Successfully merging this pull request may close these issues.

Support TLS Version and TLS Max Version
2 participants