pynacl icon indicating copy to clipboard operation
pynacl copied to clipboard

crypto_box_easy support

Open gonatienza opened this issue 1 year ago • 1 comments

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 avatar Aug 16 '24 03:08 gonatienza

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.

reaperhulk avatar Aug 30 '24 14:08 reaperhulk

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.

gonatienza avatar Aug 30 '24 20:08 gonatienza

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

reaperhulk avatar Sep 01 '24 23:09 reaperhulk