Skip to content

fix: Additional cert validation and sanity checks#187

Merged
awilliams1-cb merged 3 commits into
mainfrom
aw/cert-chain-validation
Jul 14, 2026
Merged

fix: Additional cert validation and sanity checks#187
awilliams1-cb merged 3 commits into
mainfrom
aw/cert-chain-validation

Conversation

@awilliams1-cb

Copy link
Copy Markdown
Collaborator
  • Validate that externally provided certs link to the repo-provided trusted intermediates prior to adding them the certificate chain used in signature validation
  • Short circuit validation early if provided root and intermediate certificate were not successfully extracted or are null
  • Added new tests to validate logic, existing tests still pass

@awilliams1-cb
awilliams1-cb requested a review from leopoldjoy July 13, 2026 16:48
@cb-heimdall

cb-heimdall commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

✅ Heimdall Review Status

Requirement Status More Info
Reviews 1/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
2 if repo is sensitive 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1

// Pin: the runtime intermediate must be issued by our pinned static
// intermediate; this also rejects a self-signed intermediate injected here.
const runtime = new X509Certificate(new Uint8Array(runtimeIntermediate.rawBytes));
if (runtime.ca && runtime.verify(staticInter.publicKey)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we fail closed if this runtime pin does not verify? Otherwise the root may still be trusted and verification can fall back to shorter static/root-issued paths.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our trusted-root.json has two certificates to try and build the cert chain against. If we fail early then we would short-circuit before both certificates have been checked. Implicitly a shorter path wouldn't be able to validate against the signature unless the shortened path actually did sign. In that case, the shorter path would only be the cb static intermediate which would fail even before the signature because the leaf wouldn't connect back up to it.

Comment thread src/lib/task-origin-validate.ts Outdated
if (runtime.ca && runtime.verify(staticInter.publicKey)) {
certChain.push(runtimeIntermediate);
}
certChain.push(rootCert);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we confirm the intended trust model here? Since Sigstore treats each cert in certChain as a trust anchor, adding the external root may allow paths that skip the runtime/static hierarchy.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The certs being added are being validated against the trusted intermediate, so if the root cert doesn't link to the trusted intermediate, or static intermediate isn't linked to the trusted intermediate, then they aren't added as a trust anchor. So there is no circumvention of the trusted intermediate with the addition of the hierarchy verification.

Now if the verified root or any verified intermediate (including the ones that are already in this repo) were to directly issue the leaf, then yes that would count. Our verification only ensures that a given signature originates from the cert chain that we build using the trusted roots stored in this repo.

jackchuma
jackchuma previously approved these changes Jul 13, 2026

@jackchuma jackchuma left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - my comments are just minor nits that could optionally be addressed in a follow up

Comment thread __tests__/task-origin-validate.test.ts Outdated
Comment thread __tests__/task-origin-validate.test.ts Outdated
Comment thread src/lib/task-origin-validate.ts Outdated
@awilliams1-cb
awilliams1-cb merged commit 5661022 into main Jul 14, 2026
6 checks passed
@awilliams1-cb
awilliams1-cb deleted the aw/cert-chain-validation branch July 14, 2026 16:20
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.

4 participants