Decentralize plugin security by rmuir · Pull Request #14108 · elastic/elasticsearch

rmuir · 2015-10-14T09:57:04Z

Today ES core defines all the rules for plugins and special exceptions by name. But this means if a plugin developer needs a similar thing (perhaps just a temporary hack or workaround: maybe in a third party dependency of theirs), it is not possible today, and the only solution is to disable securitymanager entirely.

This is bad for a few reasons:

causes core developers stress because they are in the loop for all problems
encourages disabling security entirely.

This PR presents an alternative, more like the android model, where plugins can include an optional plugin-security.policy file, and users have to confirm the additional requested permissions on installation (if stdin is a controlling terminal, and there is also a new explicit -b batch flag for bin/plugin too just for completeness):

@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
@     WARNING: plugin requires additional permissions     @
@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
* java.lang.RuntimePermission accessClassInPackage.sun.reflect
* java.lang.RuntimePermission closeClassLoader
* java.lang.RuntimePermission createClassLoader
* groovy.security.GroovyCodeSourcePermission /untrusted
See http://docs.oracle.com/javase/8/docs/technotes/guides/security/permissions.html
for descriptions of what these permissions allow and the associated risks.

Continue with installation? [y/N]

I think its fairly safe, we protect plugins directories pretty well now (even unix filesystem permissions are improved recently). These permissions are only granted to the plugin, like today, which means this stuff is contained pretty well. Of course it forces plugin authors to write proper security code, but I think that is ok, versus it not being possible at all. I added recommendations and an example to the plugin authors documentation to try to help with this. Even in the worst case if a plugin author adds AllPermission and screws up all the security code completely, its still better than having no security checking at all for ANY code... That is the major motivation for me wanting to do this, I think it will make things better. It just took me until now to figure out how to make the IDEs work :)

This change also allows tests to work from IDEs for such plugins without as many hacks as before, by configuring some special resources.

dweiss · 2015-10-14T10:15:41Z

Not only it looks like an awesome feature, but also a nice cleanup of existing code (I went through the diff out of curiosity). Very cool.

clintongormley · 2015-10-14T10:18:55Z

nice!

s1monw · 2015-10-14T10:39:30Z

core/src/main/resources/org/elasticsearch/bootstrap/security.policy

boom! nice once - that 46 lines of deletions is worth the change IMO already

s1monw · 2015-10-14T10:44:39Z

this looks awesome! thanks for doing this!

dadoonet · 2015-10-14T10:46:04Z

<3

… prevent user from answering unnecessary questions.

rmuir · 2015-10-14T16:57:48Z

@s1monw I pushed changes to improve the docs. I also moved the check after 'plugin already exists' check, so that the user doesn't have to enter Y/N before that case.

s1monw · 2015-10-14T18:25:15Z

LGTM

* Add ability for plugins to declare additional permissions with a custom plugin-security.policy file and corresponding AccessController logic. See the plugin author's guide for more information. * Add warning messages to users for extra plugin permissions in bin/plugin. * When bin/plugin is run interactively (stdin is a controlling terminal and -b/--batch not supplied), require user confirmation. * Improve unit test and IDE support for plugins with additional permissions by exposing plugin's metadata as a maven test resource. Closes elastic#14108 Squashed commit of the following: commit cf8ace6 Author: Robert Muir <rmuir@apache.org> Date: Wed Oct 14 13:36:05 2015 -0400 fix new unit test from master merge commit 9be3c5a Merge: 2f168b8 7368231 Author: Robert Muir <rmuir@apache.org> Date: Wed Oct 14 12:58:31 2015 -0400 Merge branch 'master' into off_my_back commit 2f168b8 Author: Robert Muir <rmuir@apache.org> Date: Wed Oct 14 12:56:04 2015 -0400 improve plugin author documentation commit 6e6c2bf Author: Robert Muir <rmuir@apache.org> Date: Wed Oct 14 12:52:14 2015 -0400 move security confirmation after 'plugin already installed' check, to prevent user from answering unnecessary questions. commit 08233a2 Author: Robert Muir <rmuir@apache.org> Date: Wed Oct 14 05:36:42 2015 -0400 Add documentation and pluginmanager support commit 05dad86 Author: Robert Muir <rmuir@apache.org> Date: Wed Oct 14 02:22:24 2015 -0400 Decentralize plugin permissions (modulo docs and pluginmanager work) Conflicts: core/src/main/java/org/elasticsearch/bootstrap/Security.java core/src/main/resources/org/elasticsearch/bootstrap/security.policy

* Add ability for plugins to declare additional permissions with a custom plugin-security.policy file and corresponding AccessController logic. See the plugin author's guide for more information. * Add warning messages to users for extra plugin permissions in bin/plugin. * When bin/plugin is run interactively (stdin is a controlling terminal and -b/--batch not supplied), require user confirmation. * Improve unit test and IDE support for plugins with additional permissions by exposing plugin's metadata as a maven test resource. Closes #14108 Squashed commit of the following: commit cc3f7ec Author: Robert Muir <rmuir@apache.org> Date: Thu Oct 15 02:15:49 2015 -0400 java 7 fixes commit 626cf16 Author: Robert Muir <rmuir@apache.org> Date: Thu Oct 15 02:09:19 2015 -0400 Don't merge file back to discovery-ec2 commit a64471d Author: Robert Muir <rmuir@apache.org> Date: Wed Oct 14 14:46:45 2015 -0400 Decentralize plugin security * Add ability for plugins to declare additional permissions with a custom plugin-security.policy file and corresponding AccessController logic. See the plugin author's guide for more information. * Add warning messages to users for extra plugin permissions in bin/plugin. * When bin/plugin is run interactively (stdin is a controlling terminal and -b/--batch not supplied), require user confirmation. * Improve unit test and IDE support for plugins with additional permissions by exposing plugin's metadata as a maven test resource. Closes #14108 Squashed commit of the following: commit cf8ace6 Author: Robert Muir <rmuir@apache.org> Date: Wed Oct 14 13:36:05 2015 -0400 fix new unit test from master merge commit 9be3c5a Merge: 2f168b8 7368231 Author: Robert Muir <rmuir@apache.org> Date: Wed Oct 14 12:58:31 2015 -0400 Merge branch 'master' into off_my_back commit 2f168b8 Author: Robert Muir <rmuir@apache.org> Date: Wed Oct 14 12:56:04 2015 -0400 improve plugin author documentation commit 6e6c2bf Author: Robert Muir <rmuir@apache.org> Date: Wed Oct 14 12:52:14 2015 -0400 move security confirmation after 'plugin already installed' check, to prevent user from answering unnecessary questions. commit 08233a2 Author: Robert Muir <rmuir@apache.org> Date: Wed Oct 14 05:36:42 2015 -0400 Add documentation and pluginmanager support commit 05dad86 Author: Robert Muir <rmuir@apache.org> Date: Wed Oct 14 02:22:24 2015 -0400 Decentralize plugin permissions (modulo docs and pluginmanager work) Conflicts: core/src/main/java/org/elasticsearch/bootstrap/Security.java core/src/main/resources/org/elasticsearch/bootstrap/security.policy

salyh · 2015-10-19T11:26:52Z

Thats a great idea and i would love to see this as soon as possible released. I am working on a shield kerberos realm (nearly finished) and i had to disable security manager entirely cause all the

Subject.doAs

stuff is with current ES policy not allowed. Would also be nice to read krb5.conf directly from /etc/.

jprante · 2015-10-19T21:59:51Z

I hoped for plugin security, thanks! Right now I have to disable security manager completely to get my groovy-based web app plugin things going https://github.com/jprante/elasticsearch-webapp

rmuir · 2015-10-19T22:57:27Z

Hi @salyh @jprante ... I have set the feature for 2.2 because it depends on a ton of other work that was done in the 2.2 codebase. I'm afraid this is about the soonest we can get this out unfortunately.

Before releasing it I have in mind add some minor improvements to it (e.g. ability to use full policy syntax, possibly rename the configuration file to improve IDE/test support, etc). I am working on some of that at the moment.

costin · 2015-10-26T19:34:06Z

@rmuir Awesome stuff! Thanks for adding this in.

rmuir added 2 commits October 14, 2015 02:22

Decentralize plugin permissions (modulo docs and pluginmanager work)

05dad86

Add documentation and pluginmanager support

08233a2

rmuir added discuss v5.0.0-alpha1 v2.2.0 labels Oct 14, 2015

s1monw reviewed Oct 14, 2015
View reviewed changes

rmuir added review :Core/Infra/Plugins Plugin API and infrastructure and removed discuss labels Oct 14, 2015

rmuir added 2 commits October 14, 2015 12:52

move security confirmation after 'plugin already installed' check, to…

6e6c2bf

… prevent user from answering unnecessary questions.

improve plugin author documentation

2f168b8

rmuir added 2 commits October 14, 2015 12:58

Merge branch 'master' into off_my_back

9be3c5a

fix new unit test from master merge

cf8ace6

rmuir closed this in 5d001d1 Oct 14, 2015

clintongormley added the >feature label Oct 15, 2015

rmuir mentioned this pull request Nov 20, 2015

SecurityManager policy for ES 2.0.0 #14880

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decentralize plugin security#14108

Decentralize plugin security#14108
rmuir wants to merge 6 commits intoelastic:masterfrom
rmuir:off_my_back

rmuir commented Oct 14, 2015

Uh oh!

dweiss commented Oct 14, 2015

Uh oh!

clintongormley commented Oct 14, 2015

Uh oh!

s1monw Oct 14, 2015

Uh oh!

s1monw commented Oct 14, 2015

Uh oh!

dadoonet commented Oct 14, 2015

Uh oh!

rmuir commented Oct 14, 2015

Uh oh!

s1monw commented Oct 14, 2015

Uh oh!

salyh commented Oct 19, 2015

Uh oh!

jprante commented Oct 19, 2015

Uh oh!

rmuir commented Oct 19, 2015

Uh oh!

costin commented Oct 26, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Conversation

rmuir commented Oct 14, 2015

Uh oh!

dweiss commented Oct 14, 2015

Uh oh!

clintongormley commented Oct 14, 2015

Uh oh!

s1monw Oct 14, 2015

Choose a reason for hiding this comment

Uh oh!

s1monw commented Oct 14, 2015

Uh oh!

dadoonet commented Oct 14, 2015

Uh oh!

rmuir commented Oct 14, 2015

Uh oh!

s1monw commented Oct 14, 2015

Uh oh!

salyh commented Oct 19, 2015

Uh oh!

jprante commented Oct 19, 2015

Uh oh!

rmuir commented Oct 19, 2015

Uh oh!

costin commented Oct 26, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants