Skip to content

Add gitsign commit-signature verification - #6121

Open
samuv wants to merge 3 commits into
skills-sig/08-verifierfrom
skills-sig/10b-gitsign-verify
Open

Add gitsign commit-signature verification#6121
samuv wants to merge 3 commits into
skills-sig/08-verifierfrom
skills-sig/10b-gitsign-verify

Conversation

@samuv

@samuv samuv commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Note

Stack 2 of RFC THV-0080 (tracking issue #5899) — stacked on #6091 (verifier wrapper); merges after it, into it, or rebases onto main once #6091 lands. This completes the git half of the verification story that #6084 deliberately deferred.

Summary

Git-installed skills need the same install-time verification as OCI artifacts. This PR adds Verifier.VerifyGit, full cryptographic verification of gitsign commit signatures:

  • CMS verification over the commit payload: the armored PKCS#7 signature is parsed and verified (message digest + signature over signed attributes) via github/smimesign/ietf-cms — the same library gitsign wraps. Importing gitsign's own pkg/git was evaluated and rejected: its package graph compiles cosign and the GCP/Azure/AWS KMS SDKs into the binary (~54 new module entries). ietf-cms was already in our dependency graph.
  • Chain of trust to Fulcio: the signing certificate chain is verified against the Fulcio roots from toolhive-core's embedded trusted material — hermetic, no network.
  • Identity extraction reuses core's normalization (GitHub Actions workflow paths, issuer OID) by summarizing the certificate through sigstore-go's SummarizeCertificate and core's IdentityFromResult.
  • Expected-identity enforcement is an explicit comparison (ErrSignerMismatch): unlike the OCI path there is no Sigstore bundle to bind a policy into. Nil expected is trust-on-first-use, same as OCI.
  • Payload plumbing: git.HeadCommit and gitresolver.ResolveResult gain the signed payload (encoded commit minus the signature header — the exact bytes the signature covers), so the install flow can hand the verifier signature + payload from one lookup.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Test improvements
  • CI/CD or build system changes

Test plan

  • Unit tests pass locally (task test on the stack tip)
  • Linting passes (task lint-fix, 0 issues)
  • New unit tests: full round trip with a synthetic Fulcio-style CA (real CMS signatures produced by the same ietf-cms package, real X.509 chains) — verify, tampered-payload rejection, untrusted-root rejection, garbage-signature rejection, identity extraction (SAN + Fulcio issuer OID), expected-identity match/mismatch matrix, unsigned/empty-payload guards; payload coverage in the git client and resolver tests (signed payload excludes the gpgsig header, present for unsigned commits too). Zero network.

Does this introduce a user-facing change?

No — nothing calls VerifyGit yet; enforcement arrives with the install-verification PR.

Special notes for reviewers

  • Signing-time semantics: verification time is anchored inside the signing certificate's own validity window, matching gitsign's verifier (its fork of ietf-cms does the same per-signer). Fulcio certs live ~10 minutes, so "valid at time.Now" would reject every real signature. Proving the actual signing time is the transparency log's job: validating the Rekor proof embedded in CMS unsigned attributes is a tracked follow-up, because the reconstruction helpers live in gitsign's internal/ and reimplementing them pulls in the cosign dependency this PR deliberately avoids. Flagging explicitly since the approved plan wanted Rekor validation in-stack — this PR ships the signature+chain half; happy to discuss whether the follow-up should block PR11.
  • The synthetic-CA test fixtures produce real signatures and real chains; only the trust anchor is test-local (verifyGitSignature takes injectable pools; the exported VerifyGit pins the embedded Fulcio roots).

Generated with Claude Code

Git-installed skills need the same install-time verification as OCI
artifacts (RFC THV-0080). This adds Verifier.VerifyGit: the commit's
CMS signature is cryptographically verified over the commit payload
via the ietf-cms package (the same library gitsign wraps — importing
gitsign itself would compile cosign and the cloud KMS SDKs into the
binary), the certificate chain is verified against the Fulcio roots in
toolhive-core's embedded trusted material, and the signer identity is
extracted with core's normalization. Expected identities from the lock
file are compared explicitly — git signatures carry no Sigstore bundle
to bind a policy into.

Verification time is anchored inside the signing certificate's own
validity window, matching gitsign's verifier: Fulcio certificates live
for minutes, and proving actual signing time is the transparency log's
job. Validating the embedded Rekor proof is a tracked follow-up — the
reconstruction helpers are gitsign-internal.

git.HeadCommit and gitresolver.ResolveResult gain the signed payload
(the encoded commit minus its signature header) so install flows can
hand the verifier exactly the bytes the signature covers.

Part of #5899.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@samuv
samuv requested a review from JAORMX as a code owner July 28, 2026 16:44
@github-actions github-actions Bot added the size/M Medium PR: 300-599 lines changed label Jul 28, 2026
JAORMX
JAORMX previously approved these changes Jul 28, 2026
ietf-cms v0.2.0 mutates a package-level variable during BER decoding,
so concurrent ParseSignedData calls race (caught by -race in CI).
Signature verification is not hot-path, so serialize the calls with a
mutex rather than forking the library the way gitsign does.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions github-actions Bot added size/L Large PR: 600-999 lines changed and removed size/M Medium PR: 300-599 lines changed labels Jul 28, 2026
@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 67.01031% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.52%. Comparing base (dafa3d2) to head (1cb4e00).

Files with missing lines Patch % Lines
pkg/skills/verifier/git.go 68.29% 19 Missing and 7 partials ⚠️
pkg/git/client.go 53.84% 3 Missing and 3 partials ⚠️
Additional details and impacted files
@@                    Coverage Diff                     @@
##           skills-sig/08-verifier    #6121      +/-   ##
==========================================================
+ Coverage                   72.41%   72.52%   +0.11%     
==========================================================
  Files                         737      739       +2     
  Lines                       75960    76113     +153     
==========================================================
+ Hits                        55005    55200     +195     
+ Misses                      17055    16984      -71     
- Partials                     3900     3929      +29     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@JAORMX JAORMX 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.

Panel-review comment on the git half of the verification stack (reviewed together with #6091 and #6129 against RFC THV-0080). One finding, inline: the missing Rekor inclusion-proof validation means the cert-validity anchor at CurrentTime: NotBefore+1min never expires, so the git path's replay window is unbounded — a historical Fulcio cert verifies forever. The PR body already flags this; the inline comment argues for landing it as visibly provisional (marked in the lock like unsigned exceptions) rather than blocking #6129 on vendoring gitsign's Rekor helpers, plus a test that pins current behavior with a TODO to flip when Rekor lands.

Everything else in this PR checked out clean: the smimesign/ietf-cms dependency choice over gitsign's pkg/git (cosign + cloud-KMS closure) is right, the cmsMu serialization of ietf-cms's global BER state is documented and correct, identity extraction reusing core's normalization via SummarizeCertificate avoids re-implementing the GitHub Actions path rules, and the synthetic-CA tests produce real signatures/chains with zero network.

Comment thread pkg/skills/verifier/git.go
Git verification checks the signature and certificate chain but not
yet the transparency-log proof of signing time, leaving the replay
window unbounded (an expired Fulcio certificate verifies forever).
Per review, that reduced assurance is now visible instead of silent:
git-verified provenance carries provisional: true in the lock file —
diff-visible like unsigned exceptions — to be removed when Rekor
inclusion-proof validation lands. A test pins the expired-certificate
behavior with a TODO to flip it then.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions github-actions Bot added size/L Large PR: 600-999 lines changed and removed size/L Large PR: 600-999 lines changed labels Jul 29, 2026
@samuv samuv self-assigned this Jul 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L Large PR: 600-999 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants