Skip to content
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.

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

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

Merged
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

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.

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.

src/crypto/chacha_poly_aead.h Outdated Show resolved Hide resolved
@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.

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

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.

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

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.

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

Copy link
Contributor

@jonatack jonatack 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, nice find.

src/test/crypto_tests.cpp Show resolved Hide resolved
@jonatack
Copy link
Contributor

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
…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 added a commit to kwvg/dash that referenced this pull request Nov 3, 2021
pravblockc pushed a commit to pravblockc/dash that referenced this pull request Nov 18, 2021
@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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

None yet

8 participants