Changing references of password encryption to hashing

On 03/09/2017 06:16 PM, Michael Paquier wrote:

As discussed here:
/messages/by-id/98cafcd0-5557-0bdf-4837-0f2b7782d0b5@joeconway.com
We are using in documentation and code comments "encryption" to define
what actually is hashing, which is confusing.

Attached is a patch for HEAD to change the documentation to match hashing.

There are a couple of things I noticed on the way:
1) There is the user-visible PQencryptPassword in libpq, which
actually hashes the password, and not encrypts it :)
2) There is as well pg_md5_encrypt() in the code, as well as there is
pg_md5_hash(). Those may be better if renamed, at least I would think
that pg_md5_encrypt should be pg_md5_password_hash, because a salt is
used as well, and the routine is dedicated to work on passwords.
Thoughts?
3) createuser also has --encrypt and --unencrypted, perhaps those
should be renamed? Honestly I don't really think that this is worth a
breakage and the option names match with the SQL commands.

My opinion is that the user visible aspects of this should be deprecated
and correct syntax provided. But perhaps that is overkill.

8<------------
    key and server key, respectively, in hexadecimal format. A password that
-   does not follow either of those formats is assumed to be unencrypted.
+   does not follow either of those formats is assumed to be in plain
format,
+   non-hashed.
   </para>
  </sect1>
8<------------
I think here, instead of "in plain format, non-hashed" it is ok to just
say "cleartext" or maybe "plaintext". Whichever is picked, it should be
used consistently.

8<------------
  <caution>
   <para>
-   To prevent unencrypted passwords from being sent across the network,
+   To prevent non-hashed passwords from being sent across the network,
8<------------
same here "non-hashed" ==> "cleartext"

8<------------
   <para>
-   Caution must be exercised when specifying an unencrypted password
+   Caution must be exercised when specifying a non-hashed password
8<------------
and here

...etc...

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

On Sun, Mar 12, 2017 at 7:51 AM, Joe Conway <mail@joeconway.com> wrote:

On 03/09/2017 06:16 PM, Michael Paquier wrote:

As discussed here:
/messages/by-id/98cafcd0-5557-0bdf-4837-0f2b7782d0b5@joeconway.com
We are using in documentation and code comments "encryption" to define
what actually is hashing, which is confusing.

Attached is a patch for HEAD to change the documentation to match hashing.

There are a couple of things I noticed on the way:
1) There is the user-visible PQencryptPassword in libpq, which
actually hashes the password, and not encrypts it :)
2) There is as well pg_md5_encrypt() in the code, as well as there is
pg_md5_hash(). Those may be better if renamed, at least I would think
that pg_md5_encrypt should be pg_md5_password_hash, because a salt is
used as well, and the routine is dedicated to work on passwords.
Thoughts?
3) createuser also has --encrypt and --unencrypted, perhaps those
should be renamed? Honestly I don't really think that this is worth a
breakage and the option names match with the SQL commands.

My opinion is that the user visible aspects of this should be deprecated
and correct syntax provided. But perhaps that is overkill.

Are ENCRYPTED and UNENCRYPTED keywords part of the SQL specification?
If yes, we had better not do 3) then. 1) and 2) would make future
back-patching harder, so I am not going to push much as I am expecting
some growling on the matter, and a doc-only patch is a step in the
good direction anyway. Extra opinions are welcome.

8<------------
key and server key, respectively, in hexadecimal format. A password that
-   does not follow either of those formats is assumed to be unencrypted.
+   does not follow either of those formats is assumed to be in plain
format,
+   non-hashed.
</para>
</sect1>
8<------------
I think here, instead of "in plain format, non-hashed" it is ok to just
say "cleartext" or maybe "plaintext". Whichever is picked, it should be
used consistently.

OK, thanks. cleartext is present in the docs already, so I have gone
for this term. Updated patch attached.
--
Michael

On 12 March 2017 at 06:51, Joe Conway <mail@joeconway.com> wrote:

My opinion is that the user visible aspects of this should be deprecated
and correct syntax provided. But perhaps that is overkill.

FWIW, in my experience, pretty much nobody understands the pretty
tangled behaviour of "WITH [ENCRYPTED] PASSWORD", you have to
understand the fact table of:

* ENCRYPTED, UNENCRYPTED or neither set
* password_encryption GUC on or off
* password begins / doesn't begin with fixed string 'md5'

to fully know what will happen.

Then of course, you have to understand how all this interacts with
pg_hba.conf's 'password' and 'md5' options.

It's a right mess. Since our catalogs don't keep track of the hash
separately to the password text and use prefixes instead, and since we
need compatibility for dumps, it's hard to do a great deal about
though.

I'm not convinced that a keyword change will do much good, the whole
thing really needs a reassessment to make sure that it's clearer to
users/admins and has fewer moving parts.

So I'm in favour of fixing the docs but I'm not keen on changing the
SQL syntax in a way that just kind of papers over part of the
problems.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Mar 13, 2017 at 04:48:21PM +0800, Craig Ringer wrote:

On 12 March 2017 at 06:51, Joe Conway <mail@joeconway.com> wrote:

My opinion is that the user visible aspects of this should be deprecated
and correct syntax provided. But perhaps that is overkill.

FWIW, in my experience, pretty much nobody understands the pretty
tangled behaviour of "WITH [ENCRYPTED] PASSWORD", you have to
understand the fact table of:

* ENCRYPTED, UNENCRYPTED or neither set
* password_encryption GUC on or off
* password begins / doesn't begin with fixed string 'md5'

to fully know what will happen.

Then of course, you have to understand how all this interacts with
pg_hba.conf's 'password' and 'md5' options.

It's a right mess. Since our catalogs don't keep track of the hash
separately to the password text and use prefixes instead, and since we
need compatibility for dumps, it's hard to do a great deal about
though.

With SCRAM coming in PG 10, is there anything we can do to clean this up
for PG 10?

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Mar 13, 2017 at 4:48 AM, Craig Ringer <craig@2ndquadrant.com> wrote:

So I'm in favour of fixing the docs but I'm not keen on changing the
SQL syntax in a way that just kind of papers over part of the
problems.

I agree. I think that trying to design new SQL syntax at this point
is unlikely to be a good idea - we're just about out of time here, and
some people who might care about this are busy on other things, and
the deadline for patches that do new things has long since passed.
But I like the idea of trying to improve the documentation.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 03/16/2017 06:19 AM, Robert Haas wrote:

On Mon, Mar 13, 2017 at 4:48 AM, Craig Ringer <craig@2ndquadrant.com> wrote:

So I'm in favour of fixing the docs but I'm not keen on changing the
SQL syntax in a way that just kind of papers over part of the
problems.

I agree. I think that trying to design new SQL syntax at this point
is unlikely to be a good idea - we're just about out of time here, and
some people who might care about this are busy on other things, and
the deadline for patches that do new things has long since passed.
But I like the idea of trying to improve the documentation.

Agreed. I think the documentation fixes definitely should be done, but
understand that the grammar is a longer term issue with backward
compatibility implications. Acknowledging the problem is the first step ;-)

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

* Joe Conway (mail@joeconway.com) wrote:

On 03/16/2017 06:19 AM, Robert Haas wrote:

On Mon, Mar 13, 2017 at 4:48 AM, Craig Ringer <craig@2ndquadrant.com> wrote:

So I'm in favour of fixing the docs but I'm not keen on changing the
SQL syntax in a way that just kind of papers over part of the
problems.

I agree. I think that trying to design new SQL syntax at this point
is unlikely to be a good idea - we're just about out of time here, and
some people who might care about this are busy on other things, and
the deadline for patches that do new things has long since passed.
But I like the idea of trying to improve the documentation.

Agreed. I think the documentation fixes definitely should be done, but
understand that the grammar is a longer term issue with backward
compatibility implications. Acknowledging the problem is the first step ;-)

+1

Thanks!

Stephen

On Thu, Mar 16, 2017 at 07:27:11AM -0700, Joe Conway wrote:

On 03/16/2017 06:19 AM, Robert Haas wrote:

On Mon, Mar 13, 2017 at 4:48 AM, Craig Ringer <craig@2ndquadrant.com> wrote:

So I'm in favour of fixing the docs but I'm not keen on changing the
SQL syntax in a way that just kind of papers over part of the
problems.

