Re-worked security settings, clean-up of connection config, fixed concurrency when shutting down the packet writer.

git-svn-id: http://svn.igniterealtime.org/svn/repos/smack/trunk@6666 b35dd754-fafc-0310-a699-88a17e54d16e

source/org/jivesoftware/smack/ConnectionConfiguration.java

@@ -30,7 +30,7 @@ import java.io.File;
  * configure the path to the trustore file that keeps the trusted CA root certificates and
  * enable or disable all or some of the checkings done while verifying server certificates.<p>
  *
- * It is also possible to configure it TLs, SASL or compression are going to be used or not.
+ * It is also possible to configure if TLS, SASL, and compression are used or not.
  *
  * @author Gaston Dombiak
  */
@@ -44,7 +44,6 @@ public class ConnectionConfiguration implements Cloneable {
     private String truststorePath;
     private String truststoreType;
     private String truststorePassword;
-    private boolean tlsEnabled = true;
     private boolean verifyChainEnabled = false;
     private boolean verifyRootCAEnabled = false;
     private boolean selfSignedCertificateEnabled = false;
@@ -68,6 +67,7 @@ public class ConnectionConfiguration implements Cloneable {
     private String password;
     private String resource;
     private boolean sendPresence;
+    private SecurityMode securityMode = SecurityMode.enabled;
 
     /**
      * Creates a new ConnectionConfiguration for the specified service name.
@@ -118,7 +118,7 @@ public class ConnectionConfiguration implements Cloneable {
 
         // Build the default path to the cacert truststore file. By default we are
         // going to use the file located in $JREHOME/lib/security/cacerts.
-        String javaHome =  System.getProperty("java.home");
+        String javaHome = System.getProperty("java.home");
         StringBuilder buffer = new StringBuilder();
         buffer.append(javaHome).append(File.separator).append("lib");
         buffer.append(File.separator).append("security");
@@ -141,7 +141,8 @@ public class ConnectionConfiguration implements Cloneable {
 
     /**
      * Returns the host to use when establishing the connection. The host and port to use
-     * might have been resolved by a DNS lookup as specified by the XMPP spec.
+     * might have been resolved by a DNS lookup as specified by the XMPP spec (and therefore
+     * may not match the {@link #getServiceName service name}.
      *
      * @return the host to use when establishing the connection.
      */
@@ -160,30 +161,28 @@ public class ConnectionConfiguration implements Cloneable {
     }
 
     /**
-     * Returns true if the client is going to try to secure the connection using TLS after
+     * Returns the TLS security mode used when making the connection. By default,
-     * the connection has been established.
+     * the mode is {@link SecurityMode#enabled}.
      *
-     * @return true if the client is going to try to secure the connection using TLS after
+     * @return the security mode.
-     *         the connection has been established.
      */
-    public boolean isTLSEnabled() {
+    public SecurityMode getSecurityMode() {
-        return tlsEnabled;
+        return securityMode;
     }
 
     /**
-     * Sets if the client is going to try to secure the connection using TLS after
+     * Sets the TLS security mode used when making the connection. By default,
-     * the connection has been established.
+     * the mode is {@link SecurityMode#enabled}.
      *
-     * @param tlsEnabled if the client is going to try to secure the connection using TLS after
+     * @param securityMode the security mode.
-     *         the connection has been established.
      */
-    public void setTLSEnabled(boolean tlsEnabled) {
+    public void setSecurityMode(SecurityMode securityMode) {
-        this.tlsEnabled = tlsEnabled;
+        this.securityMode = securityMode;
     }
 
     /**
-     * Retuns the path to the truststore file. The truststore file contains the root
+     * Retuns the path to the trust store file. The trust store file contains the root
-     * certificates of several well?known CAs. By default Smack is going to use
+     * certificates of several well known CAs. By default, will attempt to use the
      * the file located in $JREHOME/lib/security/cacerts.
      *
      * @return the path to the truststore file.
@@ -193,7 +192,7 @@ public class ConnectionConfiguration implements Cloneable {
     }
 
     /**
-     * Sets the path to the truststore file. The truststore file contains the root
+     * Sets the path to the trust store file. The truststore file contains the root
      * certificates of several well?known CAs. By default Smack is going to use
      * the file located in $JREHOME/lib/security/cacerts.
      *
@@ -203,17 +202,27 @@ public class ConnectionConfiguration implements Cloneable {
         this.truststorePath = truststorePath;
     }
 
+    /**
+     * Returns the trust store type, or <tt>null</tt> if it's not set.
+     *
+     * @return the trust store type.
+     */
     public String getTruststoreType() {
         return truststoreType;
     }
 
+    /**
+     * Sets the trust store type.
+     *
+     * @param truststoreType the trust store type.
+     */
     public void setTruststoreType(String truststoreType) {
         this.truststoreType = truststoreType;
     }
 
     /**
-     * Returns the password to use to access the truststore file. It is assumed that all
+     * Returns the password to use to access the trust store file. It is assumed that all
-     * certificates share the same password of the truststore file.
+     * certificates share the same password in the trust store.
      *
      * @return the password to use to access the truststore file.
      */
@@ -222,9 +231,8 @@ public class ConnectionConfiguration implements Cloneable {
     }
 
     /**
-     * Sets the password to use to access the truststore file. It is assumed that all
+     * Sets the password to use to access the trust store file. It is assumed that all
-     * certificates share the same password of the truststore file.
+     * certificates share the same password in the trust store.
-     *
      *
      * @param truststorePassword the password to use to access the truststore file.
      */
@@ -373,9 +381,9 @@ public class ConnectionConfiguration implements Cloneable {
     }
 
     /**
-     * Sets if the client is going to use SASL authentication when logging into the
+     * Sets whether the client will use SASL authentication when logging into the
      * server. If SASL authenticatin fails then the client will try to use non-sasl authentication.
-     * By default SASL is enabled.
+     * By default, SASL is enabled.
      *
      * @param saslAuthenticationEnabled if the client is going to use SASL authentication when
      *        logging into the server.
@@ -404,10 +412,6 @@ public class ConnectionConfiguration implements Cloneable {
         this.debuggerEnabled = debuggerEnabled;
     }
     
-    protected Object clone() throws CloneNotSupportedException {
-        return super.clone();
-    }
-    
     /**
      * Sets if the reconnection mechanism is allowed to be used. By default
      * reconnection is allowed.
@@ -429,7 +433,7 @@ public class ConnectionConfiguration implements Cloneable {
     
     /**
      * Sets the socket factory used to create new xmppConnection sockets.
-     * This is mainly used when reconnection is necessary.
+     * This is useful when connecting through SOCKS5 proxies.
      *
      * @param socketFactory used to create new sockets.
      */
@@ -437,10 +441,9 @@ public class ConnectionConfiguration implements Cloneable {
         this.socketFactory = socketFactory;
     }
 
-    
     /**
      * Returns the socket factory used to create new xmppConnection sockets.
-     * This is mainly used when reconnection is necessary.
+     * This is useful when connecting through SOCKS5 proxies.
      * 
      * @return socketFactory used to create new sockets.
      */
@@ -448,12 +451,31 @@ public class ConnectionConfiguration implements Cloneable {
         return this.socketFactory;
     }
 
-    protected void setLoginInfo(String username, String password, String resource,
+    /**
-            boolean sendPresence) {
+     * An enumeration for TLS security modes that are available when making a connection
-        this.username = username;
+     * to the XMPP server.
-        this.password = password;
+     */
-        this.resource = resource;
+    public static enum SecurityMode {
-        this.sendPresence = sendPresence;
+
+        /**
+         * Securirty via TLS encryption is required in order to connect. If the server
+         * does not offer TLS or if the TLS negotiaton fails, the connection to the server
+         * will fail.
+         */
+        required,
+
+        /**
+         * Security via TLS encryption is used whenever it's available. This is the
+         * default setting.
+         */
+        enabled,
+
+        /**
+         * Security via TLS encryption is disabled and only un-encrypted connections will
+         * be used. If only TLS encryption is available from the server, the connection
+         * will fail.
+         */
+        disabled
     }
 
     /**
@@ -461,7 +483,7 @@ public class ConnectionConfiguration implements Cloneable {
      *
      * @return the username to use when trying to reconnect to the server.
      */
-    protected String getUsername() {
+    String getUsername() {
         return this.username;
     }
 
@@ -470,17 +492,16 @@ public class ConnectionConfiguration implements Cloneable {
      *
      * @return the password to use when trying to reconnect to the server.
      */
-    protected String getPassword() {
+    String getPassword() {
         return this.password;
     }
 
-
     /**
      * Returns the resource to use when trying to reconnect to the server.
      *
      * @return the resource to use when trying to reconnect to the server.
      */
-    protected String getResource() {
+    String getResource() {
         return resource;
     }
 
@@ -489,7 +510,14 @@ public class ConnectionConfiguration implements Cloneable {
      *
      * @return true if an available presence should be sent when logging in while reconnecting
      */
-    protected boolean isSendPresence() {
+    boolean isSendPresence() {
         return sendPresence;
     }
+
+    void setLoginInfo(String username, String password, String resource, boolean sendPresence) {
+        this.username = username;
+        this.password = password;
+        this.resource = resource;
+        this.sendPresence = sendPresence;
+    }
 }

source/org/jivesoftware/smack/PacketReader.java

@@ -466,6 +466,7 @@ class PacketReader {
 
     private void parseFeatures(XmlPullParser parser) throws Exception {
         boolean startTLSReceived = false;
+        boolean startTLSRequired = false;
         boolean done = false;
         while (!done) {
             int eventType = parser.next();
@@ -473,8 +474,6 @@ class PacketReader {
             if (eventType == XmlPullParser.START_TAG) {
                 if (parser.getName().equals("starttls")) {
                     startTLSReceived = true;
-                    // Confirm the server that we want to use TLS
-                    connection.startTLSReceived();
                 }
                 else if (parser.getName().equals("mechanisms")) {
                     // The server is reporting available SASL mechanisms. Store this information
@@ -500,13 +499,36 @@ class PacketReader {
                 }
             }
             else if (eventType == XmlPullParser.END_TAG) {
-                if (parser.getName().equals("features")) {
+                if (parser.getName().equals("starttls")) {
+                    // Confirm the server that we want to use TLS
+                    connection.startTLSReceived(startTLSRequired);
+                }
+                else if (parser.getName().equals("required") && startTLSReceived) {
+                    startTLSRequired = true;
+                }
+                else if (parser.getName().equals("features")) {
                     done = true;
                 }
             }
         }
+
+        // If TLS is required but the server doesn't offer it, disconnect
+        // from the server and throw an error. First check if we've already negotiated TLS
+        // and are secure, however (features get parsed a second time after TLS is established).
+        if (!connection.isSecureConnection()) {
+            if (!startTLSReceived && connection.getConfiguration().getSecurityMode() ==
+                    ConnectionConfiguration.SecurityMode.required)
+            {
+                throw new XMPPException("Server does not support security (TLS), " +
+                        "but security required by connection configuration.",
+                        new XMPPError(XMPPError.Condition.forbidden));
+            }
+        }
+        
         // Release the lock after TLS has been negotiated or we are not insterested in TLS
-        if (!startTLSReceived || !connection.getConfiguration().isTLSEnabled()) {
+        if (!startTLSReceived || connection.getConfiguration().getSecurityMode() ==
+                ConnectionConfiguration.SecurityMode.disabled)
+        {
             releaseConnectionIDLock();
         }
     }

source/org/jivesoftware/smack/PacketWriter.java

@@ -26,8 +26,9 @@ import org.jivesoftware.smack.packet.Packet;
 import java.io.IOException;
 import java.io.Writer;
 import java.util.ArrayList;
-import java.util.LinkedList;
 import java.util.List;
+import java.util.Queue;
+import java.util.concurrent.ConcurrentLinkedQueue;
 
 /**
  * Writes packets to a XMPP server. Packets are sent using a dedicated thread. Packet
@@ -42,7 +43,7 @@ class PacketWriter {
     private Thread keepAliveThread;
     private Writer writer;
     private XMPPConnection connection;
-    final private LinkedList<Packet> queue;
+    final private Queue<Packet> queue;
     private boolean done;
     
     final protected List<ListenerWrapper> listeners = new ArrayList<ListenerWrapper>();
@@ -71,7 +72,7 @@ class PacketWriter {
      * @param connection the connection.
      */
     protected PacketWriter(XMPPConnection connection) {
-        this.queue = new LinkedList<Packet>();
+        this.queue = new ConcurrentLinkedQueue<Packet>();
         this.connection = connection;
         init();
     }
@@ -106,8 +107,8 @@ class PacketWriter {
             // may modify the content of the packet.
             processInterceptors(packet);
 
-            synchronized(queue) {
+            queue.add(packet);
-                queue.addFirst(packet);
+            synchronized (queue) {
                 queue.notifyAll();
             }
 
@@ -236,6 +237,9 @@ class PacketWriter {
      */
     public void shutdown() {
         done = true;
+        synchronized (queue) {
+            queue.notifyAll();
+        }
     }
 
     /**
@@ -244,22 +248,19 @@ class PacketWriter {
      * @return the next packet for writing.
      */
     private Packet nextPacket() {
-        synchronized(queue) {
+        Packet packet = null;
-            while (!done && queue.size() == 0) {
+        // Wait until there's a packet or we're done.
-                try {
+        while (!done && (packet = queue.poll()) == null) {
-                    queue.wait(2000);
+            try {
-                }
+                synchronized (queue) {
-                catch (InterruptedException ie) {
+                    queue.wait();
-                    // Do nothing
                 }
             }
-            if (queue.size() > 0) {
+            catch (InterruptedException ie) {
-                return queue.removeLast();
+                // Do nothing
-            }
-            else {
-                return null;
             }
         }
+        return packet;
     }
 
     private void writePackets(Thread thisThread) {
@@ -278,6 +279,20 @@ class PacketWriter {
                     }
                 }
             }
+            // Flush out the rest of the queue.
+            try {
+                synchronized (writer) {
+                   while (!queue.isEmpty()) {
+                       Packet packet = queue.remove();
+                        writer.write(packet.toXML());
+                    }
+                    writer.flush();
+                }
+            }
+            catch (Exception e) {
+                e.printStackTrace();
+            }
+
             // Close the stream.
             try {
                 writer.write("</stream:stream>");

source/org/jivesoftware/smack/XMPPConnection.java

@@ -174,7 +174,6 @@ public class XMPPConnection {
     public XMPPConnection(String serviceName) {
         // Create the configuration for this new connection
         ConnectionConfiguration config = new ConnectionConfiguration(serviceName);
-        config.setTLSEnabled(true);
         config.setCompressionEnabled(false);
         config.setSASLAuthenticationEnabled(true);
         config.setDebuggerEnabled(DEBUG_ENABLED);
@@ -1031,9 +1030,19 @@ public class XMPPConnection {
     /**
      * Notification message saying that the server supports TLS so confirm the server that we
      * want to secure the connection.
+     *
+     * @param required true when the server indicates that TLS is required.
      */
-    void startTLSReceived() {
+    void startTLSReceived(boolean required) {
-        if (!configuration.isTLSEnabled()) {
+        if (required && configuration.getSecurityMode() ==
+                ConnectionConfiguration.SecurityMode.disabled)
+        {
+            packetReader.notifyConnectionError(new IllegalStateException(
+                    "TLS required by server but not allowed by connection configuration"));
+            return;
+        }
+        
+        if (configuration.getSecurityMode() == ConnectionConfiguration.SecurityMode.disabled) {
             // Do not secure the connection using TLS since TLS was disabled
             return;
         }

test/org/jivesoftware/smack/ReconnectionTest.java

@@ -42,7 +42,6 @@ public class ReconnectionTest extends SmackTestCase {
     public void testAutomaticReconnectionWithCompression() throws Exception {
         // Create the configuration for this new connection
         ConnectionConfiguration config = new ConnectionConfiguration(getHost(), getPort());
-        config.setTLSEnabled(true);
         config.setCompressionEnabled(true);
         config.setSASLAuthenticationEnabled(true);
 
@@ -157,7 +156,6 @@ public class ReconnectionTest extends SmackTestCase {
         XMPPConnection connection;
         // Create the configuration
         ConnectionConfiguration config = new ConnectionConfiguration(getHost(), getPort());
-        config.setTLSEnabled(true);
         config.setCompressionEnabled(Boolean.getBoolean("test.compressionEnabled"));
         config.setSASLAuthenticationEnabled(true);
         connection = new XMPPConnection(config);

test/org/jivesoftware/smack/test/SmackTestCase.java

@@ -116,9 +116,7 @@ public abstract class SmackTestCase extends TestCase {
     protected XMPPConnection createConnection() {
         // Create the configuration for this new connection
         ConnectionConfiguration config = new ConnectionConfiguration(host, port);
-        config.setTLSEnabled(true);
         config.setCompressionEnabled(Boolean.getBoolean("test.compressionEnabled"));
-        config.setSASLAuthenticationEnabled(true);
         if (getSocketFactory() == null) {
             config.setSocketFactory(getSocketFactory());
         }

test/org/jivesoftware/smackx/CompressionTest.java

@@ -44,7 +44,6 @@ public class CompressionTest extends SmackTestCase {
 
         // Create the configuration for this new connection
         ConnectionConfiguration config = new ConnectionConfiguration(getHost(), getPort());
-        config.setTLSEnabled(true);
         config.setCompressionEnabled(true);
         config.setSASLAuthenticationEnabled(true);