Fix GH-7737: openssl_seal/openssl_open do not handle tagged algorithms #20687
Conversation
…thms This commit adds a seventh parameter to both two OpenSSL functions: * openssl_seal(): The new parameter is by-ref and is populated with the computed tag. * openssl_open(): The new parameter is by-value to provide the computed tag. Closes phpGH-7737
|
Merging a new feature into an extension you don't "own" without reviews, way to go. |
|
I guess Sara just didn't realise that things have changed since her previous contributions and we usually wait for reviews for a bit longer than a day... 😄 To be fair those are really unwritten rules and we should probably set some policy. I think it would good to require review from code owners or wait for some time. From a quick look, it looks mostly ok except some minor things and relatively poor test. I'm also not sure if it should be introduced without ability to set tag length and AAD which would be more consistent if openssl_encrypt / openssl_decrypt. I will think about it and will do some post review. It might potentially need reverting if there are issues. |
|
Or we might just do extra changes. Although the preference is usually to do it in the initial review. But if others have any objections, we should probably revert it stright away. Personally I don't mind if it stays in master for now. |
|
Thinking about it and checking it more, just tag is fine - we can always add other bits later and it's good that we support the main use case so let's leave it. The only things that I can see are just details and test can be improved later - it still proves that GCM can be used which is probably fine as we don't need to test that much like for encrypt / decrypt. Just please wait a bit longer next time. :) |
|
Ah @shivammathur just pinged me that |
|
Reverted |
3 participants
This commit adds a seventh parameter to both two OpenSSL functions:
Closes GH-7737