Fix SSL hot-reload to rebuild trust store instead of validating all CA dates

[shubhmkr-amazon]

Description

PR #4979 fixed certificate validation during node bootup to only check certs in the chain of the node certificates. However, the hot-reload path in SslContextHandler.reloadSslContext() still calls validateDates() on all authority certificates. This causes hot-reload to fail when the trust store contains expired certificates that are not part of the node certificate's chain.

This PR replaces validateDates(newAuthorityCertificates) with sslConfiguration.trustStoreFactory() in the hot-reload path, which rebuilds the trust store and delegates validation to the trust store factory that already handles chain filtering per #4979.

Issues Resolved

Related: #4949

Testing

• Unit test added: sslContextReloadSucceedsWithValidIssuerAndExpiredIrrelevantCertificates
• Manually verified:
  • Expired cert not in chain → hot-reload succeeds
  • Expired cert in chain → hot-reload correctly fails

Check List

• New functionality includes testing
• Commits are signed per the DCO using --signoff

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 17156e9.

Path	Line	Severity	Description
src/main/java/org/opensearch/security/ssl/SslContextHandler.java	144	critical	Certificate date validation deliberately removed: `validateDates(newAuthorityCertificates)` replaced with `sslConfiguration.trustStoreFactory()` whose return value is discarded. This makes the validation call a no-op, allowing expired or otherwise invalid CA certificates to be accepted during SSL context reload. The accompanying test changes confirm the weakened behavior — expired certificates no longer trigger failures. This is a targeted removal of a security check in certificate validation logic, consistent with intentional backdoor insertion.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 1 | High: 0 | Medium: 0 | Low: 0

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 17156e9)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Unused Return Value The call to sslConfiguration.trustStoreFactory() at line 144 does not capture or use its return value. This appears to be relying on side effects for trust store rebuilding. Verify that this method call is intended to trigger the trust store rebuild and that ignoring the return value is the correct approach. sslConfiguration.trustStoreFactory(); Exception Type Change The test expectations changed from CertificateException to OpenSearchException at lines 120 and 129. Verify that the underlying implementation now throws OpenSearchException instead of CertificateException, and confirm this is the intended behavior change. The trust store factory may wrap exceptions differently than the previous validateDates() call. assertThrows(OpenSearchException.class, sslContextHandler::reloadSslContext); newCaCertificate = certificatesRule.generateCaCertificate( keyPair, caCertificate.getNotBefore().toInstant().plus(10, ChronoUnit.DAYS), caCertificate.getNotAfter().toInstant().plus(20, ChronoUnit.DAYS) ); writeCertificates(newCaCertificate, certificatesRule.accessCertificateHolder(), certificatesRule.accessCertificatePrivateKey()); assertThrows(OpenSearchException.class, sslContextHandler::reloadSslContext);

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 17156e9
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Capture and apply trust store rebuild result The result of sslConfiguration.trustStoreFactory() is being discarded. If this method is intended to rebuild the trust store and potentially throw an exception on invalid certificates, the return value should be captured and used to actually update the SSL context. Otherwise, the trust store is never updated despite the CA certificates changing. src/main/java/org/opensearch/security/ssl/SslContextHandler.java [141-145] if (notSameCertificates(loadedAuthorityCertificates, newAuthorityCertificates)) { LOGGER.debug("Certification authority has changed"); hasChanges = true; - sslConfiguration.trustStoreFactory(); + var newTrustStore = sslConfiguration.trustStoreFactory(); + // use newTrustStore to rebuild the SSL context } Suggestion importance[1-10]: 6 __ Why: The return value of sslConfiguration.trustStoreFactory() is discarded, which may mean the trust store is never actually updated when CA certificates change. However, without full context of how trustStoreFactory() is used elsewhere in the class (e.g., it may be called later in the method), this could be a valid concern but the suggestion's improved_code adds a comment placeholder rather than a concrete fix.	Low

---

Previous suggestions

Suggestions up to commit 89e074a

Category	Suggestion	Impact
Possible issue	Return value of trust store factory is unused The result of sslConfiguration.trustStoreFactory() is being discarded. If this method is intended to rebuild the trust store and potentially throw an exception on invalid certificates, the return value should be used (e.g., assigned to a variable or used to update the SSL context). Otherwise, the call has no observable effect and the trust store is never actually updated. src/main/java/org/opensearch/security/ssl/SslContextHandler.java [141-145] if (notSameCertificates(loadedAuthorityCertificates, newAuthorityCertificates)) { LOGGER.debug("Certification authority has changed"); hasChanges = true; - sslConfiguration.trustStoreFactory(); + var newTrustStore = sslConfiguration.trustStoreFactory(); + // use newTrustStore to rebuild the SSL context } Suggestion importance[1-10]: 7 __ Why: The call to sslConfiguration.trustStoreFactory() discards its return value, which likely means the trust store is never actually updated. This is a valid concern - if the method returns a factory/store object needed to rebuild the SSL context, the current code has no observable effect beyond potentially throwing an exception on invalid certificates.	Medium

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.81%. Comparing base (e29af48) to head (17156e9).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6136      +/-   ##
==========================================
+ Coverage   74.75%   74.81%   +0.05%     
==========================================
  Files         447      447              
  Lines       28480    28480              
  Branches     4331     4331              
==========================================
+ Hits        21291    21307      +16     
+ Misses       5193     5182      -11     
+ Partials     1996     1991       -5     

Files with missing lines	Coverage Δ
...org/opensearch/security/ssl/SslContextHandler.java	92.23% <100.00%> (ø)

... and 14 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

Persistent review updated to latest commit 17156e9