Add hostname-verification at client tls connection #1208
Conversation
rdhabalia
added
the
type/enhancement
rdhabalia
changed the title
rdhabalia
force-pushed
the
host_verify
branch
from
8d3d975 to
2a09a7c
Compare
| @@ -0,0 +1,82 @@ | |||
| Certificate: | |||
There was a problem hiding this comment.
Make sure these certificates are ignored in the apache-rat:check
There was a problem hiding this comment.
I checked the rat-check plugin in pom which ignores all .cert/.key file but doesn't exclude *.pem so, I will add it too.
| * | ||
| * @param tlsHostnameVerificationEnable | ||
| */ | ||
| public void setTlsHostnameVerificationEnable(boolean tlsHostnameVerificationEnable) { |
There was a problem hiding this comment.
The allowInsecureConnection=false, which is the default, already should imply the hostname check and that is what the HTTP client is following anyway. I don't think we need a new setting.
There was a problem hiding this 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
So, as both the configs serve different purpose then shouldn't it better to give flexibility while configuring it?
There was a problem hiding this comment.
Sure, we should just make sure the same check are done in http vs protobuf for the same config
There was a problem hiding this comment.
sure, I will check if asyncHttpClient-lib we are using having this feature and if it does then I will create a separate PR.
| import org.slf4j.LoggerFactory; | ||
|
|
||
| /** | ||
| * Default {@link javax.net.ssl.HostnameVerifier} implementation. Copied from httpclient. |
There was a problem hiding this comment.
Would it be possible to depend on these classes rather than import?
If we are importing we should add a link below the ASF header stating the code was imported/copied/derived from project X at http://xyz////
There was a problem hiding this comment.
i have included that as a dep by excluding all transitive dep from it.
| @@ -74,6 +74,17 @@ | |||
| <groupId>com.google.code.gson</groupId> | |||
| <artifactId>gson</artifactId> | |||
| </dependency> | |||
|
|
|||
| <dependency> | |||
| <groupId>org.apache.httpcomponents</groupId> | |||
There was a problem hiding this comment.
This should be added to the client shading as well. Also in all the other shading config they need to reuse the same list of the client shading conf.
There was a problem hiding this comment.
oops.. I missed it. I will add it.
| @@ -138,6 +138,18 @@ flexible messaging model and an intuitive client API.</description> | |||
| </exclusions> | |||
| </dependency> | |||
|
|
|||
| <dependency> | |||
There was a problem hiding this comment.
This dependency should be added at the end of all/src/assemble/LICENSE.bin.txt
There was a problem hiding this 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.
| @@ -81,6 +81,8 @@ | |||
| <include>org.apache.pulsar:pulsar-checksum</include> | |||
| <include>net.jpountz.lz4:lz4</include> | |||
| <include>com.yahoo.datasketches:sketches-core</include> | |||
| <include>org.apache.httpcomponents:httpclient</include> | |||
| <include>commons-logging:commons-logging</include> | |||
There was a problem hiding this comment.
Can you also add these in pulsar-broker-shaded/pom.xml and pulsar-client-kafka-compat/pulsar-client-kafka/pom.xml. I know it's painful, but couldn't find a reliable way to do the shading in the different modules.
There was a problem hiding this comment.
I see..let me add it.
|
Actually I was looking into the code, and found much of duplicate code. The blocks for Using |
|
Submitted a PR #1213 to address the code duplication. |
Motivation
Right now, Pulsar client is not able to perform hostname-verification when it connects to broker over tls. Hostname-verification feature is already implemented in almost all http-client but it's not supported by netty yet. therefore, client should be able to perform hostname-verification as per RFC-2181
Modifications
Note:
(Copied DefaultHostnameVerifier from httpclient)
Result
Pulsar client will be able to enable hostname verification while communicating with broker or proxy.