Support loading system certs from Keychein on MacOS #1474

yerseg · 2023-01-24T14:21:16Z

Added support for loading certificates from the Keychain on MacOS, similar to Windows.
I have tested it in my project and it works.

I am not sure how to properly add the necessary MacOS SDK frameworks to cmake and make, I will be glad to comments.

Tachi107 · 2023-02-13T13:28:06Z

Could you please also update the meson.build file? It should be enough to add the following under the Brotli checks:

if host_machine.system() == 'darwin'
  deps += dependency('appleframeworks', modules: ['CoreFoundation', 'Security'])
endif

Thanks!

httplib.h

…expressions in cmake

yerseg · 2023-02-13T15:19:09Z

@Tachi107 thank you for review! I added the necessary deps to meson.build as you said

Tachi107

Some minor feedback

httplib.h

meson.build

yhirose · 2023-02-14T20:18:17Z

httplib.h

@@ -7834,10 +7898,11 @@ inline bool SSLClient::load_certs() {
        ret = false;
      }
    } else {
+      SSL_CTX_set_default_verify_paths(ctx_);


This call should be in #else block. Please see the original code.

I'm not very familiar with openssl, but I've come to the following.
If we fail to load certs from the system storage, then it would be correct to call SSL_CTX_set_default_verify_paths.
Am I wrong?

On Windows maybe it didn't matter, but for MacOS it has value at all I guess…

Here is the link to SSL_CTX_set_default_verify_paths:
https://www.openssl.org/docs/man3.1/man3/SSL_CTX_set_default_verify_paths.html

So if we successfully load the OS certs, we shouldn't call SSL_CTX_set_default_verify_paths.

You probably could do as below:

auto loaded = false; #ifdef _WIN32 loaded = detail::load_system_certs_on_windows(SSL_CTX_get_cert_store(ctx_)); #elif defined(__APPLE__) loaded = detail::load_system_certs_on_apple(SSL_CTX_get_cert_store(ctx_)); #endif if (!loaded) { SSL_CTX_set_default_verify_paths(ctx_); }

This means, always SSL_CTX_set_default_verify_paths gets called on linux. As for Windows and MacOS, if the detail::load_system_certs_on_??? fails, then SSL_CTX_set_default_verify_paths will be called as the last resort function.

Yeah, you're right. But when we first use SSL_CTX_set_default_verify_paths, it means that if something screws up, the system will at least try to find certificates in the default places. As far as I can tell, adding extra certificates using X509_STORE_add_cert after setting the default paths shouldn't cause any issues.

To put it another way, I believe that calling SSL_CTX_set_default_verify_paths first and then load_system_certs will either result in the same or an even better state than if load_system_certs were called first and then set_default_verify_paths.

In the original code for Windows, we don't have to call SSL_CTX_set_default_verify_paths before/after load_system_certs_on_windows. I simply would like to do the same for MacOS since it is absolutely unnecessary to call SSL_CTX_set_default_verify_paths if load_system_certs_on_apple succeeds.

If you really want to set the default path before loading certs, just call it after initializing httplib::SSLClient.

httplib::SSLClient client; SSL_CTX_set_default_verify_paths(client.ssl_context());

httplib.h

yhirose · 2023-02-15T22:50:02Z

@yerseg I have just approved the change, but the CI test on GitHub Actions reported a number of failures... Could you run the unit tests on your local machine by cd test; make? Thanks!

jimmy-park · 2023-02-16T04:54:41Z

CMakeLists.txt

@@ -206,6 +206,8 @@ target_link_libraries(${PROJECT_NAME} ${_INTERFACE_OR_PUBLIC}
 		$<$<PLATFORM_ID:Windows>:ws2_32>
 		$<$<PLATFORM_ID:Windows>:crypt32>
 		$<$<PLATFORM_ID:Windows>:cryptui>
+		# Needed for API from MacOS Security framework
+		$<$<AND:$<PLATFORM_ID:Darwin>,$<BOOL:${HTTPLIB_IS_USING_OPENSSL}>>:"-framework CoreFoundation -framework Security">


Quoting the inside of generator expression is not enough when it contains whitespaces. The whole expression should be quoted.

"$<$<AND:$<PLATFORM_ID:Darwin>,$<BOOL:${HTTPLIB_IS_USING_OPENSSL}>>:-framework CoreFoundation -framework Security>"

fixed it, thanks

yerseg · 2023-02-16T18:55:45Z

@yhirose It turned out that there is another keychain from which we need to load certificates. Now all the tests pass.

yhirose · 2023-02-16T22:25:06Z

httplib.h

+  if (!query) { return false; }
+
+  CFTypeRef security_items = nullptr;
+  OSStatus err = SecItemCopyMatching(query.get(), &security_items);


You can use auto.

yhirose · 2023-02-16T22:26:10Z

httplib.h

+
+inline bool retrieve_root_certs_from_keychain(CFObjectPtr<CFArrayRef> &certs) {
+  CFArrayRef root_security_items = nullptr;
+  OSStatus err = SecTrustCopyAnchorCertificates(&root_security_items);


You can use auto.

yhirose · 2023-02-16T22:26:46Z

httplib.h

+}
+
+inline void add_certs_to_x509_store(CFArrayRef certs, X509_STORE *store) {
+  OSStatus err = errSecSuccess;


This line can be removed.

yhirose · 2023-02-16T22:37:31Z

httplib.h

+    err = SecItemExport(cert, kSecFormatX509Cert, 0, nullptr, &cert_data);
+    CFObjectPtr<CFDataRef> cert_data_ptr(cert_data, cf_object_ptr_deleter);
+
+    if (err != errSecSuccess) { continue; }


The above 4 lines can be as below:

CFDataRef cert_data = nullptr; if (SecItemExport(cert, kSecFormatX509Cert, 0, nullptr, &cert_data) != errSecSuccess) { continue; } CFObjectPtr<CFDataRef> cert_data_ptr(cert_data, cf_object_ptr_deleter);

yhirose · 2023-02-16T22:39:41Z

httplib.h

+  CFObjectPtr<CFArrayRef> security_items_ptr(
+      reinterpret_cast<CFArrayRef>(security_items), cf_object_ptr_deleter);
+
+  if (err != errSecSuccess) { return false; }


This line should be moved above the previous line.

yhirose · 2023-02-16T22:41:53Z

httplib.h

@@ -7834,11 +7927,14 @@ inline bool SSLClient::load_certs() {
        ret = false;
      }
    } else {
+      bool loaded = false;


You can use auto.

yerseg · 2023-02-17T08:11:01Z

@yhirose fixed your comments

yhirose · 2023-02-17T17:07:25Z

@yerseg, It has been merged. Thanks for your fine contribution!

yerseg · 2023-02-17T19:11:08Z

@yhirose thank you too!

yhirose · 2023-03-10T16:26:49Z

@yerseg please see #1522. it seems like a number of users on MacOS will encounter the link problem from now on. I am now thinking to make this feature 'opt in' with CPPHTTPLIB_USE_MAC_OS_SYSTEM_CERTS or somthing, since SSL_CTX_set_default_verify_paths has been working anyway with no problem on MacOS for long time.

Or do you have any solution to reduce the expected user support caused by this? At least we have to mention CoreFoundation and Security need to be linked on README. (On Windows, we embed #pragma comment(lib, "ws2_32.lib") in httplib.h, so that we can force the compiler to link the system library.)

Thanks!

yerseg · 2023-03-10T17:59:16Z

@yhirose I have been thinking about this problem, and I’m agree that we should make this feature optional.
It will be great in case Unix toolchains provide some stuff like “#pragma comment lib” in MSVC, but we have not.
So I will add the macro in new Pull request soon.

yhirose · 2023-03-10T22:49:38Z

I added note for macOS on README for now. d1b6162

* Support loading system certs from Keychein on MacOS * review improvements: add deps to meson.build and improve conditional expressions in cmake * fix tabs * fix tabs * review improvements * fix after review * additionally load root certs from the system root keychain * cmake fix * fix * small refactoring * small refactoring --------- Co-authored-by: Sergey Kazmin <[email protected]>

Support loading system certs from Keychein on MacOS

3a50ea7

yerseg force-pushed the support_loading_certs_macos branch from b29819c to 3a50ea7 Compare January 24, 2023 16:46

Tachi107 reviewed Feb 13, 2023

View reviewed changes

httplib.h Outdated Show resolved Hide resolved

review improvements: add deps to meson.build and improve conditional …

47f2d4b

…expressions in cmake

Sergey Kazmin added 2 commits February 13, 2023 18:21

fix tabs

6357815

fix tabs

6daa459

Tachi107 reviewed Feb 13, 2023

View reviewed changes

httplib.h Outdated Show resolved Hide resolved

httplib.h Outdated Show resolved Hide resolved

meson.build Outdated Show resolved Hide resolved

review improvements

b3ab29e

yerseg requested a review from Tachi107 February 14, 2023 14:41

yhirose reviewed Feb 14, 2023

View reviewed changes

fix after review

30eae7f

yhirose reviewed Feb 15, 2023

View reviewed changes

httplib.h Outdated Show resolved Hide resolved

jimmy-park reviewed Feb 16, 2023

View reviewed changes

Sergey Kazmin added 2 commits February 16, 2023 21:48

additionally load root certs from the system root keychain

6155137

cmake fix

c933b46

fix

061bdd6

yhirose reviewed Feb 16, 2023

View reviewed changes

Sergey Kazmin added 2 commits February 17, 2023 11:06

small refactoring

ae879de

small refactoring

58b888c

yhirose merged commit 6d963fb into yhirose:master Feb 17, 2023

yerseg deleted the support_loading_certs_macos branch February 17, 2023 17:35

yhirose mentioned this pull request Mar 10, 2023

The lastest version can not link some func in macOS. #1522

Closed

jimmy-park mentioned this pull request Apr 1, 2023

Don't loading system certs from Keychain on iOS #1546

Merged

Support loading system certs from Keychein on MacOS #1474

Support loading system certs from Keychein on MacOS #1474

Uh oh!

Conversation

yerseg commented Jan 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tachi107 commented Feb 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

yerseg commented Feb 13, 2023

Uh oh!

Tachi107 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yerseg Feb 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yhirose commented Feb 15, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yerseg commented Feb 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yhirose Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yerseg commented Feb 17, 2023

Uh oh!

yhirose commented Feb 17, 2023

Uh oh!

yerseg commented Feb 17, 2023

Uh oh!

yhirose commented Mar 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yerseg commented Mar 10, 2023

Uh oh!

yhirose commented Mar 10, 2023

Uh oh!

Uh oh!

yerseg commented Jan 24, 2023 •

edited

Loading

Tachi107 commented Feb 13, 2023 •

edited

Loading

yerseg Feb 14, 2023 •

edited

Loading

yhirose Feb 16, 2023 •

edited

Loading

yhirose commented Mar 10, 2023 •

edited

Loading