Skip to content

feat(tls): add certificate hot-reload for gNMI target connections - #964

Open
bradrevans wants to merge 7 commits into
openconfig:mainfrom
bradrevans:main
Open

feat(tls): add certificate hot-reload for gNMI target connections#964
bradrevans wants to merge 7 commits into
openconfig:mainfrom
bradrevans:main

Conversation

@bradrevans

Copy link
Copy Markdown

Motivation

In large deployments with thousands of unique targets, periodically rotating client TLS certificates or CA bundles currently requires a full restart of the gnmic collector. This forces an unnecessary connectivity drop across all targets, even if only a small subset of credentials needed updating.

Solution

This PR introduces zero-downtime, mtime-based TLS hot-reloading for target connections. Because gNMI connections are long-lived, when a certificate expires or is rotated on disk, the new material is seamlessly picked up on the next TLS handshake/reconnect without needing to bounce the collector process.

Technical Implementation

  • TLS Callbacks: Replaced the static tls.X509KeyPair and CA pool assignments in utils.NewTLSConfig with Go's standard dynamic callbacks (GetClientCertificate, GetCertificate, VerifyPeerCertificate).
  • Zero I/O Cache: Added certReloader and caReloader to pkg/api/utils/tls.go. During a handshake, these helpers check os.Stat on the cert/key/ca files. If the mtime hasn't changed, they instantly return the in-memory parsed certificate (imposing zero disk I/O overhead on reconnects).
  • Safe Fallback: If a file has been modified but is unreadable or malformed (e.g., caught mid-write during rotation), the reloader catches the parsing error and safely falls back to the last known-good certificate. The system continues operating and will naturally try reading the new file again on the next reconnect cycle.

Configuration Impacts

  • Added a tls-reload global config option and --tls-reload CLI flag (enabled by default).
  • Added tls-reload to TargetConfig so users can explicitly opt-out specific legacy targets if desired.
  • Preserved Existing Behavior: All other internal components relying on utils.NewTLSConfig (e.g., outputs, inputs, loaders, apiserver) have been explicitly updated to pass false to the new hotReload parameter. This guarantees their static TLS behavior remains entirely unchanged, avoiding any unintended side effects outside of target connections.

Testing & Documentation

  • Added comprehensive tests in tls_test.go to validate thread-safety under heavy concurrent reconnects, mtime rotation logic, and mid-write garbage file fallbacks.
  • Updated docs/global_flags.md, docs/user_guide/targets/targets.md, and targets_session_sec.md to document the new behavior.

Side Notes

This is my first ever contribution to this project so my apologies if I've not followed the process properly and I would appreciate any feedback/recommendations in order to see this capability included.

@bradrevans
bradrevans marked this pull request as ready for review September 3, 2026 22:11
@bradrevans
bradrevans marked this pull request as draft September 4, 2026 08:34
@bradrevans
bradrevans marked this pull request as ready for review September 4, 2026 08:58
@bradrevans

Copy link
Copy Markdown
Author

Additionally updated targets_test.go with amended TargetConfig that includes TLSReload in order to address failing tests.

@karimra

karimra commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

Thanks for this, zero-downtime cert rotation is something large deployments have asked for, and the cert/key part of this is in good shape.
A few changes are needed on the CA reload side before merging:

1) CA verification

VerifyPeerCertificate runs after Go's built-in chain verification, and only if that step succeeded. In crypto/tls (handshake_client.go, verifyServerCertificate) the order is:

  1. verify the leaf against config.RootCAs with DNSName = ServerName
  2. VerifyPeerCertificate
  3. VerifyConnection

If step 1 fails, the handshake aborts and neither callback runs. RootCAs is still the pool loaded at startup, so a target that presents a cert signed by a new CA fails step 1 before caReloader is ever consulted. The reloader can enforce a CA that was removed from the bundle, but it cannot trust one that was added, which is the case operators need during a CA rotation.

To make CA reload real, the built-in check has to be replaced:

// only when the user did NOT set skip-verify
tlsConfig.InsecureSkipVerify = true
tlsConfig.VerifyConnection = func(cs tls.ConnectionState) error {
    if len(cs.PeerCertificates) == 0 {
        return errors.New("no peer certificate")
    }
    opts := x509.VerifyOptions{
        Roots:         caR.getPool(),
        DNSName:       cs.ServerName,
        Intermediates: x509.NewCertPool(),
    }
    for _, c := range cs.PeerCertificates[1:] {
        opts.Intermediates.AddCert(c)
    }
    _, err := cs.PeerCertificates[0].Verify(opts)
    return err
}

InsecureSkipVerify plus a callback that does the full verification is the pattern the standard library documents for this ("should be used only for testing or in combination with VerifyConnection or VerifyPeerCertificate"). Three things to get right:

  • DNSName must be set. The current callback omits it, which is harmless today only because step 1 still runs. Once step 1 is disabled, omitting it silently turns off hostname verification.
  • Use VerifyConnection, not VerifyPeerCertificate. VerifyPeerCertificate is not called on resumed sessions (see the WARNING in its doc comment and Go issue 31641).
    gNMI connections are long-lived and reconnect with session tickets, so a CA that was removed from the bundle would go unenforced on exactly the reconnect you care about. VerifyConnection runs on every connection, including resumptions, and also hands you the parsed PeerCertificates and ServerName so there is no manual DER parsing.
  • The user-facing skip-verify option and this internal flag share tlsConfig.InsecureSkipVerify. Only install the callback when the user did not ask for skip-verify, and leave a comment so nobody later reads that field to decide whether verification is on.

If you would rather keep the scope small for a first PR, the alternative is to drop caReloader entirely, keep the cert/key reloader, and have the docs say that only tls-cert/tls-key are reloaded. Either is fine with me. What I do not want to merge is CA reload that works in one direction only.

2) Default should be off for now

The flag defaults to true, so every target with a client cert changes behavior on
upgrade. I would rather ship this opt-in for one release and flip the default once a few
deployments have run it. The per-target override is good, keep that.

@bradrevans

Copy link
Copy Markdown
Author

Thanks for this, zero-downtime cert rotation is something large deployments have asked for, and the cert/key part of this is in good shape. A few changes are needed on the CA reload side before merging:

1) CA verification

VerifyPeerCertificate runs after Go's built-in chain verification, and only if that step succeeded. In crypto/tls (handshake_client.go, verifyServerCertificate) the order is:

  1. verify the leaf against config.RootCAs with DNSName = ServerName
  2. VerifyPeerCertificate
  3. VerifyConnection

If step 1 fails, the handshake aborts and neither callback runs. RootCAs is still the pool loaded at startup, so a target that presents a cert signed by a new CA fails step 1 before caReloader is ever consulted. The reloader can enforce a CA that was removed from the bundle, but it cannot trust one that was added, which is the case operators need during a CA rotation.

To make CA reload real, the built-in check has to be replaced:

// only when the user did NOT set skip-verify
tlsConfig.InsecureSkipVerify = true
tlsConfig.VerifyConnection = func(cs tls.ConnectionState) error {
    if len(cs.PeerCertificates) == 0 {
        return errors.New("no peer certificate")
    }
    opts := x509.VerifyOptions{
        Roots:         caR.getPool(),
        DNSName:       cs.ServerName,
        Intermediates: x509.NewCertPool(),
    }
    for _, c := range cs.PeerCertificates[1:] {
        opts.Intermediates.AddCert(c)
    }
    _, err := cs.PeerCertificates[0].Verify(opts)
    return err
}

InsecureSkipVerify plus a callback that does the full verification is the pattern the standard library documents for this ("should be used only for testing or in combination with VerifyConnection or VerifyPeerCertificate"). Three things to get right:

  • DNSName must be set. The current callback omits it, which is harmless today only because step 1 still runs. Once step 1 is disabled, omitting it silently turns off hostname verification.
  • Use VerifyConnection, not VerifyPeerCertificate. VerifyPeerCertificate is not called on resumed sessions (see the WARNING in its doc comment and Go issue 31641).
    gNMI connections are long-lived and reconnect with session tickets, so a CA that was removed from the bundle would go unenforced on exactly the reconnect you care about. VerifyConnection runs on every connection, including resumptions, and also hands you the parsed PeerCertificates and ServerName so there is no manual DER parsing.
  • The user-facing skip-verify option and this internal flag share tlsConfig.InsecureSkipVerify. Only install the callback when the user did not ask for skip-verify, and leave a comment so nobody later reads that field to decide whether verification is on.

If you would rather keep the scope small for a first PR, the alternative is to drop caReloader entirely, keep the cert/key reloader, and have the docs say that only tls-cert/tls-key are reloaded. Either is fine with me. What I do not want to merge is CA reload that works in one direction only.

2) Default should be off for now

The flag defaults to true, so every target with a client cert changes behavior on upgrade. I would rather ship this opt-in for one release and flip the default once a few deployments have run it. The per-target override is good, keep that.

Thanks so much for the incredibly detailed review - I really appreciate you taking the time and providing the VerifyConnection boilerplate.

I've pushed two new commits to address everything you mentioned:

  1. CA Verification via VerifyConnection:
  • I've swapped VerifyPeerCertificate for VerifyConnection so we evaluate the full chain manually on every handshake, including session ticket resumptions.
  • Internally, when tls-reload is true and the user didn't explicitly request to skip verify, we now enable InsecureSkipVerify to bypass the static Go check, allowing our callback to take full ownership using the dynamic caReloader pool and cs.ServerName for DNS validation.
  1. Conservative Defaults:
  • The --tls-reload global flag and target configuration default have been flipped to false.
  • I updated the global flags and session security documentation to reflect that this is now an opt-in feature.

Please let me know if there's anything else you'd like me to tweak!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants