Skip to content

secure: use HKDF for session key derivation, bind transcript to both peers - #35

Merged
ethical-buddy merged 10 commits into
ethical-buddy:mainfrom
SonOfTroll:main
Jul 14, 2026
Merged

secure: use HKDF for session key derivation, bind transcript to both peers#35
ethical-buddy merged 10 commits into
ethical-buddy:mainfrom
SonOfTroll:main

Conversation

@SonOfTroll

Copy link
Copy Markdown
Contributor

hey, i noticed the handshake was deriving the session key by just SHA256-ing the raw shared secret with the kind byte appended. it works, but SHA256 isn't really a proper KDF, and the resulting key isn't bound to the actual session participants, which leaves a small misbinding risk if someone is relaying the handshake in the middle
i switched it over to HKDF-SHA256 and included a transcript hash that binds both ephemeral keys and both node IDs
same overall behavior, but the key derivation is now tied to the session context
changes:

  • deriveKey() now uses HKDF with the transcript hash as the salt
  • added buildTranscript() to canonicalize the ordering (client ephemeral + ID first, server second), so both sides derive the same key regardless of who initiates
  • added x/crypto as a direct dependency (it was already present indirectly)

@ethical-buddy

Copy link
Copy Markdown
Owner

Thanks for taking this on. The HKDF direction is the right direction, but I am going to request changes before we merge this.

Main blockers:

  1. This changes the secure-session wire behavior. Older nodes derive the session key as sha256(shared || kind), while this PR derives it through HKDF over shared secret plus transcript context. That means mixed-version peers can complete the visible handshake path and then fail encrypted traffic. We need an explicit protocol/version migration path or compatibility gate before this lands.

  2. The transcript currently binds shortened VX6 node IDs, not the full static Ed25519 public keys. Since VX6 node IDs are truncated hashes, the transcript should bind the full static public keys from both handshake messages if the goal is peer-context binding.

  3. The tests currently prove the new path can round-trip, but they do not prove the security properties introduced by this change. Please add focused tests for transcript canonicalization, key separation across kind/identity/ephemeral changes, and mixed-version behavior.

I am going to make a more detailed plan in the issues section and assign it to you so we can move this forward in smaller pieces. The change is valuable, but I want us to test and roll out the HKDF shift slowly and securely instead of merging it as one implicit protocol break.

@SonOfTroll

Copy link
Copy Markdown
Contributor Author

@ethical-buddy
sorry for the insanely late follow up life happened and this kinda sat on the backburner for way longer than i intended
addressed the review comments
added version gating to the hello handshake so peers not on v2 fail immediately with a clear error instead of breaking later on the first encrypted frame
replaced node ids in the transcript with the full 32-byte ed25519 pubkeys since node ids are just truncated hashes and don't really provide useful binding also bumped the transcript and info labels to v2
added 7 focused tests covering transcript consistency key separation full pubkey binding and version mismatch handling
all 9 tests are passing and the build is clean

@ethical-buddy

Copy link
Copy Markdown
Owner

Thanks for the follow-up and for addressing the earlier review comments.

We are going to move forward with this PR as the base for the secure-session HKDF migration. The current version solves the important parts we needed before merge: it adds HKDF-based key derivation, binds the transcript to the full static Ed25519 public keys instead of truncated node IDs, includes both ephemeral keys, adds an explicit secure-session version gate, and expands the tests around transcript consistency, key separation, public-key binding, and version mismatch behavior.

Before merging, we are going to make a small final cleanup commit on top of this PR:

  • include the secure-session version in the signed handshake payload
  • restore the file license/header that was removed from internal/secure/session.go
  • possibly switch the HKDF import to the Go standard library version if it fits our supported Go version

Those are small hardening/cleanup changes, not a rejection of the PR direction. The core design is correct and this is the PR we plan to merge. Thanks again for sticking with the review and improving the migration path.

@ethical-buddy
ethical-buddy merged commit 723cfe6 into ethical-buddy:main Jul 14, 2026
1 check failed
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.

2 participants