Re: [HACKERS] Changing references of password encryption to hashing

[Noah Misch]

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  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

Re: [HACKERS] Changing references of password encryption to hashing

[Stephen Frost]

* Joe Conway (m...@joeconway.com) wrote:
> On 03/16/2017 06:19 AM, Robert Haas wrote:
> > On Mon, Mar 13, 2017 at 4:48 AM, Craig Ringer  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


signature.asc
Description: Digital signature

Re: [HACKERS] Changing references of password encryption to hashing

[Joe Conway]

On 03/16/2017 06:19 AM, Robert Haas wrote:
> On Mon, Mar 13, 2017 at 4:48 AM, Craig Ringer  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



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Changing references of password encryption to hashing

[Robert Haas]

On Mon, Mar 13, 2017 at 4:48 AM, Craig Ringer  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

Re: [HACKERS] Changing references of password encryption to hashing

[Bruce Momjian]

On Mon, Mar 13, 2017 at 04:48:21PM +0800, Craig Ringer wrote:
> On 12 March 2017 at 06:51, Joe Conway  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  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

Re: [HACKERS] Changing references of password encryption to hashing

[Craig Ringer]

On 12 March 2017 at 06:51, Joe Conway  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

Re: [HACKERS] Changing references of password encryption to hashing

[Michael Paquier]

On Sun, Mar 12, 2017 at 7:51 AM, Joe Conway  wrote:
> On 03/09/2017 06:16 PM, Michael Paquier wrote:
>> As discussed here:
>> https://www.postgresql.org/message-id/98cafcd0-5557-0bdf-4837-0f2b7782d...@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.
>
>   
> 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


hashed-passwords-doc-v2.patch
Description: Binary data

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

Re: [HACKERS] Changing references of password encryption to hashing

[Joe Conway]

On 03/09/2017 06:16 PM, Michael Paquier wrote:
> As discussed here:
> https://www.postgresql.org/message-id/98cafcd0-5557-0bdf-4837-0f2b7782d...@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.
   
  
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<
  
   
-   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<
   
-   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



signature.asc
Description: OpenPGP digital signature

[HACKERS] Changing references of password encryption to hashing

[Michael Paquier]

Hi all,

As discussed here:
https://www.postgresql.org/message-id/98cafcd0-5557-0bdf-4837-0f2b7782d...@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


hashed-passwords-doc.patch
Description: Binary data

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