On 01/17/2017 11:51 PM, Peter Eisentraut wrote:
On 1/3/17 9:09 AM, Heikki Linnakangas wrote:
Since not everyone agrees with this approach, I split this patch into
two. The first patch refactors things, replacing the isMD5() function
with get_password_type(), without changing the representation of
pg_authid.rolpassword. That is hopefully uncontroversial.
I have checked these patches.
The refactoring in the first patch seems sensible. As Michael pointed
out, there is still a reference to "plain:" in the first patch.
Fixed.
The commit message needs to be updated, because the function
plain_crypt_verify() was already added in a previous patch.
Fixed.
I'm not fond of this kind of coding
password = encrypt_password(password_type, stmt->role, password);
where the 'password' variable has a different meaning before and after.
Added a new local variable to avoid the confusion.
This error message might be a mistake:
elog(ERROR, "unrecognized password type conversion");
I rephrased the error as "cannot encrypt password to requested type",
and added a comment explaining that it cannot happen. I hope that
helped, I'm not sure why you thought it might've been a mistake.
I think some pieces from the second patch could be included in the first
patch, e.g., the parts for passwordcheck.c and user.c.
I refrained from doing that for now. It would've changed the
passwordcheck hook API in an incompatible way. Breaking the API
explicitly would be a good thing, if we added the "plain:" prefix,
because modules would need to deal with the prefix anyway. But until we
do that, better to not break the API for no good reason.
And the second
patch adds the "plain:" prefix, which not everyone agrees on.
The code also gets a little bit dubious, as it introduces an "unknown"
password type, which is sometimes treated as plaintext and sometimes as
an error. I think this is going be messy.
I would skip this patch for now at least. Too much controversy, and we
don't know how the rest of the patches for this feature will look like
to be able to know if it's worth it.
Ok, I'll drop the second patch for now. I committed the first patch
after fixing the things you and Michael pointed out. Thanks for the review!
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
