Re: Removal of plaintext password type references

On Wed, May 10, 2017 at 10:51 AM, Vaishnavi Prabakaran
<vaishnaviprabakaran(at)gmail(dot)com> wrote:
> Following recent removal of support to store password in plain text,
> modified the code to
>
> 1. Remove "PASSWORD_TYPE_PLAINTEXT" macro
> 2. Instead of using "get_password_type" to retrieve the encryption method
> AND to check if the password is already encrypted or not, modified the code
> to
> a. Use "get_password_encryption_type" function to retrieve encryption
> method.
> b. Use "isPasswordEncrypted" function to check if the password is already
> encrypted or not.
>
> These changes are mainly to increase code readability and does not change
> underlying functionality.

Actually, this patch reduces readability.

> Attached the patch for community's review.

+/*
+ * Verify whether the given password is already encrypted or not
+ */
+bool
+isPasswordEncrypted(const char *password)
+{
+ if(isMD5(password) || isSCRAM(password))
+ return true;
+ return false;
}
New hash functions may be added in the future, and we have now a
facility that can be easily extended for this purpose. We have been
already through a couple of commits to make that modular and I think
that it would be a bad idea to drop that. Please note that in this
facility we still need to be able to track passwords that are in a
plain format, because, even if Postgres does not store them in
cleartext, nothing prevents users to send to the server cleartext
strings.

In your patch, get_password_encryption_type() and
isPasswordEncrypted() are actually doing the same work. There is no
need for duplication as well.

In short, I am clearly -1 for this patch.
--
Michael