On 1/6/24 15:10, Tom Lane wrote:
Joe Conway <m...@joeconway.com> writes:
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.
I had not actually read this patchset before, but now I have, and
I have a few minor suggestions:
Many thanks!
* The API comment for PQchangePassword should specify that encryption
is done according to the server's password_encryption setting, and
probably the SGML docs should too. You shouldn't have to read the
code to discover that.
Check
* I don't especially care for the useless initializations of
encrypted_password, fmtpw, and fmtuser. In all those cases the
initial NULL is immediately replaced by a valid value. Probably
the compiler will figure out that the initializations are useless,
but human readers have to do so as well. Moreover, I think this
style is more bug-prone not less so, because if you ever change
the logic in a way that causes some code paths to fail to set
the variables, you won't get use-of-possibly-uninitialized-value
warnings from the compiler.
* Perhaps move the declaration of "buf" to the inner block where
it's actually used?
Makes sense -- fixed
* This could be shortened to just "return res":
+ if (!res)
+ return NULL;
+ else
+ return res;
Heh, apparently I needed more coffee at this point :-)
* I'd make the SGML documentation a bit more explicit about the
return value, say
+ Returns a <structname>PGresult</structname> pointer representing
+ the result of the <literal>ALTER USER</literal> command, or
+ a null pointer if the routine failed before issuing any command.
Fixed.
I also ran pgindent. I was kind of surprised/unhappy when it made me
change this (which aligned the two var names):
8<------------
<tab><tab><tab><tab>PQExpBufferData<tab>buf;
<tab><tab><tab><tab>PGresult<tab><sp><sp><sp>*res;
8<------------
to this (which leaves the var names unaligned):
8<------------
<tab><tab><tab><tab>PQExpBufferData<sp>buf;
<tab><tab><tab><tab>PGresult<sp><sp><sp>*res;
8<------------
Anyway, the resulting adjustments attached.
--
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..dcb7c9f 100644
*** a/src/interfaces/libpq/fe-auth.c
--- b/src/interfaces/libpq/fe-auth.c
*************** PQencryptPasswordConn(PGconn *conn, cons
*** 1372,1374 ****
--- 1372,1455 ----
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. The password encryption is performed by PQencryptPasswordConn(),
+ * which is passed a NULL for the algorithm argument. Hence encryption
+ * is done according to the server's password_encryption
+ * setting. 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 = PQencryptPasswordConn(conn, passwd,
+ user, NULL);
+
+ if (!encrypted_password)
+ {
+ /* PQencryptPasswordConn() already registered the error */
+ return NULL;
+ }
+ else
+ {
+ char *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 = PQescapeIdentifier(conn, user, strlen(user));
+
+ if (!fmtuser)
+ {
+ /* PQescapeIdentifier() already registered the error */
+ PQfreemem(fmtpw);
+ return NULL;
+ }
+ else
+ {
+ PQExpBufferData buf;
+ 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);
+
+ 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..983027b 100644
*** a/doc/src/sgml/libpq.sgml
--- b/doc/src/sgml/libpq.sgml
*************** char *PQencryptPasswordConn(PGconn *conn
*** 7116,7121 ****
--- 7116,7159 ----
</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.
+ <xref linkend="libpq-PQencryptPasswordConn"/> is passed a
+ <symbol>NULL</symbol> for the algorithm argument, hence encryption is
+ done according to the server's password_encryption setting.
+ </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 representing
+ the result of the <literal>ALTER USER</literal> command, or
+ a null pointer if the routine failed before issuing any command.
+ 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>
