From 5aa7e6fb5c7d3c35d49e1bf0951758860449a9d7 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Fri, 17 Jun 2022 12:30:25 +1000 Subject: [PATCH 01/13] Add setting for keep-alive duration for oidc back-channel In some environment, the back-channel connection can be dropped without sending a TCP RST to ES. When that happens, reusing the same connection results into timeout error. This PR adds a new http.connection_pool_ttl setting to control how long a connection in the OIDC back-channel pool can be idle before it is closed. This allows ES to more actively close idle connections to avoid the timeout issue. Resolves: #75515 --- .../reference/settings/security-settings.asciidoc | 15 +++++++++++++++ .../authc/oidc/OpenIdConnectRealmSettings.java | 8 ++++++++ .../qa/smoke-test-all-realms/build.gradle | 1 + .../authc/oidc/OpenIdConnectAuthenticator.java | 13 ++++++++++++- 4 files changed, 36 insertions(+), 1 deletion(-) diff --git a/docs/reference/settings/security-settings.asciidoc b/docs/reference/settings/security-settings.asciidoc index 85ad8894c6c01..3a2ad961e859b 100644 --- a/docs/reference/settings/security-settings.asciidoc +++ b/docs/reference/settings/security-settings.asciidoc @@ -1858,6 +1858,21 @@ connections allowed per endpoint. Defaults to `200`. // end::oidc-http-max-endpoint-connections-tag[] +// tag::oidc-http-connection-pool-ttl-tag[] +`http.connection_pool_ttl` {ess-icon}:: +(<>) +Controls the behavior of the http client used for back-channel communication to +the OpenID Connect Provider endpoints. Specifies the time-to-live of connections +in the connection pool. If a connection is not re-used within this timeout, +it is closed. +By default, the time-to-live is 3 minutes meaning that connections are closed +after being idle for 3 minutes. + +Configure this setting to `-1` to let the server dictates this value using the `Keep-Alive` HTTP +response header. If the header is not set by the server, the time-to-live is infinite meaning +that connections never expire. +// end::oidc-http-connection-pool-ttl-tag[] + [discrete] [[ref-oidc-ssl-settings]] ===== OpenID Connect realm SSL settings diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/oidc/OpenIdConnectRealmSettings.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/oidc/OpenIdConnectRealmSettings.java index a14effba87836..586d47c700cd1 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/oidc/OpenIdConnectRealmSettings.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/oidc/OpenIdConnectRealmSettings.java @@ -23,6 +23,7 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.TimeUnit; import java.util.function.Function; public class OpenIdConnectRealmSettings { @@ -212,6 +213,12 @@ private OpenIdConnectRealmSettings() {} "http.max_endpoint_connections", key -> Setting.intSetting(key, 200, Setting.Property.NodeScope) ); + + public static final Setting.AffixSetting HTTP_CONNECTION_POOL_TTL = Setting.affixKeySetting( + RealmSettings.realmSettingPrefix(TYPE), + "http.connection_pool_ttl", + key -> Setting.timeSetting(key, new TimeValue(3, TimeUnit.MINUTES), Setting.Property.NodeScope) + ); public static final Setting.AffixSetting HTTP_PROXY_HOST = Setting.affixKeySetting( RealmSettings.realmSettingPrefix(TYPE), "http.proxy.host", @@ -307,6 +314,7 @@ public static Set> getSettings() { HTTP_SOCKET_TIMEOUT, HTTP_MAX_CONNECTIONS, HTTP_MAX_ENDPOINT_CONNECTIONS, + HTTP_CONNECTION_POOL_TTL, HTTP_PROXY_HOST, HTTP_PROXY_PORT, HTTP_PROXY_SCHEME, diff --git a/x-pack/plugin/security/qa/smoke-test-all-realms/build.gradle b/x-pack/plugin/security/qa/smoke-test-all-realms/build.gradle index 0a552aa721c39..02e40b7c8cf42 100644 --- a/x-pack/plugin/security/qa/smoke-test-all-realms/build.gradle +++ b/x-pack/plugin/security/qa/smoke-test-all-realms/build.gradle @@ -76,6 +76,7 @@ testClusters.matching { it.name == 'javaRestTest' }.configureEach { setting 'xpack.security.authc.realms.oidc.openid7.op.authorization_endpoint', 'https://op.example.com/auth' setting 'xpack.security.authc.realms.oidc.openid7.op.jwkset_path', 'oidc-jwkset.json' setting 'xpack.security.authc.realms.oidc.openid7.claims.principal', 'sub' + setting 'xpack.security.authc.realms.oidc.openid7.http.connection_pool_ttl', '1m' keystore 'xpack.security.authc.realms.oidc.openid7.rp.client_secret', 'this-is-my-secret' // - JWT (works) setting 'xpack.security.authc.realms.jwt.jwt8.order', '8' diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java index 570e4a91be791..30287fd4c924c 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java @@ -60,6 +60,7 @@ import org.apache.http.config.RegistryBuilder; import org.apache.http.entity.ContentType; import org.apache.http.impl.auth.BasicScheme; +import org.apache.http.impl.client.DefaultConnectionKeepAliveStrategy; import org.apache.http.impl.nio.client.CloseableHttpAsyncClient; import org.apache.http.impl.nio.client.HttpAsyncClientBuilder; import org.apache.http.impl.nio.client.HttpAsyncClients; @@ -81,6 +82,7 @@ import org.elasticsearch.common.util.concurrent.ListenableFuture; import org.elasticsearch.core.CheckedRunnable; import org.elasticsearch.core.Nullable; +import org.elasticsearch.core.TimeValue; import org.elasticsearch.core.Tuple; import org.elasticsearch.rest.RestStatus; import org.elasticsearch.watcher.FileChangesListener; @@ -114,6 +116,7 @@ import static org.elasticsearch.core.Strings.format; import static org.elasticsearch.xpack.core.security.authc.oidc.OpenIdConnectRealmSettings.ALLOWED_CLOCK_SKEW; +import static org.elasticsearch.xpack.core.security.authc.oidc.OpenIdConnectRealmSettings.HTTP_CONNECTION_POOL_TTL; import static org.elasticsearch.xpack.core.security.authc.oidc.OpenIdConnectRealmSettings.HTTP_CONNECTION_READ_TIMEOUT; import static org.elasticsearch.xpack.core.security.authc.oidc.OpenIdConnectRealmSettings.HTTP_CONNECT_TIMEOUT; import static org.elasticsearch.xpack.core.security.authc.oidc.OpenIdConnectRealmSettings.HTTP_MAX_CONNECTIONS; @@ -707,7 +710,15 @@ private CloseableHttpAsyncClient createHttpClient() { .build(); HttpAsyncClientBuilder httpAsyncClientBuilder = HttpAsyncClients.custom() .setConnectionManager(connectionManager) - .setDefaultRequestConfig(requestConfig); + .setDefaultRequestConfig(requestConfig) + .setKeepAliveStrategy((response, context) -> { + final TimeValue timeValue = realmConfig.getSetting(HTTP_CONNECTION_POOL_TTL); + if (TimeValue.MINUS_ONE.equals(timeValue)) { + return DefaultConnectionKeepAliveStrategy.INSTANCE.getKeepAliveDuration(response, context); + } else { + return timeValue.millis(); + } + }); if (realmConfig.hasSetting(HTTP_PROXY_HOST)) { httpAsyncClientBuilder.setProxy( new HttpHost( From a2e3c7c8ee6fdc66b71d58c17fb7faf660d59c29 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Fri, 17 Jun 2022 12:35:25 +1000 Subject: [PATCH 02/13] Update docs/changelog/87773.yaml --- docs/changelog/87773.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 docs/changelog/87773.yaml diff --git a/docs/changelog/87773.yaml b/docs/changelog/87773.yaml new file mode 100644 index 0000000000000..39d9b87d18cbd --- /dev/null +++ b/docs/changelog/87773.yaml @@ -0,0 +1,5 @@ +pr: 87773 +summary: Add setting for keep-alive duration for oidc back-channel +area: Security +type: enhancement +issues: [] From 0b70a7f377cd03224166c0776c465914d39aef09 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Fri, 17 Jun 2022 13:54:01 +1000 Subject: [PATCH 03/13] tweak --- .../security/authc/oidc/OpenIdConnectAuthenticator.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java index 30287fd4c924c..391dfa9c0cae6 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java @@ -708,15 +708,15 @@ private CloseableHttpAsyncClient createHttpClient() { .setConnectionRequestTimeout(Math.toIntExact(realmConfig.getSetting(HTTP_CONNECTION_READ_TIMEOUT).getSeconds())) .setSocketTimeout(Math.toIntExact(realmConfig.getSetting(HTTP_SOCKET_TIMEOUT).getMillis())) .build(); + final TimeValue connectionTtl = realmConfig.getSetting(HTTP_CONNECTION_POOL_TTL); HttpAsyncClientBuilder httpAsyncClientBuilder = HttpAsyncClients.custom() .setConnectionManager(connectionManager) .setDefaultRequestConfig(requestConfig) .setKeepAliveStrategy((response, context) -> { - final TimeValue timeValue = realmConfig.getSetting(HTTP_CONNECTION_POOL_TTL); - if (TimeValue.MINUS_ONE.equals(timeValue)) { + if (TimeValue.MINUS_ONE.equals(connectionTtl)) { return DefaultConnectionKeepAliveStrategy.INSTANCE.getKeepAliveDuration(response, context); } else { - return timeValue.millis(); + return connectionTtl.millis(); } }); if (realmConfig.hasSetting(HTTP_PROXY_HOST)) { From 4920c5352239b4a0c2337df6321b8db4d2088d2a Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Fri, 17 Jun 2022 15:56:30 +1000 Subject: [PATCH 04/13] Update docs/changelog/87773.yaml Co-authored-by: Tim Vernum --- docs/changelog/87773.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/changelog/87773.yaml b/docs/changelog/87773.yaml index 39d9b87d18cbd..65b62e21a374f 100644 --- a/docs/changelog/87773.yaml +++ b/docs/changelog/87773.yaml @@ -1,5 +1,5 @@ pr: 87773 -summary: Add setting for keep-alive duration for oidc back-channel +summary: Automatically close idle connections in OIDC back-channel area: Security type: enhancement issues: [] From 2f597485a090fe260097c4591c6205465907f959 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Fri, 17 Jun 2022 16:01:00 +1000 Subject: [PATCH 05/13] Update x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java Co-authored-by: Tim Vernum --- .../security/authc/oidc/OpenIdConnectAuthenticator.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java index 391dfa9c0cae6..fc067bceea211 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java @@ -713,10 +713,13 @@ private CloseableHttpAsyncClient createHttpClient() { .setConnectionManager(connectionManager) .setDefaultRequestConfig(requestConfig) .setKeepAliveStrategy((response, context) -> { + var serverKeepAlive = DefaultConnectionKeepAliveStrategy.INSTANCE.getKeepAliveDuration(response, context); if (TimeValue.MINUS_ONE.equals(connectionTtl)) { - return DefaultConnectionKeepAliveStrategy.INSTANCE.getKeepAliveDuration(response, context); - } else { + return serverKeepAlive + } else if (serverKeepAlive == -1) { return connectionTtl.millis(); + } else { + return Math.min(serverKeepAlive, connectionTtl.millis()); } }); if (realmConfig.hasSetting(HTTP_PROXY_HOST)) { From 158cec5923b53de13a2353d8d2ee9b9eb560c464 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Fri, 17 Jun 2022 16:08:10 +1000 Subject: [PATCH 06/13] tweak --- .../authc/oidc/OpenIdConnectAuthenticator.java | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java index fc067bceea211..729782627eaff 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java @@ -82,7 +82,6 @@ import org.elasticsearch.common.util.concurrent.ListenableFuture; import org.elasticsearch.core.CheckedRunnable; import org.elasticsearch.core.Nullable; -import org.elasticsearch.core.TimeValue; import org.elasticsearch.core.Tuple; import org.elasticsearch.rest.RestStatus; import org.elasticsearch.watcher.FileChangesListener; @@ -708,19 +707,22 @@ private CloseableHttpAsyncClient createHttpClient() { .setConnectionRequestTimeout(Math.toIntExact(realmConfig.getSetting(HTTP_CONNECTION_READ_TIMEOUT).getSeconds())) .setSocketTimeout(Math.toIntExact(realmConfig.getSetting(HTTP_SOCKET_TIMEOUT).getMillis())) .build(); - final TimeValue connectionTtl = realmConfig.getSetting(HTTP_CONNECTION_POOL_TTL); + final long userConfiguredKeepAlive = realmConfig.getSetting(HTTP_CONNECTION_POOL_TTL).millis(); HttpAsyncClientBuilder httpAsyncClientBuilder = HttpAsyncClients.custom() .setConnectionManager(connectionManager) .setDefaultRequestConfig(requestConfig) .setKeepAliveStrategy((response, context) -> { var serverKeepAlive = DefaultConnectionKeepAliveStrategy.INSTANCE.getKeepAliveDuration(response, context); - if (TimeValue.MINUS_ONE.equals(connectionTtl)) { - return serverKeepAlive - } else if (serverKeepAlive == -1) { - return connectionTtl.millis(); + final long actualKeepAlive; + if (serverKeepAlive == -1) { + actualKeepAlive = userConfiguredKeepAlive; + } else if (userConfiguredKeepAlive == -1) { + actualKeepAlive = serverKeepAlive; } else { - return Math.min(serverKeepAlive, connectionTtl.millis()); + actualKeepAlive = Math.min(serverKeepAlive, userConfiguredKeepAlive); } + LOGGER.debug("effective HTTP connection keep-alive: [{}]ms", actualKeepAlive); + return actualKeepAlive; }); if (realmConfig.hasSetting(HTTP_PROXY_HOST)) { httpAsyncClientBuilder.setProxy( From 09bb6bad6ef953d3f8f49b430f1ab869ad0a39fc Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Mon, 20 Jun 2022 09:10:57 +1000 Subject: [PATCH 07/13] tweak --- .../settings/security-settings.asciidoc | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/docs/reference/settings/security-settings.asciidoc b/docs/reference/settings/security-settings.asciidoc index 3a2ad961e859b..8857f1efe3249 100644 --- a/docs/reference/settings/security-settings.asciidoc +++ b/docs/reference/settings/security-settings.asciidoc @@ -1863,14 +1863,14 @@ Defaults to `200`. (<>) Controls the behavior of the http client used for back-channel communication to the OpenID Connect Provider endpoints. Specifies the time-to-live of connections -in the connection pool. If a connection is not re-used within this timeout, -it is closed. -By default, the time-to-live is 3 minutes meaning that connections are closed -after being idle for 3 minutes. - -Configure this setting to `-1` to let the server dictates this value using the `Keep-Alive` HTTP -response header. If the header is not set by the server, the time-to-live is infinite meaning -that connections never expire. +in the connection pool (default to 3 minutes). A connection is closed if it is +idle for more than the specified timeout. + +The server can also set the `Keep-Alive` HTTP response header. The effective +time-to-live value is the smaller value between this setting and the `Keep-Alive` +reponse header. Configure this setting to `-1` to let the server dictates the value. +If the header is not set by the server and the setting has value of `-1`, +the time-to-live is infinite meaning that connections never expire. // end::oidc-http-connection-pool-ttl-tag[] [discrete] From 059308fc2afad13688b8918e475b2b0460f4a774 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Mon, 20 Jun 2022 13:30:55 +1000 Subject: [PATCH 08/13] Apply suggestions from code review Co-authored-by: Tim Vernum --- docs/reference/settings/security-settings.asciidoc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/reference/settings/security-settings.asciidoc b/docs/reference/settings/security-settings.asciidoc index 8857f1efe3249..64cf1b17110a6 100644 --- a/docs/reference/settings/security-settings.asciidoc +++ b/docs/reference/settings/security-settings.asciidoc @@ -1868,9 +1868,9 @@ idle for more than the specified timeout. The server can also set the `Keep-Alive` HTTP response header. The effective time-to-live value is the smaller value between this setting and the `Keep-Alive` -reponse header. Configure this setting to `-1` to let the server dictates the value. +reponse header. Configure this setting to `-1` to let the server dictate the value. If the header is not set by the server and the setting has value of `-1`, -the time-to-live is infinite meaning that connections never expire. +the time-to-live is infinite and connections never expire. // end::oidc-http-connection-pool-ttl-tag[] [discrete] From 058872f1fd3f78f383f1517ac4917ed1a007330f Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Mon, 20 Jun 2022 16:43:02 +1000 Subject: [PATCH 09/13] add test --- .../oidc/OpenIdConnectAuthenticator.java | 5 + .../oidc/OpenIdConnectAuthenticatorTests.java | 93 +++++++++++++++++++ 2 files changed, 98 insertions(+) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java index 729782627eaff..b4562256b234e 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java @@ -742,6 +742,11 @@ private CloseableHttpAsyncClient createHttpClient() { } } + // Package private for testing + CloseableHttpAsyncClient getHttpClient() { + return httpClient; + } + /* * Creates an {@link IDTokenValidator} based on the current Relying Party configuration */ diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticatorTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticatorTests.java index 9e012d46a9930..5474790a16794 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticatorTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticatorTests.java @@ -44,12 +44,16 @@ import com.nimbusds.openid.connect.sdk.claims.AccessTokenHash; import com.nimbusds.openid.connect.sdk.validators.IDTokenValidator; import com.nimbusds.openid.connect.sdk.validators.InvalidHashException; +import com.sun.net.httpserver.HttpServer; import org.apache.http.HttpResponse; import org.apache.http.HttpVersion; import org.apache.http.ProtocolVersion; +import org.apache.http.client.methods.HttpGet; +import org.apache.http.concurrent.FutureCallback; import org.apache.http.entity.ContentType; import org.apache.http.entity.StringEntity; +import org.apache.http.impl.nio.conn.PoolingNHttpClientConnectionManager; import org.apache.http.message.BasicHttpResponse; import org.apache.http.message.BasicStatusLine; import org.apache.logging.log4j.Level; @@ -59,6 +63,7 @@ import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.support.PlainActionFuture; import org.elasticsearch.common.logging.Loggers; +import org.elasticsearch.common.network.InetAddresses; import org.elasticsearch.common.settings.SecureString; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.util.concurrent.ThreadContext; @@ -66,15 +71,20 @@ import org.elasticsearch.core.Tuple; import org.elasticsearch.env.Environment; import org.elasticsearch.env.TestEnvironment; +import org.elasticsearch.mocksocket.MockHttpServer; import org.elasticsearch.rest.RestStatus; import org.elasticsearch.test.MockLogAppender; import org.elasticsearch.test.TestMatchers; import org.elasticsearch.xpack.core.security.authc.RealmConfig; +import org.elasticsearch.xpack.core.security.authc.oidc.OpenIdConnectRealmSettings; import org.elasticsearch.xpack.core.ssl.SSLService; import org.junit.After; import org.junit.Before; import org.mockito.Mockito; +import java.io.IOException; +import java.net.InetAddress; +import java.net.InetSocketAddress; import java.net.URI; import java.net.URISyntaxException; import java.nio.charset.StandardCharsets; @@ -91,16 +101,20 @@ import java.util.Date; import java.util.Map; import java.util.UUID; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.atomic.AtomicReference; import javax.crypto.SecretKey; import javax.crypto.spec.SecretKeySpec; import static java.time.Instant.now; +import static org.elasticsearch.xpack.core.security.authc.RealmSettings.getFullSettingKey; import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.not; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -1006,6 +1020,85 @@ public void testLogIdTokenAndNonce() throws URISyntaxException, BadJOSEException } } + public void testHttpClientConnectionTTL() throws URISyntaxException, IllegalAccessException, InterruptedException, IOException { + // Create an internal HTTP server, the expectation is: For 2 consecutive HTTP requests, the client port should be different + // because the client should not reuse the same connection after 1s + final HttpServer httpServer = MockHttpServer.createHttp(new InetSocketAddress(InetAddress.getLoopbackAddress(), 0), 0); + httpServer.start(); + + final AtomicReference firstClientPort = new AtomicReference<>(null); + httpServer.createContext("/", exchange -> { + try { + final int currentPort = exchange.getRemoteAddress().getPort(); + // Either set the first port number, otherwise the current (2nd) port number should be different from the 1st one + if (false == firstClientPort.compareAndSet(null, currentPort)) { + assertThat(currentPort, not(equalTo(firstClientPort.get()))); + } + final byte[] bytes = randomByteArrayOfLength(2); + exchange.sendResponseHeaders(200, bytes.length); + exchange.getResponseBody().write(bytes); + } finally { + exchange.close(); + } + }); + + final InetSocketAddress address = httpServer.getAddress(); + final URI uri = new URI("http://" + InetAddresses.toUriString(address.getAddress()) + ":" + address.getPort()); + + // Authenticator with a short TTL + final RealmConfig config = buildConfig( + getBasicRealmSettings().put(getFullSettingKey(REALM_NAME, OpenIdConnectRealmSettings.HTTP_CONNECTION_POOL_TTL), "1s").build(), + threadContext + ); + authenticator = new OpenIdConnectAuthenticator(config, getOpConfig(), getDefaultRpConfig(), new SSLService(env), null); + + // In addition, capture logs to show that kept alive (TTL) is honored + final Logger logger = LogManager.getLogger(PoolingNHttpClientConnectionManager.class); + final MockLogAppender appender = new MockLogAppender(); + appender.start(); + Loggers.addAppender(logger, appender); + Loggers.setLevel(logger, Level.DEBUG); + try { + appender.addExpectation( + new MockLogAppender.PatternSeenEventExpectation( + "log", + logger.getName(), + Level.DEBUG, + ".*Connection .* can be kept alive for 1.0 seconds" + ) + ); + // Issue two requests to verify the 2nd request do not reuse the 1st request's connection + for (int i = 0; i < 2; i++) { + final CountDownLatch latch = new CountDownLatch(1); + authenticator.getHttpClient().execute(new HttpGet(uri), new FutureCallback<>() { + @Override + public void completed(HttpResponse result) { + latch.countDown(); + } + + @Override + public void failed(Exception ex) { + assert false; + } + + @Override + public void cancelled() { + assert false; + } + }); + latch.await(); + Thread.sleep(1500); + } + appender.assertAllExpectationsMatched(); + } finally { + Loggers.removeAppender(logger, appender); + appender.stop(); + Loggers.setLevel(logger, (Level) null); + authenticator.close(); + httpServer.stop(1); + } + } + private OpenIdConnectProviderConfiguration getOpConfig() throws URISyntaxException { return new OpenIdConnectProviderConfiguration( new Issuer("https://op.example.com"), From 6b4db1cf06d531f0bba0e5f43e5a5d1b95d966e4 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Mon, 20 Jun 2022 18:00:24 +1000 Subject: [PATCH 10/13] test for strategy itself --- .../oidc/OpenIdConnectAuthenticator.java | 35 +++++++----- .../oidc/OpenIdConnectAuthenticatorTests.java | 56 +++++++++++++++++++ 2 files changed, 77 insertions(+), 14 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java index b4562256b234e..ac3ce61714709 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java @@ -58,6 +58,7 @@ import org.apache.http.concurrent.FutureCallback; import org.apache.http.config.Registry; import org.apache.http.config.RegistryBuilder; +import org.apache.http.conn.ConnectionKeepAliveStrategy; import org.apache.http.entity.ContentType; import org.apache.http.impl.auth.BasicScheme; import org.apache.http.impl.client.DefaultConnectionKeepAliveStrategy; @@ -707,23 +708,11 @@ private CloseableHttpAsyncClient createHttpClient() { .setConnectionRequestTimeout(Math.toIntExact(realmConfig.getSetting(HTTP_CONNECTION_READ_TIMEOUT).getSeconds())) .setSocketTimeout(Math.toIntExact(realmConfig.getSetting(HTTP_SOCKET_TIMEOUT).getMillis())) .build(); - final long userConfiguredKeepAlive = realmConfig.getSetting(HTTP_CONNECTION_POOL_TTL).millis(); + HttpAsyncClientBuilder httpAsyncClientBuilder = HttpAsyncClients.custom() .setConnectionManager(connectionManager) .setDefaultRequestConfig(requestConfig) - .setKeepAliveStrategy((response, context) -> { - var serverKeepAlive = DefaultConnectionKeepAliveStrategy.INSTANCE.getKeepAliveDuration(response, context); - final long actualKeepAlive; - if (serverKeepAlive == -1) { - actualKeepAlive = userConfiguredKeepAlive; - } else if (userConfiguredKeepAlive == -1) { - actualKeepAlive = serverKeepAlive; - } else { - actualKeepAlive = Math.min(serverKeepAlive, userConfiguredKeepAlive); - } - LOGGER.debug("effective HTTP connection keep-alive: [{}]ms", actualKeepAlive); - return actualKeepAlive; - }); + .setKeepAliveStrategy(getKeepAliveStrategy()); if (realmConfig.hasSetting(HTTP_PROXY_HOST)) { httpAsyncClientBuilder.setProxy( new HttpHost( @@ -747,6 +736,24 @@ CloseableHttpAsyncClient getHttpClient() { return httpClient; } + // Package private for testing + ConnectionKeepAliveStrategy getKeepAliveStrategy() { + final long userConfiguredKeepAlive = realmConfig.getSetting(HTTP_CONNECTION_POOL_TTL).millis(); + return (response, context) -> { + var serverKeepAlive = DefaultConnectionKeepAliveStrategy.INSTANCE.getKeepAliveDuration(response, context); + final long actualKeepAlive; + if (serverKeepAlive == -1) { + actualKeepAlive = userConfiguredKeepAlive; + } else if (userConfiguredKeepAlive == -1) { + actualKeepAlive = serverKeepAlive; + } else { + actualKeepAlive = Math.min(serverKeepAlive, userConfiguredKeepAlive); + } + LOGGER.debug("effective HTTP connection keep-alive: [{}]ms", actualKeepAlive); + return actualKeepAlive; + }; + } + /* * Creates an {@link IDTokenValidator} based on the current Relying Party configuration */ diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticatorTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticatorTests.java index 5474790a16794..2a5f6ca7aac38 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticatorTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticatorTests.java @@ -46,16 +46,20 @@ import com.nimbusds.openid.connect.sdk.validators.InvalidHashException; import com.sun.net.httpserver.HttpServer; +import org.apache.http.HeaderIterator; import org.apache.http.HttpResponse; import org.apache.http.HttpVersion; import org.apache.http.ProtocolVersion; import org.apache.http.client.methods.HttpGet; import org.apache.http.concurrent.FutureCallback; +import org.apache.http.conn.ConnectionKeepAliveStrategy; import org.apache.http.entity.ContentType; import org.apache.http.entity.StringEntity; import org.apache.http.impl.nio.conn.PoolingNHttpClientConnectionManager; +import org.apache.http.message.BasicHeader; import org.apache.http.message.BasicHttpResponse; import org.apache.http.message.BasicStatusLine; +import org.apache.http.protocol.HTTP; import org.apache.logging.log4j.Level; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -99,6 +103,8 @@ import java.util.Base64; import java.util.Collections; import java.util.Date; +import java.util.Iterator; +import java.util.List; import java.util.Map; import java.util.UUID; import java.util.concurrent.CountDownLatch; @@ -1020,6 +1026,56 @@ public void testLogIdTokenAndNonce() throws URISyntaxException, BadJOSEException } } + public void testKeepAliveStrategy() throws URISyntaxException { + final HttpResponse httpResponse = mock(HttpResponse.class); + final int serverTimeoutInSeconds = randomIntBetween(-1, 300); + final Iterator iterator = List.of(new BasicHeader("Keep-Alive", "timeout=" + serverTimeoutInSeconds)).iterator(); + when(httpResponse.headerIterator(HTTP.CONN_KEEP_ALIVE)).thenReturn(new HeaderIterator() { + @Override + public boolean hasNext() { + return iterator.hasNext(); + } + + @Override + public org.apache.http.Header nextHeader() { + return iterator.next(); + } + + @Override + public Object next() { + return iterator.next(); + } + }); + + // Authenticator with a short TTL + final Settings.Builder settingsBuilder = getBasicRealmSettings(); + final int clientTimeoutInSeconds; + if (randomBoolean()) { + clientTimeoutInSeconds = randomIntBetween(-1, 300); + settingsBuilder.put( + getFullSettingKey(REALM_NAME, OpenIdConnectRealmSettings.HTTP_CONNECTION_POOL_TTL), + clientTimeoutInSeconds + "s" + ); + } else { + clientTimeoutInSeconds = 180; // default 180s + } + final RealmConfig config = buildConfig(settingsBuilder.build(), threadContext); + authenticator = new OpenIdConnectAuthenticator(config, getOpConfig(), getDefaultRpConfig(), new SSLService(env), null); + try { + final ConnectionKeepAliveStrategy keepAliveStrategy = authenticator.getKeepAliveStrategy(); + final int keepAliveDurationInSeconds = (int) keepAliveStrategy.getKeepAliveDuration(httpResponse, null) / 1000; + if (serverTimeoutInSeconds == -1) { + assertThat(keepAliveDurationInSeconds, equalTo(clientTimeoutInSeconds)); + } else if (clientTimeoutInSeconds == -1) { + assertThat(keepAliveDurationInSeconds, equalTo(serverTimeoutInSeconds)); + } else { + assertThat(keepAliveDurationInSeconds, equalTo(Math.min(serverTimeoutInSeconds, clientTimeoutInSeconds))); + } + } finally { + authenticator.close(); + } + } + public void testHttpClientConnectionTTL() throws URISyntaxException, IllegalAccessException, InterruptedException, IOException { // Create an internal HTTP server, the expectation is: For 2 consecutive HTTP requests, the client port should be different // because the client should not reuse the same connection after 1s From 70518ff170c2403bc6881c28f0ed4a05f4dc96d4 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Mon, 20 Jun 2022 18:42:55 +1000 Subject: [PATCH 11/13] tweak --- .../oidc/OpenIdConnectAuthenticatorTests.java | 31 +++++++++++++++---- 1 file changed, 25 insertions(+), 6 deletions(-) diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticatorTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticatorTests.java index 2a5f6ca7aac38..44aa1d9af1c6c 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticatorTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticatorTests.java @@ -1026,7 +1026,7 @@ public void testLogIdTokenAndNonce() throws URISyntaxException, BadJOSEException } } - public void testKeepAliveStrategy() throws URISyntaxException { + public void testKeepAliveStrategy() throws URISyntaxException, IllegalAccessException { final HttpResponse httpResponse = mock(HttpResponse.class); final int serverTimeoutInSeconds = randomIntBetween(-1, 300); final Iterator iterator = List.of(new BasicHeader("Keep-Alive", "timeout=" + serverTimeoutInSeconds)).iterator(); @@ -1061,17 +1061,36 @@ public Object next() { } final RealmConfig config = buildConfig(settingsBuilder.build(), threadContext); authenticator = new OpenIdConnectAuthenticator(config, getOpConfig(), getDefaultRpConfig(), new SSLService(env), null); + final Logger logger = LogManager.getLogger(OpenIdConnectAuthenticator.class); + final MockLogAppender appender = new MockLogAppender(); + appender.start(); + Loggers.addAppender(logger, appender); + Loggers.setLevel(logger, Level.DEBUG); try { - final ConnectionKeepAliveStrategy keepAliveStrategy = authenticator.getKeepAliveStrategy(); - final int keepAliveDurationInSeconds = (int) keepAliveStrategy.getKeepAliveDuration(httpResponse, null) / 1000; + final int effectiveTtlInSeconds; if (serverTimeoutInSeconds == -1) { - assertThat(keepAliveDurationInSeconds, equalTo(clientTimeoutInSeconds)); + effectiveTtlInSeconds = clientTimeoutInSeconds; } else if (clientTimeoutInSeconds == -1) { - assertThat(keepAliveDurationInSeconds, equalTo(serverTimeoutInSeconds)); + effectiveTtlInSeconds = serverTimeoutInSeconds; } else { - assertThat(keepAliveDurationInSeconds, equalTo(Math.min(serverTimeoutInSeconds, clientTimeoutInSeconds))); + effectiveTtlInSeconds = Math.min(serverTimeoutInSeconds, clientTimeoutInSeconds); } + final long effectiveTtlInMs = effectiveTtlInSeconds == -1 ? -1L : effectiveTtlInSeconds * 1000L; + appender.addExpectation( + new MockLogAppender.SeenEventExpectation( + "log", + logger.getName(), + Level.DEBUG, + "effective HTTP connection keep-alive: [" + effectiveTtlInMs + "]ms" + ) + ); + final ConnectionKeepAliveStrategy keepAliveStrategy = authenticator.getKeepAliveStrategy(); + assertThat(keepAliveStrategy.getKeepAliveDuration(httpResponse, null), equalTo(effectiveTtlInMs)); + appender.assertAllExpectationsMatched(); } finally { + Loggers.removeAppender(logger, appender); + appender.stop(); + Loggers.setLevel(logger, (Level) null); authenticator.close(); } } From c41681ba7ea8c22d2c476fd7ebe8428803626769 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Mon, 20 Jun 2022 20:46:25 +1000 Subject: [PATCH 12/13] robust test --- .../oidc/OpenIdConnectAuthenticator.java | 9 +- .../oidc/OpenIdConnectAuthenticatorTests.java | 197 +++++++++++------- 2 files changed, 133 insertions(+), 73 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java index ac3ce61714709..e7222548043de 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java @@ -741,14 +741,17 @@ ConnectionKeepAliveStrategy getKeepAliveStrategy() { final long userConfiguredKeepAlive = realmConfig.getSetting(HTTP_CONNECTION_POOL_TTL).millis(); return (response, context) -> { var serverKeepAlive = DefaultConnectionKeepAliveStrategy.INSTANCE.getKeepAliveDuration(response, context); - final long actualKeepAlive; - if (serverKeepAlive == -1) { + long actualKeepAlive; + if (serverKeepAlive <= -1) { actualKeepAlive = userConfiguredKeepAlive; - } else if (userConfiguredKeepAlive == -1) { + } else if (userConfiguredKeepAlive <= -1) { actualKeepAlive = serverKeepAlive; } else { actualKeepAlive = Math.min(serverKeepAlive, userConfiguredKeepAlive); } + if (actualKeepAlive < -1) { + actualKeepAlive = -1; + } LOGGER.debug("effective HTTP connection keep-alive: [{}]ms", actualKeepAlive); return actualKeepAlive; }; diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticatorTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticatorTests.java index 44aa1d9af1c6c..cc1b08c55108e 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticatorTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticatorTests.java @@ -1026,76 +1026,8 @@ public void testLogIdTokenAndNonce() throws URISyntaxException, BadJOSEException } } - public void testKeepAliveStrategy() throws URISyntaxException, IllegalAccessException { - final HttpResponse httpResponse = mock(HttpResponse.class); - final int serverTimeoutInSeconds = randomIntBetween(-1, 300); - final Iterator iterator = List.of(new BasicHeader("Keep-Alive", "timeout=" + serverTimeoutInSeconds)).iterator(); - when(httpResponse.headerIterator(HTTP.CONN_KEEP_ALIVE)).thenReturn(new HeaderIterator() { - @Override - public boolean hasNext() { - return iterator.hasNext(); - } - - @Override - public org.apache.http.Header nextHeader() { - return iterator.next(); - } - - @Override - public Object next() { - return iterator.next(); - } - }); - - // Authenticator with a short TTL - final Settings.Builder settingsBuilder = getBasicRealmSettings(); - final int clientTimeoutInSeconds; - if (randomBoolean()) { - clientTimeoutInSeconds = randomIntBetween(-1, 300); - settingsBuilder.put( - getFullSettingKey(REALM_NAME, OpenIdConnectRealmSettings.HTTP_CONNECTION_POOL_TTL), - clientTimeoutInSeconds + "s" - ); - } else { - clientTimeoutInSeconds = 180; // default 180s - } - final RealmConfig config = buildConfig(settingsBuilder.build(), threadContext); - authenticator = new OpenIdConnectAuthenticator(config, getOpConfig(), getDefaultRpConfig(), new SSLService(env), null); - final Logger logger = LogManager.getLogger(OpenIdConnectAuthenticator.class); - final MockLogAppender appender = new MockLogAppender(); - appender.start(); - Loggers.addAppender(logger, appender); - Loggers.setLevel(logger, Level.DEBUG); - try { - final int effectiveTtlInSeconds; - if (serverTimeoutInSeconds == -1) { - effectiveTtlInSeconds = clientTimeoutInSeconds; - } else if (clientTimeoutInSeconds == -1) { - effectiveTtlInSeconds = serverTimeoutInSeconds; - } else { - effectiveTtlInSeconds = Math.min(serverTimeoutInSeconds, clientTimeoutInSeconds); - } - final long effectiveTtlInMs = effectiveTtlInSeconds == -1 ? -1L : effectiveTtlInSeconds * 1000L; - appender.addExpectation( - new MockLogAppender.SeenEventExpectation( - "log", - logger.getName(), - Level.DEBUG, - "effective HTTP connection keep-alive: [" + effectiveTtlInMs + "]ms" - ) - ); - final ConnectionKeepAliveStrategy keepAliveStrategy = authenticator.getKeepAliveStrategy(); - assertThat(keepAliveStrategy.getKeepAliveDuration(httpResponse, null), equalTo(effectiveTtlInMs)); - appender.assertAllExpectationsMatched(); - } finally { - Loggers.removeAppender(logger, appender); - appender.stop(); - Loggers.setLevel(logger, (Level) null); - authenticator.close(); - } - } - - public void testHttpClientConnectionTTL() throws URISyntaxException, IllegalAccessException, InterruptedException, IOException { + public void testHttpClientConnectionTtlBehaviour() throws URISyntaxException, IllegalAccessException, InterruptedException, + IOException { // Create an internal HTTP server, the expectation is: For 2 consecutive HTTP requests, the client port should be different // because the client should not reuse the same connection after 1s final HttpServer httpServer = MockHttpServer.createHttp(new InetSocketAddress(InetAddress.getLoopbackAddress(), 0), 0); @@ -1174,6 +1106,131 @@ public void cancelled() { } } + public void testKeepAliveStrategy() throws URISyntaxException, IllegalAccessException { + // Neither server nor client has explicit configuration + doTestKeepAliveStrategy(null, null, 180_000L); + + // Client explicitly configures for 100s + doTestKeepAliveStrategy(null, "100", 100_000L); + + // Server explicitly configures for 400s, but client's default is 180s + doTestKeepAliveStrategy("400", null, 180_000L); + + // Server explicitly configures for 120s + doTestKeepAliveStrategy("120", null, 120_000L); + + // Both server and client explicitly configures it + doTestKeepAliveStrategy("120", "90", 90_000L); + + // Both server and client explicitly configures it + doTestKeepAliveStrategy("80", "90", 80_000L); + + // Server configures negative value + doTestKeepAliveStrategy(String.valueOf(randomIntBetween(-100, -1)), null, 180_000L); + doTestKeepAliveStrategy(String.valueOf(randomIntBetween(-100, -1)), "400", 400_000L); + + // Client configures negative value, -1 is the only negative number accepted by timeSetting + doTestKeepAliveStrategy(null, "-1", -1L); + doTestKeepAliveStrategy("30", "-1", 30_000L); + + // Both server and client explicitly configures negative values + doTestKeepAliveStrategy(String.valueOf(randomIntBetween(-100, -1)), "-1", -1L); + + // Extra randomization + final int serverTtlInSeconds; + if (randomBoolean()) { + serverTtlInSeconds = randomIntBetween(-1, 300); + } else { + // Server may not set the response header + serverTtlInSeconds = -1; + } + + final int clientTtlInSeconds; + if (randomBoolean()) { + clientTtlInSeconds = randomIntBetween(-1, 300); + } else { + clientTtlInSeconds = 180; // default 180s + } + + final int effectiveTtlInSeconds; + if (serverTtlInSeconds <= -1) { + effectiveTtlInSeconds = clientTtlInSeconds; + } else if (clientTtlInSeconds <= -1) { + effectiveTtlInSeconds = serverTtlInSeconds; + } else { + effectiveTtlInSeconds = Math.min(serverTtlInSeconds, clientTtlInSeconds); + } + final long effectiveTtlInMs = effectiveTtlInSeconds <= -1 ? -1L : effectiveTtlInSeconds * 1000L; + + doTestKeepAliveStrategy( + serverTtlInSeconds == -1 ? randomFrom(String.valueOf(serverTtlInSeconds), null) : String.valueOf(serverTtlInSeconds), + clientTtlInSeconds == 180 ? randomFrom(String.valueOf(clientTtlInSeconds), null) : String.valueOf(clientTtlInSeconds), + effectiveTtlInMs + ); + } + + public void doTestKeepAliveStrategy(String serverTtlInSeconds, String clientTtlInSeconds, long effectiveTtlInMs) + throws URISyntaxException, IllegalAccessException { + final HttpResponse httpResponse = mock(HttpResponse.class); + final Iterator iterator; + if (serverTtlInSeconds != null) { + iterator = List.of(new BasicHeader("Keep-Alive", "timeout=" + serverTtlInSeconds)).iterator(); + } else { + // Server may not set the response header + iterator = Collections.emptyIterator(); + } + when(httpResponse.headerIterator(HTTP.CONN_KEEP_ALIVE)).thenReturn(new HeaderIterator() { + @Override + public boolean hasNext() { + return iterator.hasNext(); + } + + @Override + public org.apache.http.Header nextHeader() { + return iterator.next(); + } + + @Override + public Object next() { + return iterator.next(); + } + }); + + final Settings.Builder settingsBuilder = getBasicRealmSettings(); + if (clientTtlInSeconds != null) { + settingsBuilder.put( + getFullSettingKey(REALM_NAME, OpenIdConnectRealmSettings.HTTP_CONNECTION_POOL_TTL), + clientTtlInSeconds + "s" + ); + } + final RealmConfig config = buildConfig(settingsBuilder.build(), threadContext); + authenticator = new OpenIdConnectAuthenticator(config, getOpConfig(), getDefaultRpConfig(), new SSLService(env), null); + + final Logger logger = LogManager.getLogger(OpenIdConnectAuthenticator.class); + final MockLogAppender appender = new MockLogAppender(); + appender.start(); + Loggers.addAppender(logger, appender); + Loggers.setLevel(logger, Level.DEBUG); + try { + appender.addExpectation( + new MockLogAppender.SeenEventExpectation( + "log", + logger.getName(), + Level.DEBUG, + "effective HTTP connection keep-alive: [" + effectiveTtlInMs + "]ms" + ) + ); + final ConnectionKeepAliveStrategy keepAliveStrategy = authenticator.getKeepAliveStrategy(); + assertThat(keepAliveStrategy.getKeepAliveDuration(httpResponse, null), equalTo(effectiveTtlInMs)); + appender.assertAllExpectationsMatched(); + } finally { + Loggers.removeAppender(logger, appender); + appender.stop(); + Loggers.setLevel(logger, (Level) null); + authenticator.close(); + } + } + private OpenIdConnectProviderConfiguration getOpConfig() throws URISyntaxException { return new OpenIdConnectProviderConfiguration( new Issuer("https://op.example.com"), From 60220a18d2600a109edf05ddc1e2e2ac166f9439 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Mon, 20 Jun 2022 20:52:09 +1000 Subject: [PATCH 13/13] tweak --- .../security/authc/oidc/OpenIdConnectAuthenticatorTests.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticatorTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticatorTests.java index cc1b08c55108e..041e6b0d90b54 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticatorTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticatorTests.java @@ -1034,12 +1034,14 @@ public void testHttpClientConnectionTtlBehaviour() throws URISyntaxException, Il httpServer.start(); final AtomicReference firstClientPort = new AtomicReference<>(null); + final AtomicReference portTested = new AtomicReference<>(false); httpServer.createContext("/", exchange -> { try { final int currentPort = exchange.getRemoteAddress().getPort(); // Either set the first port number, otherwise the current (2nd) port number should be different from the 1st one if (false == firstClientPort.compareAndSet(null, currentPort)) { assertThat(currentPort, not(equalTo(firstClientPort.get()))); + portTested.set(true); } final byte[] bytes = randomByteArrayOfLength(2); exchange.sendResponseHeaders(200, bytes.length); @@ -1097,6 +1099,7 @@ public void cancelled() { Thread.sleep(1500); } appender.assertAllExpectationsMatched(); + assertThat(portTested.get(), is(true)); } finally { Loggers.removeAppender(logger, appender); appender.stop();