Normalize encryption on global configurations values (#6812)

api/src/main/java/org/apache/cloudstack/api/command/admin/config/UpdateCfgCmd.java

@@ -16,6 +16,7 @@
 // under the License.
 package org.apache.cloudstack.api.command.admin.config;
 
+import com.cloud.utils.crypt.DBEncryptionUtil;
 import org.apache.cloudstack.acl.RoleService;
 import org.apache.cloudstack.api.response.DomainResponse;
 import org.apache.log4j.Logger;
@@ -150,25 +151,50 @@ public class UpdateCfgCmd extends BaseCmd {
         if (cfg != null) {
             ConfigurationResponse response = _responseGenerator.createConfigurationResponse(cfg);
             response.setResponseName(getCommandName());
-            if (getZoneId() != null) {
+            response = setResponseScopes(response);
-                response.setScope("zone");
+            response = setResponseValue(response, cfg);
-            }
-            if (getClusterId() != null) {
-                response.setScope("cluster");
-            }
-            if (getStoragepoolId() != null) {
-                response.setScope("storagepool");
-            }
-            if (getAccountId() != null) {
-                response.setScope("account");
-            }
-            if (getDomainId() != null) {
-                response.setScope("domain");
-            }
-            response.setValue(value);
             this.setResponseObject(response);
         } else {
             throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to update config");
         }
     }
+
+    /**
+     * Sets the configuration value in the response. If the configuration is in the `Hidden` or `Secure` categories, the value is encrypted before being set in the response.
+     * @param response to be set with the configuration `cfg` value
+     * @param cfg to be used in setting the response value
+     * @return the response with the configuration's value
+     */
+    public ConfigurationResponse setResponseValue(ConfigurationResponse response, Configuration cfg) {
+        if (cfg.isEncrypted()) {
+            response.setValue(DBEncryptionUtil.encrypt(getValue()));
+        } else {
+            response.setValue(getValue());
+        }
+        return response;
+    }
+
+    /**
+     * Sets the scope for the Configuration response only if the field is not null.
+     * @param response to be updated
+     * @return the response updated with the scopes
+     */
+    public ConfigurationResponse setResponseScopes(ConfigurationResponse response) {
+        if (getZoneId() != null) {
+            response.setScope("zone");
+        }
+        if (getClusterId() != null) {
+            response.setScope("cluster");
+        }
+        if (getStoragepoolId() != null) {
+            response.setScope("storagepool");
+        }
+        if (getAccountId() != null) {
+            response.setScope("account");
+        }
+        if (getDomainId() != null) {
+            response.setScope("domain");
+        }
+        return response;
+    }
 }

engine/schema/src/main/java/com/cloud/domain/DomainDetailVO.java

@@ -23,7 +23,6 @@ import javax.persistence.GenerationType;
 import javax.persistence.Id;
 import javax.persistence.Table;
 
-import com.cloud.utils.db.Encrypt;
 import org.apache.cloudstack.api.InternalIdentity;
 
 @Entity
@@ -40,7 +39,6 @@ public class DomainDetailVO implements InternalIdentity {
     @Column(name = "name")
     private String name;
 
-    @Encrypt
     @Column(name = "value")
     private String value;
 

engine/schema/src/main/java/com/cloud/upgrade/dao/Upgrade41810to41900.java

@@ -17,15 +17,19 @@
 package com.cloud.upgrade.dao;
 
 import com.cloud.upgrade.SystemVmTemplateRegistration;
+import com.cloud.utils.crypt.DBEncryptionUtil;
 import com.cloud.utils.DateUtil;
 import com.cloud.utils.exception.CloudRuntimeException;
 import org.apache.log4j.Logger;
+import org.jasypt.exceptions.EncryptionOperationNotPossibleException;
 
 import java.io.InputStream;
 import java.sql.Connection;
 import java.sql.PreparedStatement;
 import java.sql.ResultSet;
 import java.sql.SQLException;
+import java.util.HashMap;
+import java.util.Map;
 import java.text.ParseException;
 import java.text.SimpleDateFormat;
 import java.util.Date;
@@ -34,6 +38,10 @@ public class Upgrade41810to41900 implements DbUpgrade, DbUpgradeSystemVmTemplate
     final static Logger LOG = Logger.getLogger(Upgrade41810to41900.class);
     private SystemVmTemplateRegistration systemVmTemplateRegistration;
 
+    private static final String ACCOUNT_DETAILS = "account_details";
+
+    private static final String DOMAIN_DETAILS = "domain_details";
+
     private final SimpleDateFormat[] formats = {
             new SimpleDateFormat("yyyy-MM-dd HH:mm:ss"), new SimpleDateFormat("MM/dd/yyyy HH:mm:ss"), new SimpleDateFormat("dd/MM/yyyy HH:mm:ss"),
             new SimpleDateFormat("EEE MMM dd HH:mm:ss z yyyy")};
@@ -66,6 +74,7 @@ public class Upgrade41810to41900 implements DbUpgrade, DbUpgradeSystemVmTemplate
 
     @Override
     public void performDataMigration(Connection conn) {
+        decryptConfigurationValuesFromAccountAndDomainScopesNotInSecureHiddenCategories(conn);
         migrateBackupDates(conn);
     }
 
@@ -95,6 +104,37 @@ public class Upgrade41810to41900 implements DbUpgrade, DbUpgradeSystemVmTemplate
         }
     }
 
+    protected void decryptConfigurationValuesFromAccountAndDomainScopesNotInSecureHiddenCategories(Connection conn) {
+        LOG.info("Decrypting global configuration values from the following tables: account_details and domain_details.");
+
+        Map<Long, String> accountsMap = getConfigsWithScope(conn, ACCOUNT_DETAILS);
+        updateConfigValuesWithScope(conn, accountsMap, ACCOUNT_DETAILS);
+        LOG.info("Successfully decrypted configurations from account_details table.");
+
+        Map<Long, String> domainsMap = getConfigsWithScope(conn, DOMAIN_DETAILS);
+        updateConfigValuesWithScope(conn, domainsMap, DOMAIN_DETAILS);
+        LOG.info("Successfully decrypted configurations from domain_details table.");
+    }
+
+    protected Map<Long, String> getConfigsWithScope(Connection conn, String table) {
+        Map<Long, String> configsToBeUpdated = new HashMap<>();
+        String selectDetails = String.format("SELECT details.id, details.value from cloud.%s details, cloud.configuration c " +
+                "WHERE details.name = c.name AND c.category NOT IN ('Hidden', 'Secure') AND details.value <> \"\" ORDER BY details.id;", table);
+
+        try (PreparedStatement pstmt = conn.prepareStatement(selectDetails)) {
+            try (ResultSet result = pstmt.executeQuery()) {
+                while (result.next()) {
+                    configsToBeUpdated.put(result.getLong("id"), result.getString("value"));
+                }
+            }
+            return configsToBeUpdated;
+        } catch (SQLException e) {
+            String message = String.format("Unable to retrieve data from table [%s] due to [%s].", table, e.getMessage());
+            LOG.error(message, e);
+            throw new CloudRuntimeException(message, e);
+        }
+    }
+
     public void migrateBackupDates(Connection conn) {
         LOG.info("Trying to convert backups' date column from varchar(255) to datetime type.");
 
@@ -125,6 +165,27 @@ public class Upgrade41810to41900 implements DbUpgrade, DbUpgradeSystemVmTemplate
         }
     }
 
