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

[Valerie Peng]

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
>>>>
>>