Skip to content

ethp2p: write the runtime TLS private key owner-only (0o600) - #1051

Merged
ch4r10t33r merged 2 commits into
mainfrom
fix/ethp2p-key-owner-only
Jul 18, 2026
Merged

ethp2p: write the runtime TLS private key owner-only (0o600)#1051
ch4r10t33r merged 2 commits into
mainfrom
fix/ethp2p-key-owner-only

Conversation

@ch4r10t33r

Copy link
Copy Markdown
Contributor

Follow-up to #1049 addressing the blocking review finding from @zclawz.

Problem

ethp2pWritePem created the generated ethp2p private-key PEM ({database_path}/ethp2p/key.pem) with Zig's default file permissions (0o666 before umask), leaving it group/world-readable under common umasks. A copy of key.pem plus the signed cert is enough to impersonate this node's ethp2p QUIC listener for the cert lifetime — which defeats #1049's whole point (no shared/committed keypair).

Fix

Thread a permissions argument through ethp2pWritePem and create the private key with 0o600; the cert (public) keeps the default. 0o600 carries no group/other bits, so umask can only clear bits — the key is owner-only regardless of the process umask (addresses the "do not rely on umask" note).

Verified on disk

Ran a node with ZEAM_ETHP2P=1 under umask 022:

-rw-r--r--  cert.pem   (644)
-rw-------  key.pem    (600)

zig build -Dethp2p=true green.

Per review (zclawz on #1049): the generated ethp2p private-key PEM was
written with default file permissions (0o666 before umask), leaving it
group/world-readable under common umasks. A copy of key.pem plus the
signed cert is enough to impersonate this node's ethp2p QUIC listener for
the cert lifetime, which defeats this PR's whole point (no shared/committed
keypair). Thread a permissions arg through ethp2pWritePem and create the
key with 0o600; the cert (public) keeps the default. 0o600 carries no
group/other bits so the file is owner-only regardless of umask.

Verified on disk: key.pem = -rw------- (600), cert.pem = -rw-r--r-- (644).
@zclawz

zclawz commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Approved from my side. The latest revision addresses my blocker.

What changed since my previous pass:

  • ethp2pWritePem now takes an optional enforced mode.
  • key.pem is created with 0o600 and then explicitly setPermissions is called on the opened file before writing key bytes, so the existing-file case is covered too. A previously lax {database_path}/ethp2p/key.pem gets tightened before the new secret is written.
  • cert.pem passes null, so it keeps normal create-time defaults instead of accidentally forcing a public cert to 0o666.

I do not see a remaining blocker in the permission fix.

Validation I ran:

  • zig fmt --check pkgs/cli/src/node.zig
  • git diff --check origin/main...HEAD
  • Tiny Zig 0.16.0 permission API compile/test for std.Io.File.Permissions.fromMode(0o600) + file.setPermissions(io, mode) passed.
  • Started PATH=/usr/local/cargo/bin:$PATH /tmp/zig-0.16.0/zig build -Dethp2p=true --summary all; it timed out locally after 240s while still compiling Rust glue, with no Zig compile error observed before timeout. GitHub CI is still the source of truth for the full gate.

As usual, leaving this as a regular PR comment rather than a formal GitHub approval review.

Review follow-up (zclawz on #1051): createFile's .permissions only applies
when the file is CREATED. A key.pem that an already-merged-#1049 node wrote
with the default 0o666 keeps that lax mode when this code rewrites it in
place (truncate=true), so the fix only covered fresh installs.

Explicitly setPermissions(0o600) on the key after open, while the file is
still empty (before the key bytes are written) so the secret is never
briefly present at a looser mode. The public cert passes null and keeps the
umask'd create default — it must not be force-set to 0o666 (world-writable).

Verified: a pre-existing key.pem at 0o666 is tightened to 0o600 in place
after the node runs (cert stays untouched).
@ch4r10t33r

Copy link
Copy Markdown
Contributor Author

Good catch — you're right, createFile's .permissions only applies on create, so a key.pem an already-merged-#1049 node wrote at 0o666 kept that mode when this code rewrote it in place. Fixed in 7de55ab: after opening, explicitly file.setPermissions(io, 0o600) on the key — done before the key bytes are written so the secret is never briefly present at a looser mode. The public cert passes null and keeps the umask'd create default (not force-set to 0o666, which would be world-writable).

Verified the pre-existing-file case directly: seeded key.pem at 0o666, ran the node, confirmed it's tightened in place:

BEFORE: key=666 cert=666
AFTER : key=600 cert=666

@ch4r10t33r
ch4r10t33r merged commit a1959ef into main Jul 18, 2026
10 checks passed
@ch4r10t33r
ch4r10t33r deleted the fix/ethp2p-key-owner-only branch July 18, 2026 19:04
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.

3 participants