-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Support for FIPS compliance mode #14912
base: main
Are you sure you want to change the base?
Support for FIPS compliance mode #14912
Conversation
❌ Gradle check result for 6016d5d: 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? |
8e8ed47
to
6016d5d
Compare
❌ Gradle check result for 8e8ed47: 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? |
❌ Gradle check result for 6016d5d: 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? |
.../identity-shiro/src/main/java/org/opensearch/identity/shiro/realm/BCryptPasswordMatcher.java
Outdated
Show resolved
Hide resolved
Could use some help maybe from @cwperks or @peternied reviewing this, please. |
buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchNode.java
Outdated
Show resolved
Hide resolved
@reta I totally get your point. But I also see a downside of your proposal. Splitting up this PR will create considerable effort and time to do so. This will inevitably prolong the time the changes remain unmerged. I am not sure, it will be faster creating separate PRs out of this one. It might also create some confusion due to the immense number of comments already discussed in this PR's reviews. Given the nature of this PR, there is quite some potential for conflicts with the upcoming SecurityManager disablement/removal. Or am I judging this wrongly? We would really like to see this effort coming to a close with the PR being merged. In the end, we will handle this change by the recommendation of this project's maintainers. If you tell us to split, we will do it. Please let us know, what way you want to pursue. |
@KarstenSchnitter thank you, the whole intent of the suggestion is to get this change reviewed, approved and merged. I am (personally) not comfortable with some of these (fips deps bundled / leaked with some modules, tons of conditionals still present in some tests, ...) and I thought going with small steps would allow us to tackle the scoped problems one by one (plus cutting the large 200 files change into smaller ones would certainly help). But I am totally fine if other maintainers see it not being necessary. |
What is breaking with the inclusion of the FIPS deps even if not running in FIPS-140-3 compliance mode? I know there's been a lot of due diligence already done with this PR so I'd like to see it merged in and any follow-up items identified and completed with sufficient headway before 3.0.0 release. I see that the gradle check has been failing, do you need any help in identifying issues in the failing test suite? |
@iigonin @beanuwave @KarstenSchnitter I appreciate the position you are in - thank you work working with us, this is a big feature and its only gotten bigger as we've got into the details. I think this grow is also causing turn arounds to be slower - I know it has with my involvement. With smaller increments will make it easier to get more maintainer engagement and codify decisions that don't need to be revisited. While breaking the changes apart will take more time in energy the trade-off is better confidence for the project. Cherry-picking from components I've reviewed in the past some candidates could be such refactoring around KeyStoreFactory or RestClientFipsAwareTestCase that provide a better foundation for the OpenSearch and work well in isolation.
100% and I would hate to lose that context, I think this PR should be preserved at least for the great dialogs and decision making. One way this could be managed is by making new PRs and creating links to relevant comments in advance of requesting the review - that creates continuity. Another thought, this PR's size will winnows down over time as incremental progress is merged in from main. I know this has been a huge undertaking and we are converging - thanks for working with all of us. |
@reta I would also appreciate multiple smaller commits so that they are easier to review. The reason I found it difficult with the modular approach is that the project already had BC dependencies in multiple modules. Unfortunately, the process of replacing old BC with FIPS counterparts cannot be split because they cannot coexist on the classpath at the same time. @cwperks The build fails on |
Thank you @beanuwave, I think we could clearly start with build tools and removing deprecated / not used settings (as I suggested) - it does not have any dependencies on FIPS but will shave off 20-30 files immediately. The other pieces will follow, at least I don't see the blockers there, but certainly some work. |
I once encountered a similar thread leak failure on #16778, but not sure how to resolve the failure seen in |
Signed-off-by: Iwan Igonin <iigonin@sternad.de> # Conflicts: # server/build.gradle # Conflicts: # server/build.gradle # Conflicts: # server/build.gradle
Signed-off-by: Iwan Igonin <iigonin@sternad.de>
Signed-off-by: Iwan Igonin <iigonin@sternad.de> # Conflicts: # buildSrc/version.properties
Signed-off-by: Iwan Igonin <iigonin@sternad.de>
Signed-off-by: Iwan Igonin <iigonin@sternad.de>
…ional tests. Signed-off-by: Iwan Igonin <iigonin@sternad.de>
Signed-off-by: Iwan Igonin <iigonin@sternad.de>
Signed-off-by: Iwan Igonin <iigonin@sternad.de>
Signed-off-by: Iwan Igonin <iigonin@sternad.de>
Signed-off-by: Iwan Igonin <iigonin@sternad.de>
Signed-off-by: Iwan Igonin <iigonin@sternad.de>
…Pattern Signed-off-by: Iwan Igonin <iigonin@sternad.de> # Conflicts: # CHANGELOG-3.0.md
Summery: - replace unsecure kerberos crypto algorithms - add 'java.security.KeyStore' to forbidden-apis - instantiate and use SecureRandom from BCFIPS library - exclude SunJCE from security providers list at runtime, when running in FIPS JVM - exclude Azure tests when running in FIPS JVM Signed-off-by: Iwan Igonin <iigonin@sternad.de>
Signed-off-by: Iwan Igonin <iigonin@sternad.de>
Signed-off-by: Iwan Igonin <iigonin@sternad.de>
Signed-off-by: Iwan Igonin <iigonin@sternad.de>
Signed-off-by: Iwan Igonin <iigonin@sternad.de>
Signed-off-by: Iwan Igonin <iigonin@sternad.de>
f88d5a5
to
96524cf
Compare
❌ Gradle check result for 96524cf: 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? |
Description
This PR makes FIPS mode available through the
OPENSEARCH_CRYPTO_STANDARD=FIPS-140-3
environmental parameter instead of thetests.fips.enabled
setting. It provides FIPS 140-3 support by replacing all BC dependencies with BCFIPS dependencies and making FIPS approved-only mode configurable at launch. Running this mode restricts the BCFIPS provider to rely solely on FIPS-certified ciphers.fips.gradle
build script is removed in order to support a single-build solution.java.security
file is added to the build to distinguish between FIPS and non-FIPS environments.fips_java.security
file is altered due to evolving security standards.security.policy
file is altered to grant necessary security permissions.Runtime limitations (known so far) that come with enabling FIPS mode:
Admins can continue to manage their systems without being impacted by this change. However, for those keen on FIPS compliance, the most common obstacle will likely be the requirement to set a stronger password for the internal keystore and also convert key and truststores to *.bcfks format.
ssl.verification_mode=NONE
setting is not permitted.Reasons for refactoring
PemUtils
, which is used by the Reindex API in cases of migrating data from a remote cluster that is TLS protected:Related Issues
opensearch-project/security#3420
Check List
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.