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

[Xuelei Fan]

Sent from my iPad

On Nov 28, 2011, at 1:52 PM, Weijun Wang <weijun.wang at oracle.com> wrote:

> OK. Let's stop arguing. The code is fine.
> 
> Something else: you said the original SSLServerCertStore class "is a little bit out of date". Do you think it's possible that someone else might code in the same way? In other words, is it only "out of date" or the implementation was wrong even in a JDK 1.4.2 view?
> 
In JDK 7 and later, all oracle providers need to implement x509extendedTrustManager rather than X509TM when a TM is needed. This class did not extends X509ExtendedTM, that's why I think it is out of date. To fix the bug, it's enough if I only extend the X509ExtendedTM.

And I try to find make more improve except the TM extensions. The implement of getAcceptedIssuers() is wrong. But as the old code does not call the method, so the issue is not exposed in JDK 6 and previous releases. 

If someone else code in the same way, of course it is possible, but it's not comply to the spec. We cannot ensure the implement will work when it does not comply to the spec.

I think the SSLServerCertStore is new in JDK 7. Did we really have the code since 1.4.2?

Xuelei

> Thanks
> Max
> 
> 
> On 11/28/2011 12:48 PM, Xuelei Fan wrote:
>> 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
>>