On 24 Jan 2016 14:49, "Marco Pivetta" > > The BC breaks may happen in future, given the number of added parameters. You even designed your additional parameters to avoid BC breaks: what will happen next time someone needs additional functionality?
I'm not aware about any new functionality that could be added here in the near future and even if there was any, new parameters can still be added. I'm pretty sure that once this happens we will have already named params support which should address all your concerns. I actualy think that with the named params this will be much better API change than all other propasals. >> >> > - more security issues if the defaults are unsafe (or become unsafe, for whatever reason) >> >> the current value (16) is effective maximum for the currently supported AEAD cipher algs and it cannot change. > > "current" was used two times in this sentence: that's exactly my fear As said later in my previous email, the dafault can be different in dependency on cipher alg. It means that you will have to have default for AES GCM always 16 (it cannot be higher due to block size) and for some FUTURE XYZ it would be 32. There will never be need to change it and no bc break can happen. >> and the same defaults. It would just be a new function that would work only for selected cipher algs. It wouldn't address any of your concerns and would just duplicate things. > > It would reduce the extent of BC breaks, as you won't need to notify everybody using `openssl_encrypt`, but just those that use `openssl_encrypt_aead`. Also, you'd be able to make the parameters mandatory, which makes the proposed signature addition [, string &$tag = NULL [, string $aad = "" [, int $tag_length = 16 ]]]] much less confusing. You may even just create a `new AEADFlags()` struct to make the hinting more clear and flexible, so that optional params aren't a problem anymore, but that would simply anger who still thinks procedural programming is a maintainable solution. I'm sorry but I don't think we should add any objects here. OpenSSL ext is purely procedural and objects wouldn't fit. It would also require some code reordering and clean up. >> >> If you mean some fancy context related set of functions, then it would require some special handling and much more work. It would add to the maintenance effort as well. As I said before I have a crypto pecl extension that does exactly that. Personaly I think that such decision would probably mean that we won't have AEAD support in openssl ext for quite a long time as it won't be done anytime soon. > > No, I just really mean following: > > `openssl_encrypt_aead(string $data , string $method , string $password, AEADFlags $flags, [, int $options = 0 [, string $iv = "" ]] )` > > Much simpler: > > * the AEAD flags are required, less error prone (you can't pass in wrong flags unless you subclassed) > * the AEAD flags encapsulate the default values, which means that the BC breaks are easier to document, and easier to "fix" in userland (simply provide a custom AEADFlags instance, with the old params) > * the API wasn't butchered just to keep BC with `openssl_encrypt` As I said above, this wouldn't be procedural. It would also require significantly more time and doesn't bring anything extra useful. Everithing that you really need in most cases is just a tag anyway. Another thing is also a maintenance point of view. If you look to the commits and handling PR's for this ext, then it's probably just me who actively maintains it which is quite sad because I have just a little bit of time (usually after work when I work... :) ) and I also have other exts to care about. So the thing is that I don't really have time for such changes even if I thought that it is useful which I don't. And considering that no one else had time or knowledge to implement AEAD support to openssl ext in the last 10 years or so (since GCM is supported by OpenSSL), it might mean that it takes another 5 years waiting if this this proposal doesn't get in which wouldn't be great for users... >> >> That being said I know that the current proposal is not the nicest but I think it fits to the current openssl ext API which is not the nicest either. > > This is indeed an adapter: if OpenSSL sucks, then don't jump off the bridge as well ;-) I have never said that OpenSSL sucks. :) It's a great library written by one of the smartest people in the world. I meant that our openssl extension doesn't have the nicest API. However the API is more or less done in the same style which we shouldn't change IMHO. Cheers Jakub
