Security Plugin cannot startup due to AccessControlException: access denied by reta · Pull Request #3317 · opensearch-project/security

reta · 2023-09-05T16:47:27Z

Description

Fixing AccessControlException after opensearch-project/OpenSearch#9289, we have 2 options here:

since the bcprov moved to core, it will be loaded by core, the plugin should not try to enforce it
plugin may try to enforce it but the core security policy should be extended to allow that

grant codeBase "${codebase.bcprov-jdk15to18}" {
  permission java.security.SecurityPermission "putProviderProperty.BC";
  permission java.security.SecurityPermission "insertProvider.BC";
  permission java.security.SecurityPermission "removeProviderProperty.BC";
};

Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)
Why these changes are required?
What is the old behavior before changes and new behavior after changes?

Issues Resolved

Closes #3309

Is this a backport? If so, please add backport PR # and/or commits #

Testing

Covered by existing tests

Check List

New functionality includes testing
New functionality has been documented
Commits are signed per the DCO using --signoff

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.

codecov · 2023-09-05T16:51:32Z

Codecov Report

Merging #3317 (366ecfd) into main (fd3a143) will increase coverage by 0.00%.
Report is 1 commits behind head on main.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main    #3317   +/-   ##
=========================================
  Coverage     63.24%   63.25%           
+ Complexity     3450     3449    -1     
=========================================
  Files           263      263           
  Lines         20040    20036    -4     
  Branches       3344     3343    -1     
=========================================
- Hits          12674    12673    -1     
+ Misses         5739     5736    -3     
  Partials       1627     1627

Files Changed	Coverage Δ
.../opensearch/security/OpenSearchSecurityPlugin.java	`84.43% <ø> (-0.10%)`	⬇️

... and 4 files with indirect coverage changes

willyborankin · 2023-09-05T16:55:00Z

@reta we had more settings:

  permission java.security.SecurityPermission "putProviderProperty.BC";
  permission java.security.SecurityPermission "insertProvider.BC";
  permission java.security.SecurityPermission "removeProviderProperty.BC";
  permission java.util.PropertyPermission "*", "write";

add @cwperks aded this fix: #3289. So all of them need to be added to the the SDK now? If yes this is a partial fix :-(

reta · 2023-09-05T16:59:33Z

permission java.util.PropertyPermission "*", "write";

It seems like this one have to go to core :( I am still looking hence this pull request is in draft state

willyborankin · 2023-09-05T17:08:05Z

permission java.util.PropertyPermission "*", "write";

It seems like this one have to go to core :( I am still looking hence this pull request is in draft state

ehhh "read,write" :-(

willyborankin · 2023-09-05T17:09:14Z

@peternied, @cwperks and @reta I think this solution is ok for 2.10 but is not good in the future. Core should not provide such permissions, more important all plugin could add additional permissions.

willyborankin · 2023-09-05T17:10:44Z

@reta im wrong we have such login in the policy.

grant codeBase "${codebase.netty-common}" {
  permission java.lang.RuntimePermission "loadLibrary.*";
};

reta · 2023-09-05T17:26:28Z

src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java

            sm.checkPermission(new SpecialPermission());
        }

-        AccessController.doPrivileged(new PrivilegedAction<Object>() {


@peternied @cwperks any reasons we need to explicitly add BouncyCastleProvider?

Ahhh it is historical. It was added this way.

From the javadoc for java.security.Security it is safe to call addProvider multiple times: https://docs.oracle.com/javase%2F7%2Fdocs%2Fapi%2F%2F/java/security/Security.html#addProvider(java.security.Provider)

the preference position in which the provider was added, or -1 if the provider was not added because it is already installed.

reta · 2023-09-05T17:29:14Z

@reta im wrong we have such login in the policy.
grant codeBase "${codebase.netty-common}" {
  permission java.lang.RuntimePermission "loadLibrary.*";
};

@willyborankin the ${codebase} placeholder is resolved relatively to plugin: /..../plugins/opensearch-security, there is no way to point to $OPENSEARCH/lib from there

willyborankin · 2023-09-05T17:31:48Z

@reta im wrong we have such login in the policy.
grant codeBase "${codebase.netty-common}" {
  permission java.lang.RuntimePermission "loadLibrary.*";
};
@willyborankin the ${codebase} placeholder is resolved relatively to plugin: /..../plugins/opensearch-security, there is no way to point to $OPENSEARCH/lib from there

got it

cwperks · 2023-09-05T18:15:14Z

add @cwperks aded this fix: #3289. So all of them need to be added to the the SDK now? If yes this is a partial fix :-(

@willyborankin you are right that the permissions added in that PR would also need to be added to core since the dependency is now coming from core. I have a PR in core to add the existing bouncy castle permissions that the security plugin has into core opensearch-project/OpenSearch#9770 (Option 2 from this PR's description)

I am trying to figure out now the reasons why the security plugin calls on Security.addProvider, but from my own understanding bouncycastle is used to setup TLS (reading certs and keys) and is used for its implementation of BCrypt

reta · 2023-09-05T18:18:23Z

I am trying to figure out now the reasons why the security plugin calls on Security.addProvider, but from my own understanding bouncycastle is used to setup TLS (reading certs and keys) and is used for its implementation of BCrypt

We definitely need to add the BC provider, but since it is in core now, it should be done by core, that would also solve any issues with other plugins trying to add/remove BC.

…denied Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

cwperks · 2023-09-05T18:21:14Z

@reta From the javadoc for java.security.Security it is safe to call addProvider multiple times: https://docs.oracle.com/javase%2F7%2Fdocs%2Fapi%2F%2F/java/security/Security.html#addProvider(java.security.Provider)

the preference position in which the provider was added, or -1 if the provider was not added because it is already installed.

reta · 2023-09-05T18:23:11Z

@reta From the javadoc for java.security.Security it is safe to call addProvider multiple times: https://docs.oracle.com/javase%2F7%2Fdocs%2Fapi%2F%2F/java/security/Security.html#addProvider(java.security.Provider)

@cwperks The problem is not how many times, the problem is who adds it first because that would impact the security policy to be taken into account (related to #3213 (comment)), and again - you need to grant the permissions to the plugin

willyborankin · 2023-09-05T18:25:26Z

I am trying to figure out now the reasons why the security plugin calls on Security.addProvider, but from my own understanding bouncycastle is used to setup TLS (reading certs and keys) and is used for its implementation of BCrypt

We definitely need to add the BC provider, but since it is in core now, it should be done by core, that would also solve any issues with other plugins trying to add/remove BC.

If you set any security provider this way it means that in the list of sec providers JDK knows about it is in the first place.So you lets say can create a cipher this way:

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

otherwise you need add a provider name:

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

We can change it but it is not so fast as you could think.

reta · 2023-09-05T19:59:24Z

Closing in favor of opensearch-project/OpenSearch#9779

reta requested review from DarshitChanpura, RyanL1997, cliu123, cwperks, davidlago, peternied, stephen-crawford and willyborankin as code owners September 5, 2023 16:47

reta marked this pull request as draft September 5, 2023 16:47

reta commented Sep 5, 2023

View reviewed changes

Security Plugin cannot startup due to AccessControlException: access …

366ecfd

…denied Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

reta force-pushed the issue-3309 branch from 7d6dac6 to 366ecfd Compare September 5, 2023 18:21

cwperks mentioned this pull request Sep 5, 2023

Add bouncycastle permissions to security.policy opensearch-project/OpenSearch#9770

Closed

6 tasks

reta closed this Sep 5, 2023

terryquigleysas mentioned this pull request Jul 19, 2024

Remove hardcoded security provider #4583

Closed

terryquigleysas mentioned this pull request Sep 12, 2024

[ENHANCEMENT] Refactor asn1TaggedObject.getObject() call #4728

Closed

Conversation

reta commented Sep 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Testing

Check List

Uh oh!

codecov bot commented Sep 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

willyborankin commented Sep 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

reta commented Sep 5, 2023

Uh oh!

willyborankin commented Sep 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

willyborankin commented Sep 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

willyborankin commented Sep 5, 2023

Uh oh!

reta Sep 5, 2023

Choose a reason for hiding this comment

Uh oh!

willyborankin Sep 5, 2023

Choose a reason for hiding this comment

Uh oh!

cwperks Sep 5, 2023

Choose a reason for hiding this comment

Uh oh!

reta commented Sep 5, 2023

Uh oh!

willyborankin commented Sep 5, 2023

Uh oh!

cwperks commented Sep 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

reta commented Sep 5, 2023

Uh oh!

cwperks commented Sep 5, 2023

Uh oh!

reta commented Sep 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

willyborankin commented Sep 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

reta commented Sep 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

reta commented Sep 5, 2023 •

edited

Loading

codecov bot commented Sep 5, 2023 •

edited

Loading

willyborankin commented Sep 5, 2023 •

edited

Loading

willyborankin commented Sep 5, 2023 •

edited

Loading

willyborankin commented Sep 5, 2023 •

edited

Loading

cwperks commented Sep 5, 2023 •

edited

Loading

reta commented Sep 5, 2023 •

edited

Loading

willyborankin commented Sep 5, 2023 •

edited

Loading