Message-ID: <55C6C102.6070809@agliodbs.com>
Date: Sat, 08 Aug 2015 19:54:58 -0700
From: Josh Berkus <josh@agliodbs.com>
Organization: PostgreSQL Experts Inc.
User-Agent: Mozilla/5.0 (X11; Linux x86_64;
 rv:31.0) Gecko/20100101 Thunderbird/31.8.0
MIME-Version: 1.0
To: Michael Paquier <michael.paquier@gmail.com>
CC: Heikki Linnakangas <hlinnaka@iki.fi>, Robert Haas <robertmhaas@gmail.com>,
 pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: WIP: SCRAM authentication
References: <55192AFE.6080106@iki.fi>
	<CAB7nPqRB_Z2B7Lz96iHbHUQBQLwmUtj64hXu0u7mMZjpSk8UUg@mail.gmail.com>
	<CAB7nPqRBtLOT=g3q2-uYE=NbBdtkLubvDbV6wfCUp3hubpa6dg@mail.gmail.com>
	<CA+TgmoZ0XxXZeGa0f1uwaCN-FCjFsAJZdJgioeesdhfR7jmEhg@mail.gmail.com>
	<55C4FCC2.3000503@iki.fi>
	<CAB7nPqRWYJmw+qJrawP=h2J5s6LFea3NUb10VJJjdBNr1Wz=+g@mail.gmail.com>
	<CA+TgmoazGS0TpAtrFw+ft1UVUevBBXew5Lo6b1jtozgykpy5PA@mail.gmail.com>
	<55C63B1E.2040101@iki.fi>
	<WM!bb6c003556a9ab74bed5c5a96b8da26b0a7e307b9d3c3626ac9dd949d0b9abdcb49dfe9c7445e6f178be1e5f52c15059!@asav-3.01.com>
	<55C679C7.4060908@agliodbs.com>
 <CAB7nPqS_OdrwXgS6btk9_2xyNPc8fvMjuj1Cg5cLgqCYP1Vbpg@mail.gmail.com>
 <WM!45ad44577280019f7f1f2c534d64ab4d701bc0d6b74ca14d53bea807481d8116362e33ef3726103af55e53072dc7b05f!@asav-2.01.com>
In-Reply-To: 
 <WM!45ad44577280019f7f1f2c534d64ab4d701bc0d6b74ca14d53bea807481d8116362e33ef3726103af55e53072dc7b05f!@asav-2.01.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

On 08/08/2015 03:21 PM, Michael Paquier wrote:
> On Sun, Aug 9, 2015 at 6:51 AM, Josh Berkus <josh@agliodbs.com> wrote:
>> 1. we drop the parameter password_encryption
>> 2. we add the parameter password_storage, which takes a list:
>>    - plain : plain text
>>    - md5 : current md5 hashes
>>    - scram : new scram hashed passwords
>>    This defaults to 'md5, scram' if not specified.
>>    This list might be extended in the future.
> 
> Perhaps using a different GUC than password_encryption is safer... I
> am not that sure. Still that's how I switched password_encryption to
> actually handle a list. Default is 'md5' with the first patch, and
> 'md5,scram' with the scram patch added and it sets the list of
> password verifiers created when PASSWORD with ENCRYPTED/UNENCRYPTED is
> used.

Well, generally I feel like if we're going to change the *type* of a GUC
parameter, we ought to change the *name*.  It's far easier for users to
figure out that the contents of a parameter need to change if the name
is also changed.

In other words, I think "invalid parameter 'password_encryption'" is an
easier to understand error message than "invalid password_encryption
type 'on'".  Besides which, password_encryption was always a misnomer.

Unless you're going to still accept "on, off" in some kind of wierd
backwards-compatibitlity mode?  If so, how does that work?

> Like password ENCRYPTED (md5,scram) or similar? If no method is
> passed, I think that we should default to password_storage instead.

Make sense.

> Also, I still think that something like PASSWORD VERIFIERS is needed,
> users may want to set the verifier user for each method after
> calculating it on client-side: we authorize that for md5 even now, and
> that's not something this spec authorizes.

I don't follow this.  Mind you, I'm not sure that I need to.

>> 5. we add the superuser-only function pg_apply_password_policy().  This
>> applies the policy expressed by password_storage, generating or erasing
>> passwords for each user.
> 
> pg_upgrade could make use of that to control password aging with an
> option to do the cleanup or not. Not sure what the default should be
> though. pg_apply_password_policy(roleid) would be useful as well to do
> it on a role base.

No objections to an optional roleid parameter, if you think people will
use it.

>> 6. We add a new connection error for "authentication __method__ not
>> supported for user"
> 
> Hm? This would let any user trying to connect with a given method know
> that if a method is used or not. What's wrong with failing as we do
> now. In case of PASSWORD NULL for example, an attempt of connection
> fails all the time with "incorrect password" or similar.

So, the DBA sets password_storage = 'scram', but doesn't take the md5
lines out of pg_hba.conf.

The app dev tries to connect using a driver which only supports md5.
What error should they get?  A user/DBA who is getting "invalid
password" is going to spend a long time debugging it.  Also, it would be
very useful to have a distinctive error in the log, so that DBAs could
see who is *trying* to connect with the wrong verifier.

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers