This patch is surprisingly compact and straightforward for providing
such complex functionality.
I have one major code comment that needs addressing:
In src/interfaces/libpq/fe-auth-scram.c, there is:
+ memcpy(ClientKey, state->conn->scram_client_key_binary,
SCRAM_MAX_KEY_LEN);
Here you are assuming that scram_client_key_binary has a fixed length,
but the allocation is
+ len = pg_b64_dec_len(strlen(conn->scram_client_key));
+ conn->scram_client_key_len = len;
+ conn->scram_client_key_binary = malloc(len);
And scram_client_key is passed by the client. There needs to be some
verification that what is passed in is of the right length.
At the moment, we only support one variant of SCRAM, so all the keys
etc. are of a fixed length. But we should make sure that this wouldn't
break in confusing ways in the future if that is no longer the case.
Attached are a few minor fix-up patches for your patches. I have marked
a couple of places where more documentation could be added.
In the future, you can squash all of this (code plus documentation) into
one patch.
postgres_fdw has this error message:
ereport(ERROR,
(errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
errmsg("password or GSSAPI delegated credentials required"),
errdetail("Non-superusers must delegate GSSAPI credentials
or provide a password in the user mapping.")));
Maybe the option of having SCRAM pass-through should be mentioned here?
It seems sort of analogous to the delegate GSSAPI credentials case.
Finally, if you have time, maybe look into whether this could also be
implemented in dblink.
From e4bffb68a4a29993462777c44a4b5f41c406039a Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Wed, 8 Jan 2025 11:36:56 +0100
Subject: [PATCH 1/3] Minor cosmetic improvements
---
contrib/postgres_fdw/Makefile | 2 +-
contrib/postgres_fdw/meson.build | 6 +++---
src/interfaces/libpq/libpq-int.h | 4 ++--
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/contrib/postgres_fdw/Makefile b/contrib/postgres_fdw/Makefile
index 6c12c8e9251..adfbd2ef758 100644
--- a/contrib/postgres_fdw/Makefile
+++ b/contrib/postgres_fdw/Makefile
@@ -8,7 +8,6 @@ OBJS = \
option.o \
postgres_fdw.o \
shippable.o
-TAP_TESTS = 1
PGFILEDESC = "postgres_fdw - foreign data wrapper for PostgreSQL"
PG_CPPFLAGS = -I$(libpq_srcdir)
@@ -18,6 +17,7 @@ EXTENSION = postgres_fdw
DATA = postgres_fdw--1.0.sql postgres_fdw--1.0--1.1.sql
postgres_fdw--1.1--1.2.sql
REGRESS = postgres_fdw query_cancel
+TAP_TESTS = 1
ifdef USE_PGXS
PG_CONFIG = pg_config
diff --git a/contrib/postgres_fdw/meson.build b/contrib/postgres_fdw/meson.build
index 61269934c18..8b29be24dee 100644
--- a/contrib/postgres_fdw/meson.build
+++ b/contrib/postgres_fdw/meson.build
@@ -42,8 +42,8 @@ tests += {
'regress_args': ['--dlpath', meson.build_root() / 'src/test/regress'],
},
'tap': {
- 'tests': [
- 't/001_auth_scram.pl',
- ],
+ 'tests': [
+ 't/001_auth_scram.pl',
+ ],
},
}
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 79097b63ec0..d29ef1cfb77 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -521,9 +521,9 @@ struct pg_conn
* tried host */
bool send_appname; /* okay to send application_name? */
size_t scram_client_key_len;
- char *scram_client_key_binary;
+ void *scram_client_key_binary;
size_t scram_server_key_len;
- char *scram_server_key_binary;
+ void *scram_server_key_binary;
/* Miscellaneous stuff */
int be_pid; /* PID of backend ---
needed for cancels */
--
2.47.1
From 1c33c6b1e011f47c1d7decd5da6e952fad21ed6d Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Wed, 8 Jan 2025 11:37:17 +0100
Subject: [PATCH 2/3] Must check return value of malloc()
---
src/interfaces/libpq/fe-connect.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/interfaces/libpq/fe-connect.c
b/src/interfaces/libpq/fe-connect.c
index 49f36f0e3d9..ebe272617e0 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -1807,6 +1807,8 @@ pqConnectOptions2(PGconn *conn)
len = pg_b64_dec_len(strlen(conn->scram_client_key));
conn->scram_client_key_len = len;
conn->scram_client_key_binary = malloc(len);
+ if (!conn->scram_client_key_binary)
+ goto oom_error;
pg_b64_decode(conn->scram_client_key,
strlen(conn->scram_client_key),
conn->scram_client_key_binary, len);
}
@@ -1818,6 +1820,8 @@ pqConnectOptions2(PGconn *conn)
len = pg_b64_dec_len(strlen(conn->scram_server_key));
conn->scram_server_key_len = len;
conn->scram_server_key_binary = malloc(len);
+ if (!conn->scram_server_key_binary)
+ goto oom_error;
pg_b64_decode(conn->scram_server_key,
strlen(conn->scram_server_key),
conn->scram_server_key_binary, len);
}
--
2.47.1
From fd96accf772c78cb27a3821224f9986103f8ded5 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Wed, 8 Jan 2025 11:37:35 +0100
Subject: [PATCH 3/3] Placeholders for more documentation
---
doc/src/sgml/libpq.sgml | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 105b22b3171..68d61015a99 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -2199,6 +2199,24 @@ <title>Parameter Key Words</title>
</listitem>
</varlistentry>
+ <varlistentry id="libpq-connect-scram-client-key"
xreflabel="scram_client_key">
+ <term><literal>scram_client_key</literal></term>
+ <listitem>
+ <para>
+ TODO
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-connect-scram-server-key"
xreflabel="scram_server_key">
+ <term><literal>scram_server_key</literal></term>
+ <listitem>
+ <para>
+ TODO
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="libpq-connect-service" xreflabel="service">
<term><literal>service</literal></term>
<listitem>
--
2.47.1
