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

[Sean Mullan]

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