[GitHub] rdhabalia commented on a change in pull request #1208: Add hostname-verification at client tls connection
rdhabalia commented on a change in pull request #1208: Add hostname-verification at client tls connection URL: https://github.com/apache/incubator-pulsar/pull/1208#discussion_r167332606 ## File path: pulsar-client-shaded/pom.xml ## @@ -81,6 +81,8 @@ org.apache.pulsar:pulsar-checksum net.jpountz.lz4:lz4 com.yahoo.datasketches:sketches-core + org.apache.httpcomponents:httpclient + commons-logging:commons-logging Review comment: I see..let me add it. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rdhabalia commented on a change in pull request #1208: Add hostname-verification at client tls connection
rdhabalia commented on a change in pull request #1208: Add hostname-verification at client tls connection URL: https://github.com/apache/incubator-pulsar/pull/1208#discussion_r167318158 ## File path: pom.xml ## @@ -138,6 +138,18 @@ flexible messaging model and an intuitive client API. + Review comment: I have added `org.apache.httpcomponents:httpclient` in client-shadeed pom and License file. However, I excluded all other jars from httpclient to avoid bringing lot of other things from it, but it requires `commons-logging:commons-logging` for internal logging without it we see ClassNotFoundException for logger class. So, I have added that dep explicitly and added to LICENSE and cliend-sahde as well. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rdhabalia commented on a change in pull request #1208: Add hostname-verification at client tls connection
rdhabalia commented on a change in pull request #1208: Add
hostname-verification at client tls connection
URL: https://github.com/apache/incubator-pulsar/pull/1208#discussion_r167165234
##
File path:
pulsar-common/src/main/java/org/apache/pulsar/common/util/ssl/DefaultHostnameVerifier.java
##
@@ -0,0 +1,297 @@
+/**
+ * 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.pulsar.common.util.ssl;
+
+import java.net.InetAddress;
+import java.net.UnknownHostException;
+import java.security.cert.Certificate;
+import java.security.cert.CertificateParsingException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.Locale;
+import java.util.NoSuchElementException;
+
+import javax.naming.InvalidNameException;
+import javax.naming.NamingException;
+import javax.naming.directory.Attribute;
+import javax.naming.directory.Attributes;
+import javax.naming.ldap.LdapName;
+import javax.naming.ldap.Rdn;
+import javax.net.ssl.HostnameVerifier;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.SSLSession;
+import javax.security.auth.x500.X500Principal;
+
+import org.apache.pulsar.common.util.ssl.PublicSuffixMatcher.DomainType;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Default {@link javax.net.ssl.HostnameVerifier} implementation. Copied from
httpclient.
Review comment:
i have included that as a dep by excluding all transitive dep from it.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rdhabalia commented on a change in pull request #1208: Add hostname-verification at client tls connection
rdhabalia commented on a change in pull request #1208: Add
hostname-verification at client tls connection
URL: https://github.com/apache/incubator-pulsar/pull/1208#discussion_r167158945
##
File path:
pulsar-client/src/main/java/org/apache/pulsar/client/api/ClientConfiguration.java
##
@@ -356,4 +357,21 @@ public void
setMaxNumberOfRejectedRequestPerConnection(int maxNumberOfRejectedRe
this.maxNumberOfRejectedRequestPerConnection =
maxNumberOfRejectedRequestPerConnection;
}
+public boolean isTlsHostnameVerificationEnable() {
+return tlsHostnameVerificationEnable;
+}
+
+/**
+ * It allows to validate hostname verification when client connects to
broker over tls. It validates incoming x509
+ * certificate and matches provided hostname(CN/SAN) with expected
broker's host name. It follows RFC 2818, 3.1. Server
+ * Identity hostname verification.
+ *
+ * @see https://tools.ietf.org/html/rfc2818";>rfc2818
+ *
+ * @param tlsHostnameVerificationEnable
+ */
+public void setTlsHostnameVerificationEnable(boolean
tlsHostnameVerificationEnable) {
Review comment:
umm.. actually one can use `allowInsecureConnection` in non-prod env which
makes client to trust all X.509 certificates without any verification using
`InsecureTrustManagerFactory.java`. However, hostname verification can be
applied on top of secured connection as well.
> that is what the HTTP client is following anyway.
Actually even HTTP client also provides separate API to [set
hostNameVerifier](http://hc.apache.org/httpcomponents-client-ga/httpclient/apidocs/org/apache/http/impl/client/HttpClientBuilder.html#setHostnameVerifier(org.apache.http.conn.ssl.X509HostnameVerifier))
So, as both the configs serve different purpose then shouldn't it better to
give flexibility while configuring it?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rdhabalia commented on a change in pull request #1208: Add hostname-verification at client tls connection
rdhabalia commented on a change in pull request #1208: Add hostname-verification at client tls connection URL: https://github.com/apache/incubator-pulsar/pull/1208#discussion_r167159624 ## File path: pulsar-broker/src/test/resources/authentication/tls/hn-verification/broker-cert.pem ## @@ -0,0 +1,82 @@ +Certificate: Review comment: I checked the rat-check plugin in [pom](https://github.com/apache/incubator-pulsar/blob/master/pom.xml#L760) which ignores all *.cert/*.key file but doesn't exclude `*.pem` so, I will add it too. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
