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

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