Code review request, 7115524: sun.security.provider.certpath.ssl.SSLServerCertStore no longer works

[Xuelei Fan]

On 11/28/2011 12:34 PM, Weijun Wang wrote:
> 
> On 11/28/2011 11:27 AM, Xuelei Fan wrote:
>> webrev: http://cr.openjdk.java.net/~xuelei/7115524/webrev.02/
>>
>> I think the class is special in that in the class the client is not
>> really want to establish a HTTPS/TLS connection with the server. The
>> purpose of the class is to get the server certificate. It does not
>> matter whether the TLS connection is negotiated or not. So the class
>> only need a parser that can read the server certificate during the TLS
>> handshaking. The "security" functions (for example, encryption
>> decryption digest, etc.) do not make sense. In such situation, we really
>> don't mind what's the underlying providers.
>>
>> Does it make sense?
> 
> Yes.
> 
> But I really don't think you need to change this behavior. If this class
> is not used by anyone else except KeyTool, there is no difference
> between a static field and a local variable. I really don't like static
> fields.
>
I think it is only used by KeyTool at present. But I really hate to see
multiple SSLContext instances and TM/TM instances in an application, it
is designed to be immutable.

I have no special preference about static fields and local variables. If
the local variable is always the same across the class life cycle, I
prefer static field; otherwise, I prefer local variable.

>>
>> I make a update, so that even the TLS handshaking failed or other IO
>> exceptions, once we are able to get the server certificate, we will
>> use it.
> 
> This is good, and you can add a comment on this.
> 
There is a two-line comment in the update.

Thanks,
Xuelei

> Thanks
> Max
> 
>>
>> Thanks,
>> Xuelei
>>
>>
>> On 11/27/2011 10:31 PM, Xuelei Fan wrote:
>>> On 11/27/2011 7:07 PM, Weijun Wang wrote:
>>>>
>>>>
>>>> On 11/27/2011 05:48 PM, Xuelei Fan wrote:
>>>>>
>>>>>
>>>>> Sent from my iPad
>>>>>
>>>>> On Nov 27, 2011, at 5:01 PM, Weijun Wang<weijun.wang at oracle.com>  
>>>>> wrote:
>>>>>
>>>>>> Anyway, I find this a little unsafe. One can add a new SSL security
>>>>>> provider at run time, and then run this method, and the new provider
>>>>>> can be loaded.
>>>>>>
>>>>> I did not follow your ideas. The new provider is loaded, and what's
>>>>> the worries then?
>>>>
>>>> Typo, I meant "cannot".
>>>>
>>>> 1. Run this method, the default SSL provider loaded
>>>> 2. Add a new SSL security provider
>>>> 3. Run this method again, still the old provider.
>>>>
>>> Got it. But this class is very special in that the security provider may
>>> be useless.
>>>
>>> It is a little complicated. I will call you tomorrow to explain more. I
>>> think we still have space to improve the stability and performance based
>>> on the latest update.
>>>
>>> Thanks,
>>> Xuelei