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

Dynamically reload cert/keys if changed on disk #1595

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 10 additions & 8 deletions rtls/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,16 @@ const (
// should appear in the returned certificate. If noverify is true, the client will not verify
// the server's certificate.
func CreateClientConfig(certFile, keyFile, caCertFile, serverName string, noverify bool) (*tls.Config, error) {
var err error

config := createBaseTLSConfig(serverName, noverify)
if certFile != "" && keyFile != "" {
config.Certificates = make([]tls.Certificate, 1)
config.Certificates[0], err = tls.LoadX509KeyPair(certFile, keyFile)
mc, err := newMonitoredCertificate(certFile, keyFile)
if err != nil {
return nil, err
}
config.GetClientCertificate = func(info *tls.CertificateRequestInfo) (*tls.Certificate, error) {
// Get() will never return nil
return mc.Get(), nil
}
}
if caCertFile != "" {
asn1Data, err := os.ReadFile(caCertFile)
Expand All @@ -61,14 +62,15 @@ func CreateClientConfig(certFile, keyFile, caCertFile, serverName string, noveri
// client. If mtls is MTLSStateEnabled, the server will require the client to present a
// valid certificate.
func CreateServerConfig(certFile, keyFile, caCertFile string, mtls MTLSState) (*tls.Config, error) {
var err error

config := createBaseTLSConfig(NoServerName, false)
config.Certificates = make([]tls.Certificate, 1)
config.Certificates[0], err = tls.LoadX509KeyPair(certFile, keyFile)
mc, err := newMonitoredCertificate(certFile, keyFile)
if err != nil {
return nil, err
}
config.GetCertificate = func(info *tls.ClientHelloInfo) (*tls.Certificate, error) {
// Get() will never return nil
return mc.Get(), nil
}
if caCertFile != "" {
asn1Data, err := os.ReadFile(caCertFile)
if err != nil {
Expand Down
83 changes: 83 additions & 0 deletions rtls/monitor.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
package rtls

import (
"crypto/tls"
"log"
"os"
"sync/atomic"
"time"
)

// monitorFiles watches the given files for changes, calling the supplied callback when a
// change is detected. The callback is passed a slice of the files that changed. The loop
// never terminates, so it's expected the caller will invoke this in a goroutine.
func monitorFiles(files []string, cb func([]string)) {
mtimes := make([]time.Time, len(files))
for {
var changed []string
for n, file := range files {
info, err := os.Stat(file)
if err != nil {
continue
}
if !mtimes[n].IsZero() && info.ModTime() != mtimes[n] {
changed = append(changed, file)
}
mtimes[n] = info.ModTime()
}
if changed != nil {
cb(changed)
}
time.Sleep(2 * time.Second)
Copy link
Member

Choose a reason for hiding this comment

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

This is not idiomatic Go. You should use the ticker pattern.

https://gobyexample.com/tickers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can adapt it easily enough. Although it's not clear to me what value using a ticker and channel will provide over sleep and callback in this case. A ticker makes sense when you want to select on multiple things (most notably a context), but in this case it was literally just a "do a thing every interval T."

Also, this could could be quite racy, since I don't see any read-write synchronization being done.

It uses an atomic.Value to hold a scalar (a pointer to tls.Certificate).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would expect to see use of the "done" channel pattern.

What is it that will signal doneness?

In this case there's no cleanup activity for the goroutine to do, it's expected to run continuously until program exit, and it doesn't impede rqlited's ability to terminate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be clear, I'm not objecting to making the changes, just asking to be administered the cluebat a little bit more. :)

}
}

// monitoredCertificate watches the given certificate and key files for changes, reloading
// the certificate if a change is detected.
type monitoredCertificate struct {
certFile string
keyFile string
// This holds a *tls.Certificate which can be updated from another goroutine, so use
// an atomic value to synchronize access
cert atomic.Value
}

// newMonitoredCertificate loads the certificate and key and spawns a goroutine to watch
// the files for changes.
func newMonitoredCertificate(certFile, keyFile string) (*monitoredCertificate, error) {
mc := &monitoredCertificate{
certFile: certFile,
keyFile: keyFile,
}
// Prime the cached certificate and induce any errors immediately
if err := mc.load(); err != nil {
return nil, err
}
// Now start watching for changes
go monitorFiles([]string{certFile, keyFile}, func(changed []string) {
if err := mc.load(); err != nil {
log.Printf("error reloading certificate %s, not replacing: %s", certFile, err)
} else {
log.Printf("reloaded certificate %s", certFile)
}
})
return mc, nil
}

// load reads the certificate and key files from disk and caches the result for Get()
func (mc *monitoredCertificate) load() error {
cert, err := tls.LoadX509KeyPair(mc.certFile, mc.keyFile)
Copy link

Choose a reason for hiding this comment

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

Question, does this safely fails if one file changed but the other still didn't?

For example, what happens if the cert file changes and there is some random slowness until the key file gets also written? Before loading them it could be wise to have a delay, unless LoadX509KeyPair fails if there is any kind of mismatch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unless LoadX509KeyPair fails if there is any kind of mismatch

Exactly. If the key doesn't match the cert, LoadX509KeyPair() returns an error and the active cert/key in memory won't be replaced. When the other file gets updated (or whatever operational issue is resolved), then this will get re-executed and at that time.

Copy link

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

Oh great then, I should find the equivalent for LoadX509KeyPair in Node.js, thanks!

if err != nil {
return err
}
mc.cert.Store(&cert)
return nil
}

// Get returns the certificate.
//
// This will never return nil provided the monitoredCertificate object was created via
// newMonitoredCertificate(),
func (mc *monitoredCertificate) Get() *tls.Certificate {
return mc.cert.Load().(*tls.Certificate)
}