pg.ddx.io pgsql-committers@postgresql.org mailing list archive
help / color / mirror / Atom feedpgsql: Refactor libpq state machine for negotiating encryption
8+ messages / 5 participants
[nested] [flat]
Commits from this thread:
2024-04-29 87d2801d4b libpq: Fix error messages when server rejects SSL or GSS
* pgsql: Refactor libpq state machine for negotiating encryption
@ 2024-04-08 01:25 Heikki Linnakangas <heikki.linnakangas@iki.fi>
0 siblings, 2 replies; 8+ messages in thread
From: Heikki Linnakangas @ 2024-04-08 01:25 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
Refactor libpq state machine for negotiating encryption
This fixes the few corner cases noted in commit 705843d294, as shown
by the changes in the test.
Author: Heikki Linnakangas, Matthias van de Meent
Reviewed-by: Jacob Champion
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/05fd30c0e730bd5238f62d2fdfdcfaf28b16b225
Modified Files
--------------
src/interfaces/libpq/fe-connect.c | 427 ++++++++++++---------
src/interfaces/libpq/libpq-int.h | 14 +-
.../libpq_encryption/t/001_negotiate_encryption.pl | 26 +-
3 files changed, 265 insertions(+), 202 deletions(-)
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: Refactor libpq state machine for negotiating encryption
@ 2024-04-10 23:18 Thomas Munro <thomas.munro@gmail.com>
parent: Heikki Linnakangas <heikki.linnakangas@iki.fi>
1 sibling, 1 reply; 8+ messages in thread
From: Thomas Munro @ 2024-04-10 23:18 UTC (permalink / raw)
To: Heikki Linnakangas <heikki.linnakangas@iki.fi>; +Cc: pgsql-committers@lists.postgresql.org
On Mon, Apr 8, 2024 at 1:25 PM Heikki Linnakangas
<heikki.linnakangas@iki.fi> wrote:
> Refactor libpq state machine for negotiating encryption
>
> This fixes the few corner cases noted in commit 705843d294, as shown
> by the changes in the test.
>
> Author: Heikki Linnakangas, Matthias van de Meent
> Reviewed-by: Jacob Champion
If -Dssl=none and -Dgssapi=disabled, compilation of fe-connect.c
fails: call to undeclared function 'encryption_negotiation_failed'. I
didn't look too hard, but maybe ENABLE_GSS and USE_GSS are confused?
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: Refactor libpq state machine for negotiating encryption
@ 2024-04-10 23:25 Tom Lane <tgl@sss.pgh.pa.us>
parent: Thomas Munro <thomas.munro@gmail.com>
0 siblings, 1 reply; 8+ messages in thread
From: Tom Lane @ 2024-04-10 23:25 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: Heikki Linnakangas <heikki.linnakangas@iki.fi>; pgsql-committers@lists.postgresql.org
Thomas Munro <thomas.munro@gmail.com> writes:
> If -Dssl=none and -Dgssapi=disabled, compilation of fe-connect.c
> fails: call to undeclared function 'encryption_negotiation_failed'. I
> didn't look too hard, but maybe ENABLE_GSS and USE_GSS are confused?
For me, configure --with-gssapi fails like that, but the other three
combinations of --with-openssl and --with-gssapi compile OK. I don't
find it terribly surprising that the buildfarm isn't covering that
combination ...
regards, tom lane
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: Refactor libpq state machine for negotiating encryption
@ 2024-04-10 23:33 Thomas Munro <thomas.munro@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 8+ messages in thread
From: Thomas Munro @ 2024-04-10 23:33 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Heikki Linnakangas <heikki.linnakangas@iki.fi>; pgsql-committers@lists.postgresql.org
On Thu, Apr 11, 2024 at 11:25 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Thomas Munro <thomas.munro@gmail.com> writes:
> > If -Dssl=none and -Dgssapi=disabled, compilation of fe-connect.c
> > fails: call to undeclared function 'encryption_negotiation_failed'. I
> > didn't look too hard, but maybe ENABLE_GSS and USE_GSS are confused?
>
> For me, configure --with-gssapi fails like that, but the other three
> combinations of --with-openssl and --with-gssapi compile OK. I don't
> find it terribly surprising that the buildfarm isn't covering that
> combination ...
Oops, right, correction to my report: it is indeed -Dssl=none
-Dgssapi=enabled that is broken, not the other combinations.
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: Refactor libpq state machine for negotiating encryption
@ 2024-04-11 17:07 Heikki Linnakangas <hlinnaka@iki.fi>
parent: Thomas Munro <thomas.munro@gmail.com>
0 siblings, 1 reply; 8+ messages in thread
From: Heikki Linnakangas @ 2024-04-11 17:07 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: Heikki Linnakangas <heikki.linnakangas@iki.fi>; pgsql-committers@lists.postgresql.org, Tom Lane <tgl@sss.pgh.pa.us>
Thanks for the report!
On 11/04/2024 02:33, Thomas Munro wrote:
> On Thu, Apr 11, 2024 at 11:25 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Thomas Munro <thomas.munro@gmail.com> writes:
>>> If -Dssl=none and -Dgssapi=disabled, compilation of fe-connect.c
>>> fails: call to undeclared function 'encryption_negotiation_failed'. I
>>> didn't look too hard, but maybe ENABLE_GSS and USE_GSS are confused?
>>
>> For me, configure --with-gssapi fails like that, but the other three
>> combinations of --with-openssl and --with-gssapi compile OK. I don't
>> find it terribly surprising that the buildfarm isn't covering that
>> combination ...
>
> Oops, right, correction to my report: it is indeed -Dssl=none
> -Dgssapi=enabled that is broken, not the other combinations.
Yes, I misspelled ENABLE_GSS as USE_GSS.
After fixing that, the new tests are failing; the expected output for
many of the cases is different when GSSAPI support is not compiled in. I
think the test tables need to be rearranged some more to take that into
account, or we will end up with a ridiculous amount of different
expected outputs.
I will take a closer look at that tomorrow. As a bandaid fix, we could
temporarily disable the new tests with that combination of configure
options, it's still better test coverage than not having the tests at
all. But given that no buildfarm members are testing that combination I
think it can wait a day.
--
Heikki Linnakangas
Neon (https://neon.tech)
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: Refactor libpq state machine for negotiating encryption
@ 2024-04-12 16:57 Heikki Linnakangas <hlinnaka@iki.fi>
parent: Heikki Linnakangas <hlinnaka@iki.fi>
0 siblings, 0 replies; 8+ messages in thread
From: Heikki Linnakangas @ 2024-04-12 16:57 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: Heikki Linnakangas <heikki.linnakangas@iki.fi>; pgsql-committers@lists.postgresql.org, Tom Lane <tgl@sss.pgh.pa.us>
On 11/04/2024 20:07, Heikki Linnakangas wrote:
> On 11/04/2024 02:33, Thomas Munro wrote:
>> On Thu, Apr 11, 2024 at 11:25 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Thomas Munro <thomas.munro@gmail.com> writes:
>>>> If -Dssl=none and -Dgssapi=disabled, compilation of fe-connect.c
>>>> fails: call to undeclared function 'encryption_negotiation_failed'. I
>>>> didn't look too hard, but maybe ENABLE_GSS and USE_GSS are confused?
>>>
>>> For me, configure --with-gssapi fails like that, but the other three
>>> combinations of --with-openssl and --with-gssapi compile OK. I don't
>>> find it terribly surprising that the buildfarm isn't covering that
>>> combination ...
>>
>> Oops, right, correction to my report: it is indeed -Dssl=none
>> -Dgssapi=enabled that is broken, not the other combinations.
>
> Yes, I misspelled ENABLE_GSS as USE_GSS.
>
> After fixing that, the new tests are failing; the expected output for
> many of the cases is different when GSSAPI support is not compiled in. I
> think the test tables need to be rearranged some more to take that into
> account, or we will end up with a ridiculous amount of different
> expected outputs.
>
> I will take a closer look at that tomorrow. As a bandaid fix, we could
> temporarily disable the new tests with that combination of configure
> options, it's still better test coverage than not having the tests at
> all. But given that no buildfarm members are testing that combination I
> think it can wait a day.
Fixed the compilation with that combination, and the expected test
output. Thanks for the report!
--
Heikki Linnakangas
Neon (https://neon.tech)
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: Refactor libpq state machine for negotiating encryption
@ 2024-04-24 12:57 Peter Eisentraut <peter@eisentraut.org>
parent: Heikki Linnakangas <heikki.linnakangas@iki.fi>
1 sibling, 1 reply; 8+ messages in thread
From: Peter Eisentraut @ 2024-04-24 12:57 UTC (permalink / raw)
To: Heikki Linnakangas <heikki.linnakangas@iki.fi>; pgsql-committers@lists.postgresql.org
On 08.04.24 03:25, Heikki Linnakangas wrote:
> Refactor libpq state machine for negotiating encryption
>
> This fixes the few corner cases noted in commit 705843d294, as shown
> by the changes in the test.
Either this or something nearby appears to have broken the error
reporting from psql or libpq when failing to get an SSL connection:
PG16:
psql 'sslmode=require host=localhost'
psql: error: connection to server at "localhost" (::1), port 65432
failed: server does not support SSL, but SSL was required
master:
psql 'sslmode=require host=localhost'
psql: error: connection to server at "localhost" (::1), port 65432 failed:
(sic, the output ends after the colon).
This commit removed that detail error message string from the code, but
I don't see any similar message that would take its place.
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: Refactor libpq state machine for negotiating encryption
@ 2024-04-25 15:04 Heikki Linnakangas <hlinnaka@iki.fi>
parent: Peter Eisentraut <peter@eisentraut.org>
0 siblings, 0 replies; 8+ messages in thread
From: Heikki Linnakangas @ 2024-04-25 15:04 UTC (permalink / raw)
To: Peter Eisentraut <peter@eisentraut.org>; Heikki Linnakangas <heikki.linnakangas@iki.fi>; pgsql-committers@lists.postgresql.org
On 24/04/2024 15:57, Peter Eisentraut wrote:
> On 08.04.24 03:25, Heikki Linnakangas wrote:
>> Refactor libpq state machine for negotiating encryption
>>
>> This fixes the few corner cases noted in commit 705843d294, as shown
>> by the changes in the test.
>
> Either this or something nearby appears to have broken the error
> reporting from psql or libpq when failing to get an SSL connection:
>
> PG16:
>
> psql 'sslmode=require host=localhost'
> psql: error: connection to server at "localhost" (::1), port 65432
> failed: server does not support SSL, but SSL was required
>
> master:
>
> psql 'sslmode=require host=localhost'
> psql: error: connection to server at "localhost" (::1), port 65432 failed:
>
> (sic, the output ends after the colon).
>
> This commit removed that detail error message string from the code, but
> I don't see any similar message that would take its place.
Thanks, attached patch puts back those messages.
This makes one change in the error message behavior compared to v16, in
the case that the server responds to GSSRequest with an error instead of
rejecting it with 'N'. Previously, libpq would hide the error that the
server sent in that case, assuming that it was because the server is an
old pre-v12 version that doesn't support GSS and doesn't understand the
GSSRequest message. A v11 server responds with a "FATAL: unsupported
frontend protocol 1234.5680: server supports 2.0 to 3.0" error if you
try to connect to it with GSS. That was a reasonable assumption when the
feature was introduced, but v12 was released a long time ago and I don't
think it's the most probable cause anymore. The attached patch changes
things so that libpq prints the error message that the server sent in
that case, making the "server responds with error to GSSRequest" case
behave the same as the "server responds with error to SSLRequest" case.
We could keep the old behavior if we wanted to, but this is the most
convenient way to handle this in the new libpq code, and makes sense
anyway IMHO.
While testing this some more, I noticed this existing case with stable
versions:
psql "host=enc-test-localhost.postgresql.example.com hostaddr=127.0.0.1
sslmode=disable gssencmode=require"
psql: error: connection to server at "127.0.0.1", port 5432 failed:
In the server log:
024-04-25 16:38:59.372 EEST [3939163] LOG: could not accept GSSAPI
security context
2024-04-25 16:38:59.372 EEST [3939163] DETAIL: No credentials were
supplied, or the credentials were unavailable or inaccessible: Key table
entry not found
I didn't have everything configured correctly, which is why it failed.
That's fine. But the psql error message is missing here too.
The attached patch does not fix that case. I think the correct fix would
go somewhere in pqsecure_open_gss(). Whenever pqsecure_open_gss()
returns PGRES_POLLING_FAILED, it should also put an error in the error
buffer with libpq_append_conn_error(). The corresponding
pgtls_open_client() function for TLS does that, and so does
pqsecure_open_gss() for many other cases, but apparently not that one.
--
Heikki Linnakangas
Neon (https://neon.tech)
Attachments:
0001-libpq-Fix-error-messages-when-server-rejects-SSL-or-.patch (text/x-patch, 4.8K) download
From fd3261a5b78dae6bcfdafe86d14736da43befbbe Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Thu, 25 Apr 2024 14:53:29 +0300
Subject: [PATCH 1/1] libpq: Fix error messages when server rejects SSL or GSS
These messages were lost in commit 05fd30c0e7. Put them back.
This makes one change in the error message behavior compared to v16,
in the case that the server responds to GSSRequest with an error
instead of rejecting it with 'N'. Previously, libpq would hide the
error that the server sent, assuming that you got the error because
the server is an old pre-v12 version that doesn't support GSS and
doesn't understand the GSSRequest message. A v11 server sends a
"FATAL: unsupported frontend protocol 1234.5680: server supports 2.0
to 3.0" error if you try to connect to it with GSS. That was a
reasonable assumption when the feature was introduced, but v12 was
released a long time ago and I don't think it's the most probable
cause anymore. The attached patch changes things so that libpq prints
the error message that the server sent in that case, making the
"server responds with error to GSSRequest" case behave the same as the
"server responds with error to SSLRequest" case.
Reported-by: Peter Eisentraut
Discussion: https://www.postgresql.org/message-id/bb3b94da-afc7-438d-8940-cb946e553d9d@eisentraut.org
---
src/interfaces/libpq/fe-connect.c | 49 ++++++++++++++++++++++---------
1 file changed, 35 insertions(+), 14 deletions(-)
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index ec20e3f3a9..4a4d0b9e80 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -2861,12 +2861,17 @@ keep_going: /* We will come back to here until there is
need_new_connection = false;
}
- /* Decide what to do next, if SSL or GSS negotiation fails */
-#define ENCRYPTION_NEGOTIATION_FAILED() \
+ /*
+ * Decide what to do next, if server rejects SSL or GSS negotiation, but
+ * the connection is still valid. If there are no options left, error out
+ * with 'msg'.
+ */
+#define ENCRYPTION_NEGOTIATION_FAILED(msg) \
do { \
switch (encryption_negotiation_failed(conn)) \
{ \
case 0: \
+ libpq_append_conn_error(conn, (msg)); \
goto error_return; \
case 1: \
conn->status = CONNECTION_MADE; \
@@ -2877,7 +2882,11 @@ keep_going: /* We will come back to here until there is
} \
} while(0);
- /* Decide what to do next, if connection fails */
+ /*
+ * Decide what to do next, if connection fails. If there are no options
+ * left, return with an error. The error message has already been written
+ * to the connection's error buffer.
+ */
#define CONNECTION_FAILED() \
do { \
if (connection_failed(conn)) \
@@ -3483,9 +3492,13 @@ keep_going: /* We will come back to here until there is
{
/* mark byte consumed */
conn->inStart = conn->inCursor;
- /* OK to do without SSL? */
- /* We can proceed using this connection */
- ENCRYPTION_NEGOTIATION_FAILED();
+
+ /*
+ * The connection is still valid, so if it's OK to
+ * continue without SSL, we can proceed using this
+ * connection. Otherwise return with an error.
+ */
+ ENCRYPTION_NEGOTIATION_FAILED("server does not support SSL, but SSL was required");
}
else if (SSLok == 'E')
{
@@ -3583,13 +3596,17 @@ keep_going: /* We will come back to here until there is
if (gss_ok == 'E')
{
/*
- * Server failure of some sort. Assume it's a
- * protocol version support failure, and let's see if
- * we can't recover (if it's not, we'll get a better
- * error message on retry). Server gets fussy if we
- * don't hang up the socket, though.
+ * Server failure of some sort, possibly protocol
+ * version support failure. We need to process and
+ * report the error message, which might be formatted
+ * according to either protocol 2 or protocol 3.
+ * Rather than duplicate the code for that, we flip
+ * into AWAITING_RESPONSE state and let the code there
+ * deal with it. Note we have *not* consumed the "E"
+ * byte here.
*/
- CONNECTION_FAILED();
+ conn->status = CONNECTION_AWAITING_RESPONSE;
+ goto keep_going;
}
/* mark byte consumed */
@@ -3597,8 +3614,12 @@ keep_going: /* We will come back to here until there is
if (gss_ok == 'N')
{
- /* We can proceed using this connection */
- ENCRYPTION_NEGOTIATION_FAILED();
+ /*
+ * The connection is still valid, so if it's OK to
+ * continue without GSS, we can proceed using this
+ * connection. Otherwise return with an error.
+ */
+ ENCRYPTION_NEGOTIATION_FAILED("server doesn't support GSSAPI encryption, but it was required");
}
else if (gss_ok != 'G')
{
--
2.39.2
end of thread, other threads:[~2024-04-25 15:04 UTC | newest]
Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-04-08 01:25 pgsql: Refactor libpq state machine for negotiating encryption Heikki Linnakangas <heikki.linnakangas@iki.fi>
2024-04-10 23:18 ` Thomas Munro <thomas.munro@gmail.com>
2024-04-10 23:25 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-04-10 23:33 ` Thomas Munro <thomas.munro@gmail.com>
2024-04-11 17:07 ` Heikki Linnakangas <hlinnaka@iki.fi>
2024-04-12 16:57 ` Heikki Linnakangas <hlinnaka@iki.fi>
2024-04-24 12:57 ` Peter Eisentraut <peter@eisentraut.org>
2024-04-25 15:04 ` Heikki Linnakangas <hlinnaka@iki.fi>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox