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

[Sean Mullan]

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