Implement querying the system certificate store on each platform #4892

fmartian · 2025-10-23T21:58:46Z

Description

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 several expired certificates

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

Also fixes (or at least relieves) The viewer has 3 expired certs #4505

Checklist

Please ensure the following before requesting review:

I have provided a clear title and detailed description for this pull request.
The PR is linked to a relevant issue with sufficient context.
I have tested the changes locally and verified they work as intended.
Documentation has been updated if needed.
Any dependent changes have been merged and published in downstream modules
I have reviewed the contributing guidelines.

- 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

…suppress warnings

Ansariel · 2025-10-24T19:54:01Z

indra/newview/llsechandler_basic.cpp

 {
+    bool success = false;
+#if LL_WINDOWS
+    HCERTSTORE cert_store = CertOpenSystemStoreA(0, storename.empty() ? "ROOT" : storename.c_str());


Is there a reason to call the ANSI version explicitly instead of using CertOpenSystemStore?

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.

I suggest adding this as a comment in-code.

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?

According to AI it might be because security framework isn't linked to the test in question.

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.

You'll need to add a find_library(SECURITY_LIBRARY Security) in Linking.cmake and add it to the list of linked libraries.

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.

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.

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.

Copilot

Pull Request Overview

This PR implements querying the system certificate store on each platform (Windows, macOS, and Linux) to provide CA certificates for SSL/TLS validation, with a fallback to the bundled certificate file. The implementation moves certificate enumeration logic into LLBasicCertificateVector and adds a new LLSystemCertificateVector class that queries platform-specific certificate stores.

Key changes:

Refactored certificate loading into reusable methods in LLBasicCertificateVector with proper validation
Added LLSystemCertificateVector class with platform-specific implementations for Windows (CryptoAPI), macOS (Security framework), and Linux (filesystem paths)
Added suppress_expire_warning parameter throughout the certificate validation chain to reduce noise from expired certificates in system stores

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
indra/newview/llsechandler_basic.h	Added `LLSystemCertificateVector` class and refactored `LLBasicCertificateVector` to support loading certificates from files/directories; cleaned up redundant declarations
indra/newview/llsechandler_basic.cpp	Implemented platform-specific system certificate store enumeration for Windows, macOS, and Linux; refactored certificate loading and validation logic; updated initialization to use system certificates
indra/newview/llsecapi.h	Added `suppress_warning` parameter to `LLCertException` and `LLCertValidationExpirationException` constructors
indra/newview/llsecapi.cpp	Modified `LLCertException` constructor to conditionally suppress warning messages based on the new parameter
indra/newview/tests/llsechandler_basic_test.cpp	Updated test mock to match new `LLCertException` constructor signature with `suppress_warning` parameter

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-17T13:54:27Z

indra/newview/llsechandler_basic.cpp

+}
+
+// verify the certificate to be valid and not expired and add it to the store. If the certificate
+// already exists in the store (based on CERT_SUBJECT_KEY_IDENTFIER), it will be ignored


Typo in comment: "CERT_SUBJECT_KEY_IDENTFIER" should be "CERT_SUBJECT_KEY_IDENTIFIER" (missing 'I')

Copilot · 2025-11-17T13:54:27Z

indra/newview/llsechandler_basic.cpp

+    CertCloseStore(cert_store, 0);
+#elif LL_DARWIN
+    // FM: Quite some googling led eventually to some old MacOS Open Source repository that showed the
+    // implementation for SecTrustSettignsCopyCertificates() and that eventually led to this code


Typo in comment: "SecTrustSettignsCopyCertificates" should be "SecTrustSettingsCopyCertificates"

Suggested change

// implementation for SecTrustSettignsCopyCertificates() and that eventually led to this code

// implementation for SecTrustSettingsCopyCertificates() and that eventually led to this code

Copilot · 2025-11-17T13:54:28Z

indra/newview/llsechandler_basic.cpp

