Chheang's Code Review Apr 12

[chheangdx]

I hope we don't have to do unit testing LOL

Overall from our discussion, seems like you are able to get the implementation working. You are just working on cleaning things up.

keycreationlib.py

1. In create_key, refactor name from hamc_hash to hmac_hash
2. For create_key, if you are trying to make it generic, I suggest only taking in secret, salt, and kdf type, and take all the other values as a single parameter under Options. This way, your library can be more modular to include other KDF algorithms in the future. https://pycryptodome.readthedocs.io/en/latest/src/protocol/kdf.html
3. I see multiple implementations here that generally do the same thing. I feel like create_key_with_pbkdf2 is redundant and should just be combined into the create_key method.

If you are interested in going further, I suggest encapsulating the methods into a Class, make create_key private (__create_key), and use create_key_with_pbkdf2 as your public method. Might be something to think of for the other parts of the program.

ciphershemes.py Suggestion to put your encrypt and decrypt methods here

Other I suggest putting your files such keycreationlib.py and ciphershemes.py into a folder so that the root directory just contains the readme, playpen, and main program.

[boscokclau]

Thanks for catching the typo. It is now fixed. I was debating if I wanted to use options. But then I started thinking a way to shield the application programmer, which I categorize them as UI code base from the encrption implemention detail, that's why the parameters are strings instead of actual modules. On the other hand, I am not a fan of using generic object as options, which the underlying code will need to do a lot of value checking. That being said, my intention is to make libraries plugable, but not making the API generic in a way that paramters, if coded in a strongly type language, of type objects. Anyhow, I hope I am express my consideration clearly. Please let me know if you have more thoughts on this.

3. The separation is intentional. create_key() is aganostic not only to libraries from PyCryptodome. It is possible, assuming there is no name conflict, or further modularize the code, that it can call corresponding method that will have library specify calls encapsulated only in the create_keywith*() function. Say, if I would ever replace PyCrytodome with another library, create_key_with_pbkdf() is the only place I need to modifiy.

Thanks for the time reviewing my code! It's helpful to encourage me to revisit the design.

[boscokclau]

@chheangdx I have committed new code pertained to creating encryotion key and hmac key. Both of which use create_key().

[chheangdx]

Update: Apr 16

1. Not sure why you have a separate file called config.py if it's not that big.
2. cryptoutil.py: Doesn't matter here because in most situation you do not run into an error. You should order your methods so that the methods (ex: __get_operation_parameters) called by other methods are written first. https://stackoverflow.com/questions/44423786/does-the-order-of-functions-in-a-python-script-matter
3. cryptoutil.py: Looks like the level of commenting for each method is inconsistent. encrypt and decrypt have large comment blocks, part of decrypt uses single line comments, and the utilities have no comments

This is all I have for you today. I skimmed through the other files and didn't see anything that caught my eye.

[boscokclau]

1. It had more. I moved most of them to filecrypto.ini. Probably a TODO: to move the remaining there.
2. The post is discussing the ordering in a different context. Order matters when it is between a function definition and a calling code. In that case, the definition must come before the calling code. The code in cryptutil.py is different. Function symbols are all read upfront when the module was first reference. At the time the calling code invokes the function, the proper symbols can be referenced. This allows code to have function order by convention that public functions will define before private functions.
3. Comments added. Thanks for keeping me honest.