You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by GitBox <gi...@apache.org> on 2020/03/04 18:57:42 UTC
[GitHub] [drill] ihuzenko opened a new pull request #2012: DRILL-7625: Add
options for SslContextFactory
ihuzenko opened a new pull request #2012: DRILL-7625: Add options for SslContextFactory
URL: https://github.com/apache/drill/pull/2012
# [DRILL-7625](https://issues.apache.org/jira/browse/DRILL-7625): Add options for SslContextFactory
## Description
Added ability to set more options on Jetty's SslContextFactory object and fixed application of drill.exec.ssl.protocol setting for Web UI client.
## Documentation
Users now can provide more granular configuration for Jetty https connector. All additional options are listed in drill-override-example.conf in this pull request.
## Testing
Added unit test.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #2012:
DRILL-7625: Add options for SslContextFactory
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2012: DRILL-7625: Add options for SslContextFactory
URL: https://github.com/apache/drill/pull/2012#discussion_r388829162
##########
File path: common/src/main/java/org/apache/drill/common/config/NestedConfig.java
##########
@@ -111,6 +111,14 @@ public String getString(String path) {
return c.getString(path);
}
+ public String getString(String path, String defaultValue) {
Review comment:
All other methods from this class implement methods from the interface with some JavaDocs.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #2012:
DRILL-7625: Add options for SslContextFactory
Posted by GitBox <gi...@apache.org>.
ihuzenko commented on a change in pull request #2012: DRILL-7625: Add options for SslContextFactory
URL: https://github.com/apache/drill/pull/2012#discussion_r388825714
##########
File path: common/src/main/java/org/apache/drill/common/config/NestedConfig.java
##########
@@ -111,6 +111,14 @@ public String getString(String path) {
return c.getString(path);
}
+ public String getString(String path, String defaultValue) {
Review comment:
won't it look strange that class and all its methods don't have Javadocs and then BOOM -> one documented method?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] agozhiy commented on a change in pull request #2012:
DRILL-7625: Add options for SslContextFactory
Posted by GitBox <gi...@apache.org>.
agozhiy commented on a change in pull request #2012: DRILL-7625: Add options for SslContextFactory
URL: https://github.com/apache/drill/pull/2012#discussion_r390350047
##########
File path: common/src/main/java/org/apache/drill/common/config/NestedConfig.java
##########
@@ -111,6 +112,19 @@ public String getString(String path) {
return c.getString(path);
}
+ /**
+ * Returns string option or passed default value if option is not set.
+ *
+ * @param path option path
+ * @param defaultValue default value
+ * @return option or default value
+ */
+ public String getString(String path, String defaultValue) {
Review comment:
There is no need for this method at all, default values can be set directly in drill-module.conf.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #2012:
DRILL-7625: Add options for SslContextFactory
Posted by GitBox <gi...@apache.org>.
ihuzenko commented on a change in pull request #2012: DRILL-7625: Add options for SslContextFactory
URL: https://github.com/apache/drill/pull/2012#discussion_r388831691
##########
File path: common/src/main/java/org/apache/drill/common/config/NestedConfig.java
##########
@@ -111,6 +111,14 @@ public String getString(String path) {
return c.getString(path);
}
+ public String getString(String path, String defaultValue) {
+ String value = hasPath(path) ? getString(path) : null;
+ if (value == null || (value = value.trim()).isEmpty()) {
+ return defaultValue;
+ }
+ return value;
Review comment:
```StringUtils.defaultIfBlank``` changes method logic, since in an original method was:
```java
private String getConfigParamWithDefault(String name, String defaultValue) {
String value = "";
if (config.hasPath(name)) {
value = config.getString(name);
}
if (value.isEmpty()) {
value = defaultValue;
}
value = value.trim();
return value;
}
```
Although I don't think that trimming was actually necessary, so I'll try your suggestion.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #2012:
DRILL-7625: Add options for SslContextFactory
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2012: DRILL-7625: Add options for SslContextFactory
URL: https://github.com/apache/drill/pull/2012#discussion_r388274269
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/ssl/SslContextFactoryConfigurator.java
##########
@@ -0,0 +1,202 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.server.rest.ssl;
+
+import org.apache.commons.lang3.RandomStringUtils;
+import org.apache.drill.common.config.DrillConfig;
+import org.apache.drill.exec.ExecConstants;
+import org.apache.drill.exec.ssl.SSLConfig;
+import org.apache.drill.exec.ssl.SSLConfigBuilder;
+import org.bouncycastle.asn1.x500.X500NameBuilder;
+import org.bouncycastle.asn1.x500.style.BCStyle;
+import org.bouncycastle.cert.X509v3CertificateBuilder;
+import org.bouncycastle.cert.jcajce.JcaX509CertificateConverter;
+import org.bouncycastle.cert.jcajce.JcaX509v3CertificateBuilder;
+import org.bouncycastle.operator.ContentSigner;
+import org.bouncycastle.operator.jcajce.JcaContentSignerBuilder;
+import org.eclipse.jetty.util.ssl.SslContextFactory;
+import org.joda.time.DateTime;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.math.BigInteger;
+import java.security.KeyPair;
+import java.security.KeyPairGenerator;
+import java.security.KeyStore;
+import java.security.SecureRandom;
+import java.security.cert.X509Certificate;
+import java.util.Date;
+import java.util.List;
+import java.util.function.Consumer;
+import java.util.function.Function;
+
+public class SslContextFactoryConfigurator {
+ private static final Logger logger = LoggerFactory.getLogger(SslContextFactoryConfigurator.class);
+
+ private final DrillConfig config;
+ private final String drillbitEndpointAddress;
+
+ public SslContextFactoryConfigurator(DrillConfig config, String drillbitEndpointAddress) {
+ this.config = config;
+ this.drillbitEndpointAddress = drillbitEndpointAddress;
+ }
+
+ public SslContextFactory configureNewSslContextFactory() throws Exception {
+ SSLConfig sslConf = new SSLConfigBuilder()
+ .config(config).mode(SSLConfig.Mode.SERVER)
+ .initializeSSLContext(false).validateKeyStore(true)
Review comment:
```suggestion
.config(config)
.mode(SSLConfig.Mode.SERVER)
.initializeSSLContext(false)
.validateKeyStore(true)
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #2012:
DRILL-7625: Add options for SslContextFactory
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2012: DRILL-7625: Add options for SslContextFactory
URL: https://github.com/apache/drill/pull/2012#discussion_r388271496
##########
File path: common/src/main/java/org/apache/drill/common/config/NestedConfig.java
##########
@@ -111,6 +111,14 @@ public String getString(String path) {
return c.getString(path);
}
+ public String getString(String path, String defaultValue) {
+ String value = hasPath(path) ? getString(path) : null;
+ if (value == null || (value = value.trim()).isEmpty()) {
+ return defaultValue;
+ }
+ return value;
Review comment:
Nit: May be used `StringUtils.defaultIfBlank)` method to simplify this code.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] agozhiy commented on a change in pull request #2012:
DRILL-7625: Add options for SslContextFactory
Posted by GitBox <gi...@apache.org>.
agozhiy commented on a change in pull request #2012: DRILL-7625: Add options for SslContextFactory
URL: https://github.com/apache/drill/pull/2012#discussion_r390351471
##########
File path: common/src/main/java/org/apache/drill/common/config/NestedConfig.java
##########
@@ -111,6 +112,19 @@ public String getString(String path) {
return c.getString(path);
}
+ /**
+ * Returns string option or passed default value if option is not set.
+ *
+ * @param path option path
+ * @param defaultValue default value
+ * @return option or default value
+ */
+ public String getString(String path, String defaultValue) {
Review comment:
There is no need for this method at all, default values can be set directly in drill-module.conf.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #2012:
DRILL-7625: Add options for SslContextFactory
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2012: DRILL-7625: Add options for SslContextFactory
URL: https://github.com/apache/drill/pull/2012#discussion_r388829162
##########
File path: common/src/main/java/org/apache/drill/common/config/NestedConfig.java
##########
@@ -111,6 +111,14 @@ public String getString(String path) {
return c.getString(path);
}
+ public String getString(String path, String defaultValue) {
Review comment:
Most of the methods from this class inherit JavaDoc from the interface they are implementing.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #2012:
DRILL-7625: Add options for SslContextFactory
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2012: DRILL-7625: Add options for SslContextFactory
URL: https://github.com/apache/drill/pull/2012#discussion_r388822999
##########
File path: distribution/src/main/resources/drill-override-example.conf
##########
@@ -113,6 +113,72 @@ drill.exec: {
# Location to keytab file for above spnego principal
spnego.keytab: "<keytab_file_location>";
},
+ jetty: {
+ server: {
+ # Optional params to set on Jetty's org.eclipse.jetty.util.ssl.SslContextFactory when drill.exec.http.ssl_enabled
+ sslContextFactory: {
+ # allows to specify cert to use when multiple non-SNI certificates are available.
+ certAlias: "certAlias",
+ # path to file that contains Certificate Revocation List
+ crlPath: "/etc/file.crl",
+ # enable Certificate Revocation List Distribution Points Support
+ enableCRLDP: false,
+ # enable On-Line Certificate Status Protocol support
+ enableOCSP: false,
+ # when set to "HTTPS" hostname verification will be enabled
+ endpointIdentificationAlgorithm: "HTTPS",
+ # accepts exact cipher suite names and/or regular expressions.
+ excludeCipherSuites: ["SSL_DHE_DSS_WITH_DES_CBC_SHA"],
+ # list of TLS/SSL protocols to exclude
+ excludeProtocols: ["TLSv1.1"],
+ # accepts exact cipher suite names and/or regular expressions.
+ includeCipherSuites: ["SSL_DHE_RSA_EXPORT_WITH_DES40_CBC_SHA", "SSL_DHE_DSS_EXPORT_WITH_DES40_CBC_SHA"],
+ # list of TLS/SSL protocols to include
+ includeProtocols: ["TLSv1.2", "TLSv1.3"],
+ # the algorithm name (default "SunX509") used by the javax.net.ssl.KeyManagerFactory
+ keyManagerFactoryAlgorithm: "SunX509",
+ # classname of custom java.security.Provider implementation
+ keyStoreProvider: "fully.qualified.class.Name",
+ # type of key store (default "JKS")
+ keyStoreType: "JKS",
+ # max number of intermediate certificates in sertificate chain
+ maxCertPathLength: -1,
+ # set true if ssl needs client authentication
+ needClientAuth: false,
+ # location of the OCSP Responder
+ ocspResponderURL: "",
+ # javax.net.ssl.SSLContext provider class name
+ provider: "fully.qualified.class.Name",
Review comment:
My objection connected with setting non-null value for this option is because it may break configuration for the case when the default context provider is used. Here is a code from `SslContextFactory`:
```
context = _sslProvider == null ? SSLContext.getInstance(_sslProtocol) : SSLContext.getInstance(_sslProtocol, _sslProvider);
```
So user may want to specify `sslProtocol` only, but with non-null value it would fail.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] cgivre commented on issue #2012: DRILL-7625: Add options
for SslContextFactory
Posted by GitBox <gi...@apache.org>.
cgivre commented on issue #2012: DRILL-7625: Add options for SslContextFactory
URL: https://github.com/apache/drill/pull/2012#issuecomment-597061241
Could we make a note to include this in the web documentation for Drill once this is committed?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #2012:
DRILL-7625: Add options for SslContextFactory
Posted by GitBox <gi...@apache.org>.
ihuzenko commented on a change in pull request #2012: DRILL-7625: Add options for SslContextFactory
URL: https://github.com/apache/drill/pull/2012#discussion_r388847580
##########
File path: distribution/src/main/resources/drill-override-example.conf
##########
@@ -113,6 +113,72 @@ drill.exec: {
# Location to keytab file for above spnego principal
spnego.keytab: "<keytab_file_location>";
},
+ jetty: {
+ server: {
+ # Optional params to set on Jetty's org.eclipse.jetty.util.ssl.SslContextFactory when drill.exec.http.ssl_enabled
+ sslContextFactory: {
+ # allows to specify cert to use when multiple non-SNI certificates are available.
+ certAlias: "certAlias",
+ # path to file that contains Certificate Revocation List
+ crlPath: "/etc/file.crl",
+ # enable Certificate Revocation List Distribution Points Support
+ enableCRLDP: false,
+ # enable On-Line Certificate Status Protocol support
+ enableOCSP: false,
+ # when set to "HTTPS" hostname verification will be enabled
+ endpointIdentificationAlgorithm: "HTTPS",
+ # accepts exact cipher suite names and/or regular expressions.
+ excludeCipherSuites: ["SSL_DHE_DSS_WITH_DES_CBC_SHA"],
+ # list of TLS/SSL protocols to exclude
+ excludeProtocols: ["TLSv1.1"],
+ # accepts exact cipher suite names and/or regular expressions.
+ includeCipherSuites: ["SSL_DHE_RSA_EXPORT_WITH_DES40_CBC_SHA", "SSL_DHE_DSS_EXPORT_WITH_DES40_CBC_SHA"],
+ # list of TLS/SSL protocols to include
+ includeProtocols: ["TLSv1.2", "TLSv1.3"],
+ # the algorithm name (default "SunX509") used by the javax.net.ssl.KeyManagerFactory
+ keyManagerFactoryAlgorithm: "SunX509",
+ # classname of custom java.security.Provider implementation
+ keyStoreProvider: "fully.qualified.class.Name",
+ # type of key store (default "JKS")
+ keyStoreType: "JKS",
+ # max number of intermediate certificates in sertificate chain
+ maxCertPathLength: -1,
+ # set true if ssl needs client authentication
+ needClientAuth: false,
+ # location of the OCSP Responder
+ ocspResponderURL: "",
+ # javax.net.ssl.SSLContext provider class name
+ provider: "fully.qualified.class.Name",
Review comment:
This file is far from Drill defaults, I'm convinced that if you'll try to actually use the file for overriding conf, your Drillbit will fail in numerous different ways. The file is here just to show a user which options are available in override conf. And handled badly, since there are a lot of options not present in the file.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] agozhiy commented on a change in pull request #2012:
DRILL-7625: Add options for SslContextFactory
Posted by GitBox <gi...@apache.org>.
agozhiy commented on a change in pull request #2012: DRILL-7625: Add options for SslContextFactory
URL: https://github.com/apache/drill/pull/2012#discussion_r390347618
##########
File path: common/src/main/java/org/apache/drill/common/config/NestedConfig.java
##########
@@ -111,6 +111,14 @@ public String getString(String path) {
return c.getString(path);
}
+ public String getString(String path, String defaultValue) {
+ String value = hasPath(path) ? getString(path) : null;
+ if (value == null || (value = value.trim()).isEmpty()) {
+ return defaultValue;
+ }
+ return value;
Review comment:
There is no need for this method at all, default values can be set directly in drill-module.conf.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] agozhiy commented on issue #2012: DRILL-7625: Add options
for SslContextFactory
Posted by GitBox <gi...@apache.org>.
agozhiy commented on issue #2012: DRILL-7625: Add options for SslContextFactory
URL: https://github.com/apache/drill/pull/2012#issuecomment-597130330
Thank you, +1.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #2012:
DRILL-7625: Add options for SslContextFactory
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2012: DRILL-7625: Add options for SslContextFactory
URL: https://github.com/apache/drill/pull/2012#discussion_r388285007
##########
File path: distribution/src/main/resources/drill-override-example.conf
##########
@@ -113,6 +113,72 @@ drill.exec: {
# Location to keytab file for above spnego principal
spnego.keytab: "<keytab_file_location>";
},
+ jetty: {
+ server: {
+ # Optional params to set on Jetty's org.eclipse.jetty.util.ssl.SslContextFactory when drill.exec.http.ssl_enabled
+ sslContextFactory: {
+ # allows to specify cert to use when multiple non-SNI certificates are available.
+ certAlias: "certAlias",
+ # path to file that contains Certificate Revocation List
+ crlPath: "/etc/file.crl",
+ # enable Certificate Revocation List Distribution Points Support
+ enableCRLDP: false,
+ # enable On-Line Certificate Status Protocol support
+ enableOCSP: false,
+ # when set to "HTTPS" hostname verification will be enabled
+ endpointIdentificationAlgorithm: "HTTPS",
+ # accepts exact cipher suite names and/or regular expressions.
+ excludeCipherSuites: ["SSL_DHE_DSS_WITH_DES_CBC_SHA"],
+ # list of TLS/SSL protocols to exclude
+ excludeProtocols: ["TLSv1.1"],
+ # accepts exact cipher suite names and/or regular expressions.
+ includeCipherSuites: ["SSL_DHE_RSA_EXPORT_WITH_DES40_CBC_SHA", "SSL_DHE_DSS_EXPORT_WITH_DES40_CBC_SHA"],
+ # list of TLS/SSL protocols to include
+ includeProtocols: ["TLSv1.2", "TLSv1.3"],
+ # the algorithm name (default "SunX509") used by the javax.net.ssl.KeyManagerFactory
+ keyManagerFactoryAlgorithm: "SunX509",
+ # classname of custom java.security.Provider implementation
+ keyStoreProvider: "fully.qualified.class.Name",
+ # type of key store (default "JKS")
+ keyStoreType: "JKS",
+ # max number of intermediate certificates in sertificate chain
+ maxCertPathLength: -1,
+ # set true if ssl needs client authentication
+ needClientAuth: false,
+ # location of the OCSP Responder
+ ocspResponderURL: "",
+ # javax.net.ssl.SSLContext provider class name
+ provider: "fully.qualified.class.Name",
Review comment:
It looks like an invalid config. Have you checked whether with these default configs Drill works fine when SSL is enabled? If it works ok, does this config affects anything?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #2012:
DRILL-7625: Add options for SslContextFactory
Posted by GitBox <gi...@apache.org>.
ihuzenko commented on a change in pull request #2012: DRILL-7625: Add options for SslContextFactory
URL: https://github.com/apache/drill/pull/2012#discussion_r388817420
##########
File path: distribution/src/main/resources/drill-override-example.conf
##########
@@ -113,6 +113,72 @@ drill.exec: {
# Location to keytab file for above spnego principal
spnego.keytab: "<keytab_file_location>";
},
+ jetty: {
+ server: {
+ # Optional params to set on Jetty's org.eclipse.jetty.util.ssl.SslContextFactory when drill.exec.http.ssl_enabled
+ sslContextFactory: {
+ # allows to specify cert to use when multiple non-SNI certificates are available.
+ certAlias: "certAlias",
+ # path to file that contains Certificate Revocation List
+ crlPath: "/etc/file.crl",
+ # enable Certificate Revocation List Distribution Points Support
+ enableCRLDP: false,
+ # enable On-Line Certificate Status Protocol support
+ enableOCSP: false,
+ # when set to "HTTPS" hostname verification will be enabled
+ endpointIdentificationAlgorithm: "HTTPS",
+ # accepts exact cipher suite names and/or regular expressions.
+ excludeCipherSuites: ["SSL_DHE_DSS_WITH_DES_CBC_SHA"],
+ # list of TLS/SSL protocols to exclude
+ excludeProtocols: ["TLSv1.1"],
+ # accepts exact cipher suite names and/or regular expressions.
+ includeCipherSuites: ["SSL_DHE_RSA_EXPORT_WITH_DES40_CBC_SHA", "SSL_DHE_DSS_EXPORT_WITH_DES40_CBC_SHA"],
+ # list of TLS/SSL protocols to include
+ includeProtocols: ["TLSv1.2", "TLSv1.3"],
+ # the algorithm name (default "SunX509") used by the javax.net.ssl.KeyManagerFactory
+ keyManagerFactoryAlgorithm: "SunX509",
+ # classname of custom java.security.Provider implementation
+ keyStoreProvider: "fully.qualified.class.Name",
+ # type of key store (default "JKS")
+ keyStoreType: "JKS",
+ # max number of intermediate certificates in sertificate chain
+ maxCertPathLength: -1,
+ # set true if ssl needs client authentication
+ needClientAuth: false,
+ # location of the OCSP Responder
+ ocspResponderURL: "",
+ # javax.net.ssl.SSLContext provider class name
+ provider: "fully.qualified.class.Name",
Review comment:
I don't know which option to set here, it seems that the option allows using a custom implementation of ```java.security.Provider``` for SSLContext. As mentioned in the comment above ```sslContextFactory:``` all the options are optional and those who will configure them are expected to know what they're doing.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #2012:
DRILL-7625: Add options for SslContextFactory
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2012: DRILL-7625: Add options for SslContextFactory
URL: https://github.com/apache/drill/pull/2012#discussion_r388829162
##########
File path: common/src/main/java/org/apache/drill/common/config/NestedConfig.java
##########
@@ -111,6 +111,14 @@ public String getString(String path) {
return c.getString(path);
}
+ public String getString(String path, String defaultValue) {
Review comment:
All other methods from this class implement methods from the interface with JavaDocs. So technically, it is a single method without JavaDoc...
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #2012:
DRILL-7625: Add options for SslContextFactory
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2012: DRILL-7625: Add options for SslContextFactory
URL: https://github.com/apache/drill/pull/2012#discussion_r388287319
##########
File path: common/src/main/java/org/apache/drill/common/config/NestedConfig.java
##########
@@ -111,6 +111,14 @@ public String getString(String path) {
return c.getString(path);
}
+ public String getString(String path, String defaultValue) {
Review comment:
Please add JavaDoc.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] agozhiy commented on a change in pull request #2012:
DRILL-7625: Add options for SslContextFactory
Posted by GitBox <gi...@apache.org>.
agozhiy commented on a change in pull request #2012: DRILL-7625: Add options for SslContextFactory
URL: https://github.com/apache/drill/pull/2012#discussion_r390350047
##########
File path: common/src/main/java/org/apache/drill/common/config/NestedConfig.java
##########
@@ -111,6 +112,19 @@ public String getString(String path) {
return c.getString(path);
}
+ /**
+ * Returns string option or passed default value if option is not set.
+ *
+ * @param path option path
+ * @param defaultValue default value
+ * @return option or default value
+ */
+ public String getString(String path, String defaultValue) {
Review comment:
There is no need for this method at all, default values can be set directly in drill-module.conf.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] asfgit closed pull request #2012: DRILL-7625: Add options
for SslContextFactory
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #2012: DRILL-7625: Add options for SslContextFactory
URL: https://github.com/apache/drill/pull/2012
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #2012:
DRILL-7625: Add options for SslContextFactory
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2012: DRILL-7625: Add options for SslContextFactory
URL: https://github.com/apache/drill/pull/2012#discussion_r388848608
##########
File path: distribution/src/main/resources/drill-override-example.conf
##########
@@ -113,6 +113,72 @@ drill.exec: {
# Location to keytab file for above spnego principal
spnego.keytab: "<keytab_file_location>";
},
+ jetty: {
+ server: {
+ # Optional params to set on Jetty's org.eclipse.jetty.util.ssl.SslContextFactory when drill.exec.http.ssl_enabled
+ sslContextFactory: {
+ # allows to specify cert to use when multiple non-SNI certificates are available.
+ certAlias: "certAlias",
+ # path to file that contains Certificate Revocation List
+ crlPath: "/etc/file.crl",
+ # enable Certificate Revocation List Distribution Points Support
+ enableCRLDP: false,
+ # enable On-Line Certificate Status Protocol support
+ enableOCSP: false,
+ # when set to "HTTPS" hostname verification will be enabled
+ endpointIdentificationAlgorithm: "HTTPS",
+ # accepts exact cipher suite names and/or regular expressions.
+ excludeCipherSuites: ["SSL_DHE_DSS_WITH_DES_CBC_SHA"],
+ # list of TLS/SSL protocols to exclude
+ excludeProtocols: ["TLSv1.1"],
+ # accepts exact cipher suite names and/or regular expressions.
+ includeCipherSuites: ["SSL_DHE_RSA_EXPORT_WITH_DES40_CBC_SHA", "SSL_DHE_DSS_EXPORT_WITH_DES40_CBC_SHA"],
+ # list of TLS/SSL protocols to include
+ includeProtocols: ["TLSv1.2", "TLSv1.3"],
+ # the algorithm name (default "SunX509") used by the javax.net.ssl.KeyManagerFactory
+ keyManagerFactoryAlgorithm: "SunX509",
+ # classname of custom java.security.Provider implementation
+ keyStoreProvider: "fully.qualified.class.Name",
+ # type of key store (default "JKS")
+ keyStoreType: "JKS",
+ # max number of intermediate certificates in sertificate chain
+ maxCertPathLength: -1,
+ # set true if ssl needs client authentication
+ needClientAuth: false,
+ # location of the OCSP Responder
+ ocspResponderURL: "",
+ # javax.net.ssl.SSLContext provider class name
+ provider: "fully.qualified.class.Name",
Review comment:
Oh, sorry, I thought it was `drill-module.conf` file. In this case, it is ok to leave as it is. Sorry for the confusion.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #2012:
DRILL-7625: Add options for SslContextFactory
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2012: DRILL-7625: Add options for SslContextFactory
URL: https://github.com/apache/drill/pull/2012#discussion_r388289566
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/ssl/SslContextFactoryConfigurator.java
##########
@@ -0,0 +1,202 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.server.rest.ssl;
+
+import org.apache.commons.lang3.RandomStringUtils;
+import org.apache.drill.common.config.DrillConfig;
+import org.apache.drill.exec.ExecConstants;
+import org.apache.drill.exec.ssl.SSLConfig;
+import org.apache.drill.exec.ssl.SSLConfigBuilder;
+import org.bouncycastle.asn1.x500.X500NameBuilder;
+import org.bouncycastle.asn1.x500.style.BCStyle;
+import org.bouncycastle.cert.X509v3CertificateBuilder;
+import org.bouncycastle.cert.jcajce.JcaX509CertificateConverter;
+import org.bouncycastle.cert.jcajce.JcaX509v3CertificateBuilder;
+import org.bouncycastle.operator.ContentSigner;
+import org.bouncycastle.operator.jcajce.JcaContentSignerBuilder;
+import org.eclipse.jetty.util.ssl.SslContextFactory;
+import org.joda.time.DateTime;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.math.BigInteger;
+import java.security.KeyPair;
+import java.security.KeyPairGenerator;
+import java.security.KeyStore;
+import java.security.SecureRandom;
+import java.security.cert.X509Certificate;
+import java.util.Date;
+import java.util.List;
+import java.util.function.Consumer;
+import java.util.function.Function;
+
+public class SslContextFactoryConfigurator {
Review comment:
Please add JavaDoc.
----------------------------------------------------------------
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
With regards,
Apache Git Services