I agree. I think that trying to design new SQL syntax at this point
is unlikely to be a good idea - we're just about out of time here, and
some people who might care about this are busy on other things, and
the deadline for patches that do new things has long since passed.
But I like the idea of trying to improve the documentation.

Seems like a good direction.

Agreed. I think the documentation fixes definitely should be done, but
understand that the grammar is a longer term issue with backward
compatibility implications. Acknowledging the problem is the first step ;-)

Most of the user-visible (including doc/) proposed changes alter material
predating v10. Therefore, I've removed this from open item status.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Is there any interest in fixing our documentation that says encrypted
when it means hashed? Should I pursue this?

---------------------------------------------------------------------------

On Fri, Mar 10, 2017 at 11:16:02AM +0900, Michael Paquier wrote:

Hi all,

As discussed here:
/messages/by-id/98cafcd0-5557-0bdf-4837-0f2b7782d0b5@joeconway.com
We are using in documentation and code comments "encryption" to define
what actually is hashing, which is confusing.

Attached is a patch for HEAD to change the documentation to match hashing.

There are a couple of things I noticed on the way:
1) There is the user-visible PQencryptPassword in libpq, which
actually hashes the password, and not encrypts it :)
2) There is as well pg_md5_encrypt() in the code, as well as there is
pg_md5_hash(). Those may be better if renamed, at least I would think
that pg_md5_encrypt should be pg_md5_password_hash, because a salt is
used as well, and the routine is dedicated to work on passwords.
Thoughts?
3) createuser also has --encrypt and --unencrypted, perhaps those
should be renamed? Honestly I don't really think that this is worth a
breakage and the option names match with the SQL commands.

I did not bother about those things in the attached, which works only
documentation and comment changes.

An open item has been added on the wiki.

Thanks,
--
Michael

diff --git a/contrib/pgcrypto/crypt-des.c b/contrib/pgcrypto/crypt-des.c
index a4aa4966bf..1f65f286ea 100644
--- a/contrib/pgcrypto/crypt-des.c
+++ b/contrib/pgcrypto/crypt-des.c
@@ -753,7 +753,7 @@ px_crypt_des(const char *key, const char *setting)
output[0] = setting[0];

/*
-		 * If the encrypted password that the salt was extracted from is only
+		 * If the hashed password that the salt was extracted from is only
* 1 character long, the salt will be corrupted.  We need to ensure
* that the output string doesn't have an extra NUL in it!
*/
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 28cdabe6fe..abbd5dd19e 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -1334,8 +1334,8 @@
<entry><structfield>rolpassword</structfield></entry>
<entry><type>text</type></entry>
<entry>
-       Password (possibly encrypted); null if none. The format depends
-       on the form of encryption used.
+       Password (possibly hashed); null if none. The format depends
+       on the form of hashing used.
</entry>
</row>

@@ -1350,19 +1350,20 @@
</table>

<para>
-   For an MD5 encrypted password, <structfield>rolpassword</structfield>
+   For an MD5-hashed password, <structfield>rolpassword</structfield>
column will begin with the string <literal>md5</> followed by a
32-character hexadecimal MD5 hash. The MD5 hash will be of the user's
password concatenated to their user name. For example, if user
<literal>joe</> has password <literal>xyzzy</>, <productname>PostgreSQL</>
will store the md5 hash of <literal>xyzzyjoe</>.  If the password is
-   encrypted with SCRAM-SHA-256, it consists of 5 fields separated by colons.
+   hashed with SCRAM-SHA-256, it consists of 5 fields separated by colons.
The first field is the constant <literal>scram-sha-256</literal>, to
identify the password as a SCRAM-SHA-256 verifier. The second field is a
salt, Base64-encoded, and the third field is the number of iterations used
to generate the password.  The fourth field and fifth field are the stored
key and server key, respectively, in hexadecimal format. A password that
-   does not follow either of those formats is assumed to be unencrypted.
+   does not follow either of those formats is assumed to be in plain format,
+   non-hashed.
</para>
</sect1>

@@ -10269,9 +10270,9 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
<entry><structfield>passwd</structfield></entry>
<entry><type>text</type></entry>
<entry></entry>
-      <entry>Password (possibly encrypted); null if none.  See
+      <entry>Password (possibly hashed); null if none.  See
<link linkend="catalog-pg-authid"><structname>pg_authid</structname></link>
-      for details of how encrypted passwords are stored.</entry>
+      for details of how hashed passwords are stored.</entry>
</row>

<row>
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 69844e5b29..994ed6c1bd 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1190,11 +1190,11 @@ include_dir 'conf.d'
When a password is specified in <xref linkend="sql-createuser"> or
<xref linkend="sql-alterrole"> without writing either <literal>ENCRYPTED</>
or <literal>UNENCRYPTED</>, this parameter determines whether the
-        password is to be encrypted. The default value is <literal>md5</>, which
+        password is to be hashed. The default value is <literal>md5</>, which
stores the password as an MD5 hash. Setting this to <literal>plain</> stores
it in plaintext. <literal>on</> and <literal>off</> are also accepted, as
aliases for <literal>md5</> and <literal>plain</>, respectively.  Setting
-        this parameter to <literal>scram</> will encrypt the password with
+        this parameter to <literal>scram</> will hash the password with
SCRAM-SHA-256.
</para>       
</listitem>
diff --git a/doc/src/sgml/passwordcheck.sgml b/doc/src/sgml/passwordcheck.sgml
index 6e6e4ef435..8cf59946eb 100644
--- a/doc/src/sgml/passwordcheck.sgml
+++ b/doc/src/sgml/passwordcheck.sgml
@@ -37,11 +37,11 @@

