Skip to content

fix: don't report success when JWT secret generation fails - #47

Open
pucedoteth wants to merge 1 commit into
giwa-io:mainfrom
pucedoteth:fix/jwt-generator-fails-loudly
Open

fix: don't report success when JWT secret generation fails#47
pucedoteth wants to merge 1 commit into
giwa-io:mainfrom
pucedoteth:fix/jwt-generator-fails-loudly

Conversation

@pucedoteth

Copy link
Copy Markdown

The bug

The jwt-generator entrypoint ends in || exit 0:

[ ! -s /shared/jwtsecret.key ] && openssl rand -hex 32 | tr -d '\n' > /shared/jwtsecret.key || exit 0

The || branch is reached in two different situations: when the key already exists — the intended no-op — and when the key was missing but generating it failed. A full disk or an unwritable /shared takes the second path, and the container still exits 0:

$ sh -c "[ ! -s ./ro/jwtsecret.key ] && openssl rand -hex 32 | tr -d '\n' > ./ro/jwtsecret.key || exit 0"
sh: ./ro/jwtsecret.key: Permission denied
exit code: 0
key present: NO

service_completed_successfully is then satisfied, so both clients start against an absent secret and fail on authrpc some seconds later, far from the cause.

That is the same end state the -s test in "regenerate empty JWT secrets" was added to prevent — reached from the other direction. The check catches a secret that is empty on arrival; this path creates one that is absent on departure.

The fix

Generate into a temp file, verify it is non-empty, then move it into place, under set -e. A failed generation now exits non-zero, and no partial secret is ever visible at the real path for a reader that races the writer.

Switched to exec form so the script is readable rather than one long folded line.

Test plan

No test harness in the repo, so I extracted the script exactly as YAML hands it to the container (yaml.safe_load(...)['services']['jwt-generator']['entrypoint'][2]) and ran it against a real filesystem, with /shared/jwtsecret.key repointed at a temp path:

case exit result
existing non-empty key 0 left untouched
missing key 0 64 bytes written
empty key 0 regenerated, 64 bytes
unwritable directory 1 fails loudly

Format is preserved — 64 hex characters, no trailing newline — and no .tmp file is left behind in any case. The same four cases against main give exit 0 for the unwritable directory with no key produced.

One note: [ -s ... ] is used rather than set -o pipefail, since the pipeline's status is tr's, not openssl's. If openssl failed while tr succeeded, an empty file would be written with status 0 — checking the temp file covers that too, and does not depend on pipefail being available in the image's shell.

The jwt-generator entrypoint ends in `|| exit 0`:

    [ ! -s /shared/jwtsecret.key ] && openssl rand -hex 32 | tr -d '\n' > /shared/jwtsecret.key || exit 0

The `||` is reached in two different situations: when the key already exists,
which is the intended no-op, and when the key was missing but generating it
failed. A full disk or an unwritable /shared both take the second path and the
container still exits 0:

    $ sh -c "[ ! -s ./ro/jwtsecret.key ] && openssl rand -hex 32 | tr -d '\n' > ./ro/jwtsecret.key || exit 0"
    sh: ./ro/jwtsecret.key: Permission denied
    exit code: 0
    key present: NO

service_completed_successfully is then satisfied, and both clients start against
an absent secret — the state the `-s` test was added to prevent, arrived at from
the other direction. reth and op-node fail on authrpc some seconds later, far
from the cause.

Generate into a temp file, check it is non-empty, and move it into place under
`set -e`, so a failed generation exits non-zero and never leaves a partial
secret where a reader could pick it up.

Behaviour is unchanged on every path that works today: an existing key is left
alone, a missing or empty one is (re)generated as 64 hex characters with no
trailing newline.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant