Skip to content

Harden preshared-key lifecycle with zeroizing ownership#164

Open
lurenjia534 wants to merge 1 commit into
mullvad:mainfrom
lurenjia534:agent/harden-psk-lifecycle
Open

Harden preshared-key lifecycle with zeroizing ownership#164
lurenjia534 wants to merge 1 commit into
mullvad:mainfrom
lurenjia534:agent/harden-psk-lifecycle

Conversation

@lurenjia534

@lurenjia534 lurenjia534 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Closes #163

Summary

Introduce a public, non-Copy PresharedKey type and use it throughout peer configuration, tunnel state, and Noise handshake state.

PresharedKey owns its bytes in zeroizing heap storage, redacts Debug, and makes secret duplication explicit through Clone. This is defense-in-depth memory-lifetime hardening; it does not claim complete erasure of every physical copy of a key.

Changes

Secret ownership

  • Add PresharedKey, backed by stable heap storage that is zeroized on drop.
  • Keep the type non-Copy; cloning creates an independent zeroizing allocation.
  • Redact PSK values from Debug.
  • Add explicit by-value, borrowed, mutable source-clearing, and raw-borrow APIs.
  • Add the zeroize dependency.

PresharedKey::take_from clears the supplied mutable source array after importing it. The by-value and borrowed constructors cannot clear copies retained by the caller.

Device and Noise state

Replace long-lived Option<[u8; 32]> PSKs in Peer, PeerMut, Tunn, Handshake, and NoiseParams.

Peer creation and update paths move PSK ownership where possible. APIs that return owned peer configurations create an explicit independent clone. Handshake PSK mixing borrows the configured bytes instead of materializing another owned array; an absent PSK continues to use WireGuard's 32-byte all-zero input.

UAPI integration

Make only the mechanical conversions required by the new core API:

  • convert the existing UAPI KeyBytes value into PresharedKey when applying configuration;
  • copy a borrowed PresharedKey into the existing wire response representation; and
  • clone an existing PSK only when an omitted UAPI field requires preserving it during peer reconstruction.

There is no intended UAPI wire-format or parsing behavior change in this PR.

Runtime behavior

WireGuard behavior remains unchanged:

  • matching PSKs complete a handshake;
  • one-sided or different PSKs fail authentication;
  • None retains the standard all-zero no-PSK input;
  • replacing or clearing a PSK does not invalidate an established transport session; and
  • an in-flight handshake reads the configured PSK when it reaches the PSK-mixing step, so it can fail if its peer generated the corresponding message using the previous key.

Public API changes

This is an intentional pre-1.0 SemVer-relevant API change:

  • Peer::preshared_key becomes Option<PresharedKey>.
  • PeerMut::set_preshared_key accepts Option<PresharedKey>.
  • Tunn::new and Tunn::new_with_rng accept Option<PresharedKey>.
  • Tunn::set_preshared_key accepts Option<PresharedKey>.
  • Tunn::preshared_key returns Option<&PresharedKey> instead of an owned array.
  • Peer::with_preshared_key([u8; 32]) remains available, but is no longer const because it allocates zeroizing storage.

Example migration:

use gotatun::PresharedKey;

let mut raw_psk = [0xA5; 32];
peer.preshared_key = Some(PresharedKey::take_from(&mut raw_psk));

if let Some(psk) = tunnel.preshared_key() {
    use_psk(psk.as_bytes());
}

Callers that need an owned value from Tunn::preshared_key() can use .cloned().

Security properties and limitations

Covered by best-effort zeroization:

  • each owned PresharedKey allocation;
  • each independently owned clone when dropped; and
  • the mutable source passed to PresharedKey::take_from.

Moving a PresharedKey moves its heap pointer at the Rust abstraction boundary rather than copying its backing array. Replacing or dropping one owner clears that allocation, but does not revoke other clones.

This does not guarantee erasure of:

  • caller-owned arrays or strings, except the exact array passed to take_from;
  • compiler-generated temporaries or CPU registers;
  • allocator history, stale pages, swap, or core dumps;
  • kernel, socket, reader, or existing UAPI-owned buffers; or
  • copies callers create from an as_bytes() borrow.

The implementation does not use mlock and does not protect live secrets from arbitrary process-memory reads. Drop-based clearing also requires destructors to run.

Testing

Tests cover:

  • the ZeroizeOnDrop contract;
  • source clearing through take_from;
  • independent clone storage;
  • redacted Debug;
  • matching, one-sided, and different PSKs;
  • established-session and in-flight-handshake behavior after PSK replacement; and
  • setting and clearing PSKs through both device update APIs.

Locally passed:

  • cargo fmt --all -- --check
  • cargo check --locked --workspace
  • cargo check --locked --workspace --no-default-features --features ring
  • cargo check --locked --workspace --all-features
  • cargo clippy --locked --workspace --all-targets --all-features -- -D warnings
  • RUSTDOCFLAGS='-D warnings' cargo doc --locked -p gotatun --all-features --no-deps
  • git diff --check
  • CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_RUNNER=env cargo test --locked -p gotatun --lib --all-features -- --skip udp::socket::linux::gro::tests::recv_many_from_grows_for_oversized_datagram --skip udp::socket::linux::gro::tests::recv_many_from_preserves_ipv6_source_addr --skip udp::socket::tests::bind_ipv4_socket_uses_ipv4 --skip udp::socket::tests::bind_retries_on_ipv6_conflict

The filtered library run completed with 81 passed, 0 failed, 7 ignored, and 4 filtered. The four socket tests were also attempted without filters; all four failed with Operation not permitted in this restricted environment, while the other 81 tests passed.

Not run locally: the seven privileged WireGuard integration tests, the cross-platform/Android CI matrix, the cargo hack feature powerset, and cargo-semver-checks. The SemVer check is expected to report the intentional API changes and requires a maintainer release decision.

Out of scope

This PR intentionally does not change:

  • KeyBytes ownership, visibility, or zeroization;
  • UAPI key parsing or validation;
  • UAPI request, response, or line-buffer handling;
  • UAPI logging or error formatting;
  • UAPI command framing or dispatch behavior;
  • session keys or other handshake-derived secret material.

Those concerns should be handled in separate, independently reviewable changes.


This change is Reviewable

@lurenjia534 lurenjia534 force-pushed the agent/harden-psk-lifecycle branch from b4b0946 to d92c619 Compare July 9, 2026 19:49
@lurenjia534 lurenjia534 marked this pull request as ready for review July 10, 2026 04:08
@lurenjia534

lurenjia534 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Hi @hulthe, could you please review this PR when you have a chance? Thank you

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.

Harden core preshared-key ownership and lifecycle

1 participant