Sort the DNS Names in the SANs

[Mehdi-Bendriss]

Description

• Category: Bug fix
• Why these changes are required?
  • During the hot reload of a certificate, comparing equivalent certificates with multiple DNS Names on the SAN will fail if the order of the entries doesn't match.
• What is the old behavior before changes and new behavior after changes?
  • Previously, the DN list of the old vs new certificates was compared as it was read, while this PR ensures both lists are sorted prior to the comparison

Issues Resolved

#4480

Testing

manual testing - as it is a pretty straightforward change.
happy to add a unit test if needed

Check List

• New functionality includes testing
• New functionality has been documented
• New Roles/Permissions have a corresponding security dashboards plugin PR
• API changes companion pull request created
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[willyborankin]

@Mehdi-Bendriss, thank you for your contribution. Unfortunately, it won't fix the problem. The method where you added the comparison is used only in the REST API. To address the bug, you need to sort the DN names in the method at:

security/src/main/java/org/opensearch/security/ssl/DefaultSecurityKeyStore.java

Line 666 in ef19743

private boolean hasValidDNs(final X509Certificate[] currentX509Certs, final X509Certificate[] newX509Certs) {

method.

[Mehdi-Bendriss]

@willyborankin thanks for the response.

I am not sure I'm following. The sorting in my PR happens in getSubjectAlternativeNames which is then called from within hasValidDNs (the method you linked) for both old and new certificates.

Is my understanding correct or did I miss something?

[willyborankin]

@willyborankin thanks for the response.

I am not sure I'm following. The sorting in my PR happens in getSubjectAlternativeNames which is then called from within hasValidDNs for both the old and new certificates.

Is my understanding correct or did I miss something?

Ahh sorry didn't notice Function<> in the method. All good my comment is misleading.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.27%. Comparing base (ef19743) to head (e188dfc).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4624      +/-   ##
==========================================
+ Coverage   65.24%   65.27%   +0.03%     
==========================================
  Files         317      317              
  Lines       22309    22311       +2     
  Branches     3588     3588              
==========================================
+ Hits        14555    14563       +8     
+ Misses       5960     5954       -6     
  Partials     1794     1794              

Files	Coverage Δ
...ensearch/security/ssl/DefaultSecurityKeyStore.java	66.66% <100.00%> (+0.12%)	⬆️

... and 3 files with indirect coverage changes

[willyborankin]

LGTM. One last thing you need to sign off your commits using git:
git commit --signoff

[Mehdi-Bendriss]

@willyborankin I signed off all commits, the DCO step still fails. Any idea?

[cwperks]

@willyborankin I signed off all commits, the DCO step still fails. Any idea?

I manually set it to pass. All of the commits are signed.