Sign in

Search issue titles and bodies, change requests and comments. Private repositories you cannot read are not included.

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.

Make the audit log one shape that a privileged change cannot skip

b1499c729a026cfda32d8a90374b3b8391ca0090 · Keenan Nemetz · 13d ago · parent d74d4de8

Make the audit log one shape that a privileged change cannot skip

Every writer used to format its own line and open its own file, so
data/audit.log held six different column layouts and a write that failed
changed nothing. A log that can be bypassed by a full disk, or by a key
comment containing a tab, is not the security promise the docs make.

internal/audit is now the only writer: one line per action, always
timestamp, action, actor, subject, detail. Every field is escaped and
capped at 4 KiB, so a key comment, an LFS lock path, a repository name or
an admin argument cannot add a column or a record. One write per line
under O_APPEND keeps the web, the SSH server and each hook process from
interleaving, and secure.OpenAppend keeps the file at 0600 including on an
upgrade from 0644.

Privileged changes now fail closed, and they are recorded before they
happen, so a failed write means the change did not occur:

  - roster, key and instance-role changes (inside the transaction)
  - access-rule changes, and a repository deletion, which records before
    removing content and forgets the rules only afterwards
  - session creation and revocation
  - a forced LFS unlock
  - an admin command over SSH, which reports audit_unavailable
  - a push that overrides a lock, an approval, a release or history

Ordinary pushes and web actions are reported instead: their changes are
already durable in git, so a write failure is logged at error level (or on
the pusher's terminal from post-receive) rather than making a working push
look rejected.

Opening an instance now checks the log is a regular file at 0600 owned by
the service account and appendable, and refuses to start otherwise. There
is no setting that disables the log or returns it to best effort.

Regression tests cover field injection through key comments, lock paths,
admin argv and repository names; 0600 on create and on upgrade; an
unwritable sink failing an ACL change, an account change, a session, a
forced unlock, an admin command and a push override while leaving the
underlying state unchanged; concurrent appends keeping whole lines; and
startup refusing a symlinked log.

server/hook/hook.go +96−32

@@ -5,7 +5,6 @@ package hook
5 5 import (
6 6 "bufio"
7 7 "fmt"
8 "github.com/rodent-software/vole/internal/secure"
9 8 "io"
10 9 "os"
11 10 "path/filepath"
@@ -20,6 +19,7 @@ import (
20 19 "github.com/rodent-software/vole/forge/identity"
21 20 "github.com/rodent-software/vole/forge/landing"
22 21 "github.com/rodent-software/vole/forge/repo"
22 "github.com/rodent-software/vole/internal/audit"
23 23 "github.com/rodent-software/vole/internal/format"
24 24 "github.com/rodent-software/vole/metadata"
25 25 )
@@ -42,8 +42,9 @@ type Config struct {
42 42 // LFSWarnBytes flags binaries committed straight into git above this size.
43 43 // Zero disables the warning.
44 44 LFSWarnBytes int64
45 // AuditPath receives lines about privileged metadata operations. Empty means
46 // no audit sink, which is only acceptable for tests.
45 // AuditPath receives lines about privileged metadata operations. When it is
46 // empty or unwritable, an override is refused and an ordinary push is answered
47 // with a warning: the refs have already moved by the time post-receive runs.
47 48 AuditPath string
48 49 Out io.Writer
49 50 In io.Reader
@@ -194,8 +195,17 @@ func PreReceive(cfg Config) int {
194 195 } else if !ff {
195 196 switch {
196 197 case role.AtLeast(access.Admin):
197 auditLine(cfg.AuditPath, fmt.Sprintf("force-push %s %s -> %s by %s in %s",
198 c.ref, format.SHA(c.old), format.SHA(c.new), actor, m.Name))
198 // A rewrite is allowed, but only on the record: an override that
199 // cannot be written is refused, which is the whole value of it.
200 if err := auditLine(cfg.AuditPath, audit.Record{
201 Action: "push.force",
202 Actor: actor,
203 Subject: m.Name,
204 Detail: fmt.Sprintf("%s %s -> %s", c.ref, format.SHA(c.old), format.SHA(c.new)),
205 }); err != nil {
206 problems = append(problems, fmt.Sprintf(
207 "%s: cannot record the forced push, refusing it: %v", c.ref, err))
208 }
199 209 case role == access.Contribute && access.IsContributorRef(actor, c.ref):
200 210 // The namespace is theirs: rebasing or deleting their own branch is
201 211 // not a rewrite of shared history.
@@ -254,8 +264,16 @@ func PreReceive(cfg Config) int {
254 264 } else if len(lockProblems) > 0 {
255 265 if role.AtLeast(access.Admin) {
256 266 for _, p := range lockProblems {
257 auditLine(cfg.AuditPath, fmt.Sprintf("lock override %s by %s in %s: %s",
258 c.ref, actor, m.Name, p))
267 if err := auditLine(cfg.AuditPath, audit.Record{
268 Action: "lock.override",
269 Actor: actor,
270 Subject: m.Name,
271 Detail: c.ref + ": " + p,
272 }); err != nil {
273 problems = append(problems, fmt.Sprintf(
274 "%s: cannot record the lock override, refusing it: %v", c.ref, err))
275 continue
276 }
259 277 reject(cfg.Out, "admin override, audited: %s", p)
260 278 }
261 279 } else {
@@ -273,8 +291,16 @@ func PreReceive(cfg Config) int {
273 291 if relProblems := releaseFindings(g, c); len(relProblems) > 0 {
274 292 if role.AtLeast(access.Admin) {
275 293 for _, p := range relProblems {
276 auditLine(cfg.AuditPath, fmt.Sprintf("release rewrite %s by %s in %s: %s",
277 c.ref, actor, m.Name, p))
294 if err := auditLine(cfg.AuditPath, audit.Record{
295 Action: "release.rewrite",
296 Actor: actor,
297 Subject: m.Name,
298 Detail: c.ref + ": " + p,
299 }); err != nil {
300 problems = append(problems, fmt.Sprintf(
301 "%s: cannot record the release rewrite, refusing it: %v", c.ref, err))
302 continue
303 }
278 304 reject(cfg.Out, "admin override, audited: %s", p)
279 305 }
280 306 } else {
@@ -299,8 +325,17 @@ func PreReceive(cfg Config) int {
299 325 continue
300 326 }
301 327 if merge != nil {
302 auditLine(cfg.AuditPath, fmt.Sprintf("merge %s via request %s (head %s) approved by %s",
303 c.ref, merge.Request.ID, format.SHA(merge.Request.Head), strings.Join(merge.Approvers, ",")))
328 // The landing itself is in the repository's history, so a log that
329 // cannot be written is reported rather than turned into a refusal.
330 if err := auditLine(cfg.AuditPath, audit.Record{
331 Action: "merge",
332 Actor: actor,
333 Subject: m.Name,
334 Detail: fmt.Sprintf("%s via request %s (head %s) approved by %s",
335 c.ref, merge.Request.ID, format.SHA(merge.Request.Head), strings.Join(merge.Approvers, ",")),
336 }); err != nil {
337 reject(cfg.Out, "audit log write failed: %v", err)
338 }
304 339 }
305 340 }
306 341 // The issues branch is user-authored content, so it is validated at the
@@ -324,13 +359,13 @@ func PreReceive(cfg Config) int {
324 359 // name must be the actor's account name. Without this, anyone with write
325 360 // access could commit approvals/<someone-else>.json and approve on their
326 361 // behalf.
327 for _, prob := range approvalOwnership(g, c, actor, role, cfg.AuditPath) {
362 for _, prob := range approvalOwnership(g, c, actor, role, cfg.AuditPath, m.Name) {
328 363 problems = append(problems, prob)
329 364 }
330 365 // A comment is somebody's own words, so changing or deleting it is
331 366 // theirs too — checked against the author on the *old* blob, which is
332 367 // the one that was validated when it landed.
333 for _, prob := range commentOwnership(g, c, actor, role, cfg.AuditPath, cfg.Out) {
368 for _, prob := range commentOwnership(g, c, actor, role, cfg.AuditPath, cfg.Out, m.Name) {
334 369 problems = append(problems, prob)
335 370 }
336 371 }
@@ -392,7 +427,7 @@ func readChanges(r io.Reader) ([]change, error) {
392 427 // Every write counts: added, modified, renamed, retyped. Checking only additions
393 428 // is not enough, because a file that already exists can simply be edited to name
394 429 // a different commit.
395 func approvalOwnership(g *git.Repo, c change, actor string, role access.Role, auditPath string) []string {
430 func approvalOwnership(g *git.Repo, c change, actor string, role access.Role, auditPath, repoName string) []string {
396 431 // On the first push the branch has no old commit, so every approval in the
397 432 // tree is new and must be checked. Skipping that would let whoever creates the
398 433 // branch seed approvals in other people's names.
@@ -430,8 +465,14 @@ func approvalOwnership(g *git.Repo, c change, actor string, role access.Role, au
430 465 problems = append(problems, fmt.Sprintf(
431 466 "requests: %s: %s may not delete %s's approval; only %s can withdraw it",
432 467 path, actor, owner, owner))
433 } else {
434 auditLine(auditPath, fmt.Sprintf("approval-removed %s by admin %s", path, actor))
468 } else if err := auditLine(auditPath, audit.Record{
469 Action: "approval.remove",
470 Actor: actor,
471 Subject: repoName,
472 Detail: path,
473 }); err != nil {
474 problems = append(problems, fmt.Sprintf(
475 "requests: %s: cannot record the removal of an approval, refusing the push: %v", path, err))
435 476 }
436 477 continue
437 478 }
@@ -460,7 +501,7 @@ func approvalOwnership(g *git.Repo, c change, actor string, role access.Role, au
460 501 // - A request's own file is not covered either, for the same reason: its state,
461 502 // head and base are shared, and updating them is the author's normal edit.
462 503 // - A first push has nothing to compare against, so it is skipped.
463 func commentOwnership(g *git.Repo, c change, actor string, role access.Role, auditPath string, out io.Writer) []string {
504 func commentOwnership(g *git.Repo, c change, actor string, role access.Role, auditPath string, out io.Writer, repoName string) []string {
464 505 if c.old == "" || c.old == git.ZeroSHA {
465 506 return nil
466 507 }
@@ -503,7 +544,20 @@ func commentOwnership(g *git.Repo, c change, actor string, role access.Role, aud
503 544 // cannot be fixed by them — but never silently: the override is audited,
504 545 // like a force-push and a release rewrite.
505 546 if role.AtLeast(access.Admin) {
506 auditLine(auditPath, fmt.Sprintf("comment-%s %s (author %s) by admin %s", st, path, author, actor))
547 action := "comment.change"
548 if st == "D" {
549 action = "comment.delete"
550 }
551 if err := auditLine(auditPath, audit.Record{
552 Action: action,
553 Actor: actor,
554 Subject: repoName,
555 Detail: fmt.Sprintf("%s (author %s)", path, author),
556 }); err != nil {
557 problems = append(problems, fmt.Sprintf(
558 "issues: %s: cannot record the override, refusing the push: %v", path, err))
559 continue
560 }
507 561 reject(out, "admin override, audited: %s %s %s's comment", actor, verb, author)
508 562 continue
509 563 }
@@ -554,16 +608,11 @@ func repoNameFromDir(dir string) string {
554 608 }
555 609
556 610 // auditLine is best-effort: failing to write the log must not change policy.
557 func auditLine(path, line string) {
558 if path == "" {
559 return
560 }
561 f, err := secure.OpenAppend(path)
562 if err != nil {
563 return
564 }
565 fmt.Fprintf(f, "%s\t%s\n", time.Now().UTC().Format(time.RFC3339), line)
566 f.Close()
611 // auditLine appends one record. Pre-receive treats an error as a refusal, because
612 // the alternative is an override that happened and cannot be found; post-receive
613 // reports it on the pusher's terminal, because the refs have already moved.
614 func auditLine(path string, r audit.Record) error {
615 return audit.Append(path, r)
567 616 }
568 617
569 618 func reject(w io.Writer, format string, args ...any) {
@@ -604,8 +653,17 @@ func PostReceive(cfg Config) int {
604 653 continue
605 654 }
606 655 old, next, ref := fields[0], fields[1], fields[2]
607 auditLine(cfg.AuditPath, fmt.Sprintf("push %s %s -> %s by %s in %s",
608 ref, shortOrNone(old), shortOrNone(next), who, repo))
656 if err := auditLine(cfg.AuditPath, audit.Record{
657 Action: "push",
658 Actor: who,
659 Subject: repo,
660 Detail: fmt.Sprintf("%s %s -> %s", ref, shortOrNone(old), shortOrNone(next)),
661 }); err != nil {
662 // The update landed before this process ran, so the only honest report
663 // is on the pusher's terminal; failing the hook would make the client
664 // re-send something that already worked.
665 fmt.Fprintf(cfg.Out, "vole: audit log write failed: %v\n", err)
666 }
609 667 // A change request opened from a branch follows it: the push that just moved
610 668 // the ref moves the request's head in the same breath. It runs here, after the
611 669 // ref update, because the new tip is the event; pre-receive could only guess.
@@ -619,8 +677,14 @@ func PostReceive(cfg Config) int {
619 677 case err != nil:
620 678 fmt.Fprintf(cfg.Out, "vole: could not move requests following %s: %v\n", branch, err)
621 679 case n > 0:
622 auditLine(cfg.AuditPath, fmt.Sprintf("request-follow %s -> %s by %s in %s (%d request(s))",
623 ref, shortOrNone(next), actor, repo, n))
680 if err := auditLine(cfg.AuditPath, audit.Record{
681 Action: "request.follow",
682 Actor: actor,
683 Subject: repo,
684 Detail: fmt.Sprintf("%s -> %s (%d request(s))", ref, shortOrNone(next), n),
685 }); err != nil {
686 fmt.Fprintf(cfg.Out, "vole: audit log write failed: %v\n", err)
687 }
624 688 }
625 689 }
626 690 if cfg.SpoolDir == "" {