Skip to content

Conversation

@rdhabalia
Copy link
Contributor

Motivation

In #1208, we have added support for hostname verification at client when client creates tls connection with broker and proxy.
However, if proxy is also not in local n/w then it would also require to support hostname verification when it connects with broker.

Modifications

add option at proxy which forces proxy to do hostname verification when it connects to broker.

Result

proxy can support hostname verification when it connects to broker.
After your change, what will change.

@rdhabalia rdhabalia added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages type/feature The PR added a new feature or issue requested a new feature labels Feb 10, 2018
@rdhabalia rdhabalia added this to the 1.22.0-incubating milestone Feb 10, 2018
@rdhabalia rdhabalia self-assigned this Feb 10, 2018
@rdhabalia rdhabalia requested review from jai1 and merlimat February 10, 2018 07:05
<dependency>
<groupId>commons-logging</groupId>
<artifactId>commons-logging</artifactId>
<version>1.1.1</version>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't need version here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's true, it's already part of pulsar-client-original so, need to include here.

conf/proxy.conf Outdated
tlsKeyFilePath=

# Validates hostname when proxy creates tls connection with broker
isTlsHostnameVerificationEnable=false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-> isTlsHostnameVerificationEnabled ?
or
enableTlsHostnameVerification

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as all the tls related config has tls prefix so, I have renamed it to tlsHostnameVerificationEnabled

Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@merlimat
Copy link
Contributor

@rdhabalia Can you also create an issue for adding hostname verification in C++?

@rdhabalia
Copy link
Contributor Author

sure, created : #1215

@rdhabalia rdhabalia merged commit a27a1e2 into apache:master Feb 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages type/feature The PR added a new feature or issue requested a new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants