Skip to content

crypto: Fix K1/K2 use in the comments in ChaCha20-Poly1305 AEAD #23271

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 21, 2021
Merged

crypto: Fix K1/K2 use in the comments in ChaCha20-Poly1305 AEAD #23271

merged 1 commit into from
Oct 21, 2021

Conversation

stratospher
Copy link
Contributor

As per #22331 and the Detailed Construction of the ChaCha20Forward4064-Poly1305@Bitcoin cipher suite mentioned in BIP 324, K1 is used for encrypting the associated data(message length) and instantiating the Poly1305 MAC while K2 is used for encrypting the payload. This PR fixes the comments which need to be updated in:

  1. The test vector in src/test/crypto_tests.cpp
  2. In src/crypto/chacha_poly_aead.h, m_chacha_main is a K2 ChaCha20 cipher instance and should be used for encrypting the payload. Also, m_chacha_header is a K1 ChaCha20 cipher instance and is used for encrypting the length and instantiating the Poly1305 MAC.

@dhruv
Copy link
Contributor

dhruv commented Oct 13, 2021 •
edited
Loading

Thanks for catching that, @stratospher

ACK dc90406

There's a failing Win64 test. I suspect you need to re-run it.

@DrahtBot
Copy link
Contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #23233 (BIP324: Add encrypted p2p transport {de}serializer by dhruv)
  • #20962 (Alter the ChaCha20Poly1305@Bitcoin AEAD to the new specification by jonasschnelli)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

This was referenced Oct 14, 2021
Closed
Closed
shaavan
shaavan reviewed Oct 15, 2021
View reviewed changes
Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

Concept ACK

This PR fixes the comments surrounding k1 and k2 keys to indicate their usages properly. As per BIP 324, I agree with the changes.

@stratospher stratospher force-pushed the fix-k1-k2 branch 2 times, most recently from 33b1ed0 to e3c15d5 Compare October 15, 2021 18:11
@stratospher
Copy link
Contributor Author

Addressed #23271 (comment). Ready for further review.

shaavan
shaavan approved these changes Oct 16, 2021
View reviewed changes
Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

ACK e3c15d5

siv2r
siv2r reviewed Oct 20, 2021
View reviewed changes
Copy link

@siv2r siv2r left a comment

Choose a reason for hiding this comment

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

Concept ACK

nit: It might be better to squash your commits.

@stratospher
Fix K1/K2 use in the comments in ChaCha20-Poly1305 AEAD
be7f413 
This is done for the ChaCha20-Poly1305 AEAD test vector
and for the K1/K2 ChaCha20 cipher instances in chacha_poly_aead.h
@stratospher
Copy link
Contributor Author

Thanks for all the reviews! Addressed #23271 (comment) and squashed the commits.
Ready for further review.

@siv2r
Copy link

siv2r commented Oct 20, 2021

ACK be7f413

Zero-1729
Zero-1729 reviewed Oct 20, 2021
View reviewed changes
Copy link
Contributor

@Zero-1729 Zero-1729 left a comment

Choose a reason for hiding this comment

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

ACK be7f413

LGTM, this patch accurately updates the comments in accordance with BIP 324.

shaavan
shaavan approved these changes Oct 20, 2021
View reviewed changes
Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

reACK be7f413

jonatack
jonatack reviewed Oct 20, 2021
View reviewed changes
Copy link
Member

@jonatack jonatack left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Concept ACK, nice find.

@jonatack
Copy link
Member

ACK be7f413

@laanwj laanwj merged commit f41aa81 into bitcoin:master Oct 21, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 21, 2021
@laanwj
Merge bitcoin#23271: crypto: Fix K1/K2 use in the comments in ChaCha2…
3457504 
…0-Poly1305 AEAD

be7f413 Fix K1/K2 use in the comments in ChaCha20-Poly1305 AEAD (=)

Pull request description:

  As per [bitcoin#22331](bitcoin#22331) and the [Detailed Construction of the ChaCha20Forward4064-Poly1305@Bitcoin cipher suite](https://gist.github.com/jonasschnelli/c530ea8421b8d0e80c51486325587c52#detailed-construction) mentioned in BIP 324, K1 is used for encrypting the associated data(message length) and instantiating the Poly1305 MAC while K2 is used for encrypting the payload. This PR fixes the comments which need to be updated in:

  1. The test vector in `src/test/crypto_tests.cpp`
  2. In `src/crypto/chacha_poly_aead.h`,  `m_chacha_main` is a K2 ChaCha20 cipher instance and should be used for encrypting the payload. Also,  `m_chacha_header` is a K1 ChaCha20 cipher instance and is used for encrypting the length and instantiating the Poly1305 MAC.

ACKs for top commit:
  siv2r:
    ACK be7f413
  jonatack:
    ACK be7f413
  Zero-1729:
    ACK be7f413
  shaavan:
    reACK be7f413

Tree-SHA512: 9d3d0f45cf95d0a87b9f04c26f04b9ea78b2f2fa578d3722146a79dd0d377b9867532fc62e02b8e1487420df7702a1f033d15db562327535940c2049cbde401f
kwvg added a commit to kwvg/dash that referenced this pull request Nov 1, 2021
@kwvg
merge bitcoin#23271: Fix K1/K2 use in the comments in ChaCha20-Poly13…
d066144 
…05 AEAD
kwvg added a commit to kwvg/dash that referenced this pull request Nov 3, 2021
@kwvg
merge bitcoin#23271: Fix K1/K2 use in the comments in ChaCha20-Poly13…
bb30e9f 
…05 AEAD
pravblockc pushed a commit to pravblockc/dash that referenced this pull request Nov 18, 2021
@kwvg @pravblockc
merge bitcoin#23271: Fix K1/K2 use in the comments in ChaCha20-Poly13…
8c16cec 
…05 AEAD
@laanwj
Copy link
Member

laanwj commented Apr 25, 2022

@stratospher Your git name is "=", you might want to change this so that crediting you in the release notes is easier next time. For 23.0 I'm going to use "stratospher".

@bitcoin bitcoin locked and limited conversation to collaborators Apr 25, 2023
@stratospher stratospher deleted the fix-k1-k2 branch July 17, 2024 06:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
4 more reviewers

@jonatack jonatack jonatack left review comments

@Zero-1729 Zero-1729 Zero-1729 left review comments

@siv2r siv2r siv2r left review comments

@shaavan shaavan shaavan approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Utils/log/libs
Projects
No open projects
P2P encryption
Status: Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@stratospher @dhruv @DrahtBot @siv2r @jonatack @laanwj @Zero-1729 @shaavan