Refactor SSL Configuration

[willyborankin]

Description

Refactor DefaultKeyStore Changes:

• Refactored DefaultKeyStore into specialized subclasses, each managing a distinct responsibility.

• Added missing tests for certificate loading, SSL parameter configuration, and related processes.

Key differences compared to the existing solution:

• Netty for PEM Certificates: Netty is now used to load PEM certificates and private keys, which allows to use FIPS instead of a BC-based solution. In the future, we can switch to JDK 21, which provides much better support for loading from PEM files compared to previous versions.

• JDK Key/Trust Store Handling: The SSL settings loader now reads all authority certificates and private keys from the key/trust store, rather than just the first encountered alias/private key.

• Certificate Date Validation: For validating notAfter and notBefore dates on certificates, we switched to the built-in JDK function, which only validates date ranges for key material certificates.

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.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 72.19321% with 213 lines in your changes missing coverage. Please review.

Project coverage is 69.85%. Comparing base (811f26d) to head (6aed562). Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...rg/opensearch/security/ssl/SslSettingsManager.java	70.66%	17 Missing and 27 partials :warning:
.../opensearch/security/ssl/config/KeyStoreUtils.java	56.32%	30 Missing and 8 partials :warning:
.../opensearch/security/ssl/config/SslParameters.java	54.41%	26 Missing and 5 partials :warning:
...rg/opensearch/security/ssl/config/Certificate.java	58.33%	15 Missing and 15 partials :warning:
...rch/security/ssl/config/KeyStoreConfiguration.java	64.78%	21 Missing and 4 partials :warning:
...h/security/ssl/config/TrustStoreConfiguration.java	63.15%	16 Missing and 5 partials :warning:
.../org/opensearch/security/ssl/SslConfiguration.java	78.00%	8 Missing and 3 partials :warning:
...rch/security/ssl/config/SslCertificatesLoader.java	88.67%	3 Missing and 3 partials :warning:
...urity/dlic/rest/api/SecuritySSLCertsApiAction.java	92.59%	0 Missing and 2 partials :warning:
...earch/security/ssl/rest/SecuritySSLInfoAction.java	90.00%	1 Missing and 1 partial :warning:
... and 3 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4671      +/-   ##
==========================================
- Coverage   70.90%   69.85%   -1.06%     
==========================================
  Files         310      320      +10     
  Lines       20950    21660     +710     
  Branches     3331     3456     +125     
==========================================
+ Hits        14855    15131     +276     
- Misses       4341     4736     +395     
- Partials     1754     1793      +39     

Files with missing lines	Coverage Δ
.../opensearch/security/OpenSearchSecurityPlugin.java	85.09% <100.00%> (+0.11%)	:arrow_up:
...security/dlic/rest/api/SecurityRestApiActions.java	80.00% <ø> (ø)
.../security/ssl/OpenSearchSecureSettingsFactory.java	81.25% <100.00%> (ø)
...arch/security/ssl/OpenSearchSecuritySSLPlugin.java	87.02% <100.00%> (+0.72%)	:arrow_up:
...org/opensearch/security/ssl/SecureSSLSettings.java	100.00% <ø> (ø)
...a/org/opensearch/security/ssl/config/CertType.java	100.00% <100.00%> (ø)
.../api/ssl/TransportCertificatesInfoNodesAction.java	89.74% <92.30%> (+10.79%)	:arrow_up:
...org/opensearch/security/ssl/SslContextHandler.java	98.59% <98.59%> (ø)
...ensearch/security/ssl/util/SSLConfigConstants.java	79.16% <90.00%> (+1.38%)	:arrow_up:
...urity/dlic/rest/api/SecuritySSLCertsApiAction.java	83.78% <92.59%> (+5.52%)	:arrow_up:
... and 9 more

... and 11 files with indirect coverage changes

[willyborankin]

Hi @terryquigleysas and @dancristiancecoi, could you please review this solution as well?

[terryquigleysas]

Hi @terryquigleysas and @dancristiancecoi, could you please review this solution as well?

Hi @willyborankin This looks good. I do have one question. Can I ask for more in information on your thinking around using Netty "to use FIPS instead of a BC-based solution" ? As a BC-based solution could use FIPS-approved libraries for those operations how is that achieved by using Netty?

[terryquigleysas]

Hi @terryquigleysas and @dancristiancecoi, could you please review this solution as well?

Hi @willyborankin This looks good. I do have one question. Can I ask for more in information on your thinking around using Netty "to use FIPS instead of a BC-based solution" ? As a BC-based solution could use FIPS-approved libraries for those operations how is that achieved by using Netty?

@willyborankin @cwperks Thank you for going over this a bit further with me. Does this mean that, in order to support FIPS here, that a change may be required in Netty to also attempt to load in the BouncyCastleFipsProvider as it does for the non-FIPS provider here https://github.com/netty/netty/blob/4.1/handler/src/main/java/io/netty/handler/ssl/BouncyCastlePemReader.java#L72-L94 ?

[willyborankin]

Hi @terryquigleysas and @dancristiancecoi, could you please review this solution as well?

Hi @willyborankin This looks good. I do have one question. Can I ask for more in information on your thinking around using Netty "to use FIPS instead of a BC-based solution" ? As a BC-based solution could use FIPS-approved libraries for those operations how is that achieved by using Netty?

@willyborankin @cwperks Thank you for going over this a bit further with me. Does this mean that, in order to support FIPS here, that a change may be required in Netty to also attempt to load in the BouncyCastleFipsProvider as it does for the non-FIPS provider here https://github.com/netty/netty/blob/4.1/handler/src/main/java/io/netty/handler/ssl/BouncyCastlePemReader.java#L72-L94 ?

Hi @terryquigleysas, this is not related to FIPS. It pertains to parsing and reading the PKCS1 PEM format, which was not supported prior to JDK 17 (https://bugs.openjdk.org/browse/JDK-8258394) and was backported to JDK1.8 and JDK11. As far as I understand from the FIPS specification, you only need to configure the recommended SSL ciphers provided by the spec, and that should suffice (please correct me if I'm wrong).

[terryquigleysas]

Hi @terryquigleysas and @dancristiancecoi, could you please review this solution as well?

Hi @willyborankin This looks good. I do have one question. Can I ask for more in information on your thinking around using Netty "to use FIPS instead of a BC-based solution" ? As a BC-based solution could use FIPS-approved libraries for those operations how is that achieved by using Netty?

@willyborankin @cwperks Thank you for going over this a bit further with me. Does this mean that, in order to support FIPS here, that a change may be required in Netty to also attempt to load in the BouncyCastleFipsProvider as it does for the non-FIPS provider here https://github.com/netty/netty/blob/4.1/handler/src/main/java/io/netty/handler/ssl/BouncyCastlePemReader.java#L72-L94 ?

Hi @terryquigleysas, this is not related to FIPS. It pertains to parsing and reading the PKCS1 PEM format, which was not supported prior to JDK 17 (https://bugs.openjdk.org/browse/JDK-8258394) and was backported to JDK1.8 and JDK11. As far as I understand from the FIPS specification, you only need to configure the recommended SSL ciphers provided by the spec, and that should suffice (please correct me if I'm wrong).

@willyborankin Thank you for the additional explanation.

[willyborankin]

@DarshitChanpura could you plz approve it one more time? I fixed conflicts

[opensearch-trigger-bot[bot]]

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/security/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/security/backport-2.x
# Create a new branch
git switch --create backport/backport-4671-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 db6e7dc1664694497d80ee0b8b727350c1086b47
# Push it to GitHub
git push --set-upstream origin backport/backport-4671-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/security/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-4671-to-2.x.

[cwperks]

@willyborankin can you open a manual backport for this PR?