+        }
+        else
+        {
+            LL_WARNS("SECAPI") << "Failed to initialize the certificate store with valid CA certificates."


Missing space between sentences in the warning message. Should be: "It's probably not possible to connect to any internet service!" or add a space before "It's"

Suggested change

LL_WARNS("SECAPI") << "Failed to initialize the certificate store with valid CA certificates."

LL_WARNS("SECAPI") << "Failed to initialize the certificate store with valid CA certificates. "

Copilot · 2025-11-17T13:54:28Z

indra/newview/llsechandler_basic.cpp

+        "/etc/pki/tls/certs", // Fedora/RHEL
+    };
+
+    const std::string certFile = getenv("SSL_CERT_FILE");


The return value of getenv("SSL_CERT_FILE") can be nullptr on some platforms, which would cause undefined behavior when constructing a std::string. Use a null check before constructing the string, e.g., const char* env_val = getenv("SSL_CERT_FILE"); const std::string certFile = env_val ? env_val : "";

Copilot · 2025-11-17T13:54:28Z

indra/newview/llsechandler_basic.cpp

+    }
+    if (!success)
+    {
+        const std::string dirs = getenv("SSL_CERT_DIR");


The return value of getenv("SSL_CERT_DIR") can be nullptr on some platforms, which would cause undefined behavior when constructing a std::string. Use a null check before constructing the string, e.g., const char* env_val = getenv("SSL_CERT_DIR"); const std::string dirs = env_val ? env_val : "";

Suggested change

const std::string dirs = getenv("SSL_CERT_DIR");

const char* env_val = getenv("SSL_CERT_DIR");

const std::string dirs = env_val ? env_val : "";

github-actions bot added the c/cpp label Oct 23, 2025

fmartian added 3 commits October 24, 2025 00:21

Add some more comments

b76958a

Create debug messages on certificate exceptions even when we want to …

eb24630

…suppress warnings

Fix compile error in test

a7d1885

Ansariel reviewed Oct 24, 2025

View reviewed changes

akleshchev requested review from Geenz and marchcat October 30, 2025 14:01

fmartian added 3 commits November 6, 2025 21:19

Merge branch 'secondlife:develop' into SystemSecurity

139b8a7

Merge branch 'secondlife:develop' into SystemSecurity

f2a6e3e

Merge branch 'secondlife:develop' into SystemSecurity

64b6248

akleshchev requested a review from Copilot November 17, 2025 13:51

Copilot started reviewing on behalf of akleshchev November 17, 2025 13:52 View session

Copilot finished reviewing on behalf of akleshchev November 17, 2025 13:53

Copilot AI reviewed Nov 17, 2025

View reviewed changes

Merge branch 'secondlife:develop' into SystemSecurity

2325bc5

	// implementation for SecTrustSettignsCopyCertificates() and that eventually led to this code
	// implementation for SecTrustSettingsCopyCertificates() and that eventually led to this code

	LL_WARNS("SECAPI") << "Failed to initialize the certificate store with valid CA certificates."
	LL_WARNS("SECAPI") << "Failed to initialize the certificate store with valid CA certificates. "

	const std::string dirs = getenv("SSL_CERT_DIR");
	const char* env_val = getenv("SSL_CERT_DIR");
	const std::string dirs = env_val ? env_val : "";

Implement querying the system certificate store on each platform #4892

Are you sure you want to change the base?

Implement querying the system certificate store on each platform #4892

Conversation

fmartian commented Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Checklist

Uh oh!

Ansariel Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

fmartian Oct 24, 2025 • 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!

akleshchev Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

fmartian Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

akleshchev Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

fmartian Oct 31, 2025 • 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!

RyeMutt Oct 31, 2025 • 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!

akleshchev Oct 31, 2025 • 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!

fmartian Oct 31, 2025 • 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!

akleshchev Oct 31, 2025 • 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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fmartian commented Oct 23, 2025 •

edited

Loading

fmartian Oct 24, 2025 •

edited

Loading

fmartian Oct 31, 2025 •

edited

Loading

RyeMutt Oct 31, 2025 •

edited

Loading

akleshchev Oct 31, 2025 •

edited

Loading

fmartian Oct 31, 2025 •

edited

Loading

akleshchev Oct 31, 2025 •

edited

Loading