-
Notifications
You must be signed in to change notification settings - Fork 1
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
Drop python-ecdsa and port all crypto operations to libsodium #24
Conversation
…tead of the main long term journalist signing key
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! 🎉 There's a couple of comments that it seems like you overlooked, but otherwise this is much easier to understand than before.
Other than that (I didn't make inline notes about it because it also isn't directly an issue of this PR) there's a couple of abbreviations that aren't easy to understand/infer the meaning of. gdh
for example may be misinterpreted as Gap-DH, which we don't use. So just noting that before the imminent freeze, it might still make sense to try and find clearer terminology
# signing_pivate_key.verify_key.verify(sig, encoder=Base64Encoder) | ||
# sooo the message can be base64 but the signature has to be byes, so the encoder | ||
# is applied only to the message apparently | ||
# signing_pivate_key.verify_key.verify(sig.message, b64decode(sig.signature), encoder=Base64Encoder) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intended as a form of documentation or did you overlook this before committing it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it was the late night, but I lost a good half an hour debugging that out so I left it as a reminder, even though I agree it is poorly written
Thanks @eaon I agree with uncommenting. I will push a couple more changes tomorrow dropping the |
…ource key derivation is now blake2b from libsodium
Following #23 (comment) we might want to rethink the usage of libsodium Box for source to journalist message encryption . |
What about using SealedBox? |
Opting to merge this while we complete the protocol discussion here #30 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reviewed this at a high level, to understand how the implementation changes from mixed ecdsa
+nacl
to all nacl
. I'm happy to approve and merge it for ongoing evaluation of the proof of concept.
See here #23.
We might want to test some more before merging, especially functionalities outside the
demo.sh
, such as attachments.