Improve parameter handling in ext/openssl #6025
Conversation
kocsismate
force-pushed
the
openssl-param-handling
branch
6 times, most recently
from
ef48987 to
3399178
Compare
|
@bukka Can you please have a look at this PR, especially at the parameter names? There are quite a few cases where I'm not very confident, so please take the current names just as a starting point. |
ext/openssl/openssl.stub.php
Outdated
| */ | ||
| function openssl_pkey_export($key, &$out, ?string $passphrase = null, ?array $configargs = null): bool {} | ||
| function openssl_pkey_export($private_key, &$output, ?string $passphrase = null, ?array $options = null): bool {} |
There was a problem hiding this comment.
isn't this working for public_key as well? Maybe we should just call it pkey in the same way as it's called internally and in openssl.
There was a problem hiding this comment.
According to the php_openssl_pkey_from_zval(zpkey, 0, passphrase, passphrase_len); invocation, it isn't. But I'll double check it.
There was a problem hiding this comment.
Yes, even in the ext/openssl/tests/openssl_pkey_export_basic.phpt test, a public key is used with openssl_pkey_get_details()
There was a problem hiding this comment.
Ok I see it in the code but it's a bit artificial as it could be possible to extend it and support pubkey using PEM_write_bio_PUBKEY. Then the name wouldn't be correct and we couldn't easily change it. As pkey is already in the function name, I think it should be fine to rename the argument to pkey. It should be done for other cases as well.
There was a problem hiding this comment.
I think this is the last remaining (important) question. @nikic Do you have any preference? To be honest, I don't like the pkey name at all, because it made me google for quite long whether it refers to a private or a public key when I was working on the resource migration. :( So I'd favor a $key, $asymmetric_key name if we really want to support public keys too.
There was a problem hiding this comment.
Just $key sounds reasonable to me here. But really, no particular preference. I do think we should use $public_key and $private_key in the APIs that only accept one of them.
There was a problem hiding this comment.
I went with $key in case of openssl_pkey_export() and openssl_pkey_export_to_file() to be a bit more forward-compatible :)
kocsismate
force-pushed
the
openssl-param-handling
branch
2 times, most recently
from
87c9016 to
ec9c9ea
Compare
kocsismate
force-pushed
the
openssl-param-handling
branch
2 times, most recently
from
9f4874b to
33343b3
Compare
kocsismate
force-pushed
the
openssl-param-handling
branch
from
33343b3 to
69366ac
Compare
ext/openssl/openssl.c
Outdated
| @@ -7296,7 +7292,7 @@ PHP_FUNCTION(openssl_decrypt) | |||
| size_t data_len, method_len, password_len, iv_len = 0, tag_len = 0, aad_len = 0; | |||
| zend_string *ret; | |||
|
|
|||
| if (zend_parse_parameters(ZEND_NUM_ARGS(), "sss|lsss", &data, &data_len, &method, &method_len, | |||
| if (zend_parse_parameters(ZEND_NUM_ARGS(), "sss|lss!s", &data, &data_len, &method, &method_len, | |||
There was a problem hiding this comment.
Is it possible to use $tag = "" instead?
There was a problem hiding this comment.
Yes, it is, based on } else if (!enc && tag && tag_len > 0) { in php_openssl_cipher_init().
| */ | ||
| function openssl_seal(string $data, &$sealdata, &$ekeys, array $pubkeys, string $method, &$iv = UNKNOWN): int|false {} | ||
| function openssl_seal(string $data, &$sealed_data, &$encrypted_keys, array $public_key, string $cipher_algorithm, &$initialization_vector = null): int|false {} |
There was a problem hiding this comment.
| function openssl_seal(string $data, &$sealed_data, &$encrypted_keys, array $public_key, string $cipher_algorithm, &$initialization_vector = null): int|false {} | |
| function openssl_seal(string $data, &$sealed_data, &$encrypted_keys, array $public_key, string $cipher_algorithm, &$iv = null): int|false {} |
IV is typically not spelled out.
There was a problem hiding this comment.
I don't feel particularly strongly about this one though. Maybe @bukka has a preference.
kocsismate
force-pushed
the
openssl-param-handling
branch
from
e17d3a4 to
f96620d
Compare
ext/openssl/openssl.c
Outdated
| @@ -4847,7 +4849,7 @@ PHP_FUNCTION(openssl_pkcs7_verify) | |||
|
|
|||
| RETVAL_LONG(-1); | |||
|
|
|||
| if (zend_parse_parameters(ZEND_NUM_ARGS(), "pl|pappp", &filename, &filename_len, | |||
| if (zend_parse_parameters(ZEND_NUM_ARGS(), "pl|p!a!p!p!p!", &filename, &filename_len, | |||
There was a problem hiding this comment.
As far as I can see $cainfo = [] should work.
There was a problem hiding this comment.
Though looking at other signatures, I see that this parameter is declared as ?array in other cases, so this makes sense for consistency. Though the default is sometimes null and sometimes []...
There was a problem hiding this comment.
You are right, so I'll change the default value of all of them to [].
| /** @param OpenSSLAsymmetricKey $privkey */ | ||
| function openssl_csr_new(array $dn, &$privkey, ?array $configargs = null, ?array $extraattribs = null): OpenSSLCertificateSigningRequest|false {} | ||
| /** @param OpenSSLAsymmetricKey $private_key */ | ||
| function openssl_csr_new(array $distinguished_names, &$private_key, ?array $options = null, ?array $extra_options = null): OpenSSLCertificateSigningRequest|false {} |
There was a problem hiding this comment.
So in this case, we are really talking about extra attributes (not options) because those are CSR attributes.
|
|
||
| function openssl_pkcs7_verify(string $filename, int $flags, string $signerscerts = UNKNOWN, array $cainfo = UNKNOWN, string $extracerts = UNKNOWN, string $content = UNKNOWN, string $pk7 = UNKNOWN): bool|int {} | ||
| function openssl_pkcs7_verify(string $filename, int $flags, ?string $output_filename = null, array $ca_info = [], ?string $untrusted_certificates_filename = null, ?string $content = null, ?string $pk7_filename = null): bool|int {} |
There was a problem hiding this comment.
If you are already changing that parameter, then pk7 is of course incorrect as it doesn't mean anything. It should be pkcs7_filename
There was a problem hiding this comment.
From the really quick look I'm not 100% sure what's the use case of that param. It's a bit confusing because output_filename is not really right name here. It should be probably called signers_certificates_filename and the last one should be probably pkcs7_output_filename. Although it's a bit inconsistent with other naming.
There was a problem hiding this comment.
also first param should be then pkcs7_input_filename which will make it a bit more consistent.
| /** @param OpenSSLCertificate|array|string $recipcerts */ | ||
| function openssl_pkcs7_encrypt(string $infile, string $outfile, $recipcerts, ?array $headers, int $flags = 0, int $cipher = OPENSSL_CIPHER_RC2_40): bool {} | ||
| /** @param OpenSSLCertificate|array|string $certificate */ | ||
| function openssl_pkcs7_encrypt(string $filename, string $output_filename, $certificate, ?array $headers, int $flags = 0, int $cipher_algorithm = OPENSSL_CIPHER_RC2_40): bool {} |
There was a problem hiding this comment.
Maybe we should make clear what's pkcs7 and name output_filename as pkcs7_output_filename. The first param should be input_filename then.
| /** @param OpenSSLAsymmetricKey|OpenSSLCertificate|array|string $signkey */ | ||
| function openssl_pkcs7_sign(string $infile, string $outfile, OpenSSLCertificate|string $signcert, $signkey, ?array $headers, int $flags = PKCS7_DETACHED, ?string $extracertsfilename = null): bool {} | ||
| /** @param OpenSSLAsymmetricKey|OpenSSLCertificate|array|string $private_key */ | ||
| function openssl_pkcs7_sign(string $filename, string $output_filename, OpenSSLCertificate|string $certificate, $private_key, ?array $headers, int $flags = PKCS7_DETACHED, ?string $untrusted_certificates_filename = null): bool {} |
There was a problem hiding this comment.
Here again input_filename and then pkcs7_output_filename
| */ | ||
| function openssl_pkcs7_decrypt(string $infilename, string $outfilename, $recipcert, $recipkey = null): bool {} | ||
| function openssl_pkcs7_decrypt(string $filename, string $output_filename, $certificate, $private_key = null): bool {} |
| /** @param array $certs */ | ||
| function openssl_pkcs7_read(string $infilename, &$certs): bool {} | ||
| /** @param array $certificates */ | ||
| function openssl_pkcs7_read(string $filename, &$certificates): bool {} |
| function openssl_cms_verify(string $filename, int $flags = 0, ?string $signerscerts = null, ?array $cainfo = null, ?string $extracerts = null, ?string $content = null, ?string $pk7 = null, ?string $sigfile = null, int $encoding = OPENSSL_ENCODING_SMIME): bool {} | ||
| function openssl_cms_verify(string $filename, int $flags = 0, ?string $certificates = null, array $ca_info = [], ?string $untrusted_certificates_filename = null, ?string $content = null, ?string $pk7 = null, ?string $sigfile = null, int $encoding = OPENSSL_ENCODING_SMIME): bool {} | ||
|
|
||
| /** @param OpenSSLCertificate|array|string $recipcerts */ | ||
| function openssl_cms_encrypt(string $infile, string $outfile, $recipcerts, ?array $headers, int $flags = 0, int $encoding = OPENSSL_ENCODING_SMIME, int $cipher = OPENSSL_CIPHER_RC2_40): bool {} | ||
| /** @param OpenSSLCertificate|array|string $certificate */ | ||
| function openssl_cms_encrypt(string $filename, string $output_filename, $certificate, ?array $headers, int $flags = 0, int $encoding = OPENSSL_ENCODING_SMIME, int $cipher_algorithm = OPENSSL_CIPHER_RC2_40): bool {} | ||
|
|
||
| /** @param OpenSSLAsymmetricKey|OpenSSLCertificate|array|string $signkey */ | ||
| function openssl_cms_sign(string $infile, string $outfile, OpenSSLCertificate|string $signcert, $signkey, ?array $headers, int $flags = 0, int $encoding = OPENSSL_ENCODING_SMIME, ?string $extracertsfilename = null): bool {} | ||
| /** @param OpenSSLAsymmetricKey|OpenSSLCertificate|array|string $private_key */ | ||
| function openssl_cms_sign(string $filename, string $output_filename, OpenSSLCertificate|string $certificate, $private_key, ?array $headers, int $flags = 0, int $encoding = OPENSSL_ENCODING_SMIME, ?string $untrusted_certificates_filename = null): bool {} | ||
|
|
||
| /** | ||
| * @param OpenSSLCertificate|string $recipcert | ||
| * @param OpenSSLAsymmetricKey|OpenSSLCertificate|array|string $recipkey | ||
| * @param OpenSSLCertificate|string $certificate | ||
| * @param OpenSSLAsymmetricKey|OpenSSLCertificate|array|string|null $private_key | ||
| */ | ||
| function openssl_cms_decrypt(string $infilename, string $outfilename, $recipcert, $recipkey = UNKNOWN, int $encoding = OPENSSL_ENCODING_SMIME): bool {} | ||
| function openssl_cms_decrypt(string $filename, string $output_filename, $certificate, $private_key = null, int $encoding = OPENSSL_ENCODING_SMIME): bool {} | ||
|
|
||
| /** @param array $certs */ | ||
| function openssl_cms_read(string $infilename, &$certs): bool {} | ||
| /** @param array $certificates */ | ||
| function openssl_cms_read(string $filename, &$certificates): bool {} |
There was a problem hiding this comment.
basically the same names like for pkcs7 but using cms instead.
There was a problem hiding this comment.
Do adding the pkcs7 and cms prefixes make the meaning of $input_filename/$output_filename more clear?
| */ | ||
| function openssl_open(string $data, &$opendata, string $ekey, $privkey, string $method, string $iv = UNKNOWN): bool {} | ||
| function openssl_open(string $data, &$output, string $encrypted_key, $private_key, string $cipher_algorithm, ?string $initialization_vector = null): bool {} |
|
|
||
| /** @param string $tag */ | ||
| function openssl_encrypt(string $data, string $method, string $password, int $options = 0, string $iv = '', &$tag = UNKNOWN, string $aad = '', int $tag_length = 16): string|false {} | ||
| function openssl_encrypt(string $data, string $cipher_algorithm, string $passphrase, int $options = 0, string $initialization_vector = "", &$tag = null, string $additional_authentication_data = "", int $tag_length = 16): string|false {} |
There was a problem hiding this comment.
please use iv and aad as they are standard terms.
|
|
||
| function openssl_decrypt(string $data, string $method, string $password, int $options = 0, string $iv = '', string $tag = UNKNOWN, string $aad = ''): string|false {} | ||
| function openssl_decrypt(string $data, string $cipher_algorithm, string $passphrase, int $options = 0, string $initialization_vector = "", string $tag = "", string $additional_authentication_data = ""): string|false {} |
There was a problem hiding this comment.
please use iv and aad as they are standard terms.
There was a problem hiding this comment.
I'm waiting for the end of your review, and commit the changes together.
|
@kocsismate Nice work in general but this still needs some additional changes. Please let me know if you are able to create a PR or if I need to do that. |
|
@bukka I can create one! |
|
Nice thanks |
| */ | ||
| function openssl_private_decrypt(string $data, &$crypted, $key, int $padding = OPENSSL_PKCS1_PADDING): bool {} | ||
| function openssl_private_decrypt(string $data, &$encrypted_data, $private_key, int $padding = OPENSSL_PKCS1_PADDING): bool {} |
There was a problem hiding this comment.
This is really other way round. First param is from (encrypted data) and the second param is to which is decrypted data. If you want to keep first as data, then the second one should be decrypted_data.
There was a problem hiding this comment.
Oops, it's very fortunate that you had another look :) Thank you!
| */ | ||
| function openssl_public_decrypt(string $data, &$crypted, $key, int $padding = OPENSSL_PKCS1_PADDING): bool {} | ||
| function openssl_public_decrypt(string $data, &$encrypted_data, $public_key, int $padding = OPENSSL_PKCS1_PADDING): bool {} |
There was a problem hiding this comment.
Again the second one should be decrypted_data
|
@kocsismate I think that's everything for today. It's a bit hard to get the correct naming as even the names in C code are a bit confusing. Will take another look when you create the next PR. |
This PR:
UNKNOWNdefault valuesstring|intZPP when possible