fix: validate release keys and smoke-run the genesis build on PRs - #254
fix: validate release keys and smoke-run the genesis build on PRs#254mordamax wants to merge 3 commits into
Conversation
CI Summary
Deploy ContractsDeployed addresses vs the committed manifestExpected is the committed manifest; actual is this CI deployment of the same pipeline.
Labelsother |
re-gius
left a comment
re-gius
left a comment
There was a problem hiding this comment.
Overall the preflight and the mnemonic removal look good to me.
However, I am not convinced of putting FACTORY_DEPLOYER_KEY in reach of every PR that touches scripts/genesis/**, since it's the same key dotns-releases uses and it can't be cheaply rotated
I would rather have it in an environment with required reviewers, or the pull_request trigger dropped in favour of push: master.
| jobs: | ||
| genesis-build-smoke: | ||
| if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
nit: add timeout-minutes: 45 or similar
There was a problem hiding this comment.
Moreover, we should add
env:
DOTNS_TLD: testnet
as from #255 and always reference ${{ env.DOTNS_TLD }} below
| # cut. The extractor's unit tests and `bash -n` cannot catch either — v0.5.6-rc1 died on a | ||
| # key `cast` could not decode, after every PR check had passed. | ||
| # | ||
| # Skipped on fork PRs: secrets are withheld there. |
There was a problem hiding this comment.
This hands a production key to any PR author, e.g. through a PR that edits build-genesis.sh to extract it. Moreover, this key is expensive to rotate since every DotNS address derives from its hash.
Instead, we could create a GitHub deployment environment (e.g., genesis-smoke) in this repository with a "Required reviewers" protection rule, so that only named reviewers can approve it, and move FACTORY_DEPLOYER_KEY to its environment secrets.
Otherwise, drop the pull_request trigger entirely and keep push: master
| push: | ||
| branches: [master] | ||
| paths: | ||
| - "scripts/genesis/**" |
There was a problem hiding this comment.
We should add some others here:
- "scripts/deploy/**" the stages the genesis is built from
- "deployments/**" what the check asserts against
- "bun.lock" since it may break the build
- "package.json" updates may break the build
- ".github/workflows/publish-release.yml"
- ".github/workflows/publish-prerelease.yml"
| # build would guess which account owns everything, and nothing downstream would | ||
| # notice a wrong guess — the parity check validates addresses, not owners. | ||
| # Not DOTNS_MNEMONIC (the whitelist workflows' operational credential) and not | ||
| # DEPLOYER_KEY (dotns-releases' own secret) for the same reason. |
There was a problem hiding this comment.
nit: why the same reason? I don't get it
| # ---- Key shape preflight ---- | ||
| # `cast` rejects a malformed key with a bare "Failed to decode private key" — no variable | ||
| # name, no shape — and a GitHub secret cannot be read back to inspect. Describe the problem | ||
| # (length, word count) without ever printing the value. Surrounding whitespace is stripped: |
There was a problem hiding this comment.
nit: ALL whitespace is stripped, not just surrounding
| # | ||
| # Skipped on fork PRs: secrets are withheld there. | ||
| on: | ||
| pull_request: |
There was a problem hiding this comment.
The preflight's fine, keep it. It's the smoke workflow I'd drop, starting with this pull_request trigger.
The release already does this. publish-release.yml builds the genesis with these same secrets on the tag, so a bad key fails the release before anything publishes. That is how v0.5.6-rc1 surfaced. The smoke runs the same check earlier, at PR time.
Moving it to PR time has a downside worth weighing. This job runs build-genesis.sh with DOTNS_ADMIN_KEY and FACTORY_DEPLOYER_KEY in env, and the paths filter includes the scripts and contracts a PR edits. So any same-repo PR can change the script (or, with ffi = true, any executed .sol) to print the keys, and the job runs it before review. The if: guards forks, not a write-access member or a leaked token. DOTNS_ADMIN_KEY owns the registry, resolvers, registrar and beacons, and cannot be rotated out of a shipped genesis.
And this is what dotns-releases is for. It's the private repo that owns deploy and release, and these credentials already live there behind its access controls. Putting the same production keys into the public contracts repo's PR CI widens where they live and hands them to PR-authored code from any contributor. A keyed rehearsal belongs there, run deliberately, not on every PR here.
The fix the incident calls for is the preflight, not a new workflow: it turns the bare decode error into a readable one, and dropping the mnemonic fallback removes the which-key ambiguity. I'd land that half, ideally as the first step of the publish workflows so a bad key fails fast, and drop the smoke.
There was a problem hiding this comment.
feel free to drop this PR, as i mentioned in matrix - it's optional
|
Closing as having that kind of CI brings risks for OS repo |
3 participants
Description
v0.5.6-rc1's genesis step died on
cast's bareFailed to decode private key— the first time anything ever consumed theDOTNS_ADMIN_KEYsecret was on master, because PR CI ran onlybash -nand the extractor's unit tests, never the script.Two changes:
DOTNS_ADMIN_KEYis now the only accepted owner key — the mnemonic fallback is gone (two credentials make the build guess which account owns everything, and the parity check validates addresses, not owners). A preflight rejects a malformed key before anvil starts and reports its shape — length, word count, a mnemonic hint with thecast wallet private-keyderivation command — never the value, since a GitHub secret cannot be read back. Surrounding whitespace is stripped: a pasted trailing newline is the classic break, verified end to end.genesis-build-smoke.ymlruns the exact release command with the real secrets on every PR touching contracts, deploy or genesis scripts, manifests, or the publish workflows — same-repo PRs only, parity unstubbed, artifact shape asserted and uploaded.The smoke run on this PR doubles as the probe for the broken secret: expect it red, read the shape from its log, re-paste the secret, re-run. Then re-dispatch the failed
Publish Beta Packagefor v0.5.6-rc1.Type
Scope
Related Issues
Follow-up to #253.
Fixes
Checklist
Code
forge buildpassesforge testpassesTesting
Security
selfdestructordelegatecallDocumentation
Breaking Changes
How to test
Exercised locally: all four malformed shapes fail fast without leaking the value; a newline-suffixed key completes the full build (33 accounts).
Notes
No Solidity changed; the contract checkboxes are inherited state.