From e998026de37a6ffeabda0fad519b6d69976205dd Mon Sep 17 00:00:00 2001 From: Andrii Landiak Date: Tue, 26 May 2026 10:36:04 +0300 Subject: [PATCH] CertificateReloadManager: detect content changes via checksum, not mtime --- .../service/CertificateReloadManager.java | 74 ++++--------------- .../service/CertificateReloadManagerTest.java | 64 ++++++++++------ 2 files changed, 54 insertions(+), 84 deletions(-) diff --git a/common/transport/transport-api/src/main/java/org/thingsboard/server/common/transport/service/CertificateReloadManager.java b/common/transport/transport-api/src/main/java/org/thingsboard/server/common/transport/service/CertificateReloadManager.java index 63f2247aba..82b7981f8f 100644 --- a/common/transport/transport-api/src/main/java/org/thingsboard/server/common/transport/service/CertificateReloadManager.java +++ b/common/transport/transport-api/src/main/java/org/thingsboard/server/common/transport/service/CertificateReloadManager.java @@ -27,7 +27,6 @@ import org.thingsboard.server.common.transport.config.ssl.SslCredentials; import org.thingsboard.server.common.transport.config.ssl.SslCredentialsConfig; import org.thingsboard.server.queue.util.TbTransportComponent; -import java.io.IOException; import java.io.InputStream; import java.nio.file.Files; import java.nio.file.Path; @@ -165,7 +164,6 @@ public class CertificateReloadManager implements SmartInitializingSingleton, Dis static class CertificateWatcher { private final List paths; private final Runnable reloadCallback; - private final Map lastModifiedMap; private final Map lastChecksumMap; private int consecutiveFailures; private String failedCombinedChecksum; @@ -173,60 +171,23 @@ public class CertificateReloadManager implements SmartInitializingSingleton, Dis CertificateWatcher(List paths, Runnable reloadCallback) { this.paths = paths; this.reloadCallback = reloadCallback; - this.lastModifiedMap = new HashMap<>(); this.lastChecksumMap = new HashMap<>(); for (Path path : paths) { - lastModifiedMap.put(path, getLastModifiedTime(path)); lastChecksumMap.put(path, calculateChecksum(path)); } this.consecutiveFailures = 0; } synchronized void checkAndReload(String name) { - boolean anyModifiedChanged = false; - for (Path path : paths) { - long currentModified = getLastModifiedTime(path); - Long lastModified = lastModifiedMap.getOrDefault(path, 0L); - if (currentModified != lastModified) { - anyModifiedChanged = true; - break; - } - } - if (!anyModifiedChanged) { - return; - } - - // Capture mtimes and checksums together before the callback runs. - // Pairing a post-callback mtime with a pre-callback checksum would let a write-during-reload be missed on the next poll. - Map currentModifiedTimes = new HashMap<>(); Map currentChecksums = new HashMap<>(); - StringBuilder combined = new StringBuilder(); for (Path path : paths) { - currentModifiedTimes.put(path, getLastModifiedTime(path)); - String checksum = calculateChecksum(path); - currentChecksums.put(path, checksum); - if (!combined.isEmpty()) { - combined.append("|"); - } - combined.append(path).append("=").append(checksum); + currentChecksums.put(path, calculateChecksum(path)); } - String combinedChecksum = combined.toString(); - - // Build old combined checksum for comparison - StringBuilder oldCombined = new StringBuilder(); - for (Path path : paths) { - if (!oldCombined.isEmpty()) { - oldCombined.append("|"); - } - oldCombined.append(path).append("=").append(lastChecksumMap.getOrDefault(path, "")); - } - String oldCombinedChecksum = oldCombined.toString(); + String combinedChecksum = combinedChecksum(currentChecksums); + String oldCombinedChecksum = combinedChecksum(lastChecksumMap); if (combinedChecksum.equals(oldCombinedChecksum)) { - // Content unchanged, just update modification times - for (Path path : paths) { - lastModifiedMap.put(path, currentModifiedTimes.get(path)); - } + // Content unchanged return; } @@ -237,41 +198,34 @@ public class CertificateReloadManager implements SmartInitializingSingleton, Dis } if (consecutiveFailures >= MAX_CONSECUTIVE_FAILURES) { - // Update modification times to avoid re-checking mtime and re-computing checksums every poll cycle - for (Path path : paths) { - lastModifiedMap.put(path, currentModifiedTimes.get(path)); - } return; } try { log.info("Certificate change detected for: {}. Triggering reload...", name); reloadCallback.run(); - for (Path path : paths) { - lastModifiedMap.put(path, currentModifiedTimes.get(path)); - lastChecksumMap.put(path, currentChecksums.get(path)); - } + lastChecksumMap.putAll(currentChecksums); consecutiveFailures = 0; failedCombinedChecksum = null; } catch (Exception e) { consecutiveFailures++; failedCombinedChecksum = combinedChecksum; - // Deliberately NOT updating the lastModifiedMap here, so the next poll cycle retries - // (mtime mismatch passes the early gate, checksum matches failedCombinedChecksum). + // Deliberately NOT updating lastChecksumMap here, so the next poll cycle still sees a differing + // checksum, re-enters this method, and retries the same content. log.error("Failed to reload certificate for {} (attempt {}/{}): {}", name, consecutiveFailures, MAX_CONSECUTIVE_FAILURES, e.getMessage(), e); } } - private long getLastModifiedTime(Path path) { - try { - if (!Files.exists(path)) { - return 0; + private String combinedChecksum(Map checksums) { + StringBuilder combined = new StringBuilder(); + for (Path path : paths) { + if (!combined.isEmpty()) { + combined.append("|"); } - return Files.getLastModifiedTime(path).toMillis(); - } catch (IOException e) { - return 0; + combined.append(path).append("=").append(checksums.getOrDefault(path, "")); } + return combined.toString(); } private String calculateChecksum(Path path) { diff --git a/common/transport/transport-api/src/test/java/org/thingsboard/server/common/transport/service/CertificateReloadManagerTest.java b/common/transport/transport-api/src/test/java/org/thingsboard/server/common/transport/service/CertificateReloadManagerTest.java index 6f78eaefae..8894937177 100644 --- a/common/transport/transport-api/src/test/java/org/thingsboard/server/common/transport/service/CertificateReloadManagerTest.java +++ b/common/transport/transport-api/src/test/java/org/thingsboard/server/common/transport/service/CertificateReloadManagerTest.java @@ -31,10 +31,7 @@ import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; -import static java.util.concurrent.TimeUnit.MILLISECONDS; -import static java.util.concurrent.TimeUnit.SECONDS; import static org.assertj.core.api.Assertions.assertThat; -import static org.awaitility.Awaitility.await; public class CertificateReloadManagerTest { @@ -59,11 +56,10 @@ public class CertificateReloadManagerTest { } } - private void writeFileAndAwaitMtimeChange(Path path, String content, long baselineMtime) throws IOException { + private void writeFileAndBumpMtime(Path path, String content, long baselineMtime) throws IOException { Files.writeString(path, content); - await().atMost(2, SECONDS) - .pollInterval(10, MILLISECONDS) - .until(() -> Files.getLastModifiedTime(path).toMillis() != baselineMtime); + // Force a strictly newer mtime: back-to-back writes can share a millisecond, hiding the change from the watcher. + Files.setLastModifiedTime(path, FileTime.fromMillis(baselineMtime + 1000)); } private long mtime(Path path) throws IOException { @@ -77,7 +73,7 @@ public class CertificateReloadManagerTest { certificateReloadManager.registerWatcher("test-cert", certFile, reloadCount::incrementAndGet); long baseline = mtime(certFile); - writeFileAndAwaitMtimeChange(certFile, "-----BEGIN CERTIFICATE-----\nTEST_CERT_V2_MODIFIED\n-----END CERTIFICATE-----\n", baseline); + writeFileAndBumpMtime(certFile, "-----BEGIN CERTIFICATE-----\nTEST_CERT_V2_MODIFIED\n-----END CERTIFICATE-----\n", baseline); ReflectionTestUtils.invokeMethod(certificateReloadManager, "checkCertificates"); @@ -85,7 +81,7 @@ public class CertificateReloadManagerTest { } @Test - public void givenCertificateFileUnchanged_whenCheckForChanges_thenShouldNotTriggerReload() throws Exception { + public void givenCertificateFileUnchanged_whenCheckForChanges_thenShouldNotTriggerReload() { AtomicInteger reloadCount = new AtomicInteger(0); certificateReloadManager.registerWatcher("test-cert", certFile, reloadCount::incrementAndGet); @@ -147,7 +143,7 @@ public class CertificateReloadManagerTest { certificateReloadManager.registerWatcher("test-key", keyFile, keyReloadCount::incrementAndGet); long baseline = mtime(keyFile); - writeFileAndAwaitMtimeChange(keyFile, "-----BEGIN PRIVATE KEY-----\nTEST_KEY_V2_MODIFIED\n-----END PRIVATE KEY-----\n", baseline); + writeFileAndBumpMtime(keyFile, "-----BEGIN PRIVATE KEY-----\nTEST_KEY_V2_MODIFIED\n-----END PRIVATE KEY-----\n", baseline); ReflectionTestUtils.invokeMethod(certificateReloadManager, "checkCertificates"); @@ -168,8 +164,8 @@ public class CertificateReloadManagerTest { long baseline1 = mtime(certFile); long baseline2 = mtime(cert2File); - writeFileAndAwaitMtimeChange(certFile, "-----BEGIN CERTIFICATE-----\nMODIFIED1\n-----END CERTIFICATE-----\n", baseline1); - writeFileAndAwaitMtimeChange(cert2File, "-----BEGIN CERTIFICATE-----\nMODIFIED2\n-----END CERTIFICATE-----\n", baseline2); + writeFileAndBumpMtime(certFile, "-----BEGIN CERTIFICATE-----\nMODIFIED1\n-----END CERTIFICATE-----\n", baseline1); + writeFileAndBumpMtime(cert2File, "-----BEGIN CERTIFICATE-----\nMODIFIED2\n-----END CERTIFICATE-----\n", baseline2); ReflectionTestUtils.invokeMethod(certificateReloadManager, "checkCertificates"); @@ -191,8 +187,8 @@ public class CertificateReloadManagerTest { long baseline1 = mtime(certFile); long baseline2 = mtime(cert2File); - writeFileAndAwaitMtimeChange(certFile, "-----BEGIN CERTIFICATE-----\nMODIFIED1\n-----END CERTIFICATE-----\n", baseline1); - writeFileAndAwaitMtimeChange(cert2File, "-----BEGIN CERTIFICATE-----\nMODIFIED2\n-----END CERTIFICATE-----\n", baseline2); + writeFileAndBumpMtime(certFile, "-----BEGIN CERTIFICATE-----\nMODIFIED1\n-----END CERTIFICATE-----\n", baseline1); + writeFileAndBumpMtime(cert2File, "-----BEGIN CERTIFICATE-----\nMODIFIED2\n-----END CERTIFICATE-----\n", baseline2); ReflectionTestUtils.invokeMethod(certificateReloadManager, "checkCertificates"); @@ -224,9 +220,7 @@ public class CertificateReloadManagerTest { for (int i = 0; i < 5; i++) { Files.writeString(certFile, "-----BEGIN CERTIFICATE-----\nCERT_VERSION_" + i + "\n-----END CERTIFICATE-----\n"); } - await().atMost(2, SECONDS) - .pollInterval(10, MILLISECONDS) - .until(() -> mtime(certFile) != baseline); + Files.setLastModifiedTime(certFile, FileTime.fromMillis(baseline + 1000)); ReflectionTestUtils.invokeMethod(certificateReloadManager, "checkCertificates"); @@ -242,7 +236,7 @@ public class CertificateReloadManagerTest { certificateReloadManager.registerWatcher("test-cert", certFile, reloadCount::incrementAndGet); long baseline = mtime(certFile); - writeFileAndAwaitMtimeChange(certFile, "-----BEGIN CERTIFICATE-----\nMODIFIED\n-----END CERTIFICATE-----\n", baseline); + writeFileAndBumpMtime(certFile, "-----BEGIN CERTIFICATE-----\nMODIFIED\n-----END CERTIFICATE-----\n", baseline); for (int i = 0; i < 5; i++) { new Thread(() -> { @@ -272,7 +266,7 @@ public class CertificateReloadManagerTest { certificateReloadManager.registerWatcher("test-cert", certFile, reloadCount::incrementAndGet); long baseline = mtime(certFile); - writeFileAndAwaitMtimeChange(certFile, originalContent, baseline); + writeFileAndBumpMtime(certFile, originalContent, baseline); ReflectionTestUtils.invokeMethod(certificateReloadManager, "checkCertificates"); @@ -289,7 +283,7 @@ public class CertificateReloadManagerTest { }); long baseline = mtime(certFile); - writeFileAndAwaitMtimeChange(certFile, "-----BEGIN CERTIFICATE-----\nBAD_CERT\n-----END CERTIFICATE-----\n", baseline); + writeFileAndBumpMtime(certFile, "-----BEGIN CERTIFICATE-----\nBAD_CERT\n-----END CERTIFICATE-----\n", baseline); for (int i = 0; i < 15; i++) { ReflectionTestUtils.invokeMethod(certificateReloadManager, "checkCertificates"); @@ -311,19 +305,41 @@ public class CertificateReloadManagerTest { }); long baseline = mtime(certFile); - writeFileAndAwaitMtimeChange(certFile, "-----BEGIN CERTIFICATE-----\nBAD_CERT\n-----END CERTIFICATE-----\n", baseline); + writeFileAndBumpMtime(certFile, "-----BEGIN CERTIFICATE-----\nBAD_CERT\n-----END CERTIFICATE-----\n", baseline); ReflectionTestUtils.invokeMethod(certificateReloadManager, "checkCertificates"); assertThat(reloadAttempts.get()).isEqualTo(1); shouldFail.set(0); long baseline2 = mtime(certFile); - writeFileAndAwaitMtimeChange(certFile, "-----BEGIN CERTIFICATE-----\nGOOD_CERT\n-----END CERTIFICATE-----\n", baseline2); + writeFileAndBumpMtime(certFile, "-----BEGIN CERTIFICATE-----\nGOOD_CERT\n-----END CERTIFICATE-----\n", baseline2); ReflectionTestUtils.invokeMethod(certificateReloadManager, "checkCertificates"); assertThat(reloadAttempts.get()).isEqualTo(2); } + @Test + public void givenContentChangedButMtimeUnchanged_whenCheckForChanges_thenShouldTriggerReload() throws Exception { + // Bug fingerprint: a cert-manager rotation that lands in the same wall-clock millisecond as the + // watcher's recorded baseline mtime. Files.getLastModifiedTime().toMillis() truncates to the ms, + // so the rotated content shares the baseline mtime and an mtime-only gate would never re-hash it. + AtomicInteger reloadCount = new AtomicInteger(0); + + certificateReloadManager.registerWatcher("test-cert", certFile, reloadCount::incrementAndGet); + + long baseline = mtime(certFile); + Files.writeString(certFile, "-----BEGIN CERTIFICATE-----\nROTATED_SAME_MS\n-----END CERTIFICATE-----\n"); + // Force the mtime back to the exact baseline millisecond — content changed, timestamp did not. + Files.setLastModifiedTime(certFile, FileTime.fromMillis(baseline)); + + // Sanity guard: the watcher observes a timestamp identical to its recorded baseline. + assertThat(mtime(certFile)).isEqualTo(baseline); + + ReflectionTestUtils.invokeMethod(certificateReloadManager, "checkCertificates"); + + assertThat(reloadCount.get()).isEqualTo(1); + } + @Test public void givenCallbackHitMaxFailures_whenFileChangesToNewContent_thenShouldResetAndRetry() throws Exception { AtomicInteger reloadAttempts = new AtomicInteger(0); @@ -337,7 +353,7 @@ public class CertificateReloadManagerTest { }); long baseline = mtime(certFile); - writeFileAndAwaitMtimeChange(certFile, "-----BEGIN CERTIFICATE-----\nBAD_CERT\n-----END CERTIFICATE-----\n", baseline); + writeFileAndBumpMtime(certFile, "-----BEGIN CERTIFICATE-----\nBAD_CERT\n-----END CERTIFICATE-----\n", baseline); for (int i = 0; i < 15; i++) { ReflectionTestUtils.invokeMethod(certificateReloadManager, "checkCertificates"); @@ -346,7 +362,7 @@ public class CertificateReloadManagerTest { shouldFail.set(0); long baseline2 = mtime(certFile); - writeFileAndAwaitMtimeChange(certFile, "-----BEGIN CERTIFICATE-----\nFIXED_CERT\n-----END CERTIFICATE-----\n", baseline2); + writeFileAndBumpMtime(certFile, "-----BEGIN CERTIFICATE-----\nFIXED_CERT\n-----END CERTIFICATE-----\n", baseline2); ReflectionTestUtils.invokeMethod(certificateReloadManager, "checkCertificates"); assertThat(reloadAttempts.get()).isEqualTo(11);