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

crypto_box_easy support #835

Merged
merged 1 commit into from
Sep 1, 2024
Merged

Conversation

gonatienza
Copy link
Contributor

@gonatienza gonatienza commented Aug 16, 2024

How about adding these bindings to support these newer apis:

int crypto_box_easy(unsigned char *c, const unsigned char *m,
                    unsigned long long mlen, const unsigned char *n,
                    const unsigned char *pk, const unsigned char *sk);

int crypto_box_open_easy(unsigned char *m, const unsigned char *c,
                         unsigned long long clen, const unsigned char *n,
                         const unsigned char *pk, const unsigned char *sk);

int crypto_box_easy_afternm(unsigned char *c, const unsigned char *m,
                            unsigned long long mlen, const unsigned char *n,
                            const unsigned char *k);

int crypto_box_open_easy_afternm(unsigned char *m, const unsigned char *c,
                                 unsigned long long clen, const unsigned char *n,
                                 const unsigned char *k);

int crypto_secretbox_easy(unsigned char *c, const unsigned char *m,
                          unsigned long long mlen, const unsigned char *n,
                          const unsigned char *k);

int crypto_secretbox_open_easy(unsigned char *m, const unsigned char *c,
                               unsigned long long clen, const unsigned char *n,
                               const unsigned char *k);

And updating SecretBox's and Box's default encrypt/decrypt methods. Did some basic testing for backwards compatibility if someone has a saved key and encrypted message. Plus it manifested better performance.

The path through the library is different:

The original NaCl crypto_box API is also supported, albeit not recommended.

crypto_box() takes a pointer to 32 bytes before the message and stores the ciphertext 16 bytes after the destination pointer, with the first 16 bytes being overwritten with zeros. crypto_box_open() takes a pointer to 16 bytes before the ciphertext and stores the message 32 bytes after the destination pointer, overwriting the first 32 bytes with zeros.

The _easy and _detached APIs are faster and improve usability by not requiring padding, copying, or tricky pointer arithmetic.

(from here)

The original NaCl crypto_secretbox API is also supported, albeit not recommended.

crypto_secretbox() takes a pointer to 32 bytes before the message, and stores the ciphertext 16 bytes after the destination pointer, the first 16 bytes being
overwritten with zeros. crypto_secretbox_open() takes a pointer to 16 bytes before the ciphertext and stores the message 32 bytes after the destination pointer,
overwriting the first 32 bytes with zeros.

The _easy and _detached APIs are faster and improve usability by not requiring padding, copying or tricky pointer arithmetic.

(from here)

Let me know.

HTH.

@gonatienza gonatienza force-pushed the crypto_box_easy branch 2 times, most recently from a30c143 to b76d15e Compare August 16, 2024 17:07
@reaperhulk
Copy link
Member

Looks like migrating to _easy variants is the right move here. Thanks for doing all this work, and apologies for taking so long to review it. PyNaCl doesn't get the attention it needs from us.

Should we be looking at removing the old variants entirely? We don't publicly document bindings so if they're not part of our public surface area and we can migrate all callers that may be a good idea.

@gonatienza
Copy link
Contributor Author

No problem whatsoever, @reaperhulk. Happy to contribute as much as I can.

In regards to your question, although the final call is really yours, I personally would not remove the old bindings. Besides the fact that someone may be using them directly, the old api's are still supported by libsodium. Eventually, if they get deprecated, I would take them off at that point.

I know the bindings are not publicly documented to be used directly, but they are documented in docstring and not generally defined as private modules/functions (e.g., nacl._bindings or nacl.bindings._crypto_box) which would imply to stay away from using them directly. Which is a good thing IMHO. I found that if you are working between different bindings (like encrypting through libsodium-js and decrypting through pynacl) you would potentially like to use the bindings directly to be consistent across your implementations of the libsodium api's.

I would definitely change the default api to be used by the high level wrappers (as committed), which I guess is probably what most people will leverage anyways.

HTH.

@reaperhulk reaperhulk merged commit e47e41e into pyca:main Sep 1, 2024
31 checks passed
@reaperhulk
Copy link
Member

Thanks for the contribution, it's great to see high quality contributions like this 😄

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

Successfully merging this pull request may close these issues.

2 participants