Conversation
Updates the integration-test pkimetal sidecar from v1.20.0 to v1.41.0. Newer ctlint emits a warning for every certificate issued by an issuer that isn't in CCADB, which fires for all of our test certificates; ignore it in both zlint configs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds a Socket option to PKIMetalConfig that, when set, makes the linter dial pkimetal over a unix domain socket via a custom http.Transport. This lets boulder-ca and cert-checker run pkimetal as a sidecar with networking disabled, reducing the production attack surface. Reconfigures the integration-test pkimetal sidecar to exercise this: runs it with network_mode: none, listening only on a shared-volume unix socket configured via /config/config.yaml (env vars don't bind keys without viper defaults, and webserverPath has none). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Socket is now the only way to configure PKIMetal, and the preceding commit was the sole place we still used Addr. Drop the field, its conditional paths in httpClient and execute, and the Addr-half of CheckApplies. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Set driver_opts on the pkimetal-socket named volume so the tmpfs it backs is initialized with uid=1001 ownership, matching the pkimetal user in the image. pkimetal can then create the socket without needing the container to run as root, and we can drop the SocketPermissions override and fall back to pkimetal's 0o600 default (the boulder container connects as root, which bypasses mode checks anyway). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds Unix-domain-socket support for communicating with the pkimetal sidecar so pkimetal can run with network_mode: none, reducing network/supply-chain exposure in integration test deployments.
Changes:
- Switch pkimetal lint configuration from HTTP
addrto a Unix socketsocketpath and update lint client code to dial viaunix. - Update docker-compose to mount a shared tmpfs volume for the pkimetal socket and run pkimetal with networking disabled.
- Add a small
wait-for-socket.shhelper and update the test entrypoint to wait for the socket instead of TCP.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
test/wait-for-socket.sh |
New helper script to block until a Unix socket exists. |
test/pkimetal-config.yaml |
pkimetal config to disable TCP listener and bind to a Unix socket path. |
test/entrypoint.sh |
Wait for pkimetal’s Unix socket instead of bpkimetal:8080. |
test/config/zlint.toml |
Update pkimetal lint config to use socket instead of addr. |
test/config-next/zlint.toml |
Same socket-based pkimetal lint config update for “next”. |
linter/lints/rfc/lint_crl_via_pkimetal.go |
Applies pkimetal CRL lint only when a socket is configured. |
linter/lints/rfc/lint_cert_via_pkimetal.go |
Implement Unix-socket HTTP transport and switch pkimetal URL construction accordingly. |
docker-compose.yml |
Add shared tmpfs volume for the socket; run pkimetal with network_mode: none and mount config. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| @@ -0,0 +1,18 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
this is a pretty straightforward port of wait-for-it.sh to support a socket. I think we could probably do this in docker-compose with a healthcheck and dependencies instead, but I figured that's too much changing at once
| // both certs and CRLs using PKIMetal. | ||
| type PKIMetalConfig struct { | ||
| Addr string `toml:"addr" comment:"The address where a pkilint REST API can be reached."` | ||
| Socket string `toml:"socket" comment:"Path to a unix socket where pkimetal is listening."` |
There was a problem hiding this comment.
Fully replacing the Addr config key is not backwards compatible. I suspect that's fine -- I know we haven't deployed this, and probably neither has anyone else -- but it's worth noting. And maybe dropping a note to Certainly.
There was a problem hiding this comment.
Yeah. If we wanted to, it would be easy to support both at the same time, but it's a bit of extra code. An earlier version of the PR had both supported: bf62423
| clientOnce sync.Once | ||
| client *http.Client |
There was a problem hiding this comment.
I don't think it makes sense to store these on the Config object.
The only reason that .execute() is a method on the config object is that it is shared between both certViaPKIMetal and crlViaPKIMetal. It doesn't semantically belong there; having it be a method on the config struct was just a convenience.
I think this PR should definitely store the http.Client on the certViaPKIMetal struct (and the crlViaPKIMetal struct), rather than on the config struct. And if you want to go farther for true code-matches-semantics, it should make execute a standalone function and supply all of the config fields as arguments.
There was a problem hiding this comment.
ok. Yeah I was definitely just copying execute(), but can do
There was a problem hiding this comment.
OK, I've made a refactor here to actually introduce a new linter/pkimetal package which contains just the code needed to Execute. I think this ends up being a lot neater, though it still has the somewhat unfortunate sync.Once that I don't love to get a reusable http client, but with the way Zlint's Configure() lifecycle is set up, I don't think we're going to do better without sidestepping zlint's config
To minimize supply chain risk, we want to run pkimetal with
networking: none.Support using a Unix socket to talk to pkimetal.
I've pulled the pkimetal client out of the lint_cert into a shared package to make it more obvious what's shared code between the crl and cert lint.
Fixes #8530
Coauthered with Claude.