Hello Niels, Simo, Thanks for the comments. I've updated MR addressing part of them (see inline for the details).
Simo Sorce <s...@redhat.com> writes: > On Mon, 2024-01-15 at 21:43 +0100, Niels Möller wrote: >> Thanks, I've had a look, and it looks pretty good to me. Some comments >> and questions: >> >> * For tests, would it make some with some test that check that >> encryption with a given message and randomness gives the expected >> output? Even better if there are any authoritative testcases for that? I would be happy to add if there are any, even if they are not so authoritative, though I wasn't even able to find ones with compatible license, in particular with SHA-2 being used as an underlying hash algorithm for MGF-1. - Project Wycheproof (Apache 2.0): https://github.com/google/wycheproof/blob/master/testvectors/rsa_oaep_2048_sha256_mgf1sha256_test.json - Python Cryptography (Apache 2.0 and BSD): https://cryptography.io/en/latest/development/custom-vectors/rsa-oaep-sha2/ In any case, I'll try to check against those vectors manually outside the Nettle repository to ensure the correctness. >> * Is it useful to have oaep_decode_mgf1 and oaep_encode_mgf1 advertised >> as public functions, or would it be better to make them internal? Made them internal functions. >> * Do you see any reasonable (i.e., with a net gain in maintainability) >> way to share more code between _oaep_sec_decrypt_variable and >> _pkcs1_sec_decrypt_variable? > > I did review this part, and to me it seem like it is more maintainable > to keep them separate, they already are tricky as it is, adding more > variability sounds to me would just make them more complex and > difficult to reason about. I agree with that, considering potential optimization opportunities by the compiler. >> * For oaep_decode_mgf1, oaep_encode_mgf1, maybe one could let the caller >> allocate and pass in the appropriate hashing context? Would be easy to >> do, e.g., in rsa_oaep_sha512_decrypt. But it looks like that would be >> inconsistent with pss_mgf1, though (which looks like it needs a >> separate hashing context). Done. >> * I think it was a design mistake to represent RSA ciphertexts as mpz_t >> rather then octet strings in Nettle's original RSA interfaces. I >> wonder if it would make sense to let the new functions take >> octet strings instead? Done. Regards, -- Daiki Ueno _______________________________________________ nettle-bugs mailing list -- nettle-bugs@lists.lysator.liu.se To unsubscribe send an email to nettle-bugs-le...@lists.lysator.liu.se
