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

Drop python-ecdsa and port all crypto operations to libsodium #24

Merged
merged 12 commits into from
Jan 30, 2024

Conversation

lsd-cat
Copy link
Member

@lsd-cat lsd-cat commented Oct 30, 2023

See here #23.

We might want to test some more before merging, especially functionalities outside the demo.sh, such as attachments.

Copy link
Contributor

@eaon eaon left a 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

Comment on lines +66 to +69
# 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)
Copy link
Contributor

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?

Copy link
Member Author

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

@lsd-cat
Copy link
Member Author

lsd-cat commented Oct 30, 2023

Thanks @eaon I agree with uncommenting. I will push a couple more changes tomorrow dropping the journalist_uid code-wise, since Ed25519 public keys are already 32 bytes long, there is no purpose of using an hash as a key id rather than the key itself, plus it adds sha3 as a dependency in case we want to interact with other programming languages (I am doing some cross-language experiments with libsodium bindings).

@lsd-cat
Copy link
Member Author

lsd-cat commented Oct 31, 2023

Following #23 (comment) we might want to rethink the usage of libsodium Box for source to journalist message encryption .

@eaon
Copy link
Contributor

eaon commented Nov 1, 2023

What about using SealedBox?

@lsd-cat
Copy link
Member Author

lsd-cat commented Jan 24, 2024

Opting to merge this while we complete the protocol discussion here #30

Copy link
Member

@cfm cfm left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants