Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use BC libraries to parse PEM files, increase key length, allow gener… #17393

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

beanuwave
Copy link

NOTE: Basically a split up from the original PR

Description

This PR should provide a smoother transition to FIPS 140 standard by eliminating deprecated code, increase security standards and use more standardized approach to parse key files.

  • Refactors PemUtils to parse private keys in formats EC, PKCS8, PKCS1, and DSA, with or without encryption, and with or without parameters.
  • Elevate exclusion for cryptographic binaries from sub-modules to project level.
  • Increase key size for google cloud storage.
  • Increased security standards in KeyTabs and algorithms for Kerberos.
  • Remove unused BC libraries from ingest-attachment plugin.
  • Use KeyStoreUtils to create a keystore at runtime with default certificates to be used in tests.

Reasons for refactoring PemUtils, which is used by the Reindex API in cases of migrating data from a remote cluster that is TLS protected:
Lack of support for evolving standards like PKCS#8.
Password-Based Key Derivation Functions such as PBKDF-OPENSSL are not supported in FIPS mode in favor of the PBKDF2 standard.
Java type safety.
It is generally a good idea to let ASN1 annotation parsing be done by external security libraries.

Related Issues

opensearch-project/security#3420

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

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.

Copy link
Contributor

❌ Gradle check result for 62d1786: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@beanuwave beanuwave force-pushed the reevaluate_security_settings branch from 62d1786 to 99b1e83 Compare February 19, 2025 14:24
Copy link
Contributor

❌ Gradle check result for 99b1e83: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@beanuwave beanuwave force-pushed the reevaluate_security_settings branch from 99b1e83 to ac4fa36 Compare February 19, 2025 14:53
Copy link
Contributor

❌ Gradle check result for ac4fa36: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@beanuwave beanuwave force-pushed the reevaluate_security_settings branch from ac4fa36 to 7a837ea Compare February 19, 2025 15:26
Copy link
Contributor

❌ Gradle check result for 7a837ea: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

…al use of known cryptographic binary extensions, remove unused BC dependencies

Signed-off-by: Iwan Igonin <[email protected]>
@beanuwave beanuwave force-pushed the reevaluate_security_settings branch from 7a837ea to bd18542 Compare February 19, 2025 16:17
Copy link
Contributor

✅ Gradle check result for bd18542: SUCCESS

Copy link

codecov bot commented Feb 19, 2025

Codecov Report

Attention: Patch coverage is 84.48276% with 9 lines in your changes missing coverage. Please review.

Project coverage is 72.32%. Comparing base (91a93da) to head (bd18542).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...opensearch/gradle/testclusters/OpenSearchNode.java 0.00% 5 Missing ⚠️
.../main/java/org/opensearch/common/ssl/PemUtils.java 93.02% 2 Missing and 1 partial ⚠️
.../opensearch/common/ssl/SslConfigurationLoader.java 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #17393      +/-   ##
============================================
- Coverage     72.38%   72.32%   -0.06%     
+ Complexity    65516    65480      -36     
============================================
  Files          5291     5291              
  Lines        304319   304065     -254     
  Branches      44176    44126      -50     
============================================
- Hits         220269   219914     -355     
- Misses        65964    66192     +228     
+ Partials      18086    17959     -127     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@cwperks cwperks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for splitting up the PR @beanuwave. I think this is much easier to review at half the number of files for the first PR. @reta when you have some time could you also take a look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants