Removal of plaintext password type references
Hi All,
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.
Attached the patch for community's review.
Thanks & Regards,
Vaishnavi,
Fujitsu Australia.
Attachments:
0001-Removal-of-plaintext-password-reference.patchapplication/octet-stream; name=0001-Removal-of-plaintext-password-reference.patchDownload
From 7c326e7b0ae7a4e614d4f21703e0987d235d42aa Mon Sep 17 00:00:00 2001
From: Vaishnavi Prabakaran <Vaishnavi.Prabakaran@au.fujitsu.com>
Date: Wed, 10 May 2017 11:39:50 +1000
Subject: [PATCH] Removal of plaintext password reference
---
src/backend/commands/user.c | 6 +++---
src/backend/libpq/auth-scram.c | 2 +-
src/backend/libpq/auth.c | 2 +-
src/backend/libpq/crypt.c | 35 +++++++++++++++++++----------------
src/include/commands/user.h | 2 +-
src/include/libpq/crypt.h | 9 +++++++--
6 files changed, 32 insertions(+), 24 deletions(-)
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 36d5f40..841fc98 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -362,7 +362,7 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
if (check_password_hook && password)
(*check_password_hook) (stmt->role,
password,
- get_password_type(password),
+ isPasswordEncrypted(password),
validUntil_datum,
validUntil_null);
@@ -724,7 +724,7 @@ AlterRole(AlterRoleStmt *stmt)
if (check_password_hook && password)
(*check_password_hook) (rolename,
password,
- get_password_type(password),
+ isPasswordEncrypted(password),
validUntil_datum,
validUntil_null);
@@ -1204,7 +1204,7 @@ RenameRole(const char *oldname, const char *newname)
datum = heap_getattr(oldtuple, Anum_pg_authid_rolpassword, dsc, &isnull);
- if (!isnull && get_password_type(TextDatumGetCString(datum)) == PASSWORD_TYPE_MD5)
+ if (!isnull && get_password_encryption_type(TextDatumGetCString(datum)) == PASSWORD_TYPE_MD5)
{
/* MD5 uses the username as salt, so just clear it on a rename */
repl_repl[Anum_pg_authid_rolpassword - 1] = true;
diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index 99feb0c..8aebaeb 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -182,7 +182,7 @@ pg_be_scram_init(const char *username, const char *shadow_pass)
*/
if (shadow_pass)
{
- int password_type = get_password_type(shadow_pass);
+ int password_type = get_password_encryption_type(shadow_pass);
if (password_type == PASSWORD_TYPE_SCRAM_SHA_256)
{
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 6d3ff68..dadfdd4 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -765,7 +765,7 @@ CheckPWChallengeAuth(Port *port, char **logdetail)
if (!shadow_pass)
pwtype = Password_encryption;
else
- pwtype = get_password_type(shadow_pass);
+ pwtype = get_password_encryption_type(shadow_pass);
/*
* If 'md5' authentication is allowed, decide whether to perform 'md5' or
diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c
index e7a6b04..1d7c73f 100644
--- a/src/backend/libpq/crypt.c
+++ b/src/backend/libpq/crypt.c
@@ -96,13 +96,24 @@ get_role_password(const char *role, char **logdetail)
* What kind of a password verifier is 'shadow_pass'?
*/
PasswordType
-get_password_type(const char *shadow_pass)
+get_password_encryption_type(const char *shadow_pass)
{
- if (strncmp(shadow_pass, "md5", 3) == 0 && strlen(shadow_pass) == MD5_PASSWD_LEN)
+ /* Currently only 2 encryption mechanism are supported, so we can skip verifying password against SCRAM */
+ if(isMD5(shadow_pass))
return PASSWORD_TYPE_MD5;
- if (strncmp(shadow_pass, "SCRAM-SHA-256$", strlen("SCRAM-SHA-256$")) == 0)
+ else
return PASSWORD_TYPE_SCRAM_SHA_256;
- return PASSWORD_TYPE_PLAINTEXT;
+}
+
+/*
+ * Verify whether the given password is already encrypted or not
+ */
+bool
+isPasswordEncrypted(const char *password)
+{
+ if(isMD5(password) || isSCRAM(password))
+ return true;
+ return false;
}
/*
@@ -116,10 +127,10 @@ char *
encrypt_password(PasswordType target_type, const char *role,
const char *password)
{
- PasswordType guessed_type = get_password_type(password);
+ bool password_encrypted = isPasswordEncrypted(password);
char *encrypted_password;
- if (guessed_type != PASSWORD_TYPE_PLAINTEXT)
+ if (password_encrypted)
{
/*
* Cannot convert an already-encrypted password from one
@@ -141,8 +152,6 @@ encrypt_password(PasswordType target_type, const char *role,
case PASSWORD_TYPE_SCRAM_SHA_256:
return pg_be_scram_build_verifier(password);
- case PASSWORD_TYPE_PLAINTEXT:
- elog(ERROR, "cannot encrypt password with 'plaintext'");
}
/*
@@ -175,7 +184,7 @@ md5_crypt_verify(const char *role, const char *shadow_pass,
Assert(md5_salt_len > 0);
- if (get_password_type(shadow_pass) != PASSWORD_TYPE_MD5)
+ if (get_password_encryption_type(shadow_pass) != PASSWORD_TYPE_MD5)
{
/* incompatible password hash format. */
*logdetail = psprintf(_("User \"%s\" has a password that cannot be used with MD5 authentication."),
@@ -232,7 +241,7 @@ plain_crypt_verify(const char *role, const char *shadow_pass,
* the password the client sent, and compare the hashes. Otherwise
* compare the plaintext passwords directly.
*/
- switch (get_password_type(shadow_pass))
+ switch (get_password_encryption_type(shadow_pass))
{
case PASSWORD_TYPE_SCRAM_SHA_256:
if (scram_verify_plain_password(role,
@@ -273,12 +282,6 @@ plain_crypt_verify(const char *role, const char *shadow_pass,
}
break;
- case PASSWORD_TYPE_PLAINTEXT:
- /*
- * We never store passwords in plaintext, so this shouldn't
- * happen.
- */
- break;
}
/*
diff --git a/src/include/commands/user.h b/src/include/commands/user.h
index 08037e0..4984faa 100644
--- a/src/include/commands/user.h
+++ b/src/include/commands/user.h
@@ -20,7 +20,7 @@
extern int Password_encryption;
/* Hook to check passwords in CreateRole() and AlterRole() */
-typedef void (*check_password_hook_type) (const char *username, const char *shadow_pass, PasswordType password_type, Datum validuntil_time, bool validuntil_null);
+typedef void (*check_password_hook_type) (const char *username, const char *shadow_pass, bool password_encrypted, Datum validuntil_time, bool validuntil_null);
extern PGDLLIMPORT check_password_hook_type check_password_hook;
diff --git a/src/include/libpq/crypt.h b/src/include/libpq/crypt.h
index 9bad67c..5516dba 100644
--- a/src/include/libpq/crypt.h
+++ b/src/include/libpq/crypt.h
@@ -26,12 +26,17 @@
*/
typedef enum PasswordType
{
- PASSWORD_TYPE_PLAINTEXT = 0,
PASSWORD_TYPE_MD5,
PASSWORD_TYPE_SCRAM_SHA_256
} PasswordType;
-extern PasswordType get_password_type(const char *shadow_pass);
+#define isMD5(password) \
+ ((strncmp(password, "md5", 3) == 0 && strlen(password) == MD5_PASSWD_LEN)? 1:0)
+
+#define isSCRAM(password) \
+ ( (strncmp(password, "SCRAM-SHA-256$", 14) == 0)? 1 :0)
+
+extern PasswordType get_password_encryption_type(const char *shadow_pass);
extern char *encrypt_password(PasswordType target_type, const char *role,
const char *password);
--
2.7.4.windows.1
On Wed, May 10, 2017 at 10:51 AM, Vaishnavi Prabakaran
<vaishnaviprabakaran@gmail.com> wrote:
Following recent removal of support to store password in plain text,
modified the code to1. 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 05/10/2017 08:01 AM, Michael Paquier wrote:
On Wed, May 10, 2017 at 10:51 AM, Vaishnavi Prabakaran
<vaishnaviprabakaran@gmail.com> wrote:Following recent removal of support to store password in plain text,
modified the code to1. 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.
Yeah, I don't think this is an improvement. Vaishnavi, did you find the
current code difficult? Perhaps some extra comments somewhere would help?
Also note that changing the signature check_password_hook_type would
break any external modules that use the hook. Removing
PASSWORD_TYPE_PLAINTEXT will do that too, because any password hook
function would use that constant (see e.g. contrib/passwordcheck). If we
were to change the signature, I'd actually like to simplify it by
removing the password_type parameter altogether. The hook function can
call get_password_type() on the password itself to get the same
information. But it's not worth changing the API and breaking external
modules for that.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Heikki Linnakangas <hlinnaka@iki.fi> writes:
Also note that changing the signature check_password_hook_type would
break any external modules that use the hook. Removing
PASSWORD_TYPE_PLAINTEXT will do that too, because any password hook
function would use that constant (see e.g. contrib/passwordcheck). If we
were to change the signature, I'd actually like to simplify it by
removing the password_type parameter altogether. The hook function can
call get_password_type() on the password itself to get the same
information. But it's not worth changing the API and breaking external
modules for that.
FWIW, I think we've never hesitated to change hook signatures across major
versions if there was a good reason for it. It seems actually rather
unlikely that an external module interested in check_password_hook would
not need to know about the SCRAM changes, so this case seems like it's
easily justifiable.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, May 10, 2017 at 10:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Heikki Linnakangas <hlinnaka@iki.fi> writes:
Also note that changing the signature check_password_hook_type would
break any external modules that use the hook. Removing
PASSWORD_TYPE_PLAINTEXT will do that too, because any password hook
function would use that constant (see e.g. contrib/passwordcheck). If we
were to change the signature, I'd actually like to simplify it by
removing the password_type parameter altogether. The hook function can
call get_password_type() on the password itself to get the same
information. But it's not worth changing the API and breaking external
modules for that.
Ahah. I just had the same thought before reading this message.
FWIW, I think we've never hesitated to change hook signatures across major
versions if there was a good reason for it. It seems actually rather
unlikely that an external module interested in check_password_hook would
not need to know about the SCRAM changes, so this case seems like it's
easily justifiable.
My modules break every couple of months (utility hook is the last one
in date), and usually fixes are no big deals. Removing password_type
is in this category, and as long as compilation fails properly people
will be able to notice problems easily.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, May 10, 2017 at 5:58 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 05/10/2017 08:01 AM, Michael Paquier wrote:
On Wed, May 10, 2017 at 10:51 AM, Vaishnavi Prabakaran
<vaishnaviprabakaran@gmail.com> wrote:Following recent removal of support to store password in plain text,
modified the code to1. 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.
Yeah, I don't think this is an improvement. Vaishnavi, did you find the
current code difficult? Perhaps some extra comments somewhere would help?
Thanks for willing to add extra comments, And current code is not difficult
but kind of gives the impression that still plaintext password storage
exists by holding "PASSWORD_TYPE_PLAINTEXT" macro and having switch case
checks for this macro.
I see "get_password_type" function call is used for 2 purposes. One is to
check the actual password encryption type during authentication process and
another purpose is to verify if the password passed is encrypted or not -
used in create/alter role command before checking the strength of
password(via passwordcheck module). So, I thought my patch will make this
purpose clear. Nevertheless, if people thinks this actually reduces their
readability then I don't mind to go with the flow.
Thanks & Regards,
Vaishnavi
Fujitsu Australia.
On Wed, May 10, 2017 at 10:22 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Wed, May 10, 2017 at 10:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Heikki Linnakangas <hlinnaka@iki.fi> writes:
Also note that changing the signature check_password_hook_type would
break any external modules that use the hook. Removing
PASSWORD_TYPE_PLAINTEXT will do that too, because any password hook
function would use that constant (see e.g. contrib/passwordcheck). If we
were to change the signature, I'd actually like to simplify it by
removing the password_type parameter altogether. The hook function can
call get_password_type() on the password itself to get the same
information. But it's not worth changing the API and breaking external
modules for that.Ahah. I just had the same thought before reading this message.
And attached is a patch to do that. I am all for this one to get a
more simple interface in place.
--
Michael
Attachments:
simplify-passwd-hook.patchapplication/octet-stream; name=simplify-passwd-hook.patchDownload
diff --git a/contrib/passwordcheck/passwordcheck.c b/contrib/passwordcheck/passwordcheck.c
index 59f73a1e6b..3c9ee4ab20 100644
--- a/contrib/passwordcheck/passwordcheck.c
+++ b/contrib/passwordcheck/passwordcheck.c
@@ -51,10 +51,11 @@ extern void _PG_init(void);
static void
check_password(const char *username,
const char *shadow_pass,
- PasswordType password_type,
Datum validuntil_time,
bool validuntil_null)
{
+ PasswordType password_type = get_password_type(shadow_pass);
+
if (password_type != PASSWORD_TYPE_PLAINTEXT)
{
/*
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 36d5f40f06..cee23e8ee0 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -362,7 +362,6 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
if (check_password_hook && password)
(*check_password_hook) (stmt->role,
password,
- get_password_type(password),
validUntil_datum,
validUntil_null);
@@ -724,7 +723,6 @@ AlterRole(AlterRoleStmt *stmt)
if (check_password_hook && password)
(*check_password_hook) (rolename,
password,
- get_password_type(password),
validUntil_datum,
validUntil_null);
diff --git a/src/include/commands/user.h b/src/include/commands/user.h
index 08037e0f81..c1c4d431d4 100644
--- a/src/include/commands/user.h
+++ b/src/include/commands/user.h
@@ -20,7 +20,7 @@
extern int Password_encryption;
/* Hook to check passwords in CreateRole() and AlterRole() */
-typedef void (*check_password_hook_type) (const char *username, const char *shadow_pass, PasswordType password_type, Datum validuntil_time, bool validuntil_null);
+typedef void (*check_password_hook_type) (const char *username, const char *shadow_pass, Datum validuntil_time, bool validuntil_null);
extern PGDLLIMPORT check_password_hook_type check_password_hook;
On Thu, May 11, 2017 at 10:08:30AM +0900, Michael Paquier wrote:
On Wed, May 10, 2017 at 10:22 PM, Michael Paquier <michael.paquier@gmail.com> wrote:
On Wed, May 10, 2017 at 10:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Heikki Linnakangas <hlinnaka@iki.fi> writes:
Also note that changing the signature check_password_hook_type would
break any external modules that use the hook. Removing
PASSWORD_TYPE_PLAINTEXT will do that too, because any password hook
function would use that constant (see e.g. contrib/passwordcheck). If we
were to change the signature, I'd actually like to simplify it by
removing the password_type parameter altogether. The hook function can
call get_password_type() on the password itself to get the same
information. But it's not worth changing the API and breaking external
modules for that.Ahah. I just had the same thought before reading this message.
And attached is a patch to do that. I am all for this one to get a
more simple interface in place.
[Action required within three days. This is a generic notification.]
The above-described topic is currently a PostgreSQL 10 open item. Heikki,
since you committed the patch believed to have created it, you own this open
item. If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know. Otherwise, please observe the policy on
open item ownership[1]/messages/by-id/20170404140717.GA2675809@tornado.leadboat.com and send a status update within three calendar days of
this message. Include a date for your subsequent status update. Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10. Consequently, I will appreciate your efforts
toward speedy resolution. Thanks.
[1]: /messages/by-id/20170404140717.GA2675809@tornado.leadboat.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 05/15/2017 07:03 AM, Noah Misch wrote:
On Thu, May 11, 2017 at 10:08:30AM +0900, Michael Paquier wrote:
On Wed, May 10, 2017 at 10:22 PM, Michael Paquier <michael.paquier@gmail.com> wrote:
On Wed, May 10, 2017 at 10:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Heikki Linnakangas <hlinnaka@iki.fi> writes:
Also note that changing the signature check_password_hook_type would
break any external modules that use the hook. Removing
PASSWORD_TYPE_PLAINTEXT will do that too, because any password hook
function would use that constant (see e.g. contrib/passwordcheck). If we
were to change the signature, I'd actually like to simplify it by
removing the password_type parameter altogether. The hook function can
call get_password_type() on the password itself to get the same
information. But it's not worth changing the API and breaking external
modules for that.Ahah. I just had the same thought before reading this message.
And attached is a patch to do that. I am all for this one to get a
more simple interface in place.[Action required within three days. This is a generic notification.]
I still don't think this worth it to change the hook function's
signature for this. Even though it's not a big deal for external modules
to adapt, it's not zero effort either. And it's not that much nicer to us.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, May 18, 2017 at 2:37 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
I still don't think this worth it to change the hook function's signature
for this. Even though it's not a big deal for external modules to adapt,
it's not zero effort either. And it's not that much nicer to us.
I favor committing the proposed patch. Otherwise, it just becomes
pointless baggage that we carry forever.
Anyone else want to vote? So far I count 3-1 in favor of making this change.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Robert Haas <robertmhaas@gmail.com> writes:
On Thu, May 18, 2017 at 2:37 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
I still don't think this worth it to change the hook function's signature
for this. Even though it's not a big deal for external modules to adapt,
it's not zero effort either. And it's not that much nicer to us.
I favor committing the proposed patch. Otherwise, it just becomes
pointless baggage that we carry forever.
Anyone else want to vote? So far I count 3-1 in favor of making this change.
Actually, on looking at the final form of the patch, it's hard to think
that it's not just useless API churn. The one existing hook user would
have to turn around and call get_password_type() anyway, so it's not
an improvement for that use-case. What's the argument that most other
use-cases wouldn't need to do the same?
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, May 19, 2017 at 5:31 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Anyone else want to vote? So far I count 3-1 in favor of making this change.
Actually, on looking at the final form of the patch, it's hard to think
that it's not just useless API churn. The one existing hook user would
have to turn around and call get_password_type() anyway, so it's not
an improvement for that use-case. What's the argument that most other
use-cases wouldn't need to do the same?
OK, make that 2-2 in favor of the change.
I guess it does seem likely that most users of the hook would need to
do the same, but it seems confusing to pass the same function both x
and f(x), so my vote is to not do that. But I'm not disposed to spend
a lot of energy arguing about it, so if other people feel differently,
that's cool. I just want to reach a decision and either do this or
drop it from the open items list.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Robert Haas <robertmhaas@gmail.com> writes:
I guess it does seem likely that most users of the hook would need to
do the same, but it seems confusing to pass the same function both x
and f(x), so my vote is to not do that.
I guess what's in the back of my mind is that the password type might
someday not be just a function of the password, but require other
inputs. That is, if we change the hook signature as proposed, then
the signature of get_password_type() also becomes part of that API.
If someday f(x) needs to become f(x,y), that becomes either more API
breakage for users of the hook, or no change at all because it's the
callers' problem.
Maybe there's no reason to believe that that will ever happen.
But I'm not disposed to spend
a lot of energy arguing about it, so if other people feel differently,
that's cool.
TBH, I'm not that hot about it either. But I'm thinking this
is an API break we don't need.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 05/20/2017 05:41 AM, Tom Lane wrote:
Robert Haas <robertmhaas@gmail.com> writes:
I guess it does seem likely that most users of the hook would need to
do the same, but it seems confusing to pass the same function both x
and f(x), so my vote is to not do that.
Right, that was my thinking. (Except that my vote is to nevertheless
keep it unchanged.)
I guess what's in the back of my mind is that the password type might
someday not be just a function of the password, but require other
inputs. That is, if we change the hook signature as proposed, then
the signature of get_password_type() also becomes part of that API.
If someday f(x) needs to become f(x,y), that becomes either more API
breakage for users of the hook, or no change at all because it's the
callers' problem.Maybe there's no reason to believe that that will ever happen.
I don't see that happening. It's too convenient that a password verifier
is self-identifying, i.e. that you can tell what kind of a verifier it
is, just by looking at the value, without any out-of-band information.
Although when we talked about the representation of password verifiers
in pg_authid.rolpassword, there was discussion on having a separate
"password type" field. I was against it, but some thought it would be a
good idea.
But I'm not disposed to spend
a lot of energy arguing about it, so if other people feel differently,
that's cool.TBH, I'm not that hot about it either. But I'm thinking this
is an API break we don't need.
I'm going to just remove this from the open items list. But if some
other committer disagrees strongly and wants to commit this, I won't object.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers