Skip to content
This repository was archived by the owner on Aug 21, 2023. It is now read-only.
This repository was archived by the owner on Aug 21, 2023. It is now read-only.

Peer review #1 #3

Closed
Closed
@VSinerva

Description

@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!

Metadata

Metadata

Assignees

No one assigned

    Labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions