feat: support separate client TLS certificates by gwossum · Pull Request #27543 · influxdata/influxdb

gwossum · 2026-07-15T14:39:06Z

Support for separate server and client certificates in dual
client/server TLSConfigManager objects.
Fix issue with subscriber not updating TLS certificate path on reload.
Subscriber now reloads all TLS parameter, not just certificate.
Fix issue with subscriber not propagating new TLS configuration
to existing clients.
Certificate monitoring will now certificate loaders using the same
certificate to avoid overly noisy logs.
Server, client, and client/server TLSConfigManager objects are now
distinct and provide better checks based on their role.
General refactoring of TLSConfigManager and support classes.

- Support for separate server and client certificates in dual client/server TLSConfigManager objects. - Certificate monitoring will now certificate loaders using the same certificate to avoid overly noisy logs. - Server, client, and client/server TLSConfigManager objects are now distinct and provide better checks based on their role. - General refactoring of TLSConfigManager and support classes.

Fix subscriber service so it now does a full TLS configuration reload. Previously the certificate was not being propagated to existing subscriber clients. Also added doing a full TLS configuration reload for all configs, including CAs.

Remove unused and misleading *tls.Config parameter from NewHTTPS.

Copilot

Pull request overview

This PR refactors the pkg/tlsconfig subsystem to support distinct server and client certificates (and roles) and integrates that into multiple services (subscriber/httpd/opentsdb), including improved runtime reload behavior and centralized certificate monitoring to reduce log noise.

Changes:

Introduces role-aware TLSConfigManager APIs (server-only, client-only, dual-role) with separate server/client certificate loaders and reconfiguration support.
Updates subscriber HTTPS dialing to resolve TLS configuration per connection (so reloads reach existing writers) and adds focused TLS reload tests.
Adds TLSCertMonitor to centralize expiration/validity monitoring across loaders and deduplicate warnings for shared cert/key pairs.

Reviewed changes

Copilot reviewed 24 out of 24 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
services/subscriber/tlsreload_internal_test.go	New internal tests validating subscriber TLS reload behavior (cert, CA pool, insecure flags, existing vs new connections).
services/subscriber/service.go	Injects cert monitor, uses `TLSManagerOpts`, reconfigures TLS manager on reload, and switches HTTPS writers to `DialTLSContext`.
services/subscriber/service_test.go	Updates tests to construct/clean up a shared cert monitor for the subscriber service.
services/subscriber/mtls_internal_test.go	Updates mTLS tests to use the new TLS config manager APIs and per-connection dialing.
services/subscriber/http.go	Switches HTTPS client configuration from fixed `*tls.Config` to `DialTLSContext` for per-connection TLS resolution.
services/subscriber/config.go	Adds `TLSManagerOpts()` to centralize mapping from subscriber config to TLS manager options.
services/opentsdb/service.go	Switches to `NewServerTLSConfigManager` and reconfigure-based reload.
services/opentsdb/service_test.go	Updates tests for new service constructor signature and cert monitor usage.
services/httpd/service.go	Switches to `NewServerTLSConfigManager` and reconfigure-based reload; injects cert monitor.
services/httpd/service_test.go	Updates httpd tests to provide a cert monitor to the service.
services/httpd/config_test.go	Updates config test to construct httpd service with cert monitor.
pkg/tlsconfig/tls_config_test.go	Adds tests for parsing/validating TLS cipher/version config behavior.
pkg/tlsconfig/loadcert.go	Extracts/introduces a `LoadCertificate` helper with permission checks and richer certificate metadata.
pkg/tlsconfig/loadcert_test.go	Adds tests for `LoadCertificate`, loaded-certificate helpers, and expiration/prematurity logging behavior.
pkg/tlsconfig/configmanager.go	Major refactor: role-aware managers, separate server/client loaders, `DialContext`, and `PrepareReconfigure`.
pkg/tlsconfig/clientcert_test.go	Adds tests for separate client certificate behavior and reconfiguration semantics.
pkg/tlsconfig/certmonitor.go	Introduces `TLSCertMonitor` to batch and deduplicate certificate issue warnings across loaders.
pkg/tlsconfig/certmonitor_test.go	Adds tests for monitor defaults, trigger behavior, deduplication, and shutdown safety.
pkg/tlsconfig/certconfig.go	Refactors `TLSCertLoader` to integrate with the shared monitor, roles, and new load/reconfigure flow.
pkg/tlsconfig/certconfig_test.go	Updates cert loader tests to use the new monitor/role-based APIs and new logging behavior.
pkg/testing/helper/helpers.go	Makes `CheckedClose` fail fast on nil closers to improve test safety.
cmd/influxd/run/server.go	Adds a shared `TLSCertMonitor` to influxd server lifecycle and threads it into services; updates subscriber reload call.
client/v2/client.go	Adds support for `DialTLSContext` on the HTTP transport so TLS can be resolved per connection (non-proxied HTTPS).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

