Implement querying the system certificate store on each platform #4892
Conversation
- Move the enumerations of the certificates into the LLBasicCertificateVector and implement a derived class LLSystemCertificateVector - For each platform try to query the system certificate store and fall back to the application provided certification bundle if that fails - Be more restrictive in issuing warning messages about rejected certificates since the system certificate store on Windows and Mac at least generally also contains a few expired certificates
| { | ||
| bool success = false; | ||
| #if LL_WINDOWS | ||
| HCERTSTORE cert_store = CertOpenSystemStoreA(0, storename.empty() ? "ROOT" : storename.c_str()); |
There was a problem hiding this comment.
Is there a reason to call the ANSI version explicitly instead of using CertOpenSystemStore?
There was a problem hiding this comment.
Actually yes. It avoids having to make a std::string to std:wstring conversion. Basically std::string is always an 8-bit string while the undecorated Win32 API may use wchar or char, depending on some "obscure" UNICODE preprocessor define. So if someone changes that define you end up with compiler errors. The undecorated Win32 API should only be used if you also pass TCHAR/LPTCHAR variables to them. Otherwise you create a potential type conversion error.
Since this string is anyhow only for special cases (needs more work to make this a somewhat useful parameter across all three platforms) and the various security stores on each platform have varying ideas about how they organize it (on my Mac only the kSecTrustSettingsDomainSystem really returns any significant number of certificates, it turned out to be a somewhat academic parameter. However I left it in as an option for now.
The idea was that by passing in different keywords, one could select a specific sort of keystore to query. On Windows this would by for instance ROOT, MY and CA, on Mac one of the three enums, on Linux ... oh well it's a mess! :-)
If we really want to use that eventually, an enum similar to what the Mac uses may be more prudent but more likely it just can go away eventually.
There was a problem hiding this comment.
I suggest adding this as a comment in-code.
There was a problem hiding this comment.
MacOS isn't quite my expertise. But I see that in the test build it fails because of these:
Undefined symbols for architecture arm64:
"_SecCertificateCopyData", referenced from:
LLSystemCertificateVector::load_from_system(std::__1::basic_string<char, std::__1::char_traits, std::__1::allocator> const&, bool) in llsechandler_basic.o
"_SecTrustSettingsCopyCertificates", referenced from:
LLSystemCertificateVector::load_from_system(std::__1::basic_string<char, std::__1::char_traits, std::__1::allocator> const&, bool) in llsechandler_basic.o
Does this mean that these SecCertificate.. APIs have been depreciated, not implemented, forgotten by Apple or what for the ARM platform?
There was a problem hiding this comment.
According to AI it might be because security framework isn't linked to the test in question.
There was a problem hiding this comment.
Missed the x86_64 failure. Those logs are unwieldly long. :-(
What about an option to disable the system store with an optional parameter to the LLSecAPIBasicHandler() constructor? If this parameter is set to true, it instantiates the LLBasicCertificateVector with the existing certificate file rather than the derived LLSystemCertificateVector.
Although that won't solve the linking issue. Have to think about this a bit more.
There was a problem hiding this comment.
You'll need to add a find_library(SECURITY_LIBRARY Security) in Linking.cmake and add it to the list of linked libraries.
There was a problem hiding this comment.
You'll need to add a find_library(SECURITY_LIBRARY Security)
Thanks! I knew that it needed a framework or library, but I just don't see where viewer is adding it or how to do it.
What about an option to disable the system store with an optional parameter to the LLSecAPIBasicHandler() constructor?
It's a linking error, not a runtime one.
There was a problem hiding this comment.
You'll need to add a find_library(SECURITY_LIBRARY Security)
Thanks! I knew that it needed a framework or library, but I just don't see where viewer is adding it or how to do it.
Already found it. It is in indra/cmake/Linking.cmake where it needs to be added. Also needs a similar addition for crypt32 in the Windows case. Wonder why it compiled and linked for me on both Mac and Windows without that.
What about an option to disable the system store with an optional parameter to the LLSecAPIBasicHandler() constructor?
It's a linking error, not a runtime one.
Yes I know. But my concern is more about the fact that a half working system store (not sure how the test systems are exactly setup and if they are a fully installed OS image) might actually make the tests fail for some reason as they are working with specific pre configured certificate resources.
There was a problem hiding this comment.
But my concern is more about the fact that a half working system store
It only uses old LLBasicCertificateVector's constructor so that part should be fine. And if not the right way might be to update test<5> to account for extra certs.
4 participants
Description
This could eventually allow to not having to ship the application provided CA certificate bundle anymore and rely on the certificates from the Operating System, which are normally regularly updated.
Related Issues
Checklist
Please ensure the following before requesting review: