On 12/24/23 10:14, Joe Conway wrote:
On 12/23/23 11:00, Tom Lane wrote:
Joe Conway <m...@joeconway.com> writes:
The attached patch set moves the guts of \password from psql into the
libpq client side -- PQchangePassword() (patch 0001).
Haven't really read the patch, just looked at the docs, but here's
a bit of bikeshedding:
Thanks!
* This seems way too eager to promote the use of md5. Surely the
default ought to be SCRAM, full stop. I question whether we even
need an algorithm parameter. Perhaps it's a good idea for
future-proofing, but we could also plan that the function would
make its own decisions based on noting the server's version.
(libpq is far more likely to be au courant about what to do than
the calling application, IMO.)
* Parameter order seems a bit odd: to me it'd be natural to write
user before password.
* Docs claim return type is char *, but then say bool (which is
also what the code says). We do not use bool in libpq's API;
the admittedly-hoary convention is to use int with 1/0 values.
Rather than quibble about that, though, I wonder if we should
make the result be the PGresult from the command, which the
caller would be responsible to free. That would document error
conditions much more flexibly. On the downside, client-side
errors would be harder to report, particularly OOM, but I think
we have solutions for that in existing functions.
* The whole business of nonblock mode seems fairly messy here,
and I wonder whether it's worth the trouble to support. If we
do want to claim it works then it ought to be tested.
All of these (except for the doc "char *" cut-n-pasteo) were due to
trying to stay close to the same interface as PQencryptPasswordConn().
But I agree with your assessment and the attached patch series addresses
all of them.
The original use of PQencryptPasswordConn() in psql passed a NULL for
the algorithm, so I dropped that argument entirely. I also swapped user
and password arguments because as you pointed out that is more natural.
This version returns PGresult. As far as special handling for
client-side errors like OOM, I don't see anything beyond returning a
NULL to signify fatal error, e,g,:
8<--------------
PGresult *
PQmakeEmptyPGresult(PGconn *conn, ExecStatusType status)
{
PGresult *result;
result = (PGresult *) malloc(sizeof(PGresult));
if (!result)
return NULL;
8<--------------
That is the approach I took.
One thing I have not done but, considered, is adding an additional
optional parameter to allow "VALID UNTIL" to be set. Seems like it would
be useful to be able to set an expiration when setting a new password.
No strong opinion about that.
Thanks -- hopefully others will weigh in on that.
I just read through the thread and my conclusion is that, specifically
related to this patch set (i.e. notwithstanding suggestions for related
features), there is consensus in favor of adding this feature.
The only code specific comments were Tom's above, which have been
addressed. If there are no serious objections I plan to commit this
relatively soon.
--
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
diff --git a/src/interfaces/libpq/exports.txt b/src/interfaces/libpq/exports.txt
index 850734a..28b861f 100644
*** a/src/interfaces/libpq/exports.txt
--- b/src/interfaces/libpq/exports.txt
*************** PQclosePrepared 188
*** 191,193 ****
--- 191,194 ----
PQclosePortal 189
PQsendClosePrepared 190
PQsendClosePortal 191
+ PQchangePassword 192
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index 912aa14..a6897f8 100644
*** a/src/interfaces/libpq/fe-auth.c
--- b/src/interfaces/libpq/fe-auth.c
*************** PQencryptPasswordConn(PGconn *conn, cons
*** 1372,1374 ****
--- 1372,1459 ----
return crypt_pwd;
}
+
+ /*
+ * PQchangePassword -- exported routine to change a password
+ *
+ * This is intended to be used by client applications that wish to
+ * change the password for a user. The password is not sent in
+ * cleartext because it is encrypted on the client side. This is
+ * good because it ensures the cleartext password is never known by
+ * the server, and therefore won't end up in logs, pg_stat displays,
+ * etc. We export the function so that clients won't be dependent
+ * on the implementation specific details with respect to how the
+ * server changes passwords.
+ *
+ * Arguments are a connection object, the SQL name of the target user,
+ * and the cleartext password.
+ *
+ * Return value is the PGresult of the executed ALTER USER statement
+ * or NULL if we never get there. The caller is responsible to PQclear()
+ * the returned PGresult.
+ *
+ * PQresultStatus() should be called to check the return value for errors,
+ * and PQerrorMessage() used to get more information about such errors.
+ */
+ PGresult *
+ PQchangePassword(PGconn *conn, const char *user, const char *passwd)
+ {
+ char *encrypted_password = NULL;
+ PQExpBufferData buf;
+
+ encrypted_password = PQencryptPasswordConn(conn, passwd, user, NULL);
+
+ if (!encrypted_password)
+ {
+ /* PQencryptPasswordConn() already registered the error */
+ return NULL;
+ }
+ else
+ {
+ char *fmtpw = NULL;
+
+ fmtpw = PQescapeLiteral(conn, encrypted_password,
+ strlen(encrypted_password));
+
+ /* no longer needed, so clean up now */
+ PQfreemem(encrypted_password);
+
+ if (!fmtpw)
+ {
+ /* PQescapeLiteral() already registered the error */
+ return NULL;
+ }
+ else
+ {
+ char *fmtuser = NULL;
+
+ fmtuser = PQescapeIdentifier(conn, user, strlen(user));
+ if (!fmtuser)
+ {
+ /* PQescapeIdentifier() already registered the error */
+ PQfreemem(fmtpw);
+ return NULL;
+ }
+ else
+ {
+ PGresult *res;
+
+ initPQExpBuffer(&buf);
+ printfPQExpBuffer(&buf, "ALTER USER %s PASSWORD %s",
+ fmtuser, fmtpw);
+
+ res = PQexec(conn, buf.data);
+
+ /* clean up */
+ termPQExpBuffer(&buf);
+ PQfreemem(fmtuser);
+ PQfreemem(fmtpw);
+
+ if (!res)
+ return NULL;
+ else
+ return res;
+ }
+ }
+ }
+ }
diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h
index 97762d5..bc8e6de 100644
*** a/src/interfaces/libpq/libpq-fe.h
--- b/src/interfaces/libpq/libpq-fe.h
*************** extern int PQenv2encoding(void);
*** 659,664 ****
--- 659,665 ----
extern char *PQencryptPassword(const char *passwd, const char *user);
extern char *PQencryptPasswordConn(PGconn *conn, const char *passwd, const char *user, const char *algorithm);
+ extern PGresult *PQchangePassword(PGconn *conn, const char *user, const char *passwd);
/* === in encnames.c === */
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 82cc091..81402e0 100644
*** a/src/bin/psql/command.c
--- b/src/bin/psql/command.c
*************** exec_command_password(PsqlScanState scan
*** 2158,2186 ****
}
else
{
! char *encrypted_password;
!
! encrypted_password = PQencryptPasswordConn(pset.db, pw1, user, NULL);
! if (!encrypted_password)
{
pg_log_info("%s", PQerrorMessage(pset.db));
success = false;
}
- else
- {
- PGresult *res;
! printfPQExpBuffer(&buf, "ALTER USER %s PASSWORD ",
! fmtId(user));
! appendStringLiteralConn(&buf, encrypted_password, pset.db);
! res = PSQLexec(buf.data);
! if (!res)
! success = false;
! else
! PQclear(res);
! PQfreemem(encrypted_password);
! }
}
free(user);
--- 2158,2172 ----
}
else
{
! PGresult *res = PQchangePassword(pset.db, user, pw1);
! if (PQresultStatus(res) != PGRES_COMMAND_OK)
{
pg_log_info("%s", PQerrorMessage(pset.db));
success = false;
}
! PQclear(res);
}
free(user);
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index ed88ac0..4c71ea4 100644
*** a/doc/src/sgml/libpq.sgml
--- b/doc/src/sgml/libpq.sgml
*************** char *PQencryptPasswordConn(PGconn *conn
*** 7116,7121 ****
--- 7116,7154 ----
</listitem>
</varlistentry>
+ <varlistentry id="libpq-PQchangePassword">
+ <term><function>PQchangePassword</function><indexterm><primary>PQchangePassword</primary></indexterm></term>
+
+ <listitem>
+ <para>
+ Changes a <productname>PostgreSQL</productname> password.
+ <synopsis>
+ PGresult *PQchangePassword(PGconn *conn, const char *user, const char *passwd);
+ </synopsis>
+ This function uses <function>PQencryptPasswordConn</function>
+ to build and execute the command <literal>ALTER USER ... PASSWORD
+ '...'</literal>, thereby changing the user's password. It exists for
+ the same reason as <function>PQencryptPasswordConn</function>, but
+ is more convenient as it both builds and runs the command for you.
+ </para>
+
+ <para>
+ The <parameter>user</parameter> and <parameter>passwd</parameter> arguments
+ are the SQL name of the target user, and the new cleartext password.
+ </para>
+
+ <para>
+ Returns a <structname>PGresult</structname> pointer or possibly a null
+ pointer. The <xref linkend="libpq-PQresultStatus"/> function
+ should be called to check the return value for any errors (including
+ the value of a null pointer, in which case it will return
+ <symbol>PGRES_FATAL_ERROR</symbol>). Use
+ <xref linkend="libpq-PQerrorMessage"/> to get more information about such
+ errors.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="libpq-PQencryptPassword">
<term><function>PQencryptPassword</function><indexterm><primary>PQencryptPassword</primary></indexterm></term>
