[9] RFR 8159488 "Deprivilege java.xml.crypto" and 8161171 "Missed the make/common/Modules.gmk file when integrating JDK-8154191"

[Valerie Peng]

I added a new test covering the basics of getInstance() methods.

Webrev updated: http://cr.openjdk.java.net/~valeriep/8159488/webrev.02

If you have a particular scenario that you want me to cover, please let 
me know.
Thanks,
Valerie

On 7/21/2016 12:55 PM, Valerie Peng wrote:
>
> Existing regression tests for the java.xml.crypto do test the 
> getInstance() methods as I observed during my own testing.
>
> Maybe not all of them are covered though. I will double check and add 
> test if necessary...
>
> Thanks,
>
> Valerie
>
>
> On 7/21/2016 12:08 PM, Sean Mullan wrote:
>> On 07/20/2016 07:57 PM, Valerie Peng wrote:
>>> Sean,
>>>
>>> I have updated webrev to rid the dependency on sun.security.jca
>>> packages. Also, I found one additional SecurityPermission is needed.
>>>
>>> Please find the updated webrev at:
>>> http://cr.openjdk.java.net/~valeriep/8159488/webrev.01/
>>>
>>>
>>> So, we should close JDK-8161710 as a duplicate of JDK-8159488 then?
>>
>> Yes. The fix looks good. Could you write a simple test which calls 
>> the getInstance methods with good/bogus algs and providers just to 
>> make sure they all work as expected? There don't seem to be any 
>> regression tests for that.
>>
>> Thanks,
>> Sean
>>
>>>
>>> Thanks,
>>> Valerie
>>>
>>> On 7/19/2016 11:57 AM, Sean Mullan wrote:
>>>> On 07/18/2016 05:38 PM, Valerie Peng wrote:
>>>>> Hi Sean,
>>>>>
>>>>> I found these two classes in java.xml.crypto module reading local 
>>>>> files:
>>>>> src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/keys/storage/implementations/CertsInFilesystemDirectoryResolver.java 
>>>>>
>>>>>
>>>>>
>>>>> src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/utils/JavaUtils.java 
>>>>>
>>>>>
>>>>
>>>> I think we should leave it out. The methods that need that permission
>>>> are in non-exported packages and are essentially unused.
>>>>
>>>> Also, you may have noticed, but I just filed
>>>> https://bugs.openjdk.java.net/browse/JDK-8161710. Fixing this would
>>>> allow us to remove this permission:
>>>>
>>>>   permission java.lang.RuntimePermission
>>>> "accessClassInPackage.sun.security.jca.*";
>>>>
>>>> I'm not sure if you have the time to tackle that as part of this
>>>> issue, but I think it would be acceptable to fix them together as part
>>>> of this issue if so.
>>>>
>>>> --Sean
>>>>
>>>>>
>>>>>
>>>>>
>>>>> If you think the File reading permission is not needed for
>>>>> java.xml.crypto module, I will remove the corresponding permission
>>>>> entry.
>>>>> Thanks,
>>>>> Valerie
>>>>>
>>>>> On 7/18/2016 12:48 PM, Sean Mullan wrote:
>>>>>> On 07/13/2016 08:10 PM, Valerie Peng wrote:
>>>>>>> Sean,
>>>>>>>
>>>>>>> Can you please review the following two webrevs?
>>>>>>>
>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8159488
>>>>>>> Webrev: http://cr.openjdk.java.net/~valeriep/8159488/
>>>>>>
>>>>>> Looks good except for this one:
>>>>>>
>>>>>>  127         // needed for reading Certs
>>>>>>  128         permission java.io.FilePermission "<<ALL 
>>>>>> FILES>>","read";
>>>>>>
>>>>>> Why is that needed?
>>>>>>
>>>>>>>
>>>>>>> While making changes for 8159488, I noticed a problem with my 
>>>>>>> earlier
>>>>>>> putback of 8154191 - the top level Modules.gmk was not integrated.
>>>>>>> So, I filed 8161171: Missed the make/common/Modules.gmk file when
>>>>>>> integrating JDK-8154191.
>>>>>>> Can you also review this? It's essentially the same change as 
>>>>>>> the one
>>>>>>> reviewed.
>>>>>>>
>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8161171
>>>>>>> Webrev: http://cr.openjdk.java.net/~valeriep/8161171/webrev.00/
>>>>>>
>>>>>> I'll skip this since Mandy already reviewed that one.
>>>>>>
>>>>>> --Sean
>>>>>
>>>
>