split off from #24748 to keep commits related to cryptography and functional test framework changes separate.
This PR adds python implementation and unit tests for HKDF, ChaCha20, Poly1305, ChaCha20Poly1305 AEAD, FSChaCha20 and FSChaCha20Poly1305 AEAD.
They’re based on https://github.com/bitcoin/bips/blob/cc177ab7bc5abcdcdf9c956ee88afd1052053328/bip-0324/reference.py for easy review.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
No conflicts as of last run.
94+
95+class TestFrameworkChacha(unittest.TestCase):
96+ def test_chacha20(self):
97+ """ChaCha20 test vectors."""
98+ for test_vector in CHACHA20_TESTS:
99+ hex_key, hex_nonce, counter, hex_output = test_vector
naming nit: the nonce from CHACHA20_TESTS is not provided as a hex string, but as a list of two integers, so the hex_ prefix doesn’t apply here. Maybe something like nonce_pair instead? (or simply nonce and rename the bytes-version below to somethig else like nonce_bytes)
0 hex_key, nonce_pair, counter, hex_output = test_vector
26@@ -27,6 +27,22 @@ def TaggedHash(tag, data):
27 ss += data
28 return hashlib.sha256(ss).digest()
29
30+# HKDF-SHA256
31+def hmac_sha256(key, data):
key.py is currently containing only code related to digital signatures (according to the module description it’s a “secp256k1 elliptic curve protocols implementation”), the HKDF would maybe be better suited in another (new?) module. Probably it generally makes sense to do a follow-up with restructuring crypto-related helpers, e.g. having an own “test_framework/crypto” folder would be nice on the long-term for better structuring I think, as the list of crypto-related modules has grown quite a bit.
As key.py is currently containing only code related to digital signatures (according to the module description it’s a “secp256k1 elliptic curve protocols implementation”), the HKDF would maybe be better suited in another (new?) module.
makes sense. what about keeping HKDF in the file where it’s used for initialising v2 transport? i’ll include this commit in the original #24748 PR with the rest of that file.
Probably it generally makes sense to do a follow-up with restructuring crypto-related helpers, e.g. having an own “test_framework/crypto” folder would be nice on the long-term for better structuring I think, as the list of crypto-related modules has grown quite a bit.
yeah! that would be great to have.
v2_p2p.py or in a (separate) crypto/ module would make more sense than in key.py.
crypto/ module.
172+
173+ enc_aead = FSChaCha20Poly1305(key)
174+ dec_aead = FSChaCha20Poly1305(key)
175+
176+ for _ in range(msg_idx):
177+ enc_aead.crypt(b"", b"", False)
nit: named arguments for the boolean here and below would be nice
0 enc_aead.crypt(b"", b"", is_decrypt=False)
109+ nonce = hex_nonce[0].to_bytes(4, 'little') + hex_nonce[1].to_bytes(8, 'little')
110+
111+ ciphertext = aead_chacha20_poly1305_encrypt(key, nonce, aad, plain)
112+ self.assertEqual(hex_cipher, ciphertext.hex())
113+ plaintext = aead_chacha20_poly1305_decrypt(key, nonce, aad, ciphertext)
114+ self.assertEqual(plain, plaintext)
None due to poly1305 tag mismatch) if a random bit in ciphertext or aad is flipped.
56@@ -55,6 +57,34 @@ def chacha20_block(key, nonce, cnt):
57 # Produce byte output
58 return b''.join(state[i].to_bytes(4, 'little') for i in range(16))
59
60+class FSChaCha20:
61+ """Rekeying wrapper stream cipher around ChaCha20."""
62+ def __init__(self, initial_key, rekey_interval=REKEY_INTERVAL):
63+ self.key = initial_key
_) (see e.g.: https://peps.python.org/pep-0008/#method-names-and-instance-variables). But that’s just a naming convention anyway and the “private” usage is not enforced by the language.
Concept ACK
Very nice! Left some non-blocking nits below, they all can be seen as potential follow-up material.
79+ ks = self.get_keystream_bytes(len(chunk))
80+ ret = bytes([ks[i] ^ chunk[i] for i in range(len(chunk))])
81+ if ((self.chunk_counter + 1) % self.rekey_interval) == 0:
82+ self._key = self.get_keystream_bytes(32)
83+ self.block_counter = 0
84+ self.keystream = b''
This line was added in FSChaCha20 for rekey interval option and self.keystream = b'' in the unit tests. That’s the only change from bip-0324/reference.py.
added self.keystream = b'' so that keystream is discarded if there is some keystream left when rekeying happens. this is only needed for FSCHACHA20_TESTS and not in BIP 324’s usage of FSChaCha20.
In BIP 324’s usage of FSChaCha20, rekeying happens after 224 messages.
so there would be no keystream left behind and nothing to discard.
FSChaCha20 in the C++ code does the same.
13+ def __init__(self, key):
14+ self.r = int.from_bytes(key[:16], 'little') & 0xffffffc0ffffffc0ffffffc0fffffff
15+ self.s = int.from_bytes(key[16:], 'little')
16+ self.acc = 0
17+
18+ def add(self, msg, length=None, pad=False):
What do you think about dropping the length/pad arguments, and dropping the “Input so far must be a multiple of 16 bytes” requirement? You can do so by buffering incoming bytes in the object until 16 are reached, and then applying the logic currently in add.
I used this somewhat adhoc interface for the BIP324 reference code because it was less code for that purpose, but it isn’t as intuitive as it could be. Perhaps if we’re going to have a proper poly1305 module here, it’s worth having a more generally usable one.
pad16() function in the aead to do the padding like in the RFC.
0@@ -0,0 +1,103 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2022 The Bitcoin Core developers
3+# Distributed under the MIT software license, see the accompanying
4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+"""Python implementation of ChaCha20 cipher"""
test/functional/test_framework/crypto/secp256k1.py has (only for tests, slow, vulnerable to side-channel attacks, designed for ease of understanding, …). I don’t think there is much risk that we’d use these files for anything but tests ourselves, but sometimes other projects copy them.
23+
24+ def tag(self):
25+ """Compute the poly1305 tag."""
26+ length = len(self.msg)
27+ for i in range((length + 15) // 16):
28+ chunk = self.msg[i * 16:i * 16 + min(16, length - i * 16)]
self.msg[i * 16:min(length, (i + 1) * 16)].
18+ MODULUS = 2**130 - 5
19+
20+ def __init__(self, key):
21+ self.r = int.from_bytes(key[:16], 'little') & 0xffffffc0ffffffc0ffffffc0fffffff
22+ self.s = int.from_bytes(key[16:], 'little')
23+ self.acc = 0
nit:
This doesn’t need to be a member variable anymore, could use a local variable acc in the tag method instead.
Thinking this further, the implementation could even be completely stateless now (as the padding has to be done by the caller anyways), only consisting of a single poly1305_tag function. The call-sites would then change from e.g.
0tag = Poly1305(mykey).add(mymsg_1).add(mymsg_2).add(mymsg_3).tag()
to
0tag = poly1305_tag(key=mykey, msg=mymsg_1+mymsg_2+mymsg_3)
acc a local variable in the tag method and removed the add function. I kept the class format for Poly1305 since it maybe neater to have the interface.
69+ def __init__(self, initial_key, rekey_interval=REKEY_INTERVAL):
70+ self._key = initial_key
71+ self.rekey_interval = rekey_interval
72+ self.block_counter = 0
73+ self.chunk_counter = 0
74+ self.keystream = b''
nit: I think all member variables are not meant to be accessed from the outside and could hence be prefixed with _ (same for the get_key_stream_bytes method).
0 self._key = initial_key
1 self._rekey_interval = rekey_interval
2 self._block_counter = 0
3 self._chunk_counter = 0
4 self._keystream = b''
_ too? i think i’ve only come across private functions prefixed with double _. so used that here.
are private functions prefixed with single _ too?
I think so, at least that’s what we also do for some tests and in the framework, see e.g $ git grep "def _[a-zA-Z]" ./test versus $ git grep "def __[a-zA-Z]" ./test (the latter yields results in the __foobar__ form for special operations like declaring ctors, dtors, operator overloading etc.). Using two underscores seems to be more than only a convention, triggering name mangling: https://stackoverflow.com/a/1301369
ACK 417eaf84a443967e27219b968be422770739495c
left two non-blocking nits below
Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>
ACK a10a9b1cadea0a5465573ff5fcac38b1cd3a9864
(the CI failure in “CI / Win64 native” seems to be unrelated to the changes in the PR)
Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>
Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>
Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>
Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>
Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>
45+def chacha20_block(key, nonce, cnt):
46+ """Compute the 64-byte output of the ChaCha20 block function.
47+ Takes as input a 32-byte key, 12-byte nonce, and 32-bit integer counter.
48+ """
49+ # Initial state.
50+ init = [0 for _ in range(16)]
In b8facb385234ae7a2b69d6cadbb7d795f3c9c6f0: non-blocker, just an idea to simplify it:
0diff --git a/test/functional/test_framework/crypto/chach
1a20.py b/test/functional/test_framework/crypto/chacha20.
2py
3index b45acd6463..de4d898921 100644
4--- a/test/functional/test_framework/crypto/chacha20.py
5+++ b/test/functional/test_framework/crypto/chacha20.py
6@@ -48,14 +48,11 @@ def chacha20_block(key, nonce, cnt):
7 Takes as input a 32-byte key, 12-byte nonce, and 32-bit integer counter.
8 """
9 # Initial state.
10- init = [0 for _ in range(16)]
11- for i in range(4):
12- init[i] = CHACHA20_CONSTANTS[i]
13- for i in range(8):
14- init[4 + i] = int.from_bytes(key[4 * i:4 * (i+1)], 'little')
15- init[12] = cnt
16- for i in range(3):
17- init[13 + i] = int.from_bytes(nonce[4 * i:4 * (i+1)], 'little')
18+ init = [0] * 16
19+ state[:4] = CHACHA20_CONSTANTS[:4]
20+ state[4:12] = [int.from_bytes(key[i:i+4], 'little') for i in range(0, 32, 4)]
21+ state[12] = cnt
22+ state[13:16] = [int.from_bytes(nonce[i:i+4], 'little') for i in range(0, 12, 4)]
23 # Perform 20 rounds.
24 state = list(init)
25 for _ in range(10):
