fix(ra): bind ACME issuance to owners and verify certificate chains - #126
csnitker-godaddy wants to merge 3 commits into
Conversation
Signed-off-by: Connor Snitker <csnitker@godaddy.com>
kperry-godaddy
left a comment
kperry-godaddy
left a comment
There was a problem hiding this comment.
Owner-bound ACME issuance and always-on chain verification close all three problems in #122 for the shipped wiring, and the removed code confirms each of them was real. What shapes my read is how owner isolation is delivered: per-owner Let's Encrypt accounts, reached through an optional interface. Both work today; both carry costs that are cheaper to settle now than after #127 and #128 build on this port shape.
Three things I'd fix before this ships, all inline. Three non-blocking notes below.
Not blocking, worth a look
- Logging: the new files add no log statements. Gate denials, the owner-mismatch rejection, per-owner account creation and the substitution of RA challenges for a born-ready provider order are all silent, and the handler logs only >= 500, so the 422s this change introduces leave no trace in RA logs.
gateOrderChallengesalso still logs through the package-global logger rather thans.logger. - Upgrade path: an ACME order pending from before this change passes the gate after the owner publishes the record, then fails at finalize with a 422 that blames the provider and marks the order FAILED terminally; a born-ready ISSUING row is told to publish from an empty
challenges[]. An explicit case forlen(order.Challenges) == 0with terminal guidance, a distinct sentinel from the adapter so the service does notMarkFailedon it, and a one-paragraph upgrade note (drain pending ACME orders first) would spare the first operator to hit it. - ANS-1 §7 and conformance item 2 in ans-registry still describe the authorization-reuse exception this PR removes. A tracking issue there keeps the reference implementation and the normative text from drifting apart.
| func (a *ACMEIssuer) ownerIssuer(accountID string) (*ACMEIssuer, error) { | ||
| a.ownersMu.Lock() | ||
| defer a.ownersMu.Unlock() | ||
| if a.owners == nil { | ||
| cache, err := lru.New[string, *ACMEIssuer](256) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| a.owners = cache | ||
| } | ||
| if issuer, ok := a.owners.Get(accountID); ok { | ||
| return issuer, nil | ||
| } | ||
| issuer, err := NewACMEIssuer(a.directoryURL, a.email, filepath.Join(a.dataDir, "owners", accountID), a.options...) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("cert: load owner ACME account: %w", err) | ||
| } | ||
| a.owners.Add(accountID, issuer) | ||
| return issuer, nil | ||
| } |
There was a problem hiding this comment.
ownerIssuer creates a fresh ACME account for every first-time owner, and the child's first CreateOrder registers it inside the registration or renewal request. Against Let's Encrypt production that runs into the limit of 10 new accounts per IP address per 3 hours, which has no override (https://letsencrypt.org/docs/rate-limits/): the eleventh new owner in a window gets a generic 500 CERT_ORDER_FAILED with no Retry-After, no distinct error, and no log line naming the owner. Existing owners are unaffected because the key is persisted, so this is an onboarding ceiling, and one that self-serve identities can exhaust deliberately.
Worth fixing before this ships, and it starts with a decision rather than code. If per-owner accounts stay (they do give each owner its own 300-orders-per-3h budget), make the limit first-class: detect the ACME 429/503 in ensureRegistered, return a retryable sentinel the service maps to 503 with Retry-After, log at WARN with the owner-account prefix, and document the ceiling next to the acme config block. The alternative is one provider account plus a mandatory RA-issued, owner-bound challenge on every order, which removes the accounts, the LRU and the scoped-ref format at the cost of two records for pending orders.
var ae *acme.Error
if errors.As(err, &ae) && (ae.StatusCode == http.StatusTooManyRequests || ae.StatusCode == http.StatusServiceUnavailable) {
a.logger.Warn().Err(err).Str("retryAfter", ae.Header.Get("Retry-After")).Msg("acme account registration throttled by provider")
return fmt.Errorf("cert: acme account registration: %w", port.ErrProviderThrottled)
}| func (s *RegistrationService) createServerOrder(ctx context.Context, ownerID, fqdn string) (*domain.CertificateOrder, error) { | ||
| if scoped, ok := s.serverCA.(port.OwnerScopedServerCertificateIssuer); ok { | ||
| return scoped.CreateOrderForOwner(ctx, ownerID, fqdn) | ||
| } | ||
| return s.serverCA.CreateOrder(ctx, fqdn) | ||
| } |
There was a problem hiding this comment.
Owner isolation is discovered here by a two-value type assertion, with the shared-account CreateOrder as the silent fallback. The finalize side of the same change took the opposite shape: OwnerID is a plain field on FinalizeOrderRequest. Nothing pins *cert.ACMEIssuer to OwnerScopedServerCertificateIssuer at compile time, all five test fakes take the fallback, and any decorator around the issuer (logging, metrics, retry) routes every order through the shared account. For the shipped adapter that becomes a 100% verify-acme outage via the finalize guard rather than a bypass; a future account-caching adapter that omits the method reopens the second reproduction in #122 with no error and no log.
Worth fixing before this ships, because #127 and #128 build on this port. Put the owner on the port itself, mirroring FinalizeOrderRequest, and delete the side interface and the probe:
type CreateOrderRequest struct {
OwnerID string
FQDN string
}
type ServerCertificateIssuer interface {
CreateOrder(ctx context.Context, req CreateOrderRequest) (*domain.CertificateOrder, error)
FinalizeOrder(ctx context.Context, req FinalizeOrderRequest) (*IssuedCert, error)
GetCACertificate(ctx context.Context) (string, error)
}ServerSelfCA ignores the owner (its tokens are per-order random), ACMEIssuer.CreateOrder takes over the owner-scoped body with an unexported child helper so the per-owner child does not re-dispatch, and the compiler lists the call sites: the service fakes in order_flow_test.go, acme_test.go, cert_test.go, acme_owner_test.go, le_live_smoke_test.go and cmd/ans-ra/validator_test.go. If that has to wait, var _ port.OwnerScopedServerCertificateIssuer = (*ACMEIssuer)(nil) plus one service test asserting the scoped method is preferred is the minimum pin.
| func (a *ACMEIssuer) finalizeOwnerOrder(ctx context.Context, req port.FinalizeOrderRequest) (*port.IssuedCert, error) { | ||
| accountID, orderURL, ok := strings.Cut(strings.TrimPrefix(req.OrderRef, scopedOrderPrefix), ":") | ||
| id, err := hex.DecodeString(accountID) | ||
| if !ok || err != nil || len(id) != sha256.Size || orderURL == "" { | ||
| return nil, errors.New("cert: invalid owner-scoped ACME order reference") | ||
| } | ||
| // Decode and re-encode rather than using the input as a filesystem path. | ||
| owner, err := a.ownerIssuer(hex.EncodeToString(id)) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| req.OrderRef = orderURL | ||
| // The parent has checked the authenticated owner against this handle. | ||
| // The child manages only that account and uses the provider's URL. | ||
| req.OwnerID = "" | ||
| issued, err := owner.FinalizeOrder(ctx, req) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| root, err := owner.GetCACertificate(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| a.mu.Lock() | ||
| a.chainRootPEM = root | ||
| a.mu.Unlock() | ||
| return issued, nil | ||
| } |
There was a problem hiding this comment.
ACMEIssuer is now both a single-account ACME client and a registry and factory of itself: six registry fields, children of the same type (which can spawn owners/<id>/owners/<id>), FinalizeOrder dispatching on the ans-acme-owner: prefix, and this function clearing req.OwnerID so the child skips the owner check. That reset is the only reason the req.OwnerID != "" condition at acme.go:277 exists, and it leaves a fail-open branch: a scoped ref with no owner finalizes on whatever account the ref names. Both RA callers set OwnerID today, so it is unreachable through HTTP, but it rests on caller discipline at a trust boundary, and every later change to finalize semantics has to reason about two modes distinguished by string content. The parent's own account key is also still generated at boot and never used.
Worth fixing before this ships, and it pairs with the port change above. Extract the registry into a wrapper that implements the port, performs the owner check once, strips the prefix and delegates to the child's plain methods; ACMEIssuer goes back to a single-account client with no prefix dispatch and no OwnerID reset, and newAccount becomes the seam for eviction and reload tests:
type ownerScopedACMEIssuer struct {
directoryURL, email, dataDir string
opts []ACMEIssuerOption
logger zerolog.Logger
mu sync.Mutex
accounts *lru.Cache[string, *ACMEIssuer]
newAccount func(dataDir string) (*ACMEIssuer, error)
}Whatever shape wins, require a non-empty OwnerID for any ref carrying the scoped prefix. One smaller thing in the same function: owner.GetCACertificate failing after a successful finalize turns an issued certificate into a permanent 500 when the provider returns a leaf-only chain (RFC 8555 §9.1 allows it), and nothing reads the copied root in ACME mode. Making the copy best-effort or dropping it avoids that.
Signed-off-by: Connor Snitker <csnitker@godaddy.com>
Signed-off-by: Connor Snitker <csnitker@godaddy.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
There are a couple of concrete issues in changed code/docs (notably error logging behavior for new 503 domain failures and a mismatch in the upgrade doc’s described error code) that should be corrected before merge.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
What changed in this PR
Tightens RA server-certificate issuance by binding ACME orders to the authenticated owner (preventing cross-owner authorization reuse) and enabling strict X.509 chain verification with configurable trust roots, with corresponding spec/docs/demo/test updates.
Changes:
- Owner-scopes ACME CreateOrder/FinalizeOrder (new
OwnerIDplumbing + owner-scoped order refs) and fails closed for legacy/no-proof cases. - Adds retryable provider-throttling classification (
CERT_PROVIDER_THROTTLED) includingRetry-Afterheader propagation. - Enables server certificate chain verification in
ans-rawiring, trusting system roots plus optional configured roots and the selected local self issuer.
| File | Description |
|---|---|
| spec/api-spec-v2.yaml | Documents new 503/409 responses for cert-provider conflicts/throttling. |
| internal/adapter/docsui/openapi/ra.yaml | Mirrors RA OpenAPI response updates for docs UI. |
| internal/port/certauthority.go | Adds OwnerID-scoped Create/Finalize requests and new provider error types. |
| internal/adapter/cert/acme.go | Refactors single-account ACME client internals; adds provider-throttle mapping. |
| internal/adapter/cert/acme_owner.go | Implements owner-scoped ACME issuer with per-owner account isolation. |
| internal/adapter/cert/provider_failure.go | Normalizes ACME 429/503 into ProviderThrottled with validated Retry-After. |
| internal/adapter/cert/validator.go | Adds configurable trust roots and passes them into chain verification. |
| internal/adapter/cert/serverselfca.go | Updates self-CA issuer to new CreateOrderRequest signature. |
| internal/domain/errors.go | Adds RetryAfter field to domain error for header emission. |
| internal/ra/handler/errors.go | Emits Retry-After header for 503 responses. |
| internal/ra/service/order_proof.go | Adds server-order creation helper and “fresh owner proof” wrapping for cached auth. |
| internal/ra/service/registration.go | Uses owner-scoped order creation and persists owner-proof challenges. |
| internal/ra/service/renewal.go | Applies owner-proof gating and passes OwnerID into finalization. |
| internal/ra/service/lifecycle.go | Fails closed for legacy/no-proof orders; replaces global logger with injected logger. |
| internal/ra/service/provider_errors.go | Centralizes mapping of provider errors into domain errors (503/409/500). |
| internal/config/config.go | Adds ca.validation.roots-file for explicit trust roots configuration. |
| cmd/ans-ra/validator.go | Builds validator with system/config/self roots; removes skip-chain-verify wiring. |
| cmd/ans-ra/main.go | Switches RA startup to new validator wiring. |
| config/ra-local.yaml | Documents always-on chain verification and optional extra roots file. |
| scripts/demo/start.sh | Wires demo config for extra trusted roots + TL public base URL. |
| scripts/demo/renewal.sh | Defaults to CSR flow; updates BYOC flow to require trusted cert+optional chain. |
| scripts/demo/renewal-acme-verify.sh | Updates narrative to reflect “fresh owner proof required” semantics. |
| docs/operations/acme-owner-upgrade.md | Adds operational guidance for owner isolation, legacy upgrade, and throttling. |
| go.mod | Promotes golang-lru/v2 to a direct dependency for owner cache. |
| internal/adapter/cert/acme_test.go | Updates tests to new internal method names and flows. |
| internal/adapter/cert/acme_owner_test.go | Adds coverage for owner isolation, persistence, and scoped finalize validation. |
| internal/adapter/cert/provider_failure_test.go | Tests Retry-After validation and throttle classification. |
| internal/adapter/cert/cert_test.go | Updates self-CA tests for new CreateOrder request shape. |
| internal/adapter/cert/le_live_smoke_test.go | Updates live staging smoke test to new ACME helpers. |
| internal/ra/service/order_flow_test.go | Updates service tests to enforce owner-proof gating and new issuer contract. |
| internal/ra/service/provider_errors_test.go | Adds unit tests for provider error classification into domain errors. |
| internal/ra/service/deployment_regression_test.go | Adds regression coverage for cross-owner cached-authorization behavior. |
| internal/ra/handler/retry_hint_test.go | Verifies Retry-After is emitted as a header (not a JSON field). |
| cmd/ans-ra/validator_test.go | Tests validator trust behavior (system/config/self) and invalid config failures. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| A registration with no persisted challenges/proof returns | ||
| `CERT_ORDER_UPGRADE_REQUIRED`; cancel it where supported or allow expiry, then | ||
| register a new version. Cancel/recreate an affected pending renewal. These | ||
| conflicts do not mark an order as a CA-reported terminal failure. |
Registration and renewal now require domain-control proof from the authenticated owner, including when the ACME provider reports an already-authorized order. Authorization belonging to another owner cannot be reused. Retrying an order whose proof the RA has already verified remains supported.
Server certificates must chain to system roots, an explicitly configured root, or the selected local issuer. The demo defaults to CSR issuance; BYOC requires a trusted certificate. Staging and private roots can be configured explicitly.
Contract comparison
The existing request shape in
spec/api-spec-v2.yamlis unchanged:RenewalSubmissionResponsealready definesstatus: [PENDING_VALIDATION, ISSUING_CERTIFICATE]and nullablechallenges.dns01/challenges.http01. Fresh owner proof uses those existing fields. No HTTP fields, routes, enums, envelope fields, or signature formats change.The behavior intentionally tightens authorization reuse. Registry ANS-1 prose that permits an already-authorized order to bypass owner proof needs a matching clarification; this PR does not claim that prose is already updated. Existing pending legacy orders without persisted proof require a new registration/version or renewal rather than silent approval.
Validation
Tests cover cached authorization across owners, pending authorization, restart and retry behavior, and trusted and untrusted certificate chains.
make build,make check, andmake test-racepass on this branch. Coverage is internal 91.01%, domain 100.00%, crypto 98.45%.Second PR in the deployment-fix stack. Preserve the RA and CA data when upgrading.
Fixes #122.
Depends on #125; this PR targets
fix/coverage-gatesto keep its review diff scoped.AI assistance
Assisted-by: Codex (GPT-6), under Connor Snitker's direction.