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

[Networking] Search for CA roots if libcurl doesn't know where they are. #4877

Merged
merged 2 commits into from May 2, 2024

Conversation

al45tair
Copy link
Contributor

Normally, distro maintainers will tell libcurl where to look when they build it, and up to now we've been relying on that. That doesn't work for the fully static Linux build, where we're building our own libcurl, and where the idea is that we'll run on any old Linux system.

To make TLS work under that circumstance, we'll need to look in a few likely places for CA root files. We only do this if libcurl doesn't already know where to look.

rdar://123434144

Normally, distro maintainers will tell libcurl where to look when they
build it, and up to now we've been relying on that.  That doesn't work
for the fully static Linux build, where we're building our own libcurl,
and where the idea is that we'll run on any old Linux system.

To make TLS work under that circumstance, we'll need to look in a few
likely places for CA root files.  We only do this if libcurl doesn't
already know where to look.

rdar://123434144
@al45tair
Copy link
Contributor Author

@swift-ci Please test

@al45tair
Copy link
Contributor Author

al45tair commented Feb 22, 2024

Looks like older curl might not define CURLINFO_CAINFO.

Update: according to the curl repo, we need curl 7.84.0 to get CURLINFO_CAINFO.

@al45tair
Copy link
Contributor Author

Not sure what that macOS failure is about. Seems we're generating references to _swift_getTypeByMangledNameInContext2, which doesn't exist?

`curl` versions before 7.84.0 don't have `CURLINFO_CAINFO`, so we
can't tell whether they've got an existing path for the CA roots
in that case.

We should disable the automatic search in that case, because it
would override the baked-in default.

rdar://123434144
@al45tair
Copy link
Contributor Author

@swift-ci Please test

@al45tair
Copy link
Contributor Author

@swift-ci test windows

@MaxDesiatov
Copy link
Member

@swift-ci test macos

@al45tair
Copy link
Contributor Author

swift-ci test macos

This code isn't used on macOS. :-)

(I realised after my comment last week, and after noticing that only Linux and Windows were required.)

@MaxDesiatov
Copy link
Member

I understand, I would like to see if macOS is still failing for all PRs or only a specific one.

Copy link
Contributor

@jrflat jrflat left a comment

Choose a reason for hiding this comment

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

Looks good! I just left a couple comments/questions

// be set, so leave things alone
var p: UnsafeMutablePointer<Int8>? = nil

try! CFURLSession_easy_getinfo_charp(rawHandle, CFURLSessionInfoCAINFO, &p).asError()
Copy link
Contributor

Choose a reason for hiding this comment

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

Have we tested with curl < 7.84.0 to make sure the NS_CURL_MISSING_CURLINFO_CAINFO properly guards this code? I think it should, but I'd just want to make sure, otherwise I think this will always fatalError since curl will return CURLE_UNKNOWN_OPTION. It might be good to just catch the error here and return instead of force trying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. The original version of this PR actually failed because of this problem, so the guard definitely works.

"/etc/pki/tls/certs/ca-bundle.crt",
"/usr/share/ssl/certs/ca-bundle.crt",
"/usr/local/share/certs/ca-root-nss.crt",
"/etc/ssl/cert.pem"
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, is there reasoning or precedence for this search order? (I don't see any issue with it, I'm just not as familiar.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took the list from curl's build system (I assumed that was a good place to look, because it already knows how to do this, just during configuration rather than at runtime).

isDirectory: &isDirectory)
&& !isDirectory.boolValue {
path.withCString { pathPtr in
try! CFURLSession_easy_setopt_ptr(rawHandle, CFURLSessionOptionCAINFO, UnsafeMutablePointer(mutating: pathPtr)).asError()
Copy link
Contributor

Choose a reason for hiding this comment

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

Same idea as above making sure with don't get CURLE_UNKNOWN_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.

Again, the we failed in one of the previous iterations because of this, so yes, it's been tested.

Copy link
Contributor

@jrflat jrflat left a comment

Choose a reason for hiding this comment

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

LGTM. macOS is failing on all PRs, and this code isn't used on macOS, so we should be good to merge

@al45tair al45tair merged commit 0607287 into apple:main May 2, 2024
2 of 3 checks passed
al45tair added a commit to al45tair/swift-corelibs-foundation that referenced this pull request May 3, 2024
[Networking] Search for CA roots if libcurl doesn't know where they are.
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.

None yet

3 participants