On 24 January 2016 at 13:08, Jakub Zelenka <bu...@php.net> wrote: > Hi, > > On 23 Jan 2016 21:01, "Marco Pivetta" <ocram...@gmail.com> wrote: > > > > Just FYI, I'm voting against this proposal, as the number of parameters > is simply growing out of control, which involves: > > - more BC breaks if default parameters change > > What bc breaks? There are no defaults except tag length and that will > never change for the curent cipher algs. >
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? > > - 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 > I'm not sure what your idea about AEAD function is > I actually have little to no knowledge on it: I'm voting purely on the proposed interface changes, which are objectively a bad approach to adding functionality to an existing API. > but if you mean an adding of a new function with the proposed signature, > then you would still have the same number of arguments > I am aware that AEAD applies (additionally) to all the previous parameters: can't use it without them. Having a separate function that just provides the AEAD functionality is still much better. > 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. > 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` > 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 ;-) Adapters are not nice code, but the consumer side should at least be nicer than the provider side. > I think that the main thing is that it adds the AEAD support and the users > could finally use it with just openssl ext. > Again: this is really just about the proposed API, not about the functionality itself. AEAD is very welcome, but the proposed API is just not a good solution to adding functionality. Cheers, Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/
