diff --git a/operator/internal/controller/cert_utils.go b/operator/internal/controller/cert_utils.go index abe50762b..ad7f29fb4 100644 --- a/operator/internal/controller/cert_utils.go +++ b/operator/internal/controller/cert_utils.go @@ -30,6 +30,7 @@ import ( "math/big" "os" "path/filepath" + "sync" "time" "github.com/NVIDIA/nodewright/operator/api/v1alpha1" @@ -243,7 +244,13 @@ func ensureDummyCert(certDir string) error { return nil } +// certWriteMu serializes writeCertAndKey. It is package-level because the writers +// (webhook bootstrap reconcile and SecretCertWatcher) share no struct, only the +// process and the cert directory. +var certWriteMu sync.Mutex + // Write cert and key to disk if CertDir is set, make sure the directory exists, and the files are 600. Will also sync from the secret to the disk. +// Writes are serialized across goroutines and each file is replaced atomically (temp file + rename). func writeCertAndKey(certPEM, keyPEM []byte, certDir string) error { if certDir == "" { return fmt.Errorf("certDir is required") @@ -254,15 +261,59 @@ func writeCertAndKey(certPEM, keyPEM []byte, certDir string) error { certPath := filepath.Join(certDir, "tls.crt") keyPath := filepath.Join(certDir, "tls.key") - if err := os.WriteFile(certPath, certPEM, 0o600); err != nil { + + // Serialize writers so a cert write and a key write from two goroutines + // (bootstrap reconcile and SecretCertWatcher) can never interleave and + // leave tls.crt from one cert next to tls.key from another. + certWriteMu.Lock() + defer certWriteMu.Unlock() + + if err := writeFileAtomic(certPath, certPEM, 0o600); err != nil { return fmt.Errorf("failed to write cert: %w", err) } - if err := os.WriteFile(keyPath, keyPEM, 0o600); err != nil { + if err := writeFileAtomic(keyPath, keyPEM, 0o600); err != nil { return fmt.Errorf("failed to write key: %w", err) } return nil } +// writeFileAtomic writes data to a temp file in the same directory as path and +// renames it into place, so readers see either the old file or the new one in +// full, never a partially written file. The temp file lives next to path so the +// rename stays on one filesystem (rename is only atomic within a filesystem). +func writeFileAtomic(path string, data []byte, perm os.FileMode) error { + tmp, err := os.CreateTemp(filepath.Dir(path), "."+filepath.Base(path)+".tmp-*") + if err != nil { + return err + } + tmpName := tmp.Name() + + if _, err := tmp.Write(data); err != nil { + _ = tmp.Close() + _ = os.Remove(tmpName) + return err + } + if err := tmp.Chmod(perm); err != nil { + _ = tmp.Close() + _ = os.Remove(tmpName) + return err + } + if err := tmp.Sync(); err != nil { + _ = tmp.Close() + _ = os.Remove(tmpName) + return err + } + if err := tmp.Close(); err != nil { + _ = os.Remove(tmpName) + return err + } + if err := os.Rename(tmpName, path); err != nil { + _ = os.Remove(tmpName) + return err + } + return nil +} + // certExistsOnDisk returns an error if the cert or key is not present on disk func certExistsOnDisk(certDir string) error { certPath := filepath.Join(certDir, "tls.crt") diff --git a/operator/internal/controller/webhook_controller_test.go b/operator/internal/controller/webhook_controller_test.go index 5663862d6..88268dab0 100644 --- a/operator/internal/controller/webhook_controller_test.go +++ b/operator/internal/controller/webhook_controller_test.go @@ -933,6 +933,33 @@ var _ = Describe("WebhookController", Ordered, func() { Expect(err).NotTo(HaveOccurred()) Expect(updated).To(BeFalse()) }) + + It("should return an error and leave the Secret untouched when the cert cannot be written to disk", func() { + diskCert, err := generateCert(serviceName, namespace, certValidityDurationYear) + Expect(err).NotTo(HaveOccurred()) + secretCert, err := generateCert(serviceName, namespace, certValidityDurationYear) + Expect(err).NotTo(HaveOccurred()) + + secret := secretCert.ToSecret(secretName, namespace, serviceName) + controller.Client = fake.NewClientBuilder().WithObjects(secret).Build() + + // Disk holds a different cert than the Secret, so rotation is triggered. + err = writeCertAndKey([]byte(diskCert.TLSCert), []byte(diskCert.TLSKey), tmpDir) + Expect(err).NotTo(HaveOccurred()) + + // Turn tls.key into a directory so the write fails even when tests run as root. + keyPath := filepath.Join(tmpDir, "tls.key") + Expect(os.Remove(keyPath)).To(Succeed()) + Expect(os.Mkdir(keyPath, 0o700)).To(Succeed()) + + origCert := append([]byte(nil), secret.Data["tls.crt"]...) + + updated, err := controller.CheckOrUpdateWebhookCertSecret(context.Background(), secret) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("writing rotated webhook cert")) + Expect(updated).To(BeFalse()) + Expect(secret.Data["tls.crt"]).To(Equal(origCert)) + }) }) })