You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by GitBox <gi...@apache.org> on 2020/04/23 12:24:30 UTC
[GitHub] [zeppelin] Reamer opened a new pull request #3747: [ZEPPELIN-4774] TLS - create Keystores with PEM files
Reamer opened a new pull request #3747:
URL: https://github.com/apache/zeppelin/pull/3747
### What is this PR for?
This PR uses crypto functions from the [bouncycastle project](https://www.bouncycastle.org/). The library was already used in `zeppelin-interpreter`.
I don't see a license link in the [LICENSE](https://github.com/apache/zeppelin/blob/master/LICENSE). I hope someone can help me to correct this situation.
### What type of PR is it?
- Feature
### Todos
* [ ] - Task
### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-4774
### How should this be tested?
* Travis-CI: https://travis-ci.org/github/Reamer/zeppelin/builds/678481267
### Questions:
* Does the licenses files need update? Maybe
* Is there breaking changes for older versions? No
* Does this needs documentation? Maybe
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a change in pull request #3747: [ZEPPELIN-4774] TLS - create Keystores with PEM files
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #3747:
URL: https://github.com/apache/zeppelin/pull/3747#discussion_r414531965
##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/server/ZeppelinServer.java
##########
@@ -438,19 +440,38 @@ private static void setupClusterManagerServer(ServiceLocator serviceLocator) {
private static SslContextFactory getSslContextFactory(ZeppelinConfiguration conf) {
SslContextFactory.Server sslContextFactory = new SslContextFactory.Server();
- // Set keystore
- sslContextFactory.setKeyStorePath(conf.getKeyStorePath());
- sslContextFactory.setKeyStoreType(conf.getKeyStoreType());
- sslContextFactory.setKeyStorePassword(conf.getKeyStorePassword());
- sslContextFactory.setKeyManagerPassword(conf.getKeyManagerPassword());
-
- if (conf.useClientAuth()) {
- sslContextFactory.setNeedClientAuth(conf.useClientAuth());
-
- // Set truststore
- sslContextFactory.setTrustStorePath(conf.getTrustStorePath());
- sslContextFactory.setTrustStoreType(conf.getTrustStoreType());
- sslContextFactory.setTrustStorePassword(conf.getTrustStorePassword());
+ // Check for PEM files
+ if (StringUtils.isNoneBlank(conf.getPemKeyFile(), conf.getPemCertFile())) {
+ File pemKey = new File(conf.getPemKeyFile());
+ File pemCert = new File(conf.getPemCertFile());
Review comment:
Thanks for your review. I added some checks, which should give more output in case of an error.
##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/server/ZeppelinServer.java
##########
@@ -438,19 +440,38 @@ private static void setupClusterManagerServer(ServiceLocator serviceLocator) {
private static SslContextFactory getSslContextFactory(ZeppelinConfiguration conf) {
SslContextFactory.Server sslContextFactory = new SslContextFactory.Server();
- // Set keystore
- sslContextFactory.setKeyStorePath(conf.getKeyStorePath());
- sslContextFactory.setKeyStoreType(conf.getKeyStoreType());
- sslContextFactory.setKeyStorePassword(conf.getKeyStorePassword());
- sslContextFactory.setKeyManagerPassword(conf.getKeyManagerPassword());
-
- if (conf.useClientAuth()) {
- sslContextFactory.setNeedClientAuth(conf.useClientAuth());
-
- // Set truststore
- sslContextFactory.setTrustStorePath(conf.getTrustStorePath());
- sslContextFactory.setTrustStoreType(conf.getTrustStoreType());
- sslContextFactory.setTrustStorePassword(conf.getTrustStorePassword());
+ // Check for PEM files
+ if (StringUtils.isNoneBlank(conf.getPemKeyFile(), conf.getPemCertFile())) {
+ File pemKey = new File(conf.getPemKeyFile());
+ File pemCert = new File(conf.getPemCertFile());
+ try {
+ String password = conf.getPemKeyPassword();
+ sslContextFactory.setKeyStore(PEMImporter.loadKeyStore(pemCert, pemKey, password));
+ sslContextFactory.setKeyStoreType("JKS");
+ sslContextFactory.setKeyStorePassword(password);
+ if (conf.useClientAuth() && StringUtils.isNotBlank(conf.getPemCAFile())) {
+ File pemCA = new File(conf.getPemCAFile());
Review comment:
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on pull request #3747: [ZEPPELIN-4774] TLS - create Keystores with PEM files
Posted by GitBox <gi...@apache.org>.
Reamer commented on pull request #3747:
URL: https://github.com/apache/zeppelin/pull/3747#issuecomment-618978970
Hi @alexott,
thanks for your review. I have corrected the points you raised.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] alexott commented on a change in pull request #3747: [ZEPPELIN-4774] TLS - create Keystores with PEM files
Posted by GitBox <gi...@apache.org>.
alexott commented on a change in pull request #3747:
URL: https://github.com/apache/zeppelin/pull/3747#discussion_r414018198
##########
File path: conf/zeppelin-site.xml.template
##########
@@ -456,6 +456,38 @@
</property>
-->
+<!--
+<property>
+ <name>zeppelin.ssl.pem.key</name>
+ <value></value>
+ <description>Path to a TLS key file saved as pem.</description>
+</property>
+-->
+
+<!--
+<property>
+ <name>zeppelin.ssl.pem.key.password</name>
+ <value></value>
+ <description>Password of the TLS key file saved as pem.</description>
+</property>
+-->
+
+<!--
+<property>
+ <name>zeppelin.ssl.pem.cert</name>
+ <value></value>
+ <description>Path to a TLS full certificate chain file saved as pem.</description>
+</property>
+-->
+
+<!--
+<property>
+ <name>zeppelin.ssl.pem.ca</name>
+ <value></value>
+ <description>Path to a TLS certificate file containing all client certificates</description>
Review comment:
Is it client certificate? not the Certificate Authority?
##########
File path: docs/setup/operation/configuration.md
##########
@@ -173,6 +173,30 @@ If both are defined, then the **environment variables** will take priority.
<td></td>
<td></td>
</tr>
+ <tr>
+ <td><h6 class="properties">ZEPPELIN_SSL_PEM_KEY</h6></td>
+ <td><h6 class="properties">zeppelin.ssl.pem.key</h6></td>
+ <td></td>
+ <td></td>
+ </tr>
+ <tr>
+ <td><h6 class="properties">ZEPPELIN_SSL_PEM_KEY_PASSWORD</h6></td>
+ <td><h6 class="properties">zeppelin.ssl.pem.key.password</h6></td>
+ <td></td>
+ <td></td>
+ </tr>
+ <tr>
+ <td><h6 class="properties">ZEPPELIN_SSL_PEM_CERT</h6></td>
+ <td><h6 class="properties">zeppelin.ssl.pem.cert</h6></td>
+ <td></td>
+ <td></td>
+ </tr>
+ <tr>
+ <td><h6 class="properties">ZEPPELIN_SSL_PEM_CA</h6></td>
+ <td><h6 class="properties">zeppelin.ssl.pem.ca</h6></td>
+ <td></td>
+ <td></td>
Review comment:
Should we add description of what this mean?
##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/server/ZeppelinServer.java
##########
@@ -438,19 +440,38 @@ private static void setupClusterManagerServer(ServiceLocator serviceLocator) {
private static SslContextFactory getSslContextFactory(ZeppelinConfiguration conf) {
SslContextFactory.Server sslContextFactory = new SslContextFactory.Server();
- // Set keystore
- sslContextFactory.setKeyStorePath(conf.getKeyStorePath());
- sslContextFactory.setKeyStoreType(conf.getKeyStoreType());
- sslContextFactory.setKeyStorePassword(conf.getKeyStorePassword());
- sslContextFactory.setKeyManagerPassword(conf.getKeyManagerPassword());
-
- if (conf.useClientAuth()) {
- sslContextFactory.setNeedClientAuth(conf.useClientAuth());
-
- // Set truststore
- sslContextFactory.setTrustStorePath(conf.getTrustStorePath());
- sslContextFactory.setTrustStoreType(conf.getTrustStoreType());
- sslContextFactory.setTrustStorePassword(conf.getTrustStorePassword());
+ // Check for PEM files
+ if (StringUtils.isNoneBlank(conf.getPemKeyFile(), conf.getPemCertFile())) {
+ File pemKey = new File(conf.getPemKeyFile());
+ File pemCert = new File(conf.getPemCertFile());
+ try {
+ String password = conf.getPemKeyPassword();
+ sslContextFactory.setKeyStore(PEMImporter.loadKeyStore(pemCert, pemKey, password));
+ sslContextFactory.setKeyStoreType("JKS");
+ sslContextFactory.setKeyStorePassword(password);
+ if (conf.useClientAuth() && StringUtils.isNotBlank(conf.getPemCAFile())) {
+ File pemCA = new File(conf.getPemCAFile());
Review comment:
here as well
##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/server/ZeppelinServer.java
##########
@@ -438,19 +440,38 @@ private static void setupClusterManagerServer(ServiceLocator serviceLocator) {
private static SslContextFactory getSslContextFactory(ZeppelinConfiguration conf) {
SslContextFactory.Server sslContextFactory = new SslContextFactory.Server();
- // Set keystore
- sslContextFactory.setKeyStorePath(conf.getKeyStorePath());
- sslContextFactory.setKeyStoreType(conf.getKeyStoreType());
- sslContextFactory.setKeyStorePassword(conf.getKeyStorePassword());
- sslContextFactory.setKeyManagerPassword(conf.getKeyManagerPassword());
-
- if (conf.useClientAuth()) {
- sslContextFactory.setNeedClientAuth(conf.useClientAuth());
-
- // Set truststore
- sslContextFactory.setTrustStorePath(conf.getTrustStorePath());
- sslContextFactory.setTrustStoreType(conf.getTrustStoreType());
- sslContextFactory.setTrustStorePassword(conf.getTrustStorePassword());
+ // Check for PEM files
+ if (StringUtils.isNoneBlank(conf.getPemKeyFile(), conf.getPemCertFile())) {
+ File pemKey = new File(conf.getPemKeyFile());
+ File pemCert = new File(conf.getPemCertFile());
Review comment:
I would be make explicit check for presence of files, for easier identification of errors...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a change in pull request #3747: [ZEPPELIN-4774] TLS - create Keystores with PEM files
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #3747:
URL: https://github.com/apache/zeppelin/pull/3747#discussion_r414531262
##########
File path: conf/zeppelin-site.xml.template
##########
@@ -456,6 +456,38 @@
</property>
-->
+<!--
+<property>
+ <name>zeppelin.ssl.pem.key</name>
+ <value></value>
+ <description>Path to a TLS key file saved as pem.</description>
+</property>
+-->
+
+<!--
+<property>
+ <name>zeppelin.ssl.pem.key.password</name>
+ <value></value>
+ <description>Password of the TLS key file saved as pem.</description>
+</property>
+-->
+
+<!--
+<property>
+ <name>zeppelin.ssl.pem.cert</name>
+ <value></value>
+ <description>Path to a TLS full certificate chain file saved as pem.</description>
+</property>
+-->
+
+<!--
+<property>
+ <name>zeppelin.ssl.pem.ca</name>
+ <value></value>
+ <description>Path to a TLS certificate file containing all client certificates</description>
Review comment:
Updated description with the help of [Apache documentation](https://httpd.apache.org/docs/current/mod/mod_ssl.html#sslcacertificatefile).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a change in pull request #3747: [ZEPPELIN-4774] TLS - create Keystores with PEM files
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #3747:
URL: https://github.com/apache/zeppelin/pull/3747#discussion_r414531455
##########
File path: docs/setup/operation/configuration.md
##########
@@ -173,6 +173,30 @@ If both are defined, then the **environment variables** will take priority.
<td></td>
<td></td>
</tr>
+ <tr>
+ <td><h6 class="properties">ZEPPELIN_SSL_PEM_KEY</h6></td>
+ <td><h6 class="properties">zeppelin.ssl.pem.key</h6></td>
+ <td></td>
+ <td></td>
+ </tr>
+ <tr>
+ <td><h6 class="properties">ZEPPELIN_SSL_PEM_KEY_PASSWORD</h6></td>
+ <td><h6 class="properties">zeppelin.ssl.pem.key.password</h6></td>
+ <td></td>
+ <td></td>
+ </tr>
+ <tr>
+ <td><h6 class="properties">ZEPPELIN_SSL_PEM_CERT</h6></td>
+ <td><h6 class="properties">zeppelin.ssl.pem.cert</h6></td>
+ <td></td>
+ <td></td>
+ </tr>
+ <tr>
+ <td><h6 class="properties">ZEPPELIN_SSL_PEM_CA</h6></td>
+ <td><h6 class="properties">zeppelin.ssl.pem.ca</h6></td>
+ <td></td>
+ <td></td>
Review comment:
Added a description
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Leemoonsoo commented on issue #3747: [ZEPPELIN-4774] TLS - create Keystores with PEM files
Posted by GitBox <gi...@apache.org>.
Leemoonsoo commented on issue #3747:
URL: https://github.com/apache/zeppelin/pull/3747#issuecomment-618448746
`/LICENSE` includes source package licenses only. Binary package licenses are placed under `/zeppelin-distribution/src/bin_license/LICENSE`. I can find bcpkix-jdk15on 1.60 [here](https://github.com/apache/zeppelin/blob/master/zeppelin-distribution/src/bin_license/LICENSE#L280).
LGTM. merge to master and branch-0.9 if no further discussions.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org