Re: scram and \password

On 04/22/2017 01:20 AM, Michael Paquier wrote:
> On Sat, Apr 22, 2017 at 5:04 AM, Heikki Linnakangas <hlinnaka(at)iki(dot)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