diff --git a/server/src/main/java/com/mirth/connect/server/controllers/DefaultConfigurationController.java b/server/src/main/java/com/mirth/connect/server/controllers/DefaultConfigurationController.java index bfee2eddbf..fda0b39bc3 100644 --- a/server/src/main/java/com/mirth/connect/server/controllers/DefaultConfigurationController.java +++ b/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() { diff --git a/server/src/main/java/com/mirth/connect/server/util/ResourceUtil.java b/server/src/main/java/com/mirth/connect/server/util/ResourceUtil.java index 6bcf506e4c..6236833a55 100644 --- a/server/src/main/java/com/mirth/connect/server/util/ResourceUtil.java +++ b/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 { diff --git a/server/src/test/java/com/mirth/connect/server/controllers/DefaultConfigurationControllerTest.java b/server/src/test/java/com/mirth/connect/server/controllers/DefaultConfigurationControllerTest.java index 93072d30b0..8c6c6bee8f 100644 --- a/server/src/test/java/com/mirth/connect/server/controllers/DefaultConfigurationControllerTest.java +++ b/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,11 +25,18 @@ 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; @@ -36,6 +44,7 @@ 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 drivers, boolean includeODBC) { assertEquals(includeODBC ? 7 : 6, drivers.size()); int i = 0;