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

Enable securitymanager #10717

Merged
merged 12 commits into from Apr 24, 2015
Merged

Enable securitymanager #10717

merged 12 commits into from Apr 24, 2015

Conversation

rmuir
Copy link
Contributor

@rmuir 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.

Security.configure(environment);
logger.info("security enabled");
} else {
logger.info("security disabled");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be a warning?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If its disabled lets issue a warning.

this logic has been refactored.

Log a warning when security is disabled.
Conflicts:
	src/main/java/org/elasticsearch/env/NodeEnvironment.java
@rmuir
Copy link
Contributor Author

rmuir commented Apr 23, 2015

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

@s1monw
Copy link
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
logger.info("security enabled");
} else {
logger.warn("security disabled");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we debug log the default? also leaning to have info the "non default" setting, thats what we try to do most times in other components to try and keep the startup logs clean and informative.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is officially back and forth at this point. I started at info and went to warn based on feedback. Guys get your shit together, im not playing this game.

@mikemccand
Copy link
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!

@rmuir
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

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
Copy link
Contributor Author

rmuir commented Apr 24, 2015

logging is removed.

@kimchy
Copy link
Member

kimchy commented Apr 24, 2015

LGTM, lets push it

Remove exposed boolean to turn off security.
Add unit test
@rmuir
Copy link
Contributor Author

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.

@@ -92,6 +91,12 @@ public boolean handle(int code) {
});
}
}

private void setupSecurity(Settings settings, Environment environment) throws Exception {
if (settings.getAsBoolean("security.manager.enabled", true)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make "security.manager.enabled" a constant please

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a constant. Just like all the other strings in this file :)

So I will add it, but I will do so under protest:

  • there are zero named constants in this file, I am just matching the style surrounding the code, for consistency.
  • named constants only used once just cause indirection hurt locality of reference to those reading the code.

In general, named constants considered harmful.

@s1monw
Copy link
Contributor

s1monw commented Apr 24, 2015

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

@kimchy
Copy link
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
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

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
Copy link
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
@tlrx
Copy link
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
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

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
Copy link
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
Copy link
Member

tlrx commented Apr 27, 2015

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

@s1monw s1monw deleted the put_me_in_coach branch April 28, 2015 11:19
@clintongormley clintongormley changed the title enable securitymanager Enable securitymanager Jun 8, 2015
@clintongormley clintongormley added >enhancement :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts labels Jun 8, 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

9 participants