Bug 315333

Comment 1 Noriaki Takatsu 2010-06-01 22:02:08 EDT

Created attachment 170733 [details]
Patch for verification against the chained certificates in the client

Comment 2 David McKnight 2010-06-02 13:02:54 EDT

Created attachment 170839 [details]
patch updated with copyright

Comment 3 Martin Oberhuber 2010-06-02 13:24:24 EDT

Dave, you need to iplog+ the ORIGINAL contribution, and not yours.

Comment 4 Martin Oberhuber 2010-06-02 13:28:44 EDT

I must admit that I do not understand the change. I also do not understand how that would relate to bug 264858 (or did you mean to reference something different?)

Noriaki or Dave, can you explain the change please.

Comment 5 Martin Oberhuber 2010-06-02 16:37:42 EDT

Comment on attachment 170839 [details]
patch updated with copyright

The other things (beside an explanation of what's the code supposed to do) I require before giving a PMC +1 on this are:

1. Justification why this is important

2. A test case description. What behavior do end users see with the bug, and how does the fix resolve that. Ideally, some steps that an arbitrary person in the Community can perform to verify that the fix has been made properly.

I hope you do not perceive this as overly bureaucratic... I'm trying to understand the impact, the risk and how to mitigate the risk by testing at this point.

Comment 6 David McKnight 2010-06-02 17:38:14 EDT

(In reply to comment #4)
> I must admit that I do not understand the change. I also do not understand how
> that would relate to bug 264858 (or did you mean to reference something
> different?)
> 
> Noriaki or Dave, can you explain the change please.



As a background, here is the Javadoc describing what the checkTrusted method is supposed to do:

http://java.sun.com/j2se/1.4.2/docs/api/javax/net/ssl/X509TrustManager.html

"Given the partial or complete certificate chain provided by the peer, build a certificate path to a trusted root and return if it can be validated and is trusted for client/server SSL authentication based on the authentication type."


In bug 264858, the following was added before the call to verify but it is unneeded and restrictive:

if (cert.getSubjectDN().equals(tcert.getIssuerDN())) {

Comment 7 David McKnight 2010-06-02 17:39:34 EDT

(In reply to comment #5)
> (From update of attachment 170839 [details])
> The other things (beside an explanation of what's the code supposed to do) I
> require before giving a PMC +1 on this are:
> 
> 1. Justification why this is important
> 
> 2. A test case description. What behavior do end users see with the bug, and
> how does the fix resolve that. Ideally, some steps that an arbitrary person in
> the Community can perform to verify that the fix has been made properly.
> 
> I hope you do not perceive this as overly bureaucratic... I'm trying to
> understand the impact, the risk and how to mitigate the risk by testing at this
> point.

I'll let Noriaki answer the justification and describe the conditions of the customer problem.

Comment 8 Noriaki Takatsu 2010-06-02 21:39:29 EDT

This is a problem when the client logs on the server by
a pair of userid and password under SSL connection.

In bug 264858, the following check was added:
   if (cert.getSubjectDN().equals(tcert.getIssuerDN())) {

This check means that the server certificate must be a self-signed certificate.
But almost all users use server certificate signed by internal Certificate Authority or public Certificate Authority (e.g. Verisign).
The following are the traces in this point under my test environments:
  cert.getSubjectDN()=CN=Yamato Server, OU=YSL, O=IBMJ, C=JP
  tcert.getIssuerDN()=CN=IBM JAPAN CA, OU=Local Certificate Authority, O=IBMJ, C=JP
That is, My server certificate ("Yamato server") is signed by our local CA,
but this correct server certificate is rejected by the above additional check
before verifying the certificate.

You can recreate this problem by using a server certificate signed by another CA.
And I haven't seen any IBM customers that use self-signed server certificate.

Comment 9 Martin Oberhuber 2010-06-02 23:47:03 EDT

+1 based on the technical description, and the fact that this is clearly a 
   regression introduced by the fix for bug 264858. The explanation is logical,
   the fix marginal, and just restores the code to a state as it was before the
   fix for bug 264858. The explanation around self-signed certificate helped
   me understand the impact of removing the if() clause.

I have committed and released the change for the I20100603-0700 I-build since the fix is small and understandable, and getting an actual build verified is of larger value here than waiting for Kevin to give his +1 (Kevin, please do so after-the-fact after reviewing the patch).

Noriaki/Dave, please verify that the fix does what you want with I20100603-0700.
Thanks for the Fix!

Comment 10 Noriaki Takatsu 2010-06-04 02:54:11 EDT

Yes, I confirmed that the fix works.


Legal Message:
 I, {Noriaki Takatsu}, declare that I developed the attached code from scratch, 
 without referencing any 3rd party materials except material licensed under 
 the EPL. 
 I am authorized by my employer to make this contribution under the EPL.