opensearch-project
Support for FIPS compliance mode
Description
- FIPS gradle build script is removed.
- All BC dependencies are replaces by BCFIPS.
- Password matcher inside Identity-Shiro that replies on BC to check if hashed passwords matches with OpenBSDBCrypt, is replaced by password4j implementation.
- Adds support for BCFKS format (*.bks) for Key & Truststores.
- Refactor parsing private keys with formats EC, PKCS8, PKCS1, DSA, w/wo encryption, w/wo parameters.
- FIPS approved-only mode can be configured over opensearch.yml file.
- java security file is added to the build.
- java policy file is altered to grant neccessary security permissions.
This PR provides FIPS 140-2 support by replacing all BC dependencies with BCFIPS dependencies and making FIPS approved-only mode configurable at launch. Running application in approved-only mode restricts BCFIPS provoder to rely solely on FIPS certified cyphers. Due to replacement of BC libraries, BCrypt password matching and private-key loading from file were replaced by alternative implementations.
Reasons for refactoring PemUtils.java that is used by Reindex API, in case of migrating data from a remote cluster that is TLS protected:
- PKCS#8 implementation was not supported by BCFIPS library.
- java type security.
- Password Based Key Derivation Functions such as PKCS#12 and OpenSSL are not supported in BCFIPS approved-only mode, because only PBKDF2 standard is approved for use in FIPS.
- generally good idea to let ASN1 annotation parsing be done by external security libraries.
Related Issues
https://github.com/opensearch-project/security/issues/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.
:x: Gradle check result for 6016d5d67b5e4cc71f91f5b8b2a0ce3fabe03455: 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?
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
:x: Gradle check result for 8e8ed47baee72d493dfab0033da080e5b759b222: 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?
:x: Gradle check result for 6016d5d67b5e4cc71f91f5b8b2a0ce3fabe03455: 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?
Could use some help maybe from @cwperks or @peternied reviewing this, please.
Thanks for the PR @beanuwave. As others have mentioned. Would it be possible to add tests for the different scenarios this PR covers such running in FIPS-enabled mode but providing jars on the classpath that aren't FIPS compliant?
How does this PR affect backward compatibility?
@beanuwave @terryquigleysas Thinking about backwards compatibility, would there be an opportunity to pass a flag at build-time to create a distribution that is FIPS-compliant? Would it make sense to have multiple distributions? 1 with bouncycastle (BC) included and 1 without BC?
Looking at the companion PR in the security plugin, I was wondering if it made sense to add logic to check if the BouncyCastleProvider class was on the classpath. If it is on the classpath then can it add the bouncycastle provider? I've written some similar code before in netty: https://github.com/netty/netty/blob/4.1/handler/src/main/java/io/netty/handler/ssl/BouncyCastlePemReader.java#L72-L94
Similar to the provider, is it possible to have different plugin-security.policy files depending on whether bouncycastle is provided or not?
@cwperks @peternied @terryquigleysas @scrawfor99 @dancristiancecoi Thanks, everyone, for your keen interest in reviewing this change. I should mention that this PR is still very much a WIP. My goal is to get initial feedback to ensure I'm heading in the right direction. Currently, I'm focusing on test coverage and plugin support, as their absence cannot be overlooked :)
As previously noted, the removal of fips.gradle would be a breaking change for the 2.x.x releases. Would it be beneficial to release FIPS compliance for both the core and companion security plugin simultaneously?
From my perspective, the fips.gradle script swaps policy and security files and replaces JSSE and JCA/JCE BC dependencies with their FIPS counterparts. If we agree that BC dependencies are not mandatory, there may be no need to exchange them at all. Policy and security files can be added during build-time or runtime. However, since BCFIPS allows runtime configurations of approvedOnly mode, it is a valuable feature to have it configurable. It’s worth mentioning that there might be issues with some plugins like Ingest or Azure, which will hopefully become apparent during testing.
Additionally, it should be noted that BCFIPS without the approvedOnly=true setting is ineffective for FIPS compliance because it allows ciphers such as DES to be instantiated. During testing, I discovered that the approvedOnly mode significantly reduces the number of allowed ciphers, also depending on whether the underlying OS is running in FIPS mode or not.
@beanuwave I've marked this pull request as 'draft' in GitHub, feel free to keep iterating and let us know if there is anything you'd like attention on. I'll keep and eye out, but I'll give it a more detailed look when you've marked this change as Ready for review.
@beanuwave @terryquigleysas Thinking about backwards compatibility, would there be an opportunity to pass a flag at build-time to create a distribution that is FIPS-compliant? Would it make sense to have multiple distributions? 1 with bouncycastle (BC) included and 1 without BC?
Looking at the companion PR in the security plugin, I was wondering if it made sense to add logic to check if the
BouncyCastleProviderclass was on the classpath. If it is on the classpath then can it add the bouncycastle provider? I've written some similar code before in netty: https://github.com/netty/netty/blob/4.1/handler/src/main/java/io/netty/handler/ssl/BouncyCastlePemReader.java#L72-L94Similar to the provider, is it possible to have different
plugin-security.policyfiles depending on whether bouncycastle is provided or not?
I have replied at https://github.com/opensearch-project/security/pull/4588#issuecomment-2250617277
:x: Gradle check result for 84e7aa9cc4725f0515ca14f366fdd45a21ec8c72: 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?
All tests are now successfully running without requiring the BC libraries, by leveraging only the SUN and BCFIPS providers. This demonstrates that including BC libraries in the build process is not essential. The next step is to identify which code cannot operate under FIPS approved-only mode and address these limitations. Notably, the following components have constraints under FIPS compliance (as detailed here):
- Azure Classic Discovery Plugin: Utilizes JKS keystore for certificates.
- Ingest Attachment Plugin.
- KeyStoreWrapper: Handles both v1 and v2 of the internal keystore.
- General usage of PKCS#12 and PBE standards.
- SQL CLI Plugin.
Plugin support remains an uncertain area. However, as long as plugins do not interfere with the SecurityManager and permissions, there should be minimal concern.
:x: Gradle check result for 219e9dc942323722c60c267733e1422d29306133: 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?
Plugin support remains an uncertain area. However, as long as plugins do not interfere with the SecurityManager and permissions, there should be minimal concern.
There can be instances when other plugins interfere with the SecurityManager and permissions as described in this comment.
:x: Gradle check result for dc753b0d5b942ca8390bd6456ad4f6c811c93aab: 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?
:x: Gradle check result for fd825eb9ffd3c35c5ecb6214e5cb774269bd21f5: 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?
:x: Gradle check result for 9bb159115c79afc858f9f4f3b543883b378a0323: 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?
:grey_exclamation: Gradle check result for 5ab8df863f75744d6b97b41e29ca11f5cb9f08da: UNSTABLE
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.
Codecov Report
Attention: Patch coverage is 72.41379% with 56 lines in your changes missing coverage. Please review.
Project coverage is 72.29%. Comparing base (
6fb0c1b) to head (6a14fdc). Report is 4 commits behind head on main.
Additional details and impacted files
@@ Coverage Diff @@
## main #14912 +/- ##
============================================
+ Coverage 72.24% 72.29% +0.04%
+ Complexity 65704 65672 -32
============================================
Files 5318 5323 +5
Lines 305674 305536 -138
Branches 44349 44315 -34
============================================
+ Hits 220834 220880 +46
+ Misses 66769 66579 -190
- Partials 18071 18077 +6
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
:x: Gradle check result for 8e5237f6e013fc63c06432931ec7fa40f6e3de0e: 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?
:x: Gradle check result for 7e202a2a67de7e948d332be3bf6a6de7ab602061: 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?
:x: Gradle check result for bbbafa9fdce21e38c72600f6462f9271a1d57782: 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?
Previously, my main focus was on running the entire test suite without the BC non-FIPS libraries, which was successfully achieved. Now, the latest changes affect FIPS compatibility by running all tests again with the additional VM parameter -Dtests.fips.enabled=true. This has resulted in several code modifications:
- Introduced an additional test parameter:
tests.fips.enabled. - Added support for the
.bcfipskeystore file format. - Keystores now require strict type matching.
- Recreated some cryptographic keys, as they are no longer maintainable with newer versions of
keytoolandOpenSSL. - Keystores without a passphrase are no longer supported because BC FIPS requires passwords with at least 112 bits strength.
Currently, the AWS S3 plugin is the remaining component that needs adjustment for FIPS mode. However, applying FIPS mode to every security-relevant thread using CryptoServicesRegistrar#setApprovedOnlyMode seems unsustainable. Setting the VM parameter org.bouncycastle.fips.approved_only=true accomplishes the same goal for the entire application and eliminates the need for FIPS-specific code snippets in core classes like AbstractRunnable or ThreadContext.
Another topic to consider is how to extend the Jenkins pipeline to run tasks like gradle check in FIPS mode. Should we implement an additional downstream process that runs on every build, even if it might almost double the execution time? Or is a separate pipeline which runs at the higher stages and can be manually triggered the way to go?
@beanuwave Thanks for making these updates - I'll need a little time to digest them. While I do so could you please address all unresolved comment threads and look into the GitHub action failures?
Directionally this is looking solid, I've got some background questions to make sure I understand some of the tradeoffs.
Definitely, can we set up a direct call? I've contacted you on slack to find a suitable timeslot.
Directionally this is looking solid, I've got some background questions to make sure I understand some of the tradeoffs.
Definitely, can we set up a direct call? I've contacted you on slack to find a suitable timeslot.
Sorry I might have been unclear, I've put those questions in comments on this PR, I don't need to schedule a call, please review the unresolved comment threads for the open questions I have, one for example https://github.com/opensearch-project/OpenSearch/pull/14912#discussion_r1690076510
:x: Gradle check result for 82280374d192382eb86d3c943c7e3adfbfe94072: 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?
:grey_exclamation: Gradle check result for a63fe98ec493253308ceb735f91c0cbd19f364c4: UNSTABLE
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.
In the latest commit, the JVM parameter fips.approved=true was replaced with the environment variable OPENSEARCH_CRYPTO_STANDARD=FIPS-140-2. Additionally, the thread-based solution for enabling FIPS mode was substituted with an extra JVM parameter, as this approach appears simpler to achieve and maintain. Furthermore, I am working on enhancing code test coverage and separating the java.security files, which require stricter regulation on permitted cryptographic algorithms.
Additionally, I am considering revisiting the FIPS mode limitations imposed by ElasticSearch, as the current tests do not seem to reflect them. Creating additional documentation and setting up FIPS-enabled Jenkins build workflows will certainly be tasks to undertake.
:x: Gradle check result for 3637ccf50c0a75c6cab404e057c5dd9dc44703cc: 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?