Conversation
Two changes for pointing web3signer at the key operation service's keystore table. The provider: aws branch piped the KMS plaintext through base64 -d before writing DECRYPTION_KEY. sync-keys base64-decodes that value itself, and the Azure branch writes base64, so the AWS branch was handing it raw bytes. Worse, raw key material does not survive a shell command substitution: a key containing a null byte was silently truncated and sync-keys then failed with "Incorrect AES key length (0 bytes)". A random 32-byte key contains a null byte about 11.8 percent of the time, so the failure came and went between key generations. The value now stays base64, matching the Azure branch. Adds dbKeystoreClientClusterId and dbKeystoreAllClusters, which render --client-cluster-id and --all-clusters (sync-keys 0.4.2+). A keystore table carrying a client_cluster_id column holds more than one cluster's keys, and without a predicate every web3signer sharing that database loads every cluster's private keys. Setting both is a template error rather than a runtime surprise. Renders verified: the pilot values produce --table-name and --client-cluster-id; the all-clusters variant produces --all-clusters; setting both fails the template; and a release using neither renders byte-identical to origin/main apart from the chart version label.
The fetch-keys init container defaults to sync-keys 0.4.1, which does not accept --client-cluster-id or --all-clusters. Releases inherit that default rather than pinning their own tag -- all six production signers do -- so bumping it here would change the image every web3signer pulls. Left at 0.4.1, with the requirement to override recorded next to the options that need it.
The previous wording claimed bumping the default would change the image every web3signer pulls including production. That is wrong: every release pins an exact chart version, and all six bfceu production signers are on 6.3.1, so publishing a new chart version reaches nobody until a HelmRelease is edited. The real reason to leave the default alone is narrower -- it would bundle a sync-keys upgrade into whichever release next adopts this chart version, which should be a separate decision. Releases using the new options pin the tag themselves.
jianhonggalaxy
previously approved these changes
Sep 7, 2026
A signer can legitimately serve several clusters -- QA's use1/hoodi-1 serves two -- and a single value forced those releases to fall back to dbKeystoreAllClusters, which then silently picks up any cluster added to the same database later. dbKeystoreClientClusterId becomes dbKeystoreClientClusterIds, a list, and the template repeats --client-cluster-id once per entry. An empty entry fails the template rather than reaching sync-keys, where a repeatable option makes an empty value indistinguishable from "a cluster was named". Naming the clusters is now preferable to dbKeystoreAllClusters in every case, because the set is closed. Renders verified: two ids produce the flag twice, one id once, an empty entry fails, and a release setting neither option still renders identically to origin/main. helm lint passes.
The cluster flags were only appended inside the encryptedDecryptionKey branch, which execs sync-keys through a shell. The plain command/args branch appended only --table-name, so on any release not using the encrypted secret the dbKeystoreClientClusterIds and dbKeystoreAllClusters values were accepted and silently ignored. Every live QA web3signer uses that plain branch, so the scoping feature would have been inert on exactly the releases being migrated: the values would look right in the HelmRelease while sync-keys ran unscoped. The flags are now built once into $fetchKeysFlags before either branch and consumed by both, so a flag cannot be added to one path and missed on the other. Also adds dbKeystoreAllowNoKeys, matching sync-keys' --allow-no-keys, for a signer that is deployed but serves no validators. QA's euw1/hoodi-2 is one: zero rows in its keystore table with web3signer running 3/3 ready. Verified by render: a legacy release is byte-identical to origin/main apart from the chart version; both branches now emit --client-cluster-id once per entry; the mutual-exclusion and empty-value guards still fail the template.
…igner flag The note said 0.4.2, but sync-keys releases via conventional commits with default_bump: false, and PR #38 carries feat: commits on top of v0.4.1, so the version that first understands these options is 0.5.0. Also records that when the chart's default image tag moves, a namespace whose keystore query returns no rows needs dbKeystoreAllowNoKeys in the same change. QA's euw1/hoodi-2 is one: zero rows with web3signer running 3/3 ready.
sync-keys #38 was retitled from feat: to fix: so it releases as a patch. With squash merge the tag action reads only the PR title as the commit header, so v0.4.1 becomes v0.4.2.
sync-keys removed --allow-no-keys, so rendering it would pass an option the CLI rejects. The refusal it overrode is unreachable for a correctly configured signer under one KOS database per namespace, and an empty table is tolerated without it -- which is the case that actually occurs. Also pins the required version at 0.4.3 and records that 0.4.2 must be skipped: it refuses a zero-row read unconditionally, which would stop QA's euw1/hoodi-2 from starting. Verified by render: a legacy release is byte-identical to origin/main apart from the chart version, cluster ids still render once per entry on both arg paths, and setting dbKeystoreAllowNoKeys now emits nothing.
The new values are inert unless set and the only behaviour change is the AWS KMS base64 fix, so a patch release is the honest signal. 6.4.3 is already published and main is at 6.4.4, so 6.4.5 is the next available patch.
… exec An init container with usesDecryptedSecret and neither command nor args could not work: the wrapper replaces the image entrypoint with /bin/sh -c, so it cannot fall back to ENTRYPOINT the way the plain command/args path does, and it cannot exec the sync-keys flags on their own because the first word would be taken as the program name. That case emitted a container that echoed an error and exited 1, so a misconfiguration surfaced during a rollout instead of at template time. Fail the render instead. Both shipped init containers set command and args, so nothing that currently renders is affected. Also correct two comments that described behaviour inaccurately: - The KMS decrypt comment said a NUL byte truncated the key. It does not truncate: command substitution drops each NUL and strips trailing newlines, so a 32-byte key comes back one byte short per NUL. Verified directly -- 32 bytes in, 31 out for one NUL and 30 for two. The one-in-eight figure is right (1 - (255/256)^32 = 11.8%). - The values.yaml note said an older sync-keys "does not understand" the new options, which left open whether it ignores them. It does not: click exits 2 with "No such option", so the init container crashloops. Worth stating, because it means the flag cannot be silently dropped and leave a signer serving more keys than intended.
…onverge The AWS branch wrote raw key bytes while the Azure branch wrote base64, and nothing recorded which one the consumer needs. sync-keys base64-decodes what it reads, so base64 is the answer and the AWS branch was wrong. Separating the two branches further would not have caught that; the encoding is a property of the file they share, so the check belongs at the one point where they meet. Reject a value that does not decode to a 16/24/32-byte key before writing it. That covers both ways this has gone wrong -- raw bytes instead of base64 (decodes to 0) and a key shortened by the shell eating a NUL (decodes to 31) -- and it fails at the cause instead of as an opaque error inside sync-keys. Verified against the live Azure value, which is 44 characters of standard base64 decoding to 32 bytes, so the assertion passes for the deployment that already works. Also revert two changes from the previous commit: - The render-time fail for an init container with neither command nor args is reverted. Both shipped init containers set both, so it was unreachable, and the existing runtime message was already clear. It only moved when the operator finds out, at the cost of a behaviour change in a shared chart. - The decrypt comment claimed the symptom was "Incorrect AES key length" on roughly one key in eight. That was wrong. Writing raw bytes where base64 was expected failed for every key, and earlier than that message. The NUL story is a genuine second hazard but a hypothetical one, so it no longer reads as the observed failure.
These values were added for a migration that turns out not to need them, and the guard behind them checks the wrong property. sync-keys refuses only a zero-row read, so naming a subset of a signer's clusters returns rows and the check stays quiet while the signer comes up Ready with a partial keystore -- the omitted cluster's validators stop signing with nothing to indicate it. The two settings do not order: naming clusters guards against reading the wrong database but allows an incomplete set, and the all-clusters override is the reverse. A control that makes the operator choose which failure they would rather have is not a control. Where one database holds one signer's keys, an unfiltered read is already the correct read, so nothing is lost by removing them. The property actually worth checking -- that the keystore holds the keys this signer should serve -- needs an expected count, which belongs in a post-deploy check rather than in values that can be typed wrong. values.yaml is byte-identical to main again. Kept from the previous commits: - the AWS KMS base64 fix, which is a real defect on that branch: it wrote raw key bytes where sync-keys base64-decodes, so it failed for every key. Now verified end to end against real KMS on both an ordinary key and one containing a NUL byte, round-tripping to the original 32 bytes. - the encoding assertion where the provider branches converge, which rejects raw bytes, a NUL-shortened key and an empty decrypt. - building the fetch-keys flags in one place, so a flag added later cannot reach only one of the two arg-rendering paths. The chart version bump stays: the base64 fix is a behaviour change and consumers pin chart versions.
kenrick-g
changed the title
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
2 participants
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.
The AWS branch of the decrypt-secret init container base64-decoded the key
before writing it, but
sync-keysbase64-decodesDECRYPTION_KEYitself. Sothat branch wrote raw bytes where base64 was expected and failed for every key,
not intermittently. The Azure branch never had the bug — it writes the base64 its
decrypt call returns.
Dropping
| base64 -dmakes the two agree. It also removes a second hazard: thevalue passes through two command substitutions before
sync-keyssees it, and ashell drops every NUL byte and strips trailing newlines, so raw 32-byte key
material arrives short. Base64 is ASCII and survives.
Verified end to end against real KMS — an ordinary key and one containing a NUL
byte both round-trip to the original 32 bytes; the old path yields 0 usable
bytes for both.
Also asserts the encoding where the provider branches converge, before the file
is written. That is where the contract belongs, since both branches feed one
consumer and nothing previously stated which encoding it needs. It rejects raw
bytes, a NUL-shortened key, and an empty decrypt, and passes for the deployment
already running.
Chart version bumped because this is a behaviour change and consumers pin chart
versions.