`c8058f1`

storage: bare repos use --shared=group + add repair backfill (SR2 #287)

Pre-fix: storage.RepoFS.InitBare ran 'git init --bare' without
--shared=group, so objects/ wound up 0755 with no group-write.
shithubd-web (runs as 'shithub' user) created repos; SSH-git
dispatched git-receive-pack as the 'git' user (in the 'shithub'
group). 'git' had read-execute on objects/ but not write, so push
failed with 'unable to create temporary object directory'.
git-upload-pack worked because read was sufficient.

Fix at the source:
- InitBare now runs 'git init --bare --shared=group --initial-branch=trunk'.
Persists core.sharedRepository=group in config; produces 2775
dirs (group write + setgid) and 0664 files. Parent dir gets 2750
so the setgid propagates from byte zero.
- CloneBareShared (fork path) prepends '-c core.sharedRepository=group'
so the cloned repo carries the contract. NB: 'git clone --shared'
alone is the alternates flag, NOT the perms flag — same word, two
meanings.
- RepairSharedPerms backfills existing repos: sets the config flag,
walks the tree, chmods g+w on files and g+w+s on dirs. Idempotent.
- 'shithubd storage repair-shared-perms' subcommand walks every
<prefix>/<owner>/<name>.git under storage.repos_root and applies
the repair. One-time use after deploying this binary on shithub-
prod (the live droplet has 1 repo created pre-fix that needs it).

Tests:
- TestInitBare_SharedGroupContract: asserts core.sharedRepository
config value + group-write bit on objects/.
- TestRepairSharedPerms_FixesPreFixRepo: builds a deliberately
pre-fix repo, calls Repair, asserts post-conditions match the
contract InitBare produces from byte zero.

Closes nothing yet — operator runs the new subcommand on the
droplet after deploy. Audit script (deploy/audit/check-droplet-
drift.sh) will pick up the binary swap and reflect drift if not.

Authored by

espadonne 3 days ago

SHA: c8058f1d873fa7b3c3ae04ad98ae2e01d80bad99
Parents: 4b9dfc2
Tree: c609637

3 changed files

Status	File	+	-
M	`cmd/shithubd/storage.go`	81	0
M	`internal/infra/storage/reposfs.go`	74	5
M	`internal/infra/storage/reposfs_test.go`	113	0

cmd/shithubd/storage.gomodified

  	},
+ }
 +var storageRepairSharedPermsCmd = &cobra.Command{
 +	Use:   "repair-shared-perms",
 +	Short: "Bring existing bare repos to core.sharedRepository=group + g+w mode bits",
 +	Long: `One-time backfill for repos created before SR2 #287 landed.
++
 +Pre-fix, bare repos were created with 'git init --bare' (no
 +--shared=group), so objects/ wound up 0755. The SSH-git push path
 +(git-receive-pack runs as the 'git' user, repos owned by 'shithub')
 +hit "unable to create temporary object directory" because the group
 +write bit was missing.
++
 +This subcommand walks every <prefix>/<owner>/<name>.git directory
 +under storage.repos_root and:
 +  - sets core.sharedRepository=group in each repo's config
 +  - chmods every file g+w
 +  - chmods every directory g+w + g+s so future writes inherit group
++
 +Idempotent. Safe to re-run. Reports a per-repo summary.`,
 +	RunE: func(cmd *cobra.Command, _ []string) error {
 +		cfg, err := config.Load(nil)
 +		if err != nil {
 +			return err
 +		}
 +		root := cfg.Storage.ReposRoot
 +		if root == "" {
 +			return errors.New("storage.repos_root not configured")
 +		}
 +		fs, err := storage.NewRepoFS(root)
 +		if err != nil {
 +			return fmt.Errorf("repofs: %w", err)
 +		}
 +		ctx, cancel := context.WithTimeout(cmd.Context(), 5*time.Minute)
 +		defer cancel()
 +		out := cmd.OutOrStdout()
 +		var ok, fail int
 +		// Walk the canonical layout: <root>/<2-letter-prefix>/<owner>/<name>.git
 +		entries, err := os.ReadDir(root)
 +		if err != nil {
 +			return fmt.Errorf("read repos_root: %w", err)
 +		}
 +		for _, prefix := range entries {
 +			if !prefix.IsDir() {
 +				continue
 +			}
 +			ownerEntries, err := os.ReadDir(root + "/" + prefix.Name())
 +			if err != nil {
 +				continue
 +			}
 +			for _, owner := range ownerEntries {
 +				if !owner.IsDir() {
 +					continue
 +				}
 +				ownerDir := root + "/" + prefix.Name() + "/" + owner.Name()
 +				repos, err := os.ReadDir(ownerDir)
 +				if err != nil {
 +					continue
 +				}
 +				for _, repo := range repos {
 +					if !repo.IsDir() || !strings.HasSuffix(repo.Name(), ".git") {
 +						continue
 +					}
 +					path := ownerDir + "/" + repo.Name()
 +					if err := fs.RepairSharedPerms(ctx, path); err != nil {
 +						_, _ = fmt.Fprintf(out, "FAIL %s: %v\n", path, err)
 +						fail++
 +						continue
 +					}
 +					_, _ = fmt.Fprintf(out, "ok   %s\n", path)
 +					ok++
 +				}
 +			}
 +		}
 +		_, _ = fmt.Fprintf(out, "\nrepaired %d repo(s); %d failure(s)\n", ok, fail)
 +		if fail > 0 {
 +			return fmt.Errorf("repair-shared-perms: %d failures", fail)
 +		}
 +		return nil
 +	},
 +}
++
  func init() {
  	storageCmd.AddCommand(storageCheckCmd)
 +	storageCmd.AddCommand(storageRepairSharedPermsCmd)
+ }
  func checkReposRoot(root string) error {

internal/infra/storage/reposfs.gomodified

  //
  // The parent directory tree is created on demand. ErrAlreadyExists is
  // returned if path is non-empty.
 +//
 +// The repo is initialized with `--shared=group`, which:
 +//
 +//   - persists `core.sharedRepository=group` in config
 +//   - sets the setgid bit on directories (2775)
 +//   - keeps group-writable mode bits on files (0664)
 +//
 +// Both shithubd-web (web pushes via the HTTPS handler, runs as the
 +// `shithub` user) and the SSH `git` user (the AuthorizedKeysCommand
 +// dispatches into a process running as `git`, which is in the
 +// `shithub` group) write to the same bare repo on disk. Without
 +// `--shared=group`, git-receive-pack via SSH fails with
 +// "unable to create temporary object directory" because objects/
 +// is 0755 and group write isn't set.
  func (r *RepoFS) InitBare(ctx context.Context, path string) error {
  	if err := r.containedInRoot(path); err != nil {
  		return err
  	if entries, err := os.ReadDir(path); err == nil && len(entries) > 0 {
  		return fmt.Errorf("%w: %s", ErrAlreadyExists, path)
+ 	}
 -	if err := os.MkdirAll(filepath.Dir(path), 0o750); err != nil {
 +	if err := os.MkdirAll(filepath.Dir(path), 0o2750); err != nil {
  		return fmt.Errorf("storage: repofs: mkdir parent: %w", err)
+ 	}
 -	if err := os.MkdirAll(path, 0o750); err != nil {
 +	if err := os.MkdirAll(path, 0o2750); err != nil {
  		return fmt.Errorf("storage: repofs: mkdir target: %w", err)
+ 	}
  	// G204: path is constructed via RepoPath (strict whitelist) and verified
  	// to live under r.root. Caller cannot inject arbitrary args.
 -	cmd := exec.CommandContext(ctx, "git", "init", "--bare", "--initial-branch=trunk", path) //nolint:gosec
 +	cmd := exec.CommandContext(ctx, "git", "init", "--bare", "--shared=group", "--initial-branch=trunk", path) //nolint:gosec
  	out, err := cmd.CombinedOutput()
  	if err != nil {
  		return fmt.Errorf("storage: repofs: git init --bare: %w (output: %s)", err, strings.TrimSpace(string(out)))
  	if entries, err := os.ReadDir(dst); err == nil && len(entries) > 0 {
  		return fmt.Errorf("%w: %s", ErrAlreadyExists, dst)
+ 	}
 -	if err := os.MkdirAll(filepath.Dir(dst), 0o750); err != nil {
 +	if err := os.MkdirAll(filepath.Dir(dst), 0o2750); err != nil {
  		return fmt.Errorf("storage: repofs: mkdir parent: %w", err)
+ 	}
 +	// `git clone --shared` (here: object-alternates flag, NOT a perms
 +	// flag — same name, different sense than init's --shared=group).
 +	// To get group-writable perms we set core.sharedRepository=group
 +	// via -c so the cloned config has it from byte zero. Without this,
 +	// SSH-git push to a fork hits the same EACCES on objects/ that
 +	// PR for SR2 #287 fixed for `git init --bare` (see InitBare).
 +	//
  	// G204: src/dst are RepoPath-derived, both verified under r.root.
 -	cmd := exec.CommandContext(ctx, "git", "clone", "--bare", "--shared", src, dst) //nolint:gosec
 +	cmd := exec.CommandContext(ctx, "git", "-c", "core.sharedRepository=group", "clone", "--bare", "--shared", src, dst) //nolint:gosec
  	out, err := cmd.CombinedOutput()
  	if err != nil {
  		// Best-effort cleanup; if removal fails too, surface the
  	return nil
+ }
 +// RepairSharedPerms brings an existing bare repo to the
 +// `--shared=group` contract InitBare now produces from byte zero
 +// (SR2 #287). Idempotent: a repo already at the contract is left
 +// alone except for explicitly setting the config (cheap).
 +//
 +// Steps:
 +//  1. `git config core.sharedRepository=group`
 +//  2. `chmod -R g+w` and `find -type d -exec chmod g+s` so future
 +//     writes inherit the group on creation.
 +//
 +// Group ownership itself is NOT changed — the shipped invariant is
 +// that all repos are owned by the `shithub` group already (the
 +// shithub user creates them). If a repo's group is wrong, that's a
 +// separate provisioning bug; this method's job is only the bits.
 +func (r *RepoFS) RepairSharedPerms(ctx context.Context, path string) error {
 +	if err := r.containedInRoot(path); err != nil {
 +		return err
 +	}
 +	if _, err := os.Stat(path); err != nil {
 +		return fmt.Errorf("storage: repofs: stat %s: %w", path, err)
 +	}
 +	// Persist the contract in config.
 +	cfg := exec.CommandContext(ctx, "git", "-C", path, "config", "core.sharedRepository", "group") //nolint:gosec
 +	if out, err := cfg.CombinedOutput(); err != nil {
 +		return fmt.Errorf("storage: repofs: git config sharedRepository: %w (output: %s)", err, strings.TrimSpace(string(out)))
 +	}
 +	// Walk the tree once: directories get +g+s, files get +g+w.
 +	// We use filepath.Walk over an exec.Command(find ...) so the
 +	// behavior is identical across Linux and macOS test harnesses.
 +	if err := filepath.Walk(path, func(p string, info os.FileInfo, err error) error {
 +		if err != nil {
 +			return err
 +		}
 +		mode := info.Mode()
 +		newMode := mode | 0o060 // group rw
 +		if info.IsDir() {
 +			newMode |= os.ModeSetgid // g+s
 +		}
 +		if newMode == mode {
 +			return nil
 +		}
 +		return os.Chmod(p, newMode)
 +	}); err != nil {
 +		return fmt.Errorf("storage: repofs: walk chmod: %w", err)
 +	}
 +	return nil
 +}
++
  // SetPreciousObjects marks a bare repo's objects as not-prunable. The
  // canonical foot-gun for forks is source-repo `git gc` removing
  // objects that forks reach via alternates; setting this on the source

internal/infra/storage/reposfs_test.gomodified

+ 	}
+ }
 +// TestInitBare_SharedGroupContract pins SR2 #287:
 +// `git init --bare --shared=group` MUST be used so two users
 +// (shithubd-web's `shithub` user and the SSH dispatcher's `git`
 +// user, both in the `shithub` group) can write to objects/.
 +//
 +// Pre-fix the SSH-git push path failed with "unable to create
 +// temporary object directory" because objects/ was 0755 with no
 +// group-write bit.
 +//
 +// We assert the persisted config + the directory mode bits the
 +// flag produces.
 +func TestInitBare_SharedGroupContract(t *testing.T) {
 +	t.Parallel()
 +	if _, err := exec.LookPath("git"); err != nil {
 +		t.Skip("git not in PATH")
 +	}
 +	r, _ := mustNewRepoFS(t)
 +	path, err := r.RepoPath("alice", "sharedgrouptest")
 +	if err != nil {
 +		t.Fatalf("RepoPath: %v", err)
 +	}
 +	if err := r.InitBare(context.Background(), path); err != nil {
 +		t.Fatalf("InitBare: %v", err)
 +	}
++
 +	// 1) config has core.sharedRepository=group. git stores this as
 +	// the integer "1" internally (0=false, 1=group, 2=all, …);
 +	// either form satisfies the contract.
 +	out, err := exec.Command("git", "--git-dir", path, "config", "--get", "core.sharedRepository").Output() //nolint:gosec
 +	if err != nil {
 +		t.Fatalf("git config: %v", err)
 +	}
 +	got := strings.TrimSpace(string(out))
 +	if got != "group" && got != "1" {
 +		t.Fatalf("core.sharedRepository = %q, want \"group\" or \"1\"", got)
 +	}
++
 +	// 2) objects/ dir has group-write set (mode bit 0o020).
 +	objects := path + "/objects"
 +	st, err := os.Stat(objects)
 +	if err != nil {
 +		t.Fatalf("stat objects: %v", err)
 +	}
 +	mode := st.Mode().Perm()
 +	if mode&0o020 == 0 {
 +		t.Fatalf("objects/ mode = %#o; group-write bit (0o020) missing — SSH push will EACCES", mode)
 +	}
 +}
++
 +// TestRepairSharedPerms_FixesPreFixRepo pins the backfill path:
 +// a repo created without --shared=group (the pre-SR2 #287 layout)
 +// gets brought to the contract by RepairSharedPerms — config flag
 +// set, group-write bit on objects/, setgid on dirs.
 +func TestRepairSharedPerms_FixesPreFixRepo(t *testing.T) {
 +	t.Parallel()
 +	if _, err := exec.LookPath("git"); err != nil {
 +		t.Skip("git not in PATH")
 +	}
 +	r, root := mustNewRepoFS(t)
 +	path, err := r.RepoPath("alice", "repairtest")
 +	if err != nil {
 +		t.Fatalf("RepoPath: %v", err)
 +	}
 +	// Create the parent dir + a deliberately pre-fix bare repo
 +	// (NO --shared=group). This simulates a live repo from before
 +	// the fix landed.
 +	if err := os.MkdirAll(path, 0o750); err != nil {
 +		t.Fatalf("mkdir: %v", err)
 +	}
 +	if out, err := exec.Command("git", "init", "--bare", "--initial-branch=trunk", path).CombinedOutput(); err != nil {
 +		t.Fatalf("pre-fix init: %v: %s", err, out)
 +	}
 +	// Sanity: the pre-fix objects/ should NOT have group-write.
 +	objects := path + "/objects"
 +	st, err := os.Stat(objects)
 +	if err != nil {
 +		t.Fatalf("stat: %v", err)
 +	}
 +	if st.Mode().Perm()&0o020 != 0 {
 +		t.Skipf("pre-fix init produced 0%o; test environment differs (umask?). Skipping.", st.Mode().Perm())
 +	}
++
 +	// Run the repair.
 +	if err := r.RepairSharedPerms(context.Background(), path); err != nil {
 +		t.Fatalf("RepairSharedPerms: %v", err)
 +	}
++
 +	// Post-condition: config has the flag.
 +	out, err := exec.Command("git", "--git-dir", path, "config", "--get", "core.sharedRepository").Output() //nolint:gosec
 +	if err != nil {
 +		t.Fatalf("git config: %v", err)
 +	}
 +	got := strings.TrimSpace(string(out))
 +	if got != "group" && got != "1" {
 +		t.Fatalf("core.sharedRepository = %q, want \"group\" or \"1\"", got)
 +	}
 +	// objects/ has g+w.
 +	st, err = os.Stat(objects)
 +	if err != nil {
 +		t.Fatalf("stat after repair: %v", err)
 +	}
 +	mode := st.Mode().Perm()
 +	if mode&0o020 == 0 {
 +		t.Fatalf("after repair, objects/ mode = %#o; group-write missing", mode)
 +	}
 +	// objects/ has setgid.
 +	if st.Mode()&os.ModeSetgid == 0 {
 +		t.Fatalf("after repair, objects/ missing setgid bit; new files won't inherit group")
 +	}
++
 +	_ = root
 +}
++
  func TestInitBare_RefusesNonEmpty(t *testing.T) {
  	t.Parallel()
  	if _, err := exec.LookPath("git"); err != nil {