On 02.11.22 20:02, Jacob Champion wrote:
A few notes:
+ else if (beresp == 'v')
+ {
+ if
(pqGetNegotiateProtocolVersion3(conn))
+ {
+ /* We'll come back when there
is more data */
+ return PGRES_POLLING_READING;
+ }
+ /* OK, we read the message; mark data
consumed */
+ conn->inStart = conn->inCursor;
+ goto error_return;
+ }
This new code path doesn't go through the message length checks that are
done for the 'R' and 'E' cases, and pqGetNegotiateProtocolVersion3()
doesn't take the message length to know where to stop anyway, so a
misbehaving server can chew up client resources.
Fixed in new patch.
It looks like the server is expecting to be able to continue the
conversation with a newer client after sending a
NegotiateProtocolVersion. Is an actual negotiation planned for the future?
The protocol documentation says:
| The client may then choose either
| to continue with the connection using the specified protocol version
| or to abort the connection.
In this case, we are choosing to abort the connection.
We could add negotiation in the future, but then we'd have to first have
a concrete case of something to negotiate about. For example, if we
added an optional performance feature into the protocol, then one could
negotiate by falling back to not using that. But for the kinds of
features I'm thinking about right now (column encryption), you can't
proceed if the feature is not supported. So I think this would need to
be considered case by case.
I think the documentation on NegotiateProtocolVersion (not introduced in
this patch) is misleading/wrong; it says that the version number sent
back is the "newest minor protocol version supported by the server for
the major protocol version requested by the client" which doesn't seem
to match the actual usage seen here.
I don't follow. If libpq sends a protocol version of 3.1, then the
server responds by saying it supports only 3.0. What are you seeing?
From 64dc983097553af9bed4293821bd45f1932e62c6 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Tue, 8 Nov 2022 09:24:29 +0100
Subject: [PATCH v3] libpq: Handle NegotiateProtocolVersion message
Before, receiving a NegotiateProtocolVersion message would result in a
confusing error message like
expected authentication request from server, but received v
This adds proper handling of this protocol message and produces an
on-topic error message from it.
Discussion:
https://www.postgresql.org/message-id/flat/f9c7862f-b864-8ef7-a861-c4638c83e209%40enterprisedb.com
---
src/interfaces/libpq/fe-connect.c | 23 ++++++++++---
src/interfaces/libpq/fe-protocol3.c | 50 +++++++++++++++++++++++++++++
src/interfaces/libpq/libpq-int.h | 1 +
3 files changed, 69 insertions(+), 5 deletions(-)
diff --git a/src/interfaces/libpq/fe-connect.c
b/src/interfaces/libpq/fe-connect.c
index 746e9b4f1efc..1e72eb92b073 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -3246,10 +3246,11 @@ PQconnectPoll(PGconn *conn)
/*
* Validate message type: we expect only an
authentication
- * request or an error here. Anything else
probably means
- * it's not Postgres on the other end at all.
+ * request, NegotiateProtocolVersion, or an
error here.
+ * Anything else probably means it's not
Postgres on the other
+ * end at all.
*/
- if (!(beresp == 'R' || beresp == 'E'))
+ if (!(beresp == 'R' || beresp == 'v' || beresp
== 'E'))
{
appendPQExpBuffer(&conn->errorMessage,
libpq_gettext("expected authentication request from server, but received %c\n"),
@@ -3267,14 +3268,15 @@ PQconnectPoll(PGconn *conn)
/*
* Try to validate message length before using
it.
* Authentication requests can't be very large,
although GSS
- * auth requests may not be that small. Errors
can be a
+ * auth requests may not be that small. Same
for
+ * NegotiateProtocolVersion. Errors can be a
* little larger, but not huge. If we see a
large apparent
* length in an error, it means we're really
talking to a
* pre-3.0-protocol server; cope. (Before
version 14, the
* server also used the old protocol for errors
that happened
* before processing the startup packet.)
*/
- if (beresp == 'R' && (msgLength < 8 ||
msgLength > 2000))
+ if ((beresp == 'R' || beresp == 'v') &&
(msgLength < 8 || msgLength > 2000))
{
appendPQExpBuffer(&conn->errorMessage,
libpq_gettext("expected authentication request from server, but received %c\n"),
@@ -3405,6 +3407,17 @@ PQconnectPoll(PGconn *conn)
goto error_return;
}
+ else if (beresp == 'v')
+ {
+ if
(pqGetNegotiateProtocolVersion3(conn))
+ {
+ /* We'll come back when there
is more data */
+ return PGRES_POLLING_READING;
+ }
+ /* OK, we read the message; mark data
consumed */
+ conn->inStart = conn->inCursor;
+ goto error_return;
+ }
/* It is an authentication request. */
conn->auth_req_received = true;
diff --git a/src/interfaces/libpq/fe-protocol3.c
b/src/interfaces/libpq/fe-protocol3.c
index f001137b7692..7bc81740ab35 100644
--- a/src/interfaces/libpq/fe-protocol3.c
+++ b/src/interfaces/libpq/fe-protocol3.c
@@ -1397,6 +1397,56 @@ reportErrorPosition(PQExpBuffer msg, const char *query,
int loc, int encoding)
}
+/*
+ * Attempt to read a NegotiateProtocolVersion message.
+ * Entry: 'v' message type and length have already been consumed.
+ * Exit: returns 0 if successfully consumed message.
+ * returns EOF if not enough data.
+ */
+int
+pqGetNegotiateProtocolVersion3(PGconn *conn)
+{
+ int tmp;
+ ProtocolVersion their_version;
+ int num;
+ PQExpBufferData buf;
+
+ initPQExpBuffer(&buf);
+ if (pqGetInt(&tmp, 4, conn) != 0)
+ return EOF;
+ their_version = tmp;
+ if (pqGetInt(&num, 4, conn) != 0)
+ return EOF;
+ for (int i = 0; i < num; i++)
+ {
+ if (pqGets(&conn->workBuffer, conn))
+ return EOF;
+ if (buf.len > 0)
+ appendPQExpBufferChar(&buf, ' ');
+ appendPQExpBufferStr(&buf, conn->workBuffer.data);
+ }
+
+ if (their_version < conn->pversion)
+ appendPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("protocol
version not supported by server: client uses %u.%u, server supports up to
%u.%u\n"),
+
PG_PROTOCOL_MAJOR(conn->pversion), PG_PROTOCOL_MINOR(conn->pversion),
+
PG_PROTOCOL_MAJOR(their_version), PG_PROTOCOL_MINOR(their_version));
+ if (num > 0)
+ appendPQExpBuffer(&conn->errorMessage,
+ libpq_ngettext("protocol
extension not supported by server: %s\n",
+
"protocol extensions not supported by server: %s\n", num),
+ buf.data);
+
+ /* neither -- server shouldn't have sent it */
+ if (!(their_version < conn->pversion) && !(num > 0))
+ appendPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("invalid %s
message"), "NegotiateProtocolVersion");
+
+ termPQExpBuffer(&buf);
+ return 0;
+}
+
+
/*
* Attempt to read a ParameterStatus message.
* This is possible in several places, so we break it out as a subroutine.
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index c75ed63a2c62..c59afac7a086 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -685,6 +685,7 @@ extern void pqParseInput3(PGconn *conn);
extern int pqGetErrorNotice3(PGconn *conn, bool isError);
extern void pqBuildErrorMessage3(PQExpBuffer msg, const PGresult *res,
PGVerbosity
verbosity, PGContextVisibility show_context);
+extern int pqGetNegotiateProtocolVersion3(PGconn *conn);
extern int pqGetCopyData3(PGconn *conn, char **buffer, int async);
extern int pqGetline3(PGconn *conn, char *s, int maxlen);
extern int pqGetlineAsync3(PGconn *conn, char *buffer, int bufsize);
--
2.38.1
