On 11/28/2011 02:16 PM, Xuelei Fan wrote:
Sent from my iPad
On Nov 28, 2011, at 1:52 PM, Weijun Wang<weijun.w...@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?
Of course not.
I just think maybe someone else had written similar code before JDK 7
and didn't realize they need to update it.
-Max
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.w...@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
