[CEL-1513] Present a client certificate to the shared buildkitd (merge before crossplane-gcloud#78) - #21
Conversation
…1513) Step 1 of 3 of the shared buildkitd mTLS cutover. The server half is CellarNode/crossplane-gcloud#78; its runbook is docs/buildkit-mtls.md there. buildkitd applies TLS per listener and one daemon cannot serve plaintext and TLS on the same TCP port, so the server cutover is atomic and spans two repos. This lands first and is correct on both sides of it: the TLS builder is used when the ARC container-hook extension has mounted the client leaf at /run/buildkit/certs, and the current plaintext create runs otherwise. `docker buildx create` records an endpoint without dialling it, so both paths are keyed on `inspect --bootstrap` rather than create's exit status. The fallback also bootstraps, so a build that can reach buildkitd on neither transport fails at this step instead of at the first `docker buildx build`. Adds .github/tests/buildkit-mtls-client.test.rb as the client half of the invariant (the server half is scripts/verify_buildkit_mtls.py in crossplane-gcloud). Its PLAINTEXT_FALLBACK_EXPECTED constant is the merge-order gate: true here, flipped to false in step 3 when the fallback is deleted. The regression it exists for is a silent downgrade — dropping the --driver-opt flags, dead-coding the TLS branch, or letting the fallback swallow a failed handshake once the server is TLS-only all leave every build green while the connection reverts to unauthenticated plaintext. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughThe deploy workflow now connects to in-cluster buildkitd with conditional mTLS and plaintext fallback. A Ruby contract test validates the workflow structure. A read-only GitHub Actions workflow runs the test for relevant changes. ChangesBuildKit mTLS integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🔵 Low · up to This PR makes backend builds try mTLS first but temporarily fall back to plaintext when the handshake fails. During the staged rollout, a listener that still accepts plaintext could allow same-repository builds to proceed without client authentication, so the change is mergeable with explicit owner awareness and timely removal of the fallback before TLS-only enforcement. Sequence Diagram(s)sequenceDiagram
participant BuildJob
participant DockerBuildx
participant Buildkitd
BuildJob->>DockerBuildx: Recreate k8s-buildkit with mTLS options
DockerBuildx->>Buildkitd: Run inspect --bootstrap over TLS
BuildJob->>DockerBuildx: Recreate with plaintext fallback if TLS fails
DockerBuildx->>Buildkitd: Run inspect --bootstrap over plaintext
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 3 files
Confidence score: 2/5
- In
.github/workflows/deploy-backend.yaml, failed or missing client-certificate setup falls back to recreatingk8s-buildkitovertcp://and continues, which can bypass mTLS and expose the build path; fail closed or require a secure endpoint instead. - In
.github/tests/buildkit-mtls-client.test.rb, the assertions verify the fallback markers and ordering but notconnected=true, so a missing TLS-success path could regress without detection; require the successful assignment in the test.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/tests/buildkit-mtls-client.test.rb">
<violation number="1" location=".github/tests/buildkit-mtls-client.test.rb:92">
P2: The test pins the fallback markers (`connected=false`, `if [ "$connected" != true ]; then`) and that the TLS attempt precedes the fallback, but it never requires the TLS-success assignment `connected=true` in the workflow. Deleting `connected=true` (workflow line 153) passes this test: the guard is still all three cert files, the driver-opt flags are present, and the fallback still bootstraps. On today's plaintext server the build stays green (a silent downgrade while certs are present, mid-sync), and after crossplane-gcloud#78 switches the server to TLS-only every normal build would fall through to the plaintext fallback and fail at bootstrap — the exact silent-downgrade then late-break this test exists to prevent. Add `connected=true` to the required markers so the fallback is proven gated on TLS success.</violation>
</file>
<file name=".github/workflows/deploy-backend.yaml">
<violation number="1" location=".github/workflows/deploy-backend.yaml:161">
P1: Custom agent: **Flag Security Vulnerabilities**
When the client certificates are absent or mTLS bootstrap fails, this fallback re-creates `k8s-buildkit` against the `tcp://` endpoint and continues after only a warning. That leaves the shared BuildKit connection unauthenticated; remove the plaintext fallback before relying on this workflow for the CEL-1513 hardening.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| echo "no buildkitd client certificate at ${BUILDKITD_CERT_DIR} - using plaintext (CEL-1513 step 1 of 3)" | ||
| fi | ||
|
|
||
| if [ "$connected" != true ]; then |
There was a problem hiding this comment.
P1: Custom agent: Flag Security Vulnerabilities
When the client certificates are absent or mTLS bootstrap fails, this fallback re-creates k8s-buildkit against the tcp:// endpoint and continues after only a warning. That leaves the shared BuildKit connection unauthenticated; remove the plaintext fallback before relying on this workflow for the CEL-1513 hardening.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/deploy-backend.yaml, line 161:
<comment>When the client certificates are absent or mTLS bootstrap fails, this fallback re-creates `k8s-buildkit` against the `tcp://` endpoint and continues after only a warning. That leaves the shared BuildKit connection unauthenticated; remove the plaintext fallback before relying on this workflow for the CEL-1513 hardening.</comment>
<file context>
@@ -94,12 +94,74 @@ jobs:
+ echo "no buildkitd client certificate at ${BUILDKITD_CERT_DIR} - using plaintext (CEL-1513 step 1 of 3)"
+ fi
+
+ if [ "$connected" != true ]; then
+ recreate
+ docker buildx inspect --bootstrap k8s-buildkit
</file context>
| fallback_markers = [ | ||
| "connected=false", | ||
| %(if [ "$connected" != true ]; then), | ||
| ] |
There was a problem hiding this comment.
P2: The test pins the fallback markers (connected=false, if [ "$connected" != true ]; then) and that the TLS attempt precedes the fallback, but it never requires the TLS-success assignment connected=true in the workflow. Deleting connected=true (workflow line 153) passes this test: the guard is still all three cert files, the driver-opt flags are present, and the fallback still bootstraps. On today's plaintext server the build stays green (a silent downgrade while certs are present, mid-sync), and after crossplane-gcloud#78 switches the server to TLS-only every normal build would fall through to the plaintext fallback and fail at bootstrap — the exact silent-downgrade then late-break this test exists to prevent. Add connected=true to the required markers so the fallback is proven gated on TLS success.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/tests/buildkit-mtls-client.test.rb, line 92:
<comment>The test pins the fallback markers (`connected=false`, `if [ "$connected" != true ]; then`) and that the TLS attempt precedes the fallback, but it never requires the TLS-success assignment `connected=true` in the workflow. Deleting `connected=true` (workflow line 153) passes this test: the guard is still all three cert files, the driver-opt flags are present, and the fallback still bootstraps. On today's plaintext server the build stays green (a silent downgrade while certs are present, mid-sync), and after crossplane-gcloud#78 switches the server to TLS-only every normal build would fall through to the plaintext fallback and fail at bootstrap — the exact silent-downgrade then late-break this test exists to prevent. Add `connected=true` to the required markers so the fallback is proven gated on TLS success.</comment>
<file context>
@@ -0,0 +1,132 @@
+
+# --- merge-order gate --------------------------------------------------------
+
+fallback_markers = [
+ "connected=false",
+ %(if [ "$connected" != true ]; then),
</file context>
| fallback_markers = [ | |
| "connected=false", | |
| %(if [ "$connected" != true ]; then), | |
| ] | |
| fallback_markers = [ | |
| "connected=false", | |
| "connected=true", | |
| %(if [ "$connected" != true ]; then), | |
| ] |
1 participant
Important
MERGE ORDER: this PR goes FIRST — before CellarNode/crossplane-gcloud#78.
It is step 1 of 3 in the buildkitd mTLS cutover and is written to be correct
against the current, plaintext server. #78 is step 2 and must not merge
until this is on
main. Merging them in the other order breaks everysame-repo backend build until this lands.
certificate does not exist in the cluster yet, so every job takes the
fallback and behaves exactly as it does on
main.arc-runnersand flips buildkitd to TLS-only.fails instead of silently downgrading CI to plaintext.
Part of CEL-1513. Server half: CellarNode/crossplane-gcloud#78 (runbook:
docs/buildkit-mtls.mdthere).Problem
deploy-backend.yamlconnects every same-repoCellarNode/*build to theshared in-cluster buildkitd over plain TCP:
buildkitd has no authorization layer, so reachability is authorization —
anything that can open that socket runs arbitrary build steps against the
shared layer cache. #78 puts the daemon into
RequireAndVerifyClientCert.This PR is the client that has to present a certificate once it does.
Why the cutover needs a compatible first step
buildkitd applies TLS per listener, and one daemon cannot serve plaintext
and TLS on the same TCP port. The server flip is therefore atomic — and it
lives in a different repository, so there is no single commit that moves both
sides. #78's runbook resolves that with a three-step order, and step 1 is this
PR: a client that works against either server.
That gives it four states to be correct in, all four exercised below:
main.github-runnerssynced first. mTLS attempted, warns, falls back. Green.buildkitdsynced first. Fails at this step, not at the first build. Re-runnable.What changed
.github/workflows/deploy-backend.yaml— the "Connect to in-clusterbuildkitd" step. The mTLS invocation is exactly the client contract #78
documents, with cert-manager's key names and absolute paths:
No
servernameoverride: the server leaf's SAN list covers the FQDN dialled.The endpoint and the certificate directory are passed as
env:data ratherthan inlined.
Two details that are load-bearing rather than stylistic:
inspect --bootstrap, not oncreate.docker buildx createonly records an endpoint — it never dials the daemon,so it exits 0 against a server it could never handshake with. Bootstrap is
the first thing that actually connects. Keying the fallback on
createwould mean the mTLS builder always "succeeds" and the first
docker buildx buildfails instead.missing, server already TLS-only) would defer a pure connectivity failure to
the first build step, where it reads as a build error rather than as "re-run
this job".
.github/tests/buildkit-mtls-client.test.rb(new) — the client half ofthe invariant; #78's
scripts/verify_buildkit_mtls.pyis the server half, andthe control only exists if both hold. It asserts the remote driver, all three
--driver-optpaths under cert-manager's key names, that the TLS branch isgated on certificate presence and nothing else, that the connection is proven
by bootstrap, and that fork PRs stay on GitHub-hosted runners with no path to
the credential.
Its
PLAINTEXT_FALLBACK_EXPECTEDconstant is the merge-order gate.truehere asserts the fallback is present and that the mTLS attempt precedes it;
step 3 flips it to
false, at which point the test refuses any branch thatreaches buildkitd without a client certificate. The gate was verified in both
directions (below), so it is not decorative.
The regression this exists for is a silent downgrade. Dropping the
--driver-optflags, dead-coding the TLS branch, or leaving the fallback inplace after the server is TLS-only all leave every build green while the
connection reverts to unauthenticated plaintext. Nothing else in CI notices.
.github/workflows/validate-backend-deploy.yaml(new) — runs that test onPRs touching
deploy-backend.yaml. A new workflow rather than a job onvalidate-static-deploy.yaml, whose name and path filters are static-deployspecific; this avoids renaming an existing check.
Verification
Behavioural dry-run. The step was extracted and run against a stubbed
dockerfor each of the four states in the table, asserting on which buildergot created and whether the process exited 0:
Sabotage coverage. 18 mutations, each breaking one link, all caught:
The last four rows are the ones worth the file existing. Three inverse tests
(clean tree, comment-only edit, unrelated step added) pass, so it is not
asserting on incidental text. Stripping every
abortturns all 18 green,so the suite is not inert.
Merge-order gate, both directions:
Out of scope
deploy-backend.yamlis the only workflow that touches the in-clusterbuildkitd endpoint —
deploy-static-website.yaml's threeself-hosted-k8sjobs build no images. Fork PRs (
build-fork) stay on GitHub-hosted runnerswith no cluster access and never see a client certificate; the new test pins
that. The registry cache and the same-repo/fork trust split are untouched.
🤖 Generated with Claude Code
Summary by cubic
Makes the shared buildkitd connection in
deploy-backend.yamluse mutual TLS when a client certificate is mounted into the job pod, with a plaintext fallback until the server flips. This is step 1 of 3 in the CEL-1513 cutover: it must land before crossplane-gcloud#78 makes buildkitd TLS-only, and step 3 removes the fallback.Key details
docker buildx inspect --bootstrap, sincecreatenever dials the daemon and exits 0 even against a server it can't handshake with.docker buildx build..github/tests/buildkit-mtls-client.test.rbpinning the client contract; itsPLAINTEXT_FALLBACK_EXPECTEDconstant is the merge-order gate, flipped tofalsein step 3.validate-backend-deploy.yamlto run that test whendeploy-backend.yamlchanges.Written for commit 599a4af. Summary will update on new commits.