Improve encrypted property storage, check keys during migration

api/src/org/labkey/api/data/AES.java

@@ -21,7 +21,7 @@
  * First reference to {@link org.labkey.api.data.PropertyManager} constructs the stores which causes initialization of the PropertyEncryption enum.
  * The AES128 enum requires the property manager (we store the standard salt in properties), so we use a holder pattern
  * (instead of normal static initialization) to implement thread-safe lazy initialization, breaking the loop.
- *
+ * <p>
  * This class is only used by the PropertyManager; other encryption users should call Encryption.getAES128() directly.
  */
 class AES

api/src/org/labkey/api/data/EncryptedPropertyStore.java

@@ -23,6 +23,7 @@
 import org.labkey.api.security.Encryption;
 import org.labkey.api.security.Encryption.DecryptionException;
 import org.labkey.api.security.Encryption.EncryptionMigrationHandler;
+import org.labkey.api.security.Encryption.AESConfig;
 import org.labkey.api.settings.AppProps;
 import org.labkey.api.util.ConfigurationException;
 import org.labkey.api.util.logging.LogHelper;
@@ -94,8 +95,10 @@ protected void appendWhereFilter(SQLFragment sql)
     }
 
     @Override
-    public void migrateEncryptedContent(String oldPassPhrase, String keySource)
+    public void migrateEncryptedContent(String oldPassPhrase, String keySource, AESConfig config)
     {
+        Encryption.Algorithm oldAes = Encryption.getAES128(oldPassPhrase, keySource, config);
+
         LOG.info("  Attempting to migrate encrypted property store values");
         TableInfo sets = PropertySchema.getInstance().getTableInfoPropertySets();
         TableInfo props = PropertySchema.getInstance().getTableInfoProperties();
@@ -104,7 +107,7 @@ public void migrateEncryptedContent(String oldPassPhrase, String keySource)
             int set = (int)map.get("Set");
             String encryption = (String)map.get("Encryption");
             String propertySetName = "\"" + map.get("Category") + "\" (Set = " + set + ")";
-            LOG.info("    Attempting to migrate encrypted property set " + propertySetName);
+            LOG.info("    Attempting to migrate encrypted property set {}", propertySetName);
             PropertyEncryption pe = PropertyEncryption.getBySerializedName(encryption);
 
             if (null != pe)
@@ -117,11 +120,24 @@ public void migrateEncryptedContent(String oldPassPhrase, String keySource)
                     {
                         String name = (String) m.get("Name");
                         String encryptedValue = (String) m.get("Value");
-                        LOG.info("      Attempting to decrypt property \"" + name + "\"");
-                        String decryptedValue = pe.decrypt(Base64.decodeBase64(encryptedValue), oldPassPhrase, keySource);
-                        String newEncryptedValue = Base64.encodeBase64String(pe.encrypt(decryptedValue));
-                        assert decryptedValue.equals(pe.decrypt(Base64.decodeBase64(newEncryptedValue)));
-                        newProps.put(name, newEncryptedValue);
+
+                        String newEncryptedValue;
+                        try
+                        {
+                            LOG.info("      Attempting to decrypt property \"{}\"", name);
+                            String decryptedValue = oldAes.decrypt(Base64.decodeBase64(encryptedValue));
+                            newEncryptedValue = Base64.encodeBase64String(pe.encrypt(decryptedValue));
+                            assert decryptedValue.equals(pe.decrypt(Base64.decodeBase64(newEncryptedValue)));
+                            if (newEncryptedValue != null)
+                            {
+                                newProps.put(name, newEncryptedValue);
+                            }
+
+                        }
+                        catch (DecryptionException e)
+                        {
+                            LOG.warn("      Failed to decrypt property \"{}\". Skipping.", name);
+                        }
                     }
 
                     for (Map.Entry<String, String> entry : newProps.entrySet())
@@ -132,15 +148,15 @@ public void migrateEncryptedContent(String oldPassPhrase, String keySource)
                         }
                         catch (RuntimeSQLException e)
                         {
-                            LOG.error("Failed to save re-encrypted property \"" + entry.getKey() + "\"", e);
+                            LOG.error("Failed to save re-encrypted property \"{}\"", entry.getKey(), e);
                         }
                     }
 
-                    LOG.info("    Successfully migrated encrypted property set " + propertySetName);
+                    LOG.info("    Successfully migrated encrypted property set {}", propertySetName);
                 }
                 catch (DecryptionException e)
                 {
-                    LOG.warn("    Failed to decrypt the previous property. Skipping encrypted property set " + propertySetName);
+                    LOG.warn("    Failed to decrypt the previous property. Skipping encrypted property set {}", propertySetName);
                 }
             }
         });

api/src/org/labkey/api/data/PropertyEncryption.java

@@ -17,7 +17,6 @@
 
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
-import org.labkey.api.security.Encryption;
 import org.labkey.api.settings.AppProps;
 import org.labkey.api.util.Compress;
 import org.labkey.api.util.ConfigurationException;
@@ -39,19 +38,13 @@ public enum PropertyEncryption
     None
         {
             @Override
-            public @NotNull byte[] encrypt(@NotNull String plainText)
+            public byte @NotNull[] encrypt(@NotNull String plainText)
             {
                 throw new IllegalStateException("Incorrect PropertyStore for this PropertyMap");
             }
 
             @Override
-            public @NotNull String decrypt(@NotNull byte[] cipherText)
-            {
-                throw new IllegalStateException("Incorrect PropertyStore for this PropertyMap");
-            }
-
-            @Override
-            public @NotNull String decrypt(@NotNull byte[] cipherText, String encryptionPassPhrase, String keySource)
+            public @NotNull String decrypt(byte @NotNull[] cipherText)
             {
                 throw new IllegalStateException("Incorrect PropertyStore for this PropertyMap");
             }
@@ -66,13 +59,13 @@ public enum PropertyEncryption
     Test
         {
             @Override
-            public @NotNull byte[] encrypt(@NotNull String plainText)
+            public byte @NotNull[] encrypt(@NotNull String plainText)
             {
                 return Compress.deflate(plainText);
             }
 
             @Override
-            public @NotNull String decrypt(@NotNull byte[] cipherText)
+            public @NotNull String decrypt(byte @NotNull[] cipherText)
             {
                 try
                 {
@@ -84,12 +77,6 @@ public enum PropertyEncryption
                 }
             }
 
-            @Override
-            public @NotNull String decrypt(@NotNull byte[] cipherText, String encryptionPassPhrase, String keySource)
-            {
-                return decrypt(cipherText);
-            }
-
             @Override
             public @NotNull String getSerializedName()
             {
@@ -100,19 +87,13 @@ public enum PropertyEncryption
     NoKey
         {
             @Override
-            public @NotNull byte[] encrypt(@NotNull String plainText)
+            public byte @NotNull[] encrypt(@NotNull String plainText)
             {
                 throw getConfigurationException();
             }
 
             @Override
-            public @NotNull String decrypt(@NotNull byte[] cipherText)
-            {
-                throw getConfigurationException();
-            }
-
-            @Override
-            public @NotNull String decrypt(@NotNull byte[] cipherText, String encryptionPassPhrase, String keySource)
+            public @NotNull String decrypt(byte @NotNull[] cipherText)
             {
                 throw getConfigurationException();
             }
@@ -132,33 +113,26 @@ private ConfigurationException getConfigurationException()
     AES128
         {
             @Override
-            public @NotNull byte[] encrypt(@NotNull String plainText)
+            public byte @NotNull[] encrypt(@NotNull String plainText)
             {
                 return AES.get().encrypt(plainText);
             }
 
             @Override
-            public @NotNull String decrypt(@NotNull byte[] cipherText)
+            public @NotNull String decrypt(byte @NotNull[] cipherText)
             {
                 return AES.get().decrypt(cipherText);
             }
 
-            @Override
-            public @NotNull String decrypt(@NotNull byte[] cipherText, String encryptionPassPhrase, String keySource)
-            {
-                return Encryption.getAES128(encryptionPassPhrase, keySource).decrypt(cipherText);
-            }
-
             @Override
             public @NotNull String getSerializedName()
             {
                 return "AES128";
             }
         };
 
-    public abstract @NotNull byte[] encrypt(@NotNull String plainText);
-    public abstract @NotNull String decrypt(@NotNull byte[] cipherText);
-    public abstract @NotNull String decrypt(@NotNull byte[] cipherText, String encryptionPassPhrase, String keySource);
+    public abstract byte @NotNull[] encrypt(@NotNull String plainText);
+    public abstract @NotNull String decrypt(byte @NotNull[] cipherText);
 
     // Canonical name to store in the property set. Do not change these return values, once they are in use!
     // Consider: if we need to, could change to a collection of names, the first being canonical, for backward

api/src/org/labkey/api/data/PropertyManager.java

@@ -24,7 +24,6 @@
 import org.apache.commons.collections4.map.AbstractMapDecorator;
 import org.apache.commons.collections4.map.UnmodifiableEntrySet;
 import org.apache.commons.collections4.set.UnmodifiableSet;
-import org.apache.commons.lang3.StringUtils;
 import org.apache.commons.lang3.Strings;
 import org.apache.logging.log4j.Logger;
 import org.jetbrains.annotations.NotNull;

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

@@ -248,7 +248,7 @@ public static boolean isLdapOrSsoEmail(ValidEmail validEmail)
     public static boolean isLdapOrSsoEmail(String emailAddress)
     {
         return AuthenticationConfigurationCache.getActiveDomains().stream()
-            .anyMatch(domain->StringUtils.endsWithIgnoreCase(emailAddress, "@" + domain));
+            .anyMatch(domain->Strings.CI.endsWith(emailAddress, "@" + domain));
     }
 
     public static boolean isRegistrationEnabled()
@@ -327,9 +327,9 @@ public static void reorderConfigurations(User user, String name, int[] rowIds)
         }
     }
 
-    static final EncryptionMigrationHandler ENCRYPTION_MIGRATION_HANDLER = (oldPassPhrase, keySource) -> {
+    static final EncryptionMigrationHandler ENCRYPTION_MIGRATION_HANDLER = (oldPassPhrase, keySource, oldConfig) -> {
+        Algorithm decryptAes = Encryption.getAES128(oldPassPhrase, keySource, oldConfig);
         _log.info("  Attempting to migrate encrypted properties in authentication configurations");
-        Algorithm decryptAes = Encryption.getAES128(oldPassPhrase, keySource);
         TableInfo tinfo = CoreSchema.getInstance().getTableInfoAuthenticationConfigurations();
         Map<Integer, String> map = new TableSelector(tinfo, PageFlowUtil.set("RowId", "EncryptedProperties"),
                 new SimpleFilter(FieldKey.fromParts("EncryptedProperties"), null, CompareType.NONBLANK), null).getValueMap(Integer.class);
@@ -339,15 +339,22 @@ public static void reorderConfigurations(User user, String name, int[] rowIds)
             try
             {
                 _log.info("    Migrating encrypted properties for configuration " + key);
-                String decryptedValue = decryptAes.decrypt(Base64.decodeBase64(value));
-                String newEncryptedValue = Base64.encodeBase64String(AES.get().encrypt(decryptedValue));
-                saveMap.put("EncryptedProperties", newEncryptedValue);
-                assert decryptedValue.equals(AES.get().decrypt(Base64.decodeBase64(newEncryptedValue)));
-                Table.update(null, tinfo, saveMap, key);
-            }
-            catch (DecryptionException e)
-            {
-                _log.info("    Failed to decrypt encrypted properties for configuration " + key + ". It will be skipped.");
+                try
+                {
+                    String decryptedValue = decryptAes.decrypt(Base64.decodeBase64(value));
+                    String newEncryptedValue = Base64.encodeBase64String(AES.get().encrypt(decryptedValue));
+                    assert decryptedValue.equals(AES.get().decrypt(Base64.decodeBase64(newEncryptedValue)));
+
+                    if (newEncryptedValue != null)
+                    {
+                        saveMap.put("EncryptedProperties", newEncryptedValue);
+                        Table.update(null, tinfo, saveMap, key);
+                    }
+                }
+                catch (DecryptionException e)
+                {
+                    _log.info("    Failed to decrypt encrypted properties for configuration " + key + ". It will be skipped.");
+                }
             }
             catch (Exception e)
             {