The mathematical properties of key generation should be more thoroughly tested.

The table should be updated after the test coverage has been improved.

As the goal is to practice algorithms and data structures, it would be interesting to implement my own character dictionary instead of using the built-in ord() and str() functions. A limited character set will also allow longer messages to be encrypted.

More testing is needed.

Currently the application has no exception handling.

The program will not work correctly if the message is too long.

Currently the program does not work with long messages.

The implementation document is still missing content. Should be done ASAP.

Other methods such as sieving should be considered to speed up the process of testing for primality.

The current version makes characters.py feel a bit cluttered. The values could be read from a separate file while initializing a new CharacterMap object.

Viimeisin commit 39fdadf

Moikka!

Projektin aihe on mielenkiintoinen ja tärkeä. Olen käyttänyt RSA-algoritmia itsekin, mutta sen toiminnan yksityiskohdat eivät olleet minulle entuudestaan tuttuja.

Dokumentaatio

Kaikki dokumentit on tehty todella huolellisesti ja niissä ohjelman toiminta on selitetty auki kattavasti.

Projektin rakenne ja koodin laatu

Projektin rakenne on järkevä ja johdonmukainen. Toiminnallisuus on paketoitu luokkien taakse, mikä helpottaa koodin lukemista. Metodit (myös yksityiset) ja luokat on kommentoitu kattavasti. Koodin tyyli ja nimeämiskäytännöt ovat johdonmukaisia.

Parannusehdotuksia:

• Joillain luokilla (esim. ConsoeIO) ei ole lainkaan tilaa, joten mielestäni voisi olla perusteltua käyttää niiden sijaan itsenäisiä funktioita samassa moduulissa. Vähintäänkin metodeista olisi mielestäni hyvä tehdä näiden luokkien kohdalla staattisia.
• Jonkin CLI-kirjaston käyttö (esim. Typer) voisi olla nykyistä helpompi vaihtoehto käyttöliittymän toteuttamiseen. En tosin ole varma, tukeeko Typer tai monet muut kirjastot mallia, jossa käyttäjä syöttää komentoja ajon aikana.
• Jotkin riippuvuudet (esim. colored) on merkitty dev-riippuvuuksiksi, vaikka ohjelma ei toimi ilman niitä.

Toiminnallisuus

Ohjelman tekstikäyttöliittymä on toimiva ja helppokäyttöinen. Ainoa itseäni vaivaava asia on se, että konsoli tyhjentyy aina ennen komennon suoritusta. Ohjelmaa olisi ehkä hieman helpompi käyttää, jos näkisi selaamatta aiemmin suorittamansa komennot.

Itse algoritmi toimii oman lyhyehkön testailuni perusteella oikein ja todella nopeasti. Ei tosin ainakaan vielä tue emojeita :(

Parannusehdotuksia:

• Konsolin tyhjennys pois tai asetuksen taakse
• Olisi kiva, jos ohjelma voisi lukea/kirjoittaa olemassaolevat avaimet jostain konfiguraatiotiedostosta, jolloin niitä ei tarvitsisi joka kerta generoida uudelleen.
• Ohjelman ohjelmallista käyttöä helpottaisi, jos käyttöliittymä toimisi siten, että käyttäjä antaa komennon ja saa siihen vastauksen, minkä jälkeen ohjelma sulkeutuu. Tämä tosin saattaisi vaatia aika paljon muutoksia.

Testit

Testit ovat selkeitä ja niistä käy hyvin ilmi kunkin testin tarkoitus. Kaipaisin ehkä hieman lisää yksikkötestejä Crypt ja KeyGenerator-luokille, mutta nykyisistäkin on varmasti apua ohjelman toimivuuden toteamisessa.

Parannusehdotuksia:

• pytest-benchmark voisi olla hyvä apukirjasto suorituskykytestaukseen, etenkin kun näyttää siltä, että algoritmia on tarve ajaa monta kierrosta hyvän suorituskykyarvion saamiseksi.

Mahdollisia bugeja

• Jos ajan esim echo 1 | poetry run invoke start ja yritän sulkea ohjelman painamalla q, suoritus näyttää jäätyvän.
• Ohjelma kaatuu, jos yrittää tulostaa avaimen ennen sen luomista.

Backspace doesn't appear to work with user input

Some of the dev dependencies should be listed as normal dependencies

Key length could be chosen by the user from a couple of preset values such as 1024, 2048 or 4096.

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!