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

[huizhe wang]

On 7/25/2016 2:46 AM, Frank Yuan wrote:
>> -----Original Message-----
>> From: huizhe wang [mailto:huizhe.wang at oracle.com]
>> Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
>>
>>
>>
>> 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:
>>>>>
>>>>> 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("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 ;-()
> I will reduce the scope of the extra permissions as possible, and make sure the safety, e.g. surround the user code(i.e. the test
> code) which requires a permission explicitly with corresponding permission assignment
>
>> 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).
> Currently FilePolicy(maybe it's better to rename to TestFilePolicy) has full access permission to user.dir and read permission to
> test.src, I think they belong to user permission, jaxp lib won't doPrivileged on this.
> Btw, it's unable to add parameter in @Listeners({ xxx.class }) unless using more tricky and complex means.
>
> I believe I will meet problem to identify if a doPrivileged is missing when I strip the extra permissions and then solve the test
> failures case by case, so I would ask you when should we add doPrivileged in jaxp/jdk library code?
> 1. Should doPrivileged only for where the permission can't be granted to the user code, e.g. read some secret system property
> 2. Or should doPrivileged for where the permission needn't be granted to the user code, e.g. read some internal property

The library code shall have *minimal permission* to perform operations 
as required by the *specification/javadoc*. The tests should avoid 
blanket permission so as to expose issues in the code they test.

For example, the Catalog API specified several System properties. It 
therefore shall give privilege to reading those and only those System 
properties. To verify that the code performs properly when Security 
Manager is present, a blanket permission, e.g. new 
PropertyPermission("*", "read, write"),  should be avoided.  Note that 
when this permission is removed from the BasePolicy, FilePolicy won't 
run since it requires permission to read "user.dir". To avoid having to 
grant the permission, the test may choose to read the property before 
setting the SecurityManager.  You might not be able to use TestNG 
Listeners in such case, or maybe you can by initializing the properties 
before the test starts.

Furthermore, the spec also requires that relative file paths be resolved 
to "user.dir", another reason not to grant that property permission in 
test.  However, the test does need to give FilePermission to any user 
files. So FilePermission to user.dir and test.src are fine.

Thanks,
Joe

>
>
> Frank
>
>> Best,
>> Joe
>>