Thread: BUG #8467: Slightly confusing pgcrypto example in docs
The following bug has been logged on the website: Bug reference: 8467 Logged by: Richard Neill Email address: postgresql@richardneill.org PostgreSQL version: 9.3.0 Operating system: Documentation bug Description: The documentation for pgcrypto: http://www.postgresql.org/docs/current/static/pgcrypto.html (and indeed all versions from 8.3-9.3) contains the following: -------------------- Example of authentication: SELECT pswhash = crypt('entered password', pswhash) FROM ... ; This returns true if the entered password is correct. -------------------- I found this confusing, because it's using the same name, "pswhash" in 2 places, one of which is a boolean. It would be, imho, clearer to write the example query as: -------------------- SELECT is_authenticated = crypt('entered password', pswhash) FROM ... ; -------------------- [Also, should the default example perhaps use gen_salt('bf'), as opposed to gen_salt('md5') ?]
On Tue, Sep 24, 2013 at 1:11 AM, <postgresql@richardneill.org> wrote: > The following bug has been logged on the website: > > Bug reference: 8467 > Logged by: Richard Neill > Email address: postgresql@richardneill.org > PostgreSQL version: 9.3.0 > Operating system: Documentation bug > Description: > > The documentation for pgcrypto: > http://www.postgresql.org/docs/current/static/pgcrypto.html > (and indeed all versions from 8.3-9.3) > contains the following: > > > -------------------- > Example of authentication: > > > SELECT pswhash = crypt('entered password', pswhash) FROM ... ; > > > This returns true if the entered password is correct. > -------------------- > > > I found this confusing, because it's using the same name, "pswhash" in 2 > places, one of which is a boolean. It would be, imho, clearer to write the > example query as: > > > -------------------- > SELECT is_authenticated = crypt('entered password', pswhash) FROM ... ; > -------------------- That would render the example incorrect. crypt(pwd, hash) returns the hash. Not a boolean. This hash needs to be compared to the stored one, as is explained in the instructions above the example. It's the whole expression, including the "pswhash = " that returns boolean. > [Also, should the default example perhaps use gen_salt('bf'), as opposed to > gen_salt('md5') ?] This, however, might be a good idea. People should of course always read the documentation, but having the examples including the "best practice" would probably be a good idea. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Dear Magnus, Thanks for your reply. On 24/09/13 18:31, Magnus Hagander wrote: >> The following bug has been logged on the website: >> >> Bug reference: 8467 >> >> The documentation for pgcrypto: >> http://www.postgresql.org/docs/current/static/pgcrypto.html >> (and indeed all versions from 8.3-9.3) >> contains the following: > >> -------[ ONE] ------------- >> Example of authentication: >> SELECT pswhash = crypt('entered password', pswhash) FROM ... ; > This returns true if the entered password is correct. >> -------------------- >> >> I found this confusing, because it's using the same name, "pswhash" in 2 >> places, one of which is a boolean. It would be, imho, clearer to write the >> example query as: >> >> --------[ TWO ]------------ >> SELECT is_authenticated = crypt('entered password', pswhash) FROM ... ; >> -------------------- > > That would render the example incorrect. crypt(pwd, hash) returns the > hash. Not a boolean. This hash needs to be compared to the stored one, > as is explained in the instructions above the example. It's the whole > expression, including the "pswhash = " that returns boolean. I'm sorry about that: I think I need to correct my proposed correction! I think I've been writing too much C recently, and so I foolishly mis-read that as returning pswhash, rather than returning the truth of the comparison. What I meant to write, for clarity, was: SELECT (pswhash = crypt('entered password', pswhash)) AS pswmatch FROM ... ; which would make it obvious that we're returning the boolean named pswmatch. > >> [Also, should the default example perhaps use gen_salt('bf'), as opposed to >> gen_salt('md5') ?] > > This, however, might be a good idea. People should of course always > read the documentation, but having the examples including the "best > practice" would probably be a good idea. Incidentally, there are 2 other things that confused me in this section. 1. Table F-18. Supported algorithms for crypt() has a column labelled "max password length". It would perhaps also be useful to know the size of column needed to store the crypted password (my original crypt using md5 easily fits in a varchar(70), whereas using bf needs the column to be varchar(100).) 2. Table F-20. Hash algorithm speeds What's the difference here between "crypt-md5" and "md5" ? If I've rightly read this, the algorithm named "md5" in the crypt() documentation is named "crypt-md5" here, whereas Table F20's "md5" algorithm seems to refer to something else - probably the "normal" version of md5. If so, it would be clearer to write that the last 2 lines ("md5" and "sha1") are for comparison only, and refer to the speed of doing an ordinary md5/sha1 sum, rather than the md5-variant of crypt(). Anyway, thanks again for your help - Postgres is a wonderful system, which I've found to be repeatedly useful. Best wishes, Richard
On Tue, Sep 24, 2013 at 11:20:55PM +0100, Richard Neill wrote:
> I'm sorry about that: I think I need to correct my proposed
> correction! I think I've been writing too much C recently, and so I
> foolishly mis-read that as returning pswhash, rather than returning
> the truth of the comparison.
>
> What I meant to write, for clarity, was:
>
> SELECT (pswhash = crypt('entered password', pswhash)) AS pswmatch FROM ... ;
>
> which would make it obvious that we're returning the boolean named pswmatch.
>
> >
> >>[Also, should the default example perhaps use gen_salt('bf'), as opposed to
> >>gen_salt('md5') ?]
> >
> >This, however, might be a good idea. People should of course always
> >read the documentation, but having the examples including the "best
> >practice" would probably be a good idea.
>
> Incidentally, there are 2 other things that confused me in this section.
>
> 1. Table F-18. Supported algorithms for crypt() has a column
> labelled "max password length". It would perhaps also be useful to
> know the size of column needed to store the crypted password (my
> original crypt using md5 easily fits in a varchar(70), whereas using
> bf needs the column to be varchar(100).)
>
>
> 2. Table F-20. Hash algorithm speeds
>
> What's the difference here between "crypt-md5" and "md5" ?
>
> If I've rightly read this, the algorithm named "md5" in the crypt()
> documentation is named "crypt-md5" here, whereas Table F20's "md5"
> algorithm seems to refer to something else - probably the "normal"
> version of md5.
>
> If so, it would be clearer to write that the last 2 lines ("md5" and
> "sha1") are for comparison only, and refer to the speed of doing an
> ordinary md5/sha1 sum, rather than the md5-variant of crypt().
>
>
> Anyway, thanks again for your help - Postgres is a wonderful system,
> which I've found to be repeatedly useful.
Based on your report, I have developed the attached doc patch which
clarifies when MD5 hash is being referenced, and when MD5 crypt is. I
have also added your other suggestions.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
Attachment
On Wed, Oct 2, 2013 at 12:00:44PM -0400, Bruce Momjian wrote: > Based on your report, I have developed the attached doc patch which > clarifies when MD5 hash is being referenced, and when MD5 crypt is. I > have also added your other suggestions. Patch applied, and backpatched to 9.3.X. Thanks for the suggestions. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
The changes shown below are incorrect, I think. On 10/2/13 12:00 PM, Bruce Momjian wrote: > *************** gen_salt(type text [, iter_count integer > *** 353,359 **** > <entry>12 years</entry> > </row> > <row> > ! <entry><literal>md5</></entry> > <entry>2345086</entry> > <entry>1 day</entry> > <entry>3 years</entry> > --- 358,364 ---- > <entry>12 years</entry> > </row> > <row> > ! <entry><literal>md5 hash</></entry> > <entry>2345086</entry> > <entry>1 day</entry> > <entry>3 years</entry> > *************** gen_salt(type text [, iter_count integer > *** 380,386 **** > </listitem> > <listitem> > <para> > ! <literal>md5</> numbers are from mdcrack 1.2. > </para> > </listitem> > <listitem> > --- 385,391 ---- > </listitem> > <listitem> > <para> > ! <literal>md5 hash</> numbers are from mdcrack 1.2. > </para> > </listitem> > <listitem> > *************** gen_random_bytes(count integer) returns > *** 1343,1349 **** > <entry>OpenBSD sys/crypto</entry> > </row> > <row> > ! <entry>MD5 and SHA1</entry> > <entry>WIDE Project</entry> > <entry>KAME kame/sys/crypto</entry> > </row> > --- 1348,1354 ---- > <entry>OpenBSD sys/crypto</entry> > </row> > <row> > ! <entry>MD5 hash and SHA1</entry> > <entry>WIDE Project</entry> > <entry>KAME kame/sys/crypto</entry> > </row>
On Thu, Oct 10, 2013 at 04:05:50PM -0400, Peter Eisentraut wrote:
> The changes shown below are incorrect, I think.
>
>
> On 10/2/13 12:00 PM, Bruce Momjian wrote:
> > *************** gen_salt(type text [, iter_count integer
> > *** 353,359 ****
> > <entry>12 years</entry>
> > </row>
> > <row>
> > ! <entry><literal>md5</></entry>
> > <entry>2345086</entry>
> > <entry>1 day</entry>
> > <entry>3 years</entry>
> > --- 358,364 ----
> > <entry>12 years</entry>
> > </row>
> > <row>
> > ! <entry><literal>md5 hash</></entry>
Uh, the table already has a mention of md5 crypt above:
<entry><literal>crypt-md5</></entry>
How can the later entry not be MD5 hash?
> > <entry>2345086</entry>
> > <entry>1 day</entry>
> > <entry>3 years</entry>
> > *************** gen_salt(type text [, iter_count integer
> > *** 380,386 ****
> > </listitem>
> > <listitem>
> > <para>
> > ! <literal>md5</> numbers are from mdcrack 1.2.
> > </para>
> > </listitem>
> > <listitem>
> > --- 385,391 ----
> > </listitem>
> > <listitem>
> > <para>
> > ! <literal>md5 hash</> numbers are from mdcrack 1.2.
> > </para>
> > </listitem>
> > <listitem>
> > *************** gen_random_bytes(count integer) returns
> > *** 1343,1349 ****
> > <entry>OpenBSD sys/crypto</entry>
> > </row>
> > <row>
> > ! <entry>MD5 and SHA1</entry>
> > <entry>WIDE Project</entry>
> > <entry>KAME kame/sys/crypto</entry>
> > </row>
> > --- 1348,1354 ----
> > <entry>OpenBSD sys/crypto</entry>
> > </row>
> > <row>
> > ! <entry>MD5 hash and SHA1</entry>
> > <entry>WIDE Project</entry>
> > <entry>KAME kame/sys/crypto</entry>
> > </row>
>
Again, "MD5 crypt" is mentioned in the same table above:
<entry>MD5 crypt</entry>
so how can this not be md5 hash?
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
On Thu, 2013-10-10 at 19:14 -0400, Bruce Momjian wrote: > > The changes shown below are incorrect, I think. > > > > > > On 10/2/13 12:00 PM, Bruce Momjian wrote: > > > *************** gen_salt(type text [, iter_count integer > > > *** 353,359 **** > > > <entry>12 years</entry> > > > </row> > > > <row> > > > ! <entry><literal>md5</></entry> > > > <entry>2345086</entry> > > > <entry>1 day</entry> > > > <entry>3 years</entry> > > > --- 358,364 ---- > > > <entry>12 years</entry> > > > </row> > > > <row> > > > ! <entry><literal>md5 hash</></entry> > > Uh, the table already has a mention of md5 crypt above: > > <entry><literal>crypt-md5</></entry> > > How can the later entry not be MD5 hash? Because what you pass to the functions is 'md5', not 'md5 hash', which is what the new text appears to indicate.
On Thu, Oct 10, 2013 at 08:22:30PM -0400, Peter Eisentraut wrote: > On Thu, 2013-10-10 at 19:14 -0400, Bruce Momjian wrote: > > > The changes shown below are incorrect, I think. > > > > > > > > > On 10/2/13 12:00 PM, Bruce Momjian wrote: > > > > *************** gen_salt(type text [, iter_count integer > > > > *** 353,359 **** > > > > <entry>12 years</entry> > > > > </row> > > > > <row> > > > > ! <entry><literal>md5</></entry> > > > > <entry>2345086</entry> > > > > <entry>1 day</entry> > > > > <entry>3 years</entry> > > > > --- 358,364 ---- > > > > <entry>12 years</entry> > > > > </row> > > > > <row> > > > > ! <entry><literal>md5 hash</></entry> > > > > Uh, the table already has a mention of md5 crypt above: > > > > <entry><literal>crypt-md5</></entry> > > > > How can the later entry not be MD5 hash? > > Because what you pass to the functions is 'md5', not 'md5 hash', which > is what the new text appears to indicate. So if we revert, will it still be clear what is MD5 and what is MD5 hash? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On Thu, Oct 10, 2013 at 08:32:32PM -0400, Bruce Momjian wrote: > > > How can the later entry not be MD5 hash? > > > > Because what you pass to the functions is 'md5', not 'md5 hash', which > > is what the new text appears to indicate. > > So if we revert, will it still be clear what is MD5 and what is MD5 hash? I mean, will it be clear what is MD5 crypt and what is MD5 hash? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On Thu, Oct 10, 2013 at 08:42:14PM -0400, Bruce Momjian wrote: > On Thu, Oct 10, 2013 at 08:32:32PM -0400, Bruce Momjian wrote: > > > > How can the later entry not be MD5 hash? > > > > > > Because what you pass to the functions is 'md5', not 'md5 hash', which > > > is what the new text appears to indicate. > > > > So if we revert, will it still be clear what is MD5 and what is MD5 hash? > > I mean, will it be clear what is MD5 crypt and what is MD5 hash? I have made the attached doc change, which places "(hash)" outside of the literal text block. You are right the literal blocks should match what is passed. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Attachment
On Thu, 2014-02-13 at 15:39 -0500, Bruce Momjian wrote: > On Thu, Oct 10, 2013 at 08:42:14PM -0400, Bruce Momjian wrote: > > On Thu, Oct 10, 2013 at 08:32:32PM -0400, Bruce Momjian wrote: > > > > > How can the later entry not be MD5 hash? > > > > > > > > Because what you pass to the functions is 'md5', not 'md5 hash', which > > > > is what the new text appears to indicate. > > > > > > So if we revert, will it still be clear what is MD5 and what is MD5 hash? > > > > I mean, will it be clear what is MD5 crypt and what is MD5 hash? > > I have made the attached doc change, which places "(hash)" outside of > the literal text block. You are right the literal blocks should match > what is passed. I think this entire line of patches should be reverted from the 9.3 branch, because it's not a bug fix, and arguably makes no sense. I also think that these patches should be reverted from the master branch, because they make no sense and don't actually address the bug report. Adding an output length column might make sense as an independent patch.