Merge pull request #807 from csmith/certs

Initial work on certificate manager

src/main/java/com/dmdirc/tls/CertificateExceptionManager.java

@@ -0,0 +1,112 @@
+package com.dmdirc.tls;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import java.security.cert.PKIXParameters;
+import java.security.cert.TrustAnchor;
+import java.security.cert.X509Certificate;
+import java.util.Collections;
+import java.util.Set;
+import java.util.stream.Collectors;
+import javax.annotation.Nullable;
+
+/**
+ * Manages certificates that the user has explicitly trusted, excepting them from the normal PKIX checks.
+ */
+public class CertificateExceptionManager {
+
+    /** Password to use for our exception keystore. */
+    private static final char[] PASSWORD = "dmdirc".toCharArray();
+
+    private final Path keyStorePath;
+
+    public CertificateExceptionManager(final Path keyStorePath) {
+        this.keyStorePath = keyStorePath;
+    }
+
+    /**
+     * Returns the set of certificates that the user has explicitly trusted.
+     */
+    public Set<X509Certificate> getExceptedCertificates() {
+        try {
+            final KeyStore keyStore = getKeyStore();
+            if (keyStore == null) {
+                return Collections.emptySet();
+            } else {
+                final PKIXParameters params = new PKIXParameters(keyStore);
+                return params.getTrustAnchors().stream()
+                        .map(TrustAnchor::getTrustedCert)
+                        .collect(Collectors.toSet());
+            }
+        } catch (GeneralSecurityException | IOException e) {
+            return Collections.emptySet();
+        }
+    }
+
+    /**
+     * Adds a new certificate that will be excepted from future PKIX checks.
+     *
+     * @return True if the certificate was successfully added; false otherwise.
+     */
+    public boolean addExceptedCertificate(final X509Certificate certificate) {
+        try {
+            final KeyStore keyStore = getKeyStore();
+            keyStore.setCertificateEntry(
+                    "excepted-" + System.currentTimeMillis(),
+                    certificate);
+            try (OutputStream os = Files.newOutputStream(keyStorePath)) {
+                keyStore.store(os, PASSWORD);
+                return true;
+            }
+        } catch (GeneralSecurityException | IOException e) {
+            return false;
+        }
+    }
+
+    /**
+     * Removes a certificate that was previously added.
+     *
+     * @return True if the remove was successful; false otherwise
+     */
+    public boolean removeExceptedCertificate(final X509Certificate certificate) {
+        try {
+            final KeyStore keyStore = getKeyStore();
+
+            @Nullable final String alias = keyStore.getCertificateAlias(certificate);
+            if (alias == null) {
+                // Not found
+                return false;
+            }
+
+            keyStore.deleteEntry(alias);
+            try (OutputStream os = Files.newOutputStream(keyStorePath)) {
+                keyStore.store(os, PASSWORD);
+                return true;
+            }
+        } catch (GeneralSecurityException | IOException e) {
+            return false;
+        }
+    }
+
+    /**
+     * Loads the existing excepted certs KeyStore from disk, if it exists, or creates a new KeyStore otherwise.
+     */
+    private KeyStore getKeyStore() throws GeneralSecurityException, IOException {
+        try (InputStream is = Files.newInputStream(keyStorePath)) {
+            final KeyStore keyStore = KeyStore.getInstance(KeyStore.getDefaultType());
+            keyStore.load(is, PASSWORD);
+            return keyStore;
+        } catch (GeneralSecurityException | IOException ex) {
+            final KeyStore keyStore = KeyStore.getInstance(KeyStore.getDefaultType());
+            keyStore.load(null, null);
+            return keyStore;
+        }
+    }
+
+
+}

src/main/java/com/dmdirc/tls/CertificateManager.java

@@ -76,12 +76,6 @@ public class CertificateManager implements X509TrustManager {
     private final AggregateConfigProvider config;
     /** The set of CAs from the global cacert file. */
     private final Set<X509Certificate> globalTrustedCAs = new HashSet<>();
-    /** Whether or not to the issue and expiry dates of the certificate. */
-    private final boolean checkDate;
-    /** Whether or not to the issuer of the certificate. */
-    private final boolean checkIssuer;
-    /** Whether or not to the hostname of the certificate. */
-    private final boolean checkHost;
     /** Used to synchronise the manager with the certificate dialog. */
     private final Semaphore actionSem = new Semaphore(0);
     /** The event bus to post errors to. */
@@ -114,9 +108,6 @@ public class CertificateManager implements X509TrustManager {
         this.connection = connection;
         this.serverName = serverName;
         this.config = config;
-        this.checkDate = config.getOptionBool("ssl", "checkdate");
-        this.checkIssuer = config.getOptionBool("ssl", "checkissuer");
-        this.checkHost = config.getOptionBool("ssl", "checkhost");
         this.userSettings = userSettings;
         this.eventBus = eventBus;
         this.keyStoreLocator = new KeyStoreLocator();
@@ -331,26 +322,22 @@ public class CertificateManager implements X509TrustManager {
         for (X509Certificate cert : chain) {
             final TrustResult trustResult = isTrusted(cert);
 
-            if (checkDate) {
-                // Check that the certificate is in-date
-                try {
-                    cert.checkValidity();
-                } catch (CertificateException ex) {
-                    problems.add(ex);
-                }
+            // Check that the certificate is in-date
+            try {
+                cert.checkValidity();
+            } catch (CertificateException ex) {
+                problems.add(ex);
             }
 
-            if (checkIssuer) {
-                // Check that we trust an issuer
-                verified |= trustResult.isTrusted();
-            }
+            // Check that we trust an issuer
+            verified |= trustResult.isTrusted();
 
             if (trustResult == TrustResult.TRUSTED_MANUALLY) {
                 manual = true;
             }
         }
 
-        if (!verified && checkIssuer) {
+        if (!verified) {
             problems.add(new CertificateNotTrustedException("Issuer is not trusted"));
         }
         return manual;
@@ -362,12 +349,10 @@ public class CertificateManager implements X509TrustManager {
      * @param chain The chain of certificates to check.
      */
     private void checkHost(final X509Certificate... chain) {
-        if (checkHost) {
-            // Check that the cert is issued to the correct host
-            if (!isValidHost(chain[0])) {
-                problems.add(new CertificateDoesntMatchHostException(
-                        "Certificate was not issued to " + serverName));
-            }
+        // Check that the cert is issued to the correct host
+        if (!isValidHost(chain[0])) {
+            problems.add(new CertificateDoesntMatchHostException(
+                    "Certificate was not issued to " + serverName));
         }
     }
 

src/test/java/com/dmdirc/tls/CertificateExceptionManagerTest.java

@@ -0,0 +1,93 @@
+package com.dmdirc.tls;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import java.security.cert.X509Certificate;
+import java.util.Set;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+/**
+ * Tests for {@link CertificateExceptionManager}.
+ *
+ * <p>These test use two certificates stored in a keystore. They were generated using:
+ *
+ * <pre>
+ * keytool -genkey -validity 18250 -keystore "keystore.ks" -storepass "dmdirc" -keypass "dmdirc" -alias "test1" -dname "CN=Test1, O=DMDirc, C=GB"
+ * keytool -genkey -validity 18250 -keystore "keystore.ks" -storepass "dmdirc" -keypass "dmdirc" -alias "test2" -dname "CN=Test2, O=DMDirc, C=GB"
+ * </pre>
+ */
+public class CertificateExceptionManagerTest {
+
+    @Rule
+    public TemporaryFolder tempFolderRule = new TemporaryFolder();
+
+    private Path keyStorePath;
+    private CertificateExceptionManager manager;
+
+    @Before
+    public void setup() throws IOException {
+        keyStorePath = tempFolderRule.newFile("certs.keystore").toPath();
+        manager = new CertificateExceptionManager(keyStorePath);
+    }
+
+    @Test
+    public void testGetCertsNoFile() {
+        assertTrue(manager.getExceptedCertificates().isEmpty());
+    }
+
+    @Test
+    public void testAddCert() throws GeneralSecurityException, IOException {
+        final X509Certificate cert = getCertificate(1);
+        assertTrue(manager.addExceptedCertificate(cert));
+        assertTrue(Files.exists(keyStorePath));
+        final Set<X509Certificate> certs = manager.getExceptedCertificates();
+        assertEquals(1, certs.size());
+        assertTrue(certs.contains(cert));
+    }
+
+    @Test
+    public void testRemoveUnknownCert() throws GeneralSecurityException, IOException {
+        final X509Certificate cert = getCertificate(1);
+        assertFalse(manager.removeExceptedCertificate(cert));
+    }
+
+    @Test
+    public void testRemoveCert() throws GeneralSecurityException, IOException {
+        final X509Certificate cert = getCertificate(1);
+        manager.addExceptedCertificate(cert);
+        assertTrue(manager.removeExceptedCertificate(cert));
+        assertTrue(manager.getExceptedCertificates().isEmpty());
+    }
+
+    @Test
+    public void testRemoveCertLeavesExisting() throws GeneralSecurityException, IOException {
+        final X509Certificate cert1 = getCertificate(1);
+        final X509Certificate cert2 = getCertificate(2);
+        manager.addExceptedCertificate(cert1);
+        manager.addExceptedCertificate(cert2);
+        assertTrue(manager.removeExceptedCertificate(cert1));
+        final Set<X509Certificate> certs = manager.getExceptedCertificates();
+        assertEquals(1, certs.size());
+        assertTrue(certs.contains(cert2));
+    }
+
+    private X509Certificate getCertificate(final int num) throws GeneralSecurityException, IOException {
+        try (InputStream is = getClass().getResourceAsStream("keystore.ks")) {
+            final KeyStore keyStore = KeyStore.getInstance(KeyStore.getDefaultType());
+            keyStore.load(is, "dmdirc".toCharArray());
+            return (X509Certificate) keyStore.getCertificate("test" + num);
+        }
+    }
+
+}