Add setting for keep-alive duration for oidc back-channel #87773

ywangd · 2022-06-17T02:34:54Z

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.

The new setting has a 3min default which means idle connections are
closed every 3 min if server response does not specify a shorter keep-alive.

Resolves: #75515

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: elastic#75515

elasticmachine · 2022-06-17T02:35:03Z

Pinging @elastic/es-security (Team:Security)

elasticsearchmachine · 2022-06-17T02:35:25Z

Hi @ywangd, I've created a changelog YAML for you.

ywangd · 2022-06-17T02:36:01Z

@tvernum This might be worth to backport to earlier versions (including 7.17?) if it proves to work. For now, I just tagged v8.4.0.

...ty/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java

docs/changelog/87773.yaml

tvernum · 2022-06-17T05:50:24Z

docs/reference/settings/security-settings.asciidoc

+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.


I think we should reword this, but I'll come back with a suggestion later.

I reword'd this paragraph and also made it consistent to the new behaviour of handling keep-alive response header.

...c/main/java/org/elasticsearch/xpack/core/security/authc/oidc/OpenIdConnectRealmSettings.java

...ty/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java

Co-authored-by: Tim Vernum <[email protected]>

…ecurity/authc/oidc/OpenIdConnectAuthenticator.java Co-authored-by: Tim Vernum <[email protected]>

ywangd · 2022-06-17T06:09:32Z

...ty/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java

+                        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;


Tweaked this part a bit more and added a debug logging.

tvernum

Everything that's here LGTM, but we do need a plan for automated testing.

docs/reference/settings/security-settings.asciidoc

tvernum · 2022-06-20T03:24:34Z

...ty/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java

+                        }
+                        LOGGER.debug("effective HTTP connection keep-alive: [{}]ms", actualKeepAlive);
+                        return actualKeepAlive;
+                    });


I think we need to come up with a test strategy for this.
We could do it with a mock HTTP Server and ensure that the connection is pulled down, but that seems like a lot of work.
Is it possible to check the value configured on the client itself?

I came up a way for testing the changes for both the configured TTL and the actual behaviour of "not reusing the same connection". Please see 058872f

Co-authored-by: Tim Vernum <[email protected]>

ywangd · 2022-06-20T06:45:38Z

...c/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticatorTests.java

+        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();
+            }
+        });


For testing the behaviour that two requests with interval greater than the TTL (1s) should use different connections.

ywangd · 2022-06-20T06:46:23Z

...c/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticatorTests.java

+            appender.addExpectation(
+                new MockLogAppender.PatternSeenEventExpectation(
+                    "log",
+                    logger.getName(),
+                    Level.DEBUG,
+                    ".*Connection .* can be kept alive for 1.0 seconds"
+                )
+            );


Testing for the configured TTL in action.

jkakavas · 2022-06-20T12:35:45Z

🚀

This PR adds a new setting to enable tcp keepalive probes for the connections used by the oidc back-channel communication. It defaults to false to keep the existing behaviour. Relates: elastic#87773

This PR adds a new setting to enable tcp keepalive probes for the connections used by the oidc back-channel communication. It defaults to true as tcp keepalive is generally useful for ES. Relates: #87773

…7773) 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. NOTE: This is a "safe" backport of elastic#87773. The key difference here is that the new setting is by default not configured, which means the PR introduces *zero* behaviour change by default. Users need to actively configure the new setting to enable the new behaviour ("automatically closing idle connections). Backport: elastic#87773

…88363) * New setting to close idle connections in OIDC back-channel (#87773) 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. NOTE: This is a "safe" backport of #87773. The key difference here is that the new setting is by default not configured, which means the PR introduces *zero* behaviour change by default. Users need to actively configure the new setting to enable the new behaviour ("automatically closing idle connections). Backport: #87773 * Make it safe by keeping default behaviour * tweak * Update x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticatorTests.java Co-authored-by: Tim Vernum <[email protected]> * address feedback Co-authored-by: Tim Vernum <[email protected]>

…7773) 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. NOTE: This is a "safe" backport of elastic#87773. The key difference here is that the new setting is by default not configured, which means the PR introduces *zero* behaviour change by default. Users need to actively configure the new setting to enable the new behaviour ("automatically closing idle connections). Backport: elastic#87773

…88412) 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. NOTE: This is a "safe" backport of #87773. The key difference here is that the new setting is by default not configured, which means the PR introduces *zero* behaviour change by default. Users need to actively configure the new setting to enable the new behaviour ("automatically closing idle connections). Backport: #87773

ywangd added >enhancement :Security/Security Security issues without another label v8.4.0 labels Jun 17, 2022

ywangd requested a review from tvernum June 17, 2022 02:34

ywangd marked this pull request as ready for review June 17, 2022 02:35

elasticmachine added the Team:Security Meta label for security team label Jun 17, 2022

Update docs/changelog/87773.yaml

a2e3c7c

ywangd commented Jun 17, 2022

View reviewed changes

...ty/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java Outdated Show resolved Hide resolved

tweak

0b70a7f

tvernum reviewed Jun 17, 2022

View reviewed changes

ywangd and others added 3 commits June 17, 2022 15:56

Update docs/changelog/87773.yaml

4920c53

Co-authored-by: Tim Vernum <[email protected]>

Update x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/s…

2f59748

…ecurity/authc/oidc/OpenIdConnectAuthenticator.java Co-authored-by: Tim Vernum <[email protected]>

tweak

158cec5

ywangd commented Jun 17, 2022

View reviewed changes

tweak

09bb6ba

ywangd requested a review from tvernum June 19, 2022 23:13

Merge remote-tracking branch 'origin/master' into es-75515

65a978c

tvernum reviewed Jun 20, 2022

View reviewed changes

ywangd and others added 2 commits June 20, 2022 13:30

Apply suggestions from code review

059308f

Co-authored-by: Tim Vernum <[email protected]>

add test

058872f

ywangd commented Jun 20, 2022

View reviewed changes

ywangd requested a review from tvernum June 20, 2022 06:46

tvernum approved these changes Jun 20, 2022

View reviewed changes

test for strategy itself

6b4db1c

ywangd added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Jun 20, 2022

ywangd removed the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Jun 20, 2022

ywangd added 3 commits June 20, 2022 18:50

tweak

70518ff

robust test

c41681b

tweak

60220a1

ywangd merged commit f075d50 into elastic:master Jun 20, 2022

ywangd mentioned this pull request Jun 21, 2022

Add setting for tcp_keepalive for oidc back-channel #87868

Merged

ywangd mentioned this pull request Jul 8, 2022

New setting to close idle connections in OIDC back-channel (#87773) #88363

Merged

ywangd mentioned this pull request Jul 11, 2022

New setting to close idle connections in OIDC back-channel (#87773) #88412

Merged

ywangd added v8.3.3 v7.17.6 labels Jul 11, 2022

ywangd mentioned this pull request Nov 17, 2022

Custom keep-alive strategy for outgoing OIDC connections #75515

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add setting for keep-alive duration for oidc back-channel #87773

Add setting for keep-alive duration for oidc back-channel #87773

ywangd commented Jun 17, 2022 •

edited

Loading

elasticmachine commented Jun 17, 2022

elasticsearchmachine commented Jun 17, 2022

ywangd commented Jun 17, 2022

tvernum Jun 17, 2022

ywangd Jun 19, 2022

ywangd Jun 17, 2022

tvernum left a comment

tvernum Jun 20, 2022

ywangd Jun 20, 2022

ywangd Jun 20, 2022

ywangd Jun 20, 2022

jkakavas commented Jun 20, 2022

Add setting for keep-alive duration for oidc back-channel #87773

Add setting for keep-alive duration for oidc back-channel #87773

Conversation

ywangd commented Jun 17, 2022 • edited Loading

elasticmachine commented Jun 17, 2022

elasticsearchmachine commented Jun 17, 2022

ywangd commented Jun 17, 2022

tvernum Jun 17, 2022

Choose a reason for hiding this comment

ywangd Jun 19, 2022

Choose a reason for hiding this comment

ywangd Jun 17, 2022

Choose a reason for hiding this comment

tvernum left a comment

Choose a reason for hiding this comment

tvernum Jun 20, 2022

Choose a reason for hiding this comment

ywangd Jun 20, 2022

Choose a reason for hiding this comment

ywangd Jun 20, 2022

Choose a reason for hiding this comment

ywangd Jun 20, 2022

Choose a reason for hiding this comment

jkakavas commented Jun 20, 2022

ywangd commented Jun 17, 2022 •

edited

Loading