Content-Type: multipart/mixed; boundary="------------0J36WaasuVmWBiW7v0RjdTrn"
Message-ID: <4c1da310-0f48-4af4-88b5-e64889ecc0b9@iki.fi>
Date: Thu, 25 Apr 2024 18:04:05 +0300
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: pgsql: Refactor libpq state machine for negotiating encryption
To: Peter Eisentraut <peter@eisentraut.org>,
 Heikki Linnakangas <heikki.linnakangas@iki.fi>,
 pgsql-committers@lists.postgresql.org
References: <E1rtdlT-001383-HT@gemulon.postgresql.org>
 <bb3b94da-afc7-438d-8940-cb946e553d9d@eisentraut.org>
Content-Language: en-US
From: Heikki Linnakangas <hlinnaka@iki.fi>
In-Reply-To: <bb3b94da-afc7-438d-8940-cb946e553d9d@eisentraut.org>
Archived-At: 
 <https://www.postgresql.org/message-id/4c1da310-0f48-4af4-88b5-e64889ecc0b9%40iki.fi>
Precedence: bulk

This is a multi-part message in MIME format.
--------------0J36WaasuVmWBiW7v0RjdTrn
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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)

--------------0J36WaasuVmWBiW7v0RjdTrn
Content-Type: text/x-patch; charset=UTF-8;
 name="0001-libpq-Fix-error-messages-when-server-rejects-SSL-or-.patch"
Content-Disposition: attachment;
 filename*0="0001-libpq-Fix-error-messages-when-server-rejects-SSL-or-.pa";
 filename*1="tch"
Content-Transfer-Encoding: base64

RnJvbSBmZDMyNjFhNWI3OGRhZTZiY2ZkYWZlODZkMTQ3MzZkYTQzYmVmYmJlIE1vbiBTZXAg
MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBIZWlra2kgTGlubmFrYW5nYXMgPGhlaWtraS5saW5u
YWthbmdhc0Bpa2kuZmk+CkRhdGU6IFRodSwgMjUgQXByIDIwMjQgMTQ6NTM6MjkgKzAzMDAK
U3ViamVjdDogW1BBVENIIDEvMV0gbGlicHE6IEZpeCBlcnJvciBtZXNzYWdlcyB3aGVuIHNl
cnZlciByZWplY3RzIFNTTCBvciBHU1MKClRoZXNlIG1lc3NhZ2VzIHdlcmUgbG9zdCBpbiBj
b21taXQgMDVmZDMwYzBlNy4gUHV0IHRoZW0gYmFjay4KClRoaXMgbWFrZXMgb25lIGNoYW5n
ZSBpbiB0aGUgZXJyb3IgbWVzc2FnZSBiZWhhdmlvciBjb21wYXJlZCB0byB2MTYsCmluIHRo
ZSBjYXNlIHRoYXQgdGhlIHNlcnZlciByZXNwb25kcyB0byBHU1NSZXF1ZXN0IHdpdGggYW4g
ZXJyb3IKaW5zdGVhZCBvZiByZWplY3RpbmcgaXQgd2l0aCAnTicuIFByZXZpb3VzbHksIGxp
YnBxIHdvdWxkIGhpZGUgdGhlCmVycm9yIHRoYXQgdGhlIHNlcnZlciBzZW50LCBhc3N1bWlu
ZyB0aGF0IHlvdSBnb3QgdGhlIGVycm9yIGJlY2F1c2UKdGhlIHNlcnZlciBpcyBhbiBvbGQg
cHJlLXYxMiB2ZXJzaW9uIHRoYXQgZG9lc24ndCBzdXBwb3J0IEdTUyBhbmQKZG9lc24ndCB1
bmRlcnN0YW5kIHRoZSBHU1NSZXF1ZXN0IG1lc3NhZ2UuIEEgdjExIHNlcnZlciBzZW5kcyBh
CiJGQVRBTDogdW5zdXBwb3J0ZWQgZnJvbnRlbmQgcHJvdG9jb2wgMTIzNC41NjgwOiBzZXJ2
ZXIgc3VwcG9ydHMgMi4wCnRvIDMuMCIgZXJyb3IgaWYgeW91IHRyeSB0byBjb25uZWN0IHRv
IGl0IHdpdGggR1NTLiBUaGF0IHdhcyBhCnJlYXNvbmFibGUgYXNzdW1wdGlvbiB3aGVuIHRo
ZSBmZWF0dXJlIHdhcyBpbnRyb2R1Y2VkLCBidXQgdjEyIHdhcwpyZWxlYXNlZCBhIGxvbmcg
dGltZSBhZ28gYW5kIEkgZG9uJ3QgdGhpbmsgaXQncyB0aGUgbW9zdCBwcm9iYWJsZQpjYXVz
ZSBhbnltb3JlLiBUaGUgYXR0YWNoZWQgcGF0Y2ggY2hhbmdlcyB0aGluZ3Mgc28gdGhhdCBs
aWJwcSBwcmludHMKdGhlIGVycm9yIG1lc3NhZ2UgdGhhdCB0aGUgc2VydmVyIHNlbnQgaW4g
dGhhdCBjYXNlLCBtYWtpbmcgdGhlCiJzZXJ2ZXIgcmVzcG9uZHMgd2l0aCBlcnJvciB0byBH
U1NSZXF1ZXN0IiBjYXNlIGJlaGF2ZSB0aGUgc2FtZSBhcyB0aGUKInNlcnZlciByZXNwb25k
cyB3aXRoIGVycm9yIHRvIFNTTFJlcXVlc3QiIGNhc2UuCgpSZXBvcnRlZC1ieTogUGV0ZXIg
RWlzZW50cmF1dApEaXNjdXNzaW9uOiBodHRwczovL3d3dy5wb3N0Z3Jlc3FsLm9yZy9tZXNz
YWdlLWlkL2JiM2I5NGRhLWFmYzctNDM4ZC04OTQwLWNiOTQ2ZTU1M2Q5ZEBlaXNlbnRyYXV0
Lm9yZwotLS0KIHNyYy9pbnRlcmZhY2VzL2xpYnBxL2ZlLWNvbm5lY3QuYyB8IDQ5ICsrKysr
KysrKysrKysrKysrKysrKystLS0tLS0tLS0KIDEgZmlsZSBjaGFuZ2VkLCAzNSBpbnNlcnRp
b25zKCspLCAxNCBkZWxldGlvbnMoLSkKCmRpZmYgLS1naXQgYS9zcmMvaW50ZXJmYWNlcy9s
aWJwcS9mZS1jb25uZWN0LmMgYi9zcmMvaW50ZXJmYWNlcy9saWJwcS9mZS1jb25uZWN0LmMK
aW5kZXggZWMyMGUzZjNhOS4uNGE0ZDBiOWU4MCAxMDA2NDQKLS0tIGEvc3JjL2ludGVyZmFj
ZXMvbGlicHEvZmUtY29ubmVjdC5jCisrKyBiL3NyYy9pbnRlcmZhY2VzL2xpYnBxL2ZlLWNv
bm5lY3QuYwpAQCAtMjg2MSwxMiArMjg2MSwxNyBAQCBrZWVwX2dvaW5nOgkJCQkJCS8qIFdl
IHdpbGwgY29tZSBiYWNrIHRvIGhlcmUgdW50aWwgdGhlcmUgaXMKIAkJbmVlZF9uZXdfY29u
bmVjdGlvbiA9IGZhbHNlOwogCX0KIAotCS8qIERlY2lkZSB3aGF0IHRvIGRvIG5leHQsIGlm
IFNTTCBvciBHU1MgbmVnb3RpYXRpb24gZmFpbHMgKi8KLSNkZWZpbmUgRU5DUllQVElPTl9O
RUdPVElBVElPTl9GQUlMRUQoKSBcCisJLyoKKwkgKiBEZWNpZGUgd2hhdCB0byBkbyBuZXh0
LCBpZiBzZXJ2ZXIgcmVqZWN0cyBTU0wgb3IgR1NTIG5lZ290aWF0aW9uLCBidXQKKwkgKiB0
aGUgY29ubmVjdGlvbiBpcyBzdGlsbCB2YWxpZC4gSWYgdGhlcmUgYXJlIG5vIG9wdGlvbnMg
bGVmdCwgZXJyb3Igb3V0CisJICogd2l0aCAnbXNnJy4KKwkgKi8KKyNkZWZpbmUgRU5DUllQ
VElPTl9ORUdPVElBVElPTl9GQUlMRUQobXNnKSBcCiAJZG8geyBcCiAJCXN3aXRjaCAoZW5j
cnlwdGlvbl9uZWdvdGlhdGlvbl9mYWlsZWQoY29ubikpIFwKIAkJeyBcCiAJCQljYXNlIDA6
IFwKKwkJCQlsaWJwcV9hcHBlbmRfY29ubl9lcnJvcihjb25uLCAobXNnKSk7IFwKIAkJCQln
b3RvIGVycm9yX3JldHVybjsgXAogCQkJY2FzZSAxOiBcCiAJCQkJY29ubi0+c3RhdHVzID0g
Q09OTkVDVElPTl9NQURFOyBcCkBAIC0yODc3LDcgKzI4ODIsMTEgQEAga2VlcF9nb2luZzoJ
CQkJCQkvKiBXZSB3aWxsIGNvbWUgYmFjayB0byBoZXJlIHVudGlsIHRoZXJlIGlzCiAJCX0g
XAogCX0gd2hpbGUoMCk7CiAKLQkvKiBEZWNpZGUgd2hhdCB0byBkbyBuZXh0LCBpZiBjb25u
ZWN0aW9uIGZhaWxzICovCisJLyoKKwkgKiBEZWNpZGUgd2hhdCB0byBkbyBuZXh0LCBpZiBj
b25uZWN0aW9uIGZhaWxzLiBJZiB0aGVyZSBhcmUgbm8gb3B0aW9ucworCSAqIGxlZnQsIHJl
dHVybiB3aXRoIGFuIGVycm9yLiBUaGUgZXJyb3IgbWVzc2FnZSBoYXMgYWxyZWFkeSBiZWVu
IHdyaXR0ZW4KKwkgKiB0byB0aGUgY29ubmVjdGlvbidzIGVycm9yIGJ1ZmZlci4KKwkgKi8K
ICNkZWZpbmUgQ09OTkVDVElPTl9GQUlMRUQoKSBcCiAJZG8geyBcCiAJCWlmIChjb25uZWN0
aW9uX2ZhaWxlZChjb25uKSkgXApAQCAtMzQ4Myw5ICszNDkyLDEzIEBAIGtlZXBfZ29pbmc6
CQkJCQkJLyogV2Ugd2lsbCBjb21lIGJhY2sgdG8gaGVyZSB1bnRpbCB0aGVyZSBpcwogCQkJ
CQl7CiAJCQkJCQkvKiBtYXJrIGJ5dGUgY29uc3VtZWQgKi8KIAkJCQkJCWNvbm4tPmluU3Rh
cnQgPSBjb25uLT5pbkN1cnNvcjsKLQkJCQkJCS8qIE9LIHRvIGRvIHdpdGhvdXQgU1NMPyAq
LwotCQkJCQkJLyogV2UgY2FuIHByb2NlZWQgdXNpbmcgdGhpcyBjb25uZWN0aW9uICovCi0J
CQkJCQlFTkNSWVBUSU9OX05FR09USUFUSU9OX0ZBSUxFRCgpOworCisJCQkJCQkvKgorCQkJ
CQkJICogVGhlIGNvbm5lY3Rpb24gaXMgc3RpbGwgdmFsaWQsIHNvIGlmIGl0J3MgT0sgdG8K
KwkJCQkJCSAqIGNvbnRpbnVlIHdpdGhvdXQgU1NMLCB3ZSBjYW4gcHJvY2VlZCB1c2luZyB0
aGlzCisJCQkJCQkgKiBjb25uZWN0aW9uLiBPdGhlcndpc2UgcmV0dXJuIHdpdGggYW4gZXJy
b3IuCisJCQkJCQkgKi8KKwkJCQkJCUVOQ1JZUFRJT05fTkVHT1RJQVRJT05fRkFJTEVEKCJz
ZXJ2ZXIgZG9lcyBub3Qgc3VwcG9ydCBTU0wsIGJ1dCBTU0wgd2FzIHJlcXVpcmVkIik7CiAJ
CQkJCX0KIAkJCQkJZWxzZSBpZiAoU1NMb2sgPT0gJ0UnKQogCQkJCQl7CkBAIC0zNTgzLDEz
ICszNTk2LDE3IEBAIGtlZXBfZ29pbmc6CQkJCQkJLyogV2Ugd2lsbCBjb21lIGJhY2sgdG8g
aGVyZSB1bnRpbCB0aGVyZSBpcwogCQkJCQlpZiAoZ3NzX29rID09ICdFJykKIAkJCQkJewog
CQkJCQkJLyoKLQkJCQkJCSAqIFNlcnZlciBmYWlsdXJlIG9mIHNvbWUgc29ydC4gIEFzc3Vt
ZSBpdCdzIGEKLQkJCQkJCSAqIHByb3RvY29sIHZlcnNpb24gc3VwcG9ydCBmYWlsdXJlLCBh
bmQgbGV0J3Mgc2VlIGlmCi0JCQkJCQkgKiB3ZSBjYW4ndCByZWNvdmVyIChpZiBpdCdzIG5v
dCwgd2UnbGwgZ2V0IGEgYmV0dGVyCi0JCQkJCQkgKiBlcnJvciBtZXNzYWdlIG9uIHJldHJ5
KS4gIFNlcnZlciBnZXRzIGZ1c3N5IGlmIHdlCi0JCQkJCQkgKiBkb24ndCBoYW5nIHVwIHRo
ZSBzb2NrZXQsIHRob3VnaC4KKwkJCQkJCSAqIFNlcnZlciBmYWlsdXJlIG9mIHNvbWUgc29y
dCwgcG9zc2libHkgcHJvdG9jb2wKKwkJCQkJCSAqIHZlcnNpb24gc3VwcG9ydCBmYWlsdXJl
LiAgV2UgbmVlZCB0byBwcm9jZXNzIGFuZAorCQkJCQkJICogcmVwb3J0IHRoZSBlcnJvciBt
ZXNzYWdlLCB3aGljaCBtaWdodCBiZSBmb3JtYXR0ZWQKKwkJCQkJCSAqIGFjY29yZGluZyB0
byBlaXRoZXIgcHJvdG9jb2wgMiBvciBwcm90b2NvbCAzLgorCQkJCQkJICogUmF0aGVyIHRo
YW4gZHVwbGljYXRlIHRoZSBjb2RlIGZvciB0aGF0LCB3ZSBmbGlwCisJCQkJCQkgKiBpbnRv
IEFXQUlUSU5HX1JFU1BPTlNFIHN0YXRlIGFuZCBsZXQgdGhlIGNvZGUgdGhlcmUKKwkJCQkJ
CSAqIGRlYWwgd2l0aCBpdC4gIE5vdGUgd2UgaGF2ZSAqbm90KiBjb25zdW1lZCB0aGUgIkUi
CisJCQkJCQkgKiBieXRlIGhlcmUuCiAJCQkJCQkgKi8KLQkJCQkJCUNPTk5FQ1RJT05fRkFJ
TEVEKCk7CisJCQkJCQljb25uLT5zdGF0dXMgPSBDT05ORUNUSU9OX0FXQUlUSU5HX1JFU1BP
TlNFOworCQkJCQkJZ290byBrZWVwX2dvaW5nOwogCQkJCQl9CiAKIAkJCQkJLyogbWFyayBi
eXRlIGNvbnN1bWVkICovCkBAIC0zNTk3LDggKzM2MTQsMTIgQEAga2VlcF9nb2luZzoJCQkJ
CQkvKiBXZSB3aWxsIGNvbWUgYmFjayB0byBoZXJlIHVudGlsIHRoZXJlIGlzCiAKIAkJCQkJ
aWYgKGdzc19vayA9PSAnTicpCiAJCQkJCXsKLQkJCQkJCS8qIFdlIGNhbiBwcm9jZWVkIHVz
aW5nIHRoaXMgY29ubmVjdGlvbiAqLwotCQkJCQkJRU5DUllQVElPTl9ORUdPVElBVElPTl9G
QUlMRUQoKTsKKwkJCQkJCS8qCisJCQkJCQkgKiBUaGUgY29ubmVjdGlvbiBpcyBzdGlsbCB2
YWxpZCwgc28gaWYgaXQncyBPSyB0bworCQkJCQkJICogY29udGludWUgd2l0aG91dCBHU1Ms
IHdlIGNhbiBwcm9jZWVkIHVzaW5nIHRoaXMKKwkJCQkJCSAqIGNvbm5lY3Rpb24uIE90aGVy
d2lzZSByZXR1cm4gd2l0aCBhbiBlcnJvci4KKwkJCQkJCSAqLworCQkJCQkJRU5DUllQVElP
Tl9ORUdPVElBVElPTl9GQUlMRUQoInNlcnZlciBkb2Vzbid0IHN1cHBvcnQgR1NTQVBJIGVu
Y3J5cHRpb24sIGJ1dCBpdCB3YXMgcmVxdWlyZWQiKTsKIAkJCQkJfQogCQkJCQllbHNlIGlm
IChnc3Nfb2sgIT0gJ0cnKQogCQkJCQl7Ci0tIAoyLjM5LjIKCg==

--------------0J36WaasuVmWBiW7v0RjdTrn--