-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Update ext/sodium parameter names #6279
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
Conversation
I'd recommend keeping the parameter names, that match the C prototypes and the libsodium documentation. Consistency is not a bad thing. |
Oh, scratch that, I didn't notice that this file didn't come from the PECL code and already had parameter names that don't match the parameter names in the actual functions at all. Well... these changes are definitely an improvement ( |
@jedisct1 The problem here is that the parameter names in the libsodium C prototypes are generally single character abbreviations, which would not fit in well with naming in other PHP extensions. For example, this is the crypto_aead_aes256gcm_decrypt() prototype: int crypto_aead_aes256gcm_decrypt(unsigned char *m,
unsigned long long *mlen_p,
unsigned char *nsec,
const unsigned char *c,
unsigned long long clen,
const unsigned char *ad,
unsigned long long adlen,
const unsigned char *npub,
const unsigned char *k) The libsodium documentation doesn't list actual prototypes, and only provides example usages instead. For this function it would be (from https://libsodium.gitbook.io/doc/secret-key_cryptography/aead/aes-256-gcm): crypto_aead_aes256gcm_decrypt(decrypted, &decrypted_len,
NULL,
ciphertext, ciphertext_len,
ADDITIONAL_DATA,
ADDITIONAL_DATA_LEN,
nonce, key) And the proposed signature here is: function sodium_crypto_aead_aes256gcm_decrypt(string $ciphertext,
string $additional_data,
string $nonce,
string $key): string|false {} Which does not match the original C prototype, but does match the terminology used in the libsodium documentation examples. |
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.
The changes look pretty good to me.
Some of original parameter names weren't correct, e.g. some functions using $keypair even though they only accept a single key. Fix a number of such issues after cross-checking with the libsodium docs and the implementation.
Update ext/sodium parameter names in preparation for names parameters support. Things look pretty good here, so this just tweaks some names to align with what we use elsewhere.
I have two questions for @jedisct1:
$c
->$ciphertext
insodium_crypto_secretstream_xchacha20poly1305_pull
? I believe that's what it is, but I'm not completely sure.$ad
,$aad
(we use this one in ext/openssl) or$additional_data
? I went for$additional_data
here, but I would be fine with any myself.