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

steps to remove dangerous security permissions #11898

Merged
merged 5 commits into from Jun 29, 2015
Merged

Conversation

rmuir
Copy link
Contributor

@rmuir rmuir commented Jun 27, 2015

These two "unsafe-like" permissions (sun.misc and reflectionFactoryAccess) should be more contained. Removing any of these permissions is like climbing mount everest, but I think we can make some progress on it.

This patch:

  • Add simple infra to do per-jar permissions.
  • Remove code copy of jsr166e and use one from maven.
  • Grant sun.misc access specifically to lucene-core.jar and jsr166e.jar.
  • Remove reflectionFactoryAccess and grant only to mockito (see below)

Caveats:

  • I couldn't yet remove sun.misc completely. 4 plugin tests fail, this is because PluginManager currently does evil hacks to add jars directly to the system classloader. This should be avoided (instead elasticsearch.bat/sh should simply setup correct classpath, keep this immutable!!!). But this patch is progress, because there is now just that one thing left to fix.
  • Plugins using mockito need to change maven configuration to use https://github.com/rmuir/securemock (can we bring this into an ES repo?). Code changes are not needed. I want to encourage use of mocking. This library is just a binary-compat replacement for Mockito (it wraps it). This scheme was tested with all plugins using mockito.

@rmuir
Copy link
Contributor Author

rmuir commented Jun 27, 2015

@s1monw @uboness appreciate your thoughts on this.

@dadoonet
Copy link
Member

This should be avoided (instead elasticsearch.bat/sh should simply setup correct classpath, keep this immutable!!!).

I just love this idea. If we won't never use hot reload, that's just a fantastic simplification.

I want to encourage use of mocking

Big +1. I really like to have unit tests in plugins and not (less) integration tests

@s1monw
Copy link
Contributor

s1monw commented Jun 29, 2015

@rmuir even though that this is not perfect this is a massive step in the right direction. Before I say anything else this LGTM and should go in as it is. We can move secure mock in our codebase on core even to make things simpler?

For PluginManager I think we should make classpath handling fixes a blocker for 2.0 and fix it the right way but lets move on here... I am leaning towards making the pluginmanager a sep module anyway but that is a different story.

@uboness
Copy link
Contributor

uboness commented Jun 29, 2015

fully agree with @s1monw here... would love to see the secure mock in core!

+1 on a blocker for fixing the plugin manager. Just as a side note... moving to CP building on the script level would be great, we just need to make sure all works well with all the packages as well (deb, rpm, etc..)

@rmuir
Copy link
Contributor Author

rmuir commented Jun 29, 2015

I'll spin off a separate issue for the mocking, and then come back to this one.

rmuir added a commit that referenced this pull request Jun 29, 2015
steps to remove dangerous security permissions
@rmuir rmuir merged commit 55c33b2 into elastic:master Jun 29, 2015
@clintongormley clintongormley added >enhancement :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts labels Jun 30, 2015
@mark-vieira mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts >enhancement Team:Delivery Meta label for Delivery team v2.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants