Bound concurrent CA-key signing, shedding on the OCSP path

[bootc]

Bounds concurrent CA-key signing, shared across certificate issuance, CRL
re-signing and the OCSP responder, and configurable via
ca_signing_concurrency. Closes #274.

What this builds on

#265 has merged, and this is rebased onto it. That matters because it is
what makes the bound load-bearing rather than dormant: AnswerOCSP now signs
outside c.mu, so concurrent OCSP requests really do become concurrent
CA-key signatures.

c.mu was, incidentally, the only thing holding CA-key use to one signature in
flight process-wide — every CAKey.Sign site held it. #265 removed that for
OCSP, which was the entire point of that change. What it left is unbounded:
/ocsp is unauthenticated (tierPublic), the only rate limiter in
internal/api is CSR-only, and a cache miss signs, so an unauthenticated
caller can drive as many concurrent signatures as it can open connections
against a signer shared with issuance.

An earlier revision of this PR was written against pre-#265 main, where the
mechanism could not be contended and the concurrency behaviour could not be
tested. It now can be, and is — see internal/ca/signboundrace_test.go, which
holds a real signature open and shows a genuinely concurrent second request
being shed.

Why a shared blocking semaphore would have been wrong

The obvious implementation reinstates the bug #265 exists to fix, by a
different route. issueLeafLocked and signCRLLocked acquire while holding
c.mu. An anonymous flood owning every slot would block that acquisition,
stalling every c.mu reader behind it — including the revocation check on the
authentication path. That is #197's process-wide stall, arrived at from the
other side.

So the bound is asymmetric:

• Issuance and CRL re-signing queue. They are authenticated and already
  serialised against each other by c.mu; refusing a certificate a client
  asked for in order to protect an unauthenticated responder would be the wrong
  way round.
• The OCSP responder sheds, answering RFC 6960 tryLater over HTTP 503
  after a bounded wait. Letting it queue would convert unbounded signing into
  unbounded queueing and bound nothing.

Both acquisitions honour ctx, so a caller that has gone away stops waiting
rather than holding c.mu on nobody's behalf. Remove either property and the
stall is back; the invariant is recorded in docs/development/locking.md under
Lock ordering.

Shedding is unusually cheap here, which is what makes it a real relief valve
rather than a slower route to the same load: an RFC 6960 non-success response
carries no signature, so a refused request costs no CA-key work at all.

The default

max(4, GOMAXPROCS), following the csr_rate_limit sentinel convention
(-1/unset → default, 0 → unbounded, positive → literal) across file, env
and flag.

#265 declined to pick a number because the right size is a property of the
deployment's signer, and that reasoning is preserved here: the default is a
safe ceiling, not a tuning. Its only job is to make the number finite.
Scaling with CPU count suits the two backends where a signature is CPU-bound —
a software key in process, and the default isolated signer, where it is
CPU-bound in the signer child. Operators with a remote signer are told, in the
docs, to lower it to that signer's capacity, which openvox-ca cannot discover.

What this does NOT protect

• It is per process. N replicas against one shared OpenBao Transit key
  permit N × the limit against that key. Documented in both metrics.md and
  openbao-transit.md.
• It does not stop the flood. TLS handshakes, goroutines and connection
  memory remain unbounded; only CA-key work is capped.
• Shedding has a real cost. A verifier that hard-fails on an unavailable
  responder treats sustained tryLater as a revocation-checking outage. This
  converts an unbounded signer load into a bounded, observable, visible
  degradation — that is the trade, and configuration.md says so plainly.
• A default set too high bounds nothing, and nothing here can detect that.
• The RemoteSigner deadline bounds the caller's wait, not the signer's
  work. net/rpc has no cancellation, so an abandoned call leaves the signer
  child still signing. It stops callers accumulating — which is what makes the
  slots recoverable — but does not reduce load on the signer. Nothing bounds
  that today.
• An abandoned call also leaves an entry in the RPC client's pending table,
  reclaimed only if a reply eventually arrives. Against a permanently wedged
  signer none does. The bound itself rate-limits this — at most limit calls
  outstanding, each holding its slot for the full timeout, so entries accrue at
  limit/timeout rather than per request — which works out at roughly a
  megabyte a day during an outage someone is already fixing. Reconnecting is
  not available as a remedy: the socketpair fd is inherited once at spawn.
  Recorded in docs/ca-key-security.md rather than engineered around.

Observability

docs/development/locking.md notes that no metric covers this at all, and that
a bound nobody can watch being approached is only half an answer. Three series
land with it: puppetca_ca_signing_in_flight, puppetca_ca_signing_limit and
puppetca_ca_signing_shed_total. The limit is published beside in-flight
because in-flight alone cannot say whether 8 concurrent signatures is
comfortable or is the ceiling, and it is emitted even when 0 — "unbounded" is
a legitimate configured value and looks identical at a glance to a bound that
is simply never reached.

Dependencies: none added, no FIPS trace owed

The bound is a buffered channel of struct{}. tryLater uses
xocsp.TryLaterErrorResponse, from golang.org/x/crypto/ocsp, already a
direct dependency and already used by this handler for
MalformedRequestErrorResponse and InternalErrorErrorResponse. The deadline
uses net/rpc and time from the standard library.

git diff origin/main...HEAD -- go.mod go.sum is empty. No module is added,
removed or upgraded, so the boringcrypto contract is untouched — no new code
path reaches a crypto primitive that was not already reached. x/sync was
considered for the semaphore and deliberately not used: it is currently an
indirect dependency, and promoting it to direct would have been a dependency
change to justify for something a channel does.

The RemoteSigner deadline, and why it is in scope

RemoteSigner.Sign was a bare rpc.Client.Call with no per-call deadline at
all, against the default deployment — key isolation is what openvox-ca serve does unless told otherwise — while openbao.Signer.Sign is already
bounded at roughly 2x its login timeout.

It reads at first like a separate concern that wandered in, and that is worth
answering directly, because it is not. A signing slot can fail to come back
three ways, and this PR closes all three:

1. the caller gives up — the ctx-aware acquire;
2. the signature panics — the deferred release (net/http recovers a
   handler panic, so the process survives and the slot would be gone for good);
3. the signer never answers — this deadline.

They are one defect class: a slot that leaves the pool and does not return. On a
bound configured at 1 or 2 — which the docs recommend for a constrained remote
signer — any one of them alone wedges issuance permanently. A concurrency bound
whose slots are never released is an outage rather than a limit, so closing two
of the three routes to an identical failure is what could not be justified.

(2) and (3) are complementary rather than redundant: a panic returns the slot
immediately, a wedged signer only after the deadline expires.

The caveat, stated tightly because it should not be oversold: net/rpc has
no cancellation, so the deadline bounds this caller's wait, not the child's
work. The slot comes back; the signer stays wedged. That is why two minutes is
a backstop rather than a tuning — it has to clear the slowest legitimate
signature, which under ca_key_provider: openbao is the child's own ~2x
LoginTimeout round trip (~20s by default). An operator who raises
LoginTimeout past a minute should know this ceiling exists.

locking.md changes

Two, and the second was deferred at first but is now due and done:

• an additive bullet under Lock ordering, recording the c.mu →
  signing-slot invariant and why a blocking acquire under c.mu is safe only
  because the OCSP path sheds and both acquires honour ctx;
• the OCSP signing is unbounded on the CA key once it leaves the global write lock #274 Known-gaps entry struck through with a Fixed: note, following
  the file's own #202/#187 convention.

The strike-through was deliberately not in the first revision: main had no
#274 entry to amend at the time — it existed only on #265's branch — so writing
one would have collided in a file four open branches were editing. #265 has
since merged, which made it due, and it landed in the rebase. The retained
analysis below the struck claim names the two clauses in it that are now
history, so it cannot be misread as current state.

[bootc]

Review Council disposition — all 8 findings accepted and acted on

Rebased onto 43c8dd407655 (conflict-free) and pushed as 48bf1661c47f.

Every finding was checked against the code before being accepted. All eight were real. Four new commits.

🟠 HIGH

1. CRL re-sign's queueing path never exercised through signCRLLocked — accepted. Confirmed: SigningConcurrency appeared only in the two new test files, never in any CRL test.

2. applyCAConfig's SigningConcurrency wiring untested — accepted, and the reviewer's framing of the failure mode is the reason it deserved its own spec: drop that line and the field stays at its zero value, which means unbounded — the bound is inert, #274 is wide open, and every other test still passes.

Fixed in test(ca): drive the queueing half through its production call sites and test(cmd): assert ca_signing_concurrency actually reaches the CA.

One correction to the recommendation on the issuance side, because it cannot be done as written: two concurrent issuances cannot contend for a slot. issueLeafLocked runs under c.mu, so c.mu serialises them long before either reaches the bound — a spec racing two Sign/Generate calls would pass with the bound deleted. The contention that can really happen is cross-path: OCSP signs without c.mu and can hold the only slot while an issuance waits. That is what the specs reproduce, and it is the interaction the asymmetric design exists for.

Mutation-tested against the failures the findings name, not just convenient ones:

Mutation	Result
drop releaseSigningSlot in signCRLLocked	CRL "returns it on success" fails
drop releaseSigningSlot in issueLeafLocked	issuance "returns it on success" fails
make the queueing acquire ignore ctx	both cancellation specs fail
delete the applyCAConfig assignment	2 of 3 wiring specs fail

The third wiring spec (explicit 0) correctly survives that last mutation — 0 is also the zero value, so it cannot distinguish. Left as-is rather than contrived into failing.

🟡 MEDIUM

3. 🛡️ RemoteSigner timeout leaks its net/rpc pending-call entry — accepted, verified against the stdlib: client.pending[seq] is deleted only when a reply for that sequence arrives or the connection tears down, so a wedged-but-open connection retains it. The regression framing is right too — before the deadline the caller never returned to make a second call.

The recommended remedy is not available here: forcing a reconnect is impossible, because the socketpair fd is inherited once at spawn and consumed by DialConn. There is no second dial.

What does bound it is the bound this PR adds, which I think the analysis missed: every Sign passes through ca_signing_concurrency, so at most limit calls are outstanding and each holds its slot for the full timeout. Entries accrue at limit/timeout, not per request — single digits per minute at the shipped default, a few hundred bytes each, and they drain whenever a reply does arrive. A permanently wedged signer costs on the order of a megabyte a day, during an outage someone is already fixing. Documented with that arithmetic in signer.go and docs/ca-key-security.md rather than engineered around; the "document the residual cost explicitly if accepted as-is" branch of the recommendation.

4. 📚 OpenBao guide claims no CA-side metric exists and 6. ⚙️ OpenBao guide claims the bound is unimplemented, two lines above the bullet documenting it — both accepted, both fixed.

Worth naming rather than quietly correcting: both paragraphs are text this branch inherited when it rebased onto merged #265, and my own edit landed immediately beside them without touching either. The inserted bullet read as an addition when it was also a contradiction.

5. 🧭 PR description understates its own locking.md changes — accepted; the description was written before the rebase and never updated after the strike-through landed. Corrected.

I also found a worse instance of the same defect the finding is about, which it did not flag: the description's opening section still said "this targets the world after #265 lands" and "on today's main the mechanism is dormant". #265 has merged, so both were false. Both sections rewritten.

7. 🧪 Leaf-issuance queueing lacks real-concurrency and cancellation coverage — accepted; covered by the same new file, with the c.mu correction above.

8. 🧪 New Prometheus metrics have no test coverage — accepted. Three specs following collector_test.go's gather/findByLabels pattern, including the case the code comment calls out: the limit is still emitted as 0 when unbounded, since an absent series cannot be alerted on. The shed counter is driven by real contention — a gated key holds a signature open while a second OCSP request is refused — rather than by poking the counter.

---

Full go test ./..., go vet, gofmt and the push hook's race suite all green locally before pushing.

[bootc]

Review Council disposition — both findings accepted, and the MEDIUM is under-rated

Rebased onto 12eda0397a1a (picking up merged #289) and pushed as c8a3c4535601. Both findings were verified against the code before being accepted; both were real.

🟠 HIGH — no handler-level test for the 503 shed branch

Accepted. Confirmed: the only cover was ocsperror_internal_test.go, which pins ocspErrorResponse in isolation. It says which constants belong together and executes none of the handler — not the 503 write, not the response body, not the log line.

The new spec fills the bound the way a real caller would rather than by reaching into the CA: one slot, a gated signer parked inside Sign holding it, a second request POSTed through the real mux. It asserts 503 with TryLaterErrorResponse and, explicitly, that the body is neither MalformedRequest nor InternalError — the three say materially different things to a verifier, and only tryLater invites the retry that makes shedding survivable. It then releases the first signature and shows the responder serving normally again, since a refusal that did not recover would be an outage rather than a queue depth.

Demonstrated rather than asserted. A coverage finding normally claims a gap; this one can be measured. Mutating the handler to write an empty body leaves ocsperror_internal_test.go green and turns the new spec red — so the isolated mapping test structurally cannot catch handler regressions. That is the gap stated as a fact about what the existing suite can discriminate.

🟡 MEDIUM — slot release not panic-safe

Accepted, and this is worth more than MEDIUM. The finding left the consequence as an assumption, and it does not hold up as merely hygienic.

There is no recover() anywhere in this repository's non-test code — but net/http's conn.serve recovers a handler panic (server.go:1936), logs it, and drops that one connection. The process survives. So on the two HTTP-reachable sites the slot is not reclaimed by a restart that never happens: it is leaked permanently, in a live server.

The pool is small by design, which sets the blast radius. ca_signing_concurrency defaults to max(4, GOMAXPROCS), but ca.go's field doc, the --ca-signing-concurrency help text and the docs all tell operators with an isolated signer or OpenBao Transit to lower it to the backend's real capacity. A configured 1 or 2 is an ordinary deployment, not a pathological one. There, one panic permanently wedges issuance and CRL re-signing — both queue, and under c.mu — and sheds every OCSP request with tryLater until restart. That is a permanent denial of service inside the control added to bound a denial of service, reached through /ocsp, which is unauthenticated.

Why it was written that way, since the reason matters more than the fix. I chose sequential release deliberately, to hold the slot for the minimum, on the stated belief that a panic mid-signature takes the process with it anyway. That belief was wrong about the runtime, not about the design. Recording it is the durable part: without it the next person to touch this primitive re-derives the same rationale and puts the bug back. It is now in releaseSigningSlot's doc comment as an invariant every future call site has to keep.

Fixed with the closure-with-defer shape, not a function-scoped defer, and that distinction is load-bearing rather than stylistic. signCRLLocked writes to storage after signing; a function-scoped defer would hold a signing slot across that write, so occupancy would measure "sign plus persist" rather than "sign" — inflating it well past the work the bound exists to meter, and on a small configured concurrency that is the difference between shedding correctly and shedding spuriously. This is rule 4 of docs/development/locking.md applied to a new primitive, alongside signing.go's existing "a panic mid-sign still frees the lock rather than wedging the CA".

Three specs pin it, one per call site, each recovering the panic the way net/http would. Each asserts the slot came back and that the CA still works afterwards, since a leak's real signature is the second request failing rather than the first.

Mutations, all against a committed tree

Mutation	Result
sequential release in ocsp.go	"the slot leaked: this CA can never sign again"
sequential release in crl.go	"the slot leaked: every future revocation would hang"
sequential release in signing.go	"the slot leaked: issuance would queue for ever"
handler writes an empty body	mapping spec green, mux spec red

---

Rebase note. This now sits on merged #289, which touches internal/metrics/collector.go, docs/metrics.md and docs/configuration.md — all files this PR also edits. The rebase was conflict-free, and I checked that it reverted nothing: zero deletions in all three files, and #289's own change is present (the sweep's per-entry re-sign and its "ran out of budget" path are absent from this tree, as they should be).

Full go test ./..., go vet, gofmt and the push hook's race suite green before pushing.

[trevor-vaughan-ai]

🟢 Review Council: APPROVE

Automated LLM review, not a human sign-off. Findings are machine-generated, may contain errors, and are advisory input to human judgment.

Models used:

• claude-opus-5
• claude-sonnet-5

Reviewed at commit 79c20d2 (standard effort).

TL;DR: PR #285 passed review with all five personas approving; four findings - three MEDIUM, one LOW - remain unresolved but non-blocking.

Findings: 🔴 0 Critical, 🟠 0 High, 🟡 3 Medium, 🔵 1 Low

Reviewer	Verdict	Findings
🛡️ Adversary (code)	✅ Approve	2 MEDIUM
📚 Curator (code)	✅ Approve	none
🧭 Guard (code)	✅ Approve	1 LOW
⚙️ Operator (code)	✅ Approve	none
🧪 Tester (code)	✅ Approve	1 MEDIUM

🟡 MEDIUM (3)

• 🛡️ Shed path logs one WARN per refused request, letting an unauthenticated caller drive log volume (internal/api/ocsp_handler.go:85)

  slog.Warn("OCSP response shed: CA signing concurrency limit reached",
  

  💡 Recommendation: Route the shed WARN through a threshold/window tracker in the shape of destructiveOpTracker (keyed on clientIP), or log only a periodic summary, so the per-event detail an operator wants during diagnosis cannot be turned into an unbounded log stream by the caller the bound is refusing. Leave puppetca_ca_signing_shed_total as the lossless signal.
  Constraint: OWASP A09:2021 (Security Logging and Monitoring Failures); CWE-779 (Logging of Excessive Data)

  💬 Full reviewer analysis

  The new 503 branch emits an unconditional WARN line, including the client IP, for every shed OCSP request. /ocsp is tierPublic and unauthenticated, and the shed branch is by construction only reachable while the CA is under exactly the concurrent-signature load this bound exists to absorb - so the one moment the control engages is the moment the process starts writing one log record per refused request, at whatever rate the caller can sustain concurrent connections (each shed costs the attacker only the 1s ocspSigningWait per connection). The signal is already carried losslessly by puppetca_ca_signing_shed_total, which the comment two lines above names as the thing to alert on. Searches run: grep -rn "shed\|Shed" --include=*.go internal/api/ (excluding tests) returns only this call site and its comment - there is no sampling or threshold wrapper around it; grep -rn "Sample\|logEvery\|rate.Limiter\|throttl" --include=*.go internal/api/ internal/ca/ (excluding tests) returns nothing. The repository already has the pattern this wants: destructiveOpTracker at internal/api/ratelimit.go:107 is a fixed-window per-identity counter documented as "When a single identity exceeds the threshold within the window, a warning is logged for operational awareness" (NIST 800-53 AU-6). Noted honestly: the pre-existing default branch ("OCSP request error") has the same per-request property for malformed input, so this is a new instance of an existing shape rather than a new class - which is why it is MEDIUM and not higher.

• 🛡️ net/rpc pending-map leak is justified by a bound that ca_signing_concurrency: 0 removes (internal/signer/signer.go:330)

  The signing bound rate-limits it. Every Sign goes through
  

  💡 Recommendation: Qualify both the comment at internal/signer/signer.go:330-335 and docs/ca-key-security.md:99-103 to state that the accrual bound holds only while ca_signing_concurrency > 0, and that disabling the bound makes the pending-map growth per-request against a wedged signer. If that is considered unacceptable rather than merely undocumented, refuse to leave the isolated-signer path unbounded (reject or clamp ca_signing_concurrency: 0 when ExternalSigner is in use) rather than documenting it.
  Constraint: OWASP A06/CWE-400 (Uncontrolled Resource Consumption); persona rule: verify compliance/bound claims rather than accepting the author's reading

  💬 Full reviewer analysis

  The new two-minute deadline abandons the wait but not the call, leaving an entry in rpc.Client's pending map that is only reclaimed if a reply arrives. The comment justifies that trade with an unqualified claim - "Every Sign goes through ca_signing_concurrency, so at most limit calls can be outstanding at a time ... Entries therefore accrue at limit/timeout, not per request" - and docs/ca-key-security.md:101 repeats it as "bounded at ca_signing_concurrency entries per two minutes, because every signature passes through that bound first." That is false in a configuration this same PR documents as legitimate. ca_signing_concurrency: 0 is described in cmd/openvox-ca/config.go and docs/configuration.md as "0 disables the bound (unbounded signing)", and internal/ca/signbound.go:112-114 makes acquireSigningSlotOrShed return nil immediately when signSlots is nil. With the bound disabled and an isolated signer that is wedged rather than dead - the exact fault the deadline exists for - every cache-missing /ocsp request from an unauthenticated caller starts a Sign that abandons a permanent pending-map entry, so accrual is per request with no CA-side ceiling, not limit/timeout. I traced the path: internal/ca/ocsp.go:367 acquireSigningSlotOrShed -> internal/ca/signbound.go:112-114 (nil signSlots, no-op) -> ocsp.CreateResponse -> RemoteSigner.Sign. This is not a regression (before the deadline each such request leaked a permanently blocked goroutine as well), but the documented escape hatch silently voids the only thing the comment offers as the leak's bound, and an operator reading "bounded at ca_signing_concurrency entries" at a value of 0 gets the opposite of the truth.

• 🧪 No test proves an OCSP cache hit bypasses a full signing bound (internal/ca/ocsp.go:271)

  	if !hasNonce {
  		if entry, ok := c.ocspCache[serialHex]; ok && time.Now().Before(entry.expiresAt) {
  			c.mu.RUnlock()
  			return OCSPAnswer{DER: bytes.Clone(entry.der), MaxAge: time.Until(entry.expiresAt)}, nil
  		}
  	}
  

  💡 Recommendation: Add a spec (e.g. alongside signboundrace_test.go or ocspshed_test.go) that: primes the OCSP cache for a serial (issue + one successful non-nonce AnswerOCSP call, or a direct cache hit through the handler), occupies the only signing slot, then asserts a second request for the same cached serial still returns 200/Good rather than tryLater - pinning that the cache short-circuit runs before the bound is consulted.
  Constraint: Coverage completeness - boundary-sensitive code the changeset introduced (severity.md: 'Missing edge case coverage for boundary-sensitive code the changeset introduced')

  💬 Full reviewer analysis

  This cache-hit branch returns before acquireSigningSlotOrShed is ever reached (that call is added later in the same function, at what is now line 367), so a cached response should be served even while the signing bound is fully occupied by a concurrent signature. That is a real, load-bearing property: the docs (docs/ca-key-security.md, docs/openbao-transit.md) tell operators the in-process cache is part of what keeps signing load down, and it only helps if cache hits are actually free of the bound. Checked against the three gates before assigning severity: (1) not a pre-existing gap - there was no bound to interact with before this changeset, so 'cache hit under a full bound' is a scenario the diff itself creates; (2) not covered elsewhere - I searched the new signing-bound specs (signbound_test.go, signboundcallsites_test.go, signboundpanic_test.go, signboundrace_test.go, ocspshed_test.go, ocsperror_internal_test.go, signingbound_test.go) and the pre-existing internal/ca/ocsp_test.go (which does have 'serves the cached response on a second call' and 'generates a fresh response (bypasses cache) when a nonce is present', but neither sets SigningConcurrency or occupies a slot) - none combine a populated cache entry with a fully occupied signSlots channel; (3) consequence is real but bounded - a future refactor that moved the slot acquisition ahead of the cache check would silently turn cache hits into 503 tryLater under load, an availability regression on the exact path operators are told to rely on, but it would not corrupt a certificate status or leak key material, so this stays at MEDIUM rather than HIGH.

🔵 LOW (1)

• 🧭 Init's stated reason for sizing the bound first is not true of its own bootstrap signatures (internal/ca/init.go:101)

  // signs on the bootstrap path, so this has to precede that rather than
  	// merely precede serving.
  	c.initSigningBound()
  

  💡 Recommendation: Reword to state what is actually required - the bound is sized before the CA can serve - and record that Init's bootstrap self-sign and initial-CRL signatures deliberately run outside it (one-shot, before any consumer exists), or route them through acquireSigningSlot so the comment's claim holds.
  Constraint: Structural Coherence

  💬 Full reviewer analysis

  The comment justifies calling initSigningBound at the top of Init on the grounds that Init's bootstrap signing needs the bound already sized. It does not: no bootstrap signature consults signSlots. grep -rn "acquireSigningSlot\|acquireSigningSlotOrShed\|releaseSigningSlot" internal/ --include="*.go" (excluding _test.go) returns acquisitions at exactly three sites - internal/ca/signing.go:562 (issueLeafLocked), internal/ca/crl.go:78 (signCRLLocked) and internal/ca/ocsp.go:367 (AnswerOCSP) - and none in init.go. Init's own signatures are the self-signed CA certificate at internal/ca/init.go:536 (x509.CreateCertificate(rand.Reader, template, template, key.Public(), key)) and the two newEmptyCRL(c.CACert, c.CAKey, ...) calls at internal/ca/init.go:337 and :583, which reach crl.go:220 directly; none of the six signCRLLocked callers (cleanup.go:157, revoke.go:392, crl.go:286, crl.go:422, supersede.go:754) is reachable from Init. The ordering is harmless, but the rationale invites a maintainer to believe bootstrap signatures are metered by the bound and counted in puppetca_ca_signing_in_flight, which they are not - every other statement of the bound's scope in this changeset correctly names only issuance, CRL re-signing and the OCSP responder (internal/ca/ca.go SigningConcurrency doc, docs/metrics.md, docs/configuration.md).

---

Produced by Review Council, an open-source multi-persona code reviewer. Spot a wrong call or want the source? File feedback or browse the repository.

[bootc]

Disposition — all four taken, though the verdict was APPROVE

Rebased onto 2e57bc739df6 and pushed as 089152d1de4e. None of these blocked, and all four were real.

🛡️ The pending-map justification has a hole where the bound is disabled

Accepted, and it is the one I would have most wanted caught. The RemoteSigner deadline's comment argues the abandoned net/rpc entry is bounded because every Sign passes through ca_signing_concurrency — entries accruing at limit/timeout rather than per request. But ca_signing_concurrency: 0 is a documented, supported value meaning unbounded signing, and it takes that rate limit with it. Entries are then bounded only by in-flight request concurrency, which on the unauthenticated /ocsp path is chosen by the caller.

Named in the comment rather than left for a reader to discover. It is a consequence an operator opts into rather than a defect in the opt-out — accepting unbounded concurrent signing is accepting unbounded pending entries, seen from the other side — but the sentence read as an unconditional guarantee, and the entire argument for leaving that leak in place rests on it. A justification with a hole exactly where a supported configuration puts one is worth stating, especially since I argued the panic finding up on the neighbouring ground that a leaked resource is permanent.

🧭 Init's stated reason was false

Accepted — and it was not merely imprecise, it was wrong. The comment claimed Init signs through the bound on the bootstrap path. It does not: the self-signature calls x509.CreateCertificate directly (init.go:536) and the bootstrap CRL goes straight to storage. Verified: Init reaches neither signCRLLocked nor issueLeafLocked.

The ordering is still correct, for a reason now stated instead: Init has several exits and only the first statement is on all of them. The fast path returns as soon as an existing CA loads — what almost every start does — so a sizing placed after it would leave the commonest deployment unbounded with nothing saying so. The bootstrap bypassing the bound is correct rather than an omission, since startup is single-threaded and has nothing to contend with.

⚙️ One WARN per refused request is an amplifier

Accepted. On an unauthenticated endpoint, one WARN per shed means an anonymous caller decides how much this CA writes to disk — a request flood becomes a log flood, amplifying the load the bound exists to shed.

Dropped to Debug, which costs nothing that was being relied on: every shed is already counted by puppetca_ca_signing_shed_total, and that metric is what the docs tell operators to alert on. The line only adds which caller provoked it — a debugging question, so it belongs at the level you turn on to ask it.

🧪 A cache hit bypassing the bound was untested

Accepted. This is the property that keeps the bound from being felt in normal service: a cached response returns under the read lock before the bound is consulted, so ordinary verifier traffic — repeat queries for a handful of serials — is answered while the bound is saturated. Without it, a full bound would shed every request rather than only those that would actually sign, and the shed rate would stop meaning what the metric claims.

Verified by mutation, and the mutation was chosen to discriminate the ordering rather than the bound: consulting the bound before the cache fails the new spec and leaves both its siblings green.

---

Two of the four were defects in prose rather than code — a justification that did not hold under a supported setting, and a stated reason that was simply untrue. That is the same class as the stale claims this branch has already corrected three times, and it is the class I now check for after every ruling and rebase rather than case by case.

Full go test ./..., go vet, gofmt and the push hook's race suite green before pushing.