Everything

php

Home » Project » Newsgroups » php.internals » [RFC][VOTE] OpenSSL AEAD support »

[RFC][VOTE] OpenSSL AEAD support

php.internals

Jakub Zelenka

10 years ago

Hi,

The vote is now open:

https://wiki.php.net/rfc/openssl_aead#voting

Cheers

Jakub

Marco Pivetta

10 years ago

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
- more security issues if the defaults are unsafe (or become unsafe, for
whatever reason)
- more cognitive load (it's arguably more complex)

In general, this simply pushes too many responsibilities on the interface.
Support for AEAD can be moved to a separate function, which involves a
minor BC break: collision with pre-existing functions, if anybody was
crazy/stupid enough to implement crypto in userland, and even worse in the
global namespace.


Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

On 20 January 2016 at 17:40, Jakub Zelenka <bukka@php.net> wrote:

Jakub Zelenka

10 years ago

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.

>  - more security issues if the defaults are unsafe (or become unsafe, for

whatever reason)

The tag length default can be dependent on a cipher algorithm of course.
That would address all concerns because the current value (16) is effective
maximum for the currently supported AEAD cipher algs and it cannot change.
Another solution could be to have no default but that would make it much
harder for users to use as it would require an understanding what the max
and min is for the selected cipher algs.

In any case I'm not sure how it is related to the growing number of
function parameters...

>  - more cognitive load (it's arguably more complex)
>
> In general, this simply pushes too many responsibilities on the interface.
> Support for AEAD can be moved to a separate function, which involves a

minor BC break: collision with pre-existing functions, if anybody was
crazy/stupid enough to implement crypto in userland, and even worse in the
global namespace.

I'm not sure what your idea about AEAD function is but if you mean an
adding of a new function with the proposed signature, then you would still
have the same number of arguments 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.

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.

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. I think that the main thing is that it adds the AEAD support and
the users could finally use it with just openssl ext.

Cheers

Jakub

Marco Pivetta

10 years ago

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/

Jakub Zelenka

10 years ago

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

Marco Pivetta

10 years ago

On 24 January 2016 at 20:34, Jakub Zelenka <bukka@php.net> wrote:

> 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.
>

Exactly: like anyone else, you are not yet aware about it. What happens in
5 years?

> >>
> >> >  - 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.
>

Doesn't really matter if you design it with an object or with optional
parameters, or even with an array of `$options`, as long as you can enforce
correct types passed in, and the order of the default parameter values
doesn't become a mess (as it currently is becoming).

> >>
> >> 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.
>

Again, the above was just a suggestion: feel free to do whatever you
prefer, but in a new endpoint.

> 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.
>

I write loads of stuff that may or may not suck from a consumer
perspective. I am blaming the API, not the developer, let's not bring it to
a personal level.

We are designing (changing, eek!) an API here: the fact that it may or not
reflect how OpenSSL itself exposes it is totally irrelevant.

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

Jakub Zelenka

10 years ago

On 24 Jan 2016 19:44, "Marco Pivetta" <ocramius@gmail.com> wrote:

>
> On 24 January 2016 at 20:34, Jakub Zelenka <bukka@php.net> wrote:
>>
>> 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.

>
>
> Exactly: like anyone else, you are not yet aware about it. What happens

in 5 years?

Of course I don't know what happens in 5 years but even if we needed to add
new parameters, it wouldn't be a BC break.

>>
>> 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.

>
> Doesn't really matter if you design it with an object or with optional

parameters, or even with an array of `$options`, as long as you can enforce
correct types passed in, and the order of the default parameter values
doesn't become a mess (as it currently is becoming).

I don't think that array would be an option as it's not realy nice when a
tag would be returned into it (tag is a ref and filled after encryption)...
the correct types are already enforced in the current implementation
(warning is triggered if they are not correct). About the order, the only
default is the last paremeter so I don't see any issue here either. As I
said I really think that we will get named params before there is any need
to add another param. After that the current API will be really good (just
look how many parameters are often in Python libraries).

>> >>
>> >> 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.

>
> I write loads of stuff that may or may not suck from a consumer

perspective. I am blaming the API, not the developer, let's not bring it to
a personal level.

Apology if it seemed that I'm bringing it on personal level. That wasn't my
intention. What I was trying to say is that I think we should try to keep
the extension consistent. I think that your idea is good in general but it
doesn't fit to the openssl ext in my opinion.

>
> We are designing (changing, eek!) an API here: the fact that it may or

not reflect how OpenSSL itself exposes it is totally irrelevant.

The OpenSSL libcrypto API was never the point here. I might have been a bit
unclear. If so, then I apologise. I actually meant other PHP functions from
openssl core extension (those starting with openssl_). There are no other
objects in the extension, just resources and functions with many
parameters. Some of them also using refs for out params. I also think that
it's more conforming with the openssl_encrypt and openssl_decrypt that got
iv parameter at some later point. We don't have a new pair of functions
with and without iv though...

Cheers

Jakub

Jakub Zelenka

10 years ago

Hi,

On Wed, Jan 20, 2016 at 4:40 PM, Jakub Zelenka <bukka@php.net> wrote:

> Hi,
>
> The vote is now open:
>
> https://wiki.php.net/rfc/openssl_aead#voting
>
>
>

The voting has ended and the RFC has been accepted with 7 votes for and 4
against.

I will merge the patch after sorting out another issue with openssl ext
(errors storing) and possibly doing a bit more testing.

Cheers

Jakub

Albert Casademont Filella

10 years ago

That's great nees thanks!!

On Wednesday, 27 January 2016, Jakub Zelenka <bukka@php.net> wrote: