Avoid writing the keystore if no changes were made

server/src/main/java/com/mirth/connect/server/controllers/DefaultConfigurationController.java

@@ -53,7 +53,6 @@
 import javax.net.ssl.SSLSocketFactory;
 import javax.xml.parsers.DocumentBuilderFactory;
 
-import com.mirth.connect.client.core.BrandingConstants;
 import org.apache.commons.codec.binary.Base64;
 import org.apache.commons.collections4.CollectionUtils;
 import org.apache.commons.collections4.MapUtils;
@@ -63,6 +62,7 @@
 import org.apache.commons.configuration2.builder.FileBasedConfigurationBuilder;
 import org.apache.commons.configuration2.ex.ConfigurationException;
 import org.apache.commons.dbutils.DbUtils;
+import org.apache.commons.io.IOUtils;
 import org.apache.commons.lang3.ArrayUtils;
 import org.apache.commons.lang3.BooleanUtils;
 import org.apache.commons.lang3.StringUtils;
@@ -75,7 +75,6 @@
 import org.apache.ibatis.session.SqlSessionManager;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
-import org.bouncycastle.asn1.ASN1Sequence;
 import org.bouncycastle.asn1.x500.X500Name;
 import org.bouncycastle.asn1.x509.AuthorityKeyIdentifier;
 import org.bouncycastle.asn1.x509.BasicConstraints;
@@ -94,6 +93,7 @@
 import com.mirth.commons.encryption.Encryptor;
 import com.mirth.commons.encryption.KeyEncryptor;
 import com.mirth.commons.encryption.Output;
+import com.mirth.connect.client.core.BrandingConstants;
 import com.mirth.connect.client.core.ControllerException;
 import com.mirth.connect.client.core.PropertiesConfigurationUtil;
 import com.mirth.connect.donkey.model.DatabaseConstants;
@@ -463,7 +463,7 @@ public void initialize() {
         } catch (Exception e) {
             logger.error("Failed to initialize configuration controller", e);
         } finally {
-            ResourceUtil.closeResourceQuietly(versionPropertiesStream);
+            IOUtils.closeQuietly(versionPropertiesStream);
         }
     }
 
@@ -1234,6 +1234,8 @@ public void initializeSecuritySettings() {
                 keyStore = KeyStore.getInstance(mirthConfig.getString("keystore.type", "JCEKS"));
             }
 
+            boolean dirtiedKeystore = false;
+
             if (keyStoreFile.exists()) {
                 keyStoreFileIs = new FileInputStream(keyStoreFile);
                 keyStore.load(keyStoreFileIs, keyStorePassword);
@@ -1256,20 +1258,23 @@ public void initializeSecuritySettings() {
                 }
 
                 keyStore.load(null, keyStorePassword);
+                dirtiedKeystore = true;
                 logger.debug("keystore file not found, created new one");
             }
 
-            configureEncryption(provider, keyStore, keyPassword);
-            generateDefaultCertificate(provider, keyStore, keyPassword);
+            dirtiedKeystore |= configureEncryption(provider, keyStore, keyPassword);
+            dirtiedKeystore |= generateDefaultCertificate(provider, keyStore, keyPassword);
 
-            // write the keystore back to the file
-            fos = new FileOutputStream(keyStoreFile);
-            keyStore.store(fos, keyStorePassword);
+            // only re-write the keystore if it was changed
+            if (dirtiedKeystore) {
+                fos = new FileOutputStream(keyStoreFile);
+                keyStore.store(fos, keyStorePassword);
+            }
         } catch (Exception e) {
             logger.error("Could not initialize security settings.", e);
         } finally {
-            ResourceUtil.closeResourceQuietly(keyStoreFileIs);
-            ResourceUtil.closeResourceQuietly(fos);
+            IOUtils.closeQuietly(keyStoreFileIs);
+            IOUtils.closeQuietly(fos);
         }
     }
 
@@ -1352,7 +1357,7 @@ private void saveMirthConfig() throws FileNotFoundException, ConfigurationExcept
         try {
             PropertiesConfigurationUtil.saveTo(mirthConfig, os);
         } finally {
-            ResourceUtil.closeResourceQuietly(os);
+            IOUtils.closeQuietly(os);
         }
     }
 
@@ -1433,8 +1438,8 @@ public void migrateKeystore() {
         } catch (Exception e) {
             logger.error("Error migrating encryption key from database to keystore.", e);
         } finally {
-            ResourceUtil.closeResourceQuietly(mirthPropsIs);
-            ResourceUtil.closeResourceQuietly(keyStoreOuputStream);
+            IOUtils.closeQuietly(mirthPropsIs);
+            IOUtils.closeQuietly(keyStoreOuputStream);
         }
     }
 
@@ -1446,17 +1451,15 @@ public void updatePropertiesConfiguration(PropertiesConfiguration config) {
     /**
      * Instantiates the encryptor and digester using the configuration properties. If the properties
      * are not found, reasonable defaults are used.
-     * 
-     * @param provider
-     *            The provider to use (ex. BC)
-     * @param keyStore
-     *            The keystore from which to load the secret encryption key
-     * @param keyPassword
-     *            The secret key password
+     *
+     * @param provider    The provider to use (ex. BC)
+     * @param keyStore    The keystore from which to load the secret encryption key
+     * @param keyPassword The secret key password
      * @throws Exception
      */
-    private void configureEncryption(Provider provider, KeyStore keyStore, char[] keyPassword) throws Exception {
+    private boolean configureEncryption(Provider provider, KeyStore keyStore, char[] keyPassword) throws Exception {
         SecretKey secretKey = null;
+        boolean dirtiedKeystore = false;
 
         if (!keyStore.containsAlias(SECRET_KEY_ALIAS)) {
             logger.debug("encryption key not found, generating new one");
@@ -1465,6 +1468,7 @@ private void configureEncryption(Provider provider, KeyStore keyStore, char[] ke
             secretKey = keyGenerator.generateKey();
             KeyStore.SecretKeyEntry entry = new KeyStore.SecretKeyEntry(secretKey);
             keyStore.setEntry(SECRET_KEY_ALIAS, entry, new KeyStore.PasswordProtection(keyPassword));
+            dirtiedKeystore = true;
         } else {
             logger.debug("found encryption key in keystore");
             secretKey = (SecretKey) keyStore.getKey(SECRET_KEY_ALIAS, keyPassword);
@@ -1509,15 +1513,18 @@ private void configureEncryption(Provider provider, KeyStore keyStore, char[] ke
             );
             // @formatter:on
         }
+
+        return dirtiedKeystore;
     }
 
     /**
      * Checks for an existing certificate to use for secure communication between the server and
-     * client. If no certficate exists, this will generate a new one.
+     * client. If no certificate exists, this will generate a new one.
      * 
      */
-    private void generateDefaultCertificate(Provider provider, KeyStore keyStore, char[] keyPassword) throws Exception {
+    private boolean generateDefaultCertificate(Provider provider, KeyStore keyStore, char[] keyPassword) throws Exception {
         final String certificateAlias = "mirthconnect";
+        boolean dirtiedKeystore = false;
 
         if (!keyStore.containsAlias(certificateAlias)) {
             // Common CA and SSL cert attributes
@@ -1555,9 +1562,12 @@ private void generateDefaultCertificate(Provider provider, KeyStore keyStore, ch
             // add the generated SSL cert to the keystore using the key password
             keyStore.setKeyEntry(certificateAlias, sslKeyPair.getPrivate(), keyPassword, new Certificate[] {
                     sslCert });
+            dirtiedKeystore = true;
         } else {
             logger.debug("found certificate in keystore");
         }
+
+        return dirtiedKeystore;
     }
 
     private boolean isDatabaseRunning() {

server/src/main/java/com/mirth/connect/server/util/ResourceUtil.java

@@ -44,6 +44,13 @@ public static InputStream getResourceStream(Class<?> clazz, String resourceName)
         return is;
     }
 
+    /**
+     * Close a Closeable resource quietly. Trap and ignore any exceptions thrown when calling close()
+     *
+     * @deprecated import org.apache.commons.io.IOUtils does this and is already on the classpath. Further deprecated by try-with-resources since Java 7
+     * @param resource the Closeable object to try and close
+     */
+    @Deprecated
     public static void closeResourceQuietly(Closeable resource) {
         if (resource != null) {
             try {

server/src/test/java/com/mirth/connect/server/controllers/DefaultConfigurationControllerTest.java

@@ -10,6 +10,7 @@
 package com.mirth.connect.server.controllers;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 import static org.mockito.ArgumentMatchers.any;
@@ -24,18 +25,26 @@
 import java.io.StringReader;
 import java.io.StringWriter;
 import java.io.Writer;
+import java.lang.reflect.Field;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.security.KeyStore;
+import java.security.Provider;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
 import java.util.Properties;
 
+import javax.crypto.KeyGenerator;
+import javax.crypto.SecretKey;
 import javax.xml.XMLConstants;
 import javax.xml.transform.Source;
 import javax.xml.transform.TransformerFactory;
 import javax.xml.transform.stream.StreamResult;
 import javax.xml.transform.stream.StreamSource;
 
 import org.apache.commons.io.FileUtils;
+import org.bouncycastle.jce.provider.BouncyCastleProvider;
 import org.junit.BeforeClass;
 import org.junit.Test;
 import org.mockito.invocation.Invocation;
@@ -46,6 +55,7 @@
 import com.google.inject.Injector;
 import com.mirth.connect.client.core.ControllerException;
 import com.mirth.connect.model.DriverInfo;
+import com.mirth.connect.model.EncryptionSettings;
 
 public class DefaultConfigurationControllerTest {
 
@@ -275,6 +285,114 @@ public void testParseDbdriversXmlWithExternalDtd() {
     	assertTrue(exceptionCaught);
     }
 
+    /*
+     * The following tests cover the keystore "dirty" tracking added so that initializeSecuritySettings()
+     * only rewrites the keystore file when something actually changed. configureEncryption() and
+     * generateDefaultCertificate() are private, so they are exercised via reflection; both live in the
+     * same package as this test. Each returns true only when it mutated the keystore.
+     */
+
+    @Test
+    public void configureEncryption_returnsTrue_andStoresKey_whenKeyMissing() throws Exception {
+        setEncryptionConfig(new EncryptionSettings(new Properties()));
+
+        Provider provider = new BouncyCastleProvider();
+        KeyStore keyStore = newEmptyKeyStore();
+        char[] keyPassword = KEYSTORE_PASSWORD;
+
+        assertFalse(keyStore.containsAlias(DefaultConfigurationController.SECRET_KEY_ALIAS));
+
+        boolean dirtied = invokeConfigureEncryption(provider, keyStore, keyPassword);
+
+        assertTrue("A newly generated encryption key should mark the keystore dirty", dirtied);
+        assertTrue("The encryption key should now be present in the keystore", keyStore.containsAlias(DefaultConfigurationController.SECRET_KEY_ALIAS));
+    }
+
+    @Test
+    public void configureEncryption_returnsFalse_whenKeyAlreadyPresent() throws Exception {
+        setEncryptionConfig(new EncryptionSettings(new Properties()));
+
+        Provider provider = new BouncyCastleProvider();
+        KeyStore keyStore = newEmptyKeyStore();
+        char[] keyPassword = KEYSTORE_PASSWORD;
+
+        // Seed the keystore with an existing secret key so the "found" branch is taken.
+        KeyGenerator keyGenerator = KeyGenerator.getInstance("AES");
+        keyGenerator.init(128);
+        SecretKey existingKey = keyGenerator.generateKey();
+        keyStore.setEntry(DefaultConfigurationController.SECRET_KEY_ALIAS, new KeyStore.SecretKeyEntry(existingKey), new KeyStore.PasswordProtection(keyPassword));
+
+        boolean dirtied = invokeConfigureEncryption(provider, keyStore, keyPassword);
+
+        assertFalse("Reusing an existing encryption key should not mark the keystore dirty", dirtied);
+    }
+
+    @Test
+    public void generateDefaultCertificate_returnsTrue_andStoresCert_whenCertMissing() throws Exception {
+        Provider provider = new BouncyCastleProvider();
+        KeyStore keyStore = newEmptyKeyStore();
+        char[] keyPassword = KEYSTORE_PASSWORD;
+
+        assertFalse(keyStore.containsAlias(CERTIFICATE_ALIAS));
+
+        boolean dirtied = invokeGenerateDefaultCertificate(provider, keyStore, keyPassword);
+
+        assertTrue("A newly generated certificate should mark the keystore dirty", dirtied);
+        assertTrue("The certificate should now be present in the keystore", keyStore.containsAlias(CERTIFICATE_ALIAS));
+    }
+
+    @Test
+    public void generateDefaultCertificate_returnsFalse_whenCertAlreadyPresent() throws Exception {
+        Provider provider = new BouncyCastleProvider();
+        KeyStore keyStore = newEmptyKeyStore();
+        char[] keyPassword = KEYSTORE_PASSWORD;
+
+        // First call generates and stores the certificate.
+        assertTrue(invokeGenerateDefaultCertificate(provider, keyStore, keyPassword));
+
+        // Second call should find the existing certificate and leave the keystore untouched.
+        boolean dirtied = invokeGenerateDefaultCertificate(provider, keyStore, keyPassword);
+
+        assertFalse("Reusing an existing certificate should not mark the keystore dirty", dirtied);
+    }
+
+    private static final char[] KEYSTORE_PASSWORD = "testpass".toCharArray();
+    private static final String CERTIFICATE_ALIAS = "mirthconnect";
+
+    private KeyStore newEmptyKeyStore() throws Exception {
+        KeyStore keyStore = KeyStore.getInstance("JCEKS");
+        keyStore.load(null, KEYSTORE_PASSWORD);
+        return keyStore;
+    }
+
+    private void setEncryptionConfig(EncryptionSettings encryptionConfig) throws Exception {
+        Field field = DefaultConfigurationController.class.getDeclaredField("encryptionConfig");
+        field.setAccessible(true);
+        field.set(null, encryptionConfig);
+    }
+
+    private boolean invokeConfigureEncryption(Provider provider, KeyStore keyStore, char[] keyPassword) throws Exception {
+        return invokePrivateDirtyMethod("configureEncryption", provider, keyStore, keyPassword);
+    }
+
+    private boolean invokeGenerateDefaultCertificate(Provider provider, KeyStore keyStore, char[] keyPassword) throws Exception {
+        return invokePrivateDirtyMethod("generateDefaultCertificate", provider, keyStore, keyPassword);
+    }
+
+    private boolean invokePrivateDirtyMethod(String methodName, Provider provider, KeyStore keyStore, char[] keyPassword) throws Exception {
+        Method method = DefaultConfigurationController.class.getDeclaredMethod(methodName, Provider.class, KeyStore.class, char[].class);
+        method.setAccessible(true);
+        try {
+            return (boolean) method.invoke(new DefaultConfigurationController(), provider, keyStore, keyPassword);
+        } catch (InvocationTargetException e) {
+            Throwable cause = e.getCause();
+            if (cause instanceof Exception) {
+                throw (Exception) cause;
+            }
+            throw e;
+        }
+    }
+
     private void assertDefaultDrivers(List<DriverInfo> drivers, boolean includeODBC) {
         assertEquals(includeODBC ? 7 : 6, drivers.size());
         int i = 0;