apache / lucene

Apache Lucene open-source search software
https://lucene.apache.org/
Apache License 2.0
2.6k stars 1.01k forks source link

Create Java security manager for forcible asserting behaviours in testing [LUCENE-4337] #5404

Closed asfimport closed 12 years ago

asfimport commented 12 years ago

Following on from conversations about mutation testing, there is an interest in building a Java security manager that is able to assert / guarantee certain behaviours


Migrated from LUCENE-4337 by Greg Bowyer (@GregBowyer), resolved Aug 30 2012 Attachments: ChrootSecurityManager.java, ChrootSecurityManagerTest.java, LUCENE-4337.patch (versions: 3), LUCENE-4337-not-working.patch Linked issues:

asfimport commented 12 years ago

Uwe Schindler (@uschindler) (migrated from JIRA)

I checked already: We dont really need an own SecurityManager implementation at all. All this can be done from command line executing the tests: http://docs.oracle.com/javase/6/docs/technotes/guides/security/PolicyFiles.html

We just need to create a PolicyFile that allows READ,EXECUTE for all files, but READ,WRITE,EXECUTE,DELETE for all files in the working copy (using a FilePermission). This policy file can be passed to the underlying Test JVM with -Dargs=... and we are done. I will later do some tests to try out. No need to write any line of code.

asfimport commented 12 years ago

Uwe Schindler (@uschindler) (migrated from JIRA)

Very simple patch:

This adds some sysprops to the test runner to throw a SecurityException, when a test tries the following:

asfimport commented 12 years ago

Uwe Schindler (@uschindler) (migrated from JIRA)

And a 3rd thing:

asfimport commented 12 years ago

Robert Muir (@rmuir) (migrated from JIRA)

Patch is really simple and nice.

Maybe we should think about adding exemptions for the known problems and get this in to prevent future problems?

asfimport commented 12 years ago

Uwe Schindler (@uschindler) (migrated from JIRA)

Attached is a patch that works around some problems:

The remaining issue is only one test spawning the MultiCore example server and writing to files inside the example server, which they should not do in tests. My idea would be to copy the example server to a tmpDir and start it from there?

asfimport commented 12 years ago

Uwe Schindler (@uschindler) (migrated from JIRA)

Now also the TestMultiCoreConfBootstrap test was fixed. The problem was that it (unlike other tests) missed to redirect the started server's dataDirs.

Now all tests pass with the policy file, a final run is going on, but I think it's ready to commit.

I will commit this tomorrow to keep an eye on Jenkins, because I don't trust 3rd party JVMs and SecurityManagers...

asfimport commented 12 years ago

Greg Bowyer (@GregBowyer) (migrated from JIRA)

I have a slightly different approach that I was testing last night that programatically creates the policy so that we dont have to allow other permissions all the time

asfimport commented 12 years ago

Greg Bowyer (@GregBowyer) (migrated from JIRA)

Gregs alternative approach (ChrootSecurityManager) (untested - just for discussion); not that I dont think we can use the approach Uwe worked on, just that I am not sure with going down the strict policy route

asfimport commented 12 years ago

Greg Bowyer (@GregBowyer) (migrated from JIRA)

Actually you know what, I think I just talked myself out of my approach, yes its a bit more generic but maybe its less transparent to people that dont spend their time reading JVM source code, at least the policy file is more obvious to those who are approaching this for the first time.

asfimport commented 12 years ago

Uwe Schindler (@uschindler) (migrated from JIRA)

The approach is also good - and can also be configured via sysprops! Funny: you also added readlink permission, but i left that out as we are on Java 6.

The good thing with being explicit is my experience from today: because today I found lots of places where e.g. Solr was opening ports on 0.0.0.0 instead of localhost(127.0.0.1)-only which is bad for tests (I was always clicking on Windows-Firewall to allow that, krrrrr). So we should limit the tests as much as possible - a strict policy rule is the best. It's also a good test of Lucene and Solr if it works with a real J2EE security manager...

If we add new features, the policy file is edited quickly. If later JDK versions add new Permissions, its also no problem, because we won't use them at the moment.

asfimport commented 12 years ago

Uwe Schindler (@uschindler) (migrated from JIRA)

Actually you know what, I think I just talked myself out of my approach, yes its a bit more generic but maybe its less transparent to people that dont spend their time reading JVM source code, at least the policy file is more obvious to those who are approaching this for the first time.

I also spend my time in reading JDK source code today! But it was more when fixing Solr test violations (die, JMX/RMI, die, die, die).

asfimport commented 12 years ago

Robert Muir (@rmuir) (migrated from JIRA)

This reminds me of fixing the contrib/remote test cases! I like this approach as a start (and the test fixes). Maybe we can iterate from here?

Perhaps it will clear up some false test fails.

asfimport commented 12 years ago

Greg Bowyer (@GregBowyer) (migrated from JIRA)

I also spend my time in reading JDK source code today! But it was more when fixing Solr test violations (die, JMX/RMI, die, die, die).

Right there with you on RMI

asfimport commented 12 years ago

Uwe Schindler (@uschindler) (migrated from JIRA)

Greg, I have a good idea, that prevents us from really specifying everything in the policy file: I would only make the AllOtherPermission available in test-framework and make it take a list of java class names (comma separated) as parameter. Then it can be added to the default policy file like:

permission org.apache.lucene.utils.AllOtherPermission "java.io.FilePermission,java.net.SocketPermission,java.security.SecurityPermission";

This would grant (like yours) all other permissions, so only the listed permissions are added to the policy file. The implies() would only check for all those classes.

I will come up with a new patch, that simpliefies this!

asfimport commented 12 years ago

Uwe Schindler (@uschindler) (migrated from JIRA)

I tried that out but failed to do it, because the policy file was not able to load the custom Permission at all (it must be signed maybe). It did not even print a System.err inside a static {} block in the class!

I also think the current explicit permissions are much better to manage and we know what our code needs (as we have to enable additional permissions when new features are added).

asfimport commented 12 years ago

Uwe Schindler (@uschindler) (migrated from JIRA)

I will commit the attached patch now to trunk and 4.x.

asfimport commented 12 years ago

Uwe Schindler (@uschindler) (migrated from JIRA)

Committed trunk revision: 1378914 Committed 4.x revision: 1378915

Thanks Greg for the good idea and some help + hints :-)

asfimport commented 12 years ago

Uwe Schindler (@uschindler) (migrated from JIRA)

Just to conclude some problems we had and why I fixed it, also for ASF Jenkins:

  1. The security policy does not prevent java classes from LISTEN on any ip adresses. Policys only check the port number. So if you bind to 0.0.0.0:123 or 127.0.0.1:123 or a.b.c.d:123 makes no difference if port 123 is allowed by security policy.
  2. The security policy as we have it does not grant ACCEPT on any different address than localhost. This means although all Solr servers opening public ports on 0.0.0.0 while running tests, the Java 'Firewall' (LOL) protects you by only allowing connections from localhost not external IPs
  3. The policy pattern to do this is defaulting by JDK's to "localhost:1024-", which means it may accept connections from localhost on remote ports >= 1024 (as client, means remote socket address as seen from acceptor) and it may listen only on ports >= 1024. This works perfectly unless you have a broken network configuration, so we added additional policies:

Finally Robert fixed the recent Jenkins issue caused by the security filesystem sandbox, so the clover tests were not able to write the clover.db file outside the sandbox. Thanks Robert!

asfimport commented 12 years ago

Uwe Schindler (@uschindler) (migrated from JIRA)

Just for completeness: I add here my ideas to create an AllButPermission (a Permission that grants everything, except a dynamically configureable list of Permission classes). The class is quite big and the small improvement of the policy file does not really make this a good alternative (although it would be cool). Unfortunately the patch works if you use the Permission directly in Java code, but from the policy file it is simply ignored. From my studies it looks like a Permission class must be in a signed code base to be used by the policy parser (like rt.jar).

Maybe Greg, you can use that as an improvement to your class.

asfimport commented 11 years ago

Uwe Schindler (@uschindler) (migrated from JIRA)

Closed after release.