Add bouncycastle permissions to security.policy #9770

cwperks · 2023-09-05T16:55:54Z

Description

Adds bouncycastle permissions into core's security.policy file. With the introduction of opensearch-encryption-sdk, the bouncycastle provider (bcprovp-jdk15to18) is now a dependency of core. The security plugin also has a direct dependency on the same artifact and has permissions in its plugin-security.policy file to grant the security plugin permission to add the bouncy castle provider.

There is an issue bringing up a node with the security plugin installed and TLS enabled because the permissions are not present in bootstrap/security.policy and the dependency has now been moved to core

Related Issues

Resolves opensearch-project/security#3309

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

Signed-off-by: Craig Perkins <[email protected]>

cwperks · 2023-09-05T16:56:39Z

@peternied @reta @Bukhtawar @gbbafna @vikasvb90

Signed-off-by: Craig Perkins <[email protected]>

cwperks · 2023-09-05T17:09:40Z

Tested by performing plugin install manually of the security plugin.

Make a local distribution of core: ./gradlew localDistro
cd build/distribution/local/opensearch-3.0.0-SNAPSHOT
Make a snapshot of the security plugin. From the root directory of the security repo run ./gradlew assemble
Plugin output is at <OPENSEARCH_SECURITY_HOME>/build/distributions/opensearch-security-3.0.0.0-SNAPSHOT.zip
Install the plugin. Example: ./bin/opensearch-plugin install file:/Users/cwperx/Projects/opensearch/security/build/distributions/opensearch-security-3.0.0.0-SNAPSHOT.zip
Run the install_demo_configuration.sh script and answer 'y' to all questions: ./plugins/opensearch-security/tools/install_demo_configuration.sh
Run OpenSearch node: ./bin/opensearch

There should be no errors on startup. The security plugin has an automated CI check for plugin install, but the check will not work until this PR is merged.

peternied

Is there a danger that this lowers the security posture of the SSL in such a way that any plugins can manipulate the SSL configuration, whereas previously this was isolated to plugins with this policy?

cwperks · 2023-09-05T17:21:47Z

Is there a danger that this lowers the security posture of the SSL in such a way that any plugins can manipulate the SSL configuration, whereas previously this was isolated to plugins with this policy?

Yes that's correct. Any plugin would be able to execute this same block as the security plugin: https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java#L334-L342

With the plugin-security.policy file, cluster admins installing plugins will accept a policy when installing a plugin. That is not the case with the policy file in this PR. The security.policy file in this PR is applicable to core and plugins.

RyanL1997 · 2023-09-05T17:28:11Z

server/src/main/resources/org/opensearch/bootstrap/security.policy

@@ -85,6 +85,12 @@ grant codeBase "${codebase.zstd-jni}" {
  permission java.lang.RuntimePermission "loadLibrary.*";
 };

+grant codeBase "${codebase.bcprov-jdk15to18}" {


Do we need this for jdk 11?

This is the way that bouncycastle releases artifacts. Until last month the securityplugin was using jdk15on, but then switched to jdk15to18 (PR where it was changed: opensearch-project/security#2901). I plan to open an issue about using jdk18on.

No, this is due to #9289, the BC has moved to core and now we need this permission to manage security providers.

github-actions · 2023-09-05T17:28:44Z

Compatibility status:

Checks if related components are compatible with change 8c85f27

Incompatible components

Incompatible components: [https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/anomaly-detection.git]

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/reporting.git]

github-actions · 2023-09-05T17:33:42Z

Compatibility status:

Checks if related components are compatible with change a3f35b2

Incompatible components

Incompatible components: [https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/anomaly-detection.git]

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/reporting.git]

github-actions · 2023-09-05T17:47:47Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/24476/
CommitID: 8c85f27

codecov · 2023-09-05T17:48:48Z

Codecov Report

Merging #9770 (a3f35b2) into main (19a13f0) will increase coverage by 0.04%.
Report is 4 commits behind head on main.
The diff coverage is n/a.

❗ Current head a3f35b2 differs from pull request most recent head 037efca. Consider uploading reports for the commit 037efca to get more accurate results

@@             Coverage Diff              @@
##               main    #9770      +/-   ##
============================================
+ Coverage     71.14%   71.19%   +0.04%     
+ Complexity    57973    57965       -8     
============================================
  Files          4825     4825              
  Lines        273348   273348              
  Branches      39841    39841              
============================================
+ Hits         194473   194598     +125     
+ Misses        62523    62371     -152     
- Partials      16352    16379      +27

see 470 files with indirect coverage changes

github-actions · 2023-09-05T17:49:49Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/24478/
CommitID: a3f35b2

Signed-off-by: Craig Perkins <[email protected]>

reta · 2023-09-05T18:12:03Z

Yes that's correct. Any plugin would be able to execute this same block as the security plugin: https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java#L334-L342

@cwperks I think this is a good moment to fix that, we know that plugins should not be adding security providers for a two reasons:

the order plugins are loaded makes it predictable (the first one will load and others will fail)
since this is in core now, we should let core load it (only libs/opensearch-encryption-sdk should do that)

willyborankin · 2023-09-05T18:31:35Z

Is there a danger that this lowers the security posture of the SSL in such a way that any plugins can manipulate the SSL configuration, whereas previously this was isolated to plugins with this policy?

Thats why it is a workaround for 2.10. But it needs to be addressed asap.

reta · 2023-09-05T18:36:09Z

Is there a danger that this lowers the security posture of the SSL in such a way that any plugins can manipulate the SSL configuration, whereas previously this was isolated to plugins with this policy?

Thats why it is a workaround for 2.10. But it needs to be addressed asap.

@peternied @cwperks @willyborankin the problem is more severe than that, the permissions in this pull request only grant the add/remove provider privileges, but we need much more, the plugin will fail down the line, for example I just added this small piece of code into security plugin initialization sequence:

       import org.bouncycastle.jcajce.provider.symmetric.util.GcmSpecUtil;
 
       ...
        if (GcmSpecUtil.gcmSpecExists()) {
            System.out.println("GcmSpec exists");
        }

It fails with java.security.AccessControlException: access denied ("java.lang.RuntimePermission" "accessDeclaredMembers") because, well, bcprov needs reflection access as well.

github-actions · 2023-09-05T18:40:43Z

Compatibility status:

Checks if related components are compatible with change 9e43830

Incompatible components

Incompatible components: [https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/neural-search.git]

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git]

Signed-off-by: Craig Perkins <[email protected]>

…earch into add-bc-permissions

willyborankin · 2023-09-05T18:41:46Z

Is there a danger that this lowers the security posture of the SSL in such a way that any plugins can manipulate the SSL configuration, whereas previously this was isolated to plugins with this policy?

Thats why it is a workaround for 2.10. But it needs to be addressed asap.

@peternied @cwperks @willyborankin the problem is more severe than that, the permissions in this pull request only grant the add/remove provider privileges, but we need much more, the plugin will fail down the line, for example I just added this small piece of code into security plugin initialization sequence:
       import org.bouncycastle.jcajce.provider.symmetric.util.GcmSpecUtil;
 
       ...
        if (GcmSpecUtil.gcmSpecExists()) {
            System.out.println("GcmSpec exists");
        }
It fails with java.security.AccessControlException: access denied ("java.lang.RuntimePermission" "accessDeclaredMembers") because, well, bcprov needs reflection access as well.

I'm afraid not only the sec plugin. Oh boy oh boy. This is a security hole. Since technically any plugin can chnage SSL behave for example or revoke access to the important functionality.

cwperks · 2023-09-05T18:48:25Z

@willyborankin I see you mentioned there is a performance hit if the security plugin explicitly passes in a provider in calls that take a provider. opensearch-project/security#3317 (comment)

i.e.

Cipher cipher = Cipher.getInstance("DESede/CBC/NoPadding", BouncyCastleProvider.PROVIDER_NAME)

instead of

Cipher cipher = Cipher.getInstance("DESede/CBC/NoPadding");

If the provider is passed in explicitly within the security plugin to calls that need one, which policy file is in effect? Any idea what the performance tax is for explicitly passing the provider in each call?

github-actions · 2023-09-05T18:51:50Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/24490/
CommitID: 9e43830
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?

willyborankin · 2023-09-05T18:53:09Z

@willyborankin I see you mentioned there is a performance hit if the security plugin explicitly passes in a provider in calls that take a provider. opensearch-project/security#3317 (comment)

i.e.
Cipher cipher = Cipher.getInstance("DESede/CBC/NoPadding", BouncyCastleProvider.PROVIDER_NAME)
instead of
Cipher cipher = Cipher.getInstance("DESede/CBC/NoPadding");
If the provider is passed in explicitly within the security plugin to calls that need one, which policy file is in effect? Any idea what the performance tax is for explicitly passing the provider in each call?

the policy file is OpenSerach requirements. If library is in libs the policy will be shared between all plugins otherwise each plugin is responsible for the policy it needs. We need a better solution in case of crypto-sdk nothing more but I do not know how it should look like.

peternied

From the dialog I see on this issue it looks like this is creating a new category of risk where previously only individual plugins needed to modify their policy file to gain access to restricted APIs, but by adding this privilege up to core there is no longer a clear isolation.

A change to security posture needs to go through a security review process as opposed to an ad-hoc 11th hour patch.

cwperks · 2023-09-05T18:59:51Z

Closing this PR. Any further discussion on this issue should happen in the security issue: opensearch-project/security#3309

github-actions · 2023-09-05T19:19:49Z

Compatibility status:

Checks if related components are compatible with change 037efca

Incompatible components

Incompatible components: [https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/anomaly-detection.git]

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/reporting.git]

github-actions · 2023-09-05T19:20:47Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/24498/
CommitID: 037efca
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?

Add bouncycastle permissions to security.policy

8c85f27

Signed-off-by: Craig Perkins <[email protected]>

Add CHANGELOG entry

a3f35b2

Signed-off-by: Craig Perkins <[email protected]>

peternied reviewed Sep 5, 2023

View reviewed changes

RyanL1997 reviewed Sep 5, 2023

View reviewed changes

Merge branch 'main' into add-bc-permissions

9e43830

Signed-off-by: Craig Perkins <[email protected]>

cwperks mentioned this pull request Sep 5, 2023

Security Plugin cannot startup due to AccessControlException: access denied opensearch-project/security#3317

Closed

3 tasks

cwperks added 2 commits September 5, 2023 14:41

Add permissions to enable security to read PKCS#1 keys

9e453dd

Signed-off-by: Craig Perkins <[email protected]>

Merge branch 'add-bc-permissions' of https://github.com/cwperks/OpenS…

037efca

…earch into add-bc-permissions

peternied requested changes Sep 5, 2023

View reviewed changes

cwperks closed this Sep 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add bouncycastle permissions to security.policy #9770

Add bouncycastle permissions to security.policy #9770

cwperks commented Sep 5, 2023 •

edited

Loading

cwperks commented Sep 5, 2023

cwperks commented Sep 5, 2023

peternied left a comment

cwperks commented Sep 5, 2023

RyanL1997 Sep 5, 2023

cwperks Sep 5, 2023

reta Sep 5, 2023

github-actions bot commented Sep 5, 2023

github-actions bot commented Sep 5, 2023

github-actions bot commented Sep 5, 2023

codecov bot commented Sep 5, 2023 •

edited

Loading

github-actions bot commented Sep 5, 2023

reta commented Sep 5, 2023

willyborankin commented Sep 5, 2023

reta commented Sep 5, 2023

github-actions bot commented Sep 5, 2023

willyborankin commented Sep 5, 2023 •

edited

Loading

cwperks commented Sep 5, 2023 •

edited

Loading

github-actions bot commented Sep 5, 2023

willyborankin commented Sep 5, 2023

peternied left a comment

cwperks commented Sep 5, 2023

github-actions bot commented Sep 5, 2023

github-actions bot commented Sep 5, 2023

Add bouncycastle permissions to security.policy #9770

Add bouncycastle permissions to security.policy #9770

Conversation

cwperks commented Sep 5, 2023 • edited Loading

Description

Related Issues

Check List

cwperks commented Sep 5, 2023

cwperks commented Sep 5, 2023

peternied left a comment

Choose a reason for hiding this comment

cwperks commented Sep 5, 2023

RyanL1997 Sep 5, 2023

Choose a reason for hiding this comment

cwperks Sep 5, 2023

Choose a reason for hiding this comment

reta Sep 5, 2023

Choose a reason for hiding this comment

github-actions bot commented Sep 5, 2023

Compatibility status:

Incompatible components

Skipped components

Compatible components

github-actions bot commented Sep 5, 2023

Compatibility status:

Incompatible components

Skipped components

Compatible components

github-actions bot commented Sep 5, 2023

Gradle Check (Jenkins) Run Completed with:

codecov bot commented Sep 5, 2023 • edited Loading

Codecov Report

github-actions bot commented Sep 5, 2023

Gradle Check (Jenkins) Run Completed with:

reta commented Sep 5, 2023

willyborankin commented Sep 5, 2023

reta commented Sep 5, 2023

github-actions bot commented Sep 5, 2023

Compatibility status:

Incompatible components

Skipped components

Compatible components

willyborankin commented Sep 5, 2023 • edited Loading

cwperks commented Sep 5, 2023 • edited Loading

github-actions bot commented Sep 5, 2023

Gradle Check (Jenkins) Run Completed with:

willyborankin commented Sep 5, 2023

peternied left a comment

Choose a reason for hiding this comment

cwperks commented Sep 5, 2023

github-actions bot commented Sep 5, 2023

Compatibility status:

Incompatible components

Skipped components

Compatible components

github-actions bot commented Sep 5, 2023

Gradle Check (Jenkins) Run Completed with:

cwperks commented Sep 5, 2023 •

edited

Loading

codecov bot commented Sep 5, 2023 •

edited

Loading

willyborankin commented Sep 5, 2023 •

edited

Loading

cwperks commented Sep 5, 2023 •

edited

Loading