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

stratospher commented at 5:32 pm on October 13, 2021: 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:

The test vector in src/test/crypto_tests.cpp
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.

DrahtBot added the label Utils/log/libs on Oct 13, 2021

dhruv commented at 8:24 pm on October 13, 2021: contributor

Thanks for catching that, @stratospher

ACK dc904064ffc9aa9cf4215d910bc496b1ced16d2c

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

DrahtBot commented at 5:26 pm on October 14, 2021: 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.

in src/crypto/chacha_poly_aead.h:120 in dc904064ff outdated

116@@ -117,8 +117,8 @@ static constexpr int AAD_PACKAGES_PER_ROUND = 21;        /* 64 / 3 round down*/
117 class ChaCha20Poly1305AEAD
118 {
119 private:
120-    ChaCha20 m_chacha_main;                                      // payload and poly1305 key-derivation cipher instance
121-    ChaCha20 m_chacha_header;                                    // AAD cipher instance (encrypted length)
122+    ChaCha20 m_chacha_main;                                      // payload
123+    ChaCha20 m_chacha_header;                                    // AAD cipher instance (encrypted length) and poly1305 key-derivation cipher instance

shaavan commented at 12:09 pm on October 15, 2021:

This is quite nitpicking, so feel free to ignore it. What do you think about reversing the order of the variable initialization?

0    ChaCha20 m_chacha_header;                                    // AAD cipher instance (encrypted length) and poly1305 key-derivation cipher instance
1    ChaCha20 m_chacha_main;                                      // payload

I am saying so because:

It would be lexicographically preferred order
It will follow k1 (for AAD cipher) and k2 (for payload) usage order

stratospher commented at 6:14 pm on October 15, 2021:

Sounds reasonable. Updated the PR to reflect lexicographic order for the variables.

shaavan commented at 12:10 pm on October 15, 2021: contributor

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 force-pushed on Oct 15, 2021

stratospher commented at 6:17 pm on October 15, 2021: contributor

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

shaavan approved

shaavan commented at 1:11 pm on October 16, 2021: contributor

ACK e3c15d555138bf9900268c05c39182b8fa98a673

siv2r commented at 0:34 am on October 20, 2021: none

Concept ACK

nit: It might be better to squash your commits.

Fix K1/K2 use in the comments in ChaCha20-Poly1305 AEAD

This is done for the ChaCha20-Poly1305 AEAD test vector
and for the K1/K2 ChaCha20 cipher instances in chacha_poly_aead.h

be7f4130f9

stratospher force-pushed on Oct 20, 2021

stratospher commented at 6:50 am on October 20, 2021: contributor

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

siv2r commented at 11:40 am on October 20, 2021: none

ACK be7f413

Zero-1729 commented at 11:48 am on October 20, 2021: contributor

ACK be7f413

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

shaavan approved

shaavan commented at 12:17 pm on October 20, 2021: contributor

reACK be7f4130f996b2564041719177f0a907e5c2011b

in src/test/crypto_tests.cpp:698 in be7f4130f9

693@@ -694,8 +694,8 @@ BOOST_AUTO_TEST_CASE(chacha20_poly1305_aead_testvector)
694 
695     TestChaCha20Poly1305AEAD(true, 0,
696         /* m  */ "0000000000000000000000000000000000000000000000000000000000000000",
697-        /* k1 (payload) */ "0000000000000000000000000000000000000000000000000000000000000000",
698-        /* k2 (AAD) */ "0000000000000000000000000000000000000000000000000000000000000000",
699+        /* k1 (AAD) */ "0000000000000000000000000000000000000000000000000000000000000000",
700+        /* k2 (payload) */ "0000000000000000000000000000000000000000000000000000000000000000",

jonatack commented at 7:50 pm on October 20, 2021:

Two questions, would it be good to use these named args in the three other TestChaCha20Poly1305AEAD() calls in this file, and name the args according to the called function (I’m not sure which is better here), e.g. something like

 0@@ -692,13 +692,16 @@ BOOST_AUTO_TEST_CASE(chacha20_poly1305_aead_testvector)
 1         "0000000000000000000000000000000000000000000000000000000000000000",
 2         "0000000000000000000000000000000000000000000000000000000000000000", "", "", "");
 3 
 4-    TestChaCha20Poly1305AEAD(true, 0,
 5-        /* m  */ "0000000000000000000000000000000000000000000000000000000000000000",
 6-        /* k1 (AAD) */ "0000000000000000000000000000000000000000000000000000000000000000",
 7-        /* k2 (payload) */ "0000000000000000000000000000000000000000000000000000000000000000",
 8-        /* AAD keystream */ "76b8e0ada0f13d90405d6ae55386bd28bdd219b8a08ded1aa836efcc8b770dc7da41597c5157488d7724e03fb8d84a376a43b8f41518a11cc387b669b2ee6586",
 9-        /* encrypted message & MAC */ "76b8e09f07e7be5551387a98ba977c732d080dcb0f29a048e3656912c6533e32d2fc11829c1b6c1df1f551cd6131ff08",
10-        /* encrypted message & MAC at sequence 999 */ "b0a03d5bd2855d60699e7d3a3133fa47be740fe4e4c1f967555e2d9271f31c3aaa7aa16ec62c5e24f040c08bb20c3598");
11+    TestChaCha20Poly1305AEAD(
12+        /*must_succeed=*/true,
13+        /*expected_aad_length=*/0,
14+        /*hex_m=*/"0000000000000000000000000000000000000000000000000000000000000000",
15+        /*hex_k1=*/"0000000000000000000000000000000000000000000000000000000000000000",
16+        /*hex_k2=*/"0000000000000000000000000000000000000000000000000000000000000000",
17+        /*hex_aad_keystream=*/"76b8e0ada0f13d90405d6ae55386bd28bdd219b8a08ded1aa836efcc8b770dc7da41597c5157488d7724e03fb8d84a376a43b8f41518a11cc387b669b2ee6586",
18+        /*hex_encrypted_message=*/"76b8e09f07e7be5551387a98ba977c732d080dcb0f29a048e3656912c6533e32d2fc11829c1b6c1df1f551cd6131ff08",
19+        /*hex_encrypted_message_seq_999=*/"b0a03d5bd2855d60699e7d3a3133fa47be740fe4e4c1f967555e2d9271f31c3aaa7aa16ec62c5e24f040c08bb20c3598");
20+
21     TestChaCha20Poly1305AEAD(true, 1,

stratospher commented at 7:21 am on October 21, 2021:

I think it would be better to keep it as it is since explaining the arguments in all the test vectors doesn’t add incremental value.

Using comments like k1 (AAD), AAD keystream etc.. for the arguments seems more human understandable to me and it’s also consistent with how the comments are written for other test vectors like HKDF_SHA256_32 in crypto_tests.

Would this be fine?

jonatack commented at 12:55 pm on October 21, 2021:

Thanks, in any case not a blocker.

jonatack commented at 7:50 pm on October 20, 2021: contributor

Concept ACK, nice find.

jonatack commented at 12:55 pm on October 21, 2021: contributor

ACK be7f4130f996b2564041719177f0a907e5c2011b

laanwj merged this on Oct 21, 2021

laanwj closed this on Oct 21, 2021

sidhujag referenced this in commit 3457504d34 on Oct 21, 2021

kwvg referenced this in commit d066144dae on Nov 1, 2021

kwvg referenced this in commit bb30e9f007 on Nov 3, 2021

pravblockc referenced this in commit 8c16cec960 on Nov 18, 2021

laanwj commented at 12:46 pm on April 25, 2022: member

@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 locked this on Apr 25, 2023

stratospher deleted the branch on Jul 17, 2024

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

Conflicts