Alex,
First off, thanks for such a thorough reply!!! Comments are inline.
1. The resulting string should have a version information. For example
> the first char. the example hash will look like
> "1$2y$07$usesomesillystringfore2uDLvp1Ii2e./U9C8sBjqp8I90dH6hi",
> instead of "$2y$07$usesomesillystringfore2uDLvp1Ii2e./U9C8sBjqp8I90dH6hi"
>
> Maybe it is legal to interpret a missing version as "this is version
> 1"? But it is important, that it has now a version for
> future-compatibility. This is, becaus encryption is developed since I
> program and will develop further within the next thousand years. With
> a version-information we can guarantee that the algorithms can work,
> even if the hash-format changes markable (because needed, because new
> unforseeable aspects of encryption come in focus). In my project, this
> version-information helped me to distinct between old password-hashes
> and new ones.
>
> [I think this is also a good argument against "password_hash() should
> have no defaults", because this is a more than obvious sign to
> everyone "hey, there could be a version 2, how will that look like?"]
>
That's what $2y$ is for. It's a standard crypt() prefix algorithm
identifier. For example you can use $1$ for md5, $5$ for sha256 and $6$ for
sha512 algorithms. In the future, if new algorithms are added, it would be
added as a new prefix for crypt()...
> 2a) I'm not happy with the behavour to raise warnings. Maybe I'm
> splitting hairs, but if there is one error, the chance is by
> experience very big, that there is another. So, if a developer for
> example will not check the results of password_hash(), false is stored
> in the database. And if he makes a second error (and the chance is as
> said very big in this case), you may be able to log in with this or do
> other ugly stuff. Don't say "impossible". I've seen things...
>
While I understand your point, I'm not sure that's as big of a deal. All it
takes is a simple if statement. It's not like a warning will be raised and
the hash will still be generated. If a warning is raised, it'll return
false. That's easy to check for, and IMHO more of a documentation problem
than a problem we should solve here...
> In my project I had really very good results, when I just throw
> exceptions in the case that something goes wrong. This is the highly
> visible sign for a bad developer, that he has made something wrong or
> for a customer, installing a new version, that he now has a big
> problem and should go back to the older version. I really want
> exceptions here, because they prevent such missuse very effective and
> cannot be overgone.
>
I want exceptions here too. But PHP doesn't use exceptions in its standard
library (aside from SPL), and I'm not sure this is a compelling enough case
to implement the first... I could be swayed, but I'm not sure the rest of
the team would.
> By minimim it should stop execution, if the password_hash() or
> password_verify() is called wrong, because I think, this is more
> secure than just to continue, which is in my eyes definitily a
> mistake.
>
I think that halting execution if either function is called wrong is a bit
drastic. Again, I understand why, but I think it's a bit overkill in this
case...
2b) I don't like that the functions sometimes will return FALSE,
> sometimes NULL. Not logical for PHP-developer and will lead by
> guarantee to code which is checked wrong. Again: I would prefer to
> fetch exceptions. If this is not possible I want either NULL or FALSE
> as result for all functions, but not NULL or FALSE.
>
I don't either. But that's how the rest of the core works...
> 3a) I would await a function "password_algos()" (like hash_algos()),
> which returns me all available algorithms.
>
> The available algorithms should also be displayed in PHPINFO(). This
> is because I need to check, if the current version has built it in.
>
That's a very good addition. It could be defined as such:
array(
NAME => constant_value
)
So,
array(
'bcrypt' => PASSWORD_BCRYPT
)
I'll add that when I get a few minutes.
3b) I don't like the encryption of the algorithm ("2y", WTF, what's
> that? :) ). Look at the function hash_algos(). There are full
> qualified names of algorithms. Why don't use just the names? Is it,
> because of some more bytes needed? :) Ok, I mean: why not using a good
> name, so that someone can guess what it is, without knowing the docs?
> That's just my thinking as PHP-developer, because this is not logical
> for me to encrypt the encryption-algorithm. :)
>
It's the standard crypt() format, which is available in almost every single
programming language out there today. Which makes interoperability really
easy.
I would be very hesitant to invent our own format here, especially for the
interoperability standpoint. There are standards that exist, and unless
there's a *strong* reason not to use them, we should...
> 3c) I think about changes. Password algorithms are a thing of changing
> over time. So I would like to have a function like
> "password_algo_info($algo)", which returns me as a developer a
> detailed information about what this algorithm needs to work propper
> and what the full description of this is (including e.g. an url, where
> I can look for more information), because I think that this is
> eventually a thing, which is extended rapidly and the documentation is
> to slow to go with it. Or you are working with an older version in
> which the algo works different. Not very important and maybe needs
> more discussion. But I think something is here better than nothing.
> Again: very low prio!
>
I'm not sure why this needs to be a function. Wouldn't the documentation on
the internet be enough for this?
> 4a) I don't like the hole CONSTANT-stuff. I don't want to have a
> doozen of fixed CONSTANTS, because I don't need them. The only
> constant is, that the list of constants will rise. :)
> Ok, more serious: I really don't need a constant for a thing, which is
> currently known to be changed in future. If the password is always
> stored with the currently most secure algorithm I don't need that.
>
And as a developer this is, what I want! I don't want to care about
> the details, I want to be sure, that this is the currently most secure
> way to do it. That's what I want. I don't want to know, which
> algorithms exists. I don't need to choose between them. I just use
> them, because some smarter guys than me have thought thouroughly about
> what is most secure. :)
>
And the common use-case should be that. But there are other use-cases that
are compelling enough to support. Use-cases such as requiring passwords to
be backwards compatible to older versions of PHP. Or requiring
compatibility with another environment (python, etc) which may not get the
new algorithm when PHP does. That's why the choice is there, and why it's
forced to be made by the developer...
> I can not think about an situation where I need to set it in my code
> under normal circumstances. And if I really need to choose an
> algorithm, it is better a configurable option in my program, thats
> just good style. (For example a configuration-file, or a call-option.)
> A constant means: It is fixed in my code. That's good, if it doesn't
> change.
>
A documentation issue...
> Another example: if I need to choose the algorithm as an
> administrator, I choose it in my admin-interface from a select-field.
> The value is then stored as as string and not as a constant-name. I
> need to create that list of available algorithms. I don't need
> constants.
>
A problem that password_get_algos would solve...
> ...snip...
>
> 4b) I don't like the idea to change a constant (PASSWORD_DEFAULT) with
> a new version of PHP. This may break in some ugly situation your hole
> PHP-update, because you didn't thought about this change. :)
> Again: I think, that chhoosing the best algorithm should normally not
> be the job of the developer. I don't see the need.
> But to make it possible I suggest to have a function
> "password_best_algo()" instead, which returns a name of
> password_algos(). Why a function instead of a constant? You can add a
> parameter "$version" to this function and in this case the function
> returns the best algorithm for that version of a password-hash. I
> think this is eventually important, but maybe I'm looking too far into
> the future or forgot to think this completly through.
>
The password_best_algo() thought is intriguing, but I guess I don't see the
changing constant to be a big deal. It's an informed choice the developer
has to make, which is better than complicating the API by making it take 3
or 4 function calls to hash a password (IMHO)...
> 5. I added something like "is_password_hash()", to check, if a hash
> has the right syntax (not semantic) to work with it. This prevents
> internally also, that hashes with version 2 can work together with the
> class for version 1.
>
There's no need. password_get_info() could be used for it, and I'm not sure
that there's even a valid use-case for such a function. Can you elaborate
over why you would use such a function?
> 6. This is hair-splitting again, but least important: I don't like the
> name "password*" for the functions. This is because the functions
> don't return a password. Impractical names will give (some less
> experienced developers) wrong ideas of what the functions may do.
>
> It should be clear: This is a hash-function, you fill in a password
> and get a hash.
>
> But "hash*" is already used. I have no good solution for that and it
> is not quite important, but how would it be with "pwhash*", "phash*"
> or "hashpass"? Something good to remember...
> Maybe needs further discussion, but as said: Not so important, maybe
> someone has a good idea...
>
And password_hash($password) doesn't communicate that the function hashes a
password? If this is really an issue, please suggest a better prefix, as I
can't think of one that's better without being cryptic...
> Some comments to the discussion points:
>
> password_make_salt() Is Not Needed: I like that function, because it
> can be used to generate a random password(). Just google for "generate
> password" to see what I mean.
> It should have a third mode, where the "/"-char is not included for
> that case, because it may make problems. Maybe it is also a good idea
> that you can pass a string, which includes the allowed chars for this
> function. And because it can be used for completly other things than
> just hashes or passwords I think renaming or an alias would be useful
> e.g. "str_random()". (UTF8?)
>
I've thought about passing in a string of allowed characters. The problem
with that is two fold:
1. It greatly increases the complexity of the algorithm for generating the
salt. Take a look at this:
https://github.com/ircmaxell/PHP-CryptLib/blob/master/lib/CryptLib/Core/BaseConverter.php
2. There's no usecase right now for salts that are of a different format
(but if one is found, it can be added). PBKDF2 would use a raw salt, and
all the other crypt algorithms would use the other format. Where's the
other need for salting?
Again: This are only suggestions from the sight of an experienced
> PHP-developer. It's totally clear, that you have another sight. But I
> hope they are helpful.
>
> Hope you are not too firm with me, I'm new to this list. :)
>
I hope I was not too firm either. I like some of the suggestions, and I
think some of them have real merit. But others concern me... Looking
forward to everyone else's thoughts...
Anthony
