-
Notifications
You must be signed in to change notification settings - Fork 4
SRVOCF-1052: Reload serving TLS certificate at runtime #173
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,78 @@ | ||||||
| package tlsreload | ||||||
|
|
||||||
| import ( | ||||||
| "crypto/sha256" | ||||||
| "crypto/tls" | ||||||
| "fmt" | ||||||
| "log/slog" | ||||||
| "os" | ||||||
| "sync/atomic" | ||||||
| "time" | ||||||
| ) | ||||||
|
|
||||||
| const certificateReloadInterval = 5 * time.Minute | ||||||
|
|
||||||
| type Reloader struct { | ||||||
| certFile string | ||||||
| keyFile string | ||||||
| current atomic.Pointer[tls.Certificate] | ||||||
| lastHash [sha256.Size]byte | ||||||
| } | ||||||
|
|
||||||
| func New(certFile, keyFile string) (*Reloader, error) { | ||||||
| reloader := &Reloader{certFile: certFile, keyFile: keyFile} | ||||||
| if err := reloader.reload(); err != nil { | ||||||
| return nil, err | ||||||
| } | ||||||
| return reloader, nil | ||||||
| } | ||||||
|
|
||||||
| func (r *Reloader) reload() error { | ||||||
| certPEM, err := os.ReadFile(r.certFile) | ||||||
| if err != nil { | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This condition has no test. |
||||||
| return fmt.Errorf("read TLS certificate: %w", err) | ||||||
| } | ||||||
| keyPEM, err := os.ReadFile(r.keyFile) | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This condition has no test. |
||||||
| if err != nil { | ||||||
| return fmt.Errorf("read TLS key: %w", err) | ||||||
| } | ||||||
|
|
||||||
| // Skip the parse and swap when the on-disk pair is unchanged. | ||||||
| digest := sha256.New() | ||||||
| digest.Write(certPEM) | ||||||
| digest.Write(keyPEM) | ||||||
| var hash [sha256.Size]byte | ||||||
| digest.Sum(hash[:0]) | ||||||
| if r.current.Load() != nil && hash == r.lastHash { | ||||||
| return nil | ||||||
| } | ||||||
|
|
||||||
| cert, err := tls.X509KeyPair(certPEM, keyPEM) | ||||||
| if err != nil { | ||||||
| return fmt.Errorf("load TLS certificate: %w", err) | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Otherwise the log from main.go reloader, err := tlsreload.New(*certFile, *keyFile)
if err != nil {
log.Fatalf("Failed to load TLS certificate: %v", err)
}will result in an akward error message like |
||||||
| } | ||||||
| r.current.Store(&cert) | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't |
||||||
| r.lastHash = hash | ||||||
|
|
||||||
| attrs := []any{"certFile", r.certFile} | ||||||
| if cert.Leaf != nil { | ||||||
| attrs = append(attrs, "notAfter", cert.Leaf.NotAfter) | ||||||
| } | ||||||
| slog.Info("loaded TLS certificate", attrs...) | ||||||
| return nil | ||||||
| } | ||||||
|
|
||||||
| func (r *Reloader) Run() { | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We probably should have graceful shutdown here (and in main.go!). The change here is small, can you add it? func (r *Reloader) Run(ctx context.Context) {
ticker := time.NewTicker(certificateReloadInterval)
defer ticker.Stop()
for {
select {
case <-ctx.Done():
return
case <-ticker.C:
if err := r.reload(); err != nil {
slog.Error("failed to reload TLS certificate", "err", err)
}
}
}
}And for main.go -> pls create a ticket. |
||||||
| ticker := time.NewTicker(certificateReloadInterval) | ||||||
| defer ticker.Stop() | ||||||
|
|
||||||
| for range ticker.C { | ||||||
| if err := r.reload(); err != nil { | ||||||
| slog.Error("failed to reload TLS certificate", "err", err) | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| func (r *Reloader) GetCertificate(_ *tls.ClientHelloInfo) (*tls.Certificate, error) { | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a public method and Reloader can be initialized without using New -> so this can break. I'd add correct error handling here. And when this methods gets some logic it'll need test coverage. |
||||||
| return r.current.Load(), nil | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,100 @@ | ||
| package tlsreload | ||
|
|
||
| import ( | ||
| "crypto/rand" | ||
| "crypto/rsa" | ||
| "crypto/tls" | ||
| "crypto/x509" | ||
| "crypto/x509/pkix" | ||
| "encoding/pem" | ||
| "math/big" | ||
| "os" | ||
| "path/filepath" | ||
| "time" | ||
|
|
||
| . "github.com/onsi/ginkgo/v2" | ||
| . "github.com/onsi/gomega" | ||
| ) | ||
|
|
||
| var _ = Describe("Reloader", func() { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pls test the package using it's public api, not internal methods. |
||
| It("uses the certificate loaded after a rotation", func() { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can add a test case for when the certs were deleted and are missing. |
||
| dir := GinkgoT().TempDir() | ||
| certFile := filepath.Join(dir, "tls.crt") | ||
| keyFile := filepath.Join(dir, "tls.key") | ||
|
|
||
| writeCertificatePair(certFile, keyFile, "old.example.com") | ||
| reloader, err := New(certFile, keyFile) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| writeCertificatePair(certFile, keyFile, "new.example.com") | ||
| Expect(reloader.reload()).To(Succeed()) | ||
|
|
||
| cert, err := reloader.GetCertificate(nil) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(certificateCommonName(cert)).To(Equal("new.example.com")) | ||
| }) | ||
|
|
||
| It("keeps the current certificate when the replacement is invalid", func() { | ||
| dir := GinkgoT().TempDir() | ||
| certFile := filepath.Join(dir, "tls.crt") | ||
| keyFile := filepath.Join(dir, "tls.key") | ||
|
|
||
| writeCertificatePair(certFile, keyFile, "current.example.com") | ||
| reloader, err := New(certFile, keyFile) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| Expect(os.WriteFile(certFile, []byte("invalid certificate"), 0600)).To(Succeed()) | ||
| Expect(reloader.reload()).To(MatchError(ContainSubstring("load TLS certificate"))) | ||
|
|
||
| cert, err := reloader.GetCertificate(nil) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(certificateCommonName(cert)).To(Equal("current.example.com")) | ||
| }) | ||
|
|
||
| It("skips reparsing when the certificate pair is unchanged", func() { | ||
| dir := GinkgoT().TempDir() | ||
| certFile := filepath.Join(dir, "tls.crt") | ||
| keyFile := filepath.Join(dir, "tls.key") | ||
|
|
||
| writeCertificatePair(certFile, keyFile, "stable.example.com") | ||
| reloader, err := New(certFile, keyFile) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| before, err := reloader.GetCertificate(nil) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| Expect(reloader.reload()).To(Succeed()) | ||
|
|
||
| after, err := reloader.GetCertificate(nil) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(after).To(BeIdenticalTo(before)) | ||
| }) | ||
| }) | ||
|
|
||
| func writeCertificatePair(certFile, keyFile, commonName string) { | ||
| key, err := rsa.GenerateKey(rand.Reader, 2048) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| template := &x509.Certificate{ | ||
| SerialNumber: big.NewInt(1), | ||
| Subject: pkix.Name{CommonName: commonName}, | ||
| DNSNames: []string{commonName}, | ||
| NotBefore: time.Now().Add(-time.Minute), | ||
| NotAfter: time.Now().Add(time.Hour), | ||
| KeyUsage: x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature, | ||
| ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, | ||
| } | ||
| der, err := x509.CreateCertificate(rand.Reader, template, template, &key.PublicKey, key) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| certPEM := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: der}) | ||
| keyPEM := pem.EncodeToMemory(&pem.Block{Type: "RSA PRIVATE KEY", Bytes: x509.MarshalPKCS1PrivateKey(key)}) | ||
| Expect(os.WriteFile(certFile, certPEM, 0600)).To(Succeed()) | ||
| Expect(os.WriteFile(keyFile, keyPEM, 0600)).To(Succeed()) | ||
| } | ||
|
|
||
| func certificateCommonName(cert *tls.Certificate) string { | ||
| parsed, err := x509.ParseCertificate(cert.Certificate[0]) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| return parsed.Subject.CommonName | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| package tlsreload | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| . "github.com/onsi/ginkgo/v2" | ||
| . "github.com/onsi/gomega" | ||
| ) | ||
|
|
||
| func TestTLSReload(t *testing.T) { | ||
| RegisterFailHandler(Fail) | ||
| RunSpecs(t, "TLS Reload Suite") | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be configurable, so that we don't need a new commit to change it.