Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Your encrypted storage can benefit from a binary encoding scheme #6

Closed
xadaemon opened this issue May 12, 2021 · 11 comments · Fixed by #7
Closed

Your encrypted storage can benefit from a binary encoding scheme #6

xadaemon opened this issue May 12, 2021 · 11 comments · Fixed by #7
Assignees
Labels
enhancement New feature or request

Comments

@xadaemon
Copy link
Contributor

xadaemon commented May 12, 2021

I would like to suggest at elara.elarautil.Util.encryptAndStore you use a binary encoding scheme like msgpack or to dump as json and encode as utf-8 and that will give you bytes to encrypt and then store while reliably being able to get the data back, the way it is right now you encode as ASCII (not compatible with utf-8 that might exist in the db) causing possible data loss, and saving the encoding to base64 round trip.

These are quite trivial changes, but I can help you implement them if you need help.
edit: also a hash/checksum to both encrypted and not ways might be a good idea.

@saurabh0719 saurabh0719 added the enhancement New feature or request label May 12, 2021
@saurabh0719
Copy link
Owner

Hi @DarthUdp

Great catch there :)
I'd be happy to assign this issue to you if you want change the encoding to make it utf friendly.

The checksum however is already on its way, I'll release an update asap.

@xadaemon
Copy link
Contributor Author

Do assign it then, it should be a quick fix

@saurabh0719
Copy link
Owner

@DarthUdp assigned it to you.

@xadaemon
Copy link
Contributor Author

@saurabh0719 there are some formatting issues with the code, as a suggestion if you don't mind, there is a tool called black that will fix and make all the code consistently formatted.

@xadaemon
Copy link
Contributor Author

And the docstrings at the files should be double quotes, sorry for the nitpicks :)

@saurabh0719
Copy link
Owner

And the docstrings at the files should be double quotes, sorry for the nitpicks :)

No problem, thanks for the tips :)

@saurabh0719
Copy link
Owner

@DarthUdp If you're still interested would you like to take up the checksum() function as well? I think you can do it with md5.hexdigest() from the hashlib library. I will probably shift my focus more towards formatting and fixing certain other issues that I have planned.

@xadaemon
Copy link
Contributor Author

@saurabh0719 Just a side note, md5 is considered deprecated and should not be used anymore, also it's a hashing algorithm, a checksum is something like crc32, if doing hashes something like sha-256 should be used if doing checksums crc32 is a popular choice.

@xadaemon
Copy link
Contributor Author

Just open another issue and assign me I will have a look when I have some spare time tomorrow

@saurabh0719
Copy link
Owner

@saurabh0719 Just a side note, md5 is considered deprecated and should not be used anymore, also it's a hashing algorithm, a checksum is something like crc32, if doing hashes something like sha-256 should be used if doing checksums crc32 is a popular choice.

Yes good point, I ended up reading a bit about crc

@saurabh0719
Copy link
Owner

Just open another issue and assign me I will have a look when I have some spare time tomorrow

I've opened a new issue, unable to assign it to you however (idk why), but I've mentioned you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants