Peer review #1

[VSinerva]

Repository was downloaded at 2022-10-06 13:47

General feedback

In general, the code is clear and readable. The overall structure of the program is clear and well designed.

Tests and functionality

The tests all passed and the coverage is perfect. They tested relevant functionality, though the key generator testing could be more thorough (I'll get back to this later). The UI is clear and easy to use and the program functions as it should. I tend to prefer CLIs that clear the screen between commands (you can look at my project for some sense of how I've usually implemented this), though this is of course a matter of personal preference.

Specific problems

Crashing

You've already mentioned the issue regarding long messages, but in general I would add exception handling so the whole program doesn't crash if you do something wrong. For example, trying to encrypt a message before generating keys causes the program to crash, instead of just giving an error message to the user.

Readability of _egcd and _modinv

I found these functions to be difficult to read, since I was not familiar with the algorithms beforehand. Mostly that is not something your code could or should fix, but I would like a comment/docstring explaining the functions' roles in the algorithm, much like you have done with _miller_rabin.

Use of the random module

To keep the project manageable, some shortcuts are reasonable, even if they make the algorithm technically less secure. However, I think you could just as easily use the cryptographically-secure secrets module for generating random numbers. It doesn't actually matter in this case, of course, but would be in line with best practices.

Lengths of numbers

The biggest flaw I found in your code is the way you are generating p and q. Firstly, you have taken a number bit_length=512, which is half of the intended key length of 1024 and setting the lengths of p and qto be half of THAT, thus giving you an OVERALL key length <= 512. Assuming this was not intentional, this is where I think better testing would be useful. I would add tests for the keygen that verify the mathematical properties as well as possible (lengths are what they should be, possibly other easy-to-implement tests if you can think of them).

In addition to this problem, the way you are choosing p and q is flawed in another way. Generating n random bits, does NOT give you a key length of n, as the random bits can contain zeroes in the most significant bits (e.g. 00001011 is actually a 4 bit number). In practice the number of zeroes is limited by probability, but I was very quickly get the key length (length of n) down from 512 to 504 with just a few tries. This StackOverflow answer contains useful information of choosing p and q and this one should be helpful in switching to the secrets module, if you choose to do so.

Conclusion

Overall you've done excellent work implementing a rather math-heavy algorithm, well done! You've also written clear instructions which made peer review a breeze. I hope I was able to provide some useful feedback to improve you're project further!

[rikurauhala]

Hi there!

Thank you so much for the useful feedback. I will look into clearing the terminal between the commands. I actually tend to run the program with something like clear && poetry run invoke start so I might actually implement that change. Great tip!

I wasn't aware of the secrets module before, thank you for pointing this out. I will definitely look into it. Seems like it would be better suited for this kind of algorithm.

I'm aware of the issue related to the key length. It is actually recommended that p and q should vary in length by a few digits as it will make it harder to guess them as they are of slightly different lengths. Of course the overall key length should still be a constant. Also, the bit length of 512 is currently hard-coded because I was experimenting with different values and initially the program always printed out the keys and I felt the longer keys would clutter the console. However, I already changed this and forgot to set the bit length back to a higher value. I will probably implement a way to manually choose the key length from a couple of preset values, let's say 1024, 2048 or 4096.

Other issues you pointed out will also be taken into account. Once again, thank you for the feedback, it was actually very useful!