subject:"Re\: Review Request 69534\: HIVE\-20992\: Split the property \"hive.metastore.dbaccess.ssl.properties\" into more coherent and user\-friendly properties."

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

2018-12-17 Thread Morio Ramdenbourg via Review Board



> 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)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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
> 
>

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

2018-12-17 Thread Morio Ramdenbourg via Review Board

---
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.

Changes
---

Changed IllegalArgumentException to RuntimeException and changed constants to
upper case

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 (updated)
-

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/

Changes: https://reviews.apache.org/r/69534/diff/5-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

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

2018-12-17 Thread Vihang Karajgaonkar via Review Board


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69534/#review211367
---


Fix it, then Ship it!




Thanks for the changes. Couple of minor comments. Rest looks good to me.


standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
Lines 133-135 (patched)


Nit, constants are recommended to be in upper-case.



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
Lines 379 (patched)


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.



standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
Lines 1082 (patched)


If you decide to change the thrown exception to RuntimeException, this 
might need a change too.


- Vihang Karajgaonkar


On Dec. 16, 2018, 3:11 a.m., Morio Ramdenbourg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69534/
> ---
> 
> (Updated Dec. 16, 2018, 3:11 a.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/5/
> 
> 
> 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
> 
>

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

2018-12-15 Thread Morio Ramdenbourg via Review Board



> On Dec. 14, 2018, 10:43 p.m., Vihang Karajgaonkar wrote:
> >

Thanks for the feedback.


> On Dec. 14, 2018, 10:43 p.m., Vihang Karajgaonkar wrote:
> > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
> > Lines 462 (patched)
> > 
> >
> > s/System/Java system

Done


> On Dec. 14, 2018, 10:43 p.m., Vihang Karajgaonkar wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
> > Line 328 (original), 342 (patched)
> > 
> >
> > This patch may introduce performance regression in the setConf method 
> > which is called for every new connection. MetastoreConf.getPassword is 
> > expensive since it needs to decrypt the truststore password. See HIVE-20740 
> > which has more details. In most common cases, these configuration 
> > properties almost never change once they are set. But they are being read 
> > again and again at every new connection initialization time. I think we can 
> > improve this by caching the db and truststore password and reading it once 
> > when HMS starts. But I guess this could be separate from this JIRA.

Thanks for pointing this out. That's true, I do see this as a potential 
performance regression. Though as you had previously identified, the database 
password is also being read in the same manner during every new connection 
initialization. I've created HIVE-21047 to address this.


> On Dec. 14, 2018, 10:43 p.m., Vihang Karajgaonkar wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
> > Lines 343 (patched)
> > 
> >
> > may be rename this to configureSSLDeprecated

Done


> On Dec. 14, 2018, 10:43 p.m., Vihang Karajgaonkar wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
> > Lines 346 (patched)
> > 
> >
> > This is a redundant log. If ssl is configured we have other logs below 
> > to tell us that. Note that logs printed in this method are going to be very 
> > frequent for each new HMS connection.

Removed


> On Dec. 14, 2018, 10:43 p.m., Vihang Karajgaonkar wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
> > Lines 354 (patched)
> > 
> >
> > The exception message suggests taht it Disables SSL and continues, but 
> > actually, the exception is uncaught and it will terminate the connection 
> > request (and infact HMS coming up). I think you can remove "Disabling SSL 
> > and continuing." if thats not needed.

Removed


> On Dec. 14, 2018, 10:43 p.m., Vihang Karajgaonkar wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
> > Lines 356 (patched)
> > 
> >
> > This comment is unclear. The getPassword method will get the encrypted 
> > password if configured right?

Correct. I've updated the comment to better reflect what getPassword() does.


> On Dec. 14, 2018, 10:43 p.m., Vihang Karajgaonkar wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
> > Lines 367-369 (patched)
> > 
> >
> > Can you define constants for javax.net.ssl* property keys at the class 
> > level?

Done


> On Dec. 14, 2018, 10:43 p.m., Vihang Karajgaonkar wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
> > Lines 371 (patched)
> > 
> >
> > Redundant log, please remove.

Removed


> On Dec. 14, 2018, 10:43 p.m., Vihang Karajgaonkar wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
> > Lines 373 (patched)
> > 
> >
> > Don't see the code where we disable and continue. Am I missing 
> > something?

I don't think I worded that exception correctly. I more of meant to say that 
SSL encrypted communication to the DB will not be occuring. But after looking 
at the code again, this exception being thrown will terminate the connection 
request either way. I removed the bit where it says "Disabling SSL and 
continuing"


> On Dec. 14, 2018, 10:43 p.m., Vihang Karajgaonkar wrote:
> >

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

2018-12-15 Thread Morio Ramdenbourg via Review Board

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69534/
---

(Updated Dec. 16, 2018, 3:11 a.m.)

Review request for hive, Adam Holley, Karthik Manamcheri, Peter Vary, and
Vihang Karajgaonkar.

Changes
---

Modified comments and method names for better readability and naming based on
Vihang's feedback

Bugs: HIVE-20992
https://issues.apache.org/jira/browse/HIVE-20992

Repository: hive-git

Description (updated)
---

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.

The property "hive.metastore.dbaccess.ssl.properties" is now
deprecated, but it will still be kept for backwards-compatibility purposes.

Diffs (updated)
-

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/5/

Changes: https://reviews.apache.org/r/69534/diff/4-5/

Testing
---

I plan to upload a document to our Wiki explaining the process of enabling TLS
to these databases.

Thanks,

Morio Ramdenbourg

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

2018-12-14 Thread Vihang Karajgaonkar via Review Board

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69534/#review211343
---

standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
Lines 462 (patched)

s/System/Java system

standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
Line 328 (original), 342 (patched)

This patch may introduce performance regression in the setConf method which 
is called for every new connection. MetastoreConf.getPassword is expensive 
since it needs to decrypt the truststore password. See HIVE-20740 which has 
more details. In most common cases, these configuration properties almost never 
change once they are set. But they are being read again and again at every new 
connection initialization time. I think we can improve this by caching the db 
and truststore password and reading it once when HMS starts. But I guess this 
could be separate from this JIRA.

standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
Lines 343 (patched)

may be rename this to configureSSLDeprecated

standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
Lines 346 (patched)

This is a redundant log. If ssl is configured we have other logs below to 
tell us that. Note that logs printed in this method are going to be very 
frequent for each new HMS connection.

standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
Lines 354 (patched)

The exception message suggests taht it Disables SSL and continues, but 
actually, the exception is uncaught and it will terminate the connection 
request (and infact HMS coming up). I think you can remove "Disabling SSL and 
continuing." if thats not needed.

standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
Lines 356 (patched)

This comment is unclear. The getPassword method will get the encrypted 
password if configured right?

standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
Lines 367-369 (patched)

Can you define constants for javax.net.ssl* property keys at the class 
level?

standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
Lines 371 (patched)

Redundant log, please remove.

standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
Lines 373 (patched)

Don't see the code where we disable and continue. Am I missing something?

standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
Line 332 (original), 398 (patched)

I would suggest change this log to say "Configuring SSL using a deprecated 
key " + ConfVars.DBACCESS_SSL_PROPS.toString() + ". This may be removed in the 
future. See HIVE-20992 for more details."

standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
Lines 1069 (patched)

suggest rename to testDeprecatedConfigIsOverriden()

- Vihang Karajgaonkar

On Dec. 14, 2018, 1:26 a.m., Morio Ramdenbourg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69534/
> ---
> 
> (Updated Dec. 14, 2018, 1:26 a.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

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

2018-12-13 Thread Karthik Manamcheri via Review Board



> On Dec. 14, 2018, 2:58 a.m., Karthik Manamcheri wrote:
> > Ship It!

LGTM! Make sure you get someone else to review it as well. Thanks.


- Karthik


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69534/#review211318
---


On Dec. 14, 2018, 1:26 a.m., Morio Ramdenbourg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69534/
> ---
> 
> (Updated Dec. 14, 2018, 1:26 a.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.
> 
> Modified comments to clearly reflect what is / is not deprecated
> 
> 
> 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/4/
> 
> 
> 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
> 
>

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

2018-12-13 Thread Karthik Manamcheri via Review Board


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69534/#review211318
---


Ship it!




Ship It!

- Karthik Manamcheri


On Dec. 14, 2018, 1:26 a.m., Morio Ramdenbourg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69534/
> ---
> 
> (Updated Dec. 14, 2018, 1:26 a.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.
> 
> Modified comments to clearly reflect what is / is not deprecated
> 
> 
> 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/4/
> 
> 
> 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
> 
>

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

2018-12-13 Thread Morio Ramdenbourg via Review Board

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69534/
---

(Updated Dec. 14, 2018, 1:25 a.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.

The property "hive.metastore.dbaccess.ssl.properties" is now
deprecated, but it will still be kept for backwards-compatibility purposes.

Modified comments to clearly reflect what is / is not deprecated

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/4/

Testing (updated)
---

Thanks,

Morio Ramdenbourg

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

2018-12-13 Thread Morio Ramdenbourg via Review Board

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69534/
---

(Updated Dec. 14, 2018, 1:26 a.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.

The property "hive.metastore.dbaccess.ssl.properties" is now
deprecated, but it will still be kept for backwards-compatibility purposes.

Modified comments to clearly reflect what is / is not deprecated

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/4/

Testing (updated)
---

I plan to upload a document to our Wiki explaining the process of enabling TLS
to these databases.

Thanks,

Morio Ramdenbourg

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

2018-12-13 Thread Morio Ramdenbourg via Review Board

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69534/
---

(Updated Dec. 14, 2018, 1:23 a.m.)

Review request for hive, Adam Holley, Karthik Manamcheri, Peter Vary, and
Vihang Karajgaonkar.

Changes
---

Made modifications to the comments and deprecation to clearly state what is /
is not supported

Bugs: HIVE-20992
https://issues.apache.org/jira/browse/HIVE-20992

Repository: hive-git

Description (updated)
---

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.

The property "hive.metastore.dbaccess.ssl.properties" is now
deprecated, but it will still be kept for backwards-compatibility purposes.

Modified comments to clearly reflect what is / is not deprecated

Diffs (updated)
-

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/4/

Changes: https://reviews.apache.org/r/69534/diff/3-4/

Testing
---

Thanks,

Morio Ramdenbourg

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

2018-12-13 Thread Morio Ramdenbourg via Review Board



> On Dec. 12, 2018, 10:54 p.m., Karthik Manamcheri wrote:
> > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
> > Lines 472 (patched)
> > 
> >
> > Is this specific to a Java version?

It looks like Java 8 and beyond can actually read more types than this - I 
found this resource here: 
https://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html#KeyStore
I'll update this to use only the types specific to Java 8.


> On Dec. 12, 2018, 10:54 p.m., Karthik Manamcheri wrote:
> > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
> > Lines 473-474 (patched)
> > 
> >
> > Move this to the top and state that if this is set to false, we'll 
> > ignore the other SSL properties.

Wanted to keep alphabetical order :) Will put a comment on top though


> On Dec. 12, 2018, 10:54 p.m., Karthik Manamcheri wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
> > Line 332 (original), 398 (patched)
> > 
> >
> > State that this is deprecated here and maybe increase the LOG to warn 
> > and note that this might be changed in next version of hive.

Done


- Morio


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69534/#review211260
---


On Dec. 14, 2018, 1:23 a.m., Morio Ramdenbourg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69534/
> ---
> 
> (Updated Dec. 14, 2018, 1:23 a.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.
> 
> Modified comments to clearly reflect what is / is not deprecated
> 
> 
> 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/4/
> 
> 
> 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
> 
>

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

2018-12-12 Thread Karthik Manamcheri via Review Board


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69534/#review211260
---




standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
Lines 472 (patched)


Is this specific to a Java version?



standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
Lines 473-474 (patched)


Move this to the top and state that if this is set to false, we'll ignore 
the other SSL properties.



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
Line 332 (original), 398 (patched)


State that this is deprecated here and maybe increase the LOG to warn and 
note that this might be changed in next version of hive.


- Karthik Manamcheri


On Dec. 10, 2018, 8:51 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:51 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
>  e598a43e4dc2d2a2c25886ae7cbafd29b47c1f24 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
>  0cf113c927f2274d085e07cd72921fb35227e1f3 
> 
> 
> Diff: https://reviews.apache.org/r/69534/diff/3/
> 
> 
> 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
> 
>

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

2018-12-10 Thread Morio Ramdenbourg via Review Board

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69534/
---

(Updated Dec. 10, 2018, 8:51 p.m.)

Review request for hive, Adam Holley, Karthik Manamcheri, Peter Vary, and
Vihang Karajgaonkar.

Changes
---

Forgot to change the description of the deprecated property.

Bugs: HIVE-20992
https://issues.apache.org/jira/browse/HIVE-20992

Repository: hive-git

Description (updated)
---

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.

The property "hive.metastore.dbaccess.ssl.properties" is now
deprecated, but it will still be kept for backwards-compatibility purposes.

Diffs (updated)
-

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/3/

Changes: https://reviews.apache.org/r/69534/diff/2-3/

Testing
---

Thanks,

Morio Ramdenbourg

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

2018-12-10 Thread Morio Ramdenbourg via Review Board

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69534/
---

(Updated Dec. 10, 2018, 8:46 p.m.)

Review request for hive, Adam Holley, Karthik Manamcheri, Peter Vary, and
Vihang Karajgaonkar.

Changes
---

Changed description

Bugs: HIVE-20992
https://issues.apache.org/jira/browse/HIVE-20992

Repository: hive-git

Description (updated)
---

The following new properties were added:

1. metastore.dbaccess.ssl.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.

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
---

Thanks,

Morio Ramdenbourg

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

2018-12-10 Thread Morio Ramdenbourg via Review Board



> 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)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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
>

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

2018-12-10 Thread Morio Ramdenbourg via Review Board

---
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 (updated)
---

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.

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
---

Thanks,

Morio Ramdenbourg

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

2018-12-10 Thread Morio Ramdenbourg via Review Board

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69534/
---

(Updated Dec. 10, 2018, 8:39 p.m.)

Review request for hive, Adam Holley, Karthik Manamcheri, Peter Vary, and
Vihang Karajgaonkar.

Changes
---

I modified the parameter names to better clarify the difference between the HMS
Clients params and the HMSDB params. I also added better descriptions for each
of the properties, and better logs and warnings.

Bugs: HIVE-20992
https://issues.apache.org/jira/browse/HIVE-20992

Repository: hive-git

Description (updated)
---

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.

The property "hive.metastore.dbaccess.ssl.properties" is now
deprecated, but it will still be kept for backwards-compatibility purposes.

Diffs (updated)
-

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/

Changes: https://reviews.apache.org/r/69534/diff/1-2/

Testing
---

Thanks,

Morio Ramdenbourg

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

2018-12-08 Thread Karthik Manamcheri via Review Board


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69534/#review211129
---




standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
Lines 460 (patched)


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.



standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
Lines 461 (patched)


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.



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
Lines 343 (patched)


haha I like the "dangerously". Could you file a ticket to remove this in 
the future and link the new ticket here.



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
Lines 347 (patched)


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.



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
Lines 353 (patched)


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"


- Karthik Manamcheri


On Dec. 8, 2018, 6:31 a.m., Morio Ramdenbourg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69534/
> ---
> 
> (Updated Dec. 8, 2018, 6:31 a.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
>  e598a43e4dc2d2a2c25886ae7cbafd29b47c1f24 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
>  0cf113c927f2274d085e07cd72921fb35227e1f3 
> 
> 
> Diff: https://reviews.apache.org/r/69534/diff/1/
> 
> 
> 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
> 
>

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

Re: Review Request 69534: HIVE-20992: Split the property "hive.metastore.dbaccess.ssl.properties" into more coherent and user-friendly properties.

19 matches

Site Navigation

Mail list logo

Footer information