fapifta commented on code in PR #6860: URL: https://github.com/apache/ozone/pull/6860#discussion_r1656933246
########## hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/conf/DelegatingProperties.java: ########## @@ -0,0 +1,173 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hdds.conf; + +import org.apache.hadoop.ozone.OzoneConfigKeys; + +import java.util.Arrays; +import java.util.Collection; +import java.util.Enumeration; +import java.util.Map; +import java.util.Properties; +import java.util.Set; + +import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_SECURITY_CRYPTO_COMPLIANCE_MODE_UNRESTRICTED; + +/** + * Delegating properties helper class. + */ +public class DelegatingProperties extends Properties { + private final Properties properties; + private final String complianceMode; + private final boolean checkCompliance; + private final Properties cryptoProperties; + + public DelegatingProperties(Properties properties, String complianceMode, Properties cryptoProperties) { + this.properties = properties; + this.complianceMode = complianceMode; + this.checkCompliance = !complianceMode.equals(OZONE_SECURITY_CRYPTO_COMPLIANCE_MODE_UNRESTRICTED); + this.cryptoProperties = cryptoProperties; + } + + public String checkCompliance(String config, String value) { + // Don't check the ozone.security.crypto.compliance.mode config, even though it's tagged as a crypto config + if (checkCompliance && cryptoProperties.containsKey(config) && + !config.equals(OzoneConfigKeys.OZONE_SECURITY_CRYPTO_COMPLIANCE_MODE)) { + + String whitelistConfig = config + "." + complianceMode + ".whitelist"; + String whitelistValue = properties.getProperty(whitelistConfig, ""); + + if (whitelistValue != null) { + String[] whitelistOptions = whitelistValue.split(","); Review Comment: Just simply splitting the string might result in values like " val ", if someone puts extra spaces to the list. I think we may trim the extra spaces from the values in the options. What do you think? ########## hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/LegacyHadoopConfigurationSource.java: ########## @@ -76,6 +164,12 @@ public static Configuration asHadoopConfiguration( } } + /** + * Use this with beware, as we are returning the Configuration object, without compliance checks. + * Currently it's only used in the OzoneConfiguration class, in an OzoneConfiguration constructor, + * so we'll handle the compliance checks in the OzoneConfiguration object. + * @return + */ public Configuration getOriginalHadoopConfiguration() { Review Comment: I kind of tend to believe that we should move this class into the org.apache.hadoop.hdds.conf package in a followup JIRA, so we can make this method and the DelegatingProperties class package private... What do you think? ########## hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/conf/DelegatingProperties.java: ########## @@ -0,0 +1,173 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hdds.conf; + +import org.apache.hadoop.ozone.OzoneConfigKeys; + +import java.util.Arrays; +import java.util.Collection; +import java.util.Enumeration; +import java.util.Map; +import java.util.Properties; +import java.util.Set; + +import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_SECURITY_CRYPTO_COMPLIANCE_MODE_UNRESTRICTED; + +/** + * Delegating properties helper class. Review Comment: Based on what we discussed already in person about the intents of this class, can you please add some more context on why this class exists, and how it is needed to ensure crypto properties are checked? Also please note in the apidoc that this class is designed to be used with the configuration related classes only, and is public because the two places where we need it is in different packages, but there should be no other uses for this class elsewhere? ########## hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/conf/OzoneConfiguration.java: ########## @@ -407,4 +420,76 @@ public int getInt(String name, String fallbackName, int defaultValue, } return Integer.parseInt(value); } + + private Properties delegatingProps; + + @Override + public synchronized void reloadConfiguration() { + super.reloadConfiguration(); + delegatingProps = null; + } + + @Override + protected final synchronized Properties getProps() { + if (delegatingProps == null) { + delegatingProps = new DelegatingProperties(super.getProps(), complianceMode, cryptoProperties); + } + return delegatingProps; + } + + /** + * Get a property value without the compliance check. It's needed to get the compliance + * mode and the whitelist parameter values in the checkCompliance method. + * + * @param key property name + * @param defaultValue default value + * @return property value, without compliance check + */ + private String getPropertyUnsafe(String key, String defaultValue) { + return super.getProps().getProperty(key, defaultValue); + } + + private Properties getCryptoProperties() { + try { + return super.getAllPropertiesByTag(ConfigTag.CRYPTO_COMPLIANCE.toString()); + } catch (NoSuchMethodError e) { + return new Properties(); + } + } + + public String checkCompliance(String config, String value) { + // Don't check the ozone.security.crypto.compliance.mode config, even though it's tagged as a crypto config + if (checkCompliance && cryptoProperties.containsKey(config) && + !config.equals(OzoneConfigKeys.OZONE_SECURITY_CRYPTO_COMPLIANCE_MODE)) { + + String whitelistConfig = config + "." + complianceMode + ".whitelist"; + String whitelistValue = getPropertyUnsafe(whitelistConfig, ""); + + if (whitelistValue != null) { + String[] whitelistOptions = whitelistValue.split(","); + + if (!Arrays.asList(whitelistOptions).contains(value)) { + throw new ConfigurationException("Not allowed configuration value! Compliance mode is set to " + + complianceMode + " and " + config + " configuration's value is not allowed. Please check the " + + whitelistConfig + " configuration."); + } + } + } + return value; + } + + @Override + public Iterator<Map.Entry<String, String>> iterator() { + Properties properties = getProps(); Review Comment: As we use getProps here, which in turn returns a DelegatingProperties object, where there is also a compliance checker method, can we just implement the iterator creation there, using the compliance check of DelegatingProperties, so that we can remove the duplication here? It would also make it possible to remove the checkCompliance and the complianceMode variables from here, and would let us move the query for these values and for the cryptoproperties into the getProps() method. As I remember this might also decrease the class initialization time, that would help to re-enable the test in the TestSnapshotManager class (though about that I am not sure, but at least it is worth checking if this suggestion can be done). I know this is done similarly in LegacyHadoopConfiguration also, and I might miss a nuance difference, but it would be great if we can do this both here and there... If you already tried and it fails, then ignore this comment, however in this case please leave a note somewhere in the code so that we don't lost the knowledge of why we have this code in all the three places. ########## hadoop-hdds/common/dev-support/findbugsExcludeFile.xml: ########## @@ -31,6 +31,10 @@ <Method name="update" /> <Bug pattern="SF_SWITCH_FALLTHROUGH,SF_SWITCH_NO_DEFAULT" /> </Match> + <Match> + <Class name="org.apache.hadoop.hdds.utils.LegacyHadoopConfigurationSource$1$1" /> Review Comment: After introducing DelegatingProperties, is this exlusion is still something that we need? ########## hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/conf/OzoneConfiguration.java: ########## @@ -407,4 +420,76 @@ public int getInt(String name, String fallbackName, int defaultValue, } return Integer.parseInt(value); } + + private Properties delegatingProps; + + @Override + public synchronized void reloadConfiguration() { + super.reloadConfiguration(); + delegatingProps = null; + } + + @Override + protected final synchronized Properties getProps() { + if (delegatingProps == null) { + delegatingProps = new DelegatingProperties(super.getProps(), complianceMode, cryptoProperties); + } + return delegatingProps; + } + + /** + * Get a property value without the compliance check. It's needed to get the compliance + * mode and the whitelist parameter values in the checkCompliance method. + * + * @param key property name + * @param defaultValue default value + * @return property value, without compliance check + */ + private String getPropertyUnsafe(String key, String defaultValue) { + return super.getProps().getProperty(key, defaultValue); + } + + private Properties getCryptoProperties() { + try { + return super.getAllPropertiesByTag(ConfigTag.CRYPTO_COMPLIANCE.toString()); + } catch (NoSuchMethodError e) { Review Comment: As we discussed, this catch is related to Hadoop 2 compatibility, can you please add some comments here to make it clear why we catch the NoSuchMethodError, and what that means from the functionality point of view? ########## hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/conf/TestOzoneConfiguration.java: ########## @@ -151,6 +152,67 @@ public void getConfigurationObject() { assertEquals(Duration.ofSeconds(3), configuration.getDuration()); } + @Test + public void testRestrictedComplianceModeWithOzoneConf() { + Configuration config = new Configuration(); + config.set("ozone.security.crypto.compliance.mode", "restricted"); + OzoneConfiguration ozoneConfig = new OzoneConfiguration(config); + + // Set it to an allowed config value + ozoneConfig.set("hdds.x509.signature.algorithm", "SHA512withDCA"); + ozoneConfig.set("hdds.x509.signature.algorithm.restricted.whitelist", "SHA512withRSA,SHA512withDCA"); + + assertEquals("restricted", ozoneConfig.get("ozone.security.crypto.compliance.mode")); + assertEquals("SHA512withRSA,SHA512withDCA", ozoneConfig.get("hdds.x509.signature.algorithm.restricted.whitelist")); + assertEquals("SHA512withDCA", ozoneConfig.get("hdds.x509.signature.algorithm")); + + // Set it to a disallowed config value + ozoneConfig.set("hdds.x509.signature.algorithm", "SHA256withRSA"); + + assertThrows(ConfigurationException.class, () -> ozoneConfig.get("hdds.x509.signature.algorithm")); + + // Check it with a Hadoop Configuration + Configuration hadoopConfig = + LegacyHadoopConfigurationSource.asHadoopConfiguration(ozoneConfig); + assertThrows(ConfigurationException.class, () -> hadoopConfig.get("hdds.x509.signature.algorithm")); + } + + @Test + public void testRestrictedComplianceModeWithLegacyHadoopConf() { + Configuration config = new Configuration(); + config.addResource("ozone-default.xml"); + config.set("ozone.security.crypto.compliance.mode", "restricted"); + LegacyHadoopConfigurationSource legacyHadoopConf = new LegacyHadoopConfigurationSource(config); + + // Set it to an allowed config value + legacyHadoopConf.set("hdds.x509.signature.algorithm", "SHA512withDCA"); + legacyHadoopConf.set("hdds.x509.signature.algorithm.restricted.whitelist", "SHA512withRSA,SHA512withDCA"); + + assertEquals("restricted", legacyHadoopConf.get("ozone.security.crypto.compliance.mode")); + assertEquals("SHA512withRSA,SHA512withDCA", + legacyHadoopConf.get("hdds.x509.signature.algorithm.restricted.whitelist")); + assertEquals("SHA512withDCA", legacyHadoopConf.get("hdds.x509.signature.algorithm")); + + // Set it to a disallowed config value + legacyHadoopConf.set("hdds.x509.signature.algorithm", "SHA256withRSA"); + + assertThrows(ConfigurationException.class, () -> legacyHadoopConf.get("hdds.x509.signature.algorithm")); + + // Check it with a Hadoop Configuration + Configuration legacyConf = LegacyHadoopConfigurationSource.asHadoopConfiguration(legacyHadoopConf); + assertThrows(ConfigurationException.class, () -> legacyConf.get("hdds.x509.signature.algorithm")); Review Comment: As tests are somewhat to define behaviour, to demonstrate the difference between asHadoopConfiguration and getOriginalHadoopConfiguration, we may add an assertion here that the original configuration object does not throw the exception even if there is an invalid value set. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