<caution>
<para>
-   To prevent unencrypted passwords from being sent across the network,
+   To prevent non-hashed passwords from being sent across the network,
written to the server log or otherwise stolen by a database administrator,
<productname>PostgreSQL</productname> allows the user to supply
-   pre-encrypted passwords. Many client programs make use of this
-   functionality and encrypt the password before sending it to the server.
+   pre-hashed passwords. Many client programs make use of this
+   functionality and hash the password before sending it to the server.
</para>
<para>
This limits the usefulness of the <filename>passwordcheck</filename>
@@ -54,7 +54,7 @@
</para>
<para>
Alternatively, you could modify <filename>passwordcheck</filename>
-   to reject pre-encrypted passwords, but forcing users to set their
+   to reject pre-hashed passwords, but forcing users to set their
passwords in clear text carries its own security risks.
</para>
</caution>
diff --git a/doc/src/sgml/pgcrypto.sgml b/doc/src/sgml/pgcrypto.sgml
index bf514aacf3..c4ce02c9dd 100644
--- a/doc/src/sgml/pgcrypto.sgml
+++ b/doc/src/sgml/pgcrypto.sgml
@@ -109,7 +109,7 @@ hmac(data bytea, key text, type text) returns bytea
<listitem>
<para>
They use a random value, called the <firstterm>salt</>, so that users
-     having the same password will have different encrypted passwords.
+     having the same password will have different hashed passwords.
This is also an additional defense against reversing the algorithm.
</para>
</listitem>
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 3d6e8eed43..52afe25090 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -294,7 +294,7 @@
<listitem>
<para>
The frontend must now send a PasswordMessage containing the
-        password (with user name) encrypted via MD5, then encrypted
+        password (with user name) hashed via MD5, then hashed
again using the 4-byte random salt specified in the
AuthenticationMD5Password message.  If this is the correct
password, the server responds with an AuthenticationOk,
@@ -2603,7 +2603,7 @@ AuthenticationMD5Password (B)
</term>
<listitem>
<para>
-                Specifies that an MD5-encrypted password is required.
+                Specifies that an MD5-hashed password is required.
</para>
</listitem>
</varlistentry>
@@ -2613,7 +2613,7 @@ AuthenticationMD5Password (B)
</term>
<listitem>
<para>
-                The salt to use when encrypting the password.
+                The salt to use when hashing the password.
</para>
</listitem>
</varlistentry>
@@ -4704,7 +4704,7 @@ PasswordMessage (F)
</term>
<listitem>
<para>
-                The password (encrypted, if requested).
+                The password (hashed, if requested).
</para>
</listitem>
</varlistentry>
diff --git a/doc/src/sgml/ref/alter_role.sgml b/doc/src/sgml/ref/alter_role.sgml
index da36ad9696..e242876967 100644
--- a/doc/src/sgml/ref/alter_role.sgml
+++ b/doc/src/sgml/ref/alter_role.sgml
@@ -81,9 +81,9 @@ ALTER ROLE { <replaceable class="PARAMETER">role_specification</replaceable> | A
roles.
The current session user cannot be renamed.
(Connect as a different user if you need to do that.)
-   Because <literal>MD5</>-encrypted passwords use the role name as
+   Because <literal>MD5</>-hashed passwords use the role name as
cryptographic salt, renaming a role clears its password if the
-   password is <literal>MD5</>-encrypted.
+   password is <literal>MD5</>-hashed.
</para>

<para>
@@ -250,7 +250,7 @@ ALTER ROLE { <replaceable class="PARAMETER">role_specification</replaceable> | A
</para>

<para>
-   Caution must be exercised when specifying an unencrypted password
+   Caution must be exercised when specifying a non-hashed password
with this command.  The password will be transmitted to the server
in cleartext, and it might also be logged in the client's command
history or the server log.  <xref linkend="app-psql">
diff --git a/doc/src/sgml/ref/create_role.sgml b/doc/src/sgml/ref/create_role.sgml
index 99d1c8336c..083fde6722 100644
--- a/doc/src/sgml/ref/create_role.sgml
+++ b/doc/src/sgml/ref/create_role.sgml
@@ -228,14 +228,14 @@ CREATE ROLE <replaceable class="PARAMETER">name</replaceable> [ [ WITH ] <replac
<listitem>
<para>
These key words control whether the password is stored
-        encrypted in the system catalogs.  (If neither is specified,
+        hashed in the system catalogs.  (If neither is specified,
the default behavior is determined by the configuration
parameter <xref linkend="guc-password-encryption">.)  If the
-        presented password string is already in MD5-encrypted or
-        SCRAM-encrypted format, then it is stored encrypted as-is,
+        presented password string is already in MD5-hashed or
+        SCRAM-hashed format, then it is stored hased as-is,
regardless of whether <literal>ENCRYPTED</> or <literal>UNENCRYPTED</>
-        is specified (since the system cannot decrypt the specified encrypted
-        password string).  This allows reloading of encrypted passwords
+        is specified (since the system cannot understand the specified hashed
+        password string).  This allows reloading of hashed passwords
during dump/restore.
</para>

@@ -396,12 +396,12 @@ CREATE ROLE <replaceable class="PARAMETER">name</replaceable> [ [ WITH ] <replac
</para>

<para>
-   Caution must be exercised when specifying an unencrypted password
+   Caution must be exercised when specifying a non-hashed password
with this command.  The password will be transmitted to the server
in cleartext, and it might also be logged in the client's command
history or the server log.  The command <xref
linkend="APP-CREATEUSER">, however, transmits
-   the password encrypted.  Also, <xref linkend="app-psql">
+   the password hash.  Also, <xref linkend="app-psql">
contains a command
<command>\password</command> that can be used to safely change the
password later.
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 2a9c412020..0860b483a7 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2207,7 +2207,7 @@ lo_import 152801
<listitem>
<para>
Changes the password of the specified user (by default, the current
-        user).  This command prompts for the new password, encrypts it, and
+        user).  This command prompts for the new password, hashes it, and
sends it to the server as an <command>ALTER ROLE</> command.  This
makes sure that the new password does not appear in cleartext in the
command history, the server log, or elsewhere.
diff --git a/doc/src/sgml/runtime.sgml b/doc/src/sgml/runtime.sgml
index 5e3d783c6a..5f6b30f8df 100644
--- a/doc/src/sgml/runtime.sgml
+++ b/doc/src/sgml/runtime.sgml
@@ -2024,15 +2024,15 @@ pg_dumpall -p 5432 | psql -d postgres -p 5433
<variablelist>

<varlistentry>
-   <term>Password Storage Encryption</term>
+   <term>Password Storage Hashing</term>
<listitem>

<para>
By default, database user passwords are stored as MD5 hashes, so
the administrator cannot determine the actual password assigned
-     to the user. If MD5 encryption is used for client authentication,
-     the unencrypted password is never even temporarily present on the
-     server because the client MD5-encrypts it before being sent
+     to the user. If MD5 hashing is used for client authentication,
+     the non-hashed password is never even temporarily present on the
+     server because the client MD5-hashes it before being sent
across the network.
</para>
</listitem>
@@ -2088,18 +2088,18 @@ pg_dumpall -p 5432 | psql -d postgres -p 5433
</varlistentry>

<varlistentry>
-   <term>Encrypting Passwords Across A Network</term>
+   <term>Hashing Passwords Across A Network</term>

<listitem>
<para>
-      The <literal>MD5</> authentication method double-encrypts the
+      The <literal>MD5</> authentication method double-hashes the
password on the client before sending it to the server. It first
-      MD5-encrypts it based on the user name, and then encrypts it
+      MD5-hashes it based on the user name, and then hashes it
based on a random salt sent by the server when the database
-      connection was made. It is this double-encrypted value that is
-      sent over the network to the server. Double-encryption not only
+      connection was made. It is this double-hashed value that is
+      sent over the network to the server. Double-hash not only
prevents the password from being discovered, it also prevents
-      another connection from using the same encrypted password to
+      another connection from using the same hashed password to
connect to the database server at a later time.
</para>
</listitem>
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 14b9779144..e4ad531fa6 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -397,7 +397,7 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)

if (password)
{
-		/* Encrypt the password to the requested format. */
+		/* Hash the password to the requested format. */
char	   *shadow_pass;

shadow_pass = encrypt_password(password_type, stmt->role, password);
@@ -806,7 +806,7 @@ AlterRole(AlterRoleStmt *stmt)
/* password */
if (password)
{
-		/* Encrypt the password to the requested format. */
+		/* Hash the password to the requested format. */
char	   *shadow_pass;

shadow_pass = encrypt_password(password_type, rolename, password);
diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index 9f78e57aae..8e7344d95a 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -147,7 +147,7 @@ pg_be_scram_init(const char *username, const char *shadow_pass, bool doomed)
/*
* Perform sanity checks on the provided password after catalog lookup.
* The authentication is bound to fail if the lookup itself failed or if
-	 * the password stored is MD5-encrypted.  Authentication is possible for
+	 * the password stored is MD5-hashed.  Authentication is possible for
* users with a valid plain password though.
*/

diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index ebf10bbbae..61217f0fa4 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -2723,7 +2723,7 @@ CheckRADIUSAuth(Port *port)
if (!pg_md5_binary(cryptvector, strlen(port->hba->radiussecret) + RADIUS_VECTOR_LENGTH, encryptedpassword + i))
{
ereport(LOG,
-					(errmsg("could not perform MD5 encryption of password")));
+					(errmsg("could not perform MD5 hash of password")));
pfree(cryptvector);
return STATUS_ERROR;
}
@@ -2924,7 +2924,7 @@ CheckRADIUSAuth(Port *port)
encryptedpassword))
{
ereport(LOG,
-			(errmsg("could not perform MD5 encryption of received packet")));
+			(errmsg("could not perform MD5 hash of received packet")));
pfree(cryptvector);
continue;
}
diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c
index bd3e936d38..2cc3344f6e 100644
--- a/src/backend/libpq/crypt.c
+++ b/src/backend/libpq/crypt.c
@@ -1,7 +1,7 @@
/*-------------------------------------------------------------------------
*
* crypt.c
- *	  Functions for dealing with encrypted passwords stored in
+ *	  Functions for dealing with hashed passwords stored in
*	  pg_authid.rolpassword.
*
* Portions Copyright (c) 1996-2017, PostgreSQL Global Development Group
@@ -150,7 +150,7 @@ encrypt_password(PasswordType target_type, const char *role,

if (!pg_md5_encrypt(password, role, strlen(role),
encrypted_password))
-						elog(ERROR, "password encryption failed");
+						elog(ERROR, "password hashing failed");
return encrypted_password;

case PASSWORD_TYPE_SCRAM:
@@ -184,7 +184,7 @@ encrypt_password(PasswordType target_type, const char *role,
* This shouldn't happen, because the above switch statements should
* handle every combination of source and target password types.
*/
-	elog(ERROR, "cannot encrypt password to requested type");
+	elog(ERROR, "cannot hash password to requested type");
return NULL;				/* keep compiler quiet */
}

@@ -221,7 +221,7 @@ md5_crypt_verify(const char *role, const char *shadow_pass,
switch (get_password_type(shadow_pass))
{
case PASSWORD_TYPE_MD5:
-			/* stored password already encrypted, only do salt */
+			/* stored password already hashed, only do salt */
if (!pg_md5_encrypt(shadow_pass + strlen("md5"),
md5_salt, md5_salt_len,
crypt_pwd))
@@ -231,7 +231,7 @@ md5_crypt_verify(const char *role, const char *shadow_pass,
break;

case PASSWORD_TYPE_PLAINTEXT:
-			/* stored password is plain, double-encrypt */
+			/* stored password is plain, double-hash */
if (!pg_md5_encrypt(shadow_pass,
role,
strlen(role),
diff --git a/src/backend/libpq/pg_hba.conf.sample b/src/backend/libpq/pg_hba.conf.sample
index 6b1778a721..6b4ab71cb6 100644
--- a/src/backend/libpq/pg_hba.conf.sample
+++ b/src/backend/libpq/pg_hba.conf.sample
@@ -45,7 +45,7 @@
# METHOD can be "trust", "reject", "md5", "password", "scram", "gss",
# "sspi", "ident", "peer", "pam", "ldap", "radius" or "cert".  Note that
# "password" sends passwords in clear text; "md5" or "scram" are preferred
-# since they send encrypted passwords.
+# since they send hashed passwords.
#
# OPTIONS are a set of options for the authentication in the format
# NAME=VALUE.  The available options depend on the different
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 07efc27a69..0dbe7017a8 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -1138,7 +1138,7 @@ exec_command(const char *cmd,

if (!encrypted_password)
{
-				psql_error("Password encryption failed.\n");
+				psql_error("Password hashing failed.\n");
success = false;
}
else
diff --git a/src/bin/scripts/createuser.c b/src/bin/scripts/createuser.c
index 3d74797a8f..b398f6a1e1 100644
--- a/src/bin/scripts/createuser.c
+++ b/src/bin/scripts/createuser.c
@@ -278,7 +278,7 @@ main(int argc, char *argv[])
newuser);
if (!encrypted_password)
{
-				fprintf(stderr, _("Password encryption failed.\n"));
+				fprintf(stderr, _("Password hashing failed.\n"));
exit(1);
}
appendStringLiteralConn(&sql, encrypted_password, conn);
@@ -358,14 +358,14 @@ help(const char *progname)
printf(_("  -d, --createdb            role can create new databases\n"));
printf(_("  -D, --no-createdb         role cannot create databases (default)\n"));
printf(_("  -e, --echo                show the commands being sent to the server\n"));
-	printf(_("  -E, --encrypted           encrypt stored password\n"));
+	printf(_("  -E, --encrypted           hash stored password\n"));
printf(_("  -g, --role=ROLE           new role will be a member of this role\n"));
printf(_("  -i, --inherit             role inherits privileges of roles it is a\n"
"                            member of (default)\n"));
printf(_("  -I, --no-inherit          role does not inherit privileges\n"));
printf(_("  -l, --login               role can login (default)\n"));
printf(_("  -L, --no-login            role cannot login\n"));
-	printf(_("  -N, --unencrypted         do not encrypt stored password\n"));
+	printf(_("  -N, --unencrypted         do not hash stored password\n"));
printf(_("  -P, --pwprompt            assign a password to new role\n"));
printf(_("  -r, --createrole          role can create new roles\n"));
printf(_("  -R, --no-createrole       role cannot create roles (default)\n"));
diff --git a/src/common/scram-common.c b/src/common/scram-common.c
index e44f38f652..712d51900d 100644
--- a/src/common/scram-common.c
+++ b/src/common/scram-common.c
@@ -148,7 +148,7 @@ scram_H(const uint8 *input, int len, uint8 *result)
}

/*
- * Encrypt password for SCRAM authentication. This basically applies the
+ * Hash password for SCRAM authentication. This basically applies the
* normalization of the password and a hash calculation using the salt
* value given by caller.
*/
diff --git a/src/include/common/md5.h b/src/include/common/md5.h
index ccaaeddbf4..37e2432021 100644
--- a/src/include/common/md5.h
+++ b/src/include/common/md5.h
@@ -4,7 +4,7 @@
*	  Interface to libpq/md5.c
*
* These definitions are needed by both frontend and backend code to work
- * with MD5-encrypted passwords.
+ * with MD5-hashed passwords.
*
* Portions Copyright (c) 1996-2017, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index 5fe7e565a0..385c0472bf 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -583,7 +583,7 @@ pg_password_sendauth(PGconn *conn, const char *password, AuthRequest areq)
char	   *crypt_pwd = NULL;
const char *pwd_to_send;

-	/* Encrypt the password if needed. */
+	/* Hash the password if needed. */

switch (areq)
{
@@ -909,14 +909,14 @@ pg_fe_getauthname(PQExpBuffer errorMessage)

/*
- * PQencryptPassword -- exported routine to encrypt a password
+ * PQencryptPassword -- exported routine to hash a password
*
* This is intended to be used by client applications that wish to send
* commands like ALTER USER joe PASSWORD 'pwd'.  The password need not
- * be sent in cleartext if it is encrypted on the client side.  This is
+ * be sent in cleartext if it is hashed on the client side.  This is
* good because it ensures the cleartext password won't end up in logs,
* pg_stat displays, etc.  We export the function so that clients won't
- * be dependent on low-level details like whether the encryption is MD5
+ * be dependent on low-level details like whether the hashing is MD5
* or something else.
*
* Arguments are the cleartext password, and the SQL name of the user it
diff --git a/src/test/regress/expected/password.out b/src/test/regress/expected/password.out
index c503e43abe..7fe09e6f00 100644
--- a/src/test/regress/expected/password.out
+++ b/src/test/regress/expected/password.out
@@ -55,14 +55,14 @@ SELECT rolname, rolpassword

ALTER ROLE regress_passwd3_new RENAME TO regress_passwd3;
-- ENCRYPTED and UNENCRYPTED passwords
-ALTER ROLE regress_passwd1 UNENCRYPTED PASSWORD 'foo'; -- unencrypted
-ALTER ROLE regress_passwd2 UNENCRYPTED PASSWORD 'md5dfa155cadd5f4ad57860162f3fab9cdb'; -- encrypted with MD5
+ALTER ROLE regress_passwd1 UNENCRYPTED PASSWORD 'foo'; -- non-hashed
+ALTER ROLE regress_passwd2 UNENCRYPTED PASSWORD 'md5dfa155cadd5f4ad57860162f3fab9cdb'; -- hashed with MD5
SET password_encryption = 'md5';
-ALTER ROLE regress_passwd3 ENCRYPTED PASSWORD 'foo'; -- encrypted with MD5
+ALTER ROLE regress_passwd3 ENCRYPTED PASSWORD 'foo'; -- hashed with MD5
ALTER ROLE regress_passwd4 ENCRYPTED PASSWORD 'scram-sha-256:VLK4RMaQLCvNtQ==:4096:3ded2376f7aafa93b1bdbd71bcc18b7d6ee50ed018029cc583d152ef3fc7d430:a6dd36dfc94c181956a6ae95f05e01b1864f0a22a2657d1de4ba84d2a24dc438'; -- client-supplied SCRAM verifier, use as it is
SET password_encryption = 'scram';
ALTER ROLE  regress_passwd5 ENCRYPTED PASSWORD 'foo'; -- create SCRAM verifier
-CREATE ROLE regress_passwd6 ENCRYPTED PASSWORD 'md53725413363ab045e20521bf36b8d8d7f'; -- encrypted with MD5, use as it is
+CREATE ROLE regress_passwd6 ENCRYPTED PASSWORD 'md53725413363ab045e20521bf36b8d8d7f'; -- hashed with MD5, use as it is
SELECT rolname, regexp_replace(rolpassword, '(scram-sha-256):([a-zA-Z0-9+/]+==):(\d+):(\w+):(\w+)', '\1:<salt>:\3:<storedkey>:<serverkey>') as rolpassword_masked
FROM pg_authid
WHERE rolname LIKE 'regress_passwd%'
diff --git a/src/test/regress/sql/password.sql b/src/test/regress/sql/password.sql
index f4b3a9ac3a..aa88d5ef82 100644
--- a/src/test/regress/sql/password.sql
+++ b/src/test/regress/sql/password.sql
@@ -43,16 +43,16 @@ SELECT rolname, rolpassword
ALTER ROLE regress_passwd3_new RENAME TO regress_passwd3;

-- ENCRYPTED and UNENCRYPTED passwords
-ALTER ROLE regress_passwd1 UNENCRYPTED PASSWORD 'foo'; -- unencrypted
-ALTER ROLE regress_passwd2 UNENCRYPTED PASSWORD 'md5dfa155cadd5f4ad57860162f3fab9cdb'; -- encrypted with MD5
+ALTER ROLE regress_passwd1 UNENCRYPTED PASSWORD 'foo'; -- non-hashed
+ALTER ROLE regress_passwd2 UNENCRYPTED PASSWORD 'md5dfa155cadd5f4ad57860162f3fab9cdb'; -- hashed with MD5
SET password_encryption = 'md5';
-ALTER ROLE regress_passwd3 ENCRYPTED PASSWORD 'foo'; -- encrypted with MD5
+ALTER ROLE regress_passwd3 ENCRYPTED PASSWORD 'foo'; -- hashed with MD5

ALTER ROLE regress_passwd4 ENCRYPTED PASSWORD 'scram-sha-256:VLK4RMaQLCvNtQ==:4096:3ded2376f7aafa93b1bdbd71bcc18b7d6ee50ed018029cc583d152ef3fc7d430:a6dd36dfc94c181956a6ae95f05e01b1864f0a22a2657d1de4ba84d2a24dc438'; -- client-supplied SCRAM verifier, use as it is

SET password_encryption = 'scram';
ALTER ROLE  regress_passwd5 ENCRYPTED PASSWORD 'foo'; -- create SCRAM verifier
-CREATE ROLE regress_passwd6 ENCRYPTED PASSWORD 'md53725413363ab045e20521bf36b8d8d7f'; -- encrypted with MD5, use as it is
+CREATE ROLE regress_passwd6 ENCRYPTED PASSWORD 'md53725413363ab045e20521bf36b8d8d7f'; -- hashed with MD5, use as it is

SELECT rolname, regexp_replace(rolpassword, '(scram-sha-256):([a-zA-Z0-9+/]+==):(\d+):(\w+):(\w+)', '\1:<salt>:\3:<storedkey>:<serverkey>') as rolpassword_masked
FROM pg_authid

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

Only you can decide what is important to you.

On 2023-11-21 22:43:48 -0500, Bruce Momjian wrote:

Is there any interest in fixing our documentation that says encrypted
when it means hashed? Should I pursue this?

What's the point of randomly reviving threads from 6 years ago, without any
further analysis?

On Wed, Nov 22, 2023 at 12:52:23PM -0800, Andres Freund wrote:

On 2023-11-21 22:43:48 -0500, Bruce Momjian wrote:

Is there any interest in fixing our documentation that says encrypted
when it means hashed? Should I pursue this?

What's the point of randomly reviving threads from 6 years ago, without any
further analysis?

Well, I feel like this is an imporant change, and got dropped because it
was determined to not be a new change in Postgres 10. Still I think it
should be addressed and am looking to see if others feel the same.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

Only you can decide what is important to you.

Bruce Momjian <bruce@momjian.us> writes:

On Wed, Nov 22, 2023 at 12:52:23PM -0800, Andres Freund wrote:

What's the point of randomly reviving threads from 6 years ago, without any
further analysis?

Well, I feel like this is an imporant change, and got dropped because it
was determined to not be a new change in Postgres 10. Still I think it
should be addressed and am looking to see if others feel the same.

You're expecting people to re-research something that dropped off the
radar years ago, probably because it wasn't sufficiently interesting
at the time. If the point hasn't been raised again since then,
I'd guess it's not any more interesting now. Most of us have better
things to do than go re-read old threads --- and dredging them up
ten at a time is not improving the odds that people will care to look.

regards, tom lane

On Wed, Nov 22, 2023 at 05:55:06PM -0500, Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

On Wed, Nov 22, 2023 at 12:52:23PM -0800, Andres Freund wrote:

What's the point of randomly reviving threads from 6 years ago, without any
further analysis?

Well, I feel like this is an imporant change, and got dropped because it
was determined to not be a new change in Postgres 10. Still I think it
should be addressed and am looking to see if others feel the same.

You're expecting people to re-research something that dropped off the
radar years ago, probably because it wasn't sufficiently interesting
at the time. If the point hasn't been raised again since then,
I'd guess it's not any more interesting now. Most of us have better
things to do than go re-read old threads --- and dredging them up
ten at a time is not improving the odds that people will care to look.

I can do the work. I was more looking for any feedback that this is a
valid area to work on. I will do some work on this and publish an
updated patch because I think using the right terms makes sense.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

Only you can decide what is important to you.

On Wed, Nov 22, 2023 at 07:01:32PM -0500, Bruce Momjian wrote:

On Wed, Nov 22, 2023 at 05:55:06PM -0500, Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

On Wed, Nov 22, 2023 at 12:52:23PM -0800, Andres Freund wrote:

What's the point of randomly reviving threads from 6 years ago, without any
further analysis?

Well, I feel like this is an imporant change, and got dropped because it
was determined to not be a new change in Postgres 10. Still I think it
should be addressed and am looking to see if others feel the same.

You're expecting people to re-research something that dropped off the
radar years ago, probably because it wasn't sufficiently interesting
at the time. If the point hasn't been raised again since then,
I'd guess it's not any more interesting now. Most of us have better
things to do than go re-read old threads --- and dredging them up
ten at a time is not improving the odds that people will care to look.

I can do the work. I was more looking for any feedback that this is a
valid area to work on. I will do some work on this and publish an
updated patch because I think using the right terms makes sense.

Let me also add that I apologize for brining up these old threads. I
feel badly I didn't address them years ago, I feel bad for the original
posters, and I do think there is some value in addressing some of them,
which I think is validated by the many useful doc patches I have made
over the past few weeks. I am over half done and hope everyone can be
patient with me while I do my best to finish this long-overdue job.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

Only you can decide what is important to you.

On Wed, Nov 22, 2023 at 08:23:42PM -0500, Bruce Momjian wrote:

Let me also add that I apologize for brining up these old threads. I
feel badly I didn't address them years ago, I feel bad for the original
posters, and I do think there is some value in addressing some of them,
which I think is validated by the many useful doc patches I have made
over the past few weeks. I am over half done and hope everyone can be
patient with me while I do my best to finish this long-overdue job.

I just finished reviewing 6k emails I kept for re-review back to 2014. I
only have the open items left to close. I am not sure why stopped
regularly reviewing such emails. Hopefully I will be better going
forward.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

Only you can decide what is important to you.

Greetings,

* Bruce Momjian (bruce@momjian.us) wrote:

Is there any interest in fixing our documentation that says encrypted
when it means hashed? Should I pursue this?

I do think we should use the correct terminology in our documentation
and would support your working on improving things in this area.

I do wonder if perhaps we would be better off by having someone spend
time on removing terribly insecure authentication methods like md5 and
ldap though ...

Thanks,

Stephen

On Tue, Nov 28, 2023 at 9:55 AM Stephen Frost <sfrost@snowman.net> wrote:

I do think we should use the correct terminology in our documentation
and would support your working on improving things in this area.

+1.

I do wonder if perhaps we would be better off by having someone spend
time on removing terribly insecure authentication methods like md5 and
ldap though ...

Wait, what's insecure about LDAP?

I think we should eventually remove MD5, but I think there's no rush.
People who care about security will have already switched, and people
who don't care about security are not required to start caring.
Eventually the maintenance burden will become large enough that it
makes sense to phase it out for that reason, but I haven't seen any
evidence that we're anywhere close to that point.

--
Robert Haas
EDB: http://www.enterprisedb.com

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:

On Tue, Nov 28, 2023 at 9:55 AM Stephen Frost <sfrost@snowman.net> wrote:

I do think we should use the correct terminology in our documentation
and would support your working on improving things in this area.

+1.

I do wonder if perhaps we would be better off by having someone spend
time on removing terribly insecure authentication methods like md5 and
ldap though ...

Wait, what's insecure about LDAP?

We pass a completely cleartext password to the server from the client.
Yes, we might encrypt it on the way with TLS, but even SSH realized how
terrible that is long, long ago and strongly discourages it these days.

The problem with ldap as an auth method is that a single compromised PG
server in an AD/ldap environment can collect up those username+password
credentials and gain access to those users *domain* level access. The
CFO logging into a PG server with LDAP auth is giving up their complete
access credentials to the entire AD domain. That's terrible.

I think we should eventually remove MD5, but I think there's no rush.

I disagree- it's a known pass-the-hash vulnerability and frankly every
release we do with it still existing is deserving of an immediate CVE
(I've been asked off-list why we don't do this, in fact).

People who care about security will have already switched, and people
who don't care about security are not required to start caring.

I wish it were this simple. It's just not though.

Eventually the maintenance burden will become large enough that it
makes sense to phase it out for that reason, but I haven't seen any
evidence that we're anywhere close to that point.

This seems to invite the idea that what people who care about this need
to do is make it painful for us to continue to keep it around, which I
really don't think is best for anyone. We know it's bad, we know it is
broken, we need to remove it, not pretend like it's not broken or not
bad.

Thanks,

Stephen

On Tue, Nov 28, 2023 at 10:16 AM Stephen Frost <sfrost@snowman.net> wrote:

We pass a completely cleartext password to the server from the client.
Yes, we might encrypt it on the way with TLS, but even SSH realized how
terrible that is long, long ago and strongly discourages it these days.

The problem with ldap as an auth method is that a single compromised PG
server in an AD/ldap environment can collect up those username+password
credentials and gain access to those users *domain* level access. The
CFO logging into a PG server with LDAP auth is giving up their complete
access credentials to the entire AD domain. That's terrible.

Good grief. I really think you need to reassess your approach to this
whole area. It is massively unhelpful to be so prescriptive about what
people are, or are not, allowed to be comfortable with from a security
perspective. And it seems like you're so convinced that you're right
that you're completely closed off to anyone else's input, so there's
not even any point in arguing.

In the reality I inhabit, many people could *massively* improve their
security by switching to LDAP from the really lame things that they're
doing right now. They would be FAR better off. It would not even be
close. I pretty much know that you aren't going to believe that and
are going to offer reasons why it isn't and can't possibly be true, or
else just say that those people are so hopeless that we shouldn't even
care about them. All I can say is that you're worrying about security
problems that are so minute as to be invisible compared to what I see.

I disagree- it's a known pass-the-hash vulnerability and frankly every
release we do with it still existing is deserving of an immediate CVE
(I've been asked off-list why we don't do this, in fact).

I agree that the issues with MD5 are way worse than the ones with
LDAP, but you're arguing here, as you did with exclusive backup mode,
that the existence of options that are bad, or that you consider to be
bad, is a problem even if nothing whatever compels people to use them.
I think that is, to borrow a phrase from Tom, arrant nonsense. Sure,
MD5 authentication has a pass-the-hash vulnerability, and that sucks.
So let's say we remove it. Then presumably we should also remove
password authentication, which has an even easier-to-exploit
vulnerability, and trust, which is easier to exploit still. And
presumably it would be right to do all of that even if SCRAM
authentication had never been implemented, so then we'd have no
support for any form of password authentication at all. And we'd
remove LDAP, too, so the most obvious way of doing password
authentication without putting the passwords in the database would
also not exist any more. Maybe we should nuke a few more too --- is
PAM any better than LDAP in this regard? Pretty soon you end up in a
world where it's either impossible to log into the database at all, or
everybody's got to use, I don't know, Kerberos and SSL certificates,
or something.

But what will happen in practice is that either users will just give
up on PostgreSQL because it refuses to let them do what they want to
do, or those SSL certificates will end up unencrypted in a
world-writable file somewhere, or get emailed around to everyone in
the group, or maybe checked into the application's git repository that
is public to the whole company behind the firewall. There won't be a
passphrase on the certificate, or it will be set to 'secret123', or
whatever. The expiration date will be set to the furthest date in the
future that the software supports, or the user won't realize they need
to worry about that and the application will suddenly go down when the
certificate expires.

You can't make people do the right thing by removing their options.
You can only make them work a little harder to get around the security
control that you tried to impose by fiat, and in the process, make
them frustrated. I imagine that the reason you didn't suggest removing
'trust' is that you admit the existence of environments where it's
safe enough -- someone working with a local PG on a loopback port on a
single-user machine, maybe a connection from a certain IP where that
IP and the DB are both behind a firewall, or a burner system that
someone just literally doesn't care about. But the "safe enough"
argument applies to everything else, too. It's the job of users to
adequately secure their systems against external attack, and it's our
job to help them, not to enforce our own views on them. It is
absolutely fine to tell users, in a thoughtful and balanced way, what
the advantages and disadvantages of different choices are. But
removing their choices is high-handed and ineffective.

--
Robert Haas
EDB: http://www.enterprisedb.com

Greetings,

On Tue, Nov 28, 2023 at 20:19 Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, Nov 28, 2023 at 10:16 AM Stephen Frost <sfrost@snowman.net> wrote:

We pass a completely cleartext password to the server from the client.
Yes, we might encrypt it on the way with TLS, but even SSH realized how
terrible that is long, long ago and strongly discourages it these days.

The problem with ldap as an auth method is that a single compromised PG
server in an AD/ldap environment can collect up those username+password
credentials and gain access to those users *domain* level access. The
CFO logging into a PG server with LDAP auth is giving up their complete
access credentials to the entire AD domain. That's terrible.

Good grief. I really think you need to reassess your approach to this
whole area. It is massively unhelpful to be so prescriptive about what
people are, or are not, allowed to be comfortable with from a security
perspective. And it seems like you're so convinced that you're right
that you're completely closed off to anyone else's input, so there's
not even any point in arguing.

You asked what the issue is with our ldap auth method. I explained it
pretty clearly. Is there something incorrect about the issues I’ve pointed
out with ldap auth? You basically came back at me saying that I’m being
unhelpful for responding with what the issues are.

In the reality I inhabit, many people could *massively* improve their

security by switching to LDAP from the really lame things that they're
doing right now. They would be FAR better off. It would not even be
close. I pretty much know that you aren't going to believe that and
are going to offer reasons why it isn't and can't possibly be true, or
else just say that those people are so hopeless that we shouldn't even
care about them. All I can say is that you're worrying about security
problems that are so minute as to be invisible compared to what I see.

I don’t know what they’re doing now, as you don’t say, and so I really
couldn’t say if ldap is better or worse for them. In some cases, sure,
perhaps ldap is better than … something else, but in 99% of cases, ldap is
being used because it seems simpler than setting up GSSAPI … but that’s a
lack of clearer documentation on our side on how to set up GSSAPI with AD
and PG, imv. I’ve tried to improve on that situation by explaining
clearly how to set up GSSAPI authentication with AD and then consistently
pointing people to that. I don’t put this out there as fiat without any
effort behind it, I’ve been working to move folks in the right direction,
I’ve put in that effort on the lists and continue to do so, feel free to
review the archives.

What I don’t understand is the argument for using ldap in an AD environment
instead of GSSAPI/sspi. Is there some reason you feel makes ldap better?
When there are two such options and one is clearly much more secure,
shouldn’t we push people to the more secure option… possibly even to the
exclusion and removal of the insecure option? Ditto below for md5 …

I disagree- it's a known pass-the-hash vulnerability and frankly every

release we do with it still existing is deserving of an immediate CVE
(I've been asked off-list why we don't do this, in fact).

I agree that the issues with MD5 are way worse than the ones with
LDAP, but you're arguing here, as you did with exclusive backup mode,
that the existence of options that are bad, or that you consider to be
bad, is a problem even if nothing whatever compels people to use them.

I’m not impressed with the conflation of this discussion and that of
exclusive backup mode. I wasn’t the one who deprecated exclusive backup
mode, tho I did agree with that move, and the reason for removing it was as
much about the complications of documenting it as for the fact that it was
actively broken.

I think that is, to borrow a phrase from Tom, arrant nonsense. Sure,

MD5 authentication has a pass-the-hash vulnerability, and that sucks.

So, given that we all agree with the CVE-worthy issue that exists with
every release where we include md5 auth, we should be applying for q CVE
prior to each release, no?

So let's say we remove it. Then presumably we should also remove

password authentication, which has an even easier-to-exploit
vulnerability, and trust, which is easier to exploit still. And
presumably it would be right to do all of that even if SCRAM
authentication had never been implemented, so then we'd have no
support for any form of password authentication at all. And we'd
remove LDAP, too, so the most obvious way of doing password
authentication without putting the passwords in the database would
also not exist any more. Maybe we should nuke a few more too --- is
PAM any better than LDAP in this regard? Pretty soon you end up in a
world where it's either impossible to log into the database at all, or
everybody's got to use, I don't know, Kerberos and SSL certificates,
or something.

This is jumping to places that just don’t exist and I don’t really
understand why.

I didn’t ask for md5 to be removed before we had SCRAM. I was here, I knew
about the issues with md5, but, no, I didn’t push for us to remove it, so
this argument that I’m being ridiculous to ask for it to be removed now
doesn’t hold any water.

We do have a much better solution now and therefore, after a number of
years of supporting both, it’s time to remove the CVE-worthy auth method.
Yes, password and PAM and trust have issues. We should consider removing
them. Why didn’t I list them? Because they don’t have alternatives
immediately available (scram in place of md5, GSSAPI in place of ldap). We
could make an argument for removing password given that we have scram and I
would be for that but it’s at least a bit better understood why it’s bad
out there- I don’t feel like folks generally realize the md5 issue that we
have because that was deprecated and removed or not implemented for most
other systems long, long ago. For trust … I’d be a lot happier if we only
enabled that in debug-enabled builds or when explicitly requested for a
build.

But what will happen in practice is that either users will just give

up on PostgreSQL because it refuses to let them do what they want to
do, or those SSL certificates will end up unencrypted in a
world-writable file somewhere, or get emailed around to everyone in
the group, or maybe checked into the application's git repository that
is public to the whole company behind the firewall. There won't be a
passphrase on the certificate, or it will be set to 'secret123', or
whatever. The expiration date will be set to the furthest date in the
future that the software supports, or the user won't realize they need
to worry about that and the application will suddenly go down when the
certificate expires.

When the goal is “authenticate to our AD environment”, asking users to go
through a different path, eg: GSSAPI, to get that has been generally
successful in my experience, both on our lists and with our direct clients.
So, no, in my experience it isn’t that people give up on PG or switch to
using client side certs that are just distributed around with weak
passwords. More frequently users are very happy to go with the better
solution and they just didn’t realize it was available to them.

I don’t mean to imply that it’s perfect though- there are clear things we
could do to improve the situation and if folks want to discuss that, I’m
all about it. I’m happy to even be told that we need xyz support before we
remove ldap as an auth method, that’s fine, I’ll work to make that happen,
but to claim that we should keep ldap support for ldap support’s usage in
an AD environment … sorry, but no. Even Microsoft tried at one point to
prevent people from using it that way- they had to back off that position
eventually in terms of the default, but good AD admins are aware of the
distinction and will push back against new systems being deployed with it.

You can't make people do the right thing by removing their options.

You can only make them work a little harder to get around the security
control that you tried to impose by fiat, and in the process, make
them frustrated. I imagine that the reason you didn't suggest removing
'trust' is that you admit the existence of environments where it's
safe enough -- someone working with a local PG on a loopback port on a
single-user machine, maybe a connection from a certain IP where that
IP and the DB are both behind a firewall, or a burner system that
someone just literally doesn't care about. But the "safe enough"
argument applies to everything else, too. It's the job of users to
adequately secure their systems against external attack, and it's our
job to help them, not to enforce our own views on them. It is
absolutely fine to tell users, in a thoughtful and balanced way, what
the advantages and disadvantages of different choices are. But
removing their choices is high-handed and ineffective.

I didn’t suggest removing trust directly because there isn’t a directly
comparable already available better alternative. I do think it should be a
bit harder for folks to use because too often they don’t realize what
they’re doing when they enable it, but I appreciate that’s a different
issue.

To parallel it to a recent change- I view it like force_parallel_mode. It
was useful for debugging and testing and perhaps still is, but we should
make it clear that it’s really intended just for that.

If asking users to work just a bit harder means that we get a good chunk of
them over to GSSAPI and off of ldap … then that is a moving us into a
clearly better position.

I’ll even accept, as others have pushed at me, that maybe we should keep
ldap because some folks still use PG with just openldap (and no Kerberos
environment), but it’s been an awful long time since anyone has even tried
to broach that argument and that’s not what is going on here, afaict anyway.

(Replying from my phone and perhaps not as concise or clear as I could be,
sorry about that, but am traveling and otherwise there would be quite an
additional delay..)

Thanks!

Stephen

On Tue, Nov 28, 2023 at 12:24 PM Stephen Frost <sfrost@snowman.net> wrote:

I don’t know what they’re doing now, as you don’t say, and so I really couldn’t say if ldap is better or worse for them. In some cases, sure, perhaps ldap is better than … something else,

That's EXACTLY right. You can't say whether LDAP is better or worse in
every scenario. And therefore you should not be proposing to remove
it.

I think that is, to borrow a phrase from Tom, arrant nonsense. Sure,
MD5 authentication has a pass-the-hash vulnerability, and that sucks.

So, given that we all agree with the CVE-worthy issue that exists with every release where we include md5 auth, we should be applying for q CVE prior to each release, no?

You know, I said in my previous email that you were so sure that you
were right that there was no point in trying to have a serious
discussion, and I can't really see how you could have proved that
point more thoroughly than you did here. You twisted my words around
to make it seem like I was agreeing with your point when you know full
well that I was doing the exact opposite of that.

Please don't do that.

--
Robert Haas
EDB: http://www.enterprisedb.com

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:

On Tue, Nov 28, 2023 at 12:24 PM Stephen Frost <sfrost@snowman.net> wrote:

I don’t know what they’re doing now, as you don’t say, and so I really couldn’t say if ldap is better or worse for them. In some cases, sure, perhaps ldap is better than … something else,

That's EXACTLY right. You can't say whether LDAP is better or worse in
every scenario. And therefore you should not be proposing to remove
it.

I had been hoping you might shed some light on just what use cases you
were referring to so that we could have a constructive discussion about
if ldap is actually a reasonable solution. I even explicitly pointed
out that there may still exist some environments, such as those with
OpenLDAP only and which don't have Kerberos that could be a valid reason
to keep ldap, but I haven't heard of any such in quite a long time.

Until more recently, an argument could be made that we needed to keep
ldap around because pgAdmin didn't support Kerberos, but that's been
addressed for a couple years now.

At some point, once there aren't any good use-cases for it to be kept,
we should remove it or at least deprecate and discourage its use, though
as I suspect everyone already knows, I really don't like deprecating
things because it means we just end up keeping them around forever.

As I tried to get at in my prior response, we should be encouraging
the use of secure options when they're available and that's something
which I actively do, on these lists and in working with clients. That I
continue to find cases of perfectly good AD deployments which could be
using Kerberos but instead are using ldap is frustrating.

What I don't understand is this push-back against even talking about the
issues with ldap or appreciation that passing around cleartext passwords
is a security issue (modern password-based authentication methods, like
SCRAM, don't even do this anymore, because it's not a good idea).

If my opinion that we should remove it is that offensive, then fine,
let's drop that part of the discussion and move on to the question of:
why are people still using ldap?

I'm pretty sure the answer is that far too often people think that's
just how you integrate PG into an AD environment- and they think it's
either the only option or sometimes believe it's a secure option. We've
had people claim on these lists that using ldap doesn't pass around
cleartext passwords, which I suspect means that maybe they thought our
'ldap' auth was actually somehow doing Kerberos under the hood (I really
wish it was, but sadly that's obviously not the case).

For some users, the issue is that our Kerberos/pg_ident/et al system
isn't as good as it could be in an AD environment, which is one of the
things that I was trying to allude to in my prior reply. In particular,
we don't currently have support for working with AD's ldap for what it
was actually meant for, which is using it to query for information like
group membership. There had been some work on this not too long ago but
sadly I haven't seen recent activity around that. What I've heard
requested are things like having an option to say "users in group X are
allowed to log in to role Y", or "users in group X are allowed to log
into database Z". Would also be great to have built-in support for
syncing from ldap the list of users which should exist in PG
(pg_ldap_sync does an alright job of this today but we could certainly
do better by having it built-in, and ideally with a background worker
that connects to ldap, does an initial sync, and then listens for
changes, instead of having to have a frequent cronjob doing the sync).

I think that is, to borrow a phrase from Tom, arrant nonsense. Sure,
MD5 authentication has a pass-the-hash vulnerability, and that sucks.

So, given that we all agree with the CVE-worthy issue that exists with every release where we include md5 auth, we should be applying for q CVE prior to each release, no?

You know, I said in my previous email that you were so sure that you
were right that there was no point in trying to have a serious
discussion, and I can't really see how you could have proved that
point more thoroughly than you did here. You twisted my words around
to make it seem like I was agreeing with your point when you know full
well that I was doing the exact opposite of that.

I quoted the part where you agreed that md5 has a known vulnerability to
point out that, given that, we should be making our users aware of that
through the normal process that we use for that. I wasn't claiming that
you agreed that we should remove md5, unless you're referring to some
other part of my response which you didn't quote, in which case it'd be
helpful if you could clarify which you were referring to so that I can
try to clarify.

I understand that you still don't want to remove md5 or ldap and I don't
think anyone following this discussion would be confused in that. I do
feel we do a disservice to our users to continue encouraging the use of
either.

Thanks,

Stephen

On Wed, Nov 29, 2023 at 2:12 AM Stephen Frost <sfrost@snowman.net> wrote:

I had been hoping you might shed some light on just what use cases you
were referring to so that we could have a constructive discussion about
if ldap is actually a reasonable solution. I even explicitly pointed
out that there may still exist some environments, such as those with
OpenLDAP only and which don't have Kerberos that could be a valid reason
to keep ldap, but I haven't heard of any such in quite a long time.

I mean, I just have a hard time seeing this as a serious discussion. I
can't believe a proposal to remove LDAP authentication would reach
consensus for any reason other than because people didn't notice the
discussion or didn't want to argue about it, and I'm sure we'd have
unhappy users later. I don't want to spend a huge amount of time
arguing over something that I think is fairly obviously a non-viable
idea. If I'm wrong and a whole bunch of people show up and say they
like it, well, then I'm wrong. But either way, literally nobody wants
to read 100 emails where you and I go around in circles. And I don't
want to write them, either.

That said, I think that the case you mention, where someone has only
LDAP and not AD, is one example of where you would want LDAP
authentication. And really, that ought to end discussion of the topic,
because surely that is a valid use case, and we don't typically remove
features that someone might find useful because one hacker suggests
it. But in my experience, that's actually missing the point
completely. The most common complaint that I see from users about
authentication goes like this:

"I have log_statement=all enabled and so every time a user changes
their password the new password ends up in plaintext in the log file
and that means PostgreSQL is insecure."

I want to make it clear that, number one, this is not a joke, and,
number two, this is not the only concern that regularly reaches me
which reflects this degree of failing to understand how to set up a
secure environment. It's just an example of the level that many people
are apparently at. You're talking about removing authentication
methods because they have subtle, conditional security hazards, but a
lot of people are still running systems with flagrant, hideous
security hazards. Making it harder for such a user to store passwords
outside the database, or making them feel like that's somehow bad or
insecure when what they're doing instead is WAY worse, is not the
right thing at all.

I'd fully support having good documentation that says "hey, here are
the low security authentication configurations, here are the
medium-security ones, here are the high security ones, and here's why
these ones are better than those ones and what they protect against
and what risks remain." That would be awesome. But I'm not in favor of
removing options. We don't know where any particular user is coming
from today, and as in the example above, it might be something REALLY
bad. Moreover, users are entitled to make prudential judgments about
which things they care about in practice. They're allowed to say "hey,
yeah, so LDAP has this weakness if the server gets suborned, but I
don't care about that because of X reason," just as they're allowed to
accept the risks of any other authentication method, including trust.
Some of the reasons people might have for accepting such risks are
doubtless extremely strong, like "it's a physically disconnected
network" or "this is a burner setup that I'm tearing down tomorrow and
will contain only non-critical data anyway," and others are doubtless
poor reasons where users take inappropriate risk. But removing
everything from PostgreSQL that can be used to hose yourself is not a
viable strategy, and it's also not right for us to try to substitute
our judgement about what is appropriate in a particular user's
environment for that user's own judgement, especially if they're
right, but even if they're wrong. We're here to provide software, not
compel obedience.

I quoted the part where you agreed that md5 has a known vulnerability to
point out that, given that, we should be making our users aware of that
through the normal process that we use for that. I wasn't claiming that
you agreed that we should remove md5, unless you're referring to some
other part of my response which you didn't quote, in which case it'd be
helpful if you could clarify which you were referring to so that I can
try to clarify.

You made it sound like I supported what you wanted to do when you knew
I didn't. That's a poor foundation for any kind of reasonable
discussion of alternatives.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Wed, Nov 29, 2023 at 04:02:11PM -0500, Robert Haas wrote:

I'd fully support having good documentation that says "hey, here are
the low security authentication configurations, here are the
medium-security ones, here are the high security ones, and here's why
these ones are better than those ones and what they protect against
and what risks remain." That would be awesome.

+1. IMO the "Password Authentication" section [0]https://www.postgresql.org/docs/devel/auth-password.html does this pretty well
already.

[0]: https://www.postgresql.org/docs/devel/auth-password.html

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Wed, Nov 29, 2023 at 5:02 PM Nathan Bossart <nathandbossart@gmail.com> wrote:

On Wed, Nov 29, 2023 at 04:02:11PM -0500, Robert Haas wrote:

I'd fully support having good documentation that says "hey, here are
the low security authentication configurations, here are the
medium-security ones, here are the high security ones, and here's why
these ones are better than those ones and what they protect against
and what risks remain." That would be awesome.

+1. IMO the "Password Authentication" section [0] does this pretty well
already.

That's limited to just the password-based methods, though, so some
broader discussion of the whole suite of available techniques could be
useful. It does call out the known weaknesses of the md5 and password,
though, which is good.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Tue, Nov 28, 2023 at 10:01:57AM -0500, Robert Haas wrote:

On Tue, Nov 28, 2023 at 9:55 AM Stephen Frost <sfrost@snowman.net> wrote:

I do think we should use the correct terminology in our documentation
and would support your working on improving things in this area.

+1.

Attached is a draft patch to use the term "hash" instead of "encrypt"
for password storage. I was not able to use Michael Paquier's version
from 2017 because the code has changed too much.

I did _not_ change the user API, so CREATE/ALTER ROLE still uses
[ENCRYPTED] PASSWORD, the GUC is still called password_encryption, and
the libpq function is still called PQencryptPasswordConn(). This makes
the user interface confusing since the API uses "encryption" but the
text calls it "hashing". Is there support for renaming the API to use
"hash" and keeping "encrypt" for backward compatiblity.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

Only you can decide what is important to you.

On 27.12.23 02:04, Bruce Momjian wrote:

I did_not_ change the user API, so CREATE/ALTER ROLE still uses
[ENCRYPTED] PASSWORD, the GUC is still called password_encryption, and
the libpq function is still called PQencryptPasswordConn(). This makes
the user interface confusing since the API uses "encryption" but the
text calls it "hashing". Is there support for renaming the API to use
"hash" and keeping "encrypt" for backward compatiblity.

Yeah, this is clearly confusing. I think we should just leave it alone.
Some gentle rewording here and there to clarify things might be
appropriate (but the text already uses hashing on some occasions), but
the blanket replacement does not make things better, I think.

On Wed, Dec 27, 2023 at 10:52:15PM +0100, Peter Eisentraut wrote:

On 27.12.23 02:04, Bruce Momjian wrote:

I did_not_ change the user API, so CREATE/ALTER ROLE still uses
[ENCRYPTED] PASSWORD, the GUC is still called password_encryption, and
the libpq function is still called PQencryptPasswordConn(). This makes
the user interface confusing since the API uses "encryption" but the
text calls it "hashing". Is there support for renaming the API to use
"hash" and keeping "encrypt" for backward compatiblity.

Yeah, this is clearly confusing. I think we should just leave it alone.
Some gentle rewording here and there to clarify things might be appropriate
(but the text already uses hashing on some occasions), but the blanket
replacement does not make things better, I think.

Seeing no more replies, I will abandon this improvement idea.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

Only you can decide what is important to you.