Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:94358
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.215.41 as permitted sender)
MIME-Version: 1.0
References: <CAGa2bXaSkozJLLtycujwmHyas9TofRw5s6JzG0EQQvPyU0pxNQ@mail.gmail.com>
 <CAPwsocEffWVhzjNHVA853J+1VfmrhstZ0inihhQVHW=VfLS6Eg@mail.gmail.com>
In-Reply-To: <CAPwsocEffWVhzjNHVA853J+1VfmrhstZ0inihhQVHW=VfLS6Eg@mail.gmail.com>
Date: Sat, 02 Jul 2016 08:39:12 +0000
Message-ID: <CAPwsocEAvcRoH7FmKHh+eUMd=044Jm8DfwyM_Mb22W4UPvXE=w@mail.gmail.com>
To: Yasuo Ohgaki <yohgaki@ohgaki.net>, "internals@lists.php.net" <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=001a114003c632c14d0536a30a07
Subject: Re: [PHP-DEV] [RFC][VOTE] Session ID without hashing
From: leight@gmail.com (Leigh)

--001a114003c632c14d0536a30a07
Content-Type: text/plain; charset=UTF-8

Actually decided to post so

On Sat, 2 Jul 2016 at 09:16 Leigh <leight@gmail.com> wrote:

> On Sat, 2 Jul 2016 at 08:36 Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
>
>> Hi all,
>>
>> Currently session module uses obsolete MD5 for session ID. With
>> CSPRNG, hashing is redundant and needless. It adds hash module
>> dependency and inefficient (There is no reason to use hash for CSPRNG
>> generated bytes).
>>
>> This proposal cleans up session code by removing hash.
>>
>> https://wiki.php.net/rfc/session-id-without-hashing
>>
>> I set vote requires 2/3 support.
>> Please describe the reason why when you against this RFC. Reasons are
>> important for improvements!
>>
>
>
 So I have a few issues that span the RFC and the implementation.

Your RFC states

> hardcoded default and php.ini-* default values are the same.

This is not the case.

Originally the session id length and character set were controlled by
session.hash_function and/or session.hash_bits_per_character. These
customisations to configuration will be lost when the user upgrades. You
have provided a mechanism to control length and charset, but it will
require new changes to the default settings. This needs to be noted as a
breaking change.

Your default for session.sid_length is 48. Up to 7.1 the session id length
is 32. Your default for session.sid_bits_per_character is 5, up to 7.1 the
session id uses 4 bits per character. This is a breaking change. (Imagine
custom session handlers that validate session id character sets, or
database schemas that will truncate after 32 characters)

Your patch updates session.use_strict_mode from 0 to 1. I actually don't
know what this changes, but it's an undocumented change.

Overall your patch looks very similar to the one I was working on earlier
in the year, although you appear to have deleted a bunch of tests that you
could have just updated. You should probably put those back, and update
them.

--001a114003c632c14d0536a30a07--