Merge pull request #283 from voxpupuli/fix/init-bootstrap-reentrancy

fix(ca): stop Init's slow path re-entering the bootstrap lock

Author: bootc

docs/development/locking.md

@@ -378,9 +378,11 @@ written as two.
   inversion itself is safe only because `Init` runs to completion before
   the server starts serving, so nothing else can be holding a distributed lock
   while waiting on `c.mu`; do not copy this pattern into anything that runs
-  while serving. Init also has a *separate*, unfixed hazard on the same lock —
-  its slow path can re-enter `bootstrap` and deadlock startup
-  ([#201](https://github.com/voxpupuli/openvox-ca/issues/201)); see known gaps.
+  while serving. Init also *had* a separate hazard on the same lock — its slow
+  path re-entered `bootstrap` and deadlocked startup
+  ([#201](https://github.com/voxpupuli/openvox-ca/issues/201)). Fixed; the rule
+  that produced it is not, because `WithLock` is still not reentrant at any
+  tier. See known gaps.
 - `bootstrap` → `hmac-key` is the first nesting in which a name owned by
   `StorageService` sits inside one owned by the CA layer, and the only one taken
   entirely within `internal/storage`. It is not the only nesting of two
@@ -764,11 +766,30 @@ state when the document was last updated and is not guaranteed exhaustive.
     is reported as RFC 6960 `internalError` rather than `malformedRequest` —
     the latter tells a verifier not to retry and logs an outage as a client
     error.
-- [#201](https://github.com/voxpupuli/openvox-ca/issues/201) — `CA.Init`'s slow
+- ~~[#201](https://github.com/voxpupuli/openvox-ca/issues/201) — `CA.Init`'s slow
   path can re-enter the `bootstrap` lock (via `finishLoadExisting` →
   `seedSupportingState`) and deadlock startup, because `WithLock` is not
   reentrant and its process-local gate ignores the context. Reachable when a
-  replica loads a CA bootstrapped elsewhere but then finds the CRL absent.
+  replica loads a CA bootstrapped elsewhere but then finds the CRL absent.~~
+  Fixed: the seeding is split into `seedSupportingStateLocked`, which does the
+  work, and `seedSupportingState`, which acquires `bootstrap` around it.
+  `finishLoadExisting` takes the variant to use from its caller, so the choice
+  is made where the lock state is known — the fast path holds nothing and
+  passes the acquiring one, the slow path is already inside the lock and passes
+  the other. What is **not** fixed is the property underneath: `WithLock` is
+  still not reentrant at any tier and its per-name gate is still a plain
+  `sync.Mutex` that ignores the context, so a second acquisition on one
+  goroutine still hangs rather than failing at the lock timeout. This closed one
+  call site, not a mechanism.
+  [initreentrancy_test.go](../../internal/ca/initreentrancy_test.go) pins both
+  halves. The slow path is driven against a backend that refuses the first
+  CA-certificate read, and the spec fails on a bounded wait for `Init` to
+  return — neither of the two shapes the Tests section below describes, because
+  a re-entrant acquisition never returns to be counted and waits on nobody else
+  to be parked behind. The fast path is the before/after acquisition count that
+  section does describe, and it is there because moving *both* call sites onto
+  the `...Locked` variant would cure the hang and leave two replicas racing to
+  seed — which the first spec alone cannot tell from a fix.
 - ~~[#202](https://github.com/voxpupuli/openvox-ca/issues/202) — `hmac_key`
   initialisation (`EnsureHMACKey`, called by `InitHMAC` *before* the
   `bootstrap` lock) is an unlocked read-modify-write, so two replicas
@@ -781,9 +802,11 @@ state when the document was last updated and is not guaranteed exhaustive.
   `bootstrap` lock, deliberately: `InitHMAC` runs on every start, and the fast
   path immediately below it loads an already-bootstrapped CA *without* a
   distributed lock. Moving it inside `bootstrap` would make every replica's
-  every start contend for that lock and would enlarge
-  [#201](https://github.com/voxpupuli/openvox-ca/issues/201)'s re-entrancy
-  hazard rather than avoid it. A separate reason, and a separate claim, is why
+  every start contend for that lock, and would enlarge the `bootstrap` critical
+  section that
+  [#201](https://github.com/voxpupuli/openvox-ca/issues/201)'s re-entrancy was
+  found inside. That gap is fixed above; the contention argument stands on its
+  own. A separate reason, and a separate claim, is why
   the new name is not *called* `bootstrap`: `MigrateService` reaches
   `EnsureHMACKey` from inside that lock and `WithLock` is not reentrant, so a
   shared name would turn a migration that met a corrupt key into a hang —

internal/ca/init.go

@@ -124,9 +124,11 @@ func (c *CA) Init(ctx context.Context) error {
 	// point rather than an oversight: InitHMAC runs on every start, and the
 	// fast path a few lines down deliberately loads an already-bootstrapped CA
 	// without taking a distributed lock. Moving it inside would make every
-	// replica's every start contend for `bootstrap`, and would enlarge #201's
-	// re-entrancy hazard rather than avoid it. Once the key exists EnsureHMACKey
-	// takes no lock either, so a warm start still costs no lock at all.
+	// replica's every start contend for `bootstrap`, and would enlarge the
+	// bootstrap critical section that #201's re-entrancy was found inside. That
+	// gap is fixed below; the contention argument stands on its own. Once the
+	// key exists EnsureHMACKey takes no lock either, so a warm start still costs
+	// no lock at all.
 	hmacCtx, cancelHMAC := context.WithTimeout(ctx, LockTimeout)
 	errHMAC := c.Storage.InitHMAC(hmacCtx)
 	cancelHMAC()
@@ -147,7 +149,7 @@ func (c *CA) Init(ctx context.Context) error {
 	// distributed lock. Once a CA exists, all replicas can read it.
 	loadErr := c.loadCA(ctx)
 	if loadErr == nil {
-		return c.finishLoadExisting(ctx)
+		return c.finishLoadExisting(ctx, c.seedSupportingState)
 	}
 
 	// When using an external signer (key isolation mode), the frontend must
@@ -166,7 +168,9 @@ func (c *CA) Init(ctx context.Context) error {
 	return c.Storage.WithLock(ctx, lockNameBootstrap, func() error {
 		if err := c.loadCA(ctx); err == nil {
 			slog.Info("Loaded CA bootstrapped by another replica", "cert", c.Storage.CACertPath())
-			return c.finishLoadExisting(ctx)
+			// ...Locked: we are inside the bootstrap critical section, and
+			// WithLock is not reentrant. See finishLoadExisting's comment.
+			return c.finishLoadExisting(ctx, c.seedSupportingStateLocked)
 		}
 		hasCert, errCert := c.Storage.HasCACert(ctx)
 		if errCert != nil {
@@ -237,9 +241,24 @@ func (c *CA) Init(ctx context.Context) error {
 // cache). c.mu must be held by the caller. When the CRL is absent from the
 // backend but the cert+key loaded successfully — the common case when an
 // existing CA cert/key is mounted via an overlay against a fresh remote
-// backend — seed the CRL, inventory, and serial counter under the bootstrap
-// lock so startup can complete.
-func (c *CA) finishLoadExisting(ctx context.Context) error {
+// backend — the CRL, inventory and serial counter are seeded so startup can
+// complete.
+//
+// That seeding is the only part of this function that takes a distributed
+// lock, and only the caller knows whether the bootstrap lock is already held,
+// so the caller passes the variant to use. Init's fast path holds nothing and
+// passes seedSupportingState, which acquires bootstrap; Init's slow path is
+// already inside the bootstrap critical section and passes
+// seedSupportingStateLocked, which does the same work without acquiring it
+// again. finishLoadExisting itself acquires nothing either way.
+//
+// Making that the caller's choice is not a stylistic preference. WithLock is
+// not reentrant at any tier — every implementation takes a plain per-name
+// sync.Mutex before it touches the network or the filesystem, and a plain
+// sync.Mutex ignores the context — so a second acquisition on this goroutine
+// does not fail at LockTimeout, it hangs startup outright. The slow path used
+// to call the acquiring variant from inside the lock, which is #201.
+func (c *CA) finishLoadExisting(ctx context.Context, seed func(context.Context) error) error {
 	slog.Info("Loaded existing CA", "cert", c.Storage.CACertPath())
 	if err := c.buildSerialIndex(ctx); err != nil {
 		slog.Warn("Failed to build OCSP serial index", "error", err)
@@ -257,7 +276,7 @@ func (c *CA) finishLoadExisting(ctx context.Context) error {
 	if c.ExternalSigner != nil {
 		return fmt.Errorf("failed to load CRL into memory: %w", err)
 	}
-	if err := c.seedSupportingState(ctx); err != nil {
+	if err := seed(ctx); err != nil {
 		return fmt.Errorf("seeding CA supporting state: %w", err)
 	}
 	if err := c.loadCRLCache(ctx); err != nil {
@@ -271,61 +290,70 @@ func (c *CA) finishLoadExisting(ctx context.Context) error {
 
 // seedSupportingState writes the public key, CRL, inventory, and serial counter
 // that bootstrapCA would normally create, for the case where the cert+key
-// already exist (e.g. mounted via an overlay against an empty backend). Runs
-// under the bootstrap lock so concurrent replicas don't race to seed.
-//
-// The public key is also a backfill: a bootstrap that failed on that blob
-// leaves a cert and key that load cleanly forever after, so this is the only
-// path that would ever write it again.
+// already exist (e.g. mounted via an overlay against an empty backend). It
+// takes the bootstrap lock so concurrent replicas don't race to seed, so the
+// caller must NOT already hold that lock — one that does wants
+// seedSupportingStateLocked below, because WithLock is not reentrant.
 func (c *CA) seedSupportingState(ctx context.Context) error {
 	ctx, cancel := context.WithTimeout(ctx, LockTimeout)
 	defer cancel()
 	return c.Storage.WithLock(ctx, lockNameBootstrap, func() error {
-		// The public key first, and before the CRL short-circuit below, because
-		// this is the one blob nothing else ever rewrites: bootstrapCA writes it
-		// before the CRL, so a bootstrap that failed on it leaves a CA whose key
-		// and certificate load cleanly forever after while ca_pub.pem stays
-		// missing. Taken from the certificate rather than the key because it is
-		// the public component every consumer already trusts, and because it is
-		// available whether the key is local or at a provider. Idempotent, so a
-		// replica that lost the race simply rewrites identical bytes.
-		if err := savePubKeyPEM(ctx, c.Storage, c.CACert.PublicKey); err != nil {
-			return fmt.Errorf("writing CA public key: %w", err)
-		}
+		return c.seedSupportingStateLocked(ctx)
+	})
+}
 
-		// Another replica may have seeded between our initial check and the
-		// lock acquisition.
-		if _, err := c.Storage.GetCRL(ctx); err == nil {
-			return nil
-		} else if !errors.Is(err, fs.ErrNotExist) {
-			return fmt.Errorf("re-checking CRL: %w", err)
-		}
-		crl, err := newEmptyCRL(c.CACert, c.CAKey, c.CRLValidityDuration())
-		if err != nil {
-			return fmt.Errorf("creating initial CRL: %w", err)
-		}
-		crlPEM := pem.EncodeToMemory(&pem.Block{Type: "X509 CRL", Bytes: crl.Raw})
-		// Bootstrap-only write: runs before any CRL consumer exists, so it
-		// deliberately skips the crlNotify signal (see signCRLLocked).
-		if err := c.Storage.UpdateCRL(ctx, crlPEM); err != nil {
-			return fmt.Errorf("writing initial CRL: %w", err)
-		}
-		if err := c.Storage.TouchInventory(ctx); err != nil {
-			return fmt.Errorf("creating inventory: %w", err)
-		}
-		hasSerial, err := c.Storage.HasSerial(ctx)
-		if err != nil {
-			return fmt.Errorf("checking serial: %w", err)
-		}
-		if !hasSerial {
-			if err := c.Storage.WriteSerial(ctx, "0001"); err != nil {
-				return fmt.Errorf("writing serial: %w", err)
-			}
-		}
-		slog.Info("Seeded CA supporting state for existing cert+key",
-			"cert", c.Storage.CACertPath())
+// seedSupportingStateLocked does the seeding itself. The caller must hold the
+// bootstrap lock, and c.mu along with it — Init is the only caller of either
+// variant, and it holds c.mu across the whole of itself.
+//
+// The public key is also a backfill: a bootstrap that failed on that blob
+// leaves a cert and key that load cleanly forever after, so this is the only
+// path that would ever write it again.
+func (c *CA) seedSupportingStateLocked(ctx context.Context) error {
+	// The public key first, and before the CRL short-circuit below, because
+	// this is the one blob nothing else ever rewrites: bootstrapCA writes it
+	// before the CRL, so a bootstrap that failed on it leaves a CA whose key
+	// and certificate load cleanly forever after while ca_pub.pem stays
+	// missing. Taken from the certificate rather than the key because it is
+	// the public component every consumer already trusts, and because it is
+	// available whether the key is local or at a provider. Idempotent, so a
+	// replica that lost the race simply rewrites identical bytes.
+	if err := savePubKeyPEM(ctx, c.Storage, c.CACert.PublicKey); err != nil {
+		return fmt.Errorf("writing CA public key: %w", err)
+	}
+
+	// Another replica may have seeded between our initial check and the
+	// lock acquisition.
+	if _, err := c.Storage.GetCRL(ctx); err == nil {
 		return nil
-	})
+	} else if !errors.Is(err, fs.ErrNotExist) {
+		return fmt.Errorf("re-checking CRL: %w", err)
+	}
+	crl, err := newEmptyCRL(c.CACert, c.CAKey, c.CRLValidityDuration())
+	if err != nil {
+		return fmt.Errorf("creating initial CRL: %w", err)
+	}
+	crlPEM := pem.EncodeToMemory(&pem.Block{Type: "X509 CRL", Bytes: crl.Raw})
+	// Bootstrap-only write: runs before any CRL consumer exists, so it
+	// deliberately skips the crlNotify signal (see signCRLLocked).
+	if err := c.Storage.UpdateCRL(ctx, crlPEM); err != nil {
+		return fmt.Errorf("writing initial CRL: %w", err)
+	}
+	if err := c.Storage.TouchInventory(ctx); err != nil {
+		return fmt.Errorf("creating inventory: %w", err)
+	}
+	hasSerial, err := c.Storage.HasSerial(ctx)
+	if err != nil {
+		return fmt.Errorf("checking serial: %w", err)
+	}
+	if !hasSerial {
+		if err := c.Storage.WriteSerial(ctx, "0001"); err != nil {
+			return fmt.Errorf("writing serial: %w", err)
+		}
+	}
+	slog.Info("Seeded CA supporting state for existing cert+key",
+		"cert", c.Storage.CACertPath())
+	return nil
 }
 
 // loadCA reads and validates the CA key and certificate from disk.