Re: SHA-2 functions

There was a complaint recently about the documentation using the widely
frowned-upon md5() function in an unrelated context as an example hash
function. This is quite common in many examples, such as hashing row
values to compare them, or hashing datums if they don't fit into an
index. But there is nothing we can easily replace md5 with, without
going to things like pgcrypto.

I also noticed while working on some SSL code that we have perfectly
good SHA-2 functionality in the server already, but it has no test
coverage outside the SCRAM tests.

So I suggest these patches that expose the new functions sha224(),
sha256(), sha384(), sha512(). That allows us to make the SSL and SCRAM
tests more robust, and it will allow them to be used in general purpose
contexts over md5().

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hello Peter,

> So I suggest these patches that expose the new functions sha224(),
> sha256(), sha384(), sha512(). That allows us to make the SSL and SCRAM
> tests more robust, and it will allow them to be used in general purpose
> contexts over md5().

Nice patch. I wonder though whether tests should include hashing
non-ASCII characters as well.

--
Best regards,
Aleksander Alekseev

On 02/19/2018 08:43 AM, Peter Eisentraut wrote:
> I also noticed while working on some SSL code that we have perfectly
> good SHA-2 functionality in the server already, but it has no test
> coverage outside the SCRAM tests.
>
> So I suggest these patches that expose the new functions sha224(),
> sha256(), sha384(), sha512(). That allows us to make the SSL and SCRAM
> tests more robust, and it will allow them to be used in general purpose
> contexts over md5().

I didn't look closely at the patch itself, but +1 on the concept.

Joe

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

On 2/19/18 09:06, Aleksander Alekseev wrote:
>> So I suggest these patches that expose the new functions sha224(),
>> sha256(), sha384(), sha512(). That allows us to make the SSL and SCRAM
>> tests more robust, and it will allow them to be used in general purpose
>> contexts over md5().
>
> Nice patch. I wonder though whether tests should include hashing
> non-ASCII characters as well.

The input is of type bytea, so the concept of non-ASCII characters
doesn't quite apply.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, Feb 19, 2018 at 03:02:02PM -0500, Peter Eisentraut wrote:
> On 2/19/18 09:06, Aleksander Alekseev wrote:
>>> So I suggest these patches that expose the new functions sha224(),
>>> sha256(), sha384(), sha512(). That allows us to make the SSL and SCRAM
>>> tests more robust, and it will allow them to be used in general purpose
>>> contexts over md5().
>>
>> Nice patch. I wonder though whether tests should include hashing
>> non-ASCII characters as well.
>
> The input is of type bytea, so the concept of non-ASCII characters
> doesn't quite apply.

Encoding issues is a reason to use bytea output in some cases. For
logical decoding this is for example also why an interface like
pg_logical_slot_peek_binary_changes() exists... Back to the patch.
--
Michael

On Mon, Feb 19, 2018 at 08:43:44AM -0500, Peter Eisentraut wrote:
> I also noticed while working on some SSL code that we have perfectly
> good SHA-2 functionality in the server already, but it has no test
> coverage outside the SCRAM tests.

Yep, the refactoring in src/common/ has been done for SCRAM. As The
first versions of the patch were for SCRAM-SHA-1, only SHA-1 functions
were moved. With SCRAM-SHA-256, the full set of APIs for 256, 386 and
512 has been moved as they share much code.

> So I suggest these patches that expose the new functions sha224(),
> sha256(), sha384(), sha512(). That allows us to make the SSL and SCRAM
> tests more robust, and it will allow them to be used in general purpose
> contexts over md5().

Huge +1 as well. This also makes sure that the non-SSL implementation
carried in Postgres is consistent what what OpenSSL has. This would
matter as well if new SSL implementations are added in the future.

+ <entry><literal>sha224('abc')</literal></entry>
+ <entry><literal>\x23097d223405d8228642a477bda2&#x200B;55b32aadbce4bda0b3f7e36c9da7</literal></entry>
Some bytea characters from the hash are not able to show up correctly?
This does not result in spaces.

+ Note that for historic reasons, the function <function>md5</function>
+ returns a hex-encoded value of type <type>text</type> whereas the SHA-2
+ functions return type <type>bytea</type>. Use the functions
+ <function>encode</function> and <function>decode</function> to convert
+ between the two.
Adding an example would be nice.

varlena.c is already large and messy. I would suggest to split into a
new file all the user-facing cryptographic functions, including md5 and
hex functions, say in src/backend/utils/adt/crypt.c.
--
Michael

On 2/19/18 21:07, Michael Paquier wrote:
> + <entry><literal>sha224('abc')</literal></entry>
> + <entry><literal>\x23097d223405d8228642a477bda2&#x200B;55b32aadbce4bda0b3f7e36c9da7</literal></entry>
> Some bytea characters from the hash are not able to show up correctly?
> This does not result in spaces.

U+200B is a zero-width space, used here to hint for possible line breaks.

> + Note that for historic reasons, the function <function>md5</function>
> + returns a hex-encoded value of type <type>text</type> whereas the SHA-2
> + functions return type <type>bytea</type>. Use the functions
> + <function>encode</function> and <function>decode</function> to convert
> + between the two.
> Adding an example would be nice.

OK

> varlena.c is already large and messy. I would suggest to split into a
> new file all the user-facing cryptographic functions, including md5 and
> hex functions, say in src/backend/utils/adt/crypt.c.

I had originally started a new file called hash.c, but I figured that
would be quite confusing. I can use crypt.c or a similar name.
Although crypt.c sounds a lot like crypt().

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> writes:
> On 2/19/18 21:07, Michael Paquier wrote:
>> varlena.c is already large and messy. I would suggest to split into a
>> new file all the user-facing cryptographic functions, including md5 and
>> hex functions, say in src/backend/utils/adt/crypt.c.

> I had originally started a new file called hash.c, but I figured that
> would be quite confusing. I can use crypt.c or a similar name.
> Although crypt.c sounds a lot like crypt().

cryptohashes.c or some such? I concur with Michael that dropping this
into varlena.c isn't a great plan.

regards, tom lane

On Tue, Feb 20, 2018 at 05:09:48PM -0500, Tom Lane wrote:
> Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> writes:
>> On 2/19/18 21:07, Michael Paquier wrote:
>>> varlena.c is already large and messy. I would suggest to split into a
>>> new file all the user-facing cryptographic functions, including md5 and
>>> hex functions, say in src/backend/utils/adt/crypt.c.
>
>> I had originally started a new file called hash.c, but I figured that
>> would be quite confusing. I can use crypt.c or a similar name.
>> Although crypt.c sounds a lot like crypt().
>
> cryptohashes.c or some such? I concur with Michael that dropping this
> into varlena.c isn't a great plan.

I think that crypto_hash.c or hash_crypt.c would be adapted as well.
crypt.c is too much generic, so including both concepts in the name is
the way to go. The name given by Tom here sounds actually nice.
--
Michael

On 2/20/18 23:04, Michael Paquier wrote:
> I think that crypto_hash.c or hash_crypt.c would be adapted as well.
> crypt.c is too much generic, so including both concepts in the name is
> the way to go. The name given by Tom here sounds actually nice.

Updated patches

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Feb 21, 2018 at 03:45:17PM -0500, Peter Eisentraut wrote:
> On 2/20/18 23:04, Michael Paquier wrote:
>> I think that crypto_hash.c or hash_crypt.c would be adapted as well.
>> crypt.c is too much generic, so including both concepts in the name is
>> the way to go. The name given by Tom here sounds actually nice.
>
> Updated patches

I have been reviewing both patches, and those look good to me.

git diff --check has one complain:
src/backend/utils/adt/cryptohashes.c:170: new blank line at EOF.
--
Michael

On 2/22/18 01:05, Michael Paquier wrote:
> On Wed, Feb 21, 2018 at 03:45:17PM -0500, Peter Eisentraut wrote:
>> On 2/20/18 23:04, Michael Paquier wrote:
>>> I think that crypto_hash.c or hash_crypt.c would be adapted as well.
>>> crypt.c is too much generic, so including both concepts in the name is
>>> the way to go. The name given by Tom here sounds actually nice.
>>
>> Updated patches
>
> I have been reviewing both patches, and those look good to me.

Committed, thanks

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services