Thread: Out-of-tree certificate interferes ssltest
Hello.
003_sslinfo.pl fails for me.
ok 6 - ssl_client_cert_present() for connection with cert
connection error: 'psql: error: connection to server at "127.0.0.1", port 61688 failed: could not read certificate file
"/home/horiguti/.postgresql/postgresql.crt":no start line'
while running 'psql -XAtq -d sslrootcert=ssl/root+server_ca.crt sslmode=require dbname=trustdb hostaddr=127.0.0.1
user=ssltestuserhost=localhost -f - -v ON_ERR
I think we don't want this behavior.
The attached fixes that and make-world successfully finished even if I
have a cert file in my home direcotory.
regareds.
--
Kyotaro Horiguchi
NTT Open Source Software Center
From 308b55f06907ccaf4ac5669daacf04fea8a18fe1 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Wed, 16 Mar 2022 16:20:46 +0900
Subject: [PATCH v1] Prevent out-of-tree certificates from interfering ssl
tests
003_sslinfo.pl fails when there is a certificate file in ~/.postgresql
directory. Prevent that failure by explicitly setting sslcert option
in connection string.
---
src/test/ssl/t/003_sslinfo.pl | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/test/ssl/t/003_sslinfo.pl b/src/test/ssl/t/003_sslinfo.pl
index 95742081f3..81da94f18d 100644
--- a/src/test/ssl/t/003_sslinfo.pl
+++ b/src/test/ssl/t/003_sslinfo.pl
@@ -93,7 +93,7 @@ $result = $node->safe_psql("certdb", "SELECT ssl_client_cert_present();",
is($result, 't', "ssl_client_cert_present() for connection with cert");
$result = $node->safe_psql("trustdb", "SELECT ssl_client_cert_present();",
- connstr => "sslrootcert=ssl/root+server_ca.crt sslmode=require " .
+ connstr => "sslcert=invalid sslrootcert=ssl/root+server_ca.crt sslmode=require " .
"dbname=trustdb hostaddr=$SERVERHOSTADDR user=ssltestuser host=localhost");
is($result, 'f', "ssl_client_cert_present() for connection without cert");
@@ -108,7 +108,7 @@ $result = $node->psql("certdb", "SELECT ssl_client_dn_field('invalid');",
is($result, '3', "ssl_client_dn_field() for an invalid field");
$result = $node->safe_psql("trustdb", "SELECT ssl_client_dn_field('commonName');",
- connstr => "sslrootcert=ssl/root+server_ca.crt sslmode=require " .
+ connstr => "sslcert=invalid sslrootcert=ssl/root+server_ca.crt sslmode=require " .
"dbname=trustdb hostaddr=$SERVERHOSTADDR user=ssltestuser host=localhost");
is($result, '', "ssl_client_dn_field() for connection without cert");
--
2.27.0
On Wed, Mar 16, 2022 at 04:36:58PM +0900, Kyotaro Horiguchi wrote: > ok 6 - ssl_client_cert_present() for connection with cert > connection error: 'psql: error: connection to server at "127.0.0.1", port 61688 failed: could not read certificate file"/home/horiguti/.postgresql/postgresql.crt": no start line' > while running 'psql -XAtq -d sslrootcert=ssl/root+server_ca.crt sslmode=require dbname=trustdb hostaddr=127.0.0.1 user=ssltestuserhost=localhost -f - -v ON_ERR > > I think we don't want this behavior. > > The attached fixes that and make-world successfully finished even if I > have a cert file in my home direcotory. That's the same issue as the one fixed in dd87799, using the same method. I'll double-check on top of looking at what you are suggesting here. -- Michael
Attachment
> On 16 Mar 2022, at 08:36, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > I think we don't want this behavior. Agreed. > The attached fixes that and make-world successfully finished even if I > have a cert file in my home direcotory. Seems correct to me, thanks! -- Daniel Gustafsson https://vmware.com/
On Wed, Mar 16, 2022 at 11:45:39AM +0100, Daniel Gustafsson wrote: > On 16 Mar 2022, at 08:36, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: >> The attached fixes that and make-world successfully finished even if I >> have a cert file in my home direcotory. > > Seems correct to me, thanks! The ultimate test I can think about to stress the robustness of this test suite is to generate various certs and keys using "make sslfiles", save them into a ~/.postgresql/ (postgresql.crt, postgresql.key, root.crl and root.crt), and then run the tests to see how much junk data the SSL scripts would feed on. With this method, I have caught a total of 71 failures, much more than reported upthread. We should really put more attention to set invalid default values for sslcert, sslkey, sslcrl, sslcrldir and sslrootcert, rather than hardcoding a couple of them in only a few places, opening ourselves to the same problem, again, each time a new test is added. The best way I can think about here is to use a string that includes all the default SSL settings, appending that at the beginning of each $common_connstr. This takes care of most the failures, except two cases related to expected failures for sslcrldir: - directory CRL belonging to a different CA - does not connect with client-side CRL directory In both cases, enforcing sslcrl to a value of "invalid" interferes with the failure scenario we expect from sslcrldir. It is possible to bypass that with something like the attached, but that's a kind of ugly hack. Another alternative would be to drop those two tests, and I am not sure how much we care about these two negative scenarios. Thoughts? -- Michael
Attachment
On Thu, Mar 17, 2022 at 02:59:26PM +0900, Michael Paquier wrote: > In both cases, enforcing sslcrl to a value of "invalid" interferes > with the failure scenario we expect from sslcrldir. It is possible to > bypass that with something like the attached, but that's a kind of > ugly hack. Another alternative would be to drop those two tests, and > I am not sure how much we care about these two negative scenarios. Actually, there is a trick I have recalled here: we can enforce sslcrl to an empty value in the connection string after the default. This still ensures that the test won't pick up any SSL data from the local environment and avoids any interferences of OpenSSL's X509_STORE_load_locations(). This gives a much simpler and cleaner patch. Thoughts? -- Michael
Attachment
At Thu, 17 Mar 2022 16:22:14 +0900, Michael Paquier <michael@paquier.xyz> wrote in > On Thu, Mar 17, 2022 at 02:59:26PM +0900, Michael Paquier wrote: > > In both cases, enforcing sslcrl to a value of "invalid" interferes > > with the failure scenario we expect from sslcrldir. It is possible to > > bypass that with something like the attached, but that's a kind of > > ugly hack. Another alternative would be to drop those two tests, and > > I am not sure how much we care about these two negative scenarios. > > Actually, there is a trick I have recalled here: we can enforce sslcrl > to an empty value in the connection string after the default. This > still ensures that the test won't pick up any SSL data from the local > environment and avoids any interferences of OpenSSL's > X509_STORE_load_locations(). This gives a much simpler and cleaner > patch. > > Thoughts? Ah! I didn't have a thought that we can specify the same parameter twice. It looks like clean and robust. $default_ssl_connstr contains all required options in PQconninfoOptions[]. The same method worked for 003_sslinfo.pl, too. (of course). regards. -- Kyotaro Horiguchi NTT Open Source Software Center
> On 17 Mar 2022, at 09:05, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > At Thu, 17 Mar 2022 16:22:14 +0900, Michael Paquier <michael@paquier.xyz> wrote in >> On Thu, Mar 17, 2022 at 02:59:26PM +0900, Michael Paquier wrote: >>> In both cases, enforcing sslcrl to a value of "invalid" interferes >>> with the failure scenario we expect from sslcrldir. It is possible to >>> bypass that with something like the attached, but that's a kind of >>> ugly hack. Another alternative would be to drop those two tests, and >>> I am not sure how much we care about these two negative scenarios. I really don't think we should drop tests based on these premises, at least not until it's raised as a problem/inconvenience but more hackers. I would prefer to instead extend the error message with hints that ~/.postgresql contents could've affected test outcome. But, as the v2 patch handles it this is mostly academic at this point. >> Actually, there is a trick I have recalled here: we can enforce sslcrl >> to an empty value in the connection string after the default. This >> still ensures that the test won't pick up any SSL data from the local >> environment and avoids any interferences of OpenSSL's >> X509_STORE_load_locations(). This gives a much simpler and cleaner >> patch. > Ah! I didn't have a thought that we can specify the same parameter > twice. It looks like clean and robust. $default_ssl_connstr contains > all required options in PQconninfoOptions[]. +1 One small concern though. This hunk: +my $default_ssl_connstr = "sslkey=invalid sslcert=invalid sslrootcert=invalid sslcrl=invalid sslcrldir=invalid"; + $common_connstr = - "user=ssltestuser dbname=trustdb sslcert=invalid hostaddr=$SERVERHOSTADDR host=common-name.pg-ssltest.test"; + "$default_ssl_connstr user=ssltestuser dbname=trustdb hostaddr=$SERVERHOSTADDR host=common-name.pg-ssltest.test"; ..together with the following changes along the lines of: - "$common_connstr sslrootcert=invalid sslmode=require", + "$common_connstr sslmode=require", ..is making it fairly hard to read the test and visualize what the connection string is and how the test should behave. I don't have a better idea off the top of my head right now, but I think this is an area to revisit and improve on. -- Daniel Gustafsson https://vmware.com/
On Thu, Mar 17, 2022 at 02:28:49PM +0100, Daniel Gustafsson wrote: > One small concern though. This hunk: > > +my $default_ssl_connstr = "sslkey=invalid sslcert=invalid sslrootcert=invalid sslcrl=invalid sslcrldir=invalid"; > + > $common_connstr = > - "user=ssltestuser dbname=trustdb sslcert=invalid hostaddr=$SERVERHOSTADDR host=common-name.pg-ssltest.test"; > + "$default_ssl_connstr user=ssltestuser dbname=trustdb hostaddr=$SERVERHOSTADDR host=common-name.pg-ssltest.test"; > > ..together with the following changes along the lines of: > > - "$common_connstr sslrootcert=invalid sslmode=require", > + "$common_connstr sslmode=require", > > ..is making it fairly hard to read the test and visualize what the connection > string is and how the test should behave. I don't have a better idea off the > top of my head right now, but I think this is an area to revisit and improve > on. I agree that this makes this set of three tests harder to follow, as we expect a root cert to *not* be set locally. Keeping the behavior documented in each individual string would be better, even if that duplicates more the keys in those final strings. Another thing that Horiguchi-san has pointed out upthread (?) is 003, where it is also possible to trigger failures once the environment is hijacked. The attached allows the full test suite to pass without issues on my side. -- Michael
Attachment
On 3/17/22 21:02, Michael Paquier wrote: > On Thu, Mar 17, 2022 at 02:28:49PM +0100, Daniel Gustafsson wrote: >> One small concern though. This hunk: >> >> +my $default_ssl_connstr = "sslkey=invalid sslcert=invalid sslrootcert=invalid sslcrl=invalid sslcrldir=invalid"; >> + >> $common_connstr = >> - "user=ssltestuser dbname=trustdb sslcert=invalid hostaddr=$SERVERHOSTADDR host=common-name.pg-ssltest.test"; >> + "$default_ssl_connstr user=ssltestuser dbname=trustdb hostaddr=$SERVERHOSTADDR host=common-name.pg-ssltest.test"; >> >> ..together with the following changes along the lines of: >> >> - "$common_connstr sslrootcert=invalid sslmode=require", >> + "$common_connstr sslmode=require", >> >> ..is making it fairly hard to read the test and visualize what the connection >> string is and how the test should behave. I don't have a better idea off the >> top of my head right now, but I think this is an area to revisit and improve >> on. > I agree that this makes this set of three tests harder to follow, as > we expect a root cert to *not* be set locally. Keeping the behavior > documented in each individual string would be better, even if that > duplicates more the keys in those final strings. > > Another thing that Horiguchi-san has pointed out upthread (?) is 003, > where it is also possible to trigger failures once the environment is > hijacked. The attached allows the full test suite to pass without > issues on my side. LGTM cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
On Fri, Mar 18, 2022 at 06:15:28PM -0400, Andrew Dunstan wrote: > On 3/17/22 21:02, Michael Paquier wrote: >> Another thing that Horiguchi-san has pointed out upthread (?) is 003, >> where it is also possible to trigger failures once the environment is >> hijacked. The attached allows the full test suite to pass without >> issues on my side. > > LGTM Thanks for looking at it. I have been able to check this stuff across all the supported branches, and failures happen down to 10. That's easy enough to see once you know how to break the tests. There were a couple of conflicts, but nothing impossible to fix, so applied down to v10. REL_11_STABLE had one extra failure in 002_scram.pl that was already fixed in v12~. -- Michael