Skip to content

[ARCH-338] Add a comment about nonce reuse in ECDH-P256#1996

Merged
pavel-raykov merged 1 commit intomainfrom
add-comment
Apr 23, 2026
Merged

[ARCH-338] Add a comment about nonce reuse in ECDH-P256#1996
pavel-raykov merged 1 commit intomainfrom
add-comment

Conversation

@pavel-raykov
Copy link
Copy Markdown
Contributor

No description provided.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 22, 2026

📊 API Diff Results

No changes detected for module github.com/smartcontractkit/chainlink-common/keystore

View full report

@pavel-raykov pavel-raykov marked this pull request as ready for review April 22, 2026 19:57
@pavel-raykov pavel-raykov requested review from a team as code owners April 22, 2026 19:57
Copilot AI review requested due to automatic review settings April 22, 2026 19:57
@pavel-raykov pavel-raykov requested review from kaleofduty and removed request for harry-anderson April 22, 2026 19:58
@pavel-raykov pavel-raykov requested review from jmank88 and removed request for vyzaldysanchez April 22, 2026 19:58
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the ECDH-P256 anonymous encryption implementation’s inline documentation to add cautionary context around how the AES-GCM nonce is handled.

Changes:

  • Adds an expanded comment describing AAD contents (nonce + ephemeral public key) and referencing RFC 5116 guidance.
  • Adds commentary about nonce “reuse” between HKDF salt and AES-GCM nonce.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread keystore/encryptor.go
Comment on lines +309 to +315
// Including nonce in AAD is in line with https://www.ietf.org/rfc/rfc5116:
// <<<The nonce is authenticated internally to the algorithm, and it is not
// necessary to include it in the AD input. The nonce MAY be included
// in P or A if it is convenient to the application.>>>
// However, the second nonce reuse in gcm.Seal(nil, nonce...) is not conventional - this means that we reuse the
// same nonce both for the symmetric key and the block cipher counter.
// It is not clear why we don't use new nonce here (by just extending nonceSizeECDHP256 size).
@pavel-raykov pavel-raykov added this pull request to the merge queue Apr 23, 2026
Merged via the queue into main with commit 9eb935b Apr 23, 2026
37 checks passed
@pavel-raykov pavel-raykov deleted the add-comment branch April 23, 2026 16:11
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.

4 participants