Preserve REST client auth despite 401 response (#30558)

The default behaviour for Apache HTTP client is to mimic the standard
browser behaviour of clearing the authentication cache (for a given
host) if that host responds with 401.
This behaviour is appropriate in a interactive browser environment
where the user is given the opportunity to provide alternative
credentials, but it is not the preferred behaviour for the ES REST
client.

X-Pack may respond with a 401 status if a request is made before the
node/cluster has recovered sufficient state to know how to handle the
provided authentication credentials - for example the security index
need to be recovered before we can authenticate native users.
In these cases the correct behaviour is to retry with the same
credentials (rather than discarding those credentials).

Author: tvernum

client/rest/src/main/java/org/elasticsearch/client/PersistentCredentialsAuthenticationStrategy.java

@@ -0,0 +1,59 @@
+/*
+ * Licensed to Elasticsearch under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch 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.elasticsearch.client;
+
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.http.HttpHost;
+import org.apache.http.auth.AuthScheme;
+import org.apache.http.impl.client.TargetAuthenticationStrategy;
+import org.apache.http.protocol.HttpContext;
+
+/**
+ * An {@link org.apache.http.client.AuthenticationStrategy} implementation that does <em>not</em> perform
+ * any special handling if authentication fails.
+ * The default handler in Apache HTTP client mimics standard browser behaviour of clearing authentication
+ * credentials if it receives a 401 response from the server. While this can be useful for browser, it is
+ * rarely the desired behaviour with the Elasticsearch REST API.
+ * If the code using the REST client has configured credentials for the REST API, then we can and should
+ * assume that this is intentional, and those credentials represent the best possible authentication
+ * mechanism to the Elasticsearch node.
+ * If we receive a 401 status, a probably cause is that the authentication mechanism in place was unable
+ * to perform the requisite password checks (the node has not yet recovered its state, or an external
+ * authentication provider was unavailable).
+ * If this occurs, then the desired behaviour is for the Rest client to retry with the same credentials
+ * (rather than trying with no credentials, or expecting the calling code to provide alternate credentials).
+ */
+final class PersistentCredentialsAuthenticationStrategy extends TargetAuthenticationStrategy {
+
+    private final Log logger = LogFactory.getLog(PersistentCredentialsAuthenticationStrategy.class);
+
+    @Override
+    public void authFailed(HttpHost host, AuthScheme authScheme, HttpContext context) {
+        if (logger.isDebugEnabled()) {
+            logger.debug("Authentication to " + host + " failed (scheme: " + authScheme.getSchemeName()
+                + "). Preserving credentials for next request");
+        }
+        // Do nothing.
+        // The superclass implementation of method will clear the credentials from the cache, but we don't
+    }
+}

client/rest/src/main/java/org/elasticsearch/client/RestClientBuilder.java

@@ -204,7 +204,8 @@ private CloseableHttpAsyncClient createHttpClient() {
             HttpAsyncClientBuilder httpClientBuilder = HttpAsyncClientBuilder.create().setDefaultRequestConfig(requestConfigBuilder.build())
                 //default settings for connection pooling may be too constraining
                 .setMaxConnPerRoute(DEFAULT_MAX_CONN_PER_ROUTE).setMaxConnTotal(DEFAULT_MAX_CONN_TOTAL)
-                .setSSLContext(SSLContext.getDefault());
+                .setSSLContext(SSLContext.getDefault())
+                .setTargetAuthenticationStrategy(new PersistentCredentialsAuthenticationStrategy());
             if (httpClientConfigCallback != null) {
                 httpClientBuilder = httpClientConfigCallback.customizeHttpClient(httpClientBuilder);
             }

client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostIntegTests.java

@@ -31,14 +31,14 @@
 import org.apache.http.entity.ContentType;
 import org.apache.http.entity.StringEntity;
 import org.apache.http.impl.client.BasicCredentialsProvider;
+import org.apache.http.impl.client.TargetAuthenticationStrategy;
 import org.apache.http.impl.nio.client.HttpAsyncClientBuilder;
+import org.apache.http.message.BasicHeader;
 import org.apache.http.nio.entity.NStringEntity;
 import org.apache.http.util.EntityUtils;
 import org.elasticsearch.mocksocket.MockHttpServer;
 import org.junit.After;
-import org.junit.AfterClass;
 import org.junit.Before;
-import org.junit.BeforeClass;
 
 import java.io.IOException;
 import java.io.InputStreamReader;
@@ -147,6 +147,8 @@ public HttpAsyncClientBuilder customizeHttpClient(final HttpAsyncClientBuilder h
                     if (usePreemptiveAuth == false) {
                         // disable preemptive auth by ignoring any authcache
                         httpClientBuilder.disableAuthCaching();
+                        // don't use the "persistent credentials strategy"
+                        httpClientBuilder.setTargetAuthenticationStrategy(new TargetAuthenticationStrategy());
                     }
 
                     return httpClientBuilder.setDefaultCredentialsProvider(credentialsProvider);
@@ -193,7 +195,7 @@ public void onFailure(Exception exception) {
         assertTrue("timeout waiting for requests to be sent", latch.await(10, TimeUnit.SECONDS));
         if (exceptions.isEmpty() == false) {
             AssertionError error = new AssertionError("expected no failures but got some. see suppressed for first 10 of ["
-                                        + exceptions.size() + "] failures");
+                + exceptions.size() + "] failures");
             for (Exception exception : exceptions.subList(0, Math.min(10, exceptions.size()))) {
                 error.addSuppressed(exception);
             }
@@ -217,7 +219,7 @@ public void testHeaders() throws IOException {
             Response esResponse;
             try {
                 esResponse = restClient.performRequest(method, "/" + statusCode, Collections.<String, String>emptyMap(), requestHeaders);
-            } catch(ResponseException e) {
+            } catch (ResponseException e) {
                 esResponse = e.getResponse();
             }
 
@@ -291,8 +293,8 @@ public void testEncodeParams() throws IOException {
     /**
      * Verify that credentials are sent on the first request with preemptive auth enabled (default when provided with credentials).
      */
-    public void testPreemptiveAuthEnabled() throws IOException  {
-        final String[] methods = { "POST", "PUT", "GET", "DELETE" };
+    public void testPreemptiveAuthEnabled() throws IOException {
+        final String[] methods = {"POST", "PUT", "GET", "DELETE"};
 
         try (RestClient restClient = createRestClient(true, true)) {
             for (final String method : methods) {
@@ -306,8 +308,8 @@ public void testPreemptiveAuthEnabled() throws IOException  {
     /**
      * Verify that credentials are <em>not</em> sent on the first request with preemptive auth disabled.
      */
-    public void testPreemptiveAuthDisabled() throws IOException  {
-        final String[] methods = { "POST", "PUT", "GET", "DELETE" };
+    public void testPreemptiveAuthDisabled() throws IOException {
+        final String[] methods = {"POST", "PUT", "GET", "DELETE"};
 
         try (RestClient restClient = createRestClient(true, false)) {
             for (final String method : methods) {
@@ -318,12 +320,31 @@ public void testPreemptiveAuthDisabled() throws IOException  {
         }
     }
 
+    /**
+     * Verify that credentials continue to be sent even if a 401 (Unauthorized) response is received
+     */
+    public void testAuthCredentialsAreNotClearedOnAuthChallenge() throws IOException {
+        final String[] methods = {"POST", "PUT", "GET", "DELETE"};
+
+        try (RestClient restClient = createRestClient(true, true)) {
+            for (final String method : methods) {
+                Header realmHeader = new BasicHeader("WWW-Authenticate", "Basic realm=\"test\"");
+                final Response response401 = bodyTest(restClient, method, 401, new Header[]{realmHeader});
+                assertThat(response401.getHeader("Authorization"), startsWith("Basic"));
+
+                final Response response200 = bodyTest(restClient, method, 200, new Header[0]);
+                assertThat(response200.getHeader("Authorization"), startsWith("Basic"));
+            }
+        }
+
+    }
+
     public void testUrlWithoutLeadingSlash() throws Exception {
         if (pathPrefix.length() == 0) {
             try {
                 restClient.performRequest("GET", "200");
                 fail("request should have failed");
-            } catch(ResponseException e) {
+            } catch (ResponseException e) {
                 assertEquals(404, e.getResponse().getStatusLine().getStatusCode());
             }
         } else {
@@ -335,8 +356,8 @@ public void testUrlWithoutLeadingSlash() throws Exception {
             {
                 //pathPrefix is not required to start with '/', will be added automatically
                 try (RestClient restClient = RestClient.builder(
-                        new HttpHost(httpServer.getAddress().getHostString(), httpServer.getAddress().getPort()))
-                        .setPathPrefix(pathPrefix.substring(1)).build()) {
+                    new HttpHost(httpServer.getAddress().getHostString(), httpServer.getAddress().getPort()))
+                    .setPathPrefix(pathPrefix.substring(1)).build()) {
                     Response response = restClient.performRequest("GET", "200");
                     //a trailing slash gets automatically added if a pathPrefix is configured
                     assertEquals(200, response.getStatusLine().getStatusCode());
@@ -350,10 +371,15 @@ private Response bodyTest(final String method) throws IOException {
     }
 
     private Response bodyTest(final RestClient restClient, final String method) throws IOException {
-        String requestBody = "{ \"field\": \"value\" }";
         int statusCode = randomStatusCode(getRandom());
+        return bodyTest(restClient, method, statusCode, new Header[0]);
+    }
+
+    private Response bodyTest(RestClient restClient, String method, int statusCode, Header[] headers) throws IOException {
+        String requestBody = "{ \"field\": \"value\" }";
         Request request = new Request(method, "/" + statusCode);
         request.setJsonEntity(requestBody);
+        request.setHeaders(headers);
         Response esResponse;
         try {
             esResponse = restClient.performRequest(request);