On Thu, Feb 27, 2025 at 5:38 AM Daniel Gustafsson <dan...@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 context
> 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 AM Jacob Champion
<jacob.champ...@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 = 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 == 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 = '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-ULqS8itNP7uc9nUmNLOvbXf2TCgBA%40mail.gmail.com
commit a4d9cbf4d1228dcc17f2961b7811321a50e74617
Author: Jacob Champion <jacob.champ...@enterprisedb.com>
Date: Tue Mar 4 13:17:12 2025 -0800
Tests
diff --git a/src/test/ssl/t/004_sni.pl b/src/test/ssl/t/004_sni.pl
index f0ce048273a..72e64c6c00d 100644
--- a/src/test/ssl/t/004_sni.pl
+++ b/src/test/ssl/t/004_sni.pl
@@ -33,6 +33,11 @@ if (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bssl\b/)
my $ssl_server = SSL::Server->new();
+sub sslkey
+{
+ return $ssl_server->sslkey(@_);
+}
+
my $node = PostgreSQL::Test::Cluster->new('primary');
$node->init;
@@ -161,4 +166,57 @@ $node->connect_fails(
"connect fails since the passphrase protected key cannot be reloaded",
expected_stderr => qr/tlsv1 unrecognized name/);
+# Test client CAs.
+
+$connstr =
+ "user=ssltestuser dbname=certdb hostaddr=$SERVERHOSTADDR sslmode=require
sslsni=1";
+
+ok(unlink($node->data_dir . '/pg_hosts.conf'));
+# example.org has an unconfigured CA.
+$node->append_conf('pg_hosts.conf', 'example.org server-cn-only.crt
server-cn-only.key ""');
+# example.com uses the client CA.
+$node->append_conf('pg_hosts.conf', 'example.com server-cn-only.crt
server-cn-only.key root+client_ca.crt');
+# example.net uses the server CA (which is wrong).
+$node->append_conf('pg_hosts.conf', 'example.net server-cn-only.crt
server-cn-only.key root+server_ca.crt');
+$node->reload;
+
+my @cases = ( "", "root+client_ca", "root+server_ca" );
+foreach my $default_ca (@cases)
+{
+ # The default CA should, ideally, not matter for the purposes of these
+ # tests, since we connect to the other hosts explicitly.
+ $ssl_server->switch_server_cert(
+ $node,
+ certfile => 'server-cn-only',
+ cafile => $default_ca);
+
+ # example.org is unconfigured and should fail.
+ $node->connect_fails(
+ "$connstr host=example.org sslcertmode=require
sslcert=ssl/client.crt " . sslkey('client.key'),
+ "example.org, $default_ca: connect with sslcert, no client CA
configured",
+ expected_stderr => qr/client certificates can only be checked
if a root certificate store is available/);
+
+ # example.com is configured and should require a valid client cert.
+ $node->connect_fails(
+ "$connstr host=example.com sslcertmode=disable",
+ "example.com, $default_ca: connect fails if no client
certificate sent",
+ expected_stderr => qr/connection requires a valid client
certificate/);
+
+ $node->connect_ok(
+ "$connstr host=example.com sslcertmode=require
sslcert=ssl/client.crt " . sslkey('client.key'),
+ "example.com, $default_ca: connect with sslcert, client
certificate sent");
+
+ # example.net is configured and should require a client cert, but will
+ # always fail verification.
+ $node->connect_fails(
+ "$connstr host=example.net sslcertmode=disable",
+ "example.net, $default_ca: connect fails if no client
certificate sent",
+ expected_stderr => qr/connection requires a valid client
certificate/);
+
+ $node->connect_fails(
+ "$connstr host=example.net sslcertmode=require
sslcert=ssl/client.crt " . sslkey('client.key'),
+ "example.net, $default_ca: connect with sslcert, client
certificate sent",
+ expected_stderr => qr/unknown ca/);
+}
+
done_testing();
diff --git a/src/test/ssl/t/SSL/Backend/OpenSSL.pm
b/src/test/ssl/t/SSL/Backend/OpenSSL.pm
index e044318531f..bdcce84003e 100644
--- a/src/test/ssl/t/SSL/Backend/OpenSSL.pm
+++ b/src/test/ssl/t/SSL/Backend/OpenSSL.pm
@@ -71,6 +71,7 @@ sub init
chmod(0600, glob "$pgdata/server-*.key")
or die "failed to change permissions on server keys: $!";
_copy_files("ssl/root+client_ca.crt", $pgdata);
+ _copy_files("ssl/root+server_ca.crt", $pgdata);
_copy_files("ssl/root_ca.crt", $pgdata);
_copy_files("ssl/root+client.crl", $pgdata);
mkdir("$pgdata/root+client-crldir")
@@ -145,7 +146,8 @@ following parameters are supported:
=item cafile => B<value>
The CA certificate file to use for the C<ssl_ca_file> GUC. If omitted it will
-default to 'root+client_ca.crt'.
+default to 'root+client_ca.crt'. If empty, no C<ssl_ca_file> configuration
+parameter will be set.
=item certfile => B<value>
@@ -180,10 +182,11 @@ sub set_server_cert
unless defined $params->{keyfile};
my $sslconf =
- "ssl_ca_file='$params->{cafile}.crt'\n"
- . "ssl_cert_file='$params->{certfile}.crt'\n"
+ "ssl_cert_file='$params->{certfile}.crt'\n"
. "ssl_key_file='$params->{keyfile}.key'\n"
. "ssl_crl_file='$params->{crlfile}'\n";
+ $sslconf .= "ssl_ca_file='$params->{cafile}.crt'\n"
+ if $params->{cafile} ne "";
$sslconf .= "ssl_crl_dir='$params->{crldir}'\n"
if defined $params->{crldir};
