GitHub Issue 1064: Exceptions during encryption migration prevent upgrade

api/src/org/labkey/api/security/Encryption.java

@@ -50,6 +50,7 @@
 
 import javax.crypto.BadPaddingException;
 import javax.crypto.Cipher;
+import javax.crypto.IllegalBlockSizeException;
 import javax.crypto.SecretKey;
 import javax.crypto.SecretKeyFactory;
 import javax.crypto.spec.GCMParameterSpec;
@@ -473,14 +474,32 @@ public String decrypt(byte @NotNull[] cipherText)
                 cipher.init(Cipher.DECRYPT_MODE, _keySpec, _config.createIvSpec(iv));
                 return new String(cipher.doFinal(encrypted), StringUtilsLabKey.DEFAULT_CHARSET);
             }
-            catch (BadPaddingException e)
+            catch (BadPaddingException | IllegalBlockSizeException e)
             {
-                // For now, assume that BadPaddingException means the key has been changed and all other
-                // exceptions are coding issues. That might change in the future...
+                // Decryption failure - likely a bad key or old algorithm.
 
-                // Track all decryption exceptions that aren't caused by TestCase (below)
+                // Track all decryption exceptions that aren't caused by TestCase.
+                // Only the production AES instance (ENCRYPTION_KEY_CHANGED keySource) attempts the fallback;
+                // migration-temporary instances bypass this block entirely.
                 if (ENCRYPTION_KEY_CHANGED.equals(_keySource))
+                {
+                    // During migration, not-yet-migrated values are in the old format. If a fallback algorithm
+                    // is registered (set by prepareMigrationFallback() when migration is known to be incomplete),
+                    // try it before giving up.
+                    Algorithm fallback = _migrationFallback;
+                    if (fallback != null)
+                    {
+                        try
+                        {
+                            return fallback.decrypt(cipherText);
+                        }
+                        catch (RuntimeException ignored)
+                        {
+                            // Both algorithms failed; fall through to increment and rethrow
+                        }
+                    }
                     DECRYPTION_EXCEPTIONS.incrementAndGet();
+                }
 
                 throw new DecryptionException("Could not decrypt this content using the " + _keySource, e);
             }
@@ -493,6 +512,9 @@ public String decrypt(byte @NotNull[] cipherText)
 
     private static final String ENCRYPTION_KEY_CHANGED = "currently configured EncryptionKey; has the key changed in " + AppProps.getInstance().getWebappConfigurationFilename() + "?";
     private static final AtomicInteger DECRYPTION_EXCEPTIONS = new AtomicInteger(0);
+    // Set by prepareMigrationFallback() when migration is known to be incomplete; cleared after migration completes.
+    // Allows HTTP requests to decrypt not-yet-migrated values without failing.
+    private static volatile Algorithm _migrationFallback = null;
 
     public static class DecryptionException extends ConfigurationException
     {
@@ -539,6 +561,39 @@ static void registerHandler(EncryptionMigrationHandler handler)
         void migrateEncryptedContent(String oldPassPhrase, String keySource, AESConfig oldConfig);
     }
 
+    /**
+     * Examines the database to determine whether algorithm or key migration is pending, and if so installs a
+     * fallback algorithm. This allows HTTP requests to transparently decrypt not-yet-migrated values during the
+     * migration window instead of failing and incrementing DECRYPTION_EXCEPTIONS.
+     * Must be called after the database and PropertyManager are available (e.g., from CoreModule.afterUpdate()).
+     * The fallback is cleared automatically once checkMigration() confirms completion.
+     */
+    public static void prepareMigrationFallback()
+    {
+        if (!isEncryptionPassPhraseSpecified())
+            return;
+
+        String oldPassPhrase = getOldEncryptionPassPhrase();
+
+        String cipher = PropertyManager.getNormalStore()
+            .getProperties(ENCRYPTION_CIPHER_CATEGORY)
+            .get(CIPHER_PROPERTY);
+
+        if (oldPassPhrase != null)
+        {
+            // Key-change migration not yet complete; use old key. If cipher is also null, old content used the
+            // legacy cipher (matching what checkMigration() will use: old key + AESConfig.legacy); otherwise
+            // old content used the current cipher.
+            AESConfig fallbackConfig = cipher == null ? AESConfig.legacy : AESConfig.current;
+            _migrationFallback = new AES(oldPassPhrase, 128, "legacy key migration fallback", fallbackConfig);
+        }
+        else if (cipher == null)
+        {
+            // Cipher migration not yet complete; fall back to legacy cipher with current key
+            _migrationFallback = new AES(getEncryptionPassPhrase(), 128, "legacy cipher migration fallback", AESConfig.legacy);
+        }
+    }
+
     public static void checkMigration()
     {
         String oldPassPhrase = getOldEncryptionPassPhrase();
@@ -547,6 +602,7 @@ public static void checkMigration()
         if (isEncryptionPassPhraseSpecified() && ModuleLoader.getInstance().shouldInsertData())
         {
             boolean migrationNeeded = false;
+            boolean migrationSucceeded = false;
             String keySource = null;
 
             if (null != oldPassPhrase)
@@ -591,11 +647,16 @@ else if (!cipher.equals(AESConfig.current.getCipherName()))
 
                     CacheManager.clearAllKnownCaches();
                 }
-                // Test to validate conversion and create a validation value if needed
+                // Test to validate conversion and create a validation value if needed.
+                // Capture the counter before the test so the save decision is based solely on whether
+                // this specific test passes, not on concurrent HTTP request decryption failures that may
+                // have incremented the counter during the (potentially long) migration of auth configurations.
+                int exceptionsBeforeFinalTest = DECRYPTION_EXCEPTIONS.get();
                 testEncryptionKey();
+                migrationSucceeded = DECRYPTION_EXCEPTIONS.get() == exceptionsBeforeFinalTest;
             }
 
-            if (DECRYPTION_EXCEPTIONS.get() == 0)
+            if (migrationSucceeded)
             {
                 if (oldPassPhrase != null)
                 {
@@ -610,6 +671,8 @@ else if (!cipher.equals(AESConfig.current.getCipherName()))
                 }
             }
         }
+
+        _migrationFallback = null;
     }
 
 
@@ -663,6 +726,55 @@ public void testBadKeyException()
             }
         }
 
+        @Test
+        public void testMigrationFallback()
+        {
+            String text = "test plaintext";
+            AES oldAlgorithm = new AES("old pass phrase", 128, "old algorithm");
+            byte[] oldEncrypted = oldAlgorithm.encrypt(text);
+
+            // Primary (production) instance: different pass phrase, keySource == ENCRYPTION_KEY_CHANGED
+            AES primary = new AES("primary pass phrase", 128, ENCRYPTION_KEY_CHANGED);
+
+            // Case 1: no fallback — primary fails and counter increments
+            int counterBefore = DECRYPTION_EXCEPTIONS.get();
+            try
+            {
+                primary.decrypt(oldEncrypted);
+                fail("Expected DecryptionException");
+            }
+            catch (DecryptionException ignored) {}
+            assertEquals(counterBefore + 1, DECRYPTION_EXCEPTIONS.get());
+
+            // Case 2: correct fallback — transparent success, counter unchanged
+            _migrationFallback = oldAlgorithm;
+            try
+            {
+                int counterBeforeFallback = DECRYPTION_EXCEPTIONS.get();
+                assertEquals(text, primary.decrypt(oldEncrypted));
+                assertEquals("Counter must not increment when fallback succeeds", counterBeforeFallback, DECRYPTION_EXCEPTIONS.get());
+            }
+            finally
+            {
+                _migrationFallback = null;
+            }
+
+            // Case 3: wrong fallback — both algorithms fail, counter increments
+            _migrationFallback = new AES("wrong pass phrase", 128, "wrong fallback");
+            int counterBeforeWrongFallback = DECRYPTION_EXCEPTIONS.get();
+            try
+            {
+                primary.decrypt(oldEncrypted);
+                fail("Expected DecryptionException");
+            }
+            catch (DecryptionException ignored) {}
+            finally
+            {
+                _migrationFallback = null;
+            }
+            assertEquals(counterBeforeWrongFallback + 1, DECRYPTION_EXCEPTIONS.get());
+        }
+
         private void test(Algorithm algorithm)
         {
             test(algorithm, algorithm);

core/src/org/labkey/core/CoreModule.java

@@ -869,6 +869,11 @@ public void afterUpdate(ModuleContext moduleContext)
                 ContainerManager.getHomeContainer();
              }
         });
+
+        // Install a fallback decryption algorithm if AES migration is pending. This prevents concurrent HTTP requests
+        // from failing to decrypt not-yet-migrated values during the migration window. Called here (afterUpdate) rather
+        // than in startupAfterSpringConfig so the fallback is active before any long-running upgrade steps run.
+        Encryption.prepareMigrationFallback();
     }
 
     private void bootstrap()