vole
Clone
Read only, no credentials:
https://rodent.software/git/vole.git
Write over SSH with a registered key:
git clone ssh://git@0.0.0.0:2222/vole.git
A free and open-source forge that lives in your Git repo.
Close the approval-forgery hole: check every write, not just additions
362932289cfbecbca4f9937def07b21671bc1aee · Keenan Nemetz · 17d ago · parent 1eeca4e6
Close the approval-forgery hole: check every write, not just additions The check that an approval is written by the principal it names used `--diff-filter=A`, so it only looked at newly added files. An approval that already existed could simply be edited: approve commit A, then rewrite approvals/<me>.json to name commit B and land B with the approver's name on it. A test now demonstrates that exact forgery before the fix and refuses it after. The check is now driven by the change status of every path: any non-deletion on `approvals/<name>.json` requires the actor to be `<name>`, and the first push (no old commit to diff, so every approval in the tree is new) is checked the same way. Deletion is limited to the owner, because removing someone else's approval forges nothing but erases their statement; an admin may still do it, and that is written to the audit log rather than happening silently. Renames land in the same net: without rename detection a move appears as an add, which the check sees, and the destination path is what the check reads. Also worth recording what the enforcement actually rests on, since the file name is the only name that carries authority: the SSH handshake resolves the presented key to a user through data/trusted_keys/<user>.keys, that mapping is one-to-one (a key already owned by another user is refused at add time and fatal at load), and the resolved name reaches the hook as VOLE_ACTOR, which the client cannot set because the exec surface is a two-entry whitelist. `author` fields in meta.json remain display-only and decide nothing. The remaining limitation is honest: this is enforcement at push time, not proof at read time. A clone shows that an approval file exists, not that the server saw its owner write it. Signed attestations in Phase 7 are what would make it verifiable offline.
internal/hook/hook.go +40−18
| @@ -163,7 +163,7 @@ func PreReceive(cfg Config) int { | |||
| 163 | 163 | // An approval is a statement about the actor's own review, so the file | |
| 164 | 164 | // name must be the actor's name. Without this, anyone with write access | |
| 165 | 165 | // could commit approvals/<someone-else>.json and approve on their behalf. | |
| 166 | for _, prob := range impostorApprovals(g, c, actor) { | ||
| 166 | for _, prob := range approvalOwnership(g, c, actor, role, cfg.AuditPath) { | ||
| 167 | 167 | problems = append(problems, prob) | |
| 168 | 168 | } | |
| 169 | 169 | } | |
| @@ -225,39 +225,61 @@ func readChanges(r io.Reader) ([]change, error) { | |||
| 225 | 225 | return out, sc.Err() | |
| 226 | 226 | } | |
| 227 | 227 | ||
| 228 | // impostorApprovals reports approvals added in this push that name someone other | ||
| 229 | // than the pusher. | ||
| 230 | func impostorApprovals(g *gitx.Repo, c change, actor string) []string { | ||
| 228 | // approvalOwnership enforces that an approval is written by the principal it | ||
| 229 | // names. The file name is how the tree records "who approved", so it has to be | ||
| 230 | // backed by the SSH identity the server resolved, not by anything the pusher | ||
| 231 | // chose to type. | ||
| 232 | // | ||
| 233 | // Every write counts: added, modified, renamed, retyped. Checking only additions | ||
| 234 | // is not enough, because a file that already exists can simply be edited to name | ||
| 235 | // a different commit. | ||
| 236 | func approvalOwnership(g *gitx.Repo, c change, actor string, role acl.Role, auditPath string) []string { | ||
| 231 | 237 | // On the first push the branch has no old commit, so every approval in the | |
| 232 | 238 | // tree is new and must be checked. Skipping that would let whoever creates the | |
| 233 | 239 | // branch seed approvals in other people's names. | |
| 234 | added := []string{} | ||
| 235 | 240 | if c.old == "" || c.old == gitx.ZeroSHA { | |
| 236 | 241 | blobs, err := g.LsTreeR(c.new) | |
| 237 | 242 | if err != nil { | |
| 238 | 243 | return []string{fmt.Sprintf("requests: cannot verify approvals in this push: %v", err)} | |
| 239 | 244 | } | |
| 245 | var problems []string | ||
| 240 | 246 | for _, b := range blobs { | |
| 241 | add := requests.ApprovalPath(b.Name) | ||
| 242 | if add != "" { | ||
| 243 | added = append(added, b.Name) | ||
| 247 | if owner := requests.ApprovalPath(b.Name); owner != "" && owner != actor { | ||
| 248 | problems = append(problems, fmt.Sprintf( | ||
| 249 | "requests: %s: %s may not create an approval in %s's name; only %s can approve as %s", | ||
| 250 | b.Name, actor, owner, owner, owner)) | ||
| 244 | 251 | } | |
| 245 | 252 | } | |
| 246 | } else { | ||
| 247 | var err error | ||
| 248 | added, err = g.AddedPaths(c.old, c.new) | ||
| 249 | if err != nil { | ||
| 250 | return []string{fmt.Sprintf("requests: cannot verify approvals in this push: %v", err)} | ||
| 251 | } | ||
| 253 | return problems | ||
| 254 | } | ||
| 255 | |||
| 256 | status, err := g.DiffStatus(c.old, c.new) | ||
| 257 | if err != nil { | ||
| 258 | return []string{fmt.Sprintf("requests: cannot verify approvals in this push: %v", err)} | ||
| 252 | 259 | } | |
| 253 | 260 | var problems []string | |
| 254 | for _, p := range added { | ||
| 255 | m := requests.ApprovalPath(p) | ||
| 256 | if m == "" || m == actor { | ||
| 261 | for path, st := range status { | ||
| 262 | owner := requests.ApprovalPath(path) | ||
| 263 | if owner == "" || owner == actor { | ||
| 264 | continue | ||
| 265 | } | ||
| 266 | if st == "D" { | ||
| 267 | // Removing someone else's approval forges nothing, but it erases their | ||
| 268 | // statement. The owner may withdraw; an admin may clean up, because the | ||
| 269 | // alternative (force-push) is heavier for a one-line fix. Neither is | ||
| 270 | // silent: the admin path is audited. | ||
| 271 | if !role.AtLeast(acl.Admin) { | ||
| 272 | problems = append(problems, fmt.Sprintf( | ||
| 273 | "requests: %s: %s may not delete %s's approval; only %s can withdraw it", | ||
| 274 | path, actor, owner, owner)) | ||
| 275 | } else { | ||
| 276 | auditLine(auditPath, fmt.Sprintf("approval-removed %s by admin %s", path, actor)) | ||
| 277 | } | ||
| 257 | 278 | continue | |
| 258 | 279 | } | |
| 259 | 280 | problems = append(problems, fmt.Sprintf( | |
| 260 | "requests: %s: %s may not add an approval in %s's name; approvals must be committed by the approver", p, actor, m)) | ||
| 281 | "requests: %s: %s may not add or change %s's approval; only %s can approve as %s", | ||
| 282 | path, actor, owner, owner, owner)) | ||
| 261 | 283 | } | |
| 262 | 284 | return problems | |
| 263 | 285 | } | |