diff --git a/cli/commands/server.go b/cli/commands/server.go index 3ac4d0b0..680fa605 100644 --- a/cli/commands/server.go +++ b/cli/commands/server.go @@ -364,6 +364,17 @@ func Server(ctx *Context, opts serverconfig.CLIFlags) error { if labs.DistributedRunners() { ctx.Log.Info("setting up etcd mTLS for distributed runners") + // SetupEtcdTLS reads the CA from disk and requires it to already + // exist. On a fresh install the coordinator's own create-if-missing + // LoadCA doesn't run until much later (Coordinator.Start), so ensure + // the CA is materialized here first. Otherwise the first boot fails + // "CA certificate not found" and crash-loops until an out-of-band + // auth generate creates it. + if _, err := coordinate.EnsureCA(ctx.Log, cfg.Server.GetDataPath()); err != nil { + ctx.Log.Error("failed to ensure CA for etcd TLS", "error", err) + return err + } + var err error etcdTLSSetup, err = coordinate.SetupEtcdTLS(ctx.Log, cfg.Server.GetDataPath(), cfg.TLS.AdditionalNames, ipSet.RawIPs()) if err != nil { diff --git a/components/coordinate/coordinate.go b/components/coordinate/coordinate.go index 97af5ae4..b6104206 100644 --- a/components/coordinate/coordinate.go +++ b/components/coordinate/coordinate.go @@ -455,59 +455,81 @@ func validateAPICertificate(cert *x509.Certificate, expectedNames []string, expe return nil } -func (c *Coordinator) LoadCA(ctx context.Context) error { - cert := filepath.Join(c.DataPath, "server", "ca.crt") - keyPath := filepath.Join(c.DataPath, "server", "ca.key") +// EnsureCA makes sure the server CA certificate and key exist on disk under +// /server, creating them if they're missing, and returns the loaded +// authority. It's safe to call before the coordinator starts. +// +// This is what lets the server bootstrap its own etcd mTLS: SetupEtcdTLS reads +// the CA from disk and requires it to already exist, so the CA must be +// materialized before that runs. Historically the only create-if-missing path +// was Coordinator.LoadCA, which doesn't run until Coordinator.Start, well after +// the etcd TLS block, so a fresh install with distributed runners enabled had +// no CA yet. Calling EnsureCA early closes that gap. +func EnsureCA(log *slog.Logger, dataPath string) (*caauth.Authority, error) { + cert := filepath.Join(dataPath, "server", "ca.crt") + keyPath := filepath.Join(dataPath, "server", "ca.key") - if data, err := os.ReadFile(cert); err == nil { - c.Log.Info("loading existing CA", "path", cert) + data, err := os.ReadFile(cert) + switch { + case err == nil: + log.Info("loading existing CA", "path", cert) key, err := os.ReadFile(keyPath) if err != nil { - return fmt.Errorf("missing key for CA: %w", err) + return nil, fmt.Errorf("missing key for CA: %w", err) } ca, err := caauth.LoadFromPEM(data, key) if err != nil { - return fmt.Errorf("failed to load CA: %w", err) + return nil, fmt.Errorf("failed to load CA: %w", err) } - c.authority = ca - } else { - c.Log.Info("generating new CA", "path", cert) + return ca, nil + case !errors.Is(err, os.ErrNotExist): + // The CA cert exists but couldn't be read (permissions, transient IO, + // etc). Bail rather than fall through and regenerate, which would + // overwrite a valid CA and invalidate every cert it ever issued. + return nil, fmt.Errorf("failed to read CA cert at %s: %w", cert, err) + } - ca, err := caauth.New(caauth.Options{ - CommonName: "miren-server", - Organization: "miren", - ValidFor: 10 * year, - }) - if err != nil { - return fmt.Errorf("failed to generate CA: %w", err) - } + log.Info("generating new CA", "path", cert) - err = os.MkdirAll(filepath.Dir(cert), 0755) - if err != nil { - return fmt.Errorf("failed to create CA directory: %w", err) - } + ca, err := caauth.New(caauth.Options{ + CommonName: "miren-server", + Organization: "miren", + ValidFor: 10 * year, + }) + if err != nil { + return nil, fmt.Errorf("failed to generate CA: %w", err) + } - cd, kd, err := ca.ExportPEM() - if err != nil { - return fmt.Errorf("failed to export CA: %w", err) - } + if err := os.MkdirAll(filepath.Dir(cert), 0755); err != nil { + return nil, fmt.Errorf("failed to create CA directory: %w", err) + } - err = os.WriteFile(cert, cd, 0644) - if err != nil { - return fmt.Errorf("failed to write CA cert: %w", err) - } + cd, kd, err := ca.ExportPEM() + if err != nil { + return nil, fmt.Errorf("failed to export CA: %w", err) + } - err = os.WriteFile(keyPath, kd, 0600) - if err != nil { - return fmt.Errorf("failed to write CA key: %w", err) - } + if err := os.WriteFile(cert, cd, 0644); err != nil { + return nil, fmt.Errorf("failed to write CA cert: %w", err) + } - c.authority = ca + if err := os.WriteFile(keyPath, kd, 0600); err != nil { + return nil, fmt.Errorf("failed to write CA key: %w", err) + } + + return ca, nil +} + +func (c *Coordinator) LoadCA(ctx context.Context) error { + ca, err := EnsureCA(c.Log, c.DataPath) + if err != nil { + return err } + c.authority = ca return nil } diff --git a/components/coordinate/etcd_tls_test.go b/components/coordinate/etcd_tls_test.go new file mode 100644 index 00000000..c58afd66 --- /dev/null +++ b/components/coordinate/etcd_tls_test.go @@ -0,0 +1,125 @@ +package coordinate_test + +import ( + "crypto/x509" + "encoding/pem" + "log/slog" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" + "miren.dev/runtime/components/coordinate" +) + +// TestEtcdTLSColdBoot pins the ordering invariant behind MIR-1464: a fresh data +// dir has no CA, so SetupEtcdTLS must fail until the CA is materialized, and +// EnsureCA (which the server now calls before the etcd TLS block) is what makes +// it succeed. This reproduces a fresh `server install` with distributed runners +// enabled, where nothing has bootstrapped a CA yet. +func TestEtcdTLSColdBoot(t *testing.T) { + r := require.New(t) + log := slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelDebug})) + + dataPath := t.TempDir() + + // Cold boot: no CA on disk yet, so SetupEtcdTLS can't proceed. + _, err := coordinate.SetupEtcdTLS(log, dataPath, nil, nil) + r.Error(err, "SetupEtcdTLS should fail before a CA exists") + + // EnsureCA is the early bootstrap step. It should create the CA on disk. + ca, err := coordinate.EnsureCA(log, dataPath) + r.NoError(err) + r.NotNil(ca) + + caCert := filepath.Join(dataPath, "server", "ca.crt") + caKey := filepath.Join(dataPath, "server", "ca.key") + r.FileExists(caCert) + r.FileExists(caKey) + + // With the CA in place, etcd TLS setup now succeeds. + result, err := coordinate.SetupEtcdTLS(log, dataPath, nil, nil) + r.NoError(err, "SetupEtcdTLS should succeed once EnsureCA has run") + r.NotNil(result) + + // The issued etcd server cert must chain to the CA we created. + roots := x509.NewCertPool() + caPEM, err := os.ReadFile(result.CAFile) + r.NoError(err) + r.True(roots.AppendCertsFromPEM(caPEM), "CA cert should be parseable") + + serverCert := parseCert(t, filepath.Join(result.CertsDir, "server.crt")) + _, err = serverCert.Verify(x509.VerifyOptions{ + Roots: roots, + DNSName: "localhost", + KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + }) + r.NoError(err, "etcd server cert should chain to the CA and cover localhost") + + // The coordinator client cert should chain to the same CA. + clientCert := parseCert(t, result.ClientCertFile) + _, err = clientCert.Verify(x509.VerifyOptions{ + Roots: roots, + KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth}, + }) + r.NoError(err, "etcd client cert should chain to the CA") +} + +// TestEnsureCAIdempotent confirms EnsureCA reuses an existing CA rather than +// regenerating it, so calling it early (server boot) and again later +// (auth generate, Coordinator.Start) all see the same authority. +func TestEnsureCAIdempotent(t *testing.T) { + r := require.New(t) + log := slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelDebug})) + + dataPath := t.TempDir() + + _, err := coordinate.EnsureCA(log, dataPath) + r.NoError(err) + + caCert := filepath.Join(dataPath, "server", "ca.crt") + first, err := os.ReadFile(caCert) + r.NoError(err) + + _, err = coordinate.EnsureCA(log, dataPath) + r.NoError(err) + + second, err := os.ReadFile(caCert) + r.NoError(err) + r.Equal(first, second, "EnsureCA should reuse the existing CA on subsequent calls") +} + +// TestEnsureCADoesNotOverwriteUnreadableCA guards against silently regenerating +// a CA when its cert exists but can't be read. A transient read error must not +// be mistaken for absence, since regenerating would clobber a valid CA and +// invalidate every cert it issued. We simulate an unreadable cert by making +// ca.crt a directory, which yields a non-ErrNotExist read error on every OS and +// even as root (unlike chmod 0000). +func TestEnsureCADoesNotOverwriteUnreadableCA(t *testing.T) { + r := require.New(t) + log := slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelDebug})) + + dataPath := t.TempDir() + caCert := filepath.Join(dataPath, "server", "ca.crt") + r.NoError(os.MkdirAll(caCert, 0755)) + + _, err := coordinate.EnsureCA(log, dataPath) + r.Error(err, "EnsureCA should refuse to proceed when the CA cert is unreadable") + + // The path must be left untouched: still a directory, no key written next to it. + info, statErr := os.Stat(caCert) + r.NoError(statErr) + r.True(info.IsDir(), "EnsureCA must not overwrite the existing path") + r.NoFileExists(filepath.Join(dataPath, "server", "ca.key")) +} + +func parseCert(t *testing.T, path string) *x509.Certificate { + t.Helper() + pemBytes, err := os.ReadFile(path) + require.NoError(t, err) + block, _ := pem.Decode(pemBytes) + require.NotNil(t, block, "expected a PEM block in %s", path) + cert, err := x509.ParseCertificate(block.Bytes) + require.NoError(t, err) + return cert +}