----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51705/#review148212 -----------------------------------------------------------
ambari-agent/src/main/python/ambari_agent/ClusterConfiguration.py (line 103) <https://reviews.apache.org/r/51705/#comment215671> This will cause problems since you're storing the obfuscated properties. If a command needed the property from the in-memory cache, it would not have the correct value anymore. Perhaps this approach is flawed in that there's no credential store being used for the in-memory passwords. ambari-agent/src/main/python/ambari_agent/ClusterConfiguration.py (line 173) <https://reviews.apache.org/r/51705/#comment215673> `i` is not a very good name here; maybe something a little clearer. ambari-agent/src/main/python/ambari_agent/ClusterConfiguration.py (lines 178 - 180) <https://reviews.apache.org/r/51705/#comment215674> That's a lot of iterating - over the entire config dictionary per-key that needs replacing - also you never exit once you have a hit. This function needs to be cleaned up for better performance. ambari-server/src/main/java/org/apache/ambari/server/actionmanager/ExecutionCommandWrapper.java (lines 182 - 183) <https://reviews.apache.org/r/51705/#comment215662> What about status commands - those send down configurations too? ambari-server/src/main/java/org/apache/ambari/server/state/ConfigHelper.java (lines 252 - 275) <https://reviews.apache.org/r/51705/#comment215665> This seems like it should be cached once after calculation for the cluster so that it doesn't need to keep generating it over and over. ambari-server/src/main/java/org/apache/ambari/server/state/ConfigHelper.java (line 270) <https://reviews.apache.org/r/51705/#comment215667> You call this property "password config types" and bind a name to a type, but then never check the type for PASSWORD in python. I'd say either make this a flat list of password-protected fields or enhance the python code to extract properties of type PASSWORD to check on. - Jonathan Hurley On Sept. 7, 2016, 5:14 p.m., Anita Jebaraj wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51705/ > ----------------------------------------------------------- > > (Updated Sept. 7, 2016, 5:14 p.m.) > > > Review request for Ambari, Di Li, Jonathan Hurley, and Nate Cole. > > > Bugs: AMBARI-18334 > https://issues.apache.org/jira/browse/AMBARI-18334 > > > Repository: ambari > > > Description > ------- > > The configurations.json file loaded in the ambari-agent cache located at > /var/lib/ambari-agent/cache/cluster_configuration contains password details > in plaintext (Ex: ssl.client.keystore.password,ssl.client.truststore.password > etc.). The values are loaded both in the memory cache and file cache, the > file seems to be used only for debugging purposes, so it would be a better > approach to mask the passwords in the file. > > Approach: > > The password_config_type is included in the heartbeat response for alert > definition command and execution command, for which the values are dumped > into the json file. The password_config_type contains the information on > which properties in the configurations has the propertyType password. Based > on the response, the json is parsed and the password values are masked before > dumping it into the configurations.json file. > > > Diffs > ----- > > ambari-agent/src/main/python/ambari_agent/ClusterConfiguration.py 72b87be > ambari-agent/src/test/python/ambari_agent/TestAlerts.py e114daa > ambari-agent/src/test/python/ambari_agent/TestClusterConfigurationCache.py > a418f6d > > ambari-server/src/main/java/org/apache/ambari/server/actionmanager/ExecutionCommandWrapper.java > 0562c15 > > ambari-server/src/main/java/org/apache/ambari/server/agent/AlertDefinitionCommand.java > 4d2e048 > > ambari-server/src/main/java/org/apache/ambari/server/agent/ExecutionCommand.java > 29737ee > > ambari-server/src/main/java/org/apache/ambari/server/state/ConfigHelper.java > 70c24f9 > > Diff: https://reviews.apache.org/r/51705/diff/ > > > Testing > ------- > > Updated the test cases. > Ran mvn test. > > Manually tested by setting up a cluster, the password fields in the > configurations.json is masked. During testing, everytime the ambari agent is > restarted, it registers with the server and the memory cache and file cache > are updated, the alerts in turn uses the value from the memory cache. > > > Thanks, > > Anita Jebaraj > >