gwossum · 2026-07-15T21:22:48Z

 	// Reload the subscriber's client TLS certificate at its currently
 	// configured path so rotated certificates are picked up.
 	if s.Subscriber != nil {
-		if af, err := s.Subscriber.PrepareReloadTLSCertificates(); err == nil {
+		if af, err := s.Subscriber.PrepareReloadTLSCertificates(config.Subscriber); err == nil {


Comment will be updated in next push.

- Add support for full httpd TLS configuration reloading. - Add usage to production TLS managers for improved logging. - Fix some existing test issues.

- Server certificates must now have the server use EKU set. - Added options to turn the sanity check errors into warnings.

- Add checks for client certificate client usage EKU. This check is more subtle than the server certificate server usage EKU check in order to avoiding currently working configurations.

devanbenz

A few comments so far. Still working through the PR.

devanbenz · 2026-07-15T19:13:02Z

+	m.queuedCertLoaders = append(m.queuedCertLoaders, cl)
+	m.mu.Unlock()
+
+	m.resetTrigger(m.triggerDelayInterval)


Is it okay that triggerDelayInterval is read outside of a mutex? https://github.com/influxdata/influxdb/pull/27543/changes#diff-311ffb824cadc6a7531617ae06abf39e0920666d6336ec3184814784adf3ee02R264 locks it during modification. Should we just do a defer unlock here?

Good catch! The lock is purposely released before sending on a channel to avoid a potential deadlock, which is why defer is not used. Normally I'd make a lambda for it, but I didn't like the way it looked when I was coding it. I've pushed changes which capture the mutex protected while the lock is held to avoid this.

This has been fixed and pushed.

devanbenz · 2026-07-15T19:14:25Z

+	m.certExpirationAdvanced = expirationAdvanced
+	m.mu.Unlock()
+
+	m.resetTrigger(m.triggerDelayInterval)


Same here: https://github.com/influxdata/influxdb/pull/27543/changes#r3590094540

Just pushed a fix and comment explaining why the channel send is done outside the lock.

devanbenz · 2026-07-15T19:22:05Z

 // can be replaced by:
 // defer CheckedClose(t, c)()
 func CheckedClose(t require.TestingT, c io.Closer) func() {
+	require.NotNil(t, c, "CheckedClose cannot be used with a nil io.Closer")


You'll never guess why I added it 😅

I feel seen

devanbenz · 2026-07-15T19:35:23Z

+	m.certCheckInterval = checkInterval
+	m.mu.Unlock()
+
+	m.resetCheckInterval(checkInterval)


Same here: https://github.com/influxdata/influxdb/pull/27543/changes#r3590094540

Pushed fix and comment.

Fixed NewDisabledTLSConfigManager so that the returned TLSConfigManager can both dial and listen plaintext.

- Fix mutex protected fields access without lock. - Add comments explaining why trigger channel sends occur outside of lock.

Renamed config options `ignore-sanity-checks` for subscriber and opentsdb services to `ignore-cert-sanity-checks`. This makes it obvious the sanity checks being ignored are for certificates and not general sanity checks. This naming matches other projects.

davidby-influx

Partial review to unblock you

davidby-influx · 2026-07-15T18:57:28Z

 		require.NotNil(t, x509Cert)
 		require.NotNil(t, x509Cert.SerialNumber)
 		require.Equal(t, goodSerial, *x509Cert.SerialNumber)
+


TODO is a standard programming convention and has built-in support in many tools, including GoLand (https://www.jetbrains.com/help/go/using-todo.html)

sorry, I was unclear. I meant, should we do this TODO?

davidby-influx · 2026-07-15T20:02:35Z

 		require.WithinDuration(t, notAfter, timeExpires, 2*time.Minute, "untilExpires varies more than expected")
 		logs.TakeAll() // dump all logs
 	}
+	time.Sleep(testWarnWaitTime)


I'm always worried about arbitrary waiting in tests, which is in a few places in here. No simple trigger available (without production code changes)?

This wait is required because the actually logging is queued up in another goroutine. The timeout on other goroutien is set to 0 should it should be almost instantaneous. I haven't had any failures yet.

davidby-influx · 2026-07-15T23:39:58Z

+
+// monitor periodically logs certificate errors with the currently registered
+// TLSCertLoader objects.
+func (m *TLSCertMonitor) monitorCerts(startWg *sync.WaitGroup, stopWg *sync.WaitGroup) {


Are these waitgroups in the struct?

They are. Passing them into the goroutine like this is a habit I've gotten into because it has some advantages in some places. Not here, though.

davidby-influx

Another partial review to keep you unblocked

davidby-influx · 2026-07-16T18:49:43Z


-	return err
+	c := &tlsCertLoaderConfig{}
+	*c = *cl.config


Any need for a deeper copy?

It's all strings, bools, and a pointer to the logger. No need for a deep copy. After needing a deep copy was going to cause me issues with the tlsConfigManagerConfig type, I made a conscious decision to make the config structs so they did not require deep copies, so they don't keep any slices or structs that are directly modified. It's all primitive data types and pointers to structs that are either conserved or completely overwritten.

davidby-influx · 2026-07-16T19:17:25Z

+
+	require.NoError(t, m.Close())
+	require.NoError(t, m.Close())
+	m.WaitForMonitorStop()


Should the WaitForMonitor be in between the two Close calls to show only one routine was started?

This is a test for idempotency. If we really wanted that test, it should be added to another test case. TestTLSCertMonitor_ClosedMonitorDoesNotBlock does kind of test that. I'm not too worried about starting up multiple goroutines because sync.Once is used to ensure we only startup one goroutine.

davidby-influx · 2026-07-16T19:43:38Z

+
+	if xc.IsExpired() {
+		log.Warn("Certificate is expired", zap.Time("NotAfter", xc.NotAfter))
+	} else if xc.IsPremature() {


Do we want to log all the issues, not just the first encountered (i.e., eliminate the elses)?

A certificate cannot simultaneously be not valid yet, about to expire, and expired.

davidby-influx · 2026-07-16T20:10:39Z

+	if err != nil {
+		return err
+	}
+	defer conn.Close()


you know what I want here... The magic of CheckedClose!

Or the error capture from the defer into the return value.

That might be better

This is one of those situations where the failed handshake maybe closes the connection before you get to this Close, so you might get no error or an unpredictable error. This error doesn't seem to have any TLS indication, so the best we can do is say Close should not return an error if the overall handshake function does not return an error. This will be in the next push.

- Improve error checking in tests. - Use th.CheckedClose in more places. - Fix some spelling mistakes.

davidby-influx

One pass through all files. But I see changes have been made

davidby-influx · 2026-07-16T21:03:13Z

+// multiple times, but it will only start one monitor goroutine.
+func (m *TLSCertMonitor) Open() error {
+	m.startOnce.Do(func() {
+		go m.monitorCerts(&m.monitorStartWg, &m.monitorStopWg)


why pass in members of the receiver? do we ever use externa, ones (in tests, perhaps)?

Habit. Sometimes it helps decouple things, but not in this case since the goroutine still accesses m for other reasons.

davidby-influx · 2026-07-16T21:17:18Z

+	c.IgnoreSignals = true
+	c.ForceTTY = true
+
+	require.Error(t, c.Run())


Do we know anything about the error?

Fixed in latest push

davidby-influx

Super close! Quite the body of work!

I will leave this for @devanbenz to follow up on any more changes.

davidby-influx · 2026-07-16T21:44:46Z

 		require.NotNil(t, x509Cert)
 		require.NotNil(t, x509Cert.SerialNumber)
 		require.Equal(t, goodSerial, *x509Cert.SerialNumber)
+


sorry, I was unclear. I meant, should we do this TODO?

gwossum added 4 commits July 14, 2026 23:08

chore: code cleanups and comment fixes

0a1fd24

chore: add tests and improve existing tests

02cab0b

gwossum self-assigned this Jul 15, 2026

gwossum added area/configuration 1.x labels Jul 15, 2026

chore: code cleanup

cc846f5

Remove unused and misleading *tls.Config parameter from NewHTTPS.

davidby-influx requested a review from Copilot July 15, 2026 16:10

Copilot started reviewing on behalf of davidby-influx July 15, 2026 16:10 View session

Copilot AI reviewed Jul 15, 2026

View reviewed changes

gwossum added 3 commits July 15, 2026 11:59

feat: httpd service supports full TLS config reloading

24e28e5

- Add support for full httpd TLS configuration reloading. - Add usage to production TLS managers for improved logging. - Fix some existing test issues.

feat: add server certificate sanity checking.

6b1012a

- Server certificates must now have the server use EKU set. - Added options to turn the sanity check errors into warnings.

feat: add client certificate sanity checks

f77fdde

- Add checks for client certificate client usage EKU. This check is more subtle than the server certificate server usage EKU check in order to avoiding currently working configurations.

devanbenz requested changes Jul 15, 2026

View reviewed changes

gwossum added 5 commits July 15, 2026 16:07

fix: disabled TLSConfigManager can dial and listen

a88759a

Fixed NewDisabledTLSConfigManager so that the returned TLSConfigManager can both dial and listen plaintext.

chore: fix issues with comments

ebbd11a

chore: fix compilation issue, slight test improvement

8c05930

fix: mutex protected fields accessed without lock

1784768

- Fix mutex protected fields access without lock. - Add comments explaining why trigger channel sends occur outside of lock.

davidby-influx requested changes Jul 15, 2026

View reviewed changes

fix: logger safety in TLSCertMonitor

9bec655

davidby-influx reviewed Jul 16, 2026

View reviewed changes

gwossum added 3 commits July 16, 2026 16:07

feat: add mTLS support to influx CLI command

3de4c7f

chore: update man page for influx CLI command

5ab7d5c

chore: test improvements and fix spelling mistakes

69c311a

- Improve error checking in tests. - Use th.CheckedClose in more places. - Fix some spelling mistakes.

davidby-influx reviewed Jul 16, 2026

View reviewed changes

chore: test improvements

630d224

Uh oh!

Conversation

gwossum commented Jul 15, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devanbenz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

davidby-influx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidby-influx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment