backup: close S3 KEYMAP fd + use openSidecarFile (PR #718, round 7)

Codex round 9 raised two issues on commit ab38eb0:

1. P1: closeBucketKeymap leaked file descriptors. recordKeymap
   stored only the *KeymapWriter; closeBucketKeymap called
   KeymapWriter.Close() which flushes the bufio buffer but does
   NOT close the underlying *os.File. A dump producing keymaps
   for many buckets accumulated descriptors until EMFILE, after
   which subsequent bucket flushes failed and the dump output
   was incomplete. Track the *os.File on s3BucketState and
   close it from closeBucketKeymap alongside the KeymapWriter
   flush.

2. P2: recordKeymap used os.Create for KEYMAP.jsonl, which
   follows symlinks and clobbers hard links. The redis encoder
   already routes through openSidecarFile for the same kind of
   sidecar; mirror that path so a stale prior run (or local
   adversary) cannot turn a missing KEYMAP into an
   arbitrary-write primitive against /etc/passwd or similar.

Test: TestS3_KeymapRefusesSymlinkAtFinalize pre-creates
KEYMAP.jsonl as a symlink to a bait file, drives a meta-suffix
rename (so recordKeymap fires), and asserts both that the
finalize returns the symlink-refusal error and that the bait
file is untouched.

Author: bootjp

internal/backup/s3.go

@@ -147,8 +147,14 @@ type s3BucketState struct {
 	// every object flushes (the prior orphan-warning path covers it).
 	activeGen uint64
 	objects   map[string]*s3ObjectState // keyed by "object\x00generation"
-	keymap    *KeymapWriter
-	keymapDir string
+	// keymap / keymapFile / keymapDir are lazily set on the first
+	// rename. KeymapWriter.Close only flushes the bufio buffer, so
+	// the *os.File is tracked separately to be closed at finalize —
+	// otherwise a dump that produces keymaps for many buckets
+	// accumulates descriptors until EMFILE (Codex P1 round 9).
+	keymap     *KeymapWriter
+	keymapFile *os.File
+	keymapDir  string
 	// incompleteUploadsJL is opened lazily on the first
 	// !s3|upload|meta or !s3|upload|part record under
 	// --include-incomplete-uploads, then reused for every subsequent
@@ -564,18 +570,27 @@ func (s *S3Encoder) computeDirPrefixes(b *s3BucketState) map[string]bool {
 }
 
 // closeBucketKeymap closes the per-bucket KEYMAP.jsonl writer (if
-// opened) and removes the file when no rename was recorded.
+// opened) and removes the file when no rename was recorded. The
+// *os.File is closed separately because KeymapWriter.Close only
+// flushes its bufio buffer; without explicit fd close, dumps that
+// produce keymaps for many buckets leak descriptors until EMFILE
+// (Codex P1 round 9).
 func closeBucketKeymap(b *s3BucketState) error {
 	if b.keymap == nil {
 		return nil
 	}
-	if err := b.keymap.Close(); err != nil {
-		return err
+	flushErr := b.keymap.Close()
+	var closeErr error
+	if b.keymapFile != nil {
+		closeErr = b.keymapFile.Close()
+	}
+	if flushErr == nil && closeErr != nil {
+		flushErr = errors.WithStack(closeErr)
 	}
 	if b.keymap.Count() == 0 && b.keymapDir != "" {
 		_ = os.Remove(filepath.Join(b.keymapDir, "KEYMAP.jsonl"))
 	}
-	return nil
+	return flushErr
 }
 
 func (s *S3Encoder) flushObjectWithCollision(b *s3BucketState, bucketDir string, obj *s3ObjectState, needsLeafDataRename bool) error {
@@ -744,12 +759,17 @@ func (s *S3Encoder) resolveObjectFilename(b *s3BucketState, obj *s3ObjectState,
 
 func (s *S3Encoder) recordKeymap(b *s3BucketState, bucketDir, encodedFilename string, original []byte, kind string) error {
 	if b.keymap == nil {
-		path := filepath.Join(bucketDir, "KEYMAP.jsonl")
-		f, err := os.Create(path) //nolint:gosec // path composed from output root
+		// openSidecarFile (per-platform) refuses both symlinks and
+		// hard-link clobber attacks. The previous os.Create here
+		// followed both, leaving an arbitrary-write primitive if a
+		// stale prior run or local adversary placed a link at the
+		// path. Codex P2 round 9.
+		f, err := openSidecarFile(filepath.Join(bucketDir, "KEYMAP.jsonl"))
 		if err != nil {
-			return errors.WithStack(err)
+			return err
 		}
 		b.keymap = NewKeymapWriter(f)
+		b.keymapFile = f
 		b.keymapDir = bucketDir
 	}
 	return b.keymap.WriteOriginal(encodedFilename, original, kind)

internal/backup/s3_test.go

@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/bootjp/elastickv/internal/s3keys"
@@ -284,6 +285,40 @@ func TestS3_PathTraversalAttemptRejected(t *testing.T) {
 	}
 }
 
+// TestS3_KeymapRefusesSymlinkAtFinalize is the regression for Codex
+// P2 round 9 on PR #718: the S3 encoder's recordKeymap was using
+// os.Create directly, which follows symlinks. A bucket whose
+// KEYMAP.jsonl path is a pre-existing symlink (from a stale prior
+// run or a local adversary) would have its target truncated when
+// the first rename was recorded. recordKeymap now goes through
+// openSidecarFile, mirroring the redis encoder's guarded open.
+func TestS3_KeymapRefusesSymlinkAtFinalize(t *testing.T) {
+	t.Parallel()
+	enc, root := newS3Encoder(t)
+	bucket := "b"
+	bucketDir := filepath.Join(root, "s3", bucket)
+	if err := os.MkdirAll(bucketDir, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	bait := filepath.Join(root, "bait-keymap")
+	if err := os.WriteFile(bait, []byte("stay-out"), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Symlink(bait, filepath.Join(bucketDir, "KEYMAP.jsonl")); err != nil {
+		t.Fatal(err)
+	}
+	// Drive a meta-suffix-collision rename so recordKeymap fires.
+	enc.WithRenameCollisions(true)
+	emitObject(t, enc, bucket, 1, "evil.elastickv-meta.json", []byte("payload"), "")
+	err := enc.Finalize()
+	if err == nil || !strings.Contains(err.Error(), "refusing to overwrite symlink") {
+		t.Fatalf("expected symlink-refusal error from KEYMAP open, got %v", err)
+	}
+	if got, _ := os.ReadFile(bait); string(got) != "stay-out" { //nolint:gosec // test path
+		t.Fatalf("bait file written through KEYMAP symlink: %q", got)
+	}
+}
+
 // TestS3_BackslashObjectKeyRejected is the regression for Codex P1
 // round 6: filepath.Join treats '\' as a separator on Windows, so
 // keys like `a\..\b` would bypass the '/'-based dot-segment scan