From bfc14227caba62de16de59fef8a09dbbdb2ec10e Mon Sep 17 00:00:00 2001 From: Florian Schmaus Date: Fri, 6 Jan 2017 15:03:28 +0100 Subject: [PATCH] Propagate stream errors on connect/login to the caller Before this, if there was a stream error response by the server to our stream open, that error response would only be handled in the reader thread, and the user would get a message like: "org.jivesoftware.smack.SmackException$NoResponseException: No response received within reply timeout. Timeout was 5000ms (~5s). While waiting for SASL mechanisms stream feature from server" while the server may actually sent something like Too many (2) failed authentications from this IP address (1xx.66.xx.xxx). The address will be unblocked at 04:24:00 06.01.2017 UTC It was necessary to change saslFeatureReceived from SmackException to XMPPException in order to return the StreamErrorException at this sync point. But this change in return required the introduction of a tlsHandled sync point for SmackException (which just acts as a wrapper for the various exception types that could occurn when establishing TLS). The tlsHandled sync point is marked successful even if no TLS was established in case none was required and/or if not supported by the server. --- .../smack/bosh/XMPPBOSHConnection.java | 3 +++ .../smack/AbstractXMPPConnection.java | 11 ++++++++-- .../jivesoftware/smack/DummyConnection.java | 1 + .../smack/tcp/XMPPTCPConnection.java | 22 ++++++++++++++----- 4 files changed, 30 insertions(+), 7 deletions(-) diff --git a/smack-bosh/src/main/java/org/jivesoftware/smack/bosh/XMPPBOSHConnection.java b/smack-bosh/src/main/java/org/jivesoftware/smack/bosh/XMPPBOSHConnection.java index 1e48c72c4..62e5d34e0 100644 --- a/smack-bosh/src/main/java/org/jivesoftware/smack/bosh/XMPPBOSHConnection.java +++ b/smack-bosh/src/main/java/org/jivesoftware/smack/bosh/XMPPBOSHConnection.java @@ -198,6 +198,9 @@ public class XMPPBOSHConnection extends AbstractXMPPConnection { + getHost() + ":" + getPort() + "."; throw new SmackException(errorMessage); } + + tlsHandled.reportSuccess(); + saslFeatureReceived.reportSuccess(); } public boolean isSecureConnection() { diff --git a/smack-core/src/main/java/org/jivesoftware/smack/AbstractXMPPConnection.java b/smack-core/src/main/java/org/jivesoftware/smack/AbstractXMPPConnection.java index 18bf784bf..2a714d2d1 100644 --- a/smack-core/src/main/java/org/jivesoftware/smack/AbstractXMPPConnection.java +++ b/smack-core/src/main/java/org/jivesoftware/smack/AbstractXMPPConnection.java @@ -188,6 +188,8 @@ public abstract class AbstractXMPPConnection implements XMPPConnection { */ protected Writer writer; + protected final SynchronizationPoint tlsHandled = new SynchronizationPoint<>(this, "establishing TLS"); + /** * Set to success if the last features stanza from the server has been parsed. A XMPP connection * handshake can invoke multiple features stanzas, e.g. when TLS is activated a second feature @@ -198,9 +200,9 @@ public abstract class AbstractXMPPConnection implements XMPPConnection { AbstractXMPPConnection.this, "last stream features received from server"); /** - * Set to success if the sasl feature has been received. + * Set to success if the SASL feature has been received. */ - protected final SynchronizationPoint saslFeatureReceived = new SynchronizationPoint( + protected final SynchronizationPoint saslFeatureReceived = new SynchronizationPoint<>( AbstractXMPPConnection.this, "SASL mechanisms stream feature from server"); /** @@ -368,11 +370,15 @@ public abstract class AbstractXMPPConnection implements XMPPConnection { saslAuthentication.init(); saslFeatureReceived.init(); lastFeaturesReceived.init(); + tlsHandled.init(); streamId = null; // Perform the actual connection to the XMPP service connectInternal(); + // TLS handled will be successful either if TLS was established, or if it was not mandatory. + tlsHandled.checkIfSuccessOrWaitOrThrow(); + // Wait with SASL auth until the SASL mechanisms have been received saslFeatureReceived.checkIfSuccessOrWaitOrThrow(); @@ -1407,6 +1413,7 @@ public abstract class AbstractXMPPConnection implements XMPPConnection { // Only proceed with SASL auth if TLS is disabled or if the server doesn't announce it if (!hasFeature(StartTls.ELEMENT, StartTls.NAMESPACE) || config.getSecurityMode() == SecurityMode.disabled) { + tlsHandled.reportSuccess(); saslFeatureReceived.reportSuccess(); } } diff --git a/smack-core/src/test/java/org/jivesoftware/smack/DummyConnection.java b/smack-core/src/test/java/org/jivesoftware/smack/DummyConnection.java index 851311b3a..1bd11e119 100644 --- a/smack-core/src/test/java/org/jivesoftware/smack/DummyConnection.java +++ b/smack-core/src/test/java/org/jivesoftware/smack/DummyConnection.java @@ -91,6 +91,7 @@ public class DummyConnection extends AbstractXMPPConnection { protected void connectInternal() { connected = true; saslFeatureReceived.reportSuccess(); + tlsHandled.reportSuccess(); streamId = "dummy-" + new Random(new Date().getTime()).nextInt(); if (reconnect) { diff --git a/smack-tcp/src/main/java/org/jivesoftware/smack/tcp/XMPPTCPConnection.java b/smack-tcp/src/main/java/org/jivesoftware/smack/tcp/XMPPTCPConnection.java index 2080c100a..4d67ad567 100644 --- a/smack-tcp/src/main/java/org/jivesoftware/smack/tcp/XMPPTCPConnection.java +++ b/smack-tcp/src/main/java/org/jivesoftware/smack/tcp/XMPPTCPConnection.java @@ -46,6 +46,7 @@ import org.jivesoftware.smack.packet.StreamOpen; import org.jivesoftware.smack.packet.Stanza; import org.jivesoftware.smack.packet.Presence; import org.jivesoftware.smack.packet.StartTls; +import org.jivesoftware.smack.packet.StreamError; import org.jivesoftware.smack.sasl.packet.SaslStreamElements; import org.jivesoftware.smack.sasl.packet.SaslStreamElements.Challenge; import org.jivesoftware.smack.sasl.packet.SaslStreamElements.SASLFailure; @@ -923,13 +924,19 @@ public class XMPPTCPConnection extends AbstractXMPPConnection { StartTls startTlsFeature = getFeature(StartTls.ELEMENT, StartTls.NAMESPACE); if (startTlsFeature != null) { if (startTlsFeature.required() && config.getSecurityMode() == SecurityMode.disabled) { - notifyConnectionError(new SecurityRequiredByServerException()); + SmackException smackException = new SecurityRequiredByServerException(); + tlsHandled.reportFailure(smackException); + notifyConnectionError(smackException); return; } if (config.getSecurityMode() != ConnectionConfiguration.SecurityMode.disabled) { sendNonza(new StartTls()); + } else { + tlsHandled.reportSuccess(); } + } else { + tlsHandled.reportSuccess(); } if (getSASLAuthentication().authenticationSuccessful()) { @@ -1028,7 +1035,13 @@ public class XMPPTCPConnection extends AbstractXMPPConnection { } break; case "error": - throw new StreamErrorException(PacketParserUtils.parseStreamError(parser)); + StreamError streamError = PacketParserUtils.parseStreamError(parser); + saslFeatureReceived.reportFailure(new StreamErrorException(streamError)); + // Mark the tlsHandled sync point as success, we will use the saslFeatureReceived sync + // point to report the error, which is checked immediately after tlsHandled in + // connectInternal(). + tlsHandled.reportSuccess(); + throw new StreamErrorException(streamError); case "features": parseFeatures(parser); break; @@ -1040,9 +1053,8 @@ public class XMPPTCPConnection extends AbstractXMPPConnection { openStream(); } catch (Exception e) { - // We report any failure regarding TLS in the second stage of XMPP - // connection establishment, namely the SASL authentication - saslFeatureReceived.reportFailure(new SmackException(e)); + SmackException smackException = new SmackException(e); + tlsHandled.reportFailure(smackException); throw e; } break;