Remove hardcoded provider #4588
Conversation
Signed-off-by: Terry Quigley <[email protected]>
Signed-off-by: Terry Quigley <[email protected]>
terryquigleysas
requested review from
cliu123,
cwperks,
DarshitChanpura,
peternied,
RyanL1997,
stephen-crawford,
reta and
willyborankin
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4588 +/- ##
==========================================
- Coverage 65.28% 65.28% -0.01%
==========================================
Files 317 317
Lines 22277 22293 +16
Branches 3582 3583 +1
==========================================
+ Hits 14544 14554 +10
- Misses 5941 5946 +5
- Partials 1792 1793 +1
|
|
Hi @terryquigleysas this looks reasonable and is obviously a simple change code-wise but it may have lots of impact which I am not sure how to quantify. Before I approve this change, could you please add a short explanation as to why this change is required? Outside of the code maybe not being needed as mentioned in the issue you linked, what is the purpose of this change? How does removing the default BC provider enable the FIPs changes you are working towards and if we remove it how can we be certain that removing the default provider won't break anything else especially considering that some of the other settings given allow-read access by the policy file are BC related? Thanks a bunch :) |
plugin-security.policy
Outdated
| permission java.security.SecurityPermission "putProviderProperty.BC"; | ||
| permission java.security.SecurityPermission "insertProvider.BC"; | ||
| permission java.security.SecurityPermission "removeProviderProperty.BC"; |
There was a problem hiding this comment.
How would this affect non-FIPS scenarios (i.e. current behavior?)
| final SecurityManager sm = System.getSecurityManager(); | ||
|
|
||
| if (sm != null) { | ||
| sm.checkPermission(new SpecialPermission()); | ||
| } |
There was a problem hiding this comment.
What was this used for? What does the special permission check do and why is it no longer needed?
There was a problem hiding this comment.
@derek-ho I think this boilerplate code setup for doPrivileged calls originates from the Elasticsearch recommendations here https://www.elastic.co/guide/en/elasticsearch/plugins/current/creating-classic-plugins.html#plugin-authors-jsm
@scrawfor99 Sure, fair questions. As you say, the code changes appear minor but may have implications. This is part of a move to remove or replace any code that is either not FIPS-compliant or will not work if Bouncy Castle FIPS-approved jars are swapped in to replace their regular jars. In this case the BouncyCastleProvider class does not exist in the FIPS libraries so this code would fail. In the POC code I brought in the FIPS libraries and was able to use the BouncyCastleFipsProvider equivalent. See 2.11...terryquigleysas:security:2.11#diff-e7ef66295cba81a49e4349781a2e9678d2b021a570e978bb4feb422b03d5d74aR334 . We cannot do this here as a) the 2 Bouncy Castle libraries cannot coexist on the same classpath b) using the FIPS libraries here would break other things, e.g. OpenSAML. The cleanest way would be to remove this call entirely as has been suggested before. It was believed to be required for Blake2b and certificate handling but possibly for legacy reasons. I have tested the Blake2b code and the demo certificates against a local JDK 17, the bundled JDK 21 and run the Bulk Integration Tests. I too was concerned that there may be other test paths or scenarios where this should not be removed and that was where I would defer to the more in-depth knowledge you guys have of the code in order to be certain it will have no adverse side-effects. There may be other approaches we can try but if we can safely remove this call it is by far the cleanest option. |
| if (Security.getProvider("BC") == null) { | ||
| Security.addProvider(new BouncyCastleProvider()); | ||
| } |
There was a problem hiding this comment.
Can you point me to the code that would be doing the instantiation instead of this block right here?
There was a problem hiding this comment.
This would now be handled by the JDK setup itself which I believe is preferred. I have tested the Blake2b code and the demo certificates against a local JDK 17, the bundled JDK 21 and run the Bulk Integration Tests.
Otherwise an alternative could be to hardcode one or both , e.g like 2.11...terryquigleysas:security:2.11#diff-e7ef66295cba81a49e4349781a2e9678d2b021a570e978bb4feb422b03d5d74aR334
For FIPS we can reconfigure the Java environment, e.g. #3420 (comment)
|
If I remember correctly, the BouncyCastleProvider provides the default list of CIPHERS that are acceptable: https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/ssl/util/SSLConfigConstants.java#L122-L234. Need to double check though. I'm surprised that this change did not cause any tests to fail. |
|
X-Posting this comment here as well. @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? 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 Thank you for the code suggestion. That sounds like a sensible approach. While I have yet to see from my testing why the call is necessary I am wary that it's difficult in this case to say with 100% certainty what all the side effects will be. I will look to adding this more defensive approach. A FIPS distribution is something that I have considered but my lack of familiarity with the OpenSearch build process has made it difficult for me to gauge how that would work and whether it would be feasible. There are several things to cover here, the first that spring to mind being:
I used a slightly different policy file in the POC 2.11...terryquigleysas:security:2.11#diff-deaa12e077af166f8bbdea4f1784c2dc043f05292ac7313a857ab1542fe2405aR59 so yes, that is an option if required. |
|
@peterzhuamazon Would you provide some insight into the OpenSearch build process? See comment above: #4588 (comment)
|
Hi, OpenSearch bundle rpm distribution can already run on FIPS enabled server (opensearch-project/opensearch-build#2099). As for whether we would have a separate FIPS distribution, I am not sure what is the issue of not able to use current distribution on FIPS enabled server. If needed, you can open an issue in opensearch-build repo and we can discuss whether it is something we can probably add as official artifact, and in which types of distribution. This could involve a big amount of changes because all of our current logic as well as folder structures of our artifacts are under the assumption of platform/arch/dist leveling. Adding FIPS is definitely an extra factor that we are not sure how much impact involved until further research. Thanks. |
Added support for Bouncy Castle FIPS provider. Signed-off-by: Terry Quigley <[email protected]>
|
@cwperks I have pushed another commit based on your suggestion above. Thanks. |
|
@terryquigleysas The new changes look good to me. Thanks! How much effort do you think it would take to write a simple test? i.e. If BCFIPS is provided on the classpath, ensure that the security plugin adds it as a security provider? |
|
The other question I have is on the permissions in |
@cwperks As the other work progresses to support FIPS we should aim to separate FIPS/non-FIPS policies more. As it stands I felt including both here offered the ability to support adding both providers at little or no risk. |
@cwperks I have been testing against a local deployment. I had to add another property to the policy file but it works as expected there. I wouldn't be sure exactly which approach to use. Is there an example of something similar in the current test code? That is something else that may become clearer as the other work progresses. I would actually expect us to set up the security file for the JDK/JVM to use Bouncy Castle FIPS provider but thought for completeness I could add the FIPS variant here too in much the same way as current done. I can remove the BCFIPS code and put it in as a separate PR is that is preferable. What do you think? |
Signed-off-by: Terry Quigley <[email protected]>
…ryquigleysas/security into remove_hardcoded_bc_provider
Signed-off-by: Terry Quigley <[email protected]>
Signed-off-by: Terry Quigley <[email protected]>
Signed-off-by: Terry Quigley <[email protected]>
|
@terryquigleysas I think its possible to use an existing integration test and assert that To test, I added a few lines to the end of InitializationIntegrationTest.testDefaultConfig to assert that the provider is not null. i.e. A similar check should work for BCFIPS if you provide the jar as a test dependency. If you comment out the code to add the provider the test should fail. |
|
Superseded by and referenced in #4605 |
Description
Category (Refactoring)
Remove call to add the BouncyCastleProvider in OpenSearchSecurityPlugin.java and any associated code.
This may have several benefits:
Issues Resolved
Resolves #4583
Testing
Bulk Integration Test Github action
Run latest 3.x Core and OpenSearch Security plugin. Smoke tests (Indexing, searching, user creation, role creation, security admin script calls) run against both:
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.