Skip to content

fix(pilot-ca): validate IsCA and BasicConstraints in loadRoot (PILOT-139)#5

Merged
TeoSlayer merged 2 commits into
mainfrom
openclaw/pilot-139-20260528-173526
May 28, 2026
Merged

fix(pilot-ca): validate IsCA and BasicConstraints in loadRoot (PILOT-139)#5
TeoSlayer merged 2 commits into
mainfrom
openclaw/pilot-139-20260528-173526

Conversation

@matthew-pilot
Copy link
Copy Markdown
Collaborator

What

loadRoot() parses root.crt and uses it as a signing root for issueBeacon without checking that the certificate is a CA. If root.crt is swapped with a self-signed non-CA cert, issueBeacon silently produces invalid cert chains that get rejected by spec-compliant TLS verifiers.

Fix

Added two guards in loadRoot() after x509.ParseCertificate:

  • !crt.IsCA → error: "root.crt: certificate is not a CA (IsCA=false)"
  • !crt.BasicConstraintsValid → error: "root.crt: basic constraints missing or invalid"

Verification

  • go build ./...
  • go vet ./...
  • go test ./... ✅ (all 23 tests pass, including new TestLoadRoot_NotCA)
  • InitRoot already sets IsCA=true, BasicConstraintsValid=true — existing flow unchanged.

Files changed

  • main.go — +6 lines in loadRoot()
  • zz_load_test.go — +60 lines (TestLoadRoot_NotCA: non-CA cert → rejected)

Closes PILOT-139

matthew-pilot added 2 commits May 28, 2026 17:36
Add TestLoadRoot_NotCA: generates a self-signed leaf-like cert
(IsCA=false), writes it as root.crt, and asserts loadRoot returns
an error. Currently fails — loadRoot does not validate IsCA.
…139)

loadRoot parses root.crt and uses it as signing root for
issueBeacon without checking that the certificate is a CA.
A swapped root.crt (self-signed non-CA) would silently produce
invalid cert chains rejected by compliant TLS verifiers.

Add two guards after x509.ParseCertificate:
- !crt.IsCA → error
- !crt.BasicConstraintsValid → error

Fixes PILOT-139.
@codecov
Copy link
Copy Markdown

codecov Bot commented May 28, 2026

Codecov Report

❌ Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
main.go 50.00% 1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@matthew-pilot
Copy link
Copy Markdown
Collaborator Author

📋 PR Status — pilot-protocol/pilot-ca#5

State: OPEN · Merge: MERGEABLE (UNSTABLE)
CI: 2/3 (test ✅, snyk ✅, codecov/patch ❌)
Canary: not run (no canary-passed / canary-running labels)
Jira: PILOT-139 — linked in PR body
Last operator activity: none (1 bot comment from codecov; no operator reviews)

⚠️ The codecov/patch failure is on 2 lines in main.go — the guard clauses (IsCA/BasicConstraintsValid checks) that have partial coverage. The test covers the first isCA path but not !BasicConstraintsValid. Consider adding a second test case for that branch.

@matthew-pilot
Copy link
Copy Markdown
Collaborator Author

🔍 Change Walkthrough — pilot-protocol/pilot-ca#5

2 files changed, +66 / −0


main.go:299-303 (+6 lines — guard clauses in loadRoot)

if !crt.IsCA {
    return nil, nil, fmt.Errorf("root.crt: certificate is not a CA (IsCA=false)")
}
if !crt.BasicConstraintsValid {
    return nil, nil, fmt.Errorf("root.crt: basic constraints missing or invalid")
}

Purpose: loadRoot() parses root.crt and uses it as a signing CA for issueBeacon(). Before this fix, there was no validation that the parsed certificate is actually a CA. If root.crt were swapped with a self-signed non-CA certificate (e.g. accidentally, or by a misconfigured deployment), issueBeacon would silently produce invalid cert chains — these get rejected by spec-compliant TLS verifiers but the error is obscure (e.g. x509: certificate signed by unknown authority on the peer, not at CA startup).

The guards reject the cert early at load time:

  • !crt.IsCA catches a cert where the IsCA basic constraint is absent or false
  • !crt.BasicConstraintsValid catches a cert where basic constraints are missing altogether (malformed)

If InitRoot created the cert (as it does in normal operation), both fields are already true — existing flow is unchanged.


zz_load_test.go:156-211 (+60 lines — TestLoadRoot_NotCA)

New test:

  1. Generates an ECDSA P-256 key pair and a self-signed leaf-like certificate with IsCA: false
  2. Writes root.crt + root.key to a temp directory
  3. Calls loadRoot() and asserts it returns an error containing "IsCA"

Coverage gap: The test only exercises the !crt.IsCA path. The !crt.BasicConstraintsValid branch is untested (this is why codecov/patch reports partial coverage). A follow-up test case generating a cert with BasicConstraintsValid: false would close the gap — but this is a noise-level CI failure given the simplicity of the guard.


Risk assessment

  • Blast radius: loadRoot() is called once at startup — fail-fast, no runtime impact
  • Backward compat: full — InitRoot already sets both fields true
  • Breaking: no — this only adds an early rejection path for misconfigured deployments

@TeoSlayer TeoSlayer merged commit 5800889 into main May 28, 2026
2 of 3 checks passed
@TeoSlayer TeoSlayer deleted the openclaw/pilot-139-20260528-173526 branch May 28, 2026 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants