Обсуждение: [HACKERS] Removal of plaintext password type references

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 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

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 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.

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

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

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

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

-- 
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 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] 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] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

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

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

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

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

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

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