On 1/15/25 09:24, Daniel Gustafsson wrote:
On 14 Jan 2025, at 13:12, Hayato Kuroda (Fujitsu) <kuroda.hay...@fujitsu.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
diff --git a/contrib/pgcrypto/expected/crypt-des.out b/contrib/pgcrypto/expected/crypt-des.out
index a462dcd..edc559d 100644
*** a/contrib/pgcrypto/expected/crypt-des.out
--- b/contrib/pgcrypto/expected/crypt-des.out
*************** FROM ctest;
*** 28,31 ****
--- 28,42 ----
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;
+ SET pgcrypto.builtin_crypto_enabled = fips;
+ UPDATE ctest SET salt = gen_salt('des');
+ UPDATE ctest SET res = crypt(data, salt);
+ RESET pgcrypto.builtin_crypto_enabled;
DROP TABLE ctest;
diff --git a/contrib/pgcrypto/openssl.c b/contrib/pgcrypto/openssl.c
index 448db33..fc930e4 100644
*** a/contrib/pgcrypto/openssl.c
--- b/contrib/pgcrypto/openssl.c
*************** ResOwnerReleaseOSSLCipher(Datum res)
*** 794,796 ****
--- 794,837 ----
{
free_openssl_cipher((OSSLCipher *) DatumGetPointer(res));
}
+
+ /*
+ * CheckBuiltinCryptoMode
+ *
+ * Function for erroring out in case built-in crypto is executed when the user
+ * has disabled it. If builtin_crypto_enabled is set to BC_OFF or BC_FIPS and
+ * OpenSSL is operating in FIPS mode the function will error out, else the
+ * query executing built-in crypto can proceed.
+ */
+ void
+ CheckBuiltinCryptoMode(void)
+ {
+ int fips_enabled;
+
+ if (builtin_crypto_enabled == BC_ON)
+ return;
+
+ if (builtin_crypto_enabled == BC_OFF)
+ ereport(ERROR,
+ errmsg("use of built-in crypto functions is disabled"));
+
+ Assert(builtin_crypto_enabled == BC_FIPS);
+
+ /*
+ * EVP_default_properties_is_fips_enabled was added in OpenSSL 3.0, before
+ * that FIPS_mode() was used to test for FIPS being enabled. The last
+ * upstream OpenSSL version before 3.0 which supported FIPS was 1.0.2, but
+ * there are forks of 1.1.1 which are FIPS certified so we still need to
+ * test with FIPS_mode() even though we don't support 1.0.2.
+ */
+ fips_enabled =
+ #if OPENSSL_VERSION_NUMBER >= 0x30000000L
+ EVP_default_properties_is_fips_enabled(NULL);
+ #else
+ FIPS_mode();
+ #endif
+
+ if (fips_enabled)
+ ereport(ERROR,
+ errmsg("use of non-FIPS certified crypto not allowed when OpenSSL is in FIPS mode"));
+ }
diff --git a/contrib/pgcrypto/pgcrypto.c b/contrib/pgcrypto/pgcrypto.c
index ebd76ee..d02a621 100644
*** a/contrib/pgcrypto/pgcrypto.c
--- b/contrib/pgcrypto/pgcrypto.c
***************
*** 38,53 ****
--- 38,84 ----
#include "px-crypt.h"
#include "px.h"
#include "utils/builtins.h"
+ #include "utils/guc.h"
#include "varatt.h"
PG_MODULE_MAGIC;
/* private stuff */
+ static const struct config_enum_entry builtin_crypto_options[] = {
+ {"on", BC_ON, false},
+ {"off", BC_OFF, false},
+ {"fips", BC_FIPS, false},
+ {NULL, 0, false}
+ };
+
typedef int (*PFN) (const char *name, void **res);
static void *find_provider(text *name, PFN provider_lookup, const char *desc,
int silent);
+ int builtin_crypto_enabled = BC_ON;
+
+ /*
+ * Entrypoint of this module.
+ */
+ void
+ _PG_init(void)
+ {
+ DefineCustomEnumVariable("pgcrypto.builtin_crypto_enabled",
+ "Sets if builtin crypto functions are enabled.",
+ "\"on\" enables builtin crypto, \"off\" unconditionally disables and \"fips\" "
+ "will disable builtin crypto if OpenSSL is in FIPS mode",
+ &builtin_crypto_enabled,
+ BC_ON,
+ builtin_crypto_options,
+ PGC_SUSET,
+ 0,
+ NULL,
+ NULL,
+ NULL);
+ MarkGUCPrefixReserved("pgcrypto");
+ }
+
/* SQL function: hash(bytea, text) returns bytea */
PG_FUNCTION_INFO_V1(pg_digest);
diff --git a/contrib/pgcrypto/px-crypt.c b/contrib/pgcrypto/px-crypt.c
index 0913ff2..96ce938 100644
*** a/contrib/pgcrypto/px-crypt.c
--- b/contrib/pgcrypto/px-crypt.c
*************** px_crypt(const char *psw, const char *sa
*** 91,96 ****
--- 91,98 ----
{
const struct px_crypt_algo *c;
+ CheckBuiltinCryptoMode();
+
for (c = px_crypt_list; c->id; c++)
{
if (!c->id_len)
*************** px_gen_salt(const char *salt_type, char
*** 135,140 ****
--- 137,144 ----
char *p;
char rbuf[16];
+ CheckBuiltinCryptoMode();
+
for (g = gen_list; g->name; g++)
if (pg_strcasecmp(g->name, salt_type) == 0)
break;
diff --git a/contrib/pgcrypto/px.h b/contrib/pgcrypto/px.h
index 471bb4e..c574f72 100644
*** a/contrib/pgcrypto/px.h
--- b/contrib/pgcrypto/px.h
***************
*** 89,94 ****
--- 89,100 ----
#define PXE_PGP_UNSUPPORTED_PUBALGO -122
#define PXE_PGP_MULTIPLE_SUBKEYS -123
+ typedef enum BuiltinCryptoOptions
+ {
+ BC_ON,
+ BC_OFF,
+ BC_FIPS,
+ } BuiltinCryptoOptions;
typedef struct px_digest PX_MD;
typedef struct px_alias PX_Alias;
*************** typedef struct px_hmac PX_HMAC;
*** 96,101 ****
--- 102,109 ----
typedef struct px_cipher PX_Cipher;
typedef struct px_combo PX_Combo;
+ extern int builtin_crypto_enabled;
+
struct px_digest
{
unsigned (*result_size) (PX_MD *h);
*************** void px_set_debug_handler(void (*handle
*** 182,187 ****
--- 190,197 ----
void px_memset(void *ptr, int c, size_t len);
+ void CheckBuiltinCryptoMode(void);
+
#ifdef PX_DEBUG
void px_debug(const char *fmt,...) pg_attribute_printf(1, 2);
#else
diff --git a/contrib/pgcrypto/sql/crypt-des.sql b/contrib/pgcrypto/sql/crypt-des.sql
index a85ec1e..bd61938 100644
*** a/contrib/pgcrypto/sql/crypt-des.sql
--- b/contrib/pgcrypto/sql/crypt-des.sql
*************** UPDATE ctest SET res = crypt(data, salt)
*** 18,21 ****
--- 18,32 ----
SELECT res = crypt(data, res) AS "worked"
FROM ctest;
+ -- check disabling of built in crypto functions
+ SET pgcrypto.builtin_crypto_enabled = off;
+ UPDATE ctest SET salt = gen_salt('des');
+ UPDATE ctest SET res = crypt(data, salt);
+ RESET pgcrypto.builtin_crypto_enabled;
+
+ SET pgcrypto.builtin_crypto_enabled = fips;
+ UPDATE ctest SET salt = gen_salt('des');
+ UPDATE ctest SET res = crypt(data, salt);
+ RESET pgcrypto.builtin_crypto_enabled;
+
DROP TABLE ctest;
diff --git a/doc/src/sgml/pgcrypto.sgml b/doc/src/sgml/pgcrypto.sgml
index 396c67f..d1034ef 100644
*** a/doc/src/sgml/pgcrypto.sgml
--- b/doc/src/sgml/pgcrypto.sgml
*************** gen_random_uuid() returns uuid
*** 1222,1227 ****
--- 1222,1265 ----
</sect3>
</sect2>
+ <sect2 id="pgcrypto-configuration-parameters">
+ <title>Configuration Parameters</title>
+
+ <para>
+ There is one configuration parameter that controls the behavior of
+ <filename>pgcrypto</filename>.
+ </para>
+
+ <variablelist>
+ <varlistentry id="pgcrypto-configuration-parameters-builtin_crypto_enabled">
+ <term>
+ <varname>pgcrypto.builtin_crypto_enabled</varname> (<type>enum</type>)
+ <indexterm>
+ <primary><varname>pgcrypto.builtin_crypto_enabled</varname> configuration
+ parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ <varname>pgcrypto.builtin_crypto_enabled</varname> determines if the
+ built in crypto functions <function>gen_salt()</function>, and
+ <function>crypt()</function> are available for use. Setting this to
+ <literal>off</literal> disables these functions. <literal>on</literal>
+ (the default) enables these functions to work normally.
+ <literal>fips</literal> disables these functions if
+ <application>OpenSSL</application> is detected to operate in FIPS mode.
+ </para>
+ </listitem>
+ </varlistentry>
+ </variablelist>
+
+ <para>
+ In ordinary usage, this parameter is set
+ in <filename>postgresql.conf</filename>, although superusers can alter it
+ on-the-fly within their own sessions.
+ </para>
+ </sect2>
+
<sect2 id="pgcrypto-author">
<title>Author</title>
