You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Li Li <li...@cloudera.com> on 2016/03/08 07:31:53 UTC
Review Request 44501: SENTRY-1076: Add SSL support,
print version info on Sentry Service webpage
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44501/
-----------------------------------------------------------
Review request for sentry, Anne Yu, Hao Hao, Lenni Kuff, and Sravya Tirukkovalur.
Repository: sentry
Description
-------
Add SSL support, print version info on Sentry Service webpage
Diffs
-----
sentry-provider/sentry-provider-db/pom.xml c32793c7601109f52542e2703ce011bb094c9c61
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java 1bdea2c55de12a999f94ea33f8709311c7c2c7f2
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 32d813cc2f3148a1e31f15ebf816aec55ec10081
sentry-provider/sentry-provider-db/src/main/webapp/SentryService.html cd19dd8b9a228e56800bd06f0714a5ab17634347
sentry-provider/sentry-provider-db/src/main/webapp/css/sentry.css e5b3d437406a881e78494547791e1b26bb4f2bc8
Diff: https://reviews.apache.org/r/44501/diff/
Testing
-------
Thanks,
Li Li
Re: Review Request 44501: SENTRY-1076: Add SSL support,
print version info on Sentry Service webpage
Posted by Li Li <li...@cloudera.com>.
> On March 17, 2016, 5:35 a.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/pom.xml, line 227
> > <https://reviews.apache.org/r/44501/diff/2/?file=1299068#file1299068line227>
> >
> > Maybe split the change to include the githash into a separate JIRA to make it easier view when this change came in in the code history
Created jira SENTRY-1141 for githash
> On March 17, 2016, 5:35 a.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java, line 70
> > <https://reviews.apache.org/r/44501/diff/2/?file=1299069#file1299069line70>
> >
> > maybe log that SSL mode is being used?
Done
> On March 17, 2016, 5:35 a.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/pom.xml, line 229
> > <https://reviews.apache.org/r/44501/diff/2/?file=1299068#file1299068line229>
> >
> > what happens if you build the raw source (no git available)
will be an empty string
> On March 17, 2016, 5:35 a.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java, line 72
> > <https://reviews.apache.org/r/44501/diff/2/?file=1299069#file1299069line72>
> >
> > Some SSL protocols are not secure (remember poodle?). I think we will want to have a way to blacklist these. Jetty should provide a way to pass in the excluded protocols.
Thanks for your remind. Will update it.
- Li
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44501/#review123983
-----------------------------------------------------------
On March 15, 2016, 1:21 a.m., Li Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44501/
> -----------------------------------------------------------
>
> (Updated March 15, 2016, 1:21 a.m.)
>
>
> Review request for sentry, Anne Yu, Hao Hao, Lenni Kuff, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Add SSL support, print version info on Sentry Service webpage
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/pom.xml 1dbfad49f7b9bb1b963221600bbd8d0400ab8d5d
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java 1bdea2c55de12a999f94ea33f8709311c7c2c7f2
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 94bd2a95c77a9691cbaa578ebf417e49c339b7ed
> sentry-provider/sentry-provider-db/src/main/webapp/SentryService.html cd19dd8b9a228e56800bd06f0714a5ab17634347
> sentry-provider/sentry-provider-db/src/main/webapp/css/sentry.css e5b3d437406a881e78494547791e1b26bb4f2bc8
>
> Diff: https://reviews.apache.org/r/44501/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Li Li
>
>
Re: Review Request 44501: SENTRY-1076: Add SSL support,
print version info on Sentry Service webpage
Posted by Lenni Kuff <ls...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44501/#review123983
-----------------------------------------------------------
sentry-provider/sentry-provider-db/pom.xml (line 227)
<https://reviews.apache.org/r/44501/#comment186331>
Maybe split the change to include the githash into a separate JIRA to make it easier view when this change came in in the code history
sentry-provider/sentry-provider-db/pom.xml (line 229)
<https://reviews.apache.org/r/44501/#comment186332>
what happens if you build the raw source (no git available)
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java (line 70)
<https://reviews.apache.org/r/44501/#comment186335>
maybe log that SSL mode is being used?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java (line 72)
<https://reviews.apache.org/r/44501/#comment186339>
Some SSL protocols are not secure (remember poodle?). I think we will want to have a way to blacklist these. Jetty should provide a way to pass in the excluded protocols.
- Lenni Kuff
On March 15, 2016, 1:21 a.m., Li Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44501/
> -----------------------------------------------------------
>
> (Updated March 15, 2016, 1:21 a.m.)
>
>
> Review request for sentry, Anne Yu, Hao Hao, Lenni Kuff, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Add SSL support, print version info on Sentry Service webpage
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/pom.xml 1dbfad49f7b9bb1b963221600bbd8d0400ab8d5d
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java 1bdea2c55de12a999f94ea33f8709311c7c2c7f2
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 94bd2a95c77a9691cbaa578ebf417e49c339b7ed
> sentry-provider/sentry-provider-db/src/main/webapp/SentryService.html cd19dd8b9a228e56800bd06f0714a5ab17634347
> sentry-provider/sentry-provider-db/src/main/webapp/css/sentry.css e5b3d437406a881e78494547791e1b26bb4f2bc8
>
> Diff: https://reviews.apache.org/r/44501/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Li Li
>
>
Re: Review Request 44501: SENTRY-1076: Add SSL support,
print version info on Sentry Service webpage
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44501/#review127983
-----------------------------------------------------------
Ship it!
Thanks for adding the test Li!
- Sravya Tirukkovalur
On March 30, 2016, 6:06 a.m., Li Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44501/
> -----------------------------------------------------------
>
> (Updated March 30, 2016, 6:06 a.m.)
>
>
> Review request for sentry, Anne Yu, Hao Hao, Lenni Kuff, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Add SSL support, print version info on Sentry Service webpage
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/pom.xml bf4dfdc1de90b1018767e2a61bee970655f02682
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java 1bdea2c55de12a999f94ea33f8709311c7c2c7f2
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 94bd2a95c77a9691cbaa578ebf417e49c339b7ed
> sentry-provider/sentry-provider-db/src/main/webapp/SentryService.html ee112ce8d39626784d5d73ef0a4c28f43e7c4f1f
> sentry-provider/sentry-provider-db/src/main/webapp/css/sentry.css e5b3d437406a881e78494547791e1b26bb4f2bc8
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryWebServerWithSSL.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java e02bd8a9ffbf73424790e6a574a2976c97c94b1f
> sentry-provider/sentry-provider-db/src/test/resources/cacerts.jks PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/resources/keystore.jks PRE-CREATION
>
> Diff: https://reviews.apache.org/r/44501/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Li Li
>
>
Re: Review Request 44501: SENTRY-1076: Add SSL support,
print version info on Sentry Service webpage
Posted by Li Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44501/
-----------------------------------------------------------
(Updated March 30, 2016, 6:06 a.m.)
Review request for sentry, Anne Yu, Hao Hao, Lenni Kuff, and Sravya Tirukkovalur.
Changes
-------
add test for SSL, fix intend, add comments on blacklist protocols
Repository: sentry
Description
-------
Add SSL support, print version info on Sentry Service webpage
Diffs (updated)
-----
sentry-provider/sentry-provider-db/pom.xml bf4dfdc1de90b1018767e2a61bee970655f02682
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java 1bdea2c55de12a999f94ea33f8709311c7c2c7f2
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 94bd2a95c77a9691cbaa578ebf417e49c339b7ed
sentry-provider/sentry-provider-db/src/main/webapp/SentryService.html ee112ce8d39626784d5d73ef0a4c28f43e7c4f1f
sentry-provider/sentry-provider-db/src/main/webapp/css/sentry.css e5b3d437406a881e78494547791e1b26bb4f2bc8
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryWebServerWithSSL.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java e02bd8a9ffbf73424790e6a574a2976c97c94b1f
sentry-provider/sentry-provider-db/src/test/resources/cacerts.jks PRE-CREATION
sentry-provider/sentry-provider-db/src/test/resources/keystore.jks PRE-CREATION
Diff: https://reviews.apache.org/r/44501/diff/
Testing
-------
Thanks,
Li Li
Re: Review Request 44501: SENTRY-1076: Add SSL support,
print version info on Sentry Service webpage
Posted by Li Li <li...@cloudera.com>.
> On March 21, 2016, 7:20 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java, line 76
> > <https://reviews.apache.org/r/44501/diff/3/?file=1308063#file1308063line76>
> >
> > What happens if user uses SSL but does not set keystore path or password in the conf file?
the sentry service will still work, except the webpage.
> On March 21, 2016, 7:20 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java, line 77
> > <https://reviews.apache.org/r/44501/diff/3/?file=1308063#file1308063line77>
> >
> > nit: intend here and below as well
Sure. Thanks Sravya!
> On March 21, 2016, 7:20 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java, line 167
> > <https://reviews.apache.org/r/44501/diff/3/?file=1308064#file1308064line167>
> >
> > Add a comment on why these are blacklisted?
Sure. Thanks Sravya!
On March 21, 2016, 7:20 p.m., Li Li wrote:
> > Thanks for fixing this Li! Some minor comments. Also, wondering about the test coverage. I believe it might be tricky to test this without setting up keystore, any thoughts?
Thanks for your remind! Yes, we need generate a keystore and trustStore. I will add test in the next patch.
- Li
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44501/#review124624
-----------------------------------------------------------
On March 21, 2016, 4:43 a.m., Li Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44501/
> -----------------------------------------------------------
>
> (Updated March 21, 2016, 4:43 a.m.)
>
>
> Review request for sentry, Anne Yu, Hao Hao, Lenni Kuff, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Add SSL support, print version info on Sentry Service webpage
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/pom.xml 1dbfad49f7b9bb1b963221600bbd8d0400ab8d5d
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java 1bdea2c55de12a999f94ea33f8709311c7c2c7f2
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 94bd2a95c77a9691cbaa578ebf417e49c339b7ed
> sentry-provider/sentry-provider-db/src/main/webapp/SentryService.html ee112ce8d39626784d5d73ef0a4c28f43e7c4f1f
> sentry-provider/sentry-provider-db/src/main/webapp/css/sentry.css e5b3d437406a881e78494547791e1b26bb4f2bc8
>
> Diff: https://reviews.apache.org/r/44501/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Li Li
>
>
Re: Review Request 44501: SENTRY-1076: Add SSL support,
print version info on Sentry Service webpage
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44501/#review124624
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java (line 74)
<https://reviews.apache.org/r/44501/#comment187247>
What happens if user uses SSL but does not set keystore path or password in the conf file?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java (line 75)
<https://reviews.apache.org/r/44501/#comment187248>
nit: intend here and below as well
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java (line 167)
<https://reviews.apache.org/r/44501/#comment187249>
Add a comment on why these are blacklisted?
Thanks for fixing this Li! Some minor comments. Also, wondering about the test coverage. I believe it might be tricky to test this without setting up keystore, any thoughts?
- Sravya Tirukkovalur
On March 21, 2016, 4:43 a.m., Li Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44501/
> -----------------------------------------------------------
>
> (Updated March 21, 2016, 4:43 a.m.)
>
>
> Review request for sentry, Anne Yu, Hao Hao, Lenni Kuff, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Add SSL support, print version info on Sentry Service webpage
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/pom.xml 1dbfad49f7b9bb1b963221600bbd8d0400ab8d5d
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java 1bdea2c55de12a999f94ea33f8709311c7c2c7f2
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 94bd2a95c77a9691cbaa578ebf417e49c339b7ed
> sentry-provider/sentry-provider-db/src/main/webapp/SentryService.html ee112ce8d39626784d5d73ef0a4c28f43e7c4f1f
> sentry-provider/sentry-provider-db/src/main/webapp/css/sentry.css e5b3d437406a881e78494547791e1b26bb4f2bc8
>
> Diff: https://reviews.apache.org/r/44501/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Li Li
>
>
Re: Review Request 44501: SENTRY-1076: Add SSL support,
print version info on Sentry Service webpage
Posted by Li Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44501/
-----------------------------------------------------------
(Updated March 21, 2016, 4:43 a.m.)
Review request for sentry, Anne Yu, Hao Hao, Lenni Kuff, and Sravya Tirukkovalur.
Repository: sentry
Description
-------
Add SSL support, print version info on Sentry Service webpage
Diffs (updated)
-----
sentry-provider/sentry-provider-db/pom.xml 1dbfad49f7b9bb1b963221600bbd8d0400ab8d5d
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java 1bdea2c55de12a999f94ea33f8709311c7c2c7f2
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 94bd2a95c77a9691cbaa578ebf417e49c339b7ed
sentry-provider/sentry-provider-db/src/main/webapp/SentryService.html ee112ce8d39626784d5d73ef0a4c28f43e7c4f1f
sentry-provider/sentry-provider-db/src/main/webapp/css/sentry.css e5b3d437406a881e78494547791e1b26bb4f2bc8
Diff: https://reviews.apache.org/r/44501/diff/
Testing
-------
Thanks,
Li Li
Re: Review Request 44501: SENTRY-1076: Add SSL support,
print version info on Sentry Service webpage
Posted by Li Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44501/
-----------------------------------------------------------
(Updated March 15, 2016, 1:21 a.m.)
Review request for sentry, Anne Yu, Hao Hao, Lenni Kuff, and Sravya Tirukkovalur.
Changes
-------
add git version
Repository: sentry
Description
-------
Add SSL support, print version info on Sentry Service webpage
Diffs (updated)
-----
sentry-provider/sentry-provider-db/pom.xml 1dbfad49f7b9bb1b963221600bbd8d0400ab8d5d
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java 1bdea2c55de12a999f94ea33f8709311c7c2c7f2
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 94bd2a95c77a9691cbaa578ebf417e49c339b7ed
sentry-provider/sentry-provider-db/src/main/webapp/SentryService.html cd19dd8b9a228e56800bd06f0714a5ab17634347
sentry-provider/sentry-provider-db/src/main/webapp/css/sentry.css e5b3d437406a881e78494547791e1b26bb4f2bc8
Diff: https://reviews.apache.org/r/44501/diff/
Testing
-------
Thanks,
Li Li