Обсуждение: SSL Tests for sslinfo extension
While playing around with the recent SSL testharness changes I wrote a test suite for sslinfo as a side effect, which seemed valuable in its own right as we currently have no coverage of this code. The small change needed to the testharness is to support installing modules, which is broken out into 0001 for easier reading. I'll park this in the next commitfest for now. -- Daniel Gustafsson https://vmware.com/
Вложения
Daniel Gustafsson <daniel@yesql.se> writes: > In order to be able to test extensions with SSL connections, allow > configure_test_server_for_ssl to create any extensions passed as > comma separated list. Each extension is created in all the test > databases which may or may not be useful. Why the comma-separated string, rather than an array reference, i.e. `extensions => [qw(foo bar baz)]`? Also, should it use `CREATE EXTENSION .. CASCADE`, in case the specified extensions depend on others? - ilmari -- "The surreality of the universe tends towards a maximum" -- Skud's Law "Never formulate a law or axiom that you're not prepared to live with the consequences of." -- Skud's Meta-Law
On 5/19/21 1:01 PM, Dagfinn Ilmari Mannsåker wrote: > Daniel Gustafsson <daniel@yesql.se> writes: > >> In order to be able to test extensions with SSL connections, allow >> configure_test_server_for_ssl to create any extensions passed as >> comma separated list. Each extension is created in all the test >> databases which may or may not be useful. > Why the comma-separated string, rather than an array reference, > i.e. `extensions => [qw(foo bar baz)]`? Also, should it use `CREATE > EXTENSION .. CASCADE`, in case the specified extensions depend on > others? > Also, instead of one line per db there should be an inner loop over the db names. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
> On 19 May 2021, at 21:05, Andrew Dunstan <andrew@dunslane.net> wrote: > > On 5/19/21 1:01 PM, Dagfinn Ilmari Mannsåker wrote: >> Daniel Gustafsson <daniel@yesql.se> writes: >> >>> In order to be able to test extensions with SSL connections, allow >>> configure_test_server_for_ssl to create any extensions passed as >>> comma separated list. Each extension is created in all the test >>> databases which may or may not be useful. >> Why the comma-separated string, rather than an array reference, >> i.e. `extensions => [qw(foo bar baz)]`? No real reason, I just haven't written Perl enough lately to "think in Perl". Fixed in the attached. >> Also, should it use `CREATE >> EXTENSION .. CASCADE`, in case the specified extensions depend on >> others? Good point. Each extension will have to be in EXTRA_INSTALL as well of course, but we should to CASCADE. > Also, instead of one line per db there should be an inner loop over the > db names. Right, I was lazily using the same approach as for CREATE DATABASE but when the list is used it two places it should be a proper list. Fixed in the attached. -- Daniel Gustafsson https://vmware.com/
Вложения
On Thu, May 20, 2021 at 08:40:48PM +0200, Daniel Gustafsson wrote: > > On 19 May 2021, at 21:05, Andrew Dunstan <andrew@dunslane.net> wrote: > > > > On 5/19/21 1:01 PM, Dagfinn Ilmari Mannsåker wrote: > >> Daniel Gustafsson <daniel@yesql.se> writes: > >> > >>> In order to be able to test extensions with SSL connections, allow > >>> configure_test_server_for_ssl to create any extensions passed as > >>> comma separated list. Each extension is created in all the test > >>> databases which may or may not be useful. > >> Why the comma-separated string, rather than an array reference, > >> i.e. `extensions => [qw(foo bar baz)]`? > > No real reason, I just haven't written Perl enough lately to "think in Perl". > Fixed in the attached. Hmm. Adding internal dependencies between the tests should be avoided if we can. What would it take to move those TAP tests to contrib/sslinfo instead? This is while keeping in mind that there was a patch aimed at refactoring the SSL test suite for NSS. -- Michael
Вложения
> On 17 Jun 2021, at 09:29, Michael Paquier <michael@paquier.xyz> wrote: > > On Thu, May 20, 2021 at 08:40:48PM +0200, Daniel Gustafsson wrote: >>> On 19 May 2021, at 21:05, Andrew Dunstan <andrew@dunslane.net> wrote: >>> >>> On 5/19/21 1:01 PM, Dagfinn Ilmari Mannsåker wrote: >>>> Daniel Gustafsson <daniel@yesql.se> writes: >>>> >>>>> In order to be able to test extensions with SSL connections, allow >>>>> configure_test_server_for_ssl to create any extensions passed as >>>>> comma separated list. Each extension is created in all the test >>>>> databases which may or may not be useful. >>>> Why the comma-separated string, rather than an array reference, >>>> i.e. `extensions => [qw(foo bar baz)]`? >> >> No real reason, I just haven't written Perl enough lately to "think in Perl". >> Fixed in the attached. > > Hmm. Adding internal dependencies between the tests should be avoided > if we can. What would it take to move those TAP tests to > contrib/sslinfo instead? This is while keeping in mind that there was > a patch aimed at refactoring the SSL test suite for NSS. It would be quite invasive as we currently don't provide the SSLServer test harness outside of src/test/ssl, and given how tailored it is today I'm not sure doing so without a rewrite would be a good idea. A longer term solution would probably be to teach PostgresNode to provide an instance set up for TLS in case the backend is compiled with TLS support, and use that for things like sslinfo. -- Daniel Gustafsson https://vmware.com/
Daniel Gustafsson <daniel@yesql.se> writes:
> On 17 Jun 2021, at 09:29, Michael Paquier <michael@paquier.xyz> wrote:
>> Hmm. Adding internal dependencies between the tests should be avoided
>> if we can. What would it take to move those TAP tests to
>> contrib/sslinfo instead? This is while keeping in mind that there was
>> a patch aimed at refactoring the SSL test suite for NSS.
> It would be quite invasive as we currently don't provide the SSLServer test
> harness outside of src/test/ssl, and given how tailored it is today I'm not
> sure doing so without a rewrite would be a good idea.
I think testing sslinfo in src/test/ssl is fine, while putting its test
inside contrib/ would be dangerous, because then the test would be run
by default. src/test/ssl is not run by default because the server it
starts is potentially accessible by other local users, and AFAICS the
same has to be true for an sslinfo test.
So I don't have any problem with this structurally, but I do have a
few nitpicks:
* I think the error message added in 0001 should complain about
missing password "encryption" not "encoding", no?
* 0002 hasn't been updated for the great PostgresNode renaming.
* 0002 needs to extend src/test/ssl/README to mention that
"make installcheck" requires having installed contrib/sslinfo,
analogous to similar comments in (eg) src/test/recovery/README.
* 0002 writes a temporary file in the source tree. This is bad;
for one thing I bet it fails under VPATH, but in any case there
is no reason to risk it. Put it in the tmp_check directory instead
(cf temp kdc files in src/test/kerberos/t/001_auth.pl). That's
safer and you needn't worry about cleaning it up.
* Hmm ... now I notice that you borrowed the key-file-copying logic
from the 001 and 002 tests, but it's just as bad practice there.
We should fix them too.
* I ran a code-coverage check and it shows that this doesn't test
ssl_issuer_field() or any of the following functions in sslinfo.c.
I think at least ssl_extension_info() is complicated enough to
deserve a test.
regards, tom lane
On Sat, Nov 27, 2021 at 02:27:19PM -0500, Tom Lane wrote: > I think testing sslinfo in src/test/ssl is fine, while putting its test > inside contrib/ would be dangerous, because then the test would be run > by default. src/test/ssl is not run by default because the server it > starts is potentially accessible by other local users, and AFAICS the > same has to be true for an sslinfo test. Ah, indeed, good point. I completely forgot that we'd better control this stuff with PG_TEST_EXTRA. -- Michael
Вложения
> On 27 Nov 2021, at 20:27, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I don't have any problem with this structurally, but I do have a > few nitpicks: Thanks for reviewing! > * I think the error message added in 0001 should complain about > missing password "encryption" not "encoding", no? Doh, of course. > * 0002 hasn't been updated for the great PostgresNode renaming. Fixed. > * 0002 needs to extend src/test/ssl/README to mention that > "make installcheck" requires having installed contrib/sslinfo, > analogous to similar comments in (eg) src/test/recovery/README. Good point, I copied over the wording from recovery/README and adapted for SSL since I think it was well written as is. (Consistency is also a good benefit.) > * 0002 writes a temporary file in the source tree. This is bad; > for one thing I bet it fails under VPATH, but in any case there > is no reason to risk it. Put it in the tmp_check directory instead > (cf temp kdc files in src/test/kerberos/t/001_auth.pl). That's > safer and you needn't worry about cleaning it up. Fixed, and see below. > * Hmm ... now I notice that you borrowed the key-file-copying logic > from the 001 and 002 tests, but it's just as bad practice there. > We should fix them too. Well spotted, I hadn't thought about that but in hindsight it's quite obviously bad. I've done this in a 0003 patch in this series which also comes with the IMO benefit of a tighter coupling between the key filename used in the test with what's in the repo by removing the _tmp suffix. To avoid concatenating with the long tmp_check path variable everywhere, I went with a lookup HASH to make it easier on the eye and harder to mess up should we change tmp path at some point. There might be ways which are more like modern Perl, but I wasn't able to think of one off the bat. > * I ran a code-coverage check and it shows that this doesn't test > ssl_issuer_field() or any of the following functions in sslinfo.c. > I think at least ssl_extension_info() is complicated enough to > deserve a test. Agreed. The attached v3 covers the issuer and extension function to at least some degree. In order to reliably test the extension I added a new cert with a CA extension. -- Daniel Gustafsson https://vmware.com/
Вложения
Daniel Gustafsson <daniel@yesql.se> writes:
> Agreed. The attached v3 covers the issuer and extension function to at least
> some degree. In order to reliably test the extension I added a new cert with a
> CA extension.
I have two remaining trivial nitpicks, for which I attach an 0004
delta patch: the README change was fat-fingered slightly, and some
of the commentary about the key file seems now obsolete.
Otherwise I think it's good to go, so I marked it RFC.
regards, tom lane
diff --git a/src/test/ssl/README b/src/test/ssl/README
index ca30f9329a..7e60700652 100644
--- a/src/test/ssl/README
+++ b/src/test/ssl/README
@@ -12,14 +12,12 @@ TCP connections on localhost. Any user on the same host is able to
log in to the test server while the tests are running. Do not run this
suite on a multi-user system where you don't trust all local users!
-NOTE: You must have given the --enable-tap-tests argument to configure.
-Also, to use "make installcheck", you must have built and installed
-contrib/sslinfo in addition to the core code.
-
Running the tests
=================
NOTE: You must have given the --enable-tap-tests argument to configure.
+Also, to use "make installcheck", you must have built and installed
+contrib/sslinfo in addition to the core code.
Run
make check
diff --git a/src/test/ssl/t/003_sslinfo.pl b/src/test/ssl/t/003_sslinfo.pl
index 61b117e6c2..cf2e8dde0f 100644
--- a/src/test/ssl/t/003_sslinfo.pl
+++ b/src/test/ssl/t/003_sslinfo.pl
@@ -37,9 +37,6 @@ my $common_connstr;
# The client's private key must not be world-readable, so take a copy
# of the key stored in the code tree and update its permissions.
-#
-# This changes ssl/client.key to ssl/client_tmp.key etc for the rest
-# of the tests.
my $client_tmp_key = "${PostgreSQL::Test::Utils::tmp_check}/client_ext.key";
copy("ssl/client_ext.key", $client_tmp_key)
or die "couldn't copy ssl/client_ext.key to $client_tmp_key for permissions change: $!";
> On 29 Nov 2021, at 23:50, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Daniel Gustafsson <daniel@yesql.se> writes: >> Agreed. The attached v3 covers the issuer and extension function to at least >> some degree. In order to reliably test the extension I added a new cert with a >> CA extension. > > I have two remaining trivial nitpicks, for which I attach an 0004 > delta patch: the README change was fat-fingered slightly, and some > of the commentary about the key file seems now obsolete. Ah yes, thanks. > Otherwise I think it's good to go, so I marked it RFC. Great! I'll take another look over it tomorrow and will go ahead with it then. -- Daniel Gustafsson https://vmware.com/
> On 30 Nov 2021, at 00:16, Daniel Gustafsson <daniel@yesql.se> wrote: >> On 29 Nov 2021, at 23:50, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Otherwise I think it's good to go, so I marked it RFC. > > Great! I'll take another look over it tomorrow and will go ahead with it then. I applied your nitpick diff and took it for another spin in CI, and pushed it. Thanks for review! -- Daniel Gustafsson https://vmware.com/