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

Enable securitymanager #10717

Merged
merged 12 commits into from Apr 24, 2015

Conversation

Projects
None yet
8 participants
@rmuir
Contributor

rmuir commented Apr 22, 2015

We have been using this in our tests for some time. Recently the permissions have gotten reasonable where we don't need read/write access everywhere.

I think we should start ES with securitymanager (just like tomcat and other things) to enhance security.

I ran the lucene benchmark suite comparing security manager off/on and there is no difference. I think a lot of concerns about perceived performance differences don't really make sense if you are managing files and sockets properly.

Here is a prototype, i tried to keep it simple (but damn if the SSD detection and other stuff we have going on fights me on that). There is nothing to configure, though security manager can be disabled in elasticsearch.yml or whatever with security.enabled=false.

We use the same actual policy file for both tests and bin/elasticsearch. We just add additional permissions based on the environment (e.g. data paths and so on) so things will work.

I didnt write any tests yet, but I can add some unit tests for the stuff here.

rmuir added some commits Apr 23, 2015

Remove crazy permissions for filestores, ssds, now that
this logic has been refactored.

Log a warning when security is disabled.
Merge branch 'master' into put_me_in_coach
Conflicts:
	src/main/java/org/elasticsearch/env/NodeEnvironment.java
@rmuir

This comment has been minimized.

Show comment
Hide comment
@rmuir

rmuir Apr 23, 2015

Contributor

I pushed commits simplifying the SSD/filestore stuff after #10755 and adding the warning as @jpountz suggested.

Contributor

rmuir commented Apr 23, 2015

I pushed commits simplifying the SSD/filestore stuff after #10755 and adding the warning as @jpountz suggested.

@s1monw

This comment has been minimized.

Show comment
Hide comment
@s1monw

s1monw Apr 23, 2015

Contributor

this change looks good to me - i am curious if we see per drops with this enabled by default etc. but I even if we do we should push this (plus unittests please) and move the decision to default enable it to a different issue if we need to. I really like that we can offer this to the users and I am really leaning towards enable by default!

Contributor

s1monw commented Apr 23, 2015

this change looks good to me - i am curious if we see per drops with this enabled by default etc. but I even if we do we should push this (plus unittests please) and move the decision to default enable it to a different issue if we need to. I really like that we can offer this to the users and I am really leaning towards enable by default!

@s1monw s1monw self-assigned this Apr 23, 2015

@mikemccand

This comment has been minimized.

Show comment
Hide comment
@mikemccand

mikemccand Apr 23, 2015

Contributor

I tested indexing tiny (log line) docs, same test I run at http://benchmarks.elastic.co.

I ran each of master and this branch two times...

Master is 8999.7 and 9160.0 docs/sec overall (6.9 M docs), while this branch is 9610.9 and 9835.4 docs/sec.

Net/net I don't think security manager hurts indexing performance!

Contributor

mikemccand commented Apr 23, 2015

I tested indexing tiny (log line) docs, same test I run at http://benchmarks.elastic.co.

I ran each of master and this branch two times...

Master is 8999.7 and 9160.0 docs/sec overall (6.9 M docs), while this branch is 9610.9 and 9835.4 docs/sec.

Net/net I don't think security manager hurts indexing performance!

@rmuir

This comment has been minimized.

Show comment
Hide comment
@rmuir

rmuir Apr 24, 2015

Contributor

Unless someone can find real performance issues or problems, i plan to push the change in 72 hours. And you guys can bikeshed logging levels somewhere else, im not doing that.

Contributor

rmuir commented Apr 24, 2015

Unless someone can find real performance issues or problems, i plan to push the change in 72 hours. And you guys can bikeshed logging levels somewhere else, im not doing that.

@kimchy

This comment has been minimized.

Show comment
Hide comment
@kimchy

kimchy Apr 24, 2015

Member

Very promising on the perf test, its great. I will fix the logging post your push, I think that we should also work on a different issue on getting the test permissions out of this file, since it will be confusing for users who look at it, even though those env vars are not set.

Member

kimchy commented Apr 24, 2015

Very promising on the perf test, its great. I will fix the logging post your push, I think that we should also work on a different issue on getting the test permissions out of this file, since it will be confusing for users who look at it, even though those env vars are not set.

@rmuir

This comment has been minimized.

Show comment
Hide comment
@rmuir

rmuir Apr 24, 2015

Contributor

i can remove the logging completely. i am happy with no logging and for someone else to deal with logging completely. its such a ridiculous thing to make bikesheds out of. I will not add any more logging statements to this codebase, trust me.

Contributor

rmuir commented Apr 24, 2015

i can remove the logging completely. i am happy with no logging and for someone else to deal with logging completely. its such a ridiculous thing to make bikesheds out of. I will not add any more logging statements to this codebase, trust me.

@rmuir

This comment has been minimized.

Show comment
Hide comment
@rmuir

rmuir Apr 24, 2015

Contributor

logging is removed.

Contributor

rmuir commented Apr 24, 2015

logging is removed.

@kimchy

This comment has been minimized.

Show comment
Hide comment
@kimchy

kimchy Apr 24, 2015

Member

LGTM, lets push it

Member

kimchy commented Apr 24, 2015

LGTM, lets push it

Remove policy config file, its a resource.
Remove exposed boolean to turn off security.
Add unit test
@rmuir

This comment has been minimized.

Show comment
Hide comment
@rmuir

rmuir Apr 24, 2015

Contributor

I made the policy file a resource (not in conf/) and removed the docs of the option in elasticsearch.yml. Someone can turn it off with security.manager.enabled=false if they dont like it. And they can set JVM_OPTS with -D's to do whatever their heart desires rather than us supporting plumbing. I also added some really simple unit tests so we had something.

Contributor

rmuir commented Apr 24, 2015

I made the policy file a resource (not in conf/) and removed the docs of the option in elasticsearch.yml. Someone can turn it off with security.manager.enabled=false if they dont like it. And they can set JVM_OPTS with -D's to do whatever their heart desires rather than us supporting plumbing. I also added some really simple unit tests so we had something.

@s1monw

This comment has been minimized.

Show comment
Hide comment
@s1monw

s1monw Apr 24, 2015

Contributor

left one tiny nitpick - LGTM otherwise feel free to push after fixing the nitpick :)

Contributor

s1monw commented Apr 24, 2015

left one tiny nitpick - LGTM otherwise feel free to push after fixing the nitpick :)

@kimchy

This comment has been minimized.

Show comment
Hide comment
@kimchy

kimchy Apr 24, 2015

Member

LGTM, @rmuir if someone provides the -Djava.security.policy option, we will still run our code as far as I can tell, just double checking it won't be a problem?

Member

kimchy commented Apr 24, 2015

LGTM, @rmuir if someone provides the -Djava.security.policy option, we will still run our code as far as I can tell, just double checking it won't be a problem?

@rmuir

This comment has been minimized.

Show comment
Hide comment
@rmuir

rmuir Apr 24, 2015

Contributor

Someone with a custom policy will have to also use the option (security.manager.enabled=false) to disable what we are doing.

Contributor

rmuir commented Apr 24, 2015

Someone with a custom policy will have to also use the option (security.manager.enabled=false) to disable what we are doing.

@kimchy

This comment has been minimized.

Show comment
Hide comment
@kimchy

kimchy Apr 24, 2015

Member

@rmuir I see, will it be trappy? which one will be applied if just -Djava.security.policy is provided (not sure about the logic of which ones wins), maybe we should not do our thing if the system property exists as well?

Member

kimchy commented Apr 24, 2015

@rmuir I see, will it be trappy? which one will be applied if just -Djava.security.policy is provided (not sure about the logic of which ones wins), maybe we should not do our thing if the system property exists as well?

@rmuir

This comment has been minimized.

Show comment
Hide comment
@rmuir

rmuir Apr 24, 2015

Contributor

I don't think we should have lots of logic to look if security manager is already set, to look if this or that system property is already set. There is no need or use-case for this IMO. It will just make our codebase even bigger than i already is.

Having a custom security policy is extremely expert. Those users can turn it off with the boolean i provided here.

We only need one way to do this. This isn't perl.

Contributor

rmuir commented Apr 24, 2015

I don't think we should have lots of logic to look if security manager is already set, to look if this or that system property is already set. There is no need or use-case for this IMO. It will just make our codebase even bigger than i already is.

Having a custom security policy is extremely expert. Those users can turn it off with the boolean i provided here.

We only need one way to do this. This isn't perl.

@kimchy

This comment has been minimized.

Show comment
Hide comment
@kimchy

kimchy Apr 24, 2015

Member

@rmuir sounds good

Member

kimchy commented Apr 24, 2015

@rmuir sounds good

rmuir added a commit that referenced this pull request Apr 24, 2015

@rmuir rmuir merged commit c4f7385 into elastic:master Apr 24, 2015

1 check passed

CLA Commit author is a member of Elasticsearch
Details

@kevinkluge kevinkluge removed the in progress label Apr 24, 2015

@tlrx

This comment has been minimized.

Show comment
Hide comment
@tlrx

tlrx Apr 27, 2015

Member

@rmuir sorry to come a day after the fair but I think this change prevents writing in a local FsRepository no?

Member

tlrx commented Apr 27, 2015

@rmuir sorry to come a day after the fair but I think this change prevents writing in a local FsRepository no?

@rmuir

This comment has been minimized.

Show comment
Hide comment
@rmuir

rmuir Apr 27, 2015

Contributor

Probably. I have no idea about this one in particular, but i am sure this change breaks all kinds of things using rogue paths etc. We should likely review all places using paths.get and open bugs for anything except environment that uses it.

Contributor

rmuir commented Apr 27, 2015

Probably. I have no idea about this one in particular, but i am sure this change breaks all kinds of things using rogue paths etc. We should likely review all places using paths.get and open bugs for anything except environment that uses it.

@tlrx

This comment has been minimized.

Show comment
Hide comment
@tlrx

tlrx Apr 27, 2015

Member

I agree, it will surely breaks things here and there. But in the case of snapshot repositories of type fs, users provide a custom path where the snapshots will be written/read (see the doc example)... I suppose it won't work unless users provide a custom policy file (and restart the JVM) or disable the security manager.

Member

tlrx commented Apr 27, 2015

I agree, it will surely breaks things here and there. But in the case of snapshot repositories of type fs, users provide a custom path where the snapshots will be written/read (see the doc example)... I suppose it won't work unless users provide a custom policy file (and restart the JVM) or disable the security manager.

@rmuir

This comment has been minimized.

Show comment
Hide comment
@rmuir

rmuir Apr 27, 2015

Contributor

Why doesnt that feature use a path configured in environment like all other paths. That is the way to fix it imo

Contributor

rmuir commented Apr 27, 2015

Why doesnt that feature use a path configured in environment like all other paths. That is the way to fix it imo

@s1monw

This comment has been minimized.

Show comment
Hide comment
@s1monw

s1monw Apr 27, 2015

Contributor

Why doesnt that feature use a path configured in environment like all other paths. That is the way to fix it imo

I agree this should not be outside of the path or we need to have a dedicated path that is configured.

Contributor

s1monw commented Apr 27, 2015

Why doesnt that feature use a path configured in environment like all other paths. That is the way to fix it imo

I agree this should not be outside of the path or we need to have a dedicated path that is configured.

@tlrx

This comment has been minimized.

Show comment
Hide comment
@tlrx

tlrx Apr 27, 2015

Member

@rmuir @s1monw I agree and opened #10828 for that.

Member

tlrx commented Apr 27, 2015

@rmuir @s1monw I agree and opened #10828 for that.

@s1monw s1monw deleted the rmuir:put_me_in_coach branch Apr 28, 2015

@clintongormley clintongormley changed the title from enable securitymanager to Enable securitymanager Jun 8, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment