Skip to content
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

Use version of org.apache.commons:commons-lang3 defined in core #3306

Merged

Conversation

cwperks
Copy link
Member

@cwperks cwperks commented Sep 5, 2023

Description

Use version of org.apache.commons:commons-lang3 defined in core

https://github.com/opensearch-project/OpenSearch/blob/main/buildSrc/version.properties#L42

Fixes issue seen on spotless upgrade PR against 2.x: #3298

  • Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)

Maintenance

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.

Signed-off-by: Craig Perkins <[email protected]>
@cwperks
Copy link
Member Author

cwperks commented Sep 5, 2023

FYI we are getting this dependency transitively through rest-highlevel-client now. It's coming through the new opensearch-encryption-sdk: https://github.com/opensearch-project/OpenSearch/blob/main/libs/encryption-sdk/build.gradle

org.apache.commons:commons-lang3:3.13.0
\--- org.opensearch:opensearch-encryption-sdk:2.10.0-SNAPSHOT
     \--- org.opensearch:opensearch:2.10.0-SNAPSHOT
          \--- org.opensearch.client:opensearch-rest-high-level-client:2.10.0-SNAPSHOT
               \--- runtimeClasspath

org.apache.commons:commons-lang3:3.12.0 -> 3.13.0
\--- runtimeClasspath

@cwperks
Copy link
Member Author

cwperks commented Sep 5, 2023

Seeing this error on plugin install:

java.lang.IllegalStateException: failed to load plugin class [org.opensearch.security.OpenSearchSecurityPlugin]
Likely root cause: java.lang.InternalError: cannot create instance of org.bouncycastle.jcajce.provider.digest.GOST3411$Mappings : java.security.AccessControlException: access denied ("java.security.SecurityPermission" "putProviderProperty.BC")
	at org.bouncycastle.jce.provider.BouncyCastleProvider.loadServiceClass(Unknown Source)
	at org.bouncycastle.jce.provider.BouncyCastleProvider.loadAlgorithms(Unknown Source)
	at org.bouncycastle.jce.provider.BouncyCastleProvider.setup(Unknown Source)
	at org.bouncycastle.jce.provider.BouncyCastleProvider.access$000(Unknown Source)
	at org.bouncycastle.jce.provider.BouncyCastleProvider$1.run(Unknown Source)
	at java.base/java.security.AccessController.doPrivileged(Native Method)
	at org.bouncycastle.jce.provider.BouncyCastleProvider.<init>(Unknown Source)
	at org.opensearch.security.OpenSearchSecurityPlugin$2.run(OpenSearchSecurityPlugin.java:338)
	at java.base/java.security.AccessController.doPrivileged(Native Method)
	at org.opensearch.security.OpenSearchSecurityPlugin.<init>(OpenSearchSecurityPlugin.java:334)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:490)
	at org.opensearch.plugins.PluginsService.loadPlugin(PluginsService.java:782)
	at org.opensearch.plugins.PluginsService.loadBundle(PluginsService.java:731)
	at org.opensearch.plugins.PluginsService.loadBundles(PluginsService.java:533)
	at org.opensearch.plugins.PluginsService.<init>(PluginsService.java:195)
	at org.opensearch.node.Node.<init>(Node.java:469)
	at org.opensearch.node.Node.<init>(Node.java:396)
	at org.opensearch.bootstrap.Bootstrap$5.<init>(Bootstrap.java:242)
	at org.opensearch.bootstrap.Bootstrap.setup(Bootstrap.java:242)
	at org.opensearch.bootstrap.Bootstrap.init(Bootstrap.java:404)
	at org.opensearch.bootstrap.OpenSearch.init(OpenSearch.java:180)
	at org.opensearch.bootstrap.OpenSearch.execute(OpenSearch.java:171)
	at org.opensearch.cli.EnvironmentAwareCommand.execute(EnvironmentAwareCommand.java:104)
	at org.opensearch.cli.Command.mainWithoutErrorHandling(Command.java:138)
	at org.opensearch.cli.Command.main(Command.java:101)
	at org.opensearch.bootstrap.OpenSearch.main(OpenSearch.java:137)
	at org.opensearch.bootstrap.OpenSearch.main(OpenSearch.java:103)

Its odd because this grant is already defined in plugin-security.policy file.

@cwperks cwperks mentioned this pull request Sep 5, 2023
3 tasks
@codecov
Copy link

codecov bot commented Sep 5, 2023

Codecov Report

Merging #3306 (17c0e4c) into main (3af850e) will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #3306      +/-   ##
============================================
- Coverage     63.27%   63.23%   -0.04%     
+ Complexity     3450     3448       -2     
============================================
  Files           263      263              
  Lines         20040    20040              
  Branches       3344     3344              
============================================
- Hits          12680    12673       -7     
- Misses         5732     5740       +8     
+ Partials       1628     1627       -1     

see 6 files with indirect coverage changes

@willyborankin
Copy link
Collaborator

@cwperks looks like implementation "org.bouncycastle:bcprov-jdk15to18:${versions.bouncycastle}" is in the SDK now.

@willyborankin
Copy link
Collaborator

willyborankin commented Sep 5, 2023

@reta looks like cryto-sdk brought BC as deps without permissions as result Sec plugin does not work and I think other plugins as well. OS now tries to check permissions via SDK but not via plugin :-)

@peternied
Copy link
Member

@peternied
Copy link
Member

I am going to push this change in, we need to figure out what is wrong with the SecurityManager, but let's handle that separately from this version alignment that needs to happen across main & 2.x

@peternied peternied merged commit fd3a143 into opensearch-project:main Sep 5, 2023
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/security/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/security/backport-2.x
# Create a new branch
git switch --create backport/backport-3306-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 fd3a143be713a31c278386a21bd2236542101b7d
# Push it to GitHub
git push --set-upstream origin backport/backport-3306-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/security/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-3306-to-2.x.

@cwperks
Copy link
Member Author

cwperks commented Sep 5, 2023

Opened backport: #3307

peternied pushed a commit to peternied/security that referenced this pull request Sep 5, 2023
@willyborankin
Copy link
Collaborator

I am going to push this change in, we need to figure out what is wrong with the SecurityManager, but let's handle that separately from this version alignment that needs to happen across main & 2.x

@peternied I suspect we need use it as static instance and do not register as a default security provider. Given the fact that we do not have good tests for permissions. It will take sometime to pass BC instance through all code base we have.

@reta
Copy link
Collaborator

reta commented Sep 5, 2023

@reta looks like cryto-sdk brought BC as deps without permissions as result Sec plugin does not work and I think other plugins as well. OS now tries to check permissions via SDK but not via plugin :-)

Yeah, I think we have a problem now ....

api project(":libs:opensearch-encryption-sdk

I don't really understand why it is needed there, but things are messed up now

@peternied
Copy link
Member

@peternied
Copy link
Member

I've created an issue to track this problem [1] impacting both main and the 2.10 line

peternied pushed a commit that referenced this pull request Sep 5, 2023
…d in core (#3307)

Backport of #3306 to
2.x

Signed-off-by: Craig Perkins <[email protected]>
stephen-crawford pushed a commit to stephen-crawford/security that referenced this pull request Sep 5, 2023
…search-project#3306)

Use version of org.apache.commons:commons-lang3 defined in core

Signed-off-by: Craig Perkins <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants