> On Sept. 8, 2016, 5:42 p.m., Jonathan Hurley wrote: > > ambari-agent/src/main/python/ambari_agent/ClusterConfiguration.py, line 103 > > <https://reviews.apache.org/r/51705/diff/1/?file=1493660#file1493660line103> > > > > 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.
Thanks for the catch, I updated the code to mask passwords only while dumping in the file cache, but missed to remove the code. Was not able to capture this in the testing. Thank you. > On Sept. 8, 2016, 5:42 p.m., Jonathan Hurley wrote: > > ambari-agent/src/main/python/ambari_agent/ClusterConfiguration.py, line 174 > > <https://reviews.apache.org/r/51705/diff/1/?file=1493660#file1493660line174> > > > > `i` is not a very good name here; maybe something a little clearer. Updated the code > On Sept. 8, 2016, 5:42 p.m., Jonathan Hurley wrote: > > ambari-agent/src/main/python/ambari_agent/ClusterConfiguration.py, lines > > 179-181 > > <https://reviews.apache.org/r/51705/diff/1/?file=1493660#file1493660line179> > > > > 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. Updated the code with a different approach > On Sept. 8, 2016, 5:42 p.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/actionmanager/ExecutionCommandWrapper.java, > > lines 182-183 > > <https://reviews.apache.org/r/51705/diff/1/?file=1493663#file1493663line182> > > > > What about status commands - those send down configurations too? The configurations in the status commands are not dumped into the configurations.json file. The values are dumped only for alert definition and execution commands. > On Sept. 8, 2016, 5:42 p.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/state/ConfigHelper.java, > > lines 252-275 > > <https://reviews.apache.org/r/51705/diff/1/?file=1493666#file1493666line252> > > > > 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. Updated the code with a different approach. Instead of getting the value from the Ambari Metainfo, I am getting the value from the config based on the desired Tags. Similar approach is being used for getting the required config properties for the response. > On Sept. 8, 2016, 5:42 p.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/state/ConfigHelper.java, > > line 270 > > <https://reviews.apache.org/r/51705/diff/1/?file=1493666#file1493666line270> > > > > 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. Yes you are right, key-value pair is not required in this case. I have updated the code - Anita ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51705/#review148212 ----------------------------------------------------------- On Sept. 12, 2016, 8:20 p.m., Anita Jebaraj wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51705/ > ----------------------------------------------------------- > > (Updated Sept. 12, 2016, 8:20 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 2bddc43 > 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 > >