Skip to content

Avoid writing the keystore if no changes were made #134

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
rogin wants to merge 1 commit into
base: main
Choose a base branch
from
+39 −22
Draft

Avoid writing the keystore if no changes were made #134

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 32 additions & 22 deletions server/src/com/mirth/connect/server/controllers/DefaultConfigurationController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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);
Expand All @@ -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) {
Copy link

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a space after 'if' before the opening parenthesis, following standard Java formatting conventions. The line should read 'if (dirtiedKeystore)' instead of 'if(dirtiedKeystore)'.

Suggested change
if(dirtiedKeystore) {
if (dirtiedKeystore) {

Copilot uses AI. Check for mistakes.
fos = new FileOutputStream(keyStoreFile);
keyStore.store(fos, keyStorePassword);
}
Comment on lines +1265 to +1272
Copy link

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new conditional keystore write logic (checking dirtiedKeystore flag before writing) lacks test coverage. Consider adding a test in DefaultConfigurationControllerTest to verify that the keystore is not written when no changes are made, such as by checking the file's modification time before and after calling initializeSecuritySettings with an existing, valid keystore.

Copilot uses AI. Check for mistakes.
} catch (Exception e) {
logger.error("Could not initialize security settings.", e);
} finally {
ResourceUtil.closeResourceQuietly(keyStoreFileIs);
ResourceUtil.closeResourceQuietly(fos);
IOUtils.closeQuietly(keyStoreFileIs);
IOUtils.closeQuietly(fos);
}
}

Expand Down Expand Up @@ -1352,7 +1357,7 @@ private void saveMirthConfig() throws FileNotFoundException, ConfigurationExcept
try {
PropertiesConfigurationUtil.saveTo(mirthConfig, os);
} finally {
ResourceUtil.closeResourceQuietly(os);
IOUtils.closeQuietly(os);
}
}

Expand Down Expand Up @@ -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);
}
}

Expand All @@ -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");
Expand All @@ -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);
Expand Down Expand Up @@ -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.
Copy link

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in the documentation: 'certficate' should be 'certificate'.

Suggested change
* client. If no certficate exists, this will generate a new one.
* client. If no certificate exists, this will generate a new one.

Copilot uses AI. Check for mistakes.
*
*/
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
Expand Down Expand Up @@ -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() {
Expand Down
7 changes: 7 additions & 0 deletions server/src/com/mirth/connect/server/util/ResourceUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down