Re: [HACKERS] scram and \password - Mailing list pgsql-hackers

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

[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