From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
To: Michael Paquier <michael.paquier@gmail.com>
Cc: Magnus Hagander <magnus@hagander.net>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: David Steele <david@pgmasters.net>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: David Fetter <david@fetter.org>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Julian Markwort <julian.markwort@uni-muenster.de>
Cc: Stephen Frost <sfrost@snowman.net>
Cc: PostgreSQL mailing lists <pgsql-hackers@postgresql.org>
Cc: Valery Popov <v.popov@postgrespro.ru>
Subject: Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
Date: Wed, 1 Feb 2017 13:13:38 +0200
Message-ID: <7aa9b0be-d08d-2a9f-acfc-c9718d974e46@iki.fi> (raw)
In-Reply-To: <a959cf82-950e-bd04-235d-aabdad792628@2ndquadrant.com>
References: <21eecdb8-365b-8347-1ae2-5278d1fe798c@iki.fi>
<CAB7nPqT2UGWm0ukdomC+mG-co6nmKer62vuSGRxwtc+jS8uQPA@mail.gmail.com>
<CAB7nPqQtD7jOpYCsB4Ga=Cz+tgh4Ngac6NAV-WLs9zxaw0Uu7w@mail.gmail.com>
<762f4875-a87e-cf0b-c06b-b357294d8be1@iki.fi>
<CAB7nPqTi5jC_AbVMOGidrVy1z6t3v==vWFvdNMxnQEN-WsutKg@mail.gmail.com>
<2d07165c-1793-e243-a2a9-e45b624c7580@iki.fi>
<CABUevEyi7uw41nzyFRh2xUzB6uOh-QmavcwDMhW0jCTTe4KavQ@mail.gmail.com>
<9a7263eb-0980-2072-4424-440bb2513dc7@iki.fi>
<CAB7nPqR4RDjr1PYKm4D+aV4gxgQduq8GWb7SmEOnvoctnvVzsw@mail.gmail.com>
<a150e7c7-7d57-bf1c-7f73-cbbfedf99243@iki.fi>
<20161216013114.GA1400@paquier.xyz>
<a356e0d2-5330-2dd8-7faa-af8758db1be4@iki.fi>
<CAB7nPqQJg_mUd7c5r4OfYPwRB5cujwPv3cZikuP8cU7cbUKi6A@mail.gmail.com>
<6831df67-7641-1a66-4985-268609a4821f@iki.fi>
<a959cf82-950e-bd04-235d-aabdad792628@2ndquadrant.com>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: hlinnaka@iki.fi, peter.eisentraut@2ndquadrant.com, michael.paquier@gmail.com, magnus@hagander.net, robertmhaas@gmail.com, andres@anarazel.de, david@pgmasters.net, tgl@sss.pgh.pa.us, david@fetter.org, alvherre@2ndquadrant.com, julian.markwort@uni-muenster.de, sfrost@snowman.net, v.popov@postgrespro.ru
Subject: Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
In-Reply-To: <7aa9b0be-d08d-2a9f-acfc-c9718d974e46@iki.fi>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox