Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1d3azu-000409-HY for pgsql-hackers@arkaria.postgresql.org; Thu, 27 Apr 2017 04:25:42 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84_2) (envelope-from ) id 1d3azt-0004zu-Tj for pgsql-hackers@arkaria.postgresql.org; Thu, 27 Apr 2017 04:25:41 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1d3azr-0004zi-UK for pgsql-hackers@postgresql.org; Thu, 27 Apr 2017 04:25:40 +0000 Received: from mail-it0-x230.google.com ([2607:f8b0:4001:c0b::230]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1d3azg-0003ha-L3 for pgsql-hackers@postgresql.org; Thu, 27 Apr 2017 04:25:39 +0000 Received: by mail-it0-x230.google.com with SMTP id 70so5121606ita.0 for ; Wed, 26 Apr 2017 21:25:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=W3SnSa9aML36PgCt0bvrEOTcSXB66t7wJ3GWw+OWAC0=; b=JJl81Av9rbMylRO0dY1azTdSwB5J8hyMHd7GbJHCQKuRal6pxoSO57aDL5Dyhxw89n G1d0ZpZiQZ7b5/851a1HcC6VMurG8a2kV/G0C+t6k/t7OzvKp8/i/SiemeZ3jxpNMGva qRm8pWdOtTT6poKmFAr3tytyt4nGUcHajlNhuKIl4uSJr5tFz3bJLqPPJAWxlmwoMoX/ UIct9ggcII/uIX4w/IcwrSWbs0w7l2ctZCvbo2R8XCF/xzjQAYBzI3FNQOsJ+y42MidV tMJRNCdAgVtpuVBLYztgqRwN1qHUL3n3T53gdagXjlH3hTS7pUq86cH/2HZKrI8EHMq7 Ky5w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=W3SnSa9aML36PgCt0bvrEOTcSXB66t7wJ3GWw+OWAC0=; b=ghBwgNj9u+GH4Ny9pXQEw9ZjzNtIg+hRYK+ALGcgGgCZCNI9WDGpZqFEFdiYi/7nwt UD6m9mRjDyZYOCV6m5y808fbvlk3fs14F0tOS5gkBPw6xEKvsxsJGGoB0B5TrpIV/idy UwEdrDxaV7AQoveKTaFjIlPRwAp8qTU/dWOMvgeW1c+6FQ8G7nd9ziY2u4H33lCIf9PW lmK/pw6btH16j5lvzm8lMLyIumnMFiApjo29JBVB6SFsb/T0d6F1UpOD8hKautPPfqyQ lplW3/c9Yu0P3jokDU+VGdZbIwKpFbpAdbm1I1/rSs8QBInPMSI+ShWhxCNuXJ94vERG vVXg== X-Gm-Message-State: AN3rC/52sUyT9vlZ3MYfYs181pFACSI40wln5+7FnBQrqQBJK64aEmUh 4DtwtyMzS9R0eDORY+/KiCG+gNU+PjBA X-Received: by 10.36.106.134 with SMTP id l128mr71212itc.23.1493267126380; Wed, 26 Apr 2017 21:25:26 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.178.76 with HTTP; Wed, 26 Apr 2017 21:25:05 -0700 (PDT) In-Reply-To: References: <5478ab75-404f-9490-9355-dc1365ce3d7f@iki.fi> <20170410035323.GA2846583@tornado.leadboat.com> <66e776c9-e02c-3683-28df-339f7f64ea66@iki.fi> From: Ashesh Vashi Date: Thu, 27 Apr 2017 09:55:05 +0530 Message-ID: Subject: Re: scram and \password To: Heikki Linnakangas Cc: Michael Paquier , Noah Misch , Robert Haas , Tom Lane , Jeff Janes , Joe Conway , pgsql-hackers Content-Type: multipart/alternative; boundary=001a113acfde9fb9ee054e1e58e2 X-Pg-Spam-Score: -2.6 (--) List-Archive: List-Help: List-ID: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: X-Mailing-List: pgsql-hackers Precedence: bulk Sender: pgsql-hackers-owner@postgresql.org --001a113acfde9fb9ee054e1e58e2 Content-Type: text/plain; charset=UTF-8 On Tue, Apr 25, 2017 at 8:56 PM, Heikki Linnakangas wrote: > On 04/22/2017 01:20 AM, Michael Paquier wrote: > >> On Sat, Apr 22, 2017 at 5:04 AM, Heikki Linnakangas >> wrote: >> >>> I'll continue reviewing the rest of the patch on Monday, but one glaring >>> issue is that we cannot add an argument to the existing libpq >>> PQencryptPassword() function, because that's part of the public API. It >>> would break all applications that use PQencryptPassword(). >>> >>> What we need to do is to add a new function. What should we call that? We >>> don't have a tradition of using "Ex" or such suffix to mark extended >>> versions of existing functions. PQencryptPasswordWithMethod(user, pass, >>> method) ? >>> >> >> Do we really want to add a new function or have a hard failure? Any >> application calling PQencryptPassword may trap itself silently if the >> server uses scram as hba key or if the default is switched to that, >> from this point of view extending the function makes sense as well. >> > > Yeah, there is that. But we simply cannot change the signature of an > existing function. It would not only produce compile-time errors when > building old applications, which would arguably be a good thing, but it > would also cause old binaries to segfault when dynamically linked with the > new libpq. > > I think it's clear that we should have a new function that takes the > algorithm as argument. But there are open decisions on what the old > PQencryptPassword() function should do, and also what the new function > should do by default, if you don't specify an algorithm: > > A) Have PQencryptPassword() return an md5 hash. > > B) Have PQencryptPassword() return a SCRAM verifier > > C) Have PQencryptPassword() return a SCRAM verifier if connected to a v10 > server, and an md5 hash otherwise. This is tricky, because > PQencryptPassword doesn't take a PGconn argument. It could behave like > PQescapeString() does, and choose md5/scram depending on the server version > of the last connection that was established. > > For the new function, it's probably best to pass a PGconn argument. That > way we can use the connection to determine the default, and it seems to be > a good idea for future-proofing too. And an extra "options" argument might > be good, while we're at it, to e.g. specify the number of iterations for > SCRAM. So all in all, I propose the documentation for these functions to be > (I chose option C from above for this): > > ---- > char * > PQencryptPasswordConn(PGconn *conn, > const char *passwd, > const char *user, > const char *method, > const char *options) > I was just thinking: - Do we need to provide the method here? We have connection object itself, it can decide from the type of connection, which method to be used. -- Thanks, Ashesh > > [this paragraph is the same as current PQencryptPassword()] > This function is intended to be used by client applications that wish to > send commands like ALTER ROLE joe PASSWORD 'pwd'. It is good practice to > not send the original cleartext password in such a command, because it > might be exposed in command logs, activity displays and so on. Instead, use > this function to convert the password to encrypted form before it is sent. > [end of unchanged part] > > This function may execute internal queries to the server to determine > appropriate defaults, using the given connection object. The call can > therefore fail if the connection is busy executing another query, or the > current transaction is aborted. > > The return value is a string allocated by malloc, or NULL if out of memory > or other error. On error, a suitable message is stored in the 'conn' > object. The caller can assume the string doesn't contain any special > characters that would require escaping. Use PQfreemem to free the result > when done with it. > > The function arguments are: > > conn > Connection object for the database where the password is to be changed. > > passwd > The new password > > user > Name of the role whose password is to be changed > > method > Name of the password encryption method to use. Currently supported > methods are "md5" or "scram-sha-256". If method is NULL, the default for > the current database is used. [i.e. this looks at password_encryption] > > options > Options specific to the encryption method, or NULL to use the defaults. > (This argument is for future expansion, there are currently no options, and > you should always pass NULL.) > > > char * > PQencryptPassword(const char *passwd, const char *user) > > PQencryptPassword is an older, deprecated version of PQencryptPasswodConn. > The difference is that PQencryptPassword does not require a connection > object. The encryption method will be chosen depending on the server > version of the last established connection, and built-in default options. > > ---- > > Thoughts? Unless someone has better ideas or objections, I'll go implement > 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 > --001a113acfde9fb9ee054e1e58e2 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable

On Tue, Apr 25, 2017 at 8:56 PM, Heikki Linn= akangas <hlinnaka@iki.fi> wrote:
On 04/22/2017 01:20 AM, Michael Paquier wrote:
On Sat, Apr 22, 2017 at 5:04 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
I'll continue reviewing the rest of the patch on Monday, but one glarin= g
issue is that we cannot add an argument to the existing libpq
PQencryptPassword() function, because that's part of the public API. It=
would break all applications that use PQencryptPassword().

What we need to do is to add a new function. What should we call that? We don't have a tradition of using "Ex" or such suffix to mark e= xtended
versions of existing functions. PQencryptPasswordWithMethod(user, pass= ,
method) ?

Do we really want to add a new function or have a hard failure? Any
application calling PQencryptPassword may trap itself silently if the
server uses scram as hba key or if the default is switched to that,
from this point of view extending the function makes sense as well.

 Yeah, there is that. But we simply cannot change the signature of an existi= ng function. It would not only produce compile-time errors when building ol= d applications, which would arguably be a good thing, but it would also cau= se old binaries to segfault when dynamically linked with the new libpq.

I think it's clear that we should have a new function that takes the al= gorithm as argument. But there are open decisions on what the old PQencrypt= Password() function should do, and also what the new function should do by = default, if you don't specify an algorithm:

A) Have PQencryptPassword() return an md5 hash.

B) Have PQencryptPassword() return a SCRAM verifier

C) Have PQencryptPassword() return a SCRAM verifier if connected to a v10 s= erver, and an md5 hash otherwise. This is tricky, because PQencryptPassword= doesn't take a PGconn argument. It could behave like PQescapeString() = does, and choose md5/scram depending on the server version of the last conn= ection that was established.

For the new function, it's probably best to pass a PGconn argument. Tha= t way we can use the connection to determine the default, and it seems to b= e a good idea for future-proofing too. And an extra "options" arg= ument might be good, while we're at it, to e.g. specify the number of i= terations for SCRAM. So all in all, I propose the documentation for these f= unctions to be (I chose option C from above for this):

----
char *
PQencryptPasswordConn(PGconn *conn,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 const char *passwd,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 const char *user,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 const char *method,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 const char *options)
I was just thinking:
- Do we need to provide the method here?
We have connection obj= ect itself, it can decide from the type of connection, which method to be u= sed.

-- Thanks, Ashesh
=C2=A0

[this paragraph is the same as current PQencryptPassword()]
This function is intended to be used by client applications that wish to se= nd commands like ALTER ROLE joe PASSWORD 'pwd'. It is good practice= to not send the original cleartext password in such a command, because it = might be exposed in command logs, activity displays and so on. Instead, use= this function to convert the password to encrypted form before it is sent.=
[end of unchanged part]

This function may execute internal queries to the server to determine appro= priate defaults, using the given connection object. The call can therefore = fail if the connection is busy executing another query, or the current tran= saction is aborted.

The return value is a string allocated by malloc, or NULL if out of memory = or other error. On error, a suitable message is stored in the 'conn'= ; object. The caller can assume the string doesn't contain any special = characters that would require escaping. Use PQfreemem to free the result wh= en done with it.

The function arguments are:

conn
=C2=A0 Connection object for the database where the password is to be chang= ed.

passwd
=C2=A0 The new password

user
=C2=A0 Name of the role whose password is to be changed

method
=C2=A0 Name of the password encryption method to use. Currently supported m= ethods are "md5" or "scram-sha-256". If method is NULL,= the default for the current database is used. [i.e. this looks at password= _encryption]

options
=C2=A0 Options specific to the encryption method, or NULL to use the defaul= ts. (This argument is for future expansion, there are currently no options,= and you should always pass NULL.)


char *
PQencryptPassword(const char *passwd, const char *user)

PQencryptPassword is an older, deprecated version of PQencryptPasswodConn. = The difference is that PQencryptPassword does not require a connection obje= ct. The encryption method will be chosen depending on the server version of= the last established connection, and built-in default options.

----

Thoughts? Unless someone has better ideas or objections, I'll go implem= ent that.

- Heikki




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

--001a113acfde9fb9ee054e1e58e2--