Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:61011
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.216.182 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <1340815507.2802.9.camel@guybrush>
References: <CAAyV7nH6FCZUqrirJ0uOApuwR64Cky3cVnRfbrcjkZrpOKiN6w@mail.gmail.com>
	<1340815507.2802.9.camel@guybrush>
Date: Wed, 27 Jun 2012 22:00:33 -0400
Message-ID: <CAAyV7nG8fRMbqe2syW_7xbDKHbOPvqG5F3hO3HMyR=JY0RELyw@mail.gmail.com>
To: =?ISO-8859-1?Q?Johannes_Schl=FCter?= <johannes@schlueters.de>
Cc: internals@lists.php.net
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PHP-DEV] [DRAFT RFC] Adding Simplified Password Hashing API
From: ircmaxell@gmail.com (Anthony Ferrara)

Johannes,

> Some comments on the "error behavior" part:
>
> =A0 =A0E_WARNING - When CRYPT is not included in core (was disabled
> =A0 =A0compile-time, or is listed in disabled_functions declaration)
>
> Disabling a different function should have no effect. This is not
> intuitive. If crypt is a dependency and is not available this function
> shouldn't be available either.

Hrm. Well, then I guess I could re-implement against crypt internally.
That would take either a slight re-implementation of the crypt()
internals, or slight refactoring of the PHP_FUNCTION(crypt) function
to enable c calls to it (even if it's disabled).

I don't like the concept of core functions disappearing if they are
not implemented. I think that does nothing but make it harder on the
developers (now having to inject a function_exists(), etc).
Additionally, since this is a security issue, I think that always
defining the function is the better approach. Otherwise, you can wind
up in a situation where someone else comes in and implements function
password_verify($password, $hash) { return true; }, which would be all
sorts of bad...  I can see the static linking to the function (instead
of the dynamic call that's there), So in this case, I personally think
the warning is appropriate.

> =A0 =A0E_WARNING - When supplied an incorrect number of arguments.
> =A0 =A0E_WARNING - When supplied a non-string first parameter (password)
>
> This should follow common semantics of zend_parse_parameters(... "s").
> i.e. it has to support objects with __toString(). Also other scalars are
> fine. (if they can be casted to string)
>
> =A0 =A0E_WARNING - If a non-string salt option is provided
>
> As above.

Yeah, I guess that's fair. Is there a macro or function like
Z_REPRESENTABLE_AS_STRING_P() or something? To make it easier to
check?

> =A0 =A0If any error is raise, false is returned by the function.
>
> In http://de.php.net/functions.internal it is documented that internal
> functions return NULL on error during parameter parsing. New exceptions
> for that should have a good reason.

The good reason is consistency. Otherwise there would be three return
values, `null`, `false` and `true` for password_verify(). Therefore, a
check of `false =3D=3D=3D password_verify($foo)` would actually fail
inadvertantly.

My opinion is that it should do what's appropriate.  The other 2 I can
live with returning null (although I disagree with it significantly),
but password_verify I think really should only ever return a
boolean...

> These things are all minor and you might consider them bad, but then
> change it globally, not by adding new inconsistencies.

Fair enough...

Thanks,

Anthony