fix(keymanager): parse key files strictly, warn on writable files, and pin the unpinned controls

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

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))
+	}
+}

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
 

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?
 

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
 

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

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(

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 {

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)

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())
+			}
+		})
+	}
+}

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())
+	}
+}