Enforce we use the right issuer chain#60
Conversation
mcpherrinm
force-pushed
the
strict-chain-selection
branch
from
9d2dbee to
209dbba
Compare
There was a problem hiding this comment.
Pull request overview
Enforces strict certificate-chain issuer pinning after ACME issuance to prevent lego’s silent fallback to a default chain, and updates local integration/dev workflow to handle Pebble’s randomized root CN by generating configuration at startup.
Changes:
- Add post-
Obtainverification that the topmost cert’s issuer matches configuredIssuerCN(andIssuerOwhen set), failing issuance otherwise. - Introduce an
integration/configgensidecar and docker-compose wiring to generate config from Pebble’s management API at startup. - Extend config
DurationwithMarshalJSONand add a config round-trip test to support config generation.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates local testing instructions to account for config generation sidecar. |
| integration/test-certs-site-config.json | Removes previously checked-in integration config example. |
| integration/configgen/main.go | New helper that fetches Pebble root CN and writes a pinned test-certs-site config. |
| Dockerfile | Builds both test-certs-site and configgen as separate targets/stages. |
| docker-compose.yml | Adds configgen service and uses a shared volume for generated config. |
| config/json_duration.go | Adds JSON marshaling for Duration to enable config generation. |
| config/config.go | Adds optional IssuerO pinning field in Site config. |
| config/config_test.go | Adds round-trip marshal/load test for config objects. |
| acme/issuer.go | Verifies returned chain issuer matches config (CN and optional O) before storing/serving. |
| acme/issuer_test.go | Adds unit tests for issuer-chain verification behavior. |
| acme/acme.go | Wires IssuerO from config into issuer instances. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Lego silently falls back to the CA's default chain if no alternate matches PreferredChain. For this site, that is unacceptable: serving the wrong chain defeats the entire point. After Obtain, verify the topmost certificate's Issuer matches the configured IssuerCN (and IssuerO, when set) and fail issuance if it does not. Pebble randomizes its root CN on each startup, so a configgen sidecar runs alongside Pebble in docker-compose, fetches the root from Pebble's management API, and writes the test-certs-site config to a shared volume. The Dockerfile gains a multi-target build that produces both binaries, and configgen builds the config using the real config.Config types (enabled by adding MarshalJSON to Duration). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mcpherrinm
force-pushed
the
strict-chain-selection
branch
from
209dbba to
ec6ed57
Compare
Pebble's docker image already ships pebble.minica.pem at /test/certs/. Mount that directory as a named volume so test-certs-site can read it via LEGO_CA_CERTIFICATES without keeping a vendored copy of the fixture in the repo. configgen and the integration test now use InsecureSkipVerify when fetching the root CN from pebble's management API: the value they want is what test-certs-site then pins to, so verifying pebble's transport cert adds nothing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mcpherrinm
changed the title
aarongable
left a comment
aarongable
left a comment
There was a problem hiding this comment.
LGTM, code does what it says on the tin.
That said, I'm a little confused about the introduction of the check against the Org field. If the point is to be able to distinguish between all the possible issuers that we use, then the Org field isn't going to help -- even for our Staging certs, we put the (STAGING) tag in the CN, not the O. If the point is to be able to distinguish between any possible certs, then why stop at the O field; why not compare the C, or the SKID?
Comparing just the CN is (imo) good enough for our purposes. Comparing the full Subject bytes and the SKID would be the Most Correct ™️ thing to do. It's unclear to me why doing something in the middle strikes a better balance.
|
Ah, I forgot to mention the O! I want to align with what boulder-observer does here, where it checks both: And since we have the code ourselves, we can do that. |
|
Fair enough! I guess my question then goes to boulder-observer: why do we choose to draw the line at checking the O= field there? But since there's prior art, I'm happy to let this one stand. |
|
Happy to re-open that discussion here and in boulder-observer, but will go with that for now |
Lego silently falls back to the CA's default chain if no alternate matches PreferredChain.
That's not great, and we'd rather be down than serve the wrong chain (eg, if a chain were to become unavailable)
To make the integration test work, we need to generate the config with the right CN, since pebble randomizes it each run. While I'm in here, avoid hardcoding the Pebble root CA in case it ever changes.
Code by Claude
Fixes #20