From 3a6f73b35fd0b91662897b2197190b3639e312a3 Mon Sep 17 00:00:00 2001 From: Tim Vernum Date: Thu, 17 May 2018 18:27:18 +1000 Subject: [PATCH] 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). --- ...tentCredentialsAuthenticationStrategy.java | 59 +++++++++++++++++++ .../client/RestClientBuilder.java | 3 +- .../RestClientSingleHostIntegTests.java | 50 ++++++++++++---- 3 files changed, 99 insertions(+), 13 deletions(-) create mode 100644 client/rest/src/main/java/org/elasticsearch/client/PersistentCredentialsAuthenticationStrategy.java diff --git a/client/rest/src/main/java/org/elasticsearch/client/PersistentCredentialsAuthenticationStrategy.java b/client/rest/src/main/java/org/elasticsearch/client/PersistentCredentialsAuthenticationStrategy.java new file mode 100644 index 0000000000000..4ae22fbe3728e --- /dev/null +++ b/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 not 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 + } +} diff --git a/client/rest/src/main/java/org/elasticsearch/client/RestClientBuilder.java b/client/rest/src/main/java/org/elasticsearch/client/RestClientBuilder.java index 8768c07161989..5f7831c67fc28 100644 --- a/client/rest/src/main/java/org/elasticsearch/client/RestClientBuilder.java +++ b/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); } diff --git a/client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostIntegTests.java b/client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostIntegTests.java index 667e38a5167d7..35cac627bbe6a 100644 --- a/client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostIntegTests.java +++ b/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.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 not 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);