Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:90885
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.217.173 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <CAEKnhAFeMj0RgpjjZgOxzBwFRj0z6NbYPJL6WfR0EuT0daXCwA@mail.gmail.com>
References: <CAEKnhAHJW_v-yHx8ZJwu8O63Yfwc+9+bJnRoJwH7qXdzWE8T=w@mail.gmail.com>
 <CADyq6sKcmXSm9ouGS-K_+0hNm=vnUq4Fa_BpW=OHYZ1+b4V4uA@mail.gmail.com> <CAEKnhAFeMj0RgpjjZgOxzBwFRj0z6NbYPJL6WfR0EuT0daXCwA@mail.gmail.com>
Date: Sun, 24 Jan 2016 15:48:54 +0100
Message-ID: <CADyq6s+dL5R4K60yQwG_f_AsSUZ85nUGdeYfVgcMdZE=NW-qRA@mail.gmail.com>
To: Jakub Zelenka <bukka@php.net>
Cc: PHP internals list <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=089e0115fc2a5aed97052a158ec9
Subject: Re: [PHP-DEV] [RFC][VOTE] OpenSSL AEAD support
From: ocramius@gmail.com (Marco Pivetta)

--089e0115fc2a5aed97052a158ec9
Content-Type: text/plain; charset=UTF-8

On 24 January 2016 at 13:08, Jakub Zelenka <bukka@php.net> wrote:

> Hi,
>
> On 23 Jan 2016 21:01, "Marco Pivetta" <ocramius@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/

--089e0115fc2a5aed97052a158ec9--