amadan

carlos / platform Public

edge: gate only where distribution exists; budget the cert publish

Review round 1.

The issuer gate was installed unconditionally while publishing was
store-conditional, and autocert consults HostPolicy BEFORE its cache
lookup — so a box that can never hold the lease and can never be mirrored
to was left with errNotIssuer forever: no orders AND no serving from its
own DirCache. Fleet boxes (barred from control/certs/** by the door ACL),
boxes with no bucket or dir, and bucket-without-pubkey boxes all landed
there. armCertIssuance now makes it one decision: wrap the cache and keep
the gate, or clear the gate entirely — which also keeps the wildcard
manager symmetric, since it reads the same gate.

Every bucket call gets its own 10s budget, detached from the caller's
cancellation. autocert calls Cache.Delete while holding challengeMu for
write, and serves tls-alpn-01 handshakes and :80 challenge requests under
that same mutex, so an unbudgeted publish wedges the issuer's front door;
WithoutCancel because an abandoned order must still take its challenge
token out of a prefix every edge mirrors.

Also corrects the claim that gating stops renewals: domainRenewal.do never
consults HostPolicy, so a stood-down box's timers keep firing — which makes
it a hard requirement that the mirror write fresh certs into the LOCAL
cache, where do()'s cache-first check can short-circuit them.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Paul Campbell pushed by paul@keymail.dev 7187b4de27a79930ed71d317c0f2d14cdd7a44c5 parent 7eff14b
3 files changed, +294 −17
  • internal/edge/certsync.go +79 −5
  • internal/edge/certsync_test.go +208 −0
  • internal/edge/edge.go +7 −12
diff --git a/internal/edge/certsync.go b/internal/edge/certsync.go
index 58c226ee6..40a3e835b 100644
--- a/internal/edge/certsync.go
+++ b/internal/edge/certsync.go
@@ -34,6 +34,7 @@ import (
"log"
"os"
"strings"
+ "time"
"golang.org/x/crypto/acme/autocert"
@@ -79,6 +80,41 @@ func (e *Edge) issuerGate() (context.Context, bool) {
// with a different fix.
var errNotIssuer = errors.New("acme: this box is not the deployment's certificate issuer")
+// armCertIssuance wires cert DISTRIBUTION and the issuance GATE together,
+// because they are one decision and not two: a box may only be told "you are
+// not the issuer" if some other box's certificates can reach it.
+//
+// WITH a deployment store: every issuance artifact publishes to
+// control/certs/ and the gate applies — one elected issuer mints, every
+// other edge serves the published copy.
+//
+// WITHOUT one there is nothing to publish to and, just as importantly,
+// nothing to mirror FROM. The gate is cleared and the box issues for itself
+// exactly as it always has. Three real box classes land here, and gating any
+// of them would leave it permanently certificate-less — no new orders AND no
+// serving out of its own on-disk cache, since autocert consults HostPolicy
+// before its cache lookup:
+//
+// - fleet boxes (CARLOS_FLEET): the housekeeper never contends there, and
+// the fleet store door's ACL denies control/certs/** by design, so no
+// future mirror can rescue them either;
+// - boxes with neither CARLOS_DEPLOYMENT_BUCKET nor CARLOS_DEPLOYMENT_DIR;
+// - boxes with a bucket but no CARLOS_RELEASE_PUBKEY, where
+// enableStaticFromEnv leaves staticStore nil.
+//
+// Clearing the field (rather than only skipping the HostPolicy wrap) is what
+// keeps the wildcard manager symmetric too: newWildcardFromEnv reads the
+// same gate a few lines later.
+func (e *Edge) armCertIssuance(inner autocert.Cache) autocert.Cache {
+ if e.staticStore == nil {
+ e.issuer = nil
+ log.Printf("edge: cert publishing off (no deployment store configured) — this box issues and keeps its own certs, as before")
+ return inner
+ }
+ log.Printf("edge: publishing issued certs to %s* in the deployment bucket", certPrefix)
+ return &publishingCache{inner: inner, s: e.staticStore, issuer: e.issuerGate}
+}
+
// issuerHostPolicy is the autocert HostPolicy: the registry check this edge
// has always applied (Allowed), plus the issuance gate.
//
@@ -92,6 +128,19 @@ var errNotIssuer = errors.New("acme: this box is not the deployment's certificat
// cached cert as a miss and would go straight to an order, so a policy that
// says yes for a cached name is a policy that lets a non-issuer order.
//
+// It does NOT stop renewals, and nothing here should be read as claiming it
+// does: autocert's renewal timers (renewal.go's domainRenewal.do) never
+// consult HostPolicy. A timer started while this box held the lease keeps
+// firing after a stand-down; each firing re-checks the cache first, and only
+// orders when the cache has nothing fresh. Two consequences, both landing on
+// Task 5: the mirror MUST write fresh certs into this box's LOCAL cache, so
+// that cache-first check short-circuits the timer before it orders; and
+// until it does, a stood-down box's timer fails HTTP-01 against our own
+// gated handler every ~30 minutes, burning the shared account's
+// failed-validation budget. (A tls-alpn-01 validation would even succeed —
+// the token-cert branch at autocert.go:288 precedes the policy check — and
+// then the gated publisher declines to publish the result.)
+//
// The flag is not a fence. Under a hung lease renewal two boxes can
// transiently both believe they hold — the housekeeper's own stand-down is
// bounded by its 30s poll, and a lost renewal is only noticed on that poll.
@@ -131,11 +180,32 @@ type publishingCache struct {
// It is the gate's boolean that is used and not its context: publishing
// is not an order, and cancelling a publish because the holding ended
// mid-write would strand a freshly minted cert on one box's disk with
- // nothing scheduled to re-publish it. The caller's context (autocert's,
- // already deadline-bounded) carries the write.
+ // nothing scheduled to re-publish it. The write runs on its own budget
+ // (publishCtx) instead.
issuer IssuerGate
}
+// certPublishTimeout budgets ONE bucket publish or delete. It exists because
+// autocert calls this cache from under its own locks: deleteHTTPToken and
+// deleteCertToken call Cache.Delete while HOLDING m.challengeMu for write,
+// and both httpToken (the :80 challenge handler) and GetCertificate's
+// tls-alpn-01 branch read under that same mutex. An unbudgeted bucket call
+// there wedges challenge serving and ALPN handshakes on the issuer for as
+// long as the network takes. The Put path is no better off inheriting
+// autocert's own 5-10 minute order deadline.
+//
+// A var, not a const, so tests can shorten it.
+var certPublishTimeout = 10 * time.Second
+
+// publishCtx budgets one bucket call, detached from the caller's
+// cancellation. WithoutCancel is deliberate: an order that has already been
+// abandoned still has to clean up its challenge token, and a cancelled
+// context would skip exactly the delete that prevents litter every edge
+// would go on to mirror and serve.
+func publishCtx(ctx context.Context) (context.Context, context.CancelFunc) {
+ return context.WithTimeout(context.WithoutCancel(ctx), certPublishTimeout)
+}
+
func (p *publishingCache) Get(ctx context.Context, key string) ([]byte, error) {
return p.inner.Get(ctx, key)
}
@@ -150,8 +220,10 @@ func (p *publishingCache) Put(ctx context.Context, key string, data []byte) erro
if !p.publishes(key) {
return nil
}
- if err := putCert(ctx, p.s, key, data); err != nil {
- log.Printf("certsync: bucket publish failed for %q: %v — the cert is live on this box; the next renewal republishes", key, err)
+ pctx, cancel := publishCtx(ctx)
+ defer cancel()
+ if err := putCert(pctx, p.s, key, data); err != nil {
+ log.Printf("certsync: bucket publish failed for %q: %v — the cert is live on this box; the issuer's next renewal republishes", key, err)
}
return nil
}
@@ -172,7 +244,9 @@ func (p *publishingCache) Delete(ctx context.Context, key string) error {
if !p.publishes(key) {
return err
}
- if derr := p.s.Delete(ctx, certKey(key)); derr != nil {
+ pctx, cancel := publishCtx(ctx)
+ defer cancel()
+ if derr := p.s.Delete(pctx, certKey(key)); derr != nil {
log.Printf("certsync: bucket delete failed for %q: %v — a spent challenge token may linger until it is overwritten", key, derr)
}
return err
diff --git a/internal/edge/certsync_test.go b/internal/edge/certsync_test.go
index 225c14462..42cbff42b 100644
--- a/internal/edge/certsync_test.go
+++ b/internal/edge/certsync_test.go
@@ -3,14 +3,18 @@ package edge
import (
"bytes"
"context"
+ "crypto/tls"
"errors"
+ "io"
"log"
"net/http/httptest"
+ "os"
"strings"
"sync"
"testing"
"time"
+ "golang.org/x/crypto/acme"
"golang.org/x/crypto/acme/autocert"
"github.com/carlosframework/platform/internal/release"
@@ -165,6 +169,118 @@ func TestPublishingCacheNonIssuerPublishesNothing(t *testing.T) {
}
}
+// hangingStore blocks every write until its context ends — a bucket that has
+// stopped answering rather than one that refuses.
+type hangingStore struct {
+ release.Store
+ mu sync.Mutex
+ entered int
+}
+
+func (h *hangingStore) enter() {
+ h.mu.Lock()
+ h.entered++
+ h.mu.Unlock()
+}
+
+func (h *hangingStore) Get(ctx context.Context, _ string) ([]byte, string, error) {
+ h.enter()
+ <-ctx.Done()
+ return nil, "", ctx.Err()
+}
+
+func (h *hangingStore) Put(ctx context.Context, _ string, _ []byte, _ string) (string, error) {
+ h.enter()
+ <-ctx.Done()
+ return "", ctx.Err()
+}
+
+func (h *hangingStore) Delete(ctx context.Context, _ string) error {
+ h.enter()
+ <-ctx.Done()
+ return ctx.Err()
+}
+
+// autocert calls Delete while holding challengeMu for write, and serves
+// tls-alpn-01 handshakes and :80 challenge requests under that same mutex —
+// so a publish that waits on a stalled bucket wedges the issuer's front
+// door. Every bucket call gets its own budget.
+func TestPublishingCacheBoundsHowLongABucketCanStall(t *testing.T) {
+ prev := certPublishTimeout
+ certPublishTimeout = 40 * time.Millisecond
+ t.Cleanup(func() { certPublishTimeout = prev })
+ log.SetOutput(io.Discard)
+ t.Cleanup(func() { log.SetOutput(os.Stderr) })
+
+ s := &hangingStore{}
+ dir := t.TempDir()
+ pc := &publishingCache{inner: autocert.DirCache(dir), s: s}
+
+ for _, step := range []struct {
+ name string
+ run func() error
+ }{
+ {"Put", func() error { return pc.Put(context.Background(), "tok+http-01", []byte("challenge")) }},
+ {"Delete", func() error { return pc.Delete(context.Background(), "tok+http-01") }},
+ } {
+ start := time.Now()
+ if err := step.run(); err != nil {
+ t.Fatalf("%s on a stalled bucket: %v", step.name, err)
+ }
+ if took := time.Since(start); took > 2*time.Second {
+ t.Fatalf("%s waited %s on a stalled bucket — the publish must carry its own budget", step.name, took)
+ }
+ }
+ if s.entered == 0 {
+ t.Fatal("test bug: the bucket was never called")
+ }
+ // The local half completed both times, which is the half that matters.
+ if _, err := pc.Get(context.Background(), "tok+http-01"); !errors.Is(err, autocert.ErrCacheMiss) {
+ t.Fatalf("local delete did not happen: %v", err)
+ }
+}
+
+// ctxAwareStore refuses work on a dead context, the way a real S3 client
+// does and the local DirStore (which ignores its context) does not.
+type ctxAwareStore struct{ release.Store }
+
+func (c ctxAwareStore) Get(ctx context.Context, key string) ([]byte, string, error) {
+ if err := ctx.Err(); err != nil {
+ return nil, "", err
+ }
+ return c.Store.Get(ctx, key)
+}
+
+func (c ctxAwareStore) Put(ctx context.Context, key string, body []byte, ifMatch string) (string, error) {
+ if err := ctx.Err(); err != nil {
+ return "", err
+ }
+ return c.Store.Put(ctx, key, body, ifMatch)
+}
+
+func (c ctxAwareStore) Delete(ctx context.Context, key string) error {
+ if err := ctx.Err(); err != nil {
+ return err
+ }
+ return c.Store.Delete(ctx, key)
+}
+
+// A cancelled order still has to clean up its challenge token: the delete
+// runs on a context detached from the caller's.
+func TestPublishingCacheDeletesEvenUnderACancelledCaller(t *testing.T) {
+ s := ctxAwareStore{release.NewDirStore(t.TempDir())}
+ pc := &publishingCache{inner: autocert.DirCache(t.TempDir()), s: s}
+ if err := pc.Put(context.Background(), "tok+http-01", []byte("challenge")); err != nil {
+ t.Fatal(err)
+ }
+ dead, cancel := context.WithCancel(context.Background())
+ cancel()
+ _ = pc.Delete(dead, "tok+http-01")
+ if _, err := certStoreGet(t, s, "control/certs/tok+http-01"); err == nil {
+ t.Fatal("an abandoned order must still take its token out of the bucket")
+ }
+}
+
// ---- the issuance gate ------------------------------------------------------
func testIssuerEdge(hosts ...string) *Edge {
@@ -177,6 +293,98 @@ func testIssuerEdge(hosts ...string) *Edge {
return e
}
+// seedCachedCert writes a cert for host into cache in autocert's own entry
+// shape (private key PEM, then the chain) — what a previous process, or Task
+// 5's mirror, leaves on a box's disk.
+func seedCachedCert(t *testing.T, cache autocert.Cache, parent, host string) {
+ t.Helper()
+ cert, key := selfSignedWildcard(t, parent, time.Now().Add(60*24*time.Hour))
+ raw, err := encodeWildcardPEM(cert, key)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if err := cache.Put(context.Background(), host, raw); err != nil {
+ t.Fatal(err)
+ }
+}
+
+// ecdsaHello is a handshake that supports ECDSA, so autocert looks under the
+// bare host name rather than "<host>+rsa".
+func ecdsaHello(host string) *tls.ClientHelloInfo {
+ return &tls.ClientHelloInfo{
+ ServerName: host,
+ SignatureSchemes: []tls.SignatureScheme{tls.ECDSAWithP256AndSHA256},
+ SupportedCurves: []tls.CurveID{tls.CurveP256},
+ CipherSuites: []uint16{tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256},
+ }
+}
+
+// offlineManager is a real autocert.Manager pointed at a dead directory, so
+// a test that accidentally reaches the ordering path fails offline and fast
+// instead of dialling Let's Encrypt.
+func offlineManager(t *testing.T, cache autocert.Cache, pol autocert.HostPolicy) *autocert.Manager {
+ return &autocert.Manager{
+ Prompt: autocert.AcceptTOS,
+ Cache: cache,
+ HostPolicy: pol,
+ Client: &acme.Client{DirectoryURL: deadDirectory(t)},
+ }
+}
+
+// A box with no deployment store has nothing to publish to AND nothing to
+// mirror from — fleet boxes (barred from control/certs/** by the door ACL),
+// boxes with no bucket or dir, and bucket-without-pubkey boxes. Gating those
+// would leave them permanently certificate-less, because autocert consults
+// HostPolicy BEFORE its cache lookup: no orders and no serving either.
+func TestNoStoreEdgeKeepsIssuingAndServingWithTheGateShut(t *testing.T) {
+ const parent, host = "bab.oncarlos.com", "app.bab.oncarlos.com"
+ dir := autocert.DirCache(t.TempDir())
+ seedCachedCert(t, dir, parent, host)
+
+ e := testIssuerEdge(host)
+ e.issuer = func() (context.Context, bool) { return nil, false } // never holds
+
+ cache := e.armCertIssuance(dir) // no staticStore
+ if _, wrapped := cache.(*publishingCache); wrapped {
+ t.Fatal("no deployment store: there is nothing to publish through")
+ }
+ if _, ok := e.issuerGate(); !ok {
+ t.Fatal("the gate must be cleared, so the wildcard path is ungated too")
+ }
+ got, err := offlineManager(t, cache, e.issuerHostPolicy()).GetCertificate(ecdsaHello(host))
+ if err != nil {
+ t.Fatalf("a box with no mirror must keep serving its own cache: %v", err)
+ }
+ if got.Leaf == nil || got.Leaf.VerifyHostname(host) != nil {
+ t.Fatalf("served cert does not cover %s", host)
+ }
+}
+
+// The same edge WITH a store stays gated: the refusal is the gate's, and the
+// cert it declines to serve is sitting right there on disk — which is the
+// whole reason Task 5's mirror layer has to sit behind this.
+func TestStoreBackedEdgeStaysGated(t *testing.T) {
+ const parent, host = "bab.oncarlos.com", "app.bab.oncarlos.com"
+ dir := autocert.DirCache(t.TempDir())
+ seedCachedCert(t, dir, parent, host)
+
+ e := testIssuerEdge(host)
+ e.staticStore = release.NewDirStore(t.TempDir())
+ e.issuer = func() (context.Context, bool) { return nil, false }
+
+ cache := e.armCertIssuance(dir)
+ if _, wrapped := cache.(*publishingCache); !wrapped {
+ t.Fatal("a deployment store means every artifact publishes")
+ }
+ if _, err := cache.Get(context.Background(), host); err != nil {
+ t.Fatalf("test bug: the local cert should still be readable: %v", err)
+ }
+ _, err := offlineManager(t, cache, e.issuerHostPolicy()).GetCertificate(ecdsaHello(host))
+ if !errors.Is(err, errNotIssuer) {
+ t.Fatalf("non-issuer with a mirror behind it = %v, want errNotIssuer", err)
+ }
+}
+
func TestHostPolicyUngatedIsTodaysBehaviour(t *testing.T) {
e := testIssuerEdge("app.bab.oncarlos.com")
pol := e.issuerHostPolicy()
diff --git a/internal/edge/edge.go b/internal/edge/edge.go
index ea01f594a..5574f1701 100644
--- a/internal/edge/edge.go
+++ b/internal/edge/edge.go
@@ -1788,18 +1788,13 @@ func RunWithActivatorReady(args []string, act Activator, cache *Cache, ready fun
if err != nil {
log.Fatalf("edge: %v", err)
}
- // Publish every issuance artifact to the control bucket (certsync.go):
- // per-host certs, the ACME account keys, HTTP-01 tokens, and — through
- // the same handle below — the DNS-01 wildcards. Only the deployment's
- // elected issuer writes; every other edge serves from the mirror of that
- // prefix. No deployment store configured means no bucket to publish to:
- // a single box keeping its certs on its own disk, exactly as before.
- if e.staticStore != nil {
- certCache = &publishingCache{inner: certCache, s: e.staticStore, issuer: e.issuerGate}
- log.Printf("edge: publishing issued certs to %s* in the deployment bucket", certPrefix)
- } else {
- log.Printf("edge: cert publishing off (no deployment store configured) — certs stay on this box")
- }
+ // Publish every issuance artifact to the control bucket, and gate
+ // issuance on being the box that may write there (certsync.go): per-host
+ // certs, the ACME account keys, HTTP-01 tokens, and — through the same
+ // handle below — the DNS-01 wildcards. One call because it is one
+ // decision: a box with no store to publish to also has no mirror to
+ // serve from, so it stays ungated and keeps issuing for itself.
+ certCache = e.armCertIssuance(certCache)
m := &autocert.Manager{
Prompt: autocert.AcceptTOS,