MIME-Version: 1.0
References: <1C81CD0D-407E-44F9-833A-DD0331C202E5@yesql.se>
 <CAOYmi+k_YBsO3jnxx9HBcChNzkzRW=Erm4yiPGsKV2_6rU+-4g@mail.gmail.com>
 <88986722-5A72-4DEC-8750-BDBF67FF8C01@yesql.se> <CAOYmi+nYV6Rr9BY4YfYyVdiQ5TzMZray6QPXwiO3pYSaow+-Tg@mail.gmail.com>
 <7E77028B-5A3A-436B-9046-8E9992E9F94A@yesql.se> <Z1jeINLtB05YMVOY@paquier.xyz>
 <E51258CD-C7B2-4BB1-AB9C-A3DD2B6592D0@yesql.se> <F4222274-15C0-40E4-84FB-8FC1355BB8B2@yesql.se>
 <CAOYmi+mSrV8hRaQkvGDf1Df4cmpv5SeTbTxppyxeonMe6MW8nA@mail.gmail.com> <0BC5B9B1-6503-4563-AAC6-33DEF264AE3F@yesql.se>
In-Reply-To: <0BC5B9B1-6503-4563-AAC6-33DEF264AE3F@yesql.se>
From: Jacob Champion <jacob.champion@enterprisedb.com>
Date: Tue, 4 Mar 2025 13:57:09 -0800
Message-ID: <CAOYmi+k=VF-2BCqfR49A92tx=_QNuL=3iT3w6FysOffKw9cxDQ@mail.gmail.com>
Subject: Re: Serverside SNI support in libpq
To: Daniel Gustafsson <daniel@yesql.se>
Cc: Michael Paquier <michael@paquier.xyz>, Pgsql Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: multipart/mixed; boundary="00000000000063b074062f8b5bb5"
Archived-At: <https://www.postgresql.org/message-id/CAOYmi%2Bk%3DVF-2BCqfR49A92tx%3D_QNuL%3D3iT3w6FysOffKw9cxDQ%40mail.gmail.com>
Precedence: bulk

--00000000000063b074062f8b5bb5
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Feb 27, 2025 at 5:38=E2=80=AFAM Daniel Gustafsson <daniel@yesql.se>=
 wrote:
> Thanks for the tests, they did in fact uncover a bug in how fallback was
> handled which is now fixed.  In doing so I revamped how the default conte=
xt
> handling is done, it now always use the GUCs in postgresql.conf for
> consistency.  The attached v6 rebase contains this as well as your tests =
as
> well as general cleanup and comment writing etc.

Great, thanks!

Revisiting my concerns from upthread:

On Thu, Jul 25, 2024 at 10:51=E2=80=AFAM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> I tried patching all that, but I continue to see nondeterministic
> behavior, including the wrong certificate chain occasionally being
> served, and the servername callback being called twice for each
> connection (?!).

1) The wrong chain being served was due to the fallback bug, now fixed.
2) The servername callback happening twice is due to the TLS 1.3
HelloRetryRequest problem with our ssl_groups (which reminded me to
ping that thread [1]). Switching to TLSv1.2 in order to more easily
see the handshake on the wire makes the problem go away, which
probably did not help my sense of growing insanity last July.

>     https://github.com/openssl/openssl/issues/6109
>
> Matt Caswell appears to be convinced that SSL_set_SSL_CTX() is
> fundamentally broken.

We briefly talked about this in Brussels, and I've been trying to find
proof. Attached are some (very rough) tests that might highlight an
issue.

Basically, the new tests set up three hosts in pg_hosts.conf: one with
no client CA, one with a valid client CA, and one with a
malfunctioning CA (root+server_ca, which can't verify our client
certs). Then it switches out the default CA underneath to make sure it
does not affect the visible behavior, since that CA should not
actually be used in the end.

Unfortunately, the failure modes change depending on the default CA.
If it's not a bug in my tests, I think this may be an indication that
SSL_set_SSL_CTX() isn't fully switching out the client verification
behavior? For example, if the default CA isn't set, the other hosts
don't appear to ask for a client certificate even if they need one.
And vice versa.

--

> +           /*
> +            * Set flag to remember whether CA store has been loaded into=
 this
> +            * SSL_context.
> +            */
> +           if (host->ssl_ca)

I think this should be `if (host->ssl_ca[0])`  -- which, incidentally,
fixes one of the new failing tests on my machine.

>  int
>  be_tls_init(bool isServerStart)
> +{
> +   SSL_CTX    *ctx;
> +   List       *sni_hosts =3D NIL;
> +   HostsLine   line;

A pointer to `line` is passed down to ssl_init_context(), but it's
only been partially initialized on the stack. Can it be
zero-initialized here instead?

> +       if (ssl_snimode =3D=3D SSL_SNIMODE_STRICT)
> +       {
> +           ereport(COMMERROR,
> +                   (errcode(ERRCODE_PROTOCOL_VIOLATION),
> +                    errmsg("no hostname provided in callback")));
> +           return SSL_TLSEXT_ERR_ALERT_FATAL;
> +       }

At the moment we're sending an `unrecognized_name` alert in strict
mode if the client doesn't send SNI. RFC 8446 suggests
`missing_extension`:

   Additionally, all implementations MUST support the use of the
   "server_name" extension with applications capable of using it.
   Servers MAY require clients to send a valid "server_name" extension.
   Servers requiring this extension SHOULD respond to a ClientHello
   lacking a "server_name" extension by terminating the connection with
   a "missing_extension" alert.

Should we do that, or should we ignore the suggestion? The problem
with missing_extension, IMO, is that there's absolutely no indication
to the client as to which extension is missing. unrecognized_name is a
little confusing in this case (there was no name sent), but at least
the end user will be able to link that to an SNI problem via search
engine.

> +#hosts_file =3D 'ConfigDir/pg_hosts.conf' # hosts configuration file
> +                   # (change requires restart)

Nitpickiest nitpick: looks like the other lines use a tab instead of a
space between the setting and the trailing comment.

Thanks,
--Jacob

[1] https://postgr.es/m/CAOYmi%2BnTwu7%3DaUGCkf6L-ULqS8itNP7uc9nUmNLOvbXf2T=
CgBA%40mail.gmail.com

--00000000000063b074062f8b5bb5
Content-Type: text/plain; charset="US-ASCII"; name="tests.patch.txt"
Content-Disposition: attachment; filename="tests.patch.txt"
Content-Transfer-Encoding: base64
Content-ID: <f_m7v0je1p0>
X-Attachment-Id: f_m7v0je1p0

Y29tbWl0IGE0ZDljYmY0ZDEyMjhkY2MxN2YyOTYxYjc4MTEzMjFhNTBlNzQ2MTcKQXV0aG9yOiBK
YWNvYiBDaGFtcGlvbiA8amFjb2IuY2hhbXBpb25AZW50ZXJwcmlzZWRiLmNvbT4KRGF0ZTogICBU
dWUgTWFyIDQgMTM6MTc6MTIgMjAyNSAtMDgwMAoKICAgIFRlc3RzCgpkaWZmIC0tZ2l0IGEvc3Jj
L3Rlc3Qvc3NsL3QvMDA0X3NuaS5wbCBiL3NyYy90ZXN0L3NzbC90LzAwNF9zbmkucGwKaW5kZXgg
ZjBjZTA0ODI3M2EuLjcyZTY0YzZjMDBkIDEwMDY0NAotLS0gYS9zcmMvdGVzdC9zc2wvdC8wMDRf
c25pLnBsCisrKyBiL3NyYy90ZXN0L3NzbC90LzAwNF9zbmkucGwKQEAgLTMzLDYgKzMzLDExIEBA
IGlmICghJEVOVntQR19URVNUX0VYVFJBfSB8fCAkRU5We1BHX1RFU1RfRVhUUkF9ICF+IC9cYnNz
bFxiLykKIAogbXkgJHNzbF9zZXJ2ZXIgPSBTU0w6OlNlcnZlci0+bmV3KCk7CiAKK3N1YiBzc2xr
ZXkKK3sKKwlyZXR1cm4gJHNzbF9zZXJ2ZXItPnNzbGtleShAXyk7Cit9CisKIG15ICRub2RlID0g
UG9zdGdyZVNRTDo6VGVzdDo6Q2x1c3Rlci0+bmV3KCdwcmltYXJ5Jyk7CiAkbm9kZS0+aW5pdDsK
IApAQCAtMTYxLDQgKzE2Niw1NyBAQCAkbm9kZS0+Y29ubmVjdF9mYWlscygKIAkiY29ubmVjdCBm
YWlscyBzaW5jZSB0aGUgcGFzc3BocmFzZSBwcm90ZWN0ZWQga2V5IGNhbm5vdCBiZSByZWxvYWRl
ZCIsCiAJZXhwZWN0ZWRfc3RkZXJyID0+IHFyL3Rsc3YxIHVucmVjb2duaXplZCBuYW1lLyk7CiAK
KyMgVGVzdCBjbGllbnQgQ0FzLgorCiskY29ubnN0ciA9CisgICJ1c2VyPXNzbHRlc3R1c2VyIGRi
bmFtZT1jZXJ0ZGIgaG9zdGFkZHI9JFNFUlZFUkhPU1RBRERSIHNzbG1vZGU9cmVxdWlyZSBzc2xz
bmk9MSI7CisKK29rKHVubGluaygkbm9kZS0+ZGF0YV9kaXIgLiAnL3BnX2hvc3RzLmNvbmYnKSk7
CisjIGV4YW1wbGUub3JnIGhhcyBhbiB1bmNvbmZpZ3VyZWQgQ0EuCiskbm9kZS0+YXBwZW5kX2Nv
bmYoJ3BnX2hvc3RzLmNvbmYnLCAnZXhhbXBsZS5vcmcgc2VydmVyLWNuLW9ubHkuY3J0IHNlcnZl
ci1jbi1vbmx5LmtleSAiIicpOworIyBleGFtcGxlLmNvbSB1c2VzIHRoZSBjbGllbnQgQ0EuCisk
bm9kZS0+YXBwZW5kX2NvbmYoJ3BnX2hvc3RzLmNvbmYnLCAnZXhhbXBsZS5jb20gc2VydmVyLWNu
LW9ubHkuY3J0IHNlcnZlci1jbi1vbmx5LmtleSByb290K2NsaWVudF9jYS5jcnQnKTsKKyMgZXhh
bXBsZS5uZXQgdXNlcyB0aGUgc2VydmVyIENBICh3aGljaCBpcyB3cm9uZykuCiskbm9kZS0+YXBw
ZW5kX2NvbmYoJ3BnX2hvc3RzLmNvbmYnLCAnZXhhbXBsZS5uZXQgc2VydmVyLWNuLW9ubHkuY3J0
IHNlcnZlci1jbi1vbmx5LmtleSByb290K3NlcnZlcl9jYS5jcnQnKTsKKyRub2RlLT5yZWxvYWQ7
CisKK215IEBjYXNlcyA9ICggIiIsICJyb290K2NsaWVudF9jYSIsICJyb290K3NlcnZlcl9jYSIg
KTsKK2ZvcmVhY2ggbXkgJGRlZmF1bHRfY2EgKEBjYXNlcykKK3sKKwkjIFRoZSBkZWZhdWx0IENB
IHNob3VsZCwgaWRlYWxseSwgbm90IG1hdHRlciBmb3IgdGhlIHB1cnBvc2VzIG9mIHRoZXNlCisJ
IyB0ZXN0cywgc2luY2Ugd2UgY29ubmVjdCB0byB0aGUgb3RoZXIgaG9zdHMgZXhwbGljaXRseS4K
Kwkkc3NsX3NlcnZlci0+c3dpdGNoX3NlcnZlcl9jZXJ0KAorCQkkbm9kZSwKKwkJY2VydGZpbGUg
PT4gJ3NlcnZlci1jbi1vbmx5JywKKwkJY2FmaWxlID0+ICRkZWZhdWx0X2NhKTsKKworCSMgZXhh
bXBsZS5vcmcgaXMgdW5jb25maWd1cmVkIGFuZCBzaG91bGQgZmFpbC4KKwkkbm9kZS0+Y29ubmVj
dF9mYWlscygKKwkJIiRjb25uc3RyIGhvc3Q9ZXhhbXBsZS5vcmcgc3NsY2VydG1vZGU9cmVxdWly
ZSBzc2xjZXJ0PXNzbC9jbGllbnQuY3J0ICIgLiBzc2xrZXkoJ2NsaWVudC5rZXknKSwKKwkJImV4
YW1wbGUub3JnLCAkZGVmYXVsdF9jYTogY29ubmVjdCB3aXRoIHNzbGNlcnQsIG5vIGNsaWVudCBD
QSBjb25maWd1cmVkIiwKKwkJZXhwZWN0ZWRfc3RkZXJyID0+IHFyL2NsaWVudCBjZXJ0aWZpY2F0
ZXMgY2FuIG9ubHkgYmUgY2hlY2tlZCBpZiBhIHJvb3QgY2VydGlmaWNhdGUgc3RvcmUgaXMgYXZh
aWxhYmxlLyk7CisKKwkjIGV4YW1wbGUuY29tIGlzIGNvbmZpZ3VyZWQgYW5kIHNob3VsZCByZXF1
aXJlIGEgdmFsaWQgY2xpZW50IGNlcnQuCisJJG5vZGUtPmNvbm5lY3RfZmFpbHMoCisJCSIkY29u
bnN0ciBob3N0PWV4YW1wbGUuY29tIHNzbGNlcnRtb2RlPWRpc2FibGUiLAorCQkiZXhhbXBsZS5j
b20sICRkZWZhdWx0X2NhOiBjb25uZWN0IGZhaWxzIGlmIG5vIGNsaWVudCBjZXJ0aWZpY2F0ZSBz
ZW50IiwKKwkJZXhwZWN0ZWRfc3RkZXJyID0+IHFyL2Nvbm5lY3Rpb24gcmVxdWlyZXMgYSB2YWxp
ZCBjbGllbnQgY2VydGlmaWNhdGUvKTsKKworCSRub2RlLT5jb25uZWN0X29rKAorCQkiJGNvbm5z
dHIgaG9zdD1leGFtcGxlLmNvbSBzc2xjZXJ0bW9kZT1yZXF1aXJlIHNzbGNlcnQ9c3NsL2NsaWVu
dC5jcnQgIiAuIHNzbGtleSgnY2xpZW50LmtleScpLAorCQkiZXhhbXBsZS5jb20sICRkZWZhdWx0
X2NhOiBjb25uZWN0IHdpdGggc3NsY2VydCwgY2xpZW50IGNlcnRpZmljYXRlIHNlbnQiKTsKKwor
CSMgZXhhbXBsZS5uZXQgaXMgY29uZmlndXJlZCBhbmQgc2hvdWxkIHJlcXVpcmUgYSBjbGllbnQg
Y2VydCwgYnV0IHdpbGwKKwkjIGFsd2F5cyBmYWlsIHZlcmlmaWNhdGlvbi4KKwkkbm9kZS0+Y29u
bmVjdF9mYWlscygKKwkJIiRjb25uc3RyIGhvc3Q9ZXhhbXBsZS5uZXQgc3NsY2VydG1vZGU9ZGlz
YWJsZSIsCisJCSJleGFtcGxlLm5ldCwgJGRlZmF1bHRfY2E6IGNvbm5lY3QgZmFpbHMgaWYgbm8g
Y2xpZW50IGNlcnRpZmljYXRlIHNlbnQiLAorCQlleHBlY3RlZF9zdGRlcnIgPT4gcXIvY29ubmVj
dGlvbiByZXF1aXJlcyBhIHZhbGlkIGNsaWVudCBjZXJ0aWZpY2F0ZS8pOworCisJJG5vZGUtPmNv
bm5lY3RfZmFpbHMoCisJCSIkY29ubnN0ciBob3N0PWV4YW1wbGUubmV0IHNzbGNlcnRtb2RlPXJl
cXVpcmUgc3NsY2VydD1zc2wvY2xpZW50LmNydCAiIC4gc3Nsa2V5KCdjbGllbnQua2V5JyksCisJ
CSJleGFtcGxlLm5ldCwgJGRlZmF1bHRfY2E6IGNvbm5lY3Qgd2l0aCBzc2xjZXJ0LCBjbGllbnQg
Y2VydGlmaWNhdGUgc2VudCIsCisJCWV4cGVjdGVkX3N0ZGVyciA9PiBxci91bmtub3duIGNhLyk7
Cit9CisKIGRvbmVfdGVzdGluZygpOwpkaWZmIC0tZ2l0IGEvc3JjL3Rlc3Qvc3NsL3QvU1NML0Jh
Y2tlbmQvT3BlblNTTC5wbSBiL3NyYy90ZXN0L3NzbC90L1NTTC9CYWNrZW5kL09wZW5TU0wucG0K
aW5kZXggZTA0NDMxODUzMWYuLmJkY2NlODQwMDNlIDEwMDY0NAotLS0gYS9zcmMvdGVzdC9zc2wv
dC9TU0wvQmFja2VuZC9PcGVuU1NMLnBtCisrKyBiL3NyYy90ZXN0L3NzbC90L1NTTC9CYWNrZW5k
L09wZW5TU0wucG0KQEAgLTcxLDYgKzcxLDcgQEAgc3ViIGluaXQKIAljaG1vZCgwNjAwLCBnbG9i
ICIkcGdkYXRhL3NlcnZlci0qLmtleSIpCiAJICBvciBkaWUgImZhaWxlZCB0byBjaGFuZ2UgcGVy
bWlzc2lvbnMgb24gc2VydmVyIGtleXM6ICQhIjsKIAlfY29weV9maWxlcygic3NsL3Jvb3QrY2xp
ZW50X2NhLmNydCIsICRwZ2RhdGEpOworCV9jb3B5X2ZpbGVzKCJzc2wvcm9vdCtzZXJ2ZXJfY2Eu
Y3J0IiwgJHBnZGF0YSk7CiAJX2NvcHlfZmlsZXMoInNzbC9yb290X2NhLmNydCIsICRwZ2RhdGEp
OwogCV9jb3B5X2ZpbGVzKCJzc2wvcm9vdCtjbGllbnQuY3JsIiwgJHBnZGF0YSk7CiAJbWtkaXIo
IiRwZ2RhdGEvcm9vdCtjbGllbnQtY3JsZGlyIikKQEAgLTE0NSw3ICsxNDYsOCBAQCBmb2xsb3dp
bmcgcGFyYW1ldGVycyBhcmUgc3VwcG9ydGVkOgogPWl0ZW0gY2FmaWxlID0+IEI8dmFsdWU+CiAK
IFRoZSBDQSBjZXJ0aWZpY2F0ZSBmaWxlIHRvIHVzZSBmb3IgdGhlIEM8c3NsX2NhX2ZpbGU+IEdV
Qy4gSWYgb21pdHRlZCBpdCB3aWxsCi1kZWZhdWx0IHRvICdyb290K2NsaWVudF9jYS5jcnQnLgor
ZGVmYXVsdCB0byAncm9vdCtjbGllbnRfY2EuY3J0Jy4gSWYgZW1wdHksIG5vIEM8c3NsX2NhX2Zp
bGU+IGNvbmZpZ3VyYXRpb24KK3BhcmFtZXRlciB3aWxsIGJlIHNldC4KIAogPWl0ZW0gY2VydGZp
bGUgPT4gQjx2YWx1ZT4KIApAQCAtMTgwLDEwICsxODIsMTEgQEAgc3ViIHNldF9zZXJ2ZXJfY2Vy
dAogCSAgdW5sZXNzIGRlZmluZWQgJHBhcmFtcy0+e2tleWZpbGV9OwogCiAJbXkgJHNzbGNvbmYg
PQotCQkic3NsX2NhX2ZpbGU9JyRwYXJhbXMtPntjYWZpbGV9LmNydCdcbiIKLQkgIC4gInNzbF9j
ZXJ0X2ZpbGU9JyRwYXJhbXMtPntjZXJ0ZmlsZX0uY3J0J1xuIgorCQkic3NsX2NlcnRfZmlsZT0n
JHBhcmFtcy0+e2NlcnRmaWxlfS5jcnQnXG4iCiAJICAuICJzc2xfa2V5X2ZpbGU9JyRwYXJhbXMt
PntrZXlmaWxlfS5rZXknXG4iCiAJICAuICJzc2xfY3JsX2ZpbGU9JyRwYXJhbXMtPntjcmxmaWxl
fSdcbiI7CisJJHNzbGNvbmYgLj0gInNzbF9jYV9maWxlPSckcGFyYW1zLT57Y2FmaWxlfS5jcnQn
XG4iCisJICBpZiAkcGFyYW1zLT57Y2FmaWxlfSBuZSAiIjsKIAkkc3NsY29uZiAuPSAic3NsX2Ny
bF9kaXI9JyRwYXJhbXMtPntjcmxkaXJ9J1xuIgogCSAgaWYgZGVmaW5lZCAkcGFyYW1zLT57Y3Js
ZGlyfTsKIAo=
--00000000000063b074062f8b5bb5--