From 7ac5cbc68f456001fe2343c8dfa970046027128b Mon Sep 17 00:00:00 2001 From: Piotr Zaniewski Date: Tue, 27 Oct 2026 09:31:19 +0100 Subject: [PATCH 1/5] feat(fork-review-check): add read-only cli for reviewed fork commits Fork commits ship in vCluster products without a reviewed PR: kine v0.16.5.vcluster.1 carries 15 direct pushes over upstream v0.16.5, and tailscale v1.96.4-loft.3 carries 14 commits with no PR. A product bump PR approves a version string, not the fork commits behind it, so SOC 2 change management has no evidence for that code. This is build step 1 of the fork review check design (loft-sh/architecture engineering-enablement/designs/fork-review-check.md): a CLI that resolves each fork commit a build consumes and checks the range from the trusted upstream base to it. Every commit must belong to a merged fork PR approved at its final head, after its last force push, by a non-author with write access; the pinned commit itself must be that PR's final head or merge result. It runs read-only, so it can measure today's gap before anything blocks. The test workflow's live job runs it against the three pins the design names with the workflow token, which answers whether a required workflow's token can read approver permission on another repo. References DEVOPS-1740 Claude-Session: https://claude.ai/code/session_017edg9gFKBwMnmsAVXxEJzn --- .github/actions/fork-review-check/README.md | 66 +++ .../actions/fork-review-check/src/check.go | 415 ++++++++++++++++ .../fork-review-check/src/check_test.go | 470 ++++++++++++++++++ .../actions/fork-review-check/src/github.go | 254 ++++++++++ .github/actions/fork-review-check/src/go.mod | 7 + .github/actions/fork-review-check/src/go.sum | 7 + .../actions/fork-review-check/src/gomod.go | 207 ++++++++ .../fork-review-check/src/gomod_test.go | 150 ++++++ .github/actions/fork-review-check/src/main.go | 305 ++++++++++++ .../fork-review-check/src/main_test.go | 93 ++++ .../actions/fork-review-check/src/report.go | 94 ++++ .github/workflows/test-fork-review-check.yaml | 69 +++ Makefile | 7 +- README.md | 12 + 14 files changed, 2154 insertions(+), 2 deletions(-) create mode 100644 .github/actions/fork-review-check/README.md create mode 100644 .github/actions/fork-review-check/src/check.go create mode 100644 .github/actions/fork-review-check/src/check_test.go create mode 100644 .github/actions/fork-review-check/src/github.go create mode 100644 .github/actions/fork-review-check/src/go.mod create mode 100644 .github/actions/fork-review-check/src/go.sum create mode 100644 .github/actions/fork-review-check/src/gomod.go create mode 100644 .github/actions/fork-review-check/src/gomod_test.go create mode 100644 .github/actions/fork-review-check/src/main.go create mode 100644 .github/actions/fork-review-check/src/main_test.go create mode 100644 .github/actions/fork-review-check/src/report.go create mode 100644 .github/workflows/test-fork-review-check.yaml diff --git a/.github/actions/fork-review-check/README.md b/.github/actions/fork-review-check/README.md new file mode 100644 index 00000000..20539d20 --- /dev/null +++ b/.github/actions/fork-review-check/README.md @@ -0,0 +1,66 @@ +# fork-review-check + +CLI behind the fork review check in [loft-sh/architecture `engineering-enablement/designs/fork-review-check.md`](https://github.com/loft-sh/architecture/blob/main/engineering-enablement/designs/fork-review-check.md). It verifies that every fork commit a product build consumes reached its loft-sh fork through a merged pull request approved by someone other than its author. + +This is build step 1 of the design: the CLI, run read-only. The workflow and release receipt that make it a required check (step 4) build on it. + +## Rules + +For every fork commit S the build consumes, over the range from the trusted upstream base to S: + +1. S is the approved final head, or the merge result, of a merged fork PR. +2. Every commit in the range belongs to a merged fork PR approved at its final head, after its last force push, by someone with write or higher permission on the fork who is neither the PR author nor the commit author. For the merge commit GitHub writes when a PR merges, the code author is the PR author, so the approver may also be the merger. +3. With `-as-of`, every approval predates that time, normally the product release. +4. The check fails and names every commit that breaks a rule. + +The upstream comes from the `fork-upstream` repository property, never from GitHub's fork parent. A fork without it fails. The base is: + +| Pin | Base | +| --- | --- | +| Fork release tag (`v0.16.5.vcluster.1`, `v1.96.4-loft.3`) | The upstream tag before the suffix (`v0.16.5`), which must be an ancestor of S | +| Commit or Go pseudo-version | The merge base of S with the upstream default branch | + +A tag without a fork suffix, a missing upstream tag, a base that is not an ancestor, or a range over 250 commits fails. + +## Usage + +```bash +export GITHUB_TOKEN=... # read access to the forks; permission lookups need more, see below + +# One fork ref: a tag or a full commit SHA. +go run . ref -repo loft-sh/kine -ref v0.16.5.vcluster.1 + +# Every loft-sh fork in the effective Go module graph of one or more modules. +GOPRIVATE=github.com/loft-sh/* go run . go -dir /path/to/vnode-internal +``` + +| Flag | Meaning | +| --- | --- | +| `-repo`, `-ref` | `ref` mode: the fork and the tag or commit to check | +| `-dir` | `go` mode: module directory, repeatable. S is the commit Go downloaded (`go mod download -json`, `Origin.Hash`), not today's tag | +| `-upstream` | `fork=upstream[@branch]`, used only while a fork has no `fork-upstream` property. The missing property still fails; the override only keeps the per-commit evidence coming | +| `-as-of` | RFC 3339 time for rule 3 | +| `-receipt` | Write the JSON receipt to this path | +| `-advisory` | Report failures as warnings and exit 0 | + +Exit codes: 0 pass or advisory, 1 a rule failed, 2 the check could not run. + +`go` mode needs `GOPRIVATE=github.com/loft-sh/*` so Go fetches loft-sh modules from git and reports the origin commit; without an origin the module fails. + +## Receipt + +The JSON receipt lists each input with its hash, each fork's `fork-upstream` value and source, the base, and every fork commit with its PR, approver, approval time and the approver's permission as read at check time. The permission endpoint returns current access only, so the receipt is the record and a later rerun can differ. + +## Token + +Reading PRs, reviews and compares on public forks works with any token. The approver permission lookup (`repos/{owner}/{repo}/collaborators/{user}/permission`) is the open question for the required workflow: `test-fork-review-check.yaml` runs the CLI against kine, tailscale and fuse with the workflow token to show whether that token can read it cross-repo. + +## Not yet covered + +Container images by digest, release binaries by sha256, source clones, and the fail-closed net for unknown fork references are step 4 of the design. + +## Tests + +```bash +make test-fork-review-check +``` diff --git a/.github/actions/fork-review-check/src/check.go b/.github/actions/fork-review-check/src/check.go new file mode 100644 index 00000000..b9fd4c45 --- /dev/null +++ b/.github/actions/fork-review-check/src/check.go @@ -0,0 +1,415 @@ +package main + +import ( + "context" + "fmt" + "regexp" + "sort" + "strings" + "time" +) + +// maxCompareCommits is the largest fork range the check accepts. A bigger +// compare is reported as truncated and fails, so no commit is skipped silently. +const maxCompareCommits = 250 + +// forkTagRe splits a fork release tag into the upstream release it names and +// the fork suffix: v0.16.5.vcluster.1 (kine) and v1.96.4-loft.3 (tailscale) +// both name an upstream tag (v0.16.5, v1.96.4). +var forkTagRe = regexp.MustCompile(`^(v\d+\.\d+\.\d+)(\.vcluster\.\d+|-loft\.\d+)$`) + +// githubMergeCommitter is the committer GitHub records on commits it writes +// itself, such as the merge commit of a pull request merged in the UI or API. +const githubMergeCommitter = "web-flow" + +// writePermissions are the collaborator permission values whose approvals +// count toward required reviews. The endpoint reports maintain as write. +var writePermissions = map[string]bool{"admin": true, "write": true} + +// pin is one fork commit a build consumes. +type pin struct { + Repo string // fork repository, owner/name + Ref string // what the build pins: a tag, a Go version or a commit + Tag string // fork release tag; empty for commit and pseudo-version pins + Commit string // S, the commit the build consumes +} + +// upstreamOverride stands in for a missing fork-upstream property so a +// read-only run can still produce per-commit evidence. The missing property +// is reported as a failure either way. +type upstreamOverride struct { + Repo string // upstream repository, owner/name + Ref string // upstream branch for commit pins; empty means its default branch +} + +type baseRecord struct { + Rule string `json:"rule"` // upstream-tag or merge-base + Ref string `json:"ref"` + Commit string `json:"commit"` +} + +type pullRecord struct { + Number int `json:"number"` + URL string `json:"url"` + Author string `json:"author"` + Head string `json:"head"` + MergeCommit string `json:"merge_commit"` + MergedAt time.Time `json:"merged_at"` +} + +type approvalRecord struct { + Approver string `json:"approver"` + SubmittedAt time.Time `json:"submitted_at"` + Commit string `json:"commit"` + Permission string `json:"permission"` // read at check time; the endpoint reports current access only +} + +type commitResult struct { + SHA string `json:"sha"` + Author string `json:"author"` + Subject string `json:"subject"` + Merge bool `json:"merge"` + Pull *pullRecord `json:"pull,omitempty"` + Approval *approvalRecord `json:"approval,omitempty"` + Pass bool `json:"pass"` + Failures []string `json:"failures,omitempty"` +} + +type reviewedTree struct { + Pull *pullRecord `json:"pull,omitempty"` + Via string `json:"via,omitempty"` // head or merge + Pass bool `json:"pass"` +} + +type forkResult struct { + Repo string `json:"repo"` + Ref string `json:"ref"` + Commit string `json:"commit"` + ForkUpstream string `json:"fork_upstream"` + ForkUpstreamSource string `json:"fork_upstream_source"` // property, override or missing + Base *baseRecord `json:"base,omitempty"` + ReviewedTree *reviewedTree `json:"reviewed_tree,omitempty"` + Commits []commitResult `json:"commits"` + Failures []string `json:"failures,omitempty"` + Pass bool `json:"pass"` +} + +// pullData caches what the rules read per pull request. +type pullData struct { + reviews []reviewInfo + lastForcePush time.Time +} + +// checker evaluates the design's rules against live GitHub data. +type checker struct { + api forkAPI + asOf time.Time // rule 3 cutoff; zero skips the rule + overrides map[string]upstreamOverride + + pulls map[string]*pullData + perms map[string]string +} + +func newChecker(api forkAPI, asOf time.Time, overrides map[string]upstreamOverride) *checker { + return &checker{ + api: api, + asOf: asOf, + overrides: overrides, + pulls: map[string]*pullData{}, + perms: map[string]string{}, + } +} + +// checkFork applies the rules to every fork commit between the trusted +// upstream base and p.Commit. +func (c *checker) checkFork(ctx context.Context, p pin) forkResult { + res := forkResult{Repo: p.Repo, Ref: p.Ref, Commit: p.Commit, Commits: []commitResult{}} + + upstream, err := c.api.ForkUpstream(ctx, p.Repo) + if err != nil { + res.Failures = append(res.Failures, fmt.Sprintf("read fork-upstream property: %v", err)) + return res + } + override, hasOverride := c.overrides[p.Repo] + switch { + case upstream != "": + res.ForkUpstream, res.ForkUpstreamSource = upstream, "property" + case hasOverride: + res.ForkUpstream, res.ForkUpstreamSource = override.Repo, "override" + res.Failures = append(res.Failures, "repo has no fork-upstream property; an org owner sets it, and the result below used the override "+override.Repo) + default: + res.ForkUpstreamSource = "missing" + res.Failures = append(res.Failures, "repo has no fork-upstream property; an org owner sets it to the upstream repository, then the check re-runs") + return res + } + + base, cmp, failure := c.resolveBase(ctx, p, res.ForkUpstream, override.Ref) + if failure != "" { + res.Failures = append(res.Failures, failure) + return res + } + res.Base = base + + for _, ci := range cmp.Commits { + res.Commits = append(res.Commits, c.checkCommit(ctx, p.Repo, ci)) + } + + if len(cmp.Commits) > 0 { + res.ReviewedTree = c.checkReviewedTree(ctx, p.Repo, commitOf(cmp.Commits, p.Commit)) + if !res.ReviewedTree.Pass { + res.Failures = append(res.Failures, fmt.Sprintf("%s is not the approved final head or merge result of a merged fork pull request (rule 1)", short(p.Commit))) + } + } + + res.Pass = len(res.Failures) == 0 + for _, cr := range res.Commits { + if !cr.Pass { + res.Pass = false + } + } + return res +} + +// resolveBase finds the trusted upstream base and the fork range above it. A +// tag pin names its base (the tag before the fork suffix), which must be an +// ancestor of S. A commit pin has no tag, so its base is the merge base with +// the upstream branch. +func (c *checker) resolveBase(ctx context.Context, p pin, upstream, upstreamRef string) (*baseRecord, comparison, string) { + var base *baseRecord + var cmp comparison + var err error + + if p.Tag != "" { + m := forkTagRe.FindStringSubmatch(p.Tag) + if m == nil { + return nil, cmp, fmt.Sprintf("fork tag %s has no fork suffix (.vcluster.N or -loft.N), so it names no upstream base; pin a suffixed fork release tag", p.Tag) + } + baseSHA, err := c.api.ResolveTag(ctx, upstream, m[1]) + if err != nil { + return nil, cmp, fmt.Sprintf("resolve upstream tag %s in %s: %v", m[1], upstream, err) + } + if baseSHA == "" { + return nil, cmp, fmt.Sprintf("upstream %s has no tag %s, so the base is ambiguous", upstream, m[1]) + } + base = &baseRecord{Rule: "upstream-tag", Ref: upstream + "@" + m[1], Commit: baseSHA} + cmp, err = c.api.Compare(ctx, p.Repo, baseSHA, p.Commit) + if err != nil { + return nil, cmp, fmt.Sprintf("compare %s...%s: %v", short(baseSHA), short(p.Commit), err) + } + if cmp.Status != "ahead" && cmp.Status != "identical" { + return nil, cmp, fmt.Sprintf("upstream base %s (%s) is not an ancestor of %s (compare status %s)", m[1], short(baseSHA), short(p.Commit), cmp.Status) + } + } else { + ref := upstreamRef + if ref == "" { + info, err := c.api.Repo(ctx, upstream) + if err != nil { + return nil, cmp, fmt.Sprintf("read upstream %s: %v", upstream, err) + } + ref = info.DefaultBranch + } + owner := strings.SplitN(upstream, "/", 2)[0] + cmp, err = c.api.Compare(ctx, p.Repo, owner+":"+ref, p.Commit) + if err != nil { + return nil, cmp, fmt.Sprintf("compare %s:%s...%s: %v", owner, ref, short(p.Commit), err) + } + if cmp.MergeBase == "" { + return nil, cmp, fmt.Sprintf("%s shares no history with %s@%s", short(p.Commit), upstream, ref) + } + base = &baseRecord{Rule: "merge-base", Ref: upstream + "@" + ref, Commit: cmp.MergeBase} + } + + if cmp.Total > maxCompareCommits { + return nil, cmp, fmt.Sprintf("compare from base %s is truncated: %d commits, the limit is %d", short(base.Commit), cmp.Total, maxCompareCommits) + } + if len(cmp.Commits) != cmp.Total { + return nil, cmp, fmt.Sprintf("compare from base %s returned %d of %d commits", short(base.Commit), len(cmp.Commits), cmp.Total) + } + return base, cmp, "" +} + +// checkCommit applies rule 2 to one fork commit: it belongs to a merged fork +// pull request with a qualifying approval. +func (c *checker) checkCommit(ctx context.Context, repo string, ci commitInfo) commitResult { + res := commitResult{SHA: ci.SHA, Author: ci.Author, Subject: ci.Subject, Merge: ci.Merge} + + pulls, err := c.mergedPulls(ctx, repo, ci.SHA) + if err != nil { + res.Failures = []string{fmt.Sprintf("list pull requests for the commit: %v", err)} + return res + } + if len(pulls) == 0 { + res.Failures = []string{"no merged pull request in " + repo + " contains this commit (direct push or bypass)"} + return res + } + + for _, pr := range pulls { + approval, reasons := c.qualifyingApproval(ctx, repo, pr, effectiveAuthor(ci, pr)) + if approval == nil { + res.Failures = append(res.Failures, reasons...) + continue + } + res.Pull, res.Approval, res.Pass, res.Failures = pullRecordOf(pr), approval, true, nil + return res + } + return res +} + +// checkReviewedTree applies rule 1 to S: it is the final head or the merge +// result of a merged fork pull request that carries a qualifying approval. +func (c *checker) checkReviewedTree(ctx context.Context, repo string, ci commitInfo) *reviewedTree { + sha := ci.SHA + pulls, err := c.mergedPulls(ctx, repo, sha) + if err != nil { + return &reviewedTree{} + } + for _, pr := range pulls { + via := "" + switch sha { + case pr.HeadSHA: + via = "head" + case pr.MergeCommit: + via = "merge" + default: + continue + } + if approval, _ := c.qualifyingApproval(ctx, repo, pr, effectiveAuthor(ci, pr)); approval != nil { + return &reviewedTree{Pull: pullRecordOf(pr), Via: via, Pass: true} + } + } + return &reviewedTree{} +} + +// qualifyingApproval returns the earliest approval on pr that meets every +// condition, or the reasons each approval fell short. +func (c *checker) qualifyingApproval(ctx context.Context, repo string, pr pullInfo, commitAuthor string) (*approvalRecord, []string) { + data, err := c.pullData(ctx, repo, pr.Number) + if err != nil { + return nil, []string{fmt.Sprintf("#%d: read reviews: %v", pr.Number, err)} + } + + var reasons []string + var approvals []reviewInfo + for _, r := range data.reviews { + if r.State == "APPROVED" { + approvals = append(approvals, r) + } + } + if len(approvals) == 0 { + return nil, []string{fmt.Sprintf("#%d: merged without an approval", pr.Number)} + } + sort.SliceStable(approvals, func(i, j int) bool { return approvals[i].SubmittedAt.Before(approvals[j].SubmittedAt) }) + + for _, r := range approvals { + reason := "" + switch { + case r.CommitID != pr.HeadSHA: + reason = fmt.Sprintf("approval by %s is on %s, not the final head %s", r.User, short(r.CommitID), short(pr.HeadSHA)) + case strings.EqualFold(r.User, pr.Author): + reason = fmt.Sprintf("approval by %s is by the pull request author", r.User) + case commitAuthor == "": + reason = fmt.Sprintf("approval by %s cannot be shown to be by a non-author: the commit author has no GitHub account", r.User) + case strings.EqualFold(r.User, commitAuthor): + reason = fmt.Sprintf("approval by %s is by the commit author", r.User) + case !data.lastForcePush.IsZero() && !r.SubmittedAt.After(data.lastForcePush): + reason = fmt.Sprintf("approval by %s predates the last force push (%s)", r.User, data.lastForcePush.Format(time.RFC3339)) + case !c.asOf.IsZero() && !r.SubmittedAt.Before(c.asOf): + reason = fmt.Sprintf("approval by %s at %s is not before the release (%s) (rule 3)", r.User, r.SubmittedAt.Format(time.RFC3339), c.asOf.Format(time.RFC3339)) + } + if reason != "" { + reasons = append(reasons, fmt.Sprintf("#%d: %s", pr.Number, reason)) + continue + } + + perm, err := c.permission(ctx, repo, r.User) + if err != nil { + reasons = append(reasons, fmt.Sprintf("#%d: read permission of %s: %v", pr.Number, r.User, err)) + continue + } + if !writePermissions[perm] { + reasons = append(reasons, fmt.Sprintf("#%d: approval by %s does not count: %s permission, write or higher needed", pr.Number, r.User, perm)) + continue + } + return &approvalRecord{Approver: r.User, SubmittedAt: r.SubmittedAt, Commit: r.CommitID, Permission: perm}, nil + } + return nil, reasons +} + +// mergedPulls lists the merged pull requests into repo that contain sha. +func (c *checker) mergedPulls(ctx context.Context, repo, sha string) ([]pullInfo, error) { + all, err := c.api.PullsWithCommit(ctx, repo, sha) + if err != nil { + return nil, err + } + var merged []pullInfo + for _, pr := range all { + if !pr.MergedAt.IsZero() && strings.EqualFold(pr.BaseRepo, repo) { + merged = append(merged, pr) + } + } + sort.Slice(merged, func(i, j int) bool { return merged[i].Number < merged[j].Number }) + return merged, nil +} + +func (c *checker) pullData(ctx context.Context, repo string, number int) (*pullData, error) { + key := fmt.Sprintf("%s#%d", repo, number) + if d, ok := c.pulls[key]; ok { + return d, nil + } + reviews, err := c.api.Reviews(ctx, repo, number) + if err != nil { + return nil, err + } + forced, err := c.api.LastForcePush(ctx, repo, number) + if err != nil { + return nil, err + } + d := &pullData{reviews: reviews, lastForcePush: forced} + c.pulls[key] = d + return d, nil +} + +func (c *checker) permission(ctx context.Context, repo, user string) (string, error) { + key := repo + "/" + strings.ToLower(user) + if p, ok := c.perms[key]; ok { + return p, nil + } + p, err := c.api.Permission(ctx, repo, user) + if err != nil { + return "", err + } + c.perms[key] = p + return p, nil +} + +func pullRecordOf(pr pullInfo) *pullRecord { + return &pullRecord{Number: pr.Number, URL: pr.URL, Author: pr.Author, Head: pr.HeadSHA, MergeCommit: pr.MergeCommit, MergedAt: pr.MergedAt} +} + +func commitOf(commits []commitInfo, sha string) commitInfo { + for _, ci := range commits { + if ci.SHA == sha { + return ci + } + } + return commitInfo{SHA: sha} +} + +// effectiveAuthor is the author the non-author rule compares approvers with. +// The merge commit GitHub writes when a pull request merges carries the +// merger as author but no code of its own, so for that commit the code author +// is the pull request author. Any other commit keeps its git author, merge +// commits pushed by hand included. +func effectiveAuthor(ci commitInfo, pr pullInfo) string { + if ci.Merge && ci.Committer == githubMergeCommitter && ci.SHA == pr.MergeCommit { + return pr.Author + } + return ci.Author +} + +func short(sha string) string { + if len(sha) > 12 { + return sha[:12] + } + return sha +} diff --git a/.github/actions/fork-review-check/src/check_test.go b/.github/actions/fork-review-check/src/check_test.go new file mode 100644 index 00000000..fd9e2e88 --- /dev/null +++ b/.github/actions/fork-review-check/src/check_test.go @@ -0,0 +1,470 @@ +package main + +import ( + "context" + "fmt" + "strings" + "testing" + "time" +) + +// fakeAPI serves canned GitHub data keyed the way forkAPI asks for it. +type fakeAPI struct { + repos map[string]repoInfo + upstreams map[string]string + tags map[string]string // repo@tag -> sha + compares map[string]comparison + pulls map[string][]pullInfo // repo@sha + reviews map[string][]reviewInfo + forcePush map[string]time.Time + perms map[string]string // repo/user + permErr error + permCalled int +} + +func newFake() *fakeAPI { + return &fakeAPI{ + repos: map[string]repoInfo{}, + upstreams: map[string]string{}, + tags: map[string]string{}, + compares: map[string]comparison{}, + pulls: map[string][]pullInfo{}, + reviews: map[string][]reviewInfo{}, + forcePush: map[string]time.Time{}, + perms: map[string]string{}, + } +} + +func (f *fakeAPI) Repo(_ context.Context, repo string) (repoInfo, error) { + r, ok := f.repos[repo] + if !ok { + return repoInfo{}, fmt.Errorf("no repo %s", repo) + } + return r, nil +} + +func (f *fakeAPI) ForkUpstream(_ context.Context, repo string) (string, error) { + return f.upstreams[repo], nil +} + +func (f *fakeAPI) ResolveTag(_ context.Context, repo, tag string) (string, error) { + return f.tags[repo+"@"+tag], nil +} + +func (f *fakeAPI) Compare(_ context.Context, repo, base, head string) (comparison, error) { + c, ok := f.compares[repo+"@"+base+"..."+head] + if !ok { + return comparison{}, fmt.Errorf("no compare %s %s...%s", repo, base, head) + } + return c, nil +} + +func (f *fakeAPI) PullsWithCommit(_ context.Context, repo, sha string) ([]pullInfo, error) { + return f.pulls[repo+"@"+sha], nil +} + +func (f *fakeAPI) Reviews(_ context.Context, repo string, number int) ([]reviewInfo, error) { + return f.reviews[fmt.Sprintf("%s#%d", repo, number)], nil +} + +func (f *fakeAPI) LastForcePush(_ context.Context, repo string, number int) (time.Time, error) { + return f.forcePush[fmt.Sprintf("%s#%d", repo, number)], nil +} + +func (f *fakeAPI) Permission(_ context.Context, repo, user string) (string, error) { + f.permCalled++ + if f.permErr != nil { + return "", f.permErr + } + if p, ok := f.perms[repo+"/"+user]; ok { + return p, nil + } + return "none", nil +} + +const ( + fork = "loft-sh/kine" + upstream = "k3s-io/kine" + baseSHA = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" + c1 = "1111111111111111111111111111111111111111" + c2 = "2222222222222222222222222222222222222222" + head = "eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee" + merge = "dddddddddddddddddddddddddddddddddddddddd" + tag = "v0.16.5.vcluster.1" +) + +var t0 = time.Date(2026, 10, 1, 12, 0, 0, 0, time.UTC) + +// reviewedFork is kine at a tag with two fork commits, both from PR #22, +// approved at its final head by bob (write) after alice opened it. The tag +// points at the PR's merge commit, written by GitHub. +func reviewedFork() *fakeAPI { + f := newFake() + f.upstreams[fork] = upstream + f.tags[upstream+"@v0.16.5"] = baseSHA + f.tags[fork+"@"+tag] = merge + f.compares[fork+"@"+baseSHA+"..."+merge] = comparison{ + Status: "ahead", Total: 3, + Commits: []commitInfo{ + {SHA: c1, Author: "alice", Committer: "alice", Subject: "add kerberos"}, + {SHA: head, Author: "alice", Committer: "alice", Subject: "fix review"}, + {SHA: merge, Author: "bob", Committer: githubMergeCommitter, Subject: "Merge pull request #22", Merge: true}, + }, + } + pr22 := pullInfo{Number: 22, URL: "https://github.com/loft-sh/kine/pull/22", Author: "alice", BaseRepo: fork, HeadSHA: head, MergeCommit: merge, MergedAt: t0.Add(2 * time.Hour)} + for _, sha := range []string{c1, head, merge} { + f.pulls[fork+"@"+sha] = []pullInfo{pr22} + } + f.reviews[fork+"#22"] = []reviewInfo{{User: "bob", State: "APPROVED", CommitID: head, SubmittedAt: t0.Add(time.Hour)}} + f.perms[fork+"/bob"] = "write" + return f +} + +func tagPin() pin { return pin{Repo: fork, Ref: tag, Tag: tag, Commit: merge} } + +func runCheck(f *fakeAPI, p pin) forkResult { + return newChecker(f, time.Time{}, nil).checkFork(context.Background(), p) +} + +func failuresOf(r forkResult) string { + var all []string + all = append(all, r.Failures...) + for _, c := range r.Commits { + all = append(all, c.Failures...) + } + return strings.Join(all, "\n") +} + +func TestReviewedForkPasses(t *testing.T) { + r := runCheck(reviewedFork(), tagPin()) + if !r.Pass { + t.Fatalf("want pass, got failures:\n%s", failuresOf(r)) + } + if r.Base.Rule != "upstream-tag" || r.Base.Commit != baseSHA { + t.Errorf("base = %+v, want upstream-tag %s", r.Base, baseSHA) + } + if r.ReviewedTree.Via != "merge" || r.ReviewedTree.Pull.Number != 22 { + t.Errorf("reviewed tree = %+v, want merge result of #22", r.ReviewedTree) + } + for _, c := range r.Commits { + if c.Approval == nil || c.Approval.Approver != "bob" || c.Approval.Permission != "write" { + t.Errorf("commit %s approval = %+v, want bob with write", short(c.SHA), c.Approval) + } + } +} + +func TestDirectPushFails(t *testing.T) { + f := reviewedFork() + delete(f.pulls, fork+"@"+c1) + r := runCheck(f, tagPin()) + if r.Pass || r.Commits[0].Pass { + t.Fatal("want the direct push to fail") + } + if !strings.Contains(r.Commits[0].Failures[0], "direct push") { + t.Errorf("failure = %q, want it to name a direct push", r.Commits[0].Failures[0]) + } + if !r.Commits[1].Pass { + t.Error("the reviewed commits should still pass on their own") + } +} + +func TestUnmergedPullDoesNotCount(t *testing.T) { + f := reviewedFork() + open := pullInfo{Number: 30, Author: "alice", BaseRepo: fork, HeadSHA: c1} + f.pulls[fork+"@"+c1] = []pullInfo{open} + r := runCheck(f, tagPin()) + if r.Commits[0].Pass { + t.Fatal("an open pull request must not satisfy rule 2") + } +} + +func TestPullIntoAnotherRepoDoesNotCount(t *testing.T) { + f := reviewedFork() + upstreamPR := pullInfo{Number: 22, Author: "alice", BaseRepo: upstream, HeadSHA: head, MergedAt: t0} + f.pulls[fork+"@"+c1] = []pullInfo{upstreamPR} + r := runCheck(f, tagPin()) + if r.Commits[0].Pass { + t.Fatal("a pull request merged into another repository must not count") + } +} + +func TestSelfApprovalFails(t *testing.T) { + f := reviewedFork() + f.reviews[fork+"#22"] = []reviewInfo{{User: "alice", State: "APPROVED", CommitID: head, SubmittedAt: t0.Add(time.Hour)}} + f.perms[fork+"/alice"] = "admin" + r := runCheck(f, tagPin()) + if r.Pass { + t.Fatal("want the PR author's own approval to fail") + } + if !strings.Contains(failuresOf(r), "pull request author") { + t.Errorf("failures do not name the PR author:\n%s", failuresOf(r)) + } +} + +func TestCommitAuthorApprovalFails(t *testing.T) { + // carol opens a PR carrying a commit dave wrote; dave approves it. + f := reviewedFork() + f.compares[fork+"@"+baseSHA+"..."+merge].Commits[0].Author = "dave" + f.reviews[fork+"#22"] = []reviewInfo{{User: "dave", State: "APPROVED", CommitID: head, SubmittedAt: t0.Add(time.Hour)}} + f.perms[fork+"/dave"] = "write" + r := runCheck(f, tagPin()) + if r.Commits[0].Pass { + t.Fatal("want an approval by the commit author to fail") + } + if !strings.Contains(r.Commits[0].Failures[0], "commit author") { + t.Errorf("failure = %q, want it to name the commit author", r.Commits[0].Failures[0]) + } +} + +func TestApproverMayMergeTheirReview(t *testing.T) { + // bob approved and merged; GitHub's merge commit names bob as author but + // carries no code, so bob's approval still counts for it. + r := runCheck(reviewedFork(), tagPin()) + merged := r.Commits[2] + if !merged.Merge || !merged.Pass { + t.Fatalf("GitHub merge commit by the approver should pass: %+v", merged) + } +} + +func TestHandMadeMergeByApproverFails(t *testing.T) { + f := reviewedFork() + f.compares[fork+"@"+baseSHA+"..."+merge].Commits[2].Committer = "bob" + r := runCheck(f, tagPin()) + if r.Commits[2].Pass { + t.Fatal("a merge commit bob pushed himself is his code; his approval cannot cover it") + } +} + +func TestStaleHeadApprovalFails(t *testing.T) { + f := reviewedFork() + f.reviews[fork+"#22"] = []reviewInfo{{User: "bob", State: "APPROVED", CommitID: c1, SubmittedAt: t0.Add(time.Hour)}} + r := runCheck(f, tagPin()) + if r.Pass { + t.Fatal("want an approval on an earlier head to fail") + } + if !strings.Contains(failuresOf(r), "not the final head") { + t.Errorf("failures do not name the stale head:\n%s", failuresOf(r)) + } +} + +func TestApprovalBeforeForcePushFails(t *testing.T) { + f := reviewedFork() + f.forcePush[fork+"#22"] = t0.Add(90 * time.Minute) + r := runCheck(f, tagPin()) + if r.Pass { + t.Fatal("want an approval older than the last force push to fail") + } + if !strings.Contains(failuresOf(r), "force push") { + t.Errorf("failures do not name the force push:\n%s", failuresOf(r)) + } +} + +func TestReadOnlyApproverFails(t *testing.T) { + f := reviewedFork() + f.perms[fork+"/bob"] = "read" + r := runCheck(f, tagPin()) + if r.Pass { + t.Fatal("want an approver with read permission to fail") + } + if !strings.Contains(failuresOf(r), "read permission") { + t.Errorf("failures do not name the permission:\n%s", failuresOf(r)) + } +} + +func TestPermissionLookupErrorFailsClosed(t *testing.T) { + f := reviewedFork() + f.permErr = fmt.Errorf("403 Resource not accessible by integration") + r := runCheck(f, tagPin()) + if r.Pass { + t.Fatal("a permission lookup error must fail the check") + } + if !strings.Contains(failuresOf(r), "403") { + t.Errorf("failures hide the lookup error:\n%s", failuresOf(r)) + } +} + +func TestPermissionIsReadOncePerApprover(t *testing.T) { + f := reviewedFork() + runCheck(f, tagPin()) + if f.permCalled != 1 { + t.Errorf("permission read %d times, want 1 (cached)", f.permCalled) + } +} + +func TestLaterQualifyingApprovalCounts(t *testing.T) { + f := reviewedFork() + f.reviews[fork+"#22"] = []reviewInfo{ + {User: "alice", State: "APPROVED", CommitID: head, SubmittedAt: t0.Add(30 * time.Minute)}, + {User: "carol", State: "CHANGES_REQUESTED", CommitID: head, SubmittedAt: t0.Add(40 * time.Minute)}, + {User: "bob", State: "APPROVED", CommitID: head, SubmittedAt: t0.Add(time.Hour)}, + } + r := runCheck(f, tagPin()) + if !r.Pass { + t.Fatalf("bob's approval qualifies, want pass:\n%s", failuresOf(r)) + } +} + +func TestNoApprovalFails(t *testing.T) { + f := reviewedFork() + f.reviews[fork+"#22"] = nil + r := runCheck(f, tagPin()) + if !strings.Contains(failuresOf(r), "merged without an approval") { + t.Errorf("failures do not say the PR had no approval:\n%s", failuresOf(r)) + } +} + +func TestApprovalAfterReleaseFails(t *testing.T) { + f := reviewedFork() + r := newChecker(f, t0.Add(30*time.Minute), nil).checkFork(context.Background(), tagPin()) + if r.Pass { + t.Fatal("want an approval after the release to fail (rule 3)") + } + if !strings.Contains(failuresOf(r), "rule 3") { + t.Errorf("failures do not cite rule 3:\n%s", failuresOf(r)) + } +} + +func TestIntermediateCommitIsNotAReviewedTree(t *testing.T) { + // Pinning c1, a commit inside PR #22, fails rule 1 even though #22 is approved. + f := reviewedFork() + f.tags[fork+"@v0.16.5.vcluster.0"] = c1 + f.compares[fork+"@"+baseSHA+"..."+c1] = comparison{Status: "ahead", Total: 1, Commits: []commitInfo{{SHA: c1, Author: "alice", Subject: "add kerberos"}}} + r := runCheck(f, pin{Repo: fork, Ref: "v0.16.5.vcluster.0", Tag: "v0.16.5.vcluster.0", Commit: c1}) + if r.Pass { + t.Fatal("want rule 1 to fail on an intermediate PR commit") + } + if r.ReviewedTree.Pass || !strings.Contains(failuresOf(r), "rule 1") { + t.Errorf("rule 1 not reported:\n%s", failuresOf(r)) + } + if !r.Commits[0].Pass { + t.Error("the commit itself is covered by #22; only rule 1 should fail") + } +} + +func TestFinalHeadIsAReviewedTree(t *testing.T) { + f := reviewedFork() + f.tags[fork+"@v0.16.5.vcluster.0"] = head + f.compares[fork+"@"+baseSHA+"..."+head] = comparison{Status: "ahead", Total: 2, Commits: f.compares[fork+"@"+baseSHA+"..."+merge].Commits[:2]} + r := runCheck(f, pin{Repo: fork, Ref: "v0.16.5.vcluster.0", Tag: "v0.16.5.vcluster.0", Commit: head}) + if !r.Pass || r.ReviewedTree.Via != "head" { + t.Fatalf("want the approved final head to pass via head: %+v\n%s", r.ReviewedTree, failuresOf(r)) + } +} + +func TestUnsuffixedTagFails(t *testing.T) { + r := runCheck(reviewedFork(), pin{Repo: fork, Ref: "v0.14.5", Tag: "v0.14.5", Commit: merge}) + if r.Pass || !strings.Contains(failuresOf(r), "no fork suffix") { + t.Fatalf("want an unsuffixed fork tag to fail:\n%s", failuresOf(r)) + } +} + +func TestLoftSuffixNamesUpstreamTag(t *testing.T) { + m := forkTagRe.FindStringSubmatch("v1.96.4-loft.3") + if m == nil || m[1] != "v1.96.4" { + t.Fatalf("v1.96.4-loft.3 split = %v, want base v1.96.4", m) + } + for _, bad := range []string{"v0.14.15-rc.2", "v1.96.4-loft", "v0.16.5.vcluster", "cmd/cigocacher/abc"} { + if forkTagRe.MatchString(bad) { + t.Errorf("%s should not parse as a fork release tag", bad) + } + } +} + +func TestMissingUpstreamTagFails(t *testing.T) { + f := reviewedFork() + delete(f.tags, upstream+"@v0.16.5") + r := runCheck(f, tagPin()) + if r.Pass || !strings.Contains(failuresOf(r), "has no tag v0.16.5") { + t.Fatalf("want a missing upstream tag to fail:\n%s", failuresOf(r)) + } +} + +func TestBaseNotAncestorFails(t *testing.T) { + f := reviewedFork() + cmp := f.compares[fork+"@"+baseSHA+"..."+merge] + cmp.Status = "diverged" + f.compares[fork+"@"+baseSHA+"..."+merge] = cmp + r := runCheck(f, tagPin()) + if r.Pass || !strings.Contains(failuresOf(r), "not an ancestor") { + t.Fatalf("want a diverged base to fail:\n%s", failuresOf(r)) + } +} + +func TestTruncatedCompareFails(t *testing.T) { + f := reviewedFork() + cmp := f.compares[fork+"@"+baseSHA+"..."+merge] + cmp.Total = maxCompareCommits + 1 + f.compares[fork+"@"+baseSHA+"..."+merge] = cmp + r := runCheck(f, tagPin()) + if r.Pass || !strings.Contains(failuresOf(r), "truncated") { + t.Fatalf("want a compare over the limit to fail:\n%s", failuresOf(r)) + } +} + +func TestIdenticalToUpstreamPasses(t *testing.T) { + f := reviewedFork() + f.compares[fork+"@"+baseSHA+"..."+baseSHA] = comparison{Status: "identical"} + r := runCheck(f, pin{Repo: fork, Ref: "v0.16.5.vcluster.0", Tag: "v0.16.5.vcluster.0", Commit: baseSHA}) + if !r.Pass || len(r.Commits) != 0 { + t.Fatalf("no fork commits means nothing to review: %+v\n%s", r, failuresOf(r)) + } +} + +func TestMissingForkUpstreamFails(t *testing.T) { + f := reviewedFork() + delete(f.upstreams, fork) + r := runCheck(f, tagPin()) + if r.Pass || r.ForkUpstreamSource != "missing" || len(r.Commits) != 0 { + t.Fatalf("want a fork without fork-upstream to fail before any lookup: %+v", r) + } +} + +func TestOverrideKeepsEvidenceButStillFails(t *testing.T) { + f := reviewedFork() + delete(f.upstreams, fork) + r := newChecker(f, time.Time{}, map[string]upstreamOverride{fork: {Repo: upstream}}).checkFork(context.Background(), tagPin()) + if r.Pass { + t.Fatal("the missing property must fail even with an override") + } + if r.ForkUpstreamSource != "override" || len(r.Commits) != 3 { + t.Fatalf("want per-commit evidence from the override: %+v", r) + } + for _, c := range r.Commits { + if !c.Pass { + t.Errorf("commit %s should pass on its own: %v", short(c.SHA), c.Failures) + } + } +} + +func TestPropertyWinsOverOverride(t *testing.T) { + f := reviewedFork() + r := newChecker(f, time.Time{}, map[string]upstreamOverride{fork: {Repo: "evil/kine"}}).checkFork(context.Background(), tagPin()) + if r.ForkUpstream != upstream || r.ForkUpstreamSource != "property" || !r.Pass { + t.Fatalf("the property must win: %+v", r) + } +} + +func TestCommitPinUsesMergeBase(t *testing.T) { + // A pseudo-version pin: no tag, so the base is the merge base with the + // upstream default branch. + f := reviewedFork() + f.repos[upstream] = repoInfo{FullName: upstream, DefaultBranch: "master"} + f.compares[fork+"@k3s-io:master..."+merge] = comparison{Status: "diverged", Total: 3, MergeBase: baseSHA, Commits: f.compares[fork+"@"+baseSHA+"..."+merge].Commits} + r := runCheck(f, pin{Repo: fork, Ref: "v0.0.0-20261001120000-dddddddddddd", Commit: merge}) + if !r.Pass { + t.Fatalf("want pass:\n%s", failuresOf(r)) + } + if r.Base.Rule != "merge-base" || r.Base.Commit != baseSHA || r.Base.Ref != upstream+"@master" { + t.Errorf("base = %+v, want merge-base %s on %s@master", r.Base, baseSHA, upstream) + } +} + +func TestCommitPinHonoursUpstreamBranchOverride(t *testing.T) { + f := reviewedFork() + delete(f.upstreams, fork) + f.compares[fork+"@k3s-io:release-1.0..."+merge] = comparison{Status: "diverged", Total: 0, MergeBase: merge} + r := newChecker(f, time.Time{}, map[string]upstreamOverride{fork: {Repo: upstream, Ref: "release-1.0"}}).checkFork(context.Background(), pin{Repo: fork, Ref: merge, Commit: merge}) + if r.Base == nil || r.Base.Ref != upstream+"@release-1.0" { + t.Fatalf("want the override branch as base ref: %+v", r) + } +} diff --git a/.github/actions/fork-review-check/src/github.go b/.github/actions/fork-review-check/src/github.go new file mode 100644 index 00000000..b2fb5246 --- /dev/null +++ b/.github/actions/fork-review-check/src/github.go @@ -0,0 +1,254 @@ +package main + +import ( + "context" + "errors" + "fmt" + "net/http" + "strings" + "time" + + "github.com/google/go-github/v88/github" +) + +// forkUpstreamProperty is the org custom property that names a fork's +// upstream repository. Only org actors can set it, so a repo admin cannot +// point the check at a different upstream. +const forkUpstreamProperty = "fork-upstream" + +type repoInfo struct { + FullName string + DefaultBranch string + Fork bool +} + +type commitInfo struct { + SHA string + Author string // GitHub login of the git author; empty when the email maps to no account + Committer string // GitHub login of the committer + Subject string + Merge bool +} + +type comparison struct { + Status string + Total int + MergeBase string + Commits []commitInfo +} + +type pullInfo struct { + Number int + URL string + Author string + BaseRepo string + HeadSHA string + MergeCommit string + MergedAt time.Time // zero when the pull request is not merged +} + +type reviewInfo struct { + User string + State string + CommitID string + SubmittedAt time.Time +} + +// forkAPI is the read-only GitHub surface the rules need. +type forkAPI interface { + Repo(ctx context.Context, repo string) (repoInfo, error) + // ForkUpstream returns the fork-upstream property, or "" when it is unset. + ForkUpstream(ctx context.Context, repo string) (string, error) + // ResolveTag returns the commit a tag points to, or "" when the tag does not exist. + ResolveTag(ctx context.Context, repo, tag string) (string, error) + Compare(ctx context.Context, repo, base, head string) (comparison, error) + PullsWithCommit(ctx context.Context, repo, sha string) ([]pullInfo, error) + Reviews(ctx context.Context, repo string, number int) ([]reviewInfo, error) + // LastForcePush returns when the pull request head was last force-pushed, or zero. + LastForcePush(ctx context.Context, repo string, number int) (time.Time, error) + // Permission returns the collaborator permission: admin, write, read or none. + Permission(ctx context.Context, repo, user string) (string, error) +} + +type githubAPI struct { + gh *github.Client +} + +func splitRepo(repo string) (string, string) { + owner, name, _ := strings.Cut(repo, "/") + return owner, name +} + +func (a githubAPI) Repo(ctx context.Context, repo string) (repoInfo, error) { + owner, name := splitRepo(repo) + r, _, err := a.gh.Repositories.Get(ctx, owner, name) + if err != nil { + return repoInfo{}, err + } + return repoInfo{FullName: r.GetFullName(), DefaultBranch: r.GetDefaultBranch(), Fork: r.GetFork()}, nil +} + +func (a githubAPI) ForkUpstream(ctx context.Context, repo string) (string, error) { + owner, name := splitRepo(repo) + values, _, err := a.gh.Repositories.GetAllCustomPropertyValues(ctx, owner, name) + if err != nil { + return "", err + } + for _, v := range values { + if v.PropertyName != forkUpstreamProperty { + continue + } + if s, ok := v.Value.(string); ok { + return strings.TrimSpace(s), nil + } + } + return "", nil +} + +func (a githubAPI) ResolveTag(ctx context.Context, repo, tag string) (string, error) { + owner, name := splitRepo(repo) + ref, _, err := a.gh.Git.GetRef(ctx, owner, name, "tags/"+tag) + if isNotFound(err) { + return "", nil + } + if err != nil { + return "", err + } + obj := ref.GetObject() + // Annotated tags point at a tag object; peel until a commit. + for obj.GetType() == "tag" { + t, _, err := a.gh.Git.GetTag(ctx, owner, name, obj.GetSHA()) + if err != nil { + return "", err + } + obj = t.GetObject() + } + if obj.GetType() != "commit" { + return "", fmt.Errorf("tag %s points to a %s, not a commit", tag, obj.GetType()) + } + return obj.GetSHA(), nil +} + +func (a githubAPI) Compare(ctx context.Context, repo, base, head string) (comparison, error) { + owner, name := splitRepo(repo) + var out comparison + opts := &github.ListOptions{PerPage: 100} + for { + cmp, resp, err := a.gh.Repositories.CompareCommits(ctx, owner, name, base, head, opts) + if err != nil { + return comparison{}, err + } + if opts.Page == 0 { + out.Status = cmp.GetStatus() + out.Total = cmp.GetTotalCommits() + out.MergeBase = cmp.GetMergeBaseCommit().GetSHA() + if out.Total > maxCompareCommits { + return out, nil + } + } + for _, rc := range cmp.Commits { + out.Commits = append(out.Commits, commitInfo{ + SHA: rc.GetSHA(), + Author: rc.GetAuthor().GetLogin(), + Committer: rc.GetCommitter().GetLogin(), + Subject: firstLine(rc.GetCommit().GetMessage()), + Merge: len(rc.Parents) > 1, + }) + } + if resp.NextPage == 0 { + return out, nil + } + opts.Page = resp.NextPage + } +} + +func (a githubAPI) PullsWithCommit(ctx context.Context, repo, sha string) ([]pullInfo, error) { + owner, name := splitRepo(repo) + var out []pullInfo + opts := &github.ListOptions{PerPage: 100} + for { + prs, resp, err := a.gh.PullRequests.ListPullRequestsWithCommit(ctx, owner, name, sha, opts) + if err != nil { + return nil, err + } + for _, pr := range prs { + out = append(out, pullInfo{ + Number: pr.GetNumber(), + URL: pr.GetHTMLURL(), + Author: pr.GetUser().GetLogin(), + BaseRepo: pr.GetBase().GetRepo().GetFullName(), + HeadSHA: pr.GetHead().GetSHA(), + MergeCommit: pr.GetMergeCommitSHA(), + MergedAt: pr.GetMergedAt().Time, + }) + } + if resp.NextPage == 0 { + return out, nil + } + opts.Page = resp.NextPage + } +} + +func (a githubAPI) Reviews(ctx context.Context, repo string, number int) ([]reviewInfo, error) { + owner, name := splitRepo(repo) + var out []reviewInfo + opts := &github.ListOptions{PerPage: 100} + for { + reviews, resp, err := a.gh.PullRequests.ListReviews(ctx, owner, name, number, opts) + if err != nil { + return nil, err + } + for _, r := range reviews { + out = append(out, reviewInfo{ + User: r.GetUser().GetLogin(), + State: r.GetState(), + CommitID: r.GetCommitID(), + SubmittedAt: r.GetSubmittedAt().Time, + }) + } + if resp.NextPage == 0 { + return out, nil + } + opts.Page = resp.NextPage + } +} + +func (a githubAPI) LastForcePush(ctx context.Context, repo string, number int) (time.Time, error) { + owner, name := splitRepo(repo) + var last time.Time + opts := &github.ListOptions{PerPage: 100} + for { + events, resp, err := a.gh.Issues.ListIssueTimeline(ctx, owner, name, number, opts) + if err != nil { + return time.Time{}, err + } + for _, e := range events { + if e.GetEvent() == "head_ref_force_pushed" && e.GetCreatedAt().After(last) { + last = e.GetCreatedAt().Time + } + } + if resp.NextPage == 0 { + return last, nil + } + opts.Page = resp.NextPage + } +} + +func (a githubAPI) Permission(ctx context.Context, repo, user string) (string, error) { + owner, name := splitRepo(repo) + p, _, err := a.gh.Repositories.GetPermissionLevel(ctx, owner, name, user) + if err != nil { + return "", err + } + return p.GetPermission(), nil +} + +func isNotFound(err error) bool { + var ghErr *github.ErrorResponse + return errors.As(err, &ghErr) && ghErr.Response != nil && ghErr.Response.StatusCode == http.StatusNotFound +} + +func firstLine(s string) string { + line, _, _ := strings.Cut(s, "\n") + return strings.TrimSpace(line) +} diff --git a/.github/actions/fork-review-check/src/go.mod b/.github/actions/fork-review-check/src/go.mod new file mode 100644 index 00000000..a3911767 --- /dev/null +++ b/.github/actions/fork-review-check/src/go.mod @@ -0,0 +1,7 @@ +module github.com/loft-sh/github-actions/fork-review-check + +go 1.26.4 + +require github.com/google/go-github/v88 v88.0.0 + +require github.com/google/go-querystring v1.2.0 // indirect diff --git a/.github/actions/fork-review-check/src/go.sum b/.github/actions/fork-review-check/src/go.sum new file mode 100644 index 00000000..06a97fa8 --- /dev/null +++ b/.github/actions/fork-review-check/src/go.sum @@ -0,0 +1,7 @@ +github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= +github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8= +github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX3N/iU= +github.com/google/go-github/v88 v88.0.0 h1:dZA9IKkPK1eXZj4ypngnpRj5FwdpTv4whix2PrQMP7M= +github.com/google/go-github/v88 v88.0.0/go.mod h1:rufTDgn2N45wjhukLTyxmvc9nilSp3mr3Rgtt6b1MPw= +github.com/google/go-querystring v1.2.0 h1:yhqkPbu2/OH+V9BfpCVPZkNmUXhb2gBxJArfhIxNtP0= +github.com/google/go-querystring v1.2.0/go.mod h1:8IFJqpSRITyJ8QhQ13bmbeMBDfmeEJZD5A0egEOmkqU= diff --git a/.github/actions/fork-review-check/src/gomod.go b/.github/actions/fork-review-check/src/gomod.go new file mode 100644 index 00000000..ce019c75 --- /dev/null +++ b/.github/actions/fork-review-check/src/gomod.go @@ -0,0 +1,207 @@ +package main + +import ( + "bytes" + "encoding/json" + "errors" + "fmt" + "io" + "os" + "os/exec" + "path/filepath" + "regexp" + "strings" +) + +// forkModulePrefix selects the modules that can carry fork code: after +// replacements, their source lives in a loft-sh repository. +const forkModulePrefix = "github.com/loft-sh/" + +// pseudoVersionRe matches Go pseudo-versions (golang.org/x/mod/module), which +// pin a commit rather than a tag. +var pseudoVersionRe = regexp.MustCompile(`^v[0-9]+\.(0\.0-|\d+\.\d+-([^+]*\.)?0\.)\d{14}-[A-Za-z0-9]+(\+[0-9A-Za-z-]+(\.[0-9A-Za-z-]+)*)?$`) + +type goModule struct { + Path string + Version string + Main bool + Replace *goModule +} + +type goOrigin struct { + VCS string + URL string + Hash string +} + +type goDownload struct { + Path string + Version string + Sum string + Origin *goOrigin + Error string +} + +// goTool runs the go command; tests replace it. +type goTool interface { + ListModules(dir string) ([]goModule, error) + Download(dir, path, version string) (goDownload, error) +} + +// inputRecord is one build input the receipt lists, with its hash. +type inputRecord struct { + Kind string `json:"kind"` + Source string `json:"source"` + Module string `json:"module,omitempty"` + Path string `json:"path"` + Version string `json:"version"` + Repo string `json:"repo"` + Commit string `json:"commit"` + Sum string `json:"sum,omitempty"` + Fork bool `json:"fork"` +} + +// goForkInputs resolves the loft-sh modules in the effective module graph of +// dir to the commits Go downloaded. It reports every loft-sh module; deciding +// which of them are forks needs the GitHub API and happens in the caller. +func goForkInputs(tool goTool, dir string) ([]inputRecord, []string, error) { + mods, err := tool.ListModules(dir) + if err != nil { + return nil, nil, err + } + + var inputs []inputRecord + var failures []string + seen := map[string]bool{} + for _, m := range mods { + if m.Main { + continue + } + eff := m + if m.Replace != nil { + if m.Replace.Version == "" { + // A directory replacement is code in the product repository itself. + continue + } + eff = *m.Replace + } + if !strings.HasPrefix(eff.Path, forkModulePrefix) { + continue + } + key := eff.Path + "@" + eff.Version + if seen[key] { + continue + } + seen[key] = true + + rec := inputRecord{Kind: "go-module", Source: filepath.Join(dir, "go.mod"), Path: eff.Path, Version: eff.Version} + if m.Replace != nil { + rec.Module = m.Path + } + dl, err := tool.Download(dir, eff.Path, eff.Version) + if err != nil { + failures = append(failures, fmt.Sprintf("%s@%s: go mod download: %v", eff.Path, eff.Version, err)) + continue + } + if dl.Origin == nil || dl.Origin.Hash == "" { + failures = append(failures, fmt.Sprintf("%s@%s: Go reported no origin commit; set GOPRIVATE=github.com/loft-sh/* so it fetches from git and records the commit", eff.Path, eff.Version)) + continue + } + rec.Commit = dl.Origin.Hash + rec.Sum = dl.Sum + rec.Repo = repoFromOrigin(dl.Origin.URL, eff.Path) + inputs = append(inputs, rec) + } + return inputs, failures, nil +} + +// pinFromGoInput turns a resolved module into the fork pin the rules check. +// A pseudo-version pins a commit; any other version names a fork tag. +func pinFromGoInput(in inputRecord) pin { + version := strings.TrimSuffix(in.Version, "+incompatible") + p := pin{Repo: in.Repo, Ref: in.Version, Commit: in.Commit} + if !pseudoVersionRe.MatchString(in.Version) { + p.Tag = version + } + return p +} + +// repoFromOrigin names the repository Go fetched from, falling back to the +// first two path elements after github.com. +func repoFromOrigin(url, path string) string { + u := strings.TrimSuffix(strings.TrimPrefix(strings.TrimPrefix(url, "https://"), "http://"), ".git") + if strings.HasPrefix(u, "github.com/") { + path = u + } + parts := strings.Split(strings.TrimPrefix(path, "github.com/"), "/") + if len(parts) < 2 { + return "" + } + return parts[0] + "/" + parts[1] +} + +type execGo struct{} + +func (execGo) ListModules(dir string) ([]goModule, error) { + args := []string{"list", "-m", "-json"} + // With a vendor directory and no workspace, go list cannot compute "all" + // from vendor/modules.txt. Go already checks that file against go.mod, so + // reading the graph from go.mod yields the same versions. + if exists(filepath.Join(dir, "vendor")) && os.Getenv("GOWORK") != "off" && !exists(filepath.Join(dir, "go.work")) { + args = append(args, "-mod=mod") + } + args = append(args, "all") + out, err := runGo(dir, args...) + if err != nil { + return nil, err + } + return decodeModules(out) +} + +func (execGo) Download(dir, path, version string) (goDownload, error) { + out, err := runGo(dir, "mod", "download", "-json", path+"@"+version) + var dl goDownload + if jerr := json.Unmarshal(out, &dl); jerr != nil { + if err != nil { + return dl, err + } + return dl, jerr + } + if dl.Error != "" { + return dl, errors.New(dl.Error) + } + return dl, err +} + +func decodeModules(out []byte) ([]goModule, error) { + var mods []goModule + dec := json.NewDecoder(bytes.NewReader(out)) + for { + var m goModule + err := dec.Decode(&m) + if errors.Is(err, io.EOF) { + return mods, nil + } + if err != nil { + return nil, fmt.Errorf("decode go list output: %w", err) + } + mods = append(mods, m) + } +} + +func runGo(dir string, args ...string) ([]byte, error) { + cmd := exec.Command("go", args...) + cmd.Dir = dir + var stderr bytes.Buffer + cmd.Stderr = &stderr + out, err := cmd.Output() + if err != nil { + return out, fmt.Errorf("go %s: %w: %s", strings.Join(args, " "), err, strings.TrimSpace(stderr.String())) + } + return out, nil +} + +func exists(path string) bool { + _, err := os.Stat(path) + return err == nil +} diff --git a/.github/actions/fork-review-check/src/gomod_test.go b/.github/actions/fork-review-check/src/gomod_test.go new file mode 100644 index 00000000..ff67139d --- /dev/null +++ b/.github/actions/fork-review-check/src/gomod_test.go @@ -0,0 +1,150 @@ +package main + +import ( + "context" + "encoding/json" + "fmt" + "strings" + "testing" + "time" +) + +type fakeGo struct { + mods []goModule + downloads map[string]goDownload // path@version +} + +func (f fakeGo) ListModules(string) ([]goModule, error) { return f.mods, nil } + +func (f fakeGo) Download(_, path, version string) (goDownload, error) { + dl, ok := f.downloads[path+"@"+version] + if !ok { + return dl, fmt.Errorf("no download %s@%s", path, version) + } + return dl, nil +} + +const fuseRev = "0b597961e33d5a2b8e3c1f6d1e8e6c4e2c1a0b9f" + +// vnodeGraph mirrors vnode-internal: bazil.org/fuse replaced by the loft-sh +// fork at a pseudo-version, a loft-sh product module, a directory replacement +// and an unrelated module. +func vnodeGraph() fakeGo { + return fakeGo{ + mods: []goModule{ + {Path: "github.com/loft-sh/vnode", Main: true}, + {Path: "bazil.org/fuse", Version: "v0.0.0-20180421153158-65cc252bf669", Replace: &goModule{Path: "github.com/loft-sh/fuse", Version: "v0.0.0-20250416130952-0b597961e33d"}}, + {Path: "github.com/loft-sh/api/v4", Version: "v4.2.1"}, + {Path: "github.com/loft-sh/local", Version: "v0.0.0", Replace: &goModule{Path: "./local"}}, + {Path: "tailscale.com", Version: "v1.96.4", Replace: &goModule{Path: "github.com/loft-sh/tailscale", Version: "v1.96.4-loft.3"}}, + {Path: "github.com/sirupsen/logrus", Version: "v1.9.3"}, + }, + downloads: map[string]goDownload{ + "github.com/loft-sh/fuse@v0.0.0-20250416130952-0b597961e33d": {Sum: "h1:fuse", Origin: &goOrigin{VCS: "git", URL: "https://github.com/loft-sh/fuse", Hash: fuseRev}}, + "github.com/loft-sh/api/v4@v4.2.1": {Sum: "h1:api", Origin: &goOrigin{VCS: "git", URL: "https://github.com/loft-sh/api", Hash: strings.Repeat("a", 40)}}, + "github.com/loft-sh/tailscale@v1.96.4-loft.3": {Sum: "h1:ts", Origin: &goOrigin{VCS: "git", URL: "https://github.com/loft-sh/tailscale", Hash: strings.Repeat("4", 40)}}, + }, + } +} + +func TestGoForkInputsResolvesLoftModules(t *testing.T) { + inputs, failures, err := goForkInputs(vnodeGraph(), "vnode") + if err != nil || len(failures) != 0 { + t.Fatalf("err %v, failures %v", err, failures) + } + got := map[string]inputRecord{} + for _, in := range inputs { + got[in.Path] = in + } + if len(got) != 3 { + t.Fatalf("want fuse, api and tailscale, got %v", inputs) + } + fuse := got["github.com/loft-sh/fuse"] + if fuse.Module != "bazil.org/fuse" || fuse.Repo != "loft-sh/fuse" || fuse.Commit != fuseRev || fuse.Sum != "h1:fuse" { + t.Errorf("fuse input = %+v", fuse) + } + if _, ok := got["github.com/loft-sh/local"]; ok { + t.Error("a directory replacement is product code, not a fork pin") + } +} + +func TestGoForkInputsNeedsOrigin(t *testing.T) { + g := vnodeGraph() + dl := g.downloads["github.com/loft-sh/fuse@v0.0.0-20250416130952-0b597961e33d"] + dl.Origin = nil + g.downloads["github.com/loft-sh/fuse@v0.0.0-20250416130952-0b597961e33d"] = dl + _, failures, _ := goForkInputs(g, "vnode") + if len(failures) != 1 || !strings.Contains(failures[0], "GOPRIVATE") { + t.Fatalf("want one failure naming the GOPRIVATE remedy, got %v", failures) + } +} + +func TestPinFromGoInput(t *testing.T) { + pseudo := pinFromGoInput(inputRecord{Repo: "loft-sh/fuse", Version: "v0.0.0-20250416130952-0b597961e33d", Commit: fuseRev}) + if pseudo.Tag != "" || pseudo.Commit != fuseRev { + t.Errorf("pseudo-version pin = %+v, want a commit pin", pseudo) + } + tagged := pinFromGoInput(inputRecord{Repo: "loft-sh/tailscale", Version: "v1.96.4-loft.3", Commit: "x"}) + if tagged.Tag != "v1.96.4-loft.3" { + t.Errorf("tag pin = %+v, want tag v1.96.4-loft.3", tagged) + } + for _, v := range []string{"v1.2.4-0.20191109021931-daa7c04131f5", "v1.2.3-pre.0.20191109021931-daa7c04131f5", "v2.0.0-20191109021931-daa7c04131f5+incompatible"} { + if !pseudoVersionRe.MatchString(v) { + t.Errorf("%s is a pseudo-version", v) + } + } +} + +func TestRepoFromOrigin(t *testing.T) { + cases := map[[2]string]string{ + {"https://github.com/loft-sh/sysbox-libs", "github.com/loft-sh/sysbox-libs/utils"}: "loft-sh/sysbox-libs", + {"", "github.com/loft-sh/sysbox-libs/utils"}: "loft-sh/sysbox-libs", + {"https://github.com/loft-sh/fuse.git", "github.com/loft-sh/fuse"}: "loft-sh/fuse", + } + for in, want := range cases { + if got := repoFromOrigin(in[0], in[1]); got != want { + t.Errorf("repoFromOrigin(%q, %q) = %q, want %q", in[0], in[1], got, want) + } + } +} + +func TestDecodeModulesStream(t *testing.T) { + var out []byte + for _, m := range vnodeGraph().mods { + b, _ := json.MarshalIndent(m, "", "\t") + out = append(out, b...) + out = append(out, '\n') + } + mods, err := decodeModules(out) + if err != nil || len(mods) != len(vnodeGraph().mods) || mods[1].Replace.Path != "github.com/loft-sh/fuse" { + t.Fatalf("decode = %v, %v", mods, err) + } +} + +// TestGoModeChecksOnlyForks runs the whole go mode: api/v4 is a loft-sh +// product repo and is listed but not checked; fuse and tailscale are forks. +func TestGoModeChecksOnlyForks(t *testing.T) { + f := newFake() + f.repos["loft-sh/fuse"] = repoInfo{FullName: "loft-sh/fuse", Fork: true} + f.repos["loft-sh/api"] = repoInfo{FullName: "loft-sh/api"} + f.repos["loft-sh/tailscale"] = repoInfo{FullName: "loft-sh/tailscale", Fork: true} + f.upstreams["loft-sh/tailscale"] = "tailscale/tailscale" + + rc, err := check(context.Background(), f, vnodeGraph(), options{mode: "go", dirs: multiFlag{"vnode"}}, time.Now()) + if err != nil { + t.Fatal(err) + } + if len(rc.Inputs) != 3 { + t.Fatalf("want 3 inputs listed, got %+v", rc.Inputs) + } + var checked []string + for _, fr := range rc.Forks { + checked = append(checked, fr.Repo) + } + if strings.Join(checked, ",") != "loft-sh/fuse,loft-sh/tailscale" { + t.Fatalf("checked %v, want fuse and tailscale", checked) + } + if rc.Pass { + t.Fatal("fuse has no fork-upstream property, so the run must fail") + } +} diff --git a/.github/actions/fork-review-check/src/main.go b/.github/actions/fork-review-check/src/main.go new file mode 100644 index 00000000..b51465c0 --- /dev/null +++ b/.github/actions/fork-review-check/src/main.go @@ -0,0 +1,305 @@ +// Command fork-review-check verifies that every fork commit a product build +// consumes reached its loft-sh fork through a merged pull request approved by +// someone other than its author (loft-sh/architecture, +// engineering-enablement/designs/fork-review-check.md). +// +// For each fork commit S it walks the range from the trusted upstream base to +// S and applies the design's rules: +// +// 1. S is the approved final head, or the merge result, of a merged fork PR. +// 2. Every commit in the range belongs to a merged fork PR approved at its +// final head, after its last force push, by someone with write or higher +// permission on the fork who is neither the PR author nor the commit author. +// 3. With -as-of, every approval predates that time (the product release). +// 4. It fails and names every commit that breaks a rule. +// +// It reads GitHub and the go command only; it writes nothing but its receipt. +// +// fork-review-check ref -repo loft-sh/kine -ref v0.16.5.vcluster.1 +// fork-review-check go -dir . -dir ./sub +// +// Exit codes: 0 pass (or -advisory), 1 a rule failed, 2 the check could not run. +package main + +import ( + "context" + "encoding/json" + "errors" + "flag" + "fmt" + "io" + "log" + "os" + "sort" + "strings" + "time" + + "github.com/google/go-github/v88/github" +) + +// version is stamped into every receipt; the action overrides it with the ref +// it runs from. +var version = "dev" + +const receiptSchema = "fork-review-check/v1" + +type receipt struct { + Schema string `json:"schema"` + CLIVersion string `json:"cli_version"` + Mode string `json:"mode"` + CheckedAt time.Time `json:"checked_at"` + AsOf *time.Time `json:"as_of,omitempty"` + Inputs []inputRecord `json:"inputs"` + Forks []forkResult `json:"forks"` + Failures []string `json:"failures,omitempty"` + Pass bool `json:"pass"` +} + +// multiFlag collects a repeatable string flag. +type multiFlag []string + +func (m *multiFlag) String() string { return strings.Join(*m, ",") } +func (m *multiFlag) Set(v string) error { *m = append(*m, v); return nil } + +type options struct { + mode string + repo string + ref string + dirs multiFlag + upstreams multiFlag + asOf string + receipt string + advisory bool +} + +func main() { + log.SetFlags(0) + os.Exit(run(os.Args[1:], os.Stdout)) +} + +func run(args []string, stdout io.Writer) int { + opts, err := parseArgs(args) + if err != nil { + log.Printf("::error::%v", err) + return 2 + } + + token := os.Getenv("GITHUB_TOKEN") + if token == "" { + log.Print("::error::GITHUB_TOKEN is required") + return 2 + } + gh, err := github.NewClient(github.WithAuthToken(token)) + if err != nil { + log.Printf("::error::create github client: %v", err) + return 2 + } + + rc, err := check(context.Background(), githubAPI{gh: gh}, execGo{}, opts, time.Now().UTC()) + if err != nil { + log.Printf("::error::%v", err) + return 2 + } + if v := os.Getenv("FORK_REVIEW_CHECK_VERSION"); v != "" { + rc.CLIVersion = v + } + + if err := writeReceipt(rc, opts.receipt); err != nil { + log.Printf("::error::write receipt: %v", err) + return 2 + } + report := renderText(rc) + fmt.Fprint(stdout, report) + appendSummary(renderMarkdown(rc)) + + if rc.Pass { + return 0 + } + if opts.advisory { + log.Print("::warning::fork review check failed; advisory mode, not blocking") + return 0 + } + log.Print("::error::fork review check failed") + return 1 +} + +func parseArgs(args []string) (options, error) { + var opts options + if len(args) == 0 { + return opts, errors.New("usage: fork-review-check ref|go [flags]") + } + opts.mode = args[0] + + fs := flag.NewFlagSet("fork-review-check "+opts.mode, flag.ContinueOnError) + fs.StringVar(&opts.repo, "repo", "", "ref mode: fork repository, owner/name") + fs.StringVar(&opts.ref, "ref", "", "ref mode: fork release tag or commit SHA") + fs.Var(&opts.dirs, "dir", "go mode: module directory to resolve (repeatable)") + fs.Var(&opts.upstreams, "upstream", "fork=upstream[@branch] used when a fork has no fork-upstream property; in ref mode a bare upstream applies to -repo (repeatable)") + fs.StringVar(&opts.asOf, "as-of", "", "RFC 3339 time every approval must predate (rule 3), normally the product release time") + fs.StringVar(&opts.receipt, "receipt", "", "write the JSON receipt to this path") + fs.BoolVar(&opts.advisory, "advisory", false, "report failures as warnings and exit 0") + if err := fs.Parse(args[1:]); err != nil { + return opts, err + } + + switch opts.mode { + case "ref": + if opts.repo == "" || opts.ref == "" { + return opts, errors.New("ref mode needs -repo and -ref") + } + case "go": + if len(opts.dirs) == 0 { + opts.dirs = multiFlag{"."} + } + default: + return opts, fmt.Errorf("unknown mode %q: want ref or go", opts.mode) + } + return opts, nil +} + +// check resolves the pins for the mode and evaluates every fork. +func check(ctx context.Context, api forkAPI, tool goTool, opts options, now time.Time) (receipt, error) { + rc := receipt{Schema: receiptSchema, CLIVersion: version, Mode: opts.mode, CheckedAt: now, Inputs: []inputRecord{}, Forks: []forkResult{}} + + var asOf time.Time + if opts.asOf != "" { + t, err := time.Parse(time.RFC3339, opts.asOf) + if err != nil { + return rc, fmt.Errorf("parse -as-of: %w", err) + } + asOf = t.UTC() + rc.AsOf = &asOf + } + overrides, err := parseUpstreams(opts.upstreams, opts.repo) + if err != nil { + return rc, err + } + + var pins []pin + switch opts.mode { + case "ref": + p, failure, err := refPin(ctx, api, opts.repo, opts.ref) + if err != nil { + return rc, err + } + if failure != "" { + rc.Failures = append(rc.Failures, failure) + } else { + pins = append(pins, p) + rc.Inputs = append(rc.Inputs, inputRecord{Kind: "fork-ref", Source: "-ref", Path: opts.repo, Version: opts.ref, Repo: opts.repo, Commit: p.Commit, Fork: true}) + } + case "go": + for _, dir := range opts.dirs { + inputs, failures, err := goForkInputs(tool, dir) + if err != nil { + return rc, err + } + rc.Failures = append(rc.Failures, failures...) + for _, in := range inputs { + fork, err := isFork(ctx, api, in.Repo) + if err != nil { + return rc, fmt.Errorf("read %s: %w", in.Repo, err) + } + in.Fork = fork + rc.Inputs = append(rc.Inputs, in) + if fork { + pins = append(pins, pinFromGoInput(in)) + } + } + } + } + + c := newChecker(api, asOf, overrides) + seen := map[string]bool{} + for _, p := range pins { + key := p.Repo + "@" + p.Commit + "@" + p.Tag + if seen[key] { + continue + } + seen[key] = true + rc.Forks = append(rc.Forks, c.checkFork(ctx, p)) + } + sort.SliceStable(rc.Forks, func(i, j int) bool { return rc.Forks[i].Repo < rc.Forks[j].Repo }) + + rc.Pass = len(rc.Failures) == 0 + for _, f := range rc.Forks { + if !f.Pass { + rc.Pass = false + } + } + return rc, nil +} + +// refPin resolves a fork ref given by hand: a tag, or a full commit SHA. +func refPin(ctx context.Context, api forkAPI, repo, ref string) (pin, string, error) { + sha, err := api.ResolveTag(ctx, repo, ref) + if err != nil { + return pin{}, "", fmt.Errorf("resolve %s in %s: %w", ref, repo, err) + } + if sha != "" { + return pin{Repo: repo, Ref: ref, Tag: ref, Commit: sha}, "", nil + } + if fullSHARe.MatchString(ref) { + return pin{Repo: repo, Ref: ref, Commit: ref}, "", nil + } + return pin{}, fmt.Sprintf("%s has no tag %s, and it is not a full commit SHA", repo, ref), nil +} + +// isFork reports whether a loft-sh repository holds fork code: GitHub marks +// it a fork, or an org owner gave it a fork-upstream property. +func isFork(ctx context.Context, api forkAPI, repo string) (bool, error) { + info, err := api.Repo(ctx, repo) + if err != nil { + return false, err + } + if info.Fork { + return true, nil + } + upstream, err := api.ForkUpstream(ctx, repo) + return upstream != "", err +} + +func parseUpstreams(values []string, refRepo string) (map[string]upstreamOverride, error) { + out := map[string]upstreamOverride{} + for _, v := range values { + fork, upstream, ok := strings.Cut(v, "=") + if !ok { + if refRepo == "" { + return nil, fmt.Errorf("-upstream %q: want fork=upstream outside ref mode", v) + } + fork, upstream = refRepo, v + } + repo, branch, _ := strings.Cut(upstream, "@") + if !strings.Contains(fork, "/") || !strings.Contains(repo, "/") { + return nil, fmt.Errorf("-upstream %q: repositories are owner/name", v) + } + out[fork] = upstreamOverride{Repo: repo, Ref: branch} + } + return out, nil +} + +func writeReceipt(rc receipt, path string) error { + if path == "" { + return nil + } + data, err := json.MarshalIndent(rc, "", " ") + if err != nil { + return err + } + return os.WriteFile(path, append(data, '\n'), 0o644) +} + +// appendSummary writes to the job summary when running in Actions. +func appendSummary(md string) { + path := os.Getenv("GITHUB_STEP_SUMMARY") + if path == "" { + return + } + f, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644) + if err != nil { + log.Printf("::warning::write job summary: %v", err) + return + } + defer f.Close() + _, _ = f.WriteString(md) +} diff --git a/.github/actions/fork-review-check/src/main_test.go b/.github/actions/fork-review-check/src/main_test.go new file mode 100644 index 00000000..edd2656d --- /dev/null +++ b/.github/actions/fork-review-check/src/main_test.go @@ -0,0 +1,93 @@ +package main + +import ( + "context" + "strings" + "testing" + "time" +) + +func TestRefModeResolvesTag(t *testing.T) { + rc, err := check(context.Background(), reviewedFork(), nil, options{mode: "ref", repo: fork, ref: tag}, time.Now()) + if err != nil { + t.Fatal(err) + } + if !rc.Pass || len(rc.Forks) != 1 || rc.Forks[0].Commit != merge { + t.Fatalf("want one passing fork at %s: %+v", merge, rc) + } + if rc.Inputs[0].Kind != "fork-ref" || rc.Inputs[0].Commit != merge { + t.Errorf("input = %+v", rc.Inputs[0]) + } +} + +func TestRefModeAcceptsFullSHA(t *testing.T) { + f := reviewedFork() + f.repos[upstream] = repoInfo{FullName: upstream, DefaultBranch: "master"} + f.compares[fork+"@k3s-io:master..."+merge] = comparison{Status: "diverged", Total: 3, MergeBase: baseSHA, Commits: f.compares[fork+"@"+baseSHA+"..."+merge].Commits} + sha := merge + rc, err := check(context.Background(), f, nil, options{mode: "ref", repo: fork, ref: sha}, time.Now()) + if err != nil { + t.Fatal(err) + } + if !rc.Pass || rc.Forks[0].Base.Rule != "merge-base" { + t.Fatalf("want a commit pin on the merge-base rule: %+v", rc.Forks) + } +} + +func TestRefModeUnknownRefFails(t *testing.T) { + rc, err := check(context.Background(), reviewedFork(), nil, options{mode: "ref", repo: fork, ref: "v9.9.9"}, time.Now()) + if err != nil { + t.Fatal(err) + } + if rc.Pass || len(rc.Failures) != 1 { + t.Fatalf("want an unresolvable ref to fail: %+v", rc) + } +} + +func TestAsOfFlagAppliesRule3(t *testing.T) { + rc, err := check(context.Background(), reviewedFork(), nil, options{mode: "ref", repo: fork, ref: tag, asOf: t0.Add(30 * time.Minute).Format(time.RFC3339)}, time.Now()) + if err != nil { + t.Fatal(err) + } + if rc.Pass || rc.AsOf == nil { + t.Fatalf("want rule 3 to fail and as_of recorded: %+v", rc) + } +} + +func TestParseArgs(t *testing.T) { + if _, err := parseArgs([]string{"ref", "-repo", fork}); err == nil { + t.Error("ref mode without -ref must fail") + } + if _, err := parseArgs([]string{"image"}); err == nil { + t.Error("an unknown mode must fail") + } + o, err := parseArgs([]string{"go"}) + if err != nil || len(o.dirs) != 1 || o.dirs[0] != "." { + t.Errorf("go mode defaults to the current directory: %+v %v", o, err) + } +} + +func TestParseUpstreams(t *testing.T) { + got, err := parseUpstreams([]string{"k3s-io/kine", "loft-sh/fuse=nestybox/fuse@master"}, fork) + if err != nil { + t.Fatal(err) + } + if got[fork].Repo != upstream || got["loft-sh/fuse"].Ref != "master" { + t.Errorf("overrides = %+v", got) + } + if _, err := parseUpstreams([]string{"kine"}, ""); err == nil { + t.Error("a bare upstream outside ref mode must fail") + } +} + +func TestReportsNameFailures(t *testing.T) { + f := reviewedFork() + delete(f.pulls, fork+"@"+c1) + rc, _ := check(context.Background(), f, nil, options{mode: "ref", repo: fork, ref: tag}, time.Now()) + text, md := renderText(rc), renderMarkdown(rc) + for _, want := range []string{"FAIL", short(c1), "direct push"} { + if !strings.Contains(text, want) || !strings.Contains(md, want) { + t.Errorf("reports miss %q:\n%s\n%s", want, text, md) + } + } +} diff --git a/.github/actions/fork-review-check/src/report.go b/.github/actions/fork-review-check/src/report.go new file mode 100644 index 00000000..d3252b59 --- /dev/null +++ b/.github/actions/fork-review-check/src/report.go @@ -0,0 +1,94 @@ +package main + +import ( + "fmt" + "regexp" + "strings" +) + +var fullSHARe = regexp.MustCompile(`^[0-9a-f]{40}$`) + +func verdict(pass bool) string { + if pass { + return "PASS" + } + return "FAIL" +} + +// renderText is the plain report for logs and local runs. +func renderText(rc receipt) string { + var b strings.Builder + fmt.Fprintf(&b, "fork review check %s: %s (%d forks)\n", rc.CLIVersion, verdict(rc.Pass), len(rc.Forks)) + for _, f := range rc.Failures { + fmt.Fprintf(&b, " FAIL %s\n", f) + } + for _, f := range rc.Forks { + fmt.Fprintf(&b, "\n%s %s @ %s: %s\n", f.Repo, f.Ref, short(f.Commit), verdict(f.Pass)) + fmt.Fprintf(&b, " fork-upstream: %s (%s)\n", orNone(f.ForkUpstream), f.ForkUpstreamSource) + if f.Base != nil { + fmt.Fprintf(&b, " base: %s %s (%s), %d fork commits\n", f.Base.Rule, f.Base.Ref, short(f.Base.Commit), len(f.Commits)) + } + if f.ReviewedTree != nil && f.ReviewedTree.Pass { + fmt.Fprintf(&b, " reviewed tree: #%d (%s)\n", f.ReviewedTree.Pull.Number, f.ReviewedTree.Via) + } + for _, msg := range f.Failures { + fmt.Fprintf(&b, " FAIL %s\n", msg) + } + for _, c := range f.Commits { + fmt.Fprintf(&b, " %s %s %s %q\n", verdict(c.Pass), short(c.SHA), orNone(c.Author), c.Subject) + if c.Pass { + fmt.Fprintf(&b, " #%d approved by %s (%s) at %s\n", c.Pull.Number, c.Approval.Approver, c.Approval.Permission, c.Approval.SubmittedAt.Format("2006-01-02T15:04:05Z")) + } + for _, msg := range c.Failures { + fmt.Fprintf(&b, " %s\n", msg) + } + } + } + return b.String() +} + +// renderMarkdown is the job summary: one table per fork. +func renderMarkdown(rc receipt) string { + var b strings.Builder + fmt.Fprintf(&b, "## Fork review check: %s\n\n", verdict(rc.Pass)) + for _, f := range rc.Failures { + fmt.Fprintf(&b, "- %s\n", f) + } + for _, f := range rc.Forks { + fmt.Fprintf(&b, "\n### %s `%s` (%s): %s\n\n", f.Repo, f.Ref, short(f.Commit), verdict(f.Pass)) + fmt.Fprintf(&b, "fork-upstream: `%s` (%s)", orNone(f.ForkUpstream), f.ForkUpstreamSource) + if f.Base != nil { + fmt.Fprintf(&b, "; base %s `%s`", f.Base.Rule, f.Base.Ref) + } + b.WriteString("\n\n") + for _, msg := range f.Failures { + fmt.Fprintf(&b, "- %s\n", msg) + } + if len(f.Commits) == 0 { + continue + } + b.WriteString("\n| Commit | Author | PR | Approver | Permission | Result |\n| --- | --- | --- | --- | --- | --- |\n") + for _, c := range f.Commits { + pr, approver, perm := "", "", "" + if c.Pull != nil { + pr = fmt.Sprintf("[#%d](%s)", c.Pull.Number, c.Pull.URL) + } + if c.Approval != nil { + approver, perm = c.Approval.Approver, c.Approval.Permission + } + result := verdict(c.Pass) + if !c.Pass { + result += ": " + strings.ReplaceAll(strings.Join(c.Failures, "; "), "|", "\\|") + } + fmt.Fprintf(&b, "| `%s` | %s | %s | %s | %s | %s |\n", short(c.SHA), orNone(c.Author), pr, approver, perm, result) + } + } + return b.String() +} + +func orNone(s string) string { + if s == "" { + return "none" + } + return s +} diff --git a/.github/workflows/test-fork-review-check.yaml b/.github/workflows/test-fork-review-check.yaml new file mode 100644 index 00000000..5bb91efe --- /dev/null +++ b/.github/workflows/test-fork-review-check.yaml @@ -0,0 +1,69 @@ +name: Test fork-review-check + +on: + push: + branches: [main] + paths: + - '.github/actions/fork-review-check/**' + - '.github/workflows/test-fork-review-check.yaml' + pull_request: + paths: + - '.github/actions/fork-review-check/**' + - '.github/workflows/test-fork-review-check.yaml' + workflow_dispatch: + +permissions: + contents: read + +jobs: + test: + runs-on: ubuntu-latest + permissions: + contents: read + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + with: + go-version-file: .github/actions/fork-review-check/src/go.mod + - run: go test ./... + working-directory: .github/actions/fork-review-check/src + + # Read-only run against the shipped fork pins the design names, with the + # workflow token a required workflow gets. Advisory: it records today's gap + # and whether that token can read approver permission on another repo, and + # never fails the build. + live: + runs-on: ubuntu-latest + permissions: + contents: read + strategy: + fail-fast: false + matrix: + include: + - repo: loft-sh/kine + ref: v0.16.5.vcluster.1 + upstream: k3s-io/kine + - repo: loft-sh/tailscale + ref: v1.96.4-loft.3 + upstream: tailscale/tailscale + # bazil.org/fuse in vnode-internal pins loft-sh/fuse at this pseudo-version commit. + - repo: loft-sh/fuse + ref: 0b597961e33ddd4c9a19e9e3981b79cc113885b5 + upstream: nestybox/fuse + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + with: + go-version-file: .github/actions/fork-review-check/src/go.mod + - name: Check ${{ matrix.repo }} ${{ matrix.ref }} + working-directory: .github/actions/fork-review-check/src + env: + GITHUB_TOKEN: ${{ github.token }} + REPO: ${{ matrix.repo }} + REF: ${{ matrix.ref }} + UPSTREAM: ${{ matrix.upstream }} + run: go run . ref -repo "$REPO" -ref "$REF" -upstream "$UPSTREAM" -advisory -receipt /dev/stdout diff --git a/Makefile b/Makefile index 2a2d55df..fad8fbdb 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: test test-secret-broker-request test-secret-broker-response test-semver-validation test-setup-semstat test-linear-pr-commenter test-link-backport-prs test-select-backport-source-prs test-release-notification test-linear-release-sync test-linear-cve-summary test-aws-test-infra test-cleanup-head-charts test-ci-test-notify test-auto-approve-bot-prs test-ai-pr-review test-ai-step test-release-summary test-publish-helm-chart test-govulncheck test-go-licenses test-run-ginkgo test-sticky-pr-comment test-repository-dispatch test-parse-label-filter test-release-branch-freeze test-prerelease-setup test-vcluster-release test-platform-release test-release-lib test-subtree-mirror test-oss-commit-sync test-oss-mirror-staleness test-backport-legacy-allowlist test-backport-legacy-split test-backport-fix-imports test-promote-release test-wait-for-release test-cve-scan test-commitlint test-resolve-github-release test-comment-triggered-check test-conflict-check build-linear-release-sync lint check-bats-jobs install-auto-doc generate-docs check-docs help +.PHONY: test test-secret-broker-request test-secret-broker-response test-semver-validation test-setup-semstat test-linear-pr-commenter test-link-backport-prs test-fork-review-check test-select-backport-source-prs test-release-notification test-linear-release-sync test-linear-cve-summary test-aws-test-infra test-cleanup-head-charts test-ci-test-notify test-auto-approve-bot-prs test-ai-pr-review test-ai-step test-release-summary test-publish-helm-chart test-govulncheck test-go-licenses test-run-ginkgo test-sticky-pr-comment test-repository-dispatch test-parse-label-filter test-release-branch-freeze test-prerelease-setup test-vcluster-release test-platform-release test-release-lib test-subtree-mirror test-oss-commit-sync test-oss-mirror-staleness test-backport-legacy-allowlist test-backport-legacy-split test-backport-fix-imports test-promote-release test-wait-for-release test-cve-scan test-commitlint test-resolve-github-release test-comment-triggered-check test-conflict-check build-linear-release-sync lint check-bats-jobs install-auto-doc generate-docs check-docs help ACTIONS_DIR := .github/actions WORKFLOWS_DIR := .github/workflows @@ -93,7 +93,7 @@ check-docs: generate-docs ## verify docs are up to date (fails if drift detected help: ## show this help @grep -E '^[a-zA-Z_-]+:.*?## .*$$' $(MAKEFILE_LIST) | awk 'BEGIN {FS = ":.*?## "}; {printf " %-30s %s\n", $$1, $$2}' -test: test-secret-broker-request test-secret-broker-response test-semver-validation test-setup-semstat test-linear-pr-commenter test-link-backport-prs test-select-backport-source-prs test-release-notification test-linear-release-sync test-linear-cve-summary test-aws-test-infra test-cleanup-head-charts test-auto-approve-bot-prs test-ai-pr-review test-ai-step test-release-summary test-ci-test-notify test-go-licenses test-publish-helm-chart test-govulncheck test-run-ginkgo test-sticky-pr-comment test-repository-dispatch test-parse-label-filter test-release-branch-freeze test-prerelease-setup test-vcluster-release test-platform-release test-release-lib test-subtree-mirror test-oss-commit-sync test-oss-mirror-staleness test-backport-legacy-allowlist test-backport-legacy-split test-backport-fix-imports test-promote-release test-wait-for-release test-cve-scan test-commitlint test-resolve-github-release test-comment-triggered-check test-conflict-check ## run all action tests +test: test-secret-broker-request test-secret-broker-response test-semver-validation test-setup-semstat test-linear-pr-commenter test-link-backport-prs test-fork-review-check test-select-backport-source-prs test-release-notification test-linear-release-sync test-linear-cve-summary test-aws-test-infra test-cleanup-head-charts test-auto-approve-bot-prs test-ai-pr-review test-ai-step test-release-summary test-ci-test-notify test-go-licenses test-publish-helm-chart test-govulncheck test-run-ginkgo test-sticky-pr-comment test-repository-dispatch test-parse-label-filter test-release-branch-freeze test-prerelease-setup test-vcluster-release test-platform-release test-release-lib test-subtree-mirror test-oss-commit-sync test-oss-mirror-staleness test-backport-legacy-allowlist test-backport-legacy-split test-backport-fix-imports test-promote-release test-wait-for-release test-cve-scan test-commitlint test-resolve-github-release test-comment-triggered-check test-conflict-check ## run all action tests test-secret-broker-request: ## run secret-broker-request pytest tests uv run --with pytest==8.3.3 pytest $(ACTIONS_DIR)/secret-broker-request/test/ -q @@ -113,6 +113,9 @@ test-linear-pr-commenter: ## run linear-pr-commenter unit tests test-link-backport-prs: ## run link-backport-prs unit tests cd $(ACTIONS_DIR)/link-backport-prs/src && go test -v ./... +test-fork-review-check: ## run fork-review-check unit tests + cd $(ACTIONS_DIR)/fork-review-check/src && go test -v ./... + test-select-backport-source-prs: ## run select-backport-source-prs bats tests bats $(ACTIONS_DIR)/select-backport-source-prs/test/*.bats diff --git a/README.md b/README.md index 18033aa8..2565ec46 100644 --- a/README.md +++ b/README.md @@ -157,6 +157,18 @@ Links modern and legacy backport PRs to the matching Linear sub-issue (`[X.Y] Co See [link-backport-prs README](./.github/actions/link-backport-prs/README.md) for detailed documentation. +### Fork Review Check + +CLI that verifies every fork commit a product build consumes reached its loft-sh fork through a merged pull request approved, at its final head, by someone with write access who is not its author. Read-only, build step 1 of the [fork review check design](https://github.com/loft-sh/architecture/blob/main/engineering-enablement/designs/fork-review-check.md); the required workflow and release receipt come next. + +**Location:** `.github/actions/fork-review-check` + +```bash +go run ./.github/actions/fork-review-check/src ref -repo loft-sh/kine -ref v0.16.5.vcluster.1 +``` + +See [fork-review-check README](./.github/actions/fork-review-check/README.md) for the rules, flags and receipt. + ### Select Backport Source PRs Action Selects one explicit source PR or merged source PRs inside a lookback window From c9cb3fcf29e51d16ba5a15a98c027ef8cee4d387 Mon Sep 17 00:00:00 2001 From: Piotr Zaniewski Date: Tue, 27 Oct 2026 10:06:23 +0100 Subject: [PATCH 2/5] feat(fork-review-check): verify digest-pinned images and release binaries loft-enterprise now pins kine by tag and digest, and the digest wins: a bump of KINE_VERSION alone builds the old image under the new label. A check that the digest matches the tag would be wrong too, since a fork tag can point at the wrong build (v0.14.16.vcluster.4 holds upstream kine v0.13.5). The design's rule closes both: the digest's build provenance must name the fork's release workflow, the tag, and the tag's commit. image mode resolves the tag to its commit, runs gh attestation verify on the digest with signer workflow, signer and source digest, source ref and no self-hosted runners, then checks the fork commits like ref mode. binary mode downloads a release asset, compares its sha256, and verifies the asset's attestation the same way. Both fail closed on an unknown reference or a missing signer workflow. Checked against the registry with gh attestation verify: the kine digest loft-enterprise pins verifies under v0.16.5.vcluster.1 and is rejected under v0.17.2.vcluster.0; the live-image CI job repeats both cases. References DEVOPS-1740 Claude-Session: https://claude.ai/code/session_017edg9gFKBwMnmsAVXxEJzn --- .github/actions/fork-review-check/README.md | 16 +- .../actions/fork-review-check/src/attest.go | 53 ++++++ .../actions/fork-review-check/src/download.go | 68 +++++++ .../actions/fork-review-check/src/gomod.go | 21 ++- .../fork-review-check/src/gomod_test.go | 2 +- .../actions/fork-review-check/src/inputs.go | 169 ++++++++++++++++++ .../fork-review-check/src/inputs_test.go | 164 +++++++++++++++++ .github/actions/fork-review-check/src/main.go | 75 ++++++-- .../fork-review-check/src/main_test.go | 18 +- .../fork-review-check/test/live-image.sh | 20 +++ .github/workflows/test-fork-review-check.yaml | 31 ++++ 11 files changed, 607 insertions(+), 30 deletions(-) create mode 100644 .github/actions/fork-review-check/src/attest.go create mode 100644 .github/actions/fork-review-check/src/download.go create mode 100644 .github/actions/fork-review-check/src/inputs.go create mode 100644 .github/actions/fork-review-check/src/inputs_test.go create mode 100755 .github/actions/fork-review-check/test/live-image.sh diff --git a/.github/actions/fork-review-check/README.md b/.github/actions/fork-review-check/README.md index 20539d20..d09db605 100644 --- a/.github/actions/fork-review-check/README.md +++ b/.github/actions/fork-review-check/README.md @@ -32,12 +32,26 @@ go run . ref -repo loft-sh/kine -ref v0.16.5.vcluster.1 # Every loft-sh fork in the effective Go module graph of one or more modules. GOPRIVATE=github.com/loft-sh/* go run . go -dir /path/to/vnode-internal + +# A digest-pinned fork image. -image and -signer-workflow are repeatable. +go run . image \ + -image ghcr.io/loft-sh/kine:v0.16.5.vcluster.1@sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef \ + -signer-workflow loft-sh/kine=.github/workflows/release.yml + +# A fork release asset. -url and -sha256 are paired in flag order. +go run . binary \ + -url https://github.com/loft-sh/kine/releases/download/v0.16.5.vcluster.1/kine \ + -sha256 0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef \ + -signer-workflow loft-sh/kine=.github/workflows/release.yml ``` | Flag | Meaning | | --- | --- | | `-repo`, `-ref` | `ref` mode: the fork and the tag or commit to check | | `-dir` | `go` mode: module directory, repeatable. S is the commit Go downloaded (`go mod download -json`, `Origin.Hash`), not today's tag | +| `-image` | `image` mode: a `ghcr.io//:@sha256:` image, repeatable | +| `-url`, `-sha256` | `binary` mode: a GitHub release asset URL and its 64-character SHA-256, paired and repeatable | +| `-signer-workflow` | `fork=.github/workflows/release.yml`, repeatable. Required for every image or binary fork | | `-upstream` | `fork=upstream[@branch]`, used only while a fork has no `fork-upstream` property. The missing property still fails; the override only keeps the per-commit evidence coming | | `-as-of` | RFC 3339 time for rule 3 | | `-receipt` | Write the JSON receipt to this path | @@ -57,7 +71,7 @@ Reading PRs, reviews and compares on public forks works with any token. The appr ## Not yet covered -Container images by digest, release binaries by sha256, source clones, and the fail-closed net for unknown fork references are step 4 of the design. +Source clones and the fail-closed net for unknown fork references are still step 4 of the design. ## Tests diff --git a/.github/actions/fork-review-check/src/attest.go b/.github/actions/fork-review-check/src/attest.go new file mode 100644 index 00000000..f1a3c5b0 --- /dev/null +++ b/.github/actions/fork-review-check/src/attest.go @@ -0,0 +1,53 @@ +package main + +import ( + "bytes" + "context" + "fmt" + "os/exec" + "strings" +) + +// attestTool verifies the provenance of a build input; tests replace it. +type attestTool interface { + Verify(ctx context.Context, subject string, args attestArgs) error +} + +type attestArgs struct { + Owner string + Repo string + SignerWorkflow string + SignerDigest string + SourceDigest string + SourceRef string +} + +type execAttest struct{} + +func (execAttest) Verify(ctx context.Context, subject string, a attestArgs) error { + args := []string{"attestation", "verify", subject} + switch { + case a.Owner != "": + args = append(args, "--owner", a.Owner) + case a.Repo != "": + args = append(args, "--repo", a.Repo) + default: + return fmt.Errorf("attestation verifier needs an owner or repository") + } + args = append(args, + "--signer-workflow", a.SignerWorkflow, + "--signer-digest", a.SignerDigest, + "--source-digest", a.SourceDigest, + "--source-ref", a.SourceRef, + "--deny-self-hosted-runners", + "--format", "json", + ) + + cmd := exec.CommandContext(ctx, "gh", args...) + var stderr bytes.Buffer + cmd.Stderr = &stderr + if _, err := cmd.Output(); err != nil { + return fmt.Errorf("gh %s: %w: %s", strings.Join(args, " "), err, strings.TrimSpace(stderr.String())) + } + return nil +} diff --git a/.github/actions/fork-review-check/src/download.go b/.github/actions/fork-review-check/src/download.go new file mode 100644 index 00000000..56439f29 --- /dev/null +++ b/.github/actions/fork-review-check/src/download.go @@ -0,0 +1,68 @@ +package main + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "fmt" + "io" + "net/http" + "os" +) + +// downloadTool downloads a release asset to a temporary file; tests replace it. +type downloadTool interface { + Download(ctx context.Context, url string) (string, error) +} + +type execDownload struct{} + +func (execDownload) Download(ctx context.Context, url string) (string, error) { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + if err != nil { + return "", err + } + resp, err := http.DefaultClient.Do(req) + if err != nil { + return "", err + } + defer resp.Body.Close() + if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices { + return "", fmt.Errorf("GET %s: %s", url, resp.Status) + } + + f, err := os.CreateTemp("", "fork-review-check-*") + if err != nil { + return "", err + } + path := f.Name() + ok := false + defer func() { + if !ok { + _ = os.Remove(path) + } + }() + if _, err := io.Copy(f, resp.Body); err != nil { + _ = f.Close() + return "", err + } + if err := f.Close(); err != nil { + return "", err + } + ok = true + return path, nil +} + +func sha256File(path string) (string, error) { + f, err := os.Open(path) + if err != nil { + return "", err + } + defer f.Close() + + h := sha256.New() + if _, err := io.Copy(h, f); err != nil { + return "", err + } + return hex.EncodeToString(h.Sum(nil)), nil +} diff --git a/.github/actions/fork-review-check/src/gomod.go b/.github/actions/fork-review-check/src/gomod.go index ce019c75..dfe821bb 100644 --- a/.github/actions/fork-review-check/src/gomod.go +++ b/.github/actions/fork-review-check/src/gomod.go @@ -50,15 +50,18 @@ type goTool interface { // inputRecord is one build input the receipt lists, with its hash. type inputRecord struct { - Kind string `json:"kind"` - Source string `json:"source"` - Module string `json:"module,omitempty"` - Path string `json:"path"` - Version string `json:"version"` - Repo string `json:"repo"` - Commit string `json:"commit"` - Sum string `json:"sum,omitempty"` - Fork bool `json:"fork"` + Kind string `json:"kind"` + Source string `json:"source"` + Module string `json:"module,omitempty"` + Path string `json:"path"` + Version string `json:"version"` + Repo string `json:"repo"` + Commit string `json:"commit"` + Sum string `json:"sum,omitempty"` + Digest string `json:"digest,omitempty"` + SHA256 string `json:"sha256,omitempty"` + SignerWorkflow string `json:"signer_workflow,omitempty"` + Fork bool `json:"fork"` } // goForkInputs resolves the loft-sh modules in the effective module graph of diff --git a/.github/actions/fork-review-check/src/gomod_test.go b/.github/actions/fork-review-check/src/gomod_test.go index ff67139d..ea3c9d48 100644 --- a/.github/actions/fork-review-check/src/gomod_test.go +++ b/.github/actions/fork-review-check/src/gomod_test.go @@ -130,7 +130,7 @@ func TestGoModeChecksOnlyForks(t *testing.T) { f.repos["loft-sh/tailscale"] = repoInfo{FullName: "loft-sh/tailscale", Fork: true} f.upstreams["loft-sh/tailscale"] = "tailscale/tailscale" - rc, err := check(context.Background(), f, vnodeGraph(), options{mode: "go", dirs: multiFlag{"vnode"}}, time.Now()) + rc, err := check(context.Background(), f, vnodeGraph(), nil, nil, options{mode: "go", dirs: multiFlag{"vnode"}}, time.Now()) if err != nil { t.Fatal(err) } diff --git a/.github/actions/fork-review-check/src/inputs.go b/.github/actions/fork-review-check/src/inputs.go new file mode 100644 index 00000000..d92f8f73 --- /dev/null +++ b/.github/actions/fork-review-check/src/inputs.go @@ -0,0 +1,169 @@ +package main + +import ( + "context" + "fmt" + "net/url" + "os" + "regexp" + "strings" +) + +var imageDigestRe = regexp.MustCompile(`^sha256:[0-9a-f]{64}$`) +var sha256Re = regexp.MustCompile(`^[0-9a-f]{64}$`) + +type imageReference struct { + Repo string + Owner string + Tag string + Digest string +} + +type binaryReference struct { + Repo string + Tag string + URL string +} + +func parseSignerWorkflows(values []string) (map[string]string, error) { + out := map[string]string{} + for _, value := range values { + repo, workflow, ok := strings.Cut(value, "=") + if !ok || !validRepo(repo) || workflow == "" { + return nil, fmt.Errorf("-signer-workflow %q: want fork=workflow path", value) + } + out[repo] = workflow + } + return out, nil +} + +func imagePin(ctx context.Context, api forkAPI, attest attestTool, signers map[string]string, source string) (pin, inputRecord, string, error) { + ref, failure := parseImageReference(source) + if failure != "" { + return pin{}, inputRecord{}, failure, nil + } + workflow, failure := signerWorkflow(signers, ref.Repo) + if failure != "" { + return pin{}, inputRecord{}, fmt.Sprintf("image %s: %s", source, failure), nil + } + commit, err := api.ResolveTag(ctx, ref.Repo, ref.Tag) + if err != nil { + return pin{}, inputRecord{}, "", fmt.Errorf("resolve image tag %s in %s: %w", ref.Tag, ref.Repo, err) + } + if commit == "" { + return pin{}, inputRecord{}, fmt.Sprintf("image %s: %s has no tag %s", source, ref.Repo, ref.Tag), nil + } + if err := attest.Verify(ctx, "oci://"+source, attestArgs{ + Owner: ref.Owner, + SignerWorkflow: workflow, + SignerDigest: commit, + SourceDigest: commit, + SourceRef: "refs/tags/" + ref.Tag, + }); err != nil { + return pin{}, inputRecord{}, fmt.Sprintf("image %s: verify provenance: %v", source, err), nil + } + return pin{Repo: ref.Repo, Ref: ref.Tag, Tag: ref.Tag, Commit: commit}, inputRecord{ + Kind: "image", + Source: source, + Path: "ghcr.io/" + ref.Repo, + Version: ref.Tag, + Repo: ref.Repo, + Commit: commit, + Digest: ref.Digest, + SignerWorkflow: workflow, + Fork: true, + }, "", nil +} + +func binaryPin(ctx context.Context, api forkAPI, attest attestTool, download downloadTool, signers map[string]string, source, want string) (pin, inputRecord, string, error) { + ref, failure := parseBinaryReference(source) + if failure != "" { + return pin{}, inputRecord{}, failure, nil + } + if !sha256Re.MatchString(want) { + return pin{}, inputRecord{}, fmt.Sprintf("binary %s: -sha256 must be 64 lowercase hexadecimal characters", source), nil + } + workflow, failure := signerWorkflow(signers, ref.Repo) + if failure != "" { + return pin{}, inputRecord{}, fmt.Sprintf("binary %s: %s", source, failure), nil + } + + path, err := download.Download(ctx, source) + if err != nil { + return pin{}, inputRecord{}, "", fmt.Errorf("download binary %s: %w", source, err) + } + defer os.Remove(path) + got, err := sha256File(path) + if err != nil { + return pin{}, inputRecord{}, "", fmt.Errorf("hash binary %s: %w", source, err) + } + if got != want { + return pin{}, inputRecord{}, fmt.Sprintf("binary %s: sha256 is %s, want %s", source, got, want), nil + } + + commit, err := api.ResolveTag(ctx, ref.Repo, ref.Tag) + if err != nil { + return pin{}, inputRecord{}, "", fmt.Errorf("resolve binary tag %s in %s: %w", ref.Tag, ref.Repo, err) + } + if commit == "" { + return pin{}, inputRecord{}, fmt.Sprintf("binary %s: %s has no tag %s", source, ref.Repo, ref.Tag), nil + } + if err := attest.Verify(ctx, path, attestArgs{ + Repo: ref.Repo, + SignerWorkflow: workflow, + SignerDigest: commit, + SourceDigest: commit, + SourceRef: "refs/tags/" + ref.Tag, + }); err != nil { + return pin{}, inputRecord{}, fmt.Sprintf("binary %s: verify provenance: %v", source, err), nil + } + return pin{Repo: ref.Repo, Ref: ref.Tag, Tag: ref.Tag, Commit: commit}, inputRecord{ + Kind: "binary", + Source: source, + Path: source, + Version: ref.Tag, + Repo: ref.Repo, + Commit: commit, + SHA256: want, + SignerWorkflow: workflow, + Fork: true, + }, "", nil +} + +func parseImageReference(source string) (imageReference, string) { + image, digest, ok := strings.Cut(source, "@") + if !ok || !imageDigestRe.MatchString(digest) { + return imageReference{}, fmt.Sprintf("image %s needs both a fork tag and sha256 digest", source) + } + name, tag, ok := strings.Cut(image, ":") + parts := strings.Split(name, "/") + if !ok || len(parts) != 3 || parts[0] != "ghcr.io" || !validRepo(parts[1]+"/"+parts[2]) || !forkTagRe.MatchString(tag) { + return imageReference{}, fmt.Sprintf("unknown fork image reference %s", source) + } + return imageReference{Repo: parts[1] + "/" + parts[2], Owner: parts[1], Tag: tag, Digest: digest}, "" +} + +func parseBinaryReference(source string) (binaryReference, string) { + u, err := url.Parse(source) + if err != nil || u.Scheme != "https" || u.Host != "github.com" || u.RawQuery != "" || u.Fragment != "" { + return binaryReference{}, fmt.Sprintf("unknown fork binary reference %s", source) + } + parts := strings.Split(strings.TrimPrefix(u.Path, "/"), "/") + if len(parts) != 6 || !validRepo(parts[0]+"/"+parts[1]) || parts[2] != "releases" || parts[3] != "download" || !forkTagRe.MatchString(parts[4]) || parts[5] == "" { + return binaryReference{}, fmt.Sprintf("unknown fork binary reference %s", source) + } + return binaryReference{Repo: parts[0] + "/" + parts[1], Tag: parts[4], URL: source}, "" +} + +func signerWorkflow(signers map[string]string, repo string) (string, string) { + workflow := signers[repo] + if workflow == "" { + return "", fmt.Sprintf("%s has no -signer-workflow mapping", repo) + } + return repo + "/" + strings.TrimPrefix(workflow, "/"), "" +} + +func validRepo(repo string) bool { + parts := strings.Split(repo, "/") + return len(parts) == 2 && parts[0] != "" && parts[1] != "" +} diff --git a/.github/actions/fork-review-check/src/inputs_test.go b/.github/actions/fork-review-check/src/inputs_test.go new file mode 100644 index 00000000..163b7e33 --- /dev/null +++ b/.github/actions/fork-review-check/src/inputs_test.go @@ -0,0 +1,164 @@ +package main + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "fmt" + "os" + "path/filepath" + "strings" + "testing" +) + +type fakeAttest struct { + err error + subjects []string + args []attestArgs +} + +func (f *fakeAttest) Verify(_ context.Context, subject string, args attestArgs) error { + f.subjects = append(f.subjects, subject) + f.args = append(f.args, args) + return f.err +} + +type fakeDownload struct { + path string + url string +} + +func (f fakeDownload) Download(_ context.Context, url string) (string, error) { + if f.url != "" && url != f.url { + return "", fmt.Errorf("downloaded %s, want %s", url, f.url) + } + return f.path, nil +} + +func imageSource(digest string) string { + return "ghcr.io/loft-sh/kine:" + tag + "@" + digest +} + +func TestImageModeResolvesAttestedImage(t *testing.T) { + digest := "sha256:" + strings.Repeat("a", 64) + source := imageSource(digest) + attest := &fakeAttest{} + rc, err := check(context.Background(), reviewedFork(), nil, attest, nil, options{ + mode: "image", + images: multiFlag{source}, + signerWorkflows: multiFlag{fork + "=.github/workflows/release.yml"}, + }, t0) + if err != nil { + t.Fatal(err) + } + if !rc.Pass || len(rc.Forks) != 1 || len(rc.Inputs) != 1 { + t.Fatalf("want one passing image: %+v", rc) + } + in := rc.Inputs[0] + if in.Kind != "image" || in.Source != source || in.Digest != digest || in.Commit != merge || in.SignerWorkflow != fork+"/.github/workflows/release.yml" { + t.Errorf("image input = %+v", in) + } + if len(attest.subjects) != 1 || attest.subjects[0] != "oci://"+source { + t.Errorf("attestation subjects = %v", attest.subjects) + } + got := attest.args[0] + if got.Owner != "loft-sh" || got.Repo != "" || got.SignerDigest != merge || got.SourceDigest != merge || got.SourceRef != "refs/tags/"+tag { + t.Errorf("attestation args = %+v", got) + } +} + +func TestImageModeAttestationFailureIsARuleFailure(t *testing.T) { + source := imageSource("sha256:" + strings.Repeat("a", 64)) + attest := &fakeAttest{err: fmt.Errorf("no matching attestation")} + rc, err := check(context.Background(), reviewedFork(), nil, attest, nil, options{ + mode: "image", + images: multiFlag{source}, + signerWorkflows: multiFlag{fork + "=.github/workflows/release.yml"}, + }, t0) + if err != nil { + t.Fatal(err) + } + if rc.Pass || !strings.Contains(strings.Join(rc.Failures, "\n"), source) { + t.Fatalf("want attestation failure naming the image: %+v", rc) + } +} + +func TestImageModeRejectsUnpinnedAndUnknownImages(t *testing.T) { + for _, source := range []string{ + "ghcr.io/loft-sh/kine:" + tag, + "quay.io/loft-sh/kine:" + tag + "@sha256:" + strings.Repeat("a", 64), + } { + rc, err := check(context.Background(), reviewedFork(), nil, nil, nil, options{ + mode: "image", + images: multiFlag{source}, + signerWorkflows: multiFlag{fork + "=.github/workflows/release.yml"}, + }, t0) + if err != nil { + t.Fatal(err) + } + if rc.Pass || len(rc.Failures) != 1 { + t.Fatalf("%s should fail as one rule failure: %+v", source, rc) + } + } +} + +func TestImageModeNeedsSignerWorkflow(t *testing.T) { + source := imageSource("sha256:" + strings.Repeat("a", 64)) + rc, err := check(context.Background(), reviewedFork(), nil, nil, nil, options{ + mode: "image", + images: multiFlag{source}, + }, t0) + if err != nil { + t.Fatal(err) + } + if rc.Pass || !strings.Contains(strings.Join(rc.Failures, "\n"), "no -signer-workflow") { + t.Fatalf("want the missing signer workflow to fail: %+v", rc) + } +} + +func TestBinaryModeResolvesAttestedAsset(t *testing.T) { + source := "https://github.com/loft-sh/kine/releases/download/" + tag + "/kine" + path := filepath.Join(t.TempDir(), "kine") + if err := os.WriteFile(path, []byte("kine"), 0o600); err != nil { + t.Fatal(err) + } + sum := sha256.Sum256([]byte("kine")) + want := hex.EncodeToString(sum[:]) + attest := &fakeAttest{} + rc, err := check(context.Background(), reviewedFork(), nil, attest, fakeDownload{path: path, url: source}, options{ + mode: "binary", + urls: multiFlag{source}, + sha256: multiFlag{want}, + signerWorkflows: multiFlag{fork + "=.github/workflows/release.yml"}, + }, t0) + if err != nil { + t.Fatal(err) + } + if !rc.Pass || len(rc.Inputs) != 1 || rc.Inputs[0].Kind != "binary" || rc.Inputs[0].SHA256 != want { + t.Fatalf("want one passing binary: %+v", rc) + } + if len(attest.args) != 1 || attest.args[0].Repo != fork || attest.args[0].Owner != "" { + t.Errorf("attestation args = %+v", attest.args) + } +} + +func TestBinaryModeRejectsSHA256Mismatch(t *testing.T) { + source := "https://github.com/loft-sh/kine/releases/download/" + tag + "/kine" + path := filepath.Join(t.TempDir(), "kine") + if err := os.WriteFile(path, []byte("kine"), 0o600); err != nil { + t.Fatal(err) + } + attest := &fakeAttest{} + rc, err := check(context.Background(), reviewedFork(), nil, attest, fakeDownload{path: path, url: source}, options{ + mode: "binary", + urls: multiFlag{source}, + sha256: multiFlag{strings.Repeat("0", 64)}, + signerWorkflows: multiFlag{fork + "=.github/workflows/release.yml"}, + }, t0) + if err != nil { + t.Fatal(err) + } + if rc.Pass || !strings.Contains(strings.Join(rc.Failures, "\n"), "sha256 is") || len(attest.args) != 0 { + t.Fatalf("want a checksum rule failure before attestation: %+v", rc) + } +} diff --git a/.github/actions/fork-review-check/src/main.go b/.github/actions/fork-review-check/src/main.go index b51465c0..68f5f3f1 100644 --- a/.github/actions/fork-review-check/src/main.go +++ b/.github/actions/fork-review-check/src/main.go @@ -13,10 +13,13 @@ // 3. With -as-of, every approval predates that time (the product release). // 4. It fails and names every commit that breaks a rule. // -// It reads GitHub and the go command only; it writes nothing but its receipt. +// It reads GitHub, the go command and attestation evidence. It writes only its +// receipt and temporary downloaded release assets. // // fork-review-check ref -repo loft-sh/kine -ref v0.16.5.vcluster.1 // fork-review-check go -dir . -dir ./sub +// fork-review-check image -image ghcr.io/loft-sh/kine:v0.16.5.vcluster.1@sha256:... +// fork-review-check binary -url https://github.com/loft-sh/kine/releases/download/v0.16.5.vcluster.1/kine -sha256 ... // // Exit codes: 0 pass (or -advisory), 1 a rule failed, 2 the check could not run. package main @@ -62,14 +65,18 @@ func (m *multiFlag) String() string { return strings.Join(*m, ",") } func (m *multiFlag) Set(v string) error { *m = append(*m, v); return nil } type options struct { - mode string - repo string - ref string - dirs multiFlag - upstreams multiFlag - asOf string - receipt string - advisory bool + mode string + repo string + ref string + dirs multiFlag + images multiFlag + urls multiFlag + sha256 multiFlag + upstreams multiFlag + signerWorkflows multiFlag + asOf string + receipt string + advisory bool } func main() { @@ -95,7 +102,7 @@ func run(args []string, stdout io.Writer) int { return 2 } - rc, err := check(context.Background(), githubAPI{gh: gh}, execGo{}, opts, time.Now().UTC()) + rc, err := check(context.Background(), githubAPI{gh: gh}, execGo{}, execAttest{}, execDownload{}, opts, time.Now().UTC()) if err != nil { log.Printf("::error::%v", err) return 2 @@ -126,7 +133,7 @@ func run(args []string, stdout io.Writer) int { func parseArgs(args []string) (options, error) { var opts options if len(args) == 0 { - return opts, errors.New("usage: fork-review-check ref|go [flags]") + return opts, errors.New("usage: fork-review-check ref|go|image|binary [flags]") } opts.mode = args[0] @@ -134,7 +141,11 @@ func parseArgs(args []string) (options, error) { fs.StringVar(&opts.repo, "repo", "", "ref mode: fork repository, owner/name") fs.StringVar(&opts.ref, "ref", "", "ref mode: fork release tag or commit SHA") fs.Var(&opts.dirs, "dir", "go mode: module directory to resolve (repeatable)") + fs.Var(&opts.images, "image", "image mode: digest-pinned ghcr.io fork image (repeatable)") + fs.Var(&opts.urls, "url", "binary mode: GitHub release asset URL (repeatable)") + fs.Var(&opts.sha256, "sha256", "binary mode: release asset SHA-256, paired with -url (repeatable)") fs.Var(&opts.upstreams, "upstream", "fork=upstream[@branch] used when a fork has no fork-upstream property; in ref mode a bare upstream applies to -repo (repeatable)") + fs.Var(&opts.signerWorkflows, "signer-workflow", "fork=workflow path used to verify an image or binary (repeatable)") fs.StringVar(&opts.asOf, "as-of", "", "RFC 3339 time every approval must predate (rule 3), normally the product release time") fs.StringVar(&opts.receipt, "receipt", "", "write the JSON receipt to this path") fs.BoolVar(&opts.advisory, "advisory", false, "report failures as warnings and exit 0") @@ -151,14 +162,22 @@ func parseArgs(args []string) (options, error) { if len(opts.dirs) == 0 { opts.dirs = multiFlag{"."} } + case "image": + if len(opts.images) == 0 { + return opts, errors.New("image mode needs -image") + } + case "binary": + if len(opts.urls) == 0 || len(opts.sha256) == 0 || len(opts.urls) != len(opts.sha256) { + return opts, errors.New("binary mode needs paired -url and -sha256 flags") + } default: - return opts, fmt.Errorf("unknown mode %q: want ref or go", opts.mode) + return opts, fmt.Errorf("unknown mode %q: want ref, go, image or binary", opts.mode) } return opts, nil } // check resolves the pins for the mode and evaluates every fork. -func check(ctx context.Context, api forkAPI, tool goTool, opts options, now time.Time) (receipt, error) { +func check(ctx context.Context, api forkAPI, tool goTool, attest attestTool, download downloadTool, opts options, now time.Time) (receipt, error) { rc := receipt{Schema: receiptSchema, CLIVersion: version, Mode: opts.mode, CheckedAt: now, Inputs: []inputRecord{}, Forks: []forkResult{}} var asOf time.Time @@ -174,6 +193,10 @@ func check(ctx context.Context, api forkAPI, tool goTool, opts options, now time if err != nil { return rc, err } + signers, err := parseSignerWorkflows(opts.signerWorkflows) + if err != nil { + return rc, err + } var pins []pin switch opts.mode { @@ -207,6 +230,32 @@ func check(ctx context.Context, api forkAPI, tool goTool, opts options, now time } } } + case "image": + for _, source := range opts.images { + p, in, failure, err := imagePin(ctx, api, attest, signers, source) + if err != nil { + return rc, err + } + if failure != "" { + rc.Failures = append(rc.Failures, failure) + continue + } + pins = append(pins, p) + rc.Inputs = append(rc.Inputs, in) + } + case "binary": + for i, source := range opts.urls { + p, in, failure, err := binaryPin(ctx, api, attest, download, signers, source, opts.sha256[i]) + if err != nil { + return rc, err + } + if failure != "" { + rc.Failures = append(rc.Failures, failure) + continue + } + pins = append(pins, p) + rc.Inputs = append(rc.Inputs, in) + } } c := newChecker(api, asOf, overrides) diff --git a/.github/actions/fork-review-check/src/main_test.go b/.github/actions/fork-review-check/src/main_test.go index edd2656d..29788172 100644 --- a/.github/actions/fork-review-check/src/main_test.go +++ b/.github/actions/fork-review-check/src/main_test.go @@ -8,7 +8,7 @@ import ( ) func TestRefModeResolvesTag(t *testing.T) { - rc, err := check(context.Background(), reviewedFork(), nil, options{mode: "ref", repo: fork, ref: tag}, time.Now()) + rc, err := check(context.Background(), reviewedFork(), nil, nil, nil, options{mode: "ref", repo: fork, ref: tag}, time.Now()) if err != nil { t.Fatal(err) } @@ -25,7 +25,7 @@ func TestRefModeAcceptsFullSHA(t *testing.T) { f.repos[upstream] = repoInfo{FullName: upstream, DefaultBranch: "master"} f.compares[fork+"@k3s-io:master..."+merge] = comparison{Status: "diverged", Total: 3, MergeBase: baseSHA, Commits: f.compares[fork+"@"+baseSHA+"..."+merge].Commits} sha := merge - rc, err := check(context.Background(), f, nil, options{mode: "ref", repo: fork, ref: sha}, time.Now()) + rc, err := check(context.Background(), f, nil, nil, nil, options{mode: "ref", repo: fork, ref: sha}, time.Now()) if err != nil { t.Fatal(err) } @@ -35,7 +35,7 @@ func TestRefModeAcceptsFullSHA(t *testing.T) { } func TestRefModeUnknownRefFails(t *testing.T) { - rc, err := check(context.Background(), reviewedFork(), nil, options{mode: "ref", repo: fork, ref: "v9.9.9"}, time.Now()) + rc, err := check(context.Background(), reviewedFork(), nil, nil, nil, options{mode: "ref", repo: fork, ref: "v9.9.9"}, time.Now()) if err != nil { t.Fatal(err) } @@ -45,7 +45,7 @@ func TestRefModeUnknownRefFails(t *testing.T) { } func TestAsOfFlagAppliesRule3(t *testing.T) { - rc, err := check(context.Background(), reviewedFork(), nil, options{mode: "ref", repo: fork, ref: tag, asOf: t0.Add(30 * time.Minute).Format(time.RFC3339)}, time.Now()) + rc, err := check(context.Background(), reviewedFork(), nil, nil, nil, options{mode: "ref", repo: fork, ref: tag, asOf: t0.Add(30 * time.Minute).Format(time.RFC3339)}, time.Now()) if err != nil { t.Fatal(err) } @@ -59,7 +59,13 @@ func TestParseArgs(t *testing.T) { t.Error("ref mode without -ref must fail") } if _, err := parseArgs([]string{"image"}); err == nil { - t.Error("an unknown mode must fail") + t.Error("image mode without -image must fail") + } + if _, err := parseArgs([]string{"binary", "-url", "https://github.com/loft-sh/kine/releases/download/" + tag + "/kine"}); err == nil { + t.Error("binary mode without -sha256 must fail") + } + if _, err := parseSignerWorkflows([]string{"loft-sh/kine"}); err == nil { + t.Error("a signer workflow without a mapping must fail") } o, err := parseArgs([]string{"go"}) if err != nil || len(o.dirs) != 1 || o.dirs[0] != "." { @@ -83,7 +89,7 @@ func TestParseUpstreams(t *testing.T) { func TestReportsNameFailures(t *testing.T) { f := reviewedFork() delete(f.pulls, fork+"@"+c1) - rc, _ := check(context.Background(), f, nil, options{mode: "ref", repo: fork, ref: tag}, time.Now()) + rc, _ := check(context.Background(), f, nil, nil, nil, options{mode: "ref", repo: fork, ref: tag}, time.Now()) text, md := renderText(rc), renderMarkdown(rc) for _, want := range []string{"FAIL", short(c1), "direct push"} { if !strings.Contains(text, want) || !strings.Contains(md, want) { diff --git a/.github/actions/fork-review-check/test/live-image.sh b/.github/actions/fork-review-check/test/live-image.sh new file mode 100755 index 00000000..88ee4c12 --- /dev/null +++ b/.github/actions/fork-review-check/test/live-image.sh @@ -0,0 +1,20 @@ +#!/usr/bin/env bash +# Runs image mode against the real registry and attestations and checks one +# expectation: WANT=attested means the image's provenance verified, so it is +# listed as an input; WANT=rejected means verification failed and is reported. +set -euo pipefail + +receipt=$(mktemp) +go run . image -image "${IMAGE:?}" \ + -signer-workflow loft-sh/kine=.github/workflows/release.yml \ + -upstream loft-sh/kine=k3s-io/kine \ + -advisory -receipt "$receipt" + +attested=$(jq '[.inputs[] | select(.kind == "image")] | length' "$receipt") +rejected=$(jq '[.failures[]? | select(contains("verify provenance"))] | length' "$receipt") +case "${WANT:?}" in + attested) [[ "$attested" == 1 && "$rejected" == 0 ]] ;; + rejected) [[ "$attested" == 0 && "$rejected" == 1 ]] ;; + *) echo "::error::WANT must be attested or rejected" >&2; exit 2 ;; +esac || { echo "::error::$IMAGE: want $WANT, got attested=$attested rejected=$rejected" >&2; exit 1; } +echo "$IMAGE: $WANT as expected" diff --git a/.github/workflows/test-fork-review-check.yaml b/.github/workflows/test-fork-review-check.yaml index 5bb91efe..1ed440f1 100644 --- a/.github/workflows/test-fork-review-check.yaml +++ b/.github/workflows/test-fork-review-check.yaml @@ -67,3 +67,34 @@ jobs: REF: ${{ matrix.ref }} UPSTREAM: ${{ matrix.upstream }} run: go run . ref -repo "$REPO" -ref "$REF" -upstream "$UPSTREAM" -advisory -receipt /dev/stdout + + # The image path against the real registry and attestations: the kine digest + # loft-enterprise pins must verify against its tag, and the same digest under + # another tag (a bump of KINE_VERSION alone) must not. + live-image: + runs-on: ubuntu-latest + permissions: + contents: read + strategy: + fail-fast: false + matrix: + include: + - image: ghcr.io/loft-sh/kine:v0.16.5.vcluster.1@sha256:7e72281879c7683b3d28d48b330e2f2db9d8a53a8015db10712819a0b8ec855a + want: attested + - image: ghcr.io/loft-sh/kine:v0.17.2.vcluster.0@sha256:7e72281879c7683b3d28d48b330e2f2db9d8a53a8015db10712819a0b8ec855a + want: rejected + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + with: + go-version-file: .github/actions/fork-review-check/src/go.mod + - name: Check ${{ matrix.image }} + working-directory: .github/actions/fork-review-check/src + env: + GITHUB_TOKEN: ${{ github.token }} + GH_TOKEN: ${{ github.token }} + IMAGE: ${{ matrix.image }} + WANT: ${{ matrix.want }} + run: bash ../test/live-image.sh From 6289eb6f6464753ef743b599c4457d8e3007a580 Mon Sep 17 00:00:00 2001 From: Piotr Zaniewski Date: Tue, 27 Oct 2026 11:00:11 +0100 Subject: [PATCH 3/5] feat(fork-review-check): scan build inputs and ship the action with a receipt Product release workflows cannot list every fork their build consumes by hand: kine reaches loft-enterprise as an image, loft-sh/kubernetes as a release binary and a source clone, and vnode-internal as a Go replace. A list kept by hand drifts, and the design (loft-sh/architecture fork-review-check.md, build step 4) asks the check to fail any fork reference no resolver understands, so a new form cannot slip past review. scan walks go.mod, go.work, Dockerfiles, shell scripts, Makefiles, goreleaser files and workflows, resolves each fork reference through the existing go, image and ref paths, and fails references it cannot pin. A templated reference passes only when the caller names that fork with the matching explicit flag. Non-fork loft-sh repos and image names with no repo behind them are ignored. The composite action and the workflow_call wrapper give release workflows one step that writes the JSON receipt to attach to the release. The action script keeps the CLI exit code but still publishes the receipt and pass outputs on a rule failure, so an advisory caller can read them. Builds on 7ac5cbc and c9cb3fc. Claude-Session: https://claude.ai/code/session_017edg9gFKBwMnmsAVXxEJzn --- .github/actions/fork-review-check/README.md | 66 +++- .github/actions/fork-review-check/action.yml | 93 +++++ .../actions/fork-review-check/src/action.sh | 62 ++++ .github/actions/fork-review-check/src/main.go | 180 ++++++--- .github/actions/fork-review-check/src/scan.go | 326 +++++++++++++++++ .../fork-review-check/src/scan_test.go | 343 ++++++++++++++++++ .../fork-review-check/test/action.bats | 97 +++++ .github/workflows/fork-review-check.yaml | 121 ++++++ .github/workflows/test-fork-review-check.yaml | 51 +++ Makefile | 1 + README.md | 10 +- docs/workflows/fork-review-check.md | 46 +++ 12 files changed, 1339 insertions(+), 57 deletions(-) create mode 100644 .github/actions/fork-review-check/action.yml create mode 100755 .github/actions/fork-review-check/src/action.sh create mode 100644 .github/actions/fork-review-check/src/scan.go create mode 100644 .github/actions/fork-review-check/src/scan_test.go create mode 100755 .github/actions/fork-review-check/test/action.bats create mode 100644 .github/workflows/fork-review-check.yaml create mode 100644 docs/workflows/fork-review-check.md diff --git a/.github/actions/fork-review-check/README.md b/.github/actions/fork-review-check/README.md index d09db605..a9a77a0c 100644 --- a/.github/actions/fork-review-check/README.md +++ b/.github/actions/fork-review-check/README.md @@ -2,7 +2,7 @@ CLI behind the fork review check in [loft-sh/architecture `engineering-enablement/designs/fork-review-check.md`](https://github.com/loft-sh/architecture/blob/main/engineering-enablement/designs/fork-review-check.md). It verifies that every fork commit a product build consumes reached its loft-sh fork through a merged pull request approved by someone other than its author. -This is build step 1 of the design: the CLI, run read-only. The workflow and release receipt that make it a required check (step 4) build on it. +It implements build steps 1 and 4 of the design: input resolution and the receipt a product release attaches. ## Rules @@ -61,6 +61,52 @@ Exit codes: 0 pass or advisory, 1 a rule failed, 2 the check could not run. `go` mode needs `GOPRIVATE=github.com/loft-sh/*` so Go fetches loft-sh modules from git and reports the origin commit; without an origin the module fails. +## Scan mode + +`scan` is the default mode of the composite action. It walks the build-input files below `-root` (`go.mod`, `go.work`, Dockerfiles, shell scripts, Makefiles, `.goreleaser` files and workflows) and resolves the fork commit S from what the build consumes. It skips `.git`, `vendor`, `node_modules` and `testdata`, and ignores whole-line comments. + +| Form | Recognised shape | How S is resolved | Templated reference | +| --- | --- | --- | --- | +| Go | A `github.com/loft-sh/...` reference in `go.mod` or `go.work` | The effective Go module graph and `Origin.Hash` | Not applicable | +| Image | Dockerfile `FROM` or `COPY --from=` `ghcr.io/loft-sh/:@sha256:` | The fork release tag after provenance verification | Pass a digest-pinned `-image` for that fork | +| Source | `git clone -b` or `git clone --branch` from `github.com/loft-sh/`, or a workflow's `repository: loft-sh/` checkout | The tag or full SHA, as in `ref` mode | Pass `-source fork=tag-or-full-SHA` | +| Binary | `github.com/loft-sh//releases/download/...` | Explicit URL and SHA-256 after checksum and provenance verification | Pass paired `-url` and `-sha256` | +| Link | Release tags, blobs, trees, pulls, issues, and commits | Ignored | Not applicable | + +Any other fork reference fails, so a new build-input form cannot slip past the check. A templated reference (a variable with no default in the same file) passes only when the matching explicit flag names that fork. A `loft-sh` repository that is not a fork, or an image name with no repository behind it, is ignored. + +## Action inputs + + + +| INPUT | TYPE | REQUIRED | DEFAULT | DESCRIPTION | +|------------------|--------|----------|------------------------------|----------------------------------------------------------------------------------| +| advisory | string | false | `"true"` | Report rule failures without failing the
calling step. | +| as-of | string | false | | RFC 3339 release time that approvals
must predate. | +| binaries | string | false | | Newline-separated GitHub release asset URL and
SHA-256 pairs. | +| dirs | string | false | | Newline-separated Go module directories. | +| github-token | string | false | `"${{ github.token }}"` | GitHub token used for read-only repository,
review, and permission lookups. | +| images | string | false | | Newline-separated digest-pinned ghcr.io fork images. | +| mode | string | false | `"scan"` | CLI mode. scan discovers supported build
inputs by default. | +| receipt | string | false | `"fork-review-receipt.json"` | Path for the JSON evidence receipt. | +| root | string | false | `"."` | Root directory for scan mode. | +| signer-workflows | string | false | | Newline-separated fork=.github/workflows/release.yml attestation mappings. | +| sources | string | false | | Newline-separated fork=tag-or-full-SHA source clone pins. | +| upstreams | string | false | | Newline-separated fork=upstream[@branch] temporary upstream mappings. | + + + +## Action outputs + + + +| OUTPUT | TYPE | DESCRIPTION | +|---------|--------|----------------------------------------| +| pass | string | Whether the receipt passed every rule. | +| receipt | string | Path to the JSON evidence receipt. | + + + ## Receipt The JSON receipt lists each input with its hash, each fork's `fork-upstream` value and source, the base, and every fork commit with its PR, approver, approval time and the approver's permission as read at check time. The permission endpoint returns current access only, so the receipt is the record and a later rerun can differ. @@ -69,9 +115,23 @@ The JSON receipt lists each input with its hash, each fork's `fork-upstream` val Reading PRs, reviews and compares on public forks works with any token. The approver permission lookup (`repos/{owner}/{repo}/collaborators/{user}/permission`) is the open question for the required workflow: `test-fork-review-check.yaml` runs the CLI against kine, tailscale and fuse with the workflow token to show whether that token can read it cross-repo. -## Not yet covered +## Product release caller + +```yaml +- name: Check fork review evidence + id: fork-review + uses: loft-sh/github-actions/.github/actions/fork-review-check@fork-review-check/v1 + with: + receipt: fork-review-receipt.json + +- name: Attach fork review receipt + env: + GH_TOKEN: ${{ github.token }} + TAG: ${{ github.ref_name }} + run: gh release upload "$TAG" fork-review-receipt.json +``` -Source clones and the fail-closed net for unknown fork references are still step 4 of the design. +The reusable `fork-review-check.yaml` workflow wraps the action for releases that also want the receipt as an Actions artifact. See [docs/workflows/fork-review-check.md](../../../docs/workflows/fork-review-check.md). ## Tests diff --git a/.github/actions/fork-review-check/action.yml b/.github/actions/fork-review-check/action.yml new file mode 100644 index 00000000..4b5fbab0 --- /dev/null +++ b/.github/actions/fork-review-check/action.yml @@ -0,0 +1,93 @@ +name: 'Fork review check' +description: 'Verifies that every loft-sh fork commit consumed by a build has approved fork review evidence' + +inputs: + mode: + description: 'CLI mode. scan discovers supported build inputs by default.' + required: false + default: 'scan' + root: + description: 'Root directory for scan mode.' + required: false + default: '.' + dirs: + description: 'Newline-separated Go module directories.' + required: false + default: '' + images: + description: 'Newline-separated digest-pinned ghcr.io fork images.' + required: false + default: '' + binaries: + description: 'Newline-separated GitHub release asset URL and SHA-256 pairs.' + required: false + default: '' + sources: + description: 'Newline-separated fork=tag-or-full-SHA source clone pins.' + required: false + default: '' + signer-workflows: + description: 'Newline-separated fork=.github/workflows/release.yml attestation mappings.' + required: false + default: '' + upstreams: + description: 'Newline-separated fork=upstream[@branch] temporary upstream mappings.' + required: false + default: '' + as-of: + description: 'RFC 3339 release time that approvals must predate.' + required: false + default: '' + advisory: + description: 'Report rule failures without failing the calling step.' + required: false + default: 'true' + receipt: + description: 'Path for the JSON evidence receipt.' + required: false + default: 'fork-review-receipt.json' + github-token: + description: 'GitHub token used for read-only repository, review, and permission lookups.' + required: false + default: ${{ github.token }} + +outputs: + receipt: + description: 'Path to the JSON evidence receipt.' + value: ${{ steps.check.outputs.receipt }} + pass: + description: 'Whether the receipt passed every rule.' + value: ${{ steps.check.outputs.pass }} + +runs: + using: 'composite' + steps: + - name: Set up Go + uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + with: + go-version-file: ${{ github.action_path }}/src/go.mod + + - name: Build fork-review-check + shell: bash + run: go build -o "$RUNNER_TEMP/fork-review-check" "${{ github.action_path }}/src" + + - name: Check fork review evidence + id: check + shell: bash + env: + FORK_REVIEW_CHECK_BIN: ${{ runner.temp }}/fork-review-check + FORK_REVIEW_CHECK_VERSION: ${{ github.action_ref }} + GITHUB_TOKEN: ${{ inputs.github-token }} + GH_TOKEN: ${{ inputs.github-token }} + INPUT_MODE: ${{ inputs.mode }} + INPUT_ROOT: ${{ inputs.root }} + INPUT_DIRS: ${{ inputs.dirs }} + INPUT_IMAGES: ${{ inputs.images }} + INPUT_BINARIES: ${{ inputs.binaries }} + INPUT_SOURCES: ${{ inputs.sources }} + INPUT_SIGNER_WORKFLOWS: ${{ inputs.signer-workflows }} + INPUT_UPSTREAMS: ${{ inputs.upstreams }} + INPUT_AS_OF: ${{ inputs.as-of }} + INPUT_ADVISORY: ${{ inputs.advisory }} + INPUT_RECEIPT: ${{ inputs.receipt }} + run: bash "${{ github.action_path }}/src/action.sh" diff --git a/.github/actions/fork-review-check/src/action.sh b/.github/actions/fork-review-check/src/action.sh new file mode 100755 index 00000000..8e16ee54 --- /dev/null +++ b/.github/actions/fork-review-check/src/action.sh @@ -0,0 +1,62 @@ +#!/usr/bin/env bash +set -euo pipefail + +trim() { + local value=$1 + value="${value#"${value%%[![:space:]]*}"}" + printf '%s' "${value%"${value##*[![:space:]]}"}" +} + +append_lines() { + local flag=$1 + local values=$2 + local value + while IFS= read -r value || [ -n "$value" ]; do + value=$(trim "$value") + [ -n "$value" ] || continue + argv+=("$flag" "$value") + done <<< "$values" +} + +argv=( + "$FORK_REVIEW_CHECK_BIN" + "$INPUT_MODE" + -root "$INPUT_ROOT" + -receipt "$INPUT_RECEIPT" + "-advisory=$INPUT_ADVISORY" +) + +append_lines -dir "$INPUT_DIRS" +append_lines -image "$INPUT_IMAGES" +append_lines -source "$INPUT_SOURCES" +append_lines -signer-workflow "$INPUT_SIGNER_WORKFLOWS" +append_lines -upstream "$INPUT_UPSTREAMS" + +if [ -n "$INPUT_AS_OF" ]; then + argv+=(-as-of "$INPUT_AS_OF") +fi + +binary= +while IFS= read -r binary || [ -n "$binary" ]; do + binary=$(trim "$binary") + [ -n "$binary" ] || continue + read -r url sha256 extra <<< "$binary" + if [ -z "${url:-}" ] || [ -z "${sha256:-}" ] || [ -n "${extra:-}" ]; then + echo "::error::binaries entries must be " >&2 + exit 2 + fi + argv+=(-url "$url" -sha256 "$sha256") +done <<< "$INPUT_BINARIES" + +# A rule failure (exit 1) still writes the receipt, so publish the outputs +# before passing the exit code through. +status=0 +"${argv[@]}" || status=$? + +if [ -s "$INPUT_RECEIPT" ]; then + { + printf 'receipt=%s\n' "$INPUT_RECEIPT" + printf 'pass=%s\n' "$(jq -r '.pass' "$INPUT_RECEIPT")" + } >> "$GITHUB_OUTPUT" +fi +exit "$status" diff --git a/.github/actions/fork-review-check/src/main.go b/.github/actions/fork-review-check/src/main.go index 68f5f3f1..b17c6742 100644 --- a/.github/actions/fork-review-check/src/main.go +++ b/.github/actions/fork-review-check/src/main.go @@ -18,6 +18,7 @@ // // fork-review-check ref -repo loft-sh/kine -ref v0.16.5.vcluster.1 // fork-review-check go -dir . -dir ./sub +// fork-review-check scan -root . // fork-review-check image -image ghcr.io/loft-sh/kine:v0.16.5.vcluster.1@sha256:... // fork-review-check binary -url https://github.com/loft-sh/kine/releases/download/v0.16.5.vcluster.1/kine -sha256 ... // @@ -68,6 +69,8 @@ type options struct { mode string repo string ref string + root string + sources multiFlag dirs multiFlag images multiFlag urls multiFlag @@ -133,17 +136,19 @@ func run(args []string, stdout io.Writer) int { func parseArgs(args []string) (options, error) { var opts options if len(args) == 0 { - return opts, errors.New("usage: fork-review-check ref|go|image|binary [flags]") + return opts, errors.New("usage: fork-review-check ref|go|image|binary|scan [flags]") } opts.mode = args[0] fs := flag.NewFlagSet("fork-review-check "+opts.mode, flag.ContinueOnError) fs.StringVar(&opts.repo, "repo", "", "ref mode: fork repository, owner/name") fs.StringVar(&opts.ref, "ref", "", "ref mode: fork release tag or commit SHA") + fs.StringVar(&opts.root, "root", ".", "scan mode: root directory to inspect") fs.Var(&opts.dirs, "dir", "go mode: module directory to resolve (repeatable)") fs.Var(&opts.images, "image", "image mode: digest-pinned ghcr.io fork image (repeatable)") fs.Var(&opts.urls, "url", "binary mode: GitHub release asset URL (repeatable)") fs.Var(&opts.sha256, "sha256", "binary mode: release asset SHA-256, paired with -url (repeatable)") + fs.Var(&opts.sources, "source", "scan mode: fork=tag-or-full-SHA source clone pin (repeatable)") fs.Var(&opts.upstreams, "upstream", "fork=upstream[@branch] used when a fork has no fork-upstream property; in ref mode a bare upstream applies to -repo (repeatable)") fs.Var(&opts.signerWorkflows, "signer-workflow", "fork=workflow path used to verify an image or binary (repeatable)") fs.StringVar(&opts.asOf, "as-of", "", "RFC 3339 time every approval must predate (rule 3), normally the product release time") @@ -154,6 +159,10 @@ func parseArgs(args []string) (options, error) { } switch opts.mode { + case "scan": + if len(opts.urls) != len(opts.sha256) { + return opts, errors.New("scan mode needs paired -url and -sha256 flags") + } case "ref": if opts.repo == "" || opts.ref == "" { return opts, errors.New("ref mode needs -repo and -ref") @@ -171,11 +180,102 @@ func parseArgs(args []string) (options, error) { return opts, errors.New("binary mode needs paired -url and -sha256 flags") } default: - return opts, fmt.Errorf("unknown mode %q: want ref, go, image or binary", opts.mode) + return opts, fmt.Errorf("unknown mode %q: want ref, go, image, binary or scan", opts.mode) } return opts, nil } +type inputResolver struct { + ctx context.Context + api forkAPI + tool goTool + attest attestTool + download downloadTool + signers map[string]string + rc *receipt + pins []pin + forks map[string]bool +} + +func (r *inputResolver) add(p pin, in inputRecord, failure string, err error) error { + if err != nil { + return err + } + if failure != "" { + r.rc.Failures = append(r.rc.Failures, failure) + return nil + } + r.pins = append(r.pins, p) + r.rc.Inputs = append(r.rc.Inputs, in) + return nil +} + +func (r *inputResolver) goDirs(dirs []string) error { + for _, dir := range dirs { + inputs, failures, err := goForkInputs(r.tool, dir) + if err != nil { + return err + } + r.rc.Failures = append(r.rc.Failures, failures...) + for _, in := range inputs { + fork, err := r.fork(in.Repo) + if err != nil { + return fmt.Errorf("read %s: %w", in.Repo, err) + } + in.Fork = fork + r.rc.Inputs = append(r.rc.Inputs, in) + if fork { + r.pins = append(r.pins, pinFromGoInput(in)) + } + } + } + return nil +} + +func (r *inputResolver) images(sources []string) error { + for _, source := range sources { + if err := r.add(imagePin(r.ctx, r.api, r.attest, r.signers, source)); err != nil { + return err + } + } + return nil +} + +func (r *inputResolver) binaries(urls, hashes []string) error { + if len(urls) != len(hashes) { + return errors.New("binary inputs need paired -url and -sha256 flags") + } + for i, source := range urls { + if err := r.add(binaryPin(r.ctx, r.api, r.attest, r.download, r.signers, source, hashes[i])); err != nil { + return err + } + } + return nil +} + +func (r *inputResolver) sources(values []string, source string) error { + for _, value := range values { + repo, ref, err := sourceParts(value) + if err != nil { + return err + } + p, failure, err := refPin(r.ctx, r.api, repo, ref) + in := inputRecord{ + Kind: "fork-source", + Source: source, + Path: repo, + Version: ref, + Repo: repo, + Commit: p.Commit, + Fork: true, + } + if err := r.add(p, in, failure, err); err != nil { + return err + } + } + return nil +} + // check resolves the pins for the mode and evaluates every fork. func check(ctx context.Context, api forkAPI, tool goTool, attest attestTool, download downloadTool, opts options, now time.Time) (receipt, error) { rc := receipt{Schema: receiptSchema, CLIVersion: version, Mode: opts.mode, CheckedAt: now, Inputs: []inputRecord{}, Forks: []forkResult{}} @@ -198,69 +298,49 @@ func check(ctx context.Context, api forkAPI, tool goTool, attest attestTool, dow return rc, err } - var pins []pin + r := inputResolver{ + ctx: ctx, api: api, tool: tool, attest: attest, download: download, + signers: signers, rc: &rc, + } switch opts.mode { case "ref": p, failure, err := refPin(ctx, api, opts.repo, opts.ref) - if err != nil { - return rc, err + in := inputRecord{ + Kind: "fork-ref", + Source: "-ref", + Path: opts.repo, + Version: opts.ref, + Repo: opts.repo, + Commit: p.Commit, + Fork: true, } - if failure != "" { - rc.Failures = append(rc.Failures, failure) - } else { - pins = append(pins, p) - rc.Inputs = append(rc.Inputs, inputRecord{Kind: "fork-ref", Source: "-ref", Path: opts.repo, Version: opts.ref, Repo: opts.repo, Commit: p.Commit, Fork: true}) + if err := r.add(p, in, failure, err); err != nil { + return rc, err } case "go": - for _, dir := range opts.dirs { - inputs, failures, err := goForkInputs(tool, dir) - if err != nil { - return rc, err - } - rc.Failures = append(rc.Failures, failures...) - for _, in := range inputs { - fork, err := isFork(ctx, api, in.Repo) - if err != nil { - return rc, fmt.Errorf("read %s: %w", in.Repo, err) - } - in.Fork = fork - rc.Inputs = append(rc.Inputs, in) - if fork { - pins = append(pins, pinFromGoInput(in)) - } - } + if err := r.goDirs(opts.dirs); err != nil { + return rc, err } case "image": - for _, source := range opts.images { - p, in, failure, err := imagePin(ctx, api, attest, signers, source) - if err != nil { - return rc, err - } - if failure != "" { - rc.Failures = append(rc.Failures, failure) - continue - } - pins = append(pins, p) - rc.Inputs = append(rc.Inputs, in) + if err := r.images(opts.images); err != nil { + return rc, err } case "binary": - for i, source := range opts.urls { - p, in, failure, err := binaryPin(ctx, api, attest, download, signers, source, opts.sha256[i]) - if err != nil { - return rc, err - } - if failure != "" { - rc.Failures = append(rc.Failures, failure) - continue - } - pins = append(pins, p) - rc.Inputs = append(rc.Inputs, in) + if err := r.binaries(opts.urls, opts.sha256); err != nil { + return rc, err } + case "scan": + if err := r.scan(opts); err != nil { + return rc, err + } + } + if opts.mode == "scan" { + sort.Strings(rc.Failures) } c := newChecker(api, asOf, overrides) seen := map[string]bool{} - for _, p := range pins { + for _, p := range r.pins { key := p.Repo + "@" + p.Commit + "@" + p.Tag if seen[key] { continue diff --git a/.github/actions/fork-review-check/src/scan.go b/.github/actions/fork-review-check/src/scan.go new file mode 100644 index 00000000..1110a953 --- /dev/null +++ b/.github/actions/fork-review-check/src/scan.go @@ -0,0 +1,326 @@ +package main + +import ( + "fmt" + "io/fs" + "os" + "path/filepath" + "regexp" + "strings" +) + +var scanRepoRe = regexp.MustCompile(`(?:github\.com|ghcr\.io)/loft-sh/([A-Za-z0-9._-]+)`) +var scanVarRe = regexp.MustCompile(`\$\{([A-Za-z_][A-Za-z0-9_]*)\}|\$([A-Za-z_][A-Za-z0-9_]*)`) + +// scanCheckoutRe matches actions/checkout's repository input, a source clone +// that names the fork without a github.com prefix. +var scanCheckoutRe = regexp.MustCompile(`^(?:-\s*)?repository:\s*["']?loft-sh/([A-Za-z0-9._-]+)`) + +func scanFile(path string) bool { + name := filepath.Base(path) + switch { + case name == "go.mod", name == "go.work", name == "Makefile": + return true + case name == "Dockerfile", strings.HasPrefix(name, "Dockerfile."): + return true + case strings.HasSuffix(name, ".Dockerfile"), strings.HasSuffix(name, ".dockerfile"): + return true + case strings.HasSuffix(name, ".sh"), strings.HasSuffix(name, ".mk"): + return true + case name == ".goreleaser.yml", name == ".goreleaser.yaml": + return true + } + return scanWorkflow(path) +} + +func scanWorkflow(path string) bool { + name := filepath.Base(path) + return strings.Contains("/"+filepath.ToSlash(path), "/.github/workflows/") && + (strings.HasSuffix(name, ".yml") || strings.HasSuffix(name, ".yaml")) +} + +func scanComment(path, line string) bool { + line = strings.TrimSpace(line) + switch filepath.Base(path) { + case "go.mod", "go.work": + return strings.HasPrefix(line, "//") + default: + return strings.HasPrefix(line, "#") + } +} + +func scanExpand(value string, args map[string]string) string { + return scanVarRe.ReplaceAllStringFunc(value, func(variable string) string { + name := strings.TrimPrefix(variable, "$") + name = strings.TrimSuffix(strings.TrimPrefix(name, "{"), "}") + if value, ok := args[name]; ok { + return value + } + return variable + }) +} + +func scanArg(line string, args map[string]string) { + fields := strings.Fields(line) + if len(fields) < 2 || !strings.EqualFold(fields[0], "ARG") { + return + } + name, value, ok := strings.Cut(strings.TrimSpace(line[len(fields[0]):]), "=") + if ok { + args[strings.TrimSpace(name)] = scanExpand(strings.Trim(value, `"'`), args) + } +} + +func scanImage(line, repo string) string { + fields := strings.Fields(line) + if len(fields) < 2 { + return "" + } + var image string + switch { + case strings.EqualFold(fields[0], "FROM"): + for _, field := range fields[1:] { + if !strings.HasPrefix(field, "--") { + image = strings.Trim(field, `"'`) + break + } + } + case strings.EqualFold(fields[0], "COPY"): + for _, field := range fields[1:] { + if strings.HasPrefix(field, "--from=") { + image = strings.Trim(strings.TrimPrefix(field, "--from="), `"'`) + break + } + } + } + match := scanRepoRe.FindStringSubmatch(image) + if len(match) != 2 || "loft-sh/"+strings.TrimSuffix(match[1], ".git") != repo { + return "" + } + if !strings.HasPrefix(image, "ghcr.io/") { + return "" + } + return image +} + +func scanSource(line string) (string, bool) { + fields := strings.Fields(line) + git, clone := false, false + for _, field := range fields { + git = git || field == "git" + clone = clone || field == "clone" + } + if !git || !clone { + return "", false + } + for i, field := range fields { + if (field == "-b" || field == "--branch") && i+1 < len(fields) { + return strings.Trim(fields[i+1], `"'`), true + } + if strings.HasPrefix(field, "--branch=") { + return strings.Trim(strings.TrimPrefix(field, "--branch="), `"'`), true + } + } + return "", true +} + +func sourceParts(value string) (string, string, error) { + repo, ref, ok := strings.Cut(value, "=") + if !ok || !validRepo(repo) || ref == "" { + return "", "", fmt.Errorf("-source %q: want fork=tag-or-full-SHA", value) + } + return repo, ref, nil +} + +// fork caches scan discovery. A GitHub 404 is not a fork: ghcr image names do +// not necessarily identify GitHub repositories. +func (r *inputResolver) fork(repo string) (bool, error) { + if r.forks == nil { + return isFork(r.ctx, r.api, repo) + } + if fork, ok := r.forks[repo]; ok { + return fork, nil + } + info, err := r.api.Repo(r.ctx, repo) + if isNotFound(err) { + r.forks[repo] = false + return false, nil + } + if err != nil { + return false, err + } + fork := info.Fork + if !fork { + upstream, err := r.api.ForkUpstream(r.ctx, repo) + if err != nil { + return false, err + } + fork = upstream != "" + } + r.forks[repo] = fork + return fork, nil +} + +func (r *inputResolver) scan(opts options) error { + r.forks = map[string]bool{} + covered := map[string]bool{} + for _, source := range opts.images { + if ref, failure := parseImageReference(source); failure == "" { + covered["image:"+ref.Repo] = true + } + } + for _, source := range opts.urls { + if ref, failure := parseBinaryReference(source); failure == "" { + covered["binary:"+ref.Repo] = true + } + } + for _, source := range opts.sources { + repo, _, err := sourceParts(source) + if err != nil { + return err + } + covered["source:"+repo] = true + } + + if err := r.goDirs(opts.dirs); err != nil { + return err + } + if err := r.images(opts.images); err != nil { + return err + } + if err := r.binaries(opts.urls, opts.sha256); err != nil { + return err + } + if err := r.sources(opts.sources, "-source"); err != nil { + return err + } + + goDirs := map[string]bool{} + for _, dir := range opts.dirs { + abs, err := filepath.Abs(dir) + if err != nil { + return err + } + goDirs[abs] = true + } + root := opts.root + if root == "" { + root = "." + } + return filepath.WalkDir(root, func(path string, entry fs.DirEntry, walkErr error) error { + if walkErr != nil { + return fmt.Errorf("scan %s: %w", path, walkErr) + } + if entry.IsDir() { + switch entry.Name() { + case ".git", "vendor", "node_modules", "testdata": + return filepath.SkipDir + } + return nil + } + if !entry.Type().IsRegular() || !scanFile(path) { + return nil + } + data, err := os.ReadFile(path) + if err != nil { + return fmt.Errorf("read build input %s: %w", path, err) + } + rel, err := filepath.Rel(root, path) + if err != nil { + return err + } + args := map[string]string{} + for i, raw := range strings.Split(string(data), "\n") { + if scanComment(path, raw) { + continue + } + line := strings.TrimSpace(raw) + scanArg(line, args) + line = scanExpand(line, args) + location := fmt.Sprintf("%s:%d", filepath.ToSlash(rel), i+1) + if match := scanCheckoutRe.FindStringSubmatch(line); match != nil && scanWorkflow(path) { + repo := "loft-sh/" + strings.TrimSuffix(match[1], ".git") + fork, err := r.fork(repo) + if err != nil { + return fmt.Errorf("read %s: %w", repo, err) + } + if fork { + r.cover(location, "source", repo, "source", covered) + } + } + for _, match := range scanRepoRe.FindAllStringSubmatchIndex(line, -1) { + repo := "loft-sh/" + strings.TrimSuffix(line[match[2]:match[3]], ".git") + fork, err := r.fork(repo) + if err != nil { + return fmt.Errorf("read %s: %w", repo, err) + } + if !fork { + continue + } + suffix := line[match[1]:] + if scanLink(suffix) { + continue + } + if name := filepath.Base(path); name == "go.mod" || name == "go.work" { + dir, err := filepath.Abs(filepath.Dir(path)) + if err != nil { + return err + } + if !goDirs[dir] { + goDirs[dir] = true + if err := r.goDirs([]string{dir}); err != nil { + return err + } + } + continue + } + if strings.HasPrefix(suffix, "/releases/download/") { + r.cover(location, "binary", repo, "url", covered) + continue + } + if image := scanImage(line, repo); image != "" { + if _, failure := parseImageReference(image); failure == "" { + if err := r.images([]string{image}); err != nil { + return err + } + continue + } + r.cover(location, "image", repo, "image", covered) + continue + } + if ref, clone := scanSource(line); clone { + if ref != "" && !strings.Contains(ref, "$") { + if err := r.sources([]string{repo + "=" + ref}, location); err != nil { + return err + } + continue + } + r.cover(location, "source", repo, "source", covered) + continue + } + r.rc.Failures = append(r.rc.Failures, + fmt.Sprintf("%s: reference to fork %s that no resolver understands", location, repo)) + } + } + return nil + }) +} + +func (r *inputResolver) cover(location, form, repo, flag string, covered map[string]bool) { + if !covered[form+":"+repo] { + r.rc.Failures = append(r.rc.Failures, + fmt.Sprintf("%s: %s reference to %s is not pinned in the file; pass it with -%s", + location, form, repo, flag)) + } +} + +func scanLink(suffix string) bool { + for _, prefix := range []string{ + "/releases/tag/", "/blob/", "/tree/", "/pull/", "/issues/", "/commit/", + } { + if strings.HasPrefix(suffix, prefix) { + return true + } + } + return false +} diff --git a/.github/actions/fork-review-check/src/scan_test.go b/.github/actions/fork-review-check/src/scan_test.go new file mode 100644 index 00000000..3eb3b5cd --- /dev/null +++ b/.github/actions/fork-review-check/src/scan_test.go @@ -0,0 +1,343 @@ +package main + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "fmt" + "net/http" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/google/go-github/v88/github" +) + +func scanFixture(t *testing.T, files map[string]string) string { + t.Helper() + root := t.TempDir() + for name, body := range files { + path := filepath.Join(root, name) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte(body), 0o600); err != nil { + t.Fatal(err) + } + } + return root +} + +func scanFork() *fakeAPI { + f := reviewedFork() + f.repos[fork] = repoInfo{FullName: fork, Fork: true} + return f +} + +func scanOptions(root string) options { + return options{ + mode: "scan", + root: root, + signerWorkflows: multiFlag{fork + "=.github/workflows/release.yml"}, + } +} + +func scanCheck(t *testing.T, root string, f forkAPI, tool goTool, attest attestTool, download downloadTool, opts options) receipt { + t.Helper() + rc, err := check(context.Background(), f, tool, attest, download, opts, t0) + if err != nil { + t.Fatal(err) + } + if rc.Mode != "scan" { + t.Fatalf("receipt mode = %q, want scan", rc.Mode) + } + return rc +} + +func TestScanResolvesImageAndSourceARGDefaults(t *testing.T) { + digest := "sha256:" + strings.Repeat("a", 64) + root := scanFixture(t, map[string]string{ + "Dockerfile": "ARG TAG=\"v0.16.5.vcluster.1\"\n" + + "ARG DIGEST=" + digest + "\n" + + "FROM ghcr.io/loft-sh/kine:${TAG}@${DIGEST}\n" + + "RUN git clone -b \"$TAG\" https://github.com/loft-sh/kine.git\n", + }) + attest := &fakeAttest{} + rc := scanCheck(t, root, scanFork(), nil, attest, nil, scanOptions(root)) + if !rc.Pass || len(rc.Inputs) != 2 || len(rc.Forks) != 1 { + t.Fatalf("scan result = %+v", rc) + } + if rc.Inputs[0].Kind != "image" || rc.Inputs[1].Kind != "fork-source" || rc.Inputs[1].Source != "Dockerfile:4" { + t.Fatalf("inputs = %+v", rc.Inputs) + } + if got := attest.subjects; len(got) != 1 || got[0] != "oci://"+imageSource(digest) { + t.Fatalf("attestation subjects = %v", got) + } +} + +func TestScanTemplatedInputsNeedExplicitCoverage(t *testing.T) { + image := "ghcr.io/loft-sh/kine:" + tag + "@sha256:" + strings.Repeat("a", 64) + cases := []struct { + name string + file string + opts func(options) options + download downloadTool + want string + wantPass bool + }{ + { + name: "image", + file: "FROM ghcr.io/loft-sh/kine:" + tag + "\n", + want: "Dockerfile:1: image reference to loft-sh/kine is not pinned in the file; pass it with -image", + }, + { + name: "covered image", + file: "FROM ghcr.io/loft-sh/kine:" + tag + "\n", + opts: func(o options) options { + o.images = multiFlag{image} + return o + }, + wantPass: true, + }, + { + name: "source", + file: "ARG TAG\nRUN git clone --branch ${TAG} https://github.com/loft-sh/kine.git\n", + want: "Dockerfile:2: source reference to loft-sh/kine is not pinned in the file; pass it with -source", + }, + { + name: "covered source", + file: "ARG TAG\nRUN git clone --branch ${TAG} https://github.com/loft-sh/kine.git\n", + opts: func(o options) options { + o.sources = multiFlag{fork + "=" + tag} + return o + }, + wantPass: true, + }, + { + name: "binary", + file: "curl -LO https://github.com/loft-sh/kine/releases/download/" + tag + "/kine\n", + want: "build.sh:1: binary reference to loft-sh/kine is not pinned in the file; pass it with -url", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + name := "Dockerfile" + if strings.Contains(tc.file, "curl") { + name = "build.sh" + } + root := scanFixture(t, map[string]string{name: tc.file}) + opts := scanOptions(root) + if tc.opts != nil { + opts = tc.opts(opts) + } + rc := scanCheck(t, root, scanFork(), nil, &fakeAttest{}, tc.download, opts) + if rc.Pass != tc.wantPass { + t.Fatalf("pass = %t, want %t: %+v", rc.Pass, tc.wantPass, rc) + } + if tc.want != "" && !strings.Contains(strings.Join(rc.Failures, "\n"), tc.want) { + t.Fatalf("failures = %v, want %q", rc.Failures, tc.want) + } + }) + } +} + +func TestScanCoveredBinaryUsesNormalResolver(t *testing.T) { + root := t.TempDir() + asset := filepath.Join(root, "asset") + if err := os.WriteFile(asset, []byte("kine"), 0o600); err != nil { + t.Fatal(err) + } + sum := sha256.Sum256([]byte("kine")) + want := hex.EncodeToString(sum[:]) + url := "https://github.com/loft-sh/kine/releases/download/" + tag + "/kine" + if err := os.WriteFile(filepath.Join(root, "build.sh"), []byte("curl -LO "+url+"\n"), 0o600); err != nil { + t.Fatal(err) + } + opts := scanOptions(root) + opts.urls = multiFlag{url} + opts.sha256 = multiFlag{want} + rc := scanCheck(t, root, scanFork(), nil, &fakeAttest{}, fakeDownload{path: asset, url: url}, opts) + if !rc.Pass || len(rc.Inputs) != 1 || rc.Inputs[0].Kind != "binary" { + t.Fatalf("scan result = %+v", rc) + } +} + +func TestScanIgnoresLinksCommentsAndNonForks(t *testing.T) { + root := scanFixture(t, map[string]string{ + "build.sh": "# https://github.com/loft-sh/kine\n" + + "curl https://github.com/loft-sh/kine/releases/tag/" + tag + "\n" + + "echo https://github.com/loft-sh/product\n", + "go.mod": "// require github.com/loft-sh/kine v" + tag + "\n", + }) + f := scanFork() + f.repos["loft-sh/product"] = repoInfo{FullName: "loft-sh/product"} + rc := scanCheck(t, root, f, nil, nil, nil, scanOptions(root)) + if !rc.Pass || len(rc.Inputs) != 0 || len(rc.Failures) != 0 { + t.Fatalf("scan result = %+v", rc) + } +} + +type missingRepoAPI struct{ *fakeAPI } + +func (f missingRepoAPI) Repo(ctx context.Context, repo string) (repoInfo, error) { + if repo == "loft-sh/kine-fips" { + return repoInfo{}, &github.ErrorResponse{Response: &http.Response{StatusCode: http.StatusNotFound}} + } + return f.fakeAPI.Repo(ctx, repo) +} + +func TestScanIgnoresMissingImageRepository(t *testing.T) { + root := scanFixture(t, map[string]string{ + "Dockerfile": "FROM ghcr.io/loft-sh/kine-fips:latest\n", + }) + rc := scanCheck(t, root, missingRepoAPI{scanFork()}, nil, nil, nil, scanOptions(root)) + if !rc.Pass || len(rc.Failures) != 0 { + t.Fatalf("scan result = %+v", rc) + } +} + +func TestScanUnknownReferenceNamesFileAndLine(t *testing.T) { + root := scanFixture(t, map[string]string{ + "build.sh": "echo https://github.com/loft-sh/kine\n", + }) + rc := scanCheck(t, root, scanFork(), nil, nil, nil, scanOptions(root)) + want := "build.sh:1: reference to fork loft-sh/kine that no resolver understands" + if rc.Pass || len(rc.Failures) != 1 || rc.Failures[0] != want { + t.Fatalf("failures = %v, want %q", rc.Failures, want) + } +} + +type scanGo struct { + fakeGo + dirs []string +} + +func (g *scanGo) ListModules(dir string) ([]goModule, error) { + g.dirs = append(g.dirs, dir) + return g.fakeGo.ListModules(dir) +} + +func TestScanRunsGoResolverOncePerDirectory(t *testing.T) { + root := scanFixture(t, map[string]string{ + "go.mod": "module example.test/product\n\nrequire github.com/loft-sh/kine " + tag + "\n", + "go.work": "go 1.25\n\nuse .\n\nreplace github.com/loft-sh/kine => github.com/loft-sh/kine " + tag + "\n", + }) + g := &scanGo{fakeGo: fakeGo{ + mods: []goModule{{Path: "github.com/" + fork, Version: tag}}, + downloads: map[string]goDownload{ + "github.com/" + fork + "@" + tag: { + Origin: &goOrigin{URL: "https://github.com/" + fork, Hash: merge}, + }, + }, + }} + rc := scanCheck(t, root, scanFork(), g, nil, nil, scanOptions(root)) + if !rc.Pass || len(rc.Inputs) != 1 || len(g.dirs) != 1 || rc.Inputs[0].Kind != "go-module" { + t.Fatalf("scan result = %+v, calls = %v", rc, g.dirs) + } +} + +func TestScanSkipsExcludedAndNonInputFiles(t *testing.T) { + root := scanFixture(t, map[string]string{ + "README.md": "https://github.com/loft-sh/kine\n", + "go.sum": "https://github.com/loft-sh/kine\n", + "main.go": "https://github.com/loft-sh/kine\n", + "vendor/x.sh": "https://github.com/loft-sh/kine\n", + "testdata/x.sh": "https://github.com/loft-sh/kine\n", + "node_modules/x.sh": "https://github.com/loft-sh/kine\n", + ".git/x.sh": "https://github.com/loft-sh/kine\n", + }) + rc := scanCheck(t, root, scanFork(), nil, nil, nil, scanOptions(root)) + if !rc.Pass || len(rc.Failures) != 0 { + t.Fatalf("scan result = %+v", rc) + } +} + +func TestScanFailuresAreSortedAndArgsNeedPairs(t *testing.T) { + root := scanFixture(t, map[string]string{ + "z.sh": "echo https://github.com/loft-sh/kine\n", + "a.sh": "echo https://github.com/loft-sh/kine\n", + }) + rc := scanCheck(t, root, scanFork(), nil, nil, nil, scanOptions(root)) + if got := strings.Join(rc.Failures, "\n"); !strings.HasPrefix(got, "a.sh:1\n") && !strings.HasPrefix(got, "a.sh:1:") { + t.Fatalf("failures are not sorted: %v", rc.Failures) + } + opts, err := parseArgs([]string{"scan"}) + if err != nil || opts.root != "." { + t.Fatalf("scan args = %+v, %v", opts, err) + } + if _, err := parseArgs([]string{"scan", "-url", "https://example.test/file"}); err == nil { + t.Fatal("scan URL without SHA-256 must fail") + } +} + +func TestScanSourceFullCommit(t *testing.T) { + root := scanFixture(t, map[string]string{ + "build.sh": "git clone -b " + merge + " https://github.com/loft-sh/kine.git\n", + }) + f := scanFork() + f.repos[upstream] = repoInfo{FullName: upstream, DefaultBranch: "master"} + f.compares[fork+"@k3s-io:master..."+merge] = comparison{ + Status: "diverged", Total: 3, MergeBase: baseSHA, + Commits: f.compares[fork+"@"+baseSHA+"..."+merge].Commits, + } + rc := scanCheck(t, root, f, nil, nil, nil, scanOptions(root)) + if !rc.Pass || rc.Inputs[0].Commit != merge { + t.Fatalf("scan result = %+v", rc) + } +} + +func TestSourceParts(t *testing.T) { + if _, _, err := sourceParts("loft-sh/kine"); err == nil { + t.Fatal("source without a ref must fail") + } + repo, ref, err := sourceParts(fork + "=" + tag) + if err != nil || repo != fork || ref != tag { + t.Fatalf("source = %q, %q, %v", repo, ref, err) + } +} + +func TestScanUsesClockForReceipt(t *testing.T) { + root := scanFixture(t, map[string]string{}) + now := time.Date(2026, 10, 27, 9, 0, 0, 0, time.UTC) + rc, err := check(context.Background(), scanFork(), nil, nil, nil, scanOptions(root), now) + if err != nil || !rc.CheckedAt.Equal(now) { + t.Fatalf("receipt = %+v, err = %v", rc, err) + } +} + +func TestScanOperationalRepoErrorStopsCheck(t *testing.T) { + root := scanFixture(t, map[string]string{ + "build.sh": "echo https://github.com/loft-sh/kine\n", + }) + f := scanFork() + f.repos = nil + _, err := check(context.Background(), f, nil, nil, nil, scanOptions(root), t0) + if err == nil || !strings.Contains(fmt.Sprint(err), "read loft-sh/kine") { + t.Fatalf("error = %v", err) + } +} + +func TestScanCheckoutOfForkNeedsSource(t *testing.T) { + root := scanFixture(t, map[string]string{ + ".github/workflows/release.yaml": "steps:\n" + + " - uses: actions/checkout@v7\n" + + " with:\n" + + " repository: loft-sh/kine\n" + + " ref: ${{ env.KINE_VERSION }}\n", + "hack/build.yaml": "repository: loft-sh/kine\n", + }) + rc := scanCheck(t, root, scanFork(), nil, nil, nil, scanOptions(root)) + want := ".github/workflows/release.yaml:4: source reference to loft-sh/kine is not pinned in the file; pass it with -source" + if rc.Pass || len(rc.Failures) != 1 || rc.Failures[0] != want { + t.Fatalf("failures = %q, want only %q", rc.Failures, want) + } + + opts := scanOptions(root) + opts.sources = multiFlag{fork + "=" + tag} + rc = scanCheck(t, root, scanFork(), nil, nil, nil, opts) + if !rc.Pass || len(rc.Inputs) != 1 || rc.Inputs[0].Kind != "fork-source" { + t.Fatalf("scan result = %+v", rc) + } +} diff --git a/.github/actions/fork-review-check/test/action.bats b/.github/actions/fork-review-check/test/action.bats new file mode 100755 index 00000000..7bdf414a --- /dev/null +++ b/.github/actions/fork-review-check/test/action.bats @@ -0,0 +1,97 @@ +#!/usr/bin/env bats + +SCRIPT="$BATS_TEST_DIRNAME/../src/action.sh" + +setup() { + MOCK_DIR=$(mktemp -d) + export MOCK_DIR + export GITHUB_OUTPUT="$MOCK_DIR/github_output" + export FORK_REVIEW_CHECK_BIN="$MOCK_DIR/fork-review-check" + export ARGS_FILE="$MOCK_DIR/args" + export INPUT_MODE=scan + export INPUT_ROOT=target + export INPUT_DIRS=$' cmd/api \ncmd/worker' + export INPUT_IMAGES='ghcr.io/loft-sh/kine:v0.16.5.vcluster.1@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' + export INPUT_BINARIES='https://github.com/loft-sh/kine/releases/download/v0.16.5.vcluster.1/kine bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb' + export INPUT_SOURCES='loft-sh/kine=v0.16.5.vcluster.1' + export INPUT_SIGNER_WORKFLOWS='loft-sh/kine=.github/workflows/release.yml' + export INPUT_UPSTREAMS='loft-sh/kine=k3s-io/kine' + export INPUT_AS_OF='2026-10-27T09:00:00Z' + export INPUT_ADVISORY=true + export INPUT_RECEIPT="$MOCK_DIR/receipt.json" + + cat > "$FORK_REVIEW_CHECK_BIN" <<'MOCK' +#!/usr/bin/env bash +set -euo pipefail +printf '%s\n' "$@" > "$ARGS_FILE" +for ((i = 1; i <= $#; i++)); do + if [ "${!i}" = "-receipt" ]; then + next=$((i + 1)) + printf '{"pass":true}\n' > "${!next}" + fi +done +MOCK + chmod +x "$FORK_REVIEW_CHECK_BIN" +} + +teardown() { + rm -rf "$MOCK_DIR" +} + +@test "passes every supported input as an argument" { + run bash "$SCRIPT" + [ "$status" -eq 0 ] + + [ "$(cat "$ARGS_FILE")" = "scan +-root +target +-receipt +$MOCK_DIR/receipt.json +-advisory=true +-dir +cmd/api +-dir +cmd/worker +-image +ghcr.io/loft-sh/kine:v0.16.5.vcluster.1@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +-source +loft-sh/kine=v0.16.5.vcluster.1 +-signer-workflow +loft-sh/kine=.github/workflows/release.yml +-upstream +loft-sh/kine=k3s-io/kine +-as-of +2026-10-27T09:00:00Z +-url +https://github.com/loft-sh/kine/releases/download/v0.16.5.vcluster.1/kine +-sha256 +bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" ] + [ "$(cat "$GITHUB_OUTPUT")" = "receipt=$MOCK_DIR/receipt.json +pass=true" ] +} + +@test "writes outputs and passes the exit code through on a rule failure" { + cat > "$FORK_REVIEW_CHECK_BIN" <<'MOCK' +#!/usr/bin/env bash +printf '{"pass":false}\n' > "$INPUT_RECEIPT" +exit 1 +MOCK + run bash "$SCRIPT" + [ "$status" -eq 1 ] + [ "$(cat "$GITHUB_OUTPUT")" = "receipt=$MOCK_DIR/receipt.json +pass=false" ] +} + +@test "skips outputs when the check cannot run" { + printf '#!/usr/bin/env bash\nexit 2\n' > "$FORK_REVIEW_CHECK_BIN" + run bash "$SCRIPT" + [ "$status" -eq 2 ] + [ ! -s "$GITHUB_OUTPUT" ] +} + +@test "rejects malformed binary input" { + INPUT_BINARIES='https://github.com/loft-sh/kine/releases/download/v0.16.5.vcluster.1/kine' \ + run bash "$SCRIPT" + [ "$status" -eq 2 ] + [[ "$output" == *"binaries entries must be "* ]] +} diff --git a/.github/workflows/fork-review-check.yaml b/.github/workflows/fork-review-check.yaml new file mode 100644 index 00000000..95f9b583 --- /dev/null +++ b/.github/workflows/fork-review-check.yaml @@ -0,0 +1,121 @@ +name: Fork review check + +on: + workflow_call: + inputs: + mode: + description: 'CLI mode.' + required: false + type: string + default: 'scan' + root: + description: 'Root directory for scan mode.' + required: false + type: string + default: '.' + dirs: + description: 'Newline-separated Go module directories.' + required: false + type: string + default: '' + images: + description: 'Newline-separated digest-pinned fork images.' + required: false + type: string + default: '' + binaries: + description: 'Newline-separated release URL and SHA-256 pairs.' + required: false + type: string + default: '' + sources: + description: 'Newline-separated fork=tag-or-full-SHA source pins.' + required: false + type: string + default: '' + signer-workflows: + description: 'Newline-separated attestation workflow mappings.' + required: false + type: string + default: '' + upstreams: + description: 'Newline-separated temporary upstream mappings.' + required: false + type: string + default: '' + as-of: + description: 'RFC 3339 release time that approvals must predate.' + required: false + type: string + default: '' + advisory: + description: 'Report rule failures without failing the caller.' + required: false + type: boolean + default: true + receipt: + description: 'Path for the JSON evidence receipt.' + required: false + type: string + default: 'fork-review-receipt.json' + ref: + description: 'Caller ref to check out. Empty uses the event ref.' + required: false + type: string + default: '' + artifact-name: + description: 'Name for the uploaded receipt artifact.' + required: false + type: string + default: 'fork-review-receipt' + secrets: + github-token: + description: 'Token for GitHub read APIs. Defaults to the workflow token.' + required: false + outputs: + pass: + description: 'Whether every fork review rule passed.' + value: ${{ jobs.check.outputs.pass }} + artifact-name: + description: 'Name of the uploaded receipt artifact.' + value: ${{ jobs.check.outputs.artifact-name }} + +permissions: {} + +jobs: + check: + runs-on: ubuntu-latest + permissions: + contents: read + outputs: + pass: ${{ steps.check.outputs.pass }} + artifact-name: ${{ inputs.artifact-name }} + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + ref: ${{ inputs.ref }} + persist-credentials: false + + - id: check + uses: loft-sh/github-actions/.github/actions/fork-review-check@fork-review-check/v1 # zizmor: ignore[unpinned-uses] -- internal loft-sh/github-actions coordination tag + with: + mode: ${{ inputs.mode }} + root: ${{ inputs.root }} + dirs: ${{ inputs.dirs }} + images: ${{ inputs.images }} + binaries: ${{ inputs.binaries }} + sources: ${{ inputs.sources }} + signer-workflows: ${{ inputs.signer-workflows }} + upstreams: ${{ inputs.upstreams }} + as-of: ${{ inputs.as-of }} + advisory: ${{ inputs.advisory }} + receipt: ${{ inputs.receipt }} + github-token: ${{ secrets.github-token || github.token }} + + - name: Upload fork review receipt + if: always() + uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 + with: + name: ${{ inputs.artifact-name }} + path: ${{ inputs.receipt }} + if-no-files-found: warn diff --git a/.github/workflows/test-fork-review-check.yaml b/.github/workflows/test-fork-review-check.yaml index 1ed440f1..75557c9a 100644 --- a/.github/workflows/test-fork-review-check.yaml +++ b/.github/workflows/test-fork-review-check.yaml @@ -5,10 +5,12 @@ on: branches: [main] paths: - '.github/actions/fork-review-check/**' + - '.github/workflows/fork-review-check.yaml' - '.github/workflows/test-fork-review-check.yaml' pull_request: paths: - '.github/actions/fork-review-check/**' + - '.github/workflows/fork-review-check.yaml' - '.github/workflows/test-fork-review-check.yaml' workflow_dispatch: @@ -30,6 +32,55 @@ jobs: - run: go test ./... working-directory: .github/actions/fork-review-check/src + bats: + runs-on: ubuntu-latest + permissions: + contents: read + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + - uses: bats-core/bats-action@77d6fb60505b4d0d1d73e48bd035b55074bbfb43 # 4.0.0 + with: + support-install: false + assert-install: false + detik-install: false + file-install: false + - name: Run fork-review-check action tests + run: bats .github/actions/fork-review-check/test/*.bats + - uses: bats-core/bats-action@77d6fb60505b4d0d1d73e48bd035b55074bbfb43 # 4.0.0 + with: + support-install: false + assert-install: false + detik-install: false + file-install: false + - name: Test action wrapper + run: bats .github/actions/fork-review-check/test/*.bats + + live-scan: + continue-on-error: true + runs-on: ubuntu-latest + permissions: + contents: read + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + repository: loft-sh/kubernetes + path: target + persist-credentials: false + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + with: + go-version-file: .github/actions/fork-review-check/src/go.mod + - name: Scan loft-sh/kubernetes + working-directory: .github/actions/fork-review-check/src + env: + GITHUB_TOKEN: ${{ github.token }} + GH_TOKEN: ${{ github.token }} + run: go run . scan -root "$GITHUB_WORKSPACE/target" -advisory -receipt /dev/stdout + # Read-only run against the shipped fork pins the design names, with the # workflow token a required workflow gets. Advisory: it records today's gap # and whether that token can read approver permission on another repo, and diff --git a/Makefile b/Makefile index fad8fbdb..691b46d8 100644 --- a/Makefile +++ b/Makefile @@ -115,6 +115,7 @@ test-link-backport-prs: ## run link-backport-prs unit tests test-fork-review-check: ## run fork-review-check unit tests cd $(ACTIONS_DIR)/fork-review-check/src && go test -v ./... + bats $(ACTIONS_DIR)/fork-review-check/test/*.bats test-select-backport-source-prs: ## run select-backport-source-prs bats tests bats $(ACTIONS_DIR)/select-backport-source-prs/test/*.bats diff --git a/README.md b/README.md index 2565ec46..ba43c44f 100644 --- a/README.md +++ b/README.md @@ -159,15 +159,17 @@ See [link-backport-prs README](./.github/actions/link-backport-prs/README.md) fo ### Fork Review Check -CLI that verifies every fork commit a product build consumes reached its loft-sh fork through a merged pull request approved, at its final head, by someone with write access who is not its author. Read-only, build step 1 of the [fork review check design](https://github.com/loft-sh/architecture/blob/main/engineering-enablement/designs/fork-review-check.md); the required workflow and release receipt come next. +Composite action and CLI that verify every fork commit a product build consumes reached its loft-sh fork through a merged pull request approved, at its final head, by someone with write access who is not its author. Read-only. By default it scans the caller's build inputs and writes a JSON receipt for the product release. Build steps 1 and 4 of the [fork review check design](https://github.com/loft-sh/architecture/blob/main/engineering-enablement/designs/fork-review-check.md). Released under the `fork-review-check/v1` tag, shared with the `fork-review-check.yaml` reusable workflow. **Location:** `.github/actions/fork-review-check` -```bash -go run ./.github/actions/fork-review-check/src ref -repo loft-sh/kine -ref v0.16.5.vcluster.1 +```yaml +- uses: loft-sh/github-actions/.github/actions/fork-review-check@fork-review-check/v1 + with: + receipt: fork-review-receipt.json ``` -See [fork-review-check README](./.github/actions/fork-review-check/README.md) for the rules, flags and receipt. +See [fork-review-check README](./.github/actions/fork-review-check/README.md) for the rules, scan forms, inputs and receipt. ### Select Backport Source PRs Action diff --git a/docs/workflows/fork-review-check.md b/docs/workflows/fork-review-check.md new file mode 100644 index 00000000..1d1f23cf --- /dev/null +++ b/docs/workflows/fork-review-check.md @@ -0,0 +1,46 @@ +# Fork review check + +Runs the [fork-review-check](../../.github/actions/fork-review-check/README.md) composite action on the caller's ref, then uploads its JSON evidence receipt as an Actions artifact. + +## Secrets + + + +| SECRET | REQUIRED | DESCRIPTION | +|--------------|----------|------------------------------------------------------------------| +| github-token | false | Token for GitHub read APIs. Defaults
to the workflow token. | + + + +## Inputs + + + +| INPUT | TYPE | REQUIRED | DEFAULT | DESCRIPTION | +|------------------|---------|----------|------------------------------|---------------------------------------------------------| +| advisory | boolean | false | `true` | Report rule failures without failing the
caller. | +| artifact-name | string | false | `"fork-review-receipt"` | Name for the uploaded receipt artifact. | +| as-of | string | false | | RFC 3339 release time that approvals
must predate. | +| binaries | string | false | | Newline-separated release URL and SHA-256 pairs. | +| dirs | string | false | | Newline-separated Go module directories. | +| images | string | false | | Newline-separated digest-pinned fork images. | +| mode | string | false | `"scan"` | CLI mode. | +| receipt | string | false | `"fork-review-receipt.json"` | Path for the JSON evidence receipt. | +| ref | string | false | | Caller ref to check out. Empty
uses the event ref. | +| root | string | false | `"."` | Root directory for scan mode. | +| signer-workflows | string | false | | Newline-separated attestation workflow mappings. | +| sources | string | false | | Newline-separated fork=tag-or-full-SHA source pins. | +| upstreams | string | false | | Newline-separated temporary upstream mappings. | + + + +## Outputs + + + +| OUTPUT | VALUE | DESCRIPTION | +|---------------|---------------------------------------------|----------------------------------------| +| artifact-name | `"${{ jobs.check.outputs.artifact-name }}"` | Name of the uploaded receipt artifact. | +| pass | `"${{ jobs.check.outputs.pass }}"` | Whether every fork review rule passed. | + + From cc708a6969e19a54ece5eecaaa867c402592ab36 Mon Sep 17 00:00:00 2001 From: Piotr Zaniewski Date: Tue, 27 Oct 2026 11:03:09 +0100 Subject: [PATCH 4/5] fix(fork-review-check): trace repo-less ghcr images to their package repository scan treated any ghcr.io/loft-sh image whose name matched no repository as not a fork and skipped it. kine-fips is such an image: today loft-sh/fips-builds builds it from upstream rancher/kine, so skipping it was right by luck. A fork that published an image under another name would have passed with no review evidence, a hole in the fail-closed net the design promises auditors. The scan now asks GitHub which repository the container package is linked to. A non-fork build repository is still ignored; a fork publishing under another name fails, and so does a package the token cannot read, because the image could carry fork code. The job needs packages: read for that lookup. Claude-Session: https://claude.ai/code/session_017edg9gFKBwMnmsAVXxEJzn --- .github/actions/fork-review-check/README.md | 4 +- .../fork-review-check/src/check_test.go | 5 ++ .../actions/fork-review-check/src/github.go | 14 +++++ .github/actions/fork-review-check/src/main.go | 2 + .github/actions/fork-review-check/src/scan.go | 54 +++++++++++++++++-- .../fork-review-check/src/scan_test.go | 48 +++++++++++++++-- .github/workflows/fork-review-check.yaml | 1 + .github/workflows/test-fork-review-check.yaml | 1 + 8 files changed, 120 insertions(+), 9 deletions(-) diff --git a/.github/actions/fork-review-check/README.md b/.github/actions/fork-review-check/README.md index a9a77a0c..75e12d8b 100644 --- a/.github/actions/fork-review-check/README.md +++ b/.github/actions/fork-review-check/README.md @@ -73,7 +73,7 @@ Exit codes: 0 pass or advisory, 1 a rule failed, 2 the check could not run. | Binary | `github.com/loft-sh//releases/download/...` | Explicit URL and SHA-256 after checksum and provenance verification | Pass paired `-url` and `-sha256` | | Link | Release tags, blobs, trees, pulls, issues, and commits | Ignored | Not applicable | -Any other fork reference fails, so a new build-input form cannot slip past the check. A templated reference (a variable with no default in the same file) passes only when the matching explicit flag names that fork. A `loft-sh` repository that is not a fork, or an image name with no repository behind it, is ignored. +Any other fork reference fails, so a new build-input form cannot slip past the check. A templated reference (a variable with no default in the same file) passes only when the matching explicit flag names that fork. A `loft-sh` repository that is not a fork is ignored. An image whose name matches no repository (`ghcr.io/loft-sh/kine-fips`) is traced to the repository GitHub links its package to: a non-fork build repository such as `loft-sh/fips-builds` is ignored, a fork publishing under another name fails, and a package that cannot be read fails. ## Action inputs @@ -113,7 +113,7 @@ The JSON receipt lists each input with its hash, each fork's `fork-upstream` val ## Token -Reading PRs, reviews and compares on public forks works with any token. The approver permission lookup (`repos/{owner}/{repo}/collaborators/{user}/permission`) is the open question for the required workflow: `test-fork-review-check.yaml` runs the CLI against kine, tailscale and fuse with the workflow token to show whether that token can read it cross-repo. +Reading PRs, reviews and compares on public forks works with any token. `scan` also reads container package metadata, so the job needs `packages: read`. The approver permission lookup (`repos/{owner}/{repo}/collaborators/{user}/permission`) is the open question for the required workflow: `test-fork-review-check.yaml` runs the CLI against kine, tailscale and fuse with the workflow token to show whether that token can read it cross-repo. ## Product release caller diff --git a/.github/actions/fork-review-check/src/check_test.go b/.github/actions/fork-review-check/src/check_test.go index fd9e2e88..7849090a 100644 --- a/.github/actions/fork-review-check/src/check_test.go +++ b/.github/actions/fork-review-check/src/check_test.go @@ -20,6 +20,7 @@ type fakeAPI struct { perms map[string]string // repo/user permErr error permCalled int + packages map[string]string // owner/name -> linked repo } func newFake() *fakeAPI { @@ -43,6 +44,10 @@ func (f *fakeAPI) Repo(_ context.Context, repo string) (repoInfo, error) { return r, nil } +func (f *fakeAPI) PackageRepo(_ context.Context, owner, name string) (string, error) { + return f.packages[owner+"/"+name], nil +} + func (f *fakeAPI) ForkUpstream(_ context.Context, repo string) (string, error) { return f.upstreams[repo], nil } diff --git a/.github/actions/fork-review-check/src/github.go b/.github/actions/fork-review-check/src/github.go index b2fb5246..da50577b 100644 --- a/.github/actions/fork-review-check/src/github.go +++ b/.github/actions/fork-review-check/src/github.go @@ -68,6 +68,9 @@ type forkAPI interface { LastForcePush(ctx context.Context, repo string, number int) (time.Time, error) // Permission returns the collaborator permission: admin, write, read or none. Permission(ctx context.Context, repo, user string) (string, error) + // PackageRepo returns the repository a container package is linked to, or + // "" when the package does not exist or has no repository. + PackageRepo(ctx context.Context, owner, name string) (string, error) } type githubAPI struct { @@ -88,6 +91,17 @@ func (a githubAPI) Repo(ctx context.Context, repo string) (repoInfo, error) { return repoInfo{FullName: r.GetFullName(), DefaultBranch: r.GetDefaultBranch(), Fork: r.GetFork()}, nil } +func (a githubAPI) PackageRepo(ctx context.Context, owner, name string) (string, error) { + p, _, err := a.gh.Organizations.GetPackage(ctx, owner, "container", name) + if isNotFound(err) { + return "", nil + } + if err != nil { + return "", err + } + return p.GetRepository().GetFullName(), nil +} + func (a githubAPI) ForkUpstream(ctx context.Context, repo string) (string, error) { owner, name := splitRepo(repo) values, _, err := a.gh.Repositories.GetAllCustomPropertyValues(ctx, owner, name) diff --git a/.github/actions/fork-review-check/src/main.go b/.github/actions/fork-review-check/src/main.go index b17c6742..4963b9af 100644 --- a/.github/actions/fork-review-check/src/main.go +++ b/.github/actions/fork-review-check/src/main.go @@ -195,6 +195,8 @@ type inputResolver struct { rc *receipt pins []pin forks map[string]bool + // published maps a ghcr.io/loft-sh image name to the repository that publishes it. + published map[string]string } func (r *inputResolver) add(p pin, in inputRecord, failure string, err error) error { diff --git a/.github/actions/fork-review-check/src/scan.go b/.github/actions/fork-review-check/src/scan.go index 1110a953..b539afc1 100644 --- a/.github/actions/fork-review-check/src/scan.go +++ b/.github/actions/fork-review-check/src/scan.go @@ -9,7 +9,7 @@ import ( "strings" ) -var scanRepoRe = regexp.MustCompile(`(?:github\.com|ghcr\.io)/loft-sh/([A-Za-z0-9._-]+)`) +var scanRepoRe = regexp.MustCompile(`(github\.com|ghcr\.io)/loft-sh/([A-Za-z0-9._-]+)`) var scanVarRe = regexp.MustCompile(`\$\{([A-Za-z_][A-Za-z0-9_]*)\}|\$([A-Za-z_][A-Za-z0-9_]*)`) // scanCheckoutRe matches actions/checkout's repository input, a source clone @@ -94,7 +94,7 @@ func scanImage(line, repo string) string { } } match := scanRepoRe.FindStringSubmatch(image) - if len(match) != 2 || "loft-sh/"+strings.TrimSuffix(match[1], ".git") != repo { + if len(match) != 3 || "loft-sh/"+strings.TrimSuffix(match[2], ".git") != repo { return "" } if !strings.HasPrefix(image, "ghcr.io/") { @@ -161,8 +161,31 @@ func (r *inputResolver) fork(repo string) (bool, error) { return fork, nil } +// imageRepo returns the repository that publishes ghcr.io/loft-sh/: the +// repository of the same name, else the one GitHub links the package to. It +// is "" when neither exists. A failed package lookup is a rule failure, since +// the image could come from a fork. +func (r *inputResolver) imageRepo(name string) (string, string, error) { + repo := "loft-sh/" + name + if published, ok := r.published[repo]; ok { + return published, "", nil + } + published := repo + if _, err := r.api.Repo(r.ctx, repo); isNotFound(err) { + published, err = r.api.PackageRepo(r.ctx, "loft-sh", name) + if err != nil { + return "", fmt.Sprintf("cannot tell which repository publishes ghcr.io/%s: %v", repo, err), nil + } + } else if err != nil { + return "", "", fmt.Errorf("read %s: %w", repo, err) + } + r.published[repo] = published + return published, "", nil +} + func (r *inputResolver) scan(opts options) error { r.forks = map[string]bool{} + r.published = map[string]string{} covered := map[string]bool{} for _, source := range opts.images { if ref, failure := parseImageReference(source); failure == "" { @@ -249,7 +272,32 @@ func (r *inputResolver) scan(opts options) error { } } for _, match := range scanRepoRe.FindAllStringSubmatchIndex(line, -1) { - repo := "loft-sh/" + strings.TrimSuffix(line[match[2]:match[3]], ".git") + repo := "loft-sh/" + strings.TrimSuffix(line[match[4]:match[5]], ".git") + if line[match[2]:match[3]] == "ghcr.io" { + published, failure, err := r.imageRepo(strings.TrimPrefix(repo, "loft-sh/")) + if err != nil { + return err + } + if failure != "" { + r.rc.Failures = append(r.rc.Failures, location+": "+failure) + continue + } + if published == "" { + continue + } + if published != repo { + fork, err := r.fork(published) + if err != nil { + return fmt.Errorf("read %s: %w", published, err) + } + if fork { + r.rc.Failures = append(r.rc.Failures, fmt.Sprintf( + "%s: image ghcr.io/%s is published by fork %s under another name, which no resolver understands", + location, repo, published)) + } + continue + } + } fork, err := r.fork(repo) if err != nil { return fmt.Errorf("read %s: %w", repo, err) diff --git a/.github/actions/fork-review-check/src/scan_test.go b/.github/actions/fork-review-check/src/scan_test.go index 3eb3b5cd..bf1d1697 100644 --- a/.github/actions/fork-review-check/src/scan_test.go +++ b/.github/actions/fork-review-check/src/scan_test.go @@ -4,6 +4,7 @@ import ( "context" "crypto/sha256" "encoding/hex" + "errors" "fmt" "net/http" "os" @@ -188,13 +189,52 @@ func (f missingRepoAPI) Repo(ctx context.Context, repo string) (repoInfo, error) return f.fakeAPI.Repo(ctx, repo) } -func TestScanIgnoresMissingImageRepository(t *testing.T) { +type packageErrAPI struct{ missingRepoAPI } + +func (f packageErrAPI) PackageRepo(context.Context, string, string) (string, error) { + return "", errors.New("403 Resource not accessible by integration") +} + +// kine-fips has no repository of its own. Which repository publishes the +// package decides whether it can carry fork code. +func TestScanResolvesImageWithoutRepositoryThroughItsPackage(t *testing.T) { root := scanFixture(t, map[string]string{ "Dockerfile": "FROM ghcr.io/loft-sh/kine-fips:latest\n", }) - rc := scanCheck(t, root, missingRepoAPI{scanFork()}, nil, nil, nil, scanOptions(root)) - if !rc.Pass || len(rc.Failures) != 0 { - t.Fatalf("scan result = %+v", rc) + cases := []struct { + name string + publish string + api func(*fakeAPI) forkAPI + want string + }{ + {name: "no package", api: func(f *fakeAPI) forkAPI { return missingRepoAPI{f} }}, + {name: "published by a non-fork build repo", publish: "loft-sh/fips-builds", api: func(f *fakeAPI) forkAPI { return missingRepoAPI{f} }}, + { + name: "published by a fork under another name", publish: fork, + api: func(f *fakeAPI) forkAPI { return missingRepoAPI{f} }, + want: "Dockerfile:1: image ghcr.io/loft-sh/kine-fips is published by fork loft-sh/kine under another name, which no resolver understands", + }, + { + name: "package lookup fails", api: func(f *fakeAPI) forkAPI { return packageErrAPI{missingRepoAPI{f}} }, + want: "Dockerfile:1: cannot tell which repository publishes ghcr.io/loft-sh/kine-fips: 403 Resource not accessible by integration", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + f := scanFork() + f.repos["loft-sh/fips-builds"] = repoInfo{FullName: "loft-sh/fips-builds"} + f.packages = map[string]string{"loft-sh/kine-fips": tc.publish} + rc := scanCheck(t, root, tc.api(f), nil, nil, nil, scanOptions(root)) + if tc.want == "" { + if !rc.Pass || len(rc.Failures) != 0 { + t.Fatalf("scan result = %+v", rc) + } + return + } + if rc.Pass || len(rc.Failures) != 1 || rc.Failures[0] != tc.want { + t.Fatalf("failures = %q, want only %q", rc.Failures, tc.want) + } + }) } } diff --git a/.github/workflows/fork-review-check.yaml b/.github/workflows/fork-review-check.yaml index 95f9b583..c04a952a 100644 --- a/.github/workflows/fork-review-check.yaml +++ b/.github/workflows/fork-review-check.yaml @@ -87,6 +87,7 @@ jobs: runs-on: ubuntu-latest permissions: contents: read + packages: read outputs: pass: ${{ steps.check.outputs.pass }} artifact-name: ${{ inputs.artifact-name }} diff --git a/.github/workflows/test-fork-review-check.yaml b/.github/workflows/test-fork-review-check.yaml index 75557c9a..7996b7f7 100644 --- a/.github/workflows/test-fork-review-check.yaml +++ b/.github/workflows/test-fork-review-check.yaml @@ -62,6 +62,7 @@ jobs: runs-on: ubuntu-latest permissions: contents: read + packages: read steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: From 7ae75e492320941ad0f4161dce0d4064b06fc569 Mon Sep 17 00:00:00 2001 From: Piotr Zaniewski Date: Tue, 27 Oct 2026 17:21:05 +0100 Subject: [PATCH 5/5] fix(fork-review-check): verify reviewed inputs and commits Builds using vendored Go code can compile sources that differ from the downloaded modules, and a PR association can survive changes to the reviewed head. Require a trusted vendor verifier and prove each commit is covered by the approved head or the verified merge result.\n\nA web-flow committer label alone can be spoofed, so the merge exception also requires GitHub signature verification. --- .github/actions/fork-review-check/README.md | 7 +- .../actions/fork-review-check/src/check.go | 42 +++- .../fork-review-check/src/check_test.go | 43 +++- .../actions/fork-review-check/src/github.go | 2 + .../fork-review-check/src/github_test.go | 33 +++ .../actions/fork-review-check/src/gomod.go | 163 ++++++++++++++- .../fork-review-check/src/gomod_test.go | 196 ++++++++++++++++++ .github/actions/fork-review-check/src/main.go | 25 ++- .../fork-review-check/src/main_test.go | 23 ++ 9 files changed, 517 insertions(+), 17 deletions(-) create mode 100644 .github/actions/fork-review-check/src/github_test.go diff --git a/.github/actions/fork-review-check/README.md b/.github/actions/fork-review-check/README.md index 20539d20..b7db9a37 100644 --- a/.github/actions/fork-review-check/README.md +++ b/.github/actions/fork-review-check/README.md @@ -9,7 +9,7 @@ This is build step 1 of the design: the CLI, run read-only. The workflow and rel For every fork commit S the build consumes, over the range from the trusted upstream base to S: 1. S is the approved final head, or the merge result, of a merged fork PR. -2. Every commit in the range belongs to a merged fork PR approved at its final head, after its last force push, by someone with write or higher permission on the fork who is neither the PR author nor the commit author. For the merge commit GitHub writes when a PR merges, the code author is the PR author, so the approver may also be the merger. +2. Every commit in the range must be an ancestor of an approved final head, that head itself, or a verified `web-flow` merge result of the merged PR. The approval must be after the last force push, by someone with write or higher permission on the fork who is neither the PR author nor the commit author. For a verified merge commit recorded as the PR's merge result with `web-flow` as committer, the code author is the PR author, so the approver may also be the merger. Rewritten commits from rebase merges that cannot be tied to the approved head fail closed. 3. With `-as-of`, every approval predates that time, normally the product release. 4. The check fails and names every commit that breaks a rule. @@ -38,6 +38,7 @@ GOPRIVATE=github.com/loft-sh/* go run . go -dir /path/to/vnode-internal | --- | --- | | `-repo`, `-ref` | `ref` mode: the fork and the tag or commit to check | | `-dir` | `go` mode: module directory, repeatable. S is the commit Go downloaded (`go mod download -json`, `Origin.Hash`), not today's tag | +| `-vendor-verifier` | `go` mode: trusted command that verifies the committed vendor tree in a temporary checkout. Required when a module has `vendor/` | | `-upstream` | `fork=upstream[@branch]`, used only while a fork has no `fork-upstream` property. The missing property still fails; the override only keeps the per-commit evidence coming | | `-as-of` | RFC 3339 time for rule 3 | | `-receipt` | Write the JSON receipt to this path | @@ -47,6 +48,10 @@ Exit codes: 0 pass or advisory, 1 a rule failed, 2 the check could not run. `go` mode needs `GOPRIVATE=github.com/loft-sh/*` so Go fetches loft-sh modules from git and reports the origin commit; without an origin the module fails. +When a module contains `vendor/`, the downloaded module graph alone does not verify the code compiled from that directory. The check requires `-vendor-verifier`, for example `-vendor-verifier 'bash hack/verify-vendor.sh'` for a repository that provides that verifier. It also requires a clean Git checkout, including no ignored files in `vendor/`. The command runs in a temporary copy of the repository and must fail when the committed vendor tree differs from the generated tree. It is trusted code, not a sandbox: run this check only with a trusted command and repository. A missing or failing verifier fails the check. The consumer is responsible for selecting a verifier that preserves its vendor symlinks and checks all relevant files. + +Workspace-level `vendor/` is not supported and fails closed. Use a module without workspace vendoring until a workspace verifier is supported. + ## Receipt The JSON receipt lists each input with its hash, each fork's `fork-upstream` value and source, the base, and every fork commit with its PR, approver, approval time and the approver's permission as read at check time. The permission endpoint returns current access only, so the receipt is the record and a later rerun can differ. diff --git a/.github/actions/fork-review-check/src/check.go b/.github/actions/fork-review-check/src/check.go index b9fd4c45..d25278da 100644 --- a/.github/actions/fork-review-check/src/check.go +++ b/.github/actions/fork-review-check/src/check.go @@ -244,6 +244,11 @@ func (c *checker) checkCommit(ctx context.Context, repo string, ci commitInfo) c } for _, pr := range pulls { + covered, reason := c.commitCoveredByHead(ctx, repo, ci, pr) + if !covered { + res.Failures = append(res.Failures, reason) + continue + } approval, reasons := c.qualifyingApproval(ctx, repo, pr, effectiveAuthor(ci, pr)) if approval == nil { res.Failures = append(res.Failures, reasons...) @@ -255,6 +260,31 @@ func (c *checker) checkCommit(ctx context.Context, repo string, ci commitInfo) c return res } +// commitCoveredByHead proves that an associated commit is in the approved +// final head, or is the exact verified GitHub merge result for that PR. +func (c *checker) commitCoveredByHead(ctx context.Context, repo string, ci commitInfo, pr pullInfo) (bool, string) { + if ci.SHA != "" && ci.SHA == pr.HeadSHA { + return true, "" + } + if ci.SHA != "" && ci.SHA == pr.MergeCommit { + if isGitHubMergeResult(ci, pr) { + return true, "" + } + return false, fmt.Sprintf("#%d: merge result lacks a verified signature or web-flow committer", pr.Number) + } + if ci.SHA == "" || pr.HeadSHA == "" { + return false, fmt.Sprintf("#%d: cannot verify commit ancestry without a commit and final head", pr.Number) + } + cmp, err := c.api.Compare(ctx, repo, ci.SHA, pr.HeadSHA) + if err != nil { + return false, fmt.Sprintf("#%d: compare commit to final head: %v", pr.Number, err) + } + if (cmp.Status != "ahead" && cmp.Status != "identical") || !strings.EqualFold(cmp.MergeBase, ci.SHA) { + return false, fmt.Sprintf("#%d: commit is not an ancestor of the final head (compare status %s, merge base %s)", pr.Number, cmp.Status, short(cmp.MergeBase)) + } + return true, "" +} + // checkReviewedTree applies rule 1 to S: it is the final head or the merge // result of a merged fork pull request that carries a qualifying approval. func (c *checker) checkReviewedTree(ctx context.Context, repo string, ci commitInfo) *reviewedTree { @@ -269,7 +299,11 @@ func (c *checker) checkReviewedTree(ctx context.Context, repo string, ci commitI case pr.HeadSHA: via = "head" case pr.MergeCommit: - via = "merge" + if isGitHubMergeResult(ci, pr) { + via = "merge" + } else { + continue + } default: continue } @@ -401,12 +435,16 @@ func commitOf(commits []commitInfo, sha string) commitInfo { // is the pull request author. Any other commit keeps its git author, merge // commits pushed by hand included. func effectiveAuthor(ci commitInfo, pr pullInfo) string { - if ci.Merge && ci.Committer == githubMergeCommitter && ci.SHA == pr.MergeCommit { + if ci.Merge && isGitHubMergeResult(ci, pr) { return pr.Author } return ci.Author } +func isGitHubMergeResult(ci commitInfo, pr pullInfo) bool { + return ci.SHA != "" && ci.SHA == pr.MergeCommit && ci.Committer == githubMergeCommitter && ci.Verified +} + func short(sha string) string { if len(sha) > 12 { return sha[:12] diff --git a/.github/actions/fork-review-check/src/check_test.go b/.github/actions/fork-review-check/src/check_test.go index fd9e2e88..61ff7c5c 100644 --- a/.github/actions/fork-review-check/src/check_test.go +++ b/.github/actions/fork-review-check/src/check_test.go @@ -108,9 +108,10 @@ func reviewedFork() *fakeAPI { Commits: []commitInfo{ {SHA: c1, Author: "alice", Committer: "alice", Subject: "add kerberos"}, {SHA: head, Author: "alice", Committer: "alice", Subject: "fix review"}, - {SHA: merge, Author: "bob", Committer: githubMergeCommitter, Subject: "Merge pull request #22", Merge: true}, + {SHA: merge, Author: "bob", Committer: githubMergeCommitter, Verified: true, Subject: "Merge pull request #22", Merge: true}, }, } + f.compares[fork+"@"+c1+"..."+head] = comparison{Status: "ahead", MergeBase: c1} pr22 := pullInfo{Number: 22, URL: "https://github.com/loft-sh/kine/pull/22", Author: "alice", BaseRepo: fork, HeadSHA: head, MergeCommit: merge, MergedAt: t0.Add(2 * time.Hour)} for _, sha := range []string{c1, head, merge} { f.pulls[fork+"@"+sha] = []pullInfo{pr22} @@ -168,6 +169,46 @@ func TestDirectPushFails(t *testing.T) { } } +func TestAssociatedCommitRemovedFromFinalHeadFails(t *testing.T) { + f := reviewedFork() + f.compares[fork+"@"+c1+"..."+head] = comparison{Status: "diverged"} + r := runCheck(f, tagPin()) + if r.Commits[0].Pass { + t.Fatal("a commit associated with the PR but absent from its final head must fail") + } +} + +func TestFinalHeadAncestryLookupErrorFailsClosed(t *testing.T) { + f := reviewedFork() + delete(f.compares, fork+"@"+c1+"..."+head) + r := runCheck(f, tagPin()) + if r.Commits[0].Pass { + t.Fatal("a failed ancestry lookup must not establish that the final head includes the commit") + } +} + +func TestFinalHeadComparisonMustNameCommitAsMergeBase(t *testing.T) { + f := reviewedFork() + f.compares[fork+"@"+c1+"..."+head] = comparison{Status: "ahead", MergeBase: baseSHA} + r := runCheck(f, tagPin()) + if r.Commits[0].Pass { + t.Fatal("a comparison whose merge base differs from the commit must fail") + } +} + +func TestUnsignedWebFlowMergeFails(t *testing.T) { + f := reviewedFork() + f.compares[fork+"@"+baseSHA+"..."+merge].Commits[2].Verified = false + r := runCheck(f, tagPin()) + if r.Commits[2].Pass || r.ReviewedTree.Pass { + t.Fatal("an unsigned merge result must not be treated as a GitHub-generated merge") + } + ci := f.compares[fork+"@"+baseSHA+"..."+merge].Commits[2] + if author := effectiveAuthor(ci, f.pulls[fork+"@"+merge][0]); author != "bob" { + t.Fatalf("unsigned merge author = %q, want git author bob", author) + } +} + func TestUnmergedPullDoesNotCount(t *testing.T) { f := reviewedFork() open := pullInfo{Number: 30, Author: "alice", BaseRepo: fork, HeadSHA: c1} diff --git a/.github/actions/fork-review-check/src/github.go b/.github/actions/fork-review-check/src/github.go index b2fb5246..d2a8f502 100644 --- a/.github/actions/fork-review-check/src/github.go +++ b/.github/actions/fork-review-check/src/github.go @@ -26,6 +26,7 @@ type commitInfo struct { SHA string Author string // GitHub login of the git author; empty when the email maps to no account Committer string // GitHub login of the committer + Verified bool // GitHub's verification result for the commit signature Subject string Merge bool } @@ -151,6 +152,7 @@ func (a githubAPI) Compare(ctx context.Context, repo, base, head string) (compar SHA: rc.GetSHA(), Author: rc.GetAuthor().GetLogin(), Committer: rc.GetCommitter().GetLogin(), + Verified: rc.GetCommit().GetVerification().GetVerified(), Subject: firstLine(rc.GetCommit().GetMessage()), Merge: len(rc.Parents) > 1, }) diff --git a/.github/actions/fork-review-check/src/github_test.go b/.github/actions/fork-review-check/src/github_test.go new file mode 100644 index 00000000..fd068f16 --- /dev/null +++ b/.github/actions/fork-review-check/src/github_test.go @@ -0,0 +1,33 @@ +package main + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "testing" + + "github.com/google/go-github/v88/github" +) + +func TestCompareReadsCommitSignatureVerification(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + fmt.Fprint(w, `{"status":"ahead","total_commits":3,"commits":[ + {"sha":"signed","commit":{"verification":{"verified":true}}}, + {"sha":"unsigned","commit":{"verification":{"verified":false}}}, + {"sha":"missing","commit":{}} + ]}`) + })) + defer server.Close() + client, err := github.NewClient(github.WithEnterpriseURLs(server.URL, server.URL)) + if err != nil { + t.Fatal(err) + } + cmp, err := (githubAPI{gh: client}).Compare(context.Background(), "owner/repo", "base", "head") + if err != nil { + t.Fatal(err) + } + if len(cmp.Commits) != 3 || !cmp.Commits[0].Verified || cmp.Commits[1].Verified || cmp.Commits[2].Verified { + t.Fatalf("unexpected verification states: %+v", cmp.Commits) + } +} diff --git a/.github/actions/fork-review-check/src/gomod.go b/.github/actions/fork-review-check/src/gomod.go index ce019c75..2611e247 100644 --- a/.github/actions/fork-review-check/src/gomod.go +++ b/.github/actions/fork-review-check/src/gomod.go @@ -140,15 +140,170 @@ func repoFromOrigin(url, path string) string { return parts[0] + "/" + parts[1] } +// verifyVendor checks the committed vendor tree using a caller-provided +// verifier in a temporary clone. The verifier is trusted code, not sandboxed. +func verifyVendor(dir, verifier string) error { + // Workspace vendoring belongs to the workspace root, not an individual + // module. Do not certify it with a module-scoped verifier. + if st, err := os.Stat(dir); err == nil && st.IsDir() { + out, err := runGo(dir, "env", "GOWORK") + if err != nil { + return fmt.Errorf("detect Go workspace: %w", err) + } + workspace := strings.TrimSpace(string(out)) + if workspace != "" && workspace != "off" { + path := filepath.Join(filepath.Dir(workspace), "vendor") + if _, err := os.Lstat(path); err == nil { + return fmt.Errorf("workspace vendor tree %s requires workspace verification, which is not supported", path) + } else if !errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("inspect workspace vendor tree: %w", err) + } + } + } else if err != nil && !errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("inspect module directory: %w", err) + } + vendor := filepath.Join(dir, "vendor") + info, err := os.Lstat(vendor) + if errors.Is(err, os.ErrNotExist) { + return nil + } + if err != nil { + return fmt.Errorf("inspect vendor tree: %w", err) + } + if !info.IsDir() { + return fmt.Errorf("vendor path %s is not a directory", vendor) + } + if strings.TrimSpace(verifier) == "" { + return fmt.Errorf("%s exists: configure a trusted vendor verifier", vendor) + } + module, err := filepath.EvalSymlinks(dir) + if err != nil { + return fmt.Errorf("resolve module directory: %w", err) + } + module, err = filepath.Abs(module) + if err != nil { + return err + } + runGit := func(at string, args ...string) ([]byte, error) { + cmd := exec.Command("git", args...) + cmd.Dir = at + out, err := cmd.CombinedOutput() + if err != nil { + return nil, fmt.Errorf("git %s: %w: %s", strings.Join(args, " "), err, strings.TrimSpace(string(out))) + } + return out, nil + } + out, err := runGit(module, "rev-parse", "--show-toplevel") + if err != nil { + return fmt.Errorf("vendor verification needs a Git checkout: %w", err) + } + root, err := filepath.EvalSymlinks(strings.TrimSpace(string(out))) + if err != nil { + return err + } + rel, err := filepath.Rel(root, module) + if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return fmt.Errorf("module directory is outside its Git checkout") + } + // A clean checkout binds the clone to the source the build will consume. + // Include ignored vendor files: Git's normal status hides them, but Go can + // still compile them. Do not follow symlinks outside the checkout. + clean := func() error { + status, err := runGit(root, "status", "--porcelain", "--untracked-files=all") + if err != nil { + return err + } + if len(status) != 0 { + return fmt.Errorf("vendor verification requires a clean checkout") + } + status, err = runGit(root, "status", "--porcelain", "--untracked-files=all", "--ignored", "--", filepath.Join(rel, "vendor")) + if err != nil { + return err + } + if len(status) != 0 { + return fmt.Errorf("vendor tree contains uncommitted or ignored files") + } + return nil + } + if err := clean(); err != nil { + return err + } + err = filepath.WalkDir(filepath.Join(module, "vendor"), func(path string, entry os.DirEntry, walkErr error) error { + if walkErr != nil { + return walkErr + } + if entry.Type()&os.ModeSymlink == 0 { + return nil + } + target, err := filepath.EvalSymlinks(path) + if err != nil { + return fmt.Errorf("resolve vendor symlink %s: %w", path, err) + } + r, err := filepath.Rel(root, target) + if err != nil || r == ".." || strings.HasPrefix(r, ".."+string(filepath.Separator)) { + return fmt.Errorf("vendor symlink %s points outside the checkout", path) + } + status, err := runGit(root, "status", "--porcelain", "--untracked-files=all", "--ignored", "--", r) + if err != nil { + return err + } + if len(status) != 0 { + return fmt.Errorf("vendor symlink target %s contains uncommitted or ignored files", path) + } + return nil + }) + if err != nil { + return err + } + head, err := runGit(root, "rev-parse", "--verify", "HEAD") + if err != nil { + return err + } + tmp, err := os.MkdirTemp("", "fork-review-vendor-*") + if err != nil { + return err + } + defer os.RemoveAll(tmp) + clone := filepath.Join(tmp, "checkout") + if _, err := runGit(root, "-c", "core.hooksPath=/dev/null", "clone", "--no-hardlinks", "--no-checkout", "--", root, clone); err != nil { + return err + } + if _, err := runGit(clone, "-c", "core.hooksPath=/dev/null", "checkout", "--detach", strings.TrimSpace(string(head))); err != nil { + return err + } + if err := clean(); err != nil { + return err + } + current, err := runGit(root, "rev-parse", "--verify", "HEAD") + if err != nil { + return err + } + if !bytes.Equal(head, current) { + return fmt.Errorf("checkout HEAD changed during vendor verification") + } + cmd := exec.Command("sh", "-c", verifier) + cmd.Dir = filepath.Join(clone, rel) + output, err := cmd.CombinedOutput() + if err != nil { + return fmt.Errorf("vendor verifier failed: %w: %s", err, strings.TrimSpace(string(output))) + } + return nil +} + type execGo struct{} func (execGo) ListModules(dir string) ([]goModule, error) { args := []string{"list", "-m", "-json"} // With a vendor directory and no workspace, go list cannot compute "all" - // from vendor/modules.txt. Go already checks that file against go.mod, so - // reading the graph from go.mod yields the same versions. - if exists(filepath.Join(dir, "vendor")) && os.Getenv("GOWORK") != "off" && !exists(filepath.Join(dir, "go.work")) { - args = append(args, "-mod=mod") + // from vendor/modules.txt. The vendor verifier checks that source separately. + if exists(filepath.Join(dir, "vendor")) { + work, err := runGo(dir, "env", "GOWORK") + if err != nil { + return nil, fmt.Errorf("detect Go workspace: %w", err) + } + if strings.TrimSpace(string(work)) == "" || strings.TrimSpace(string(work)) == "off" { + args = append(args, "-mod=mod") + } } args = append(args, "all") out, err := runGo(dir, args...) diff --git a/.github/actions/fork-review-check/src/gomod_test.go b/.github/actions/fork-review-check/src/gomod_test.go index ff67139d..7b346d8b 100644 --- a/.github/actions/fork-review-check/src/gomod_test.go +++ b/.github/actions/fork-review-check/src/gomod_test.go @@ -4,11 +4,207 @@ import ( "context" "encoding/json" "fmt" + "os" + "os/exec" + "path/filepath" "strings" "testing" "time" ) +func vendorFixture(t *testing.T) string { + t.Helper() + dir := t.TempDir() + run := func(args ...string) { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v: %s", args, err, out) + } + } + run("init", "-q") + run("config", "user.name", "Test") + run("config", "user.email", "test@example.com") + if err := os.MkdirAll(filepath.Join(dir, "vendor"), 0755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "vendor", "source.go"), []byte("original"), 0644); err != nil { + t.Fatal(err) + } + if err := os.Symlink("source.go", filepath.Join(dir, "vendor", "link")); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, ".gitignore"), []byte("vendor/ignored\n"), 0644); err != nil { + t.Fatal(err) + } + run("add", ".") + run("commit", "-qm", "fixture") + return dir +} + +func TestVerifyVendorRequiresVerifier(t *testing.T) { + dir := vendorFixture(t) + if err := verifyVendor(dir, ""); err == nil || !strings.Contains(err.Error(), "verifier") { + t.Fatalf("expected missing verifier error, got %v", err) + } + if err := verifyVendor(t.TempDir(), ""); err != nil { + t.Fatal(err) + } +} + +func TestVerifyVendorRejectsBrokenVendorLink(t *testing.T) { + dir := t.TempDir() + if err := os.Symlink("missing", filepath.Join(dir, "vendor")); err != nil { + t.Fatal(err) + } + if err := verifyVendor(dir, "true"); err == nil { + t.Fatal("expected broken vendor link to fail") + } +} + +func TestVerifyVendorRunsInIsolatedCopy(t *testing.T) { + dir := vendorFixture(t) + command := "test -L vendor/link && test -d .git && test \"$(cat vendor/link)\" = original && printf changed > vendor/source.go" + if err := verifyVendor(dir, command); err != nil { + t.Fatal(err) + } + data, err := os.ReadFile(filepath.Join(dir, "vendor", "source.go")) + if err != nil || string(data) != "original" { + t.Fatalf("original changed: %q, %v", data, err) + } + if err := verifyVendor(dir, "exit 7"); err == nil { + t.Fatal("expected verifier failure") + } +} + +func TestVerifyVendorRejectsDirtyAndIgnoredVendor(t *testing.T) { + for _, file := range []string{"source.go", "untracked", "ignored"} { + t.Run(file, func(t *testing.T) { + dir := vendorFixture(t) + if err := os.WriteFile(filepath.Join(dir, "vendor", file), []byte("changed"), 0644); err != nil { + t.Fatal(err) + } + if err := verifyVendor(dir, "true"); err == nil { + t.Fatal("expected dirty vendor failure") + } + }) + } +} + +func TestVerifyVendorRejectsSymlinkOutsideCheckout(t *testing.T) { + dir := vendorFixture(t) + if err := os.Symlink(t.TempDir(), filepath.Join(dir, "vendor", "outside")); err != nil { + t.Fatal(err) + } + cmd := exec.Command("git", "add", "vendor/outside") + cmd.Dir = dir + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git add: %v: %s", err, out) + } + cmd = exec.Command("git", "commit", "-qm", "outside link") + cmd.Dir = dir + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git commit: %v: %s", err, out) + } + if err := verifyVendor(dir, "true"); err == nil || !strings.Contains(err.Error(), "outside") { + t.Fatalf("expected outside symlink error, got %v", err) + } +} + +func TestVerifyVendorRejectsIgnoredSymlinkTargetContent(t *testing.T) { + dir := vendorFixture(t) + if err := os.MkdirAll(filepath.Join(dir, "staging"), 0755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "staging", "tracked"), []byte("tracked"), 0644); err != nil { + t.Fatal(err) + } + if err := os.Symlink("../staging", filepath.Join(dir, "vendor", "staging")); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, ".gitignore"), []byte("staging/ignored\n"), 0644); err != nil { + t.Fatal(err) + } + cmd := exec.Command("git", "add", "-A") + cmd.Dir = dir + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git add: %v: %s", err, out) + } + cmd = exec.Command("git", "commit", "-qm", "staging link") + cmd.Dir = dir + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git commit: %v: %s", err, out) + } + if err := os.WriteFile(filepath.Join(dir, "staging", "ignored"), []byte("uncommitted"), 0644); err != nil { + t.Fatal(err) + } + if err := verifyVendor(dir, "true"); err == nil { + t.Fatal("expected ignored target content to fail") + } +} + +func TestVerifyVendorRunsInModuleDirectory(t *testing.T) { + root := vendorFixture(t) + module := filepath.Join(root, "sub") + if err := os.MkdirAll(module, 0755); err != nil { + t.Fatal(err) + } + if err := os.Rename(filepath.Join(root, "vendor"), filepath.Join(module, "vendor")); err != nil { + t.Fatal(err) + } + cmd := exec.Command("git", "add", "-A") + cmd.Dir = root + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git add: %v: %s", err, out) + } + cmd = exec.Command("git", "commit", "-qm", "nested module") + cmd.Dir = root + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git commit: %v: %s", err, out) + } + if err := verifyVendor(module, "test -L vendor/link && test ! -d .git"); err != nil { + t.Fatal(err) + } +} + +func TestVerifyVendorRejectsWorkspaceVendor(t *testing.T) { + root := t.TempDir() + module := filepath.Join(root, "sub") + if err := os.MkdirAll(module, 0755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Join(root, "vendor"), 0755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(root, "go.work"), []byte("go 1.24\n\nuse ./sub\n"), 0644); err != nil { + t.Fatal(err) + } + t.Setenv("GOWORK", "auto") + if err := verifyVendor(module, "true"); err == nil || !strings.Contains(err.Error(), "workspace") { + t.Fatalf("expected workspace vendor error, got %v", err) + } +} + +func TestListModulesWithVendorAndWorkspaceDisabled(t *testing.T) { + dir := t.TempDir() + if err := os.MkdirAll(filepath.Join(dir, "vendor"), 0755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "go.mod"), []byte("module example.com/test\n\ngo 1.24\n"), 0644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "vendor", "modules.txt"), nil, 0644); err != nil { + t.Fatal(err) + } + t.Setenv("GOWORK", "off") + t.Setenv("GOFLAGS", "-mod=vendor") + mods, err := (execGo{}).ListModules(dir) + if err != nil || len(mods) != 1 || mods[0].Path != "example.com/test" { + t.Fatalf("modules = %+v, err = %v", mods, err) + } +} + type fakeGo struct { mods []goModule downloads map[string]goDownload // path@version diff --git a/.github/actions/fork-review-check/src/main.go b/.github/actions/fork-review-check/src/main.go index b51465c0..40b4832b 100644 --- a/.github/actions/fork-review-check/src/main.go +++ b/.github/actions/fork-review-check/src/main.go @@ -13,7 +13,8 @@ // 3. With -as-of, every approval predates that time (the product release). // 4. It fails and names every commit that breaks a rule. // -// It reads GitHub and the go command only; it writes nothing but its receipt. +// It reads GitHub and the go command. For vendored modules, it runs a trusted +// verifier in a temporary checkout, and it can write a receipt. // // fork-review-check ref -repo loft-sh/kine -ref v0.16.5.vcluster.1 // fork-review-check go -dir . -dir ./sub @@ -62,14 +63,15 @@ func (m *multiFlag) String() string { return strings.Join(*m, ",") } func (m *multiFlag) Set(v string) error { *m = append(*m, v); return nil } type options struct { - mode string - repo string - ref string - dirs multiFlag - upstreams multiFlag - asOf string - receipt string - advisory bool + mode string + repo string + ref string + dirs multiFlag + vendorVerifier string + upstreams multiFlag + asOf string + receipt string + advisory bool } func main() { @@ -134,6 +136,7 @@ func parseArgs(args []string) (options, error) { fs.StringVar(&opts.repo, "repo", "", "ref mode: fork repository, owner/name") fs.StringVar(&opts.ref, "ref", "", "ref mode: fork release tag or commit SHA") fs.Var(&opts.dirs, "dir", "go mode: module directory to resolve (repeatable)") + fs.StringVar(&opts.vendorVerifier, "vendor-verifier", "", "go mode: trusted command to verify vendor in an isolated checkout") fs.Var(&opts.upstreams, "upstream", "fork=upstream[@branch] used when a fork has no fork-upstream property; in ref mode a bare upstream applies to -repo (repeatable)") fs.StringVar(&opts.asOf, "as-of", "", "RFC 3339 time every approval must predate (rule 3), normally the product release time") fs.StringVar(&opts.receipt, "receipt", "", "write the JSON receipt to this path") @@ -190,6 +193,10 @@ func check(ctx context.Context, api forkAPI, tool goTool, opts options, now time } case "go": for _, dir := range opts.dirs { + if err := verifyVendor(dir, opts.vendorVerifier); err != nil { + rc.Failures = append(rc.Failures, fmt.Sprintf("%s: verify vendor: %v", dir, err)) + continue + } inputs, failures, err := goForkInputs(tool, dir) if err != nil { return rc, err diff --git a/.github/actions/fork-review-check/src/main_test.go b/.github/actions/fork-review-check/src/main_test.go index edd2656d..89303a7c 100644 --- a/.github/actions/fork-review-check/src/main_test.go +++ b/.github/actions/fork-review-check/src/main_test.go @@ -2,6 +2,8 @@ package main import ( "context" + "os" + "path/filepath" "strings" "testing" "time" @@ -67,6 +69,27 @@ func TestParseArgs(t *testing.T) { } } +func TestGoModeRequiresVendorVerification(t *testing.T) { + dir := t.TempDir() + if err := os.Mkdir(filepath.Join(dir, "vendor"), 0o755); err != nil { + t.Fatal(err) + } + rc, err := check(context.Background(), newFake(), vnodeGraph(), options{mode: "go", dirs: multiFlag{dir}}, time.Now()) + if err != nil { + t.Fatal(err) + } + if rc.Pass || !strings.Contains(strings.Join(rc.Failures, " "), "vendor") { + t.Fatalf("unverified vendor tree must fail: %+v", rc) + } +} + +func TestParseVendorVerifier(t *testing.T) { + o, err := parseArgs([]string{"go", "-vendor-verifier", "bash hack/verify-vendor.sh"}) + if err != nil || o.vendorVerifier != "bash hack/verify-vendor.sh" { + t.Fatalf("vendor verifier = %q, error %v", o.vendorVerifier, err) + } +} + func TestParseUpstreams(t *testing.T) { got, err := parseUpstreams([]string{"k3s-io/kine", "loft-sh/fuse=nestybox/fuse@master"}, fork) if err != nil {