Conversation
Closes #222. `New` chmodded `KeysDir` to `0700` unconditionally. The comment above it says the point is to tighten a directory that arrives too open — but with no read of the current mode it also *widened* a stricter one, so a directory handed over at `0500` came back `0700` after the first start. `docs/key-management.md` tells people to mount that directory read-only, and it holds the Ed25519 private key and the refresh secret, so granting write back unasked is the opposite of what the code claims. `tightenDirMode` now reads the mode and only clears bits: `0755` -> `0700`, `0500` stays `0500`. ### Measured, go1.26.5 The observable effect is the directory mode, so both directions are asserted — a fix that just deleted the chmod would pass the first test while silently dropping the tightening the code exists for. | | Before | After | |---|---|---| | Directory at `0500` | becomes `0700` | stays `0500` | | Directory at `0755` | becomes `0700` | becomes `0700` | The first row is a failing test on `develop`, which is how I confirmed the bug is real rather than a reading of the source. `go build`, `go vet`, `golangci-lint` (0 issues) and `go test -race ./...` all pass. ### Mutation testing, including two that did not bite | Mutation | Result | |---|---| | Remove the `tightenDirMode` call | tightening test fails, as expected | | Remove only the early return | **still passes** | | Change only `chmod(dir, mode&dirMode)` back to `chmod(dir, dirMode)` | **still passes** | | Remove both | loosening test fails, as expected | The two that survive are worth stating rather than hiding: the early return and the mask each independently prevent the loosening, so neither alone is load-bearing. I only trusted the test once the mutation that removes both — which is exactly the code as it stands on `develop` — turned it red. ### Relationship to #221 Same package, different change, so it is a separate branch off `develop`. Whichever merges second may need a trivial rebase; the hunks are in different parts of `New`. Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
Closes #220. The key directory carried three files and nothing saying what wrote them, which made this project's own migrate-forward rule unenforceable: with no marker, a future format change could only guess from the shape of a file or regenerate — and regenerating invalidates every refresh-token and API-key hash a consumer has stored, logging out all of their users. `metadata.json` now records the format version, when the material was first written, and the live signing key's id. No secret in it; the key id already travels in every JWT header. ```json { "format": 1, "created": "2026-07-27T02:41:09Z", "key_id": "3f9a1c07d5b2e846" } ``` ### The decisions, so they are easy to argue with - **A separate file, not a PEM header.** People read those `.pem` files with `openssl`; putting authcore's bookkeeping inside them breaks that expectation. - **Read before anything is parsed or generated.** A directory from a newer authcore is refused while its files are still untouched, rather than after this build has interpreted material in a layout it does not know. - **A corrupt marker fails closed** — a loader that cannot tell what wrote the keys must not guess — but the error names the escape, because the file holds no key material: delete it and the next start re-adopts the keys. The test exercises that escape rather than trusting the sentence. - **No marker means the pre-marker layout**, adopted in place. This is the one migration that has to exist from day one. - **Adoption dates from the private key's mtime**, not from the moment of the upgrade, so a directory that is months old does not claim to have been created by the release that adopted it. - **Writing is never fatal.** A read-only KeysDir is a supported deployment and this is bookkeeping. ### What I measured, go1.26.5 `go build`, `go vet`, `golangci-lint` (0 issues), and `go test -race ./...` all pass; `internal/keymanager` coverage moves 85.7% -> 87.5%. The assertions are that key material *survives*, not that code ran, so I mutated the implementation to check the tests actually bite: | Mutation | Expected | Result | |---|---|---| | Drop the newer-format guard | future-directory test fails | fails | | Rewrite the descriptor on every load | no-churn test fails | fails | | Always stamp `time.Now()` instead of the key mtime | adoption-date test fails | fails | | Make a failed descriptor write fatal | write-failure test fails | fails | That last one is worth a note, because my first version of the test did **not** catch it. I had written it as "chmod the directory to 0500 and expect startup to survive" — but `New` chmods the key directory back to `0700` before writing anything, so the permission bits never survive to the write, and the mutation sailed through green. The permission-bit scenario is unreachable through `New`; the real one is a filesystem that refuses the write, which a missing directory reproduces portably. The test is now white-box against `syncMetadata`, and it fails when the mutation lands. Worth knowing separately: **`New` will loosen a deliberately read-only-by-mode KeysDir to 0700**, which is not what the surrounding comments imply. ### Not in scope `internal/keymanager` is still under the coverage gate at 87.5% — that is #218, unchanged by this beyond the small rise. --------- Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
Jaro-c
added
type:chore
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release PR for v1.11.5.
What changes for a consumer
The key directory is only ever tightened, never loosened (fix(keymanager): tighten the key directory, never loosen it #223, New loosens a deliberately restrictive KeysDir to 0700 #222).
Newused to chmodKeysDirto0700unconditionally, so a directory an operator had deliberately closed to0500came back0700after the first start. It holds the Ed25519 private key and the refresh secret.Honest about the reach: on a genuinely read-only mount — a Kubernetes Secret, which is what
docs/key-management.mdrecommends — the chmod fails and nothing happened. The case that bit is a writable filesystem where the operator set0500themselves.The key directory now records its layout version in
metadata.json(feat(keymanager): record the on-disk key layout version #221, On-disk key material has no format version, so the migrate-forward rule cannot be honoured #220). Nothing changes for anyone today; this is what makes a future format change migratable instead of forcing delete-and-regenerate, which would invalidate every refresh-token and API-key hash a consumer has stored and log out all of their users. Directories written before this release are adopted in place on the next start, keys untouched, and nothing is required of anyone.Patch: no API change, and no behaviour change beyond ceasing to widen permissions.
Verified on go1.26.5
go build,go vet,golangci-lint(0 issues),go test -race ./...across all nine packages, andgovulncheckreporting no vulnerabilities my code calls. Aggregate coverage 91.1%;internal/keymanagermoves 85.7% -> 87.4%.Both changes were mutation-tested, and where a mutation failed to turn a test red I said so in the pull request rather than trusting the green — that is how #222 was found in the first place.