Remove psql's -W option
Folks,
I'd like to $Subject for 12.
There are scripts it could break, but not ones that weren't already
broken in ways important to access control.
What say?
Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
Attachments:
0001-Remove-the-W-option-from-psql-v01.patchtext/x-diff; charset=us-asciiDownload
From f3382131fd2172eed32052231d4d4ec968bd2c6d Mon Sep 17 00:00:00 2001
From: David Fetter <david@fetter.org>
Date: Sat, 21 Jul 2018 14:26:59 -0700
Subject: [PATCH] Remove the -W option from psql (v01)
To: pgsql-hackers@postgresql.org
EFOOTGUN
---
doc/src/sgml/ref/psql-ref.sgml | 25 -------------------------
src/bin/psql/help.c | 1 -
src/bin/psql/startup.c | 6 +-----
3 files changed, 1 insertion(+), 31 deletions(-)
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index b17039d60f..c0ef7102d0 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -503,31 +503,6 @@ EOF
</listitem>
</varlistentry>
- <varlistentry>
- <term><option>-W</option></term>
- <term><option>--password</option></term>
- <listitem>
- <para>
- Force <application>psql</application> to prompt for a
- password before connecting to a database.
- </para>
-
- <para>
- This option is never essential, since <application>psql</application>
- will automatically prompt for a password if the server demands
- password authentication. However, <application>psql</application>
- will waste a connection attempt finding out that the server wants a
- password. In some cases it is worth typing <option>-W</option> to avoid
- the extra connection attempt.
- </para>
-
- <para>
- Note that this option will remain set for the entire session,
- and so it affects uses of the meta-command
- <command>\connect</command> as well as the initial connection attempt.
- </para>
- </listitem>
- </varlistentry>
<varlistentry>
<term><option>-x</option></term>
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index 702e742af4..cd7614364c 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -138,7 +138,6 @@ usage(unsigned short int pager)
env = user;
fprintf(output, _(" -U, --username=USERNAME database user name (default: \"%s\")\n"), env);
fprintf(output, _(" -w, --no-password never prompt for password\n"));
- fprintf(output, _(" -W, --password force password prompt (should happen automatically)\n"));
fprintf(output, _("\nFor more information, type \"\\?\" (for internal commands) or \"\\help\" (for SQL\n"
"commands) from within psql, or consult the psql section in the PostgreSQL\n"
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index be57574cd3..432a583583 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -464,7 +464,6 @@ parse_psql_options(int argc, char *argv[], struct adhoc_opts *options)
{"variable", required_argument, NULL, 'v'},
{"version", no_argument, NULL, 'V'},
{"no-password", no_argument, NULL, 'w'},
- {"password", no_argument, NULL, 'W'},
{"expanded", no_argument, NULL, 'x'},
{"no-psqlrc", no_argument, NULL, 'X'},
{"help", optional_argument, NULL, 1},
@@ -476,7 +475,7 @@ parse_psql_options(int argc, char *argv[], struct adhoc_opts *options)
memset(options, 0, sizeof *options);
- while ((c = getopt_long(argc, argv, "aAbc:d:eEf:F:h:HlL:no:p:P:qR:sStT:U:v:VwWxXz?01",
+ while ((c = getopt_long(argc, argv, "aAbc:d:eEf:F:h:HlL:no:p:P:qR:sStT:U:v:VwxXz?01",
long_options, &optindex)) != -1)
{
switch (c)
@@ -615,9 +614,6 @@ parse_psql_options(int argc, char *argv[], struct adhoc_opts *options)
case 'w':
pset.getPassword = TRI_NO;
break;
- case 'W':
- pset.getPassword = TRI_YES;
- break;
case 'x':
pset.popt.topt.expanded = true;
break;
--
2.17.1
On 21/07/18 23:58, David Fetter wrote:
Folks,
I'd like to $Subject for 12.
There are scripts it could break, but not ones that weren't already
broken in ways important to access control.What say?
I say it should at least throw a sensible error for a few versions
before it's removed completely.
-1 on this patch
+1 for removing the "feature"
--
Vik Fearing +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Hello David,
I'd like to $Subject for 12.
There are scripts it could break, but not ones that weren't already
broken in ways important to access control.What say?
What is the rational?
I'm unsure of the logic behind removing -W (--password) but keeping -w
(--no-password), especially as the internal logic seems kept by the patch.
--
Fabien.
On 22/07/18 00:41, Fabien COELHO wrote:
Hello David,
I'd like to $Subject for 12.
There are scripts it could break, but not ones that weren't already
broken in ways important to access control.What say?
What is the rational?
It's first on our list of things not to do:
https://wiki.postgresql.org/wiki/Don't_Do_This#Don.27t_use_psql_-W_or_--password
--
Vik Fearing +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Vik Fearing <vik.fearing@2ndquadrant.com> writes:
On 22/07/18 00:41, Fabien COELHO wrote:
What is the rational?
It's first on our list of things not to do:
https://wiki.postgresql.org/wiki/Don't_Do_This#Don.27t_use_psql_-W_or_--password
As I recall, when this has been discussed in the past, people objected
because they didn't like either (1) the extra server process fork and/or
network round trip caused when a password is needed, or (2) the server
log entry that gets generated about client disconnecting without supplying
a password. (We don't log anything about it normally, but I'm not sure
that that's always true when using PAM, LDAP, connection poolers, etc.)
While those are surely niche concerns, it's not really apparent to me
what we gain by breaking them.
A possible positive reason for removing the option would be if we could
clean up this mess:
/messages/by-id/E1egDgC-000302-FN@gemulon.postgresql.org
But no fair citing that argument without presenting an actual clean-up
patch, because it's not obvious how much cleaner we could make it.
BTW, all of our client programs have this switch, so if we did agree
to remove it, this patch doesn't go nearly far enough.
regards, tom lane
PS: I found some interesting back-story here:
/messages/by-id/200712091148.54294.xzilla@users.sourceforge.net
It's first on our list of things not to do:
https://wiki.postgresql.org/wiki/Don't_Do_This#Don.27t_use_psql_-W_or_--passwordAs I recall, when this has been discussed in the past, people objected
because they didn't like either (1) the extra server process fork and/or
network round trip caused when a password is needed,
Looking at the protocol documentation, I cannot see why a fork would be
needed, because the password could be asked for when required at the
protocol level. If the client knew.
However, libpq does not seem to expose this logic, and/or it is not used
by "psql" which simply loops over "PQconnectdbParams", which seems to
reconnect from scratch each time (connectDBStart ends with
pqDropConnection).
Given the depth of function (PQconnectStartParams, connectDBStart,
PQconnectPool, pg_fe_sendauth, pg_password_sendauth), changing this
behavior without re-designing the whole connection functions and rewriting
the client logic, thus breaking compatibility, looks like a pain.
A possible positive reason for removing the option would be if we could
clean up this mess:
/messages/by-id/E1egDgC-000302-FN@gemulon.postgresql.org
But no fair citing that argument without presenting an actual clean-up
patch, because it's not obvious how much cleaner we could make it.
Yep.
ISTM that the only way out would be to provide a callback function that
could be used to ask for the password, and that could be called probably
from pg_fe_sendauth (?) and implement the logic currently in psql main,
and probably other clients as well.
PQsetPasswordCallback(myfunction);
And nothing else would be changed at the client level.
However the compatibility is non trivial because of the link dependency.
Maybe there could be a define so that a client could be compatible with
older lib versions, eg:
#ifdef LIBPQ_HAS_SET_PASSWORD_CALLBACK
PQsetPasswordCallback(myfunction);
#endif
Possibly this is acceptable. Not sure.
Otherwise ISTM that "-W/--password" still has some minimal value thus does
not deserve to be thrown out that quickly.
--
Fabien
On Sun, Jul 22, 2018 at 9:35 AM, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
Otherwise ISTM that "-W/--password" still has some minimal value thus does
not deserve to be thrown out that quickly.
I think I agree. I don't think this option is really hurting
anything, so I'm not quite sure why we would want to abruptly get rid
of it.
I also think your other question is a good one. It seems like the
fact that we need to reconnect -- rather than just prompting for the
password and then sending it when we get it -- is an artifact of how
libpq is designed rather than an intrinsic limitation of the protocol.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Mon, Jul 23, 2018 at 11:20:46AM -0400, Robert Haas wrote:
On Sun, Jul 22, 2018 at 9:35 AM, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
Otherwise ISTM that "-W/--password" still has some minimal value thus does
not deserve to be thrown out that quickly.I think I agree. I don't think this option is really hurting
anything, so I'm not quite sure why we would want to abruptly get rid
of it.I also think your other question is a good one. It seems like the
fact that we need to reconnect -- rather than just prompting for the
password and then sending it when we get it -- is an artifact of how
libpq is designed rather than an intrinsic limitation of the protocol.
Am I understanding correctly that doing the following would be
acceptable, assuming good code quality?
- Rearrange libpq so it doesn't force this behavior.
- Deprecate the -W option uniformly in the code we ship by documenting
it and making it send warnings to stderr.
Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
Robert Haas <robertmhaas@gmail.com> writes:
I also think your other question is a good one. It seems like the
fact that we need to reconnect -- rather than just prompting for the
password and then sending it when we get it -- is an artifact of how
libpq is designed rather than an intrinsic limitation of the protocol.
Well, it's a limitation of the libpq API. The problem is that it's the
application, not libpq, that's in charge of actually asking the user for
a password. Right now we inform the app that it needs to do that by
passing back a failed PGconn with appropriate state. We could imagine
passing back a PGconn with a half-finished open connection, and asking
the app to re-submit that PGconn along with a password so we could
continue the auth handshake. But it'd require changing apps to do that.
Also, doing things like that would incur the risk of exceeding
authentication_timeout while the user is typing his password. So we'd
also need some additional complexity to retry in that situation.
regards, tom lane