Repository: searchbox-io/Jest
Pull request #592 (2018-05-24 07:33:55)

Scrub server URIs before logging them in AbstractJestClient
As a security best practice, access credentials such as the user info in the URI of Elasticsearch node URIs should never be logged in plaintext.

This PR makes sure that credentials won't be logged (on either DEBUG or INFO) on startup.


Patch:
diff --git a/jest-common/src/main/java/io/searchbox/client/AbstractJestClient.java b/jest-common/src/main/java/io/searchbox/client/AbstractJestClient.java
index cc9d8463..164159aa 100644
--- a/jest-common/src/main/java/io/searchbox/client/AbstractJestClient.java
+++ b/jest-common/src/main/java/io/searchbox/client/AbstractJestClient.java
@@ -1,6 +1,7 @@
 package io.searchbox.client;
 
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
@@ -13,6 +14,8 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
+import java.net.URI;
+import java.net.URISyntaxException;
 import java.util.List;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicInteger;
@@ -49,13 +52,13 @@ public abstract class AbstractJestClient implements JestClient {
         if (servers.equals(serverPoolReference.get().getServers())) {
             if (log.isDebugEnabled()) {
                 log.debug("Server pool already contains same list of servers: {}",
-                        Joiner.on(',').join(servers));
+                        Joiner.on(',').join(scrubServerURIs(servers)));
             }
             return;
         }
         if (log.isInfoEnabled()) {
             log.info("Setting server pool to a list of {} servers: [{}]",
-                      servers.size(), Joiner.on(',').join(servers));
+                      servers.size(), Joiner.on(',').join(scrubServerURIs(servers)));
         }
         serverPoolReference.set(new ServerPool(servers));
 
@@ -64,6 +67,27 @@ public abstract class AbstractJestClient implements JestClient {
         }
     }
 
+    @VisibleForTesting
+    Set<String> scrubServerURIs(Set<String> servers) {
+        final ImmutableSet.Builder<String> scrubbedServers = ImmutableSet.builder();
+        for (String server : servers) {
+            final URI originalURI = URI.create(server);
+            try {
+                final URI scrubbedURI = new URI(originalURI.getScheme(),
+                        null, // Remove user info
+                        originalURI.getHost(),
+                        originalURI.getPort(),
+                        originalURI.getPath(),
+                        originalURI.getQuery(),
+                        originalURI.getFragment());
+                scrubbedServers.add(scrubbedURI.toString());
+            } catch (URISyntaxException e) {
+                log.debug("Couldn't scrub server URI " + originalURI, e);
+            }
+        }
+        return scrubbedServers.build();
+    }
+
     /**
      * {@inheritDoc}
      */


Test patch:
diff --git a/jest-common/src/test/java/io/searchbox/client/AbstractJestClientTest.java b/jest-common/src/test/java/io/searchbox/client/AbstractJestClientTest.java
index eb36c2d0..82188ba5 100644
--- a/jest-common/src/test/java/io/searchbox/client/AbstractJestClientTest.java
+++ b/jest-common/src/test/java/io/searchbox/client/AbstractJestClientTest.java
@@ -1,5 +1,6 @@
 package io.searchbox.client;
 
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Maps;
 import io.searchbox.action.Action;
 import org.junit.Test;
@@ -63,6 +64,39 @@ public class AbstractJestClientTest {
         assertTrue(set.contains("http://localhost:9400"));
     }
 
+    @Test
+    public void testGetElasticSearchServerWithCredentials() {
+        final Set<String> set = ImmutableSet.of(
+                "http://user:pass@localhost:9200",
+                "http://user:pass@localhost:9300",
+                "http://user:pass@localhost:9400");
+        client.setServers(set);
+
+        Set<String> serverList = new HashSet<>();
+
+        for (int i = 0; i < set.size(); i++) {
+            serverList.add(client.getNextServer());
+        }
+
+        assertEquals("round robin does not work", 3, serverList.size());
+
+        assertTrue(set.contains("http://user:pass@localhost:9200"));
+        assertTrue(set.contains("http://user:pass@localhost:9300"));
+        assertTrue(set.contains("http://user:pass@localhost:9400"));
+    }
+
+    @Test
+    public void testScrubServerURIs() {
+        final Set<String> set = ImmutableSet.of(
+                "http://user:pass@localhost:9200",
+                "http://localhost:9300/path?query#fragment");
+        final Set<String> scrubbedURIs = client.scrubServerURIs(set);
+
+        assertEquals(2, scrubbedURIs.size());
+        assertTrue(scrubbedURIs.contains("http://localhost:9200"));
+        assertTrue(scrubbedURIs.contains("http://localhost:9300/path?query#fragment"));
+    }
+
     @Test
     public void testGetElasticSearchServerIsThreadsafe() throws Exception {
         final int NUM_THREADS = 12;