OF-405 Addressing review comments from Simon White

src/java/org/jivesoftware/openfire/net/SASLAuthentication.java

@@ -182,37 +182,8 @@ public class SASLAuthentication {
         if (!(session instanceof ClientSession) && !(session instanceof IncomingServerSession)) {
             return "";
         }
-        StringBuilder sb = new StringBuilder(195);
-        sb.append("<mechanisms xmlns=\"urn:ietf:params:xml:ns:xmpp-sasl\">");
-        if (session instanceof IncomingServerSession) {
-            // Server connections don't follow the same rules as clients
-            if (session.isSecure()) {
-                boolean usingSelfSigned = true;
-
-                try {
-                    X509Certificate trusted = CertificateManager.getEndEntityCertificate(session.getConnection().getPeerCertificates(), SSLConfig.getKeyStore(), SSLConfig.gets2sTrustStore());
-                    usingSelfSigned = trusted == null;
-                } catch (IOException ex) {
-                    Log.warn("Exception occurred while trying to determine whether remote certificate is trusted. Proceeding as if it is.", ex);
-                    usingSelfSigned = true;
-                }
-                
-                if (!usingSelfSigned) {
-                    // Offer SASL EXTERNAL only if TLS has already been negotiated and the peer is not
-                    // using a self-signed certificate
-                    sb.append("<mechanism>EXTERNAL</mechanism>");
-                }
-            }
-        }
-        else {
-            for (String mech : getSupportedMechanisms()) {
-                sb.append("<mechanism>");
-                sb.append(mech);
-                sb.append("</mechanism>");
-            }
-        }
-        sb.append("</mechanisms>");
-        return sb.toString();
+        Element mechs = getSASLMechanismsElement(session);
+        return mechs.asXML();
     }
 
     public static Element getSASLMechanismsElement(Session session) {
@@ -230,7 +201,7 @@ public class SASLAuthentication {
                     X509Certificate trusted = CertificateManager.getEndEntityCertificate(((LocalSession)session).getConnection().getPeerCertificates(), SSLConfig.getKeyStore(), SSLConfig.gets2sTrustStore());
                     usingSelfSigned = trusted == null;
                 } catch (IOException ex) {
-                    Log.warn("Exception occurred while trying to determine whether remote certificate is trusted. Proceeding as if it is.", ex);
+                    Log.warn("Exception occurred while trying to determine whether remote certificate is trusted. Treating as untrusted.", ex);
                     usingSelfSigned = true;
                 }
                 if (!usingSelfSigned) {

src/java/org/jivesoftware/util/CertificateManager.java

@@ -223,10 +223,11 @@ public class CertificateManager {
      * Decide whether or not to trust the given supplied certificate chain, returning the
      * End Entity Certificate in this case where it can, and null otherwise.
      * A self-signed certificate will, for example, return null.
-     * For certain failures, we SHOULD generate an exception - revocations and the like.
+     * For certain failures, we SHOULD generate an exception - revocations and the like,
+     * but we currently do not.
      *
      * @param certChain an array of X509Certificate where the first one is the endEntityCertificate.
-     * @return a boolean indicating whether the endEntityCertificate should be trusted.
+     * @return trusted end-entity certificate, or null.
      */
     public static X509Certificate getEndEntityCertificate(Certificate chain[],
             KeyStore certStore, KeyStore trustStore) {
@@ -235,7 +236,7 @@ public class CertificateManager {
         }
         X509Certificate first = (X509Certificate) chain[0];
         if (chain.length == 1
-                && first.getSubjectDN().equals(first.getIssuerDN())) {
+                && first.getSubjectX500Principal().equals(first.getIssuerX500Principal())) {
             // Chain is single cert, and self-signed.
             try {
                 if (trustStore.getCertificateAlias(first) != null) {
@@ -243,7 +244,7 @@ public class CertificateManager {
                     return first;
                 }
             } catch (KeyStoreException e) {
-                // Ignore.
+                Log.warn("Keystore error while looking for self-signed cert; assuming untrusted.");
             }
             return null;
         }
@@ -299,7 +300,7 @@ public class CertificateManager {
                 ls.add((X509Certificate) chain[i]);
             }
             for (X509Certificate last = ls.get(ls.size() - 1); !last
-                    .getIssuerDN().equals(last.getSubjectDN()); last = ls
+                    .getIssuerX500Principal().equals(last.getSubjectX500Principal()); last = ls
                     .get(ls.size() - 1)) {
                 X509CertSelector sel = new X509CertSelector();
                 sel.setSubject(last.getIssuerX500Principal());
@@ -319,6 +320,7 @@ public class CertificateManager {
         } catch (CertPathValidatorException e) {
             Log.warn("Path validator: " + e.getMessage());
         } catch (Exception e) {
+            Log.warn("Unkown exception while validating certificate chain: " + e.getMessage());
         }
         return null;
     }