[GitHub] zookeeper pull request #648: ZOOKEEPER-3156: Add in option to canonicalize h...

2018-11-06 Thread revans2 
Github user revans2 closed the pull request at:

https://github.com/apache/zookeeper/pull/648


---

[GitHub] zookeeper pull request #648: ZOOKEEPER-3156: Add in option to canonicalize h...

2018-10-01 Thread anmolnar 
Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/648#discussion_r221612755
  
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -997,12 +999,31 @@ private void startConnect(InetSocketAddress addr) 
throws IOException {
 setName(getName().replaceAll("\\(.*\\)",
 "(" + addr.getHostName() + ":" + addr.getPort() + 
")"));
 if (ZooKeeperSaslClient.isEnabled()) {
+String hostName = addr.getHostName();
+
+boolean canonicalize = true;
+try {
+canonicalize = 
Boolean.parseBoolean(System.getProperty(ZK_SASL_CLIENT_CANONICALIZE_HOSTNAME, 
"true"));
+} catch (IllegalArgumentException ea) {
+//ignored ...
+}
+
+if (canonicalize) {
+InetAddress ia = addr.getAddress();
+if (ia == null) {
+throw new IllegalArgumentException("Connection 
address should have already been resolved by the HostProvider.");
--- End diff --

Would you please change this to something like 'Unable to canonicalize 
address {}, because it's not resolveable'?


---

[GitHub] zookeeper pull request #648: ZOOKEEPER-3156: Add in option to canonicalize h...

2018-10-01 Thread anmolnar 
Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/648#discussion_r221613169
  
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -997,12 +999,31 @@ private void startConnect(InetSocketAddress addr) 
throws IOException {
 setName(getName().replaceAll("\\(.*\\)",
 "(" + addr.getHostName() + ":" + addr.getPort() + 
")"));
 if (ZooKeeperSaslClient.isEnabled()) {
+String hostName = addr.getHostName();
+
+boolean canonicalize = true;
+try {
+canonicalize = 
Boolean.parseBoolean(System.getProperty(ZK_SASL_CLIENT_CANONICALIZE_HOSTNAME, 
"true"));
+} catch (IllegalArgumentException ea) {
+//ignored ...
+}
+
+if (canonicalize) {
+InetAddress ia = addr.getAddress();
+if (ia == null) {
+throw new IllegalArgumentException("Connection 
address should have already been resolved by the HostProvider.");
+}
+//Update the actual address so we are
+hostName = ia.getCanonicalHostName();
--- End diff --

You might want to do the following:
```java
String canonicalHostName = ia.getCanonicalHostName();
if (!canonicalHostName.equals(ia.getHostAddress())) {
hostName = canonicalHostName;
}
```

In order to avoid using literal IP address when security check fails.


---

[GitHub] zookeeper pull request #648: ZOOKEEPER-3156: Add in option to canonicalize h...

2018-10-01 Thread anmolnar 
Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/648#discussion_r221612869
  
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -997,12 +999,31 @@ private void startConnect(InetSocketAddress addr) 
throws IOException {
 setName(getName().replaceAll("\\(.*\\)",
 "(" + addr.getHostName() + ":" + addr.getPort() + 
")"));
 if (ZooKeeperSaslClient.isEnabled()) {
+String hostName = addr.getHostName();
+
+boolean canonicalize = true;
+try {
+canonicalize = 
Boolean.parseBoolean(System.getProperty(ZK_SASL_CLIENT_CANONICALIZE_HOSTNAME, 
"true"));
+} catch (IllegalArgumentException ea) {
+//ignored ...
+}
+
+if (canonicalize) {
+InetAddress ia = addr.getAddress();
+if (ia == null) {
+throw new IllegalArgumentException("Connection 
address should have already been resolved by the HostProvider.");
+}
+//Update the actual address so we are
+hostName = ia.getCanonicalHostName();
+LOG.debug("Canonicalized address to {}", hostName);
--- End diff --

+1


---

[GitHub] zookeeper pull request #648: ZOOKEEPER-3156: Add in option to canonicalize h...

2018-09-28 Thread eolivelli 
Github user eolivelli commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/648#discussion_r221389383
  
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -997,12 +999,31 @@ private void startConnect(InetSocketAddress addr) 
throws IOException {
 setName(getName().replaceAll("\\(.*\\)",
 "(" + addr.getHostName() + ":" + addr.getPort() + 
")"));
 if (ZooKeeperSaslClient.isEnabled()) {
+String hostName = addr.getHostName();
+
+boolean canonicalize = true;
+try {
+canonicalize = 
Boolean.parseBoolean(System.getProperty(ZK_SASL_CLIENT_CANONICALIZE_HOSTNAME, 
"true"));
+} catch (IllegalArgumentException ea) {
+//ignored ...
+}
+
+if (canonicalize) {
+InetAddress ia = addr.getAddress();
+if (ia == null) {
+throw new IllegalArgumentException("Connection 
address should have already been resolved by the HostProvider.");
+}
+//Update the actual address so we are
+hostName = ia.getCanonicalHostName();
+LOG.debug("Canonicalized address to {}", hostName);
--- End diff --

Nit: if LOG.isDebugEnabled


---

[GitHub] zookeeper pull request #648: ZOOKEEPER-3156: Add in option to canonicalize h...

2018-09-28 Thread revans2 
Github user revans2 commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/648#discussion_r221377101
  
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -990,6 +992,27 @@ private void sendPing() {
 private boolean saslLoginFailed = false;
 
 private void startConnect(InetSocketAddress addr) throws 
IOException {
+boolean canonicalize = true;
+try {
+canonicalize = 
Boolean.parseBoolean(System.getProperty(ZK_SASL_CLIENT_CANONICALIZE_HOSTNAME, 
"true"));
+} catch (IllegalArgumentException ea) {
+//ignored ...
+}
+
+if (canonicalize) {
+try {
+InetAddress ia = addr.getAddress();
+LOG.warn("ia {}", ia);
+if (ia == null) {
+ia = InetAddress.getByName(addr.getHostName());
+}
+String host = (ia != null) ? ia.getCanonicalHostName() 
: addr.getHostName();
+addr = new 
InetSocketAddress(InetAddress.getByAddress(host, ia.getAddress()), 
addr.getPort());
--- End diff --

But you said that we already have the IP address, which is what it is going 
to use, so never mind, it should be fine.


---

[GitHub] zookeeper pull request #648: ZOOKEEPER-3156: Add in option to canonicalize h...

2018-09-28 Thread revans2 
Github user revans2 commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/648#discussion_r221376870
  
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -990,6 +992,27 @@ private void sendPing() {
 private boolean saslLoginFailed = false;
 
 private void startConnect(InetSocketAddress addr) throws 
IOException {
+boolean canonicalize = true;
+try {
+canonicalize = 
Boolean.parseBoolean(System.getProperty(ZK_SASL_CLIENT_CANONICALIZE_HOSTNAME, 
"true"));
+} catch (IllegalArgumentException ea) {
+//ignored ...
+}
+
+if (canonicalize) {
+try {
+InetAddress ia = addr.getAddress();
+LOG.warn("ia {}", ia);
+if (ia == null) {
+ia = InetAddress.getByName(addr.getHostName());
+}
+String host = (ia != null) ? ia.getCanonicalHostName() 
: addr.getHostName();
+addr = new 
InetSocketAddress(InetAddress.getByAddress(host, ia.getAddress()), 
addr.getPort());
--- End diff --

I put it here because there is a possible race.  The entire reason we have 
the setup we do is so that we can change the nodes in the cluster without 
changing the config on the client side.  If the code that establishes the 
connection uses a different address from the code that creates the principal 
there is the possibility, because of the magic of DNS caching, that the 
connection would be made to a different box from the one the SASL client is 
expecting.  If that happens, and we have the default Client section in the jaas 
conf, it will not result in an error.  Instead the ZK client logs something 
about SASL failed and still connects but is not authorized to do anything.  If 
it failed and retried a different node I would be happy to move it, but it does 
not and that failure would not be transparent to the end user.


---

[GitHub] zookeeper pull request #648: ZOOKEEPER-3156: Add in option to canonicalize h...

2018-09-28 Thread anmolnar 
Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/648#discussion_r221367697
  
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -990,6 +992,27 @@ private void sendPing() {
 private boolean saslLoginFailed = false;
 
 private void startConnect(InetSocketAddress addr) throws 
IOException {
+boolean canonicalize = true;
+try {
+canonicalize = 
Boolean.parseBoolean(System.getProperty(ZK_SASL_CLIENT_CANONICALIZE_HOSTNAME, 
"true"));
+} catch (IllegalArgumentException ea) {
+//ignored ...
+}
+
+if (canonicalize) {
--- End diff --

You only want to do this if SASL client is enabled. Hence the name of the 
switch contains `sasl.client`. 


---

[GitHub] zookeeper pull request #648: ZOOKEEPER-3156: Add in option to canonicalize h...

2018-09-28 Thread anmolnar 
Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/648#discussion_r221367723
  
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -990,6 +992,27 @@ private void sendPing() {
 private boolean saslLoginFailed = false;
 
 private void startConnect(InetSocketAddress addr) throws 
IOException {
+boolean canonicalize = true;
+try {
+canonicalize = 
Boolean.parseBoolean(System.getProperty(ZK_SASL_CLIENT_CANONICALIZE_HOSTNAME, 
"true"));
+} catch (IllegalArgumentException ea) {
+//ignored ...
+}
+
+if (canonicalize) {
+try {
+InetAddress ia = addr.getAddress();
+LOG.warn("ia {}", ia);
--- End diff --

Debugging?


---

[GitHub] zookeeper pull request #648: ZOOKEEPER-3156: Add in option to canonicalize h...

2018-09-28 Thread anmolnar 
Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/648#discussion_r221368841
  
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -990,6 +992,27 @@ private void sendPing() {
 private boolean saslLoginFailed = false;
 
 private void startConnect(InetSocketAddress addr) throws 
IOException {
+boolean canonicalize = true;
+try {
+canonicalize = 
Boolean.parseBoolean(System.getProperty(ZK_SASL_CLIENT_CANONICALIZE_HOSTNAME, 
"true"));
+} catch (IllegalArgumentException ea) {
+//ignored ...
+}
+
+if (canonicalize) {
+try {
+InetAddress ia = addr.getAddress();
+LOG.warn("ia {}", ia);
+if (ia == null) {
+ia = InetAddress.getByName(addr.getHostName());
+}
+String host = (ia != null) ? ia.getCanonicalHostName() 
: addr.getHostName();
+addr = new 
InetSocketAddress(InetAddress.getByAddress(host, ia.getAddress()), 
addr.getPort());
--- End diff --

I'm thinking of how much value does it have to replace the address with the 
canonicalized version for the entire client. We might want to implement this 
strictly and only for `ZooKeeperSaslClient`.

I'd rather move this logic to the creation of `ZooKeeperSaslClient`. 

Something like:
```java
zooKeeperSaslClient =  new ZooKeeperSaslClient(principalUserName + "/" + 
canonicalize ? addr.getAddress().getCanonicalHostName() : addr.getHostName());
```


---

[GitHub] zookeeper pull request #648: ZOOKEEPER-3156: Add in option to canonicalize h...

2018-09-28 Thread anmolnar 
Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/648#discussion_r221367991
  
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -990,6 +992,27 @@ private void sendPing() {
 private boolean saslLoginFailed = false;
 
 private void startConnect(InetSocketAddress addr) throws 
IOException {
+boolean canonicalize = true;
+try {
+canonicalize = 
Boolean.parseBoolean(System.getProperty(ZK_SASL_CLIENT_CANONICALIZE_HOSTNAME, 
"true"));
+} catch (IllegalArgumentException ea) {
+//ignored ...
+}
+
+if (canonicalize) {
+try {
+InetAddress ia = addr.getAddress();
+LOG.warn("ia {}", ia);
+if (ia == null) {
+ia = InetAddress.getByName(addr.getHostName());
--- End diff --

If the original address is unresolved, you should throw exception 
immediately. That means HostProvider already tried to resolve the address, but 
failed. No point trying it again here.


---

[GitHub] zookeeper pull request #648: ZOOKEEPER-3156: Add in option to canonicalize h...

2018-09-28 Thread revans2 
GitHub user revans2 opened a pull request:

https://github.com/apache/zookeeper/pull/648

ZOOKEEPER-3156: Add in option to canonicalize host name



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/revans2/zookeeper ZOOKEEPER-3156-3.4

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/648.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #648


commit 7ecd6c9151c0e7be749cfead5f141d673a8663f6
Author: Robert Evans 
Date:   2018-09-26T20:40:26Z

ZOOKEEPER-3156: Add in option to canonicalize host name




---