> On Dec. 8, 2018, 7:49 p.m., Karthik Manamcheri wrote: > >
Thanks for the feedback! > On Dec. 8, 2018, 7:49 p.m., Karthik Manamcheri wrote: > > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java > > Lines 460 (patched) > > <https://reviews.apache.org/r/69534/diff/1/?file=2112677#file2112677line460> > > > > In each property state that this is for the database connection from > > HMS to the backend database. More redundant documentation is better since > > we have the client <-> HMS ssl configurations as well. Done > On Dec. 8, 2018, 7:49 p.m., Karthik Manamcheri wrote: > > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java > > Lines 461 (patched) > > <https://reviews.apache.org/r/69534/diff/1/?file=2112677#file2112677line461> > > > > I think we should rename the property to indicate that this for the > > dbaccess and for client access. Consider renaming to > > "hive.metastore.dbaccess.ssl.trustStore" etc. At first I was concerned with straying away from the curring naming convention in Hive when it came to Java system properties, such as "javax.jdo.option.ConnectionUserName", but after second thought I do see the benefit in clearly separating the dbaccess properties from the client access properties. > On Dec. 8, 2018, 7:49 p.m., Karthik Manamcheri wrote: > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java > > Lines 343 (patched) > > <https://reviews.apache.org/r/69534/diff/1/?file=2112678#file2112678line343> > > > > haha I like the "dangerously". Could you file a ticket to remove this > > in the future and link the new ticket here. Done! > On Dec. 8, 2018, 7:49 p.m., Karthik Manamcheri wrote: > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java > > Lines 347 (patched) > > <https://reviews.apache.org/r/69534/diff/1/?file=2112678#file2112678line347> > > > > Should we validate the SSL configs here and print info if something is > > missing. We can then fall back to not use SSL if configs are missing. Java does allow an empty truststore password (though not recommended), but it does give me the idea to at least give the user a fair warning about the implications of doing that. Also, truststore path already has a check for an empty string, and truststore type is already verified using the StringSetValidator in MetastoreConf. > On Dec. 8, 2018, 7:49 p.m., Karthik Manamcheri wrote: > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java > > Lines 353 (patched) > > <https://reviews.apache.org/r/69534/diff/1/?file=2112678#file2112678line353> > > > > I prefer using exceptions which tell the user what to do in addition to > > what is wrong. In this case, rephrase as, "SSL has been enabled but trust > > store path is empty. Set the xxx property to enable SSL. Disabling SSL and > > continuing" Done! - Morio ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69534/#review211129 ----------------------------------------------------------- On Dec. 10, 2018, 8:40 p.m., Morio Ramdenbourg wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69534/ > ----------------------------------------------------------- > > (Updated Dec. 10, 2018, 8:40 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. metastore.dbaccess.ssl.truststore.path > 3. metastore.dbaccess.ssl.truststore.password > 4. metastore.dbaccess.ssl.truststore.type > > 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 > e598a43e4dc2d2a2c25886ae7cbafd29b47c1f24 > > standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java > 0cf113c927f2274d085e07cd72921fb35227e1f3 > > > Diff: https://reviews.apache.org/r/69534/diff/2/ > > > 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. > > > Thanks, > > Morio Ramdenbourg > >
