Bug 341724 - [xmpp] XMPPS connection fails on handshake if server requires client SSL certificate
Summary: [xmpp] XMPPS connection fails on handshake if server requires client SSL cert...
Status: NEW
Alias: None
Product: ECF
Classification: RT
Component: ecf.providers (show other bugs)
Version: unspecified   Edit
Hardware: PC All
: P3 enhancement (vote)
Target Milestone: ---   Edit
Assignee: ecf.core-inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-03 14:20 EDT by Teddy Yueh CLA
Modified: 2011-04-22 13:46 EDT (History)
1 user (show)

See Also:


Attachments
Suggested patch for issue 1/2 (22.96 KB, patch)
2011-04-08 21:30 EDT, Teddy Yueh CLA
slewis: iplog+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Teddy Yueh CLA 2011-04-03 14:20:05 EDT
Build Identifier: 

If the server that is being connected to requires a client SSL certificate, the following exception is thrown:

When connecting, I get the following error:

javax.net.ssl.SSLProtocolException: Handshake message sequence violation, 2
	at com.sun.net.ssl.internal.ssl.ClientHandshaker.processMessage(Unknown Source)
	at com.sun.net.ssl.internal.ssl.Handshaker.processLoop(Unknown Source)
	at com.sun.net.ssl.internal.ssl.Handshaker.process_record(Unknown Source)
	at com.sun.net.ssl.internal.ssl.SSLSocketImpl.readRecord(Unknown Source)
	at com.sun.net.ssl.internal.ssl.SSLSocketImpl.performInitialHandshake(Unknown Source)
	at com.sun.net.ssl.internal.ssl.SSLSocketImpl.startHandshake(Unknown Source)
	at com.sun.net.ssl.internal.ssl.SSLSocketImpl.startHandshake(Unknown Source)
	at org.jivesoftware.smack.XMPPConnection.proceedTLSReceived(XMPPConnection.java:1258)

This is because the XMPPSContainer currently does not have any way to provide a CallbackHandler to XmppConnection.

Further, there is a bug in the proceedTLSReceived method in XmppConnection. The catch-all implementation for keystores of type other than PKCS11 and Apple are handled backwards:

        else {
                ks = KeyStore.getInstance(configuration.getKeystoreType());
                try {
                    pcb = new PasswordCallback("Keystore Password: ",false);
                    ks.load(new     
        FileInputStream(configuration.getKeystorePath()), pcb.getPassword());
                    callbackHandler.handle(new Callback[]{pcb});
                }
                catch(Exception e) {
                    ks = null;
                    pcb = null;
                }
            }

After searching on Ignite forums, I discovered this has already been documented:

    http://issues.igniterealtime.org/browse/SMACK-322

Unfortunately, it looks like it's been over 1/2 a year and it has not been resolved.

Reproducible: Always

Steps to Reproduce:
(I used Java Key Store (JKS))

1. Configure any server (i.e. OpenFire) to require client certificate for authentication.
2. Do set up work to sign server cert on client system and vice versa.
3. Use the XMPPSContainer to connect (with or without system property  javax.net.ssl.keyStore set) to the server.
4. Kaboom!
Comment 1 Scott Lewis CLA 2011-04-03 14:38:43 EDT
Thanks for the report.

Does this bug identify two different issues?  As I understand it, the first one is an ECF issue...i.e.:

>This is because the XMPPSContainer currently does not have any way to provide a
>CallbackHandler to XmppConnection.

This could be dealt with by adding such support to ECFConnection...and the appropriate way might be to add to the xmpp provider the means to use the ECF IConnectContext callback to get the certificate (rather than just get password authentication information as it does now).

But the second issue seems to be a problem with Smack itself (in XmppConnection)...i.e.:  

>After searching on Ignite forums, I discovered this has already been
>documented:
>
>    http://issues.igniterealtime.org/browse/SMACK-322
>
>Unfortunately, it looks like it's been over 1/2 a year and it has not been
>resolved.

Although if presented with a patch we could fix this in our copy of Smack, it would be much better if it was fixed upstream/in Smack...and we moved to a more recent version (the Smack version we are currently using is kind of old at this point...i.e. 3.1.0).  

But in any case...if my analysis is correct, and these are two separate issues, I would suggest that this bug be focused on the first one (which we/ECF can immediately do something about) and the second one either result in a new bug for Smack/upstream, and/or if necessary a second bug for ECF...so that these can be tracked separately.
Comment 2 Teddy Yueh CLA 2011-04-03 15:01:10 EDT
I'll continue all replies here instead of the forum (http://www.eclipse.org/forums/index.php?t=msg&goto=663117&S=1c1ff39551ff44ea91f2277178734c70) for tracking purposes.

I agree that it is 2 separate issues. But to my understanding, both need to be solved before ECF can provide certs to the server. I like your idea of using the IConnectContext callback to get the certificate to encapsulate the issue in ECF, but looking at smack's XmppConnection, I don't know how to provide that certificate to where it needs to be.

XmppConnection also expects to be the one retrieving all certificates. This is evident in that they require all CallbackHandlers provided to handle PasswordCallback and then use the obtained password to load the keystore. I've only learned about SSL a couple days ago while trying to get this to work, so I'm probably missing key knowledge to know how to implement your suggestion.

But it seems to me that providing the CallbackHandler obtained via IConnectContext to obtain the password should be all the the ECF XMPPSContainer should be doing. Then it's up to XmppConnection to do the rest.

Thanks for your input on this, Scott!
Comment 3 Scott Lewis CLA 2011-04-03 15:13:53 EDT
(In reply to comment #2)
> I'll continue all replies here instead of the forum
> (http://www.eclipse.org/forums/index.php?t=msg&goto=663117&S=1c1ff39551ff44ea91f2277178734c70)
> for tracking purposes.

Ok.

> 
> I agree that it is 2 separate issues. But to my understanding, both need to be
> solved before ECF can provide certs to the server. I like your idea of using
> the IConnectContext callback to get the certificate to encapsulate the issue in
> ECF, but looking at smack's XmppConnection, I don't know how to provide that
> certificate to where it needs to be.
> 
> XmppConnection also expects to be the one retrieving all certificates. This is
> evident in that they require all CallbackHandlers provided to handle
> PasswordCallback and then use the obtained password to load the keystore. I've
> only learned about SSL a couple days ago while trying to get this to work, so
> I'm probably missing key knowledge to know how to implement your suggestion.

I don't know the internals of XmppConnection very well, so unfortunately I can't be of much immediate help with this.  I suggest that you bring up the Smack/XmppConnection bug issue on ecf-dev mailing list, however, as I think there are others (committers, contributors, and ECF community members) that know more about Smack and SSL authentication...and the internals of XmppConnection bugs/issues than I do.  For example, there was a community member named Bjorn that has worked more with Smack than I have...and they identified other bugs (on ecf-dev mailing list) in Smack itself that they also would like to have fixed in Smack upstream.  

If I approach Matt Tucker with proposed fixes to Smack, it would be nice if we had a number of them all ready to go so that Smack could make all the necessary fixes.

> 
> But it seems to me that providing the CallbackHandler obtained via
> IConnectContext to obtain the password should be all the the ECF XMPPSContainer
> should be doing. Then it's up to XmppConnection to do the rest.

That sounds good/right to me.  If more support/addition for the XMPPSContainer is needed (and/or other callback handlers then please let me know.

And...let all know on ecf-dev mailing list, as this will make it easier/quicker to use all the knowledge in the community (about ECF, Smack, SSL, etc).

> 
> Thanks for your input on this, Scott!

No problem.  Please just help me push these fixes through...to Smack...to ECF...etc...and then hopefully ECF's xmpp provider will better meet your...and other community members'...needs.
Comment 4 Teddy Yueh CLA 2011-04-03 15:24:52 EDT
I'm doing more digging around and the smack issue may have been addressed in another issue:

    http://issues.igniterealtime.org/browse/SMACK-234

The code repo shows that XmppConnection is now fixed. So perhaps the thing to do is just to get a new jar of smack and update XMPPSContainer as discussed before.

I won't have any more time today to work on this, but will dive back in tomorrow. I'll start with polling the mailing-list.

Enjoy what's left of the weekend!

Thanks,
Teddy
Comment 5 Scott Lewis CLA 2011-04-03 15:37:08 EDT
(In reply to comment #4)
> I'm doing more digging around and the smack issue may have been addressed in
> another issue:
> 
>     http://issues.igniterealtime.org/browse/SMACK-234
> 
> The code repo shows that XmppConnection is now fixed. So perhaps the thing to
> do is just to get a new jar of smack and update XMPPSContainer as discussed
> before.
> 
> I won't have any more time today to work on this, but will dive back in
> tomorrow. I'll start with polling the mailing-list.

Ok.  Another issue to consider...once we get a version of Smack that has this and/or other fixes we need to get this new version of Smack through the EF IP review process before we can distribute it with next release of ECF.

We'll have to figure out how this can be done the most quickly.

> 
> Enjoy what's left of the weekend!
> 
> Thanks,
> Teddy

Thanks...you too.  Note...please ping me directly via slewis at composent.com if I seem to lose focus on this as time goes by...as I've got a lot of other things pending (both ECF and other).
Comment 6 Scott Lewis CLA 2011-04-04 14:08:13 EDT
(In reply to comment #4)
> I'm doing more digging around and the smack issue may have been addressed in
> another issue:
> 
>     http://issues.igniterealtime.org/browse/SMACK-234
> 
> The code repo shows that XmppConnection is now fixed. So perhaps the thing to
> do is just to get a new jar of smack and update XMPPSContainer as discussed
> before.

Teddy.  Has this fixed version of XmppConnection been part of an official Smack release?  If so, what versions?

If not, we'll need to figure out how to handle this, as the ip process at EF may make it difficult to incorporate fixes upstream (i.e. in XmppConnection) that have not been released by Jive yet.

Another thing we could do...I could talk with Matt Tucker about the Smack release schedule, and see if there is a pending release...and if there is not then encourage one :).
Comment 7 Teddy Yueh CLA 2011-04-04 15:25:39 EDT
Scott,

I downloaded the newest jars from http://www.igniterealtime.org/projects/smack/ and the fix is in the jar set.

As far as it being an "official" release, it's listed as beta. But then again, version 3.1.0 (the version ECF is using) is still listed as beta.

I've tapped the mailing-list. No replies yet, but this seems more and more straight-forward now.

Teddy
Comment 8 Teddy Yueh CLA 2011-04-05 13:25:36 EDT
I've run into a snag. The CallbackHandler expected by smack is in the package, javax.security.auth.callback, where as the one provided by the IConnectContext is in org.eclipse.ecf.core.security.

I'm also not sure whether authenticating via an SSL certificate removes the need for a user password. In other words, can the password stored in the IConnectContext be the keystore password? If so, I can simply wrap the ECF CallbackHandler and have it be a delegate. However, I'm hesitant to put such a wrapper in the org.eclipse.ecf.provier.xmpp project.

I believe the better way to do it, though, is simply provide an SWT dialog for the user to provide the keystore password, but again, am hesitant about adding it to org.eclipse.ecf.provier.xmpp instead of org.eclipse.ecf.provier.xmpp.ui. But if I do the latter, I'd create a cyclical dependency between xmpp and xmpp.ui d-_-b.

In my limited understanding of ECF code, it seems like either route would be best placed in org.eclipse.ecf.ssl or something, which I'm surprised is not involved in XMPPS in the first place.

I'll proceed with the wrapping of CallbackHandler approach unless someone else has a better idea.

-Teddy
Comment 9 Scott Lewis CLA 2011-04-05 13:36:48 EDT
(In reply to comment #8)
> I've run into a snag. The CallbackHandler expected by smack is in the package,
> javax.security.auth.callback, where as the one provided by the IConnectContext
> is in org.eclipse.ecf.core.security.

So just for reference...the reason this is the case is that when ECF was initially introduced, we could not introduce a dependency on javax.security.auth.callback...so essentially we had to have our own CallbackHandler.


> 
> I'm also not sure whether authenticating via an SSL certificate removes the
> need for a user password. In other words, can the password stored in the
> IConnectContext be the keystore password? If so, I can simply wrap the ECF
> CallbackHandler and have it be a delegate. However, I'm hesitant to put such a
> wrapper in the org.eclipse.ecf.provier.xmpp project.
> 
> I believe the better way to do it, though, is simply provide an SWT dialog for
> the user to provide the keystore password, but again, am hesitant about adding
> it to org.eclipse.ecf.provier.xmpp instead of org.eclipse.ecf.provier.xmpp.ui.
> But if I do the latter, I'd create a cyclical dependency between xmpp and
> xmpp.ui d-_-b.
> 
> In my limited understanding of ECF code, it seems like either route would be
> best placed in org.eclipse.ecf.ssl or something, which I'm surprised is not
> involved in XMPPS in the first place.
> 
> I'll proceed with the wrapping of CallbackHandler approach unless someone else
> has a better idea.

Given the org.eclipse.ecf.core.security.ObjectCallback class it should be possible to wrap javax.  I haven't thought it through, but if you want assistance with this I will try to provide it.  Feel free to contact me at slewis at composent.com.

If it becomes necessary, we could introduce new classes to org.eclipse.ecf.ssl fragment (perhaps in split package) but this would be a significant API addition, and require us to go through a minor release, release review, etc., etc.  So there are good practical reasons to avoid it.
Comment 10 Teddy Yueh CLA 2011-04-06 20:02:01 EDT
Submitted patch for issue 1 to Scott. Issue 2 pending direction.
Comment 11 Teddy Yueh CLA 2011-04-08 21:30:25 EDT
Created attachment 192883 [details]
Suggested patch for issue 1/2

Patch will have noticeable affect until org.jivesoftware.smack code is updated.
Comment 12 Teddy Yueh CLA 2011-04-08 21:31:23 EDT
(In reply to comment #11)
> Created attachment 192883 [details]
> Suggested patch for issue 1/2
> 
> Patch will have noticeable affect until org.jivesoftware.smack code is updated.

Ugh. Patch will NOT have not have noticeable affect until org.jivesoftware.smack code is updated.
Comment 13 Scott Lewis CLA 2011-04-22 13:45:44 EDT
(In reply to comment #11)
> Created attachment 192883 [details]
> Suggested patch for issue 1/2
> 
> Patch will have noticeable affect until org.jivesoftware.smack code is updated.

Ok.  After reviewing, testing for regression, and making only minor changes, I've pushed the changes to the ECF code to master:

http://git.eclipse.org/c/ecf/org.eclipse.ecf.git/commit/?id=b59513f0a384f990eda2a83283511f136be9bf2c

As Teddy says in comment 12, however, this change will only have noticeable effect after the smack library code is updated.  This will involve some process (for IP review, etc) and the timing and mechanism for doing this has to be discussed offline...to figure out what to do, with what version of smack, and when.