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

Support hot-reloading of TLS certificates #110082

Closed
legrego opened this issue Aug 25, 2021 · 6 comments
Closed

Support hot-reloading of TLS certificates #110082

legrego opened this issue Aug 25, 2021 · 6 comments
Labels
enhancement New value added to drive a business result Feature:Configuration Settings in kibana.yml Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc triage_needed

Comments

@legrego
Copy link
Member

legrego commented Aug 25, 2021

Describe the feature:

Updating TLS certificates currently requires a restart of the Kibana process. This behavior is inconsistent with Elasticsearch, which supports hot-reloading of certificates, so long as the file names do not change.

Hot-reloading is beneficial because it reduces the impact of a certificate rotation. Restarting a Kibana instance, even in an HA setup requires coordination. This is especially evident in larger automated deployments, such as what we would find on ECE/ESS.

I would like for us to investigate the feasibility of supporting a hot-reload of:

  1. Browser <--> Kibana certificate and key files
  2. Kibana <--> Elasticsearch certificate and key files

Note that I'm not asking for changes to kibana.yml to be picked up, we are interested in monitoring and responding to changes to the certificate and key files themselves.

cc @elastic/kibana-security

@legrego legrego added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc enhancement New value added to drive a business result Feature:Configuration Settings in kibana.yml labels Aug 25, 2021
@elasticmachine
Copy link
Contributor

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

@mshustov
Copy link
Contributor

mshustov commented Sep 1, 2021

Browser <--> Kibana certificate and key files

nodejs supports this functionality via setSecureContext

The server.setSecureContext() method replaces the secure context of an existing server. Existing connections to the server are not interrupted.

Although, an additional investigation is required to make sure Hapi supports this functionality. We might end up contributing this functionality to Hapi.

Kibana <--> Elasticsearch certificate and key files

We could re-create a new client instance and update references to them

this.asInternalUser = configureClient(config, { logger, type, getExecutionContext });
this.rootScopedClient = configureClient(config, {

But there might be a problem when a plugin stores a reference to a particular client instance in a closure

// asInternalUser
const request = requestFactory({ client: elasticsearch.asInternalUser });
setTimeout(() => request.ping(), 1000)

// asScopedUser
const request = requestFactory({ client: elasticsearch.asScoped(request).asCurrentUser });
setTimeout(() => request.ping(), 1000)

In this case, the reference to a new instance won't be updated.

@delvedor Would it be possible to support TLS cert reloading in the Elasticsearch client once a client instance is created?

@delvedor
Copy link
Member

delvedor commented Sep 3, 2021

I did some experiments, and you can swap the client certificates by closing the currently open connections and create new ones with the updated configuration.
Unfortunately, the current client implementation does not close the client if there are in-flight requests, but you can change this behavior by adding new client methods.

'use strict'

const http = require('http')
const { Client, ConnectionPool, Connection } = require('@elastic/elasticsearch')

class MyConnection extends Connection {
  constructor (opts) {
    super(opts)
    this.check = Math.random() // added for the sake of the logs
  }
}

class MyConnectionPool extends ConnectionPool {
  emptyWithoutWaiting () {
    for (const connection of this.connections) {
      // it won't kill in-flight request
      connection.close(() => {})
    }
    this.connections = [] // reset the connections array
    this.size = 0
  }
}

class MyClient extends Client {
  // the connection pool is shared among every client instance, even child clients.
  // if you update its internal state you are changing its configuration without
  // need to create a new one, which won't work with child clients.
  updateCerts (opts = {}) {
    this.connectionPool.emptyWithoutWaiting()
    this.connectionPool._ssl = opts.ssl
    this.connectionPool._caFingerprint = opts.caFingerprint
    this.connectionPool.addConnection(opts.node || opts.nodes)
  }
}

function handler (req, res) {
  console.log(req.method, req.url)
  res.setHeader('content-type', 'application/json')
  res.setHeader('x-elastic-product', 'Elasticsearch')
  res.end(JSON.stringify({ version: { number: '8.0.0-SNAPSHOT' } }))
}

const server = http.createServer(handler)
server.listen(3000, async () => {
  const client = new MyClient({
    node: 'http://localhost:3000',
    Connection: MyConnection,
    ConnectionPool: MyConnectionPool
  })

  client.on('response', (err, event) => {
    if (err) throw err
    console.log(event.meta.connection.check) // verify that we actually changed the connection instance
  })

  await client.cat.indices()
  const child = client.child()
  await child.cat.indices() // works with child clients
  setTimeout(() => child.cat.indices(), 1000) // works with clients inside closures

  client.updateCerts({ node: 'http://localhost:3000' })

  await client.cat.indices()
  await child.cat.indices()
})

The key to make this work is that updateCerts should be a synchronous operation, so there will be no conflict between new requests and in-flight requests.

@lukeelmers
Copy link
Member

Although, an additional investigation is required to make sure Hapi supports this functionality.

Doesn't solve the problem with the ES client, but from an initial investigation it looks like hapi blindly passes the provided TLS options directly to Node https, so it should be possible to update @kbn/server-http-tools to provide SecureContextOptions instead of TlsOptions when configuring the hapi server. Then we'd be able to leverage the setSecureContext API as mentioned above.

const tlsOptions: TLSOptions = {
ca: ssl.certificateAuthorities,
cert: ssl.certificate,
ciphers: config.ssl.cipherSuites?.join(':'),
// We use the server's cipher order rather than the client's to prevent the BEAST attack.
honorCipherOrder: true,
key: ssl.key,
passphrase: ssl.keyPassphrase,
secureOptions: ssl.getSecureOptions ? ssl.getSecureOptions() : undefined,
requestCert: ssl.requestCert,
rejectUnauthorized: ssl.rejectUnauthorized,
};

@sebgl
Copy link

sebgl commented Feb 28, 2023

There are some subtleties to take into consideration about how K8s upgrades files (like certificates) mounted from K8s secrets in a Pod filesystem. Rather than updating the existing files, it atomically swaps a directory symlink to the updated versions of the files. This blogpost explains it very well: https://ahmet.im/blog/kubernetes-inotify/.
We should make sure our implementation covers that TLS cert files upgrade flow.

@pgayvallet
Copy link
Contributor

Closing in favor of #101072

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result Feature:Configuration Settings in kibana.yml Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc triage_needed
Projects
None yet
Development

No branches or pull requests

8 participants