From d5aa07b4b88d8d4924c9f8d91ea2083bd6c0698c Mon Sep 17 00:00:00 2001 From: Danil Silantyev Date: Sat, 15 Aug 2026 05:13:09 +0500 Subject: [PATCH 1/2] refactor: delete what buys nothing, and give the trust path one definition MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three statements in this repository cost maintenance and bought no enforcement. upstream_go_mod was required to be non-empty and read by nothing — not fetched, not hashed, not parsed, not compared. Either URL could have been replaced with arbitrary text and every check stayed green. It is deleted rather than made executable, because nothing needs the fact it was gesturing at. The git executable was a constant in signatureverify and a raw literal in provenance, and the isolated git environment was spelled out twice. One fact in two places, in the code path where drift matters most: which file the name git refers to decides what a signature verdict is worth. internal/trustedexec is now the single definition. It searches a fixed, ordered list of absolute directories and never reads PATH, because whoever can set an environment variable must not be able to choose the program that rules on a signature. The result must be a regular, non-symlink file that is not group or world writable. Children of the trust path get exactly those directories as their PATH and nothing they inherited. A Homebrew macOS host can now verify, which the single hardcoded path did not allow. The cost is stated rather than hidden: a host keeping its tools outside those directories — Nix and Guix in practice — cannot verify at all, because every mechanism that would cover them is an ambient input into the one code path that must not have any. Failure names what was searched instead of surfacing as an obscure exec error. The GDS anchor said the module carried no semver tag, so the consumer contract was the pinned commit. That stopped being true at v0.1.2. It now declares semver and version-tag, and says why those are two axes rather than one: what this repository promises a consumer, and how this estate chooses to take it. --- .../goals/no-unenforced-ceremony.json | 139 ++++++++++++++++++ .../goals/self-verified-journals.json | 66 ++++++++- .gds/repository.yaml | 11 +- CHANGELOG.md | 22 +++ SECURITY.md | 17 +++ internal/cicontract/contract.go | 9 +- internal/provenance/verify.go | 10 +- internal/signatureverify/verify.go | 31 ++-- internal/signatureverify/verify_test.go | 10 +- internal/trustedexec/resolve.go | 105 +++++++++++++ internal/trustedexec/resolve_test.go | 93 ++++++++++++ security-tools.json | 6 +- 12 files changed, 483 insertions(+), 36 deletions(-) create mode 100644 .agent-runtime/goals/no-unenforced-ceremony.json create mode 100644 internal/trustedexec/resolve.go create mode 100644 internal/trustedexec/resolve_test.go diff --git a/.agent-runtime/goals/no-unenforced-ceremony.json b/.agent-runtime/goals/no-unenforced-ceremony.json new file mode 100644 index 0000000..cd20d87 --- /dev/null +++ b/.agent-runtime/goals/no-unenforced-ceremony.json @@ -0,0 +1,139 @@ +{ + "schema_version": "v1alpha1", + "revision": 11, + "goal": { + "id": "no-unenforced-ceremony", + "intent": "Every retained contract field buys enforcement and trust tooling resolves without ambient paths", + "acceptance": [ + { + "id": "no-dead-field", + "acceptance": "The upstream_go_mod field is deleted or verified against real upstream content", + "status": "complete", + "evidence": [ + { + "type": "file", + "reference": "security-tools.json; internal/cicontract/contract.go", + "result": "The field is deleted from the contract file and its Go type; nothing referenced it." + } + ] + }, + { + "id": "resolved-binaries", + "acceptance": "Trust binaries are resolved and identity-checked rather than hardcoded, with one source for the fact", + "status": "complete", + "evidence": [ + { + "type": "file", + "reference": "internal/trustedexec/resolve.go", + "result": "One resolver, fixed absolute search, never PATH, regular non-symlink and not group or world writable, shared by signatureverify and provenance." + } + ] + }, + { + "id": "anchor-truth", + "acceptance": "The GDS anchor declares semver compatibility and version-tag pinning, matching how the control plane actually consumes the module", + "status": "complete", + "evidence": [ + { + "type": "file", + "reference": ".gds/repository.yaml", + "result": "compatibility semver and pin_policy version-tag, with the two axes explained and the false no-semver-tag justification removed." + } + ] + } + ], + "non_goals": [ + "Weaken any fail-closed signature or provenance check" + ], + "state": "active", + "current_phase": "closure", + "receipts": { + "completeness_omission_audit": { + "phase": "completeness_omission_audit", + "summary": "Nix and Guix hosts still cannot verify, because their tools are outside every absolute directory the resolver will search and any mechanism covering them is an ambient input into the trust path. Stated in the security model with the reason and the failure message that names what was searched.", + "evidence": [ + { + "type": "test", + "reference": "internal/trustedexec TestSearchNamesWhatItLookedAt", + "result": "A missing tool fails naming every directory searched and stating that PATH is never read." + } + ], + "recorded_at": "2026-08-15T00:12:44.466737374Z" + }, + "execute": { + "phase": "execute", + "summary": "Removed upstream_go_mod from the contract file and its type, routed signatureverify and provenance through trustedexec, and set the anchor to semver and version-tag.", + "evidence": [ + { + "type": "command", + "reference": "grep for /usr/bin across internal/signatureverify and internal/provenance", + "result": "No occurrences outside the resolver; the isolated environment has one definition." + } + ], + "recorded_at": "2026-08-15T00:12:44.344133143Z" + }, + "gap_plan": { + "phase": "gap_plan", + "summary": "Delete the field rather than make it executable, because nothing needs it. Give the trust tools one resolver that never reads PATH. State both axes in the anchor: what the module promises and how this estate takes it.", + "evidence": [ + { + "type": "file", + "reference": "internal/trustedexec", + "result": "A fixed ordered search over absolute directories keeps the property the hardcoded path had, and covers a Homebrew macOS host that the single path did not." + } + ], + "recorded_at": "2026-08-15T00:12:44.308001589Z" + }, + "orient": { + "phase": "orient", + "summary": "Three statements bought nothing. upstream_go_mod was required non-empty and read by nothing. The git executable was written as a constant in signatureverify and as a raw literal in provenance, with the isolated environment spelled out twice. The GDS anchor justified commit-contract pinning with a comment saying no semver tag existed, which stopped being true at v0.1.2.", + "evidence": [ + { + "type": "file", + "reference": "security-tools.json; internal/provenance/verify.go; .gds/repository.yaml", + "result": "Either upstream URL could be replaced with arbitrary text with every check still green; one fact about the trust path lived in two places; the anchor contradicted four live tags and two published releases." + } + ], + "recorded_at": "2026-08-15T00:12:44.266651931Z" + }, + "reconcile": { + "phase": "reconcile", + "summary": "Composing the two environments naively duplicated every git configuration setting, so the shared half now lives in trustedexec and the signature verifier adds only what is specific to running git interactively.", + "evidence": [ + { + "type": "test", + "reference": "internal/signatureverify TestIsolatedGitEnvironmentRejectsAmbientOverrideChannels", + "result": "The isolated environment is asserted against the resolver's own search path rather than a hardcoded string, so the two cannot drift." + } + ], + "recorded_at": "2026-08-15T00:12:44.383787796Z" + }, + "self_review": { + "phase": "self_review", + "summary": "The resolver keeps the property that made the hardcoded path defensible in the first place: no environment input decides which program rules on a signature. It is tested for that directly rather than assumed.", + "evidence": [ + { + "type": "test", + "reference": "internal/trustedexec TestNeverReadsThePathEnvironment", + "result": "With PATH pointing at a planted git, resolution still returns the fixed location." + } + ], + "recorded_at": "2026-08-15T00:12:44.423642378Z" + }, + "verify": { + "phase": "verify", + "summary": "gofmt, go vet, staticcheck, full tests under umask 002, race, and all seven contract checkers.", + "evidence": [ + { + "type": "test", + "reference": "local gate transcript 2026-08-15", + "result": "All green." + } + ], + "recorded_at": "2026-08-15T00:12:44.50916971Z" + } + }, + "created_at": "2026-08-15T00:12:44.224755101Z", + "updated_at": "2026-08-15T00:12:44.624414366Z" + } +} diff --git a/.agent-runtime/goals/self-verified-journals.json b/.agent-runtime/goals/self-verified-journals.json index 46ef193..fa8342c 100644 --- a/.agent-runtime/goals/self-verified-journals.json +++ b/.agent-runtime/goals/self-verified-journals.json @@ -1,6 +1,6 @@ { "schema_version": "v1alpha1", - "revision": 8, + "revision": 13, "goal": { "id": "self-verified-journals", "intent": "This repository's tracked Goal journals are proven valid by an executable checker", @@ -8,30 +8,82 @@ { "id": "journal-checker", "acceptance": "A required CI checker validates every tracked journal against the Go contract and the published schema", - "status": "pending" + "status": "complete", + "evidence": [ + { + "type": "file", + "reference": "cmd/check-goal-journals; internal/journalverify; .github/workflows/ci.yml; .gds/repository.yaml", + "result": "A required CI step holds every tracked journal to the Go contract and the published schema, and the CI contract requires the step so it cannot be dropped silently." + } + ] }, { "id": "journals-valid", "acceptance": "goal status succeeds on every tracked journal", - "status": "pending" + "status": "complete", + "evidence": [ + { + "type": "command", + "reference": "go run ./cmd/check-goal-journals", + "result": "goal journals valid: 9 tracked, accepted by the Goal contract and the published schema." + } + ] }, { "id": "first-release-true", "acceptance": "The first-release journal states the published outcome and closes on its evidence", - "status": "pending" + "status": "complete", + "evidence": [ + { + "type": "file", + "reference": ".agent-runtime/goals/first-v0-release.json", + "result": "State completed at closure with all seven criteria met, recording that the release shipped as v0.1.2 then v0.1.3, and keeping the unpublished v0.1.0 and v0.1.1 tags as declared debt." + } + ] }, { "id": "no-shipped-invalid", "acceptance": "A built source archive contains no artifact the product rejects", - "status": "pending" + "status": "complete", + "evidence": [ + { + "type": "command", + "reference": "source archive built from the commit, every journal extracted and fed to agent-runtime goal status", + "result": "All accepted; the same archive built from the previous commit still rejects first-v0-release.json." + } + ] } ], "non_goals": [ "Widen the phase vocabulary to accommodate a malformed journal" ], - "state": "active", + "state": "completed", "current_phase": "closure", "receipts": { + "closure": { + "phase": "closure", + "summary": "Merged as PR #58 with all four required checks green.", + "evidence": [ + { + "type": "link", + "reference": "https://github.com/NDDev-it-com/agent-runtime/pull/58", + "result": "Merged to main." + } + ], + "recorded_at": "2026-08-15T00:07:29.602650529Z", + "closure": { + "achieved_outcome": "The Goal contract has the executable checker every other contract here already had, and the module no longer ships an artifact it rejects.", + "cleanup": "Task branch deleted locally and on origin.", + "remaining": [], + "next_work": [ + { + "type": "issue", + "reference": "no unenforced ceremony", + "result": "Delete the contract field nothing verifies, give the trust binaries one definition, and make the GDS anchor state how the module is actually consumed." + } + ] + } + }, "completeness_omission_audit": { "phase": "completeness_omission_audit", "summary": "Registered the checker in the CI contract as well as the workflow, so it cannot be dropped from ci.yml while the contract stays green -- the same failure class the workflow model was built to close.", @@ -118,6 +170,6 @@ } }, "created_at": "2026-08-15T00:02:27.292800011Z", - "updated_at": "2026-08-15T00:02:27.487402016Z" + "updated_at": "2026-08-15T00:07:29.602650529Z" } } diff --git a/.gds/repository.yaml b/.gds/repository.yaml index f6bc372..05f1305 100644 --- a/.gds/repository.yaml +++ b/.gds/repository.yaml @@ -96,10 +96,13 @@ module: # Pinned as a gitlink by the control plane. Not vendored, not published to # a package registry, and not executed from its own repository. - "git-submodule" - # The published surface is `v1alpha1` and carries no semver tag, so the - # consumer contract is the exact pinned commit until a tagged line exists. - compatibility: "commit-contract" - pin_policy: "default-branch-commit" + # The module publishes semver releases with attested assets, and the control + # plane pins by release tag. Both halves are stated because they are separate + # axes: the first is what this repository promises a consumer, the second is + # how this estate chooses to take it. The earlier reading — no semver tag, so + # the contract is the pinned commit — stopped being true at v0.1.2. + compatibility: "semver" + pin_policy: "version-tag" publication: registry: "none" github_release: "required" diff --git a/CHANGELOG.md b/CHANGELOG.md index 51e16c2..6390202 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,14 @@ contract. ## [Unreleased] +### Removed + +- `upstream_go_mod` is gone from `security-tools.json` and its Go type. It was + required to be non-empty and nothing read, fetched, hashed, parsed or compared + it, so it bought no evidence while creating another value to keep in step by + hand. Either URL could have been replaced with arbitrary text and every check + stayed green. + ### Added - `cmd/check-goal-journals` holds every tracked Goal journal to the Go contract @@ -19,6 +27,20 @@ contract. ### Fixed +- `git` and `ssh-keygen` have one definition. `internal/provenance` repeated + `/usr/bin/git` as a raw literal beside `internal/signatureverify`'s constant, + and the isolated environment was written out twice — one fact in two places + that could drift apart in the code path where drift matters most. Both now + resolve through `internal/trustedexec`, which searches a fixed ordered list of + absolute directories, never `PATH`, and requires the result to be a regular, + non-symlink file that is not group or world writable. This also lets a + Homebrew macOS host verify, which the single hardcoded path did not. +- `.gds/repository.yaml` states how the module is actually consumed. Its module + block declared `commit-contract` and `default-branch-commit`, justified by a + comment saying no semver tag existed — which stopped being true at `v0.1.2`. + It now declares `semver` and `version-tag`, and says why those are separate + axes: what this repository promises a consumer, and how this estate takes it. + - `.agent-runtime/goals/first-v0-release.json` is a journal this module accepts. It recorded each recovery cycle under an invented receipt key, which the Goal contract does not permit, so `agent-runtime goal status` refused to load it — diff --git a/SECURITY.md b/SECURITY.md index c816b7f..7590fa8 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -60,6 +60,23 @@ Avoid placing secrets in manifests, instructions, command arguments, logs, or issue reports. Captured command output may contain sensitive data and should be handled accordingly by callers. +### Executables the trust path runs + +Signature and provenance verification shell out to `git` and `ssh-keygen`, so +which file those names refer to decides what a verdict is worth. They are never +resolved through `PATH`: the runtime searches a fixed, ordered list of absolute +directories — `/usr/bin`, `/bin`, `/usr/local/bin`, `/opt/homebrew/bin` — and +requires the result to be a regular, non-symlink file that is not group or +world writable. Children of the trust path are given exactly those directories +as their `PATH` and nothing they inherited. + +The cost is real and deliberate: a host that keeps its tools outside those +directories, which in practice means Nix and Guix, cannot run verification at +all. Every mechanism that would cover them — an environment override, a `PATH` +search — is an ambient input into the one code path that must not have any. +Verification fails with a message naming what was searched rather than an +obscure exec error. + ## Release integrity Official releases originate only from annotated signed `vMAJOR.MINOR.PATCH` diff --git a/internal/cicontract/contract.go b/internal/cicontract/contract.go index f15bc69..367e6ec 100644 --- a/internal/cicontract/contract.go +++ b/internal/cicontract/contract.go @@ -24,10 +24,9 @@ type Contract struct { CompatibilityGo string `json:"compatibility_go"` } type Tool struct { - Module string `json:"module"` - Version string `json:"version"` - MinimumGo string `json:"minimum_go"` - UpstreamGoMod string `json:"upstream_go_mod"` + Module string `json:"module"` + Version string `json:"version"` + MinimumGo string `json:"minimum_go"` } func Load(path string) (Contract, error) { @@ -45,7 +44,7 @@ func Load(path string) (Contract, error) { return Contract{}, errors.New("contract has missing or unsupported fields") } for name, tool := range map[string]Tool{"govulncheck": c.Govulncheck, "staticcheck": c.Staticcheck} { - if tool.Module == "" || tool.Version == "" || tool.MinimumGo == "" || tool.UpstreamGoMod == "" { + if tool.Module == "" || tool.Version == "" || tool.MinimumGo == "" { return Contract{}, fmt.Errorf("contract tool %q has missing or unsupported fields", name) } } diff --git a/internal/provenance/verify.go b/internal/provenance/verify.go index 20eb79a..a5b8e2b 100644 --- a/internal/provenance/verify.go +++ b/internal/provenance/verify.go @@ -25,6 +25,8 @@ import ( "github.com/ProtonMail/go-crypto/openpgp" "github.com/NDDev-it-com/agent-runtime/internal/signatureverify" + + "github.com/NDDev-it-com/agent-runtime/internal/trustedexec" ) const maxAPIBytes = 8 << 20 @@ -564,9 +566,13 @@ func actionRunID(details string) (int64, error) { } func readSignedCommit(ctx context.Context, root, sha string) ([]byte, []byte, error) { - command := exec.CommandContext(ctx, "/usr/bin/git", "cat-file", "commit", sha) + git, err := trustedexec.Git() + if err != nil { + return nil, nil, err + } + command := exec.CommandContext(ctx, git, "cat-file", "commit", sha) command.Dir = root - command.Env = []string{"PATH=/usr/bin:/bin", "GIT_CONFIG_NOSYSTEM=1", "GIT_CONFIG_GLOBAL=" + os.DevNull, "GIT_CONFIG_COUNT=0", "GIT_NO_REPLACE_OBJECTS=1"} + command.Env = trustedexec.Environment raw, err := command.Output() if err != nil { return nil, nil, fmt.Errorf("read integration commit object: %w", err) diff --git a/internal/signatureverify/verify.go b/internal/signatureverify/verify.go index 7332b5d..f1f5e46 100644 --- a/internal/signatureverify/verify.go +++ b/internal/signatureverify/verify.go @@ -16,12 +16,12 @@ import ( "path/filepath" "regexp" "strings" + + "github.com/NDDev-it-com/agent-runtime/internal/trustedexec" ) const ( AllowedSignersPath = ".github/release-allowed-signers" - gitExecutable = "/usr/bin/git" - sshKeygenExecutable = "/usr/bin/ssh-keygen" maxAllowlistBytes = 16 << 10 CanonicalPrincipal = "danilsilantyevwork@gmail.com" CanonicalFingerprint = "SHA256:3L6V7EKdHGQyLcr4NPsFC86EYbi3/7f2X6kMni7LmNI" @@ -74,7 +74,7 @@ var canonicalTrustPolicy = trustPolicy{ principal: CanonicalPrincipal, keyType: "ED25519", fingerprint: CanonicalFingerprint, } -// runner executes git, and only git, at gitExecutable. It carries no +// runner executes git, and only git, at the path trustedexec resolves. It carries no // executable parameter so no call site can introduce a different binary into // the trust path. type runner interface { @@ -90,12 +90,16 @@ type verifyOptions struct { type execRunner struct{} func (execRunner) run(ctx context.Context, args []string, dir string, env []string) ([]byte, []byte, error) { - command := exec.CommandContext(ctx, gitExecutable, args...) + git, err := trustedexec.Git() + if err != nil { + return nil, nil, err + } + command := exec.CommandContext(ctx, git, args...) command.Dir = dir command.Env = env var stdout, stderr bytes.Buffer command.Stdout, command.Stderr = &stdout, &stderr - err := command.Run() + err = command.Run() return stdout.Bytes(), stderr.Bytes(), err } @@ -185,10 +189,14 @@ func verifyWithOptions(ctx context.Context, request Request, policy trustPolicy, if err := snapshot.revalidate(); err != nil { return Result{}, err } + sshKeygen, err := trustedexec.SSHKeygen() + if err != nil { + return Result{}, err + } verifyArgs := []string{ "-c", "gpg.format=ssh", "-c", "gpg.ssh.allowedSignersFile=" + snapshot.path, - "-c", "gpg.ssh.program=" + sshKeygenExecutable, + "-c", "gpg.ssh.program=" + sshKeygen, "-c", "gpg.ssh.revocationFile=" + os.DevNull, "-c", "gpg.minTrustLevel=fully", "verify-" + string(request.Kind), "--raw", request.ObjectSHA, @@ -364,10 +372,9 @@ func writeEvidence(writer io.Writer, data []byte, description string) error { } func isolatedGitEnvironment() []string { - environment := []string{ - "GIT_CONFIG_NOSYSTEM=1", "GIT_CONFIG_GLOBAL=" + os.DevNull, "GIT_CONFIG_COUNT=0", - "GIT_TERMINAL_PROMPT=0", "GIT_NO_REPLACE_OBJECTS=1", "GIT_PAGER=cat", "PAGER=cat", "LC_ALL=C", "LANG=C", - "PATH=/usr/bin:/bin", - } - return environment + // The shared half — the search path and the git configuration isolation — + // lives in trustedexec so the provenance verifier cannot drift from this + // one. Only settings specific to running git interactively are added here. + environment := append([]string(nil), trustedexec.Environment...) + return append(environment, "GIT_TERMINAL_PROMPT=0", "GIT_PAGER=cat", "PAGER=cat", "LC_ALL=C", "LANG=C") } diff --git a/internal/signatureverify/verify_test.go b/internal/signatureverify/verify_test.go index f2f3d64..fcd9845 100644 --- a/internal/signatureverify/verify_test.go +++ b/internal/signatureverify/verify_test.go @@ -13,6 +13,8 @@ import ( "path/filepath" "strings" "testing" + + "github.com/NDDev-it-com/agent-runtime/internal/trustedexec" ) func TestCanonicalTrustAnchorIdentity(t *testing.T) { @@ -126,8 +128,12 @@ func TestVerifierUsesCommandLocalTrustAndPreservesCommandFailure(t *testing.T) { } verifyCall := commands.calls[2] joined := strings.Join(verifyCall.args, " ") + sshKeygen, keygenErr := trustedexec.SSHKeygen() + if keygenErr != nil { + t.Fatal(keygenErr) + } if !strings.Contains(joined, "-c gpg.format=ssh -c gpg.ssh.allowedSignersFile=") || - !strings.Contains(joined, "-c gpg.ssh.program="+sshKeygenExecutable) || + !strings.Contains(joined, "-c gpg.ssh.program="+sshKeygen) || !strings.Contains(joined, "-c gpg.ssh.revocationFile="+os.DevNull) || !strings.Contains(joined, "-c gpg.minTrustLevel=fully") || strings.Contains(joined, filepath.Join(fixture.repository, AllowedSignersPath)) { @@ -179,7 +185,7 @@ func TestIsolatedGitEnvironmentRejectsAmbientOverrideChannels(t *testing.T) { t.Fatalf("ambient override survived: %s", forbidden) } } - for _, required := range []string{"GIT_CONFIG_NOSYSTEM=1", "GIT_CONFIG_GLOBAL=" + os.DevNull, "GIT_CONFIG_COUNT=0", "GIT_NO_REPLACE_OBJECTS=1", "GIT_PAGER=cat", "PAGER=cat", "LC_ALL=C", "LANG=C", "PATH=/usr/bin:/bin"} { + for _, required := range []string{"GIT_CONFIG_NOSYSTEM=1", "GIT_CONFIG_GLOBAL=" + os.DevNull, "GIT_CONFIG_COUNT=0", "GIT_NO_REPLACE_OBJECTS=1", "GIT_PAGER=cat", "PAGER=cat", "LC_ALL=C", "LANG=C", "PATH=" + trustedexec.SearchPathValue()} { if !strings.Contains(joined, "\n"+required+"\n") { t.Fatalf("missing isolated environment setting %q: %q", required, environment) } diff --git a/internal/trustedexec/resolve.go b/internal/trustedexec/resolve.go new file mode 100644 index 0000000..3dea4da --- /dev/null +++ b/internal/trustedexec/resolve.go @@ -0,0 +1,105 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +// Package trustedexec resolves the executables the trust path runs. +// +// Signature and provenance verification shell out to git and ssh-keygen, and +// which file those names refer to decides what the verdict is worth. Resolving +// them through the ambient PATH would let anyone who can set an environment +// variable choose the program that decides whether a commit is signed, so this +// package never reads PATH. It searches a fixed, ordered list of absolute +// directories and requires the result to be a regular, non-symlink, executable +// file. +// +// The trade is deliberate and it is not free: a host that keeps its tools +// outside these directories — Nix and Guix are the ones that matter — cannot +// run verification at all. Every mechanism that would cover them, an +// environment override or a PATH search, is an ambient input into the one code +// path that must not have any. Failing with a clear message naming what was +// searched is the better half of that trade, and SECURITY.md states it. +package trustedexec + +import ( + "fmt" + "os" + "path/filepath" + "strings" + "sync" +) + +// searchPath is the whole search. It is a list rather than a single constant so +// that a Homebrew macOS host works without weakening anything: every entry is +// absolute and none comes from the environment. +var searchPath = []string{"/usr/bin", "/bin", "/usr/local/bin", "/opt/homebrew/bin"} + +// Environment is the child environment the trust path runs commands under. It +// is here rather than duplicated at each call site because a second copy is a +// second thing to keep right. +var Environment = []string{ + "PATH=" + SearchPathValue(), + "GIT_CONFIG_NOSYSTEM=1", + "GIT_CONFIG_GLOBAL=" + os.DevNull, + "GIT_CONFIG_COUNT=0", + "GIT_NO_REPLACE_OBJECTS=1", +} + +// SearchPathValue renders the search path as a PATH value. A child of the trust +// path gets exactly the directories this package is willing to resolve from, +// and nothing it inherited. +func SearchPathValue() string { return strings.Join(searchPath, ":") } + +type resolution struct { + path string + err error +} + +var ( + cache sync.Map // name -> resolution + cacheMu sync.Mutex +) + +// Git returns the git executable the trust path runs. +func Git() (string, error) { return Resolve("git") } + +// SSHKeygen returns the ssh-keygen executable the trust path runs. +func SSHKeygen() (string, error) { return Resolve("ssh-keygen") } + +// Resolve returns the absolute path of a trusted tool, resolved once per +// process. A name containing a separator is rejected rather than treated as a +// path, because accepting one would make the search optional. +func Resolve(name string) (string, error) { + if name == "" || strings.ContainsRune(name, os.PathSeparator) { + return "", fmt.Errorf("trusted executable name %q must be a bare name", name) + } + if cached, ok := cache.Load(name); ok { + got := cached.(resolution) + return got.path, got.err + } + cacheMu.Lock() + defer cacheMu.Unlock() + if cached, ok := cache.Load(name); ok { + got := cached.(resolution) + return got.path, got.err + } + got := search(name) + cache.Store(name, got) + return got.path, got.err +} + +func search(name string) resolution { + for _, directory := range searchPath { + candidate := filepath.Join(directory, name) + // Lstat, not Stat: a symlink here would let whatever it points at be + // substituted without touching the directory this package trusts. + info, err := os.Lstat(candidate) + if err != nil || !info.Mode().IsRegular() || info.Mode().Perm()&0o111 == 0 { + continue + } + // A tool anyone can rewrite is a tool anyone can replace, and the + // verdict it produces would be theirs rather than the repository's. + if info.Mode().Perm()&0o022 != 0 { + return resolution{err: fmt.Errorf("trusted executable %s is group or world writable", candidate)} + } + return resolution{path: candidate} + } + return resolution{err: fmt.Errorf("trusted executable %q was not found in %s; verification never reads PATH, so a host that keeps its tools elsewhere cannot run it", name, strings.Join(searchPath, ", "))} +} diff --git a/internal/trustedexec/resolve_test.go b/internal/trustedexec/resolve_test.go new file mode 100644 index 0000000..6596274 --- /dev/null +++ b/internal/trustedexec/resolve_test.go @@ -0,0 +1,93 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package trustedexec + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func TestResolvesTheToolsTheTrustPathRuns(t *testing.T) { + t.Parallel() + for _, resolve := range []func() (string, error){Git, SSHKeygen} { + path, err := resolve() + if err != nil { + t.Fatalf("a supported host must resolve the trust tools: %v", err) + } + if !filepath.IsAbs(path) { + t.Fatalf("resolved %q is not absolute", path) + } + info, err := os.Lstat(path) + if err != nil || !info.Mode().IsRegular() { + t.Fatalf("resolved %q is not a regular file: %v", path, err) + } + } +} + +// TestNeverReadsThePathEnvironment is the property the fixed search exists for: +// whoever can set an environment variable must not be able to choose the +// program that decides whether a commit is signed. +func TestNeverReadsThePathEnvironment(t *testing.T) { + fake := t.TempDir() + impostor := filepath.Join(fake, "git") + if err := os.WriteFile(impostor, []byte("#!/bin/sh\nexit 0\n"), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", fake) + // Resolution is cached per process, so ask the search directly rather than + // through the cache to prove the environment is not consulted. + got := search("git") + if got.err != nil { + t.Fatalf("resolution failed: %v", got.err) + } + if strings.HasPrefix(got.path, fake) { + t.Fatalf("resolution followed PATH to %q", got.path) + } +} + +func TestRejectsNamesThatWouldSkipTheSearch(t *testing.T) { + t.Parallel() + for _, name := range []string{"", "/usr/bin/git", "../git", "sub/git"} { + if _, err := Resolve(name); err == nil { + t.Errorf("name %q bypassed the search", name) + } + } +} + +// TestSearchNamesWhatItLookedAt covers the honesty half of the trade: a host +// that keeps its tools elsewhere cannot verify anything, so the failure has to +// say why rather than surfacing as an obscure exec error. +func TestSearchNamesWhatItLookedAt(t *testing.T) { + t.Parallel() + got := search("definitely-not-a-real-trust-tool") + if got.err == nil { + t.Fatal("a missing tool resolved") + } + message := got.err.Error() + for _, directory := range searchPath { + if !strings.Contains(message, directory) { + t.Errorf("the failure does not name %s: %s", directory, message) + } + } + if !strings.Contains(message, "never reads PATH") { + t.Errorf("the failure does not explain the constraint: %s", message) + } +} + +// TestRefusesAWritableTool covers substitution by anyone who can write the +// binary rather than the directory. +func TestRefusesAWritableTool(t *testing.T) { + dir := t.TempDir() + tool := filepath.Join(dir, "toolish") + if err := os.WriteFile(tool, []byte("#!/bin/sh\n"), 0o777); err != nil { + t.Fatal(err) + } + original := searchPath + t.Cleanup(func() { searchPath = original }) + searchPath = []string{dir} + if got := search("toolish"); got.err == nil { + t.Fatal("a group and world writable tool was accepted") + } +} diff --git a/security-tools.json b/security-tools.json index 2738cbb..4355b2f 100644 --- a/security-tools.json +++ b/security-tools.json @@ -4,14 +4,12 @@ "govulncheck": { "module": "golang.org/x/vuln/cmd/govulncheck", "version": "v1.7.0", - "minimum_go": "1.25.0", - "upstream_go_mod": "https://github.com/golang/vuln/blob/v1.7.0/go.mod" + "minimum_go": "1.25.0" }, "staticcheck": { "module": "honnef.co/go/tools/cmd/staticcheck", "version": "v0.7.0", - "minimum_go": "1.25.0", - "upstream_go_mod": "https://github.com/dominikh/go-tools/blob/v0.7.0/go.mod" + "minimum_go": "1.25.0" }, "security_go": "1.26.6", "compatibility_go": "1.25" From 4669c5a24e2f07f93ebe0f50e32351d02e26e0c6 Mon Sep 17 00:00:00 2001 From: Danil Silantyev Date: Sat, 15 Aug 2026 05:18:10 +0500 Subject: [PATCH 2/2] fix(trustedexec): set the writable-tool fixture mode explicitly CI rejected the first attempt on both platforms. The fixture created its file with WriteFile(0o777), and a creation mode is masked by the process umask: under the 022 umask CI uses the file landed at 0755, was correctly accepted, and the case asserted nothing. It only passed locally because the suite had been run under umask 002, where 0777 becomes 0775 and is group writable. This repository has paid for that lesson once already, in the release suite, and the new test reintroduced it. Modes are now set with Chmod, which the umask does not touch, and asserted before the case runs. Group-writable, world-writable and both are covered, together with the owner-writable control that must resolve, and a symlinked tool that a plain Stat would have followed. The search also no longer reaches through the package-level list, so a test can exercise the rules without mutating state that parallel tests read. Verified under umask 022, 002 and 077, and with -race. --- .../goals/no-unenforced-ceremony.json | 9 ++- internal/trustedexec/resolve.go | 8 ++- internal/trustedexec/resolve_test.go | 68 ++++++++++++++++--- 3 files changed, 73 insertions(+), 12 deletions(-) diff --git a/.agent-runtime/goals/no-unenforced-ceremony.json b/.agent-runtime/goals/no-unenforced-ceremony.json index cd20d87..de8fe2b 100644 --- a/.agent-runtime/goals/no-unenforced-ceremony.json +++ b/.agent-runtime/goals/no-unenforced-ceremony.json @@ -1,6 +1,6 @@ { "schema_version": "v1alpha1", - "revision": 11, + "revision": 12, "goal": { "id": "no-unenforced-ceremony", "intent": "Every retained contract field buys enforcement and trust tooling resolves without ambient paths", @@ -56,6 +56,11 @@ "type": "test", "reference": "internal/trustedexec TestSearchNamesWhatItLookedAt", "result": "A missing tool fails naming every directory searched and stating that PATH is never read." + }, + { + "type": "test", + "reference": "https://github.com/NDDev-it-com/agent-runtime/actions/runs/31852939515", + "result": "CI rejected the first attempt: the writable-tool fixture used WriteFile(0o777), which the umask masks, so it landed at 0755 under the 022 umask CI uses and proved nothing. Modes are now set with Chmod and asserted before use, and the suite is run under umask 022, 002 and 077." } ], "recorded_at": "2026-08-15T00:12:44.466737374Z" @@ -134,6 +139,6 @@ } }, "created_at": "2026-08-15T00:12:44.224755101Z", - "updated_at": "2026-08-15T00:12:44.624414366Z" + "updated_at": "2026-08-15T00:18:10.11806904Z" } } diff --git a/internal/trustedexec/resolve.go b/internal/trustedexec/resolve.go index 3dea4da..400bfad 100644 --- a/internal/trustedexec/resolve.go +++ b/internal/trustedexec/resolve.go @@ -85,8 +85,12 @@ func Resolve(name string) (string, error) { return got.path, got.err } -func search(name string) resolution { - for _, directory := range searchPath { +func search(name string) resolution { return searchIn(searchPath, name) } + +// searchIn is separate so a test can exercise the rules without mutating the +// package's own search path, which parallel tests read. +func searchIn(directories []string, name string) resolution { + for _, directory := range directories { candidate := filepath.Join(directory, name) // Lstat, not Stat: a symlink here would let whatever it points at be // substituted without touching the directory this package trusts. diff --git a/internal/trustedexec/resolve_test.go b/internal/trustedexec/resolve_test.go index 6596274..ab3796e 100644 --- a/internal/trustedexec/resolve_test.go +++ b/internal/trustedexec/resolve_test.go @@ -38,7 +38,7 @@ func TestNeverReadsThePathEnvironment(t *testing.T) { t.Setenv("PATH", fake) // Resolution is cached per process, so ask the search directly rather than // through the cache to prove the environment is not consulted. - got := search("git") + got := searchIn(searchPath, "git") if got.err != nil { t.Fatalf("resolution failed: %v", got.err) } @@ -61,7 +61,7 @@ func TestRejectsNamesThatWouldSkipTheSearch(t *testing.T) { // say why rather than surfacing as an obscure exec error. func TestSearchNamesWhatItLookedAt(t *testing.T) { t.Parallel() - got := search("definitely-not-a-real-trust-tool") + got := searchIn(searchPath, "definitely-not-a-real-trust-tool") if got.err == nil { t.Fatal("a missing tool resolved") } @@ -78,16 +78,68 @@ func TestSearchNamesWhatItLookedAt(t *testing.T) { // TestRefusesAWritableTool covers substitution by anyone who can write the // binary rather than the directory. +// +// The modes are set with Chmod rather than passed to WriteFile because the +// creation mode is masked by the process umask: written as 0777 under the 022 +// umask CI uses, the file lands at 0755 and the case proves nothing. This +// repository has paid for that lesson once already, in the release suite. func TestRefusesAWritableTool(t *testing.T) { + t.Parallel() + for name, mode := range map[string]os.FileMode{ + "group writable": 0o775, + "world writable": 0o757, + "both": 0o777, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + tool := filepath.Join(dir, "toolish") + if err := os.WriteFile(tool, []byte("#!/bin/sh\n"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Chmod(tool, mode); err != nil { + t.Fatal(err) + } + info, err := os.Lstat(tool) + if err != nil || info.Mode().Perm() != mode { + t.Fatalf("the fixture is not %v: %v (%v)", mode, info.Mode().Perm(), err) + } + if got := searchIn([]string{dir}, "toolish"); got.err == nil { + t.Fatalf("a tool at mode %v was accepted", mode) + } + }) + } + // The same tool, writable only by its owner, must resolve. dir := t.TempDir() tool := filepath.Join(dir, "toolish") - if err := os.WriteFile(tool, []byte("#!/bin/sh\n"), 0o777); err != nil { + if err := os.WriteFile(tool, []byte("#!/bin/sh\n"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Chmod(tool, 0o755); err != nil { + t.Fatal(err) + } + if got := searchIn([]string{dir}, "toolish"); got.err != nil { + t.Fatalf("an owner-writable executable was rejected: %v", got.err) + } +} + +// TestRefusesASymlinkedTool covers substitution through a link, which a plain +// Stat would follow without noticing. +func TestRefusesASymlinkedTool(t *testing.T) { + t.Parallel() + dir := t.TempDir() + real := filepath.Join(dir, "real") + if err := os.WriteFile(real, []byte("#!/bin/sh\n"), 0o600); err != nil { t.Fatal(err) } - original := searchPath - t.Cleanup(func() { searchPath = original }) - searchPath = []string{dir} - if got := search("toolish"); got.err == nil { - t.Fatal("a group and world writable tool was accepted") + if err := os.Chmod(real, 0o755); err != nil { + t.Fatal(err) + } + link := filepath.Join(dir, "toolish") + if err := os.Symlink(real, link); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + if got := searchIn([]string{dir}, "toolish"); got.err == nil { + t.Fatal("a symlinked tool was accepted") } }