RFR: 8155039: Simplify code to setup SSLContextImpl and TrustManagerFactoryImpl

[Sean Mullan]

On 06/09/2016 10:32 PM, Mandy Chung wrote:
> Hi Claes,
>
> I don’t like the PropertiesWrapper idea.  The caller should be
> cautious in storing any sensitive information.  For the system
> properties, these callsites use it in the local scope that I don’t
> see any reason and benefit to introduce a wrapper.  I didn’t follow
> this discussion closely and I may miss some reason ?

The original code used multiple calls to System.getProperty wrapped in a 
doPrivileged. Claes' first iteration of the fix changed this to use a 
GetPropertyAction.privilegedGetProperties method that returned a 
Properties object. I expressed a concern that this was now exposing an 
object that, if accidentally leaked to untrusted code could cause much 
more damage than the original code (since the code would be able to 
set/get/remove *any* system property). Hence the current fix which uses 
a wrapper class which is not exported.

--Sean

>
> Mandy
>
>> On Jun 9, 2016, at 5:31 PM, Claes Redestad
>> <claes.redestad at oracle.com> wrote:
>>
>> Hi,
>>
>> by using a non-exported type, PropertiesWrapper, to encapsulate
>> Properties this change makes it impossible for a JDK developer to
>> accidentally leak system properties outside of java.base without
>> breaking encapsulation. I believe that was yours and Sean's main
>> concern about the API changes to GetPropertyAction that this is
>> hopefully a final amendment to.
>>
>> Generally the changes to streamline system property access bring
>> about minor improvements to startup and footprint by reducing the
>> number of classes generated and loaded as well as the number of
>> doPriv calls done.
>>
>> /Claes
>>
>> On 2016-06-08 03:24, Xuelei Fan wrote:
>>> Hi Claes,
>>>
>>> What's the necessary to get all system properties for just one
>>> specific one?  Can you explain more about the necessary to make
>>> the change?
>>>
>>> Thanks, Xuelei
>>>
>>> On 6/8/2016 3:44 AM, Claes Redestad wrote:
>>>> Hi,
>>>>
>>>> there is some lingering concern that this and related changes
>>>> make it that much easier to accidentally leak the system
>>>> Properties object outside of core modules. By wrapping access
>>>> to the system Properties object in a class residing in a
>>>> non-exported package we disallow this at little to no cost:
>>>>
>>>> http://cr.openjdk.java.net/~redestad/8155039/webrev.02/
>>>>
>>>> /Claes
>>>>
>>>> On 2016-05-12 15:37, Claes Redestad wrote:
>>>>> Hi,
>>>>>
>>>>> the API this improvement depends on was updated to reflect
>>>>> more clearly that this is taking a privileged action:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8155775
>>>>>
>>>>> Here's the updated webrev:
>>>>> http://cr.openjdk.java.net/~redestad/8155039/webrev.01/
>>>>>
>>>>> Thanks!
>>>>>
>>>>> /Claes
>>>>>
>>>>> On 2016-04-25 19:28, Claes Redestad wrote:
>>>>>> Hi,
>>>>>>
>>>>>> SSLContextImpl and TrustManagerFactoryImpl has some setup
>>>>>> code that could be simplified, getting rid of a couple of
>>>>>> anonymous classes in the process.
>>>>>>
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~redestad/8155039/webrev.00 Bug:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8155039
>>>>>>
>>>>>> Alternatively we could remove OpenFileInputStreamAction
>>>>>> instead since these two uses introduced here are actually
>>>>>> the only active uses of this old convenience class.
>>>>>>
>>>>>> Thanks!
>>>>>>
>>>>>> /Claes
>>>>>
>>>
>