+    protected void updateConfigValuesWithScope(Connection conn, Map<Long, String> configsToBeUpdated, String table) {
+        String updateConfigValues = String.format("UPDATE cloud.%s SET value = ? WHERE id = ?;", table);
+
+        for (Map.Entry<Long, String> config : configsToBeUpdated.entrySet()) {
+            try (PreparedStatement pstmt = conn.prepareStatement(updateConfigValues)) {
+                String decryptedValue = DBEncryptionUtil.decrypt(config.getValue());
+
+                pstmt.setString(1, decryptedValue);
+                pstmt.setLong(2, config.getKey());
+
+                LOG.info(String.format("Updating config with ID [%s] to value [%s].", config.getKey(), decryptedValue));
+                pstmt.executeUpdate();
+            } catch (SQLException | EncryptionOperationNotPossibleException e) {
+                String message = String.format("Unable to update config value with ID [%s] on table [%s] due to [%s]. The config value may already be decrypted.",
+                        config.getKey(), table, e);
+                LOG.error(message);
+                throw new CloudRuntimeException(message, e);
+            }
+        }
+    }
+
     private void fetchDatesAndMigrateToNewColumn(Connection conn) {
         String selectBackupDates = "SELECT `id`, `old_date` FROM `cloud`.`backups` WHERE 1;";
         String date;

engine/schema/src/main/java/com/cloud/user/AccountDetailVO.java

@@ -25,8 +25,6 @@ import javax.persistence.Table;
 
 import org.apache.cloudstack.api.InternalIdentity;
 
-import com.cloud.utils.db.Encrypt;
-
 @Entity
 @Table(name = "account_details")
 public class AccountDetailVO implements InternalIdentity {
@@ -41,7 +39,6 @@ public class AccountDetailVO implements InternalIdentity {
     @Column(name = "name")
     private String name;
 
-    @Encrypt
     @Column(name = "value", length=4096)
     private String value;
 

framework/config/src/main/java/org/apache/cloudstack/framework/config/impl/ConfigurationVO.java

@@ -170,7 +170,7 @@ public class ConfigurationVO implements Configuration {
 
     @Override
     public boolean isEncrypted() {
-        return "Hidden".equals(getCategory()) || "Secure".equals(getCategory());
+        return StringUtils.equalsAny(getCategory(), "Hidden", "Secure");
     }
 
     @Override

framework/db/src/main/java/com/cloud/utils/crypt/EncryptionSecretKeyChanger.java

@@ -475,6 +475,8 @@ public class EncryptionSecretKeyChanger {
 
             // migrate resource details values
             migrateHostDetails(conn);
+            migrateEncryptedAccountDetails(conn);
+            migrateEncryptedDomainDetails(conn);
             migrateClusterDetails(conn);
             migrateImageStoreDetails(conn);
             migrateStoragePoolDetails(conn);
@@ -497,6 +499,30 @@ public class EncryptionSecretKeyChanger {
         return true;
     }
 
+    private void migrateEncryptedAccountDetails(Connection conn) {
+        System.out.println("Beginning migration of account_details encrypted values");
+
+        String tableName = "account_details";
+        String selectSql = "SELECT details.id, details.value from account_details details, cloud.configuration c " +
+                "WHERE details.name = c.name AND c.category IN ('Hidden', 'Secure') AND details.value <> \"\" ORDER BY details.id;";
+        String updateSql = "UPDATE cloud.account_details SET value = ? WHERE id = ?;";
+        migrateValueAndUpdateDatabaseById(conn, tableName, selectSql, updateSql, false);
+
+        System.out.println("End migration of account details values");
+    }
+
+    private void migrateEncryptedDomainDetails(Connection conn) {
+        System.out.println("Beginning migration of domain_details encrypted values");
+
+        String tableName = "domain_details";
+        String selectSql = "SELECT details.id, details.value from domain_details details, cloud.configuration c " +
+                "WHERE details.name = c.name AND c.category IN ('Hidden', 'Secure') AND details.value <> \"\" ORDER BY details.id;";
+        String updateSql = "UPDATE cloud.domain_details SET value = ? WHERE id = ?;";
+        migrateValueAndUpdateDatabaseById(conn, tableName, selectSql, updateSql, false);
+
+        System.out.println("End migration of domain details values");
+    }
+
     protected String migrateValue(String value) {
         if (StringUtils.isEmpty(value)) {
             return value;

server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java

@@ -47,6 +47,7 @@ import javax.naming.ConfigurationException;
 
 
 import com.cloud.hypervisor.HypervisorGuru;
+import com.cloud.utils.crypt.DBEncryptionUtil;
 import org.apache.cloudstack.acl.SecurityChecker;
 import org.apache.cloudstack.affinity.AffinityGroup;
 import org.apache.cloudstack.affinity.AffinityGroupService;
@@ -664,7 +665,7 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati
 
     @Override
     @DB
-    public String updateConfiguration(final long userId, final String name, final String category, final String value, final String scope, final Long resourceId) {
+    public String updateConfiguration(final long userId, final String name, final String category, String value, final String scope, final Long resourceId) {
         final String validationMsg = validateConfigurationValue(name, value, scope);
 
         if (validationMsg != null) {
@@ -677,6 +678,11 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati
         // if scope is mentioned as global or not mentioned then it is normal
         // global parameter updation
         if (scope != null && !scope.isEmpty() && !ConfigKey.Scope.Global.toString().equalsIgnoreCase(scope)) {
+            boolean valueEncrypted = shouldEncryptValue(category);
+            if (valueEncrypted) {
+                value = DBEncryptionUtil.encrypt(value);
+            }
+
             switch (ConfigKey.Scope.valueOf(scope)) {
             case Zone:
                 final DataCenterVO zone = _zoneDao.findById(resourceId);
@@ -767,7 +773,8 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati
             default:
                 throw new InvalidParameterValueException("Scope provided is invalid");
             }
-            return value;
+
+            return valueEncrypted ? DBEncryptionUtil.decrypt(value) : value;
         }
 
         // Execute all updates in a single transaction
@@ -864,6 +871,10 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati
         return _configDao.getValue(name);
     }
 
+    private boolean shouldEncryptValue(String category) {
+        return StringUtils.equalsAny(category, "Hidden", "Secure");
+    }
+
     /**
      * Updates the 'hypervisor.list' value to match the new custom hypervisor name set as newValue if the previous value was set
      */
@@ -890,10 +901,11 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati
         final Long imageStoreId = cmd.getImageStoreId();
         Long accountId = cmd.getAccountId();
         Long domainId = cmd.getDomainId();
-        CallContext.current().setEventDetails(" Name: " + name + " New Value: " + (name.toLowerCase().contains("password") ? "*****" : value == null ? "" : value));
         // check if config value exists
         final ConfigurationVO config = _configDao.findByName(name);
-        String catergory = null;
+        String category = null;
+        String eventValue = encryptEventValueIfConfigIsEncrypted(config, value);
+        CallContext.current().setEventDetails(String.format(" Name: %s New Value: %s", name, eventValue));
 
         final Account caller = CallContext.current().getCallingAccount();
         if (_accountMgr.isDomainAdmin(caller.getId())) {
@@ -912,9 +924,9 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati
                 s_logger.warn("Probably the component manager where configuration variable " + name + " is defined needs to implement Configurable interface");
                 throw new InvalidParameterValueException("Config parameter with name " + name + " doesn't exist");
             }
-            catergory = _configDepot.get(name).category();
+            category = _configDepot.get(name).category();
         } else {
-            catergory = config.getCategory();
+            category = config.getCategory();
         }
 
         validateIpAddressRelatedConfigValues(name, value);
@@ -971,7 +983,7 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati
             value = (id == null) ? null : "";
         }
 
-        final String updatedValue = updateConfiguration(userId, name, catergory, value, scope, id);
+        final String updatedValue = updateConfiguration(userId, name, category, value, scope, id);
         if (value == null && updatedValue == null || updatedValue.equalsIgnoreCase(value)) {
             return _configDao.findByName(name);
         } else {
@@ -979,6 +991,13 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati
         }
     }
 
+    private String encryptEventValueIfConfigIsEncrypted(ConfigurationVO config, String value) {
+        if (config != null && config.isEncrypted()) {
+           return  "*****";
+        }
+        return Objects.requireNonNullElse(value, "");
+    }
+
     private ParamCountPair getParamCount(Map<String, Long> scopeMap) {
         Long id = null;
         int paramCount = 0;