Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add setting for tcp_keepalive for oidc back-channel #87868

Merged

Conversation

ywangd
Copy link
Member

@ywangd ywangd commented Jun 21, 2022

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

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
@ywangd ywangd added >enhancement :Security/Security Security issues without another label v8.4.0 labels Jun 21, 2022
@elasticsearchmachine
Copy link
Collaborator

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

@ywangd
Copy link
Member Author

ywangd commented Jun 21, 2022

NOTE: This is currently a draft PR because:

  1. I cannot find a way to test tcp keepalive
  2. Depending on the OS's configuration, the keepalive probe may not be enough keep the connection up. Add setting for keep-alive duration for oidc back-channel #87773 is still the right candidate to address Custom keep-alive strategy for outgoing OIDC connections #75515. But maybe this change can be useful for some other situations.

Comment on lines +696 to +698
ConnectingIOReactor ioReactor = new DefaultConnectingIOReactor(
IOReactorConfig.custom().setSoKeepAlive(realmConfig.getSetting(HTTP_TCP_KEEP_ALIVE)).build()
);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no easy way to test this setting. Other than invasive reflection, one possibility is to subclass DefaultConnectingIOReactor and override the prepareSocket method where we can assert the socket configuration. It feels heavy weighted for testing though. We may need the subclass in production if we decide to support for configuring keepalive_time, interval etc in future. So I think we can save the test till we actually need that in production.

@ywangd ywangd marked this pull request as ready for review June 22, 2022 00:55
@elasticmachine elasticmachine added the Team:Security Meta label for security team label Jun 22, 2022
@elasticmachine
Copy link
Collaborator

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

@ywangd ywangd requested review from DaveCTurner and tvernum June 22, 2022 00:55
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to at least confirm manually that this does enable keepalives on the connection (e.g. using ss -toe). I expect it does so this LGTM.

If we were to do something like #84653 then we could check this timer directly in a test (at least on Linux).

@ywangd
Copy link
Member Author

ywangd commented Jun 30, 2022

It would be good to at least confirm manually that this does enable keepalives on the connection (e.g. using ss -toe).

Thanks for the tip. I manually verified it and it worked:

Before

ESTAB 0 0 ***:60142 ***:443 uid:1000 ino:253847 sk:2002 <->

After

ESTAB 0 0 *:53448 *:443 timer:(keepalive,119min,0) uid:1000 ino:261040 sk:4001 <->

Copy link
Contributor

@tvernum tvernum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, pending fixed typo in docs.

docs/reference/settings/security-settings.asciidoc Outdated Show resolved Hide resolved
@ywangd
Copy link
Member Author

ywangd commented Jul 7, 2022

@elasticmachine update branch

@ywangd ywangd added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Jul 7, 2022
@elasticsearchmachine elasticsearchmachine merged commit 36336fe into elastic:master Jul 7, 2022
@ywangd ywangd deleted the oidc-back-channel-sokeepalive branch July 7, 2022 02:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) >enhancement :Security/Security Security issues without another label Team:Security Meta label for security team v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants