From 874a22489e50ed4dec9c21bd154c1bd30e480c90 Mon Sep 17 00:00:00 2001 From: Florian Schmaus Date: Sun, 27 Apr 2014 12:27:12 +0200 Subject: [PATCH] Remove resource binding out of sasl auth Follow XEP-170 recommendations: Resource binding *after* compression. Fixes also a bunch of race conditions related to the wait()/notify() pattern used in the early stages of a Connection where createPacketCollectorAndSend(Packet).nextResultOrThrow() can not be used. Those wait()/notify() patterns are currently - SASL authentication - compression - resource binding Fixes SMACK-454 --- .../jivesoftware/smack/BOSHPacketReader.java | 4 +- .../smack/XMPPBOSHConnection.java | 14 +- .../smack/SASLAuthentication.java | 195 ++++++------------ .../jivesoftware/smack/XMPPConnection.java | 94 ++++++++- .../jivesoftware/smack/DummyConnection.java | 2 +- .../org/jivesoftware/smack/PacketReader.java | 4 +- .../jivesoftware/smack/XMPPTCPConnection.java | 40 ++-- 7 files changed, 188 insertions(+), 165 deletions(-) diff --git a/bosh/src/main/java/org/jivesoftware/smack/BOSHPacketReader.java b/bosh/src/main/java/org/jivesoftware/smack/BOSHPacketReader.java index 201a866fb..56700f296 100644 --- a/bosh/src/main/java/org/jivesoftware/smack/BOSHPacketReader.java +++ b/bosh/src/main/java/org/jivesoftware/smack/BOSHPacketReader.java @@ -148,10 +148,10 @@ public class BOSHPacketReader implements BOSHClientResponseListener { } else if (parser.getName().equals("bind")) { // The server requires the client to bind a resource to the // stream - connection.getSASLAuthentication().bindingRequired(); + connection.serverRequiresBinding(); } else if (parser.getName().equals("session")) { // The server supports sessions - connection.getSASLAuthentication().sessionsSupported(); + connection.serverSupportsSession(); } else if (parser.getName().equals("register")) { AccountManager.getInstance(connection).setSupportsAccountCreation(true); } diff --git a/bosh/src/main/java/org/jivesoftware/smack/XMPPBOSHConnection.java b/bosh/src/main/java/org/jivesoftware/smack/XMPPBOSHConnection.java index 680840405..9608f46a1 100644 --- a/bosh/src/main/java/org/jivesoftware/smack/XMPPBOSHConnection.java +++ b/bosh/src/main/java/org/jivesoftware/smack/XMPPBOSHConnection.java @@ -135,7 +135,8 @@ public class XMPPBOSHConnection extends XMPPConnection { this.config = config; } - public void connect() throws SmackException { + @Override + void connectInternal() throws SmackException { if (connected) { throw new IllegalStateException("Already connected to a server."); } @@ -146,7 +147,6 @@ public class XMPPBOSHConnection extends XMPPConnection { client.close(); client = null; } - saslAuthentication.init(); sessionID = null; authID = null; @@ -250,18 +250,18 @@ public class XMPPBOSHConnection extends XMPPConnection { // Do partial version of nameprep on the username. username = username.toLowerCase(Locale.US).trim(); - String response; if (saslAuthentication.hasNonAnonymousAuthentication()) { // Authenticate using SASL if (password != null) { - response = saslAuthentication.authenticate(username, password, resource); + saslAuthentication.authenticate(username, password, resource); } else { - response = saslAuthentication.authenticate(resource, config.getCallbackHandler()); + saslAuthentication.authenticate(resource, config.getCallbackHandler()); } } else { throw new SaslException("No non-anonymous SASL authentication mechanism available"); } + String response = bindResourceAndEstablishSession(resource); // Set the user. if (response != null) { this.user = response; @@ -303,15 +303,15 @@ public class XMPPBOSHConnection extends XMPPConnection { throw new AlreadyLoggedInException(); } - String response; if (saslAuthentication.hasAnonymousAuthentication()) { - response = saslAuthentication.authenticateAnonymously(); + saslAuthentication.authenticateAnonymously(); } else { // Authenticate using Non-SASL throw new SaslException("No anonymous SASL authentication mechanism available"); } + String response = bindResourceAndEstablishSession(null); // Set the user value. this.user = response; // Update the serviceName with the one returned by the server diff --git a/core/src/main/java/org/jivesoftware/smack/SASLAuthentication.java b/core/src/main/java/org/jivesoftware/smack/SASLAuthentication.java index f582010ef..a5c2a068d 100644 --- a/core/src/main/java/org/jivesoftware/smack/SASLAuthentication.java +++ b/core/src/main/java/org/jivesoftware/smack/SASLAuthentication.java @@ -21,11 +21,16 @@ import org.jivesoftware.smack.SmackException.NoResponseException; import org.jivesoftware.smack.SmackException.NotConnectedException; import org.jivesoftware.smack.SmackException.ResourceBindingNotOfferedException; import org.jivesoftware.smack.XMPPException.XMPPErrorException; -import org.jivesoftware.smack.packet.Bind; import org.jivesoftware.smack.packet.Packet; -import org.jivesoftware.smack.packet.Session; -import org.jivesoftware.smack.sasl.*; +import org.jivesoftware.smack.sasl.SASLAnonymous; +import org.jivesoftware.smack.sasl.SASLCramMD5Mechanism; +import org.jivesoftware.smack.sasl.SASLDigestMD5Mechanism; +import org.jivesoftware.smack.sasl.SASLErrorException; +import org.jivesoftware.smack.sasl.SASLExternalMechanism; +import org.jivesoftware.smack.sasl.SASLGSSAPIMechanism; +import org.jivesoftware.smack.sasl.SASLMechanism; import org.jivesoftware.smack.sasl.SASLMechanism.SASLFailure; +import org.jivesoftware.smack.sasl.SASLPlainMechanism; import javax.security.auth.callback.CallbackHandler; import javax.security.sasl.SaslException; @@ -80,8 +85,6 @@ public class SASLAuthentication { */ private boolean saslNegotiated; - private boolean resourceBinded; - private boolean sessionSupported; /** * The SASL related error condition if there was one provided by the server. */ @@ -211,7 +214,6 @@ public class SASLAuthentication { * * @param resource the desired resource. * @param cbh the CallbackHandler used to get information from the user - * @return the full JID provided by the server while binding a resource to the connection. * @throws IOException * @throws XMPPErrorException * @throws NoResponseException @@ -219,7 +221,7 @@ public class SASLAuthentication { * @throws ResourceBindingNotOfferedException * @throws NotConnectedException */ - public String authenticate(String resource, CallbackHandler cbh) throws IOException, + public void authenticate(String resource, CallbackHandler cbh) throws IOException, NoResponseException, XMPPErrorException, SASLErrorException, ResourceBindingNotOfferedException, NotConnectedException { // Locate the SASLMechanism to use String selectedMechanism = null; @@ -245,20 +247,17 @@ public class SASLAuthentication { e); } - // Trigger SASL authentication with the selected mechanism. We use - // connection.getHost() since GSAPI requires the FQDN of the server, which - // may not match the XMPP domain. - currentMechanism.authenticate(connection.getHost(), cbh); - - // Wait until SASL negotiation finishes synchronized (this) { - if (!saslNegotiated && saslFailure == null) { - try { - wait(30000); - } - catch (InterruptedException e) { - // Ignore - } + // Trigger SASL authentication with the selected mechanism. We use + // connection.getHost() since GSAPI requires the FQDN of the server, which + // may not match the XMPP domain. + currentMechanism.authenticate(connection.getHost(), cbh); + try { + // Wait until SASL negotiation finishes + wait(connection.getPacketReplyTimeout()); + } + catch (InterruptedException e) { + // Ignore } } @@ -268,14 +267,9 @@ public class SASLAuthentication { throw new SASLErrorException(selectedMechanism, saslFailure); } - if (saslNegotiated) { - // Bind a resource for this connection and - return bindResourceAndEstablishSession(resource); - } - else { + if (!saslNegotiated) { throw new NoResponseException(); } - } else { throw new SaslException( @@ -294,14 +288,13 @@ public class SASLAuthentication { * @param username the username that is authenticating with the server. * @param password the password to send to the server. * @param resource the desired resource. - * @return the full JID provided by the server while binding a resource to the connection. * @throws XMPPErrorException * @throws SASLErrorException * @throws IOException * @throws SaslException * @throws SmackException */ - public String authenticate(String username, String password, String resource) + public void authenticate(String username, String password, String resource) throws XMPPErrorException, SASLErrorException, SaslException, IOException, SmackException { // Locate the SASLMechanism to use @@ -326,26 +319,27 @@ public class SASLAuthentication { e); } - // Trigger SASL authentication with the selected mechanism. We use - // connection.getHost() since GSAPI requires the FQDN of the server, which - // may not match the XMPP domain. - - // The serviceName is basically the value that XMPP server sends to the client as being - // the location of the XMPP service we are trying to connect to. This should have the - // format: host ["/" serv-name ] as per RFC-2831 guidelines - String serviceName = connection.getServiceName(); - currentMechanism.authenticate(username, connection.getHost(), serviceName, password); - - // Wait until SASL negotiation finishes synchronized (this) { - if (!saslNegotiated && saslFailure == null) { - try { - wait(30000); - } - catch (InterruptedException e) { - // Ignore - } + // Trigger SASL authentication with the selected mechanism. We use + // connection.getHost() since GSAPI requires the FQDN of the server, which + // may not match the XMPP domain. + + // The serviceName is basically the value that XMPP server sends to the client as + // being + // the location of the XMPP service we are trying to connect to. This should have + // the + // format: host ["/" serv-name ] as per RFC-2831 guidelines + String serviceName = connection.getServiceName(); + currentMechanism.authenticate(username, connection.getHost(), serviceName, password); + + try { + // Wait until SASL negotiation finishes + wait(connection.getPacketReplyTimeout()); } + catch (InterruptedException e) { + // Ignore + } + } if (saslFailure != null) { @@ -354,11 +348,7 @@ public class SASLAuthentication { throw new SASLErrorException(selectedMechanism, saslFailure); } - if (saslNegotiated) { - // Bind a resource for this connection and - return bindResourceAndEstablishSession(resource); - } - else { + if (!saslNegotiated) { throw new NoResponseException(); } } @@ -376,75 +366,36 @@ public class SASLAuthentication { * The server will assign a full JID with a randomly generated resource and possibly with * no username. * - * @return the full JID provided by the server while binding a resource to the connection. * @throws SASLErrorException * @throws IOException * @throws SaslException * @throws XMPPErrorException if an error occures while authenticating. * @throws SmackException if there was no response from the server. */ - public String authenticateAnonymously() throws SASLErrorException, SaslException, IOException, SmackException, XMPPErrorException { - currentMechanism = new SASLAnonymous(this); - currentMechanism.authenticate(null,null,null,""); + public void authenticateAnonymously() throws SASLErrorException, SaslException, IOException, + SmackException, XMPPErrorException { + currentMechanism = new SASLAnonymous(this); - // Wait until SASL negotiation finishes - synchronized (this) { - if (!saslNegotiated && saslFailure == null) { - try { - wait(5000); - } - catch (InterruptedException e) { - // Ignore - } - } - } - - if (saslFailure != null) { - // SASL authentication failed and the server may have closed the connection - // so throw an exception - throw new SASLErrorException(currentMechanism.toString(), saslFailure); - } - - if (saslNegotiated) { - // Bind a resource for this connection and - return bindResourceAndEstablishSession(null); - } - else { - throw new NoResponseException(); - } - } - - private String bindResourceAndEstablishSession(String resource) throws XMPPErrorException, - ResourceBindingNotOfferedException, NoResponseException, NotConnectedException { - // Wait until server sends response containing the element + // Wait until SASL negotiation finishes synchronized (this) { - if (!resourceBinded) { - try { - wait(30000); - } - catch (InterruptedException e) { - // Ignore - } + currentMechanism.authenticate(null, null, null, ""); + try { + wait(connection.getPacketReplyTimeout()); + } + catch (InterruptedException e) { + // Ignore } } - if (!resourceBinded) { - // Server never offered resource binding, which is REQURIED in XMPP client and server - // implementations as per RFC6120 7.2 - throw new ResourceBindingNotOfferedException(); + if (saslFailure != null) { + // SASL authentication failed and the server may have closed the connection + // so throw an exception + throw new SASLErrorException(currentMechanism.toString(), saslFailure); } - Bind bindResource = new Bind(); - bindResource.setResource(resource); - - Bind response = (Bind) connection.createPacketCollectorAndSend(bindResource).nextResultOrThrow(); - String userJID = response.getJid(); - - if (sessionSupported && !connection.getConfiguration().isLegacySessionDisabled()) { - Session session = new Session(); - connection.createPacketCollectorAndSend(session).nextResultOrThrow(); + if (!saslNegotiated) { + throw new NoResponseException(); } - return userJID; } /** @@ -486,10 +437,12 @@ public class SASLAuthentication { * Notification message saying that SASL authentication was successful. The next step * would be to bind the resource. */ - synchronized void authenticated() { + void authenticated() { saslNegotiated = true; // Wake up the thread that is waiting in the #authenticate method - notify(); + synchronized (this) { + notify(); + } } /** @@ -499,34 +452,18 @@ public class SASLAuthentication { * @param saslFailure the SASL failure as reported by the server * @see RFC6120 6.5 */ - synchronized void authenticationFailed(SASLFailure saslFailure) { + void authenticationFailed(SASLFailure saslFailure) { this.saslFailure = saslFailure; // Wake up the thread that is waiting in the #authenticate method - notify(); - } - - /** - * Notification message saying that the server requires the client to bind a - * resource to the stream. - */ - synchronized void bindingRequired() { - resourceBinded = true; - // Wake up the thread that is waiting in the #authenticate method - notify(); + synchronized (this) { + notify(); + } } public void send(Packet stanza) throws NotConnectedException { connection.sendPacket(stanza); } - /** - * Notification message saying that the server supports sessions. When a server supports - * sessions the client needs to send a Session packet after successfully binding a resource - * for the session. - */ - void sessionsSupported() { - sessionSupported = true; - } /** * Initializes the internal state in order to be able to be reused. The authentication @@ -536,7 +473,5 @@ public class SASLAuthentication { protected void init() { saslNegotiated = false; saslFailure = null; - resourceBinded = false; - sessionSupported = false; } } diff --git a/core/src/main/java/org/jivesoftware/smack/XMPPConnection.java b/core/src/main/java/org/jivesoftware/smack/XMPPConnection.java index 25cd0c2db..18bca1197 100644 --- a/core/src/main/java/org/jivesoftware/smack/XMPPConnection.java +++ b/core/src/main/java/org/jivesoftware/smack/XMPPConnection.java @@ -44,13 +44,17 @@ import javax.security.sasl.SaslException; import org.jivesoftware.smack.SmackException.NoResponseException; import org.jivesoftware.smack.SmackException.NotConnectedException; import org.jivesoftware.smack.SmackException.ConnectionException; +import org.jivesoftware.smack.SmackException.ResourceBindingNotOfferedException; +import org.jivesoftware.smack.XMPPException.XMPPErrorException; import org.jivesoftware.smack.compression.XMPPInputOutputStream; import org.jivesoftware.smack.debugger.SmackDebugger; import org.jivesoftware.smack.filter.IQReplyFilter; import org.jivesoftware.smack.filter.PacketFilter; +import org.jivesoftware.smack.packet.Bind; import org.jivesoftware.smack.packet.IQ; import org.jivesoftware.smack.packet.Packet; import org.jivesoftware.smack.packet.Presence; +import org.jivesoftware.smack.packet.Session; /** * The abstract XMPPConnection class provides an interface for connections to a XMPP server and @@ -90,6 +94,7 @@ import org.jivesoftware.smack.packet.Presence; * @author Matt Tucker * @author Guenther Niess */ +@SuppressWarnings("javadoc") public abstract class XMPPConnection { private static final Logger LOGGER = Logger.getLogger(XMPPConnection.class.getName()); @@ -210,6 +215,9 @@ public abstract class XMPPConnection { */ private int port; + private Boolean bindingRequired; + private boolean sessionSupported; + /** * Create an executor to deliver incoming packets to listeners. We'll use a single thread with an unbounded queue. */ @@ -334,17 +342,37 @@ public abstract class XMPPConnection { /** * Establishes a connection to the XMPP server and performs an automatic login * only if the previous connection state was logged (authenticated). It basically - * creates and maintains a connection to the server.

- *

- * Listeners will be preserved from a previous connection if the reconnection - * occurs after an abrupt termination. + * creates and maintains a connection to the server. + *

+ * Listeners will be preserved from a previous connection. * * @throws XMPPException if an error occurs on the XMPP protocol level. - * @throws SmackException if an error occurs somehwere else besides XMPP protocol level. + * @throws SmackException if an error occurs somewhere else besides XMPP protocol level. * @throws IOException * @throws ConnectionException with detailed information about the failed connection. */ - public abstract void connect() throws SmackException, IOException, XMPPException; + public void connect() throws SmackException, IOException, XMPPException { + saslAuthentication.init(); + bindingRequired = false; + sessionSupported = false; + connectInternal(); + } + + /** + * Abstract method that concrete subclasses of XMPPConnection need to implement to perform their + * way of XMPP connection establishment. Implementations must guarantee that this method will + * block until the last features stanzas has been parsed and the features have been reported + * back to XMPPConnection (e.g. by calling @{link {@link XMPPConnection#serverRequiresBinding()} + * and such). + *

+ * Also implementations are required to perform an automatic login if the previous connection + * state was logged (authenticated). + * + * @throws SmackException + * @throws IOException + * @throws XMPPException + */ + abstract void connectInternal() throws SmackException, IOException, XMPPException; /** * Logs in to the server using the strongest authentication mode supported by @@ -418,6 +446,60 @@ public abstract class XMPPConnection { */ public abstract void loginAnonymously() throws XMPPException, SmackException, SaslException, IOException; + /** + * Notification message saying that the server requires the client to bind a + * resource to the stream. + */ + void serverRequiresBinding() { + bindingRequired = true; + // Wake up the thread that is waiting + synchronized(bindingRequired) { + bindingRequired.notify(); + } + } + + /** + * Notification message saying that the server supports sessions. When a server supports + * sessions the client needs to send a Session packet after successfully binding a resource + * for the session. + */ + void serverSupportsSession() { + sessionSupported = true; + } + + String bindResourceAndEstablishSession(String resource) throws XMPPErrorException, + ResourceBindingNotOfferedException, NoResponseException, NotConnectedException { + synchronized (bindingRequired) { + if (!bindingRequired) { + try { + // Wait until server sends response containing the element + bindingRequired.wait(getPacketReplyTimeout()); + } + catch (InterruptedException e) { + // Ignore + } + } + } + + if (!bindingRequired) { + // Server never offered resource binding, which is REQURIED in XMPP client and server + // implementations as per RFC6120 7.2 + throw new ResourceBindingNotOfferedException(); + } + + Bind bindResource = new Bind(); + bindResource.setResource(resource); + + Bind response = (Bind) createPacketCollectorAndSend(bindResource).nextResultOrThrow(); + String userJID = response.getJid(); + + if (sessionSupported && !getConfiguration().isLegacySessionDisabled()) { + Session session = new Session(); + createPacketCollectorAndSend(session).nextResultOrThrow(); + } + return userJID; + } + /** * Sends the specified packet to the server. * diff --git a/core/src/test/java/org/jivesoftware/smack/DummyConnection.java b/core/src/test/java/org/jivesoftware/smack/DummyConnection.java index 30b546482..4776b7d37 100644 --- a/core/src/test/java/org/jivesoftware/smack/DummyConnection.java +++ b/core/src/test/java/org/jivesoftware/smack/DummyConnection.java @@ -75,7 +75,7 @@ public class DummyConnection extends XMPPConnection { } @Override - public void connect() { + void connectInternal() { connectionID = "dummy-" + new Random(new Date().getTime()).nextInt(); if (reconnect) { diff --git a/tcp/src/main/java/org/jivesoftware/smack/PacketReader.java b/tcp/src/main/java/org/jivesoftware/smack/PacketReader.java index 648041f60..87bb8ce77 100644 --- a/tcp/src/main/java/org/jivesoftware/smack/PacketReader.java +++ b/tcp/src/main/java/org/jivesoftware/smack/PacketReader.java @@ -308,7 +308,7 @@ class PacketReader { } else if (parser.getName().equals("bind")) { // The server requires the client to bind a resource to the stream - connection.getSASLAuthentication().bindingRequired(); + connection.serverRequiresBinding(); } // Set the entity caps node for the server if one is send // See http://xmpp.org/extensions/xep-0115.html#stream @@ -325,7 +325,7 @@ class PacketReader { } else if (parser.getName().equals("session")) { // The server supports sessions - connection.getSASLAuthentication().sessionsSupported(); + connection.serverSupportsSession(); } else if (parser.getName().equals("ver")) { if (parser.getNamespace().equals("urn:xmpp:features:rosterver")) { diff --git a/tcp/src/main/java/org/jivesoftware/smack/XMPPTCPConnection.java b/tcp/src/main/java/org/jivesoftware/smack/XMPPTCPConnection.java index dcad1efa3..a7191ef85 100644 --- a/tcp/src/main/java/org/jivesoftware/smack/XMPPTCPConnection.java +++ b/tcp/src/main/java/org/jivesoftware/smack/XMPPTCPConnection.java @@ -108,6 +108,10 @@ public class XMPPTCPConnection extends XMPPConnection { */ private boolean serverAckdCompression = false; + /** + * Lock for the wait()/notify() pattern for the compression negotiation + */ + private final Object compressionLock = new Object(); /** * Creates a new connection to the specified XMPP server. A DNS SRV lookup will be @@ -230,20 +234,26 @@ public class XMPPTCPConnection extends XMPPConnection { // Do partial version of nameprep on the username. username = username.toLowerCase(Locale.US).trim(); - String response; if (saslAuthentication.hasNonAnonymousAuthentication()) { // Authenticate using SASL if (password != null) { - response = saslAuthentication.authenticate(username, password, resource); + saslAuthentication.authenticate(username, password, resource); } else { - response = saslAuthentication.authenticate(resource, config.getCallbackHandler()); + saslAuthentication.authenticate(resource, config.getCallbackHandler()); } } else { throw new SaslException("No non-anonymous SASL authentication mechanism available"); } + // If compression is enabled then request the server to use stream compression. XEP-170 + // recommends to perform stream compression before resource binding. + if (config.isCompressionEnabled()) { + useCompression(); + } + // Set the user. + String response = bindResourceAndEstablishSession(resource); if (response != null) { this.user = response; // Update the serviceName with the one returned by the server @@ -256,11 +266,6 @@ public class XMPPTCPConnection extends XMPPConnection { } } - // If compression is enabled then request the server to use stream compression - if (config.isCompressionEnabled()) { - useCompression(); - } - // Indicate that we're now authenticated. authenticated = true; anonymous = false; @@ -292,14 +297,14 @@ public class XMPPTCPConnection extends XMPPConnection { throw new AlreadyLoggedInException(); } - String response; if (saslAuthentication.hasAnonymousAuthentication()) { - response = saslAuthentication.authenticateAnonymously(); + saslAuthentication.authenticateAnonymously(); } else { throw new SaslException("No anonymous SASL authentication mechanism available"); } + String response = bindResourceAndEstablishSession(null); // Set the user value. this.user = response; // Update the serviceName with the one returned by the server @@ -396,8 +401,6 @@ public class XMPPTCPConnection extends XMPPConnection { reader = null; writer = null; - - saslAuthentication.init(); } public synchronized void disconnect(Presence unavailablePresence) { @@ -786,11 +789,11 @@ public class XMPPTCPConnection extends XMPPConnection { } if ((compressionHandler = maybeGetCompressionHandler()) != null) { - synchronized (this) { + synchronized (compressionLock) { requestStreamCompression(compressionHandler.getCompressionMethod()); // Wait until compression is being used or a timeout happened try { - wait(getPacketReplyTimeout()); + compressionLock.wait(getPacketReplyTimeout()); } catch (InterruptedException e) { // Ignore. @@ -835,8 +838,10 @@ public class XMPPTCPConnection extends XMPPConnection { * Notifies the XMPP connection that stream compression negotiation is done so that the * connection process can proceed. */ - synchronized void streamCompressionNegotiationDone() { - this.notify(); + void streamCompressionNegotiationDone() { + synchronized (compressionLock) { + compressionLock.notify(); + } } /** @@ -851,7 +856,8 @@ public class XMPPTCPConnection extends XMPPConnection { * @throws SmackException * @throws IOException */ - public void connect() throws SmackException, IOException, XMPPException { + @Override + void connectInternal() throws SmackException, IOException, XMPPException { // Establishes the connection, readers and writers connectUsingConfiguration(config); // TODO is there a case where connectUsing.. does not throw an exception but connected is