Re: Replace current implementations in crypt() and gen_salt() to OpenSSL

From: Joe Conway <mail(at)joeconway(dot)com>
To: Daniel Gustafsson <daniel(at)yesql(dot)se>, "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com>
Cc: Peter Eisentraut <peter(at)eisentraut(dot)org>, Robert Haas <robertmhaas(at)gmail(dot)com>, "Koshi Shibagaki (Fujitsu)" <shibagaki(dot)koshi(at)fujitsu(dot)com>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Replace current implementations in crypt() and gen_salt() to OpenSSL
Date: 2025-01-18 17:32:10
Message-ID: e16b6d8e-5f8c-4f6a-85ed-e1fb59ba6b3a@joeconway.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 1/15/25 09:24, Daniel Gustafsson wrote:
>> On 14 Jan 2025, at 13:12, Hayato Kuroda (Fujitsu) <kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
>> Similar with [1], `pg_gen_salt_rounds` is not an SQL function.
>> I think we do not have to mention the function because it's just another implementation of gen_salt().
>> Also, use <function> instead of <literal>.
>
> Fixed.
>
>> I think we must call MarkGUCPrefixReserved() to catch the mis-spell.
>
> Good point, fixed.
>
>> Also: I'm not sure whether we should bump the version of pgcrypto. It should be done when
>> the API is changed, but the patch does not do. Thought?
>
> I don't think this constitutes a change which warrants a version bump so I've
> left that out for now.
>
> The attached includes a rename from "legacy_crypto" to "builtin_crypto". While
> legacy might apply now, there is work ongoing to modernize the algorithms
> supported by crypt [0] so legacy might not be applicable soon (this GUC would
> however still be relevant as the proposed code isn't FIPS certified). Builtin
> seems like a more future-proof choice in terms of naming.

I wonder if it makes any sense to test the "fips" value of the GUC here:
8<----------------
--- a/contrib/pgcrypto/expected/crypt-des.out
+++ b/contrib/pgcrypto/expected/crypt-des.out
@@ -28,4 +28,11 @@ FROM ctest;
t
(1 row)

+-- check disabling of built in crypto functions
+SET pgcrypto.builtin_crypto_enabled = off;
+UPDATE ctest SET salt = gen_salt('des');
+ERROR: use of built-in crypto functions is disabled
+UPDATE ctest SET res = crypt(data, salt);
+ERROR: use of built-in crypto functions is disabled
+RESET pgcrypto.builtin_crypto_enabled;
DROP TABLE ctest;
8<----------------

The usual case would be "fips disabled", in which case it ought to work
same as "on".

Maybe we could document that the test should fail if fips is enabled?

FWIW I have not tested at all on a fips enabled machine. I will see
about doing that...

--
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Attachment Content-Type Size
v9-0001-pgcrypto-Make-it-possible-to-disable-built-in-cry.patch text/x-patch 7.8 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2025-01-18 18:17:46 Re: [PATCH] Fix jsonb comparison for raw scalar pseudo arrays
Previous Message Robert Treat 2025-01-18 17:10:23 Re: Eagerly scan all-visible pages to amortize aggressive vacuum