One key per name, not one per ending - #253
Merged
Merged
Conversation
`keyPaths` keyed certificates off the TLD, justified as "that is the granularity the registry stores". It is the opposite of what the registry stores. Migration 009 says so, and says why: Per name rather than per TLD, and that is forced by 008: names under a TLD are sold, so `blue.eggs` can belong to someone who does not own `.eggs`. Hanging keys off the TLD would let its operator publish a key for a name they already sold. A shared per-ending key is that hole in private-key form. The ending's operator holds the key for every name they sold, and every buyer holds a key that signs for every other buyer. 009 closed it at the pin layer; this reintroduced it one layer down, where it is worse — a pin can be withdrawn, a distributed private key cannot. The certificate carried `DNS:*.<tld>` for the same reason, so each buyer's certificate asserted authority over every other name in a namespace they merely bought into. Now `DNS:<name>` alone. Found because it broke nginx on a live box: `moshcode site` wrote conf.d blocks pointing at /etc/ssl/moshpit/hacker.crt while the certificates on disk were per-name, so `nginx -t` failed with three missing files and the box could not reload. Per-name paths agree with what setup-origin.sh has written all along. Path sanitising now keeps dots, so `alt.2600` stays `alt.2600.crt`. Runs of dots collapse to one, so no `..` survives to mean "parent": `../../etc/passwd` becomes `etcpasswd`, a harmless filename inside the key directory. 841 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
vu1nz Security Review0 finding(s) in PR #? No security issues found. |
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
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.
keyPathskeyed certificates off the TLD, justified in its own comment as "that is the granularity the registry stores".That is the opposite of what the registry stores. Migration
009_moshpit_pins.sqlsays so, and says why:Why this matters
A shared per-ending key is that hole in private-key form:
009 closed this at the pin layer. Keying certificates off the TLD reintroduced it one layer down, where it is worse — a pin can be withdrawn, a distributed private key cannot.
The certificate carried
DNS:*.<tld>for the same reason, so each buyer's certificate asserted authority over every other name in a namespace they merely bought into. NowDNS:<name>alone.How it surfaced
It broke nginx on a live box.
moshcode sitewroteconf.dblocks pointing at/etc/ssl/moshpit/hacker.crt, while the certificates on disk are per-name:Nothing went down — nginx kept its last good config — but it could not reload, so a reboot would have taken every site on the box with it. Per-name paths agree with what
setup-origin.shhas been writing all along.Sanitising
Keeping the label means keeping the dot, so the traversal guard had to change rather than just widen:
chovy.hacker→chovy.hacker.crtalt.2600→alt.2600.crt(the old alphanumerics-only strip would have eaten the dot and merged the digits into the label)../../etc/passwd→etcpasswd.key— runs of dots collapse to one, so no..survives to mean "parent"; the result is a harmless filename inside the key directory rather than a path outside itTests
Rewrote the two tests that pinned the old behaviour — including one whose title asserted the wrong invariant — and added coverage for the property that actually matters:
841 pass, 0 fail.
Note for deployment
Boxes that already ran the old
moshcode sitehaveconf.dblocks referencing per-ending paths. Ondev.profullstack.comI moved the three offending files to/root/moshpit-confd-backup/to restorenginx -t;sites-enabledalready served all three names correctly. Anywhere else will need the same, or a re-run after this ships.🤖 Generated with Claude Code