HDDS-10604. Whitelist based compliance check for crypto related configuration options

[dombizita]

What changes were proposed in this pull request?

The goal of this is change is that whenever we ask for a configuration option's value, we are ensuring to do the necessary compliance checks.

In Ozone we have:

• OzoneConfiguration class, which extends the Hadoop's Configuration class. It also implements the MutableConfigurationSource Ozone interface.
• LegacyHadoopConfigurationSource (Configuration source to wrap Hadoop Configuration object), which also implements MutableConfigurationSource. This has a Configuration field, which is used to get configuration options.
• InMemoryConfiguration, which is only used for testing, so I didn't change anything there.

I went through the code and checked how do we get configs. Overall we want to ensure if we get any config property, the compliance check went through before returning them. To make this happen, we only need to make sure that the Configuration#getProps method gives us a Properties object back, thats getProperty method does the compliance check. I created a DelegatingProperties helper class, so we'll delegate the operations to the parent's properties and also do the compliance check in the getProperty. I also added the compliance check to the Properties iterator(), as with that we can access the properties values without calling the getProperty() method.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-10604

How was this patch tested?

Added unit tests, CI: https://github.com/dombizita/ozone/actions/runs/9645109413

[fapifta]

Thank you for your work on this one @dombizita!

I know we have looked at problems arose in CI runs together, but now as I look at the changes altogether a few things I realized, please find my comments inline with a few suggestions/question/ideas.

[dombizita]

With the recent changes I added, when we call the entrySet() method in the DelegatingProperties class it throws UnsupportedOperationException. The goal with this is that we can't get values from its Properties object without the compliance check. I thought that the entrySet is only used in the iterator() method (which I handle in the DelegatingProperties class), but from the failures we see that in the Configuration class we use it elsewhere too, e.g. in the getValByRegex() method.

Error:  Tests run: 6, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.654 s <<< FAILURE! - in org.apache.hadoop.hdds.TestHddsUtils
Error:  org.apache.hadoop.hdds.TestHddsUtils.testRedactSensitivePropsForLogging  Time elapsed: 0.015 s  <<< ERROR!
java.lang.UnsupportedOperationException: Entryset is not available in DelegatingProperties
	at org.apache.hadoop.hdds.conf.DelegatingProperties.entrySet(DelegatingProperties.java:165)
	at org.apache.hadoop.conf.Configuration.getValByRegex(Configuration.java:3935)
	at org.apache.hadoop.hdds.conf.OzoneConfiguration.getOzoneProperties(OzoneConfiguration.java:281)
	at org.apache.hadoop.hdds.HddsUtils.processForLogging(HddsUtils.java:792)
	at org.apache.hadoop.hdds.TestHddsUtils.testRedactSensitivePropsForLogging(TestHddsUtils.java:256)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

I'll look into this later.

[dombizita]

I checked where do we use the entrySet() method in the Configuration class on its Properties (I didn't find usages in other configuration related classes). These are the occurrences:

• iterator() - it's handled as we overwrite the iterator() method in both the OzoneConfiguration and the LegacyHadoopConfigurationSource classes
• overlay() - we are just putting everything form one Properties object to another Properties object. This shouldn't cause problems, as later when we read from them we will do the compliance check
• dumpConfiguration() - it doesn't call the getValue() on the entrySet element, shouldn't be a problem
• write() - this will call the item.getValue(), but it will only write it out to a DataOutput, so once we read from this, we will do the compliance check
• getValByRegex() - we are calling the item.getValue(), bust we are only checking if it's a String, we don't do anything else with it. Later when we are returning with the key value map, we are getting the value with this call substituteVars(getProps().getProperty(item)) where the item is the key name.

Based on this, we can say that the only occurrence where we are actually doing something with a configuration value that we got through a props.entrySet() call is handled. But because of the other usages I can't throw an UnsupportedOperationException. I update my patch, so I can see a better CI run, based on the ongoing review I can change it later.

[fapifta]

Thank you @dombizita for going after all the possible cases where we use the entryset, and also to summarize it here. It is really pleasant to look at the change overall as it became elegant and simple at the end of the day, so even it took some time, I think it does worth it looking at what we have now.