search

foundertherapy / django-cryptographic-fields

A set of fields that wrap standard Django fields with encryption provided by the python cryptography library.
MIT License
29 stars 34 forks source link

Add random salting to the encryption #5

Closed LucasRoesler closed 9 years ago

LucasRoesler commented 9 years ago
danaspiegel commented 9 years ago

@LucasRoesler I think the biggest issue with your changes is the nomenclature you use. I think all the passed variables currently called salt should be called salt_length to make it clear that the actual generation of the randomized salted value happens at encryption time, and that the developer is really just specifying the length of the salted bytes that's generated.

Further, I have a question re: how useful it is to allow changing of the salt length on a per field basis. We don't want the salted value to be easily brute force guessed, but is 10 bytes much different from 12 or 15? Why not just chose a secure length and make that the default so everyone can have a reasonable level of security. 99%+ of developers aren't going to have an opinion on the length of salt used.

cc: @levigross

LucasRoesler commented 9 years ago

I made some tweaks to the salting stuff. First, I made a poor naming decision, so I updated the parameter names to be clearer. Second, I changed how I was calculating the salt, so that I could more easily strip it out during the decrypt step. Hopefully the code is clearer now.

LucasRoesler commented 9 years ago

I am going to close this because I am silly and didn't read https://cryptography.io/en/latest/fernet/#implementation well enough, salting is already handled (properly I might add) by the cryptography library.