> On Aug. 27, 2016, 3:20 a.m., Chaoyu Tang wrote: > > common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java, line 72 > > <https://reviews.apache.org/r/51435/diff/2/?file=1486030#file1486030line72> > > > > hive.conf.hidden.list is usually not default specified in > > configurations (e.g. that from hdfs-site.xml) other than HiveConf, so the > > hiddenListStr should be retruned as null for these configuraitons, and the > > properties listed in HiveConf hidden list are still not able to be stripped > > from this Config, right? > > The S3 credentials I think could be defined in the hdfs configuration > > but they are specified in HiveConf hiddenList, they should be stripped and > > have we tested them? > > Peter Vary wrote: > Thanks Chaoyu! > > You are absolutely right! > I missed this, because when it was not defined in the actual > configuration xml-s, then it used the default value - so removed the S3 > credentials anyway. > > Added it to the interestingPropNames, so it will be copied from the > HiveConf, if there is any. If it is not in the following files, or the > hiveConf, then we could still use the default value. > HCatalog specific config files: "core-default.xml", "core-site.xml", > "mapred-default.xml", "mapred-site.xml", "hdfs-site.xml", > "webhcat-default.xml", "webhcat-site.xml" > > Thanks for the catch. > > Any other idea? I might have missed something more - hcatalog is new for > me. > > Thanks, > Peter > > Chaoyu Tang wrote: > Hi Peter, > Here is what I understand the code: > AppConf loads the configurations from xml files specified in > HADOOP_CONF_FILENAMES and TEMPLETON_CONF_FILENAMES which does not include > hive-site.xml, so it might contain the proprety such as S3 credentials which > are probably specified in hdfs-site.xml. Since appConf does not defined the > property hive.conf.hidden.list, so > the line String hiddenListStr = HiveConf.getVar(configuration, > HiveConf.ConfVars.HIVE_CONF_HIDDEN_LIST); > returns null when HiveConfUtil.dumpConfig(this, sb) is called in > AppConf.dumpEnvironent and the passed in parameter configuration is AppConf > instance in this case. Therefore the S3 credentials in this AppConf could not > be stripped. > The new change only adds (or appends) the hiveConf hive.conf.hidden.list > key/value pair to the value of property HIVE_PROPS_NAME > (templeton.hive.properties), but the HIVE_CONF_HIDDEN_LIST is still undefined > in AppConf and the property like S3 credentials are still not stripped even > they have been specified in hiveConf hidden list. Does it make sense? So in > order to strip the properties speified in hive.conf.hidden.list, the property > hive.conf.hidden.list should be added to AppConf, right? > > Peter Vary wrote: > Hi Chaoyu, > > When starting the Main.java from Intellij in debug mode, I found the > following: > - AppConf loads, as you wrote the HADOOP_CONF_FILENAMES and > TEMPLETON_CONF_FILENAMES - no hive-site.xml here > - If the hive.conf.hidden.list is in any of the files above, then it is > used (not in the default case - probably not a good idea to have this value > there anyway) > - If my understanding is correct, if HCatalog uses Hive Metastore, then > it expects to have hive-site.xml on classpath - see the comments of the > AppConfig.handleHiveProperties method - if this is true, then we are ok, and > the hive.conf.hidden.list is set now, and the new patch (with changes after > your previous comment) will copy the value to the configuration, and the > credentials are removed > - If hive-site.xml is not on the classpath, then I am not sure what will > happen when we try to create a HiveConf object in the first line of the > handleHiveProperties method, but if no exception is thrown, then the > HiveConf.getVar(configuration, HiveConf.ConfVars.HIVE_CONF_HIDDEN_LIST) will > return the default value defined in HiveConf object - at least this is what I > saw during my tests. > > Most probably you know the HCatalog better than me, do you think it is a > valid usecase not to have a hive-conf.xml on the classpath? If so, what > should be our solution here? Adding a hive.conf. variables to the > webhcat-site.xml files? Add a new templeton specific configuration variable? > > Thanks for your time, and review, > Peter > > Chaoyu Tang wrote: > "HiveConf hiveConf = new HiveConf()" in handleHiveProperties loads > hive-site.xml (if any) to its method variable hiveConf, but not the "this" > AppConf, right? so the "this" (AppConf) passed to > HiveConfUtil.dumpConfig(this, sb) does still not have the > HIVE_CONF_HIDDEN_LIST defined unless you call something like > set(HiveConf.ConfVars.HIVE_CONF_HIDDEN_LIST.varname, > HiveConf.getVar(hiveConf, HiveConf.ConfVars.HIVE_CONF_HIDDEN_LIST)) to copy > the property from hiveConf to this AppConf. handleHiveProperties only appends > the HIVE_CONF_HIDDEN_LIST (key=value) from hiveConf to the value of AppConf > property HIVE_PROPS_NAME, right? > > Peter Vary wrote: > Thanks Chaoyu, > > Finally I got it. I have thought I understand the method comment, but I > did not :( It was setting a new config variable, and not copying the original > ones.
One more thing: It might be a good idea to add it to the interestingPropNames too, so it could be passed to the client, so the logging could be redacted too. - Peter ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51435/#review147068 ----------------------------------------------------------- On Aug. 31, 2016, 3:06 p.m., Peter Vary wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51435/ > ----------------------------------------------------------- > > (Updated Aug. 31, 2016, 3:06 p.m.) > > > Review request for hive, Chaoyu Tang, Gabor Szadovszky, Sergio Pena, and > Barna Zsombor Klara. > > > Bugs: HIVE-14426 > https://issues.apache.org/jira/browse/HIVE-14426 > > > Repository: hive-git > > > Description > ------- > > Used HiveConf.stripHiddenConfigurations to remove sensitive information from > configuration dump. Had to refactor, so it could be applied to simple > Configuration objects too, like AppConfig. > > Used Utilities.maskIfPassword to remove sensitive information from property > dump > > > Diffs > ----- > > common/src/java/org/apache/hadoop/hive/common/LogUtils.java 599e798 > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 14a538b > common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java 073a978 > common/src/test/org/apache/hadoop/hive/common/TestLogUtils.java > PRE-CREATION > > hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/AppConfig.java > dd1208b > > hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/TempletonUtils.java > 201e647 > ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java a542dc4 > ql/src/java/org/apache/hadoop/hive/ql/exec/mr/MapredLocalTask.java ac922ce > > ql/src/java/org/apache/hadoop/hive/ql/exec/spark/HiveSparkClientFactory.java > 8e5c0da > ql/src/test/org/apache/hadoop/hive/ql/exec/TestUtilities.java 0786f9b > > Diff: https://reviews.apache.org/r/51435/diff/ > > > Testing > ------- > > Manually - checked both > > > Thanks, > > Peter Vary > >
