[HTTP Server] support TLS config hot reload via `SIGHUP` #171823

pgayvallet · 2023-11-23T08:29:39Z

Summary

Add support for hot reloading the Kibana server's TLS configuration, using the same SIGHUP-based reload signal, as already implemented for other parts of the Kibana configuration (e.g logging)

Note:

hot reloading is only supported for the server TLS configuration (server.ssl), not for the whole server.* config prefix
swaping the certificate files (without modifying the kibana config itself) is supported
it is not possible to toggle TLS (enabling or disabling) without restarting Kibana
hot reloading requires to force the process to reload its configuration by sending a SIGHUP signal

Example / how to test

Before

server.ssl.enabled: true
server.ssl.certificate: /path-to-kibana/packages/kbn-dev-utils/certs/kibana.crt
server.ssl.key: /path-to-kibana/packages/kbn-dev-utils/certs/kibana.key

Changing the config

server.ssl.enabled: true
server.ssl.certificate: /path-to-kibana/packages/kbn-dev-utils/certs/elasticsearch.crt
server.ssl.key: /path-to-kibana/packages/kbn-dev-utils/certs/elasticsearch.key

kill -SIGHUP {KIBANA_PID}

After

Release notes

It is now possible to hot reload Kibana's TLS (server.ssl) configuration by updating it and then sending a SIGHUP signal to the Kibana process.

Note that TLS cannot be toggled (disabled/enabled) that way, and that hot reload only works for the TLS configuration, not other properties of the server config prefix.

…fix'

pgayvallet

Self-review

pgayvallet · 2023-11-23T20:23:41Z

packages/core/http/core-http-server-internal/src/http_server.ts

+    // only hot-reloading TLS config - don't need to subscribe if TLS is initially disabled,
+    // given we can't hot-switch from/to enabled/disabled.
+    if (config.ssl.enabled) {
+      const configSubscription = config$
+        .pipe(pairwise())
+        .subscribe(([{ ssl: prevSslConfig }, { ssl: newSslConfig }]) => {
+          if (prevSslConfig.enabled !== newSslConfig.enabled) {
+            this.log.warn(
+              'Incompatible TLS config change detected - TLS cannot be toggled without a full server reboot.'
+            );
+            return;
+          }
+
+          const sameConfig = newSslConfig.isEqualTo(prevSslConfig);
+
+          if (!sameConfig) {
+            this.log.info('TLS configuration change detected - reloading TLS configuration.');
+            setTlsConfig(this.server!, newSslConfig);
+          }
+        });
+      this.subscriptions.push(configSubscription);
+    }


The main addition / entry point of the PR. The HTTP server now has access to the config observable (instead of the config object) and listen to changes to check if the SSL config was updated. If so, we propagate the changes to the underlying tls server

pgayvallet · 2023-11-23T20:26:41Z

packages/kbn-server-http-tools/src/set_tls_config.ts

+function isServerTLS(server: HttpServer): server is TlsServer {
+  return 'setSecureContext' in server;
+}


NIT: I could probably have used server instanceof TlsServer instead, but tbh this works and is easier to stub for testing.

pgayvallet · 2023-11-23T20:28:06Z

packages/kbn-server-http-tools/src/ssl/ssl_config.ts

+  public isEqualTo(otherConfig: SslConfig) {
+    if (this === otherConfig) {
+      return true;
+    }
+    return isEqual(this, otherConfig);
+  }


Had to implement a comparison func on the SslConfig class. Wasn't strictly necessary, but it allows to know when the SSL config really changed to avoid propagating the changes on every SIGHUP (and also to have better logging given we only output an info message when the config really changed)

pgayvallet · 2023-11-23T20:28:58Z

src/core/server/integration_tests/http/set_tls_config.test.ts

+describe('setTlsConfig', () => {
+  const CSP_CONFIG = cspConfig.schema.validate({});
+  const EXTERNAL_URL_CONFIG = externalUrlConfig.schema.validate({});


Integration test against a vanilla HAPI server

pgayvallet · 2023-11-23T20:29:17Z

src/core/server/integration_tests/http/tls_config_reload.test.ts

+describe('HttpServer - TLS config', () => {
+  let server: HttpServer;
+  let logger: ReturnType<typeof loggingSystemMock.createLogger>;


Integration test against our HttpServer.

elasticmachine · 2023-11-23T20:33:52Z

Pinging @elastic/kibana-core (Team:Core)

kibana-ci · 2023-11-23T22:07:22Z

💚 Build Succeeded

Buildkite Build
Commit: 0844ea0

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/core-http-server-internal`	53	52	-1
`@kbn/server-http-tools`	51	57	+6
total			+5

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`@kbn/core-http-server-internal`	8	9	+1

Unknown metric groups

API count

id	before	after	diff
`@kbn/core-http-server-internal`	59	58	-1
`@kbn/server-http-tools`	54	61	+7
total			+6

History

💚 Build #178529 succeeded 4e8abb3
💛 Build #178431 was flaky f0283c7
💔 Build #178418 failed 8932936
💔 Build #178354 failed 3e851f6c6427f469b02567b234b15a58f2bec2b8
💔 Build #178340 failed f15536e

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

TinaHeiligers

I pulle and ran the code and it works as intended. The implementation looks fine too.

One question that came to mind is making the change to SSL more noticeable. ATM, the info logs might get lost in a sea of debug logs. If we changed logging to warn on SSL certs updates, it might be more prevalent.

TinaHeiligers · 2023-11-23T22:54:20Z

packages/core/http/core-http-server-internal/src/http_server.ts

+        .pipe(pairwise())
+        .subscribe(([{ ssl: prevSslConfig }, { ssl: newSslConfig }]) => {
+          if (prevSslConfig.enabled !== newSslConfig.enabled) {
+            this.log.warn(


If someone's debugging why an update to their certs "isn't working", they might be tempted to enable debug logging. Warning logs can get lost in those.

I wonder if we shouldn't make the warning more prominent 🤔

TinaHeiligers · 2023-11-23T22:58:20Z

packages/core/http/core-http-server-internal/src/http_server.ts

+          const sameConfig = newSslConfig.isEqualTo(prevSslConfig);
+
+          if (!sameConfig) {
+            this.log.info('TLS configuration change detected - reloading TLS configuration.');


I think this should also be a warning log: If someone's intentionally making a change/update they'll know to ignore the log line. If, however, something's gone wrong and we're using logs to figure out what happened, we'd be less likely to ignore a warning log entry than an info-level one.

The whole SIGHUP reload chain's logging is INFO based (which is the correct level ihmo, it's informative), so I think I'd rather keep that consistent.

TinaHeiligers · 2023-11-23T23:01:30Z

packages/kbn-config/src/config_service.ts

    return this.config$.pipe(
      map((config) => config.get(path)),
-      distinctUntilChanged(isEqual),
+      ignoreUnchanged ? distinctUntilChanged(isEqual) : identity,


you made me look identity up 😉

kibanamachine · 2023-11-24T09:09:21Z

💔 All backports failed

Status	Branch	Result
❌	8.11	Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 171823

Questions ?

Please refer to the Backport tool documentation

) ## Summary Fix elastic#54368 Add support for hot reloading the Kibana server's TLS configuration, using the same `SIGHUP`-based reload signal, as already implemented for other parts of the Kibana configuration (e.g `logging`) **Note:** - hot reloading is only supported for the server TLS configuration (`server.ssl`), not for the whole `server.*` config prefix - swaping the certificate files (without modifying the kibana config itself) is supported - it is not possible to toggle TLS (enabling or disabling) without restarting Kibana - hot reloading requires to force the process to reload its configuration by sending a `SIGHUP` signal ### Example / how to test #### Before ```yaml server.ssl.enabled: true server.ssl.certificate: /path-to-kibana/packages/kbn-dev-utils/certs/kibana.crt server.ssl.key: /path-to-kibana/packages/kbn-dev-utils/certs/kibana.key ``` <img width="550" alt="Screenshot 2023-11-23 at 15 11 28" src="https://github.com/elastic/kibana/assets/1532934/1226d161-a9f2-4d62-a3de-37161829f187"> #### Changing the config ```yaml server.ssl.enabled: true server.ssl.certificate: /path-to-kibana/packages/kbn-dev-utils/certs/elasticsearch.crt server.ssl.key: /path-to-kibana/packages/kbn-dev-utils/certs/elasticsearch.key ``` ```bash kill -SIGHUP {KIBANA_PID} ``` <img width="865" alt="Screenshot 2023-11-23 at 15 18 21" src="https://github.com/elastic/kibana/assets/1532934/c9412b2e-d70e-4cf0-8eaf-4db70a45af60"> #### After <img width="547" alt="Screenshot 2023-11-23 at 15 18 43" src="https://github.com/elastic/kibana/assets/1532934/c839f04f-4adb-456d-a174-4f0ebd5c234c"> ## Release notes It is now possible to hot reload Kibana's TLS (`server.ssl`) configuration by updating it and then sending a `SIGHUP` signal to the Kibana process. Note that TLS cannot be toggled (disabled/enabled) that way, and that hot reload only works for the TLS configuration, not other properties of the `server` config prefix. --------- Co-authored-by: kibanamachine <[email protected]> (cherry picked from commit 87213e7) # Conflicts: # src/core/tsconfig.json

pgayvallet · 2023-11-24T09:23:15Z

💚 All backports created successfully

Status	Branch	Result
✅	8.11

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…tic#171823) (elastic#171907) # Backport This will backport the following commits from `main` to `8.11`: - [[HTTP Server] support TLS config hot reload via `SIGHUP` (elastic#171823)](elastic#171823)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)

create base test against raw HAPI server

f15536e

pgayvallet changed the title ~~create base test against raw HAPI server~~ [HTTP Server] support TLS config hot reload via SIGHUP Nov 23, 2023

pgayvallet added 2 commits November 23, 2023 11:00

implement part of the mechanism

fda6041

implement compare logic

0aa3d12

pgayvallet force-pushed the kbn-54368-tls-hot-reload branch from 3e851f6 to 0aa3d12 Compare November 23, 2023 10:41

kibanamachine and others added 2 commits November 23, 2023 10:49

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

f72ae4d

…fix'

start adapting tests

8932936

pgayvallet added release_note:enhancement Feature:http Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.12.0 labels Nov 23, 2023

pgayvallet added 3 commits November 23, 2023 12:09

fix test usages

f0283c7

add unit tests

3f8e90a

add more integration tests

4e8abb3

pgayvallet commented Nov 23, 2023

View reviewed changes

pgayvallet marked this pull request as ready for review November 23, 2023 20:33

pgayvallet requested a review from a team as a code owner November 23, 2023 20:33

pgayvallet mentioned this pull request Nov 23, 2023

[FEATURE]: Kibana to watch certificate changes and reload them on-the-fly #54368

Closed

handle case where config did not change and only cert files did

0844ea0

pgayvallet added the backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) label Nov 23, 2023

TinaHeiligers approved these changes Nov 23, 2023

View reviewed changes

pgayvallet merged commit 87213e7 into elastic:main Nov 24, 2023
28 checks passed

pgayvallet mentioned this pull request Nov 24, 2023

[8.11] [HTTP Server] support TLS config hot reload via SIGHUP (#171823) #171907

Merged

kibanamachine added the v8.11.2 label Nov 24, 2023

pgayvallet mentioned this pull request Mar 4, 2024

Respond to TLS certificate/key changes without requiring a restart #101072

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HTTP Server] support TLS config hot reload via `SIGHUP` #171823

[HTTP Server] support TLS config hot reload via `SIGHUP` #171823

pgayvallet commented Nov 23, 2023 •

edited by kibanamachine

Loading

pgayvallet left a comment

pgayvallet Nov 23, 2023

pgayvallet Nov 23, 2023

pgayvallet Nov 23, 2023

pgayvallet Nov 23, 2023

pgayvallet Nov 23, 2023

elasticmachine commented Nov 23, 2023

kibana-ci commented Nov 23, 2023

API count

TinaHeiligers left a comment

TinaHeiligers Nov 23, 2023

TinaHeiligers Nov 23, 2023

pgayvallet Nov 24, 2023

TinaHeiligers Nov 23, 2023

kibanamachine commented Nov 24, 2023

pgayvallet commented Nov 24, 2023

[HTTP Server] support TLS config hot reload via SIGHUP #171823

[HTTP Server] support TLS config hot reload via SIGHUP #171823

Conversation

pgayvallet commented Nov 23, 2023 • edited by kibanamachine Loading

Summary

Example / how to test

Before

Changing the config

After

Release notes

pgayvallet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Nov 23, 2023

kibana-ci commented Nov 23, 2023

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Public APIs missing exports

API count

History

TinaHeiligers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Nov 24, 2023

💔 All backports failed

Manual backport

Questions ?

pgayvallet commented Nov 24, 2023

💚 All backports created successfully

Questions ?

[HTTP Server] support TLS config hot reload via `SIGHUP` #171823

[HTTP Server] support TLS config hot reload via `SIGHUP` #171823

pgayvallet commented Nov 23, 2023 •

edited by kibanamachine

Loading