On Tue, Apr 25, 2017 at 8:56 PM, Heikki Linnakangas <hlinn...@iki.fi> wrote:
> On 04/22/2017 01:20 AM, Michael Paquier wrote: > >> On Sat, Apr 22, 2017 at 5:04 AM, Heikki Linnakangas <hlinn...@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) > 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 >
