> On Dec. 17, 2018, 8:35 p.m., Vihang Karajgaonkar wrote: > > Thanks for the changes. Couple of minor comments. Rest looks good to me.
Thanks. Will make these minor changes right away. > On Dec. 17, 2018, 8:35 p.m., Vihang Karajgaonkar wrote: > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java > > Lines 133-135 (patched) > > <https://reviews.apache.org/r/69534/diff/5/?file=2114310#file2114310line133> > > > > Nit, constants are recommended to be in upper-case. Oops, completely forgot. Thanks for reminding me > On Dec. 17, 2018, 8:35 p.m., Vihang Karajgaonkar wrote: > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java > > Lines 379 (patched) > > <https://reviews.apache.org/r/69534/diff/5/?file=2114310#file2114310line379> > > > > you are catching IOException and changing it to > > IllegalArgumentException which may not be the right thing to do all the > > times. I would suggest changing this to RuntimeException since setConf() > > doesn't throw checked exceptions. Also, would be good to have e thrown as a > > cause as well, so would suggest using throw new RuntimeException("Failed to > > set .., e); constructor instead. Agreed, I think the RuntimeException makes more sense - changed it > On Dec. 17, 2018, 8:35 p.m., Vihang Karajgaonkar wrote: > > standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java > > Lines 1082 (patched) > > <https://reviews.apache.org/r/69534/diff/5/?file=2114311#file2114311line1082> > > > > If you decide to change the thrown exception to RuntimeException, this > > might need a change too. This one checks the empty truststore path, which still throws an IllegalArgumentException on line 359 in ObjectStore. Though since IllegalArgumentException inherits from RuntimeException, it'll pass despite which one I put. Will keep the IllegalArgumentException :) - Morio ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69534/#review211367 ----------------------------------------------------------- On Dec. 17, 2018, 9:17 p.m., Morio Ramdenbourg wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69534/ > ----------------------------------------------------------- > > (Updated Dec. 17, 2018, 9:17 p.m.) > > > Review request for hive, Adam Holley, Karthik Manamcheri, Peter Vary, and > Vihang Karajgaonkar. > > > Bugs: HIVE-20992 > https://issues.apache.org/jira/browse/HIVE-20992 > > > Repository: hive-git > > > Description > ------- > > The following new properties were added: > > 1. metastore.dbaccess.use.SSL (hive.metastore.dbaccess.use.SSL) > 2. javax.net.ssl.trustStore > 3. javax.net.ssl.trustStorePassword > 4. javax.net.ssl.trustStoreType > > This was in an effort to guide the user towards an easier SSL > configuration experience. This is the minimum requirement to set up SSL > encryption to the HMS backend store. > > This also solves the issue of the truststore password being stored in > plain text. It can now be encrypted by default and loaded through the > MetastoreConf.getPassword() method which handles secure password access > > The property "hive.metastore.dbaccess.ssl.properties" is now > deprecated, but it will still be kept for backwards-compatibility purposes. > > > Diffs > ----- > > > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java > e25a8cf9a19d78c0cc00bb2e5e0abee4d851ad98 > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java > 3fa21b768cd120cd89343c9ccd142d5e2ccdef2e > > standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java > 0cf113c927f2274d085e07cd72921fb35227e1f3 > > > Diff: https://reviews.apache.org/r/69534/diff/6/ > > > Testing > ------- > > Tests: > 1. Unit tests were added to cover the functionality of configuring the Java > system properties. > 2. Performed some manual and sanity tests to ensure that SSL was still > configurable to a remote DB. I performed these on MySQL, PostgreSQL, Oracle, > and Derby DB by creating generic DB hosts and setting them up with SSL. Once > SSL was set up, I triggered the metastore to perform database calls, and > captured packets using tcpdump. I then uploaded my packet captures to > Wireshark, and ensured that none of the data was human-readable. > > I plan to upload a document to our Wiki explaining the process of enabling > TLS to these databases. > > > Thanks, > > Morio Ramdenbourg > >
