You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2022/12/12 16:25:52 UTC
[GitHub] [cloudstack] weizhouapache commented on a diff in pull request #6812: Normalize encryption on global configurations values
weizhouapache commented on code in PR #6812:
URL: https://github.com/apache/cloudstack/pull/6812#discussion_r1046022231
##########
engine/schema/src/main/java/com/cloud/upgrade/dao/Upgrade41710to41800.java:
##########
@@ -151,6 +188,28 @@ protected Map<Long, Date> selectTariffs(Connection conn, Integer usageType, bool
}
}
+ 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.",
Review Comment:
@BryanMLima
does it make sense to ignore the error ?
I would prefer to throw an exception.
##########
engine/schema/src/main/java/com/cloud/upgrade/dao/Upgrade41710to41800.java:
##########
@@ -98,6 +104,37 @@ public void updateSystemVmTemplates(Connection conn) {
}
}
+ protected void decryptGlobalConfigurationValues(Connection conn) {
Review Comment:
the method name is misunderstanding.
only the Hidden/Secure configuration values in account and domain details are NOT decrypted, all others are decrypted.
##########
engine/schema/src/main/java/com/cloud/upgrade/dao/Upgrade41710to41800.java:
##########
@@ -151,6 +188,28 @@ protected Map<Long, Date> selectTariffs(Connection conn, Integer usageType, bool
}
}
+ 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);
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(message, e);
Review Comment:
this is not needed
##########
engine/schema/src/main/java/com/cloud/upgrade/dao/Upgrade41710to41800.java:
##########
@@ -98,6 +104,37 @@ public void updateSystemVmTemplates(Connection conn) {
}
}
+ protected void decryptGlobalConfigurationValues(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') ORDER BY details.id;", table);
Review Comment:
@BryanMLima
Might it be possible that the value is null or blank ?
it would be good to add a condition check in the sql query.
##########
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java:
##########
@@ -856,10 +856,24 @@ public Configuration updateConfiguration(final UpdateCfgCmd cmd) throws InvalidP
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 {
+ category = config.getCategory();
+ }
+
+ String value = cmd.getValue();
+ boolean isConfigEncrypted = config != null && config.isEncrypted();
+ if (isConfigEncrypted) {
+ value = DBEncryptionUtil.encrypt(value);
+ }
+
+ String eventValue;
+ if (isConfigEncrypted) {
Review Comment:
these lines can be combined into a line
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org