-
Notifications
You must be signed in to change notification settings - Fork 460
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
[Windows] Build curl for Windows without openssl. #4545
[Windows] Build curl for Windows without openssl. #4545
Conversation
@RichardHaselgrove, could you please test this PR? |
@AenBleidd - I'll get onto it within the hour. I want to re-run the VS2019 build first, so I can get you the error messages you asked for. I'll be on more familiar territory to do a PR test under VS2013 - would that be acceptable? |
No, this is for VS2019 only. |
Btw, you can just use build artifacts from GitHub actions to test this |
That would be nice, because as you can see from our CI, it's build with VS2019 without any issues |
Test machine is booted back into Win 10. I'll post results from the build back in my issue from last night, then try the artifacts from here. |
2735ad8
to
9fbbad1
Compare
Provisional report only. I've reverted to a standard v7.16.11 installation, and verified that it failed to connect with two of the projects that are affected by the current outage - GPUGrid and CPDN. I then replaced boinc.exe and boincmgr.exe with the artifacts from here: noting that ca-bundle.crt is still deployed by the test build. But I deleted it from the working folder to test - I also deleted the curl and OpenSSL DLLs from the standard installation. Client attached successfully to GPUGrid. I'll check it again on Windows 7 - I haven't got any older versions in use at the moment, though I know some volunteers do. |
@AenBleidd I think you also need to modify client/http_curl.cpp lines 489 - 539 for this to work. |
Tried it under Windows 7 - same procedure as last time. Got this in the log, which seems about right.
I'll check up on the 'get_project_cfg' failure direct with the project. I've allowed production work to resume, and scheduler contacts and downloads seem to be proceeding normally. |
Build it with schannel instead that allows curl to use Windows certificate store and doesn't relay on ca-bundle.crt anymore. This fixes BOINC#4542 Signed-off-by: Vitalii Koshura <[email protected]>
9fbbad1
to
ee640ea
Compare
@CharlieFenton,probably you're right. Fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a working MS Windows computer so I can't do any testing, but this looks OK as far as I can tell. I am not very knowledgeable about ssl and encryption, so I don't know whether any code in lib/crypt.cpp depends on the certificate.
Works well on Win7 SP1 and higher (have no Vista to check on it also) |
@AenBleidd Please excuse my ignorance about vcpkg, but in the changed files I do not see anything that builds curl without OpenSSL. according to the help in curl's configure file, this involves building curl with the option My understanding from this document is that to use Windows built-in certificate you should build cur with either I see nothing in this PR that does any of these things. To answer your question from PR #4544, the above referenced document says that curl can be told to use MacOS system certificate store by Secure Transport Support by building with this option: |
@AenBleidd Please ignore my previous question. I see there is a reference to channel in your changed vcpkg call. |
@AenBleidd Please see my suggestions here for an alternate approach. |
It works on Windows Vista SP2 also. |
@CharlieFenton, according to this: https://curl.se/libcurl/c/CURLOPT_CAINFO.html
So probably we can put back the code that checks for ca-bundle.crt file and instead put some option to use ca-bundle.crt instead of Windows certificate store in case of some errors. What do you think? |
Would this not always use the ca-bundle.crt file instead of the Windows certificate store, unless this code is executed only after an error? In any case, I think the alternate approach we have been discussing in PR #4544 might be a better long-term solution than the one in this PR for the reasons I gave here. |
Specifically, I mean the idea of adding code to the BOINC client so it automatically downloads a new ca-bundle.crt when the old one is close to expiration. |
Do we have anyone in the development community who can remember the original rationale for deciding on the current Windows procedure in the first place? As I remember it, it was Rom Walton who did much of the Windows work. |
I checked this solution on Windows Vista. And it still works. That means that Vista that was not updated since 2017 still works fine with BOINC. I doubt anyone still uses it but anyway. Later we can add a code to update directly Windows certificate store with necessary certificates if necessary |
I believe it's because before Windows Vista there was no Windows certificate store support. |
Makes sense. Anyone still running Windows XP (and I believe there are a few) will know that they're on their own when problems like this strike. They still have the opportunity to replace their certificate bundle manually. |
If the question is why ca-bundle.crt is put in the BOINC Programs directory and not the data directory, then looking at https://boinc.berkeley.edu/trac/wiki/ClientSetupLogicWinFileLayout this was the setup for the client version 5, which solely ran from the C:\Program Files\BOINC directory. Those were Windows 95, 98, ME and XP days, maybe even 2000. When the client setup was changed to present logic (https://boinc.berkeley.edu/trac/wiki/ClientSetupLogicWinSix, https://boinc.berkeley.edu/trac/wiki/ClientSetupWinSix) the setup of the BOINC Programs directory was left unchanged. |
By the way, I also found https://boinc.berkeley.edu/trac/wiki/AdminReleaseManagement which stipulates:
Which apparently isn't followed these days, maybe we should bring it back. |
That's exactly what I did in my last PR. I plan to automate this process somehow to always have on master the latest ca-bundle.crt file |
And that was added in Version 18 by David Anderson - on May 30, 2020. I wonder why that date seems familiar in this context? |
Because a day later he released 7.17.7? (
https://boinc.berkeley.edu/dl/?C=M;O=D)
|
Precisely. |
That link shows some potentially useful ways of automatically updating ca-bundle.crt.
and
However, we may not want to have every BOINC client download the certificates from curl.se because the web page implies their servers could get overloaded:
I strongly feel this PR should be put on hold in favor of having the BOINC client automatically update ca-bundle.crt (and of course, use a current version of OpenSSL.) |
@davidpanderson, what do you think? Should we go with this solution (use Windows certificate store) or should I implement automatic ca-bundle.crt update? |
Let's use the Windows certificate store. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, let's get this show on the road. I approve.
Build it with schannel instead that allows curl to use Windows certificate store and doesn't relay on ca-bundle.crt anymore.
This fixes #4542
Signed-off-by: Vitalii Koshura [email protected]