RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests

huizhe wang huizhe.wang at oracle.com
Mon Jul 25 06:20:57 UTC 2016



On 7/22/2016 5:53 AM, Daniel Fuchs wrote:
> On 22/07/16 10:15, Frank Yuan wrote:
>> Hi Daniel
>>
>> Thank you very much for your review and the comments!
>>
>>> -----Original Message-----
>>> From: Daniel Fuchs [mailto:daniel.fuchs at oracle.com]
>>> Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP 
>>> unit tests
>>>
>>> Hi Frank,
>>>
>>> I see that in order to be able to run the tests, you were forced
>>> to add a few permissions that the test/test infrastructure need
>>> to setup things:
>>>
>>>   107         addPermission(new SecurityPermission("getPolicy"));
>>>   108         addPermission(new SecurityPermission("setPolicy"));
>>>   109         addPermission(new RuntimePermission("getClassLoader"));
>>>   110         addPermission(new 
>>> RuntimePermission("createClassLoader"));
>>>   111         addPermission(new 
>>> RuntimePermission("setSecurityManager"));
>>>   112         addPermission(new 
>>> RuntimePermission("createSecurityManager"));
>>>   113         addPermission(new RuntimePermission("modifyThread"));
>>>   114         addPermission(new PropertyPermission("*", "read, 
>>> write"));
>>>   115         addPermission(new 
>>> ReflectPermission("suppressAccessChecks"));
>>>   116         addPermission(new RuntimePermission("setIO"));
>>>   117         addPermission(new 
>>> RuntimePermission("setContextClassLoader"));
>>>   118         addPermission(new 
>>> RuntimePermission("accessDeclaredMembers"));
>>>
>>> These are quite powerful permissions, and adding them by default
>>> also means that you might miss a bug - if e.g. a doPrivileged is
>>> missing somewhere in the JAXP code when jaxp tries to e.g. get/create
>>> a classloader, or read a system property, you might not see
>>> it.

Very true. Some of these permissions came from our standalone JAXP tests 
that were granted to ant and junit.

>> Yes, I agree completely. I would control the permission assignment 
>> more tightly:
>> 1. only allow the following necessary permissions in default:
>>     addPermission(new SecurityPermission("getPolicy"));
>>     addPermission(new SecurityPermission("setPolicy"));
>>     addPermission(new RuntimePermission("setSecurityManager"));

These are safe for the current code base. So may be add a note to remind 
for any future changes.

>> 2. other permissions in current default set are commonly used for the 
>> tests, so I would add more Policy classes like existing
>> FilePolicy, etc.
>>   //ClassLoaderPolicy, an individual test may only require some of 
>> them, but I would put them in only one class if you agree
>>     addPermission(new RuntimePermission("getClassLoader"));
>>     addPermission(new RuntimePermission("createClassLoader"));
>>     addPermission(new RuntimePermission("setContextClassLoader"));
>>
>>   // PropertyPolicy, there are various properties needed by the 
>> tests, I would not classify them further...
>>     addPermission(new PropertyPermission("*", "read, write"));
>>
>>   //Reflection permissions, jtreg may require them in deed, so I may 
>> add them in default set
>>     addPermission(new ReflectPermission("suppressAccessChecks"));
>>     addPermission(new RuntimePermission("accessDeclaredMembers"));
>>
>>   //other permissions are used in less tests, I may use 
>> tryRunWithTmpPermission instead of Policy class
>>     addPermission(new RuntimePermission("setIO"));
>>     addPermission(new RuntimePermission("createSecurityManager"));
>>     addPermission(new RuntimePermission("modifyThread"));
>>
>> How about you think?

My understanding is that you intend to grand specific permissions 
limited to the test that will extend these policies, e.g. FilePolicy. I 
think this is ok and an improvement.

>
> It would definitely improve things - but then all the classes
> in the test that runs with this new policy class will inherit
> from these permissions as well. This may or may not be an issue.
> (I confess I haven't looked at all the webrev ;-()

Yeah, it would be good to make sure a policy is safe with the code. If 
both the test code and the code may need a permission, we may have a 
conflict that we need to solve. It may be good to start with the basic 
permission, and add only if required by the test code, with a note 
preferably noting what exactly is needed. "DefaultFeaturesTest" in this 
regard, doesn't seem to require FilePolicy, but 
CatalogReferCircularityTest requires permission to where the source 
files are located, in this case, it would be good to make it specific. 
For example, instead of being called "FilePolicy", it may be clearer to 
add a parameter so that it's known where the File permission is given 
(e.g. the source dir in this case).

Best,
Joe

>
>
>>> I had a similar issue when writing logging test, were I wanted
>>> to temporarily disable permission checking in the middle of a test
>>> to perform an infrastructure configuration.
>>>
>>> So what I did is use an ThreadLocal<AtomicBoolean> to temporarily
>>> disable permission checking - which allows me in my tests to do things
>>> like:
>>>
>>> boolean before = allowAll.get().get();
>>> allowAll.get().set(true);
>>> try {
>>>     do something that requires a permission
>>> } finally {
>>>     allowAll.get().set(before);
>>> }
>>>
>> JAXPTestUtilities.tryRunWithTmpPermission is similar with this, see 
>> the example:
>> http://cr.openjdk.java.net/~fyuan/8067170/webrev.01/test/javax/xml/jaxp/unittest/stream/XMLEventReaderTest/Bug6555001.java.sdiff.htm 
>>
>> l
>
> Yes that part looks fine.
>
> cheers,
>
> -- daniel
>
>>> My implementation of Policy::implies also checks for
>>>
>>> if (allowAll.get().get()) return true;
>>>
>>> This allows me to control more tightly the set of permissions
>>> I want my test to run under, while still being able to
>>> perform any action I want to set up things without having
>>> to give the same permission to all.
>>>
>>> Hope this helps,
>>>
>>> -- daniel
>>>
>>>
>>>
>>> On 22/07/16 07:59, Frank Yuan wrote:
>>>> According to Amy's suggestion, re-generate a webrev 
>>>> http://cr.openjdk.java.net/~fyuan/8067170/webrev.01/ as well as fix 
>>>> some
>> issues,
>>>> please check.
>>>>
>>>> Thanks
>>>> Frank
>>>>
>>>>> -----Original Message-----
>>>>> From: Amy Lu [mailto:amy.lu at oracle.com]
>>>>> Sent: Monday, July 18, 2016 5:42 PM
>>>>> To: Frank Yuan; 'core-libs-dev'
>>>>> Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on 
>>>>> JAXP unit tests
>>>>>
>>>>> On 7/18/16 5:32 PM, Frank Yuan wrote:
>>>>>> Btw, I moved internaltest into unittest because it's unnecessary 
>>>>>> to separate them.
>>>>>
>>>>> Maybe you'd like to regenerate the webrev with hg move for those 
>>>>> files?
>>>>>
>>>>> Thanks,
>>>>> Amy
>>>>
>>>>
>>
>>
>



More information about the core-libs-dev mailing list