Improving psql's \password command - Mailing list pgsql-hackers

The attached patch responds to the discussion at [1].  That thread
pointed out that if you issue "\password" with no argument, psql tries
to set the password for the role identified by PQuser(conn), that is,
the originally requested login name.  That's problematic because
if you've done SET SESSION AUTHORIZATION or SET ROLE, you may no
longer have permissions to set the password for the original role.
Even if you do, that might not be what you were expecting, especially
since the psql documentation specifically says it sets the password
for "the current user".

The solution adopted here is to do "SELECT CURRENT_USER" to acquire
the correct role name, and to include the role name in the password
prompt so as to make it very clear which password is to be set.

While testing that, I noticed another bit of user-unfriendliness:
there's no obvious way to get out of it if you realize you are
setting the wrong user's password.  simple_prompt() ignores
control-C, and when you give up and press return, you'll just
get the prompt to enter the password again.  If at this point
you have the presence of mind to enter a deliberately different
string, you'll be out of the woods.  If you don't, and just hit
return again, you will get this response from the backend:

NOTICE:  empty string is not a valid password, clearing password

which is just about the worst default behavior I can think of.
If you're superuser, and you meant to set the password for user1
but typed user2 instead, you just clobbered user2's password,
and you have no easy way to undo that.

What I propose here is that we just refuse to let you set an
empty password this way:

postgres=# \password joe
Enter new password for user "joe":
Empty string is not a valid password.
postgres=#

If you actually want to clear the password, I don't see anything
wrong with entering "alter user joe password null" explicitly.

It's not clear to me whether we ought to back-patch some or all
of this.  In the other thread I expressed doubt about back-patching
security-relevant behavior changes, and that still seems like a
serious concern; but on the other hand, the behaviors enumerated
above are pretty bad.  Since \password is probably only used
interactively, it seems like we could get away with making these
changes: the addition of the user name to the prompt should be
enough to cue anyone about what's really going to happen.

A compromise position could be to keep PQuser() as the default
target role name in the back branches, but back-patch the other
aspects (the prompt addition and the exit on empty password).

Thoughts?

            regards, tom lane

[1] https://www.postgresql.org/message-id/flat/B340250F-A0E3-43BF-A1FB-2AE36003F68D%40gmail.com

diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 49d4c0e3ce..fac4b1e87f 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -2023,28 +2023,53 @@ exec_command_password(PsqlScanState scan_state, bool active_branch)

     if (active_branch)
     {
-        char       *opt0 = psql_scan_slash_option(scan_state,
+        char       *user = psql_scan_slash_option(scan_state,
                                                   OT_SQLID, NULL, true);
-        char       *pw1;
-        char       *pw2;
+        char       *pw1 = NULL;
+        char       *pw2 = NULL;
+        PQExpBufferData buf;

-        pw1 = simple_prompt("Enter new password: ", false);
-        pw2 = simple_prompt("Enter it again: ", false);
+        initPQExpBuffer(&buf);

-        if (strcmp(pw1, pw2) != 0)
+        if (user == NULL)
         {
-            pg_log_error("Passwords didn't match.");
-            success = false;
+            /* Fetch current user so we can report whose PW will be changed */
+            PGresult   *res;
+
+            res = PSQLexec("SELECT CURRENT_USER");
+            if (!res)
+                success = false;
+            else
+            {
+                user = pg_strdup(PQgetvalue(res, 0, 0));
+                PQclear(res);
+            }
         }
-        else
-        {
-            char       *user;
-            char       *encrypted_password;

-            if (opt0)
-                user = opt0;
+        if (success)
+        {
+            printfPQExpBuffer(&buf, _("Enter new password for user \"%s\": "),
+                              user);
+            pw1 = simple_prompt(buf.data, false);
+            if (*pw1 == '\0')
+            {
+                pg_log_error("Empty string is not a valid password.");
+                success = false;
+            }
             else
-                user = PQuser(pset.db);
+            {
+                pw2 = simple_prompt("Enter it again: ", false);
+                if (strcmp(pw1, pw2) != 0)
+                {
+                    pg_log_error("Passwords didn't match.");
+                    success = false;
+                }
+            }
+        }
+
+        if (success)
+        {
+            char       *encrypted_password;

             encrypted_password = PQencryptPasswordConn(pset.db, pw1, user, NULL);

@@ -2055,15 +2080,12 @@ exec_command_password(PsqlScanState scan_state, bool active_branch)
             }
             else
             {
-                PQExpBufferData buf;
                 PGresult   *res;

-                initPQExpBuffer(&buf);
                 printfPQExpBuffer(&buf, "ALTER USER %s PASSWORD ",
                                   fmtId(user));
                 appendStringLiteralConn(&buf, encrypted_password, pset.db);
                 res = PSQLexec(buf.data);
-                termPQExpBuffer(&buf);
                 if (!res)
                     success = false;
                 else
@@ -2072,10 +2094,13 @@ exec_command_password(PsqlScanState scan_state, bool active_branch)
             }
         }

-        if (opt0)
-            free(opt0);
-        free(pw1);
-        free(pw2);
+        if (user)
+            free(user);
+        if (pw1)
+            free(pw1);
+        if (pw2)
+            free(pw2);
+        termPQExpBuffer(&buf);
     }
     else
         ignore_slash_options(scan_state);