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

[Valerie Peng]

Right, it doesn't. I will update it.

Thanks,

Valerie


On 7/25/2016 8:49 AM, Sean Mullan wrote:
> Does the test need to run in othervm? It doesn't seem like it adds or 
> changes the order of providers.
>
> Otherwise looks good.
>
> --Sean
>
> On 07/22/2016 06:19 PM, Valerie Peng wrote:
>>
>> 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
>>>>>>>
>>>>>
>>>
>>