Skip to content

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

Closed
wants to merge 4 commits into from
Closed

Conversation

nikic
Copy link
Member

@nikic nikic commented Oct 6, 2020

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:

  • Does it make sense to rename $c -> $ciphertext in sodium_crypto_secretstream_xchacha20poly1305_pull? I believe that's what it is, but I'm not completely sure.
  • Is there any preference between the names $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.

@jedisct1
Copy link
Contributor

jedisct1 commented Oct 7, 2020

I'd recommend keeping the parameter names, that match the C prototypes and the libsodium documentation. Consistency is not a bad thing.

@jedisct1
Copy link
Contributor

jedisct1 commented Oct 7, 2020

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 (string is a little bit generic and actually misleading here), even though I would still suggest using the same names as in the C prototypes and documentation.

@nikic
Copy link
Member Author

nikic commented Oct 8, 2020

@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.

Copy link
Member

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

nikic added 4 commits October 13, 2020 09:43
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants