From ac43fab0a615bd0b47f2c47fb779c8cc16c18f5e Mon Sep 17 00:00:00 2001
From: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
Date: Fri, 25 Sep 2026 19:38:53 -0500
Subject: [PATCH 1/3] fix(keymanager): parse key files strictly, warn on
 writable files and directories, and pin the mode, tighten and recovery
 controls

Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
---
 internal/keymanager/fsguard.go                |   6 +-
 internal/keymanager/generate.go               |  49 ++++-
 internal/keymanager/keymanager.go             |  10 +-
 internal/keymanager/load.go                   |   1 +
 internal/keymanager/mode_table_test.go        |  97 ++++++++++
 internal/keymanager/modecheck.go              |  40 +++-
 internal/keymanager/modecheck_test.go         |   2 +-
 internal/keymanager/publish.go                |   6 +-
 .../keymanager/review_round2_internal_test.go | 182 ++++++++++++++++++
 .../keymanager/transaction_branches_test.go   |  34 ++--
 10 files changed, 393 insertions(+), 34 deletions(-)
 create mode 100644 internal/keymanager/mode_table_test.go
 create mode 100644 internal/keymanager/review_round2_internal_test.go

diff --git a/internal/keymanager/fsguard.go b/internal/keymanager/fsguard.go
index c15190c..d85d0ef 100644
--- a/internal/keymanager/fsguard.go
+++ b/internal/keymanager/fsguard.go
@@ -70,8 +70,10 @@ func inspect(dir, name string) (fileState, error) {
 // exists reports whether a managed filename has something the loader must
 // deal with. A dangling symlink is not counted, for the reason inspect gives:
 // it is refused at the write instead. A classification error is reported as
-// present so the caller fails closed rather than generating over an entry it
-// could not read, a symlink loop being the case that is cheap to produce.
+// present, a symlink loop being the case that is cheap to produce. Nothing
+// generates on the strength of this answer: inspectKeySet decides that, and
+// it returns the classification error itself. exists feeds the .gitignore
+// check and the file lists in error messages.
 func exists(dir, name string) bool {
 	state, err := inspect(dir, name)
 	return state != fileAbsent || err != nil
diff --git a/internal/keymanager/generate.go b/internal/keymanager/generate.go
index 4998c39..3a6b704 100755
--- a/internal/keymanager/generate.go
+++ b/internal/keymanager/generate.go
@@ -1,6 +1,7 @@
 package keymanager
 
 import (
+	"bytes"
 	"crypto/ed25519"
 	"crypto/x509"
 	"encoding/hex"
@@ -94,11 +95,11 @@ func readPublicKey(path string) (ed25519.PublicKey, error) {
 // decodeEd25519PrivatePEM parses a PKCS#8 PEM block into an Ed25519 private key.
 // src names the origin (a path or "input") for error messages.
 func decodeEd25519PrivatePEM(data []byte, src string) (ed25519.PrivateKey, error) {
-	block, _ := pem.Decode(data)
-	if block == nil {
-		return nil, fmt.Errorf("no PEM block found in %q", src)
+	der, err := decodePEMBlock(data, "PRIVATE KEY", src)
+	if err != nil {
+		return nil, err
 	}
-	raw, err := x509.ParsePKCS8PrivateKey(block.Bytes)
+	raw, err := x509.ParsePKCS8PrivateKey(der)
 	if err != nil {
 		return nil, fmt.Errorf("parse PKCS#8 private key from %q: %w", src, err)
 	}
@@ -112,11 +113,11 @@ func decodeEd25519PrivatePEM(data []byte, src string) (ed25519.PrivateKey, error
 // decodeEd25519PublicPEM parses a PKIX PEM block into an Ed25519 public key.
 // src names the origin (a path or "input") for error messages.
 func decodeEd25519PublicPEM(data []byte, src string) (ed25519.PublicKey, error) {
-	block, _ := pem.Decode(data)
-	if block == nil {
-		return nil, fmt.Errorf("no PEM block found in %q", src)
+	der, err := decodePEMBlock(data, "PUBLIC KEY", src)
+	if err != nil {
+		return nil, err
 	}
-	raw, err := x509.ParsePKIXPublicKey(block.Bytes)
+	raw, err := x509.ParsePKIXPublicKey(der)
 	if err != nil {
 		return nil, fmt.Errorf("parse PKIX public key from %q: %w", src, err)
 	}
@@ -127,6 +128,34 @@ func decodeEd25519PublicPEM(data []byte, src string) (ed25519.PublicKey, error)
 	return key, nil
 }
 
+// decodePEMBlock returns the DER bytes of the one PEM block in data, which
+// must be labelled wantType, carry no headers, and be the only thing in data
+// apart from whitespace. pem.Decode alone returns the first block wherever
+// it sits and ignores the label, the headers and whatever follows, so a file
+// holding two keys signed with whichever came first, and a PKCS#8 key under a
+// CERTIFICATE label or behind Proc-Type headers was accepted (measured
+// 2026-09-25). Key material has one canonical shape here.
+func decodePEMBlock(data []byte, wantType, src string) ([]byte, error) {
+	trimmed := bytes.TrimSpace(data)
+	if !bytes.HasPrefix(trimmed, []byte("-----BEGIN ")) {
+		return nil, fmt.Errorf("%q does not start with a PEM block", src)
+	}
+	block, rest := pem.Decode(trimmed)
+	if block == nil {
+		return nil, fmt.Errorf("no PEM block found in %q", src)
+	}
+	if block.Type != wantType {
+		return nil, fmt.Errorf("PEM block in %q is labelled %q, want %q", src, block.Type, wantType)
+	}
+	if len(block.Headers) != 0 {
+		return nil, fmt.Errorf("PEM block in %q carries headers; an encrypted or annotated block is not accepted", src)
+	}
+	if len(bytes.TrimSpace(rest)) != 0 {
+		return nil, fmt.Errorf("%q holds more than one PEM block, or text after it", src)
+	}
+	return block.Bytes, nil
+}
+
 // ----- Refresh secret ---------------------------------------------------------
 
 // loadRefreshSecret reads, validates, and hex-decodes the secret file.
@@ -138,7 +167,9 @@ func loadRefreshSecret(path string) ([]byte, error) {
 	hexStr := strings.TrimSpace(string(data))
 	secret, err := hex.DecodeString(hexStr)
 	if err != nil {
-		return nil, fmt.Errorf("decode refresh secret in %q: %w", path, err)
+		// Not %w: the hex error quotes the offending byte, which is a byte
+		// of the secret, and this message ends up in a log.
+		return nil, fmt.Errorf("refresh secret in %q is not hex encoded", path)
 	}
 	if len(secret) != refreshSecretLen {
 		return nil, fmt.Errorf(
diff --git a/internal/keymanager/keymanager.go b/internal/keymanager/keymanager.go
index 8741247..399bb4c 100755
--- a/internal/keymanager/keymanager.go
+++ b/internal/keymanager/keymanager.go
@@ -226,7 +226,15 @@ func newByStaging(dir string, meta *metadata, log logger) (*KeyManager, error) {
 	// may have created KeysDir without syncing its parent entry yet, and
 	// this process is about to publish keys that other processes will use.
 	if err := syncDir(filepath.Dir(dir)); err != nil {
-		return nil, fmt.Errorf("sync parent of keys directory: %w", err)
+		// A parent with search but no read permission (0711, 0311) cannot be
+		// opened for fsync, and nothing here needs to read it. The load path
+		// treats the same sync as best effort; so does this one since
+		// 2026-09-25. Any other failure still stops the first run.
+		if errors.Is(err, fs.ErrPermission) {
+			log.Warn("authcore/keymanager: could not sync the parent of %q (continuing): %v", dir, err)
+		} else {
+			return nil, fmt.Errorf("sync parent of keys directory: %w", err)
+		}
 	}
 	staging, priv, pub, secret, err := createStagingSet(dir)
 	if err != nil {
diff --git a/internal/keymanager/load.go b/internal/keymanager/load.go
index b4ab084..1426578 100644
--- a/internal/keymanager/load.go
+++ b/internal/keymanager/load.go
@@ -101,6 +101,7 @@ func Load(dir string, log logger) (*KeyManager, error) {
 	// not a finding.
 	warnIfReadableByOthers(privPath, log)
 	warnIfReadableByOthers(secretPath, log)
+	warnIfDirWritableByOthers(dir, log)
 
 	keyID := computeKeyID(pub)
 	reportLeftovers(dir, log)
diff --git a/internal/keymanager/mode_table_test.go b/internal/keymanager/mode_table_test.go
new file mode 100644
index 0000000..ab1438c
--- /dev/null
+++ b/internal/keymanager/mode_table_test.go
@@ -0,0 +1,97 @@
+package keymanager_test
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+	"runtime"
+	"strings"
+	"testing"
+
+	"github.com/Glyndor/authcore/internal/keymanager"
+)
+
+// The permission warning on both load paths, for both secret files, at every
+// mode that opens the file to others. Before 2026-09-25 the suite pinned the
+// private key on New and the refresh secret on Load only, and read bits only:
+// the crossed pairs and the write-only modes (0602, 0620) loaded silently.
+func TestModeWarn_everyPathEveryFileEveryOpenMode(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("Unix permission bits do not apply on Windows")
+	}
+	paths := map[string]func(string, *modeWarnLogger) error{
+		"New":  func(dir string, l *modeWarnLogger) error { _, err := keymanager.New(dir, l); return err },
+		"Load": func(dir string, l *modeWarnLogger) error { _, err := keymanager.Load(dir, l); return err },
+	}
+	for pathName, open := range paths {
+		for _, file := range []string{"ed25519_private.pem", "refresh_secret.key"} {
+			for _, mode := range []os.FileMode{0o640, 0o604, 0o602, 0o620, 0o600, 0o400} {
+				t.Run(fmt.Sprintf("%s/%s/%04o", pathName, file, mode), func(t *testing.T) {
+					dir := seededDir(t)
+					target := filepath.Join(dir, file)
+					if err := os.Chmod(target, mode); err != nil {
+						t.Fatal(err)
+					}
+					capture := &modeWarnLogger{}
+					if err := open(dir, capture); err != nil {
+						t.Fatalf("%s: %v", pathName, err)
+					}
+					want := 0
+					if mode&0o066 != 0 {
+						want = 1
+					}
+					got := capture.countModeWarns()
+					if got != want {
+						t.Fatalf("mode warns = %d, want %d; entries: %q", got, want, capture.modeWarnEntries())
+					}
+					if want == 1 && !strings.Contains(capture.modeWarnEntries()[0], target) {
+						t.Fatalf("the warning does not name %s: %q", target, capture.modeWarnEntries())
+					}
+				})
+			}
+		}
+	}
+}
+
+// Load warns when KeysDir itself can be written by others without the sticky
+// bit, which is where a key file could be replaced. New tightens the
+// directory instead; Load never touches it, and said nothing until
+// 2026-09-25. A sticky 1777 directory is a Kubernetes Secret mount and is
+// not reported.
+func TestModeWarn_LoadReportsAWritableKeysDir(t *testing.T) {
+	if runtime.GOOS == "windows" || os.Geteuid() == 0 {
+		t.Skip("needs Unix mode bits and a non-root user")
+	}
+	for mode, want := range map[os.FileMode]int{
+		0o777:                 1,
+		0o733:                 1,
+		0o700 | os.ModeSticky: 0,
+		0o777 | os.ModeSticky: 0,
+		0o700:                 0,
+		0o750:                 0,
+	} {
+		t.Run(fmt.Sprintf("%04o", mode), func(t *testing.T) {
+			dir := seededDir(t)
+			if err := os.Chmod(dir, mode); err != nil {
+				t.Fatal(err)
+			}
+			t.Cleanup(func() { _ = os.Chmod(dir, 0o700) })
+			capture := &modeWarnLogger{}
+			if _, err := keymanager.Load(dir, capture); err != nil {
+				t.Fatalf("Load: %v", err)
+			}
+			got := 0
+			for _, w := range capture.modeWarnEntries() {
+				if strings.Contains(w, "key directory") && strings.Contains(w, "writable by group or others") {
+					got++
+				}
+			}
+			if got != want {
+				t.Fatalf("directory warns = %d, want %d; entries: %q", got, want, capture.modeWarnEntries())
+			}
+			if fi, _ := os.Stat(dir); fi.Mode().Perm() != mode.Perm() {
+				t.Fatalf("Load changed the directory mode to %04o", fi.Mode().Perm())
+			}
+		})
+	}
+}
diff --git a/internal/keymanager/modecheck.go b/internal/keymanager/modecheck.go
index a90b6b4..c0b1902 100644
--- a/internal/keymanager/modecheck.go
+++ b/internal/keymanager/modecheck.go
@@ -15,9 +15,12 @@ import (
 	"runtime"
 )
 
-// warnIfReadableByOthers logs a single Warn when path is readable by group or
-// others. The check is a no-op on Windows, returns silently when path cannot be
-// statted, and never changes the file's mode.
+// warnIfReadableByOthers logs a single Warn when path is readable or writable
+// by group or others. The check is a no-op on Windows, returns silently when
+// path cannot be statted, and never changes the file's mode. A writable secret
+// is the worse case: until 2026-09-25 a refresh_secret.key at 0602 loaded
+// silently while 0644 warned, and a file others can replace is a file whose
+// contents are theirs.
 //
 // It is called only for the private key and the refresh secret on the load
 // paths; files authcore wrote itself at 0600 are skipped so a generation run
@@ -38,14 +41,37 @@ func warnIfReadableByOthers(path string, log logger) {
 		return
 	}
 
-	// 0o044 is the group-read and other-read bits. The Podman default (0444)
-	// and the Kubernetes default (0644) both set them.
-	if fi.Mode().Perm()&0o044 != 0 {
+	// 0o066 is the group and other read and write bits. The Podman default
+	// (0444) and the Kubernetes default (0644) both set the read ones.
+	if fi.Mode().Perm()&0o066 != 0 {
 		log.Warn(
-			"authcore/keymanager: %s is readable by group or others "+
+			"authcore/keymanager: %s is readable or writable by group or others "+
 				"(mode %04o); restrict it to the process that uses it, "+
 				"for example mode 0400 or 0600, or set mode and uid on the "+
 				"Podman or Kubernetes secret",
 			path, fi.Mode().Perm())
 	}
 }
+
+// warnIfDirWritableByOthers logs a single Warn when dir can be written by
+// group or others without the sticky bit, which is where a key file could be
+// replaced. New tightens KeysDir to 0700; Load, the production path, never
+// touched the directory and said nothing about it until 2026-09-25. A sticky
+// 1777 directory is what Kubernetes mounts a Secret under, so it is not
+// reported.
+func warnIfDirWritableByOthers(dir string, log logger) {
+	if runtime.GOOS == "windows" {
+		return
+	}
+	fi, err := os.Stat(dir)
+	if err != nil {
+		return
+	}
+	if fi.Mode().Perm()&0o022 != 0 && fi.Mode()&os.ModeSticky == 0 {
+		log.Warn(
+			"authcore/keymanager: key directory %s is writable by group or others "+
+				"(mode %04o); anyone with that access can replace the key files. "+
+				"Restrict it to the owner, for example mode 0700",
+			dir, fi.Mode().Perm())
+	}
+}
diff --git a/internal/keymanager/modecheck_test.go b/internal/keymanager/modecheck_test.go
index 82d4383..52840c7 100644
--- a/internal/keymanager/modecheck_test.go
+++ b/internal/keymanager/modecheck_test.go
@@ -23,7 +23,7 @@ import (
 // modeWarnTag is the marker the warn helper prefixes its message with. Tests
 // scope their counts to it so an unrelated Warn (gitignore, metadata, leftover
 // staging) does not pollute the assertion.
-const modeWarnTag = "readable by group or others"
+const modeWarnTag = "by group or others"
 
 // modeWarnLogger captures Warn calls so a test can assert on what was logged.
 // It mirrors the recorder that lives in the internal test helpers and only
diff --git a/internal/keymanager/publish.go b/internal/keymanager/publish.go
index 9af0960..d692009 100644
--- a/internal/keymanager/publish.go
+++ b/internal/keymanager/publish.go
@@ -50,9 +50,9 @@ func waitTimeoutError(dir string) error {
 		dir, presentStr, missingStr)
 }
 
-// presentAndMissing lists the three key filenames by presence in dir. A
-// classification error (symlink loop, hostile entry) is reported as present
-// so the caller fails closed rather than generating over an unreadable entry.
+// presentAndMissing lists the three key filenames by presence in dir, for
+// error messages. A classification error (symlink loop, hostile entry) counts
+// as present, so the message names the entry the operator has to look at.
 func presentAndMissing(dir string) (present, missing []string) {
 	for _, name := range []string{filePrivateKey, filePublicKey, fileRefreshSecret} {
 		if exists(dir, name) {
diff --git a/internal/keymanager/review_round2_internal_test.go b/internal/keymanager/review_round2_internal_test.go
new file mode 100644
index 0000000..ba4f0d1
--- /dev/null
+++ b/internal/keymanager/review_round2_internal_test.go
@@ -0,0 +1,182 @@
+package keymanager
+
+import (
+	"crypto/ed25519"
+	"crypto/rand"
+	"crypto/x509"
+	"encoding/hex"
+	"encoding/pem"
+	"os"
+	"path/filepath"
+	"runtime"
+	"strings"
+	"testing"
+)
+
+// Controls the 2026-09-25 review found working but unpinned, plus the two
+// parsers it found lenient. Each test names the mutation that used to
+// survive: without it, the control could be removed with the suite green.
+
+// New only ever tightens KeysDir. os.Chmod(dir, dirMode) instead of
+// mode&dirMode survived: a 0555 directory gained owner write.
+func TestTightenDirMode_onlyEverTightens(t *testing.T) {
+	if runtime.GOOS == "windows" || os.Geteuid() == 0 {
+		t.Skip("needs Unix mode bits and a non-root user")
+	}
+	for _, tc := range []struct{ before, after os.FileMode }{
+		{0o555, 0o500}, // strips group and other, never adds owner write
+		{0o750, 0o700},
+		{0o500, 0o500},
+	} {
+		dir := t.TempDir()
+		if _, err := New(dir, silentLog{}); err != nil {
+			t.Fatalf("provision: %v", err)
+		}
+		if err := os.Chmod(dir, tc.before); err != nil {
+			t.Fatal(err)
+		}
+		if _, err := New(dir, silentLog{}); err != nil {
+			t.Fatalf("New on a %04o directory: %v", tc.before, err)
+		}
+		fi, err := os.Stat(dir)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if fi.Mode().Perm() != tc.after {
+			t.Errorf("KeysDir %04o after New is %04o, want %04o", tc.before, fi.Mode().Perm(), tc.after)
+		}
+		_ = os.Chmod(dir, 0o700)
+	}
+}
+
+// Recovery picks the staging directory whose private key matches the
+// published one, whichever sorts first. With the comparison always true the
+// first directory in name order was used, and an unrelated leftover that
+// sorted first turned a recoverable state into a refusal.
+func TestRecovery_picksTheStagingThatMatchesEvenWhenItSortsLast(t *testing.T) {
+	dir := t.TempDir()
+	unrelated, _, _, _, err := createStagingSet(dir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	matching, _, pub, _, err := createStagingSet(dir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	first := filepath.Join(dir, stagingPrefix+"0000000000000000")
+	last := filepath.Join(dir, stagingPrefix+"ffffffffffffffff")
+	if err := os.Rename(unrelated, first); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Rename(matching, last); err != nil {
+		t.Fatal(err)
+	}
+	// An interrupted publication: the private key is linked, the rest is not.
+	if err := os.Link(filepath.Join(last, filePrivateKey), filepath.Join(dir, filePrivateKey)); err != nil {
+		t.Fatal(err)
+	}
+
+	km, err := New(dir, silentLog{})
+	if err != nil {
+		t.Fatalf("New on the interrupted publication: %v, want it recovered from the matching staging", err)
+	}
+	if want := computeKeyID(pub); km.KeyID() != want {
+		t.Fatalf("recovered key id %q, want %q from the matching staging", km.KeyID(), want)
+	}
+	if !km.PublicKey().Equal(pub) {
+		t.Fatal("the recovered public key is not the one whose private key was published")
+	}
+}
+
+func pemPair(t *testing.T) (privPEM, pubPEM []byte, priv ed25519.PrivateKey) {
+	t.Helper()
+	pub, priv, err := ed25519.GenerateKey(rand.Reader)
+	if err != nil {
+		t.Fatal(err)
+	}
+	privDER, err := x509.MarshalPKCS8PrivateKey(priv)
+	if err != nil {
+		t.Fatal(err)
+	}
+	pubDER, err := x509.MarshalPKIXPublicKey(pub)
+	if err != nil {
+		t.Fatal(err)
+	}
+	return pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Bytes: privDER}),
+		pem.EncodeToMemory(&pem.Block{Type: "PUBLIC KEY", Bytes: pubDER}), priv
+}
+
+// The PEM decoders accept one block, of the expected type, with no headers
+// and nothing else in the input. pem.Decode alone returned the first block
+// wherever it sat and ignored the rest.
+func TestPEMDecoders_acceptOneCanonicalBlockOnly(t *testing.T) {
+	privPEM, pubPEM, _ := pemPair(t)
+	otherPriv, _, _ := pemPair(t)
+	secret := []byte(strings.Repeat("k", 32))
+
+	if _, err := FromPEM(privPEM, pubPEM, secret); err != nil {
+		t.Fatalf("the canonical pair: %v, want accepted", err)
+	}
+	// Leading and trailing whitespace is what an editor or a secret manager
+	// adds, not a second block.
+	if _, err := FromPEM(append([]byte("\n"), privPEM...), append(pubPEM, '\n', '\n'), secret); err != nil {
+		t.Fatalf("whitespace around the blocks: %v, want accepted", err)
+	}
+
+	block, _ := pem.Decode(privPEM)
+	relabelled := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: block.Bytes})
+	withHeaders := pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Headers: map[string]string{"Proc-Type": "4,ENCRYPTED"}, Bytes: block.Bytes})
+	for name, tc := range map[string]struct {
+		privPEM []byte
+		reason  string
+	}{
+		"two private blocks":      {append(append([]byte{}, privPEM...), otherPriv...), "more than one PEM block"},
+		"text before the block":   {append([]byte("Bag Attributes\n    friendlyName: k\n"), privPEM...), "does not start with a PEM block"},
+		"text after the block":    {append(append([]byte{}, privPEM...), []byte("trailing note\n")...), "more than one PEM block, or text after it"},
+		"CERTIFICATE label":       {relabelled, `labelled "CERTIFICATE", want "PRIVATE KEY"`},
+		"encryption headers":      {withHeaders, "carries headers"},
+		"public key as private":   {pubPEM, `labelled "PUBLIC KEY", want "PRIVATE KEY"`},
+		"BEGIN line and no block": {[]byte("-----BEGIN PRIVATE KEY-----\nnot a block\n"), "no PEM block found"},
+	} {
+		t.Run(name, func(t *testing.T) {
+			_, err := FromPEM(tc.privPEM, pubPEM, secret)
+			if err == nil || !strings.Contains(err.Error(), tc.reason) {
+				t.Fatalf("FromPEM = %v, want a refusal naming %q", err, tc.reason)
+			}
+		})
+	}
+	if _, err := FromPEM(privPEM, privPEM, secret); err == nil || !strings.Contains(err.Error(), `labelled "PRIVATE KEY", want "PUBLIC KEY"`) {
+		t.Fatalf("a private key in the public slot: %v, want the label refusal", err)
+	}
+}
+
+// The error for a refresh secret that is not hex names the file and nothing
+// of its contents; the hex package's own error quotes the offending byte.
+func TestRefreshSecretErrorDoesNotEchoTheFile(t *testing.T) {
+	dir := t.TempDir()
+	if _, err := New(dir, silentLog{}); err != nil {
+		t.Fatal(err)
+	}
+	bad := "s3cr3t-value-that-is-not-hex-encoded-at-all-and-must-not-be-logged!\n"
+	if err := os.WriteFile(filepath.Join(dir, fileRefreshSecret), []byte(bad), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	_, err := Load(dir, silentLog{})
+	if err == nil || !strings.Contains(err.Error(), "is not hex encoded") {
+		t.Fatalf("Load = %v, want the not-hex refusal", err)
+	}
+	for _, fragment := range []string{"s3cr3t", "invalid byte", "U+", "'s'"} {
+		if strings.Contains(err.Error(), fragment) {
+			t.Errorf("the error echoes the file (%q): %v", fragment, err)
+		}
+	}
+	// A well-formed file of the same length still loads, so the refusal is
+	// about the encoding and not the length.
+	good := hex.EncodeToString([]byte(strings.Repeat("k", 32))) + "\n"
+	if err := os.WriteFile(filepath.Join(dir, fileRefreshSecret), []byte(good), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := Load(dir, silentLog{}); err != nil {
+		t.Fatalf("Load with a hex secret: %v", err)
+	}
+}
diff --git a/internal/keymanager/transaction_branches_test.go b/internal/keymanager/transaction_branches_test.go
index 296b3fe..354893d 100644
--- a/internal/keymanager/transaction_branches_test.go
+++ b/internal/keymanager/transaction_branches_test.go
@@ -547,12 +547,13 @@ func TestReportLeftoversWarnsOnIncompleteStaging(t *testing.T) {
 	}
 }
 
-// New's "sync parent of keys directory failed" branch. Make the parent of
-// an empty KeysDir unreadable so syncDir(parent) returns an error; assert
-// New returns that error and that no key file was published. Skip on
+// A parent with search but no read permission cannot be opened for the
+// fsync before the first publish. Nothing needs to read it, so New warns and
+// publishes; until 2026-09-25 it refused with "sync parent of keys
+// directory" while the same layout loaded an existing set fine. Skip on
 // Windows, and when running as root, because root ignores directory mode
 // bits and the chmod would not block the Open call.
-func TestNewSyncsParentBeforePublishing(t *testing.T) {
+func TestNewWarnsWhenTheParentCannotBeSynced(t *testing.T) {
 	if runtime.GOOS == "windows" {
 		t.Skip("Unix permission bits do not apply on Windows")
 	}
@@ -572,16 +573,27 @@ func TestNewSyncsParentBeforePublishing(t *testing.T) {
 	}
 
 	dir := filepath.Join(parent, "keys")
-	_, err := New(dir, silentLog{})
-	if err == nil {
-		t.Fatal("New succeeded on a directory under a parent the process cannot search")
+	log := &captureLogger{}
+	first, err := New(dir, log)
+	if err != nil {
+		t.Fatalf("New under an unreadable parent: %v, want the keys published with a warning", err)
+	}
+	warned := false
+	for _, w := range log.warnings {
+		if strings.Contains(w, "could not sync the parent") {
+			warned = true
+		}
 	}
-	if !strings.Contains(err.Error(), "sync parent of keys directory") {
-		t.Errorf("error must come from the parent sync, got: %v", err)
+	if !warned {
+		t.Errorf("no warning about the parent sync; warnings: %q", log.warnings)
 	}
 	for _, name := range []string{filePrivateKey, filePublicKey, fileRefreshSecret} {
-		if _, err := os.Stat(filepath.Join(dir, name)); !os.IsNotExist(err) {
-			t.Errorf("%s should not have been published, stat gave: %v", name, err)
+		if _, err := os.Stat(filepath.Join(dir, name)); err != nil {
+			t.Errorf("%s was not published: %v", name, err)
 		}
 	}
+	again, err := New(dir, silentLog{})
+	if err != nil || again.KeyID() != first.KeyID() {
+		t.Fatalf("second New = %v, key id %q; want the published set, %q", err, again.KeyID(), first.KeyID())
+	}
 }

From 4510438b3bfdd19ddefda37f400189a9fdb7f91b Mon Sep 17 00:00:00 2001
From: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
Date: Fri, 25 Sep 2026 19:38:53 -0500
Subject: [PATCH 2/3] fix: refuse an empty KeyID from a KeyStore and a previous
 key with the current key's id

Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
---
 auth/jwt/jwt.go               | 12 +++++--
 auth/jwt/previous_key_test.go | 31 +++++++++++++++++
 docs/key-management.md        | 13 ++++++--
 keystore.go                   |  7 ++++
 keystore_keyid_test.go        | 63 +++++++++++++++++++++++++++++++++++
 module.go                     | 11 +++---
 6 files changed, 128 insertions(+), 9 deletions(-)
 create mode 100644 auth/jwt/previous_key_test.go
 create mode 100644 keystore_keyid_test.go

diff --git a/auth/jwt/jwt.go b/auth/jwt/jwt.go
index a9dfd20..5fae1f5 100755
--- a/auth/jwt/jwt.go
+++ b/auth/jwt/jwt.go
@@ -170,8 +170,16 @@ func New[T any](p authcore.Provider, cfg ...Config) (*JWT[T], error) {
 	// Build the verification key set: the current key plus any previous public
 	// keys still in their rotation overlap. Signing always uses the current key.
 	j.verifyKeys = map[string]ed25519.PublicKey{j.kid: j.pub}
-	for _, prev := range resolved.PreviousPublicKeys {
-		j.verifyKeys[keymanager.KeyID(prev)] = prev
+	for i, prev := range resolved.PreviousPublicKeys {
+		kid := keymanager.KeyID(prev)
+		// A previous key registered under the current kid replaced the
+		// current key in this map, and every token the module then issued
+		// failed its own verification (measured 2026-09-25 with a KeyStore
+		// reporting a stale kid). Refuse it at startup instead.
+		if kid == j.kid {
+			return nil, fmt.Errorf("%w: previous public key %d has the current signing key's id %q", ErrInvalidConfig, i, kid)
+		}
+		j.verifyKeys[kid] = prev
 	}
 
 	j.initialised = true
diff --git a/auth/jwt/previous_key_test.go b/auth/jwt/previous_key_test.go
new file mode 100644
index 0000000..89fb0ef
--- /dev/null
+++ b/auth/jwt/previous_key_test.go
@@ -0,0 +1,31 @@
+package jwt
+
+import (
+	"crypto/ed25519"
+	"errors"
+	"strings"
+	"testing"
+)
+
+// A previous key with the current key's id would replace the current key in
+// the verification set, and every token then issued failed its own
+// verification (measured 2026-09-25 with a KeyStore reporting a stale kid).
+// New refuses it; a different previous key is registered as before.
+func TestNew_refusesAPreviousKeyWithTheCurrentKeyID(t *testing.T) {
+	p := newFakeProvider(t)
+	cfg := DefaultConfig()
+	cfg.PreviousPublicKeys = []ed25519.PublicKey{p.Keys().PublicKey()}
+	_, err := New[struct{}](p, cfg)
+	if !errors.Is(err, ErrInvalidConfig) || !strings.Contains(err.Error(), "has the current signing key's id") {
+		t.Fatalf("New with the current key listed as previous = %v, want ErrInvalidConfig naming it", err)
+	}
+
+	cfg.PreviousPublicKeys = []ed25519.PublicKey{newFakeProvider(t).Keys().PublicKey()}
+	j, err := New[struct{}](p, cfg)
+	if err != nil {
+		t.Fatalf("New with a different previous key: %v", err)
+	}
+	if len(j.verifyKeys) != 2 {
+		t.Fatalf("%d verification keys, want 2", len(j.verifyKeys))
+	}
+}
diff --git a/docs/key-management.md b/docs/key-management.md
index bbed376..0aa2cd4 100644
--- a/docs/key-management.md
+++ b/docs/key-management.md
@@ -238,9 +238,16 @@ them to `NewKeyStoreFromKeys` or `NewKeyStoreFromPEM`, and to write a custom
 > the material from a secret manager / KMS via a `KeyStore` instead of leaving it
 > in plaintext on disk.
 
-The `KeyID()` accessor returns a 16-character hex digest derived from the public
-key. It is embedded in every token's `kid` JOSE header. Verification selects the
-key by `kid` and rejects any token whose `kid` is not one the module accepts.
+The `KeyID()` accessor returns the identifier of the signing key. It is embedded
+in every token's `kid` JOSE header; verification selects the key by `kid` and
+rejects any token whose `kid` is not one the module accepts. The built-in
+stores derive it from the public key as a 16-character hex digest. A custom
+`Keys` must return a non-empty value that stays the same for the same key
+across restarts (`New` refuses an empty one), and must not reuse the id of a
+key listed in `jwt.Config.PreviousPublicKeys`: `jwt.New` refuses a previous
+key whose id equals the current one, because registering it would replace the
+current key in the verification set and every token then issued would fail
+its own verification.
 
 ## The refresh secret protects credentials and encrypted fields
 
diff --git a/keystore.go b/keystore.go
index 376bf1d..cd3d3a5 100644
--- a/keystore.go
+++ b/keystore.go
@@ -64,6 +64,13 @@ func validateLoadedKeys(keys Keys) error {
 	if err := keymanager.ValidateMaterial(keys.PrivateKey(), keys.PublicKey(), keys.RefreshSecret()); err != nil {
 		return fmt.Errorf("KeyStore.Load returned unusable key material: %w", err)
 	}
+	// The kid goes into every token header and selects the verification key.
+	// An empty one registered the current key under "", so a token with no
+	// kid header verified against it, contrary to what auth/jwt promises.
+	if keys.KeyID() == "" {
+		return errors.New("KeyStore.Load returned Keys with an empty KeyID; " +
+			"return a stable, non-empty identifier for the signing key")
+	}
 	return nil
 }
 
diff --git a/keystore_keyid_test.go b/keystore_keyid_test.go
new file mode 100644
index 0000000..24d2f11
--- /dev/null
+++ b/keystore_keyid_test.go
@@ -0,0 +1,63 @@
+package authcore_test
+
+import (
+	"bytes"
+	"crypto/ed25519"
+	"errors"
+	"strings"
+	"testing"
+
+	"github.com/Glyndor/authcore"
+)
+
+// relabelStore hands out the material of a real store under a KeyID of the
+// test's choosing, which is what a custom Keys can do.
+type relabelStore struct {
+	inner authcore.KeyStore
+	id    string
+}
+
+type relabelledKeys struct {
+	authcore.Keys
+	id string
+}
+
+func (k relabelledKeys) KeyID() string { return k.id }
+
+func (s relabelStore) Load() (authcore.Keys, error) {
+	keys, err := s.inner.Load()
+	if err != nil {
+		return nil, err
+	}
+	return relabelledKeys{keys, s.id}, nil
+}
+
+// New refuses a KeyStore whose Keys report an empty KeyID: the current key
+// was registered under "" and a token with no kid header verified against
+// it. A custom non-empty id is accepted, since the derivation is the store's
+// business.
+func TestNew_refusesAnEmptyKeyID(t *testing.T) {
+	seed := bytes.Repeat([]byte{0x42}, ed25519.SeedSize)
+	priv := ed25519.NewKeyFromSeed(seed)
+	inner, err := authcore.NewKeyStoreFromKeys(priv, priv.Public().(ed25519.PublicKey), bytes.Repeat([]byte{0x11}, 32))
+	if err != nil {
+		t.Fatal(err)
+	}
+	cfg := authcore.DefaultConfig()
+	cfg.EnableLogs = false
+
+	cfg.KeyStore = relabelStore{inner, ""}
+	_, err = authcore.New(cfg)
+	if !errors.Is(err, authcore.ErrKeyManager) || !strings.Contains(err.Error(), "empty KeyID") {
+		t.Fatalf("New with an empty KeyID = %v, want ErrKeyManager naming it", err)
+	}
+
+	cfg.KeyStore = relabelStore{inner, "release-2026-09"}
+	ac, err := authcore.New(cfg)
+	if err != nil {
+		t.Fatalf("New with a custom KeyID: %v, want accepted", err)
+	}
+	if ac.Keys().KeyID() != "release-2026-09" {
+		t.Fatalf("KeyID = %q, want the store's", ac.Keys().KeyID())
+	}
+}
diff --git a/module.go b/module.go
index 644231e..3d7616d 100755
--- a/module.go
+++ b/module.go
@@ -21,10 +21,13 @@ type Keys interface {
 	// The caller must not modify the returned slice.
 	RefreshSecret() []byte
 
-	// KeyID returns the stable identifier for the current signing key.
-	// It is derived from the public key and embedded in the "kid" JOSE header
-	// of every issued token so that verifiers can select the correct key when
-	// multiple keys are in circulation (e.g. during key rotation).
+	// KeyID returns the stable, non-empty identifier for the current signing
+	// key. It is embedded in the "kid" JOSE header of every issued token so
+	// that verifiers can select the correct key when multiple keys are in
+	// circulation (e.g. during key rotation). The built-in stores derive it
+	// from the public key; a custom Keys must return the same value for the
+	// same key on every start, or tokens issued before a restart stop
+	// verifying.
 	KeyID() string
 }
 

From f2bbaf7b818cf0a72fa8ca03f8fde4daa0e2b08b Mon Sep 17 00:00:00 2001
From: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
Date: Fri, 25 Sep 2026 19:38:53 -0500
Subject: [PATCH 3/3] docs: stop advising to delete the key directory, and
 describe concurrent first start as it works

Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
---
 docs/containers.md | 17 ++++++++++-------
 docs/faq.md        | 10 +++++++---
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/docs/containers.md b/docs/containers.md
index b2c7001..a94bd56 100644
--- a/docs/containers.md
+++ b/docs/containers.md
@@ -157,13 +157,16 @@ else
 fi
 ```
 
-Concurrent first start is the trap this whole pattern avoids. Eight containers
-on one empty named volume, started at once against an unprepared volume, hit
-`refusing to write "/keys/ed25519_private.pem": something already exists
-there` on 43 of 80 starts in one batch and 51 of 80 in another. The containers
-that lost exited with that error; started again, they loaded the winner's keys,
-and no two containers ever held different keys. Concurrent first start is being
-reworked. Until then, create the keys **once** before starting replicas.
+Concurrent first start works since v1.14.0, and the pattern above is still
+the one to use. Several replicas started at once on one empty volume race to
+publish a key set; the loser of the race waits for the winner's set and loads
+it, so every replica ends up with the same keys. Measured on 2026-09-25 with
+separate processes: 180 starts without the race detector and 900 with it, no
+failure and no two processes holding different keys. Before v1.14.0 the
+losers exited with `refusing to write "/keys/ed25519_private.pem": something
+already exists there`, 43 and 51 of 80 starts in two batches. Creating the
+keys once beforehand is still simpler to reason about, and it is the only
+way to get a load-only deployment.
 
 ## Replicas
 
diff --git a/docs/faq.md b/docs/faq.md
index 70fb13b..a44ea82 100644
--- a/docs/faq.md
+++ b/docs/faq.md
@@ -45,9 +45,13 @@ authcore could not read or create its key files. Check that:
 
 1. `KeysDir` (default `.authcore`) is writable by the process.
 2. The directory is not a read-only filesystem (common in some container setups).
-3. Existing key files are not corrupted — delete `.authcore` and let authcore
-   regenerate them. **Warning:** regenerating keys invalidates every token
-   currently in circulation.
+3. The key files are the ones you provisioned, all three of them. Restore a
+   missing or damaged file from a backup. Do not delete the directory to make
+   authcore regenerate: a new `refresh_secret.key` invalidates every stored
+   refresh-token, API-key and recovery-code hash and makes every `auth/field`
+   encrypted column unreadable for good (`docs/key-management.md`). authcore
+   refuses to regenerate over a directory whose `metadata.json` records a key
+   set, for the same reason.
 
 ## Can I verify tokens issued before I rotated my signing key?