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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
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.
<!--174a7506f384e20aa4161008e828411d-->
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)
hex_key, nonce_pair, counter, hex_output = test_vector
oops, done.
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):
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. 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.
Yeah, I think having HKDF in either v2_p2p.py or in a (separate) crypto/ module would make more sense than in key.py.
done. used a separate 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
enc_aead.crypt(b"", b"", is_decrypt=False)
done.
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)
Follow-up idea: it might be nice to also test that the decryption fails (i.e. returns None due to poly1305 tag mismatch) if a random bit in ciphertext or aad is flipped.
can be done in a follow up if it's desirable to have more tests.
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
small nit: not sure if it's really worth to change it, but in Python member variables and methods that shouldn't be accessed from the outside are usually prefixed with an underscore (_) (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.
done.
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.
Makes sense; 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.
true, makes sense to have a general poly1305. used a separate 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"""
(Here and in other files) Perhaps add a similar warning as 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.
done.
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)]
Nit: maybe slightly more readable self.msg[i * 16:min(length, (i + 1) * 16)].
nice, done.
ACK fb462495adcab99c575f49d4d069214aec898779. Left some nits.
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.
tag = Poly1305(mykey).add(mymsg_1).add(mymsg_2).add(mymsg_3).tag()
to
tag = poly1305_tag(key=mykey, msg=mymsg_1+mymsg_2+mymsg_3)
true, made 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).
self._key = initial_key
self._rekey_interval = rekey_interval
self._block_counter = 0
self._chunk_counter = 0
self._keystream = b''
are private functions prefixed with single _ 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
interesting! done.
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:
diff --git a/test/functional/test_framework/crypto/chach
a20.py b/test/functional/test_framework/crypto/chacha20.
py
index b45acd6463..de4d898921 100644
--- a/test/functional/test_framework/crypto/chacha20.py
+++ b/test/functional/test_framework/crypto/chacha20.py
@@ -48,14 +48,11 @@ def chacha20_block(key, nonce, cnt):
Takes as input a 32-byte key, 12-byte nonce, and 32-bit integer counter.
"""
# Initial state.
- init = [0 for _ in range(16)]
- for i in range(4):
- init[i] = CHACHA20_CONSTANTS[i]
- for i in range(8):
- init[4 + i] = int.from_bytes(key[4 * i:4 * (i+1)], 'little')
- init[12] = cnt
- for i in range(3):
- init[13 + i] = int.from_bytes(nonce[4 * i:4 * (i+1)], 'little')
+ init = [0] * 16
+ state[:4] = CHACHA20_CONSTANTS[:4]
+ state[4:12] = [int.from_bytes(key[i:i+4], 'little') for i in range(0, 32, 4)]
+ state[12] = cnt
+ state[13:16] = [int.from_bytes(nonce[i:i+4], 'little') for i in range(0, 12, 4)]
# Perform 20 rounds.
state = list(init)
for _ in range(10):
done.
re-ACK c534c0871038ded72dc9078cc91e030ceb746196
utACK c534c0871038ded72dc9078cc91e030ceb746196
ACK c534c0871038ded72dc9078cc91e030ceb746196