Everything

php

Home » Project » Newsgroups » php.internals » [RFC][VOTE] Precise session management »

[RFC][VOTE] Precise session management

php.internals

Yasuo Ohgaki

10 years ago

Hi all,

This mail is for RFC vote notice.

The RFC is
https://wiki.php.net/rfc/precise_session_management
The PR is
https://github.com/php/php-src/pull/1734

I suppose almost all questions and discussion is finished, but
if you have any, please comment/ask.

Vote starts 2016-03-09-09:00(UTC) and ends 2016-03-23-09:00(UTC)
https://wiki.php.net/rfc/precise_session_management#vote

Please note that I set 2/3 of supporters is required to pass even if it
is not strictly required. Please let us know the reason why if one
would like to vote against this RFC. Thank you.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Yasuo Ohgaki

10 years ago

Hi all,

On Wed, Mar 9, 2016 at 6:14 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

> The RFC is
> https://wiki.php.net/rfc/precise_session_management

---------
Change session_destroy()

Accept bool parameter for session data removal.

bool session_gc(void)
---------

The function should be session_destroy(), not session_gc().
I fixed this, but it should not matter for voting.
Sorry.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Derick Rethans

10 years ago

On Wed, 9 Mar 2016, Yasuo Ohgaki wrote:

> Hi all,
>
> This mail is for RFC vote notice.
>
> The RFC is
> https://wiki.php.net/rfc/precise_session_management
> The PR is
> https://github.com/php/php-src/pull/1734
>
> I suppose almost all questions and discussion is finished, but
> if you have any, please comment/ask.
>
> Vote starts 2016-03-09-09:00(UTC) and ends 2016-03-23-09:00(UTC)
> https://wiki.php.net/rfc/precise_session_management#vote
>
> Please note that I set 2/3 of supporters is required to pass even if it
> is not strictly required. Please let us know the reason why if one
> would like to vote against this RFC. Thank you.

Voted for, but I have added some comments to the PR.

cheers,
Derick

Yasuo Ohgaki

10 years ago

Hi Derick,

On Wed, Mar 9, 2016 at 8:48 PM, Derick Rethans <derick@php.net> wrote:

>> Hi all,
>>
>> This mail is for RFC vote notice.
>>
>> The RFC is
>> https://wiki.php.net/rfc/precise_session_management
>> The PR is
>> https://github.com/php/php-src/pull/1734
>>
>> I suppose almost all questions and discussion is finished, but
>> if you have any, please comment/ask.
>>
>> Vote starts 2016-03-09-09:00(UTC) and ends 2016-03-23-09:00(UTC)
>> https://wiki.php.net/rfc/precise_session_management#vote
>>
>> Please note that I set 2/3 of supporters is required to pass even if it
>> is not strictly required. Please let us know the reason why if one
>> would like to vote against this RFC. Thank you.
>
> Voted for, but I have added some comments to the PR.

Thank you for comments.

Session module returns FALSE for invalid status, therefore it would be
better to return FALSE. IMO. However, this patch is for 7.1, so it can
be changed. If we are going to change this, it would be nicer to
change them all.

I left some old changes in php.ini-*, these should be fixed.
Thank you.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Yasuo Ohgaki

10 years ago

Hi Derick,

On Thu, Mar 10, 2016 at 6:08 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

> Session module returns FALSE for invalid status, therefore it would be
> better to return FALSE. IMO. However, this patch is for 7.1, so it can
> be changed. If we are going to change this, it would be nicer to
> change them all.

I think I misunderstood your comment.
It return array and it may return NULL and raise error. I'll change
the return type and leave others alone.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Levi Morrison

10 years ago

On Wed, Mar 9, 2016 at 2:14 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

> The RFC is
> https://wiki.php.net/rfc/precise_session_management
> The PR is
> https://github.com/php/php-src/pull/1734
>
> I suppose almost all questions and discussion is finished, but
> if you have any, please comment/ask.

After talking to a few people I still don't understand this RFC. That
makes me nervous. I think partly this is due to the many issues trying
to be solved inside this one RFC. To be frank, a large part of it also
comes from the fact that the phrasing is difficult to understand.

In summary, I can't feel good about voting yes on this RFC because I
don't understand it. I've honestly tried, but I don't really get it,
nor have several other people I have talked to. Additionally there
appear to be BC breaks though I can't really judge their impact. Given
that this is a minor release I don't think it's a good idea to break
BC.

I will wait for someone to respond to this email before voting, but
unless I am convinced by further discussion I will be voting no.

Yasuo Ohgaki

10 years ago

Hi Levi,

On Sat, Mar 12, 2016 at 1:50 AM, Levi Morrison <levim@php.net> wrote:

> On Wed, Mar 9, 2016 at 2:14 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
>> The RFC is
>> https://wiki.php.net/rfc/precise_session_management
>> The PR is
>> https://github.com/php/php-src/pull/1734
>>
>> I suppose almost all questions and discussion is finished, but
>> if you have any, please comment/ask.
>
> After talking to a few people I still don't understand this RFC. That
> makes me nervous. I think partly this is due to the many issues trying
> to be solved inside this one RFC. To be frank, a large part of it also
> comes from the fact that the phrasing is difficult to understand.
>
> In summary, I can't feel good about voting yes on this RFC because I
> don't understand it. I've honestly tried, but I don't really get it,
> nor have several other people I have talked to. Additionally there
> appear to be BC breaks though I can't really judge their impact. Given
> that this is a minor release I don't think it's a good idea to break
> BC.
>
> I will wait for someone to respond to this email before voting, but
> unless I am convinced by further discussion I will be voting no.

Thank you for feedback.

The main points of this RFC are
- Avoid race conditions result in session loss.
- Make session abuse a lot harder. i.e. More secure than now.

To achieve those 2 objectives, timestamp management is introduced because
- There is no reliable way to synchronize web server and client
state. i.e. Cookie as well as session ID by transid.
- There is no reliable way to remove obsolete session by default.
i.e. Probability based GC is not good enough and allows attackers to
steal PHP session forever.

Because there is no way to synchronize server/client and probability
based GC is hardly acceptable, eventually consistent approach is used.

Eventually consistent approach is the same as it is now, but new
session manager does it much precise/stricter way by using timestamps.

Will this explanation help to understand it?

IMHO, this is mandatory change for session module to be more reliable
and secure than now. I hope I explained well reason behind the change.
Normal users don't know risks that this RFC is trying to
resolve/mitigate. Therefore, we are better to release it asap.

BC issue is not severe. Users' test scripts may find behavior changes,
but it does not affect session functionality in apps at all. (It
should not at least, if it does, it's bug)

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Yasuo Ohgaki

10 years ago

Hi Levi,

On Sat, Mar 12, 2016 at 9:24 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

>
> BC issue is not severe. Users' test scripts may find behavior changes,
> but it does not affect session functionality in apps at all. (It
> should not at least, if it does, it's bug)

Additional note on this. There are changes may break apps such as
automatic session ID regeneration, but these could be disabled or
changed by INI.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Pascal MARTIN

10 years ago

Le 09/03/2016 10:14, Yasuo Ohgaki a écrit :

> Vote starts 2016-03-09-09:00(UTC) and ends 2016-03-23-09:00(UTC)
> https://wiki.php.net/rfc/precise_session_management#vote

Hi,

At AFUP, we would be +1 on this RFC.

Basically: better security and pretty-much no bc-break, is a good thing.

Thanks for your work on this!

--
Pascal MARTIN, AFUP - French UG
http://php-internals.afup.org/

Andrey Andreev

10 years ago

Hi,

On Tue, Mar 22, 2016 at 9:52 PM, Pascal MARTIN, AFUP <
mailing@pascal-martin.fr> wrote:

> Le 09/03/2016 10:14, Yasuo Ohgaki a écrit :
>
>> Vote starts 2016-03-09-09:00(UTC) and ends 2016-03-23-09:00(UTC)
>> https://wiki.php.net/rfc/precise_session_management#vote
>>
>
> Hi,
>
> At AFUP, we would be +1 on this RFC.
>
> Basically: better security and pretty-much no bc-break, is a good thing.
>
> Thanks for your work on this!
>

I respect your opinion in wanting to support the proposal, but "pretty much
no BC break"?

The RFC itself lists 7 points under "Backwards Incompatible Changes" and
there's at least one more in session_destroy() not deleting data
immediately.
It's also not hard to imagine a lot of BC breaks and other unadressed
problems in custom handlers passed to session_set_save_handler().

Cheers,
Andrey.

Yasuo Ohgaki

10 years ago

Hi Andrey,

On Wed, Mar 23, 2016 at 5:03 AM, Andrey Andreev <narf@devilix.net> wrote:

>
> On Tue, Mar 22, 2016 at 9:52 PM, Pascal MARTIN, AFUP <
> mailing@pascal-martin.fr> wrote:
>
>> Le 09/03/2016 10:14, Yasuo Ohgaki a écrit :
>>
>>> Vote starts 2016-03-09-09:00(UTC) and ends 2016-03-23-09:00(UTC)
>>> https://wiki.php.net/rfc/precise_session_management#vote
>>>
>>
>> Hi,
>>
>> At AFUP, we would be +1 on this RFC.
>>
>> Basically: better security and pretty-much no bc-break, is a good thing.
>>
>> Thanks for your work on this!
>>
>
> I respect your opinion in wanting to support the proposal, but "pretty much
> no BC break"?

Offending features may be disabled/changed by INI settings. I think it ok
to say pretty much no BC issue.

>
> The RFC itself lists 7 points under "Backwards Incompatible Changes" and
> there's at least one more in session_destroy() not deleting data
> immediately.
> It's also not hard to imagine a lot of BC breaks and other unadressed
> problems in custom handlers passed to session_set_save_handler().

The difference that user/3rd party save handlers will see is an additional
array in session data. The additional array should not cause any problems
with session save handlers.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Stas Malyshev

10 years ago

Hi!

> Offending features may be disabled/changed by INI settings. I think it ok
> to say pretty much no BC issue.

I think this approach will lead us into problems. "It may be disabled by
INI setting" does not mean the problem does not exist (and btw not
everything that changes can be disabled AFAIR). Most BC breaks have
workarounds, but they still are BC breaks. Moreover, some of the changes
are default changes, which means basically users will have to undo what
this RFC does - if we think they'd have to do it, why change it in the
first place?

I also feel that sheer size of this RFC, the fact that it tried to fix
many unrelated issues, and amount of changes it brings leads to the
situation where people don't really discuss it properly but instead rely
on claims in the RFC that everything is fine, because it is very hard to
properly consider changes of this size with this many moving parts. At
least the discussion on the list was rather scarce as far as I can see.

> The difference that user/3rd party save handlers will see is an additional
> array in session data. The additional array should not cause any problems
> with session save handlers.

The change is not limited to additional data. This additional data is
active - it controls how sessions are supposed to behave. All these need
to be updated when something happens, all these need to be checked,
validated and user for various logic on various stages of session
lifetime. So saying "it's just some additional data, nothing of it" is
not true - it's not just data. Maybe everything is still OK with the
session handlers - but this needs to be thoroughly verified, it does not
come for free.

--
Stas Malyshev
smalyshev@gmail.com

Yasuo Ohgaki

10 years ago

Hi Stas,

On Thu, Mar 24, 2016 at 5:44 AM, Stanislav Malyshev <smalyshev@gmail.com> wrote:

>> Offending features may be disabled/changed by INI settings. I think it ok
>> to say pretty much no BC issue.
>
> I think this approach will lead us into problems. "It may be disabled by
> INI setting" does not mean the problem does not exist (and btw not
> everything that changes can be disabled AFAIR). Most BC breaks have
> workarounds, but they still are BC breaks. Moreover, some of the changes
> are default changes, which means basically users will have to undo what
> this RFC does - if we think they'd have to do it, why change it in the
> first place?

Right. Not everything cannot be disabled, but it should be matter for
applications. Session data is managed more strict manner by
timestamping and apps do not have to care about it. It just works more
precisely than now.

Only concern is test program that inspects session behaviors. New
session will not delete session data immediately and contains
additional array. Therefore, save handler function execution order
changes and test program may detect existence of to be deleted
sessions and/or additional array.

>
> I also feel that sheer size of this RFC, the fact that it tried to fix
> many unrelated issues, and amount of changes it brings leads to the
> situation where people don't really discuss it properly but instead rely
> on claims in the RFC that everything is fine, because it is very hard to
> properly consider changes of this size with this many moving parts. At
> least the discussion on the list was rather scarce as far as I can see.

I'll try to divide into pieces next time.

>
>> The difference that user/3rd party save handlers will see is an additional
>> array in session data. The additional array should not cause any problems
>> with session save handlers.
>
> The change is not limited to additional data. This additional data is
> active - it controls how sessions are supposed to behave. All these need
> to be updated when something happens, all these need to be checked,
> validated and user for various logic on various stages of session
> lifetime. So saying "it's just some additional data, nothing of it" is
> not true - it's not just data. Maybe everything is still OK with the
> session handlers - but this needs to be thoroughly verified, it does not
> come for free.

As you mentioned, this RFC contains BC. Most important for apps is
automatic session ID regeneration. Automatic session ID regeneration
is enabled by default, but it can be disabled if apps cannot support
this. Another issue is obsolete session data. Currently, it is left
for GC or removed immediately. Leaving obsolete session data to GC is
not good, but removing it immediately is not good also. We need to
change the behavior.

The basic logic is used and tested by very large site, so it can be
said it's tested and worked.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net